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

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

[Chang Rebecca Swee Fun]

Hi,

This is a revised version for gpio-sch.

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:
This patch series is about enabling legacy GPIO support for Quark X1000.
The patches were developed on top of Mika Westerberg's commit on
consolidating core and resume banks. Please refer to the link below for
more information about his commit.
https://lkml.org/lkml/2014/8/17/13

The patches are generally enable GPIO support for Intel Quark X1000 SoC.
In the first patch of the series, I've also done some consolidating work as
there are similar algorithms that can be merged and generalized.

The second patch is about adding Quark X1000 pci ids and gpio pins supported
in the legacy gpio bridge.

The last patch in the series is about enable IRQ handling for gpio-sch. Intel
Quark X1000's legacy GPIO is an IRQ based GPIO. The IRQ resources will be
provided by MFD's lpc_sch.c. The changes in MFD (lpc_sch.c) required in order
to support gpio-sch has been merged into linux-next.git.

The patches has been built and tested working on Intel Galileo board.

Thank you.

Regards
Rebecca

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 |  362 ++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 318 insertions(+), 55 deletions(-)

-- 
1.7.9.5

[PATCHv2 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

[PATCHv2 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 690904a..64683a9 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -356,25 +356,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

[PATCHv2 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 |  267 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 253 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 952990f..332ffaf 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;
+	int 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 = 0;
+		}
+	}
+
 	spin_lock_init(&sch->lock);
 	sch->iobase = res->start;
 	sch->chip = sch_gpio_chip;
@@ -251,17 +435,72 @@ 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 add GPIO IRQ descs\n");
+			sch->irq_base = -1;
+			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);
+	int err;
 
-	gpiochip_remove(&sch->chip);
-	return 0;
+	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);
+	}
+
+	err = gpiochip_remove(&sch->chip);
+	if (err)
+		dev_err(&pdev->dev,
+			"%s gpiochip_remove() failed\n", __func__);
+
+	return err;
 }
 
 static struct platform_driver sch_gpio_driver = {
-- 
1.7.9.5

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

[Andy Shevchenko]

On Fri, Sep 26, 2014 at 1:07 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);
> -       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);

Looks like internal parentheses are redundant.

> +}
>
> +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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Sep 26, 2014 at 1:07 PM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> 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>
> ---
>  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 690904a..64683a9 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -356,25 +356,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"

Do we need 'X1000' here right now?

>         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

I think you may use comma before 'or' word.

> +         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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCHv2 0/3] Enable Quark X1000 support in gpio-sch

[Alexandre Courbot]

On Fri, Sep 26, 2014 at 7:07 PM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:
> Hi,
>
> This is a revised version for gpio-sch.
>
> 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:
> This patch series is about enabling legacy GPIO support for Quark X1000.
> The patches were developed on top of Mika Westerberg's commit on
> consolidating core and resume banks. Please refer to the link below for
> more information about his commit.
> https://lkml.org/lkml/2014/8/17/13

Sorry for the late review. I tried to apply the patch you mentioned
above before your series, and even Mika's patch won't apply on Linus'
devel branch or today's -next. This make it difficult to make a good
review. Could you rebase and resend this series once all its
dependencies have been merge by Linus W. ?

RE: [PATCHv2 0/3] Enable Quark X1000 support in gpio-sch

[Chang, Rebecca Swee Fun]

> -----Original Message-----
> From: Alexandre Courbot [mailto:gnurou@gmail.com]
> Sent: 09 October, 2014 2:29 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; Westerberg, Mika; GPIO Subsystem Mailing List; Linux Kernel
> Mailing List
> Subject: Re: [PATCHv2 0/3] Enable Quark X1000 support in gpio-sch
> 
> On Fri, Sep 26, 2014 at 7:07 PM, Chang Rebecca Swee Fun
> <rebecca.swee.fun.chang@intel.com> wrote:
> > Hi,
> >
> > This is a revised version for gpio-sch.
> >
> > 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:
> > This patch series is about enabling legacy GPIO support for Quark X1000.
> > The patches were developed on top of Mika Westerberg's commit on
> > consolidating core and resume banks. Please refer to the link below
> > for more information about his commit.
> > https://lkml.org/lkml/2014/8/17/13
> 
> Sorry for the late review. I tried to apply the patch you mentioned above before
> your series, and even Mika's patch won't apply on Linus'
> devel branch or today's -next. This make it difficult to make a good review.
> Could you rebase and resend this series once all its dependencies have been
> merge by Linus W. ?

Hi, I've noticed that Mika had sent a V2series on his work. Referring to his submission in: https://lkml.org/lkml/2014/9/16/213, this patch was able to patch on today's kernel tree. The patches on my series are able to patch in too. Do I need to resend since there is no changes on my side?

Rebecca

RE: [PATCHv2 0/3] Enable Quark X1000 support in gpio-sch

[Chang, Rebecca Swee Fun]

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



> -----Original Message-----
> From: Alexandre Courbot [mailto:gnurou@gmail.com]
> Sent: 09 October, 2014 2:29 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; Westerberg, Mika; GPIO Subsystem Mailing List; Linux Kernel
> Mailing List
> Subject: Re: [PATCHv2 0/3] Enable Quark X1000 support in gpio-sch
> 
> On Fri, Sep 26, 2014 at 7:07 PM, Chang Rebecca Swee Fun
> <rebecca.swee.fun.chang@intel.com> wrote:
> > Hi,
> >
> > This is a revised version for gpio-sch.
> >
> > 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:
> > This patch series is about enabling legacy GPIO support for Quark X1000.
> > The patches were developed on top of Mika Westerberg's commit on
> > consolidating core and resume banks. Please refer to the link below
> > for more information about his commit.
> > https://lkml.org/lkml/2014/8/17/13
> 
> Sorry for the late review. I tried to apply the patch you mentioned above before
> your series, and even Mika's patch won't apply on Linus'
> devel branch or today's -next. This make it difficult to make a good review.
> Could you rebase and resend this series once all its dependencies have been
> merge by Linus W. ?

Hi, I've noticed that Mika had sent a V2series on his work. Referring to his submission in: https://lkml.org/lkml/2014/9/16/213, this patch was able to patch on today's kernel tree. The patches on my series are able to patch in too. Do I need to resend since there is no changes on my side?

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: [PATCHv2 3/3] gpio: sch: Enable IRQ support for Quark X1000

[Linus Walleij]

On Fri, Sep 26, 2014 at 12:07 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>
(...)

This patch needs to be rebased on the gpio git "devel" branch or
Torvalds' HEAD before I can apply it.

>  #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

So the initial SCH driver for the Intel Poulsbo was submitted by Denis
Turischev in 2010.

Does these registers exist and work on the Poulsbo as well?

Is it really enough to distinguish between these variants by
checking if we're getting an IRQ resource on the device or not?
Is there some version register or so?

>  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;
> +       int irq_support;

Isn't that a bool?

> +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +       sch->irq_support = !!irq;

Yeah, it's a bool....

> +       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 = 0;

= false;

> +       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 add GPIO IRQ descs\n");

Failed to *allocate* actually...

> +                       sch->irq_base = -1;

This is overzealous. Drop it.

> +                       goto err_sch_intr_chip;

You're bailing out anyway, see.

>  static int sch_gpio_remove(struct platform_device *pdev)
>  {
>         struct sch_gpio *sch = platform_get_drvdata(pdev);
> +       int err;
>
> -       gpiochip_remove(&sch->chip);
> -       return 0;
> +       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);
> +       }
> +
> +       err = gpiochip_remove(&sch->chip);
> +       if (err)
> +               dev_err(&pdev->dev,
> +                       "%s gpiochip_remove() failed\n", __func__);

So gpiochip_remove() does *NOT* return an error in the current
kernel. We just removed that return value from the SCH driver in the
previous cycle for the reason that we were killing off the return type.
commit 9f5132ae82fdbb047cc187bf689a81c8cc0de7fa
"gpio: remove all usage of gpio_remove retval in driver/gpio"

So don't reintroduce stuff we're actively trying to get rid of.

Apart from this is looks OK, Mika can you ACK the end result?

Yours,
Linus Walleij

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

[Chang, Rebecca Swee Fun]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 15 October, 2014 3:13 PM
> To: Chang, Rebecca Swee Fun; Denis Turischev
> Cc: Westerberg, Mika; GPIO Subsystem Mailing List; Linux Kernel Mailing List
> Subject: Re: [PATCHv2 3/3] gpio: sch: Enable IRQ support for Quark X1000
> 
> On Fri, Sep 26, 2014 at 12:07 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>
> (...)
> 
> This patch needs to be rebased on the gpio git "devel" branch or Torvalds'
> HEAD before I can apply it.

I will rebase and resend with the fixes below.

> 
> >  #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
> 
> So the initial SCH driver for the Intel Poulsbo was submitted by Denis Turischev
> in 2010.
> 
> Does these registers exist and work on the Poulsbo as well?
> 
> Is it really enough to distinguish between these variants by checking if we're
> getting an IRQ resource on the device or not?
> Is there some version register or so?

The register values defined here are offset value, they are not the exact register address. 
They are not version register as it just carries a register offset value.

> >  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;
> > +       int irq_support;
> 
> Isn't that a bool?
> 
> > +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +       sch->irq_support = !!irq;
> 
> Yeah, it's a bool....

I will change it to bool.

> 
> > +       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 = 0;
> 
> = false;

Noted
> 
> > +       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 add GPIO IRQ descs\n");
> 
> Failed to *allocate* actually...
> 
> > +                       sch->irq_base = -1;
> 
> This is overzealous. Drop it.
> 
> > +                       goto err_sch_intr_chip;
> 
> You're bailing out anyway, see.

Noted. I will change the phrase accordingly and remove the expression on next submission.

> 
> >  static int sch_gpio_remove(struct platform_device *pdev)  {
> >         struct sch_gpio *sch = platform_get_drvdata(pdev);
> > +       int err;
> >
> > -       gpiochip_remove(&sch->chip);
> > -       return 0;
> > +       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);
> > +       }
> > +
> > +       err = gpiochip_remove(&sch->chip);
> > +       if (err)
> > +               dev_err(&pdev->dev,
> > +                       "%s gpiochip_remove() failed\n", __func__);
> 
> So gpiochip_remove() does *NOT* return an error in the current
> kernel. We just removed that return value from the SCH driver in the
> previous cycle for the reason that we were killing off the return type.
> commit 9f5132ae82fdbb047cc187bf689a81c8cc0de7fa
> "gpio: remove all usage of gpio_remove retval in driver/gpio"
> 
> So don't reintroduce stuff we're actively trying to get rid of.
> 
> Apart from this is looks OK, Mika can you ACK the end result?

Noted with thanks. I will do the changes required and resend the series.
Thanks.

Regards
Rebecca

RE: [PATCHv2 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: 4700 bytes --]



> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 15 October, 2014 3:13 PM
> To: Chang, Rebecca Swee Fun; Denis Turischev
> Cc: Westerberg, Mika; GPIO Subsystem Mailing List; Linux Kernel Mailing List
> Subject: Re: [PATCHv2 3/3] gpio: sch: Enable IRQ support for Quark X1000
> 
> On Fri, Sep 26, 2014 at 12:07 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>
> (...)
> 
> This patch needs to be rebased on the gpio git "devel" branch or Torvalds'
> HEAD before I can apply it.

I will rebase and resend with the fixes below.

> 
> >  #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
> 
> So the initial SCH driver for the Intel Poulsbo was submitted by Denis Turischev
> in 2010.
> 
> Does these registers exist and work on the Poulsbo as well?
> 
> Is it really enough to distinguish between these variants by checking if we're
> getting an IRQ resource on the device or not?
> Is there some version register or so?

The register values defined here are offset value, they are not the exact register address. 
They are not version register as it just carries a register offset value.

> >  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;
> > +       int irq_support;
> 
> Isn't that a bool?
> 
> > +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +       sch->irq_support = !!irq;
> 
> Yeah, it's a bool....

I will change it to bool.

> 
> > +       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 = 0;
> 
> = false;

Noted
> 
> > +       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 add GPIO IRQ descs\n");
> 
> Failed to *allocate* actually...
> 
> > +                       sch->irq_base = -1;
> 
> This is overzealous. Drop it.
> 
> > +                       goto err_sch_intr_chip;
> 
> You're bailing out anyway, see.

Noted. I will change the phrase accordingly and remove the expression on next submission.

> 
> >  static int sch_gpio_remove(struct platform_device *pdev)  {
> >         struct sch_gpio *sch = platform_get_drvdata(pdev);
> > +       int err;
> >
> > -       gpiochip_remove(&sch->chip);
> > -       return 0;
> > +       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);
> > +       }
> > +
> > +       err = gpiochip_remove(&sch->chip);
> > +       if (err)
> > +               dev_err(&pdev->dev,
> > +                       "%s gpiochip_remove() failed\n", __func__);
> 
> So gpiochip_remove() does *NOT* return an error in the current
> kernel. We just removed that return value from the SCH driver in the
> previous cycle for the reason that we were killing off the return type.
> commit 9f5132ae82fdbb047cc187bf689a81c8cc0de7fa
> "gpio: remove all usage of gpio_remove retval in driver/gpio"
> 
> So don't reintroduce stuff we're actively trying to get rid of.
> 
> Apart from this is looks OK, Mika can you ACK the end result?

Noted with thanks. I will do the changes required and resend the series.
Thanks.

Regards
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: [PATCHv2 0/3] Enable Quark X1000 support in gpio-sch

[Alexandre Courbot]

On Mon, Oct 13, 2014 at 12:41 PM, Chang, Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Alexandre Courbot [mailto:gnurou@gmail.com]
>> Sent: 09 October, 2014 2:29 PM
>> To: Chang, Rebecca Swee Fun
>> Cc: Linus Walleij; Westerberg, Mika; GPIO Subsystem Mailing List; Linux Kernel
>> Mailing List
>> Subject: Re: [PATCHv2 0/3] Enable Quark X1000 support in gpio-sch
>>
>> On Fri, Sep 26, 2014 at 7:07 PM, Chang Rebecca Swee Fun
>> <rebecca.swee.fun.chang@intel.com> wrote:
>> > Hi,
>> >
>> > This is a revised version for gpio-sch.
>> >
>> > 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:
>> > This patch series is about enabling legacy GPIO support for Quark X1000.
>> > The patches were developed on top of Mika Westerberg's commit on
>> > consolidating core and resume banks. Please refer to the link below
>> > for more information about his commit.
>> > https://lkml.org/lkml/2014/8/17/13
>>
>> Sorry for the late review. I tried to apply the patch you mentioned above before
>> your series, and even Mika's patch won't apply on Linus'
>> devel branch or today's -next. This make it difficult to make a good review.
>> Could you rebase and resend this series once all its dependencies have been
>> merge by Linus W. ?
>
> Hi, I've noticed that Mika had sent a V2series on his work. Referring to his submission in: https://lkml.org/lkml/2014/9/16/213, this patch was able to patch on today's kernel tree. The patches on my series are able to patch in too. Do I need to resend since there is no changes on my side?

I got your v3 applied on top of Linus' devel tree and Mika's patch
without any issue. Thanks.