Re: [08/15] power: supply: bq24190_charger: Add support for external fuel gauge

[Liam Breck <liam@networkimprov.net>]

On Sat, Mar 18, 2017 at 4:02 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 18-03-17 20:18, Liam Breck wrote:
>>
>> On Sat, Mar 18, 2017 at 7:31 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 18-03-17 08:10, Liam Breck wrote:
>>>>
>>>>
>>>> On Fri, 17 Mar 2017 10:55:20 +0100, Hans de Goede wrote:
>>>>
>>>>> Some platforms with a bq24190_charger have an external fuel gauge which
>>>>> makes it possible to reliably report battery (dis)charge state, at
>>>>> support for this by adding an optional get_ext_bat_property callback
>>>>> to the platform_data and using this if the platform provides it.
>>>>
>>>>
>>>>
>>>> Please do not pollute the charger with fuel-gauge functionality; use the
>>>> gauge driver directly.
>>>
>>>
>>>
>>> That is not how the userspace ABI works, the userspace ABI says one
>>> battery one power_supply device, so we need to combine the data from
>>> the 2 sources.
>>
>>
>> Reference?
>
>
> This is just how things work, if you define 2 battery type power-supplies
> upower will export battery info for 2 batteries to userspace and
> battery panel applets will show 2 batteries (which is useful for
> laptops which actually have 2 batteries like eg the Lenovo T440, X240, etc.
> which have both an internal battery and a replace battery at the back.

Have you looked into patching upower? I'd say the de facto ABI is
separate charger and gauge devices...

>> See patch 04 discussion about fuel-gauge as primary point of contact.
>>
>> Another approach to consider: a pseudo-driver which uses sysfs or
>> callbacks into two actual drivers.
>
>
> Already answered in the patch 04 discussion.
>
>>>> And calling into a mystery module with a pointer from platform-data is
>>>> scary! St. Patrick's
>>>> Day is wrong for this; try April Fool's or Halloween ;-)
>>>
>>>
>>>
>>> Since we control both the caller and the callee I fail to see how
>>> this is scary in any way.
>>
>>
>> This patch looks like a hack to me. Pls find another way.
>
>
> Poking sysfs files from kernelspace is a much bugger hack, that +
> the code duplication required in making the fuel-gauge the leading
> driver really makes me believe this "hack" is the best solution.
>
> Also keep in mind that the fuel-gauge has no status interrupt for
> events like power getting plugged in charging being done, fault
> interrupts, etc. So we would also need some way to tap into
> the bq24190's interrupt from the fuel-gauge driver.
>
> Anways lets continue this discussion in the patch 04 part
> of the thread.
>
>
> Regards,
>
> Hans
>
>
>
>
>>>>> By convention the callback will return -ENXIO when it is not ready yet,
>>>>> or the driver providing it has been unbound from its device. Since it
>>>>> returns the same error when unbound it cannot return -EPROBE_DEFER
>>>>> as that is not a valid errno.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>>  drivers/power/supply/bq24190_charger.c | 41
>>>>> +++++++++++++++++++++++++++++++---
>>>>>  include/linux/power/bq24190_charger.h  |  4 ++++
>>>>>  2 files changed, 42 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>>> b/drivers/power/supply/bq24190_charger.c
>>>>> index 9014dee..9fe69a5 100644
>>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>>> @@ -1111,7 +1111,10 @@ static int bq24190_battery_get_property(struct
>>>>> power_supply *psy,
>>>>>                 ret = 0;
>>>>>                 break;
>>>>>         default:
>>>>> -               ret = -ENODATA;
>>>>> +               if (bdi->pdata && bdi->pdata->get_ext_bat_property)
>>>>> +                       ret = bdi->pdata->get_ext_bat_property(psp,
>>>>> val);
>>>>> +               else
>>>>> +                       ret = -ENODATA;
>>>>>         }
>>>>>
>>>>>         pm_runtime_put_sync(bdi->dev);
>>>>> @@ -1168,12 +1171,31 @@ static enum power_supply_property
>>>>> bq24190_battery_properties[] = {
>>>>>         POWER_SUPPLY_PROP_TECHNOLOGY,
>>>>>         POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
>>>>>         POWER_SUPPLY_PROP_SCOPE,
>>>>> +       /* Begin of extended battery properties */
>>>>> +       POWER_SUPPLY_PROP_VOLTAGE_NOW,
>>>>> +       POWER_SUPPLY_PROP_VOLTAGE_AVG,
>>>>> +       POWER_SUPPLY_PROP_VOLTAGE_OCV,
>>>>> +       POWER_SUPPLY_PROP_CURRENT_NOW,
>>>>> +       POWER_SUPPLY_PROP_CURRENT_AVG,
>>>>> +       POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
>>>>> +       POWER_SUPPLY_PROP_CHARGE_FULL,
>>>>> +       POWER_SUPPLY_PROP_CHARGE_NOW,
>>>>>  };
>>>>>
>>>>>  static const struct power_supply_desc bq24190_battery_desc = {
>>>>>         .name                   = "bq24190-battery",
>>>>>         .type                   = POWER_SUPPLY_TYPE_BATTERY,
>>>>>         .properties             = bq24190_battery_properties,
>>>>> +       .num_properties         = 6,
>>>>> +       .get_property           = bq24190_battery_get_property,
>>>>> +       .set_property           = bq24190_battery_set_property,
>>>>> +       .property_is_writeable  =
>>>>> bq24190_battery_property_is_writeable,
>>>>> +};
>>>>> +
>>>>> +static const struct power_supply_desc bq24190_ext_battery_desc = {
>>>>> +       .name                   = "bq24190-battery",
>>>>> +       .type                   = POWER_SUPPLY_TYPE_BATTERY,
>>>>> +       .properties             = bq24190_battery_properties,
>>>>>         .num_properties         =
>>>>> ARRAY_SIZE(bq24190_battery_properties),
>>>>>         .get_property           = bq24190_battery_get_property,
>>>>>         .set_property           = bq24190_battery_set_property,
>>>>> @@ -1336,6 +1358,15 @@ static int bq24190_probe(struct i2c_client
>>>>> *client,
>>>>>                 return -EINVAL;
>>>>>         }
>>>>>
>>>>> +       if (bdi->pdata && bdi->pdata->get_ext_bat_property) {
>>>>> +               union power_supply_propval val;
>>>>> +
>>>>> +               /* Check external fuel gauge is ready */
>>>>> +               ret = bdi->pdata->get_ext_bat_property(0, &val);
>>>>> +               if (ret == -ENXIO)
>>>>> +                       return -EPROBE_DEFER;
>>>>> +       }
>>>>> +
>>>>>         pm_runtime_enable(dev);
>>>>>         pm_runtime_resume(dev);
>>>>>
>>>>> @@ -1357,8 +1388,12 @@ static int bq24190_probe(struct i2c_client
>>>>> *client,
>>>>>         }
>>>>>
>>>>>         battery_cfg.drv_data = bdi;
>>>>> -       bdi->battery = power_supply_register(dev,
>>>>> &bq24190_battery_desc,
>>>>> -                                               &battery_cfg);
>>>>> +       if (bdi->pdata && bdi->pdata->get_ext_bat_property)
>>>>> +               bdi->battery = power_supply_register(dev,
>>>>> +                                   &bq24190_ext_battery_desc,
>>>>> &battery_cfg);
>>>>> +       else
>>>>> +               bdi->battery = power_supply_register(dev,
>>>>> +                                   &bq24190_battery_desc,
>>>>> &battery_cfg);
>>>>>         if (IS_ERR(bdi->battery)) {
>>>>>                 dev_err(dev, "Can't register battery\n");
>>>>>                 ret = PTR_ERR(bdi->battery);
>>>>> diff --git a/include/linux/power/bq24190_charger.h
>>>>> b/include/linux/power/bq24190_charger.h
>>>>> index 8d918cb..02d248b 100644
>>>>> --- a/include/linux/power/bq24190_charger.h
>>>>> +++ b/include/linux/power/bq24190_charger.h
>>>>> @@ -9,8 +9,12 @@
>>>>>  #ifndef _BQ24190_CHARGER_H_
>>>>>  #define _BQ24190_CHARGER_H_
>>>>>
>>>>> +#include <linux/power_supply.h>
>>>>> +
>>>>>  struct bq24190_platform_data {
>>>>>         bool no_register_reset;
>>>>> +       int (*get_ext_bat_property)(enum power_supply_property prop,
>>>>> +                                   union power_supply_propval *val);
>>>>>  };
>>>>>
>>>>>  #endif
>>>>
>>>>
>>>>
>>>
>