| Message ID | 20190410103926.8781-3-alexandre.belloni@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpio: lpc32xx: enable interrupts for port 3 | expand |
On Wed, Apr 10, 2019 at 12:39 PM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > Interrupt support was disabled "temporarily" in commit 320a6480ef24 ("gpio: > lpc32xx: disable broken to_irq support"). > > Reenable to_irq for port 3 as they are directly connected to an interrupt > controller and a simple lookup is working. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> Unfortunately this seems to be one of these hacks for hierarchical interrupts that does not quite cut it. > +static int lpc32xx_gpio_to_irq_p3(struct gpio_chip *chip, unsigned int offset) > + return of_irq_get_byname(chip->of_node, chip->names[offset]); This works as long as consuming drivers pick a GPIO first using gpiod_get() (and variants) and then convert it to an IRQ using gpiod_to_irq(). But it doesn't work when some consumer is just picking an IRQ off of the node without picking the GPIO first. And in DT that is OK, this DT node is definately an interrupt controller. (Leaving out the attribute for interrupt controller as is currently the case doesn't really fix the issue, it is just inconsistent.) Look at Brian Masney's series for the qualcomm pin controller chips for inspiration on how to do things right, see: commit 9d2b563bc23acfa93e7716b3396fd2f79fa8f0cd and down. Also: https://marc.info/?l=linux-gpio&m=154959228527643&w=2 Especially note how he removes the kind of hack you are adding in e.g. commit a796fab2c605d6340512c51c6c3fa1c581357993 Yours, Linus Walleij
On Thu, Apr 11, 2019 at 9:55 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, Apr 10, 2019 at 12:39 PM Alexandre Belloni > <alexandre.belloni@bootlin.com> wrote: > > > Interrupt support was disabled "temporarily" in commit 320a6480ef24 ("gpio: > > lpc32xx: disable broken to_irq support"). > > > > Reenable to_irq for port 3 as they are directly connected to an interrupt > > controller and a simple lookup is working. > > > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > Unfortunately this seems to be one of these hacks for > hierarchical interrupts that does not quite cut it. > > > +static int lpc32xx_gpio_to_irq_p3(struct gpio_chip *chip, unsigned int offset) > > + return of_irq_get_byname(chip->of_node, chip->names[offset]); > > This works as long as consuming drivers pick a GPIO first > using gpiod_get() (and variants) and then convert it to an > IRQ using gpiod_to_irq(). > > But it doesn't work when some consumer is just picking an > IRQ off of the node without picking the GPIO first. > And in DT that is OK, this DT node is definately an interrupt > controller. (Leaving out the attribute for interrupt controller > as is currently the case doesn't really fix the issue, it is > just inconsistent.) > > Look at Brian Masney's series for the qualcomm pin controller > chips for inspiration on how to do things right, see: > commit 9d2b563bc23acfa93e7716b3396fd2f79fa8f0cd > and down. > > Also: > https://marc.info/?l=linux-gpio&m=154959228527643&w=2 > > Especially note how he removes the kind of hack you are > adding in e.g. > commit a796fab2c605d6340512c51c6c3fa1c581357993 > > Yours, > Linus Walleij > Hi Alexandre, you can also look at the original attempt, in 2015, to replace this driver by a new version: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386708.html Regards, Sylvain > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/gpio/gpio-lpc32xx.c b/drivers/gpio/gpio-lpc32xx.c index aa74cc4d8b14..24f09b08e319 100644 --- a/drivers/gpio/gpio-lpc32xx.c +++ b/drivers/gpio/gpio-lpc32xx.c @@ -22,6 +22,7 @@ #include <linux/errno.h> #include <linux/gpio/driver.h> #include <linux/of.h> +#include <linux/of_irq.h> #include <linux/platform_device.h> #include <linux/module.h> @@ -385,14 +386,9 @@ static int lpc32xx_gpio_to_irq_p01(struct gpio_chip *chip, unsigned offset) return -ENXIO; } -static int lpc32xx_gpio_to_irq_gpio_p3(struct gpio_chip *chip, unsigned offset) +static int lpc32xx_gpio_to_irq_p3(struct gpio_chip *chip, unsigned int offset) { - return -ENXIO; -} - -static int lpc32xx_gpio_to_irq_gpi_p3(struct gpio_chip *chip, unsigned offset) -{ - return -ENXIO; + return of_irq_get_byname(chip->of_node, chip->names[offset]); } static struct lpc32xx_gpio_chip lpc32xx_gpiochip[] = { @@ -451,7 +447,7 @@ static struct lpc32xx_gpio_chip lpc32xx_gpiochip[] = { .direction_output = lpc32xx_gpio_dir_output_p3, .set = lpc32xx_gpio_set_value_p3, .request = lpc32xx_gpio_request, - .to_irq = lpc32xx_gpio_to_irq_gpio_p3, + .to_irq = lpc32xx_gpio_to_irq_p3, .base = LPC32XX_GPIO_P3_GRP, .ngpio = LPC32XX_GPIO_P3_MAX, .names = gpio_p3_names, @@ -465,7 +461,7 @@ static struct lpc32xx_gpio_chip lpc32xx_gpiochip[] = { .direction_input = lpc32xx_gpio_dir_in_always, .get = lpc32xx_gpi_get_value, .request = lpc32xx_gpio_request, - .to_irq = lpc32xx_gpio_to_irq_gpi_p3, + .to_irq = lpc32xx_gpio_to_irq_p3, .base = LPC32XX_GPI_P3_GRP, .ngpio = LPC32XX_GPI_P3_MAX, .names = gpi_p3_names,
Interrupt support was disabled "temporarily" in commit 320a6480ef24 ("gpio: lpc32xx: disable broken to_irq support"). Reenable to_irq for port 3 as they are directly connected to an interrupt controller and a simple lookup is working. Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> --- drivers/gpio/gpio-lpc32xx.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)