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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxx>