gpio: Renesas R-Car GPIO driver
Subsystems (3)
Magnus Damm
maintainerPATCH 1/1 gpio: Renesas R-Car GPIO driver
From: Magnus Damm <[email protected]>
This patch implements a GPIO driver for the R-Car series of SoCs from Renesas. This driver is designed to be reusable between multiple SoCs that share the same basic building block, but so far it has only been used on R-Car H1 (r8a7779).
Each driver instance handles 32 GPIOs with individually maskable IRQs. The driver operates on a single I/O memory range and the 32 GPIOs are hooked up a single interrupt.
In the case of R-Car H1 either external IRQ pins or GPIOs with interrupts can be used for on-board interupts. For external IRQs 4 pins are supported, and in the case of GPIO there are 202 GPIOS as 202 interrupts hooked up via 6 driver instances and to the GIC and the Cortex-A9 Quad.
At this point this driver is interfacing as a regular platform device driver. In the future DT support will be submitted as an incremental feature patch.
---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rcar.c | 406 +++++++++++++++++++++++++++++++ include/linux/platform_data/gpio-rcar.h | 29 ++ 4 files changed, 442 insertions(+)
drivers/gpio/Kconfig +6 -0
@@ -201,6 +201,12 @@ config GPIO_PXA help Say yes here to support the PXA GPIO device +config GPIO_RCAR + tristate "Renesas R-Car GPIO" + depends on ARM + help + Say yes here to support GPIO on Renesas R-Car SoCs. + config GPIO_SPEAR_SPICS bool "ST SPEAr13xx SPI Chip Select as GPIO support" depends on PLAT_SPEAR
drivers/gpio/Makefile +1 -0
@@ -56,6 +56,7 @@ obj-$(CONFIG_GPIO_PL061) += gpio-pl061.o obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o +obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o obj-$(CONFIG_PLAT_SAMSUNG) += gpio-samsung.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o obj-$(CONFIG_GPIO_SCH) += gpio-sch.o
drivers/gpio/gpio-rcar.c new file +406 -0
406 lines not on this page · raw message
include/linux/platform_data/gpio-rcar.h new file +29 -0
@@ -0,0 +1,29 @@ +/* +* Renesas R-Car GPIO Support +* +* Copyright (C) 2013 Magnus Damm +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +*/ + +#ifndef __GPIO_RCAR_H__ +#define __GPIO_RCAR_H__ + +struct gpio_rcar_config { +unsigned int gpio_base; +unsigned int irq_base; +unsigned int number_of_pins; +}; + +#endif /* __GPIO_RCAR_H__ */
Hi Magnus,
Thank you for the patch.
Quoted 75 lines · On Tuesday 05 March 2013 09:32:19 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
This patch implements a GPIO driver for the R-Car series of SoCs from Renesas. This driver is designed to be reusable between multiple SoCs that share the same basic building block, but so far it has only been used on R-Car H1 (r8a7779).
Each driver instance handles 32 GPIOs with individually maskable IRQs. The driver operates on a single I/O memory range and the 32 GPIOs are hooked up a single interrupt.
In the case of R-Car H1 either external IRQ pins or GPIOs with interrupts can be used for on-board interupts. For external IRQs 4 pins are supported, and in the case of GPIO there are 202 GPIOS as 202 interrupts hooked up via 6 driver instances and to the GIC and the Cortex-A9 Quad.
At this point this driver is interfacing as a regular platform device driver. In the future DT support will be submitted as an incremental feature patch.
Signed-off-by Magnus Damm <[email protected]>---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rcar.c | 406 ++++++++++++++++++++++++++++ include/linux/platform_data/gpio-rcar.h | 29 ++ 4 files changed, 442 insertions(+)drivers/gpio/Kconfig +6
-0@@ -201,6 +201,12 @@ config GPIO_PXA help Say yes here to support the PXA GPIO device +config GPIO_RCAR + tristate "Renesas R-Car GPIO" + depends on ARM + help + Say yes here to support GPIO on Renesas R-Car SoCs. + config GPIO_SPEAR_SPICS bool "ST SPEAr13xx SPI Chip Select as GPIO support" depends on PLAT_SPEARdrivers/gpio/Makefile +1
-0@@ -56,6 +56,7 @@ obj-$(CONFIG_GPIO_PL061) += gpio-pl061.o obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o +obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o obj-$(CONFIG_PLAT_SAMSUNG) += gpio-samsung.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o obj-$(CONFIG_GPIO_SCH) += gpio-sch.odrivers/gpio/gpio-rcar.c new file +17
-0@@ -0,0 +1,406 @@ +/* +* Renesas R-Car GPIO Support +* +* Copyright (C) 2013 Magnus Damm +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
Quoted 1 line
USA + */
You can remove the last paragraph.
Quoted 5 lines
drivers/gpio/gpio-rcar.c new file +5
-0+ +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> +#include <linux/interrupt.h>
drivers/gpio/gpio-rcar.c line 33
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+#include <linux/ioport.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/bitops.h> +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/platform_data/gpio-rcar.h>
Could you please sort the headers alphabetically ?
Quoted 12 lines
drivers/gpio/gpio-rcar.c new file +12
-0+ +struct gpio_rcar_priv { +void __iomem *base; +spinlock_t lock; +struct gpio_rcar_config config; +struct platform_device *pdev; +struct gpio_chip gpio_chip; +struct irq_chip irq_chip; +struct irq_domain *irq_domain; +}; + +#define IOINTSEL 0x00
drivers/gpio/gpio-rcar.c line 55
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+#define INOUTSEL 0x04 +#define OUTDT 0x08 +#define INDT 0x0c +#define INTDT 0x10 +#define INTCLR 0x14 +#define INTMSK 0x18 +#define MSKCLR 0x1c +#define POSNEG 0x20 +#define EDGLEVEL 0x24 +#define FILONOFF 0x28
#define'd values are usually tab-aligned, but that's up to you.
drivers/gpio/gpio-rcar.c line 66
Quoted 11 lines
+static inline unsigned long
gpio_rcar_read(struct gpio_rcar_priv *p, int offs)drivers/gpio/gpio-rcar.c new file +9
-0+{ +return ioread32 (p->base + offs); +} + +static inline void gpio_rcar_write (struct gpio_rcar_priv *p,int offs, +unsigned long value) +{ +iowrite32 (value, p->base + offs); +}
You often perform read-update-write operations, would it make sense to define gpio_rcar_clr() and gpio_rcar_set() functions ?
Quoted 42 lines
drivers/gpio/gpio-rcar.c new file +42
-0+static void gpio_rcar_irq_disable (struct irq_data *d) +{ +struct gpio_rcar_priv *p =irq_data_get_irq_chip_data (d); + +gpio_rcar_write (p,INTMSK , ~BIT (irqd_to_hwirq (d))); +} + +static void gpio_rcar_irq_enable (struct irq_data *d) +{ +struct gpio_rcar_priv *p =irq_data_get_irq_chip_data (d); + +gpio_rcar_write (p,MSKCLR ,BIT (irqd_to_hwirq (d))); +} + +static void gpio_rcar_config_interrupt_input_mode (struct gpio_rcar_priv *p, +unsigned int hwirq, +bool active_high_rising_edge, +bool level_trigger) +{ +unsigned long bit =BIT (hwirq); +unsigned long flags, tmp; + +/* follow steps in the GPIO documentation for +* "Setting Edge-Sensitive Interrupt Input Mode" and +* "Setting Level-Sensitive Interrupt Input Mode" +*/ + +spin_lock_irqsave (&p->lock, flags); + +/* Configure postive or negative logic in POSNEG */ + tmp =gpio_rcar_read (p,POSNEG ); +if (active_high_rising_edge) + tmp &= ~bit; +else + tmp |= bit; +gpio_rcar_write (p,POSNEG , tmp); + +/* Configure edge or level trigger in EDGLEVEL */ + tmp =gpio_rcar_read (p,EDGLEVEL ); +if (level_trigger) + tmp &= ~bit; +else
drivers/gpio/gpio-rcar.c line 119
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+ tmp |= bit; +gpio_rcar_write (p,EDGLEVEL , tmp); + +/* Select "Interrupt Input Mode" in IOINTSEL */ + tmp =gpio_rcar_read (p,IOINTSEL ); + tmp |= bit; +gpio_rcar_write (p,IOINTSEL , tmp); + +/* Write INTCLR in case of edge trigger */ +if (!level_trigger)
Maybe you could do so unconditionally.
drivers/gpio/gpio-rcar.c line 120
Quoted 1 line
drivers/gpio/gpio-rcar.c new file +1
-0+gpio_rcar_write (p,INTCLR , bit);
I suppose the idea here it to avoid spurious interrupts when switching to edge-triggered interrupt mode. If the interrupt was already enabled wouldn't it still be registered before you clear the flag ?
drivers/gpio/gpio-rcar.c line 130
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+ +spin_unlock_irqrestore (&p->lock, flags); +} + +static int gpio_rcar_irq_set_type (struct irq_data *d,unsigned int type) +{ +struct gpio_rcar_priv *p =irq_data_get_irq_chip_data (d); +unsigned int hwirq =irqd_to_hwirq (d); + +pr_debug ("gpio: sense irq = %d, type = %d\n" , hwirq, type);
dev_dbg() ?
Quoted 14 lines
drivers/gpio/gpio-rcar.c new file +14
-0+ +switch (type &IRQ_TYPE_SENSE_MASK ) { +case IRQ_TYPE_LEVEL_HIGH : +gpio_rcar_config_interrupt_input_mode (p, hwirq, true, true); +break ; +case IRQ_TYPE_LEVEL_LOW : +gpio_rcar_config_interrupt_input_mode (p, hwirq, false, true); +break ; +case IRQ_TYPE_EDGE_RISING : +gpio_rcar_config_interrupt_input_mode (p, hwirq, true, false); +break ; +case IRQ_TYPE_EDGE_FALLING : +gpio_rcar_config_interrupt_input_mode (p, hwirq, false, false); +break ;
drivers/gpio/gpio-rcar.c line 154
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+default : +return -EINVAL ; + } +return 0 ; +} + +static irqreturn_t gpio_rcar_irq_handler (int irq,void *dev_id) +{ +struct gpio_rcar_priv *p = dev_id; +unsigned long pending;
I would use u32 here as the register is 32-bits wide.
Quoted 56 lines
drivers/gpio/gpio-rcar.c new file +18
-0+unsigned int offset, irqs_handled =0 ; + +while ((pending =gpio_rcar_read (p,INTDT ))) { + offset =__ffs (pending); +gpio_rcar_write (p,INTCLR ,BIT (offset)); +generic_handle_irq (irq_find_mapping (p->irq_domain, offset)); + irqs_handled++; + } + +return irqs_handled ?IRQ_HANDLED :IRQ_NONE ; +} + +static inline struct gpio_rcar_priv *gpio_to_priv (struct gpio_chip *chip) +{ + return container_of(chip, struct gpio_rcar_priv, gpio_chip); +} + +static void gpio_rcar_config_general_input_output_mode(struct gpio_chip*chip,
drivers/gpio/gpio-rcar.c new file +36
-0+ unsigned int gpio, + bool output) +{ +struct gpio_rcar_priv *p =gpio_to_priv (chip); +unsigned long bit =BIT (gpio); +unsigned long flags, tmp; + +/* follow steps in the GPIO documentation for +* "Setting General Output Mode" and +* "Setting General Input Mode" +*/ + +spin_lock_irqsave (&p->lock, flags); + +/* Configure postive logic in POSNEG */ + tmp =gpio_rcar_read (p,POSNEG ); + tmp &= ~bit; +gpio_rcar_write (p,POSNEG , tmp); + +/* Select "General Input/Output Mode" in IOINTSEL */ + tmp =gpio_rcar_read (p,IOINTSEL ); + tmp &= ~bit; +gpio_rcar_write (p,IOINTSEL , tmp); + +/* Select Input Mode or Output Mode in INOUTSEL */ + tmp =gpio_rcar_read (p,INOUTSEL ); +if (output) + tmp |= bit; +else + tmp &= ~bit; +gpio_rcar_write (p,INOUTSEL , tmp); + +spin_unlock_irqrestore (&p->lock, flags); +} + +static int gpio_rcar_direction_input(struct gpio_chip *chip, unsignedoffset)
drivers/gpio/gpio-rcar.c line 217
Quoted 9 lines
drivers/gpio/gpio-rcar.c new file +9
-0+{ +gpio_rcar_config_general_input_output_mode (chip, offset, false); +return 0 ; +} + +static int gpio_rcar_get (struct gpio_chip *chip,unsigned offset) +{ +return (int )(gpio_rcar_read (gpio_to_priv (chip),INDT ) &BIT (offset)); +}
Isn't the get operation supposed to return 0 or 1 ? In that case
return !!(gpio_rcar_read(gpio_to_priv(chip), INDT) & BIT(offset));
would be better.
drivers/gpio/gpio-rcar.c line 223
Quoted 6 lines
+static void
gpio_rcar_set(struct gpio_chip *chip, unsigned offset, int value)drivers/gpio/gpio-rcar.c new file +4
-0+{ +struct gpio_rcar_priv *p =gpio_to_priv (chip); +unsigned long bit =BIT (offset); +unsigned long flags, tmp;
I would declare both bit and tmp as u32.
Quoted 17 lines
drivers/gpio/gpio-rcar.c new file +13
-0+spin_lock_irqsave (&p->lock, flags); + + tmp =gpio_rcar_read (p,OUTDT ); +if (value) + tmp |= bit; +else + tmp &= ~bit; +gpio_rcar_write (p,OUTDT , tmp); + +spin_unlock_irqrestore (&p->lock, flags); +} + +static int gpio_rcar_direction_output(struct gpio_chip *chip, unsignedoffset,
drivers/gpio/gpio-rcar.c new file +3
-0+ int value) +{ +/* write GPIO value to output before selecting output mode of pin */
drivers/gpio/gpio-rcar.c line 250
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+gpio_rcar_set (chip, offset, value); +gpio_rcar_config_general_input_output_mode (chip, offset, true); +return 0 ; +} + +static int gpio_rcar_to_irq (struct gpio_chip *chip,unsigned offset) +{ +return irq_create_mapping (gpio_to_priv (chip)->irq_domain, offset); +} +
I would personally move the irq_chip operations handlers here to group all irq_chip-related functions together, but that's up to you.
drivers/gpio/gpio-rcar.c line 256
Quoted 7 lines
+static int gpio_rcar_irq_domain_map(struct
irq_domain*h, unsigned int virq,drivers/gpio/gpio-rcar.c new file +5
-0+ irq_hw_number_t hw) +{ +struct gpio_rcar_priv *p = h->host_data; + +pr_debug ("gpio: map hw irq = %d, virq = %d\n" , (int )hw, virq);
Maybe dev_dbg() ?
drivers/gpio/gpio-rcar.c line 259
Quoted 3 lines
drivers/gpio/gpio-rcar.c new file +3
-0+ +irq_set_chip_data (virq, h->host_data); +irq_set_chip_and_handler (virq, &p->irq_chip, handle_level_irq);
I'm not too familiar with the IRQ core. GPIO interrupts can be configured as either edge-trigerred or level-trigerred. Can handle_level_irq() handle both ?
Quoted 9 lines
drivers/gpio/gpio-rcar.c new file +9
-0+set_irq_flags (virq,IRQF_VALID );/* kill me now */ +return 0 ; +} + +static struct irq_domain_ops gpio_rcar_irq_domain_ops = { + .map = gpio_rcar_irq_domain_map, +}; + +static int gpio_rcar_probe (struct platform_device *pdev)
drivers/gpio/gpio-rcar.c line 278
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+{ +struct gpio_rcar_config *pdata = pdev->dev.platform_data; +struct gpio_rcar_priv *p; +struct resource *io, *irq; +struct gpio_chip *gpio_chip; +struct irq_chip *irq_chip; +const char *name =dev_name (&pdev->dev); +int ret; + + p =kzalloc (sizeof (*p),GFP_KERNEL );
You can use devm_kzalloc() to simplify error management.
drivers/gpio/gpio-rcar.c line 281
Quoted 3 lines
drivers/gpio/gpio-rcar.c new file +3
-0+if (!p) { +dev_err (&pdev->dev,"failed to allocate driver data\n" ); + ret = -ENOMEM ;
You can return -ENOMEM; directly here.
drivers/gpio/gpio-rcar.c line 286
Quoted 5 lines
drivers/gpio/gpio-rcar.c new file +5
-0+ goto err0; + } + +/* deal with driver instance configuration */ +if (pdata)
Shouldn't you return an error if pdata == NULL ?
drivers/gpio/gpio-rcar.c line 287
Quoted 1 line
drivers/gpio/gpio-rcar.c new file +1
-0+memcpy (&p->config, pdata,sizeof (*pdata));
p->config = *pdata;
seems to be the preferred style nowadays. You could also store a pointer to a struct gpio_rcar_config in gpio_rcar_priv, but I suppose you have decided not to do so in prevision for DT support.
drivers/gpio/gpio-rcar.c line 298
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+ p->pdev = pdev; +platform_set_drvdata (pdev, p); +spin_lock_init (&p->lock); + + io =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + irq =platform_get_resource (pdev,IORESOURCE_IRQ ,0 ); + +if (!io || !irq) { +dev_err (&pdev->dev,"missing IRQ or IOMEM\n" ); + ret = -EINVAL ;
return -EINVAL;
drivers/gpio/gpio-rcar.c line 302
Quoted 4 lines
drivers/gpio/gpio-rcar.c new file +4
-0+ goto err1; + } + + p->base =ioremap_nocache (io->start,resource_size (io));
devm_ioremap_nocache()
drivers/gpio/gpio-rcar.c line 305
Quoted 3 lines
drivers/gpio/gpio-rcar.c new file +3
-0+if (!p->base) { +dev_err (&pdev->dev,"failed to remap I/O memory\n" ); + ret = -ENXIO ;
return -ENXIO;
Quoted 21 lines
drivers/gpio/gpio-rcar.c new file +21
-0+ goto err1; + } + + gpio_chip = &p->gpio_chip; + gpio_chip->direction_input = gpio_rcar_direction_input; + gpio_chip->get = gpio_rcar_get; + gpio_chip->direction_output = gpio_rcar_direction_output; + gpio_chip->set = gpio_rcar_set; + gpio_chip->to_irq = gpio_rcar_to_irq; + gpio_chip->label = name; + gpio_chip->owner =THIS_MODULE ; + gpio_chip->base = p->config.gpio_base; + gpio_chip->ngpio = p->config.number_of_pins; + + irq_chip = &p->irq_chip; + irq_chip->name = name; + irq_chip->irq_mask = gpio_rcar_irq_disable; + irq_chip->irq_unmask = gpio_rcar_irq_enable; + irq_chip->irq_enable = gpio_rcar_irq_enable; + irq_chip->irq_disable = gpio_rcar_irq_disable; + irq_chip->irq_set_type = gpio_rcar_irq_set_type;
drivers/gpio/gpio-rcar.c line 336
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+ irq_chip->flags =IRQCHIP_SKIP_SET_WAKE ; + + p->irq_domain =irq_domain_add_simple (pdev->dev.of_node, + p->config.number_of_pins, + p->config.irq_base, + &gpio_rcar_irq_domain_ops, p); +if (!p->irq_domain) { + ret = -ENXIO ; +dev_err (&pdev->dev,"cannot initialize irq domain\n" ); + goto err2;
return -ENXIO;
drivers/gpio/gpio-rcar.c line 339
Quoted 3 lines
drivers/gpio/gpio-rcar.c new file +3
-0+ } + +if (request_irq (irq->start, gpio_rcar_irq_handler,0 , name, p)) {
devm_request_irq()
Quoted 4 lines
drivers/gpio/gpio-rcar.c new file +4
-0+dev_err (&pdev->dev,"failed to request IRQ\n" ); + ret = -ENOENT ; + goto err3; + }
drivers/gpio/gpio-rcar.c line 353
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+ + ret =gpiochip_add (gpio_chip); +if (ret) { +dev_err (&pdev->dev,"failed to add GPIO controller\n" ); + goto err4; + } + +dev_info (&pdev->dev,"driving %d GPIOs\n" , p->config.number_of_pins); + +/* warn in case of mismatch if irq base is specified */
When does this happen ?
drivers/gpio/gpio-rcar.c line 357
Quoted 4 lines
drivers/gpio/gpio-rcar.c new file +4
-0+if (p->config.irq_base) { + ret =irq_find_mapping (p->irq_domain,0 ); +if (p->config.irq_base != ret) + dev_warn(&pdev->dev, "irq base mismatch (%d/%d)\n",
p->config.irq_base is unsigned and irq_find_mapping() returns an unsigned. %u/%u would this be more appropriate.
Quoted 5 lines
drivers/gpio/gpio-rcar.c new file +5
-0+ p->config.irq_base, ret); + } + +return 0 ; +
drivers/gpio/gpio-rcar.c line 372
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+err4: +free_irq (irq->start, p); +err3: +irq_domain_remove (p->irq_domain); +err2: +iounmap (p->base); +err1: +kfree (p); +err0: +return ret;
With the devm_* functions used above you will have a single error label to call irq_domain_remove.
Quoted 66 lines
drivers/gpio/gpio-rcar.c new file +34
-0+} + +static int gpio_rcar_remove (struct platform_device *pdev) +{ +struct gpio_rcar_priv *p =platform_get_drvdata (pdev); +struct resource *irq; +int ret; + + ret =gpiochip_remove (&p->gpio_chip); +if (ret) +return ret; + + irq =platform_get_resource (pdev,IORESOURCE_IRQ ,0 ); + +free_irq (irq->start, p); +irq_domain_remove (p->irq_domain); +iounmap (p->base); +kfree (p); +return 0 ; +} + +static struct platform_driver gpio_rcar_device_driver = { + .probe = gpio_rcar_probe, + .remove = gpio_rcar_remove, + .driver = { + .name ="gpio_rcar" , + } +}; + +module_platform_driver (gpio_rcar_device_driver); + +MODULE_AUTHOR ("Magnus Damm" ); +MODULE_DESCRIPTION ("Renesas R-Car GPIO Driver" ); +MODULE_LICENSE ("GPL v2" );include/linux/platform_data/gpio-rcar.h new file +0
-009:07:35.000000000 +0900 @@ -0,0 +1,29 @@
+/*
+ * Renesas R-Car GPIO Support
+ *
+ * Copyright (C) 2013 Magnus Damm
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */include/linux/platform_data/gpio-rcar.h new file +11
-0+ +#ifndef __GPIO_RCAR_H__ +#define __GPIO_RCAR_H__ + +struct gpio_rcar_config { +unsigned int gpio_base; +unsigned int irq_base; +unsigned int number_of_pins; +}; + +#endif /* __GPIO_RCAR_H__ */
Signature · 3 lines
Regards, Laurent Pinchart
Magnus Damm
maintainerHI Laurent,
Quoted 3 lines · Laurent Pinchart
Hi Magnus,
Thank you for the patch.
Thanks for your review!
Quoted 71 lines · On Tuesday 05 March 2013 09:32:19 Magnus Damm wrote:
Quoted 70 lines
From: Magnus Damm <[email protected]>
This patch implements a GPIO driver for the R-Car series of SoCs from Renesas. This driver is designed to be reusable between multiple SoCs that share the same basic building block, but so far it has only been used on R-Car H1 (r8a7779).
Each driver instance handles 32 GPIOs with individually maskable IRQs. The driver operates on a single I/O memory range and the 32 GPIOs are hooked up a single interrupt.
In the case of R-Car H1 either external IRQ pins or GPIOs with interrupts can be used for on-board interupts. For external IRQs 4 pins are supported, and in the case of GPIO there are 202 GPIOS as 202 interrupts hooked up via 6 driver instances and to the GIC and the Cortex-A9 Quad.
At this point this driver is interfacing as a regular platform device driver. In the future DT support will be submitted as an incremental feature patch.
Signed-off-by Magnus Damm <[email protected]>---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rcar.c | 406 ++++++++++++++++++++++++++++ include/linux/platform_data/gpio-rcar.h | 29 ++ 4 files changed, 442 insertions(+)drivers/gpio/Kconfig 2013-03-05 09:07:34.000000000 +0900 renamed from drivers/gpio/Kconfig +6
-0@@ -201,6 +201,12 @@ config GPIO_PXA help Say yes here to support the PXA GPIO device +config GPIO_RCAR + tristate "Renesas R-Car GPIO" + depends on ARM + help + Say yes here to support GPIO on Renesas R-Car SoCs. + config GPIO_SPEAR_SPICS bool "ST SPEAr13xx SPI Chip Select as GPIO support" depends on PLAT_SPEARdrivers/gpio/Makefile 2013-03-05 09:07:34.000000000 +0900 renamed from drivers/gpio/Makefile +1
-0@@ -56,6 +56,7 @@ obj-$(CONFIG_GPIO_PL061) += gpio-pl061.o obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o +obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o obj-$(CONFIG_PLAT_SAMSUNG) += gpio-samsung.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o obj-$(CONFIG_GPIO_SCH) += gpio-sch.odrivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +12
-0@@ -0,0 +1,406 @@ +/* + * Renesas R-Car GPIO Support + * + * Copyright (C) 2013 Magnus Damm + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
Quoted 8 lines
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +5
-0+ * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307USA + */
You can remove the last paragraph.
Sure!
Quoted 9 lines
Quoted 9 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +9
-0+ +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> +#include <linux/interrupt.h> +#include <linux/ioport.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/irqdomain.h>
Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +6
-0+#include <linux/bitops.h> +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/platform_data/gpio-rcar.h>Could you please sort the headers alphabetically ?
Uhm, ok, if that makes you happy! =)
Quoted 25 lines
Quoted 12 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +12
-0+ +struct gpio_rcar_priv { + void __iomem *base; + spinlock_t lock; + struct gpio_rcar_config config; + struct platform_device *pdev; + struct gpio_chip gpio_chip; + struct irq_chip irq_chip; + struct irq_domain *irq_domain; +}; + +#define IOINTSEL 0x00drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 10 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +10
-0+#define INOUTSEL 0x04 +#define OUTDT 0x08 +#define INDT 0x0c +#define INTDT 0x10 +#define INTCLR 0x14 +#define INTMSK 0x18 +#define MSKCLR 0x1c +#define POSNEG 0x20 +#define EDGLEVEL 0x24 +#define FILONOFF 0x28#define'd values are usually tab-aligned, but that's up to you.
Quoted 14 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 11 lines
+static inline unsigned long
gpio_rcar_read(struct gpio_rcar_priv *p, int offs)drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +9
-0+{ + return ioread32(p->base + offs); +} + +static inline void gpio_rcar_write(struct gpio_rcar_priv *p, int offs, + unsigned long value) +{ + iowrite32(value, p->base + offs); +}You often perform read-update-write operations, would it make sense to define
gpio_rcar_clr()andgpio_rcar_set()functions ?
Good idea, I'll check and see if it reduces the code size.
Quoted 46 lines
Quoted 46 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +46
-0+static void gpio_rcar_irq_disable(struct irq_data *d) +{ + struct gpio_rcar_priv *p = irq_data_get_irq_chip_data(d); + + gpio_rcar_write(p, INTMSK, ~BIT(irqd_to_hwirq(d))); +} + +static void gpio_rcar_irq_enable(struct irq_data *d) +{ + struct gpio_rcar_priv *p = irq_data_get_irq_chip_data(d); + + gpio_rcar_write(p, MSKCLR, BIT(irqd_to_hwirq(d))); +} + +static void gpio_rcar_config_interrupt_input_mode(struct gpio_rcar_priv *p, + unsigned int hwirq, + bool active_high_rising_edge, + bool level_trigger) +{ + unsigned long bit = BIT(hwirq); + unsigned long flags, tmp; + + /* follow steps in the GPIO documentation for + * "Setting Edge-Sensitive Interrupt Input Mode" and + * "Setting Level-Sensitive Interrupt Input Mode" + */ + + spin_lock_irqsave(&p->lock, flags); + + /* Configure postive or negative logic in POSNEG */ + tmp = gpio_rcar_read(p, POSNEG); + if (active_high_rising_edge) + tmp &= ~bit; + else + tmp |= bit; + gpio_rcar_write(p, POSNEG, tmp); + + /* Configure edge or level trigger in EDGLEVEL */ + tmp = gpio_rcar_read(p, EDGLEVEL); + if (level_trigger) + tmp &= ~bit; + else + tmp |= bit; + gpio_rcar_write(p, EDGLEVEL, tmp); + + /* Select "Interrupt Input Mode" in IOINTSEL */
Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +6
-0+ tmp = gpio_rcar_read(p, IOINTSEL); + tmp |= bit; + gpio_rcar_write(p, IOINTSEL, tmp); + + /* Write INTCLR in case of edge trigger */ + if (!level_trigger)Maybe you could do so unconditionally.
I could, but I wouldn't follow the recommended sequence in the data sheet then.
Quoted 5 lines
Quoted 1 line
+
gpio_rcar_write(p, INTCLR, bit);I suppose the idea here it to avoid spurious interrupts when switching to edge-triggered interrupt mode. If the interrupt was already enabled wouldn't it still be registered before you clear the flag ?
Maybe. Perhaps I shall use IRQCHIP_SET_TYPE_MASKED here?
Quoted 4 lines
Quoted 4 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +4
-0+ + spin_unlock_irqrestore(&p->lock, flags); +} +
Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +6
-0+static int gpio_rcar_irq_set_type(struct irq_data *d, unsigned int type) +{ + struct gpio_rcar_priv *p = irq_data_get_irq_chip_data(d); + unsigned int hwirq = irqd_to_hwirq(d); + + pr_debug("gpio: sense irq = %d, type = %d\n", hwirq, type);
dev_dbg()?
Sure, why not?
Quoted 18 lines
Quoted 18 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +18
-0+ + switch (type & IRQ_TYPE_SENSE_MASK) { + case IRQ_TYPE_LEVEL_HIGH: + gpio_rcar_config_interrupt_input_mode(p, hwirq, true, true); + break; + case IRQ_TYPE_LEVEL_LOW: + gpio_rcar_config_interrupt_input_mode(p, hwirq, false, true); + break; + case IRQ_TYPE_EDGE_RISING: + gpio_rcar_config_interrupt_input_mode(p, hwirq, true, false); + break; + case IRQ_TYPE_EDGE_FALLING: + gpio_rcar_config_interrupt_input_mode(p, hwirq, false, false); + break; + default: + return -EINVAL; + } + return 0;
Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +6
-0+} + +static irqreturn_t gpio_rcar_irq_handler(int irq, void *dev_id) +{ + struct gpio_rcar_priv *p = dev_id; + unsigned long pending;I would use u32 here as the register is 32-bits wide.
Ok, that's fine with me too.
Quoted 68 lines
Quoted 56 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +18
-0+ unsigned int offset, irqs_handled = 0; + + while ((pending = gpio_rcar_read(p, INTDT))) { + offset = __ffs(pending); + gpio_rcar_write(p, INTCLR, BIT(offset)); + generic_handle_irq(irq_find_mapping(p->irq_domain, offset)); + irqs_handled++; + } + + return irqs_handled ? IRQ_HANDLED : IRQ_NONE; +} + +static inline struct gpio_rcar_priv *gpio_to_priv(struct gpio_chip *chip) +{ + return container_of(chip, struct gpio_rcar_priv, gpio_chip); +} + +static void gpio_rcar_config_general_input_output_mode(struct gpio_chip*chip,
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +36
-0+ unsigned int gpio, + bool output) +{ + struct gpio_rcar_priv *p = gpio_to_priv(chip); + unsigned long bit = BIT(gpio); + unsigned long flags, tmp; + + /* follow steps in the GPIO documentation for + * "Setting General Output Mode" and + * "Setting General Input Mode" + */ + + spin_lock_irqsave(&p->lock, flags); + + /* Configure postive logic in POSNEG */ + tmp = gpio_rcar_read(p, POSNEG); + tmp &= ~bit; + gpio_rcar_write(p, POSNEG, tmp); + + /* Select "General Input/Output Mode" in IOINTSEL */ + tmp = gpio_rcar_read(p, IOINTSEL); + tmp &= ~bit; + gpio_rcar_write(p, IOINTSEL, tmp); + + /* Select Input Mode or Output Mode in INOUTSEL */ + tmp = gpio_rcar_read(p, INOUTSEL); + if (output) + tmp |= bit; + else + tmp &= ~bit; + gpio_rcar_write(p, INOUTSEL, tmp); + + spin_unlock_irqrestore(&p->lock, flags); +} + +static int gpio_rcar_direction_input(struct gpio_chip *chip, unsignedoffset)
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 9 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +9
-0+{ + gpio_rcar_config_general_input_output_mode(chip, offset, false); + return 0; +} + +static int gpio_rcar_get(struct gpio_chip *chip, unsigned offset) +{ + return (int)(gpio_rcar_read(gpio_to_priv(chip), INDT) & BIT(offset)); +}Isn't the get operation supposed to return 0 or 1 ? In that case
Quoted 3 lines
return !!(gpio_rcar_read(gpio_to_priv(chip), INDT) & BIT(offset));would be better.
I recall that for micro optimization purpose I believe the double negation is unnecessary for the GPIO ->get() operation.
Or perhaps things have changed?
Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
+static void
gpio_rcar_set(struct gpio_chip *chip, unsigned offset, int value)drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +4
-0+{ + struct gpio_rcar_priv *p = gpio_to_priv(chip); + unsigned long bit = BIT(offset); + unsigned long flags, tmp;I would declare both bit and tmp as u32.
I don't mind that myself either.
Quoted 31 lines
Quoted 17 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +13
-0+ spin_lock_irqsave(&p->lock, flags); + + tmp = gpio_rcar_read(p, OUTDT); + if (value) + tmp |= bit; + else + tmp &= ~bit; + gpio_rcar_write(p, OUTDT, tmp); + + spin_unlock_irqrestore(&p->lock, flags); +} + +static int gpio_rcar_direction_output(struct gpio_chip *chip, unsignedoffset,
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+ int value) +{ + /* write GPIO value to output before selecting output mode of pin */drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 10 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +10
-0+ gpio_rcar_set(chip, offset, value); + gpio_rcar_config_general_input_output_mode(chip, offset, true); + return 0; +} + +static int gpio_rcar_to_irq(struct gpio_chip *chip, unsigned offset) +{ + return irq_create_mapping(gpio_to_priv(chip)->irq_domain, offset); +} +I would personally move the
irq_chipoperations handlers here to group all irq_chip-related functions together, but that's up to you.
Quoted 9 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 7 lines
+static int gpio_rcar_irq_domain_map(struct
irq_domain*h, unsigned int virq,drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +5
-0+ irq_hw_number_t hw) +{ + struct gpio_rcar_priv *p = h->host_data; + + pr_debug("gpio: map hw irq = %d, virq = %d\n", (int)hw, virq);Maybe
dev_dbg()?
Good idea!
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 3 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+ + irq_set_chip_data(virq, h->host_data); + irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq);I'm not too familiar with the IRQ core. GPIO interrupts can be configured as either edge-trigerred or level-trigerred. Can
handle_level_irq()handle both ?
Good question. I assume so, what I've seen is that usually ->set_type() is configuring the hardware but handle_level_irq() is used regardless. If there is any driver anywhere that deals with this differently I'd be happy to hear. =)
Quoted 13 lines
Quoted 13 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +13
-0+ set_irq_flags(virq, IRQF_VALID); /* kill me now */ + return 0; +} + +static struct irq_domain_ops gpio_rcar_irq_domain_ops = { + .map = gpio_rcar_irq_domain_map, +}; + +static int gpio_rcar_probe(struct platform_device *pdev) +{ + struct gpio_rcar_config *pdata = pdev->dev.platform_data; + struct gpio_rcar_priv *p; + struct resource *io, *irq;
Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +6
-0+ struct gpio_chip *gpio_chip; + struct irq_chip *irq_chip; + const char *name = dev_name(&pdev->dev); + int ret; + + p = kzalloc(sizeof(*p), GFP_KERNEL);You can use
devm_kzalloc()to simplify error management.
Yep, devm is not bad.
Quoted 5 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 3 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+ if (!p) { + dev_err(&pdev->dev, "failed to allocate driver data\n"); + ret = -ENOMEM;You can return
-ENOMEM; directly here.
I prefer to have a single point of error return.
Quoted 7 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 5 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +5
-0+ goto err0; + } + + /* deal with driver instance configuration */ + if (pdata)Shouldn't you return an error if pdata = NULL ?
Platform data isn't required by this driver.
Quoted 7 lines
Quoted 1 line
+
memcpy(&p->config, pdata, sizeof(*pdata));p->config = *pdata;seems to be the preferred style nowadays. You could also store a pointer to a struct
gpio_rcar_configingpio_rcar_priv, but I suppose you have decided not to do so in prevision for DT support.
Yes because of upcoming DT support. And this way the platform data can be put in some special init section too. As for memcpy(), I like to explicitly use that compared over your suggestion. This way it is easy to see which code that shouldn't happen during hot path.
Quoted 13 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 10 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +10
-0+ p->pdev = pdev; + platform_set_drvdata(pdev, p); + spin_lock_init(&p->lock); + + io = platform_get_resource(pdev, IORESOURCE_MEM, 0); + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + + if (!io || !irq) { + dev_err(&pdev->dev, "missing IRQ or IOMEM\n"); + ret = -EINVAL;return -EINVAL;
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 4 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +4
-0+ goto err1; + } + + p->base = ioremap_nocache(io->start, resource_size(io));
devm_ioremap_nocache()
Yep, devm.
Quoted 31 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 3 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+ if (!p->base) { + dev_err(&pdev->dev, "failed to remap I/O memory\n"); + ret = -ENXIO;return -ENXIO;Quoted 25 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +25
-0+ goto err1; + } + + gpio_chip = &p->gpio_chip; + gpio_chip->direction_input = gpio_rcar_direction_input; + gpio_chip->get = gpio_rcar_get; + gpio_chip->direction_output = gpio_rcar_direction_output; + gpio_chip->set = gpio_rcar_set; + gpio_chip->to_irq = gpio_rcar_to_irq; + gpio_chip->label = name; + gpio_chip->owner = THIS_MODULE; + gpio_chip->base = p->config.gpio_base; + gpio_chip->ngpio = p->config.number_of_pins; + + irq_chip = &p->irq_chip; + irq_chip->name = name; + irq_chip->irq_mask = gpio_rcar_irq_disable; + irq_chip->irq_unmask = gpio_rcar_irq_enable; + irq_chip->irq_enable = gpio_rcar_irq_enable; + irq_chip->irq_disable = gpio_rcar_irq_disable; + irq_chip->irq_set_type = gpio_rcar_irq_set_type; + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE; + + p->irq_domain = irq_domain_add_simple(pdev->dev.of_node, + p->config.number_of_pins,
Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +6
-0+ p->config.irq_base, + &gpio_rcar_irq_domain_ops, p); + if (!p->irq_domain) { + ret = -ENXIO; + dev_err(&pdev->dev, "cannot initialize irq domain\n"); + goto err2;return
-ENXIO;
In case of devm, yes.
Quoted 5 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 3 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+ } + + if (request_irq(irq->start, gpio_rcar_irq_handler, 0, name, p)) {
devm_request_irq()
Yes, like the devm patch for gpio-em does it. =)
Quoted 8 lines
Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +8
-0+ dev_err(&pdev->dev, "failed to request IRQ\n"); + ret = -ENOENT; + goto err3; + } + + ret = gpiochip_add(gpio_chip); + if (ret) { + dev_err(&pdev->dev, "failed to add GPIO controller\n");
Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +6
-0+ goto err4; + } + + dev_info(&pdev->dev, "driving %d GPIOs\n", p->config.number_of_pins); + + /* warn in case of mismatch if irq base is specified */When does this happen ?
If happens if the user has requested a certain static IRQ base but this interrupts were already installed there by some other driver.
Quoted 7 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 4 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +4
-0+ if (p->config.irq_base) { + ret = irq_find_mapping(p->irq_domain, 0); + if (p->config.irq_base != ret) + dev_warn(&pdev->dev, "irq base mismatch (%d/%d)\n",
p->config.irq_baseis unsigned andirq_find_mapping()returns an unsigned. %u/%u would this be more appropriate.
Ok!
Quoted 10 lines
Quoted 10 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +10
-0+ p->config.irq_base, ret); + } + + return 0; + +err4: + free_irq(irq->start, p); +err3: + irq_domain_remove(p->irq_domain); +err2:
Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 5 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +5
-0+ iounmap(p->base); +err1: + kfree(p); +err0: + return ret;With the devm_* functions used above you will have a single error label to call
irq_domain_remove.
Right, that makes the code simpler, I agree. I actually had devm planned as an incremental feature.
So what is the preferred way to handle update of this driver? I intend to add devm and DT support anyway, and I want both of them to be incremental to avoid making back porting more difficult than absolutely necessary.
Shall I make a V2 with everything except DT and devm, doesn't that sound pretty straight forward?
Thanks,
/ magnus
Hi Magnus,
Quoted 6 lines · On Tuesday 12 March 2013 14:27:25 Magnus Damm wrote:
Quoted 3 lines · Laurent Pinchart
Hi Magnus,
Thank you for the patch.
Thanks for your review!
You're welcome.
Quoted 34 lines
Quoted 34 lines · On Tuesday 05 March 2013 09:32:19 Magnus Damm wrote:
Quoted 33 lines
From: Magnus Damm <[email protected]>
This patch implements a GPIO driver for the R-Car series of SoCs from Renesas. This driver is designed to be reusable between multiple SoCs that share the same basic building block, but so far it has only been used on R-Car H1 (r8a7779).
Each driver instance handles 32 GPIOs with individually maskable IRQs. The driver operates on a single I/O memory range and the 32 GPIOs are hooked up a single interrupt.
In the case of R-Car H1 either external IRQ pins or GPIOs with interrupts can be used for on-board interupts. For external IRQs 4 pins are supported, and in the case of GPIO there are 202 GPIOS as 202 interrupts hooked up via 6 driver instances and to the GIC and the Cortex-A9 Quad.
At this point this driver is interfacing as a regular platform device driver. In the future DT support will be submitted as an incremental feature patch.
Signed-off-by Magnus Damm <[email protected]>---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rcar.c | 406 +++++++++++++++++++++++++ include/linux/platform_data/gpio-rcar.h | 29 ++ 4 files changed, 442 insertions(+)drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +0
-0@@ -0,0 +1,406 @@
[snip]
Quoted 43 lines
Quoted 32 lines
Quoted 32 lines
+static void gpio_rcar_config_interrupt_input_mode(struct
gpio_rcar_priv
*p, + unsigned int hwirq,
+ boolactive_high_rising_edge, +
boollevel_trigger) +{drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +27
-0+ unsigned long bit = BIT(hwirq); + unsigned long flags, tmp; + + /* follow steps in the GPIO documentation for + * "Setting Edge-Sensitive Interrupt Input Mode" and + * "Setting Level-Sensitive Interrupt Input Mode" + */ + + spin_lock_irqsave(&p->lock, flags); + + /* Configure postive or negative logic in POSNEG */ + tmp = gpio_rcar_read(p, POSNEG); + if (active_high_rising_edge) + tmp &= ~bit; + else + tmp |= bit; + gpio_rcar_write(p, POSNEG, tmp); + + /* Configure edge or level trigger in EDGLEVEL */ + tmp = gpio_rcar_read(p, EDGLEVEL); + if (level_trigger) + tmp &= ~bit; + else + tmp |= bit; + gpio_rcar_write(p, EDGLEVEL, tmp); + + /* Select "Interrupt Input Mode" in IOINTSEL */Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +6
-0+ tmp = gpio_rcar_read(p, IOINTSEL); + tmp |= bit; + gpio_rcar_write(p, IOINTSEL, tmp); + + /* Write INTCLR in case of edge trigger */ + if (!level_trigger)Maybe you could do so unconditionally.
I could, but I wouldn't follow the recommended sequence in the data sheet then.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
+
gpio_rcar_write(p, INTCLR, bit);I suppose the idea here it to avoid spurious interrupts when switching to edge-triggered interrupt mode. If the interrupt was already enabled wouldn't it still be registered before you clear the flag ?
Maybe. Perhaps I shall use
IRQCHIP_SET_TYPE_MASKEDhere?
That's a good idea.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+ + spin_unlock_irqrestore(&p->lock, flags); +}
[snip]
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+static int gpio_rcar_get(struct gpio_chip *chip, unsigned offset) +{ + return (int)(gpio_rcar_read(gpio_to_priv(chip), INDT) &BIT(offset));
+}Isn't the get operation supposed to return 0 or 1 ? In that case
Quoted 8 lines
Quoted 3 lines
return !!(gpio_rcar_read(gpio_to_priv(chip), INDT) & BIT(offset));would be better.
I recall that for micro optimization purpose I believe the double negation is unnecessary for the GPIO ->get() operation.
Or perhaps things have changed?
I'm not sure to be honest, I was hoping you would know :-)
[snip]
Quoted 16 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +5
-0+static int gpio_rcar_probe(struct platform_device *pdev) +{ + struct gpio_rcar_config *pdata = pdev->dev.platform_data; + struct gpio_rcar_priv *p; + struct resource *io, *irq;Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +6
-0+ struct gpio_chip *gpio_chip; + struct irq_chip *irq_chip; + const char *name = dev_name(&pdev->dev); + int ret; + + p = kzalloc(sizeof(*p), GFP_KERNEL);You can use
devm_kzalloc()to simplify error management.Yep, devm is not bad.
Quoted 7 lines
Quoted 5 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 3 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+ if (!p) { + dev_err(&pdev->dev, "failed to allocate driver data\n"); + ret = -ENOMEM;You can return
-ENOMEM; directly here.I prefer to have a single point of error return.
So do I when there's any cleanup to be done in the error path, which isn't the case here. I'm OK if you keep the goto though.
Quoted 14 lines
Quoted 7 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 5 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +5
-0+ goto err0; + } + + /* deal with driver instance configuration */ + if (pdata)Shouldn't you return an error if pdata == NULL ?
Platform data isn't required by this driver.
Quoted 4 lines
Quoted 2 lines
+
memcpy(&p->config, pdata, sizeof(*pdata));p->config = *pdata;
Quoted 8 lines
Quoted 3 lines
seems to be the preferred style nowadays. You could also store a pointer to a struct
gpio_rcar_configingpio_rcar_priv, but I suppose you have decided not to do so in prevision for DT support.Yes because of upcoming DT support. And this way the platform data can be put in some special init section too. As for
memcpy(), I like to explicitly use that compared over your suggestion. This way it is easy to see which code that shouldn't happen during hot path.
I wasn't too fond of the structure assignment style to start with but changed my mind after receiving patches that removed memcpy() calls from across the whole kernel :-)
Quoted 123 lines
Quoted 13 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 11 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +10
-0+ p->pdev = pdev; + platform_set_drvdata(pdev, p); + spin_lock_init(&p->lock); + + io = platform_get_resource(pdev, IORESOURCE_MEM, 0); + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + + if (!io || !irq) { + dev_err(&pdev->dev, "missing IRQ or IOMEM\n"); + ret = -EINVAL;return -EINVAL;Quoted 1 line
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 4 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +4
-0+ goto err1; + } + + p->base = ioremap_nocache(io->start, resource_size(io));
devm_ioremap_nocache()Yep, devm.
Quoted 32 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 4 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+ if (!p->base) { + dev_err(&pdev->dev, "failed to remap I/O memory\n"); + ret = -ENXIO;return -ENXIO;Quoted 27 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +26
-0+ goto err1; + } + + gpio_chip = &p->gpio_chip; + gpio_chip->direction_input = gpio_rcar_direction_input; + gpio_chip->get = gpio_rcar_get; + gpio_chip->direction_output = gpio_rcar_direction_output; + gpio_chip->set = gpio_rcar_set; + gpio_chip->to_irq = gpio_rcar_to_irq; + gpio_chip->label = name; + gpio_chip->owner = THIS_MODULE; + gpio_chip->base = p->config.gpio_base; + gpio_chip->ngpio = p->config.number_of_pins; + + irq_chip = &p->irq_chip; + irq_chip->name = name; + irq_chip->irq_mask = gpio_rcar_irq_disable; + irq_chip->irq_unmask = gpio_rcar_irq_enable; + irq_chip->irq_enable = gpio_rcar_irq_enable; + irq_chip->irq_disable = gpio_rcar_irq_disable; + irq_chip->irq_set_type = gpio_rcar_irq_set_type; + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE; + + p->irq_domain = irq_domain_add_simple(pdev->dev.of_node, + p->config.number_of_pins, + p->config.irq_base,Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
+
&gpio_rcar_irq_domain_ops,
p);drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +4
-0+ if (!p->irq_domain) { + ret = -ENXIO; + dev_err(&pdev->dev, "cannot initialize irq domain\n"); + goto err2;return
-ENXIO;In case of devm, yes.
Quoted 5 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 3 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+ } + + if (request_irq(irq->start, gpio_rcar_irq_handler, 0, name, p)) {
devm_request_irq()Yes, like the devm patch for gpio-em does it. =)
Quoted 9 lines
Quoted 9 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +9
-0+ dev_err(&pdev->dev, "failed to request IRQ\n"); + ret = -ENOENT; + goto err3; + } + + ret = gpiochip_add(gpio_chip); + if (ret) { + dev_err(&pdev->dev, "failed to add GPIO controller\n"); + goto err4;Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +3
-0+ } + + dev_info(&pdev->dev, "driving %d GPIOs\n",
p->config.number_of_pins);drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +2
-0+ + /* warn in case of mismatch if irq base is specified */When does this happen ?
If happens if the user has requested a certain static IRQ base but this interrupts were already installed there by some other driver.
Quoted 7 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 4 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +4
-0+ if (p->config.irq_base) { + ret = irq_find_mapping(p->irq_domain, 0); + if (p->config.irq_base != ret) + dev_warn(&pdev->dev, "irq base mismatch (%d/%d)\n",
p->config.irq_baseis unsigned andirq_find_mapping()returns an unsigned. %u/%u would this be more appropriate.Ok!
Quoted 10 lines
Quoted 10 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +10
-0+ p->config.irq_base, ret); + } + + return 0; + +err4: + free_irq(irq->start, p); +err3: + irq_domain_remove(p->irq_domain); +err2:Quoted 8 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900
Quoted 5 lines
drivers/gpio/gpio-rcar.c 2013-03-05 09:13:23.000000000 +0900 new file +5
-0+ iounmap(p->base); +err1: + kfree(p); +err0: + return ret;With the devm_* functions used above you will have a single error label to call
irq_domain_remove.
Quoted 6 lines
Right, that makes the code simpler, I agree. I actually had devm planned as an incremental feature.
So what is the preferred way to handle update of this driver? I intend to add devm and DT support anyway, and I want both of them to be incremental to avoid making back porting more difficult than absolutely necessary.
How far back do you need to backport the driver ? devm_kzalloc was introduced in 2.6.21, devm_ioremap_nocache in 2.6.26 and devm_request_irq in 2.6.30. That gives you nearly 4 years of backporting, I hope it would be enough :-)
Quoted 2 lines
Shall I make a V2 with everything except DT and devm, doesn't that sound pretty straight forward?
Could you submit a v2 with all the changes except DT ?
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 10 lines · Magnus Damm
drivers/gpio/gpio-rcar.c new file +10
-0+static int gpio_rcar_irq_set_type (struct irq_data *d,unsigned int type) +{ +struct gpio_rcar_priv *p =irq_data_get_irq_chip_data (d); +unsigned int hwirq =irqd_to_hwirq (d); + +pr_debug ("gpio: sense irq = %d, type = %d\n" , hwirq, type); + +switch (type &IRQ_TYPE_SENSE_MASK ) { +case IRQ_TYPE_LEVEL_HIGH : +gpio_rcar_config_interrupt_input_mode (p, hwirq, true, true);
drivers/gpio/gpio-rcar.c line 144
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+break ; +case IRQ_TYPE_LEVEL_LOW : +gpio_rcar_config_interrupt_input_mode (p, hwirq, false, true); +break ; +case IRQ_TYPE_EDGE_RISING : +gpio_rcar_config_interrupt_input_mode (p, hwirq, true, false); +break ; +case IRQ_TYPE_EDGE_FALLING : +gpio_rcar_config_interrupt_input_mode (p, hwirq, false, false); +break ;
It is quite common to request IRQ on *both* rising and falling edges, what happens then?
See <linux/irqs.h>
IRQ_TYPE_EDGE_RISING = 0x00000001,
IRQ_TYPE_EDGE_FALLING = 0x00000002,
IRQ_TYPE_EDGE_BOTH = (IRQ_TYPE_EDGE_FALLING |IRQ_TYPE_EDGE_RISING),
I guess then you get just rising edge...
drivers/gpio/gpio-rcar.c line 149
Quoted 5 lines
drivers/gpio/gpio-rcar.c new file +5
-0+default : +return -EINVAL ; + } +return 0 ; +}
(...)
The rest I think is already covered by Laurent's comments.
Yours,
Linus Walleij
Magnus Damm
maintainerHi Linus,
Quoted 26 lines · Linus Walleij
Quoted 10 lines · Magnus Damm
drivers/gpio/gpio-rcar.c new file +10
-0+static int gpio_rcar_irq_set_type (struct irq_data *d,unsigned int type) +{ +struct gpio_rcar_priv *p =irq_data_get_irq_chip_data (d); +unsigned int hwirq =irqd_to_hwirq (d); + +pr_debug ("gpio: sense irq = %d, type = %d\n" , hwirq, type); + +switch (type &IRQ_TYPE_SENSE_MASK ) { +case IRQ_TYPE_LEVEL_HIGH : +gpio_rcar_config_interrupt_input_mode (p, hwirq, true, true);drivers/gpio/gpio-rcar.c line 144
Quoted 10 lines
drivers/gpio/gpio-rcar.c new file +10
-0+break ; +case IRQ_TYPE_LEVEL_LOW : +gpio_rcar_config_interrupt_input_mode (p, hwirq, false, true); +break ; +case IRQ_TYPE_EDGE_RISING : +gpio_rcar_config_interrupt_input_mode (p, hwirq, true, false); +break ; +case IRQ_TYPE_EDGE_FALLING : +gpio_rcar_config_interrupt_input_mode (p, hwirq, false, false); +break ;It is quite common to request IRQ on *both* rising and falling edges, what happens then?
Quoted 7 lines
See <
linux/irqs.h>IRQ_TYPE_EDGE_RISING = 0x00000001, IRQ_TYPE_EDGE_FALLING = 0x00000002, IRQ_TYPE_EDGE_BOTH = (IRQ_TYPE_EDGE_FALLING |
IRQ_TYPE_EDGE_RISING),I guess then you get just rising edge...
I may be wrong, but the switch above does AND with IRQ_TYPE_SENSE_MASK which makes me believe that the BOTH case will result in -EINVAL.
The -EINVAL is IMO correct here since the most basic version of this GPIO block (used in r8a7779) does not suport BOTH edges. I've noticed that newer versions of that GPIO hardware block has additional registers where this can be selected. My plan is to submit an incremental patch for such feature in the not so distant future.
Quoted 1 line
The rest I think is already covered by Laurent's comments.
Ok, thanks for your help!
/ magnus
Quoted 4 lines · Magnus Damm
Quoted 2 lines · Linus Walleij
It is quite common to request IRQ on *both* rising and falling edges, what happens then?
(...)
Quoted 2 lines
I may be wrong, but the switch above does AND with
IRQ_TYPE_SENSE_MASKwhich makes me believe that the BOTH case will result in-EINVAL.
Yes you're right, I wasn't quite following. The code only allows for one distinct flag, fine!
Yours,
Linus Walleij