[[RFC PATCH]] gpio: gpio-mxc: make sure gpio is input when request IRQ

From: Markus Niebel <redacted>

When requesting an GPIO irq for imx Soc two things are missing:
- there is no check, if the GPIO is already requested
- there is no check, if the GPIO is configured as input

The first case can lead to reconfiguring the GPIO pin from third
party while it is used as IRQ pin, second case will (eventually)
prevent IRQ from being seen by SOC becaus the pin is driven by
Soc

This patch tries to implement (logic taken roughly from gpio-omap)
- basic check if gpio already requested
- if needed requests the gpio and configures as IN.
- if gpio is already requested it is only verified if pin is IN
- gpio is locked as irq

Tested on a not mainlined i.MX6 based hardware with pin configured
by bootloader as OUT HIGH and expecting a low active IRQ.

Signed-off-by: Markus Niebel <redacted>
---
 drivers/gpio/gpio-mxc.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index db83b3c..4316a38 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c

@@ -175,6 +175,31 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 	u32 gpio = port->bgc.gc.base + gpio_idx;
 	int edge;
 	void __iomem *reg = port->base;
+	int ret = 0;
+
+	if (!gpiochip_is_requested(&port->bgc.gc, gpio_idx)) {
+		char label[32];
+
+		snprintf(label, 32, "gpio%u-irq", gpio);
+		ret = gpio_request_one(gpio, GPIOF_DIR_IN, label);
+	} else {
+		val = readl(port->base + GPIO_GDIR);
+		if (val & BIT(gpio_idx))
+			ret = -EINVAL;
+	}
+
+	if (ret) {
+		dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",
+			gpio_idx);
+		return ret;
+	}
+
+	ret = gpio_lock_as_irq(&port->bgc.gc, gpio_idx);
+	if (ret) {
+		dev_err(port->bgc.gc.dev, "unable to lock gpio_idx %u for IRQ\n",
+			gpio_idx);
+		return ret;
+	}
 
 	port->both_edges &= ~(1 << gpio_idx);
 	switch (type) {

@@ -231,6 +256,15 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 	return 0;
 }
 
+static void gpio_irq_shutdown(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct mxc_gpio_port *port = gc->private;
+	u32 gpio_idx = d->hwirq;
+
+	gpio_unlock_as_irq(&port->bgc.gc, gpio_idx);
+}
+
 static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
 {
 	void __iomem *reg = port->base;

@@ -353,6 +387,7 @@ static void __init mxc_gpio_init_gc(struct mxc_gpio_port *port, int irq_base)
 	ct->chip.irq_mask = irq_gc_mask_clr_bit;
 	ct->chip.irq_unmask = irq_gc_mask_set_bit;
 	ct->chip.irq_set_type = gpio_set_irq_type;
+	ct->chip.irq_shutdown = gpio_irq_shutdown;
 	ct->chip.irq_set_wake = gpio_set_wake_irq;
 	ct->regs.ack = GPIO_ISR;
 	ct->regs.mask = GPIO_IMR;

-- 
1.7.9.5

On Wed, Jul 16, 2014 at 03:51:04PM +0200, Markus Niebel wrote:

I'm not sure it's correct to call gpio_request_one() from .set_irq_type
hook.  It looks like a API usage violation to me.  It should really be
called from client driver.

+	} else {
+		val = readl(port->base + GPIO_GDIR);
+		if (val & BIT(gpio_idx))
+			ret = -EINVAL;

It says that the GPIO is requested by someone, but we're not really sure
if it's the correct one, i.e. the one is requesting set_irq_type.

+	}
+
+	if (ret) {
+		dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",
+			gpio_idx);
+		return ret;
+	}

Having said that, I'm not sure any above changes is really necessary.
If any, I would say only gpiochip_is_requested() check makes some sense,
but we should just fail out if the GPIO hasn't been requested.  Nothing
more can be done in there.

+
+	ret = gpio_lock_as_irq(&port->bgc.gc, gpio_idx);
+	if (ret) {
+		dev_err(port->bgc.gc.dev, "unable to lock gpio_idx %u for IRQ\n",
+			gpio_idx);
+		return ret;
+	}

This and the following changes do make sense to me.

Shawn

Am 22.07.2014 08:28, wrote Shawn Guo:

On Wed, Jul 16, 2014 at 03:51:04PM +0200, Markus Niebel wrote:

quoted

From: Markus Niebel <redacted>

When requesting an GPIO irq for imx Soc two things are missing:
- there is no check, if the GPIO is already requested
- there is no check, if the GPIO is configured as input

The first case can lead to reconfiguring the GPIO pin from third
party while it is used as IRQ pin, second case will (eventually)
prevent IRQ from being seen by SOC becaus the pin is driven by
Soc

This patch tries to implement (logic taken roughly from gpio-omap)
- basic check if gpio already requested
- if needed requests the gpio and configures as IN.
- if gpio is already requested it is only verified if pin is IN
- gpio is locked as irq

Tested on a not mainlined i.MX6 based hardware with pin configured
by bootloader as OUT HIGH and expecting a low active IRQ.

Signed-off-by: Markus Niebel <redacted>
---
 drivers/gpio/gpio-mxc.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index db83b3c..4316a38 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c

@@ -175,6 +175,31 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 	u32 gpio = port->bgc.gc.base + gpio_idx;
 	int edge;
 	void __iomem *reg = port->base;
+	int ret = 0;
+
+	if (!gpiochip_is_requested(&port->bgc.gc, gpio_idx)) {
+		char label[32];
+
+		snprintf(label, 32, "gpio%u-irq", gpio);
+		ret = gpio_request_one(gpio, GPIOF_DIR_IN, label);

I'm not sure it's correct to call gpio_request_one() from .set_irq_type
hook.  It looks like a API usage violation to me.  It should really be
called from client driver.

Thats why it is an RFC. I add Linus Walleij to the cc-list.

Let me describe the problem:

Currently client drivers have simply interrupts and interrupt-parent
in their bindings, but no interrupt-gpios. Therefore in this case a
client does not know about a dedicated gpio which is to be requested
and configured.

quoted

+	} else {
+		val = readl(port->base + GPIO_GDIR);
+		if (val & BIT(gpio_idx))
+			ret = -EINVAL;

It says that the GPIO is requested by someone, but we're not really sure
if it's the correct one, i.e. the one is requesting set_irq_type.

Yes, but the current situation is even worse (in my eyes): an IRQ can be requested and an
independend party can request and configure the gpio as output ...

quoted

+	}
+
+	if (ret) {
+		dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",
+			gpio_idx);
+		return ret;
+	}

Having said that, I'm not sure any above changes is really necessary.
If any, I would say only gpiochip_is_requested() check makes some sense,
but we should just fail out if the GPIO hasn't been requested.  Nothing
more can be done in there.

Going that way as a consequence a reworked device tree binding for gpio irq is needed
(just like in the platform data days) when providing a gpio number and the client has
to request gpio and irq - correct me if I'm wrong.

Maybe here is something needed with deeper knowledge in the gpio subsystem.

quoted

+
+	ret = gpio_lock_as_irq(&port->bgc.gc, gpio_idx);
+	if (ret) {
+		dev_err(port->bgc.gc.dev, "unable to lock gpio_idx %u for IRQ\n",
+			gpio_idx);
+		return ret;
+	}

This and the following changes do make sense to me.

Shawn

quoted

 
 	port->both_edges &= ~(1 << gpio_idx);
 	switch (type) {

@@ -231,6 +256,15 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 	return 0;
 }
 
+static void gpio_irq_shutdown(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct mxc_gpio_port *port = gc->private;
+	u32 gpio_idx = d->hwirq;
+
+	gpio_unlock_as_irq(&port->bgc.gc, gpio_idx);
+}
+
 static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
 {
 	void __iomem *reg = port->base;

@@ -353,6 +387,7 @@ static void __init mxc_gpio_init_gc(struct mxc_gpio_port *port, int irq_base)
 	ct->chip.irq_mask = irq_gc_mask_clr_bit;
 	ct->chip.irq_unmask = irq_gc_mask_set_bit;
 	ct->chip.irq_set_type = gpio_set_irq_type;
+	ct->chip.irq_shutdown = gpio_irq_shutdown;
 	ct->chip.irq_set_wake = gpio_set_wake_irq;
 	ct->regs.ack = GPIO_ISR;
 	ct->regs.mask = GPIO_IMR;

-- 
1.7.9.5

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Jul 16, 2014 at 3:51 PM, Markus Niebel
[off-list ref] wrote:

Wut? Auto-request here? This can not be right.

+       } else {
+               val = readl(port->base + GPIO_GDIR);
+               if (val & BIT(gpio_idx))
+                       ret = -EINVAL;
+       }
+
+       if (ret) {
+               dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",
+                       gpio_idx);
+               return ret;
+       }
+
+       ret = gpio_lock_as_irq(&port->bgc.gc, gpio_idx);

This call should be done in the new
.irq_request_resources callback in struct irq_chip.

This call should be done in the new
.irq_release_resources callback in struct irq_chip.

Looking at the driver, it should be switched to using the new
irqchip helpers inside gpiolib, see e.g.
commit 7bcbce55f20e41c014df4d5d9c8448f7b5e49d79
"gpio: pca953x: use gpiolib irqchip helpers"

Yours,
Linus Walleij