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

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

[Chang Rebecca Swee Fun]

Hi,

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

[PATCH 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..2189c22 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -43,6 +43,8 @@ struct sch_gpio {
 
 #define to_sch_gpio(c)	container_of(c, struct sch_gpio, chip)
 
+static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val);
+
 static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
 				unsigned reg)
 {
@@ -63,94 +65,89 @@ 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_enable(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_disable(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_enable(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)
 {
 	struct sch_gpio *sch = to_sch_gpio(gc);
-	u8 curr_dirs;
-	unsigned short offset, bit;
 
 	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);
-	if (curr_dirs & (1 << bit))
-		outb(curr_dirs & ~(1 << bit), sch->iobase + offset);
-
+	sch_gpio_disable(sch, gpio_num, GIO);
 	spin_unlock(&sch->lock);
 
 	/*
@@ -166,6 +163,20 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
 	return 0;
 }
 
+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);
+
+	spin_lock(&sch->lock);
+	sch_gpio_reg_set(gc, gpio_num, GLV, val);
+	spin_unlock(&sch->lock);
+}
+
 static struct gpio_chip sch_gpio_chip = {
 	.label			= "sch_gpio",
 	.owner			= THIS_MODULE,
@@ -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_enable(sch, 8, GEN);
+		sch_gpio_enable(sch, 9, GEN);
 		/*
 		 * SUS_GPIO[2:0] enabled by default
 		 * Enable SUS_GPIO3 resume powered gpio explicitly
 		 */
-		sch_gpio_enable(sch, 13);
+		sch_gpio_enable(sch, 13, GEN);
 		break;
 
 	case PCI_DEVICE_ID_INTEL_ITC_LPC:
-- 
1.7.9.5

[PATCH 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 2189c22..38d6e74 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

[PATCH 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 38d6e74..c6c64a5 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)
@@ -67,10 +80,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio)
 
 static void sch_gpio_enable(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);
@@ -79,15 +93,16 @@ static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio, unsigned reg)
 	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_disable(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_disable(struct sch_gpio *sch, unsigned gpio, unsigned reg)
 	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_enable(sch, gpio_num, GIO);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 	return 0;
 }
 
@@ -145,10 +161,11 @@ 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_disable(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
@@ -171,10 +188,18 @@ 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_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 = {
@@ -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_enable(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_disable(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_enable(sch, gpio_num, GTPE);
+		sch_gpio_disable(sch, gpio_num, GTNE);
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		sch_gpio_enable(sch, gpio_num, GTNE);
+		sch_gpio_disable(sch, gpio_num, GTPE);
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		sch_gpio_enable(sch, gpio_num, GTPE);
+		sch_gpio_enable(sch, gpio_num, GTNE);
+		break;
+
+	case IRQ_TYPE_NONE:
+		sch_gpio_disable(sch, gpio_num, GTPE);
+		sch_gpio_disable(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_disable(sch, gpio_num, GTPE);
+		sch_gpio_disable(sch, gpio_num, GTNE);
+		sch_gpio_disable(sch, gpio_num, GGPE);
+		sch_gpio_disable(sch, gpio_num, GSMI);
+
+		if (gpio_num >= 2)
+			sch_gpio_disable(sch, gpio_num, RGNMIEN);
+		else
+			sch_gpio_disable(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: [PATCH 1/3] gpio: sch: Consolidate similar algorithms

[Mika Westerberg]

On Wed, Sep 17, 2014 at 04:49:03PM +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>
> ---
>  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..2189c22 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -43,6 +43,8 @@ struct sch_gpio {
>  
>  #define to_sch_gpio(c)	container_of(c, struct sch_gpio, chip)
>  
> +static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val);
> +

Is it possible to move the sch_gpio_set() function here instead of
forward declaring it?

>  static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
>  				unsigned reg)
>  {
> @@ -63,94 +65,89 @@ 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_enable(struct sch_gpio *sch, unsigned gpio, unsigned reg)

I don't see much value in doing this.

To me sch_gpio_enable(sch, gpio) is more intuitive than
sch_gpio_enable(sch, gpio, GEN). Why do I need to pass register to
enable function in the first place? It should know better how to enable
the GPIO, right?

Same goes for others.

>  {
>  	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_disable(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_enable(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)
>  {
>  	struct sch_gpio *sch = to_sch_gpio(gc);
> -	u8 curr_dirs;
> -	unsigned short offset, bit;
>  
>  	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);
> -	if (curr_dirs & (1 << bit))
> -		outb(curr_dirs & ~(1 << bit), sch->iobase + offset);
> -
> +	sch_gpio_disable(sch, gpio_num, GIO);
>  	spin_unlock(&sch->lock);
>  
>  	/*
> @@ -166,6 +163,20 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>  	return 0;
>  }
>  
> +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);
> +
> +	spin_lock(&sch->lock);
> +	sch_gpio_reg_set(gc, gpio_num, GLV, val);
> +	spin_unlock(&sch->lock);
> +}
> +
>  static struct gpio_chip sch_gpio_chip = {
>  	.label			= "sch_gpio",
>  	.owner			= THIS_MODULE,
> @@ -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_enable(sch, 8, GEN);
> +		sch_gpio_enable(sch, 9, GEN);

For example here, which one is more readable?

>  		/*
>  		 * SUS_GPIO[2:0] enabled by default
>  		 * Enable SUS_GPIO3 resume powered gpio explicitly
>  		 */
> -		sch_gpio_enable(sch, 13);
> +		sch_gpio_enable(sch, 13, GEN);

Or here?

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

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

[Mika Westerberg]

On Wed, Sep 17, 2014 at 04:49:04PM +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>

One question, see below. But in general looks good, so

Reviewed-by: Mika Westerberg <mika.westerberg@linux.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 2189c22..38d6e74 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:

Is this PCI ID provided by some other patch series?

> +		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/

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

[Mika Westerberg]

On Wed, Sep 17, 2014 at 04:49:05PM +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>
> ---
>  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 38d6e74..c6c64a5 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)
> @@ -67,10 +80,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio)
>  
>  static void sch_gpio_enable(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);
> @@ -79,15 +93,16 @@ static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio, unsigned reg)
>  	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_disable(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_disable(struct sch_gpio *sch, unsigned gpio, unsigned reg)
>  	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_enable(sch, gpio_num, GIO);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  	return 0;
>  }
>  
> @@ -145,10 +161,11 @@ 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_disable(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
> @@ -171,10 +188,18 @@ 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_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 = {
> @@ -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_enable(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_disable(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_enable(sch, gpio_num, GTPE);
> +		sch_gpio_disable(sch, gpio_num, GTNE);
> +		break;
> +
> +	case IRQ_TYPE_EDGE_FALLING:
> +		sch_gpio_enable(sch, gpio_num, GTNE);
> +		sch_gpio_disable(sch, gpio_num, GTPE);
> +		break;
> +
> +	case IRQ_TYPE_EDGE_BOTH:
> +		sch_gpio_enable(sch, gpio_num, GTPE);
> +		sch_gpio_enable(sch, gpio_num, GTNE);
> +		break;
> +
> +	case IRQ_TYPE_NONE:
> +		sch_gpio_disable(sch, gpio_num, GTPE);
> +		sch_gpio_disable(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_disable(sch, gpio_num, GTPE);
> +		sch_gpio_disable(sch, gpio_num, GTNE);
> +		sch_gpio_disable(sch, gpio_num, GGPE);
> +		sch_gpio_disable(sch, gpio_num, GSMI);
> +
> +		if (gpio_num >= 2)
> +			sch_gpio_disable(sch, gpio_num, RGNMIEN);
> +		else
> +			sch_gpio_disable(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;
> +		}

Was there some reason why you can't use gpiochip_irqchip_* helpers here?

> +
> +		/* 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);

This seems weird, typically irqchip drivers don't call request_irq()
directly but instead irq_set_chained_handler() or similar. With
gpiochip_irqchip_* stuff you don't need even that.

> +		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
> 
> --
> 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/

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

[Chang, Rebecca Swee Fun]

> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: 18 September, 2014 7:23 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] gpio: sch: Add support for Intel Quark X1000 SoC
> 
> On Wed, Sep 17, 2014 at 04:49:04PM +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>
> 
> One question, see below. But in general looks good, so
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.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
> > 2189c22..38d6e74 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:
> 
> Is this PCI ID provided by some other patch series?

Yes, this PCI ID has been submitted to upstream through MFD subsystem tree.

Rebecca

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

[Chang, Rebecca Swee Fun]

Replied inline. :)

> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: 18 September, 2014 7:17 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/3] gpio: sch: Consolidate similar algorithms
> 
> On Wed, Sep 17, 2014 at 04:49:03PM +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>
> > ---
> >  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..2189c22 100644
> > --- a/drivers/gpio/gpio-sch.c
> > +++ b/drivers/gpio/gpio-sch.c
> > @@ -43,6 +43,8 @@ struct sch_gpio {
> >
> >  #define to_sch_gpio(c)	container_of(c, struct sch_gpio, chip)
> >
> > +static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int
> > +val);
> > +
> 
> Is it possible to move the sch_gpio_set() function here instead of forward
> declaring it?

Yes, it is possible. There is a reason I submitted the code in this structure. By putting the sch_gpio_set() function below will makes the diff patch looks neat and easy for review.
If it doesn't make sense to make the patch for easy reviewing, I can change by moving the function above.

> 
> >  static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
> >  				unsigned reg)
> >  {
> > @@ -63,94 +65,89 @@ 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_enable(struct sch_gpio *sch, unsigned gpio,
> > +unsigned reg)
> 
> I don't see much value in doing this.
> 
> To me sch_gpio_enable(sch, gpio) is more intuitive than sch_gpio_enable(sch,
> gpio, GEN). Why do I need to pass register to enable function in the first place?
> It should know better how to enable the GPIO, right?
> 
> Same goes for others.

The register values are required when it comes to IRQ handling. By passing in the registers values, we can make full use of the algorithms without introducing extra/similar algorithms to compute other register offset values.
For example, we have other offset values to handle such as:-
GTPE	0x0C
GTNE	0x10
GGPE	0x14
GSMI	0x18
GTS	0x1C
CGNMIEN	0x40
RGNMIEN	0x44

Regards
Rebecca

> 
> >  {
> >  	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_disable(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_enable(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)
> >  {
> >  	struct sch_gpio *sch = to_sch_gpio(gc);
> > -	u8 curr_dirs;
> > -	unsigned short offset, bit;
> >
> >  	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);
> > -	if (curr_dirs & (1 << bit))
> > -		outb(curr_dirs & ~(1 << bit), sch->iobase + offset);
> > -
> > +	sch_gpio_disable(sch, gpio_num, GIO);
> >  	spin_unlock(&sch->lock);
> >
> >  	/*
> > @@ -166,6 +163,20 @@ static int sch_gpio_direction_out(struct gpio_chip
> *gc, unsigned gpio_num,
> >  	return 0;
> >  }
> >
> > +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);
> > +
> > +	spin_lock(&sch->lock);
> > +	sch_gpio_reg_set(gc, gpio_num, GLV, val);
> > +	spin_unlock(&sch->lock);
> > +}
> > +
> >  static struct gpio_chip sch_gpio_chip = {
> >  	.label			= "sch_gpio",
> >  	.owner			= THIS_MODULE,
> > @@ -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_enable(sch, 8, GEN);
> > +		sch_gpio_enable(sch, 9, GEN);
> 
> For example here, which one is more readable?
> 
> >  		/*
> >  		 * SUS_GPIO[2:0] enabled by default
> >  		 * Enable SUS_GPIO3 resume powered gpio explicitly
> >  		 */
> > -		sch_gpio_enable(sch, 13);
> > +		sch_gpio_enable(sch, 13, GEN);
> 
> Or here?
> 
> >  		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/

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

['Mika Westerberg']

On Mon, Sep 22, 2014 at 05:43:36AM +0000, Chang, Rebecca Swee Fun wrote:
> Replied inline. :)
> 
> > -----Original Message-----
> > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> > Sent: 18 September, 2014 7:17 PM
> > To: Chang, Rebecca Swee Fun
> > Cc: Linus Walleij; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 1/3] gpio: sch: Consolidate similar algorithms
> > 
> > On Wed, Sep 17, 2014 at 04:49:03PM +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>
> > > ---
> > >  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..2189c22 100644
> > > --- a/drivers/gpio/gpio-sch.c
> > > +++ b/drivers/gpio/gpio-sch.c
> > > @@ -43,6 +43,8 @@ struct sch_gpio {
> > >
> > >  #define to_sch_gpio(c)	container_of(c, struct sch_gpio, chip)
> > >
> > > +static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int
> > > +val);
> > > +
> > 
> > Is it possible to move the sch_gpio_set() function here instead of forward
> > declaring it?
> 
> Yes, it is possible. There is a reason I submitted the code in this
> structure. By putting the sch_gpio_set() function below will makes the
> diff patch looks neat and easy for review.  If it doesn't make sense
> to make the patch for easy reviewing, I can change by moving the
> function above.

I think that we are interested in the end result (e.g) the driver and if
we can avoid forward declarations the better.

> > 
> > >  static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
> > >  				unsigned reg)
> > >  {
> > > @@ -63,94 +65,89 @@ 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_enable(struct sch_gpio *sch, unsigned gpio,
> > > +unsigned reg)
> > 
> > I don't see much value in doing this.
> > 
> > To me sch_gpio_enable(sch, gpio) is more intuitive than sch_gpio_enable(sch,
> > gpio, GEN). Why do I need to pass register to enable function in the first place?
> > It should know better how to enable the GPIO, right?
> > 
> > Same goes for others.
> 
> The register values are required when it comes to IRQ handling. By
> passing in the registers values, we can make full use of the
> algorithms without introducing extra/similar algorithms to compute
> other register offset values.
> For example, we have other offset values to handle such as:-
> GTPE	0x0C
> GTNE	0x10
> GGPE	0x14
> GSMI	0x18
> GTS	0x1C
> CGNMIEN	0x40
> RGNMIEN	0x44

Well, can we at least call it something else than sch_gpio_enable()?
Perhaps sch_gpio_set_value() or so?

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

[Linus Walleij]

On Wed, Sep 17, 2014 at 10:49 AM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:

> 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

I'm waiting for Mika's comments to be addressed for this patch
series before reviewing further.

Yours,
Linus Walleij

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

[Chang, Rebecca Swee Fun]

> -----Original Message-----
> From: 'Mika Westerberg' [mailto:mika.westerberg@linux.intel.com]
> Sent: 22 September, 2014 5:25 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/3] gpio: sch: Consolidate similar algorithms
> 
> On Mon, Sep 22, 2014 at 05:43:36AM +0000, Chang, Rebecca Swee Fun wrote:
> > Replied inline. :)
> >
> > > -----Original Message-----
> > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> > > Sent: 18 September, 2014 7:17 PM
> > > To: Chang, Rebecca Swee Fun
> > > Cc: Linus Walleij; linux-gpio@vger.kernel.org;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 1/3] gpio: sch: Consolidate similar algorithms
> > >
> > > On Wed, Sep 17, 2014 at 04:49:03PM +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>
> > > > ---
> > > >  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..2189c22 100644
> > > > --- a/drivers/gpio/gpio-sch.c
> > > > +++ b/drivers/gpio/gpio-sch.c
> > > > @@ -43,6 +43,8 @@ struct sch_gpio {
> > > >
> > > >  #define to_sch_gpio(c)	container_of(c, struct sch_gpio, chip)
> > > >
> > > > +static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num,
> > > > +int val);
> > > > +
> > >
> > > Is it possible to move the sch_gpio_set() function here instead of
> > > forward declaring it?
> >
> > Yes, it is possible. There is a reason I submitted the code in this
> > structure. By putting the sch_gpio_set() function below will makes the
> > diff patch looks neat and easy for review.  If it doesn't make sense
> > to make the patch for easy reviewing, I can change by moving the
> > function above.
> 
> I think that we are interested in the end result (e.g) the driver and if we can
> avoid forward declarations the better.

Alright. I can move it up to avoid the forward declaration.

> 
> > >
> > > >  static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
> > > >  				unsigned reg)
> > > >  {
> > > > @@ -63,94 +65,89 @@ 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_enable(struct sch_gpio *sch, unsigned gpio,
> > > > +unsigned reg)
> > >
> > > I don't see much value in doing this.
> > >
> > > To me sch_gpio_enable(sch, gpio) is more intuitive than
> > > sch_gpio_enable(sch, gpio, GEN). Why do I need to pass register to enable
> function in the first place?
> > > It should know better how to enable the GPIO, right?
> > >
> > > Same goes for others.
> >
> > The register values are required when it comes to IRQ handling. By
> > passing in the registers values, we can make full use of the
> > algorithms without introducing extra/similar algorithms to compute
> > other register offset values.
> > For example, we have other offset values to handle such as:-
> > GTPE	0x0C
> > GTNE	0x10
> > GGPE	0x14
> > GSMI	0x18
> > GTS	0x1C
> > CGNMIEN	0x40
> > RGNMIEN	0x44
> 
> Well, can we at least call it something else than sch_gpio_enable()?
> Perhaps sch_gpio_set_value() or so?

sch_gpio_set_value() sounds good. After think twice, I intend to merge sch_gpio_enable() and sch_gpio_disable() into one functions. Using variable "enable" as an indicator, I can control whether to enable or disable when calling the function. Here is my draft:

static void sch_gpio_set_value(struct sch_gpio *sch, unsigned gpio, unsigned reg, unsigned int enable)
{
	unsigned long flags;
	unsigned short offset, bit;
	u8 set;

	spin_lock_irqsave(&sch->lock, flags);
	offset = sch_gpio_offset(sch, gpio, reg);
	bit = sch_gpio_bit(sch, gpio);

	set = inb(sch->iobase + offset); 
	if (enable)
		outb(set | BIT(bit), sch->iobase + offset);
	else
		outb(disable & ~BIT(bit), sch->iobase + offset);

	spin_unlock_irqrestore(&sch->lock, flags);
}

Do you think this make sense? I am in the progress of implementing and testing.

Rebecca

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

['Mika Westerberg']

On Wed, Sep 24, 2014 at 12:55:07AM +0000, Chang, Rebecca Swee Fun wrote:
> > > The register values are required when it comes to IRQ handling. By
> > > passing in the registers values, we can make full use of the
> > > algorithms without introducing extra/similar algorithms to compute
> > > other register offset values.
> > > For example, we have other offset values to handle such as:-
> > > GTPE	0x0C
> > > GTNE	0x10
> > > GGPE	0x14
> > > GSMI	0x18
> > > GTS	0x1C
> > > CGNMIEN	0x40
> > > RGNMIEN	0x44
> > 
> > Well, can we at least call it something else than sch_gpio_enable()?
> > Perhaps sch_gpio_set_value() or so?
> 
> sch_gpio_set_value() sounds good. After think twice, I intend to merge
> sch_gpio_enable() and sch_gpio_disable() into one functions. Using
> variable "enable" as an indicator, I can control whether to enable or
> disable when calling the function. Here is my draft:

Actually sch_gpio_set_value() is too close to sch_gpio_set() which sets
the GPIO to 1 or 0. How about sch_gpio_register_set() or something along
those lines?

And I don't think it is good idea to add yet another functionality, like
enable there. Please leave sch_gpio_enable()/sch_gpio_disable() as is.

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

[Chang, Rebecca Swee Fun]

> -----Original Message-----
> From: 'Mika Westerberg' [mailto:mika.westerberg@linux.intel.com]
> Sent: 24 September, 2014 5:51 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/3] gpio: sch: Consolidate similar algorithms
> 
> On Wed, Sep 24, 2014 at 12:55:07AM +0000, Chang, Rebecca Swee Fun wrote:
> > > > The register values are required when it comes to IRQ handling. By
> > > > passing in the registers values, we can make full use of the
> > > > algorithms without introducing extra/similar algorithms to compute
> > > > other register offset values.
> > > > For example, we have other offset values to handle such as:-
> > > > GTPE	0x0C
> > > > GTNE	0x10
> > > > GGPE	0x14
> > > > GSMI	0x18
> > > > GTS	0x1C
> > > > CGNMIEN	0x40
> > > > RGNMIEN	0x44
> > >
> > > Well, can we at least call it something else than sch_gpio_enable()?
> > > Perhaps sch_gpio_set_value() or so?
> >
> > sch_gpio_set_value() sounds good. After think twice, I intend to merge
> > sch_gpio_enable() and sch_gpio_disable() into one functions. Using
> > variable "enable" as an indicator, I can control whether to enable or
> > disable when calling the function. Here is my draft:
> 
> Actually sch_gpio_set_value() is too close to sch_gpio_set() which sets the GPIO
> to 1 or 0. How about sch_gpio_register_set() or something along those lines?
> 
> And I don't think it is good idea to add yet another functionality, like enable
> there. Please leave sch_gpio_enable()/sch_gpio_disable() as is.

Alright, I will change the sch_gpio_enable()/sch_gpio_disable() into sch_gpio_register_set()/sch_gpio_register_clear().
Thanks.

Rebecca

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

[Chang, Rebecca Swee Fun]

> > @@ -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;
> > +		}
> 
> Was there some reason why you can't use gpiochip_irqchip_* helpers here?

I will look into the helpers function and see what I can change here.

> 
> > +
> > +		/* 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);
> 
> This seems weird, typically irqchip drivers don't call request_irq() directly but
> instead irq_set_chained_handler() or similar. With
> gpiochip_irqchip_* stuff you don't need even that.
> 
Regarding this, gpio-sch is actually using shared interrupts and the IRQ resources are from ACPI SCI. As per my understanding, resources from ACPI SCI might be shared for power management purposes. In this case, irq_set_chained_handler() might not be able to use here. What do you think?

Rebecca

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

['Mika Westerberg']

On Fri, Sep 26, 2014 at 09:14:48AM +0000, Chang, Rebecca Swee Fun wrote:
> > > @@ -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;
> > > +		}
> > 
> > Was there some reason why you can't use gpiochip_irqchip_* helpers here?
> 
> I will look into the helpers function and see what I can change here.
> 
> > 
> > > +
> > > +		/* 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);
> > 
> > This seems weird, typically irqchip drivers don't call request_irq() directly but
> > instead irq_set_chained_handler() or similar. With
> > gpiochip_irqchip_* stuff you don't need even that.
> > 
> Regarding this, gpio-sch is actually using shared interrupts and the
> IRQ resources are from ACPI SCI. As per my understanding, resources
> from ACPI SCI might be shared for power management purposes. In this
> case, irq_set_chained_handler() might not be able to use here. What do
> you think?

I think you are right. And then you can't use gpiochip_irqchip_* helpers
either :-(

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

[Chang, Rebecca Swee Fun]

> -----Original Message-----
> From: 'Mika Westerberg' [mailto:mika.westerberg@linux.intel.com]
> Sent: 26 September, 2014 5:18 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/3] gpio: sch: Enable IRQ support for Quark X1000
> 
> On Fri, Sep 26, 2014 at 09:14:48AM +0000, Chang, Rebecca Swee Fun wrote:
> > > > @@ -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;
> > > > +		}
> > >
> > > Was there some reason why you can't use gpiochip_irqchip_* helpers here?
> >
> > I will look into the helpers function and see what I can change here.
> >
> > >
> > > > +
> > > > +		/* 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);
> > >
> > > This seems weird, typically irqchip drivers don't call request_irq()
> > > directly but instead irq_set_chained_handler() or similar. With
> > > gpiochip_irqchip_* stuff you don't need even that.
> > >
> > Regarding this, gpio-sch is actually using shared interrupts and the
> > IRQ resources are from ACPI SCI. As per my understanding, resources
> > from ACPI SCI might be shared for power management purposes. In this
> > case, irq_set_chained_handler() might not be able to use here. What do
> > you think?
> 
> I think you are right. And then you can't use gpiochip_irqchip_* helpers either :-
> (

Then I shall submit the changes for the first patch in V2 series for further review. Thanks.

Rebecca

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

[Linus Walleij]

On Fri, Sep 26, 2014 at 11:18 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Sep 26, 2014 at 09:14:48AM +0000, Chang, Rebecca Swee Fun wrote:

>> > > +
>> > > +         /* 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);
>> >
>> > This seems weird, typically irqchip drivers don't call request_irq() directly but
>> > instead irq_set_chained_handler() or similar. With
>> > gpiochip_irqchip_* stuff you don't need even that.
>> >
>> Regarding this, gpio-sch is actually using shared interrupts and the
>> IRQ resources are from ACPI SCI. As per my understanding, resources
>> from ACPI SCI might be shared for power management purposes. In this
>> case, irq_set_chained_handler() might not be able to use here. What do
>> you think?
>
> I think you are right. And then you can't use gpiochip_irqchip_* helpers
> either :-(

No since that implies that the gpiochip lives in its own irqdomain,
and this driver uses a linear range of irqs provided from another
domain, just allocates descriptors for them.

I'll take a look at this and see if it's merge material now.

Yours,
Linus Walleij