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
Consolidating similar algorithms into common functions to make
GPIO SCH simpler and manageable.
Signed-off-by: Chang Rebecca Swee Fun <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
---
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
On Wed, Nov 26, 2014 at 6:48 AM, Chang Rebecca Swee Fun
<[email protected]> 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
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 <[email protected]>
> ---
> drivers/gpio/gpio-sch.c | 81 ++++++++++++++++-------------------------------
> 1 file changed, 28 insertions(+), 53 deletions(-)
Looks good to me,
Reviewed-by: Mika Westerberg <[email protected]>
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 <[email protected]>
> ---
> 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
2014-11-26 5:47 GMT+01:00 Chang Rebecca Swee Fun
<[email protected]>:
> Consolidating similar algorithms into common functions to make
> GPIO SCH simpler and manageable.
>
> Signed-off-by: Chang Rebecca Swee Fun <[email protected]>
Patch already applied. If you changed anything, send incremental
patches on top of my devel branch or linux-next or something...
Yours,
Linus Walleij
On Fri, Nov 28, 2014 at 2:54 PM, Linus Walleij <[email protected]> wrote:
> 2014-11-26 5:47 GMT+01:00 Chang Rebecca Swee Fun
> <[email protected]>:
>
>> Consolidating similar algorithms into common functions to make
>> GPIO SCH simpler and manageable.
>>
>> Signed-off-by: Chang Rebecca Swee Fun <[email protected]>
>
> 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
On Wed, Nov 26, 2014 at 5:48 AM, Chang Rebecca Swee Fun
<[email protected]> 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 <[email protected]>
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
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:[email protected]]
> 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
> <[email protected]> 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
> > <[email protected]>
>
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
2014-11-26 13:47 GMT+09:00 Chang Rebecca Swee Fun
<[email protected]>:
> Consolidating similar algorithms into common functions to make
> GPIO SCH simpler and manageable.
>
> Signed-off-by: Chang Rebecca Swee Fun <[email protected]>
> ---
> 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 <[email protected]>
On Wed, Nov 26, 2014 at 1:47 PM, Chang Rebecca Swee Fun
<[email protected]> 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 <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
Reviewed-by: Alexandre Courbot <[email protected]>
On Wed, Dec 3, 2014 at 11:26 AM, Chang, Rebecca Swee Fun
<[email protected]> 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.