| Message ID | 20190105183618.6519-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: adc: axp288: Fix TS-pin handling | expand |
On Sat, 5 Jan 2019 19:36:18 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > Prior to this commit there were 3 issues with our handling of the TS-pin: > > 1) There are 2 ways how the firmware can disable monitoring of the TS-pin > for designs which do not have a temperature-sensor for the battery: > a) Clearing bit 0 of the AXP20X_ADC_EN1 register > b) Setting bit 2 of the AXP288_ADC_TS_PIN_CTRL monitoring > > Prior to this commit we were unconditionally setting both bits to the > value used on devices with a TS. This causes the temperature protection to > kick in on devices without a TS, such as the Jumper ezbook v2, causing > them to not charge under Linux. > > This commit fixes this by using regmap_update_bits when updating these 2 > registers, leaving the 2 mentioned bits alone. > > The next 2 problems are related to our handling of the current-source > for the TS-pin. The current-source used for the battery temp-sensor (TS) > is shared with the GPADC. For proper fuel-gauge and charger operation the > TS current-source needs to be permanently on. But to read the GPADC we > need to temporary switch the TS current-source to ondemand, so that the > GPADC can use it, otherwise we will always read an all 0 value. > > 2) Problem 2 is we were writing hardcoded values to the ADC TS pin-ctrl > register, overwriting various other unrelated bits. Specifically we were > overwriting the current-source setting for the TS and GPIO0 pins, forcing > it to 80ųA independent of its original setting. On a Chuwi Vi10 tablet > this was causing us to get a too high adc value (due to a too high > current-source) resulting in the following errors being logged: > > ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] > ACPI Error: Method parse/execution failed \_SB.SXP1._TMP, AE_ERROR > > This commit fixes this by using regmap_update_bits to change only the > relevant bits. > > 3) After reading the GPADC channel we were unconditionally enabling the > TS current-source even on devices where the TS-pin is not used and the > current-source thus was off before axp288_adc_read_raw call. > > This commit fixes this by making axp288_adc_set_ts a nop on devices where > the ADC is not enabled for the TS-pin. > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1610545 > Fixes: 3091141d7803 ("iio: adc: axp288: Fix the GPADC pin ...") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Applied to the fixes-togreg branch of iio.git and marked for stable. Thanks, Jonathan > --- > Changes in v2: > -Make sure we enable the TS current-source on probe when the TS pin is enabled > --- > drivers/iio/adc/axp288_adc.c | 76 ++++++++++++++++++++++++++++-------- > 1 file changed, 60 insertions(+), 16 deletions(-) > > diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c > index 031d568b4972..4e339cfd0c54 100644 > --- a/drivers/iio/adc/axp288_adc.c > +++ b/drivers/iio/adc/axp288_adc.c > @@ -27,9 +27,18 @@ > #include <linux/iio/machine.h> > #include <linux/iio/driver.h> > > -#define AXP288_ADC_EN_MASK 0xF1 > -#define AXP288_ADC_TS_PIN_GPADC 0xF2 > -#define AXP288_ADC_TS_PIN_ON 0xF3 > +/* > + * This mask enables all ADCs except for the battery temp-sensor (TS), that is > + * left as-is to avoid breaking charging on devices without a temp-sensor. > + */ > +#define AXP288_ADC_EN_MASK 0xF0 > +#define AXP288_ADC_TS_ENABLE 0x01 > + > +#define AXP288_ADC_TS_CURRENT_ON_OFF_MASK GENMASK(1, 0) > +#define AXP288_ADC_TS_CURRENT_OFF (0 << 0) > +#define AXP288_ADC_TS_CURRENT_ON_WHEN_CHARGING (1 << 0) > +#define AXP288_ADC_TS_CURRENT_ON_ONDEMAND (2 << 0) > +#define AXP288_ADC_TS_CURRENT_ON (3 << 0) > > enum axp288_adc_id { > AXP288_ADC_TS, > @@ -44,6 +53,7 @@ enum axp288_adc_id { > struct axp288_adc_info { > int irq; > struct regmap *regmap; > + bool ts_enabled; > }; > > static const struct iio_chan_spec axp288_adc_channels[] = { > @@ -115,21 +125,33 @@ static int axp288_adc_read_channel(int *val, unsigned long address, > return IIO_VAL_INT; > } > > -static int axp288_adc_set_ts(struct regmap *regmap, unsigned int mode, > - unsigned long address) > +/* > + * The current-source used for the battery temp-sensor (TS) is shared > + * with the GPADC. For proper fuel-gauge and charger operation the TS > + * current-source needs to be permanently on. But to read the GPADC we > + * need to temporary switch the TS current-source to ondemand, so that > + * the GPADC can use it, otherwise we will always read an all 0 value. > + */ > +static int axp288_adc_set_ts(struct axp288_adc_info *info, > + unsigned int mode, unsigned long address) > { > int ret; > > - /* channels other than GPADC do not need to switch TS pin */ > + /* No need to switch the current-source if the TS pin is disabled */ > + if (!info->ts_enabled) > + return 0; > + > + /* Channels other than GPADC do not need the current source */ > if (address != AXP288_GP_ADC_H) > return 0; > > - ret = regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, mode); > + ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL, > + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, mode); > if (ret) > return ret; > > /* When switching to the GPADC pin give things some time to settle */ > - if (mode == AXP288_ADC_TS_PIN_GPADC) > + if (mode == AXP288_ADC_TS_CURRENT_ON_ONDEMAND) > usleep_range(6000, 10000); > > return 0; > @@ -145,14 +167,14 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev, > mutex_lock(&indio_dev->mlock); > switch (mask) { > case IIO_CHAN_INFO_RAW: > - if (axp288_adc_set_ts(info->regmap, AXP288_ADC_TS_PIN_GPADC, > + if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON_ONDEMAND, > chan->address)) { > dev_err(&indio_dev->dev, "GPADC mode\n"); > ret = -EINVAL; > break; > } > ret = axp288_adc_read_channel(val, chan->address, info->regmap); > - if (axp288_adc_set_ts(info->regmap, AXP288_ADC_TS_PIN_ON, > + if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON, > chan->address)) > dev_err(&indio_dev->dev, "TS pin restore\n"); > break; > @@ -164,13 +186,35 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev, > return ret; > } > > -static int axp288_adc_set_state(struct regmap *regmap) > +static int axp288_adc_initialize(struct axp288_adc_info *info) > { > - /* ADC should be always enabled for internal FG to function */ > - if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, AXP288_ADC_TS_PIN_ON)) > - return -EIO; > + int ret, adc_enable_val; > + > + /* > + * Determine if the TS pin is enabled and set the TS current-source > + * accordingly. > + */ > + ret = regmap_read(info->regmap, AXP20X_ADC_EN1, &adc_enable_val); > + if (ret) > + return ret; > + > + if (adc_enable_val & AXP288_ADC_TS_ENABLE) { > + info->ts_enabled = true; > + ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL, > + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, > + AXP288_ADC_TS_CURRENT_ON); > + } else { > + info->ts_enabled = false; > + ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL, > + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, > + AXP288_ADC_TS_CURRENT_OFF); > + } > + if (ret) > + return ret; > > - return regmap_write(regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK); > + /* Turn on the ADC for all channels except TS, leave TS as is */ > + return regmap_update_bits(info->regmap, AXP20X_ADC_EN1, > + AXP288_ADC_EN_MASK, AXP288_ADC_EN_MASK); > } > > static const struct iio_info axp288_adc_iio_info = { > @@ -200,7 +244,7 @@ static int axp288_adc_probe(struct platform_device *pdev) > * Set ADC to enabled state at all time, including system suspend. > * otherwise internal fuel gauge functionality may be affected. > */ > - ret = axp288_adc_set_state(axp20x->regmap); > + ret = axp288_adc_initialize(info); > if (ret) { > dev_err(&pdev->dev, "unable to enable ADC device\n"); > return ret;
diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c index 031d568b4972..4e339cfd0c54 100644 --- a/drivers/iio/adc/axp288_adc.c +++ b/drivers/iio/adc/axp288_adc.c @@ -27,9 +27,18 @@ #include <linux/iio/machine.h> #include <linux/iio/driver.h> -#define AXP288_ADC_EN_MASK 0xF1 -#define AXP288_ADC_TS_PIN_GPADC 0xF2 -#define AXP288_ADC_TS_PIN_ON 0xF3 +/* + * This mask enables all ADCs except for the battery temp-sensor (TS), that is + * left as-is to avoid breaking charging on devices without a temp-sensor. + */ +#define AXP288_ADC_EN_MASK 0xF0 +#define AXP288_ADC_TS_ENABLE 0x01 + +#define AXP288_ADC_TS_CURRENT_ON_OFF_MASK GENMASK(1, 0) +#define AXP288_ADC_TS_CURRENT_OFF (0 << 0) +#define AXP288_ADC_TS_CURRENT_ON_WHEN_CHARGING (1 << 0) +#define AXP288_ADC_TS_CURRENT_ON_ONDEMAND (2 << 0) +#define AXP288_ADC_TS_CURRENT_ON (3 << 0) enum axp288_adc_id { AXP288_ADC_TS, @@ -44,6 +53,7 @@ enum axp288_adc_id { struct axp288_adc_info { int irq; struct regmap *regmap; + bool ts_enabled; }; static const struct iio_chan_spec axp288_adc_channels[] = { @@ -115,21 +125,33 @@ static int axp288_adc_read_channel(int *val, unsigned long address, return IIO_VAL_INT; } -static int axp288_adc_set_ts(struct regmap *regmap, unsigned int mode, - unsigned long address) +/* + * The current-source used for the battery temp-sensor (TS) is shared + * with the GPADC. For proper fuel-gauge and charger operation the TS + * current-source needs to be permanently on. But to read the GPADC we + * need to temporary switch the TS current-source to ondemand, so that + * the GPADC can use it, otherwise we will always read an all 0 value. + */ +static int axp288_adc_set_ts(struct axp288_adc_info *info, + unsigned int mode, unsigned long address) { int ret; - /* channels other than GPADC do not need to switch TS pin */ + /* No need to switch the current-source if the TS pin is disabled */ + if (!info->ts_enabled) + return 0; + + /* Channels other than GPADC do not need the current source */ if (address != AXP288_GP_ADC_H) return 0; - ret = regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, mode); + ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL, + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, mode); if (ret) return ret; /* When switching to the GPADC pin give things some time to settle */ - if (mode == AXP288_ADC_TS_PIN_GPADC) + if (mode == AXP288_ADC_TS_CURRENT_ON_ONDEMAND) usleep_range(6000, 10000); return 0; @@ -145,14 +167,14 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev, mutex_lock(&indio_dev->mlock); switch (mask) { case IIO_CHAN_INFO_RAW: - if (axp288_adc_set_ts(info->regmap, AXP288_ADC_TS_PIN_GPADC, + if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON_ONDEMAND, chan->address)) { dev_err(&indio_dev->dev, "GPADC mode\n"); ret = -EINVAL; break; } ret = axp288_adc_read_channel(val, chan->address, info->regmap); - if (axp288_adc_set_ts(info->regmap, AXP288_ADC_TS_PIN_ON, + if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON, chan->address)) dev_err(&indio_dev->dev, "TS pin restore\n"); break; @@ -164,13 +186,35 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev, return ret; } -static int axp288_adc_set_state(struct regmap *regmap) +static int axp288_adc_initialize(struct axp288_adc_info *info) { - /* ADC should be always enabled for internal FG to function */ - if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, AXP288_ADC_TS_PIN_ON)) - return -EIO; + int ret, adc_enable_val; + + /* + * Determine if the TS pin is enabled and set the TS current-source + * accordingly. + */ + ret = regmap_read(info->regmap, AXP20X_ADC_EN1, &adc_enable_val); + if (ret) + return ret; + + if (adc_enable_val & AXP288_ADC_TS_ENABLE) { + info->ts_enabled = true; + ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL, + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, + AXP288_ADC_TS_CURRENT_ON); + } else { + info->ts_enabled = false; + ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL, + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, + AXP288_ADC_TS_CURRENT_OFF); + } + if (ret) + return ret; - return regmap_write(regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK); + /* Turn on the ADC for all channels except TS, leave TS as is */ + return regmap_update_bits(info->regmap, AXP20X_ADC_EN1, + AXP288_ADC_EN_MASK, AXP288_ADC_EN_MASK); } static const struct iio_info axp288_adc_iio_info = { @@ -200,7 +244,7 @@ static int axp288_adc_probe(struct platform_device *pdev) * Set ADC to enabled state at all time, including system suspend. * otherwise internal fuel gauge functionality may be affected. */ - ret = axp288_adc_set_state(axp20x->regmap); + ret = axp288_adc_initialize(info); if (ret) { dev_err(&pdev->dev, "unable to enable ADC device\n"); return ret;
Prior to this commit there were 3 issues with our handling of the TS-pin: 1) There are 2 ways how the firmware can disable monitoring of the TS-pin for designs which do not have a temperature-sensor for the battery: a) Clearing bit 0 of the AXP20X_ADC_EN1 register b) Setting bit 2 of the AXP288_ADC_TS_PIN_CTRL monitoring Prior to this commit we were unconditionally setting both bits to the value used on devices with a TS. This causes the temperature protection to kick in on devices without a TS, such as the Jumper ezbook v2, causing them to not charge under Linux. This commit fixes this by using regmap_update_bits when updating these 2 registers, leaving the 2 mentioned bits alone. The next 2 problems are related to our handling of the current-source for the TS-pin. The current-source used for the battery temp-sensor (TS) is shared with the GPADC. For proper fuel-gauge and charger operation the TS current-source needs to be permanently on. But to read the GPADC we need to temporary switch the TS current-source to ondemand, so that the GPADC can use it, otherwise we will always read an all 0 value. 2) Problem 2 is we were writing hardcoded values to the ADC TS pin-ctrl register, overwriting various other unrelated bits. Specifically we were overwriting the current-source setting for the TS and GPIO0 pins, forcing it to 80ųA independent of its original setting. On a Chuwi Vi10 tablet this was causing us to get a too high adc value (due to a too high current-source) resulting in the following errors being logged: ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] ACPI Error: Method parse/execution failed \_SB.SXP1._TMP, AE_ERROR This commit fixes this by using regmap_update_bits to change only the relevant bits. 3) After reading the GPADC channel we were unconditionally enabling the TS current-source even on devices where the TS-pin is not used and the current-source thus was off before axp288_adc_read_raw call. This commit fixes this by making axp288_adc_set_ts a nop on devices where the ADC is not enabled for the TS-pin. BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1610545 Fixes: 3091141d7803 ("iio: adc: axp288: Fix the GPADC pin ...") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: -Make sure we enable the TS current-source on probe when the TS pin is enabled --- drivers/iio/adc/axp288_adc.c | 76 ++++++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 16 deletions(-)