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

From: Linus Walleij
Date: Sun Jun 07 2009 - 17:33:41 EST


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

2009/6/7 Baruch Siach <baruch@xxxxxxxxxx>:
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>");
> +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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/