Re: [linux-pm] [PATCH] PowerOP, PowerOP Core, 1/2

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

Pavel Machek wrote:
> Hi!
> 
>> The PowerOP Core provides completely arch independent interface
>> to create and control operating points which consist of arbitrary
>> subset of power parameters available on a certain platform.
>> Also, PowerOP Core provides optional SysFS interface to access
>> operating point from userspace.
> 
> Please inline patches and sign them off.

hmm, seems my bad. will double check.

> 
> Also if you are providing new userland interface, describe it... in
> Documentation/ABI.

seems already discussed

>> +struct powerop_driver {
>> +	char *name;
>> +	void *(*create_point) (const char *pwr_params, va_list args);
>> +	int (*set_point) (void *md_opt);
>> +	int (*get_point) (void *md_opt, const char *pwr_params, va_list args);
>> +};
> 
> We can certainly get better interface than va_list, right?

Please elaborate.

> 
>> +
>> +#
>> +# powerop
>> +#
>> +
>> +menu "PowerOP (Power Management)"
>> +
>> +config POWEROP
>> +	tristate "PowerOP Core"
>> +	help
> 
> Hohum, this is certainly going to be clear to confused user...

please elaborate.

>> +	list_add_tail(&opt->node, &named_opt_list);
>> +	strcpy(registered_names[registered_opt_number], id);
>> +	registered_opt_number++;
>> +	up(&named_opt_list_mutex);
>> +
>> +	blocking_notifier_call_chain(&powerop_notifier_list,
>> +				     POWEROP_REGISTER_EVENT, id);
>> +	return 0;
>> +
>> +      fail_set_name:
>> +	kfree(opt->md_opt);
>> +
>> +      fail_opt_create:
>> +	kfree(registered_names[registered_opt_number]);
>> +
>> +      fail_name_nomem:
>> +	kfree(opt);
>> +	return err;
>> +}
> 
> Careful about spaces vs. tabs...

will double check but I'm pretty sure I ran all files thru lindent.


> ...so, you got support for 20 operating points... And this should
> include devices, too, right? 

sorry, don't understand the question. an operating point is a set of platform 
power parameters.

>How is it going to work on 8cpu box? will
> you have states like cpu1_800MHz_cpu2_1600MHz_cpu3_800MHz_... ?

i do not operate with term 'state' so I don't understand what it means here.

	Eugeny

> 								Pavel