Re: [RFC PATCH v3 6/7] gpiolib: enable GPIO interrupt to wake up a system from sleep

[Linus Walleij]

On Fri, May 3, 2024 at 1:15 PM Alex Soo <yuklin.soo@xxxxxxxxxxxxxxxx> wrote:

> Add function gpiochip_wakeup_irq_setup() to configure and enable a
> GPIO pin with interrupt wakeup capability according to user-defined
> wakeup-gpios property in the device tree. Interrupt generated by
> toggling the logic level (rising/falling edge) on the specified
> GPIO pin can wake up a system from sleep mode.
>
> Signed-off-by: Alex Soo <yuklin.soo@xxxxxxxxxxxxxxxx>

This is a very helpful patch I think.

I'm looking forward to the next iteration.

> @@ -1045,8 +1047,15 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>                 if (ret)
>                         goto err_remove_irqchip;
>         }
> +
> +       ret = gpiochip_wakeup_irq_setup(gc);
> +       if (ret)
> +               goto err_remove_device;

Do we have any in-tree drivers that do this by themselves already?

In that case we should convert them to use this function in the same
patch to avoid regressions.

> +static irqreturn_t gpio_wake_irq_handler(int irq, void *data)
> +{
> +       struct irq_data *irq_data = data;

I'm minimalist so I usually just call the parameter "d" instead of "data"
and irq_data I would call *id but it's your pick.

> +
> +       if (!irq_data || irq != irq_data->irq)
> +               return IRQ_NONE;
> +
> +       return IRQ_HANDLED;

Please add some debug print:

struct gpio_chip *gc = irq_data->chip_data;

chip_dbg(gc, "GPIO wakeup on IRQ %d\n", irq);

The rest looks good to me (after fixing Andy's comments!)

I would perhaps put some
debug print that "GPIO wakeup enabled at offset %d" in the
end as well, so people can easily follow this in the debug prints.

Yours,
Linus Walleij