Re: [PATCH] iio: adc: axp288: Fix TS-pin handling

[Jonathan Cameron <jic23@kernel.org>]

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;