By the way, I have one particular nit about proplib, and it came about
while setting up properties for bus speeds (UART related for AR5312/5315
ports).  Basically, I find prop_number to be somewhat awkward to use. 
First off, in this particular case, I needed to pass a frequency that
was 32-bits.  I.e. I was taking the property and storing it into a
32-bit variable.  (The variable has to be 32-bit without breaking com(4)
all over again.)

Secondly, I would like a short-hand for
prop_dictionary_get/prop_number_integer (and the intervening
KASSERT(prop_type(prop) == PROP_NUMBER) dance.

Something along the lines of:

rv = prop_dictionary_get_int(device_properties(dev), "frequency", &value);

would be a little bit easier.  Return an error if the property isn't
found or is not a number.  Even better, let me pass a default in, and
give me a 32-bit version.

    (void) prop_dictionary_get_int32(device_properties(dev),
"frequency", &value, defaultfreq);

This would eliminate probably between 3 and 5 lines of code.  Once could
even imagine an alias using "devprop" that implies the
device_properties(dev) call.  So you'd have:

    (void) devprop_get_int32(dev, "frequency", &value, defaultfreq);

If a lot of drivers did this, then it might even result in smaller
object code, because the repeated calls to device_properties() would be
moved to a single function call. 

Just some feedback to think about.

    -- Garrett

Juan RP wrote:
> On Fri, 1 Sep 2006 14:48:25 -0700
> Jason Thorpe <thorpej@shagadelic.org> wrote:
>
>   
>> On Sep 1, 2006, at 2:19 PM, Juan RP wrote:
>>
>>     
>>>> Please make sure the dictionary key names have meaningful namespace
>>>> prefixes to them.
>>>>         
>>> Does that mean that the key of the dictionary must not have names like
>>> cpu0?
>>>       
>> I would say:
>>
>> void
>> cpu_attach(device_t parent, device_t self, void *aux)
>> {
>> 	prop_array_t array;
>>
>> 	...
>>
>> 	array = /* build the supported frequencies array */;
>>
>> 	prop_dictionary_set(device_properties(self),
>> 			    "cpu-supported-clock-frequencies", array);
>> 	prop_object_release(array);
>>
>> 	...
>> }
>>
>> Make sense?  Or maybe I'm not understanding how you want to group the  
>> information together.
>>     
>
> Yes, it's ok.
>
>   
>>>> Glad someone is taking in interest in cleaning this all up!  Yay
>>>> sysmon!  Yay proplib! :-)
>>>>         
>>> I'm starting to love it... I've attached the test code to create the  
>>> template.
>>> <prop_dictionary.c>
>>>       
>> A few comments about your test program:
>>
>>   --- snip ---
>>
>> 	array = prop_array_create_with_capacity(MAX_FREQS);
>>
>>   --- snip ---
>>
>> You don't have to specify the capacity -- the array with automatically  
>> expand as necessary.  That said, if you know that it will contain  
>> exactly two entries and no more, then create with that capacity.  But  
>> your example creates an array with a larger capacity than entries that  
>> you add.
>>     
>  
> Ah ok, good to know. 
>  
>   
>>   --- snip ---
>>
>> 	if (array == NULL ||
>> 	    !prop_dictionary_set(dict, tm->device, array))
>> 		return 1;
>>
>>   --- snip ---
>>
>> You need to prop_object_release() the array, like so:
>>
>> 	if (array == NULL ||
>> 	    !prop_dictionary_set(dict, tm->device, array)) {
>> 		prop_object_release(array);
>> 		return 1;
>> 	}
>> 	prop_object_release(array);
>>
>> Reasons:
>> 1- On failure, you need to free the array.
>>
>> 2- On success, the dictionary has retained it (bumped the ref count),  
>> and you need to release your reference now that you're done with the  
>> array, else you'll leak the reference and thus the memory.
>>     
>
> Do I need to release references with every obj used in the code?
>
> And when the obj is stored in a dictionary or array  its refcount
> is increased, but how can I use it in another code?
>
> My plan is to construct the array with supported frequencies in the MI
> driver and pass it to sysmon, but I will need to use this array too in
> the userland code.
>
> Am I right?
>  
>   
>> And finally, construct the array BEFORE storing it in the dictionary.   
>> That way the "I am done with the array" is more clear, and you also  
>> avoid having partially constructed data visible in the dictionary.
>>     
>
> Thanks for your comments.
>   


-- 
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecomputer.com/
Phone: 951 325-2134  Fax: 951 325-2191