Re: [09/15] power: supply: bq24190_charger: Add voltage_max_design prop to battery

Hans de Goede
2017-03-18T23:11:15Z

Hi,

On 18-03-17 20:34, Liam Breck wrote:
> On Sat, Mar 18, 2017 at 7:34 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:21 +0100, Hans de Goede wrote:
>>>
>>>> When combined with an external fuel-gauge, upower needs
>>>> voltage_max_design
>>>> as it internally does all its calculations in Watts and converts the
>>>> charge_foo properties from A to Watts by using voltage_max_design.
>>>
>>>
>>> This is a battery characteristic which should be obtained from the fuel
>>> gauge (e.g. V at charge
>>> termination)
>>
>>
>> V at charge termination is *exactly* what bq24190_charger_get_voltage()
>> returns. And the fuel-gauge only measures charging it does not control
>> it, so unless we had some nvram to store things in over time the fg
>> driver cannot no what "V at charge termination" will be, where as the
>> bq24190_charger code actually knows as it controls at which voltage
>> the charger switches from constant current to constant voltage mode.
>>
>>> or external battery config.
>>>
>>> See also https://patchwork.kernel.org/patch/9626487/
>>
>>
>> There is no external battery config on x86.
>
> ACPI?

Ideally yes, in practice no.

> The battery data/config should really be used to set the charger
> register in question.

Again the contents of the register is the config, but as mentioned
in the patch04 discussion I'm fine with adding a flag to read the
info from the registers, rather then just not touching the registers,
just as long as the going from register contents to "battery data"
is done in the bq24190_charger driver as that already has all the
lookup tables, etc.

> charger/voltage_max already has this. Can you find a way to use that?

Nope, upower will look at the battery/max_voltage attribute, so we
need it under the battery power_supply, we cannot just make up
new userspace ABI here.

Regards,

Hans

>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> drivers/power/supply/bq24190_charger.c | 10 +++++++++-
>>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>> b/drivers/power/supply/bq24190_charger.c
>>>> index 9fe69a5..82cb33d 100644
>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>> @@ -1103,6 +1103,13 @@ static int bq24190_battery_get_property(struct
>>>> power_supply *psy,
>>>> val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
>>>> ret = 0;
>>>> break;
>>>> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>>>> + /*
>>>> + * Report charger configured voltage as max design
>>>> voltage,
>>>> + * not entirely correct, but userspace needs something
>>>> here.
>>>> + */
>>>> + ret = bq24190_charger_get_voltage(bdi, val);
>>>> + break;
>>>> case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>>>> ret = bq24190_battery_get_temp_alert_max(bdi, val);
>>>> break;
>>>> @@ -1169,6 +1176,7 @@ static enum power_supply_property
>>>> bq24190_battery_properties[] = {
>>>> POWER_SUPPLY_PROP_HEALTH,
>>>> POWER_SUPPLY_PROP_ONLINE,
>>>> POWER_SUPPLY_PROP_TECHNOLOGY,
>>>> + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
>>>> POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
>>>> POWER_SUPPLY_PROP_SCOPE,
>>>> /* Begin of extended battery properties */
>>>> @@ -1186,7 +1194,7 @@ static const struct power_supply_desc
>>>> bq24190_battery_desc = {
>>>> .name = "bq24190-battery",
>>>> .type = POWER_SUPPLY_TYPE_BATTERY,
>>>> .properties = bq24190_battery_properties,
>>>> - .num_properties = 6,
>>>> + .num_properties = 7,
>>>> .get_property = bq24190_battery_get_property,
>>>> .set_property = bq24190_battery_set_property,
>>>> .property_is_writeable = bq24190_battery_property_is_writeable,