] gpio: gpio-mxc: make sure gpio is input when request IRQ
Subsystems (2)
RFC PATCH 1/1 ] gpio: gpio-mxc: make sure gpio is input when request IRQ
From: Markus Niebel <[email protected]>
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.
--- drivers/gpio/gpio-mxc.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
drivers/gpio/gpio-mxc.c +35 -0
@@ -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 ;
Signature · 1 line
1.7.9.5
RFC PATCH 1/1 ] gpio: gpio-mxc: make sure gpio is input when request IRQ
Quoted 30 lines · Markus Niebel
From: Markus Niebel <[email protected]>
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 inputThe 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 irqTested 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 <[email protected]>--- drivers/gpio/gpio-mxc.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)drivers/gpio/gpio-mxc.c +0
-0
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 184
Quoted 10 lines
drivers/gpio/gpio-mxc.c +7
-0@@ -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.
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 188
Quoted 4 lines
drivers/gpio/gpio-mxc.c +4
-0+ }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.
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 195
Quoted 7 lines
drivers/gpio/gpio-mxc.c +7
-0+ } + +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.
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 202
Quoted 7 lines
drivers/gpio/gpio-mxc.c +7
-0+ + 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 30 lines
drivers/gpio/gpio-mxc.c +10
-0port->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 ;Signature · 1 line
1.7.9.5
RFC PATCH 1/1 ] gpio: gpio-mxc: make sure gpio is input when request IRQ
Am 22.07.2014 08:28, wrote Shawn Guo:
Quoted 37 lines · Markus Niebel
Quoted 36 lines
From: Markus Niebel <[email protected]>
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 inputThe 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 irqTested 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 <[email protected]>--- drivers/gpio/gpio-mxc.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)drivers/gpio/gpio-mxc.c +3
-0@@ -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)) {
Quoted 8 lines
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 184
Quoted 4 lines
drivers/gpio/gpio-mxc.c +4
-0+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 9 lines
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 188
Quoted 5 lines
drivers/gpio/gpio-mxc.c +4
-0+ }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 4 lines
Quoted 4 lines
drivers/gpio/gpio-mxc.c +4
-0+ } + +if (ret) { + dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",
Quoted 8 lines
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 195
Quoted 3 lines
drivers/gpio/gpio-mxc.c +3
-0+ 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 49 lines
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 202
Quoted 8 lines
drivers/gpio/gpio-mxc.c +7
-0+ + 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 30 lines
drivers/gpio/gpio-mxc.c +10
-0port->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 ;Signature · 1 line
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
RFC PATCH 1/1 ] gpio: gpio-mxc: make sure gpio is input when request IRQ
+ Grant and devicetree list
Quoted 47 lines · Markus Niebel
Am 22.07.2014 08:28, wrote Shawn Guo:
Quoted 37 lines · Markus Niebel
Quoted 36 lines
From: Markus Niebel <[email protected]>
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 inputThe 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 irqTested 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 <[email protected]>--- drivers/gpio/gpio-mxc.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)drivers/gpio/gpio-mxc.c +3
-0@@ -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)) {Quoted 8 lines
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 184
Quoted 4 lines
drivers/gpio/gpio-mxc.c +4
-0+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.
Quoted 8 lines
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.
Okay. I understand your problem now. This is an issue in case we specify a GPIO to be used as interrupt from device tree. In non-DT world, client driver knows this is an interrupt on GPIO, and therefore takes the responsibility to request and configure the GPIO to work as interrupt. But in DT case, client driver does not know whether it's an IRQ line or GPIO based interrupt. The consequence is that GPIO subsystem, OF subsystem, client driver, none of the three is requesting and configuring the GPIO to be used as interrupt.
This is a common issue instead of i.MX specific, and should be addressed globally, I think.
Shawn
Quoted 83 lines
Quoted 10 lines
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 188
Quoted 6 lines
drivers/gpio/gpio-mxc.c +4
-0+ }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 4 lines
Quoted 4 lines
drivers/gpio/gpio-mxc.c +4
-0+ } + +if (ret) { + dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",Quoted 8 lines
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 195
Quoted 3 lines
drivers/gpio/gpio-mxc.c +3
-0+ 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 49 lines
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 202
Quoted 8 lines
drivers/gpio/gpio-mxc.c +7
-0+ + 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 30 lines
drivers/gpio/gpio-mxc.c +10
-0port->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 ;Signature · 1 line
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
RFC PATCH 1/1 ] gpio: gpio-mxc: make sure gpio is input when request IRQ
Quoted 16 lines · Shawn Guo
Quoted 4 lines · Markus Niebel
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.
Okay. I understand your problem now. This is an issue in case we specify a GPIO to be used as interrupt from device tree. In non-DT world, client driver knows this is an interrupt on GPIO, and therefore takes the responsibility to request and configure the GPIO to work as interrupt. But in DT case, client driver does not know whether it's an IRQ line or GPIO based interrupt. The consequence is that GPIO subsystem, OF subsystem, client driver, none of the three is requesting and configuring the GPIO to be used as interrupt.
Quoted 2 lines
This is a common issue instead of i.MX specific, and should be addressed globally, I think.
This is very well documented in Documentation/gpio/driver.txt these days. Quoting:
"
It is legal for any IRQ consumer to request an IRQ from any irqchip no matter if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and irq_chip are orthogonal, and offering their services independent of each other.
gpiod_to_irq() is just a convenience function to figure out the IRQ for a certain GPIO line and should not be relied upon to have been called before the IRQ is used.
So always prepare the hardware and make it ready for action in respective callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having been called first.
This orthogonality leads to ambiguities that we need to solve: if there is competition inside the subsystem which side is using the resource (a certain GPIO line and register for example) it needs to deny certain operations and keep track of usage inside of the gpiolib subsystem. This is why the API below exists.
Locking IRQ usage
-----------------
Input GPIOs can be used as IRQ signals. When this happens, a driver is requested to mark the GPIO as being used as an IRQ:
int gpiod_lock_as_irq(struct gpio_desc *desc)
This will prevent the use of non-irq related GPIO APIs until the GPIO IRQ lock is released:
void gpiod_unlock_as_irq(struct gpio_desc *desc)
When implementing an irqchip inside a GPIO driver, these two functions should typically be called in the .startup() and .shutdown() callbacks from the irqchip.
"
So: make sure each API can be used orthogonally by just poking into the hardware registers. Do not call gpio_request_one().
Yours,
Linus Walleij
RFC PATCH 1/1 ] gpio: gpio-mxc: make sure gpio is input when request IRQ
Am 23.07.2014 18:14, wrote Linus Walleij:
Quoted 17 lines · Shawn Guo
Quoted 16 lines · Markus Niebel
Quoted 5 lines
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.
Okay. I understand your problem now. This is an issue in case we specify a GPIO to be used as interrupt from device tree. In non-DT world, client driver knows this is an interrupt on GPIO, and therefore takes the responsibility to request and configure the GPIO to work as interrupt. But in DT case, client driver does not know whether it's an IRQ line or GPIO based interrupt. The consequence is that GPIO subsystem, OF subsystem, client driver, none of the three is requesting and configuring the GPIO to be used as interrupt.
Quoted 6 lines
Quoted 2 lines
This is a common issue instead of i.MX specific, and should be addressed globally, I think.
This is very well documented in
Documentation/gpio/driver.txtthese days. Quoting:
Sorry for not carefully reading the docs and many thanks for the helpful comments.
Quoted 6 lines
"
It is legal for any IRQ consumer to request an IRQ from any irqchip no matter if that is a combined GPIO+IRQ driver. The basic premise is thatgpio_chipandirq_chipare orthogonal, and offering their services independent of each other.
Quoted 8 lines
gpiod_to_irq()is just a convenience function to figure out the IRQ for a certain GPIO line and should not be relied upon to have been called before the IRQ is used.So always prepare the hardware and make it ready for action in respective callbacks from the GPIO and irqchip APIs. Do not rely on
gpiod_to_irq()having been called first.
So a gpio driver is responsible to read status of gpio lines and flag any gpio line currently configured as out (base on the information read from hardware registers) on driver probe time - correct? If yes is the driver allowed to call gpiod_get_direction() to have the FLAG_IS_OUT set in the gpiolib layer?
Quoted 19 lines
This orthogonality leads to ambiguities that we need to solve: if there is competition inside the subsystem which side is using the resource (a certain GPIO line and register for example) it needs to deny certain operations and keep track of usage inside of the gpiolib subsystem. This is why the API below exists.
Locking IRQ usage
-----------------
Input GPIOs can be used as IRQ signals. When this happens, a driver is requested to mark the GPIO as being used as an IRQ:int
gpiod_lock_as_irq(struct gpio_desc *desc)This will prevent the use of non-irq related GPIO APIs until the GPIO IRQ lock is released:
void
gpiod_unlock_as_irq(struct gpio_desc *desc)
Quoted 7 lines
When implementing an irqchip inside a GPIO driver, these two functions should typically be called in the .startup() and .shutdown() callbacks from the irqchip.
"So: make sure each API can be used orthogonally by just poking into the hardware registers. Do not call
gpio_request_one().
Makes sense.
Thanks again
Markus
Quoted 8 lines
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 1/1 ] gpio: gpio-mxc: make sure gpio is input when request IRQ
Quoted 10 lines · Markus Niebel
Am 23.07.2014 18:14, wrote Linus Walleij:
Quoted 4 lines
So always prepare the hardware and make it ready for action in respective callbacks from the GPIO and irqchip APIs. Do not rely on
gpiod_to_irq()having been called first.So a gpio driver is responsible to read status of gpio lines and flag any gpio line currently configured as out (base on the information read from hardware registers) on driver probe time - correct?
I don't think anyone reads that information explicitly to set up these flags.
Drivers just leave the pins in their power-on maiden state without trying to figure out how they're set-up. But as you say, if you call gpiod_get_direction() on them, the flag gets set up indeed.
So usually these flags are set by code, calling
gpiod_[get/set]_direction().
Then they do get flagged as outputs or inputs.
Quoted 2 lines
If yes is the driver allowed to call
gpiod_get_direction()to have theFLAG_IS_OUTset in the gpiolib layer?
I don't know if it'd be a good idea to loop over all gpios in a new irqchip and fetch the direction just to get the flags right, so far we haven't done that and I don't know what the usecase would be.
If we need that we should do it in gpiolib for all drivers don't you think?
But then we need a rationale for doing it, other than it's nice :-) It is already called on-the-fly by debugfs when a user needs that info.
Yours,
Linus Walleij
RFC PATCH 1/1 ] gpio: gpio-mxc: make sure gpio is input when request IRQ
Am 25.07.2014 13:38, wrote Linus Walleij:
Quoted 24 lines
Quoted 10 lines · Markus Niebel
Am 23.07.2014 18:14, wrote Linus Walleij:
Quoted 4 lines
So always prepare the hardware and make it ready for action in respective callbacks from the GPIO and irqchip APIs. Do not rely on
gpiod_to_irq()having been called first.So a gpio driver is responsible to read status of gpio lines and flag any gpio line currently configured as out (base on the information read from hardware registers) on driver probe time - correct?
I don't think anyone reads that information explicitly to set up these flags.
Drivers just leave the pins in their power-on maiden state without trying to figure out how they're set-up. But as you say, if you call
gpiod_get_direction()on them, the flag gets set up indeed.So usually these flags are set by code, calling
gpiod_[get/set]_direction().
Then they do get flagged as outputs or inputs.
Quoted 7 lines
Quoted 2 lines
If yes is the driver allowed to call
gpiod_get_direction()to have theFLAG_IS_OUTset in the gpiolib layer?I don't know if it'd be a good idea to loop over all gpios in a new irqchip and fetch the direction just to get the flags right, so far we haven't done that and I don't know what the usecase would be.
I've came to a situation where it would have been helpful to know - bootloader configured a pin as output and linux tried to configure the pin as IRQ input. *YES I KNOW THIS IS NOT CORRECT* but it took some time to see, what happened.
Quoted 2 lines
If we need that we should do it in gpiolib for all drivers don't you think?
A pragmatic / lean solution would be to deny the IRQ configuration when seeing the pin configured as output in hardware and print out an error on the gpio driver level.
Quoted 4 lines
But then we need a rationale for doing it, other than it's nice :-) It is already called on-the-fly by debugfs when a user needs that info.
See above, I think an error on a misconfigured system would be enough. Maybe the documentation for gpio drivers should have a hint for driver implementors.
So first step would be to convert the driver to the irqchip helper and then add calls to gpiod_[un]lock_as_irq at the right places.
Will try to do that after my holiday.
Quoted 4 lines
Yours,
Linus Walleij
Yours,
Markus Niebel
RFC PATCH 1/1 ] gpio: gpio-mxc: make sure gpio is input when request IRQ
Quoted 9 lines · Markus Niebel
Am 25.07.2014 13:38, wrote Linus Walleij:
Quoted 3 lines
I don't know if it'd be a good idea to loop over all gpios in a new irqchip and fetch the direction just to get the flags right, so far we haven't done that and I don't know what the usecase would be.
I've came to a situation where it would have been helpful to know - bootloader configured a pin as output and linux tried to configure the pin as IRQ input. *YES I KNOW THIS IS NOT CORRECT* but it took some time to see, what happened.
Hm, maybe we should call gpiod_get_direction() first in gpio_lock_as_irq(), so we check the hardware and know there that the flag is correctly set here?
Quoted 6 lines
Quoted 2 lines
If we need that we should do it in gpiolib for all drivers don't you think?
A pragmatic / lean solution would be to deny the IRQ configuration when seeing the pin configured as output in hardware and print out an error on the gpio driver level.
I guess that is what I'm suggesting, not sure if I follow correctly.
Yours,
Linus Walleij
RFC PATCH 1/1 ] gpio: gpio-mxc: make sure gpio is input when request IRQ
In reply to Markus NiebelQuoted 30 lines · Markus Niebel
From: Markus Niebel <[email protected]>
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 inputThe 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 irqTested 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 <[email protected]>--- drivers/gpio/gpio-mxc.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)drivers/gpio/gpio-mxc.c +0
-0
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 184
Quoted 10 lines
drivers/gpio/gpio-mxc.c +7
-0@@ -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);
Wut? Auto-request here? This can not be right.
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 197
Quoted 13 lines
drivers/gpio/gpio-mxc.c +13
-0+ }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.
Quoted 10 lines
drivers/gpio/gpio-mxc.c +5
-0+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 ;
drivers/gpio/gpio-mxc.c static int gpio_set_irq_type(struct irq_data *d, u32 type) · line 266
Quoted 10 lines
drivers/gpio/gpio-mxc.c +8
-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); +}
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