On Fri, 30 Dec 2016 16:46:03 +0000
Jonathan Cameron <jic23@xxxxxxxxxx> 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. What is the other register?
> > 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@xxxxxxxxxx>
> 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]
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
