Re: [PATCH] gpio: xilinx: Add interrupt support

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

sob., 6 cze 2020 o 01:57 Robert Hancock <hancock@xxxxxxxxxxxxx> napisał(a):
>
> Adds interrupt support to the Xilinx GPIO driver so that rising and
> falling edge line events can be supported. Since interrupt support is
> an optional feature in the Xilinx IP, the driver continues to support
> devices which have no interrupt provided.
>
> These changes are based on a patch in the Xilinx Linux Git tree,
> "gpio: xilinx: Add irq support to the driver" from Srinivas Neeli, but
> include a number of fixes and improvements such as supporting both
> rising and falling edge events.
>
> Signed-off-by: Robert Hancock <hancock@xxxxxxxxxxxxx>
> ---
>  drivers/gpio/Kconfig       |   1 +
>  drivers/gpio/gpio-xilinx.c | 247 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 233 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index bcacd9c74aa80..5f91e7829fb7d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -652,6 +652,7 @@ config GPIO_XGENE_SB
>
>  config GPIO_XILINX
>         tristate "Xilinx GPIO support"
> +       select GPIOLIB_IRQCHIP
>         help
>           Say yes here to support the Xilinx FPGA GPIO device
>
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index 67f9f82e0db0e..42ae12801d0bf 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -14,6 +14,9 @@
>  #include <linux/io.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
>
>  /* Register Offset Definitions */
>  #define XGPIO_DATA_OFFSET   (0x0)      /* Data register  */
> @@ -21,6 +24,11 @@
>
>  #define XGPIO_CHANNEL_OFFSET   0x8
>
> +#define XGPIO_GIER_OFFSET      0x11c /* Global Interrupt Enable */
> +#define XGPIO_GIER_IE          BIT(31)
> +#define XGPIO_IPISR_OFFSET     0x120 /* IP Interrupt Status */
> +#define XGPIO_IPIER_OFFSET     0x128 /* IP Interrupt Enable */
> +
>  /* Read/Write access to the GPIO registers */
>  #if defined(CONFIG_ARCH_ZYNQ) || defined(CONFIG_X86)
>  # define xgpio_readreg(offset)         readl(offset)
> @@ -35,17 +43,27 @@
>   * @gc: GPIO chip
>   * @regs: register block
>   * @gpio_width: GPIO width for every channel
> - * @gpio_state: GPIO state shadow register
> + * @gpio_state: GPIO write state shadow register
> + * @gpio_last_irq_read: GPIO read state register from last interrupt
>   * @gpio_dir: GPIO direction shadow register
>   * @gpio_lock: Lock used for synchronization
> + * @irq: IRQ used by GPIO device
> + * @irq_enable: GPIO irq enable/disable bitfield
> + * @irq_rising_edge: GPIO irq rising edge enable/disable bitfield
> + * @irq_falling_edge: GPIO irq rising edge enable/disable bitfield
>   */
>  struct xgpio_instance {
>         struct gpio_chip gc;
>         void __iomem *regs;
>         unsigned int gpio_width[2];
>         u32 gpio_state[2];
> +       u32 gpio_last_irq_read[2];
>         u32 gpio_dir[2];
> -       spinlock_t gpio_lock[2];
> +       spinlock_t gpio_lock;
> +       int irq;
> +       u32 irq_enable[2];
> +       u32 irq_rising_edge[2];
> +       u32 irq_falling_edge[2];

I don't know this driver very well. Could you explain why the two
instances of these fields and why are you removing the second lock?

[snip!]

>         chip->gc.base = -1;
> @@ -336,6 +530,29 @@ static int xgpio_probe(struct platform_device *pdev)
>
>         xgpio_save_regs(chip);
>
> +       chip->irq = platform_get_irq(pdev, 0);

Why not simply: platform_get_irq_optional()?

> +       if (chip->irq <= 0) {
> +               dev_info(&pdev->dev, "GPIO IRQ not set\n");
> +       } else {
> +               u32 temp;
> +
> +               /* Disable per-channel interrupts */
> +               xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, 0);
> +               /* Clear any existing per-channel interrupts */
> +               temp = xgpio_readreg(chip->regs + XGPIO_IPISR_OFFSET);
> +               xgpio_writereg(chip->regs + XGPIO_IPISR_OFFSET, temp);
> +               /* Enable global interrupts */
> +               xgpio_writereg(chip->regs + XGPIO_GIER_OFFSET, XGPIO_GIER_IE);
> +
> +               chip->gc.irq.chip = &xgpio_irqchip;
> +               chip->gc.irq.handler = handle_bad_irq;
> +               chip->gc.irq.default_type = IRQ_TYPE_NONE;
> +               chip->gc.irq.parent_handler = xgpio_irqhandler;
> +               chip->gc.irq.parent_handler_data = chip;
> +               chip->gc.irq.parents = &chip->irq;
> +               chip->gc.irq.num_parents = 1;
> +       }
> +
>         status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
>         if (status) {
>                 dev_err(&pdev->dev, "failed to add GPIO chip\n");
> --
> 2.26.2
>

Bart