Re: [04/15] power: supply: bq24190_charger: Add no_register_reset pdata flag

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4675 bytes --]

Hi,

On Sun, Mar 19, 2017 at 10:42:00AM +0100, Hans de Goede wrote:
> On 19-03-17 09:22, Hans de Goede wrote:
> > > Then consider the pseudo-driver concept. That would be generally
> > > useful for any charger/gauge pairing. Both drivers would provide
> > > callbacks to it.
> > 
> > So your rejecting a patch which adds 30 lines of code for some
> > vague generic pseudo-driver concept without offering any design
> > direction on what such a concept would look like and without taking
> > into account that so far this seems to be a one-of problem.
> 
> I still think the pseudo-driver idea is a bad idea. Having 1 driver
> which somehow combines things from 2 drivers without clear semantics
> feels wrong.

Apart from being ugly, that does not work anyways, how should the
drivers know when its ok to expose their info to userspace and when
its not.

> As said before I think your suggested solution lacks any
> design direction, I've been thinking about this a bit more and
> I've come up with an alternative solution with clearly defined
> goals and semantics.
> 
> We want one driver which is solidly in control of the charger
> since getting that wrong is quite bad and we want to extend
> the information it is exporting to userspace in the form of
> power_supply properties with some extra info.
> 
> So taking that as a starting point and generalizing that,
> I think that if we want we can make something more generic then
> my original patch for this. My idea is to introduce something
> called a power_supply_properties_provider, with an API
> like this:

Thanks for thinking about this. From what I can see it should
be enough to just avoid exposing a battery device at all in
the charger driver, though. It does not really provide useful
information anyways.

> typedef int (*power_supply_properties_provider_get_property_t)(
> 	enum power_supply_property prop, union power_supply_propval *val,
> 	void *driver_data);
> 
> struct power_supply_properties_provider;
> 
> struct power_supply_properties_provider *
> power_supply_properties_provider_register(
> 	const char *name,
> 	power_supply_properties_provider_get_property_t get_property,
> 	const enum power_supply_property *properties,
> 	size_t num_properties,
> 	void *driver_data);
> 
> void power_supply_properties_provider_unregister(
> 	struct power_supply_properties_provider *pspp);
> 
> struct power_supply_properties_provider *
> 	power_supply_properties_provider_get(const char *name);
> 
> void power_supply_properties_provider_put(
> 	struct power_supply_properties_provider *pspp);
> 
> int power_supply_properties_provider_merge_properties(
> 	struct power_supply_properties_provider *pspp,
> 	const enum power_supply_property *properties,
> 	size_t num_properties,
> 	enum power_supply_property **merged_properties_ret,
> 	size_t *merged_num_properties_ret);
> 
> int power_supply_properties_provider_get_property(
> 	struct power_supply_properties_provider *pspp,
> 	enum power_supply_property prop,
> 	union power_supply_propval *val);
> 
> So in this case the faul-gauge driver would call
> power_supply_properties_provider_register() from probe and
> power_supply_properties_provider_unregister() from remove.
> 
> The struct power_supply_properties_provider will be ref-counted
> so that it will stick around after unregister in case any
> consumers who have gotten a ref through
> power_supply_properties_provider_get can still call
> power_supply_properties_provider_get_property, which after
> unregister will simply always return -ENXIO.
> 
> A driver wanting to use extra properties like bq24190_charger
> will call power_supply_properties_provider_get (with a name
> provided through platform_data), use a dynamically allocated
> struct power_supply_desc and fill the properties of that
> using power_supply_properties_provider_merge_properties
> to merge its own properties with the
> power_supply_properties_provider's properties and in its
> get_property method have a default label which calls
> power_supply_properties_provider_get_property.
> 
> This would still add quite a bit more code then my original
> patch for what may very well end up being a one-of solution,
> but I guess we may encounter this problem more often and
> this will offer a nice generic and clean way for dealing
> with adding extra info from other sources to a power_supply
> driver, so I'm happy to turn this idea / design into working
> code if people like this better then my original patch.
> 
> Liam, Sebastian what do you think of the above proposal?
> 
> Regards,
> 
> Hans
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]