[PATCH v2 1/9] mfd: axp20x: Mark AXP20X_VBUS_IPSOUT_MGMT as volatile

[PATCH v2 1/9] mfd: axp20x: Mark AXP20X_VBUS_IPSOUT_MGMT as volatile

[Samuel Holland]

On AXP288 and newer PMICs, bit 7 of AXP20X_VBUS_IPSOUT_MGMT can be set
to prevent using the VBUS input. However, when the VBUS unplugged and
plugged back in, the bit automatically resets to zero.

We need to set the register as volatile to prevent regmap from caching
that bit. Otherwise, regcache will think the bit is already set and not
write the register.

Fixes: cd53216625a0 ("mfd: axp20x: Fix axp288 volatile ranges")
Cc: stable@vger.kernel.org
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/mfd/axp20x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index a4aaadaa0cb0..aa59496e4376 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -126,7 +126,7 @@ static const struct regmap_range axp288_writeable_ranges[] = {
 static const struct regmap_range axp288_volatile_ranges[] = {
 	regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP288_POWER_REASON),
 	regmap_reg_range(AXP288_BC_GLOBAL, AXP288_BC_GLOBAL),
-	regmap_reg_range(AXP288_BC_DET_STAT, AXP288_BC_DET_STAT),
+	regmap_reg_range(AXP288_BC_DET_STAT, AXP20X_VBUS_IPSOUT_MGMT),
 	regmap_reg_range(AXP20X_CHRG_BAK_CTRL, AXP20X_CHRG_BAK_CTRL),
 	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IPSOUT_V_HIGH_L),
 	regmap_reg_range(AXP20X_TIMER_CTRL, AXP20X_TIMER_CTRL),
-- 
2.23.0

Re: [PATCH v2 1/9] mfd: axp20x: Mark AXP20X_VBUS_IPSOUT_MGMT as volatile

[Chen-Yu Tsai]

On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On AXP288 and newer PMICs, bit 7 of AXP20X_VBUS_IPSOUT_MGMT can be set
> to prevent using the VBUS input. However, when the VBUS unplugged and
> plugged back in, the bit automatically resets to zero.
>
> We need to set the register as volatile to prevent regmap from caching
> that bit. Otherwise, regcache will think the bit is already set and not
> write the register.
>
> Fixes: cd53216625a0 ("mfd: axp20x: Fix axp288 volatile ranges")
> Cc: stable@vger.kernel.org
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Re: [PATCH v2 1/9] mfd: axp20x: Mark AXP20X_VBUS_IPSOUT_MGMT as volatile

[Lee Jones]

On Sat, 04 Jan 2020, Samuel Holland wrote:

> On AXP288 and newer PMICs, bit 7 of AXP20X_VBUS_IPSOUT_MGMT can be set
> to prevent using the VBUS input. However, when the VBUS unplugged and
> plugged back in, the bit automatically resets to zero.
> 
> We need to set the register as volatile to prevent regmap from caching
> that bit. Otherwise, regcache will think the bit is already set and not
> write the register.
> 
> Fixes: cd53216625a0 ("mfd: axp20x: Fix axp288 volatile ranges")
> Cc: stable@vger.kernel.org
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/mfd/axp20x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 2/9] power: supply: axp20x_ac_power: Fix reporting online status

[Samuel Holland]

AXP803/AXP813 have a flag that enables/disables the AC power supply
input. This flag does not affect the status bits in PWR_INPUT_STATUS.
Its effect can be verified by checking the battery charge/discharge
state (bit 2 of PWR_INPUT_STATUS), or by examining the current draw on
the AC input.

Take this flag into account when getting the ONLINE property of the AC
input, on PMICs where this flag is present.

Fixes: 7693b5643fd2 ("power: supply: add AC power supply driver for AXP813")
Cc: stable@vger.kernel.org
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/power/supply/axp20x_ac_power.c | 31 +++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c
index 0d34a932b6d5..ca0a28f72a27 100644
--- a/drivers/power/supply/axp20x_ac_power.c
+++ b/drivers/power/supply/axp20x_ac_power.c
@@ -23,6 +23,8 @@
 #define AXP20X_PWR_STATUS_ACIN_PRESENT	BIT(7)
 #define AXP20X_PWR_STATUS_ACIN_AVAIL	BIT(6)
 
+#define AXP813_ACIN_PATH_SEL		BIT(7)
+
 #define AXP813_VHOLD_MASK		GENMASK(5, 3)
 #define AXP813_VHOLD_UV_TO_BIT(x)	((((x) / 100000) - 40) << 3)
 #define AXP813_VHOLD_REG_TO_UV(x)	\
@@ -40,6 +42,7 @@ struct axp20x_ac_power {
 	struct power_supply *supply;
 	struct iio_channel *acin_v;
 	struct iio_channel *acin_i;
+	bool has_acin_path_sel;
 };
 
 static irqreturn_t axp20x_ac_power_irq(int irq, void *devid)
@@ -86,6 +89,17 @@ static int axp20x_ac_power_get_property(struct power_supply *psy,
 			return ret;
 
 		val->intval = !!(reg & AXP20X_PWR_STATUS_ACIN_AVAIL);
+
+		/* ACIN_PATH_SEL disables ACIN even if ACIN_AVAIL is set. */
+		if (power->has_acin_path_sel) {
+			ret = regmap_read(power->regmap, AXP813_ACIN_PATH_CTRL,
+					  &reg);
+			if (ret)
+				return ret;
+
+			val->intval &= !!(reg & AXP813_ACIN_PATH_SEL);
+		}
+
 		return 0;
 
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
@@ -224,21 +238,25 @@ static const struct power_supply_desc axp813_ac_power_desc = {
 struct axp_data {
 	const struct power_supply_desc	*power_desc;
 	bool				acin_adc;
+	bool				acin_path_sel;
 };
 
 static const struct axp_data axp20x_data = {
-	.power_desc = &axp20x_ac_power_desc,
-	.acin_adc = true,
+	.power_desc	= &axp20x_ac_power_desc,
+	.acin_adc	= true,
+	.acin_path_sel	= false,
 };
 
 static const struct axp_data axp22x_data = {
-	.power_desc = &axp22x_ac_power_desc,
-	.acin_adc = false,
+	.power_desc	= &axp22x_ac_power_desc,
+	.acin_adc	= false,
+	.acin_path_sel	= false,
 };
 
 static const struct axp_data axp813_data = {
-	.power_desc = &axp813_ac_power_desc,
-	.acin_adc = false,
+	.power_desc	= &axp813_ac_power_desc,
+	.acin_adc	= false,
+	.acin_path_sel	= true,
 };
 
 static int axp20x_ac_power_probe(struct platform_device *pdev)
@@ -282,6 +300,7 @@ static int axp20x_ac_power_probe(struct platform_device *pdev)
 	}
 
 	power->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	power->has_acin_path_sel = axp_data->acin_path_sel;
 
 	platform_set_drvdata(pdev, power);
 
-- 
2.23.0

Re: [linux-sunxi] [PATCH v2 2/9] power: supply: axp20x_ac_power: Fix reporting online status

[Chen-Yu Tsai]

On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
>
> AXP803/AXP813 have a flag that enables/disables the AC power supply
> input. This flag does not affect the status bits in PWR_INPUT_STATUS.
> Its effect can be verified by checking the battery charge/discharge
> state (bit 2 of PWR_INPUT_STATUS), or by examining the current draw on
> the AC input.
>
> Take this flag into account when getting the ONLINE property of the AC
> input, on PMICs where this flag is present.
>
> Fixes: 7693b5643fd2 ("power: supply: add AC power supply driver for AXP813")
> Cc: stable@vger.kernel.org
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Re: [linux-sunxi] [PATCH v2 2/9] power: supply: axp20x_ac_power: Fix reporting online status

[Julian Calaby]

Hi Samuel,

On Sun, Jan 5, 2020 at 12:24 PM Samuel Holland <samuel@sholland.org> wrote:
>
> AXP803/AXP813 have a flag that enables/disables the AC power supply
> input. This flag does not affect the status bits in PWR_INPUT_STATUS.
> Its effect can be verified by checking the battery charge/discharge
> state (bit 2 of PWR_INPUT_STATUS), or by examining the current draw on
> the AC input.
>
> Take this flag into account when getting the ONLINE property of the AC
> input, on PMICs where this flag is present.
>
> Fixes: 7693b5643fd2 ("power: supply: add AC power supply driver for AXP813")
> Cc: stable@vger.kernel.org
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/power/supply/axp20x_ac_power.c | 31 +++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c
> index 0d34a932b6d5..ca0a28f72a27 100644
> --- a/drivers/power/supply/axp20x_ac_power.c
> +++ b/drivers/power/supply/axp20x_ac_power.c
> @@ -23,6 +23,8 @@
>  #define AXP20X_PWR_STATUS_ACIN_PRESENT BIT(7)
>  #define AXP20X_PWR_STATUS_ACIN_AVAIL   BIT(6)
>
> +#define AXP813_ACIN_PATH_SEL           BIT(7)
> +
>  #define AXP813_VHOLD_MASK              GENMASK(5, 3)
>  #define AXP813_VHOLD_UV_TO_BIT(x)      ((((x) / 100000) - 40) << 3)
>  #define AXP813_VHOLD_REG_TO_UV(x)      \
> @@ -40,6 +42,7 @@ struct axp20x_ac_power {
>         struct power_supply *supply;
>         struct iio_channel *acin_v;
>         struct iio_channel *acin_i;
> +       bool has_acin_path_sel;
>  };
>
>  static irqreturn_t axp20x_ac_power_irq(int irq, void *devid)
> @@ -86,6 +89,17 @@ static int axp20x_ac_power_get_property(struct power_supply *psy,
>                         return ret;
>
>                 val->intval = !!(reg & AXP20X_PWR_STATUS_ACIN_AVAIL);
> +
> +               /* ACIN_PATH_SEL disables ACIN even if ACIN_AVAIL is set. */
> +               if (power->has_acin_path_sel) {

Do we need to check this bit if ACIN_AVAIL is not set?

> +                       ret = regmap_read(power->regmap, AXP813_ACIN_PATH_CTRL,
> +                                         &reg);
> +                       if (ret)
> +                               return ret;
> +
> +                       val->intval &= !!(reg & AXP813_ACIN_PATH_SEL);

If we only check this bit if ACIN_AVAIL is set, then we don't need the
"&" in the "&=". (I'm assuming that val->intval is an int, not a bool,
otherwise this is the wrong operator)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH v2 2/9] power: supply: axp20x_ac_power: Fix reporting online status

[Samuel Holland]

Hi Julian,

On 1/5/20 7:00 AM, Julian Calaby wrote:
> On Sun, Jan 5, 2020 at 12:24 PM Samuel Holland <samuel@sholland.org> wrote:
>>
>> AXP803/AXP813 have a flag that enables/disables the AC power supply
>> input. This flag does not affect the status bits in PWR_INPUT_STATUS.
>> Its effect can be verified by checking the battery charge/discharge
>> state (bit 2 of PWR_INPUT_STATUS), or by examining the current draw on
>> the AC input.
>>
>> Take this flag into account when getting the ONLINE property of the AC
>> input, on PMICs where this flag is present.
>>
>> Fixes: 7693b5643fd2 ("power: supply: add AC power supply driver for AXP813")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  drivers/power/supply/axp20x_ac_power.c | 31 +++++++++++++++++++++-----
>>  1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c
>> index 0d34a932b6d5..ca0a28f72a27 100644
>> --- a/drivers/power/supply/axp20x_ac_power.c
>> +++ b/drivers/power/supply/axp20x_ac_power.c
>> @@ -23,6 +23,8 @@
>>  #define AXP20X_PWR_STATUS_ACIN_PRESENT BIT(7)
>>  #define AXP20X_PWR_STATUS_ACIN_AVAIL   BIT(6)
>>
>> +#define AXP813_ACIN_PATH_SEL           BIT(7)
>> +
>>  #define AXP813_VHOLD_MASK              GENMASK(5, 3)
>>  #define AXP813_VHOLD_UV_TO_BIT(x)      ((((x) / 100000) - 40) << 3)
>>  #define AXP813_VHOLD_REG_TO_UV(x)      \
>> @@ -40,6 +42,7 @@ struct axp20x_ac_power {
>>         struct power_supply *supply;
>>         struct iio_channel *acin_v;
>>         struct iio_channel *acin_i;
>> +       bool has_acin_path_sel;
>>  };
>>
>>  static irqreturn_t axp20x_ac_power_irq(int irq, void *devid)
>> @@ -86,6 +89,17 @@ static int axp20x_ac_power_get_property(struct power_supply *psy,
>>                         return ret;
>>
>>                 val->intval = !!(reg & AXP20X_PWR_STATUS_ACIN_AVAIL);
>> +
>> +               /* ACIN_PATH_SEL disables ACIN even if ACIN_AVAIL is set. */
>> +               if (power->has_acin_path_sel) {
> 
> Do we need to check this bit if ACIN_AVAIL is not set?

No, we don't. However due to regcache this won't actually cause another read
from the device. If I send a v3, I'll move the && to  the if statement.

>> +                       ret = regmap_read(power->regmap, AXP813_ACIN_PATH_CTRL,
>> +                                         &reg);
>> +                       if (ret)
>> +                               return ret;
>> +
>> +                       val->intval &= !!(reg & AXP813_ACIN_PATH_SEL);
> 
> If we only check this bit if ACIN_AVAIL is set, then we don't need the
> "&" in the "&=". (I'm assuming that val->intval is an int, not a bool,
> otherwise this is the wrong operator)

val->intval is an int, but it only ever takes the values 0 or 1. The !!
expression coerces an integer to the range of a boolean. So the two ways of
deriving the value ("&=" here vs "&& val->intval" in the if statement) are
equivalent.

> Thanks,

Thanks!
Samuel