| Message ID | 20190104221054.26200-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | ACPI / PMIC: xpower: Fix TS-pin current-source handling | expand |
On Fri, Jan 04, 2019 at 11:10:54PM +0100, Hans de Goede wrote: > 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. > > The switching from on to on-ondemand is not necessary when the TS > current-source is off (this happens on devices which do not have a TS). > > Prior to this commit there were 2 issues with our handling of the TS > current-source switching: > > 1) 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 acpi_lpat_raw_to_temp() returning -ENOENT, resulting in: > > 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. > > 2) At the end of intel_xpower_pmic_get_raw_temp() 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 on entry of the function. > > This commit fixes this by checking if the TS current-source is off when > entering intel_xpower_pmic_get_raw_temp() and if so it is left as is. > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Fixes: 58eefe2f3f53 ("ACPI / PMIC: xpower: Do pinswitch ... reading GPADC") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/pmic/intel_pmic_xpower.c | 41 +++++++++++++++++++++------ > 1 file changed, 33 insertions(+), 8 deletions(-) > > diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c > index 2579675b7082..e7c0006e6602 100644 > --- a/drivers/acpi/pmic/intel_pmic_xpower.c > +++ b/drivers/acpi/pmic/intel_pmic_xpower.c > @@ -20,8 +20,11 @@ > #define GPI1_LDO_ON (3 << 0) > #define GPI1_LDO_OFF (4 << 0) > > -#define AXP288_ADC_TS_PIN_GPADC 0xf2 > -#define AXP288_ADC_TS_PIN_ON 0xf3 > +#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) > > static struct pmic_table power_table[] = { > { > @@ -212,22 +215,44 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg, > */ > static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) > { > + int ret, adc_ts_pin_ctrl; > u8 buf[2]; > - int ret; > > - ret = regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, > - AXP288_ADC_TS_PIN_GPADC); > + /* > + * 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. > + * > + * Note that the switching from on to on-ondemand is not necessary > + * when the TS current-source is off (this happens on devices which > + * do not use the TS-pin). > + */ > + ret = regmap_read(regmap, AXP288_ADC_TS_PIN_CTRL, &adc_ts_pin_ctrl); > if (ret) > return ret; > > - /* After switching to the GPADC pin give things some time to settle */ > - usleep_range(6000, 10000); > + if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { > + ret = regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, > + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, > + AXP288_ADC_TS_CURRENT_ON_ONDEMAND); > + if (ret) > + return ret; > + > + /* Wait a bit after switching the current-source */ > + usleep_range(6000, 10000); > + } > > ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2); > if (ret == 0) > ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f); > > - regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, AXP288_ADC_TS_PIN_ON); > + if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { > + regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, > + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, > + AXP288_ADC_TS_CURRENT_ON); > + } > > return ret; > } > -- > 2.19.1 >
On Fri, Jan 4, 2019 at 11:11 PM Hans de Goede <hdegoede@redhat.com> wrote: > > 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. > > The switching from on to on-ondemand is not necessary when the TS > current-source is off (this happens on devices which do not have a TS). > > Prior to this commit there were 2 issues with our handling of the TS > current-source switching: > > 1) 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 acpi_lpat_raw_to_temp() returning -ENOENT, resulting in: > > 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. > > 2) At the end of intel_xpower_pmic_get_raw_temp() 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 on entry of the function. > > This commit fixes this by checking if the TS current-source is off when > entering intel_xpower_pmic_get_raw_temp() and if so it is left as is. > > Fixes: 58eefe2f3f53 ("ACPI / PMIC: xpower: Do pinswitch ... reading GPADC") AFAICS this commit went in during the 4.14 cycle, so I'm assuming the patch to be stable-candidate material, right?
Hi, On 07-01-19 11:59, Rafael J. Wysocki wrote: > On Fri, Jan 4, 2019 at 11:11 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> 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. >> >> The switching from on to on-ondemand is not necessary when the TS >> current-source is off (this happens on devices which do not have a TS). >> >> Prior to this commit there were 2 issues with our handling of the TS >> current-source switching: >> >> 1) 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 acpi_lpat_raw_to_temp() returning -ENOENT, resulting in: >> >> 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. >> >> 2) At the end of intel_xpower_pmic_get_raw_temp() 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 on entry of the function. >> >> This commit fixes this by checking if the TS current-source is off when >> entering intel_xpower_pmic_get_raw_temp() and if so it is left as is. >> >> Fixes: 58eefe2f3f53 ("ACPI / PMIC: xpower: Do pinswitch ... reading GPADC") > > AFAICS this commit went in during the 4.14 cycle, so I'm assuming the > patch to be stable-candidate material, right? Right. Regards, Hans
On Friday, January 4, 2019 11:10:54 PM CET Hans de Goede wrote: > 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. > > The switching from on to on-ondemand is not necessary when the TS > current-source is off (this happens on devices which do not have a TS). > > Prior to this commit there were 2 issues with our handling of the TS > current-source switching: > > 1) 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 acpi_lpat_raw_to_temp() returning -ENOENT, resulting in: > > 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. > > 2) At the end of intel_xpower_pmic_get_raw_temp() 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 on entry of the function. > > This commit fixes this by checking if the TS current-source is off when > entering intel_xpower_pmic_get_raw_temp() and if so it is left as is. > > Fixes: 58eefe2f3f53 ("ACPI / PMIC: xpower: Do pinswitch ... reading GPADC") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/pmic/intel_pmic_xpower.c | 41 +++++++++++++++++++++------ > 1 file changed, 33 insertions(+), 8 deletions(-) > > diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c > index 2579675b7082..e7c0006e6602 100644 > --- a/drivers/acpi/pmic/intel_pmic_xpower.c > +++ b/drivers/acpi/pmic/intel_pmic_xpower.c > @@ -20,8 +20,11 @@ > #define GPI1_LDO_ON (3 << 0) > #define GPI1_LDO_OFF (4 << 0) > > -#define AXP288_ADC_TS_PIN_GPADC 0xf2 > -#define AXP288_ADC_TS_PIN_ON 0xf3 > +#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) > > static struct pmic_table power_table[] = { > { > @@ -212,22 +215,44 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg, > */ > static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) > { > + int ret, adc_ts_pin_ctrl; > u8 buf[2]; > - int ret; > > - ret = regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, > - AXP288_ADC_TS_PIN_GPADC); > + /* > + * 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. > + * > + * Note that the switching from on to on-ondemand is not necessary > + * when the TS current-source is off (this happens on devices which > + * do not use the TS-pin). > + */ > + ret = regmap_read(regmap, AXP288_ADC_TS_PIN_CTRL, &adc_ts_pin_ctrl); > if (ret) > return ret; > > - /* After switching to the GPADC pin give things some time to settle */ > - usleep_range(6000, 10000); > + if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { > + ret = regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, > + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, > + AXP288_ADC_TS_CURRENT_ON_ONDEMAND); > + if (ret) > + return ret; > + > + /* Wait a bit after switching the current-source */ > + usleep_range(6000, 10000); > + } > > ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2); > if (ret == 0) > ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f); > > - regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, AXP288_ADC_TS_PIN_ON); > + if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { > + regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, > + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, > + AXP288_ADC_TS_CURRENT_ON); > + } > > return ret; > } > Applied, thanks!
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c index 2579675b7082..e7c0006e6602 100644 --- a/drivers/acpi/pmic/intel_pmic_xpower.c +++ b/drivers/acpi/pmic/intel_pmic_xpower.c @@ -20,8 +20,11 @@ #define GPI1_LDO_ON (3 << 0) #define GPI1_LDO_OFF (4 << 0) -#define AXP288_ADC_TS_PIN_GPADC 0xf2 -#define AXP288_ADC_TS_PIN_ON 0xf3 +#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) static struct pmic_table power_table[] = { { @@ -212,22 +215,44 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg, */ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) { + int ret, adc_ts_pin_ctrl; u8 buf[2]; - int ret; - ret = regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, - AXP288_ADC_TS_PIN_GPADC); + /* + * 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. + * + * Note that the switching from on to on-ondemand is not necessary + * when the TS current-source is off (this happens on devices which + * do not use the TS-pin). + */ + ret = regmap_read(regmap, AXP288_ADC_TS_PIN_CTRL, &adc_ts_pin_ctrl); if (ret) return ret; - /* After switching to the GPADC pin give things some time to settle */ - usleep_range(6000, 10000); + if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { + ret = regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, + AXP288_ADC_TS_CURRENT_ON_ONDEMAND); + if (ret) + return ret; + + /* Wait a bit after switching the current-source */ + usleep_range(6000, 10000); + } ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2); if (ret == 0) ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f); - regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, AXP288_ADC_TS_PIN_ON); + if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { + regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, + AXP288_ADC_TS_CURRENT_ON); + } return ret; }
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. The switching from on to on-ondemand is not necessary when the TS current-source is off (this happens on devices which do not have a TS). Prior to this commit there were 2 issues with our handling of the TS current-source switching: 1) 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 acpi_lpat_raw_to_temp() returning -ENOENT, resulting in: 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. 2) At the end of intel_xpower_pmic_get_raw_temp() 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 on entry of the function. This commit fixes this by checking if the TS current-source is off when entering intel_xpower_pmic_get_raw_temp() and if so it is left as is. Fixes: 58eefe2f3f53 ("ACPI / PMIC: xpower: Do pinswitch ... reading GPADC") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/pmic/intel_pmic_xpower.c | 41 +++++++++++++++++++++------ 1 file changed, 33 insertions(+), 8 deletions(-)