[PATCH V4 0/5] gpio-xilinx: Update on xilinx gpio driver

[PATCH V4 0/5] gpio-xilinx: Update on xilinx gpio driver

[Srinivas Neeli]

This patch series does the following:
-Simplify with dev_err_probe().
-Reduce spinlock array to array.
-Add interrupt support
-Add support for suspend and resume
-Add check for gpio-width
---
Changes in V4:
-Created new patch to simplify code with dev_err_probe().
-Updated minor review comments.
-Created new patch to check gpio-width.
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 (5):
  gpio: gpio-xilinx: Simplify with dev_err_probe()
  gpio: gpio-xilinx: Reduce spinlock array to array
  gpio: gpio-xilinx: Add interrupt support
  gpio: gpio-xilinx: Add support for suspend and resume
  gpio: gpio-xilinx: Add check if width exceeds 32

 drivers/gpio/Kconfig       |   3 +
 drivers/gpio/gpio-xilinx.c | 367 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 347 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH V4 1/5] gpio: gpio-xilinx: Simplify with dev_err_probe()

[Srinivas Neeli]

Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Changes in V4:
-New patch
---
 drivers/gpio/gpio-xilinx.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index be539381fd82..d010a63d5d15 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -357,11 +357,8 @@ static int xgpio_probe(struct platform_device *pdev)
 	}
 
 	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);
-	}
+	if (IS_ERR(chip->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(chip->clk), "input clock not found.\n");
 
 	status = clk_prepare_enable(chip->clk);
 	if (status < 0) {
-- 
2.7.4

[PATCH V4 2/5] gpio: gpio-xilinx: Reduce spinlock array to array

[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 V4:
-None.
Changes in V3:
-Created new patch for spinlock changes.
---
 drivers/gpio/gpio-xilinx.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index d010a63d5d15..f88db56543c2 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);
 }
 
 /**
@@ -144,7 +144,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++) {
@@ -155,9 +155,9 @@ 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);
 			index =  xgpio_index(chip, i);
-			spin_lock_irqsave(&chip->gpio_lock[index], flags);
+			spin_lock_irqsave(&chip->gpio_lock, flags);
 		}
 		if (__test_and_clear_bit(i, mask)) {
 			offset =  xgpio_offset(chip, i);
@@ -171,7 +171,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);
 }
 
 /**
@@ -190,14 +190,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 +221,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 +236,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;
 }
@@ -312,7 +312,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);
 
 	if (of_property_read_u32(np, "xlnx,is-dual", &is_dual))
 		is_dual = 0;
@@ -336,7 +336,6 @@ static int xgpio_probe(struct platform_device *pdev)
 					 &chip->gpio_width[1]))
 			chip->gpio_width[1] = 32;
 
-		spin_lock_init(&chip->gpio_lock[1]);
 	}
 
 	chip->gc.base = -1;
-- 
2.7.4

[PATCH V4 3/5] 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.
Depends on OF_GPIO framework for of_xlate function to translate
gpiospec to the GPIO number and flags.

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 V4:
-Added more commit description.
Changes 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       |   3 +
 drivers/gpio/gpio-xilinx.c | 242 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 241 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index c70f46e80a3b..da2cca80d6cd 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -690,6 +690,9 @@ config GPIO_XGENE_SB
 
 config GPIO_XILINX
 	tristate "Xilinx GPIO support"
+	select GPIOLIB_IRQCHIP
+	select IRQ_DOMAIN_HIERARCHY
+	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 f88db56543c2..437c50e72aa1 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;
 };
 
@@ -277,6 +295,183 @@ static int xgpio_remove(struct platform_device *pdev)
 }
 
 /**
+ * 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
  *
@@ -289,7 +484,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)
@@ -305,6 +503,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
@@ -343,6 +545,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;
@@ -367,14 +570,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

[PATCH V4 4/5] 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 V4:
-Adjust code to remove conflicts.
Changes in V3:
-Created new patch for suspend and resume.
---
 drivers/gpio/gpio-xilinx.c | 94 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 437c50e72aa1..e47ae08167f8 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 */
@@ -277,6 +278,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_remove - Remove method for the GPIO device.
  * @pdev: pointer to the platform device
@@ -289,7 +323,10 @@ static int xgpio_remove(struct platform_device *pdev)
 {
 	struct xgpio_instance *gpio = platform_get_drvdata(pdev);
 
-	clk_disable_unprepare(gpio->clk);
+	if (!pm_runtime_suspended(&pdev->dev))
+		clk_disable_unprepare(gpio->clk);
+
+	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
@@ -304,6 +341,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
@@ -548,6 +625,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);
@@ -567,6 +646,9 @@ static int xgpio_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to prepare clk\n");
 		return status;
 	}
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
 
 	xgpio_save_regs(chip);
 
@@ -591,7 +673,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;
@@ -601,12 +683,15 @@ 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_unprepare_clk:
+err_pm_put:
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
 	clk_disable_unprepare(chip->clk);
 	return status;
 }
@@ -624,6 +709,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

[PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32

[Srinivas Neeli]

Add check to see if gpio-width property does not exceed 32.
If it exceeds then return -EINVAL.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
Changes in V4:
-New patch.
---
 drivers/gpio/gpio-xilinx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index e47ae08167f8..ddec718e114c 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -591,6 +591,9 @@ 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;
 
+	if (chip->gpio_width[0] > 32)
+		return -EINVAL;
+
 	spin_lock_init(&chip->gpio_lock);
 
 	if (of_property_read_u32(np, "xlnx,is-dual", &is_dual))
@@ -615,6 +618,8 @@ static int xgpio_probe(struct platform_device *pdev)
 					 &chip->gpio_width[1]))
 			chip->gpio_width[1] = 32;
 
+		if (chip->gpio_width[1] > 32)
+			return -EINVAL;
 	}
 
 	chip->gc.base = -1;
-- 
2.7.4

Re: [PATCH V4 1/5] gpio: gpio-xilinx: Simplify with dev_err_probe()

[Linus Walleij]

On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote:

> Common pattern of handling deferred probe can be simplified with
> dev_err_probe(). Less code and also it prints the error value.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH V4 2/5] gpio: gpio-xilinx: Reduce spinlock array to array

[Linus Walleij]

On Wed, Jan 6, 2021 at 1:27 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>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH V4 3/5] gpio: gpio-xilinx: Add interrupt support

[Linus Walleij]

On Wed, Jan 6, 2021 at 1:27 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.
> Depends on OF_GPIO framework for of_xlate function to translate
> gpiospec to the GPIO number and flags.
>
> 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
> +       select IRQ_DOMAIN_HIERARCHY

Why do you select IRQ_DOMAIN_HIERARCHY?

You don't seem to use it.

> +#include <linux/of_gpio.h>

No modern driver should include this header. I suspect you
can just drop it.

See drivers/gpio/TODO

> +       /* 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
> @@ -343,6 +545,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;

This looks a bit dangerous. What actually happens if this is something
like 3. The translate function is not going to work. I would just dev_err()
and bail out if #gpio-cells != 2.

Other than that this looks good!

Yours,
Linus Walleij

Re: [PATCH V4 4/5] gpio: gpio-xilinx: Add support for suspend and resume

[Linus Walleij]

On Wed, Jan 6, 2021 at 1:27 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>
(...)

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

That's clever. I think more GPIO drivers should be doing it like this,
today I think most just ignore the return code.

> +static int __maybe_unused xgpio_suspend(struct device *dev)
> +static int __maybe_unused xgpio_resume(struct device *dev)

Those look good.


>  /**
>   * xgpio_remove - Remove method for the GPIO device.
>   * @pdev: pointer to the platform device
> @@ -289,7 +323,10 @@ static int xgpio_remove(struct platform_device *pdev)
>  {
>         struct xgpio_instance *gpio = platform_get_drvdata(pdev);
>
> -       clk_disable_unprepare(gpio->clk);
> +       if (!pm_runtime_suspended(&pdev->dev))
> +               clk_disable_unprepare(gpio->clk);
> +
> +       pm_runtime_disable(&pdev->dev);

This looks complex and racy. What if the device is resumed after you
executed the
first part of the statement.

The normal sequence is:

pm_runtime_get_sync(dev);
pm_runtime_put_noidle(dev);
pm_runtime_disable(dev);

This will make sure the clock is enabled and pm runtime is disabled.
After this you can unconditionally call clk_disable_unprepare(gpio->clk);

It is what you are doing on the errorpath of probe().

Yours,
Linus Walleij

Re: [PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32

[Linus Walleij]

On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote:

> Add check to see if gpio-width property does not exceed 32.
> If it exceeds then return -EINVAL.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>

Aha

> @@ -591,6 +591,9 @@ 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;

This xlnx,gpio-width seems very much like the standard ngpios property
from Documentation/devicetree/bindings/gpio/gpio.txt
but I guess not much to do about that now. :/

Do you think you can add support for both?

> +       if (chip->gpio_width[0] > 32)
> +               return -EINVAL;

This looks OK.

Yours,
Linus Walleij

Re: [PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32

[Michal Simek]

On 07. 01. 21 11:17, Linus Walleij wrote:
> On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote:
> 
>> Add check to see if gpio-width property does not exceed 32.
>> If it exceeds then return -EINVAL.
>>
>> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> 
> Aha
> 
>> @@ -591,6 +591,9 @@ 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;
> 
> This xlnx,gpio-width seems very much like the standard ngpios property
> from Documentation/devicetree/bindings/gpio/gpio.txt
> but I guess not much to do about that now. :/
> 
> Do you think you can add support for both?

support for both is definitely possible but we need to handle also gpio
width for second channel referenced by xlnx,gpio2-widht now.

It means we could end up in situation which can be misleading for users
where ngpios will be 10 and xlnx,gpio2-width another 10 and in total we
have 20 gpios.

I think that it is better not to start to mess with ngpios property not
to confuse people which are coming from other SOCs because ngpios can
suggest all gpios assigned to this controller.

And in second case where ngpios is total number of gpios and if
xlnx,gpio2-width is defined you can find width for first bank.
But it is questionable if this improve situation here.

Please correct me if my logic is not correct.
Definitely this should be done separately out of this patch.

> 
>> +       if (chip->gpio_width[0] > 32)
>> +               return -EINVAL;
> 
> This looks OK.

Does it mean ack for this patch?

Thanks,
Michal

Re: [PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32

[Linus Walleij]

On Thu, Jan 7, 2021 at 11:29 AM Michal Simek <michal.simek@xilinx.com> wrote:
> On 07. 01. 21 11:17, Linus Walleij wrote:
> > On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote:

> >> @@ -591,6 +591,9 @@ 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;
> >
> > This xlnx,gpio-width seems very much like the standard ngpios property
> > from Documentation/devicetree/bindings/gpio/gpio.txt
> > but I guess not much to do about that now. :/
> >
> > Do you think you can add support for both?
>
> support for both is definitely possible but we need to handle also gpio
> width for second channel referenced by xlnx,gpio2-widht now.
>
> It means we could end up in situation which can be misleading for users
> where ngpios will be 10 and xlnx,gpio2-width another 10 and in total we
> have 20 gpios.

OK that is confusing. Let's not do that then.

> I think that it is better not to start to mess with ngpios property not
> to confuse people which are coming from other SOCs because ngpios can
> suggest all gpios assigned to this controller.

OK I agree.

> >> +       if (chip->gpio_width[0] > 32)
> >> +               return -EINVAL;
> >
> > This looks OK.
>
> Does it mean ack for this patch?

Yeah after explanations this patch is fine:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

It's just that this hardware with paired controllers is a bit weird so it will
lead to discussions all the time because it's hard to understand.

Yours,
Linus Walleij

Re: [PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32

[Michal Simek]

On 07. 01. 21 11:47, Linus Walleij wrote:
> On Thu, Jan 7, 2021 at 11:29 AM Michal Simek <michal.simek@xilinx.com> wrote:
>> On 07. 01. 21 11:17, Linus Walleij wrote:
>>> On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote:
> 
>>>> @@ -591,6 +591,9 @@ 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;
>>>
>>> This xlnx,gpio-width seems very much like the standard ngpios property
>>> from Documentation/devicetree/bindings/gpio/gpio.txt
>>> but I guess not much to do about that now. :/
>>>
>>> Do you think you can add support for both?
>>
>> support for both is definitely possible but we need to handle also gpio
>> width for second channel referenced by xlnx,gpio2-widht now.
>>
>> It means we could end up in situation which can be misleading for users
>> where ngpios will be 10 and xlnx,gpio2-width another 10 and in total we
>> have 20 gpios.
> 
> OK that is confusing. Let's not do that then.
> 
>> I think that it is better not to start to mess with ngpios property not
>> to confuse people which are coming from other SOCs because ngpios can
>> suggest all gpios assigned to this controller.
> 
> OK I agree.
> 
>>>> +       if (chip->gpio_width[0] > 32)
>>>> +               return -EINVAL;
>>>
>>> This looks OK.
>>
>> Does it mean ack for this patch?
> 
> Yeah after explanations this patch is fine:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> It's just that this hardware with paired controllers is a bit weird so it will
> lead to discussions all the time because it's hard to understand.

Maybe it should be described a little bit differently in DT.
Just to have gpio node and every bank could be described as a child node
where standard properties could be used and irq will be shared.

Thanks,
Michal

RE: [PATCH V4 4/5] gpio: gpio-xilinx: Add support for suspend and resume

[Srinivas Neeli]

Hi Linus,

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Thursday, January 7, 2021 3:17 PM
> 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>;
> William Breathitt Gray <vilhelm.gray@gmail.com>; Syed Nayyar Waris
> <syednwaris@gmail.com>; 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 V4 4/5] gpio: gpio-xilinx: Add support for suspend and
> resume
> 
> On Wed, Jan 6, 2021 at 1:27 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>
> (...)
> 
> > +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;
> > +}
> 
> That's clever. I think more GPIO drivers should be doing it like this, today I
> think most just ignore the return code.
> 
> > +static int __maybe_unused xgpio_suspend(struct device *dev) static
> > +int __maybe_unused xgpio_resume(struct device *dev)
> 
> Those look good.
> 
> 
> >  /**
> >   * xgpio_remove - Remove method for the GPIO device.
> >   * @pdev: pointer to the platform device @@ -289,7 +323,10 @@ static
> > int xgpio_remove(struct platform_device *pdev)  {
> >         struct xgpio_instance *gpio = platform_get_drvdata(pdev);
> >
> > -       clk_disable_unprepare(gpio->clk);
> > +       if (!pm_runtime_suspended(&pdev->dev))
> > +               clk_disable_unprepare(gpio->clk);
> > +
> > +       pm_runtime_disable(&pdev->dev);
> 
> This looks complex and racy. What if the device is resumed after you
> executed the first part of the statement.

Could you please explain more on this.
What is the need to call pm_runtime_get_sync(); in remove API ?

> 
> The normal sequence is:
> 
> pm_runtime_get_sync(dev);
> pm_runtime_put_noidle(dev);
> pm_runtime_disable(dev);
> 
> This will make sure the clock is enabled and pm runtime is disabled.
> After this you can unconditionally call clk_disable_unprepare(gpio->clk);
> 
> It is what you are doing on the errorpath of probe().
> 
> Yours,
> Linus Walleij

Re: [PATCH V4 4/5] gpio: gpio-xilinx: Add support for suspend and resume

[Linus Walleij]

On Fri, Jan 8, 2021 at 12:41 PM Srinivas Neeli <sneeli@xilinx.com> wrote:
> > On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com>
> > wrote:

> > >  /**
> > >   * xgpio_remove - Remove method for the GPIO device.
> > >   * @pdev: pointer to the platform device @@ -289,7 +323,10 @@ static
> > > int xgpio_remove(struct platform_device *pdev)  {
> > >         struct xgpio_instance *gpio = platform_get_drvdata(pdev);
> > >
> > > -       clk_disable_unprepare(gpio->clk);
> > > +       if (!pm_runtime_suspended(&pdev->dev))
> > > +               clk_disable_unprepare(gpio->clk);
> > > +
> > > +       pm_runtime_disable(&pdev->dev);
> >
> > This looks complex and racy. What if the device is resumed after you
> > executed the first part of the statement.
>
> Could you please explain more on this.
> What is the need to call pm_runtime_get_sync(); in remove API ?

I explain that on the lines right below your comment ;D

> > The normal sequence is:
> >
> > pm_runtime_get_sync(dev);
> > pm_runtime_put_noidle(dev);
> > pm_runtime_disable(dev);
> >
> > This will make sure the clock is enabled and pm runtime is disabled.
> > After this you can unconditionally call clk_disable_unprepare(gpio->clk);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Yours,
Linus Walleij