| Message ID | 20240503111436.113089-7-yuklin.soo@starfivetech.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Fri, May 03, 2024 at 07:14:35PM +0800, Alex Soo kirjoitti: > 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. ... > +static irqreturn_t gpio_wake_irq_handler(int irq, void *data) > +{ > + struct irq_data *irq_data = data; > + > + if (!irq_data || irq != irq_data->irq) Hmm... When irq_data can be NULL? > + return IRQ_NONE; > + > + return IRQ_HANDLED; > +} ... > +static int gpiochip_wakeup_irq_setup(struct gpio_chip *gc) > +{ > + struct device *dev = gc->parent; > + struct gpio_irq_chip *girq = &gc->irq; > + struct gpio_desc *wakeup_gpiod; > + struct irq_desc *wakeup_irqd; > + struct irq_domain *irq_domain; > + struct irq_data *irq_data; > + unsigned int offset; > + int wakeup_irq; > + int ret; > + > + if (!(device_property_read_bool(dev, "wakeup-source"))) Too many parentheses. > + return 0; > + > + irq_domain = girq->domain; > + Unneeded blank line. Ditto for all similar cases. > + if (!irq_domain) { > + dev_err(dev, "Couldn't allocate IRQ domain\n"); > + return -ENXIO; return dev_err_probe(...); Ditto for all similar cases. > + } ... > + wakeup_gpiod = devm_gpiod_get_optional(dev, "wakeup", GPIOD_IN); > + > + if (IS_ERR(wakeup_gpiod)) { > + dev_err(dev, "invalid wakeup gpio: %lu\n", PTR_ERR(wakeup_gpiod)); > + return PTR_ERR(wakeup_gpiod); > + } > + if (!wakeup_gpiod) { > + dev_dbg(dev, "property wakeup-gpios is not defined\n"); > + return 0; > + } > + > + offset = gpio_chip_hwgpio(wakeup_gpiod); > + wakeup_irq = gpiod_to_irq(wakeup_gpiod); > + if (wakeup_irq < 0) { > + dev_err(dev, "failed to convert wakeup GPIO to IRQ\n"); > + return wakeup_irq; > + } > + irq_domain->ops->map(irq_domain, wakeup_irq, offset); > + wakeup_irqd = irq_to_desc(wakeup_irq); > + irq_data = irq_get_irq_data(wakeup_irq); What these lines are for? > + girq->handler = handle_edge_irq; > + > + if (!(wakeup_irqd->status_use_accessors & IRQ_NOREQUEST)) { > + device_init_wakeup(dev, 1); > + ret = devm_request_threaded_irq(dev, wakeup_irq, NULL, > + gpio_wake_irq_handler, > + IRQF_TRIGGER_FALLING | > + IRQF_TRIGGER_RISING | What's wrong with the flags from DT? > + IRQF_ONESHOT | > + IRQF_SHARED, > + "pm-wakeup-gpio", irq_data); > + if (ret) { > + dev_err(dev, "unable to request wakeup IRQ: %d\n", ret); > + return ret; > + } > + }
On Fri, May 3, 2024 at 1:15 PM Alex Soo <yuklin.soo@starfivetech.com> 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@starfivetech.com> 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
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 94903fc1c145..92cfbc34abb0 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -19,6 +19,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/pinctrl/consumer.h> +#include <linux/pm_wakeirq.h> #include <linux/seq_file.h> #include <linux/slab.h> #include <linux/spinlock.h> @@ -96,6 +97,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gc); static int gpiochip_irqchip_init_hw(struct gpio_chip *gc); static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gc); static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gc); +static int gpiochip_wakeup_irq_setup(struct gpio_chip *gc); static bool gpiolib_initialized; @@ -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; + return 0; +err_remove_device: + gcdev_unregister(gdev); err_remove_irqchip: gpiochip_irqchip_remove(gc); err_remove_irqchip_mask: @@ -1874,6 +1883,84 @@ static int gpiochip_irqchip_add_allocated_domain(struct gpio_chip *gc, return 0; } +static irqreturn_t gpio_wake_irq_handler(int irq, void *data) +{ + struct irq_data *irq_data = data; + + if (!irq_data || irq != irq_data->irq) + return IRQ_NONE; + + return IRQ_HANDLED; +} + +static int gpiochip_wakeup_irq_setup(struct gpio_chip *gc) +{ + struct device *dev = gc->parent; + struct gpio_irq_chip *girq = &gc->irq; + struct gpio_desc *wakeup_gpiod; + struct irq_desc *wakeup_irqd; + struct irq_domain *irq_domain; + struct irq_data *irq_data; + unsigned int offset; + int wakeup_irq; + int ret; + + if (!(device_property_read_bool(dev, "wakeup-source"))) + return 0; + + irq_domain = girq->domain; + + if (!irq_domain) { + dev_err(dev, "Couldn't allocate IRQ domain\n"); + return -ENXIO; + } + + wakeup_gpiod = devm_gpiod_get_optional(dev, "wakeup", GPIOD_IN); + + if (IS_ERR(wakeup_gpiod)) { + dev_err(dev, "invalid wakeup gpio: %lu\n", PTR_ERR(wakeup_gpiod)); + return PTR_ERR(wakeup_gpiod); + } + if (!wakeup_gpiod) { + dev_dbg(dev, "property wakeup-gpios is not defined\n"); + return 0; + } + + offset = gpio_chip_hwgpio(wakeup_gpiod); + wakeup_irq = gpiod_to_irq(wakeup_gpiod); + if (wakeup_irq < 0) { + dev_err(dev, "failed to convert wakeup GPIO to IRQ\n"); + return wakeup_irq; + } + irq_domain->ops->map(irq_domain, wakeup_irq, offset); + wakeup_irqd = irq_to_desc(wakeup_irq); + irq_data = irq_get_irq_data(wakeup_irq); + girq->handler = handle_edge_irq; + + if (!(wakeup_irqd->status_use_accessors & IRQ_NOREQUEST)) { + device_init_wakeup(dev, 1); + ret = devm_request_threaded_irq(dev, wakeup_irq, NULL, + gpio_wake_irq_handler, + IRQF_TRIGGER_FALLING | + IRQF_TRIGGER_RISING | + IRQF_ONESHOT | + IRQF_SHARED, + "pm-wakeup-gpio", irq_data); + if (ret) { + dev_err(dev, "unable to request wakeup IRQ: %d\n", ret); + return ret; + } + } + + ret = dev_pm_set_wake_irq(dev, wakeup_irq); + if (ret) { + dev_err(dev, "failed to enable gpio irq wake\n"); + return ret; + } + + return 0; +} + /** * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip * @gc: the GPIO chip to add the IRQ chip to
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@starfivetech.com> --- drivers/gpio/gpiolib.c | 87 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+)