[2/8] gpio: zynq: Wakeup gpio controller when it is used as IRQ controller

Submitted by Michal Simek on Aug. 7, 2017, 11:01 a.m.

Details

Message ID 72d3cd83bed792a23ab60cf9b6d51b618f5aa084.1502103715.git.michal.simek@xilinx.com
State New
Headers show

Commit Message

Michal Simek Aug. 7, 2017, 11:01 a.m.
From: Borsodi Petr <Petr.Borsodi@i.cz>

There is a problem with GPIO driver when used as IRQ controller.
It is not working because the module is sleeping (clock is disabled).
The patch enables clocks when IP is used as IRQ controller.

Signed-off-by: Borsodi Petr <Petr.Borsodi@i.cz>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 drivers/gpio/gpio-zynq.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

Comments

Linus Walleij Aug. 14, 2017, 1:53 p.m.
On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <michal.simek@xilinx.com> wrote:

> From: Borsodi Petr <Petr.Borsodi@i.cz>
>
> There is a problem with GPIO driver when used as IRQ controller.
> It is not working because the module is sleeping (clock is disabled).
> The patch enables clocks when IP is used as IRQ controller.
>
> Signed-off-by: Borsodi Petr <Petr.Borsodi@i.cz>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

I'm a bit worried about this patch.

> +static int zynq_gpio_irq_request_resources(struct irq_data *d)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +       int ret;
> +
> +       if (!try_module_get(chip->gpiodev->owner))
> +               return -ENODEV;

You are poking around in gpiolib internals, I don't really like that.
I prefer that you use accessors and try to make the core deal with
this instead of fixing it up with a local hack in the driver.

> +       ret = pm_runtime_get_sync(chip->parent);
> +       if (ret < 0) {
> +               module_put(chip->gpiodev->owner);
> +               return ret;
> +       }

What you essentially do is disable runtime PM while IRQs are in
use, the patch commit log should say this.

> +       if (gpiochip_lock_as_irq(chip, d->hwirq)) {
> +               chip_err(chip, "unable to lock HW IRQ %lu for IRQ\n", d->hwirq);
> +               pm_runtime_put(chip->parent);
> +               module_put(chip->gpiodev->owner);
> +               return -EINVAL;
> +       }

This is essentially a separate patch that should be done orthogonally.
(I don't care super-much about that though.)

> +static void zynq_gpio_irq_release_resources(struct irq_data *d)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +
> +       gpiochip_unlock_as_irq(chip, d->hwirq);
> +       pm_runtime_put(chip->parent);
> +       module_put(chip->gpiodev->owner);
> +}
(...)
> +       .irq_request_resources = zynq_gpio_irq_request_resources,
> +       .irq_release_resources = zynq_gpio_irq_release_resources,

Look at this from gpiolib.c:

static int gpiochip_irq_reqres(struct irq_data *d)
{
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);

        if (!try_module_get(chip->gpiodev->owner))
                return -ENODEV;

        if (gpiochip_lock_as_irq(chip, d->hwirq)) {
                chip_err(chip,
                        "unable to lock HW IRQ %lu for IRQ\n",
                        d->hwirq);
                module_put(chip->gpiodev->owner);
                return -EINVAL;
        }
        return 0;
}

static void gpiochip_irq_relres(struct irq_data *d)
{
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);

        gpiochip_unlock_as_irq(chip, d->hwirq);
        module_put(chip->gpiodev->owner);
}

If you add pm_runtime_get_sync()/put to this and export
the functions you have the same thing and you can just reuse this
code instead of copying it.

Arguably the above should indeed have that runtime PM code
(unless we know a better way to deal with IRQs).

So can we fix this in the core and reuse it from there?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 064033803449..5198fa6e016a 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -11,6 +11,7 @@ 
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/gpio.h>
 #include <linux/gpio/driver.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -20,6 +21,8 @@ 
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
 
+#include "gpiolib.h"
+
 #define DRIVER_NAME "zynq-gpio"
 
 /* Maximum banks */
@@ -498,6 +501,38 @@  static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
 	return 0;
 }
 
+static int zynq_gpio_irq_request_resources(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	int ret;
+
+	if (!try_module_get(chip->gpiodev->owner))
+		return -ENODEV;
+
+	ret = pm_runtime_get_sync(chip->parent);
+	if (ret < 0) {
+		module_put(chip->gpiodev->owner);
+		return ret;
+	}
+
+	if (gpiochip_lock_as_irq(chip, d->hwirq)) {
+		chip_err(chip, "unable to lock HW IRQ %lu for IRQ\n", d->hwirq);
+		pm_runtime_put(chip->parent);
+		module_put(chip->gpiodev->owner);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static void zynq_gpio_irq_release_resources(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	gpiochip_unlock_as_irq(chip, d->hwirq);
+	pm_runtime_put(chip->parent);
+	module_put(chip->gpiodev->owner);
+}
+
 /* irq chip descriptor */
 static struct irq_chip zynq_gpio_level_irqchip = {
 	.name		= DRIVER_NAME,
@@ -507,6 +542,8 @@  static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
 	.irq_unmask	= zynq_gpio_irq_unmask,
 	.irq_set_type	= zynq_gpio_set_irq_type,
 	.irq_set_wake	= zynq_gpio_set_wake,
+	.irq_request_resources = zynq_gpio_irq_request_resources,
+	.irq_release_resources = zynq_gpio_irq_release_resources,
 	.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED |
 			  IRQCHIP_MASK_ON_SUSPEND,
 };
@@ -519,6 +556,8 @@  static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
 	.irq_unmask	= zynq_gpio_irq_unmask,
 	.irq_set_type	= zynq_gpio_set_irq_type,
 	.irq_set_wake	= zynq_gpio_set_wake,
+	.irq_request_resources = zynq_gpio_irq_request_resources,
+	.irq_release_resources = zynq_gpio_irq_release_resources,
 	.flags		= IRQCHIP_MASK_ON_SUSPEND,
 };