[PATCH] gpio: Renesas R-Car GPIO driver

[PATCH] gpio: Renesas R-Car GPIO driver

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

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 <damm@opensource.se>
---

 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(+)

--- 0001/drivers/gpio/Kconfig
+++ work/drivers/gpio/Kconfig	2013-03-05 09:07:34.000000000 +0900
@@ -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
--- 0001/drivers/gpio/Makefile
+++ work/drivers/gpio/Makefile	2013-03-05 09:07:34.000000000 +0900
@@ -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
--- /dev/null
+++ work/drivers/gpio/gpio-rcar.c	2013-03-05 09:13:23.000000000 +0900
@@ -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  USA
+ */
+
+#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>
+#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>
+
+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
+#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
+
+static inline unsigned long gpio_rcar_read(struct gpio_rcar_priv *p, int offs)
+{
+	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);
+}
+
+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 */
+	tmp = gpio_rcar_read(p, IOINTSEL);
+	tmp |= bit;
+	gpio_rcar_write(p, IOINTSEL, tmp);
+
+	/* Write INTCLR in case of edge trigger */
+	if (!level_trigger)
+		gpio_rcar_write(p, INTCLR, bit);
+
+	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);
+
+	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;
+}
+
+static irqreturn_t gpio_rcar_irq_handler(int irq, void *dev_id)
+{
+	struct gpio_rcar_priv *p = dev_id;
+	unsigned long pending;
+	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,
+						       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, unsigned offset)
+{
+	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));
+}
+
+static void gpio_rcar_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct gpio_rcar_priv *p = gpio_to_priv(chip);
+	unsigned long bit = BIT(offset);
+	unsigned long flags, tmp;
+
+	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, unsigned offset,
+				      int value)
+{
+	/* write GPIO value to output before selecting output mode of pin */
+	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);
+}
+
+static int gpio_rcar_irq_domain_map(struct irq_domain *h, unsigned int virq,
+				 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);
+
+	irq_set_chip_data(virq, h->host_data);
+	irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq);
+	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;
+	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);
+	if (!p) {
+		dev_err(&pdev->dev, "failed to allocate driver data\n");
+		ret = -ENOMEM;
+		goto err0;
+	}
+
+	/* deal with driver instance configuration */
+	if (pdata)
+		memcpy(&p->config, pdata, sizeof(*pdata));
+
+	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;
+		goto err1;
+	}
+
+	p->base = ioremap_nocache(io->start, resource_size(io));
+	if (!p->base) {
+		dev_err(&pdev->dev, "failed to remap I/O memory\n");
+		ret = -ENXIO;
+		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,
+					      &gpio_rcar_irq_domain_ops, p);
+	if (!p->irq_domain) {
+		ret = -ENXIO;
+		dev_err(&pdev->dev, "cannot initialize irq domain\n");
+		goto err2;
+	}
+
+	if (request_irq(irq->start, gpio_rcar_irq_handler, 0, name, p)) {
+		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;
+	}
+
+	dev_info(&pdev->dev, "driving %d GPIOs\n", p->config.number_of_pins);
+
+	/* warn in case of mismatch if irq base is specified */
+	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, ret);
+	}
+
+	return 0;
+
+err4:
+	free_irq(irq->start, p);
+err3:
+	irq_domain_remove(p->irq_domain);
+err2:
+	iounmap(p->base);
+err1:
+	kfree(p);
+err0:
+	return ret;
+}
+
+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");
--- /dev/null
+++ work/include/linux/platform_data/gpio-rcar.h	2013-03-05 09: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
+ */
+
+#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__ */

Re: [PATCH] gpio: Renesas R-Car GPIO driver

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Tuesday 05 March 2013 09:32:19 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> 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 <damm@opensource.se>
> ---
> 
>  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(+)
> 
> --- 0001/drivers/gpio/Kconfig
> +++ work/drivers/gpio/Kconfig	2013-03-05 09:07:34.000000000 +0900
> @@ -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
> --- 0001/drivers/gpio/Makefile
> +++ work/drivers/gpio/Makefile	2013-03-05 09:07:34.000000000 +0900
> @@ -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
> --- /dev/null
> +++ work/drivers/gpio/gpio-rcar.c	2013-03-05 09:13:23.000000000 +0900
> @@ -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 
> USA + */

You can remove the last paragraph.

> +
> +#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>
> +#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 ?

> +
> +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
> +#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.

> +static inline unsigned long gpio_rcar_read(struct gpio_rcar_priv *p, int
> offs)
> +{
> +	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 ?

> +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 */
> +	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.

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

> +
> +	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() ?

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

> +	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,
> +						       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, unsigned
> offset)
> +{
> +	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.

> +static void gpio_rcar_set(struct gpio_chip *chip, unsigned offset, int
> value)
> +{
> +	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.

> +	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, unsigned
> offset,
> +				      int value)
> +{
> +	/* write GPIO value to output before selecting output mode of pin */
> +	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.

> +static int gpio_rcar_irq_domain_map(struct irq_domain *h, unsigned int
> virq,
> +				 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() ?

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

> +	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;
> +	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.

> +	if (!p) {
> +		dev_err(&pdev->dev, "failed to allocate driver data\n");
> +		ret = -ENOMEM;

You can return -ENOMEM; directly here.

> +		goto err0;
> +	}
> +
> +	/* deal with driver instance configuration */
> +	if (pdata)

Shouldn't you return an error if pdata == NULL ?

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

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

> +		goto err1;
> +	}
> +
> +	p->base = ioremap_nocache(io->start, resource_size(io));

devm_ioremap_nocache()

> +	if (!p->base) {
> +		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> +		ret = -ENXIO;

	return -ENXIO;

> +		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,
> +					      &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;

> +	}
> +
> +	if (request_irq(irq->start, gpio_rcar_irq_handler, 0, name, p)) {

devm_request_irq()

> +		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;
> +	}
> +
> +	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 (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.

> +				 p->config.irq_base, ret);
> +	}
> +
> +	return 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.

> +}
> +
> +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");
> --- /dev/null
> +++ work/include/linux/platform_data/gpio-rcar.h	2013-03-05
> 09: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 + */
> +
> +#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__ */

-- 
Regards,

Laurent Pinchart

Re: [PATCH] gpio: Renesas R-Car GPIO driver

[Magnus Damm]

HI Laurent,

On Sun, Mar 10, 2013 at 3:16 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.

Thanks for your review!

> On Tuesday 05 March 2013 09:32:19 Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> 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 <damm@opensource.se>
>> ---
>>
>>  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(+)
>>
>> --- 0001/drivers/gpio/Kconfig
>> +++ work/drivers/gpio/Kconfig 2013-03-05 09:07:34.000000000 +0900
>> @@ -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
>> --- 0001/drivers/gpio/Makefile
>> +++ work/drivers/gpio/Makefile        2013-03-05 09:07:34.000000000 +0900
>> @@ -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
>> --- /dev/null
>> +++ work/drivers/gpio/gpio-rcar.c     2013-03-05 09:13:23.000000000 +0900
>> @@ -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
>> USA + */
>
> You can remove the last paragraph.

Sure!

>> +
>> +#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>
>> +#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! =)

>> +
>> +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
>> +#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.
>
>> +static inline unsigned long gpio_rcar_read(struct gpio_rcar_priv *p, int
>> offs)
>> +{
>> +     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 ?

Good idea, I'll check and see if it reduces the code size.

>> +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 */
>> +     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.

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

>> +
>> +     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() ?

Sure, why not?

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

>> +     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,
>> +                                                    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, unsigned
>> offset)
>> +{
>> +     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.

I recall that for micro optimization purpose I believe the double
negation is unnecessary for the GPIO ->get() operation.

Or perhaps things have changed?

>> +static void gpio_rcar_set(struct gpio_chip *chip, unsigned offset, int
>> value)
>> +{
>> +     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.

>> +     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, unsigned
>> offset,
>> +                                   int value)
>> +{
>> +     /* write GPIO value to output before selecting output mode of pin */
>> +     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.
>
>> +static int gpio_rcar_irq_domain_map(struct irq_domain *h, unsigned int
>> virq,
>> +                              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!

>> +
>> +     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. =)

>> +     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;
>> +     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.

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

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

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

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.

>> +     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;
>
>> +             goto err1;
>> +     }
>> +
>> +     p->base = ioremap_nocache(io->start, resource_size(io));
>
> devm_ioremap_nocache()

Yep, devm.

>> +     if (!p->base) {
>> +             dev_err(&pdev->dev, "failed to remap I/O memory\n");
>> +             ret = -ENXIO;
>
>         return -ENXIO;
>
>> +             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,
>> +                                           &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.

>> +     }
>> +
>> +     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. =)

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

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

Ok!

>> +                              p->config.irq_base, ret);
>> +     }
>> +
>> +     return 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.

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

Re: [PATCH] gpio: Renesas R-Car GPIO driver

[Laurent Pinchart]

Hi Magnus,

On Tuesday 12 March 2013 14:27:25 Magnus Damm wrote:
> On Sun, Mar 10, 2013 at 3:16 AM, Laurent Pinchart wrote:
> > Hi Magnus,
> > 
> > Thank you for the patch.
> 
> Thanks for your review!

You're welcome.

> > On Tuesday 05 March 2013 09:32:19 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> 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 <damm@opensource.se>
> >> ---
> >> 
> >>  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(+)
> >> 
> >> --- /dev/null
> >> +++ work/drivers/gpio/gpio-rcar.c     2013-03-05 09:13:23.000000000 +0900
> >> @@ -0,0 +1,406 @@

[snip]

> >> +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 */
> >> +     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.
> >> +             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?

That's a good idea.

> >> +
> >> +     spin_unlock_irqrestore(&p->lock, flags);
> >> +}

[snip]

> >> +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.
> 
> 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]

> >> +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;
> >> +     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.
> 
> >> +     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.

> >> +             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.
> 
> >> +             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.
> 
> 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 :-)

> >> +     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;
> >> 
> >> +             goto err1;
> >> +     }
> >> +
> >> +     p->base = ioremap_nocache(io->start, resource_size(io));
> > 
> > devm_ioremap_nocache()
> 
> Yep, devm.
> 
> >> +     if (!p->base) {
> >> +             dev_err(&pdev->dev, "failed to remap I/O memory\n");
> >> +             ret = -ENXIO;
> >> 
> >         return -ENXIO;
> >> 
> >> +             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,
> >> +                                           &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.
> 
> >> +     }
> >> +
> >> +     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. =)
> 
> >> +             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;
> >> +     }
> >> +
> >> +     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.
> 
> >> +     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.
> 
> Ok!
> 
> >> +                              p->config.irq_base, ret);
> >> +     }
> >> +
> >> +     return 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.
> 
> 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 :-)

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH] gpio: Renesas R-Car GPIO driver

[Linus Walleij]

On Tue, Mar 5, 2013 at 1:32 AM, Magnus Damm <magnus.damm@gmail.com> wrote:

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


> +       default:
> +               return -EINVAL;
> +       }
> +       return 0;
> +}

(...)

The rest I think is already covered by Laurent's comments.

Yours,
Linus Walleij

Re: [PATCH] gpio: Renesas R-Car GPIO driver

[Magnus Damm]

Hi Linus,

On Thu, Mar 14, 2013 at 2:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Mar 5, 2013 at 1:32 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>
>> +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);
>> +               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...

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.

> The rest I think is already covered by Laurent's comments.

Ok, thanks for your help!

/ magnus

Re: [PATCH] gpio: Renesas R-Car GPIO driver

[Linus Walleij]

On Thu, Mar 14, 2013 at 5:07 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Thu, Mar 14, 2013 at 2:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> It is quite common to request IRQ on *both* rising and falling edges,
>> what happens then?
(...)
> 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.

Yes you're right, I wasn't quite following. The code only
allows for one distinct flag, fine!

Yours,
Linus Walleij