Messages in this thread | | | From | Nicolas Boichat <> | Date | Mon, 10 Aug 2020 15:32:14 +0800 | Subject | Re: [PATCH] power: supply: sbs-battery: don't assume i2c errors as battery disconnect |
| |
On Mon, Aug 10, 2020 at 3:02 PM Ikjoon Jang <ikjn@chromium.org> wrote: > > Current sbs-battery considers all smbus errors as diconnection events > when battery-detect pin isn't supplied, and restored to present state back > on any successful transaction were made. > > This can leads to unlimited state changes between present and !present > when one unsupported command was requested and other following commands > were successful, e.g. udev rules tries to read multiple properties. > > This patch checks battery presence by reading well-known register to > check battery existence. > > Signed-off-by: Ikjoon Jang <ikjn@chromium.org> > --- > drivers/power/supply/sbs-battery.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c > index 73461321eb05..483f814385a3 100644 > --- a/drivers/power/supply/sbs-battery.c > +++ b/drivers/power/supply/sbs-battery.c > @@ -875,10 +875,17 @@ static int sbs_get_property(struct power_supply *psy, > return -EINVAL; > } > > - if (!chip->gpio_detect && > - chip->is_present != (ret >= 0)) { > - sbs_update_presence(chip, (ret >= 0)); > - power_supply_changed(chip->power_supply); > + if (!chip->gpio_detect && chip->is_present != (ret >=0)) { > + bool old_present = chip->is_present; > + union power_supply_propval val; > + > + ret = sbs_get_battery_presence_and_health( > + client, POWER_SUPPLY_PROP_PRESENT, &val); > + > + sbs_update_presence(chip, !ret && !!(val.intval));
No need for the !!: !ret && val.intval .
> + > + if (old_present != chip->is_present) > + power_supply_changed(chip->power_supply); > } > > done: > @@ -1063,11 +1070,11 @@ static int sbs_probe(struct i2c_client *client) > * to the battery. > */ > if (!(force_load || chip->gpio_detect)) { > - rc = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); > - > - if (rc < 0) { > - dev_err(&client->dev, "%s: Failed to get device status\n", > - __func__); > + union power_supply_propval val; > + rc = sbs_get_battery_presence_and_health( > + client, POWER_SUPPLY_PROP_PRESENT, &val); > + if (rc < 0 || !val.intval) { > + dev_err(&client->dev, "Failed to get present status\n"); > goto exit_psupply;
sbs_get_battery_presence_and_health also reads sbs_data[REG_STATUS].addr. That's good.
However, with psp == POWER_SUPPLY_PROP_PRESENT, sbs_get_battery_presence_and_health always returns 0, but sets val.intval = 0 in case of error.
This means that you goto exit_psupply with rc == 0, and sbs_probe does not fail.
I wonder if you should just drop this change, and use sbs_read_word_data(client, sbs_data[REG_STATUS].addr); above, too.
> } > } > -- > 2.28.0.236.gb10cc79966-goog >
| |