Re: [LINUX PATCH V3 5/9] gpio: gpio-xilinx: Add interrupt support

2020-11-17 Thread Linus Walleij
Hi Srinivas!

On Thu, Nov 12, 2020 at 6:12 PM Srinivas Neeli
 wrote:

> 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.
>
> Signed-off-by: Robert Hancock 
> Signed-off-by: Shubhrajyoti Datta 
> Signed-off-by: Srinivas Neeli 

(...)
>  config GPIO_XILINX
> tristate "Xilinx GPIO support"
> +   select GPIOLIB_IRQCHIP
> +   depends on OF_GPIO
> help
>   Say yes here to support the Xilinx FPGA GPIO device

Please add:
select IRQ_DOMAIN_HIERARCHY

Because your driver requires this.

> +   /* Update cells with gpio-cells value */
> +   if (of_property_read_u32(np, "#gpio-cells", &cells))
> +   dev_dbg(&pdev->dev, "Missing gpio-cells property\n");
(...)
> +   chip->gc.of_gpio_n_cells = cells;

Why is this necessary?
Mention in the commit.

Other than that this looks very good and good use
of the hierarchical IRQ feature in gpiolib!

Yours,
Linus Walleij


Re: [LINUX PATCH V3 5/9] gpio: gpio-xilinx: Add interrupt support

2020-11-16 Thread Robert Hancock
On Thu, 2020-11-12 at 22:42 +0530, Srinivas Neeli wrote:
> 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.
> 
> Signed-off-by: Robert Hancock 
> Signed-off-by: Shubhrajyoti Datta 
> Signed-off-by: Srinivas Neeli 
> ---
> Chnages in V3:
> -Created separate patch for Clock changes and runtime resume
>  and suspend.
> -Updated minor review comments.
> 
> Changes in V2:
> -Added check for return value of platform_get_irq() API.
> -Updated code to support rising edge and falling edge.
> -Added xgpio_xlate() API to support switch.
> -Added MAINTAINERS fragment
> ---
>  drivers/gpio/Kconfig   |   2 +
>  drivers/gpio/gpio-xilinx.c | 242
> -
>  2 files changed, 240 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 5d4de5cd6759..cf4959891eab 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -677,6 +677,8 @@ config GPIO_XGENE_SB
>  
>  config GPIO_XILINX
>   tristate "Xilinx GPIO support"
> + select GPIOLIB_IRQCHIP
> + depends on OF_GPIO

This OF_GPIO dependency was previously removed - is this required? It
appears the code is now setting of_gpio_n_cells but I am not sure if
this is necessary or helpful since the other of_gpio functions are not
used.

>   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 69bdf1910215..80a06ded 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -10,9 +10,12 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -22,6 +25,11 @@
>  
>  #define XGPIO_CHANNEL_OFFSET 0x8
>  
> +#define XGPIO_GIER_OFFSET0x11c /* Global Interrupt Enable */
> +#define XGPIO_GIER_IEBIT(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)
> @@ -36,9 +44,14 @@
>   * @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 falling edge enable/disable bitfield
>   * @clk: clock resource for this driver
>   */
>  struct xgpio_instance {
> @@ -46,8 +59,13 @@ struct xgpio_instance {
>   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;   /* For serializing operations */
> + int irq;
> + u32 irq_enable[2];
> + u32 irq_rising_edge[2];
> + u32 irq_falling_edge[2];
>   struct clk *clk;
>  };
>  
> @@ -258,6 +276,183 @@ static void xgpio_save_regs(struct
> xgpio_instance *chip)
>  }
>  
>  /**
> + * xgpio_irq_ack - Acknowledge a child GPIO interrupt.
> + * @irq_data: per IRQ and chip data passed down to chip functions
> + * This currently does nothing, but irq_ack is unconditionally
> called by
> + * handle_edge_irq and therefore must be defined.
> + */
> +static void xgpio_irq_ack(struct irq_data *irq_data)
> +{
> +}
> +
> +/**
> + * xgpio_irq_mask - Write the specified signal of the GPIO device.
> + * @irq_data: per IRQ and chip data passed down to chip functions
> + */
> +static void xgpio_irq_mask(struct irq_data *irq_data)
> +{
> + unsigned long flags;
> + struct xgpio_instance *chip =
> irq_data_get_irq_chip_data(irq_data);
> + int irq_offset = irqd_to_hwirq(irq_data);
> + int index = xgpio_index(chip, irq_offset);
> + int offset = xgpio_offset(chip, irq_offset);
> +
> + spin_lock_irqsave(&chip->gpio_lock, flags);
> +
> + chip->irq_enable[index] &= ~BIT(offset);
> +
> + if (!chip->irq_enable[index]) {
> + /* Disable per channel interrupt */
> + u32 temp = xgpio_readreg(chip->regs +
> XGPIO_IPIER_OFFSET);
> +
> + temp &= ~BIT(index);
> + xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
> + }
> + spin_unlock_irqrestore(&chip->gpio_lock, flags);
> +}
> +
> +/**
> + * xgpio_irq_unmask - Write the specified signal of the GPIO device.
> + * @irq_data: per IRQ and chip data