On Tue, 4 Jul 2017 15:52:42 +0200
Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> 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.
Fair enough. I added a line saying the exact cause wasn't
fully understood so we were going back to the known working
case.
Applied to the fixes-togreg branch of iio.git.
thanks,
Jonathan
>
> 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;
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html