2020-01-15 03:42:45

by Samuel Holland

[permalink] [raw]
Subject: [PATCH v4 0/4] X-Powers Power Supply Improvements

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

Patch 1 is preparation for changes in following patches.
Patch 2 allows userspace to take the power supply offline.
Patch 3 allows userspace to control the wakeup behavior.
Patch 4 avoids polling USB VBUS presence when possible.

Changes since v3:
- Rebase on power-supply/for-next
- Add Reviewed-by (1-2)

Changes since v2:
- Patch 1 was merged
- Only check ACIN_PATH_SEL when necessary (1)
- Update commit message (5)
- Avoided reordering lines until/unless necessary (5, 7)
- Update comment and add ID check in axp20x_usb_power_set_property
(it seemed more correct than adding another comment) (6)
- Add Reviewed-by where there were no comments (2-4, 7-8)

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 (4):
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/power/supply/axp20x_usb_power.c | 217 ++++++++++++++++++------
1 file changed, 169 insertions(+), 48 deletions(-)

--
2.23.0


2020-01-15 03:42:55

by Samuel Holland

[permalink] [raw]
Subject: [PATCH v4 3/4] power: supply: axp20x_usb_power: Add wakeup control

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.

Reviewed-by: Chen-Yu Tsai <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---
drivers/power/supply/axp20x_usb_power.c | 83 ++++++++++++++++++++-----
1 file changed, 67 insertions(+), 16 deletions(-)

diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index f14736041c41..1732304bc96e 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)
@@ -440,45 +443,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)
{
@@ -525,15 +568,19 @@ static int axp20x_usb_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;

platform_set_drvdata(pdev, power);

- axp_data = of_device_get_match_data(&pdev->dev);
power->axp20x_id = axp_data->axp20x_id;
power->regmap = axp20x->regmap;
+ power->num_irqs = axp_data->num_irq_names;

if (power->axp20x_id == AXP202_ID) {
/* Enable vbus valid checking */
@@ -568,19 +615,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);
@@ -620,8 +670,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

2020-01-15 21:31:11

by Sebastian Reichel

[permalink] [raw]
Subject: Re: [PATCH v4 0/4] X-Powers Power Supply Improvements

Hi,

Thanks, all queued to power-supply's for-next branch.

-- Sebastian

On Tue, Jan 14, 2020 at 09:40:44PM -0600, Samuel Holland wrote:
> This series adds some improvements to the axp20x_usb_power power supply
> driver to better support suspend/resume and use on mobile devices.
>
> Patch 1 is preparation for changes in following patches.
> Patch 2 allows userspace to take the power supply offline.
> Patch 3 allows userspace to control the wakeup behavior.
> Patch 4 avoids polling USB VBUS presence when possible.
>
> Changes since v3:
> - Rebase on power-supply/for-next
> - Add Reviewed-by (1-2)
>
> Changes since v2:
> - Patch 1 was merged
> - Only check ACIN_PATH_SEL when necessary (1)
> - Update commit message (5)
> - Avoided reordering lines until/unless necessary (5, 7)
> - Update comment and add ID check in axp20x_usb_power_set_property
> (it seemed more correct than adding another comment) (6)
> - Add Reviewed-by where there were no comments (2-4, 7-8)
>
> 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 (4):
> 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/power/supply/axp20x_usb_power.c | 217 ++++++++++++++++++------
> 1 file changed, 169 insertions(+), 48 deletions(-)
>
> --
> 2.23.0
>


Attachments:
(No filename) (1.72 kB)
signature.asc (849.00 B)
Download all attachments