[PATCH v2 0/9] X-Powers Power Supply Improvements

[PATCH v2 0/9] X-Powers Power Supply Improvements

[Samuel Holland]

This series adds some improvements to the axp20x* power supply drivers
to better support suspend/resume and use on mobile devices.

The first two patches fix bugs I found while testing the ONLINE control
added in later patches.

Patches 3 and 7 allow userspace to take the power supplies offline.
Patches 4 and 8 allow userspace to control the wakeup behavior.

Patch 9 avoids polling USB VBUS presence when possible. While working on
the RSB driver, I was seeing ~50 transfers per second, while idle and
tracked it down to this VBUS polling (20 reads/second). The polling
often caused the CPU to clock up and back down, which triggered the
remaining transfers (changes to the CPU voltage).

Unfortunately, I don't see a way to avoid the polling when running on
battery (where it matters most), other than to move the polling back to
the USB PHY driver.

Changes since v1:
 - Add patches 1-2
 - Shift value properly in calls to regmap_update_bits (3, 7)
 - Use #ifdef instead of #if to avoid -Wundef warnings (4, 8)
 - Poll once after an IRQ, instead of setting power->online in the IRQ (9)
 - Poll once on resume, in case the state changed during suspend (9)

Samuel Holland (9):
  mfd: axp20x: Mark AXP20X_VBUS_IPSOUT_MGMT as volatile
  power: supply: axp20x_ac_power: Fix reporting online status
  power: supply: axp20x_ac_power: Allow offlining
  power: supply: axp20x_ac_power: Add wakeup control
  power: supply: axp20x_usb_power: Remove unused device_node
  power: supply: axp20x_usb_power: Use a match structure
  power: supply: axp20x_usb_power: Allow offlining
  power: supply: axp20x_usb_power: Add wakeup control
  power: supply: axp20x_usb_power: Only poll while offline

 drivers/mfd/axp20x.c                    |   2 +-
 drivers/power/supply/axp20x_ac_power.c  | 131 +++++++++++---
 drivers/power/supply/axp20x_usb_power.c | 220 ++++++++++++++++++------
 3 files changed, 276 insertions(+), 77 deletions(-)

-- 
2.23.0

[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

[PATCH v2 3/9] power: supply: axp20x_ac_power: Allow offlining

[Samuel Holland]

AXP803/AXP813 have a flag that enables/disables the AC power supply
input. Allow control of this flag via the ONLINE property on those
variants.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/power/supply/axp20x_ac_power.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c
index ca0a28f72a27..bc2663cd47fa 100644
--- a/drivers/power/supply/axp20x_ac_power.c
+++ b/drivers/power/supply/axp20x_ac_power.c
@@ -24,6 +24,7 @@
 #define AXP20X_PWR_STATUS_ACIN_AVAIL	BIT(6)
 
 #define AXP813_ACIN_PATH_SEL		BIT(7)
+#define AXP813_ACIN_PATH_SEL_TO_BIT(x)	(!!(x) << 7)
 
 #define AXP813_VHOLD_MASK		GENMASK(5, 3)
 #define AXP813_VHOLD_UV_TO_BIT(x)	((((x) / 100000) - 40) << 3)
@@ -157,6 +158,11 @@ static int axp813_ac_power_set_property(struct power_supply *psy,
 	struct axp20x_ac_power *power = power_supply_get_drvdata(psy);
 
 	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+		return regmap_update_bits(power->regmap, AXP813_ACIN_PATH_CTRL,
+					  AXP813_ACIN_PATH_SEL,
+					  AXP813_ACIN_PATH_SEL_TO_BIT(val->intval));
+
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
 		if (val->intval < 4000000 || val->intval > 4700000)
 			return -EINVAL;
@@ -183,7 +189,8 @@ static int axp813_ac_power_set_property(struct power_supply *psy,
 static int axp813_ac_power_prop_writeable(struct power_supply *psy,
 					  enum power_supply_property psp)
 {
-	return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
+	return psp == POWER_SUPPLY_PROP_ONLINE ||
+	       psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
 	       psp == POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT;
 }
 
-- 
2.23.0

Re: [linux-sunxi] [PATCH v2 3/9] power: supply: axp20x_ac_power: Allow offlining

[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. Allow control of this flag via the ONLINE property on those
> variants.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

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

[PATCH v2 4/9] power: supply: axp20x_ac_power: Add wakeup control

[Samuel Holland]

The AC power supply input can be used as a wakeup source. Hook up the
ACIN_PLUGIN IRQ to trigger wakeup based on userspace configuration.

To do this, we must remember the list of IRQs for the life of the
device. To know how much space to allocate for the flexible array
member, we switch from using a NULL sentinel to using an array length.

Because we now depend on the specific order of the IRQs (we assume
ACIN_PLUGIN is first and always present), failing to acquire an IRQ
during probe must be a fatal error.

To avoid spuriously waking up the system when the AC power supply is
not configured as a wakeup source, we must explicitly disable all non-
wake IRQs during system suspend. This is because the SoC's NMI input is
shared among all IRQs on the AXP PMIC. Due to the use of regmap-irq, the
individual IRQs within the PMIC are nested threaded interrupts, and are
therefore not automatically disabled during system suspend.

The upshot is that if any other device within the MFD (such as the power
key) is an enabled wakeup source, all enabled IRQs within the PMIC will
cause wakeup. We still need to call enable_irq_wake() when we *do* want
wakeup, in case those other wakeup sources on the PMIC are all disabled.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/power/supply/axp20x_ac_power.c | 91 +++++++++++++++++++++-----
 1 file changed, 74 insertions(+), 17 deletions(-)

diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c
index bc2663cd47fa..177b5c1eee8f 100644
--- a/drivers/power/supply/axp20x_ac_power.c
+++ b/drivers/power/supply/axp20x_ac_power.c
@@ -15,6 +15,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
 #include <linux/power_supply.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
@@ -44,6 +45,8 @@ struct axp20x_ac_power {
 	struct iio_channel *acin_v;
 	struct iio_channel *acin_i;
 	bool has_acin_path_sel;
+	unsigned int num_irqs;
+	unsigned int irqs[];
 };
 
 static irqreturn_t axp20x_ac_power_irq(int irq, void *devid)
@@ -242,38 +245,86 @@ static const struct power_supply_desc axp813_ac_power_desc = {
 	.set_property = axp813_ac_power_set_property,
 };
 
+static const char * const axp20x_irq_names[] = {
+	"ACIN_PLUGIN",
+	"ACIN_REMOVAL",
+};
+
 struct axp_data {
 	const struct power_supply_desc	*power_desc;
+	const char * const		*irq_names;
+	unsigned int			num_irq_names;
 	bool				acin_adc;
 	bool				acin_path_sel;
 };
 
 static const struct axp_data axp20x_data = {
 	.power_desc	= &axp20x_ac_power_desc,
+	.irq_names	= axp20x_irq_names,
+	.num_irq_names	= ARRAY_SIZE(axp20x_irq_names),
 	.acin_adc	= true,
 	.acin_path_sel	= false,
 };
 
 static const struct axp_data axp22x_data = {
 	.power_desc	= &axp22x_ac_power_desc,
+	.irq_names	= axp20x_irq_names,
+	.num_irq_names	= ARRAY_SIZE(axp20x_irq_names),
 	.acin_adc	= false,
 	.acin_path_sel	= false,
 };
 
 static const struct axp_data axp813_data = {
 	.power_desc	= &axp813_ac_power_desc,
+	.irq_names	= axp20x_irq_names,
+	.num_irq_names	= ARRAY_SIZE(axp20x_irq_names),
 	.acin_adc	= false,
 	.acin_path_sel	= true,
 };
 
+#ifdef CONFIG_PM_SLEEP
+static int axp20x_ac_power_suspend(struct device *dev)
+{
+	struct axp20x_ac_power *power = dev_get_drvdata(dev);
+	int i = 0;
+
+	/*
+	 * Allow wake via ACIN_PLUGIN only.
+	 *
+	 * As nested threaded IRQs are not automatically disabled during
+	 * suspend, we must explicitly disable the remainder of the IRQs.
+	 */
+	if (device_may_wakeup(&power->supply->dev))
+		enable_irq_wake(power->irqs[i++]);
+	while (i < power->num_irqs)
+		disable_irq(power->irqs[i++]);
+
+	return 0;
+}
+
+static int axp20x_ac_power_resume(struct device *dev)
+{
+	struct axp20x_ac_power *power = dev_get_drvdata(dev);
+	int i = 0;
+
+	if (device_may_wakeup(&power->supply->dev))
+		disable_irq_wake(power->irqs[i++]);
+	while (i < power->num_irqs)
+		enable_irq(power->irqs[i++]);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(axp20x_ac_power_pm_ops, axp20x_ac_power_suspend,
+						 axp20x_ac_power_resume);
+
 static int axp20x_ac_power_probe(struct platform_device *pdev)
 {
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
 	struct power_supply_config psy_cfg = {};
 	struct axp20x_ac_power *power;
 	const struct axp_data *axp_data;
-	static const char * const irq_names[] = { "ACIN_PLUGIN", "ACIN_REMOVAL",
-		NULL };
 	int i, irq, ret;
 
 	if (!of_device_is_available(pdev->dev.of_node))
@@ -284,12 +335,14 @@ static int axp20x_ac_power_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
+	axp_data = of_device_get_match_data(&pdev->dev);
+
+	power = devm_kzalloc(&pdev->dev,
+			     struct_size(power, irqs, axp_data->num_irq_names),
+			     GFP_KERNEL);
 	if (!power)
 		return -ENOMEM;
 
-	axp_data = of_device_get_match_data(&pdev->dev);
-
 	if (axp_data->acin_adc) {
 		power->acin_v = devm_iio_channel_get(&pdev->dev, "acin_v");
 		if (IS_ERR(power->acin_v)) {
@@ -308,6 +361,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;
+	power->num_irqs = axp_data->num_irq_names;
 
 	platform_set_drvdata(pdev, power);
 
@@ -321,20 +375,22 @@ static int axp20x_ac_power_probe(struct platform_device *pdev)
 		return PTR_ERR(power->supply);
 
 	/* Request irqs after registering, as irqs may trigger immediately */
-	for (i = 0; irq_names[i]; i++) {
-		irq = platform_get_irq_byname(pdev, irq_names[i]);
+	for (i = 0; i < axp_data->num_irq_names; i++) {
+		irq = platform_get_irq_byname(pdev, axp_data->irq_names[i]);
 		if (irq < 0) {
-			dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
-				 irq_names[i], irq);
-			continue;
+			dev_err(&pdev->dev, "No IRQ for %s: %d\n",
+				axp_data->irq_names[i], irq);
+			return irq;
 		}
-		irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
-		ret = devm_request_any_context_irq(&pdev->dev, irq,
+		power->irqs[i] = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
+		ret = devm_request_any_context_irq(&pdev->dev, power->irqs[i],
 						   axp20x_ac_power_irq, 0,
 						   DRVNAME, power);
-		if (ret < 0)
-			dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
-				 irq_names[i], ret);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "Error requesting %s IRQ: %d\n",
+				axp_data->irq_names[i], ret);
+			return ret;
+		}
 	}
 
 	return 0;
@@ -357,8 +413,9 @@ MODULE_DEVICE_TABLE(of, axp20x_ac_power_match);
 static struct platform_driver axp20x_ac_power_driver = {
 	.probe = axp20x_ac_power_probe,
 	.driver = {
-		.name = DRVNAME,
-		.of_match_table = axp20x_ac_power_match,
+		.name		= DRVNAME,
+		.of_match_table	= axp20x_ac_power_match,
+		.pm		= &axp20x_ac_power_pm_ops,
 	},
 };
 
-- 
2.23.0

Re: [linux-sunxi] [PATCH v2 4/9] power: supply: axp20x_ac_power: Add wakeup control

[Chen-Yu Tsai]

On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
>
> The AC power supply input can be used as a wakeup source. Hook up the
> ACIN_PLUGIN IRQ to trigger wakeup based on userspace configuration.
>
> To do this, we must remember the list of IRQs for the life of the
> device. To know how much space to allocate for the flexible array
> member, we switch from using a NULL sentinel to using an array length.
>
> Because we now depend on the specific order of the IRQs (we assume
> ACIN_PLUGIN is first and always present), failing to acquire an IRQ
> during probe must be a fatal error.
>
> To avoid spuriously waking up the system when the AC power supply is
> not configured as a wakeup source, we must explicitly disable all non-
> wake IRQs during system suspend. This is because the SoC's NMI input is
> shared among all IRQs on the AXP PMIC. Due to the use of regmap-irq, the
> individual IRQs within the PMIC are nested threaded interrupts, and are
> therefore not automatically disabled during system suspend.
>
> The upshot is that if any other device within the MFD (such as the power
> key) is an enabled wakeup source, all enabled IRQs within the PMIC will
> cause wakeup. We still need to call enable_irq_wake() when we *do* want
> wakeup, in case those other wakeup sources on the PMIC are all disabled.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/power/supply/axp20x_ac_power.c | 91 +++++++++++++++++++++-----
>  1 file changed, 74 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c
> index bc2663cd47fa..177b5c1eee8f 100644
> --- a/drivers/power/supply/axp20x_ac_power.c
> +++ b/drivers/power/supply/axp20x_ac_power.c
> @@ -15,6 +15,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm.h>
>  #include <linux/power_supply.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
> @@ -44,6 +45,8 @@ struct axp20x_ac_power {
>         struct iio_channel *acin_v;
>         struct iio_channel *acin_i;
>         bool has_acin_path_sel;
> +       unsigned int num_irqs;
> +       unsigned int irqs[];
>  };
>
>  static irqreturn_t axp20x_ac_power_irq(int irq, void *devid)
> @@ -242,38 +245,86 @@ static const struct power_supply_desc axp813_ac_power_desc = {
>         .set_property = axp813_ac_power_set_property,
>  };
>
> +static const char * const axp20x_irq_names[] = {
> +       "ACIN_PLUGIN",
> +       "ACIN_REMOVAL",
> +};
> +
>  struct axp_data {
>         const struct power_supply_desc  *power_desc;
> +       const char * const              *irq_names;
> +       unsigned int                    num_irq_names;
>         bool                            acin_adc;
>         bool                            acin_path_sel;
>  };
>
>  static const struct axp_data axp20x_data = {
>         .power_desc     = &axp20x_ac_power_desc,
> +       .irq_names      = axp20x_irq_names,
> +       .num_irq_names  = ARRAY_SIZE(axp20x_irq_names),
>         .acin_adc       = true,
>         .acin_path_sel  = false,
>  };
>
>  static const struct axp_data axp22x_data = {
>         .power_desc     = &axp22x_ac_power_desc,
> +       .irq_names      = axp20x_irq_names,
> +       .num_irq_names  = ARRAY_SIZE(axp20x_irq_names),
>         .acin_adc       = false,
>         .acin_path_sel  = false,
>  };
>
>  static const struct axp_data axp813_data = {
>         .power_desc     = &axp813_ac_power_desc,
> +       .irq_names      = axp20x_irq_names,
> +       .num_irq_names  = ARRAY_SIZE(axp20x_irq_names),
>         .acin_adc       = false,
>         .acin_path_sel  = true,
>  };
>
> +#ifdef CONFIG_PM_SLEEP
> +static int axp20x_ac_power_suspend(struct device *dev)
> +{
> +       struct axp20x_ac_power *power = dev_get_drvdata(dev);
> +       int i = 0;
> +
> +       /*
> +        * Allow wake via ACIN_PLUGIN only.
> +        *
> +        * As nested threaded IRQs are not automatically disabled during
> +        * suspend, we must explicitly disable the remainder of the IRQs.
> +        */
> +       if (device_may_wakeup(&power->supply->dev))
> +               enable_irq_wake(power->irqs[i++]);
> +       while (i < power->num_irqs)
> +               disable_irq(power->irqs[i++]);
> +
> +       return 0;
> +}
> +
> +static int axp20x_ac_power_resume(struct device *dev)
> +{
> +       struct axp20x_ac_power *power = dev_get_drvdata(dev);
> +       int i = 0;
> +
> +       if (device_may_wakeup(&power->supply->dev))
> +               disable_irq_wake(power->irqs[i++]);
> +       while (i < power->num_irqs)
> +               enable_irq(power->irqs[i++]);
> +
> +       return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(axp20x_ac_power_pm_ops, axp20x_ac_power_suspend,
> +                                                axp20x_ac_power_resume);
> +
>  static int axp20x_ac_power_probe(struct platform_device *pdev)
>  {
>         struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>         struct power_supply_config psy_cfg = {};
>         struct axp20x_ac_power *power;
>         const struct axp_data *axp_data;
> -       static const char * const irq_names[] = { "ACIN_PLUGIN", "ACIN_REMOVAL",
> -               NULL };
>         int i, irq, ret;
>
>         if (!of_device_is_available(pdev->dev.of_node))
> @@ -284,12 +335,14 @@ static int axp20x_ac_power_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> -       power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
> +       axp_data = of_device_get_match_data(&pdev->dev);
> +
> +       power = devm_kzalloc(&pdev->dev,
> +                            struct_size(power, irqs, axp_data->num_irq_names),
> +                            GFP_KERNEL);
>         if (!power)
>                 return -ENOMEM;
>
> -       axp_data = of_device_get_match_data(&pdev->dev);
> -

This change doesn't seem related, nor is it needed.


>         if (axp_data->acin_adc) {
>                 power->acin_v = devm_iio_channel_get(&pdev->dev, "acin_v");
>                 if (IS_ERR(power->acin_v)) {
> @@ -308,6 +361,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;
> +       power->num_irqs = axp_data->num_irq_names;
>
>         platform_set_drvdata(pdev, power);
>
> @@ -321,20 +375,22 @@ static int axp20x_ac_power_probe(struct platform_device *pdev)
>                 return PTR_ERR(power->supply);
>
>         /* Request irqs after registering, as irqs may trigger immediately */
> -       for (i = 0; irq_names[i]; i++) {
> -               irq = platform_get_irq_byname(pdev, irq_names[i]);
> +       for (i = 0; i < axp_data->num_irq_names; i++) {
> +               irq = platform_get_irq_byname(pdev, axp_data->irq_names[i]);
>                 if (irq < 0) {
> -                       dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
> -                                irq_names[i], irq);
> -                       continue;
> +                       dev_err(&pdev->dev, "No IRQ for %s: %d\n",
> +                               axp_data->irq_names[i], irq);
> +                       return irq;
>                 }
> -               irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
> -               ret = devm_request_any_context_irq(&pdev->dev, irq,
> +               power->irqs[i] = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
> +               ret = devm_request_any_context_irq(&pdev->dev, power->irqs[i],
>                                                    axp20x_ac_power_irq, 0,
>                                                    DRVNAME, power);
> -               if (ret < 0)
> -                       dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
> -                                irq_names[i], ret);
> +               if (ret < 0) {
> +                       dev_err(&pdev->dev, "Error requesting %s IRQ: %d\n",
> +                               axp_data->irq_names[i], ret);
> +                       return ret;
> +               }
>         }
>
>         return 0;
> @@ -357,8 +413,9 @@ MODULE_DEVICE_TABLE(of, axp20x_ac_power_match);
>  static struct platform_driver axp20x_ac_power_driver = {
>         .probe = axp20x_ac_power_probe,
>         .driver = {
> -               .name = DRVNAME,
> -               .of_match_table = axp20x_ac_power_match,
> +               .name           = DRVNAME,
> +               .of_match_table = axp20x_ac_power_match,
> +               .pm             = &axp20x_ac_power_pm_ops,
>         },
>  };
>

Otherwise,

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

Re: [linux-sunxi] [PATCH v2 4/9] power: supply: axp20x_ac_power: Add wakeup control

[Chen-Yu Tsai]

On Sun, Jan 5, 2020 at 6:24 PM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
> >
> > The AC power supply input can be used as a wakeup source. Hook up the
> > ACIN_PLUGIN IRQ to trigger wakeup based on userspace configuration.
> >
> > To do this, we must remember the list of IRQs for the life of the
> > device. To know how much space to allocate for the flexible array
> > member, we switch from using a NULL sentinel to using an array length.
> >
> > Because we now depend on the specific order of the IRQs (we assume
> > ACIN_PLUGIN is first and always present), failing to acquire an IRQ
> > during probe must be a fatal error.
> >
> > To avoid spuriously waking up the system when the AC power supply is
> > not configured as a wakeup source, we must explicitly disable all non-
> > wake IRQs during system suspend. This is because the SoC's NMI input is
> > shared among all IRQs on the AXP PMIC. Due to the use of regmap-irq, the
> > individual IRQs within the PMIC are nested threaded interrupts, and are
> > therefore not automatically disabled during system suspend.
> >
> > The upshot is that if any other device within the MFD (such as the power
> > key) is an enabled wakeup source, all enabled IRQs within the PMIC will
> > cause wakeup. We still need to call enable_irq_wake() when we *do* want
> > wakeup, in case those other wakeup sources on the PMIC are all disabled.
> >
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
> > ---
> >  drivers/power/supply/axp20x_ac_power.c | 91 +++++++++++++++++++++-----
> >  1 file changed, 74 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c
> > index bc2663cd47fa..177b5c1eee8f 100644
> > --- a/drivers/power/supply/axp20x_ac_power.c
> > +++ b/drivers/power/supply/axp20x_ac_power.c

[...]

> > @@ -284,12 +335,14 @@ static int axp20x_ac_power_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >         }
> >
> > -       power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
> > +       axp_data = of_device_get_match_data(&pdev->dev);
> > +
> > +       power = devm_kzalloc(&pdev->dev,
> > +                            struct_size(power, irqs, axp_data->num_irq_names),
> > +                            GFP_KERNEL);
> >         if (!power)
> >                 return -ENOMEM;
> >
> > -       axp_data = of_device_get_match_data(&pdev->dev);
> > -
>
> This change doesn't seem related, nor is it needed.

Nevermind, I see it.

> >         if (axp_data->acin_adc) {
> >                 power->acin_v = devm_iio_channel_get(&pdev->dev, "acin_v");
> >                 if (IS_ERR(power->acin_v)) {

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

[PATCH v2 5/9] power: supply: axp20x_usb_power: Remove unused device_node

[Samuel Holland]

This member of struct axp20x_usb_power is not used anywhere.
Remove it.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/power/supply/axp20x_usb_power.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index 5f0a5722b19e..dd3f3f12e41d 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -57,7 +57,6 @@
 #define DEBOUNCE_TIME			msecs_to_jiffies(50)
 
 struct axp20x_usb_power {
-	struct device_node *np;
 	struct regmap *regmap;
 	struct power_supply *supply;
 	enum axp20x_variants axp20x_id;
@@ -465,7 +464,6 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 	power->axp20x_id = (enum axp20x_variants)of_device_get_match_data(
 								&pdev->dev);
 
-	power->np = pdev->dev.of_node;
 	power->regmap = axp20x->regmap;
 
 	if (power->axp20x_id == AXP202_ID) {
-- 
2.23.0

Re: [PATCH v2 5/9] power: supply: axp20x_usb_power: Remove unused device_node

[Chen-Yu Tsai]

On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
>
> This member of struct axp20x_usb_power is not used anywhere.
> Remove it.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

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

AFAICT this could be the first patch in the series, or ordered
just after the fixes.

[PATCH v2 6/9] power: supply: axp20x_usb_power: Use a match structure

[Samuel Holland]

Instead of ad-hoc variant ID checks throughout the code, let's start
moving the variant-specific details to a match structure. This allows
for future flexibility, and it better matches the other axp20x power
supply drivers.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/power/supply/axp20x_usb_power.c | 91 ++++++++++++++++---------
 1 file changed, 60 insertions(+), 31 deletions(-)

diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index dd3f3f12e41d..2d7272e19a87 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -405,6 +405,50 @@ static const struct power_supply_desc axp22x_usb_power_desc = {
 	.set_property = axp20x_usb_power_set_property,
 };
 
+static const char * const axp20x_irq_names[] = {
+	"VBUS_PLUGIN",
+	"VBUS_REMOVAL",
+	"VBUS_VALID",
+	"VBUS_NOT_VALID",
+	NULL
+};
+
+static const char * const axp22x_irq_names[] = {
+	"VBUS_PLUGIN",
+	"VBUS_REMOVAL",
+	NULL
+};
+
+struct axp_data {
+	const struct power_supply_desc	*power_desc;
+	const char * const		*irq_names;
+	enum axp20x_variants		axp20x_id;
+};
+
+static const struct axp_data axp202_data = {
+	.power_desc	= &axp20x_usb_power_desc,
+	.irq_names	= axp20x_irq_names,
+	.axp20x_id	= AXP202_ID,
+};
+
+static const struct axp_data axp221_data = {
+	.power_desc	= &axp22x_usb_power_desc,
+	.irq_names	= axp22x_irq_names,
+	.axp20x_id	= AXP221_ID,
+};
+
+static const struct axp_data axp223_data = {
+	.power_desc	= &axp22x_usb_power_desc,
+	.irq_names	= axp22x_irq_names,
+	.axp20x_id	= AXP223_ID,
+};
+
+static const struct axp_data axp813_data = {
+	.power_desc	= &axp22x_usb_power_desc,
+	.irq_names	= axp22x_irq_names,
+	.axp20x_id	= AXP813_ID,
+};
+
 static int configure_iio_channels(struct platform_device *pdev,
 				  struct axp20x_usb_power *power)
 {
@@ -440,12 +484,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
 	struct power_supply_config psy_cfg = {};
 	struct axp20x_usb_power *power;
-	static const char * const axp20x_irq_names[] = { "VBUS_PLUGIN",
-		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID", NULL };
-	static const char * const axp22x_irq_names[] = {
-		"VBUS_PLUGIN", "VBUS_REMOVAL", NULL };
-	const char * const *irq_names;
-	const struct power_supply_desc *usb_power_desc;
+	const struct axp_data *axp_data;
 	int i, irq, ret;
 
 	if (!of_device_is_available(pdev->dev.of_node))
@@ -456,15 +495,16 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	axp_data = of_device_get_match_data(&pdev->dev);
+
 	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
 	if (!power)
 		return -ENOMEM;
 
-	platform_set_drvdata(pdev, power);
-	power->axp20x_id = (enum axp20x_variants)of_device_get_match_data(
-								&pdev->dev);
-
 	power->regmap = axp20x->regmap;
+	power->axp20x_id = axp_data->axp20x_id;
+
+	platform_set_drvdata(pdev, power);
 
 	if (power->axp20x_id == AXP202_ID) {
 		/* Enable vbus valid checking */
@@ -481,18 +521,6 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 
 		if (ret)
 			return ret;
-
-		usb_power_desc = &axp20x_usb_power_desc;
-		irq_names = axp20x_irq_names;
-	} else if (power->axp20x_id == AXP221_ID ||
-		   power->axp20x_id == AXP223_ID ||
-		   power->axp20x_id == AXP813_ID) {
-		usb_power_desc = &axp22x_usb_power_desc;
-		irq_names = axp22x_irq_names;
-	} else {
-		dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
-			axp20x->variant);
-		return -EINVAL;
 	}
 
 	if (power->axp20x_id == AXP813_ID) {
@@ -504,17 +532,18 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 	psy_cfg.of_node = pdev->dev.of_node;
 	psy_cfg.drv_data = power;
 
-	power->supply = devm_power_supply_register(&pdev->dev, usb_power_desc,
+	power->supply = devm_power_supply_register(&pdev->dev,
+						   axp_data->power_desc,
 						   &psy_cfg);
 	if (IS_ERR(power->supply))
 		return PTR_ERR(power->supply);
 
 	/* Request irqs after registering, as irqs may trigger immediately */
-	for (i = 0; irq_names[i]; i++) {
-		irq = platform_get_irq_byname(pdev, irq_names[i]);
+	for (i = 0; axp_data->irq_names[i]; i++) {
+		irq = platform_get_irq_byname(pdev, axp_data->irq_names[i]);
 		if (irq < 0) {
 			dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
-				 irq_names[i], irq);
+				 axp_data->irq_names[i], irq);
 			continue;
 		}
 		irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
@@ -522,7 +551,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 				axp20x_usb_power_irq, 0, DRVNAME, power);
 		if (ret < 0)
 			dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
-				 irq_names[i], ret);
+				 axp_data->irq_names[i], ret);
 	}
 
 	INIT_DELAYED_WORK(&power->vbus_detect, axp20x_usb_power_poll_vbus);
@@ -544,16 +573,16 @@ static int axp20x_usb_power_remove(struct platform_device *pdev)
 static const struct of_device_id axp20x_usb_power_match[] = {
 	{
 		.compatible = "x-powers,axp202-usb-power-supply",
-		.data = (void *)AXP202_ID,
+		.data = &axp202_data,
 	}, {
 		.compatible = "x-powers,axp221-usb-power-supply",
-		.data = (void *)AXP221_ID,
+		.data = &axp221_data,
 	}, {
 		.compatible = "x-powers,axp223-usb-power-supply",
-		.data = (void *)AXP223_ID,
+		.data = &axp223_data,
 	}, {
 		.compatible = "x-powers,axp813-usb-power-supply",
-		.data = (void *)AXP813_ID,
+		.data = &axp813_data,
 	}, { /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
-- 
2.23.0

Re: [linux-sunxi] [PATCH v2 6/9] power: supply: axp20x_usb_power: Use a match structure

[Chen-Yu Tsai]

On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
>
> Instead of ad-hoc variant ID checks throughout the code, let's start
> moving the variant-specific details to a match structure. This allows
> for future flexibility, and it better matches the other axp20x power
> supply drivers.

You should probably mention that there are still parts of the code
where ID matching is done.

> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/power/supply/axp20x_usb_power.c | 91 ++++++++++++++++---------
>  1 file changed, 60 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> index dd3f3f12e41d..2d7272e19a87 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -405,6 +405,50 @@ static const struct power_supply_desc axp22x_usb_power_desc = {
>         .set_property = axp20x_usb_power_set_property,
>  };
>
> +static const char * const axp20x_irq_names[] = {
> +       "VBUS_PLUGIN",
> +       "VBUS_REMOVAL",
> +       "VBUS_VALID",
> +       "VBUS_NOT_VALID",
> +       NULL
> +};
> +
> +static const char * const axp22x_irq_names[] = {
> +       "VBUS_PLUGIN",
> +       "VBUS_REMOVAL",
> +       NULL
> +};
> +
> +struct axp_data {
> +       const struct power_supply_desc  *power_desc;
> +       const char * const              *irq_names;
> +       enum axp20x_variants            axp20x_id;
> +};
> +
> +static const struct axp_data axp202_data = {
> +       .power_desc     = &axp20x_usb_power_desc,
> +       .irq_names      = axp20x_irq_names,
> +       .axp20x_id      = AXP202_ID,
> +};
> +
> +static const struct axp_data axp221_data = {
> +       .power_desc     = &axp22x_usb_power_desc,
> +       .irq_names      = axp22x_irq_names,
> +       .axp20x_id      = AXP221_ID,
> +};
> +
> +static const struct axp_data axp223_data = {
> +       .power_desc     = &axp22x_usb_power_desc,
> +       .irq_names      = axp22x_irq_names,
> +       .axp20x_id      = AXP223_ID,
> +};
> +
> +static const struct axp_data axp813_data = {
> +       .power_desc     = &axp22x_usb_power_desc,
> +       .irq_names      = axp22x_irq_names,
> +       .axp20x_id      = AXP813_ID,
> +};
> +
>  static int configure_iio_channels(struct platform_device *pdev,
>                                   struct axp20x_usb_power *power)
>  {
> @@ -440,12 +484,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>         struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>         struct power_supply_config psy_cfg = {};
>         struct axp20x_usb_power *power;
> -       static const char * const axp20x_irq_names[] = { "VBUS_PLUGIN",
> -               "VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID", NULL };
> -       static const char * const axp22x_irq_names[] = {
> -               "VBUS_PLUGIN", "VBUS_REMOVAL", NULL };
> -       const char * const *irq_names;
> -       const struct power_supply_desc *usb_power_desc;
> +       const struct axp_data *axp_data;
>         int i, irq, ret;
>
>         if (!of_device_is_available(pdev->dev.of_node))
> @@ -456,15 +495,16 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> +       axp_data = of_device_get_match_data(&pdev->dev);
> +
>         power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>         if (!power)
>                 return -ENOMEM;
>
> -       platform_set_drvdata(pdev, power);
> -       power->axp20x_id = (enum axp20x_variants)of_device_get_match_data(
> -                                                               &pdev->dev);
> -
>         power->regmap = axp20x->regmap;
> +       power->axp20x_id = axp_data->axp20x_id;
> +
> +       platform_set_drvdata(pdev, power);

Not sure why this needs to be reordered.

>         if (power->axp20x_id == AXP202_ID) {
>                 /* Enable vbus valid checking */
> @@ -481,18 +521,6 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>
>                 if (ret)
>                         return ret;
> -
> -               usb_power_desc = &axp20x_usb_power_desc;
> -               irq_names = axp20x_irq_names;
> -       } else if (power->axp20x_id == AXP221_ID ||
> -                  power->axp20x_id == AXP223_ID ||
> -                  power->axp20x_id == AXP813_ID) {
> -               usb_power_desc = &axp22x_usb_power_desc;
> -               irq_names = axp22x_irq_names;
> -       } else {
> -               dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
> -                       axp20x->variant);
> -               return -EINVAL;
>         }
>
>         if (power->axp20x_id == AXP813_ID) {
> @@ -504,17 +532,18 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>         psy_cfg.of_node = pdev->dev.of_node;
>         psy_cfg.drv_data = power;
>
> -       power->supply = devm_power_supply_register(&pdev->dev, usb_power_desc,
> +       power->supply = devm_power_supply_register(&pdev->dev,
> +                                                  axp_data->power_desc,
>                                                    &psy_cfg);
>         if (IS_ERR(power->supply))
>                 return PTR_ERR(power->supply);
>
>         /* Request irqs after registering, as irqs may trigger immediately */
> -       for (i = 0; irq_names[i]; i++) {
> -               irq = platform_get_irq_byname(pdev, irq_names[i]);
> +       for (i = 0; axp_data->irq_names[i]; i++) {
> +               irq = platform_get_irq_byname(pdev, axp_data->irq_names[i]);
>                 if (irq < 0) {
>                         dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
> -                                irq_names[i], irq);
> +                                axp_data->irq_names[i], irq);
>                         continue;
>                 }
>                 irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
> @@ -522,7 +551,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>                                 axp20x_usb_power_irq, 0, DRVNAME, power);
>                 if (ret < 0)
>                         dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
> -                                irq_names[i], ret);
> +                                axp_data->irq_names[i], ret);
>         }
>
>         INIT_DELAYED_WORK(&power->vbus_detect, axp20x_usb_power_poll_vbus);
> @@ -544,16 +573,16 @@ static int axp20x_usb_power_remove(struct platform_device *pdev)
>  static const struct of_device_id axp20x_usb_power_match[] = {
>         {
>                 .compatible = "x-powers,axp202-usb-power-supply",
> -               .data = (void *)AXP202_ID,
> +               .data = &axp202_data,
>         }, {
>                 .compatible = "x-powers,axp221-usb-power-supply",
> -               .data = (void *)AXP221_ID,
> +               .data = &axp221_data,
>         }, {
>                 .compatible = "x-powers,axp223-usb-power-supply",
> -               .data = (void *)AXP223_ID,
> +               .data = &axp223_data,
>         }, {
>                 .compatible = "x-powers,axp813-usb-power-supply",
> -               .data = (void *)AXP813_ID,
> +               .data = &axp813_data,
>         }, { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
> --
> 2.23.0

Otherwise,

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

Re: [linux-sunxi] [PATCH v2 6/9] power: supply: axp20x_usb_power: Use a match structure

[Samuel Holland]

On 1/5/20 4:34 AM, Chen-Yu Tsai wrote:
> On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
>>
>> Instead of ad-hoc variant ID checks throughout the code, let's start
>> moving the variant-specific details to a match structure. This allows
>> for future flexibility, and it better matches the other axp20x power
>> supply drivers.
> 
> You should probably mention that there are still parts of the code
> where ID matching is done.

Will do for v3.

>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  drivers/power/supply/axp20x_usb_power.c | 91 ++++++++++++++++---------
>>  1 file changed, 60 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
>> index dd3f3f12e41d..2d7272e19a87 100644
>> --- a/drivers/power/supply/axp20x_usb_power.c
>> +++ b/drivers/power/supply/axp20x_usb_power.c
>> @@ -405,6 +405,50 @@ static const struct power_supply_desc axp22x_usb_power_desc = {
>>         .set_property = axp20x_usb_power_set_property,
>>  };
>>
>> +static const char * const axp20x_irq_names[] = {
>> +       "VBUS_PLUGIN",
>> +       "VBUS_REMOVAL",
>> +       "VBUS_VALID",
>> +       "VBUS_NOT_VALID",
>> +       NULL
>> +};
>> +
>> +static const char * const axp22x_irq_names[] = {
>> +       "VBUS_PLUGIN",
>> +       "VBUS_REMOVAL",
>> +       NULL
>> +};
>> +
>> +struct axp_data {
>> +       const struct power_supply_desc  *power_desc;
>> +       const char * const              *irq_names;
>> +       enum axp20x_variants            axp20x_id;
>> +};
>> +
>> +static const struct axp_data axp202_data = {
>> +       .power_desc     = &axp20x_usb_power_desc,
>> +       .irq_names      = axp20x_irq_names,
>> +       .axp20x_id      = AXP202_ID,
>> +};
>> +
>> +static const struct axp_data axp221_data = {
>> +       .power_desc     = &axp22x_usb_power_desc,
>> +       .irq_names      = axp22x_irq_names,
>> +       .axp20x_id      = AXP221_ID,
>> +};
>> +
>> +static const struct axp_data axp223_data = {
>> +       .power_desc     = &axp22x_usb_power_desc,
>> +       .irq_names      = axp22x_irq_names,
>> +       .axp20x_id      = AXP223_ID,
>> +};
>> +
>> +static const struct axp_data axp813_data = {
>> +       .power_desc     = &axp22x_usb_power_desc,
>> +       .irq_names      = axp22x_irq_names,
>> +       .axp20x_id      = AXP813_ID,
>> +};
>> +
>>  static int configure_iio_channels(struct platform_device *pdev,
>>                                   struct axp20x_usb_power *power)
>>  {
>> @@ -440,12 +484,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>>         struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>>         struct power_supply_config psy_cfg = {};
>>         struct axp20x_usb_power *power;
>> -       static const char * const axp20x_irq_names[] = { "VBUS_PLUGIN",
>> -               "VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID", NULL };
>> -       static const char * const axp22x_irq_names[] = {
>> -               "VBUS_PLUGIN", "VBUS_REMOVAL", NULL };
>> -       const char * const *irq_names;
>> -       const struct power_supply_desc *usb_power_desc;
>> +       const struct axp_data *axp_data;
>>         int i, irq, ret;
>>
>>         if (!of_device_is_available(pdev->dev.of_node))
>> @@ -456,15 +495,16 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>>                 return -EINVAL;
>>         }
>>
>> +       axp_data = of_device_get_match_data(&pdev->dev);
>> +
>>         power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>>         if (!power)
>>                 return -ENOMEM;
>>
>> -       platform_set_drvdata(pdev, power);
>> -       power->axp20x_id = (enum axp20x_variants)of_device_get_match_data(
>> -                                                               &pdev->dev);
>> -
>>         power->regmap = axp20x->regmap;
>> +       power->axp20x_id = axp_data->axp20x_id;
>> +
>> +       platform_set_drvdata(pdev, power);
> 
> Not sure why this needs to be reordered.

It doesn't necessarily; moving the call to platform_set_drvdata() matches the
order in axp20x_ac_power, which makes it easier to compare the two probe
functions. I can drop it for v3 if you prefer.

>>         if (power->axp20x_id == AXP202_ID) {
>>                 /* Enable vbus valid checking */
>> @@ -481,18 +521,6 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>>
>>                 if (ret)
>>                         return ret;
>> -
>> -               usb_power_desc = &axp20x_usb_power_desc;
>> -               irq_names = axp20x_irq_names;
>> -       } else if (power->axp20x_id == AXP221_ID ||
>> -                  power->axp20x_id == AXP223_ID ||
>> -                  power->axp20x_id == AXP813_ID) {
>> -               usb_power_desc = &axp22x_usb_power_desc;
>> -               irq_names = axp22x_irq_names;
>> -       } else {
>> -               dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
>> -                       axp20x->variant);
>> -               return -EINVAL;
>>         }
>>
>>         if (power->axp20x_id == AXP813_ID) {
>> @@ -504,17 +532,18 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>>         psy_cfg.of_node = pdev->dev.of_node;
>>         psy_cfg.drv_data = power;
>>
>> -       power->supply = devm_power_supply_register(&pdev->dev, usb_power_desc,
>> +       power->supply = devm_power_supply_register(&pdev->dev,
>> +                                                  axp_data->power_desc,
>>                                                    &psy_cfg);
>>         if (IS_ERR(power->supply))
>>                 return PTR_ERR(power->supply);
>>
>>         /* Request irqs after registering, as irqs may trigger immediately */
>> -       for (i = 0; irq_names[i]; i++) {
>> -               irq = platform_get_irq_byname(pdev, irq_names[i]);
>> +       for (i = 0; axp_data->irq_names[i]; i++) {
>> +               irq = platform_get_irq_byname(pdev, axp_data->irq_names[i]);
>>                 if (irq < 0) {
>>                         dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
>> -                                irq_names[i], irq);
>> +                                axp_data->irq_names[i], irq);
>>                         continue;
>>                 }
>>                 irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
>> @@ -522,7 +551,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>>                                 axp20x_usb_power_irq, 0, DRVNAME, power);
>>                 if (ret < 0)
>>                         dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
>> -                                irq_names[i], ret);
>> +                                axp_data->irq_names[i], ret);
>>         }
>>
>>         INIT_DELAYED_WORK(&power->vbus_detect, axp20x_usb_power_poll_vbus);
>> @@ -544,16 +573,16 @@ static int axp20x_usb_power_remove(struct platform_device *pdev)
>>  static const struct of_device_id axp20x_usb_power_match[] = {
>>         {
>>                 .compatible = "x-powers,axp202-usb-power-supply",
>> -               .data = (void *)AXP202_ID,
>> +               .data = &axp202_data,
>>         }, {
>>                 .compatible = "x-powers,axp221-usb-power-supply",
>> -               .data = (void *)AXP221_ID,
>> +               .data = &axp221_data,
>>         }, {
>>                 .compatible = "x-powers,axp223-usb-power-supply",
>> -               .data = (void *)AXP223_ID,
>> +               .data = &axp223_data,
>>         }, {
>>                 .compatible = "x-powers,axp813-usb-power-supply",
>> -               .data = (void *)AXP813_ID,
>> +               .data = &axp813_data,
>>         }, { /* sentinel */ }
>>  };
>>  MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
>> --
>> 2.23.0
> 
> Otherwise,
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> 

Re: [linux-sunxi] [PATCH v2 6/9] power: supply: axp20x_usb_power: Use a match structure

[Chen-Yu Tsai]

On Mon, Jan 6, 2020 at 1:59 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 1/5/20 4:34 AM, Chen-Yu Tsai wrote:
> > On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
> >>
> >> Instead of ad-hoc variant ID checks throughout the code, let's start
> >> moving the variant-specific details to a match structure. This allows
> >> for future flexibility, and it better matches the other axp20x power
> >> supply drivers.
> >
> > You should probably mention that there are still parts of the code
> > where ID matching is done.
>
> Will do for v3.
>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>  drivers/power/supply/axp20x_usb_power.c | 91 ++++++++++++++++---------
> >>  1 file changed, 60 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> >> index dd3f3f12e41d..2d7272e19a87 100644
> >> --- a/drivers/power/supply/axp20x_usb_power.c
> >> +++ b/drivers/power/supply/axp20x_usb_power.c
> >> @@ -405,6 +405,50 @@ static const struct power_supply_desc axp22x_usb_power_desc = {
> >>         .set_property = axp20x_usb_power_set_property,
> >>  };
> >>
> >> +static const char * const axp20x_irq_names[] = {
> >> +       "VBUS_PLUGIN",
> >> +       "VBUS_REMOVAL",
> >> +       "VBUS_VALID",
> >> +       "VBUS_NOT_VALID",
> >> +       NULL
> >> +};
> >> +
> >> +static const char * const axp22x_irq_names[] = {
> >> +       "VBUS_PLUGIN",
> >> +       "VBUS_REMOVAL",
> >> +       NULL
> >> +};
> >> +
> >> +struct axp_data {
> >> +       const struct power_supply_desc  *power_desc;
> >> +       const char * const              *irq_names;
> >> +       enum axp20x_variants            axp20x_id;
> >> +};
> >> +
> >> +static const struct axp_data axp202_data = {
> >> +       .power_desc     = &axp20x_usb_power_desc,
> >> +       .irq_names      = axp20x_irq_names,
> >> +       .axp20x_id      = AXP202_ID,
> >> +};
> >> +
> >> +static const struct axp_data axp221_data = {
> >> +       .power_desc     = &axp22x_usb_power_desc,
> >> +       .irq_names      = axp22x_irq_names,
> >> +       .axp20x_id      = AXP221_ID,
> >> +};
> >> +
> >> +static const struct axp_data axp223_data = {
> >> +       .power_desc     = &axp22x_usb_power_desc,
> >> +       .irq_names      = axp22x_irq_names,
> >> +       .axp20x_id      = AXP223_ID,
> >> +};
> >> +
> >> +static const struct axp_data axp813_data = {
> >> +       .power_desc     = &axp22x_usb_power_desc,
> >> +       .irq_names      = axp22x_irq_names,
> >> +       .axp20x_id      = AXP813_ID,
> >> +};
> >> +
> >>  static int configure_iio_channels(struct platform_device *pdev,
> >>                                   struct axp20x_usb_power *power)
> >>  {
> >> @@ -440,12 +484,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
> >>         struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> >>         struct power_supply_config psy_cfg = {};
> >>         struct axp20x_usb_power *power;
> >> -       static const char * const axp20x_irq_names[] = { "VBUS_PLUGIN",
> >> -               "VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID", NULL };
> >> -       static const char * const axp22x_irq_names[] = {
> >> -               "VBUS_PLUGIN", "VBUS_REMOVAL", NULL };
> >> -       const char * const *irq_names;
> >> -       const struct power_supply_desc *usb_power_desc;
> >> +       const struct axp_data *axp_data;
> >>         int i, irq, ret;
> >>
> >>         if (!of_device_is_available(pdev->dev.of_node))
> >> @@ -456,15 +495,16 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
> >>                 return -EINVAL;
> >>         }
> >>
> >> +       axp_data = of_device_get_match_data(&pdev->dev);
> >> +
> >>         power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
> >>         if (!power)
> >>                 return -ENOMEM;
> >>
> >> -       platform_set_drvdata(pdev, power);
> >> -       power->axp20x_id = (enum axp20x_variants)of_device_get_match_data(
> >> -                                                               &pdev->dev);
> >> -
> >>         power->regmap = axp20x->regmap;
> >> +       power->axp20x_id = axp_data->axp20x_id;
> >> +
> >> +       platform_set_drvdata(pdev, power);
> >
> > Not sure why this needs to be reordered.
>
> It doesn't necessarily; moving the call to platform_set_drvdata() matches the
> order in axp20x_ac_power, which makes it easier to compare the two probe
> functions. I can drop it for v3 if you prefer.

I was referring to the whole hunk. Seems of_device_get_match_data() needs to
be before devm_kzalloc() due to changes in the next patch. I would keep the
ordering the same in this patch, and do the shuffling when needed, i.e. in
the next patch. Please also mention any not required but desired reordering
in the commit log, such as moving platform_set_drvdata() to match the other
drivers.

Thanks
ChenYu

> >>         if (power->axp20x_id == AXP202_ID) {
> >>                 /* Enable vbus valid checking */
> >> @@ -481,18 +521,6 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
> >>
> >>                 if (ret)
> >>                         return ret;
> >> -
> >> -               usb_power_desc = &axp20x_usb_power_desc;
> >> -               irq_names = axp20x_irq_names;
> >> -       } else if (power->axp20x_id == AXP221_ID ||
> >> -                  power->axp20x_id == AXP223_ID ||
> >> -                  power->axp20x_id == AXP813_ID) {
> >> -               usb_power_desc = &axp22x_usb_power_desc;
> >> -               irq_names = axp22x_irq_names;
> >> -       } else {
> >> -               dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
> >> -                       axp20x->variant);
> >> -               return -EINVAL;
> >>         }
> >>
> >>         if (power->axp20x_id == AXP813_ID) {
> >> @@ -504,17 +532,18 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
> >>         psy_cfg.of_node = pdev->dev.of_node;
> >>         psy_cfg.drv_data = power;
> >>
> >> -       power->supply = devm_power_supply_register(&pdev->dev, usb_power_desc,
> >> +       power->supply = devm_power_supply_register(&pdev->dev,
> >> +                                                  axp_data->power_desc,
> >>                                                    &psy_cfg);
> >>         if (IS_ERR(power->supply))
> >>                 return PTR_ERR(power->supply);
> >>
> >>         /* Request irqs after registering, as irqs may trigger immediately */
> >> -       for (i = 0; irq_names[i]; i++) {
> >> -               irq = platform_get_irq_byname(pdev, irq_names[i]);
> >> +       for (i = 0; axp_data->irq_names[i]; i++) {
> >> +               irq = platform_get_irq_byname(pdev, axp_data->irq_names[i]);
> >>                 if (irq < 0) {
> >>                         dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
> >> -                                irq_names[i], irq);
> >> +                                axp_data->irq_names[i], irq);
> >>                         continue;
> >>                 }
> >>                 irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
> >> @@ -522,7 +551,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
> >>                                 axp20x_usb_power_irq, 0, DRVNAME, power);
> >>                 if (ret < 0)
> >>                         dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
> >> -                                irq_names[i], ret);
> >> +                                axp_data->irq_names[i], ret);
> >>         }
> >>
> >>         INIT_DELAYED_WORK(&power->vbus_detect, axp20x_usb_power_poll_vbus);
> >> @@ -544,16 +573,16 @@ static int axp20x_usb_power_remove(struct platform_device *pdev)
> >>  static const struct of_device_id axp20x_usb_power_match[] = {
> >>         {
> >>                 .compatible = "x-powers,axp202-usb-power-supply",
> >> -               .data = (void *)AXP202_ID,
> >> +               .data = &axp202_data,
> >>         }, {
> >>                 .compatible = "x-powers,axp221-usb-power-supply",
> >> -               .data = (void *)AXP221_ID,
> >> +               .data = &axp221_data,
> >>         }, {
> >>                 .compatible = "x-powers,axp223-usb-power-supply",
> >> -               .data = (void *)AXP223_ID,
> >> +               .data = &axp223_data,
> >>         }, {
> >>                 .compatible = "x-powers,axp813-usb-power-supply",
> >> -               .data = (void *)AXP813_ID,
> >> +               .data = &axp813_data,
> >>         }, { /* sentinel */ }
> >>  };
> >>  MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
> >> --
> >> 2.23.0
> >
> > Otherwise,
> >
> > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> >
>

[PATCH v2 7/9] power: supply: axp20x_usb_power: Allow offlining

[Samuel Holland]

AXP803/AXP813 have a flag that enables/disables the USB power supply
input. Allow control of this flag via the ONLINE property on those
variants.

It may be necessary to offline the USB power supply input when using
the USB port in OTG mode, or to allow userspace to disable charging.

When the USB VBUS input is disabled via the PATH_SEL bit, the VBUS_USED
bit in PWR_INPUT_STATUS is cleared, so there is no change needed when
getting the property.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/power/supply/axp20x_usb_power.c | 27 +++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index 2d7272e19a87..68443f264dff 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -29,6 +29,9 @@
 
 #define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
 
+#define AXP20X_VBUS_PATH_SEL		BIT(7)
+#define AXP20X_VBUS_PATH_SEL_OFFSET	7
+
 #define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
 #define AXP20X_VBUS_VHOLD_MASK		GENMASK(5, 3)
 #define AXP20X_VBUS_VHOLD_OFFSET	3
@@ -263,6 +266,16 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static int axp813_usb_power_set_online(struct axp20x_usb_power *power,
+				       int intval)
+{
+	int val = !intval << AXP20X_VBUS_PATH_SEL_OFFSET;
+
+	return regmap_update_bits(power->regmap,
+				  AXP20X_VBUS_IPSOUT_MGMT,
+				  AXP20X_VBUS_PATH_SEL, val);
+}
+
 static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
 					    int intval)
 {
@@ -344,6 +357,9 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
 	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
 
 	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+		return axp813_usb_power_set_online(power, val->intval);
+
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
 		return axp20x_usb_power_set_voltage_min(power, val->intval);
 
@@ -363,6 +379,17 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
 static int axp20x_usb_power_prop_writeable(struct power_supply *psy,
 					   enum power_supply_property psp)
 {
+	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
+
+	/*
+	 * Both AXP2xx and AXP8xx have a VBUS path select flag.
+	 * On AXP2xx, setting the flag enables VBUS (ignoring N_VBUSEN).
+	 * On AXP8xx, setting the flag disables VBUS (ignoring N_VBUSEN).
+	 * So we only expose the control on AXP8xx where it is meaningful.
+	 */
+	if (psp == POWER_SUPPLY_PROP_ONLINE)
+		return power->axp20x_id == AXP813_ID;
+
 	return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
 	       psp == POWER_SUPPLY_PROP_CURRENT_MAX;
 }
-- 
2.23.0

Re: [linux-sunxi] [PATCH v2 7/9] power: supply: axp20x_usb_power: Allow offlining

[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 USB power supply
> input. Allow control of this flag via the ONLINE property on those
> variants.
>
> It may be necessary to offline the USB power supply input when using
> the USB port in OTG mode, or to allow userspace to disable charging.

Any idea how the former would be implemented? AFAIK this isn't allowed
right now.

As for disabling charging, wouldn't it make more sense to disable the
charger?

Either way, these are not directly related to the changes. I'm just curious.

> When the USB VBUS input is disabled via the PATH_SEL bit, the VBUS_USED
> bit in PWR_INPUT_STATUS is cleared, so there is no change needed when
> getting the property.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/power/supply/axp20x_usb_power.c | 27 +++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> index 2d7272e19a87..68443f264dff 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -29,6 +29,9 @@
>
>  #define AXP20X_USB_STATUS_VBUS_VALID   BIT(2)
>
> +#define AXP20X_VBUS_PATH_SEL           BIT(7)
> +#define AXP20X_VBUS_PATH_SEL_OFFSET    7
> +
>  #define AXP20X_VBUS_VHOLD_uV(b)                (4000000 + (((b) >> 3) & 7) * 100000)
>  #define AXP20X_VBUS_VHOLD_MASK         GENMASK(5, 3)
>  #define AXP20X_VBUS_VHOLD_OFFSET       3
> @@ -263,6 +266,16 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>         return 0;
>  }
>
> +static int axp813_usb_power_set_online(struct axp20x_usb_power *power,
> +                                      int intval)
> +{
> +       int val = !intval << AXP20X_VBUS_PATH_SEL_OFFSET;
> +
> +       return regmap_update_bits(power->regmap,
> +                                 AXP20X_VBUS_IPSOUT_MGMT,
> +                                 AXP20X_VBUS_PATH_SEL, val);
> +}
> +
>  static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
>                                             int intval)
>  {
> @@ -344,6 +357,9 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
>         struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
>
>         switch (psp) {
> +       case POWER_SUPPLY_PROP_ONLINE:
> +               return axp813_usb_power_set_online(power, val->intval);
> +

I would add a comment here pointing to the next change as to why there's
only an axp813-specific callback used here.

>         case POWER_SUPPLY_PROP_VOLTAGE_MIN:
>                 return axp20x_usb_power_set_voltage_min(power, val->intval);
>
> @@ -363,6 +379,17 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
>  static int axp20x_usb_power_prop_writeable(struct power_supply *psy,
>                                            enum power_supply_property psp)
>  {
> +       struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> +
> +       /*
> +        * Both AXP2xx and AXP8xx have a VBUS path select flag.
> +        * On AXP2xx, setting the flag enables VBUS (ignoring N_VBUSEN).
> +        * On AXP8xx, setting the flag disables VBUS (ignoring N_VBUSEN).
> +        * So we only expose the control on AXP8xx where it is meaningful.
> +        */
> +       if (psp == POWER_SUPPLY_PROP_ONLINE)
> +               return power->axp20x_id == AXP813_ID;
> +
>         return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
>                psp == POWER_SUPPLY_PROP_CURRENT_MAX;
>  }
> --

Otherwise,

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

Re: [linux-sunxi] [PATCH v2 7/9] power: supply: axp20x_usb_power: Allow offlining

[Samuel Holland]

On 1/5/20 4:40 AM, Chen-Yu Tsai wrote:
> On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
>>
>> AXP803/AXP813 have a flag that enables/disables the USB power supply
>> input. Allow control of this flag via the ONLINE property on those
>> variants.
>>
>> It may be necessary to offline the USB power supply input when using
>> the USB port in OTG mode, or to allow userspace to disable charging.
> 
> Any idea how the former would be implemented? AFAIK this isn't allowed
> right now.

Pinephone currently has AXP N_VBUSEN/DRIVEVBUS floating, so the hardware doesn't
automatically disable the VBUS path when enabling the boost regulator driving
it. This doubles the current draw from the battery.

The USB PHY driver would need to call:

    union power_supply_propval val = { .intval = false };
    power_supply_set_property(data->vbus_power_supply,
                              POWER_SUPPLY_PROP_ONLINE, &val);

or similar to set VBUS offline in sun4i_usb_phy_power_on(), and set it back
online in sun4i_usb_phy_power_off().

> As for disabling charging, wouldn't it make more sense to disable the
> charger?

Yes, I see now that there's a bit at 33H[7] for this. I don't see an obvious
property to hook it up to, though. Maybe POWER_SUPPLY_PROP_CHARGE_TYPE ==
POWER_SUPPLY_CHARGE_TYPE_NONE?

> Either way, these are not directly related to the changes. I'm just curious.
> 
>> When the USB VBUS input is disabled via the PATH_SEL bit, the VBUS_USED
>> bit in PWR_INPUT_STATUS is cleared, so there is no change needed when
>> getting the property.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  drivers/power/supply/axp20x_usb_power.c | 27 +++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
>> index 2d7272e19a87..68443f264dff 100644
>> --- a/drivers/power/supply/axp20x_usb_power.c
>> +++ b/drivers/power/supply/axp20x_usb_power.c
>> @@ -29,6 +29,9 @@
>>
>>  #define AXP20X_USB_STATUS_VBUS_VALID   BIT(2)
>>
>> +#define AXP20X_VBUS_PATH_SEL           BIT(7)
>> +#define AXP20X_VBUS_PATH_SEL_OFFSET    7
>> +
>>  #define AXP20X_VBUS_VHOLD_uV(b)                (4000000 + (((b) >> 3) & 7) * 100000)
>>  #define AXP20X_VBUS_VHOLD_MASK         GENMASK(5, 3)
>>  #define AXP20X_VBUS_VHOLD_OFFSET       3
>> @@ -263,6 +266,16 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>>         return 0;
>>  }
>>
>> +static int axp813_usb_power_set_online(struct axp20x_usb_power *power,
>> +                                      int intval)
>> +{
>> +       int val = !intval << AXP20X_VBUS_PATH_SEL_OFFSET;
>> +
>> +       return regmap_update_bits(power->regmap,
>> +                                 AXP20X_VBUS_IPSOUT_MGMT,
>> +                                 AXP20X_VBUS_PATH_SEL, val);
>> +}
>> +
>>  static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
>>                                             int intval)
>>  {
>> @@ -344,6 +357,9 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
>>         struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
>>
>>         switch (psp) {
>> +       case POWER_SUPPLY_PROP_ONLINE:
>> +               return axp813_usb_power_set_online(power, val->intval);
>> +
> 
> I would add a comment here pointing to the next change as to why there's
> only an axp813-specific callback used here.

I'll add this for v3.

>>         case POWER_SUPPLY_PROP_VOLTAGE_MIN:
>>                 return axp20x_usb_power_set_voltage_min(power, val->intval);
>>
>> @@ -363,6 +379,17 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
>>  static int axp20x_usb_power_prop_writeable(struct power_supply *psy,
>>                                            enum power_supply_property psp)
>>  {
>> +       struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
>> +
>> +       /*
>> +        * Both AXP2xx and AXP8xx have a VBUS path select flag.
>> +        * On AXP2xx, setting the flag enables VBUS (ignoring N_VBUSEN).
>> +        * On AXP8xx, setting the flag disables VBUS (ignoring N_VBUSEN).
>> +        * So we only expose the control on AXP8xx where it is meaningful.
>> +        */
>> +       if (psp == POWER_SUPPLY_PROP_ONLINE)
>> +               return power->axp20x_id == AXP813_ID;
>> +
>>         return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
>>                psp == POWER_SUPPLY_PROP_CURRENT_MAX;
>>  }
>> --
> 
> Otherwise,
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> 

Re: [linux-sunxi] [PATCH v2 7/9] power: supply: axp20x_usb_power: Allow offlining

[Chen-Yu Tsai]

On Mon, Jan 6, 2020 at 1:47 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 1/5/20 4:40 AM, Chen-Yu Tsai wrote:
> > On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
> >>
> >> AXP803/AXP813 have a flag that enables/disables the USB power supply
> >> input. Allow control of this flag via the ONLINE property on those
> >> variants.
> >>
> >> It may be necessary to offline the USB power supply input when using
> >> the USB port in OTG mode, or to allow userspace to disable charging.
> >
> > Any idea how the former would be implemented? AFAIK this isn't allowed
> > right now.
>
> Pinephone currently has AXP N_VBUSEN/DRIVEVBUS floating, so the hardware doesn't
> automatically disable the VBUS path when enabling the boost regulator driving
> it. This doubles the current draw from the battery.
>
> The USB PHY driver would need to call:
>
>     union power_supply_propval val = { .intval = false };
>     power_supply_set_property(data->vbus_power_supply,
>                               POWER_SUPPLY_PROP_ONLINE, &val);
>
> or similar to set VBUS offline in sun4i_usb_phy_power_on(), and set it back
> online in sun4i_usb_phy_power_off().

Ah, OK. That's a valid use case. I had something else in mind, the OTG host
mode with charger one.

> > As for disabling charging, wouldn't it make more sense to disable the
> > charger?
>
> Yes, I see now that there's a bit at 33H[7] for this. I don't see an obvious
> property to hook it up to, though. Maybe POWER_SUPPLY_PROP_CHARGE_TYPE ==
> POWER_SUPPLY_CHARGE_TYPE_NONE?

Maybe? I suppose the sysfs ABI docs would have some clues.

> > Either way, these are not directly related to the changes. I'm just curious.
> >
> >> When the USB VBUS input is disabled via the PATH_SEL bit, the VBUS_USED
> >> bit in PWR_INPUT_STATUS is cleared, so there is no change needed when
> >> getting the property.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>  drivers/power/supply/axp20x_usb_power.c | 27 +++++++++++++++++++++++++
> >>  1 file changed, 27 insertions(+)
> >>
> >> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> >> index 2d7272e19a87..68443f264dff 100644
> >> --- a/drivers/power/supply/axp20x_usb_power.c
> >> +++ b/drivers/power/supply/axp20x_usb_power.c
> >> @@ -29,6 +29,9 @@
> >>
> >>  #define AXP20X_USB_STATUS_VBUS_VALID   BIT(2)
> >>
> >> +#define AXP20X_VBUS_PATH_SEL           BIT(7)
> >> +#define AXP20X_VBUS_PATH_SEL_OFFSET    7
> >> +
> >>  #define AXP20X_VBUS_VHOLD_uV(b)                (4000000 + (((b) >> 3) & 7) * 100000)
> >>  #define AXP20X_VBUS_VHOLD_MASK         GENMASK(5, 3)
> >>  #define AXP20X_VBUS_VHOLD_OFFSET       3
> >> @@ -263,6 +266,16 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
> >>         return 0;
> >>  }
> >>
> >> +static int axp813_usb_power_set_online(struct axp20x_usb_power *power,
> >> +                                      int intval)
> >> +{
> >> +       int val = !intval << AXP20X_VBUS_PATH_SEL_OFFSET;
> >> +
> >> +       return regmap_update_bits(power->regmap,
> >> +                                 AXP20X_VBUS_IPSOUT_MGMT,
> >> +                                 AXP20X_VBUS_PATH_SEL, val);
> >> +}
> >> +
> >>  static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
> >>                                             int intval)
> >>  {
> >> @@ -344,6 +357,9 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
> >>         struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> >>
> >>         switch (psp) {
> >> +       case POWER_SUPPLY_PROP_ONLINE:
> >> +               return axp813_usb_power_set_online(power, val->intval);
> >> +
> >
> > I would add a comment here pointing to the next change as to why there's
> > only an axp813-specific callback used here.
>
> I'll add this for v3.

Thanks
ChenYu

> >>         case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> >>                 return axp20x_usb_power_set_voltage_min(power, val->intval);
> >>
> >> @@ -363,6 +379,17 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
> >>  static int axp20x_usb_power_prop_writeable(struct power_supply *psy,
> >>                                            enum power_supply_property psp)
> >>  {
> >> +       struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> >> +
> >> +       /*
> >> +        * Both AXP2xx and AXP8xx have a VBUS path select flag.
> >> +        * On AXP2xx, setting the flag enables VBUS (ignoring N_VBUSEN).
> >> +        * On AXP8xx, setting the flag disables VBUS (ignoring N_VBUSEN).
> >> +        * So we only expose the control on AXP8xx where it is meaningful.
> >> +        */
> >> +       if (psp == POWER_SUPPLY_PROP_ONLINE)
> >> +               return power->axp20x_id == AXP813_ID;
> >> +
> >>         return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
> >>                psp == POWER_SUPPLY_PROP_CURRENT_MAX;
> >>  }
> >> --
> >
> > Otherwise,
> >
> > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> >
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/f0c5e260-dcc3-2744-21cd-305e4534f2be%40sholland.org.

[PATCH v2 8/9] power: supply: axp20x_usb_power: Add wakeup control

[Samuel Holland]

The USB power supply input can be used as a wakeup source. Hook up the
VBUS_PLUGIN IRQ to trigger wakeup based on userspace configuration.

To do this, we must remember the list of IRQs for the life of the
device. To know how much space to allocate for the flexible array
member, we switch from using a NULL sentinel to using an array length.

Because we now depend on the specific order of the IRQs (we assume
VBUS_PLUGIN is first and always present), failing to acquire an IRQ
during probe must be a fatal error.

To avoid spuriously waking up the system when the USB power supply is
not configured as a wakeup source, we must explicitly disable all non-
wake IRQs during system suspend. This is because the SoC's NMI input is
shared among all IRQs on the AXP PMIC. Due to the use of regmap-irq, the
individual IRQs within the PMIC are nested threaded interrupts, and are
therefore not automatically disabled during system suspend.

The upshot is that if any other device within the MFD (such as the power
key) is an enabled wakeup source, all enabled IRQs within the PMIC will
cause wakeup. We still need to call enable_irq_wake() when we *do* want
wakeup, in case those other wakeup sources on the PMIC are all disabled.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/power/supply/axp20x_usb_power.c | 80 ++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 15 deletions(-)

diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index 68443f264dff..0d033954c4dc 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
 #include <linux/power_supply.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
@@ -67,6 +68,8 @@ struct axp20x_usb_power {
 	struct iio_channel *vbus_i;
 	struct delayed_work vbus_detect;
 	unsigned int old_status;
+	unsigned int num_irqs;
+	unsigned int irqs[];
 };
 
 static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
@@ -437,45 +440,85 @@ static const char * const axp20x_irq_names[] = {
 	"VBUS_REMOVAL",
 	"VBUS_VALID",
 	"VBUS_NOT_VALID",
-	NULL
 };
 
 static const char * const axp22x_irq_names[] = {
 	"VBUS_PLUGIN",
 	"VBUS_REMOVAL",
-	NULL
 };
 
 struct axp_data {
 	const struct power_supply_desc	*power_desc;
 	const char * const		*irq_names;
+	unsigned int			num_irq_names;
 	enum axp20x_variants		axp20x_id;
 };
 
 static const struct axp_data axp202_data = {
 	.power_desc	= &axp20x_usb_power_desc,
 	.irq_names	= axp20x_irq_names,
+	.num_irq_names	= ARRAY_SIZE(axp20x_irq_names),
 	.axp20x_id	= AXP202_ID,
 };
 
 static const struct axp_data axp221_data = {
 	.power_desc	= &axp22x_usb_power_desc,
 	.irq_names	= axp22x_irq_names,
+	.num_irq_names	= ARRAY_SIZE(axp22x_irq_names),
 	.axp20x_id	= AXP221_ID,
 };
 
 static const struct axp_data axp223_data = {
 	.power_desc	= &axp22x_usb_power_desc,
 	.irq_names	= axp22x_irq_names,
+	.num_irq_names	= ARRAY_SIZE(axp22x_irq_names),
 	.axp20x_id	= AXP223_ID,
 };
 
 static const struct axp_data axp813_data = {
 	.power_desc	= &axp22x_usb_power_desc,
 	.irq_names	= axp22x_irq_names,
+	.num_irq_names	= ARRAY_SIZE(axp22x_irq_names),
 	.axp20x_id	= AXP813_ID,
 };
 
+#ifdef CONFIG_PM_SLEEP
+static int axp20x_usb_power_suspend(struct device *dev)
+{
+	struct axp20x_usb_power *power = dev_get_drvdata(dev);
+	int i = 0;
+
+	/*
+	 * Allow wake via VBUS_PLUGIN only.
+	 *
+	 * As nested threaded IRQs are not automatically disabled during
+	 * suspend, we must explicitly disable the remainder of the IRQs.
+	 */
+	if (device_may_wakeup(&power->supply->dev))
+		enable_irq_wake(power->irqs[i++]);
+	while (i < power->num_irqs)
+		disable_irq(power->irqs[i++]);
+
+	return 0;
+}
+
+static int axp20x_usb_power_resume(struct device *dev)
+{
+	struct axp20x_usb_power *power = dev_get_drvdata(dev);
+	int i = 0;
+
+	if (device_may_wakeup(&power->supply->dev))
+		disable_irq_wake(power->irqs[i++]);
+	while (i < power->num_irqs)
+		enable_irq(power->irqs[i++]);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(axp20x_usb_power_pm_ops, axp20x_usb_power_suspend,
+						  axp20x_usb_power_resume);
+
 static int configure_iio_channels(struct platform_device *pdev,
 				  struct axp20x_usb_power *power)
 {
@@ -524,12 +567,15 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 
 	axp_data = of_device_get_match_data(&pdev->dev);
 
-	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
+	power = devm_kzalloc(&pdev->dev,
+			     struct_size(power, irqs, axp_data->num_irq_names),
+			     GFP_KERNEL);
 	if (!power)
 		return -ENOMEM;
 
 	power->regmap = axp20x->regmap;
 	power->axp20x_id = axp_data->axp20x_id;
+	power->num_irqs = axp_data->num_irq_names;
 
 	platform_set_drvdata(pdev, power);
 
@@ -566,19 +612,22 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 		return PTR_ERR(power->supply);
 
 	/* Request irqs after registering, as irqs may trigger immediately */
-	for (i = 0; axp_data->irq_names[i]; i++) {
+	for (i = 0; i < axp_data->num_irq_names; i++) {
 		irq = platform_get_irq_byname(pdev, axp_data->irq_names[i]);
 		if (irq < 0) {
-			dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
-				 axp_data->irq_names[i], irq);
-			continue;
+			dev_err(&pdev->dev, "No IRQ for %s: %d\n",
+				axp_data->irq_names[i], irq);
+			return irq;
+		}
+		power->irqs[i] = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
+		ret = devm_request_any_context_irq(&pdev->dev, power->irqs[i],
+						   axp20x_usb_power_irq, 0,
+						   DRVNAME, power);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "Error requesting %s IRQ: %d\n",
+				axp_data->irq_names[i], ret);
+			return ret;
 		}
-		irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
-		ret = devm_request_any_context_irq(&pdev->dev, irq,
-				axp20x_usb_power_irq, 0, DRVNAME, power);
-		if (ret < 0)
-			dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
-				 axp_data->irq_names[i], ret);
 	}
 
 	INIT_DELAYED_WORK(&power->vbus_detect, axp20x_usb_power_poll_vbus);
@@ -618,8 +667,9 @@ static struct platform_driver axp20x_usb_power_driver = {
 	.probe = axp20x_usb_power_probe,
 	.remove = axp20x_usb_power_remove,
 	.driver = {
-		.name = DRVNAME,
-		.of_match_table = axp20x_usb_power_match,
+		.name		= DRVNAME,
+		.of_match_table	= axp20x_usb_power_match,
+		.pm		= &axp20x_usb_power_pm_ops,
 	},
 };
 
-- 
2.23.0

Re: [linux-sunxi] [PATCH v2 8/9] power: supply: axp20x_usb_power: Add wakeup control

[Chen-Yu Tsai]

On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
>
> The USB power supply input can be used as a wakeup source. Hook up the
> VBUS_PLUGIN IRQ to trigger wakeup based on userspace configuration.
>
> To do this, we must remember the list of IRQs for the life of the
> device. To know how much space to allocate for the flexible array
> member, we switch from using a NULL sentinel to using an array length.
>
> Because we now depend on the specific order of the IRQs (we assume
> VBUS_PLUGIN is first and always present), failing to acquire an IRQ
> during probe must be a fatal error.
>
> To avoid spuriously waking up the system when the USB power supply is
> not configured as a wakeup source, we must explicitly disable all non-
> wake IRQs during system suspend. This is because the SoC's NMI input is
> shared among all IRQs on the AXP PMIC. Due to the use of regmap-irq, the
> individual IRQs within the PMIC are nested threaded interrupts, and are
> therefore not automatically disabled during system suspend.
>
> The upshot is that if any other device within the MFD (such as the power
> key) is an enabled wakeup source, all enabled IRQs within the PMIC will
> cause wakeup. We still need to call enable_irq_wake() when we *do* want
> wakeup, in case those other wakeup sources on the PMIC are all disabled.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

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

[PATCH v2 9/9] power: supply: axp20x_usb_power: Only poll while offline

[Samuel Holland]

Investigation on the AXP803 shows that VBUS_PLUGIN/VBUS_REMOVAL IRQs are
triggered on the rising/falling edge of AXP20X_PWR_STATUS_VBUS_USED. The
reason IRQs do not arrive while N_VBUSEN/DRIVEVBUS is high is because
AXP20X_PWR_STATUS_VBUS_USED also never goes high.

This also means that if VBUS is online, a VBUS_REMOVAL IRQ is received
immediately on setting N_VBUSEN/DRIVEVBUS high (and VBUS_PLUGIN shortly
after it is set back low). This was also verified to be the case when
manually offlining VBUS through AXP20X_VBUS_PATH_SELECT.

As long as VBUS is online, a present->absent transition necessarily
implies an online->offline transition. Since will cause an IRQ, there is
no need to poll while VBUS is online.

To ensure the driver's view of VBUS online status remains accurate,
unconditionally poll once when receiving an IRQ and when resuming. If
VBUS is still online at that time, polling will cease until the next
VBUS_REMOVAL IRQ.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/power/supply/axp20x_usb_power.c | 30 +++++++++++++++++--------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index 0d033954c4dc..4bf119082e91 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -68,16 +68,32 @@ struct axp20x_usb_power {
 	struct iio_channel *vbus_i;
 	struct delayed_work vbus_detect;
 	unsigned int old_status;
+	unsigned int online;
 	unsigned int num_irqs;
 	unsigned int irqs[];
 };
 
+static bool axp20x_usb_vbus_needs_polling(struct axp20x_usb_power *power)
+{
+	/*
+	 * Polling is only necessary while VBUS is offline. While online, a
+	 * present->absent transition implies an online->offline transition
+	 * and will triger the VBUS_REMOVAL IRQ.
+	 */
+	if (power->axp20x_id >= AXP221_ID && !power->online)
+		return true;
+
+	return false;
+}
+
 static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
 {
 	struct axp20x_usb_power *power = devid;
 
 	power_supply_changed(power->supply);
 
+	mod_delayed_work(system_wq, &power->vbus_detect, DEBOUNCE_TIME);
+
 	return IRQ_HANDLED;
 }
 
@@ -97,17 +113,11 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
 		power_supply_changed(power->supply);
 
 	power->old_status = val;
+	power->online = val & AXP20X_PWR_STATUS_VBUS_USED;
 
 out:
-	mod_delayed_work(system_wq, &power->vbus_detect, DEBOUNCE_TIME);
-}
-
-static bool axp20x_usb_vbus_needs_polling(struct axp20x_usb_power *power)
-{
-	if (power->axp20x_id >= AXP221_ID)
-		return true;
-
-	return false;
+	if (axp20x_usb_vbus_needs_polling(power))
+		mod_delayed_work(system_wq, &power->vbus_detect, DEBOUNCE_TIME);
 }
 
 static int axp20x_get_current_max(struct axp20x_usb_power *power, int *val)
@@ -512,6 +522,8 @@ static int axp20x_usb_power_resume(struct device *dev)
 	while (i < power->num_irqs)
 		enable_irq(power->irqs[i++]);
 
+	mod_delayed_work(system_wq, &power->vbus_detect, DEBOUNCE_TIME);
+
 	return 0;
 }
 #endif
-- 
2.23.0

Re: [linux-sunxi] [PATCH v2 9/9] power: supply: axp20x_usb_power: Only poll while offline

[Chen-Yu Tsai]

On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote:
>
> Investigation on the AXP803 shows that VBUS_PLUGIN/VBUS_REMOVAL IRQs are
> triggered on the rising/falling edge of AXP20X_PWR_STATUS_VBUS_USED. The
> reason IRQs do not arrive while N_VBUSEN/DRIVEVBUS is high is because
> AXP20X_PWR_STATUS_VBUS_USED also never goes high.
>
> This also means that if VBUS is online, a VBUS_REMOVAL IRQ is received
> immediately on setting N_VBUSEN/DRIVEVBUS high (and VBUS_PLUGIN shortly
> after it is set back low). This was also verified to be the case when
> manually offlining VBUS through AXP20X_VBUS_PATH_SELECT.
>
> As long as VBUS is online, a present->absent transition necessarily
> implies an online->offline transition. Since will cause an IRQ, there is
> no need to poll while VBUS is online.
>
> To ensure the driver's view of VBUS online status remains accurate,
> unconditionally poll once when receiving an IRQ and when resuming. If
> VBUS is still online at that time, polling will cease until the next
> VBUS_REMOVAL IRQ.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

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