[PATCH] ACPI / PMIC: xpower: Do pinswitch magic when reading GPADC

[PATCH] ACPI / PMIC: xpower: Do pinswitch magic when reading GPADC

[Hans de Goede]

Testing has shown that the TS-pin's bias-current needs to be disabled
when reading the GPIO0 pin in GPADC mode.

It seems that there is only 1 bias current source and to be able to use it
for the GPIO0 pin in GPADC mode it must be temporarily turned off for the
TS pin, but the datasheet does not mention this.

This commit adds the necessary writes to turn the TS pin BIAS current
off before and back on after reading the GPADC. This fixes the GPADC
always returning a reading of 0.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/pmic/intel_pmic_xpower.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index 3b7d5be5b7ed..6c99d3f81095 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -27,6 +27,9 @@
 #define GPI1_LDO_ON		(3 << 0)
 #define GPI1_LDO_OFF		(4 << 0)
 
+#define AXP288_ADC_TS_PIN_GPADC	0xf2
+#define AXP288_ADC_TS_PIN_ON	0xf3
+
 static struct pmic_table power_table[] = {
 	{
 		.address = 0x00,
@@ -209,11 +212,23 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
 static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
 {
 	u8 buf[2];
+	int ret;
 
-	if (regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2))
-		return -EIO;
+	ret = regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL,
+			   AXP288_ADC_TS_PIN_GPADC);
+	if (ret)
+		return ret;
+
+	/* After switching to the GPADC pin give things some time to settle */
+	usleep_range(6000, 10000);
+
+	ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2);
+	if (ret == 0)
+		ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f);
+
+	regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, AXP288_ADC_TS_PIN_ON);
 
-	return (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
+	return ret;
 }
 
 static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
-- 
2.13.0

Re: [PATCH] ACPI / PMIC: xpower: Do pinswitch magic when reading GPADC

[Andy Shevchenko]

On Sat, 2017-07-08 at 15:40 +0200, Hans de Goede wrote:
> Testing has shown that the TS-pin's bias-current needs to be disabled
> when reading the GPIO0 pin in GPADC mode.
> 
> It seems that there is only 1 bias current source and to be able to
> use it
> for the GPIO0 pin in GPADC mode it must be temporarily turned off for
> the
> TS pin, but the datasheet does not mention this.
> 
> This commit adds the necessary writes to turn the TS pin BIAS current
> off before and back on after reading the GPADC. This fixes the GPADC
> always returning a reading of 0.
> 
> 

> +	/* After switching to the GPADC pin give things some time to
> settle */
> +	usleep_range(6000, 10000);

msleep(6); ?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] ACPI / PMIC: xpower: Do pinswitch magic when reading GPADC

[Hans de Goede]

Hi,

On 08-07-17 15:58, Andy Shevchenko wrote:
> On Sat, 2017-07-08 at 15:40 +0200, Hans de Goede wrote:
>> Testing has shown that the TS-pin's bias-current needs to be disabled
>> when reading the GPIO0 pin in GPADC mode.
>>
>> It seems that there is only 1 bias current source and to be able to
>> use it
>> for the GPIO0 pin in GPADC mode it must be temporarily turned off for
>> the
>> TS pin, but the datasheet does not mention this.
>>
>> This commit adds the necessary writes to turn the TS pin BIAS current
>> off before and back on after reading the GPADC. This fixes the GPADC
>> always returning a reading of 0.
>>
>>
> 
>> +	/* After switching to the GPADC pin give things some time to
>> settle */
>> +	usleep_range(6000, 10000);
> 
> msleep(6); ?

That will make checkpatch unhappy and may sleep for as long as 20ms, also see:
Documentation/timers/timers-howto.txt which advices against using msleep for
short sleeps.

Regards,

Hans

Re: [PATCH] ACPI / PMIC: xpower: Do pinswitch magic when reading GPADC

[Rafael J. Wysocki]

On Saturday, July 08, 2017 04:58:03 PM Andy Shevchenko wrote:
> On Sat, 2017-07-08 at 15:40 +0200, Hans de Goede wrote:
> > Testing has shown that the TS-pin's bias-current needs to be disabled
> > when reading the GPIO0 pin in GPADC mode.
> > 
> > It seems that there is only 1 bias current source and to be able to
> > use it
> > for the GPIO0 pin in GPADC mode it must be temporarily turned off for
> > the
> > TS pin, but the datasheet does not mention this.
> > 
> > This commit adds the necessary writes to turn the TS pin BIAS current
> > off before and back on after reading the GPADC. This fixes the GPADC
> > always returning a reading of 0.
> > 
> > 
> 
> > +	/* After switching to the GPADC pin give things some time to
> > settle */
> > +	usleep_range(6000, 10000);
> 
> msleep(6); ?
> 
> 

Is this the only issue you have with the $subject patch?

Thanks,
Rafael

Re: [PATCH] ACPI / PMIC: xpower: Do pinswitch magic when reading GPADC

[Andy Shevchenko]

On Fri, 2017-07-21 at 23:12 +0200, Rafael J. Wysocki wrote:
> On Saturday, July 08, 2017 04:58:03 PM Andy Shevchenko wrote:
> > On Sat, 2017-07-08 at 15:40 +0200, Hans de Goede wrote:
> > > Testing has shown that the TS-pin's bias-current needs to be
> > > disabled
> > > when reading the GPIO0 pin in GPADC mode.
> > > 
> > > It seems that there is only 1 bias current source and to be able
> > > to
> > > use it
> > > for the GPIO0 pin in GPADC mode it must be temporarily turned off
> > > for
> > > the
> > > TS pin, but the datasheet does not mention this.
> > > 
> > > This commit adds the necessary writes to turn the TS pin BIAS
> > > current
> > > off before and back on after reading the GPADC. This fixes the
> > > GPADC
> > > always returning a reading of 0.
> > > 
> > > 
> > > +	/* After switching to the GPADC pin give things some time
> > > to
> > > settle */
> > > +	usleep_range(6000, 10000);
> > 
> > msleep(6); ?
> > 
> Is this the only issue you have with the $subject patch?

As Hans explained to me there is no issue.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] ACPI / PMIC: xpower: Do pinswitch magic when reading GPADC

[Rafael J. Wysocki]

On Sunday, July 23, 2017 03:28:01 PM Andy Shevchenko wrote:
> On Fri, 2017-07-21 at 23:12 +0200, Rafael J. Wysocki wrote:
> > On Saturday, July 08, 2017 04:58:03 PM Andy Shevchenko wrote:
> > > On Sat, 2017-07-08 at 15:40 +0200, Hans de Goede wrote:
> > > > Testing has shown that the TS-pin's bias-current needs to be
> > > > disabled
> > > > when reading the GPIO0 pin in GPADC mode.
> > > > 
> > > > It seems that there is only 1 bias current source and to be able
> > > > to
> > > > use it
> > > > for the GPIO0 pin in GPADC mode it must be temporarily turned off
> > > > for
> > > > the
> > > > TS pin, but the datasheet does not mention this.
> > > > 
> > > > This commit adds the necessary writes to turn the TS pin BIAS
> > > > current
> > > > off before and back on after reading the GPADC. This fixes the
> > > > GPADC
> > > > always returning a reading of 0.
> > > > 
> > > > 
> > > > +	/* After switching to the GPADC pin give things some time
> > > > to
> > > > settle */
> > > > +	usleep_range(6000, 10000);
> > > 
> > > msleep(6); ?
> > > 
> > Is this the only issue you have with the $subject patch?
> 
> As Hans explained to me there is no issue.

OK, so patch applied.  Thanks!