Re: [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications

[Jonathan Cameron <jic23@kernel.org>]

On 14/12/16 13:55, Hans de Goede wrote:
> For some reason the axp288_adc driver was modifying the
> AXP288_ADC_TS_PIN_CTRL register, changing bits 0-1 depending on
> whether the GP_ADC channel or another channel was written.
> 
> These bits control when a bias current is send to the TS_PIN, the
> GP_ADC has its own pin and a separate bit in another register to
> control the bias current.
> 
> Not only does changing when to enable the TS_PIN bias current
> (always or only when sampling) when reading the GP_ADC make no sense
> at all, the code is modifying these bits is writing the entire register,
> assuming that all the other bits have their default value.
> 
> So if the firmware has configured a different bias-current for either
> pin, then that change gets clobbered by the write, likewise if the
> firmware has set bit 2 to indicate that the battery has no thermal sensor,
> this will get clobbered by the write.
> 
> This commit fixes all this, by simply removing all writes to the
> AXP288_ADC_TS_PIN_CTRL register, they are not needed to read the
> GP_ADC pin, and can actually be harmful.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
I guess you probably have more up to date contact details than I do,
but seems worth trying to cc Jacob Pan on this to see if we can find
out what the original reasoning behind this was.  Seems a very odd
thing to do with no purpose!

If Jacob isn't contactable we'll fall back to guessing it was just
an oddity of driver evolution.

Jonathan
> ---
>  drivers/iio/adc/axp288_adc.c | 32 +-------------------------------
>  1 file changed, 1 insertion(+), 31 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
> index 7fd2494..64799ad 100644
> --- a/drivers/iio/adc/axp288_adc.c
> +++ b/drivers/iio/adc/axp288_adc.c
> @@ -28,8 +28,6 @@
>  #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
>  
>  enum axp288_adc_id {
>  	AXP288_ADC_TS,
> @@ -123,16 +121,6 @@ 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)
> -{
> -	/* channels other than GPADC do not need to switch TS pin */
> -	if (address != AXP288_GP_ADC_H)
> -		return 0;
> -
> -	return regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, mode);
> -}
> -
>  static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>  			struct iio_chan_spec const *chan,
>  			int *val, int *val2, long mask)
> @@ -143,16 +131,7 @@ 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,
> -					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,
> -						chan->address))
> -			dev_err(&indio_dev->dev, "TS pin restore\n");
>  		break;
>  	default:
>  		ret = -EINVAL;
> @@ -162,15 +141,6 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -static int axp288_adc_set_state(struct regmap *regmap)
> -{
> -	/* 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;
> -
> -	return regmap_write(regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK);
> -}
> -
>  static const struct iio_info axp288_adc_iio_info = {
>  	.read_raw = &axp288_adc_read_raw,
>  	.driver_module = THIS_MODULE,
> @@ -199,7 +169,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 = regmap_write(info->regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK);
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to enable ADC device\n");
>  		return ret;
>