[PATCHv3 0/3] Enable Quark X1000 support in gpio-sch

[PATCHv3 0/3] Enable Quark X1000 support in gpio-sch

[Chang Rebecca Swee Fun]

Hi,

This is a revised version for enabling Quark X1000 in gpio-sch.

Change log for V3:
The patches had been rebased on "devel" branch and it has dependency on
Mika Westerberg's commit at: https://lkml.org/lkml/2014/9/16/213

Patch 3:
- Change variable type of irq_support to bool.
- Update error message and remove redundant code.
- Revert gpiochip_remove() to avoid it to return a value.

The changes had been verified by passing build test and functional test
on Intel Galileo board.

Thank you.
Regards,
Rebecca

Change log for V2:
Patch 1:
- Move sch_gpio_get() and sch_gpio_set() to avoid forward declaration.
- Changed sch_gpio_enable()/sch_gpio_disable() to sch_gpio_register_set()/
  sch_gpio_register_clear().

Patch 3:
- Changed all sch_gpio_enable()/sch_gpio_disable() to sch_gpio_register_set()/
  sch_gpio_register_clear().

Version 1:
Initial version.

Chang Rebecca Swee Fun (3):
  gpio: sch: Consolidate similar algorithms
  gpio: sch: Add support for Intel Quark X1000 SoC
  gpio: sch: Enable IRQ support for Quark X1000

 drivers/gpio/Kconfig    |   11 +-
 drivers/gpio/gpio-sch.c |  353 ++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 311 insertions(+), 53 deletions(-)

-- 
1.7.9.5

[PATCHv3 1/3] gpio: sch: Consolidate similar algorithms

[Chang Rebecca Swee Fun]

Consolidating similar algorithms into common functions to make
GPIO SCH simpler and manageable.

Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
---
 drivers/gpio/gpio-sch.c |   95 ++++++++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 42 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 99720c8..6e89be9 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -63,94 +63,105 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio)
 	return gpio % 8;
 }
 
-static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio)
+static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
+				  unsigned reg)
 {
 	unsigned short offset, bit;
 	u8 enable;
 
 	spin_lock(&sch->lock);
 
-	offset = sch_gpio_offset(sch, gpio, GEN);
+	offset = sch_gpio_offset(sch, gpio, reg);
 	bit = sch_gpio_bit(sch, gpio);
 
 	enable = inb(sch->iobase + offset);
-	if (!(enable & (1 << bit)))
-		outb(enable | (1 << bit), sch->iobase + offset);
+	if (!(enable & BIT(bit)))
+		outb(enable | BIT(bit), sch->iobase + offset);
 
 	spin_unlock(&sch->lock);
 }
 
-static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned  gpio_num)
+static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
+				    unsigned reg)
 {
-	struct sch_gpio *sch = to_sch_gpio(gc);
-	u8 curr_dirs;
 	unsigned short offset, bit;
+	u8 disable;
 
 	spin_lock(&sch->lock);
 
-	offset = sch_gpio_offset(sch, gpio_num, GIO);
-	bit = sch_gpio_bit(sch, gpio_num);
-
-	curr_dirs = inb(sch->iobase + offset);
+	offset = sch_gpio_offset(sch, gpio, reg);
+	bit = sch_gpio_bit(sch, gpio);
 
-	if (!(curr_dirs & (1 << bit)))
-		outb(curr_dirs | (1 << bit), sch->iobase + offset);
+	disable = inb(sch->iobase + offset);
+	if (disable & BIT(bit))
+		outb(disable & ~BIT(bit), sch->iobase + offset);
 
 	spin_unlock(&sch->lock);
-	return 0;
 }
 
-static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
+static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
 {
 	struct sch_gpio *sch = to_sch_gpio(gc);
-	int res;
 	unsigned short offset, bit;
+	u8 curr_dirs;
 
-	offset = sch_gpio_offset(sch, gpio_num, GLV);
-	bit = sch_gpio_bit(sch, gpio_num);
+	offset = sch_gpio_offset(sch, gpio, reg);
+	bit = sch_gpio_bit(sch, gpio);
 
-	res = !!(inb(sch->iobase + offset) & (1 << bit));
+	curr_dirs = !!(inb(sch->iobase + offset) & BIT(bit));
 
-	return res;
+	return curr_dirs;
 }
 
-static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
+static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg,
+			     int val)
 {
 	struct sch_gpio *sch = to_sch_gpio(gc);
-	u8 curr_vals;
 	unsigned short offset, bit;
+	u8 curr_dirs;
 
-	spin_lock(&sch->lock);
-
-	offset = sch_gpio_offset(sch, gpio_num, GLV);
-	bit = sch_gpio_bit(sch, gpio_num);
+	offset = sch_gpio_offset(sch, gpio, reg);
+	bit = sch_gpio_bit(sch, gpio);
 
-	curr_vals = inb(sch->iobase + offset);
+	curr_dirs = inb(sch->iobase + offset);
 
 	if (val)
-		outb(curr_vals | (1 << bit), sch->iobase + offset);
+		outb(curr_dirs | BIT(bit), sch->iobase + offset);
 	else
-		outb((curr_vals & ~(1 << bit)), sch->iobase + offset);
+		outb((curr_dirs & ~BIT(bit)), sch->iobase + offset);
+}
 
+static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
+{
+	struct sch_gpio *sch = to_sch_gpio(gc);
+
+	spin_lock(&sch->lock);
+	sch_gpio_register_set(sch, gpio_num, GIO);
 	spin_unlock(&sch->lock);
+	return 0;
 }
 
-static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
-				  int val)
+static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
+{
+	return sch_gpio_reg_get(gc, gpio_num, GLV);
+}
+
+static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
 {
 	struct sch_gpio *sch = to_sch_gpio(gc);
-	u8 curr_dirs;
-	unsigned short offset, bit;
 
 	spin_lock(&sch->lock);
+	sch_gpio_reg_set(gc, gpio_num, GLV, val);
+	spin_unlock(&sch->lock);
+}
 
-	offset = sch_gpio_offset(sch, gpio_num, GIO);
-	bit = sch_gpio_bit(sch, gpio_num);
-
-	curr_dirs = inb(sch->iobase + offset);
-	if (curr_dirs & (1 << bit))
-		outb(curr_dirs & ~(1 << bit), sch->iobase + offset);
+static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
+				  int val)
+{
+	struct sch_gpio *sch = to_sch_gpio(gc);
 
+	spin_lock(&sch->lock);
+	sch_gpio_register_clear(sch, gpio_num, GIO);
 	spin_unlock(&sch->lock);
 
 	/*
@@ -209,13 +220,13 @@ static int sch_gpio_probe(struct platform_device *pdev)
 		 * GPIO7 is configured by the CMC as SLPIOVR
 		 * Enable GPIO[9:8] core powered gpios explicitly
 		 */
-		sch_gpio_enable(sch, 8);
-		sch_gpio_enable(sch, 9);
+		sch_gpio_register_set(sch, 8, GEN);
+		sch_gpio_register_set(sch, 9, GEN);
 		/*
 		 * SUS_GPIO[2:0] enabled by default
 		 * Enable SUS_GPIO3 resume powered gpio explicitly
 		 */
-		sch_gpio_enable(sch, 13);
+		sch_gpio_register_set(sch, 13, GEN);
 		break;
 
 	case PCI_DEVICE_ID_INTEL_ITC_LPC:
-- 
1.7.9.5

[PATCHv3 2/3] gpio: sch: Add support for Intel Quark X1000 SoC

[Chang Rebecca Swee Fun]

Intel Quark X1000 provides a total of 16 GPIOs. The GPIOs are split between
the legacy I/O bridge and the GPIO controller.

GPIO-SCH is the GPIO pins on legacy bridge for Intel Quark SoC.
Intel Quark X1000 has 2 GPIOs powered by the core power well and 6 from
the suspend power well.

This piece of work is derived from Dan O'Donovan's initial work for Quark
X1000 enabling.

Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
---
 drivers/gpio/Kconfig    |   11 +++++++++--
 drivers/gpio/gpio-sch.c |    6 ++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0959ca9..0e60f93 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -371,25 +371,32 @@ config GPIO_VR41XX
 	  Say yes here to support the NEC VR4100 series General-purpose I/O Uint
 
 config GPIO_SCH
-	tristate "Intel SCH/TunnelCreek/Centerton GPIO"
+	tristate "Intel SCH/TunnelCreek/Centerton/Quark X1000 GPIO"
 	depends on PCI && X86
 	select MFD_CORE
 	select LPC_SCH
 	help
 	  Say yes here to support GPIO interface on Intel Poulsbo SCH,
-	  Intel Tunnel Creek processor or Intel Centerton processor.
+	  Intel Tunnel Creek processor, Intel Centerton processor or
+	  Intel Quark X1000 SoC.
+
 	  The Intel SCH contains a total of 14 GPIO pins. Ten GPIOs are
 	  powered by the core power rail and are turned off during sleep
 	  modes (S3 and higher). The remaining four GPIOs are powered by
 	  the Intel SCH suspend power supply. These GPIOs remain
 	  active during S3. The suspend powered GPIOs can be used to wake the
 	  system from the Suspend-to-RAM state.
+
 	  The Intel Tunnel Creek processor has 5 GPIOs powered by the
 	  core power rail and 9 from suspend power supply.
+
 	  The Intel Centerton processor has a total of 30 GPIO pins.
 	  Twenty-one are powered by the core power rail and 9 from the
 	  suspend power supply.
 
+	  The Intel Quark X1000 SoC has 2 GPIOs powered by the core
+	  power well and 6 from the suspend power well.
+
 config GPIO_ICH
 	tristate "Intel ICH GPIO"
 	depends on PCI && X86
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 6e89be9..952990f 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -241,6 +241,12 @@ static int sch_gpio_probe(struct platform_device *pdev)
 		sch->chip.ngpio = 30;
 		break;
 
+	case PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB:
+		sch->core_base = 0;
+		sch->resume_base = 2;
+		sch->chip.ngpio = 8;
+		break;
+
 	default:
 		return -ENODEV;
 	}
-- 
1.7.9.5

[PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000

[Chang Rebecca Swee Fun]

Intel Quark X1000 GPIO controller supports interrupt handling for
both core power well and resume power well. This patch is to enable
the IRQ support and provide IRQ handling for Intel Quark X1000
GPIO-SCH device driver.

This piece of work is derived from Dan O'Donovan's initial work for
Quark X1000 enabling.

Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
---
 drivers/gpio/gpio-sch.c |  257 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 245 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 952990f..dd84b1f 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -28,17 +28,30 @@
 #include <linux/pci_ids.h>
 
 #include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 
 #define GEN	0x00
 #define GIO	0x04
 #define GLV	0x08
+#define GTPE	0x0C
+#define GTNE	0x10
+#define GGPE	0x14
+#define GSMI	0x18
+#define GTS	0x1C
+#define CGNMIEN	0x40
+#define RGNMIEN	0x44
 
 struct sch_gpio {
 	struct gpio_chip chip;
+	struct irq_data data;
 	spinlock_t lock;
 	unsigned short iobase;
 	unsigned short core_base;
 	unsigned short resume_base;
+	int irq_base;
+	int irq_num;
+	bool irq_support;
 };
 
 #define to_sch_gpio(c)	container_of(c, struct sch_gpio, chip)
@@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio)
 static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
 				  unsigned reg)
 {
+	unsigned long flags;
 	unsigned short offset, bit;
 	u8 enable;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 
 	offset = sch_gpio_offset(sch, gpio, reg);
 	bit = sch_gpio_bit(sch, gpio);
@@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
 	if (!(enable & BIT(bit)))
 		outb(enable | BIT(bit), sch->iobase + offset);
 
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 }
 
 static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
 				    unsigned reg)
 {
+	unsigned long flags;
 	unsigned short offset, bit;
 	u8 disable;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 
 	offset = sch_gpio_offset(sch, gpio, reg);
 	bit = sch_gpio_bit(sch, gpio);
@@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
 	if (disable & BIT(bit))
 		outb(disable & ~BIT(bit), sch->iobase + offset);
 
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 }
 
 static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
@@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg,
 static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
 {
 	struct sch_gpio *sch = to_sch_gpio(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_register_set(sch, gpio_num, GIO);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 	return 0;
 }
 
@@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
 static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
 {
 	struct sch_gpio *sch = to_sch_gpio(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(gc, gpio_num, GLV, val);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 }
 
 static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
 				  int val)
 {
 	struct sch_gpio *sch = to_sch_gpio(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_register_clear(sch, gpio_num, GIO);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 
 	/*
 	 * according to the datasheet, writing to the level register has no
@@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
 	return 0;
 }
 
+static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+	struct sch_gpio *sch = to_sch_gpio(gc);
+
+	return sch->irq_base + offset;
+}
+
 static struct gpio_chip sch_gpio_chip = {
 	.label			= "sch_gpio",
 	.owner			= THIS_MODULE,
@@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = {
 	.get			= sch_gpio_get,
 	.direction_output	= sch_gpio_direction_out,
 	.set			= sch_gpio_set,
+	.to_irq			= sch_gpio_to_irq,
 };
 
+static void sch_gpio_irq_enable(struct irq_data *d)
+{
+	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+	u32 gpio_num;
+
+	gpio_num = d->irq - sch->irq_base;
+	sch_gpio_register_set(sch, gpio_num, GGPE);
+}
+
+static void sch_gpio_irq_disable(struct irq_data *d)
+{
+	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+	u32 gpio_num;
+
+	gpio_num = d->irq - sch->irq_base;
+	sch_gpio_register_clear(sch, gpio_num, GGPE);
+}
+
+static void sch_gpio_irq_ack(struct irq_data *d)
+{
+	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+	u32 gpio_num;
+
+	gpio_num = d->irq - sch->irq_base;
+	sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1);
+}
+
+static int sch_gpio_irq_type(struct irq_data *d, unsigned type)
+{
+	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
+	unsigned long flags;
+	u32 gpio_num;
+
+	if (d == NULL)
+		return -EINVAL;
+
+	gpio_num = d->irq - sch->irq_base;
+
+	spin_lock_irqsave(&sch->lock, flags);
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		sch_gpio_register_set(sch, gpio_num, GTPE);
+		sch_gpio_register_clear(sch, gpio_num, GTNE);
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		sch_gpio_register_set(sch, gpio_num, GTNE);
+		sch_gpio_register_clear(sch, gpio_num, GTPE);
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		sch_gpio_register_set(sch, gpio_num, GTPE);
+		sch_gpio_register_set(sch, gpio_num, GTNE);
+		break;
+
+	case IRQ_TYPE_NONE:
+		sch_gpio_register_clear(sch, gpio_num, GTPE);
+		sch_gpio_register_clear(sch, gpio_num, GTNE);
+		break;
+
+	default:
+		spin_unlock_irqrestore(&sch->lock, flags);
+		return -EINVAL;
+	}
+
+	spin_unlock_irqrestore(&sch->lock, flags);
+
+	return 0;
+}
+
+static struct irq_chip sch_irq_chip = {
+	.irq_enable	= sch_gpio_irq_enable,
+	.irq_disable	= sch_gpio_irq_disable,
+	.irq_ack	= sch_gpio_irq_ack,
+	.irq_set_type	= sch_gpio_irq_type,
+};
+
+static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num)
+{
+	unsigned int i;
+
+	for (i = 0; i < num; i++) {
+		irq_set_chip_data(i + sch->irq_base, sch);
+		irq_set_chip_and_handler_name(i + sch->irq_base,
+					      &sch_irq_chip,
+					      handle_simple_irq,
+					      "sch_gpio_irq_chip");
+	}
+}
+
+static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num)
+{
+	unsigned int i;
+
+	for (i = 0; i < num; i++) {
+		irq_set_chip_data(i + sch->irq_base, 0);
+		irq_set_chip_and_handler_name(i + sch->irq_base, 0, 0, 0);
+	}
+}
+
+static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int num)
+{
+	unsigned long flags;
+	unsigned int gpio_num;
+
+	spin_lock_irqsave(&sch->lock, flags);
+
+	for (gpio_num = 0; gpio_num < num; gpio_num++) {
+		sch_gpio_register_clear(sch, gpio_num, GTPE);
+		sch_gpio_register_clear(sch, gpio_num, GTNE);
+		sch_gpio_register_clear(sch, gpio_num, GGPE);
+		sch_gpio_register_clear(sch, gpio_num, GSMI);
+
+		if (gpio_num >= 2)
+			sch_gpio_register_clear(sch, gpio_num, RGNMIEN);
+		else
+			sch_gpio_register_clear(sch, gpio_num, CGNMIEN);
+
+		/* clear any pending interrupts */
+		sch_gpio_reg_set(&sch->chip, gpio_num, GTS, 1);
+	}
+
+	spin_unlock_irqrestore(&sch->lock, flags);
+}
+
+static irqreturn_t sch_gpio_irq_handler(int irq, void *dev_id)
+{
+	struct sch_gpio *sch = dev_id;
+	int res;
+	unsigned int i;
+	int ret = IRQ_NONE;
+
+	for (i = 0; i < sch->chip.ngpio; i++) {
+		res = sch_gpio_reg_get(&sch->chip, i, GTS);
+		if (res) {
+			/* clear by setting GTS to 1 */
+			sch_gpio_reg_set(&sch->chip, i, GTS, 1);
+			generic_handle_irq(sch->irq_base + i);
+			ret = IRQ_HANDLED;
+		}
+	}
+
+	return ret;
+}
+
 static int sch_gpio_probe(struct platform_device *pdev)
 {
 	struct sch_gpio *sch;
-	struct resource *res;
+	struct resource *res, *irq;
+	int err;
 
 	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
 	if (!sch)
@@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev)
 				 pdev->name))
 		return -EBUSY;
 
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	sch->irq_support = !!irq;
+	if (sch->irq_support) {
+		sch->irq_num = irq->start;
+		if (sch->irq_num < 0) {
+			dev_warn(&pdev->dev,
+				 "failed to obtain irq number for device\n");
+			sch->irq_support = false;
+		}
+	}
+
 	spin_lock_init(&sch->lock);
 	sch->iobase = res->start;
 	sch->chip = sch_gpio_chip;
@@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	err = gpiochip_add(&sch->chip);
+	if (err < 0)
+		goto err_sch_gpio;
+
+	if (sch->irq_support) {
+		sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
+						NUMA_NO_NODE);
+		if (sch->irq_base < 0) {
+			dev_err(&pdev->dev,
+				"failed to allocate GPIO IRQ descs\n");
+			goto err_sch_intr_chip;
+		}
+
+		/* disable interrupts */
+		sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
+
+		err = request_irq(sch->irq_num, sch_gpio_irq_handler,
+				  IRQF_SHARED, KBUILD_MODNAME, sch);
+		if (err) {
+			dev_err(&pdev->dev,
+				"%s failed to request IRQ\n", __func__);
+			goto err_sch_request_irq;
+		}
+
+		sch_gpio_irqs_init(sch, sch->chip.ngpio);
+	}
+
 	platform_set_drvdata(pdev, sch);
 
-	return gpiochip_add(&sch->chip);
+	return 0;
+
+err_sch_request_irq:
+	irq_free_descs(sch->irq_base, sch->chip.ngpio);
+
+err_sch_intr_chip:
+	if (gpiochip_remove(&sch->chip))
+		dev_err(&pdev->dev,
+			"%s gpiochip_remove() failed\n", __func__);
+
+err_sch_gpio:
+	release_region(res->start, resource_size(res));
+
+	return err;
 }
 
 static int sch_gpio_remove(struct platform_device *pdev)
 {
 	struct sch_gpio *sch = platform_get_drvdata(pdev);
 
+	if (sch->irq_support) {
+		sch_gpio_irqs_deinit(sch, sch->chip.ngpio);
+
+		if (sch->irq_num >= 0)
+			free_irq(sch->irq_num, sch);
+
+		irq_free_descs(sch->irq_base, sch->chip.ngpio);
+	}
+
 	gpiochip_remove(&sch->chip);
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000

[Varka Bhadram]

On 10/16/2014 09:21 AM, Chang Rebecca Swee Fun wrote:
> Intel Quark X1000 GPIO controller supports interrupt handling for
> both core power well and resume power well. This patch is to enable
> the IRQ support and provide IRQ handling for Intel Quark X1000
> GPIO-SCH device driver.

(...)

>   static int sch_gpio_probe(struct platform_device *pdev)
>   {
>   	struct sch_gpio *sch;
> -	struct resource *res;
> +	struct resource *res, *irq;
> +	int err;
>   
>   	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
>   	if (!sch)
> @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev)
>   				 pdev->name))
>   		return -EBUSY;
>   
> +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	sch->irq_support = !!irq;
> +	if (sch->irq_support) {
> +		sch->irq_num = irq->start;
> +		if (sch->irq_num < 0) {
> +			dev_warn(&pdev->dev,
> +				 "failed to obtain irq number for device\n");
> +			sch->irq_support = false;
> +		}
> +	}
> +
>   	spin_lock_init(&sch->lock);
>   	sch->iobase = res->start;
>   	sch->chip = sch_gpio_chip;
> @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev)
>   		return -ENODEV;
>   	}
>   
> +	err = gpiochip_add(&sch->chip);
> +	if (err < 0)
> +		goto err_sch_gpio;
> +
> +	if (sch->irq_support) {
> +		sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
> +						NUMA_NO_NODE);
> +		if (sch->irq_base < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to allocate GPIO IRQ descs\n");
> +			goto err_sch_intr_chip;
> +		}
> +
> +		/* disable interrupts */
> +		sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
> +
> +		err = request_irq(sch->irq_num, sch_gpio_irq_handler,
> +				  IRQF_SHARED, KBUILD_MODNAME, sch);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"%s failed to request IRQ\n", __func__);
> +			goto err_sch_request_irq;
> +		}
> +
> +		sch_gpio_irqs_init(sch, sch->chip.ngpio);
> +	}
> +
>   	platform_set_drvdata(pdev, sch);
>   
> -	return gpiochip_add(&sch->chip);
> +	return 0;
> +
> +err_sch_request_irq:
> +	irq_free_descs(sch->irq_base, sch->chip.ngpio);
> +
> +err_sch_intr_chip:
> +	if (gpiochip_remove(&sch->chip))

gpiochip_remove() return 'void' [0].

> +		dev_err(&pdev->dev,
> +			"%s gpiochip_remove() failed\n", __func__);
> +
> +err_sch_gpio:
> +	release_region(res->start, resource_size(res));
> +
> +	return err;
>   }

[0]:https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/tree/drivers/gpio/gpiolib.c#n311


-- 
Regards,
Varka Bhadram.

Re: [PATCHv3 1/3] gpio: sch: Consolidate similar algorithms

[Mika Westerberg]

On Thu, Oct 16, 2014 at 11:51:13AM +0800, Chang Rebecca Swee Fun wrote:
> Consolidating similar algorithms into common functions to make
> GPIO SCH simpler and manageable.
> 
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCHv3 2/3] gpio: sch: Add support for Intel Quark X1000 SoC

[Mika Westerberg]

On Thu, Oct 16, 2014 at 11:51:14AM +0800, Chang Rebecca Swee Fun wrote:
> Intel Quark X1000 provides a total of 16 GPIOs. The GPIOs are split between
> the legacy I/O bridge and the GPIO controller.
> 
> GPIO-SCH is the GPIO pins on legacy bridge for Intel Quark SoC.
> Intel Quark X1000 has 2 GPIOs powered by the core power well and 6 from
> the suspend power well.
> 
> This piece of work is derived from Dan O'Donovan's initial work for Quark
> X1000 enabling.
> 
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000

[Mika Westerberg]

On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote:
> Intel Quark X1000 GPIO controller supports interrupt handling for
> both core power well and resume power well. This patch is to enable
> the IRQ support and provide IRQ handling for Intel Quark X1000
> GPIO-SCH device driver.
> 
> This piece of work is derived from Dan O'Donovan's initial work for
> Quark X1000 enabling.
> 
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>

In addition to what Varka Bhadram pointed out, I have few comments. See
below.

Overall, looks good.

> ---
>  drivers/gpio/gpio-sch.c |  257 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 245 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 952990f..dd84b1f 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -28,17 +28,30 @@
>  #include <linux/pci_ids.h>
>  
>  #include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
>  
>  #define GEN	0x00
>  #define GIO	0x04
>  #define GLV	0x08
> +#define GTPE	0x0C
> +#define GTNE	0x10
> +#define GGPE	0x14
> +#define GSMI	0x18
> +#define GTS	0x1C
> +#define CGNMIEN	0x40
> +#define RGNMIEN	0x44
>  
>  struct sch_gpio {
>  	struct gpio_chip chip;
> +	struct irq_data data;
>  	spinlock_t lock;
>  	unsigned short iobase;
>  	unsigned short core_base;
>  	unsigned short resume_base;
> +	int irq_base;
> +	int irq_num;
> +	bool irq_support;
>  };
>  
>  #define to_sch_gpio(c)	container_of(c, struct sch_gpio, chip)
> @@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio)
>  static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
>  				  unsigned reg)
>  {
> +	unsigned long flags;
>  	unsigned short offset, bit;
>  	u8 enable;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  
>  	offset = sch_gpio_offset(sch, gpio, reg);
>  	bit = sch_gpio_bit(sch, gpio);
> @@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
>  	if (!(enable & BIT(bit)))
>  		outb(enable | BIT(bit), sch->iobase + offset);
>  
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  }
>  
>  static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
>  				    unsigned reg)
>  {
> +	unsigned long flags;
>  	unsigned short offset, bit;
>  	u8 disable;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  
>  	offset = sch_gpio_offset(sch, gpio, reg);
>  	bit = sch_gpio_bit(sch, gpio);
> @@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
>  	if (disable & BIT(bit))
>  		outb(disable & ~BIT(bit), sch->iobase + offset);
>  
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  }
>  
>  static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
> @@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg,
>  static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
>  {
>  	struct sch_gpio *sch = to_sch_gpio(gc);
> +	unsigned long flags;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_register_set(sch, gpio_num, GIO);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  	return 0;
>  }
>  
> @@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
>  static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
>  {
>  	struct sch_gpio *sch = to_sch_gpio(gc);
> +	unsigned long flags;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_reg_set(gc, gpio_num, GLV, val);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  }
>  
>  static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>  				  int val)
>  {
>  	struct sch_gpio *sch = to_sch_gpio(gc);
> +	unsigned long flags;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_register_clear(sch, gpio_num, GIO);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  
>  	/*
>  	 * according to the datasheet, writing to the level register has no
> @@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>  	return 0;
>  }
>  
> +static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> +	struct sch_gpio *sch = to_sch_gpio(gc);
> +
> +	return sch->irq_base + offset;
> +}
> +
>  static struct gpio_chip sch_gpio_chip = {
>  	.label			= "sch_gpio",
>  	.owner			= THIS_MODULE,
> @@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = {
>  	.get			= sch_gpio_get,
>  	.direction_output	= sch_gpio_direction_out,
>  	.set			= sch_gpio_set,
> +	.to_irq			= sch_gpio_to_irq,
>  };
>  
> +static void sch_gpio_irq_enable(struct irq_data *d)
> +{
> +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> +	u32 gpio_num;
> +
> +	gpio_num = d->irq - sch->irq_base;
> +	sch_gpio_register_set(sch, gpio_num, GGPE);
> +}
> +
> +static void sch_gpio_irq_disable(struct irq_data *d)
> +{
> +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> +	u32 gpio_num;
> +
> +	gpio_num = d->irq - sch->irq_base;
> +	sch_gpio_register_clear(sch, gpio_num, GGPE);
> +}
> +
> +static void sch_gpio_irq_ack(struct irq_data *d)
> +{
> +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> +	u32 gpio_num;
> +
> +	gpio_num = d->irq - sch->irq_base;
> +	sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1);

Maybe use the new sch_gpio_register_set() here instead?

> +}
> +
> +static int sch_gpio_irq_type(struct irq_data *d, unsigned type)
> +{
> +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> +	unsigned long flags;
> +	u32 gpio_num;
> +
> +	if (d == NULL)
> +		return -EINVAL;

How can that happen?

> +
> +	gpio_num = d->irq - sch->irq_base;
> +
> +	spin_lock_irqsave(&sch->lock, flags);
> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		sch_gpio_register_set(sch, gpio_num, GTPE);
> +		sch_gpio_register_clear(sch, gpio_num, GTNE);
> +		break;
> +
> +	case IRQ_TYPE_EDGE_FALLING:
> +		sch_gpio_register_set(sch, gpio_num, GTNE);
> +		sch_gpio_register_clear(sch, gpio_num, GTPE);
> +		break;
> +
> +	case IRQ_TYPE_EDGE_BOTH:
> +		sch_gpio_register_set(sch, gpio_num, GTPE);
> +		sch_gpio_register_set(sch, gpio_num, GTNE);
> +		break;
> +
> +	case IRQ_TYPE_NONE:
> +		sch_gpio_register_clear(sch, gpio_num, GTPE);
> +		sch_gpio_register_clear(sch, gpio_num, GTNE);
> +		break;
> +
> +	default:
> +		spin_unlock_irqrestore(&sch->lock, flags);
> +		return -EINVAL;
> +	}
> +
> +	spin_unlock_irqrestore(&sch->lock, flags);
> +
> +	return 0;
> +}
> +
> +static struct irq_chip sch_irq_chip = {
> +	.irq_enable	= sch_gpio_irq_enable,
> +	.irq_disable	= sch_gpio_irq_disable,
> +	.irq_ack	= sch_gpio_irq_ack,
> +	.irq_set_type	= sch_gpio_irq_type,
> +};
> +
> +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < num; i++) {
> +		irq_set_chip_data(i + sch->irq_base, sch);
> +		irq_set_chip_and_handler_name(i + sch->irq_base,
> +					      &sch_irq_chip,
> +					      handle_simple_irq,
> +					      "sch_gpio_irq_chip");

Hmm, I wonder if this 

		irq_set_chip_and_handler_name(i + sch->irq_base, &sch_irq_chip,
					handle_simple_irq, "sch_gpio_irq_chip");

looks better. Up to you.


> +	}
> +}
> +
> +static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < num; i++) {
> +		irq_set_chip_data(i + sch->irq_base, 0);
> +		irq_set_chip_and_handler_name(i + sch->irq_base, 0, 0, 0);
> +	}
> +}
> +
> +static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int num)
> +{
> +	unsigned long flags;
> +	unsigned int gpio_num;
> +
> +	spin_lock_irqsave(&sch->lock, flags);
> +
> +	for (gpio_num = 0; gpio_num < num; gpio_num++) {
> +		sch_gpio_register_clear(sch, gpio_num, GTPE);
> +		sch_gpio_register_clear(sch, gpio_num, GTNE);
> +		sch_gpio_register_clear(sch, gpio_num, GGPE);
> +		sch_gpio_register_clear(sch, gpio_num, GSMI);
> +
> +		if (gpio_num >= 2)
> +			sch_gpio_register_clear(sch, gpio_num, RGNMIEN);
> +		else
> +			sch_gpio_register_clear(sch, gpio_num, CGNMIEN);
> +
> +		/* clear any pending interrupts */
> +		sch_gpio_reg_set(&sch->chip, gpio_num, GTS, 1);
> +	}
> +
> +	spin_unlock_irqrestore(&sch->lock, flags);
> +}
> +
> +static irqreturn_t sch_gpio_irq_handler(int irq, void *dev_id)
> +{
> +	struct sch_gpio *sch = dev_id;
> +	int res;
> +	unsigned int i;
> +	int ret = IRQ_NONE;
> +
> +	for (i = 0; i < sch->chip.ngpio; i++) {
> +		res = sch_gpio_reg_get(&sch->chip, i, GTS);
> +		if (res) {
> +			/* clear by setting GTS to 1 */
> +			sch_gpio_reg_set(&sch->chip, i, GTS, 1);
> +			generic_handle_irq(sch->irq_base + i);
> +			ret = IRQ_HANDLED;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int sch_gpio_probe(struct platform_device *pdev)
>  {
>  	struct sch_gpio *sch;
> -	struct resource *res;
> +	struct resource *res, *irq;
> +	int err;
>  
>  	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
>  	if (!sch)
> @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev)
>  				 pdev->name))
>  		return -EBUSY;
>  
> +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	sch->irq_support = !!irq;
> +	if (sch->irq_support) {
> +		sch->irq_num = irq->start;
> +		if (sch->irq_num < 0) {

Is this really possible? I mean can't you just detect the irq support
by looking sch->irq_num? If it is > 0 then it has interrupt support. I
think that irq 0 is not valid.

> +			dev_warn(&pdev->dev,
> +				 "failed to obtain irq number for device\n");
> +			sch->irq_support = false;
> +		}
> +	}
> +
>  	spin_lock_init(&sch->lock);
>  	sch->iobase = res->start;
>  	sch->chip = sch_gpio_chip;
> @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	err = gpiochip_add(&sch->chip);
> +	if (err < 0)
> +		goto err_sch_gpio;
> +
> +	if (sch->irq_support) {
> +		sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
> +						NUMA_NO_NODE);
> +		if (sch->irq_base < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to allocate GPIO IRQ descs\n");
> +			goto err_sch_intr_chip;
> +		}
> +
> +		/* disable interrupts */
> +		sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
> +
> +		err = request_irq(sch->irq_num, sch_gpio_irq_handler,
> +				  IRQF_SHARED, KBUILD_MODNAME, sch);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"%s failed to request IRQ\n", __func__);
> +			goto err_sch_request_irq;
> +		}
> +
> +		sch_gpio_irqs_init(sch, sch->chip.ngpio);
> +	}
> +
>  	platform_set_drvdata(pdev, sch);
>  
> -	return gpiochip_add(&sch->chip);
> +	return 0;
> +
> +err_sch_request_irq:
> +	irq_free_descs(sch->irq_base, sch->chip.ngpio);
> +
> +err_sch_intr_chip:
> +	if (gpiochip_remove(&sch->chip))
> +		dev_err(&pdev->dev,
> +			"%s gpiochip_remove() failed\n", __func__);
> +
> +err_sch_gpio:
> +	release_region(res->start, resource_size(res));

This is not needed as devm_request_region() will clean it up for you
automatically. Even if probe() fails.

> +
> +	return err;
>  }
>  
>  static int sch_gpio_remove(struct platform_device *pdev)
>  {
>  	struct sch_gpio *sch = platform_get_drvdata(pdev);
>  
> +	if (sch->irq_support) {
> +		sch_gpio_irqs_deinit(sch, sch->chip.ngpio);
> +
> +		if (sch->irq_num >= 0)
> +			free_irq(sch->irq_num, sch);
> +
> +		irq_free_descs(sch->irq_base, sch->chip.ngpio);
> +	}
> +
>  	gpiochip_remove(&sch->chip);
>  	return 0;
>  }
> -- 
> 1.7.9.5

Re: [PATCHv3 1/3] gpio: sch: Consolidate similar algorithms

[Alexandre Courbot]

On Thu, Oct 16, 2014 at 12:51 PM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:
> Consolidating similar algorithms into common functions to make
> GPIO SCH simpler and manageable.
>
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> ---
>  drivers/gpio/gpio-sch.c |   95 ++++++++++++++++++++++++++---------------------
>  1 file changed, 53 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 99720c8..6e89be9 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -63,94 +63,105 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio)
>         return gpio % 8;
>  }
>
> -static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio)
> +static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
> +                                 unsigned reg)
>  {
>         unsigned short offset, bit;
>         u8 enable;
>
>         spin_lock(&sch->lock);

...

>
> -       offset = sch_gpio_offset(sch, gpio, GEN);
> +       offset = sch_gpio_offset(sch, gpio, reg);
>         bit = sch_gpio_bit(sch, gpio);
>
>         enable = inb(sch->iobase + offset);

I see identical blocks of code in sch_gpio_register_clear(),
sch_gpio_reg_get() and sch_gpio_reg_set() (and possibly other
functions?). Maybe this could be factorized into an inline function
that would return the "enable" variable?

Also, "enable" looks like the wrong name here. The exact same result
is later called "disable" and "curr_dirs" later.

> -       if (!(enable & (1 << bit)))
> -               outb(enable | (1 << bit), sch->iobase + offset);
> +       if (!(enable & BIT(bit)))
> +               outb(enable | BIT(bit), sch->iobase + offset);
>
>         spin_unlock(&sch->lock);
>  }
>
> -static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned  gpio_num)
> +static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
> +                                   unsigned reg)
>  {
> -       struct sch_gpio *sch = to_sch_gpio(gc);
> -       u8 curr_dirs;
>         unsigned short offset, bit;
> +       u8 disable;
>
>         spin_lock(&sch->lock);
>
> -       offset = sch_gpio_offset(sch, gpio_num, GIO);
> -       bit = sch_gpio_bit(sch, gpio_num);
> -
> -       curr_dirs = inb(sch->iobase + offset);
> +       offset = sch_gpio_offset(sch, gpio, reg);
> +       bit = sch_gpio_bit(sch, gpio);
>
> -       if (!(curr_dirs & (1 << bit)))
> -               outb(curr_dirs | (1 << bit), sch->iobase + offset);
> +       disable = inb(sch->iobase + offset);
> +       if (disable & BIT(bit))
> +               outb(disable & ~BIT(bit), sch->iobase + offset);
>
>         spin_unlock(&sch->lock);
> -       return 0;
>  }
>
> -static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
> +static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
>  {
>         struct sch_gpio *sch = to_sch_gpio(gc);
> -       int res;
>         unsigned short offset, bit;
> +       u8 curr_dirs;
>
> -       offset = sch_gpio_offset(sch, gpio_num, GLV);
> -       bit = sch_gpio_bit(sch, gpio_num);
> +       offset = sch_gpio_offset(sch, gpio, reg);
> +       bit = sch_gpio_bit(sch, gpio);
>
> -       res = !!(inb(sch->iobase + offset) & (1 << bit));
> +       curr_dirs = !!(inb(sch->iobase + offset) & BIT(bit));
>
> -       return res;
> +       return curr_dirs;
>  }
>
> -static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
> +static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg,
> +                            int val)
>  {
>         struct sch_gpio *sch = to_sch_gpio(gc);
> -       u8 curr_vals;
>         unsigned short offset, bit;
> +       u8 curr_dirs;
>
> -       spin_lock(&sch->lock);
> -
> -       offset = sch_gpio_offset(sch, gpio_num, GLV);
> -       bit = sch_gpio_bit(sch, gpio_num);
> +       offset = sch_gpio_offset(sch, gpio, reg);
> +       bit = sch_gpio_bit(sch, gpio);
>
> -       curr_vals = inb(sch->iobase + offset);
> +       curr_dirs = inb(sch->iobase + offset);
>
>         if (val)
> -               outb(curr_vals | (1 << bit), sch->iobase + offset);
> +               outb(curr_dirs | BIT(bit), sch->iobase + offset);
>         else
> -               outb((curr_vals & ~(1 << bit)), sch->iobase + offset);
> +               outb((curr_dirs & ~BIT(bit)), sch->iobase + offset);
> +}

Mmmm this really looks like sch_gpio_register_set() and
sch_gpio_register_clear() could just call sch_gpio_reg_set() right
after acquiring the spinlock. Also the names are very similar and it
is not clear why you need two different functions here. Couldn't you
call sch_gpio_reg_set(gc, gpio, reg, 1) in place of
sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for
sch_gpio_register_clear()? You may need to acquire the spinlock before
some call sites, but that's preferable to having very similar
functions bearing a very similar name IMHO.

>
> +static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
> +{
> +       struct sch_gpio *sch = to_sch_gpio(gc);
> +
> +       spin_lock(&sch->lock);
> +       sch_gpio_register_set(sch, gpio_num, GIO);

So here you are acquiring sch->lock, then calling
sch_gpio_register_set() which will try to acquire the same spinlock
first thing. Wouldn't that deadlock? Have you tested changing the
direction of a GPIO? This again speaks in favor or reducing the number
of similar functions in this file.

Unless I misunderstood something there are still some issues that make
this file harder to understand than it should, and which can sometimes
bork the system altogether. It is a good idea to cleanup this file,
but please try to go one step further - this should simplify locking
and ultimately get rid of the locking issues.

And hopefully I will also take less time to review v4. :P

Re: [PATCHv3 2/3] gpio: sch: Add support for Intel Quark X1000 SoC

[Alexandre Courbot]

On Thu, Oct 16, 2014 at 7:06 PM, Mika Westerberg
<mika.westerberg@intel.com> wrote:
> On Thu, Oct 16, 2014 at 11:51:14AM +0800, Chang Rebecca Swee Fun wrote:
>> Intel Quark X1000 provides a total of 16 GPIOs. The GPIOs are split between
>> the legacy I/O bridge and the GPIO controller.
>>
>> GPIO-SCH is the GPIO pins on legacy bridge for Intel Quark SoC.
>> Intel Quark X1000 has 2 GPIOs powered by the core power well and 6 from
>> the suspend power well.
>>
>> This piece of work is derived from Dan O'Donovan's initial work for Quark
>> X1000 enabling.
>>
>> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000

[Alexandre Courbot]

On Thu, Oct 16, 2014 at 12:51 PM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:
> Intel Quark X1000 GPIO controller supports interrupt handling for
> both core power well and resume power well. This patch is to enable
> the IRQ support and provide IRQ handling for Intel Quark X1000
> GPIO-SCH device driver.
>
> This piece of work is derived from Dan O'Donovan's initial work for
> Quark X1000 enabling.
>
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> ---
>  drivers/gpio/gpio-sch.c |  257 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 245 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 952990f..dd84b1f 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -28,17 +28,30 @@
>  #include <linux/pci_ids.h>
>
>  #include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
>
>  #define GEN    0x00
>  #define GIO    0x04
>  #define GLV    0x08
> +#define GTPE   0x0C
> +#define GTNE   0x10
> +#define GGPE   0x14
> +#define GSMI   0x18
> +#define GTS    0x1C
> +#define CGNMIEN        0x40
> +#define RGNMIEN        0x44
>
>  struct sch_gpio {
>         struct gpio_chip chip;
> +       struct irq_data data;
>         spinlock_t lock;
>         unsigned short iobase;
>         unsigned short core_base;
>         unsigned short resume_base;
> +       int irq_base;
> +       int irq_num;
> +       bool irq_support;
>  };
>
>  #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip)
> @@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio)
>  static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
>                                   unsigned reg)
>  {
> +       unsigned long flags;
>         unsigned short offset, bit;
>         u8 enable;
>
> -       spin_lock(&sch->lock);
> +       spin_lock_irqsave(&sch->lock, flags);
>
>         offset = sch_gpio_offset(sch, gpio, reg);
>         bit = sch_gpio_bit(sch, gpio);
> @@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
>         if (!(enable & BIT(bit)))
>                 outb(enable | BIT(bit), sch->iobase + offset);
>
> -       spin_unlock(&sch->lock);
> +       spin_unlock_irqrestore(&sch->lock, flags);
>  }
>
>  static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
>                                     unsigned reg)
>  {
> +       unsigned long flags;
>         unsigned short offset, bit;
>         u8 disable;
>
> -       spin_lock(&sch->lock);
> +       spin_lock_irqsave(&sch->lock, flags);
>
>         offset = sch_gpio_offset(sch, gpio, reg);
>         bit = sch_gpio_bit(sch, gpio);
> @@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
>         if (disable & BIT(bit))
>                 outb(disable & ~BIT(bit), sch->iobase + offset);
>
> -       spin_unlock(&sch->lock);
> +       spin_unlock_irqrestore(&sch->lock, flags);
>  }
>
>  static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
> @@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg,
>  static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
>  {
>         struct sch_gpio *sch = to_sch_gpio(gc);
> +       unsigned long flags;
>
> -       spin_lock(&sch->lock);
> +       spin_lock_irqsave(&sch->lock, flags);
>         sch_gpio_register_set(sch, gpio_num, GIO);
> -       spin_unlock(&sch->lock);
> +       spin_unlock_irqrestore(&sch->lock, flags);
>         return 0;
>  }
>
> @@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
>  static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
>  {
>         struct sch_gpio *sch = to_sch_gpio(gc);
> +       unsigned long flags;
>
> -       spin_lock(&sch->lock);
> +       spin_lock_irqsave(&sch->lock, flags);
>         sch_gpio_reg_set(gc, gpio_num, GLV, val);
> -       spin_unlock(&sch->lock);
> +       spin_unlock_irqrestore(&sch->lock, flags);
>  }
>
>  static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>                                   int val)
>  {
>         struct sch_gpio *sch = to_sch_gpio(gc);
> +       unsigned long flags;
>
> -       spin_lock(&sch->lock);
> +       spin_lock_irqsave(&sch->lock, flags);
>         sch_gpio_register_clear(sch, gpio_num, GIO);
> -       spin_unlock(&sch->lock);
> +       spin_unlock_irqrestore(&sch->lock, flags);
>
>         /*
>          * according to the datasheet, writing to the level register has no
> @@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>         return 0;
>  }
>
> +static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct sch_gpio *sch = to_sch_gpio(gc);
> +
> +       return sch->irq_base + offset;
> +}
> +
>  static struct gpio_chip sch_gpio_chip = {
>         .label                  = "sch_gpio",
>         .owner                  = THIS_MODULE,
> @@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = {
>         .get                    = sch_gpio_get,
>         .direction_output       = sch_gpio_direction_out,
>         .set                    = sch_gpio_set,
> +       .to_irq                 = sch_gpio_to_irq,
>  };
>
> +static void sch_gpio_irq_enable(struct irq_data *d)
> +{
> +       struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> +       u32 gpio_num;
> +
> +       gpio_num = d->irq - sch->irq_base;
> +       sch_gpio_register_set(sch, gpio_num, GGPE);
> +}
> +
> +static void sch_gpio_irq_disable(struct irq_data *d)
> +{
> +       struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> +       u32 gpio_num;
> +
> +       gpio_num = d->irq - sch->irq_base;
> +       sch_gpio_register_clear(sch, gpio_num, GGPE);
> +}
> +
> +static void sch_gpio_irq_ack(struct irq_data *d)
> +{
> +       struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> +       u32 gpio_num;
> +
> +       gpio_num = d->irq - sch->irq_base;
> +       sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1);
> +}
> +
> +static int sch_gpio_irq_type(struct irq_data *d, unsigned type)
> +{
> +       struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> +       unsigned long flags;
> +       u32 gpio_num;
> +
> +       if (d == NULL)
> +               return -EINVAL;
> +
> +       gpio_num = d->irq - sch->irq_base;
> +
> +       spin_lock_irqsave(&sch->lock, flags);
> +
> +       switch (type) {
> +       case IRQ_TYPE_EDGE_RISING:
> +               sch_gpio_register_set(sch, gpio_num, GTPE);
> +               sch_gpio_register_clear(sch, gpio_num, GTNE);

You will have the same problem as I pointed out in patch 1/3 that
sch_gpio_register_set/clear() will try to acquire the already-locked
sch->lock. No way this can ever work, or I am under a serious
misapprehension.

> +               break;
> +
> +       case IRQ_TYPE_EDGE_FALLING:
> +               sch_gpio_register_set(sch, gpio_num, GTNE);
> +               sch_gpio_register_clear(sch, gpio_num, GTPE);
> +               break;
> +
> +       case IRQ_TYPE_EDGE_BOTH:
> +               sch_gpio_register_set(sch, gpio_num, GTPE);
> +               sch_gpio_register_set(sch, gpio_num, GTNE);
> +               break;
> +
> +       case IRQ_TYPE_NONE:
> +               sch_gpio_register_clear(sch, gpio_num, GTPE);
> +               sch_gpio_register_clear(sch, gpio_num, GTNE);
> +               break;
> +
> +       default:
> +               spin_unlock_irqrestore(&sch->lock, flags);
> +               return -EINVAL;
> +       }
> +
> +       spin_unlock_irqrestore(&sch->lock, flags);
> +
> +       return 0;
> +}
> +
> +static struct irq_chip sch_irq_chip = {
> +       .irq_enable     = sch_gpio_irq_enable,
> +       .irq_disable    = sch_gpio_irq_disable,
> +       .irq_ack        = sch_gpio_irq_ack,
> +       .irq_set_type   = sch_gpio_irq_type,
> +};
> +
> +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < num; i++) {
> +               irq_set_chip_data(i + sch->irq_base, sch);
> +               irq_set_chip_and_handler_name(i + sch->irq_base,
> +                                             &sch_irq_chip,
> +                                             handle_simple_irq,
> +                                             "sch_gpio_irq_chip");
> +       }
> +}
> +
> +static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < num; i++) {
> +               irq_set_chip_data(i + sch->irq_base, 0);
> +               irq_set_chip_and_handler_name(i + sch->irq_base, 0, 0, 0);
> +       }
> +}
> +
> +static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int num)
> +{
> +       unsigned long flags;
> +       unsigned int gpio_num;
> +
> +       spin_lock_irqsave(&sch->lock, flags);
> +
> +       for (gpio_num = 0; gpio_num < num; gpio_num++) {
> +               sch_gpio_register_clear(sch, gpio_num, GTPE);
> +               sch_gpio_register_clear(sch, gpio_num, GTNE);
> +               sch_gpio_register_clear(sch, gpio_num, GGPE);
> +               sch_gpio_register_clear(sch, gpio_num, GSMI);

Same here.

> +
> +               if (gpio_num >= 2)
> +                       sch_gpio_register_clear(sch, gpio_num, RGNMIEN);
> +               else
> +                       sch_gpio_register_clear(sch, gpio_num, CGNMIEN);
> +
> +               /* clear any pending interrupts */
> +               sch_gpio_reg_set(&sch->chip, gpio_num, GTS, 1);
> +       }
> +
> +       spin_unlock_irqrestore(&sch->lock, flags);
> +}
> +
> +static irqreturn_t sch_gpio_irq_handler(int irq, void *dev_id)
> +{
> +       struct sch_gpio *sch = dev_id;
> +       int res;
> +       unsigned int i;
> +       int ret = IRQ_NONE;
> +
> +       for (i = 0; i < sch->chip.ngpio; i++) {
> +               res = sch_gpio_reg_get(&sch->chip, i, GTS);
> +               if (res) {
> +                       /* clear by setting GTS to 1 */
> +                       sch_gpio_reg_set(&sch->chip, i, GTS, 1);
> +                       generic_handle_irq(sch->irq_base + i);
> +                       ret = IRQ_HANDLED;
> +               }
> +       }
> +
> +       return ret;
> +}
> +
>  static int sch_gpio_probe(struct platform_device *pdev)
>  {
>         struct sch_gpio *sch;
> -       struct resource *res;
> +       struct resource *res, *irq;
> +       int err;
>
>         sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
>         if (!sch)
> @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev)
>                                  pdev->name))
>                 return -EBUSY;
>
> +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +       sch->irq_support = !!irq;
> +       if (sch->irq_support) {
> +               sch->irq_num = irq->start;
> +               if (sch->irq_num < 0) {
> +                       dev_warn(&pdev->dev,
> +                                "failed to obtain irq number for device\n");
> +                       sch->irq_support = false;
> +               }
> +       }
> +
>         spin_lock_init(&sch->lock);
>         sch->iobase = res->start;
>         sch->chip = sch_gpio_chip;
> @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> +       err = gpiochip_add(&sch->chip);
> +       if (err < 0)
> +               goto err_sch_gpio;
> +
> +       if (sch->irq_support) {
> +               sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
> +                                               NUMA_NO_NODE);
> +               if (sch->irq_base < 0) {
> +                       dev_err(&pdev->dev,
> +                               "failed to allocate GPIO IRQ descs\n");
> +                       goto err_sch_intr_chip;
> +               }
> +
> +               /* disable interrupts */
> +               sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
> +
> +               err = request_irq(sch->irq_num, sch_gpio_irq_handler,
> +                                 IRQF_SHARED, KBUILD_MODNAME, sch);
> +               if (err) {
> +                       dev_err(&pdev->dev,
> +                               "%s failed to request IRQ\n", __func__);
> +                       goto err_sch_request_irq;
> +               }
> +
> +               sch_gpio_irqs_init(sch, sch->chip.ngpio);
> +       }
> +
>         platform_set_drvdata(pdev, sch);
>
> -       return gpiochip_add(&sch->chip);
> +       return 0;
> +
> +err_sch_request_irq:
> +       irq_free_descs(sch->irq_base, sch->chip.ngpio);
> +
> +err_sch_intr_chip:
> +       if (gpiochip_remove(&sch->chip))
> +               dev_err(&pdev->dev,
> +                       "%s gpiochip_remove() failed\n", __func__);
> +
> +err_sch_gpio:
> +       release_region(res->start, resource_size(res));
> +
> +       return err;
>  }
>
>  static int sch_gpio_remove(struct platform_device *pdev)
>  {
>         struct sch_gpio *sch = platform_get_drvdata(pdev);
>
> +       if (sch->irq_support) {
> +               sch_gpio_irqs_deinit(sch, sch->chip.ngpio);
> +
> +               if (sch->irq_num >= 0)
> +                       free_irq(sch->irq_num, sch);
> +
> +               irq_free_descs(sch->irq_base, sch->chip.ngpio);
> +       }
> +
>         gpiochip_remove(&sch->chip);
>         return 0;
>  }
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv3 1/3] gpio: sch: Consolidate similar algorithms

[Chang, Rebecca Swee Fun]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 7758 bytes --]

Thanks for the review comments. Please check my reply below.

> -----Original Message-----
> From: Alexandre Courbot [mailto:gnurou@gmail.com]
> Sent: 17 October, 2014 4:44 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; Westerberg, Mika; GPIO Subsystem Mailing List; Linux Kernel
> Mailing List; Denis Turischev
> Subject: Re: [PATCHv3 1/3] gpio: sch: Consolidate similar algorithms
> 
> On Thu, Oct 16, 2014 at 12:51 PM, Chang Rebecca Swee Fun
> <rebecca.swee.fun.chang@intel.com> wrote:
> > Consolidating similar algorithms into common functions to make GPIO
> > SCH simpler and manageable.
> >
> > Signed-off-by: Chang Rebecca Swee Fun
> > <rebecca.swee.fun.chang@intel.com>
> > ---
> >  drivers/gpio/gpio-sch.c |   95 ++++++++++++++++++++++++++--------------------
> -
> >  1 file changed, 53 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c index
> > 99720c8..6e89be9 100644
> > --- a/drivers/gpio/gpio-sch.c
> > +++ b/drivers/gpio/gpio-sch.c
> > @@ -63,94 +63,105 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch,
> unsigned gpio)
> >         return gpio % 8;
> >  }
> >
> > -static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio)
> > +static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
> > +                                 unsigned reg)
> >  {
> >         unsigned short offset, bit;
> >         u8 enable;
> >
> >         spin_lock(&sch->lock);
> 
> ...
> 
> >
> > -       offset = sch_gpio_offset(sch, gpio, GEN);
> > +       offset = sch_gpio_offset(sch, gpio, reg);
> >         bit = sch_gpio_bit(sch, gpio);
> >
> >         enable = inb(sch->iobase + offset);
> 
> I see identical blocks of code in sch_gpio_register_clear(),
> sch_gpio_reg_get() and sch_gpio_reg_set() (and possibly other functions?).
> Maybe this could be factorized into an inline function that would return the
> "enable" variable?
> 
> Also, "enable" looks like the wrong name here. The exact same result is later
> called "disable" and "curr_dirs" later.

Referring to your comments below, this will be get rid of after the sch_gpio_register_set() and sch_gpio_register_clear() being replaced by sch_gpio_reg_set(gc, gpio, reg, 1) and sch_gpio_reg_set(gc, gpio, reg, 0).
There will be less identical block of "offset", "bit", and "enable" nor "disable". So there is no need to factorize the identical blocks into inline function.

> 
> > -       if (!(enable & (1 << bit)))
> > -               outb(enable | (1 << bit), sch->iobase + offset);
> > +       if (!(enable & BIT(bit)))
> > +               outb(enable | BIT(bit), sch->iobase + offset);
> >
> >         spin_unlock(&sch->lock);
> >  }
> >
> > -static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned
> > gpio_num)
> > +static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
> > +                                   unsigned reg)
> >  {
> > -       struct sch_gpio *sch = to_sch_gpio(gc);
> > -       u8 curr_dirs;
> >         unsigned short offset, bit;
> > +       u8 disable;
> >
> >         spin_lock(&sch->lock);
> >
> > -       offset = sch_gpio_offset(sch, gpio_num, GIO);
> > -       bit = sch_gpio_bit(sch, gpio_num);
> > -
> > -       curr_dirs = inb(sch->iobase + offset);
> > +       offset = sch_gpio_offset(sch, gpio, reg);
> > +       bit = sch_gpio_bit(sch, gpio);
> >
> > -       if (!(curr_dirs & (1 << bit)))
> > -               outb(curr_dirs | (1 << bit), sch->iobase + offset);
> > +       disable = inb(sch->iobase + offset);
> > +       if (disable & BIT(bit))
> > +               outb(disable & ~BIT(bit), sch->iobase + offset);
> >
> >         spin_unlock(&sch->lock);
> > -       return 0;
> >  }
> >
> > -static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
> > +static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio,
> > +unsigned reg)
> >  {
> >         struct sch_gpio *sch = to_sch_gpio(gc);
> > -       int res;
> >         unsigned short offset, bit;
> > +       u8 curr_dirs;
> >
> > -       offset = sch_gpio_offset(sch, gpio_num, GLV);
> > -       bit = sch_gpio_bit(sch, gpio_num);
> > +       offset = sch_gpio_offset(sch, gpio, reg);
> > +       bit = sch_gpio_bit(sch, gpio);
> >
> > -       res = !!(inb(sch->iobase + offset) & (1 << bit));
> > +       curr_dirs = !!(inb(sch->iobase + offset) & BIT(bit));
> >
> > -       return res;
> > +       return curr_dirs;
> >  }
> >
> > -static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int
> > val)
> > +static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned
> reg,
> > +                            int val)
> >  {
> >         struct sch_gpio *sch = to_sch_gpio(gc);
> > -       u8 curr_vals;
> >         unsigned short offset, bit;
> > +       u8 curr_dirs;
> >
> > -       spin_lock(&sch->lock);
> > -
> > -       offset = sch_gpio_offset(sch, gpio_num, GLV);
> > -       bit = sch_gpio_bit(sch, gpio_num);
> > +       offset = sch_gpio_offset(sch, gpio, reg);
> > +       bit = sch_gpio_bit(sch, gpio);
> >
> > -       curr_vals = inb(sch->iobase + offset);
> > +       curr_dirs = inb(sch->iobase + offset);
> >
> >         if (val)
> > -               outb(curr_vals | (1 << bit), sch->iobase + offset);
> > +               outb(curr_dirs | BIT(bit), sch->iobase + offset);
> >         else
> > -               outb((curr_vals & ~(1 << bit)), sch->iobase + offset);
> > +               outb((curr_dirs & ~BIT(bit)), sch->iobase + offset); }
> 
> Mmmm this really looks like sch_gpio_register_set() and
> sch_gpio_register_clear() could just call sch_gpio_reg_set() right after
> acquiring the spinlock. Also the names are very similar and it is not clear why
> you need two different functions here. Couldn't you call sch_gpio_reg_set(gc,
> gpio, reg, 1) in place of
> sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for
> sch_gpio_register_clear()? You may need to acquire the spinlock before some
> call sites, but that's preferable to having very similar functions bearing a very
> similar name IMHO.

Thanks for pointing that out. I think I can replaced all sch_gpio_register_set() and sch_gpio_register_clear() with sch_gpio_reg_set(gc, gpio, reg, 0/1).
Regarding the spinlock, in fact, I have tested using the GPIO driver through sysfs. I didn't encounter any deadlock issue, but the double locking is really an issue.
This double lock problem can also be fix by calling sch_gpio_reg_set(gc, gpio, reg, 0/1) in place of sch_gpio_register_set() and sch_gpio_register_clear().

> 
> >
> > +static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned
> > +gpio_num) {
> > +       struct sch_gpio *sch = to_sch_gpio(gc);
> > +
> > +       spin_lock(&sch->lock);
> > +       sch_gpio_register_set(sch, gpio_num, GIO);
> 
> So here you are acquiring sch->lock, then calling
> sch_gpio_register_set() which will try to acquire the same spinlock first thing.
> Wouldn't that deadlock? Have you tested changing the direction of a GPIO? This
> again speaks in favor or reducing the number of similar functions in this file.
> 
> Unless I misunderstood something there are still some issues that make this file
> harder to understand than it should, and which can sometimes bork the system
> altogether. It is a good idea to cleanup this file, but please try to go one step
> further - this should simplify locking and ultimately get rid of the locking issues.
> 
> And hopefully I will also take less time to review v4. :P

Thanks for the review. I will take note on the locking part and resend later or next week. Appreciate your review comments. Thank you!

Rebecca
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000

[Chang, Rebecca Swee Fun]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3698 bytes --]

> > +
> > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type)
> > +{
> > +       struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > +       unsigned long flags;
> > +       u32 gpio_num;
> > +
> > +       if (d == NULL)
> > +               return -EINVAL;
> > +
> > +       gpio_num = d->irq - sch->irq_base;
> > +
> > +       spin_lock_irqsave(&sch->lock, flags);
> > +
> > +       switch (type) {
> > +       case IRQ_TYPE_EDGE_RISING:
> > +               sch_gpio_register_set(sch, gpio_num, GTPE);
> > +               sch_gpio_register_clear(sch, gpio_num, GTNE);
> 
> You will have the same problem as I pointed out in patch 1/3 that
> sch_gpio_register_set/clear() will try to acquire the already-locked
> sch->lock. No way this can ever work, or I am under a serious
> misapprehension.
> 
> > +               break;
> > +
> > +       case IRQ_TYPE_EDGE_FALLING:
> > +               sch_gpio_register_set(sch, gpio_num, GTNE);
> > +               sch_gpio_register_clear(sch, gpio_num, GTPE);
> > +               break;
> > +
> > +       case IRQ_TYPE_EDGE_BOTH:
> > +               sch_gpio_register_set(sch, gpio_num, GTPE);
> > +               sch_gpio_register_set(sch, gpio_num, GTNE);
> > +               break;
> > +
> > +       case IRQ_TYPE_NONE:
> > +               sch_gpio_register_clear(sch, gpio_num, GTPE);
> > +               sch_gpio_register_clear(sch, gpio_num, GTNE);
> > +               break;
> > +
> > +       default:
> > +               spin_unlock_irqrestore(&sch->lock, flags);
> > +               return -EINVAL;
> > +       }
> > +
> > +       spin_unlock_irqrestore(&sch->lock, flags);
> > +
> > +       return 0;
> > +}
> > +
> > +static struct irq_chip sch_irq_chip = {
> > +       .irq_enable     = sch_gpio_irq_enable,
> > +       .irq_disable    = sch_gpio_irq_disable,
> > +       .irq_ack        = sch_gpio_irq_ack,
> > +       .irq_set_type   = sch_gpio_irq_type,
> > +};
> > +
> > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num)
> > +{
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < num; i++) {
> > +               irq_set_chip_data(i + sch->irq_base, sch);
> > +               irq_set_chip_and_handler_name(i + sch->irq_base,
> > +                                             &sch_irq_chip,
> > +                                             handle_simple_irq,
> > +                                             "sch_gpio_irq_chip");
> > +       }
> > +}
> > +
> > +static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num)
> > +{
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < num; i++) {
> > +               irq_set_chip_data(i + sch->irq_base, 0);
> > +               irq_set_chip_and_handler_name(i + sch->irq_base, 0, 0, 0);
> > +       }
> > +}
> > +
> > +static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int num)
> > +{
> > +       unsigned long flags;
> > +       unsigned int gpio_num;
> > +
> > +       spin_lock_irqsave(&sch->lock, flags);
> > +
> > +       for (gpio_num = 0; gpio_num < num; gpio_num++) {
> > +               sch_gpio_register_clear(sch, gpio_num, GTPE);
> > +               sch_gpio_register_clear(sch, gpio_num, GTNE);
> > +               sch_gpio_register_clear(sch, gpio_num, GGPE);
> > +               sch_gpio_register_clear(sch, gpio_num, GSMI);
> 
> Same here.


Alright. I should have noticed this double locking issue earlier. Thanks. I think the next version will be much cleaner. Thanks for spending time and effort to review.

Rebecca
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000

[Chang, Rebecca Swee Fun]

Sorry for the late reply, was working on something else.

> -----Original Message-----
> From: Westerberg, Mika
> Sent: 16 October, 2014 6:19 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; GPIO Subsystem Mailing List; Linux Kernel Mailing List; Denis
> Turischev
> Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
> 
> On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote:
> > Intel Quark X1000 GPIO controller supports interrupt handling for both
> > core power well and resume power well. This patch is to enable the IRQ
> > support and provide IRQ handling for Intel Quark X1000 GPIO-SCH device
> > driver.
> >
> > This piece of work is derived from Dan O'Donovan's initial work for
> > Quark X1000 enabling.
> >
> > Signed-off-by: Chang Rebecca Swee Fun
> > <rebecca.swee.fun.chang@intel.com>
> 
> In addition to what Varka Bhadram pointed out, I have few comments. See
> below.
> 
> Overall, looks good.
> 
> > ---
> >  drivers/gpio/gpio-sch.c |  257
> > ++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 245 insertions(+), 12 deletions(-)
> >
(...)

> > +static void sch_gpio_irq_disable(struct irq_data *d) {
> > +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > +	u32 gpio_num;
> > +
> > +	gpio_num = d->irq - sch->irq_base;
> > +	sch_gpio_register_clear(sch, gpio_num, GGPE); }
> > +
> > +static void sch_gpio_irq_ack(struct irq_data *d) {
> > +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > +	u32 gpio_num;
> > +
> > +	gpio_num = d->irq - sch->irq_base;
> > +	sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1);
> 
> Maybe use the new sch_gpio_register_set() here instead?

According to Alexandre's review feedback, sch_gpio_register_set() and  sch_gpio_register_clear() actually having similar block of codes with sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio, reg, 1) in place of sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for sch_gpio_register_clear(). I'm now in the progress of reworking the patches in that direction.

> 
> > +}
> > +
> > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) {
> > +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > +	unsigned long flags;
> > +	u32 gpio_num;
> > +
> > +	if (d == NULL)
> > +		return -EINVAL;
> 
> How can that happen?

This is just to ensure the irq_data struct contains data. If you think this is no needed, I will remove it in next submission.
(...)
> > +
> > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int
> > +num) {
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		irq_set_chip_data(i + sch->irq_base, sch);
> > +		irq_set_chip_and_handler_name(i + sch->irq_base,
> > +					      &sch_irq_chip,
> > +					      handle_simple_irq,
> > +					      "sch_gpio_irq_chip");
> 
> Hmm, I wonder if this
> 
> 		irq_set_chip_and_handler_name(i + sch->irq_base,
> &sch_irq_chip,
> 					handle_simple_irq,
> "sch_gpio_irq_chip");
> 
> looks better. Up to you.
> 
I think I will take what you've suggested. :)

(...)
> >  static int sch_gpio_probe(struct platform_device *pdev)  {
> >  	struct sch_gpio *sch;
> > -	struct resource *res;
> > +	struct resource *res, *irq;
> > +	int err;
> >
> >  	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
> >  	if (!sch)
> > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device
> *pdev)
> >  				 pdev->name))
> >  		return -EBUSY;
> >
> > +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +	sch->irq_support = !!irq;
> > +	if (sch->irq_support) {
> > +		sch->irq_num = irq->start;
> > +		if (sch->irq_num < 0) {
> 
> Is this really possible? I mean can't you just detect the irq support by looking
> sch->irq_num? If it is > 0 then it has interrupt support. I think that irq 0 is not
> valid.

Yes, I can do that, but the irq_num was being passed from MFD LPC_SCH. This implementation is to avoid enabling irq support if LPC_SCH passed in a negative value.
As for irq 0, I think it is valid. Based on my observation in "cat /proc/interrupt" there is a 0 irq line available.

> 
> > +			dev_warn(&pdev->dev,
> > +				 "failed to obtain irq number for device\n");
> > +			sch->irq_support = false;
> > +		}
> > +	}
> > +
> >  	spin_lock_init(&sch->lock);
> >  	sch->iobase = res->start;
> >  	sch->chip = sch_gpio_chip;
> > @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device
> *pdev)
> >  		return -ENODEV;
> >  	}
> >
> > +	err = gpiochip_add(&sch->chip);
> > +	if (err < 0)
> > +		goto err_sch_gpio;
> > +
> > +	if (sch->irq_support) {
> > +		sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
> > +						NUMA_NO_NODE);
> > +		if (sch->irq_base < 0) {
> > +			dev_err(&pdev->dev,
> > +				"failed to allocate GPIO IRQ descs\n");
> > +			goto err_sch_intr_chip;
> > +		}
> > +
> > +		/* disable interrupts */
> > +		sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
> > +
> > +		err = request_irq(sch->irq_num, sch_gpio_irq_handler,
> > +				  IRQF_SHARED, KBUILD_MODNAME, sch);
> > +		if (err) {
> > +			dev_err(&pdev->dev,
> > +				"%s failed to request IRQ\n", __func__);
> > +			goto err_sch_request_irq;
> > +		}
> > +
> > +		sch_gpio_irqs_init(sch, sch->chip.ngpio);
> > +	}
> > +
> >  	platform_set_drvdata(pdev, sch);
> >
> > -	return gpiochip_add(&sch->chip);
> > +	return 0;
> > +
> > +err_sch_request_irq:
> > +	irq_free_descs(sch->irq_base, sch->chip.ngpio);
> > +
> > +err_sch_intr_chip:
> > +	if (gpiochip_remove(&sch->chip))
> > +		dev_err(&pdev->dev,
> > +			"%s gpiochip_remove() failed\n", __func__);
> > +
> > +err_sch_gpio:
> > +	release_region(res->start, resource_size(res));
> 
> This is not needed as devm_request_region() will clean it up for you
> automatically. Even if probe() fails.

Noted with thanks.

Rebecca

Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000

[Westerberg, Mika]

On Wed, Nov 05, 2014 at 11:33:01AM +0200, Chang, Rebecca Swee Fun wrote:
> Sorry for the late reply, was working on something else.
> 
> > -----Original Message-----
> > From: Westerberg, Mika
> > Sent: 16 October, 2014 6:19 PM
> > To: Chang, Rebecca Swee Fun
> > Cc: Linus Walleij; GPIO Subsystem Mailing List; Linux Kernel Mailing List; Denis
> > Turischev
> > Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
> > 
> > On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote:
> > > Intel Quark X1000 GPIO controller supports interrupt handling for both
> > > core power well and resume power well. This patch is to enable the IRQ
> > > support and provide IRQ handling for Intel Quark X1000 GPIO-SCH device
> > > driver.
> > >
> > > This piece of work is derived from Dan O'Donovan's initial work for
> > > Quark X1000 enabling.
> > >
> > > Signed-off-by: Chang Rebecca Swee Fun
> > > <rebecca.swee.fun.chang@intel.com>
> > 
> > In addition to what Varka Bhadram pointed out, I have few comments. See
> > below.
> > 
> > Overall, looks good.
> > 
> > > ---
> > >  drivers/gpio/gpio-sch.c |  257
> > > ++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 245 insertions(+), 12 deletions(-)
> > >
> (...)
> 
> > > +static void sch_gpio_irq_disable(struct irq_data *d) {
> > > +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > > +	u32 gpio_num;
> > > +
> > > +	gpio_num = d->irq - sch->irq_base;
> > > +	sch_gpio_register_clear(sch, gpio_num, GGPE); }
> > > +
> > > +static void sch_gpio_irq_ack(struct irq_data *d) {
> > > +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > > +	u32 gpio_num;
> > > +
> > > +	gpio_num = d->irq - sch->irq_base;
> > > +	sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1);
> > 
> > Maybe use the new sch_gpio_register_set() here instead?
> 
> According to Alexandre's review feedback, sch_gpio_register_set() and
> sch_gpio_register_clear() actually having similar block of codes with
> sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio,
> reg, 1) in place of sch_gpio_register_set() and sch_gpio_reg_set(gc,
> gpio, reg, 0) for sch_gpio_register_clear(). I'm now in the progress
> of reworking the patches in that direction.

Sounds good.

> > 
> > > +}
> > > +
> > > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) {
> > > +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > > +	unsigned long flags;
> > > +	u32 gpio_num;
> > > +
> > > +	if (d == NULL)
> > > +		return -EINVAL;
> > 
> > How can that happen?
> 
> This is just to ensure the irq_data struct contains data. If you think
> this is no needed, I will remove it in next submission.

I think you can drop that check. Also if 'd' would be NULL you will
crash already in

	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);

> (...)
> > > +
> > > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int
> > > +num) {
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < num; i++) {
> > > +		irq_set_chip_data(i + sch->irq_base, sch);
> > > +		irq_set_chip_and_handler_name(i + sch->irq_base,
> > > +					      &sch_irq_chip,
> > > +					      handle_simple_irq,
> > > +					      "sch_gpio_irq_chip");
> > 
> > Hmm, I wonder if this
> > 
> > 		irq_set_chip_and_handler_name(i + sch->irq_base,
> > &sch_irq_chip,
> > 					handle_simple_irq,
> > "sch_gpio_irq_chip");
> > 
> > looks better. Up to you.
> > 
> I think I will take what you've suggested. :)
> 
> (...)
> > >  static int sch_gpio_probe(struct platform_device *pdev)  {
> > >  	struct sch_gpio *sch;
> > > -	struct resource *res;
> > > +	struct resource *res, *irq;
> > > +	int err;
> > >
> > >  	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
> > >  	if (!sch)
> > > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device
> > *pdev)
> > >  				 pdev->name))
> > >  		return -EBUSY;
> > >
> > > +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > > +	sch->irq_support = !!irq;
> > > +	if (sch->irq_support) {
> > > +		sch->irq_num = irq->start;
> > > +		if (sch->irq_num < 0) {
> > 
> > Is this really possible? I mean can't you just detect the irq support by looking
> > sch->irq_num? If it is > 0 then it has interrupt support. I think that irq 0 is not
> > valid.
> 
> Yes, I can do that, but the irq_num was being passed from MFD LPC_SCH.
> This implementation is to avoid enabling irq support if LPC_SCH passed
> in a negative value.  As for irq 0, I think it is valid. Based on my
> observation in "cat /proc/interrupt" there is a 0 irq line available.

I see.

In that case, how about following?

	irq = platform_get_irq(pdev, 0)
	if (irq >= 0) {
		/* Add irq support here */
	}

> 
> > 
> > > +			dev_warn(&pdev->dev,
> > > +				 "failed to obtain irq number for device\n");
> > > +			sch->irq_support = false;
> > > +		}
> > > +	}
> > > +
> > >  	spin_lock_init(&sch->lock);
> > >  	sch->iobase = res->start;
> > >  	sch->chip = sch_gpio_chip;
> > > @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device
> > *pdev)
> > >  		return -ENODEV;
> > >  	}
> > >
> > > +	err = gpiochip_add(&sch->chip);
> > > +	if (err < 0)
> > > +		goto err_sch_gpio;
> > > +
> > > +	if (sch->irq_support) {
> > > +		sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
> > > +						NUMA_NO_NODE);
> > > +		if (sch->irq_base < 0) {
> > > +			dev_err(&pdev->dev,
> > > +				"failed to allocate GPIO IRQ descs\n");
> > > +			goto err_sch_intr_chip;
> > > +		}
> > > +
> > > +		/* disable interrupts */
> > > +		sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
> > > +
> > > +		err = request_irq(sch->irq_num, sch_gpio_irq_handler,
> > > +				  IRQF_SHARED, KBUILD_MODNAME, sch);
> > > +		if (err) {
> > > +			dev_err(&pdev->dev,
> > > +				"%s failed to request IRQ\n", __func__);
> > > +			goto err_sch_request_irq;
> > > +		}
> > > +
> > > +		sch_gpio_irqs_init(sch, sch->chip.ngpio);
> > > +	}
> > > +
> > >  	platform_set_drvdata(pdev, sch);
> > >
> > > -	return gpiochip_add(&sch->chip);
> > > +	return 0;
> > > +
> > > +err_sch_request_irq:
> > > +	irq_free_descs(sch->irq_base, sch->chip.ngpio);
> > > +
> > > +err_sch_intr_chip:
> > > +	if (gpiochip_remove(&sch->chip))
> > > +		dev_err(&pdev->dev,
> > > +			"%s gpiochip_remove() failed\n", __func__);
> > > +
> > > +err_sch_gpio:
> > > +	release_region(res->start, resource_size(res));
> > 
> > This is not needed as devm_request_region() will clean it up for you
> > automatically. Even if probe() fails.
> 
> Noted with thanks.
> 
> Rebecca