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
