* [PATCH v2] iio: adc: axp288: Fix TS-pin handling
@ 2019-01-05 18:36 Hans de Goede
2019-01-12 17:16 ` Jonathan Cameron
0 siblings, 1 reply; 2+ messages in thread
From: Hans de Goede @ 2019-01-05 18:36 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hans de Goede, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Chen-Yu Tsai, linux-iio
Prior to this commit there were 3 issues with our handling of the TS-pin:
1) There are 2 ways how the firmware can disable monitoring of the TS-pin
for designs which do not have a temperature-sensor for the battery:
a) Clearing bit 0 of the AXP20X_ADC_EN1 register
b) Setting bit 2 of the AXP288_ADC_TS_PIN_CTRL monitoring
Prior to this commit we were unconditionally setting both bits to the
value used on devices with a TS. This causes the temperature protection to
kick in on devices without a TS, such as the Jumper ezbook v2, causing
them to not charge under Linux.
This commit fixes this by using regmap_update_bits when updating these 2
registers, leaving the 2 mentioned bits alone.
The next 2 problems are related to our handling of the current-source
for the TS-pin. The current-source used for the battery temp-sensor (TS)
is shared with the GPADC. For proper fuel-gauge and charger operation the
TS current-source needs to be permanently on. But to read the GPADC we
need to temporary switch the TS current-source to ondemand, so that the
GPADC can use it, otherwise we will always read an all 0 value.
2) Problem 2 is we were writing hardcoded values to the ADC TS pin-ctrl
register, overwriting various other unrelated bits. Specifically we were
overwriting the current-source setting for the TS and GPIO0 pins, forcing
it to 80ųA independent of its original setting. On a Chuwi Vi10 tablet
this was causing us to get a too high adc value (due to a too high
current-source) resulting in the following errors being logged:
ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion]
ACPI Error: Method parse/execution failed \_SB.SXP1._TMP, AE_ERROR
This commit fixes this by using regmap_update_bits to change only the
relevant bits.
3) After reading the GPADC channel we were unconditionally enabling the
TS current-source even on devices where the TS-pin is not used and the
current-source thus was off before axp288_adc_read_raw call.
This commit fixes this by making axp288_adc_set_ts a nop on devices where
the ADC is not enabled for the TS-pin.
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1610545
Fixes: 3091141d7803 ("iio: adc: axp288: Fix the GPADC pin ...")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Make sure we enable the TS current-source on probe when the TS pin is enabled
---
drivers/iio/adc/axp288_adc.c | 76 ++++++++++++++++++++++++++++--------
1 file changed, 60 insertions(+), 16 deletions(-)
diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
index 031d568b4972..4e339cfd0c54 100644
--- a/drivers/iio/adc/axp288_adc.c
+++ b/drivers/iio/adc/axp288_adc.c
@@ -27,9 +27,18 @@
#include <linux/iio/machine.h>
#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
+/*
+ * This mask enables all ADCs except for the battery temp-sensor (TS), that is
+ * left as-is to avoid breaking charging on devices without a temp-sensor.
+ */
+#define AXP288_ADC_EN_MASK 0xF0
+#define AXP288_ADC_TS_ENABLE 0x01
+
+#define AXP288_ADC_TS_CURRENT_ON_OFF_MASK GENMASK(1, 0)
+#define AXP288_ADC_TS_CURRENT_OFF (0 << 0)
+#define AXP288_ADC_TS_CURRENT_ON_WHEN_CHARGING (1 << 0)
+#define AXP288_ADC_TS_CURRENT_ON_ONDEMAND (2 << 0)
+#define AXP288_ADC_TS_CURRENT_ON (3 << 0)
enum axp288_adc_id {
AXP288_ADC_TS,
@@ -44,6 +53,7 @@ enum axp288_adc_id {
struct axp288_adc_info {
int irq;
struct regmap *regmap;
+ bool ts_enabled;
};
static const struct iio_chan_spec axp288_adc_channels[] = {
@@ -115,21 +125,33 @@ 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)
+/*
+ * The current-source used for the battery temp-sensor (TS) is shared
+ * with the GPADC. For proper fuel-gauge and charger operation the TS
+ * current-source needs to be permanently on. But to read the GPADC we
+ * need to temporary switch the TS current-source to ondemand, so that
+ * the GPADC can use it, otherwise we will always read an all 0 value.
+ */
+static int axp288_adc_set_ts(struct axp288_adc_info *info,
+ unsigned int mode, unsigned long address)
{
int ret;
- /* channels other than GPADC do not need to switch TS pin */
+ /* No need to switch the current-source if the TS pin is disabled */
+ if (!info->ts_enabled)
+ return 0;
+
+ /* Channels other than GPADC do not need the current source */
if (address != AXP288_GP_ADC_H)
return 0;
- ret = regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, mode);
+ ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL,
+ AXP288_ADC_TS_CURRENT_ON_OFF_MASK, mode);
if (ret)
return ret;
/* When switching to the GPADC pin give things some time to settle */
- if (mode == AXP288_ADC_TS_PIN_GPADC)
+ if (mode == AXP288_ADC_TS_CURRENT_ON_ONDEMAND)
usleep_range(6000, 10000);
return 0;
@@ -145,14 +167,14 @@ 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,
+ if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON_ONDEMAND,
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,
+ if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON,
chan->address))
dev_err(&indio_dev->dev, "TS pin restore\n");
break;
@@ -164,13 +186,35 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
return ret;
}
-static int axp288_adc_set_state(struct regmap *regmap)
+static int axp288_adc_initialize(struct axp288_adc_info *info)
{
- /* 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;
+ int ret, adc_enable_val;
+
+ /*
+ * Determine if the TS pin is enabled and set the TS current-source
+ * accordingly.
+ */
+ ret = regmap_read(info->regmap, AXP20X_ADC_EN1, &adc_enable_val);
+ if (ret)
+ return ret;
+
+ if (adc_enable_val & AXP288_ADC_TS_ENABLE) {
+ info->ts_enabled = true;
+ ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL,
+ AXP288_ADC_TS_CURRENT_ON_OFF_MASK,
+ AXP288_ADC_TS_CURRENT_ON);
+ } else {
+ info->ts_enabled = false;
+ ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL,
+ AXP288_ADC_TS_CURRENT_ON_OFF_MASK,
+ AXP288_ADC_TS_CURRENT_OFF);
+ }
+ if (ret)
+ return ret;
- return regmap_write(regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK);
+ /* Turn on the ADC for all channels except TS, leave TS as is */
+ return regmap_update_bits(info->regmap, AXP20X_ADC_EN1,
+ AXP288_ADC_EN_MASK, AXP288_ADC_EN_MASK);
}
static const struct iio_info axp288_adc_iio_info = {
@@ -200,7 +244,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 = axp288_adc_initialize(info);
if (ret) {
dev_err(&pdev->dev, "unable to enable ADC device\n");
return ret;
--
2.20.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] iio: adc: axp288: Fix TS-pin handling
2019-01-05 18:36 [PATCH v2] iio: adc: axp288: Fix TS-pin handling Hans de Goede
@ 2019-01-12 17:16 ` Jonathan Cameron
0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Cameron @ 2019-01-12 17:16 UTC (permalink / raw)
To: Hans de Goede
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Chen-Yu Tsai, linux-iio
On Sat, 5 Jan 2019 19:36:18 +0100
Hans de Goede <hdegoede@redhat.com> wrote:
> Prior to this commit there were 3 issues with our handling of the TS-pin:
>
> 1) There are 2 ways how the firmware can disable monitoring of the TS-pin
> for designs which do not have a temperature-sensor for the battery:
> a) Clearing bit 0 of the AXP20X_ADC_EN1 register
> b) Setting bit 2 of the AXP288_ADC_TS_PIN_CTRL monitoring
>
> Prior to this commit we were unconditionally setting both bits to the
> value used on devices with a TS. This causes the temperature protection to
> kick in on devices without a TS, such as the Jumper ezbook v2, causing
> them to not charge under Linux.
>
> This commit fixes this by using regmap_update_bits when updating these 2
> registers, leaving the 2 mentioned bits alone.
>
> The next 2 problems are related to our handling of the current-source
> for the TS-pin. The current-source used for the battery temp-sensor (TS)
> is shared with the GPADC. For proper fuel-gauge and charger operation the
> TS current-source needs to be permanently on. But to read the GPADC we
> need to temporary switch the TS current-source to ondemand, so that the
> GPADC can use it, otherwise we will always read an all 0 value.
>
> 2) Problem 2 is we were writing hardcoded values to the ADC TS pin-ctrl
> register, overwriting various other unrelated bits. Specifically we were
> overwriting the current-source setting for the TS and GPIO0 pins, forcing
> it to 80ųA independent of its original setting. On a Chuwi Vi10 tablet
> this was causing us to get a too high adc value (due to a too high
> current-source) resulting in the following errors being logged:
>
> ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion]
> ACPI Error: Method parse/execution failed \_SB.SXP1._TMP, AE_ERROR
>
> This commit fixes this by using regmap_update_bits to change only the
> relevant bits.
>
> 3) After reading the GPADC channel we were unconditionally enabling the
> TS current-source even on devices where the TS-pin is not used and the
> current-source thus was off before axp288_adc_read_raw call.
>
> This commit fixes this by making axp288_adc_set_ts a nop on devices where
> the ADC is not enabled for the TS-pin.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1610545
> Fixes: 3091141d7803 ("iio: adc: axp288: Fix the GPADC pin ...")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.
Thanks,
Jonathan
> ---
> Changes in v2:
> -Make sure we enable the TS current-source on probe when the TS pin is enabled
> ---
> drivers/iio/adc/axp288_adc.c | 76 ++++++++++++++++++++++++++++--------
> 1 file changed, 60 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
> index 031d568b4972..4e339cfd0c54 100644
> --- a/drivers/iio/adc/axp288_adc.c
> +++ b/drivers/iio/adc/axp288_adc.c
> @@ -27,9 +27,18 @@
> #include <linux/iio/machine.h>
> #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
> +/*
> + * This mask enables all ADCs except for the battery temp-sensor (TS), that is
> + * left as-is to avoid breaking charging on devices without a temp-sensor.
> + */
> +#define AXP288_ADC_EN_MASK 0xF0
> +#define AXP288_ADC_TS_ENABLE 0x01
> +
> +#define AXP288_ADC_TS_CURRENT_ON_OFF_MASK GENMASK(1, 0)
> +#define AXP288_ADC_TS_CURRENT_OFF (0 << 0)
> +#define AXP288_ADC_TS_CURRENT_ON_WHEN_CHARGING (1 << 0)
> +#define AXP288_ADC_TS_CURRENT_ON_ONDEMAND (2 << 0)
> +#define AXP288_ADC_TS_CURRENT_ON (3 << 0)
>
> enum axp288_adc_id {
> AXP288_ADC_TS,
> @@ -44,6 +53,7 @@ enum axp288_adc_id {
> struct axp288_adc_info {
> int irq;
> struct regmap *regmap;
> + bool ts_enabled;
> };
>
> static const struct iio_chan_spec axp288_adc_channels[] = {
> @@ -115,21 +125,33 @@ 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)
> +/*
> + * The current-source used for the battery temp-sensor (TS) is shared
> + * with the GPADC. For proper fuel-gauge and charger operation the TS
> + * current-source needs to be permanently on. But to read the GPADC we
> + * need to temporary switch the TS current-source to ondemand, so that
> + * the GPADC can use it, otherwise we will always read an all 0 value.
> + */
> +static int axp288_adc_set_ts(struct axp288_adc_info *info,
> + unsigned int mode, unsigned long address)
> {
> int ret;
>
> - /* channels other than GPADC do not need to switch TS pin */
> + /* No need to switch the current-source if the TS pin is disabled */
> + if (!info->ts_enabled)
> + return 0;
> +
> + /* Channels other than GPADC do not need the current source */
> if (address != AXP288_GP_ADC_H)
> return 0;
>
> - ret = regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, mode);
> + ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL,
> + AXP288_ADC_TS_CURRENT_ON_OFF_MASK, mode);
> if (ret)
> return ret;
>
> /* When switching to the GPADC pin give things some time to settle */
> - if (mode == AXP288_ADC_TS_PIN_GPADC)
> + if (mode == AXP288_ADC_TS_CURRENT_ON_ONDEMAND)
> usleep_range(6000, 10000);
>
> return 0;
> @@ -145,14 +167,14 @@ 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,
> + if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON_ONDEMAND,
> 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,
> + if (axp288_adc_set_ts(info, AXP288_ADC_TS_CURRENT_ON,
> chan->address))
> dev_err(&indio_dev->dev, "TS pin restore\n");
> break;
> @@ -164,13 +186,35 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> return ret;
> }
>
> -static int axp288_adc_set_state(struct regmap *regmap)
> +static int axp288_adc_initialize(struct axp288_adc_info *info)
> {
> - /* 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;
> + int ret, adc_enable_val;
> +
> + /*
> + * Determine if the TS pin is enabled and set the TS current-source
> + * accordingly.
> + */
> + ret = regmap_read(info->regmap, AXP20X_ADC_EN1, &adc_enable_val);
> + if (ret)
> + return ret;
> +
> + if (adc_enable_val & AXP288_ADC_TS_ENABLE) {
> + info->ts_enabled = true;
> + ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL,
> + AXP288_ADC_TS_CURRENT_ON_OFF_MASK,
> + AXP288_ADC_TS_CURRENT_ON);
> + } else {
> + info->ts_enabled = false;
> + ret = regmap_update_bits(info->regmap, AXP288_ADC_TS_PIN_CTRL,
> + AXP288_ADC_TS_CURRENT_ON_OFF_MASK,
> + AXP288_ADC_TS_CURRENT_OFF);
> + }
> + if (ret)
> + return ret;
>
> - return regmap_write(regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK);
> + /* Turn on the ADC for all channels except TS, leave TS as is */
> + return regmap_update_bits(info->regmap, AXP20X_ADC_EN1,
> + AXP288_ADC_EN_MASK, AXP288_ADC_EN_MASK);
> }
>
> static const struct iio_info axp288_adc_iio_info = {
> @@ -200,7 +244,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 = axp288_adc_initialize(info);
> if (ret) {
> dev_err(&pdev->dev, "unable to enable ADC device\n");
> return ret;
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-01-12 17:16 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-05 18:36 [PATCH v2] iio: adc: axp288: Fix TS-pin handling Hans de Goede
2019-01-12 17:16 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).