[LINUX PATCH V3 0/9] gpio-xilinx: Update on xilinx gpio driver

[LINUX PATCH V3 0/9] gpio-xilinx: Update on xilinx gpio driver

[Srinivas Neeli]

This patch series does the following:
-Add clock support
-Add interrupt support
-Add support for suspend and resume
-Add remove support
-Add MAINTAINERS fragment
---
Changes in V3:
-Created separate patch to arrange headers in sorting order.
-Updated dt-bindings.
-Created separate patch for Clock changes and runtime resume.
 and suspend.
-Created separate patch for spinlock changes.
-Created separate patch for remove support.
-Fixed coverity errors.
-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.

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 (9):
  gpio: gpio-xilinx: Arrange headers in sorting order
  dt-bindings: gpio: gpio-xilinx: Add clk support to xilinx soft gpio IP
  gpio: gpio-xilinx: Add clock support
  gpio: gpio-xilinx: Reduce spinlock array to single
  gpio: gpio-xilinx: Add interrupt support
  gpio: gpio-xilinx: Add remove function
  gpio: gpio-xilinx: Add support for suspend and resume
  gpio: gpio-xilinx: Check return value of of_property_read_u32
  MAINTAINERS: add fragment for xilinx GPIO drivers

 .../devicetree/bindings/gpio/gpio-xilinx.txt       |   2 +
 MAINTAINERS                                        |  10 +
 drivers/gpio/Kconfig                               |   2 +
 drivers/gpio/gpio-xilinx.c                         | 398 +++++++++++++++++++--
 4 files changed, 390 insertions(+), 22 deletions(-)

-- 
2.7.4

[LINUX PATCH V3 1/9] gpio: gpio-xilinx: Arrange headers in sorting order

[Srinivas Neeli]

Arrange header files in sorted order.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Changes in V3:
-Created new patch for sorting header files.
---
 drivers/gpio/gpio-xilinx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 3ba1a993c85e..17a8a8f90d84 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -6,13 +6,13 @@
  */
 
 #include <linux/bitops.h>
-#include <linux/init.h>
 #include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
-#include <linux/io.h>
-#include <linux/gpio/driver.h>
 #include <linux/slab.h>
 
 /* Register Offset Definitions */
-- 
2.7.4

Re: [LINUX PATCH V3 1/9] gpio: gpio-xilinx: Arrange headers in sorting order

[Linus Walleij]

On Thu, Nov 12, 2020 at 6:12 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:

> Arrange header files in sorted order.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
> Changes in V3:
> -Created new patch for sorting header files.

Patch applied.

Yours,
Linus Walleij

[LINUX PATCH V3 2/9] dt-bindings: gpio: gpio-xilinx: Add clk support to xilinx soft gpio IP

[Srinivas Neeli]

Specify clock property in binding.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Changes in V3:
-Created new patch for dt-bindings.
---
 Documentation/devicetree/bindings/gpio/gpio-xilinx.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-xilinx.txt b/Documentation/devicetree/bindings/gpio/gpio-xilinx.txt
index 08eed2335db0..e506f30e1a95 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-xilinx.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-xilinx.txt
@@ -13,6 +13,7 @@ Required properties:
 - gpio-controller : Marks the device node as a GPIO controller.
 
 Optional properties:
+- clocks : Input clock specifier. Refer to common clock bindings.
 - interrupts : Interrupt mapping for GPIO IRQ.
 - xlnx,all-inputs : if n-th bit is setup, GPIO-n is input
 - xlnx,dout-default : if n-th bit is 1, GPIO-n default value is 1
@@ -29,6 +30,7 @@ Example:
 gpio: gpio@40000000 {
 	#gpio-cells = <2>;
 	compatible = "xlnx,xps-gpio-1.00.a";
+	clocks = <&clkc25>;
 	gpio-controller ;
 	interrupt-parent = <&microblaze_0_intc>;
 	interrupts = < 6 2 >;
-- 
2.7.4

Re: [LINUX PATCH V3 2/9] dt-bindings: gpio: gpio-xilinx: Add clk support to xilinx soft gpio IP

[Linus Walleij]

On Thu, Nov 12, 2020 at 6:12 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:

> Specify clock property in binding.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>

Patch applied, consider rewriting this binding in YAML.

Yours,
Linus Walleij

[LINUX PATCH V3 3/9] gpio: gpio-xilinx: Add clock support

[Srinivas Neeli]

Adds clock support to the Xilinx GPIO driver.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Chnages in V3:
-Created separate patch for Clock changes.
---
 drivers/gpio/gpio-xilinx.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 17a8a8f90d84..99d603bfb6f0 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
 #include <linux/init.h>
@@ -38,6 +39,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;
@@ -46,6 +48,7 @@ struct xgpio_instance {
 	u32 gpio_state[2];
 	u32 gpio_dir[2];
 	spinlock_t gpio_lock[2];
+	struct clk *clk;
 };
 
 static inline int xgpio_index(struct xgpio_instance *chip, int gpio)
@@ -333,11 +336,25 @@ static int xgpio_probe(struct platform_device *pdev)
 		return PTR_ERR(chip->regs);
 	}
 
+	chip->clk = devm_clk_get_optional(&pdev->dev, NULL);
+	if (IS_ERR(chip->clk)) {
+		if (PTR_ERR(chip->clk) != -EPROBE_DEFER)
+			dev_dbg(&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;
+	}
+
 	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");
+		clk_disable_unprepare(chip->clk);
 		return status;
 	}
 
-- 
2.7.4

Re: [LINUX PATCH V3 3/9] gpio: gpio-xilinx: Add clock support

[Linus Walleij]

On Thu, Nov 12, 2020 at 6:13 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:

> Adds clock support to the Xilinx GPIO driver.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>

(...)
> +       chip->clk = devm_clk_get_optional(&pdev->dev, NULL);
> +       if (IS_ERR(chip->clk)) {
> +               if (PTR_ERR(chip->clk) != -EPROBE_DEFER)
> +                       dev_dbg(&pdev->dev, "Input clock not found\n");
> +               return PTR_ERR(chip->clk);
> +       }

You can now use return dev_err_probe(dev, ret, "failed to get clock\n");
to avoid all the comparing with -EPROBE_DEFER.

Yours,
Linus Walleij

Re: [LINUX PATCH V3 3/9] gpio: gpio-xilinx: Add clock support

[Linus Walleij]

On Wed, Nov 18, 2020 at 12:53 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Nov 12, 2020 at 6:13 PM Srinivas Neeli
> <srinivas.neeli@xilinx.com> wrote:
>
> > Adds clock support to the Xilinx GPIO driver.
> >
> > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
>
> (...)
> > +       chip->clk = devm_clk_get_optional(&pdev->dev, NULL);
> > +       if (IS_ERR(chip->clk)) {
> > +               if (PTR_ERR(chip->clk) != -EPROBE_DEFER)
> > +                       dev_dbg(&pdev->dev, "Input clock not found\n");
> > +               return PTR_ERR(chip->clk);
> > +       }
>
> You can now use return dev_err_probe(dev, ret, "failed to get clock\n");
> to avoid all the comparing with -EPROBE_DEFER.

Patch applied anyways, this can be done separately.

Yours,
Linus Walleij

[LINUX PATCH V3 4/9] gpio: gpio-xilinx: Reduce spinlock array to single

[Srinivas Neeli]

Changed spinlock array to single. It is preparation for irq support which
is shared between two channels that's why spinlock should be only one.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Changes in V3:
-Created new patch for spinlock changes.
---
 drivers/gpio/gpio-xilinx.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 99d603bfb6f0..69bdf1910215 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -47,7 +47,7 @@ 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;	/* For serializing operations */
 	struct clk *clk;
 };
 
@@ -113,7 +113,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)
@@ -124,7 +124,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);
 }
 
 /**
@@ -148,8 +148,7 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 	DECLARE_BITMAP(new, 64);
 	DECLARE_BITMAP(changed, 64);
 
-	spin_lock_irqsave(&chip->gpio_lock[0], flags);
-	spin_lock(&chip->gpio_lock[1]);
+	spin_lock_irqsave(&chip->gpio_lock, flags);
 
 	bitmap_set_value(old, state[0], 0, width[0]);
 	bitmap_set_value(old, state[1], width[0], width[1]);
@@ -170,8 +169,7 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 		xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET +
 				XGPIO_CHANNEL_OFFSET, state[1]);
 
-	spin_unlock(&chip->gpio_lock[1]);
-	spin_unlock_irqrestore(&chip->gpio_lock[0], flags);
+	spin_unlock_irqrestore(&chip->gpio_lock, flags);
 }
 
 /**
@@ -190,14 +188,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;
 }
@@ -221,7 +219,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)
@@ -236,7 +234,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,8 +292,7 @@ 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[1]);
+	spin_lock_init(&chip->gpio_lock);
 
 	if (of_property_read_u32(np, "xlnx,is-dual", &is_dual))
 		is_dual = 0;
-- 
2.7.4

Re: [LINUX PATCH V3 4/9] gpio: gpio-xilinx: Reduce spinlock array to single

[Linus Walleij]

On Thu, Nov 12, 2020 at 6:12 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:

> Changed spinlock array to single. It is preparation for irq support which
> is shared between two channels that's why spinlock should be only one.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>

This patch does not apply to my "devel" tree.

Yours,
Linus Walleij

[LINUX PATCH V3 5/9] gpio: 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>
---
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
 	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..855550a06ded 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -10,9 +10,12 @@
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/of_platform.h>
 #include <linux/slab.h>
 
@@ -22,6 +25,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)
@@ -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 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);
+
+	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 -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;
+			unsigned int irq;
+
+			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) {
+				irq = irq_find_mapping(chip->gc.irq.domain,
+						       offset + bit);
+				generic_handle_irq(irq);
+			}
+		}
+		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
  *
@@ -270,7 +465,10 @@ 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;
+	struct gpio_irq_chip *girq;
+	u32 temp;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
@@ -285,6 +483,10 @@ static int xgpio_probe(struct platform_device *pdev)
 	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_dbg(&pdev->dev, "Missing gpio-cells property\n");
+
 	/*
 	 * Check device node and parent device node for device width
 	 * and assume default width of 32
@@ -321,6 +523,7 @@ 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.get = xgpio_get;
 	chip->gc.set = xgpio_set;
 	chip->gc.set_multiple = xgpio_set_multiple;
@@ -348,14 +551,45 @@ static int xgpio_probe(struct platform_device *pdev)
 
 	xgpio_save_regs(chip);
 
+	chip->irq = platform_get_irq_optional(pdev, 0);
+	if (chip->irq <= 0)
+		goto skip_irq;
+
+	/* 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);
+
+	girq = &chip->gc.irq;
+	girq->chip = &xgpio_irqchip;
+	girq->parent_handler = xgpio_irqhandler;
+	girq->num_parents = 1;
+	girq->parents = devm_kcalloc(&pdev->dev, 1,
+				     sizeof(*girq->parents),
+				     GFP_KERNEL);
+	if (!girq->parents) {
+		status = -ENOMEM;
+		goto err_unprepare_clk;
+	}
+	girq->parents[0] = chip->irq;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_bad_irq;
+
+skip_irq:
 	status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
 	if (status) {
 		dev_err(&pdev->dev, "failed to add GPIO chip\n");
-		clk_disable_unprepare(chip->clk);
-		return status;
+		goto err_unprepare_clk;
 	}
 
 	return 0;
+
+err_unprepare_clk:
+	clk_disable_unprepare(chip->clk);
+	return status;
 }
 
 static const struct of_device_id xgpio_of_match[] = {
-- 
2.7.4

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

[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 <hancock@sedsystems.ca>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
> 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..855550a06ded 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -10,9 +10,12 @@
>  #include <linux/errno.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/init.h>
> +#include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/irq.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/of_gpio.h>
>  #include <linux/of_platform.h>
>  #include <linux/slab.h>
>  
> @@ -22,6 +25,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)
> @@ -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 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);
> +
> +	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 -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;
> +			unsigned int irq;
> +
> +			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) {
> +				irq = irq_find_mapping(chip-
> >gc.irq.domain,
> +						       offset + bit);
> +				generic_handle_irq(irq);
> +			}
> +		}
> +		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
>   *
> @@ -270,7 +465,10 @@ 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;
> +	struct gpio_irq_chip *girq;
> +	u32 temp;
>  
>  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>  	if (!chip)
> @@ -285,6 +483,10 @@ static int xgpio_probe(struct platform_device
> *pdev)
>  	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_dbg(&pdev->dev, "Missing gpio-cells property\n");
> +
>  	/*
>  	 * Check device node and parent device node for device width
>  	 * and assume default width of 32
> @@ -321,6 +523,7 @@ 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.get = xgpio_get;
>  	chip->gc.set = xgpio_set;
>  	chip->gc.set_multiple = xgpio_set_multiple;
> @@ -348,14 +551,45 @@ static int xgpio_probe(struct platform_device
> *pdev)
>  
>  	xgpio_save_regs(chip);
>  
> +	chip->irq = platform_get_irq_optional(pdev, 0);
> +	if (chip->irq <= 0)
> +		goto skip_irq;
> +
> +	/* 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);
> +
> +	girq = &chip->gc.irq;
> +	girq->chip = &xgpio_irqchip;
> +	girq->parent_handler = xgpio_irqhandler;
> +	girq->num_parents = 1;
> +	girq->parents = devm_kcalloc(&pdev->dev, 1,
> +				     sizeof(*girq->parents),
> +				     GFP_KERNEL);
> +	if (!girq->parents) {
> +		status = -ENOMEM;
> +		goto err_unprepare_clk;
> +	}
> +	girq->parents[0] = chip->irq;
> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_bad_irq;
> +
> +skip_irq:
>  	status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
>  	if (status) {
>  		dev_err(&pdev->dev, "failed to add GPIO chip\n");
> -		clk_disable_unprepare(chip->clk);
> -		return status;
> +		goto err_unprepare_clk;
>  	}
>  
>  	return 0;
> +
> +err_unprepare_clk:
> +	clk_disable_unprepare(chip->clk);
> +	return status;
>  }
>  
>  static const struct of_device_id xgpio_of_match[] = {
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

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

[Linus Walleij]

Hi Srinivas!

On Thu, Nov 12, 2020 at 6:12 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.
>
> 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>

(...)
>  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

[LINUX PATCH V3 6/9] gpio: gpio-xilinx: Add remove function

[Srinivas Neeli]

Added remove function support.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Changes in V3:
-Created new patch for remove function.
---
 drivers/gpio/gpio-xilinx.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 855550a06ded..9abef56eca32 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -453,6 +453,24 @@ static void xgpio_irqhandler(struct irq_desc *desc)
 }
 
 /**
+ * xgpio_remove - Remove method for the GPIO device.
+ * @pdev: pointer to the platform device
+ *
+ * This function remove gpiochips and frees all the allocated resources.
+ *
+ * Return: 0 always
+ */
+static int xgpio_remove(struct platform_device *pdev)
+{
+	struct xgpio_instance *gpio = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(gpio->clk);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+/**
  * xgpio_of_probe - Probe method for the GPIO device.
  * @pdev: pointer to the platform device
  *
@@ -601,6 +619,7 @@ MODULE_DEVICE_TABLE(of, xgpio_of_match);
 
 static struct platform_driver xgpio_plat_driver = {
 	.probe		= xgpio_probe,
+	.remove		= xgpio_remove,
 	.driver		= {
 			.name = "gpio-xilinx",
 			.of_match_table	= xgpio_of_match,
-- 
2.7.4

Re: [LINUX PATCH V3 6/9] gpio: gpio-xilinx: Add remove function

[Linus Walleij]

On Thu, Nov 12, 2020 at 6:13 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:

> Added remove function support.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
> Changes in V3:
> -Created new patch for remove function.

Patch applied despite not applying 4 or 5: this needs to go
in with the clock support.

Yours,
Linus Walleij

[LINUX PATCH V3 7/9] gpio: gpio-xilinx: Add support for suspend and resume

[Srinivas Neeli]

Add support for suspend and resume, pm runtime suspend and resume.
Added free and request calls.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Changes in V3:
-Created new patch for suspend and resume.
---
 drivers/gpio/gpio-xilinx.c | 89 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 87 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 9abef56eca32..48f2a6c894f5 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -17,6 +17,7 @@
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
 #include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 /* Register Offset Definitions */
@@ -275,6 +276,39 @@ 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;
+
+	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 xgpio_instance *gpio = dev_get_drvdata(dev);
+	struct irq_data *data = irq_get_irq_data(gpio->irq);
+
+	if (!data) {
+		dev_err(dev, "irq_get_irq_data() failed\n");
+		return -EINVAL;
+	}
+
+	if (!irqd_is_wakeup_set(data))
+		return pm_runtime_force_suspend(dev);
+
+	return 0;
+}
+
 /**
  * xgpio_irq_ack - Acknowledge a child GPIO interrupt.
  * @irq_data: per IRQ and chip data passed down to chip functions
@@ -285,6 +319,46 @@ static void xgpio_irq_ack(struct irq_data *irq_data)
 {
 }
 
+static int __maybe_unused xgpio_resume(struct device *dev)
+{
+	struct xgpio_instance *gpio = dev_get_drvdata(dev);
+	struct irq_data *data = irq_get_irq_data(gpio->irq);
+
+	if (!data) {
+		dev_err(dev, "irq_get_irq_data() failed\n");
+		return -EINVAL;
+	}
+
+	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_irq_mask - Write the specified signal of the GPIO device.
  * @irq_data: per IRQ and chip data passed down to chip functions
@@ -544,6 +618,8 @@ static int xgpio_probe(struct platform_device *pdev)
 	chip->gc.of_gpio_n_cells = cells;
 	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);
@@ -566,6 +642,10 @@ static int xgpio_probe(struct platform_device *pdev)
 		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);
 
@@ -590,7 +670,7 @@ static int xgpio_probe(struct platform_device *pdev)
 				     GFP_KERNEL);
 	if (!girq->parents) {
 		status = -ENOMEM;
-		goto err_unprepare_clk;
+		goto err_pm_put;
 	}
 	girq->parents[0] = chip->irq;
 	girq->default_type = IRQ_TYPE_NONE;
@@ -600,12 +680,16 @@ static int xgpio_probe(struct platform_device *pdev)
 	status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
 	if (status) {
 		dev_err(&pdev->dev, "failed to add GPIO chip\n");
-		goto err_unprepare_clk;
+		goto err_pm_put;
 	}
 
+	pm_runtime_put(&pdev->dev);
 	return 0;
 
+err_pm_put:
+	pm_runtime_put_sync(&pdev->dev);
 err_unprepare_clk:
+	pm_runtime_disable(&pdev->dev);
 	clk_disable_unprepare(chip->clk);
 	return status;
 }
@@ -623,6 +707,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: [LINUX PATCH V3 7/9] gpio: gpio-xilinx: Add support for suspend and resume

[Linus Walleij]

Hi Srinivas,

On Thu, Nov 12, 2020 at 6:13 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:

> Add support for suspend and resume, pm runtime suspend and resume.
> Added free and request calls.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
> Changes in V3:
> -Created new patch for suspend and resume.

(...)

I'm following the idea here I think.

> @@ -544,6 +618,8 @@ static int xgpio_probe(struct platform_device *pdev)

> +       pm_runtime_enable(&pdev->dev);
> +       status = pm_runtime_get_sync(&pdev->dev);
> +       if (status < 0)
> +               goto err_unprepare_clk;

Now the clock is enabled a second time. Because
runtime PM kicks in.

Do this instead:
pm_runtime_get_noresume()
pm_runtime_set_active()
pm_runtime_enable()

Now runtime PM knows it is active and will not call
runtime resume and enable the clock a second time.

> +       pm_runtime_put(&pdev->dev);
>         return 0;

This is right, now pm runtime will gate the clock
until the first GPIO is requested.

> +err_pm_put:
> +       pm_runtime_put_sync(&pdev->dev);
>  err_unprepare_clk:
> +       pm_runtime_disable(&pdev->dev);
>         clk_disable_unprepare(chip->clk);
>         return status;

Use this on the errorpath instead:
pm_runtime_put_noidle()
pm_runtime_disable()
clk_disable_unprepare();

Now the code will not call runtime suspend to
gate the clock a second time.

Double-check the references to the clock and check
in debugfs that the clock really gets disabled if you're
not using any GPIOs.

Yours,
Linus Walleij

[LINUX PATCH V3 8/9] gpio: gpio-xilinx: Check return value of of_property_read_u32

[Srinivas Neeli]

In two different instances the return value of "of_property_read_u32"
API was neither captured nor checked.
Fixed it by capturing the return value and then checking for any error.

Addresses-Coverity: "check_return"
Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Changes in V3:
-Created new patch to fix coverity warnings.
---
 drivers/gpio/gpio-xilinx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 48f2a6c894f5..8a5f0a0265e6 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -569,7 +569,8 @@ 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]))
+		chip->gpio_state[0] = 0x0;
 
 	/* Update GPIO direction shadow register with default value */
 	if (of_property_read_u32(np, "xlnx,tri-default", &chip->gpio_dir[0]))
@@ -593,8 +594,9 @@ static int xgpio_probe(struct platform_device *pdev)
 
 	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]))
+			chip->gpio_state[1] = 0x0;
 
 		/* Update GPIO direction shadow register with default value */
 		if (of_property_read_u32(np, "xlnx,tri-default-2",
-- 
2.7.4

Re: [LINUX PATCH V3 8/9] gpio: gpio-xilinx: Check return value of of_property_read_u32

[Linus Walleij]

On Thu, Nov 12, 2020 at 6:13 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:

> In two different instances the return value of "of_property_read_u32"
> API was neither captured nor checked.
> Fixed it by capturing the return value and then checking for any error.
>
> Addresses-Coverity: "check_return"
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>

Patch applied.

Yours,
Linus Walleij

[LINUX PATCH V3 9/9] MAINTAINERS: add fragment for xilinx GPIO drivers

[Srinivas Neeli]

Added entry for xilinx GPIO drivers.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
Acked-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
Changes in V3:
-None
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 87452fca5235..89a7c045a213 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19283,6 +19283,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: [LINUX PATCH V3 9/9] MAINTAINERS: add fragment for xilinx GPIO drivers

[Linus Walleij]

On Thu, Nov 12, 2020 at 6:13 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>

Patch applied.

Yours,
Linus Walleij

Re: [LINUX PATCH V3 0/9] gpio-xilinx: Update on xilinx gpio driver

[Michal Simek]

On 12. 11. 20 18:12, Srinivas Neeli wrote:
> This patch series does the following:
> -Add clock support
> -Add interrupt support
> -Add support for suspend and resume
> -Add remove support
> -Add MAINTAINERS fragment
> ---
> Changes in V3:
> -Created separate patch to arrange headers in sorting order.
> -Updated dt-bindings.
> -Created separate patch for Clock changes and runtime resume.
>  and suspend.
> -Created separate patch for spinlock changes.
> -Created separate patch for remove support.
> -Fixed coverity errors.
> -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.
> 
> 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 (9):
>   gpio: gpio-xilinx: Arrange headers in sorting order
>   dt-bindings: gpio: gpio-xilinx: Add clk support to xilinx soft gpio IP
>   gpio: gpio-xilinx: Add clock support
>   gpio: gpio-xilinx: Reduce spinlock array to single
>   gpio: gpio-xilinx: Add interrupt support
>   gpio: gpio-xilinx: Add remove function
>   gpio: gpio-xilinx: Add support for suspend and resume
>   gpio: gpio-xilinx: Check return value of of_property_read_u32
>   MAINTAINERS: add fragment for xilinx GPIO drivers
> 
>  .../devicetree/bindings/gpio/gpio-xilinx.txt       |   2 +
>  MAINTAINERS                                        |  10 +
>  drivers/gpio/Kconfig                               |   2 +
>  drivers/gpio/gpio-xilinx.c                         | 398 +++++++++++++++++++--
>  4 files changed, 390 insertions(+), 22 deletions(-)
> 

For the whole series.
Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

Re: [LINUX PATCH V3 0/9] gpio-xilinx: Update on xilinx gpio driver

[Linus Walleij]

On Thu, Nov 12, 2020 at 6:13 PM Srinivas Neeli
<srinivas.neeli@xilinx.com> wrote:

> Srinivas Neeli (9):

>   gpio: gpio-xilinx: Arrange headers in sorting order
>   dt-bindings: gpio: gpio-xilinx: Add clk support to xilinx soft gpio IP
>   gpio: gpio-xilinx: Add clock support
>   gpio: gpio-xilinx: Reduce spinlock array to single
>   gpio: gpio-xilinx: Add interrupt support
>   gpio: gpio-xilinx: Add remove function
>   gpio: gpio-xilinx: Add support for suspend and resume
>   gpio: gpio-xilinx: Check return value of of_property_read_u32
>   MAINTAINERS: add fragment for xilinx GPIO drivers

So I applied patches 1, 2, 3, 6, 8, 9 now so you can
focus on developing and resending the remaining patches.

Yours,
Linus Walleij

RE: [LINUX PATCH V3 0/9] gpio-xilinx: Update on xilinx gpio driver

[Srinivas Neeli]

Hi Linus,

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Wednesday, November 18, 2020 6:12 AM
> To: Srinivas Neeli <sneeli@xilinx.com>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>; Michal Simek
> <michals@xilinx.com>; Shubhrajyoti Datta <shubhraj@xilinx.com>; Srinivas
> Goud <sgoud@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>;
> 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: [LINUX PATCH V3 0/9] gpio-xilinx: Update on xilinx gpio driver
> 
> On Thu, Nov 12, 2020 at 6:13 PM Srinivas Neeli <srinivas.neeli@xilinx.com>
> wrote:
> 
> > Srinivas Neeli (9):
> 
> >   gpio: gpio-xilinx: Arrange headers in sorting order
> >   dt-bindings: gpio: gpio-xilinx: Add clk support to xilinx soft gpio IP
> >   gpio: gpio-xilinx: Add clock support
> >   gpio: gpio-xilinx: Reduce spinlock array to single
> >   gpio: gpio-xilinx: Add interrupt support
> >   gpio: gpio-xilinx: Add remove function
> >   gpio: gpio-xilinx: Add support for suspend and resume
> >   gpio: gpio-xilinx: Check return value of of_property_read_u32
> >   MAINTAINERS: add fragment for xilinx GPIO drivers
> 
> So I applied patches 1, 2, 3, 6, 8, 9 now so you can focus on developing and
> resending the remaining patches.

Will address remaining patches in V4 series.
> 
> Yours,
> Linus Walleij