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

[Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Sun, 1 Jan 2017 12:19:40 +0100
Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Hi,
> 
> On 30-12-16 19:15, Jacob Pan wrote:
> > On Fri, 30 Dec 2016 16:46:03 +0000
> > Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >  
> >> 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.
> >>>  
> > It has been a while. Just looked at the datasheet, reg 84h is for
> > ADC and TS pin control. IIRC, we had to set the TS bits in order to
> > make ADC read to work.  
> 
> The bits the code I'm removing are manipulating are bits 0 & 1 of
> reg 84h which are:
> 
> 1-0 Current source from TS pin on/off enable bit [1:0]
> 
> 00: off
> 01: on when charging battery, off when not charging
> 10: on in ADC phase and off when out of the ADC phase, for power
> saving 11: always on
> 
> And they are being toggled between 10 and 11, so in both
> cases the current source is enabled when reading the adc
> value. Specifically the code this patch removes are setting
> these bits to 10 before reading and back to 11 after reading,
> which makes no sense.
> 
> And to make things even funkier, this manipulation is only
> done when reading the GP_ADC, which is the ADC function of
> GPIO0, whereas these bits control the bias current source
> for the TS pin, which is a completely different pin.
> 
> So all in all this entire bit of code seems to be big NOP.
> 
It could have been a quirk we had to do on our platforms, I just
cannot recall the details. Are you testing this on x86 platforms?

> > What is the other register?  
> 
> The register to actually enable / disable the bias current
> source for GPIO0 / the GPADC pin is register 85h, where setting
> bit 2 enables the GPIO0 output current.
> 
> Regards,
> 
> Hans
> 
> 
> 
> >>> 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. 
> > Agreed, it would be better to do read-modify-write and not to
> > touch the other bits.  
> >>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>  
> >> 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;
> >>>  
> >>  
> >
> > [Jacob Pan]
> >  

[Jacob Pan]