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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 01-07-17 11:46, Jonathan Cameron wrote:
> On Fri, 30 Jun 2017 19:42:54 +0200
> Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> > Inheriting the ADC BIAS current settings from the BIOS instead of
> > hardcoding then causes the AXP288 to disable charging (I think it
> > mis-detects an overheated battery) on at least one model tablet.
> >
> > So lets go back to hard coding the values, this reverts
> > commit fa2849e9649b ("iio: adc: axp288: Drop bogus
> > AXP288_ADC_TS_PIN_CTRL register modifications"), fixing charging not
> > working on the model tablet in question.
> Given the original patch description, I'm a little worried we are
> changing too much by doing a straight forward revert.

Perhaps.

> Should we be addressing just the relevant bit related to the
> thermal sensor instead?

According to the bugreport I received it is this write:

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

Which fixes the regression, so looking at the register documentation
we actually need to change the bias currents as well as set the
bits which control whether the bias current us always on or only on
when sampling to bias always on.

> I suppose the revert is the low risk option, but I would really
> like a slightly better understanding of what is going on here if
> possible.

The problem is that we really don't know what is going on
exactly, all we can be sure of is that the original code was there
for a reason after all. The datasheet for the axp288 unfortunaly
does not offer a clue. Possibly it is just outright wrong.

As such I would prefer to just go for the revert.

Regards,

Hans


> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: Umberto Ixxo <sfumato1977@xxxxxxxxx>
> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > ---
> >   drivers/iio/adc/axp288_adc.c | 32 +++++++++++++++++++++++++++++++-
> >   1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
> > index 64799ad7ebad..7fd24949c0c1 100644
> > --- a/drivers/iio/adc/axp288_adc.c
> > +++ b/drivers/iio/adc/axp288_adc.c
> > @@ -28,6 +28,8 @@
> >   #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,
> > @@ -121,6 +123,16 @@ 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)
> > @@ -131,7 +143,16 @@ 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;
> > @@ -141,6 +162,15 @@ 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,
> > @@ -169,7 +199,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 = regmap_write(info->regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK);
> > +	ret = axp288_adc_set_state(axp20x->regmap);
> >   	if (ret) {
> >   		dev_err(&pdev->dev, "unable to enable ADC device\n");
> >   		return ret;