Subject: Re: Making a common API for cpu frequency drivers
To: Juan RP <juan@xtrarom.org>
From: Garrett D'Amore <garrett_damore@tadpole.com>
List: tech-kern
Date: 09/01/2006 15:36:33
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