Re: [RFC] PowerOP, PowerOP Core 1/3

["Eugeny S. Mints" <eugeny.mints@gmail.com>]

Pavel Machek wrote:
> Hi!
>
>   
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index fc2d744..f8eaf31 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -65,6 +65,7 @@ obj-$(CONFIG_ISDN)		+= isdn/
>>  obj-$(CONFIG_EDAC)		+= edac/
>>  obj-$(CONFIG_MCA)		+= mca/
>>  obj-$(CONFIG_EISA)		+= eisa/
>> +obj-$(CONFIG_POWEROP)		+= powerop/
>>     
>
> Can we make it drivers/power? It is probably suitable for more than
> powerop.
>
>   
>> +static int
>> +get_point(void *md_opt, const char *pwr_params, va_list args)
>> +{
>>     
>
> Uhuh... is there a way to use something safer than unchecked va_list
> s?
>
>   
please elaborate.
>   
>> +int 
>> +powerop_driver_register(struct powerop_driver *p)
>> +{
>> +	int error = 0;
>> +
>> +	if (! powerop_driver) {
>>     
>
> No space between ! and argument...
>
>   
>> +/* 
>> + * powerop_register_point - add new operating point with a given name to
>> + * operating points list. A caller passes power parameters for new operating
>> + * points as pairs of name/value and passes only those parameter names the
>> + * caller is interested in. PowerOP Core calls powerop driver to initialize
>> + * arch dependent part of new operating point and links new named operating 
>> + * point to the list maintained by PowerOP Core
>> + * 
>> + *
>> + * INPUT
>> + *   id         - operating point name
>> + *   pwr_params - set of (power parameter name, value) pairs
>> + *
>> + * OUTPUT
>> + *   none
>> + *
>> + * RETURN
>> + *   zero on success, error code otherwise
>> + *    
>> + */
>> +int 
>> +powerop_register_point(const char *id, const char *pwr_params, ...)
>> +{
>> +	int err = 0;
>> +	struct powerop_point *opt;
>> +	va_list args;
>> +
>> +	if ((!powerop_initialized) || (id == NULL))
>> +		return -EINVAL;
>> +
>> +	if ((opt = kmalloc(sizeof(struct powerop_point), GFP_KERNEL)) == NULL)
>> +		return -ENOMEM;
>> +
>> +	memset(opt, 0, sizeof(struct powerop_point));
>>     
>
> kzalloc?
>
> Why do we want one more string parser in kernel?
>   
referencing power parameters by name  interface is result of  gradual 
evolution of PowerOP interface mainly in order to address the 
requirement to be truly arch independent interface and to get rid of 
sharing an arch dependent data structures (a structure which defines 
operating point for a certain platform) between layers on top and 
beneath PowerOP (see the picture of the layers; thus a layer which 
creates operating points with help of PowerOP don't need to include arch 
dependent definition of operating point structure from PM Core layer). 
The latter fixes the overall PM architecture with OOP design concept.

Thanks,
Eugeny
> 							Pavel
>