| Message ID | 20190104221305.26314-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: adc: axp288: Fix TS-pin handling | expand |
On Fri, 4 Jan 2019 23:13:05 +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> To me this looks fine (really minor comment inline). I'll just wait until rc1 is out so as to have a suitable base to gather a few fixes on before applying this. Give me a poke if I seem to have forgotten in a week or so! Will mark it for stable when I apply. Thanks, Jonathan > --- > drivers/iio/adc/axp288_adc.c | 74 ++++++++++++++++++++++++++++-------- > 1 file changed, 58 insertions(+), 16 deletions(-) > > diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c > index 031d568b4972..99a6b804fd49 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,33 @@ 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 if it is not enabled, > + * turn the TS current-source off, so that the shared current-source > + * will be available for the GPADC. > + */ > + 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; > + } else { > + info->ts_enabled = false; Really minor but I'd have found this clearer as info->ts_enabled = adc_enable_val & AXP288_ADC_TS_ENABLE; if (info->ts_enabled) { ... > + 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 +242,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;
Hi, On 05-01-19 17:31, Jonathan Cameron wrote: > On Fri, 4 Jan 2019 23:13:05 +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> > > To me this looks fine (really minor comment inline). > > I'll just wait until rc1 is out so as to have a suitable base to gather > a few fixes on before applying this. Thank you for the review. Note I noticed one small oversight this morning, we no longer set the TS current-source to on on probe when the TS pin is enabled. In practice it seems the firmware always sets the TS pin to on in this case, but better safe then sorry and we did explictly set the TS current-source to on before this patch. I've prepared a v2 fixing this. Regards, Hans >> --- >> drivers/iio/adc/axp288_adc.c | 74 ++++++++++++++++++++++++++++-------- >> 1 file changed, 58 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c >> index 031d568b4972..99a6b804fd49 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,33 @@ 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 if it is not enabled, >> + * turn the TS current-source off, so that the shared current-source >> + * will be available for the GPADC. >> + */ >> + 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; >> + } else { >> + info->ts_enabled = false; > Really minor but I'd have found this clearer as > info->ts_enabled = adc_enable_val & AXP288_ADC_TS_ENABLE; > if (info->ts_enabled) { ... > >> + 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 +242,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..99a6b804fd49 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,33 @@ 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 if it is not enabled, + * turn the TS current-source off, so that the shared current-source + * will be available for the GPADC. + */ + 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; + } 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 +242,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> --- drivers/iio/adc/axp288_adc.c | 74 ++++++++++++++++++++++++++++-------- 1 file changed, 58 insertions(+), 16 deletions(-)