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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 20-03-17 14:54, Hans de Goede wrote:
> 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.

Ok, it looks like this approach should work. Downside is that
the fuel-gauge does not get interrupts when changing state,
but I can use extcon to get power being plugged in / removed
which is the most important state change to immediately
notify userspace about.

I will rework the patch-set accordingly. For now I'm just
going to throw in a patch completely removing the battery
power_supply from the bq24190_charger driver. If that is a
problem then that one can be replaced by one using a device
property to enable/disable the battery power_supply device,
question if we are going for a property for this, what should
the default be ? I tend toward a default of off / no battery
interface unless requested but that may break compatibility
with some existing dt files + userspace expecting it to be
there ?

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