Messages in this thread |  | | From | Chen-Yu Tsai <> | Date | Sun, 5 Jan 2020 18:24:42 +0800 | Subject | Re: [linux-sunxi] [PATCH v2 4/9] power: supply: axp20x_ac_power: Add wakeup control |
| |
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>
|  |