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@xxxxxxxxxx>
> ---
> 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!
