Re: [05/15] power: supply: bq24190_charger: Limit charging voltage to 4.3V

Liam Breck
2017-03-18T19:01:47Z

On Sat, Mar 18, 2017 at 7:24 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:17 +0100, Hans de Goede wrote:
>>
>>> When the platform data asks us to not reset the charger to its default
>>> values and instead trust the firmware set values check the charging
>>> voltage and clamp it to 4.304V.
>>>
>>> Some firmwares set really too high voltages, e.g. the GPD-win I've been
>>> working on uses 4.384V. New LiHV (High Voltage) batteries may be charged
>>> upto 4.35V but that significantly impacts their lifetime, limit charging
>>> to 4.304V for safety and lifetime reasons.
>>
>>
>> See comments on patch 04; pls specify the offending firmwares and exactly
>> how they
>> misconfigure the charger. Include URLs to datasheets, etc.
>
>
> As mentioned this is a x86 machine, without any datasheets. I can add an URL
> to
> the page describing the device. Also misconfiguring may be a bit of a strong
> word, this is a LiHV battery so 4.384V is mostly fine, I would just rather
> use a max charge voltage with slightly more margin.

You can also ask for docs, and cut/paste sections, if not in Cantonese :-)

>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> drivers/power/supply/bq24190_charger.c | 27 ++++++++++++++++++++++++---
>>> 1 file changed, 24 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index c92a40e4..7bca8d0 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -504,11 +504,32 @@ static int bq24190_set_mode_host(struct
>>> bq24190_dev_info *bdi)
>>>
>>> static int bq24190_register_reset(struct bq24190_dev_info *bdi)
>>> {
>>> - int ret, limit = 100;
>>> + int ret, voltage, limit = 100;
>>> u8 v;
>>>
>>> - if (bdi->pdata && bdi->pdata->no_register_reset)
>>> - return 0;
>>> + if (bdi->pdata && bdi->pdata->no_register_reset) {
>>
>>
>> See comments on patch 04. Invoke this in set_mode_host() using value from
>> settings config.
>
>
> Again, this is x86, there is no settings config (or one that we can get too)
> other then the one stored in the registers of the IC.

Addressed on patch 04 discussion.

>>
>>> + /*
>>> + * We've been asked to keep the firmware settings as is,
>>> but
>>> + * some firmwares set really too high voltages (e.g.
>>> 4.384V).
>>> + * New LiHV (High Voltage) batteries may be charged upto
>>> 4.35V
>>> + * but that significantly impacts their lifetime, limit
>>> + * charging to 4.304V for safety and lifetime reasons.
>>> + */
>>
>>
>> Docs which belong in external config.
>
>
> Again there is NO external config. The i2c-adapter driver which registers
> the client (and passed in any config) is a generic driver for any board
> using a Cherrytrail Whiskey Cove PMIC. All that the ACPI tables tell us
> is that there is a Cherrytrail Whiskey Cove PMIC and matching fuel-gauge,
> for anything else we are on our own.

External config is platform_data. Put this comment above your
platform_data instance. Include the limit value in it.

But first consider making the fuel-gauge the envoy to userspace as
discussed in patch 04.

>>
>>> + ret = bq24190_get_field_val(bdi, BQ24190_REG_CVC,
>>> + BQ24190_REG_CVC_VREG_MASK,
>>> BQ24190_REG_CVC_VREG_SHIFT,
>>> + bq24190_cvc_vreg_values,
>>> + ARRAY_SIZE(bq24190_cvc_vreg_values), &voltage);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (voltage <= 4304000)
>>> + return 0;
>>
>>
>> The limit value should come from settings config.
>
>
> See above.
>
>>
>>> + return bq24190_set_field_val(bdi, BQ24190_REG_CVC,
>>> + BQ24190_REG_CVC_VREG_MASK,
>>> BQ24190_REG_CVC_VREG_SHIFT,
>>> + bq24190_cvc_vreg_values,
>>> + ARRAY_SIZE(bq24190_cvc_vreg_values), 4304000);
>>> + }
>>>
>>> /* Reset the registers */
>>> ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>
>>
>
> Regards,
>
> Hans