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

Hans de Goede
2017-03-20T13:55:41Z

Hi,

On 20-03-17 06:27, Sebastian Reichel wrote:
> Hi,
>
> On Sun, Mar 19, 2017 at 10:42:00AM +0100, Hans de Goede wrote:

<snip>

>> 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.

Interesting suggestion, 2 remarks / questions:

1) Given that we do not want to break existing setups
which may depend on the existing battery interface on the bq24190
driver and add a flag to not register it, or are you suggesting
to simply remove it all together? Removing it all together would
be better from a maintenance pov. Liam, you indicated that you
are the main user of the bq24190 driver currently, do you need
the battery power_supply interface for your use case ?

2) Not using the battery interface of the bq24190 driver at all
means that the fuelgauge driver needs to grow some extra
properties, specifically it will need to start reporting status,
something which the bq24190 driver really has a better idea
of the the fuel gauge.

Note that I do not have documentation on the fuel-gauge (not even
an out of tree driver from android-x86). It is all based on dumping
registers every 5 minutes during a charge / discharge cycle and
then puzzling together which register is what. I'm quite confident
I've this all right, but this does make adding a status property
harder (might involve some heuristics instead of being able
to get the absolute truth from the charge ic itself).

As such I think the power_supply_properties_provider proposal I'm
doing might still be a good idea. Anyways I will dig through the
dumps I've some more to see if I can find a good way to get
charging status from the fuel gauge and get back to you when
I'm done with looking at the dumps.

Regards,

Hans

>
>> 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
>>