[PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver

[PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver

[Srinivas Neeli]

This patch series does the following:
-Add clock adaption support
-Add interupt support
-Add MAINTAINERS fragment

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.
---
Tested Below scenarios:
-Tested Loop Back.(channel 1.0 connected to channel 2.0)
-Tested External switch(Used DIP switch)
-Tested Cascade scenario(Here gpio controller acting as
 an interrupt controller).
---


Srinivas Neeli (3):
  gpio: xilinx: Add clock adaptation support
  gpio: xilinx: Add interrupt support
  MAINTAINERS: add fragment for xilinx GPIO drivers

 MAINTAINERS                |  10 ++
 drivers/gpio/Kconfig       |   1 +
 drivers/gpio/gpio-xilinx.c | 404 ++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 395 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH V2 1/3] gpio: xilinx: Add clock adaptation support

[Srinivas Neeli]

Add support of clock adaptation for AXI GPIO driver.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Changes in V2:
Add check for return value of platform_get_irq() API.
---
 drivers/gpio/gpio-xilinx.c | 111 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 67f9f82e0db0..d103613e787a 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -14,6 +14,8 @@
 #include <linux/io.h>
 #include <linux/gpio/driver.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
+#include <linux/clk.h>
 
 /* Register Offset Definitions */
 #define XGPIO_DATA_OFFSET   (0x0)	/* Data register  */
@@ -38,6 +40,7 @@
  * @gpio_state: GPIO state shadow register
  * @gpio_dir: GPIO direction shadow register
  * @gpio_lock: Lock used for synchronization
+ * @clk: clock resource for this driver
  */
 struct xgpio_instance {
 	struct gpio_chip gc;
@@ -45,7 +48,8 @@ struct xgpio_instance {
 	unsigned int gpio_width[2];
 	u32 gpio_state[2];
 	u32 gpio_dir[2];
-	spinlock_t gpio_lock[2];
+	spinlock_t gpio_lock[2];	/* For serializing operations */
+	struct clk *clk;
 };
 
 static inline int xgpio_index(struct xgpio_instance *chip, int gpio)
@@ -256,6 +260,83 @@ static void xgpio_save_regs(struct xgpio_instance *chip)
 		       chip->gpio_dir[1]);
 }
 
+static int xgpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	int ret = pm_runtime_get_sync(chip->parent);
+
+	/*
+	 * If the device is already active pm_runtime_get() will return 1 on
+	 * success, but gpio_request still needs to return 0.
+	 */
+	return ret < 0 ? ret : 0;
+}
+
+static void xgpio_free(struct gpio_chip *chip, unsigned int offset)
+{
+	pm_runtime_put(chip->parent);
+}
+
+static int __maybe_unused xgpio_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct irq_data *data;
+	int irq = platform_get_irq(pdev, 0);
+
+	if (irq < 0) {
+		dev_info(&pdev->dev, "platform_get_irq returned %d\n", irq);
+		return irq;
+	}
+
+	data = irq_get_irq_data(irq);
+	if (!irqd_is_wakeup_set(data))
+		return pm_runtime_force_suspend(dev);
+
+	return 0;
+}
+
+static int __maybe_unused xgpio_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct irq_data *data;
+	int irq = platform_get_irq(pdev, 0);
+
+	if (irq < 0) {
+		dev_info(&pdev->dev, "platform_get_irq returned %d\n", irq);
+		return irq;
+	}
+
+	data = irq_get_irq_data(irq);
+
+	if (!irqd_is_wakeup_set(data))
+		return pm_runtime_force_resume(dev);
+
+	return 0;
+}
+
+static int __maybe_unused xgpio_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct xgpio_instance *gpio = platform_get_drvdata(pdev);
+
+	clk_disable(gpio->clk);
+
+	return 0;
+}
+
+static int __maybe_unused xgpio_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct xgpio_instance *gpio = platform_get_drvdata(pdev);
+
+	return clk_enable(gpio->clk);
+}
+
+static const struct dev_pm_ops xgpio_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(xgpio_suspend, xgpio_resume)
+	SET_RUNTIME_PM_OPS(xgpio_runtime_suspend,
+			   xgpio_runtime_resume, NULL)
+};
+
 /**
  * xgpio_of_probe - Probe method for the GPIO device.
  * @pdev: pointer to the platform device
@@ -324,6 +405,8 @@ static int xgpio_probe(struct platform_device *pdev)
 	chip->gc.direction_output = xgpio_dir_out;
 	chip->gc.get = xgpio_get;
 	chip->gc.set = xgpio_set;
+	chip->gc.request = xgpio_request;
+	chip->gc.free = xgpio_free;
 	chip->gc.set_multiple = xgpio_set_multiple;
 
 	chip->gc.label = dev_name(&pdev->dev);
@@ -334,15 +417,38 @@ static int xgpio_probe(struct platform_device *pdev)
 		return PTR_ERR(chip->regs);
 	}
 
+	chip->clk = devm_clk_get_optional(&pdev->dev, "s_axi_aclk");
+	if (IS_ERR(chip->clk)) {
+		if (PTR_ERR(chip->clk) != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Input clock not found\n");
+		return PTR_ERR(chip->clk);
+	}
+	status = clk_prepare_enable(chip->clk);
+	if (status < 0) {
+		dev_err(&pdev->dev, "Failed to prepare clk\n");
+		return status;
+	}
+	pm_runtime_enable(&pdev->dev);
+	status = pm_runtime_get_sync(&pdev->dev);
+	if (status < 0)
+		goto err_unprepare_clk;
+
 	xgpio_save_regs(chip);
 
 	status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
 	if (status) {
 		dev_err(&pdev->dev, "failed to add GPIO chip\n");
-		return status;
+		goto err_pm_put;
 	}
 
+	pm_runtime_put(&pdev->dev);
 	return 0;
+err_pm_put:
+	pm_runtime_put(&pdev->dev);
+err_unprepare_clk:
+	pm_runtime_disable(&pdev->dev);
+	clk_unprepare(chip->clk);
+	return status;
 }
 
 static const struct of_device_id xgpio_of_match[] = {
@@ -357,6 +463,7 @@ static struct platform_driver xgpio_plat_driver = {
 	.driver		= {
 			.name = "gpio-xilinx",
 			.of_match_table	= xgpio_of_match,
+			.pm = &xgpio_dev_pm_ops,
 	},
 };
 
-- 
2.7.4

Re: [PATCH V2 1/3] gpio: xilinx: Add clock adaptation support

[Bartosz Golaszewski]

On Thu, Jul 23, 2020 at 4:06 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:
>
> Add support of clock adaptation for AXI GPIO driver.
>

Please make the commit message more specific. I can tell from the
patch that it's about power management but I've never heard anyone
referring to it as clock adaptation. There's also a lot of runtime pm
code in here. Be more descriptive.

> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
> Changes in V2:
> Add check for return value of platform_get_irq() API.
> ---
>  drivers/gpio/gpio-xilinx.c | 111 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index 67f9f82e0db0..d103613e787a 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -14,6 +14,8 @@
>  #include <linux/io.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/slab.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/clk.h>

Alphabetical order of includes?

>
>  /* Register Offset Definitions */
>  #define XGPIO_DATA_OFFSET   (0x0)      /* Data register  */
> @@ -38,6 +40,7 @@
>   * @gpio_state: GPIO state shadow register
>   * @gpio_dir: GPIO direction shadow register
>   * @gpio_lock: Lock used for synchronization
> + * @clk: clock resource for this driver
>   */
>  struct xgpio_instance {
>         struct gpio_chip gc;
> @@ -45,7 +48,8 @@ struct xgpio_instance {
>         unsigned int gpio_width[2];
>         u32 gpio_state[2];
>         u32 gpio_dir[2];
> -       spinlock_t gpio_lock[2];
> +       spinlock_t gpio_lock[2];        /* For serializing operations */

This looks like it was sneaked into an unrelated patch.

> +       struct clk *clk;
>  };
>
>  static inline int xgpio_index(struct xgpio_instance *chip, int gpio)
> @@ -256,6 +260,83 @@ static void xgpio_save_regs(struct xgpio_instance *chip)
>                        chip->gpio_dir[1]);
>  }
>
> +static int xgpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +       int ret = pm_runtime_get_sync(chip->parent);
> +
> +       /*
> +        * If the device is already active pm_runtime_get() will return 1 on
> +        * success, but gpio_request still needs to return 0.
> +        */
> +       return ret < 0 ? ret : 0;
> +}
> +
> +static void xgpio_free(struct gpio_chip *chip, unsigned int offset)
> +{
> +       pm_runtime_put(chip->parent);
> +}
> +
> +static int __maybe_unused xgpio_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct irq_data *data;
> +       int irq = platform_get_irq(pdev, 0);
> +
> +       if (irq < 0) {
> +               dev_info(&pdev->dev, "platform_get_irq returned %d\n", irq);
> +               return irq;
> +       }
> +
> +       data = irq_get_irq_data(irq);
> +       if (!irqd_is_wakeup_set(data))
> +               return pm_runtime_force_suspend(dev);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused xgpio_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct irq_data *data;
> +       int irq = platform_get_irq(pdev, 0);
> +
> +       if (irq < 0) {
> +               dev_info(&pdev->dev, "platform_get_irq returned %d\n", irq);
> +               return irq;
> +       }

No, don't do this on every suspend/resume - just call
platform_get_irq() in probe() and store the irq number for later use.
This way you only check it once. Also why would you log the return
value?

> +
> +       data = irq_get_irq_data(irq);
> +
> +       if (!irqd_is_wakeup_set(data))
> +               return pm_runtime_force_resume(dev);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused xgpio_runtime_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct xgpio_instance *gpio = platform_get_drvdata(pdev);
> +
> +       clk_disable(gpio->clk);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused xgpio_runtime_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct xgpio_instance *gpio = platform_get_drvdata(pdev);
> +
> +       return clk_enable(gpio->clk);
> +}
> +
> +static const struct dev_pm_ops xgpio_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(xgpio_suspend, xgpio_resume)
> +       SET_RUNTIME_PM_OPS(xgpio_runtime_suspend,
> +                          xgpio_runtime_resume, NULL)
> +};
> +
>  /**
>   * xgpio_of_probe - Probe method for the GPIO device.
>   * @pdev: pointer to the platform device
> @@ -324,6 +405,8 @@ static int xgpio_probe(struct platform_device *pdev)
>         chip->gc.direction_output = xgpio_dir_out;
>         chip->gc.get = xgpio_get;
>         chip->gc.set = xgpio_set;
> +       chip->gc.request = xgpio_request;
> +       chip->gc.free = xgpio_free;
>         chip->gc.set_multiple = xgpio_set_multiple;
>
>         chip->gc.label = dev_name(&pdev->dev);
> @@ -334,15 +417,38 @@ static int xgpio_probe(struct platform_device *pdev)
>                 return PTR_ERR(chip->regs);
>         }
>
> +       chip->clk = devm_clk_get_optional(&pdev->dev, "s_axi_aclk");
> +       if (IS_ERR(chip->clk)) {
> +               if (PTR_ERR(chip->clk) != -EPROBE_DEFER)
> +                       dev_err(&pdev->dev, "Input clock not found\n");

How is this an error if the clock is optional?

> +               return PTR_ERR(chip->clk);
> +       }
> +       status = clk_prepare_enable(chip->clk);
> +       if (status < 0) {
> +               dev_err(&pdev->dev, "Failed to prepare clk\n");
> +               return status;
> +       }
> +       pm_runtime_enable(&pdev->dev);
> +       status = pm_runtime_get_sync(&pdev->dev);
> +       if (status < 0)
> +               goto err_unprepare_clk;
> +
>         xgpio_save_regs(chip);
>
>         status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
>         if (status) {
>                 dev_err(&pdev->dev, "failed to add GPIO chip\n");
> -               return status;
> +               goto err_pm_put;
>         }
>
> +       pm_runtime_put(&pdev->dev);
>         return 0;
> +err_pm_put:
> +       pm_runtime_put(&pdev->dev);
> +err_unprepare_clk:
> +       pm_runtime_disable(&pdev->dev);
> +       clk_unprepare(chip->clk);
> +       return status;
>  }
>
>  static const struct of_device_id xgpio_of_match[] = {
> @@ -357,6 +463,7 @@ static struct platform_driver xgpio_plat_driver = {
>         .driver         = {
>                         .name = "gpio-xilinx",
>                         .of_match_table = xgpio_of_match,
> +                       .pm = &xgpio_dev_pm_ops,
>         },
>  };
>
> --
> 2.7.4
>

Bart

[PATCH V2 2/3] gpio: xilinx: Add interrupt support

[Srinivas Neeli]

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 <hancock@sedsystems.ca>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Changes in V2:
Updated code to support rising edge and falling edge.
Added xgpio_xlate() API to support gpio-keys.
Updated code to check return value of of_property_read_u32.
---
 drivers/gpio/Kconfig       |   1 +
 drivers/gpio/gpio-xilinx.c | 295 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 277 insertions(+), 19 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 05e0801c6a78..239f8eb7a8eb 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 d103613e787a..509606e92a1c 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -16,6 +16,9 @@
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
 #include <linux/clk.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  */
@@ -23,6 +26,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)
@@ -37,9 +45,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 {
@@ -47,8 +60,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[2];	/* For serializing operations */
+	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;
 };
 
@@ -114,7 +132,7 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 	int index =  xgpio_index(chip, gpio);
 	int offset =  xgpio_offset(chip, gpio);
 
-	spin_lock_irqsave(&chip->gpio_lock[index], flags);
+	spin_lock_irqsave(&chip->gpio_lock, flags);
 
 	/* Write to GPIO signal and set its direction to output */
 	if (val)
@@ -125,7 +143,7 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 	xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET +
 		       xgpio_regoffset(chip, gpio), chip->gpio_state[index]);
 
-	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
+	spin_unlock_irqrestore(&chip->gpio_lock, flags);
 }
 
 /**
@@ -145,7 +163,7 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 	int index = xgpio_index(chip, 0);
 	int offset, i;
 
-	spin_lock_irqsave(&chip->gpio_lock[index], flags);
+	spin_lock_irqsave(&chip->gpio_lock, flags);
 
 	/* Write to GPIO signals */
 	for (i = 0; i < gc->ngpio; i++) {
@@ -156,9 +174,7 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 			xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET +
 				       index * XGPIO_CHANNEL_OFFSET,
 				       chip->gpio_state[index]);
-			spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
 			index =  xgpio_index(chip, i);
-			spin_lock_irqsave(&chip->gpio_lock[index], flags);
 		}
 		if (__test_and_clear_bit(i, mask)) {
 			offset =  xgpio_offset(chip, i);
@@ -172,7 +188,7 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 	xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET +
 		       index * XGPIO_CHANNEL_OFFSET, chip->gpio_state[index]);
 
-	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
+	spin_unlock_irqrestore(&chip->gpio_lock, flags);
 }
 
 /**
@@ -191,14 +207,14 @@ static int xgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 	int index =  xgpio_index(chip, gpio);
 	int offset =  xgpio_offset(chip, gpio);
 
-	spin_lock_irqsave(&chip->gpio_lock[index], flags);
+	spin_lock_irqsave(&chip->gpio_lock, flags);
 
 	/* Set the GPIO bit in shadow register and set direction as input */
 	chip->gpio_dir[index] |= BIT(offset);
 	xgpio_writereg(chip->regs + XGPIO_TRI_OFFSET +
 		       xgpio_regoffset(chip, gpio), chip->gpio_dir[index]);
 
-	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
+	spin_unlock_irqrestore(&chip->gpio_lock, flags);
 
 	return 0;
 }
@@ -222,7 +238,7 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	int index =  xgpio_index(chip, gpio);
 	int offset =  xgpio_offset(chip, gpio);
 
-	spin_lock_irqsave(&chip->gpio_lock[index], flags);
+	spin_lock_irqsave(&chip->gpio_lock, flags);
 
 	/* Write state of GPIO signal */
 	if (val)
@@ -237,7 +253,7 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	xgpio_writereg(chip->regs + XGPIO_TRI_OFFSET +
 			xgpio_regoffset(chip, gpio), chip->gpio_dir[index]);
 
-	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
+	spin_unlock_irqrestore(&chip->gpio_lock, flags);
 
 	return 0;
 }
@@ -294,6 +310,45 @@ static int __maybe_unused xgpio_suspend(struct device *dev)
 	return 0;
 }
 
+/**
+ * xgpio_xlate - Translate gpio_spec to the GPIO number and flags
+ * @gc: Pointer to gpio_chip device structure.
+ * @gpiospec:  gpio specifier as found in the device tree
+ * @flags: A flags pointer based on binding
+ *
+ * Return:
+ * irq number otherwise -EINVAL
+ */
+static int xgpio_xlate(struct gpio_chip *gc,
+		       const struct of_phandle_args *gpiospec, u32 *flags)
+{
+	if (gc->of_gpio_n_cells < 2) {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
+		return -EINVAL;
+
+	if (gpiospec->args[0] >= gc->ngpio)
+		return -EINVAL;
+
+	if (flags)
+		*flags = gpiospec->args[1];
+
+	return gpiospec->args[0];
+}
+
+/**
+ * xgpio_irq_ack - Acknowledge a child GPIO interrupt.
+ * This currently does nothing, but irq_ack is unconditionally called by
+ * handle_edge_irq and therefore must be defined.
+ * @irq_data: per irq and chip data passed down to chip functions
+ */
+static void xgpio_irq_ack(struct irq_data *irq_data)
+{
+}
+
 static int __maybe_unused xgpio_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -338,6 +393,176 @@ static const struct dev_pm_ops xgpio_dev_pm_ops = {
 };
 
 /**
+ * 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);
+
+	dev_dbg(chip->gc.parent, "Disable %d irq, irq_enable_mask 0x%x\n",
+		irq_offset, chip->irq_enable[index]);
+
+	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 passed down to chip functions
+ */
+static void xgpio_irq_unmask(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);
+	u32 old_enable = chip->irq_enable[index];
+
+	spin_lock_irqsave(&chip->gpio_lock, flags);
+
+	chip->irq_enable[index] |= BIT(offset);
+
+	dev_dbg(chip->gc.parent, "Enable %d irq, irq_enable_mask 0x%x\n",
+		irq_offset, chip->irq_enable[index]);
+
+	if (!old_enable) {
+		/* Clear any existing per-channel interrupts */
+		u32 val = xgpio_readreg(chip->regs + XGPIO_IPISR_OFFSET) &
+			BIT(index);
+
+		if (val)
+			xgpio_writereg(chip->regs + XGPIO_IPISR_OFFSET, val);
+
+		/* Update GPIO IRQ read data before enabling interrupt*/
+		val = xgpio_readreg(chip->regs + XGPIO_DATA_OFFSET +
+				    index * XGPIO_CHANNEL_OFFSET);
+		chip->gpio_last_irq_read[index] = val;
+
+		/* Enable per channel interrupt */
+		val = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
+		val |= BIT(index);
+		xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, val);
+	}
+
+	spin_unlock_irqrestore(&chip->gpio_lock, flags);
+}
+
+/**
+ * xgpio_set_irq_type - Write the specified signal of the GPIO device.
+ * @irq_data: Per irq and chip data passed down to chip functions
+ * @type: Interrupt type that is to be set for the gpio pin
+ *
+ * Return:
+ * 0 if interrupt type is supported otherwise otherwise -EINVAL
+ */
+static int xgpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
+{
+	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);
+
+	/* The Xilinx GPIO hardware provides a single interrupt status
+	 * indication for any state change in a given GPIO channel (bank).
+	 * Therefore, only rising edge or falling edge triggers are
+	 * supported.
+	 */
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_BOTH:
+		chip->irq_rising_edge[index] |= BIT(offset);
+		chip->irq_falling_edge[index] |= BIT(offset);
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		chip->irq_rising_edge[index] |= BIT(offset);
+		chip->irq_falling_edge[index] &= ~BIT(offset);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		chip->irq_rising_edge[index] &= ~BIT(offset);
+		chip->irq_falling_edge[index] |= BIT(offset);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	irq_set_handler_locked(irq_data, handle_edge_irq);
+	return 0;
+}
+
+static struct irq_chip xgpio_irqchip = {
+	.name           = "gpio-xilinx",
+	.irq_ack	= xgpio_irq_ack,
+	.irq_mask       = xgpio_irq_mask,
+	.irq_unmask     = xgpio_irq_unmask,
+	.irq_set_type   = xgpio_set_irq_type,
+};
+
+/**
+ * xgpio_irqhandler - Gpio interrupt service routine
+ * @desc: Pointer to interrupt description
+ */
+static void xgpio_irqhandler(struct irq_desc *desc)
+{
+	struct xgpio_instance *chip = irq_desc_get_handler_data(desc);
+	struct irq_chip *irqchip = irq_desc_get_chip(desc);
+	u32 num_channels = chip->gpio_width[1] ? 2 : 1;
+	u32 offset = 0, index;
+	u32 status = xgpio_readreg(chip->regs + XGPIO_IPISR_OFFSET);
+
+	xgpio_writereg(chip->regs + XGPIO_IPISR_OFFSET, status);
+
+	chained_irq_enter(irqchip, desc);
+	for (index = 0; index < num_channels; index++) {
+		if ((status & BIT(index))) {
+			unsigned long rising_events, falling_events, all_events;
+			unsigned long flags;
+			u32 data, bit;
+
+			spin_lock_irqsave(&chip->gpio_lock, flags);
+			data = xgpio_readreg(chip->regs + XGPIO_DATA_OFFSET +
+					     index * XGPIO_CHANNEL_OFFSET);
+			rising_events = data &
+					~chip->gpio_last_irq_read[index] &
+					chip->irq_enable[index] &
+					chip->irq_rising_edge[index];
+			falling_events = ~data &
+					 chip->gpio_last_irq_read[index] &
+					 chip->irq_enable[index] &
+					 chip->irq_falling_edge[index];
+			dev_dbg(chip->gc.parent,
+				"IRQ chan %u rising 0x%lx falling 0x%lx\n",
+				index, rising_events, falling_events);
+			all_events = rising_events | falling_events;
+			chip->gpio_last_irq_read[index] = data;
+			spin_unlock_irqrestore(&chip->gpio_lock, flags);
+
+			for_each_set_bit(bit, &all_events, 32) {
+				generic_handle_irq(irq_find_mapping
+					(chip->gc.irq.domain, offset + bit));
+			}
+		}
+		offset += chip->gpio_width[index];
+	}
+
+	chained_irq_exit(irqchip, desc);
+}
+
+/**
  * xgpio_of_probe - Probe method for the GPIO device.
  * @pdev: pointer to the platform device
  *
@@ -350,7 +575,8 @@ static int xgpio_probe(struct platform_device *pdev)
 	struct xgpio_instance *chip;
 	int status = 0;
 	struct device_node *np = pdev->dev.of_node;
-	u32 is_dual;
+	u32 is_dual = 0;
+	u32 cells = 2;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
@@ -359,12 +585,18 @@ static int xgpio_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, chip);
 
 	/* Update GPIO state shadow register with default value */
-	of_property_read_u32(np, "xlnx,dout-default", &chip->gpio_state[0]);
+	if (of_property_read_u32(np, "xlnx,dout-default",
+				 &chip->gpio_state[0]))
+		dev_info(&pdev->dev, "Missing xlnx,dout-default property\n");
 
 	/* Update GPIO direction shadow register with default value */
 	if (of_property_read_u32(np, "xlnx,tri-default", &chip->gpio_dir[0]))
 		chip->gpio_dir[0] = 0xFFFFFFFF;
 
+	/* Update cells with gpio-cells value */
+	if (of_property_read_u32(np, "#gpio-cells", &cells))
+		dev_info(&pdev->dev, "Missing gpio-cells property\n");
+
 	/*
 	 * Check device node and parent device node for device width
 	 * and assume default width of 32
@@ -372,15 +604,17 @@ static int xgpio_probe(struct platform_device *pdev)
 	if (of_property_read_u32(np, "xlnx,gpio-width", &chip->gpio_width[0]))
 		chip->gpio_width[0] = 32;
 
-	spin_lock_init(&chip->gpio_lock[0]);
+	spin_lock_init(&chip->gpio_lock);
 
 	if (of_property_read_u32(np, "xlnx,is-dual", &is_dual))
 		is_dual = 0;
 
 	if (is_dual) {
 		/* Update GPIO state shadow register with default value */
-		of_property_read_u32(np, "xlnx,dout-default-2",
-				     &chip->gpio_state[1]);
+		if (of_property_read_u32(np, "xlnx,dout-default-2",
+					 &chip->gpio_state[1]))
+			dev_info(&pdev->dev,
+				 "Missing xlnx,dout-default-2 property\n");
 
 		/* Update GPIO direction shadow register with default value */
 		if (of_property_read_u32(np, "xlnx,tri-default-2",
@@ -394,8 +628,6 @@ static int xgpio_probe(struct platform_device *pdev)
 		if (of_property_read_u32(np, "xlnx,gpio2-width",
 					 &chip->gpio_width[1]))
 			chip->gpio_width[1] = 32;
-
-		spin_lock_init(&chip->gpio_lock[1]);
 	}
 
 	chip->gc.base = -1;
@@ -403,6 +635,8 @@ static int xgpio_probe(struct platform_device *pdev)
 	chip->gc.parent = &pdev->dev;
 	chip->gc.direction_input = xgpio_dir_in;
 	chip->gc.direction_output = xgpio_dir_out;
+	chip->gc.of_gpio_n_cells = cells;
+	chip->gc.of_xlate = xgpio_xlate;
 	chip->gc.get = xgpio_get;
 	chip->gc.set = xgpio_set;
 	chip->gc.request = xgpio_request;
@@ -435,6 +669,29 @@ static int xgpio_probe(struct platform_device *pdev)
 
 	xgpio_save_regs(chip);
 
+	chip->irq = platform_get_irq_optional(pdev, 0);
+	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 = (unsigned int *)&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.7.4

Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support

[Andy Shevchenko]

On Thu, Jul 23, 2020 at 5:08 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> 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.

...

> +#include <linux/irqchip/chained_irq.h>

Not sure I see a user of it.

...

> +/**
> + * xgpio_xlate - Translate gpio_spec to the GPIO number and flags
> + * @gc: Pointer to gpio_chip device structure.
> + * @gpiospec:  gpio specifier as found in the device tree
> + * @flags: A flags pointer based on binding
> + *
> + * Return:
> + * irq number otherwise -EINVAL
> + */
> +static int xgpio_xlate(struct gpio_chip *gc,
> +                      const struct of_phandle_args *gpiospec, u32 *flags)
> +{
> +       if (gc->of_gpio_n_cells < 2) {
> +               WARN_ON(1);
> +               return -EINVAL;
> +       }
> +
> +       if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
> +               return -EINVAL;
> +
> +       if (gpiospec->args[0] >= gc->ngpio)
> +               return -EINVAL;
> +
> +       if (flags)
> +               *flags = gpiospec->args[1];
> +
> +       return gpiospec->args[0];
> +}

This looks like a very standart xlate function for GPIO. Why do you
need to open-code it?

...

> +/**
> + * xgpio_irq_ack - Acknowledge a child GPIO interrupt.

> + * This currently does nothing, but irq_ack is unconditionally called by
> + * handle_edge_irq and therefore must be defined.

This should go after parameter description(s).

> + * @irq_data: per irq and chip data passed down to chip functions
> + */

...

>  /**
> + * xgpio_irq_mask - Write the specified signal of the GPIO device.
> + * @irq_data: per irq and chip data passed down to chip functions

In all comments irq -> IRQ.

> + */
> +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 you convert your data structure to use bitmaps (and respective API) like

#define XILINX_NGPIOS  64
...
  DECLARE_BITMAP(irq_enable, XILINX_NGPIOS);
...

it will make code better to read and understand. For example, here it
will be just
__clear_bit(offset, chip->irq_enable);

> +       dev_dbg(chip->gc.parent, "Disable %d irq, irq_enable_mask 0x%x\n",
> +               irq_offset, chip->irq_enable[index]);

Under spin lock?! Hmm...

> +       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);
> +}

...

> +       for (index = 0; index < num_channels; index++) {
> +               if ((status & BIT(index))) {

If gpio_width is the same among banks, you can use for_each_set_bit()
here as well.

...

> +                       for_each_set_bit(bit, &all_events, 32) {
> +                               generic_handle_irq(irq_find_mapping
> +                                       (chip->gc.irq.domain, offset + bit));

Strange indentation. Maybe a temporary variable helps?

...

> +       chip->irq = platform_get_irq_optional(pdev, 0);
> +       if (chip->irq <= 0) {
> +               dev_info(&pdev->dev, "GPIO IRQ not set\n");

Why do you need an optional variant if you print an error anyway?

> +       } else {


...

> +               chip->gc.irq.parents = (unsigned int *)&chip->irq;
> +               chip->gc.irq.num_parents = 1;

Current pattern is to use devm_kcalloc() for it (Linus has plans to
simplify this in the future and this will help him to find what
patterns are being used)

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH V2 2/3] gpio: xilinx: Add interrupt support

[Srinivas Neeli]

Hi Andy Shevchenko,

Thanks for the review.

Accepted comments will address in V3 and Added few comments in inline.

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Thursday, July 23, 2020 11:33 PM
> To: Srinivas Neeli <sneeli@xilinx.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski
> <bgolaszewski@baylibre.com>; Michal Simek <michals@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Srinivas Goud
> <sgoud@xilinx.com>; open list:GPIO SUBSYSTEM <linux-
> gpio@vger.kernel.org>; linux-arm Mailing List <linux-arm-
> kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; git <git@xilinx.com>
> Subject: Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
> 
> On Thu, Jul 23, 2020 at 5:08 PM Srinivas Neeli <srinivas.neeli@xilinx.com>
> 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.
> 
> ...
> 
> > +#include <linux/irqchip/chained_irq.h>
> 
> Not sure I see a user of it.
> 
> ...
we are using chained_irq_enter() and chained_irq_exit()
APIs , so need "chained_irq.h"
> 
> > +/**
> > + * xgpio_xlate - Translate gpio_spec to the GPIO number and flags
> > + * @gc: Pointer to gpio_chip device structure.
> > + * @gpiospec:  gpio specifier as found in the device tree
> > + * @flags: A flags pointer based on binding
> > + *
> > + * Return:
> > + * irq number otherwise -EINVAL
> > + */
> > +static int xgpio_xlate(struct gpio_chip *gc,
> > +                      const struct of_phandle_args *gpiospec, u32
> > +*flags) {
> > +       if (gc->of_gpio_n_cells < 2) {
> > +               WARN_ON(1);
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
> > +               return -EINVAL;
> > +
> > +       if (gpiospec->args[0] >= gc->ngpio)
> > +               return -EINVAL;
> > +
> > +       if (flags)
> > +               *flags = gpiospec->args[1];
> > +
> > +       return gpiospec->args[0];
> > +}
> 
> This looks like a very standart xlate function for GPIO. Why do you need to
> open-code it?
> 
> ...
> 
> > +/**
> > + * xgpio_irq_ack - Acknowledge a child GPIO interrupt.
> 
> > + * This currently does nothing, but irq_ack is unconditionally called
> > + by
> > + * handle_edge_irq and therefore must be defined.
> 
> This should go after parameter description(s).
> 
> > + * @irq_data: per irq and chip data passed down to chip functions */
> 
> ...
> 
> >  /**
> > + * xgpio_irq_mask - Write the specified signal of the GPIO device.
> > + * @irq_data: per irq and chip data passed down to chip functions
> 
> In all comments irq -> IRQ.
> 
> > + */
> > +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 you convert your data structure to use bitmaps (and respective API) like
> 
> #define XILINX_NGPIOS  64
> ...
>   DECLARE_BITMAP(irq_enable, XILINX_NGPIOS);
> ...
> 
> it will make code better to read and understand. For example, here it
> will be just
> __clear_bit(offset, chip->irq_enable);
> 
> > +       dev_dbg(chip->gc.parent, "Disable %d irq, irq_enable_mask 0x%x\n",
> > +               irq_offset, chip->irq_enable[index]);
> 
> Under spin lock?! Hmm...
> 
> > +       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);
> > +}
> 
> ...
> 
> > +       for (index = 0; index < num_channels; index++) {
> > +               if ((status & BIT(index))) {
> 
> If gpio_width is the same among banks, you can use for_each_set_bit()
> here as well.
> 
> ...
gpio_wdith vary depends on design. We can configure gpio pins for each bank.

> 
> > +                       for_each_set_bit(bit, &all_events, 32) {
> > +                               generic_handle_irq(irq_find_mapping
> > +                                       (chip->gc.irq.domain, offset + bit));
> 
> Strange indentation. Maybe a temporary variable helps?
> 
> ...
> 
> > +       chip->irq = platform_get_irq_optional(pdev, 0);
> > +       if (chip->irq <= 0) {
> > +               dev_info(&pdev->dev, "GPIO IRQ not set\n");
> 
> Why do you need an optional variant if you print an error anyway?

Here intention is just printing a debug message to user.

> 
> > +       } else {
> 
> 
> ...
> 
> > +               chip->gc.irq.parents = (unsigned int *)&chip->irq;
> > +               chip->gc.irq.num_parents = 1;
> 
> Current pattern is to use devm_kcalloc() for it (Linus has plans to
> simplify this in the future and this will help him to find what
> patterns are being used)

I didn't get this , Could you please explain more.
> 
> --
> With Best Regards,
> Andy Shevchenko


Adding Robert Hancock to mail chain ( by mistake suppressed cc list) .

Hi Robert, 
Could you please provide your comments.

Thanks
Srinivas Neeli

Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support

[Andy Shevchenko]

On Fri, Jul 24, 2020 at 8:15 PM Srinivas Neeli <sneeli@xilinx.com> wrote:

...

> > > +#include <linux/irqchip/chained_irq.h>
> >
> > Not sure I see a user of it.
> >
> > ...
> we are using chained_irq_enter() and chained_irq_exit()
> APIs , so need "chained_irq.h"

I see.  But gpio/driver.h does it for you.

...

> > > +       for (index = 0; index < num_channels; index++) {
> > > +               if ((status & BIT(index))) {
> >
> > If gpio_width is the same among banks, you can use for_each_set_bit()
> > here as well.
> >
> > ...
> gpio_wdith vary depends on design. We can configure gpio pins for each bank.

I see.

...

> > > +       chip->irq = platform_get_irq_optional(pdev, 0);
> > > +       if (chip->irq <= 0) {
> > > +               dev_info(&pdev->dev, "GPIO IRQ not set\n");
> >
> > Why do you need an optional variant if you print an error anyway?
>
> Here intention is just printing a debug message to user.

Debug message should be debug, and not info. But in any case, I would
rather drop it, or use platform_get_irq() b/c now you have a code
controversy.

...

> > > +               chip->gc.irq.parents = (unsigned int *)&chip->irq;
> > > +               chip->gc.irq.num_parents = 1;
> >
> > Current pattern is to use devm_kcalloc() for it (Linus has plans to
> > simplify this in the future and this will help him to find what
> > patterns are being used)
>
> I didn't get this , Could you please explain more.

              girq->num_parents = 1;
              girq->parents = devm_kcalloc(dev, girq->num_parents, ...);
   if (!girq->parents)
     return -ENOMEM;

girq->parents[0] = chip->irq;

Also, use temporary variable for IRQ chip structure:
 girq = &chip->gc.irq;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support

[Robert Hancock]

On 2020-07-23 12:03 p.m., Andy Shevchenko wrote:
>> +/**
>> + * xgpio_xlate - Translate gpio_spec to the GPIO number and flags
>> + * @gc: Pointer to gpio_chip device structure.
>> + * @gpiospec:  gpio specifier as found in the device tree
>> + * @flags: A flags pointer based on binding
>> + *
>> + * Return:
>> + * irq number otherwise -EINVAL
>> + */
>> +static int xgpio_xlate(struct gpio_chip *gc,
>> +                      const struct of_phandle_args *gpiospec, u32 *flags)
>> +{
>> +       if (gc->of_gpio_n_cells < 2) {
>> +               WARN_ON(1);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
>> +               return -EINVAL;
>> +
>> +       if (gpiospec->args[0] >= gc->ngpio)
>> +               return -EINVAL;
>> +
>> +       if (flags)
>> +               *flags = gpiospec->args[1];
>> +
>> +       return gpiospec->args[0];
>> +}
> 
> This looks like a very standart xlate function for GPIO. Why do you
> need to open-code it?

Indeed, this seems the same as the of_gpio_simple_xlate callback which 
is used if no xlate callback is specified, so I'm not sure why this is 
necessary?

> 
> ...
> 
>> +/**
>> + * xgpio_irq_ack - Acknowledge a child GPIO interrupt.
> 
>> + * This currently does nothing, but irq_ack is unconditionally called by
>> + * handle_edge_irq and therefore must be defined.
> 
> This should go after parameter description(s).
> 
>> + * @irq_data: per irq and chip data passed down to chip functions
>> + */
> 
> ...
> 
>>   /**
>> + * xgpio_irq_mask - Write the specified signal of the GPIO device.
>> + * @irq_data: per irq and chip data passed down to chip functions
> 
> In all comments irq -> IRQ.
> 
>> + */
>> +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 you convert your data structure to use bitmaps (and respective API) like
> 
> #define XILINX_NGPIOS  64
> ...
>    DECLARE_BITMAP(irq_enable, XILINX_NGPIOS);
> ...
> 
> it will make code better to read and understand. For example, here it
> will be just
> __clear_bit(offset, chip->irq_enable);
> 
>> +       dev_dbg(chip->gc.parent, "Disable %d irq, irq_enable_mask 0x%x\n",
>> +               irq_offset, chip->irq_enable[index]);
> 
> Under spin lock?! Hmm...
> 
>> +       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);
>> +}
> 
> ...
> 
>> +       for (index = 0; index < num_channels; index++) {
>> +               if ((status & BIT(index))) {
> 
> If gpio_width is the same among banks, you can use for_each_set_bit()
> here as well.
> 
> ...
> 
>> +                       for_each_set_bit(bit, &all_events, 32) {
>> +                               generic_handle_irq(irq_find_mapping
>> +                                       (chip->gc.irq.domain, offset + bit));
> 
> Strange indentation. Maybe a temporary variable helps?
> 
> ...
> 
>> +       chip->irq = platform_get_irq_optional(pdev, 0);
>> +       if (chip->irq <= 0) {
>> +               dev_info(&pdev->dev, "GPIO IRQ not set\n");
> 
> Why do you need an optional variant if you print an error anyway?
> 
>> +       } else {
> 
> 
> ...
> 
>> +               chip->gc.irq.parents = (unsigned int *)&chip->irq;
>> +               chip->gc.irq.num_parents = 1;
> 
> Current pattern is to use devm_kcalloc() for it (Linus has plans to
> simplify this in the future and this will help him to find what
> patterns are being used)
> 

-- 
Robert Hancock
Senior Hardware Designer
SED Systems, a division of Calian Ltd.
Email: hancock@sedsystems.ca

Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3576 bytes --]

Hi Srinivas,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on gpio/for-next]
[also build test ERROR on linus/master v5.8-rc6 next-20200723]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Srinivas-Neeli/gpio-xilinx-Update-on-xilinx-gpio-driver/20200723-220826
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: x86_64-randconfig-a003-20200723 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-xilinx.c: In function 'xgpio_xlate':
>> drivers/gpio/gpio-xilinx.c:325:8: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
     325 |  if (gc->of_gpio_n_cells < 2) {
         |        ^~
   In file included from arch/x86/include/asm/bug.h:92,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from arch/x86/include/asm/preempt.h:7,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:51,
                    from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/linux/stat.h:19,
                    from include/linux/module.h:13,
                    from drivers/gpio/gpio-xilinx.c:11:
   drivers/gpio/gpio-xilinx.c:330:39: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
     330 |  if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
         |                                       ^~
   include/asm-generic/bug.h:118:25: note: in definition of macro 'WARN_ON'
     118 |  int __ret_warn_on = !!(condition);    \
         |                         ^~~~~~~~~
   drivers/gpio/gpio-xilinx.c: In function 'xgpio_probe':
   drivers/gpio/gpio-xilinx.c:638:10: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
     638 |  chip->gc.of_gpio_n_cells = cells;
         |          ^
>> drivers/gpio/gpio-xilinx.c:639:10: error: 'struct gpio_chip' has no member named 'of_xlate'
     639 |  chip->gc.of_xlate = xgpio_xlate;
         |          ^

vim +325 drivers/gpio/gpio-xilinx.c

   312	
   313	/**
   314	 * xgpio_xlate - Translate gpio_spec to the GPIO number and flags
   315	 * @gc: Pointer to gpio_chip device structure.
   316	 * @gpiospec:  gpio specifier as found in the device tree
   317	 * @flags: A flags pointer based on binding
   318	 *
   319	 * Return:
   320	 * irq number otherwise -EINVAL
   321	 */
   322	static int xgpio_xlate(struct gpio_chip *gc,
   323			       const struct of_phandle_args *gpiospec, u32 *flags)
   324	{
 > 325		if (gc->of_gpio_n_cells < 2) {
   326			WARN_ON(1);
   327			return -EINVAL;
   328		}
   329	
   330		if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
   331			return -EINVAL;
   332	
   333		if (gpiospec->args[0] >= gc->ngpio)
   334			return -EINVAL;
   335	
   336		if (flags)
   337			*flags = gpiospec->args[1];
   338	
   339		return gpiospec->args[0];
   340	}
   341	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39376 bytes --]

Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support

[Linus Walleij]

On Fri, Jul 24, 2020 at 6:12 AM kernel test robot <lkp@intel.com> wrote:

>    drivers/gpio/gpio-xilinx.c: In function 'xgpio_probe':
>    drivers/gpio/gpio-xilinx.c:638:10: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
>      638 |  chip->gc.of_gpio_n_cells = cells;
>          |          ^
> >> drivers/gpio/gpio-xilinx.c:639:10: error: 'struct gpio_chip' has no member named 'of_xlate'
>      639 |  chip->gc.of_xlate = xgpio_xlate;
>          |          ^

This is probably because your driver needs:

depends on OF_GPIO

in KConfig

Yours,
Linus Walleij

RE: [PATCH V2 2/3] gpio: xilinx: Add interrupt support

[Srinivas Neeli]

Hi Linus,

Thanks for the review

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Friday, July 24, 2020 2:52 PM
> To: kernel test robot <lkp@intel.com>
> Cc: Srinivas Neeli <sneeli@xilinx.com>; Bartosz Golaszewski
> <bgolaszewski@baylibre.com>; Michal Simek <michals@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Srinivas Goud
> <sgoud@xilinx.com>; kbuild-all@lists.01.org; open list:GPIO SUBSYSTEM
> <linux-gpio@vger.kernel.org>; Linux ARM <linux-arm-
> kernel@lists.infradead.org>; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>
> Subject: Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
> 
> On Fri, Jul 24, 2020 at 6:12 AM kernel test robot <lkp@intel.com> wrote:
> 
> >    drivers/gpio/gpio-xilinx.c: In function 'xgpio_probe':
> >    drivers/gpio/gpio-xilinx.c:638:10: error: 'struct gpio_chip' has no member
> named 'of_gpio_n_cells'
> >      638 |  chip->gc.of_gpio_n_cells = cells;
> >          |          ^
> > >> drivers/gpio/gpio-xilinx.c:639:10: error: 'struct gpio_chip' has no
> member named 'of_xlate'
> >      639 |  chip->gc.of_xlate = xgpio_xlate;
> >          |          ^
> 
> This is probably because your driver needs:
> 
> depends on OF_GPIO
> 
> in KConfig
> 
I will address in v3.

> Yours,
> Linus Walleij

[PATCH V2 3/3] MAINTAINERS: add fragment for xilinx GPIO drivers

[Srinivas Neeli]

Added entry for xilinx GPIO drivers.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ea296f213e45..71c40b0ddef6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18900,6 +18900,16 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/net/can/xilinx_can.txt
 F:	drivers/net/can/xilinx_can.c
 
+XILINX GPIO DRIVER
+M:	Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
+R:	Srinivas Neeli <srinivas.neeli@xilinx.com>
+R:	Michal Simek <michal.simek@xilinx.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/gpio/gpio-xilinx.txt
+F:	Documentation/devicetree/bindings/gpio/gpio-zynq.txt
+F:	drivers/gpio/gpio-xilinx.c
+F:	drivers/gpio/gpio-zynq.c
+
 XILINX SD-FEC IP CORES
 M:	Derek Kiernan <derek.kiernan@xilinx.com>
 M:	Dragan Cvetic <dragan.cvetic@xilinx.com>
-- 
2.7.4

Re: [PATCH V2 3/3] MAINTAINERS: add fragment for xilinx GPIO drivers

[Michal Simek]

On 23. 07. 20 16:06, Srinivas Neeli wrote:
> Added entry for xilinx GPIO drivers.
> 
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
>  MAINTAINERS | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ea296f213e45..71c40b0ddef6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18900,6 +18900,16 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/net/can/xilinx_can.txt
>  F:	drivers/net/can/xilinx_can.c
>  
> +XILINX GPIO DRIVER
> +M:	Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> +R:	Srinivas Neeli <srinivas.neeli@xilinx.com>
> +R:	Michal Simek <michal.simek@xilinx.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/gpio/gpio-xilinx.txt
> +F:	Documentation/devicetree/bindings/gpio/gpio-zynq.txt
> +F:	drivers/gpio/gpio-xilinx.c
> +F:	drivers/gpio/gpio-zynq.c
> +
>  XILINX SD-FEC IP CORES
>  M:	Derek Kiernan <derek.kiernan@xilinx.com>
>  M:	Dragan Cvetic <dragan.cvetic@xilinx.com>
> 

Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

Re: [PATCH V2 3/3] MAINTAINERS: add fragment for xilinx GPIO drivers

[Shubhrajyoti Datta]

On Thu, Jul 23, 2020 at 7:37 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:
>
> Added entry for xilinx GPIO drivers.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
Acked-by: : Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>

RE: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver

[Srinivas Neeli]

Hi Robert,

Will send the next version(v3) after Mainline Window close.

Thanks
Srinivas Neeli.

> -----Original Message-----
> From: Robert Hancock <robert.hancock@calian.com>
> Sent: Monday, October 19, 2020 11:18 PM
> To: Srinivas Neeli <sneeli@xilinx.com>; Michal Simek <michals@xilinx.com>;
> Srinivas Goud <sgoud@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>
> Cc: git <git@xilinx.com>; linux-gpio <linux-gpio@vger.kernel.org>
> Subject: Re: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver
> 
> Hi Srinivas,
> 
> Just checking if there is any update on this patchset? It looks like there was
> some feedback but I didn't see another version submitted.
> 
> Robert Hancock
> Senior Hardware Designer, Advanced Technologies www.calian.com
> www.sedsystems.ca
> 
> From: Srinivas Neeli <srinivas.neeli@xilinx.com>
> Sent: July 23, 2020 8:36 AM
> To: bgolaszewski <bgolaszewski@baylibre.com>; linus.walleij
> <linus.walleij@linaro.org>; michal.simek <michal.simek@xilinx.com>; sgoud
> <sgoud@xilinx.com>; shubhrajyoti.datta <shubhrajyoti.datta@xilinx.com>
> Cc: git <git@xilinx.com>; linux-arm-kernel <linux-arm-
> kernel@lists.infradead.org>; linux-gpio <linux-gpio@vger.kernel.org>; linux-
> kernel <linux-kernel@vger.kernel.org>
> Subject: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver
> 
> This patch series does the following:
> -Add clock adaption support
> -Add interupt support
> -Add MAINTAINERS fragment
> 
> 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.
> ---
> Tested Below scenarios:
> -Tested Loop Back.(channel 1.0 connected to channel 2.0) -Tested External
> switch(Used DIP switch) -Tested Cascade scenario(Here gpio controller acting
> as
>  an interrupt controller).
> ---
> 
> 
> Srinivas Neeli (3):
>   gpio: xilinx: Add clock adaptation support
>   gpio: xilinx: Add interrupt support
>   MAINTAINERS: add fragment for xilinx GPIO drivers
> 
>  MAINTAINERS                |  10 ++
>  drivers/gpio/Kconfig       |   1 +
>  drivers/gpio/gpio-xilinx.c | 404
> ++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 395 insertions(+), 20 deletions(-)
> 
> --
> 2.7.4

Re: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver

[Michal Simek]

Hi,

let me correct this. Merge Window. :-)
5.10-rc1 should be tagged on Sunday Oct 25. After that patches can be
send again.

Thanks,
Michal

On 20. 10. 20 8:59, Srinivas Neeli wrote:
> Hi Robert,
> 
> Will send the next version(v3) after Mainline Window close.
> 
> Thanks
> Srinivas Neeli.
> 
>> -----Original Message-----
>> From: Robert Hancock <robert.hancock@calian.com>
>> Sent: Monday, October 19, 2020 11:18 PM
>> To: Srinivas Neeli <sneeli@xilinx.com>; Michal Simek <michals@xilinx.com>;
>> Srinivas Goud <sgoud@xilinx.com>; Shubhrajyoti Datta
>> <shubhraj@xilinx.com>
>> Cc: git <git@xilinx.com>; linux-gpio <linux-gpio@vger.kernel.org>
>> Subject: Re: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver
>>
>> Hi Srinivas,
>>
>> Just checking if there is any update on this patchset? It looks like there was
>> some feedback but I didn't see another version submitted.
>>
>> Robert Hancock
>> Senior Hardware Designer, Advanced Technologies www.calian.com
>> www.sedsystems.ca
>>
>> From: Srinivas Neeli <srinivas.neeli@xilinx.com>
>> Sent: July 23, 2020 8:36 AM
>> To: bgolaszewski <bgolaszewski@baylibre.com>; linus.walleij
>> <linus.walleij@linaro.org>; michal.simek <michal.simek@xilinx.com>; sgoud
>> <sgoud@xilinx.com>; shubhrajyoti.datta <shubhrajyoti.datta@xilinx.com>
>> Cc: git <git@xilinx.com>; linux-arm-kernel <linux-arm-
>> kernel@lists.infradead.org>; linux-gpio <linux-gpio@vger.kernel.org>; linux-
>> kernel <linux-kernel@vger.kernel.org>
>> Subject: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver
>>
>> This patch series does the following:
>> -Add clock adaption support
>> -Add interupt support
>> -Add MAINTAINERS fragment
>>
>> 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.
>> ---
>> Tested Below scenarios:
>> -Tested Loop Back.(channel 1.0 connected to channel 2.0) -Tested External
>> switch(Used DIP switch) -Tested Cascade scenario(Here gpio controller acting
>> as
>>  an interrupt controller).
>> ---
>>
>>
>> Srinivas Neeli (3):
>>   gpio: xilinx: Add clock adaptation support
>>   gpio: xilinx: Add interrupt support
>>   MAINTAINERS: add fragment for xilinx GPIO drivers
>>
>>  MAINTAINERS                |  10 ++
>>  drivers/gpio/Kconfig       |   1 +
>>  drivers/gpio/gpio-xilinx.c | 404
>> ++++++++++++++++++++++++++++++++++++++++++---
>>  3 files changed, 395 insertions(+), 20 deletions(-)
>>
>> --
>> 2.7.4

Re: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver

[Robert Hancock]

On Tue, 2020-10-20 at 09:22 +0200, Michal Simek wrote:
> Hi,
> 
> let me correct this. Merge Window. :-)
> 5.10-rc1 should be tagged on Sunday Oct 25. After that patches can be
> send again.
> 
> Thanks,
> Michal
> 

Hi, just wondering if you are still planning to submit a v3 of this
patchset?

> On 20. 10. 20 8:59, Srinivas Neeli wrote:
> > Hi Robert,
> > 
> > Will send the next version(v3) after Mainline Window close.
> > 
> > Thanks
> > Srinivas Neeli.
> > 
> > > -----Original Message-----
> > > From: Robert Hancock <robert.hancock@calian.com>
> > > Sent: Monday, October 19, 2020 11:18 PM
> > > To: Srinivas Neeli <sneeli@xilinx.com>; Michal Simek <
> > > michals@xilinx.com>;
> > > Srinivas Goud <sgoud@xilinx.com>; Shubhrajyoti Datta
> > > <shubhraj@xilinx.com>
> > > Cc: git <git@xilinx.com>; linux-gpio <linux-gpio@vger.kernel.org>
> > > Subject: Re: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio
> > > driver
> > > 
> > > Hi Srinivas,
> > > 
> > > Just checking if there is any update on this patchset? It looks
> > > like there was
> > > some feedback but I didn't see another version submitted.
> > > 
> > > Robert Hancock
> > > Senior Hardware Designer, Advanced Technologies 
> > > https://can01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.calian.com%2F&amp;data=04%7C01%7Crobert.hancock%40calian.com%7Cd2512bd3dbac441f436108d874c8ff30%7C23b57807562f49ad92c43bb0f07a1fdf%7C0%7C0%7C637387753918575684%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nintj27wpUCrWfCJggVTKRqanPiM7AnXH5M2Vcst14c%3D&amp;reserved=0
> > > https://can01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.sedsystems.ca%2F&amp;data=04%7C01%7Crobert.hancock%40calian.com%7Cd2512bd3dbac441f436108d874c8ff30%7C23b57807562f49ad92c43bb0f07a1fdf%7C0%7C0%7C637387753918575684%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Y0%2Fhy01tuwvrFUAszMqkit6C3Qam9Ktr1q5vNbqkoLc%3D&amp;reserved=0
> > > 
> > > From: Srinivas Neeli <srinivas.neeli@xilinx.com>
> > > Sent: July 23, 2020 8:36 AM
> > > To: bgolaszewski <bgolaszewski@baylibre.com>; linus.walleij
> > > <linus.walleij@linaro.org>; michal.simek <michal.simek@xilinx.com
> > > >; sgoud
> > > <sgoud@xilinx.com>; shubhrajyoti.datta <
> > > shubhrajyoti.datta@xilinx.com>
> > > Cc: git <git@xilinx.com>; linux-arm-kernel <linux-arm-
> > > kernel@lists.infradead.org>; linux-gpio <
> > > linux-gpio@vger.kernel.org>; linux-
> > > kernel <linux-kernel@vger.kernel.org>
> > > Subject: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver
> > > 
> > > This patch series does the following:
> > > -Add clock adaption support
> > > -Add interupt support
> > > -Add MAINTAINERS fragment
> > > 
> > > 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.
> > > ---
> > > Tested Below scenarios:
> > > -Tested Loop Back.(channel 1.0 connected to channel 2.0) -Tested
> > > External
> > > switch(Used DIP switch) -Tested Cascade scenario(Here gpio
> > > controller acting
> > > as
> > >  an interrupt controller).
> > > ---
> > > 
> > > 
> > > Srinivas Neeli (3):
> > >   gpio: xilinx: Add clock adaptation support
> > >   gpio: xilinx: Add interrupt support
> > >   MAINTAINERS: add fragment for xilinx GPIO drivers
> > > 
> > >  MAINTAINERS                |  10 ++
> > >  drivers/gpio/Kconfig       |   1 +
> > >  drivers/gpio/gpio-xilinx.c | 404
> > > ++++++++++++++++++++++++++++++++++++++++++---
> > >  3 files changed, 395 insertions(+), 20 deletions(-)
> > > 
> > > --
> > > 2.7.4
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

RE: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver

[Srinivas Neeli]

Hi Robert,

> -----Original Message-----
> From: Robert Hancock <robert.hancock@calian.com>
> Sent: Saturday, November 7, 2020 3:59 AM
> To: Shubhrajyoti Datta <shubhraj@xilinx.com>; Michal Simek
> <michals@xilinx.com>; Srinivas Neeli <sneeli@xilinx.com>; Srinivas Goud
> <sgoud@xilinx.com>
> Cc: git <git@xilinx.com>; linux-gpio@vger.kernel.org
> Subject: Re: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver
> 
> On Tue, 2020-10-20 at 09:22 +0200, Michal Simek wrote:
> > Hi,
> >
> > let me correct this. Merge Window. :-)
> > 5.10-rc1 should be tagged on Sunday Oct 25. After that patches can be
> > send again.
> >
> > Thanks,
> > Michal
> >
> 
> Hi, just wondering if you are still planning to submit a v3 of this patchset?

working on it, have to test some functions , will send this week

Thanks
Srinivas Neeli

> 
> > On 20. 10. 20 8:59, Srinivas Neeli wrote:
> > > Hi Robert,
> > >
> > > Will send the next version(v3) after Mainline Window close.
> > >
> > > Thanks
> > > Srinivas Neeli.
> > >
> > > > -----Original Message-----
> > > > From: Robert Hancock <robert.hancock@calian.com>
> > > > Sent: Monday, October 19, 2020 11:18 PM
> > > > To: Srinivas Neeli <sneeli@xilinx.com>; Michal Simek <
> > > > michals@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>;
> > > > Shubhrajyoti Datta <shubhraj@xilinx.com>
> > > > Cc: git <git@xilinx.com>; linux-gpio <linux-gpio@vger.kernel.org>
> > > > Subject: Re: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio
> > > > driver
> > > >
> > > > Hi Srinivas,
> > > >
> > > > Just checking if there is any update on this patchset? It looks
> > > > like there was some feedback but I didn't see another version
> > > > submitted.
> > > >
> > > > Robert Hancock
> > > > Senior Hardware Designer, Advanced Technologies
> > > >
> https://can01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fw
> > > >
> ww.calian.com%2F&amp;data=04%7C01%7Crobert.hancock%40calian.com%
> 7C
> > > >
> d2512bd3dbac441f436108d874c8ff30%7C23b57807562f49ad92c43bb0f07a1fd
> > > >
> f%7C0%7C0%7C637387753918575684%7CUnknown%7CTWFpbGZsb3d8eyJWI
> joiMC4
> > > >
> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&a
> mp
> > > >
> ;sdata=nintj27wpUCrWfCJggVTKRqanPiM7AnXH5M2Vcst14c%3D&amp;reser
> ved
> > > > =0
> > > >
> https://can01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fw
> > > >
> ww.sedsystems.ca%2F&amp;data=04%7C01%7Crobert.hancock%40calian.co
> m
> > > >
> %7Cd2512bd3dbac441f436108d874c8ff30%7C23b57807562f49ad92c43bb0f07a
> > > >
> 1fdf%7C0%7C0%7C637387753918575684%7CUnknown%7CTWFpbGZsb3d8ey
> JWIjoi
> > > >
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100
> 0&
> > > >
> amp;sdata=Y0%2Fhy01tuwvrFUAszMqkit6C3Qam9Ktr1q5vNbqkoLc%3D&am
> p;res
> > > > erved=0
> > > >
> > > > From: Srinivas Neeli <srinivas.neeli@xilinx.com>
> > > > Sent: July 23, 2020 8:36 AM
> > > > To: bgolaszewski <bgolaszewski@baylibre.com>; linus.walleij
> > > > <linus.walleij@linaro.org>; michal.simek <michal.simek@xilinx.com
> > > > >; sgoud
> > > > <sgoud@xilinx.com>; shubhrajyoti.datta <
> > > > shubhrajyoti.datta@xilinx.com>
> > > > Cc: git <git@xilinx.com>; linux-arm-kernel <linux-arm-
> > > > kernel@lists.infradead.org>; linux-gpio <
> > > > linux-gpio@vger.kernel.org>; linux- kernel
> > > > <linux-kernel@vger.kernel.org>
> > > > Subject: [PATCH V2 0/3] gpio-xilinx: Update on xilinx gpio driver
> > > >
> > > > This patch series does the following:
> > > > -Add clock adaption support
> > > > -Add interupt support
> > > > -Add MAINTAINERS fragment
> > > >
> > > > 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.
> > > > ---
> > > > Tested Below scenarios:
> > > > -Tested Loop Back.(channel 1.0 connected to channel 2.0) -Tested
> > > > External switch(Used DIP switch) -Tested Cascade scenario(Here
> > > > gpio controller acting as  an interrupt controller).
> > > > ---
> > > >
> > > >
> > > > Srinivas Neeli (3):
> > > >   gpio: xilinx: Add clock adaptation support
> > > >   gpio: xilinx: Add interrupt support
> > > >   MAINTAINERS: add fragment for xilinx GPIO drivers
> > > >
> > > >  MAINTAINERS                |  10 ++
> > > >  drivers/gpio/Kconfig       |   1 +
> > > >  drivers/gpio/gpio-xilinx.c | 404
> > > > ++++++++++++++++++++++++++++++++++++++++++---
> > > >  3 files changed, 395 insertions(+), 20 deletions(-)
> > > >
> > > > --
> > > > 2.7.4
> --
> Robert Hancock
> Senior Hardware Designer, Calian Advanced Technologies www.calian.com