[PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[Baruch Siach]

This is a driver for the ARM PrimeCell PL061 GPIO AMBA peripheral. The driver
is implemented using the gpiolib framework.

This driver also includes support for the use of the PL061 as an interrupt
controller (secondary).

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
Changes in v4:
	- Depend on CONFIG_ARM_AMBA
	- Do not request the gpio for IRQ automatically, just warn if it's not 
	  requested
	- Remove SZ_4K
	- Iterate through a linked list find the source of interrupt when 
	  multiple PL061s are connected to the same IRQ trigger
	- Move hardware register defines into the .c file

Changes in v3:
	- amba driver instead of a platform driver
	- Move include/linux/gpio/pl061.h => include/linux/amba/pl061.h

Changes in v2:
	- Address Andrew Morton's comments
	- Pass checkpatch.pl
	- Add changelog comment

 drivers/gpio/Kconfig       |    6 +
 drivers/gpio/Makefile      |    1 +
 drivers/gpio/pl061.c       |  342 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/amba/pl061.h |   17 +++
 4 files changed, 366 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/pl061.c
 create mode 100644 include/linux/amba/pl061.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index edb0253..585376f 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -67,6 +67,12 @@ config GPIO_SYSFS
 
 comment "Memory mapped GPIO expanders:"
 
+config GPIO_PL061
+	bool "PrimeCell PL061 GPIO support"
+	depends on ARM_AMBA
+	help
+	  Say yes here to support the PrimeCell PL061 GPIO device
+
 config GPIO_XILINX
 	bool "Xilinx GPIO support"
 	depends on PPC_OF
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 49ac64e..ef90203 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_GPIO_MAX732X)	+= max732x.o
 obj-$(CONFIG_GPIO_MCP23S08)	+= mcp23s08.o
 obj-$(CONFIG_GPIO_PCA953X)	+= pca953x.o
 obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o
+obj-$(CONFIG_GPIO_PL061)	+= pl061.o
 obj-$(CONFIG_GPIO_TWL4030)	+= twl4030-gpio.o
 obj-$(CONFIG_GPIO_XILINX)	+= xilinx_gpio.o
 obj-$(CONFIG_GPIO_BT8XX)	+= bt8xxgpio.o
diff --git a/drivers/gpio/pl061.c b/drivers/gpio/pl061.c
new file mode 100644
index 0000000..c48e82e
--- /dev/null
+++ b/drivers/gpio/pl061.c
@@ -0,0 +1,342 @@
+/*
+ *  linux/drivers/gpio/pl061.c
+ *
+ *  Copyright (C) 2008, 2009 Provigent Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Driver for the ARM PrimeCell(tm) General Purpose Input/Output (PL061)
+ *
+ * Data sheet: ARM DDI 0190B, September 2000
+ */
+#include <linux/spinlock.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/list.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/irq.h>
+#include <linux/bitops.h>
+#include <linux/workqueue.h>
+#include <linux/gpio.h>
+#include <linux/device.h>
+#include <linux/amba/bus.h>
+#include <linux/amba/pl061.h>
+
+#define GPIODIR 0x400
+#define GPIOIS  0x404
+#define GPIOIBE 0x408
+#define GPIOIEV 0x40C
+#define GPIOIE  0x410
+#define GPIORIS 0x414
+#define GPIOMIS 0x418
+#define GPIOIC  0x41C
+
+#define PL061_GPIO_NR	8
+
+#define PL061_REG_SIZE	(4*1024)
+
+struct pl061_gpio {
+	struct list_head	list;
+
+	/* Each of the two spinlocks protects a different set of hardware
+	 * regiters and data structurs. This decouples the code of the IRQ from
+	 * the GPIO code. This also makes the case of a GPIO routine call from
+	 * the IRQ code simpler.
+	 */
+	spinlock_t		lock;		/* GPIO registers */
+	spinlock_t		irq_lock;	/* IRQ registers */
+
+	void __iomem		*base;
+	struct gpio_chip	gc;
+};
+
+static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
+	unsigned long flags;
+	unsigned char gpiodir;
+
+	if (offset >= gc->ngpio)
+		return -EINVAL;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	gpiodir = readb(chip->base + GPIODIR);
+	gpiodir &= ~(1 << offset);
+	writeb(gpiodir, chip->base + GPIODIR);
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return 0;
+}
+
+static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
+		int value)
+{
+	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
+	unsigned long flags;
+	unsigned char gpiodir;
+
+	if (offset >= gc->ngpio)
+		return -EINVAL;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
+	gpiodir = readb(chip->base + GPIODIR);
+	gpiodir |= 1 << offset;
+	writeb(gpiodir, chip->base + GPIODIR);
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return 0;
+}
+
+static int pl061_get_value(struct gpio_chip *gc, unsigned offset)
+{
+	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
+
+	return !!readb(chip->base + (1 << (offset + 2)));
+}
+
+static void pl061_set_value(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
+
+	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
+}
+
+/*
+ * PL061 GPIO IRQ
+ */
+static void pl061_irq_disable(unsigned irq)
+{
+	struct pl061_gpio *chip = get_irq_chip_data(irq);
+	int offset = irq_to_gpio(irq) - chip->gc.base;
+	unsigned long flags;
+	u8 gpioie;
+
+	spin_lock_irqsave(&chip->irq_lock, flags);
+	gpioie = readb(chip->base + GPIOIE);
+	gpioie &= ~(1 << offset);
+	writeb(gpioie, chip->base + GPIOIE);
+	spin_unlock_irqrestore(&chip->irq_lock, flags);
+}
+
+static void pl061_irq_enable(unsigned irq)
+{
+	struct pl061_gpio *chip = get_irq_chip_data(irq);
+	int offset = irq_to_gpio(irq) - chip->gc.base;
+	unsigned long flags;
+	u8 gpioie;
+
+	spin_lock_irqsave(&chip->irq_lock, flags);
+	gpioie = readb(chip->base + GPIOIE);
+	gpioie |= 1 << offset;
+	writeb(gpioie, chip->base + GPIOIE);
+	spin_unlock_irqrestore(&chip->irq_lock, flags);
+}
+
+static unsigned int pl061_irq_startup(unsigned irq)
+{
+	if (gpio_request(irq_to_gpio(irq), "IRQ") == 0)
+		pr_warning("%s: warning: GPIO%d has not been requested\n",
+				__func__, irq_to_gpio(irq));
+
+	pl061_irq_enable(irq);
+
+	return 0;
+}
+
+static int pl061_irq_type(unsigned irq, unsigned trigger)
+{
+	struct pl061_gpio *chip = get_irq_chip_data(irq);
+	int offset = irq_to_gpio(irq) - chip->gc.base;
+	unsigned long flags;
+	u8 gpiois, gpioibe, gpioiev;
+
+	if (irq_to_gpio(irq) < 0)
+		return -EINVAL;
+
+	spin_lock_irqsave(&chip->irq_lock, flags);
+
+	gpioiev = readb(chip->base + GPIOIEV);
+
+	gpiois = readb(chip->base + GPIOIS);
+	if (trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
+		gpiois |= 1 << offset;
+		if (trigger & IRQ_TYPE_LEVEL_HIGH)
+			gpioiev |= 1 << offset;
+		else
+			gpioiev &= ~(1 << offset);
+	} else
+		gpiois &= ~(1 << offset);
+	writeb(gpiois, chip->base + GPIOIS);
+
+	gpioibe = readb(chip->base + GPIOIBE);
+	if ((trigger & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH)
+		gpioibe |= 1 << offset;
+	else {
+		gpioibe &= ~(1 << offset);
+		if (trigger & IRQ_TYPE_EDGE_RISING)
+			gpioiev |= 1 << offset;
+		else
+			gpioiev &= ~(1 << offset);
+	}
+	writeb(gpioibe, chip->base + GPIOIBE);
+
+	writeb(gpioiev, chip->base + GPIOIEV);
+
+	spin_unlock_irqrestore(&chip->irq_lock, flags);
+
+	return 0;
+}
+
+static struct irq_chip pl061_irqchip = {
+	.name		= "GPIO",
+	.startup	= pl061_irq_startup,
+	.enable		= pl061_irq_enable,
+	.disable	= pl061_irq_disable,
+	.set_type	= pl061_irq_type,
+};
+
+static void pl061_irq_handler(unsigned irq, struct irq_desc *desc)
+{
+	struct list_head *chip_list = get_irq_chip_data(irq);
+	struct list_head *ptr;
+	struct pl061_gpio *chip;
+
+	desc->chip->ack(irq);
+	list_for_each(ptr, chip_list) {
+		unsigned long pending;
+		int gpio;
+
+		chip = list_entry(ptr, struct pl061_gpio, list);
+		pending = readb(chip->base + GPIOMIS);
+		writeb(pending, chip->base + GPIOIC);
+
+		if (pending == 0)
+			continue;
+
+		for_each_bit(gpio, &pending, PL061_GPIO_NR)
+			generic_handle_irq(gpio_to_irq(gpio));
+	}
+	desc->chip->unmask(irq);
+}
+
+static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
+{
+	struct pl061_platform_data *pdata;
+	struct pl061_gpio *chip;
+	struct list_head *chip_list;
+	int ret, irq, i;
+
+	pdata = dev->dev.platform_data;
+	if (pdata == NULL)
+		return -ENODEV;
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL)
+		return -ENOMEM;
+
+	if (!request_mem_region(dev->res.start, PL061_REG_SIZE, "pl061")) {
+		ret = -EBUSY;
+		goto free_mem;
+	}
+
+	chip->base = ioremap(dev->res.start, PL061_REG_SIZE);
+	if (chip->base == NULL) {
+		ret = -ENOMEM;
+		goto release_region;
+	}
+
+	spin_lock_init(&chip->lock);
+	spin_lock_init(&chip->irq_lock);
+	INIT_LIST_HEAD(&chip->list);
+
+	chip->gc.direction_input = pl061_direction_input;
+	chip->gc.direction_output = pl061_direction_output;
+	chip->gc.get = pl061_get_value;
+	chip->gc.set = pl061_set_value;
+	chip->gc.base = pdata->gpio_base;
+	chip->gc.ngpio = PL061_GPIO_NR;
+	chip->gc.label = dev_name(&dev->dev);
+	chip->gc.dev = &dev->dev;
+	chip->gc.owner = THIS_MODULE;
+
+	ret = gpiochip_add(&chip->gc);
+	if (ret)
+		goto iounmap;
+
+	/*
+	 * irq_chip support
+	 */
+	writeb(0, chip->base + GPIOIE); /* disable irqs */
+	irq = dev->irq[0];
+	if (irq < 0) {
+		ret = -ENODEV;
+		goto iounmap;
+	}
+	set_irq_chained_handler(irq, pl061_irq_handler);
+	if (!pdata->is_irq_initialized || !pdata->is_irq_initialized(irq)) {
+		chip_list = kmalloc(sizeof(*chip_list), GFP_KERNEL);
+		if (chip_list == NULL) {
+			ret = -ENOMEM;
+			goto iounmap;
+		}
+		INIT_LIST_HEAD(chip_list);
+		set_irq_chip_data(irq, chip_list);
+	} else
+		chip_list = get_irq_chip_data(irq);
+	list_add(&chip->list, chip_list);
+
+	for (i = 0; i < PL061_GPIO_NR; i++) {
+		if (pdata->directions & (1 << i))
+			pl061_direction_output(&chip->gc, i,
+					pdata->values & (1 << i));
+		else
+			pl061_direction_input(&chip->gc, i);
+
+		set_irq_chip(gpio_to_irq(i+pdata->gpio_base), &pl061_irqchip);
+		set_irq_handler(gpio_to_irq(i+pdata->gpio_base),
+				handle_simple_irq);
+		set_irq_flags(gpio_to_irq(i+pdata->gpio_base), IRQF_VALID);
+		set_irq_chip_data(gpio_to_irq(i+pdata->gpio_base), chip);
+	}
+
+	return 0;
+
+iounmap:
+	iounmap(chip->base);
+release_region:
+	release_mem_region(dev->res.start, PL061_REG_SIZE);
+free_mem:
+	kfree(chip);
+
+	return ret;
+}
+
+static struct amba_id pl061_ids[] __initdata = {
+	{
+		.id	= 0x00041061,
+		.mask	= 0x000fffff,
+	},
+	{ 0, 0 },
+};
+
+static struct amba_driver pl061_gpio_driver = {
+	.drv = {
+		.name	= "pl061_gpio",
+	},
+	.id_table	= pl061_ids,
+	.probe		= pl061_probe,
+};
+
+static int __init pl061_gpio_init(void)
+{
+	return amba_driver_register(&pl061_gpio_driver);
+}
+subsys_initcall(pl061_gpio_init);
+
+MODULE_AUTHOR("Baruch Siach <baruch@tkos.co.il>");
+MODULE_DESCRIPTION("PL061 GPIO driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/amba/pl061.h b/include/linux/amba/pl061.h
new file mode 100644
index 0000000..90de39e
--- /dev/null
+++ b/include/linux/amba/pl061.h
@@ -0,0 +1,17 @@
+/* platform data for the PL061 GPIO driver */
+
+struct pl061_platform_data {
+	/* number of the first GPIO */
+	unsigned	gpio_base;
+
+	u8		directions;	/* startup directions, 1: out, 0: in */
+	u8		values;		/* startup values */
+
+	/* This callback may be used when you have more than one PL061
+	 * connected to the same IRQ trigger. This callback must return 0 on
+	 * the first time it is called for each irq, and 1 on any other call.
+	 * In case you are not unfortunate enough to have this setup, this
+	 * pointer must be set to NULL.
+	 */
+	int		(*is_irq_initialized)(int irq);
+};
-- 
1.6.3.1

Re: [PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[Linus Walleij]

OK sorry for coming in so late, this is really not so important but anyway:

2009/6/7 Baruch Siach <baruch@tkos.co.il>:
> This is a driver for the ARM PrimeCell PL061 GPIO AMBA peripheral. The driver
> is implemented using the gpiolib framework.
>
> This driver also includes support for the use of the PL061 as an interrupt
> controller (secondary).
>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> Changes in v4:
>        - Depend on CONFIG_ARM_AMBA
>        - Do not request the gpio for IRQ automatically, just warn if it's not
>          requested
>        - Remove SZ_4K

Yes but defined a new one 4*1024 instead, but I'll show, hang on...

>        - Iterate through a linked list find the source of interrupt when
>          multiple PL061s are connected to the same IRQ trigger
>        - Move hardware register defines into the .c file
>
> Changes in v3:
>        - amba driver instead of a platform driver
>        - Move include/linux/gpio/pl061.h => include/linux/amba/pl061.h
>
> Changes in v2:
>        - Address Andrew Morton's comments
>        - Pass checkpatch.pl
>        - Add changelog comment
>
>  drivers/gpio/Kconfig       |    6 +
>  drivers/gpio/Makefile      |    1 +
>  drivers/gpio/pl061.c       |  342 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/amba/pl061.h |   17 +++
>  4 files changed, 366 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/gpio/pl061.c
>  create mode 100644 include/linux/amba/pl061.h
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index edb0253..585376f 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -67,6 +67,12 @@ config GPIO_SYSFS
>
>  comment "Memory mapped GPIO expanders:"
>
> +config GPIO_PL061
> +       bool "PrimeCell PL061 GPIO support"
> +       depends on ARM_AMBA
> +       help
> +         Say yes here to support the PrimeCell PL061 GPIO device
> +
>  config GPIO_XILINX
>        bool "Xilinx GPIO support"
>        depends on PPC_OF
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 49ac64e..ef90203 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_GPIO_MAX732X)      += max732x.o
>  obj-$(CONFIG_GPIO_MCP23S08)    += mcp23s08.o
>  obj-$(CONFIG_GPIO_PCA953X)     += pca953x.o
>  obj-$(CONFIG_GPIO_PCF857X)     += pcf857x.o
> +obj-$(CONFIG_GPIO_PL061)       += pl061.o
>  obj-$(CONFIG_GPIO_TWL4030)     += twl4030-gpio.o
>  obj-$(CONFIG_GPIO_XILINX)      += xilinx_gpio.o
>  obj-$(CONFIG_GPIO_BT8XX)       += bt8xxgpio.o
> diff --git a/drivers/gpio/pl061.c b/drivers/gpio/pl061.c
> new file mode 100644
> index 0000000..c48e82e
> --- /dev/null
> +++ b/drivers/gpio/pl061.c
> @@ -0,0 +1,342 @@
> +/*
> + *  linux/drivers/gpio/pl061.c
> + *
> + *  Copyright (C) 2008, 2009 Provigent Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Driver for the ARM PrimeCell(tm) General Purpose Input/Output (PL061)
> + *
> + * Data sheet: ARM DDI 0190B, September 2000
> + */
> +#include <linux/spinlock.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/list.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/irq.h>
> +#include <linux/bitops.h>
> +#include <linux/workqueue.h>
> +#include <linux/gpio.h>
> +#include <linux/device.h>
> +#include <linux/amba/bus.h>
> +#include <linux/amba/pl061.h>
> +
> +#define GPIODIR 0x400
> +#define GPIOIS  0x404
> +#define GPIOIBE 0x408
> +#define GPIOIEV 0x40C
> +#define GPIOIE  0x410
> +#define GPIORIS 0x414
> +#define GPIOMIS 0x418
> +#define GPIOIC  0x41C
> +
> +#define PL061_GPIO_NR  8
> +
> +#define PL061_REG_SIZE (4*1024)

Then you could just
#define PL061_REG_SIZE SZ_4K
But you can remove it, look below:

> +
> +struct pl061_gpio {
> +       struct list_head        list;
> +
> +       /* Each of the two spinlocks protects a different set of hardware
> +        * regiters and data structurs. This decouples the code of the IRQ from
> +        * the GPIO code. This also makes the case of a GPIO routine call from
> +        * the IRQ code simpler.
> +        */
> +       spinlock_t              lock;           /* GPIO registers */
> +       spinlock_t              irq_lock;       /* IRQ registers */
> +
> +       void __iomem            *base;
> +       struct gpio_chip        gc;
> +};
> +
> +static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> +       unsigned long flags;
> +       unsigned char gpiodir;
> +
> +       if (offset >= gc->ngpio)
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&chip->lock, flags);
> +       gpiodir = readb(chip->base + GPIODIR);
> +       gpiodir &= ~(1 << offset);
> +       writeb(gpiodir, chip->base + GPIODIR);
> +       spin_unlock_irqrestore(&chip->lock, flags);
> +
> +       return 0;
> +}
> +
> +static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
> +               int value)
> +{
> +       struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> +       unsigned long flags;
> +       unsigned char gpiodir;
> +
> +       if (offset >= gc->ngpio)
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&chip->lock, flags);
> +       writeb(!!value << offset, chip->base + (1 << (offset + 2)));
> +       gpiodir = readb(chip->base + GPIODIR);
> +       gpiodir |= 1 << offset;
> +       writeb(gpiodir, chip->base + GPIODIR);
> +       spin_unlock_irqrestore(&chip->lock, flags);
> +
> +       return 0;
> +}
> +
> +static int pl061_get_value(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> +
> +       return !!readb(chip->base + (1 << (offset + 2)));
> +}
> +
> +static void pl061_set_value(struct gpio_chip *gc, unsigned offset, int value)
> +{
> +       struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> +
> +       writeb(!!value << offset, chip->base + (1 << (offset + 2)));
> +}
> +
> +/*
> + * PL061 GPIO IRQ
> + */
> +static void pl061_irq_disable(unsigned irq)
> +{
> +       struct pl061_gpio *chip = get_irq_chip_data(irq);
> +       int offset = irq_to_gpio(irq) - chip->gc.base;
> +       unsigned long flags;
> +       u8 gpioie;
> +
> +       spin_lock_irqsave(&chip->irq_lock, flags);
> +       gpioie = readb(chip->base + GPIOIE);
> +       gpioie &= ~(1 << offset);
> +       writeb(gpioie, chip->base + GPIOIE);
> +       spin_unlock_irqrestore(&chip->irq_lock, flags);
> +}
> +
> +static void pl061_irq_enable(unsigned irq)
> +{
> +       struct pl061_gpio *chip = get_irq_chip_data(irq);
> +       int offset = irq_to_gpio(irq) - chip->gc.base;
> +       unsigned long flags;
> +       u8 gpioie;
> +
> +       spin_lock_irqsave(&chip->irq_lock, flags);
> +       gpioie = readb(chip->base + GPIOIE);
> +       gpioie |= 1 << offset;
> +       writeb(gpioie, chip->base + GPIOIE);
> +       spin_unlock_irqrestore(&chip->irq_lock, flags);
> +}
> +
> +static unsigned int pl061_irq_startup(unsigned irq)
> +{
> +       if (gpio_request(irq_to_gpio(irq), "IRQ") == 0)
> +               pr_warning("%s: warning: GPIO%d has not been requested\n",
> +                               __func__, irq_to_gpio(irq));
> +
> +       pl061_irq_enable(irq);
> +
> +       return 0;
> +}
> +
> +static int pl061_irq_type(unsigned irq, unsigned trigger)
> +{
> +       struct pl061_gpio *chip = get_irq_chip_data(irq);
> +       int offset = irq_to_gpio(irq) - chip->gc.base;
> +       unsigned long flags;
> +       u8 gpiois, gpioibe, gpioiev;
> +
> +       if (irq_to_gpio(irq) < 0)
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&chip->irq_lock, flags);
> +
> +       gpioiev = readb(chip->base + GPIOIEV);
> +
> +       gpiois = readb(chip->base + GPIOIS);
> +       if (trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
> +               gpiois |= 1 << offset;
> +               if (trigger & IRQ_TYPE_LEVEL_HIGH)
> +                       gpioiev |= 1 << offset;
> +               else
> +                       gpioiev &= ~(1 << offset);
> +       } else
> +               gpiois &= ~(1 << offset);
> +       writeb(gpiois, chip->base + GPIOIS);
> +
> +       gpioibe = readb(chip->base + GPIOIBE);
> +       if ((trigger & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH)
> +               gpioibe |= 1 << offset;
> +       else {
> +               gpioibe &= ~(1 << offset);
> +               if (trigger & IRQ_TYPE_EDGE_RISING)
> +                       gpioiev |= 1 << offset;
> +               else
> +                       gpioiev &= ~(1 << offset);
> +       }
> +       writeb(gpioibe, chip->base + GPIOIBE);
> +
> +       writeb(gpioiev, chip->base + GPIOIEV);
> +
> +       spin_unlock_irqrestore(&chip->irq_lock, flags);
> +
> +       return 0;
> +}
> +
> +static struct irq_chip pl061_irqchip = {
> +       .name           = "GPIO",
> +       .startup        = pl061_irq_startup,
> +       .enable         = pl061_irq_enable,
> +       .disable        = pl061_irq_disable,
> +       .set_type       = pl061_irq_type,
> +};
> +
> +static void pl061_irq_handler(unsigned irq, struct irq_desc *desc)
> +{
> +       struct list_head *chip_list = get_irq_chip_data(irq);
> +       struct list_head *ptr;
> +       struct pl061_gpio *chip;
> +
> +       desc->chip->ack(irq);
> +       list_for_each(ptr, chip_list) {
> +               unsigned long pending;
> +               int gpio;
> +
> +               chip = list_entry(ptr, struct pl061_gpio, list);
> +               pending = readb(chip->base + GPIOMIS);
> +               writeb(pending, chip->base + GPIOIC);
> +
> +               if (pending == 0)
> +                       continue;
> +
> +               for_each_bit(gpio, &pending, PL061_GPIO_NR)
> +                       generic_handle_irq(gpio_to_irq(gpio));
> +       }
> +       desc->chip->unmask(irq);
> +}
> +
> +static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
> +{
> +       struct pl061_platform_data *pdata;
> +       struct pl061_gpio *chip;
> +       struct list_head *chip_list;
> +       int ret, irq, i;
> +
> +       pdata = dev->dev.platform_data;
> +       if (pdata == NULL)
> +               return -ENODEV;
> +
> +       chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +       if (chip == NULL)
> +               return -ENOMEM;
> +
> +       if (!request_mem_region(dev->res.start, PL061_REG_SIZE, "pl061")) {
> +               ret = -EBUSY;
> +               goto free_mem;
> +       }
> +
> +       chip->base = ioremap(dev->res.start, PL061_REG_SIZE);

Just do this:
chip->base = ioremap(dev->res.start, resource_size(dev->res));

Hm perhaps this should be fixed in a few more primecell drivers,
I'll have a check...

> +       if (chip->base == NULL) {
> +               ret = -ENOMEM;
> +               goto release_region;
> +       }
> +
> +       spin_lock_init(&chip->lock);
> +       spin_lock_init(&chip->irq_lock);
> +       INIT_LIST_HEAD(&chip->list);
> +
> +       chip->gc.direction_input = pl061_direction_input;
> +       chip->gc.direction_output = pl061_direction_output;
> +       chip->gc.get = pl061_get_value;
> +       chip->gc.set = pl061_set_value;
> +       chip->gc.base = pdata->gpio_base;
> +       chip->gc.ngpio = PL061_GPIO_NR;
> +       chip->gc.label = dev_name(&dev->dev);
> +       chip->gc.dev = &dev->dev;
> +       chip->gc.owner = THIS_MODULE;
> +
> +       ret = gpiochip_add(&chip->gc);
> +       if (ret)
> +               goto iounmap;
> +
> +       /*
> +        * irq_chip support
> +        */
> +       writeb(0, chip->base + GPIOIE); /* disable irqs */
> +       irq = dev->irq[0];
> +       if (irq < 0) {
> +               ret = -ENODEV;
> +               goto iounmap;
> +       }
> +       set_irq_chained_handler(irq, pl061_irq_handler);
> +       if (!pdata->is_irq_initialized || !pdata->is_irq_initialized(irq)) {
> +               chip_list = kmalloc(sizeof(*chip_list), GFP_KERNEL);
> +               if (chip_list == NULL) {
> +                       ret = -ENOMEM;
> +                       goto iounmap;
> +               }
> +               INIT_LIST_HEAD(chip_list);
> +               set_irq_chip_data(irq, chip_list);
> +       } else
> +               chip_list = get_irq_chip_data(irq);
> +       list_add(&chip->list, chip_list);
> +
> +       for (i = 0; i < PL061_GPIO_NR; i++) {
> +               if (pdata->directions & (1 << i))
> +                       pl061_direction_output(&chip->gc, i,
> +                                       pdata->values & (1 << i));
> +               else
> +                       pl061_direction_input(&chip->gc, i);
> +
> +               set_irq_chip(gpio_to_irq(i+pdata->gpio_base), &pl061_irqchip);
> +               set_irq_handler(gpio_to_irq(i+pdata->gpio_base),
> +                               handle_simple_irq);
> +               set_irq_flags(gpio_to_irq(i+pdata->gpio_base), IRQF_VALID);
> +               set_irq_chip_data(gpio_to_irq(i+pdata->gpio_base), chip);
> +       }
> +
> +       return 0;
> +
> +iounmap:
> +       iounmap(chip->base);
> +release_region:
> +       release_mem_region(dev->res.start, PL061_REG_SIZE);
> +free_mem:
> +       kfree(chip);
> +
> +       return ret;
> +}
> +
> +static struct amba_id pl061_ids[] __initdata = {
> +       {
> +               .id     = 0x00041061,
> +               .mask   = 0x000fffff,
> +       },
> +       { 0, 0 },
> +};
> +
> +static struct amba_driver pl061_gpio_driver = {
> +       .drv = {
> +               .name   = "pl061_gpio",
> +       },
> +       .id_table       = pl061_ids,
> +       .probe          = pl061_probe,
> +};
> +
> +static int __init pl061_gpio_init(void)
> +{
> +       return amba_driver_register(&pl061_gpio_driver);
> +}
> +subsys_initcall(pl061_gpio_init);
> +
> +MODULE_AUTHOR("Baruch Siach <baruch@tkos.co.il>");
> +MODULE_DESCRIPTION("PL061 GPIO driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/amba/pl061.h b/include/linux/amba/pl061.h
> new file mode 100644
> index 0000000..90de39e
> --- /dev/null
> +++ b/include/linux/amba/pl061.h
> @@ -0,0 +1,17 @@
> +/* platform data for the PL061 GPIO driver */
> +
> +struct pl061_platform_data {
> +       /* number of the first GPIO */
> +       unsigned        gpio_base;
> +
> +       u8              directions;     /* startup directions, 1: out, 0: in */
> +       u8              values;         /* startup values */
> +
> +       /* This callback may be used when you have more than one PL061
> +        * connected to the same IRQ trigger. This callback must return 0 on
> +        * the first time it is called for each irq, and 1 on any other call.
> +        * In case you are not unfortunate enough to have this setup, this
> +        * pointer must be set to NULL.
> +        */
> +       int             (*is_irq_initialized)(int irq);
> +};
> --
> 1.6.3.1
>
> --
> 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/
>

Yours,
Linus Walleij

Re: [PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[Russell King - ARM Linux]

On Sun, Jun 07, 2009 at 11:33:23PM +0200, Linus Walleij wrote:
> OK sorry for coming in so late, this is really not so important but anyway:
> 
> 2009/6/7 Baruch Siach <baruch@tkos.co.il>:
> > This is a driver for the ARM PrimeCell PL061 GPIO AMBA peripheral. The driver
> > is implemented using the gpiolib framework.
> >
> > This driver also includes support for the use of the PL061 as an interrupt
> > controller (secondary).
> >
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> > Changes in v4:
> >        - Depend on CONFIG_ARM_AMBA
> >        - Do not request the gpio for IRQ automatically, just warn if it's not
> >          requested
> >        - Remove SZ_4K
> 
> Yes but defined a new one 4*1024 instead, but I'll show, hang on...

Hint: it got changed because we don't want to use SZ_4K in drivers which
can be used externally to ARM.

Re: [PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[Linus Walleij]

2009/6/7 Russell King - ARM Linux <linux@arm.linux.org.uk>:

>> Yes but defined a new one 4*1024 instead, but I'll show, hang on...
>
> Hint: it got changed because we don't want to use SZ_4K in drivers which
> can be used externally to ARM.

OK then my most recent patch does away with some of that burden too...

Linus Walleij

Re: [PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[Linus Walleij]

2009/6/7 Linus Walleij <linus.ml.walleij@gmail.com>:

>> +       chip->base = ioremap(dev->res.start, PL061_REG_SIZE);
>
> Just do this:
> chip->base = ioremap(dev->res.start, resource_size(dev->res));

And that should read:
chip->base = ioremap(dev->res.start, resource_size(&dev->res));

Sadly my in-brain C parser is working badly at this hour.

Linus Walleij

Re: [PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[Andrew Morton]

On Sun,  7 Jun 2009 21:38:55 +0300
Baruch Siach <baruch@tkos.co.il> wrote:

> This is a driver for the ARM PrimeCell PL061 GPIO AMBA peripheral. The driver
> is implemented using the gpiolib framework.
> 
> This driver also includes support for the use of the PL061 as an interrupt
> controller (secondary).
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> Changes in v4:
> 	- Depend on CONFIG_ARM_AMBA
> 	- Do not request the gpio for IRQ automatically, just warn if it's not 
> 	  requested
> 	- Remove SZ_4K
> 	- Iterate through a linked list find the source of interrupt when 
> 	  multiple PL061s are connected to the same IRQ trigger
> 	- Move hardware register defines into the .c file

I generated an incremental diff so that we can see what you did.


>  drivers/gpio/Kconfig       |    1 
>  drivers/gpio/pl061.c       |   92 ++++++++++++++++-------------------
>  include/linux/amba/pl061.h |   19 +++----
>  3 files changed, 54 insertions(+), 58 deletions(-)
> 
> diff -puN drivers/gpio/Kconfig~gpio-driver-for-primecell-pl061-gpio-controller-v4 drivers/gpio/Kconfig
> --- a/drivers/gpio/Kconfig~gpio-driver-for-primecell-pl061-gpio-controller-v4
> +++ a/drivers/gpio/Kconfig
> @@ -69,6 +69,7 @@ comment "Memory mapped GPIO expanders:"
>  
>  config GPIO_PL061
>  	bool "PrimeCell PL061 GPIO support"
> +	depends on ARM_AMBA
>  	help
>  	  Say yes here to support the PrimeCell PL061 GPIO device
>  
> diff -puN drivers/gpio/pl061.c~gpio-driver-for-primecell-pl061-gpio-controller-v4 drivers/gpio/pl061.c
> --- a/drivers/gpio/pl061.c~gpio-driver-for-primecell-pl061-gpio-controller-v4
> +++ a/drivers/gpio/pl061.c
> @@ -14,6 +14,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/errno.h>
>  #include <linux/module.h>
> +#include <linux/list.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
>  #include <linux/irq.h>
> @@ -24,9 +25,22 @@
>  #include <linux/amba/bus.h>
>  #include <linux/amba/pl061.h>
>  
> +#define GPIODIR 0x400
> +#define GPIOIS  0x404
> +#define GPIOIBE 0x408
> +#define GPIOIEV 0x40C
> +#define GPIOIE  0x410
> +#define GPIORIS 0x414
> +#define GPIOMIS 0x418
> +#define GPIOIC  0x41C
> +
>  #define PL061_GPIO_NR	8
>  
> +#define PL061_REG_SIZE	(4*1024)
> +
>  struct pl061_gpio {
> +	struct list_head	list;

Some commentary which describes this field would be nice.  What lock
protects it, what other list_head this list is anchored to, etc

>  	/* Each of the two spinlocks protects a different set of hardware
>  	 * regiters and data structurs. This decouples the code of the IRQ from
>  	 * the GPIO code. This also makes the case of a GPIO routine call from
> @@ -37,12 +51,8 @@ struct pl061_gpio {
>  
>  	void __iomem		*base;
>  	struct gpio_chip	gc;
> -	struct work_struct	gpio_free_work;
> -	DECLARE_BITMAP(gpios_to_free, PL061_GPIO_NR);
>  };
>  
> -static u32 (*pl061_pending_irq)(int irq);
> -
>  static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
>  {
>  	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> @@ -112,16 +122,6 @@ static void pl061_irq_disable(unsigned i
>  	spin_unlock_irqrestore(&chip->irq_lock, flags);
>  }
>  
> -static void pl061_irq_shutdown(unsigned irq)
> -{
> -	struct pl061_gpio *chip = get_irq_chip_data(irq);
> -	int offset = irq_to_gpio(irq) - chip->gc.base;
> -
> -	pl061_irq_disable(irq);
> -	set_bit(offset, chip->gpios_to_free);
> -	schedule_work(&chip->gpio_free_work);
> -}
> -
>  static void pl061_irq_enable(unsigned irq)
>  {
>  	struct pl061_gpio *chip = get_irq_chip_data(irq);
> @@ -138,16 +138,10 @@ static void pl061_irq_enable(unsigned ir
>  
>  static unsigned int pl061_irq_startup(unsigned irq)
>  {
> -	int ret;
> -
> -	ret = gpio_request(irq_to_gpio(irq), "IRQ");
> -	if (ret < 0) {
> -		pr_warning("%s: warning: gpio_request(%d) returned %d\n",
> -				__func__, irq_to_gpio(irq), ret);
> -		return 0;
> -	}
> +	if (gpio_request(irq_to_gpio(irq), "IRQ") == 0)
> +		pr_warning("%s: warning: GPIO%d has not been requested\n",
> +				__func__, irq_to_gpio(irq));

This is wrong, isn't it?  gpio_request() returns 0 on success.

> -	gpio_direction_input(irq_to_gpio(irq));
>  	pl061_irq_enable(irq);
>  
>  	return 0;
> @@ -202,45 +196,38 @@ static struct irq_chip pl061_irqchip = {
>  	.startup	= pl061_irq_startup,
>  	.enable		= pl061_irq_enable,
>  	.disable	= pl061_irq_disable,
> -	.shutdown	= pl061_irq_shutdown,
>  	.set_type	= pl061_irq_type,
>  };
>  
>  static void pl061_irq_handler(unsigned irq, struct irq_desc *desc)
>  {
> +	struct list_head *chip_list = get_irq_chip_data(irq);
> +	struct list_head *ptr;
> +	struct pl061_gpio *chip;
> +
>  	desc->chip->ack(irq);
> -	while (1) {
> +	list_for_each(ptr, chip_list) {

What locking protects the newly-added list?

>  		unsigned long pending;
>  		int gpio;
>  
> -		pending = pl061_pending_irq(irq);
> +		chip = list_entry(ptr, struct pl061_gpio, list);
> +		pending = readb(chip->base + GPIOMIS);
> +		writeb(pending, chip->base + GPIOIC);
> +
>  		if (pending == 0)
> -			break;
> +			continue;
>  
> -		for_each_bit(gpio, &pending, BITS_PER_LONG)
> +		for_each_bit(gpio, &pending, PL061_GPIO_NR)
>  			generic_handle_irq(gpio_to_irq(gpio));
>  	}
>  	desc->chip->unmask(irq);
>  }
>  
> -static void pl061_gpio_free(struct work_struct *work)
> -{
> -	struct pl061_gpio *chip = container_of(work, struct pl061_gpio,
> -			gpio_free_work);
> -	int offset;
> -
> -	for_each_bit(offset, chip->gpios_to_free, PL061_GPIO_NR) {
> -		int gpio = offset + chip->gc.base;
> -
> -		if (test_and_clear_bit(offset, chip->gpios_to_free))
> -			gpio_free(gpio);
> -	}
> -}
> -
>  static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
>  {
>  	struct pl061_platform_data *pdata;
>  	struct pl061_gpio *chip;
> +	struct list_head *chip_list;
>  	int ret, irq, i;
>  
>  	pdata = dev->dev.platform_data;
> @@ -251,12 +238,12 @@ static int __init pl061_probe(struct amb
>  	if (chip == NULL)
>  		return -ENOMEM;
>  
> -	if (request_mem_region(dev->res.start, SZ_4K, "pl061") == NULL) {
> +	if (!request_mem_region(dev->res.start, PL061_REG_SIZE, "pl061")) {
>  		ret = -EBUSY;
>  		goto free_mem;
>  	}
>  
> -	chip->base = ioremap(dev->res.start, SZ_4K);
> +	chip->base = ioremap(dev->res.start, PL061_REG_SIZE);
>  	if (chip->base == NULL) {
>  		ret = -ENOMEM;
>  		goto release_region;
> @@ -264,8 +251,7 @@ static int __init pl061_probe(struct amb
>  
>  	spin_lock_init(&chip->lock);
>  	spin_lock_init(&chip->irq_lock);
> -
> -	INIT_WORK(&chip->gpio_free_work, pl061_gpio_free);
> +	INIT_LIST_HEAD(&chip->list);
>  
>  	chip->gc.direction_input = pl061_direction_input;
>  	chip->gc.direction_output = pl061_direction_output;
> @@ -291,7 +277,17 @@ static int __init pl061_probe(struct amb
>  		goto iounmap;
>  	}
>  	set_irq_chained_handler(irq, pl061_irq_handler);
> -	pl061_pending_irq = pdata->pending_irqs;
> +	if (!pdata->is_irq_initialized || !pdata->is_irq_initialized(irq)) {
> +		chip_list = kmalloc(sizeof(*chip_list), GFP_KERNEL);
> +		if (chip_list == NULL) {
> +			ret = -ENOMEM;
> +			goto iounmap;
> +		}
> +		INIT_LIST_HEAD(chip_list);
> +		set_irq_chip_data(irq, chip_list);
> +	} else
> +		chip_list = get_irq_chip_data(irq);
> +	list_add(&chip->list, chip_list);
>  
>  	for (i = 0; i < PL061_GPIO_NR; i++) {
>  		if (pdata->directions & (1 << i))
> @@ -312,7 +308,7 @@ static int __init pl061_probe(struct amb
>  iounmap:
>  	iounmap(chip->base);
>  release_region:
> -	release_mem_region(dev->res.start, SZ_4K);
> +	release_mem_region(dev->res.start, PL061_REG_SIZE);
>  free_mem:
>  	kfree(chip);
>  
> diff -puN include/linux/amba/pl061.h~gpio-driver-for-primecell-pl061-gpio-controller-v4 include/linux/amba/pl061.h
> --- a/include/linux/amba/pl061.h~gpio-driver-for-primecell-pl061-gpio-controller-v4
> +++ a/include/linux/amba/pl061.h
> @@ -1,18 +1,17 @@
>  /* platform data for the PL061 GPIO driver */
>  
> -#define GPIODIR 0x400
> -#define GPIOIS  0x404
> -#define GPIOIBE 0x408
> -#define GPIOIEV 0x40C
> -#define GPIOIE  0x410
> -#define GPIORIS 0x414
> -#define GPIOMIS 0x418
> -#define GPIOIC  0x41C
> -
>  struct pl061_platform_data {
>  	/* number of the first GPIO */
>  	unsigned	gpio_base;
> -	u32		(*pending_irqs)(int irq);
> +
>  	u8		directions;	/* startup directions, 1: out, 0: in */
>  	u8		values;		/* startup values */
> +
> +	/* This callback may be used when you have more than one PL061
> +	 * connected to the same IRQ trigger. This callback must return 0 on
> +	 * the first time it is called for each irq, and 1 on any other call.
> +	 * In case you are not unfortunate enough to have this setup, this
> +	 * pointer must be set to NULL.
> +	 */
> +	int		(*is_irq_initialized)(int irq);
>  };
> _
> 
> 

Re: [PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[Baruch Siach]

Hi Andrew,

On Tue, Jun 09, 2009 at 02:02:39PM -0700, Andrew Morton wrote:
> On Sun,  7 Jun 2009 21:38:55 +0300
> Baruch Siach <baruch@tkos.co.il> wrote:
> 
> > This is a driver for the ARM PrimeCell PL061 GPIO AMBA peripheral. The driver
> > is implemented using the gpiolib framework.
> > 
> > This driver also includes support for the use of the PL061 as an interrupt
> > controller (secondary).
> > 
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> > Changes in v4:
> > 	- Depend on CONFIG_ARM_AMBA
> > 	- Do not request the gpio for IRQ automatically, just warn if it's not 
> > 	  requested
> > 	- Remove SZ_4K
> > 	- Iterate through a linked list find the source of interrupt when 
> > 	  multiple PL061s are connected to the same IRQ trigger
> > 	- Move hardware register defines into the .c file
> 
> I generated an incremental diff so that we can see what you did.
> 
> 
> >  drivers/gpio/Kconfig       |    1 
> >  drivers/gpio/pl061.c       |   92 ++++++++++++++++-------------------
> >  include/linux/amba/pl061.h |   19 +++----
> >  3 files changed, 54 insertions(+), 58 deletions(-)
> > 
> > diff -puN drivers/gpio/Kconfig~gpio-driver-for-primecell-pl061-gpio-controller-v4 drivers/gpio/Kconfig
> > --- a/drivers/gpio/Kconfig~gpio-driver-for-primecell-pl061-gpio-controller-v4
> > +++ a/drivers/gpio/Kconfig
> > @@ -69,6 +69,7 @@ comment "Memory mapped GPIO expanders:"
> >  
> >  config GPIO_PL061
> >  	bool "PrimeCell PL061 GPIO support"
> > +	depends on ARM_AMBA
> >  	help
> >  	  Say yes here to support the PrimeCell PL061 GPIO device
> >  
> > diff -puN drivers/gpio/pl061.c~gpio-driver-for-primecell-pl061-gpio-controller-v4 drivers/gpio/pl061.c
> > --- a/drivers/gpio/pl061.c~gpio-driver-for-primecell-pl061-gpio-controller-v4
> > +++ a/drivers/gpio/pl061.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/spinlock.h>
> >  #include <linux/errno.h>
> >  #include <linux/module.h>
> > +#include <linux/list.h>
> >  #include <linux/io.h>
> >  #include <linux/ioport.h>
> >  #include <linux/irq.h>
> > @@ -24,9 +25,22 @@
> >  #include <linux/amba/bus.h>
> >  #include <linux/amba/pl061.h>
> >  
> > +#define GPIODIR 0x400
> > +#define GPIOIS  0x404
> > +#define GPIOIBE 0x408
> > +#define GPIOIEV 0x40C
> > +#define GPIOIE  0x410
> > +#define GPIORIS 0x414
> > +#define GPIOMIS 0x418
> > +#define GPIOIC  0x41C
> > +
> >  #define PL061_GPIO_NR	8
> >  
> > +#define PL061_REG_SIZE	(4*1024)
> > +
> >  struct pl061_gpio {
> > +	struct list_head	list;
> 
> Some commentary which describes this field would be nice.  What lock
> protects it, what other list_head this list is anchored to, etc

I'll add a comment.

Regarding the lock, the only list_add() is done in the probe method, and this 
method is serialized, as far as I know.

> >  	/* Each of the two spinlocks protects a different set of hardware
> >  	 * regiters and data structurs. This decouples the code of the IRQ from
> >  	 * the GPIO code. This also makes the case of a GPIO routine call from
> > @@ -37,12 +51,8 @@ struct pl061_gpio {
> >  
> >  	void __iomem		*base;
> >  	struct gpio_chip	gc;
> > -	struct work_struct	gpio_free_work;
> > -	DECLARE_BITMAP(gpios_to_free, PL061_GPIO_NR);
> >  };
> >  
> > -static u32 (*pl061_pending_irq)(int irq);
> > -
> >  static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
> >  {
> >  	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> > @@ -112,16 +122,6 @@ static void pl061_irq_disable(unsigned i
> >  	spin_unlock_irqrestore(&chip->irq_lock, flags);
> >  }
> >  
> > -static void pl061_irq_shutdown(unsigned irq)
> > -{
> > -	struct pl061_gpio *chip = get_irq_chip_data(irq);
> > -	int offset = irq_to_gpio(irq) - chip->gc.base;
> > -
> > -	pl061_irq_disable(irq);
> > -	set_bit(offset, chip->gpios_to_free);
> > -	schedule_work(&chip->gpio_free_work);
> > -}
> > -
> >  static void pl061_irq_enable(unsigned irq)
> >  {
> >  	struct pl061_gpio *chip = get_irq_chip_data(irq);
> > @@ -138,16 +138,10 @@ static void pl061_irq_enable(unsigned ir
> >  
> >  static unsigned int pl061_irq_startup(unsigned irq)
> >  {
> > -	int ret;
> > -
> > -	ret = gpio_request(irq_to_gpio(irq), "IRQ");
> > -	if (ret < 0) {
> > -		pr_warning("%s: warning: gpio_request(%d) returned %d\n",
> > -				__func__, irq_to_gpio(irq), ret);
> > -		return 0;
> > -	}
> > +	if (gpio_request(irq_to_gpio(irq), "IRQ") == 0)
> > +		pr_warning("%s: warning: GPIO%d has not been requested\n",
> > +				__func__, irq_to_gpio(irq));
> 
> This is wrong, isn't it?  gpio_request() returns 0 on success.

Russell said that gpio configuration is the responsibility of the platform 
code. Here I just warn when the gpio has not been requested, and thus 
gpio_request() succeeds. I'll add a comment.

> > -	gpio_direction_input(irq_to_gpio(irq));
> >  	pl061_irq_enable(irq);
> >  
> >  	return 0;
> > @@ -202,45 +196,38 @@ static struct irq_chip pl061_irqchip = {
> >  	.startup	= pl061_irq_startup,
> >  	.enable		= pl061_irq_enable,
> >  	.disable	= pl061_irq_disable,
> > -	.shutdown	= pl061_irq_shutdown,
> >  	.set_type	= pl061_irq_type,
> >  };
> >  
> >  static void pl061_irq_handler(unsigned irq, struct irq_desc *desc)
> >  {
> > +	struct list_head *chip_list = get_irq_chip_data(irq);
> > +	struct list_head *ptr;
> > +	struct pl061_gpio *chip;
> > +
> >  	desc->chip->ack(irq);
> > -	while (1) {
> > +	list_for_each(ptr, chip_list) {
> 
> What locking protects the newly-added list?

Do we need locking even though we list_add() only at probe time? (Compiling as 
a module is not supported, so this only happens at boot time).

> >  		unsigned long pending;
> >  		int gpio;
> >  
> > -		pending = pl061_pending_irq(irq);
> > +		chip = list_entry(ptr, struct pl061_gpio, list);
> > +		pending = readb(chip->base + GPIOMIS);
> > +		writeb(pending, chip->base + GPIOIC);
> > +
> >  		if (pending == 0)
> > -			break;
> > +			continue;
> >  
> > -		for_each_bit(gpio, &pending, BITS_PER_LONG)
> > +		for_each_bit(gpio, &pending, PL061_GPIO_NR)
> >  			generic_handle_irq(gpio_to_irq(gpio));
> >  	}
> >  	desc->chip->unmask(irq);
> >  }
> >  
> > -static void pl061_gpio_free(struct work_struct *work)
> > -{
> > -	struct pl061_gpio *chip = container_of(work, struct pl061_gpio,
> > -			gpio_free_work);
> > -	int offset;
> > -
> > -	for_each_bit(offset, chip->gpios_to_free, PL061_GPIO_NR) {
> > -		int gpio = offset + chip->gc.base;
> > -
> > -		if (test_and_clear_bit(offset, chip->gpios_to_free))
> > -			gpio_free(gpio);
> > -	}
> > -}
> > -
> >  static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
> >  {
> >  	struct pl061_platform_data *pdata;
> >  	struct pl061_gpio *chip;
> > +	struct list_head *chip_list;
> >  	int ret, irq, i;
> >  
> >  	pdata = dev->dev.platform_data;
> > @@ -251,12 +238,12 @@ static int __init pl061_probe(struct amb
> >  	if (chip == NULL)
> >  		return -ENOMEM;
> >  
> > -	if (request_mem_region(dev->res.start, SZ_4K, "pl061") == NULL) {
> > +	if (!request_mem_region(dev->res.start, PL061_REG_SIZE, "pl061")) {
> >  		ret = -EBUSY;
> >  		goto free_mem;
> >  	}
> >  
> > -	chip->base = ioremap(dev->res.start, SZ_4K);
> > +	chip->base = ioremap(dev->res.start, PL061_REG_SIZE);
> >  	if (chip->base == NULL) {
> >  		ret = -ENOMEM;
> >  		goto release_region;
> > @@ -264,8 +251,7 @@ static int __init pl061_probe(struct amb
> >  
> >  	spin_lock_init(&chip->lock);
> >  	spin_lock_init(&chip->irq_lock);
> > -
> > -	INIT_WORK(&chip->gpio_free_work, pl061_gpio_free);
> > +	INIT_LIST_HEAD(&chip->list);
> >  
> >  	chip->gc.direction_input = pl061_direction_input;
> >  	chip->gc.direction_output = pl061_direction_output;
> > @@ -291,7 +277,17 @@ static int __init pl061_probe(struct amb
> >  		goto iounmap;
> >  	}
> >  	set_irq_chained_handler(irq, pl061_irq_handler);
> > -	pl061_pending_irq = pdata->pending_irqs;
> > +	if (!pdata->is_irq_initialized || !pdata->is_irq_initialized(irq)) {
> > +		chip_list = kmalloc(sizeof(*chip_list), GFP_KERNEL);
> > +		if (chip_list == NULL) {
> > +			ret = -ENOMEM;
> > +			goto iounmap;
> > +		}
> > +		INIT_LIST_HEAD(chip_list);
> > +		set_irq_chip_data(irq, chip_list);
> > +	} else
> > +		chip_list = get_irq_chip_data(irq);
> > +	list_add(&chip->list, chip_list);
> >  
> >  	for (i = 0; i < PL061_GPIO_NR; i++) {
> >  		if (pdata->directions & (1 << i))
> > @@ -312,7 +308,7 @@ static int __init pl061_probe(struct amb
> >  iounmap:
> >  	iounmap(chip->base);
> >  release_region:
> > -	release_mem_region(dev->res.start, SZ_4K);
> > +	release_mem_region(dev->res.start, PL061_REG_SIZE);
> >  free_mem:
> >  	kfree(chip);
> >  
> > diff -puN include/linux/amba/pl061.h~gpio-driver-for-primecell-pl061-gpio-controller-v4 include/linux/amba/pl061.h
> > --- a/include/linux/amba/pl061.h~gpio-driver-for-primecell-pl061-gpio-controller-v4
> > +++ a/include/linux/amba/pl061.h
> > @@ -1,18 +1,17 @@
> >  /* platform data for the PL061 GPIO driver */
> >  
> > -#define GPIODIR 0x400
> > -#define GPIOIS  0x404
> > -#define GPIOIBE 0x408
> > -#define GPIOIEV 0x40C
> > -#define GPIOIE  0x410
> > -#define GPIORIS 0x414
> > -#define GPIOMIS 0x418
> > -#define GPIOIC  0x41C
> > -
> >  struct pl061_platform_data {
> >  	/* number of the first GPIO */
> >  	unsigned	gpio_base;
> > -	u32		(*pending_irqs)(int irq);
> > +
> >  	u8		directions;	/* startup directions, 1: out, 0: in */
> >  	u8		values;		/* startup values */
> > +
> > +	/* This callback may be used when you have more than one PL061
> > +	 * connected to the same IRQ trigger. This callback must return 0 on
> > +	 * the first time it is called for each irq, and 1 on any other call.
> > +	 * In case you are not unfortunate enough to have this setup, this
> > +	 * pointer must be set to NULL.
> > +	 */
> > +	int		(*is_irq_initialized)(int irq);
> >  };
> > _
> > 
> > 

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[Andrew Morton]

On Wed, 10 Jun 2009 10:22:31 +0300 Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Andrew,
> 
> > >  static unsigned int pl061_irq_startup(unsigned irq)
> > >  {
> > > -	int ret;
> > > -
> > > -	ret = gpio_request(irq_to_gpio(irq), "IRQ");
> > > -	if (ret < 0) {
> > > -		pr_warning("%s: warning: gpio_request(%d) returned %d\n",
> > > -				__func__, irq_to_gpio(irq), ret);
> > > -		return 0;
> > > -	}
> > > +	if (gpio_request(irq_to_gpio(irq), "IRQ") == 0)
> > > +		pr_warning("%s: warning: GPIO%d has not been requested\n",
> > > +				__func__, irq_to_gpio(irq));
> > 
> > This is wrong, isn't it?  gpio_request() returns 0 on success.
> 
> Russell said that gpio configuration is the responsibility of the platform 
> code. Here I just warn when the gpio has not been requested, and thus 
> gpio_request() succeeds. I'll add a comment.

OK.

If the gpio_request() accidentally succeeded, should we gpio_free() the
result here?

Should the gpio core provide a primitive to check that a gpio has been
properly requested rathe rthan open-coding it here?

> > >  static void pl061_irq_handler(unsigned irq, struct irq_desc *desc)
> > >  {
> > > +	struct list_head *chip_list = get_irq_chip_data(irq);
> > > +	struct list_head *ptr;
> > > +	struct pl061_gpio *chip;
> > > +
> > >  	desc->chip->ack(irq);
> > > -	while (1) {
> > > +	list_for_each(ptr, chip_list) {
> > 
> > What locking protects the newly-added list?
> 
> Do we need locking even though we list_add() only at probe time?

Nope.  I guess.  It depends on the driver.  hotplug/hot-remove needs to
beconsidered often.

> (Compiling as 
> a module is not supported, so this only happens at boot time).

The probe handler is probably serialised against everything else even if
the driver _is_ a module.

Re: [PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[Felipe Balbi]

Hi,

On Wed, Jun 10, 2009 at 09:44:47AM +0200, ext Andrew Morton wrote:
> On Wed, 10 Jun 2009 10:22:31 +0300 Baruch Siach <baruch@tkos.co.il> wrote:
> 
> > Hi Andrew,
> > 
> > > >  static unsigned int pl061_irq_startup(unsigned irq)
> > > >  {
> > > > -	int ret;
> > > > -
> > > > -	ret = gpio_request(irq_to_gpio(irq), "IRQ");
> > > > -	if (ret < 0) {
> > > > -		pr_warning("%s: warning: gpio_request(%d) returned %d\n",
> > > > -				__func__, irq_to_gpio(irq), ret);
> > > > -		return 0;
> > > > -	}
> > > > +	if (gpio_request(irq_to_gpio(irq), "IRQ") == 0)
> > > > +		pr_warning("%s: warning: GPIO%d has not been requested\n",
> > > > +				__func__, irq_to_gpio(irq));
> > > 
> > > This is wrong, isn't it?  gpio_request() returns 0 on success.
> > 
> > Russell said that gpio configuration is the responsibility of the platform 
> > code. Here I just warn when the gpio has not been requested, and thus 
> > gpio_request() succeeds. I'll add a comment.
> 
> OK.
> 
> If the gpio_request() accidentally succeeded, should we gpio_free() the
> result here?
> 
> Should the gpio core provide a primitive to check that a gpio has been
> properly requested rathe rthan open-coding it here?

how about passing a setup() and cleanup() pointers via platform_data to the driver ?

Then, during probe(), the driver calls setup() which would
gpio_request() and during remove() it calls cleanup() to gpio_free();

would that be ok ?

-- 
balbi

Re: [PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[Baruch Siach]

On Wed, Jun 10, 2009 at 12:44:47AM -0700, Andrew Morton wrote:
> On Wed, 10 Jun 2009 10:22:31 +0300 Baruch Siach <baruch@tkos.co.il> wrote:
> 
> > Hi Andrew,
> > 
> > > >  static unsigned int pl061_irq_startup(unsigned irq)
> > > >  {
> > > > -	int ret;
> > > > -
> > > > -	ret = gpio_request(irq_to_gpio(irq), "IRQ");
> > > > -	if (ret < 0) {
> > > > -		pr_warning("%s: warning: gpio_request(%d) returned %d\n",
> > > > -				__func__, irq_to_gpio(irq), ret);
> > > > -		return 0;
> > > > -	}
> > > > +	if (gpio_request(irq_to_gpio(irq), "IRQ") == 0)
> > > > +		pr_warning("%s: warning: GPIO%d has not been requested\n",
> > > > +				__func__, irq_to_gpio(irq));
> > > 
> > > This is wrong, isn't it?  gpio_request() returns 0 on success.
> > 
> > Russell said that gpio configuration is the responsibility of the platform 
> > code. Here I just warn when the gpio has not been requested, and thus 
> > gpio_request() succeeds. I'll add a comment.
> 
> OK.
> 
> If the gpio_request() accidentally succeeded, should we gpio_free() the
> result here?

I don't think so. This is just a warning. We do use this GPIO here, so I guess 
we should keep it requested.

> Should the gpio core provide a primitive to check that a gpio has been
> properly requested rathe rthan open-coding it here?

Probably. The author (maintainer?) of gpiolib, David Brownell, is not 
responsive at the moment. (I have an SPI master driver waiting for his 
review/ack).

> > > >  static void pl061_irq_handler(unsigned irq, struct irq_desc *desc)
> > > >  {
> > > > +	struct list_head *chip_list = get_irq_chip_data(irq);
> > > > +	struct list_head *ptr;
> > > > +	struct pl061_gpio *chip;
> > > > +
> > > >  	desc->chip->ack(irq);
> > > > -	while (1) {
> > > > +	list_for_each(ptr, chip_list) {
> > > 
> > > What locking protects the newly-added list?
> > 
> > Do we need locking even though we list_add() only at probe time?
> 
> Nope.  I guess.  It depends on the driver.  hotplug/hot-remove needs to
> beconsidered often.
> 
> > (Compiling as 
> > a module is not supported, so this only happens at boot time).
> 
> The probe handler is probably serialised against everything else even if
> the driver _is_ a module.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH v4] gpio: driver for PrimeCell PL061 GPIO controller

[David Brownell]

On Wednesday 10 June 2009, Baruch Siach wrote:
> > Should the gpio core provide a primitive to check that a gpio has been
> > properly requested rathe rthan open-coding it here?
> 
> Probably. 

I happen to strongly dislike primitives that are fundamentally
MT-unsafe, except in non-threaded environments ... so I would
disagree about that.  :)

If you want to see if a GPIO is available, allocate it.
The only answer you can really depend on is "yes, it's
mine now".  You can't rely on other threads not allocating,
or not de-allocating, the GPIO later ... unless they can't,
because it's not theirs.

But that's beside the point, since GPIO drivers shouldn't
be in the business of requesting the GPIOs they manage.
Such layer violations are another sign that you may not be
thinking very clearly about things.

In this case, whoever allocated that GPIO for IRQ purposes
should have done the gpio_request() before handing out
the gpio_to_irq() results to however many drivers ended
up needing to use it.

- Dave