Patchwork ACPI / PMIC: xpower: Fix TS-pin current-source handling

login
register
mail settings
Submitter Hans de Goede
Date Jan. 4, 2019, 10:10 p.m.
Message ID <20190104221054.26200-1-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/693233/
State New
Headers show

Comments

Hans de Goede - Jan. 4, 2019, 10:10 p.m.
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(-)
Andy Shevchenko - Jan. 6, 2019, 5:28 p.m.
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
>
Rafael J. Wysocki - Jan. 7, 2019, 10:59 a.m.
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?
Hans de Goede - Jan. 7, 2019, 11:04 a.m.
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
Rafael J. Wysocki - Jan. 11, 2019, 10:29 a.m.
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!

Patch

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;
 }