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

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

[Chang Rebecca Swee Fun]

Hi all,

This is a revised version for enabling Quark X1000 support in gpio-sch.
This version of patch series had changed according to the feedback provided
by Alexandre and Mika.

Change log for V4:
Patch 1:
- Removed redundant/duplicated functions of sch_gpio_register_set() and
  sch_gpio_register_clear(). The function call had been replaced by
  sch_gpio_reg_set(gc, gpio, reg, 1) in place of sch_gpio_register_set() and
  sch_gpio_reg_set(gc, gpio, reg, 0) for sch_gpio_register_clear().
- Resolved double spinlock issue caught by Alexandre.

Patch 3:
- Dropped the usage of "if" block that checking irq_data struct
- Restructured the irq detect by using platform_get_irq(pdev, 0) instead of
  platform_get_resource(pdev, IORESOURCE_IRQ, 0) to get IRQ resources from
  MFD LPS-SCH.

The patches need to be patched on top of Mika Westerberg's commit at:
gpio: sch: Consolidate core and resume banks
http://marc.info/?l=linux-kernel&m=141392647225885&w=2

The patches has been verifed and tested working on Galileo Board. GPIO sysfs
was able to export gpio pins and changing pin direction. GPIO values were
able to controlled and interrupt was enabled.

Please help to review the patches so that we can make it to upstream kernel
as soon as possible. Thanks for all the review comments during this period of
review cycle.

Regards,
Rebecca

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

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

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

Version 1:
Initial version.

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

 drivers/gpio/Kconfig    |   11 +-
 drivers/gpio/gpio-sch.c |  316 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 266 insertions(+), 61 deletions(-)

-- 
1.7.9.5

[PATCHv4 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 |   81 ++++++++++++++++-------------------------------
 1 file changed, 28 insertions(+), 53 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 99720c8..7967f14 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -63,75 +63,59 @@ 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 int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
 {
+	struct sch_gpio *sch = to_sch_gpio(gc);
 	unsigned short offset, bit;
-	u8 enable;
-
-	spin_lock(&sch->lock);
+	u8 curr_dirs;
 
-	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);
+	curr_dirs = !!(inb(sch->iobase + offset) & BIT(bit));
 
-	spin_unlock(&sch->lock);
+	return curr_dirs;
 }
 
-static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned  gpio_num)
+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_dirs;
 	unsigned short offset, bit;
+	u8 curr_dirs;
 
-	spin_lock(&sch->lock);
-
-	offset = sch_gpio_offset(sch, gpio_num, GIO);
-	bit = sch_gpio_bit(sch, gpio_num);
+	offset = sch_gpio_offset(sch, gpio, reg);
+	bit = sch_gpio_bit(sch, gpio);
 
 	curr_dirs = inb(sch->iobase + offset);
 
-	if (!(curr_dirs & (1 << bit)))
-		outb(curr_dirs | (1 << bit), sch->iobase + offset);
+	if (val)
+		outb(curr_dirs | BIT(bit), sch->iobase + offset);
+	else
+		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_reg_set(sch, gpio_num, GIO, 1);
 	spin_unlock(&sch->lock);
 	return 0;
 }
 
 static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
 {
-	struct sch_gpio *sch = to_sch_gpio(gc);
-	int res;
-	unsigned short offset, bit;
-
-	offset = sch_gpio_offset(sch, gpio_num, GLV);
-	bit = sch_gpio_bit(sch, gpio_num);
-
-	res = !!(inb(sch->iobase + offset) & (1 << bit));
-
-	return res;
+	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_vals;
-	unsigned short offset, bit;
 
 	spin_lock(&sch->lock);
-
-	offset = sch_gpio_offset(sch, gpio_num, GLV);
-	bit = sch_gpio_bit(sch, gpio_num);
-
-	curr_vals = inb(sch->iobase + offset);
-
-	if (val)
-		outb(curr_vals | (1 << bit), sch->iobase + offset);
-	else
-		outb((curr_vals & ~(1 << bit)), sch->iobase + offset);
-
+	sch_gpio_reg_set(gc, gpio_num, GLV, val);
 	spin_unlock(&sch->lock);
 }
 
@@ -139,18 +123,9 @@ 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_reg_set(sch, gpio_num, GIO, 0);
 	spin_unlock(&sch->lock);
 
 	/*
@@ -209,13 +184,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_reg_set(sch, 8, GEN, 1);
+		sch_gpio_reg_set(sch, 9, GEN, 1);
 		/*
 		 * SUS_GPIO[2:0] enabled by default
 		 * Enable SUS_GPIO3 resume powered gpio explicitly
 		 */
-		sch_gpio_enable(sch, 13);
+		sch_gpio_reg_set(sch, 13, GEN, 1);
 		break;
 
 	case PCI_DEVICE_ID_INTEL_ITC_LPC:
-- 
1.7.9.5

[PATCHv4 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>
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 414d055..24c4f83 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -394,25 +394,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 7967f14..1a465bb 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -205,6 +205,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

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

[Chang Rebecca Swee Fun]

ntel 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 |  231 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 224 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 1a465bb..498746c 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;
+	int irq_base;
+	bool irq_support;
 };
 
 #define to_sch_gpio(c)	container_of(c, struct sch_gpio, chip)
@@ -98,10 +111,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_reg_set(sch, gpio_num, GIO, 1);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 	return 0;
 }
 
@@ -113,20 +127,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_reg_set(sch, gpio_num, GIO, 0);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 
 	/*
 	 * according to the datasheet, writing to the level register has no
@@ -141,6 +157,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,
@@ -148,12 +171,155 @@ 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_reg_set(sch, gpio_num, GGPE, 1);
+}
+
+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_reg_set(sch, gpio_num, GGPE, 0);
+}
+
+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;
+
+	gpio_num = d->irq - sch->irq_base;
+
+	spin_lock_irqsave(&sch->lock, flags);
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		sch_gpio_reg_set(sch, gpio_num, GTPE, 1);
+		sch_gpio_reg_set(sch, gpio_num, GTNE, 0);
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		sch_gpio_reg_set(sch, gpio_num, GTNE, 1);
+		sch_gpio_reg_set(sch, gpio_num, GTPE, 0);
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		sch_gpio_reg_set(sch, gpio_num, GTPE, 1);
+		sch_gpio_reg_set(sch, gpio_num, GTNE, 1);
+		break;
+
+	case IRQ_TYPE_NONE:
+		sch_gpio_reg_set(sch, gpio_num, GTPE, 0);
+		sch_gpio_reg_set(sch, gpio_num, GTNE, 0);
+		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_reg_set(sch, gpio_num, GTPE, 0);
+		sch_gpio_reg_set(sch, gpio_num, GTNE, 0);
+		sch_gpio_reg_set(sch, gpio_num, GGPE, 0);
+		sch_gpio_reg_set(sch, gpio_num, GSMI, 0);
+
+		if (gpio_num >= 2)
+			sch_gpio_reg_set(sch, gpio_num, RGNMIEN, 0);
+		else
+			sch_gpio_reg_set(sch, gpio_num, CGNMIEN, 0);
+
+		/* 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;
+	int err;
 
 	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
 	if (!sch)
@@ -167,6 +333,15 @@ static int sch_gpio_probe(struct platform_device *pdev)
 				 pdev->name))
 		return -EBUSY;
 
+	sch->irq = platform_get_irq(pdev, 0);
+	if (sch->irq >= 0) {
+		sch->irq_support = true;
+	} else {
+		dev_warn(&pdev->dev,
+			 "failed to obtain irq number for device\n");
+		sch->irq_support = false;
+	}
+
 	spin_lock_init(&sch->lock);
 	sch->iobase = res->start;
 	sch->chip = sch_gpio_chip;
@@ -215,15 +390,57 @@ static int sch_gpio_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	gpiochip_add(&sch->chip);
+
+	if (sch->irq_support) {
+		sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
+						NUMA_NO_NODE);
+		if (sch->irq_base < 0) {
+			dev_err(&pdev->dev,
+				"failed to allocate GPIO IRQ descs\n");
+			goto err_sch_intr_chip;
+		}
+
+		/* disable interrupts */
+		sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
+
+		err = request_irq(sch->irq, 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:
+	gpiochip_remove(&sch->chip);
+
+	return err;
 }
 
 static int sch_gpio_remove(struct platform_device *pdev)
 {
 	struct sch_gpio *sch = platform_get_drvdata(pdev);
 
+	if (sch->irq_support) {
+		sch_gpio_irqs_deinit(sch, sch->chip.ngpio);
+
+		if (sch->irq >= 0)
+			free_irq(sch->irq, sch);
+
+		irq_free_descs(sch->irq_base, sch->chip.ngpio);
+	}
+
 	gpiochip_remove(&sch->chip);
 	return 0;
 }
-- 
1.7.9.5

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

[Andy Shevchenko]

On Wed, Nov 26, 2014 at 6:48 AM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:
> ntel 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.

Minor comments.

[]

>  static int sch_gpio_probe(struct platform_device *pdev)
>  {
>         struct sch_gpio *sch;
>         struct resource *res;
> +       int err;
>
>         sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
>         if (!sch)
> @@ -167,6 +333,15 @@ static int sch_gpio_probe(struct platform_device *pdev)
>                                  pdev->name))
>                 return -EBUSY;
>
> +       sch->irq = platform_get_irq(pdev, 0);
> +       if (sch->irq >= 0) {
> +               sch->irq_support = true;

Do we need irq_support at all? Can we substitute it by sch->irq >= 0 or … < 0?

> +       } else {
> +               dev_warn(&pdev->dev,
> +                        "failed to obtain irq number for device\n");
> +               sch->irq_support = false;
> +       }
> +
>         spin_lock_init(&sch->lock);
>         sch->iobase = res->start;
>         sch->chip = sch_gpio_chip;
> @@ -215,15 +390,57 @@ static int sch_gpio_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> +       gpiochip_add(&sch->chip);
> +
> +       if (sch->irq_support) {
> +               sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
> +                                               NUMA_NO_NODE);
> +               if (sch->irq_base < 0) {
> +                       dev_err(&pdev->dev,
> +                               "failed to allocate GPIO IRQ descs\n");
> +                       goto err_sch_intr_chip;
> +               }
> +
> +               /* disable interrupts */
> +               sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
> +
> +               err = request_irq(sch->irq, 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:
> +       gpiochip_remove(&sch->chip);
> +
> +       return err;
>  }
>
>  static int sch_gpio_remove(struct platform_device *pdev)
>  {
>         struct sch_gpio *sch = platform_get_drvdata(pdev);
>
> +       if (sch->irq_support) {
> +               sch_gpio_irqs_deinit(sch, sch->chip.ngpio);
> +
> +               if (sch->irq >= 0)

Is it possible to have irq_support = true and sch->irq < 0?

> +                       free_irq(sch->irq, sch);
> +
> +               irq_free_descs(sch->irq_base, sch->chip.ngpio);
> +       }
> +
>         gpiochip_remove(&sch->chip);
>         return 0;
>  }

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andy Shevchenko]

On Wed, Nov 26, 2014 at 6:48 AM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:
> ntel 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.

Minor comments.

[]

>  static int sch_gpio_probe(struct platform_device *pdev)
>  {
>         struct sch_gpio *sch;
>         struct resource *res;
> +       int err;
>
>         sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
>         if (!sch)
> @@ -167,6 +333,15 @@ static int sch_gpio_probe(struct platform_device *pdev)
>                                  pdev->name))
>                 return -EBUSY;
>
> +       sch->irq = platform_get_irq(pdev, 0);
> +       if (sch->irq >= 0) {
> +               sch->irq_support = true;

Do we need irq_support at all? Can we substitute it by sch->irq >= 0 or … < 0?

> +       } else {
> +               dev_warn(&pdev->dev,
> +                        "failed to obtain irq number for device\n");
> +               sch->irq_support = false;
> +       }
> +
>         spin_lock_init(&sch->lock);
>         sch->iobase = res->start;
>         sch->chip = sch_gpio_chip;
> @@ -215,15 +390,57 @@ static int sch_gpio_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> +       gpiochip_add(&sch->chip);
> +
> +       if (sch->irq_support) {
> +               sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
> +                                               NUMA_NO_NODE);
> +               if (sch->irq_base < 0) {
> +                       dev_err(&pdev->dev,
> +                               "failed to allocate GPIO IRQ descs\n");
> +                       goto err_sch_intr_chip;
> +               }
> +
> +               /* disable interrupts */
> +               sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
> +
> +               err = request_irq(sch->irq, 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:
> +       gpiochip_remove(&sch->chip);
> +
> +       return err;
>  }
>
>  static int sch_gpio_remove(struct platform_device *pdev)
>  {
>         struct sch_gpio *sch = platform_get_drvdata(pdev);
>
> +       if (sch->irq_support) {
> +               sch_gpio_irqs_deinit(sch, sch->chip.ngpio);
> +
> +               if (sch->irq >= 0)

Is it possible to have irq_support = true and sch->irq < 0?

> +                       free_irq(sch->irq, sch);
> +
> +               irq_free_descs(sch->irq_base, sch->chip.ngpio);
> +       }
> +
>         gpiochip_remove(&sch->chip);
>         return 0;
>  }

-- 
With Best Regards,
Andy Shevchenko

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

[Mika Westerberg]

On Wed, Nov 26, 2014 at 12:47:58PM +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 |   81 ++++++++++++++++-------------------------------
>  1 file changed, 28 insertions(+), 53 deletions(-)

Looks good to me,

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

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

[Mika Westerberg]

On Wed, Nov 26, 2014 at 12:48:00PM +0800, Chang Rebecca Swee Fun wrote:
> ntel Quark X1000 GPIO controller supports interrupt handling for

Typo it should say Intel.

> 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 |  231 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 224 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 1a465bb..498746c 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;
> +	int irq_base;
> +	bool irq_support;
>  };
>  
>  #define to_sch_gpio(c)	container_of(c, struct sch_gpio, chip)
> @@ -98,10 +111,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_reg_set(sch, gpio_num, GIO, 1);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  	return 0;
>  }
>  
> @@ -113,20 +127,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_reg_set(sch, gpio_num, GIO, 0);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  
>  	/*
>  	 * according to the datasheet, writing to the level register has no
> @@ -141,6 +157,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,
> @@ -148,12 +171,155 @@ 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_reg_set(sch, gpio_num, GGPE, 1);

Don't you need to take the lock here before calling sch_gpio_reg_set()?

> +}
> +
> +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_reg_set(sch, gpio_num, GGPE, 0);

Ditto

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

Ditto

> +}
> +
> +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;
> +
> +	gpio_num = d->irq - sch->irq_base;
> +
> +	spin_lock_irqsave(&sch->lock, flags);
> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		sch_gpio_reg_set(sch, gpio_num, GTPE, 1);
> +		sch_gpio_reg_set(sch, gpio_num, GTNE, 0);
> +		break;
> +
> +	case IRQ_TYPE_EDGE_FALLING:
> +		sch_gpio_reg_set(sch, gpio_num, GTNE, 1);
> +		sch_gpio_reg_set(sch, gpio_num, GTPE, 0);
> +		break;
> +
> +	case IRQ_TYPE_EDGE_BOTH:
> +		sch_gpio_reg_set(sch, gpio_num, GTPE, 1);
> +		sch_gpio_reg_set(sch, gpio_num, GTNE, 1);
> +		break;
> +
> +	case IRQ_TYPE_NONE:
> +		sch_gpio_reg_set(sch, gpio_num, GTPE, 0);
> +		sch_gpio_reg_set(sch, gpio_num, GTNE, 0);
> +		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_reg_set(sch, gpio_num, GTPE, 0);
> +		sch_gpio_reg_set(sch, gpio_num, GTNE, 0);
> +		sch_gpio_reg_set(sch, gpio_num, GGPE, 0);
> +		sch_gpio_reg_set(sch, gpio_num, GSMI, 0);
> +
> +		if (gpio_num >= 2)
> +			sch_gpio_reg_set(sch, gpio_num, RGNMIEN, 0);
> +		else
> +			sch_gpio_reg_set(sch, gpio_num, CGNMIEN, 0);
> +
> +		/* 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;
> +	int err;
>  
>  	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
>  	if (!sch)
> @@ -167,6 +333,15 @@ static int sch_gpio_probe(struct platform_device *pdev)
>  				 pdev->name))
>  		return -EBUSY;
>  
> +	sch->irq = platform_get_irq(pdev, 0);
> +	if (sch->irq >= 0) {
> +		sch->irq_support = true;
> +	} else {
> +		dev_warn(&pdev->dev,
> +			 "failed to obtain irq number for device\n");

Why dev_warn()?

If I use this driver on Minnowboard which does not support interrupts,
this kind of warning makes me think that there is something wrong even
if it is actually OK.

> +		sch->irq_support = false;
> +	}
> +
>  	spin_lock_init(&sch->lock);
>  	sch->iobase = res->start;
>  	sch->chip = sch_gpio_chip;
> @@ -215,15 +390,57 @@ static int sch_gpio_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	gpiochip_add(&sch->chip);
> +
> +	if (sch->irq_support) {
> +		sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio,
> +						NUMA_NO_NODE);
> +		if (sch->irq_base < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to allocate GPIO IRQ descs\n");
> +			goto err_sch_intr_chip;
> +		}
> +
> +		/* disable interrupts */
> +		sch_gpio_irq_disable_all(sch, sch->chip.ngpio);
> +
> +		err = request_irq(sch->irq, 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:
> +	gpiochip_remove(&sch->chip);
> +
> +	return err;
>  }
>  
>  static int sch_gpio_remove(struct platform_device *pdev)
>  {
>  	struct sch_gpio *sch = platform_get_drvdata(pdev);
>  
> +	if (sch->irq_support) {
> +		sch_gpio_irqs_deinit(sch, sch->chip.ngpio);
> +
> +		if (sch->irq >= 0)
> +			free_irq(sch->irq, sch);
> +
> +		irq_free_descs(sch->irq_base, sch->chip.ngpio);
> +	}
> +
>  	gpiochip_remove(&sch->chip);
>  	return 0;
>  }
> -- 
> 1.7.9.5

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

[Linus Walleij]

2014-11-26 5:47 GMT+01:00 Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com>:

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

Patch already applied. If you changed anything, send incremental
patches on top of my devel branch or linux-next or something...

Yours,
Linus Walleij

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

[Linus Walleij]

On Fri, Nov 28, 2014 at 2:54 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2014-11-26 5:47 GMT+01:00 Chang Rebecca Swee Fun
> <rebecca.swee.fun.chang@intel.com>:
>
>> 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>
>
> Patch already applied. If you changed anything, send incremental
> patches on top of my devel branch or linux-next or something...

Bah it was *NOT* already applied. But it doesn't apply so please rebase
the patch set to my "devel" branch in the GPIO tree and resend.

Yours,
Linus Walleij

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

[Linus Walleij]

On Wed, Nov 26, 2014 at 5:48 AM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:

> ntel 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 is just adding handling of cascading interrupts from a
GPIO chip as far as I can tell. We don't do that with local
per-driver hacks anymore if there is no special reason, we have
helpers on gpiolib to handle this.

Make your Kconfig select GPIOLIB_IRQCHIP and take it from
there, look at how other drivers using GPIOLIB_IRQCHIP are
done. Read the documentation for chained irqchips in
Documentation/gpio/driver.txt

Yours,
Linus Walleij

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

[Chang, Rebecca Swee Fun]

Hi Linus,
Noted, while I continue to work on this IRQ feature, can I resend patch 1 and patch 2 (rebase to devel branch) in v5 thread so that you can pull them in first? 

Regards,
Rebecca

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 28 November, 2014 10:03 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linux Kernel Mailing List; GPIO Subsystem Mailing List; Westerberg, Mika;
> Denis Turischev; Alexandre Courbot
> Subject: Re: [PATCHv4 3/3] gpio: sch: Enable IRQ support for Quark X1000
> 
> On Wed, Nov 26, 2014 at 5:48 AM, Chang Rebecca Swee Fun
> <rebecca.swee.fun.chang@intel.com> wrote:
> 
> > ntel 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 is just adding handling of cascading interrupts from a GPIO chip as far as I
> can tell. We don't do that with local per-driver hacks anymore if there is no
> special reason, we have helpers on gpiolib to handle this.
> 
> Make your Kconfig select GPIOLIB_IRQCHIP and take it from there, look at how
> other drivers using GPIOLIB_IRQCHIP are done. Read the documentation for
> chained irqchips in Documentation/gpio/driver.txt
> 
> Yours,
> Linus Walleij

RE: [PATCHv4 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: 1688 bytes --]

Hi Linus,
Noted, while I continue to work on this IRQ feature, can I resend patch 1 and patch 2 (rebase to devel branch) in v5 thread so that you can pull them in first? 

Regards,
Rebecca

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 28 November, 2014 10:03 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linux Kernel Mailing List; GPIO Subsystem Mailing List; Westerberg, Mika;
> Denis Turischev; Alexandre Courbot
> Subject: Re: [PATCHv4 3/3] gpio: sch: Enable IRQ support for Quark X1000
> 
> On Wed, Nov 26, 2014 at 5:48 AM, Chang Rebecca Swee Fun
> <rebecca.swee.fun.chang@intel.com> wrote:
> 
> > ntel 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 is just adding handling of cascading interrupts from a GPIO chip as far as I
> can tell. We don't do that with local per-driver hacks anymore if there is no
> special reason, we have helpers on gpiolib to handle this.
> 
> Make your Kconfig select GPIOLIB_IRQCHIP and take it from there, look at how
> other drivers using GPIOLIB_IRQCHIP are done. Read the documentation for
> chained irqchips in Documentation/gpio/driver.txt
> 
> Yours,
> Linus Walleij
ÿôèº{.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: [PATCHv4 1/3] gpio: sch: Consolidate similar algorithms

[Alexandre Courbot]

2014-11-26 13:47 GMT+09:00 Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com>:
> 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 |   81 ++++++++++++++++-------------------------------
>  1 file changed, 28 insertions(+), 53 deletions(-)

Your v3 had this:

 drivers/gpio/gpio-sch.c |   95 ++++++++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 42 deletions(-)

Just looking at these two lines we can already tell this v4 is going
to be *much* better. :)

>
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 99720c8..7967f14 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -63,75 +63,59 @@ 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 int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
>  {
> +       struct sch_gpio *sch = to_sch_gpio(gc);
>         unsigned short offset, bit;
> -       u8 enable;
> -
> -       spin_lock(&sch->lock);
> +       u8 curr_dirs;

This name sounds weird in this function. It has been copied from the
direction setting functions, but this function is much more generic.
So maybe come with a more generic name as well?

>
> -       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);
> +       curr_dirs = !!(inb(sch->iobase + offset) & BIT(bit));
>
> -       spin_unlock(&sch->lock);
> +       return curr_dirs;
>  }
>
> -static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned  gpio_num)
> +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_dirs;
>         unsigned short offset, bit;
> +       u8 curr_dirs;

Same here.

Meh, I guess that's my only complain... This really makes the driver
easier to understand, so after fixing this minor issue and rebasing on
Linus' devel branch, feel free to add my

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

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

[Alexandre Courbot]

On Wed, Nov 26, 2014 at 1:47 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>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

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

[Alexandre Courbot]

On Wed, Dec 3, 2014 at 11:26 AM, Chang, Rebecca Swee Fun
<rebecca.swee.fun.chang@intel.com> wrote:
> Hi Linus,
> Noted, while I continue to work on this IRQ feature, can I resend patch 1 and patch 2 (rebase to devel branch) in v5 thread so that you can pull them in first?

Yes, please do so. The first two patches are independant from this
feature and it will allow everyone to get them off their head.