Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

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

Hi,

On 22-03-17 19:41, Liam Breck wrote:
> On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Resetting the charger should never be necessary it should always have
>> sane values programmed. If it is running with invalid values while we
>> are not running (system turned off or suspended) there is a big problem
>> as that may lead to overcharging the battery.
>>
>> The reset in suspend() is meant to put the charger back into default
>> mode, but this is not necessary and not a good idea. If the charger has
>> been programmed with a higher max charge_current / charge_voltage then
>> putting it back in default-mode will reset those to the safe power-on
>> defaults, leading to slower charging, or charging to a lower voltage
>> (and thus not using the full capacity) while suspended which is
>> undesirable. Reprogramming the max charge_current / charge_voltage
>> after the reset will not help here as that will put the charger back
>> in host mode and start the i2c watchdog if the host then does not do
>> anything for 40s (iow if we're suspended for more then 40s) the watchdog
>> expires resetting the device to default-mode, including resetting all
>> the registers to there safe power-on defaults. So the only way to keep
>> using custom charge settings while suspending is to keep the charger in
>> its normal running state with the i2c watchdog disabled. This is fine
>> as the charger will still automatically switch from constant current
>> to constant voltage and stop charging when the battery is full.
>>
>> Besides never being necessary resetting the charger also causes problems
>> on systems where the charge voltage limit is set higher then the reset
>> value, if this is the case and the charger is reset while charging and
>> the battery voltage is between the 2 voltages, then about half the time
>> the charger gets confused and claims to be charging (REG08 contains 0x64)
>> but in reality the charger has decoupled itself from VBUS (Q1 off) and
>> is drawing 0A from VBUS, leaving the system running from the battery.
>>
>> Cc: Liam Breck <kernel@networkimprov.net>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -This is a new patch in v2 of this patch-set
>> ---
>>  drivers/power/supply/bq24190_charger.c | 58 ----------------------------------
>>  1 file changed, 58 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index 7a2a496..b535f24 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -526,40 +526,6 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>>         return bq24190_write(bdi, BQ24190_REG_CTTC, v);
>>  }
>>
>> -static int bq24190_register_reset(struct bq24190_dev_info *bdi)
>> -{
>> -       int ret, limit = 100;
>> -       u8 v;
>> -
>> -       /* Reset the registers */
>> -       ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
>> -                       BQ24190_REG_POC_RESET_MASK,
>> -                       BQ24190_REG_POC_RESET_SHIFT,
>> -                       0x1);
>> -       if (ret < 0)
>> -               return ret;
>> -
>> -       /* Reset bit will be cleared by hardware so poll until it is */
>> -       do {
>> -               ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
>> -                               BQ24190_REG_POC_RESET_MASK,
>> -                               BQ24190_REG_POC_RESET_SHIFT,
>> -                               &v);
>> -               if (ret < 0)
>> -                       return ret;
>> -
>> -               if (!v)
>> -                       break;
>> -
>> -               udelay(10);
>> -       } while (--limit);
>> -
>> -       if (!limit)
>> -               return -EIO;
>> -
>> -       return 0;
>> -}
>
> Let's leave this in with __maybe_unused.

I don't like leaving dead code around and bringing it back from git
history is quite easy.

Regards,

Hans


>
>>  /* Charger power supply property routines */
>>
>>  static int bq24190_charger_get_charge_type(struct bq24190_dev_info *bdi,
>> @@ -1380,10 +1346,6 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>>                 return -ENODEV;
>>         }
>>
>> -       ret = bq24190_register_reset(bdi);
>> -       if (ret < 0)
>> -               return ret;
>> -
>>         ret = bq24190_set_mode_host(bdi);
>>         if (ret < 0)
>>                 return ret;
>> @@ -1534,7 +1496,6 @@ static int bq24190_remove(struct i2c_client *client)
>>                 pm_runtime_put_noidle(bdi->dev);
>>         }
>>
>> -       bq24190_register_reset(bdi);
>>         bq24190_sysfs_remove_group(bdi);
>>         power_supply_unregister(bdi->battery);
>>         power_supply_unregister(bdi->charger);
>> @@ -1577,23 +1538,6 @@ static __maybe_unused int bq24190_runtime_resume(struct device *dev)
>>
>>  static __maybe_unused int bq24190_pm_suspend(struct device *dev)
>>  {
>> -       struct i2c_client *client = to_i2c_client(dev);
>> -       struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
>> -       int error;
>> -
>> -       error = pm_runtime_get_sync(bdi->dev);
>> -       if (error < 0) {
>> -               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
>> -               pm_runtime_put_noidle(bdi->dev);
>> -       }
>> -
>> -       bq24190_register_reset(bdi);
>> -
>> -       if (error >= 0) {
>> -               pm_runtime_mark_last_busy(bdi->dev);
>> -               pm_runtime_put_autosuspend(bdi->dev);
>> -       }
>> -
>>         return 0;
>>  }
>>
>> @@ -1612,8 +1556,6 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
>>                 pm_runtime_put_noidle(bdi->dev);
>>         }
>>
>> -       bq24190_register_reset(bdi);
>> -       bq24190_set_mode_host(bdi);
>>         bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>>
>>         if (error >= 0) {
>> --
>> 2.9.3
>>