linux-kernel - RE: [PATCH] power_supply: Added support for power supply attribute sources

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 20 Jul 2012 09:54:33 +0000
From:	"Pallala, Ramakrishna" <ramakrishna.pallala@...el.com>
To:	Anton Vorontsov <cbouatmailru@...il.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] power_supply: Added support for power supply attribute
 sources

Hi Anton,

Sorry for replying late. But please find my response below.

> > This patch adds the generic support to register the drivers as power
> > supply attribute(properties) sources and adds an interface to read
> > these attributes from power supply class drivers.
> 
> So, you would add power_supply_attributes_register() calls into ADC drivers?
> This is not right.
> 
> The right approach would be to write a power supply driver that would accept
> ADC device/channel (or just a callback) for getting needed information to report.
> 
> I.e.
> 
> /*
>  * Here I just made up adc_channel struct for simplicity of the
>  * example; For real ADC dev, you really want to use Industrial IO
>  * framework, i.e. include/linux/iio/iio.h.
>  */
> struct adc_channel {
> 	...
> 	int (*get_value)(struct *adc_channel);
> };
> 
> struct adc_power_supply_platform_data {
> 	struct adc_channel *voltage;
> 	struct adc_channel *current;
> };
> 
> And the "adc power supply" driver would then call:
> 
> ...
> case POWER_SUPPLY_PROP_CURRENT:
> 	prop->intval = voltage->get_value(voltage); ...
> 
> Sure, sometimes it's not only ADC, but sensors, regulators and so forth. So pass
> all the devices to the power_supply driver, and teach the driver to work with the
> facilities.

I got your point in IIO framework, but the main reason for submitting this patch was to avoid adding platform level hooks.
If we have 5 such parameters we have to add 5 different hooks to the platform code. With this patch the driver code need not be changed or
Its platform code or data structures.

Also IMO this approach will be functionally and logically correct. We can view the power supply subsystem is collection of power supply classes
and each class is collection of power supply attributes. And these attributes can be provided by same power supply driver or it can get from other drivers
in a generic way through power supply subsystem.

> [..]
> > +struct power_supply_attr_query {
> > +	enum power_supply_property property;
> > +	enum power_supply_type type;
> > +	/* variable to store result */
> > +	union power_supply_propval res;
> > +};
> [...]
> > +extern int power_supply_get_external_attr(
> > +				struct power_supply_attr_query *query);
> 
> And even if we'd consider adding this feature, the interface seems very limited.
> What if there are two, say, batteries?

I thought through point when I was submitting this patch. My initial thought was to add a
name variable in the query structure which will mention the source name. If the data is available
only from this source we get the data otherwise not.
If a user does not provide this name(NULL) we still give whatever is available.
If we have multiple batteries we have to do source selection and this has to go to platform code.

Please let me know your thoughts on it.
If you think  something can be improved in this approach please let me know I would be happy to implement it.

Thanks,
Ram