[PATCH 0/4] vf610: Add GPIO support

[PATCH 0/4] vf610: Add GPIO support

[Stefan Agner]

This patchset adds GPIO support for Vybrid. The first patch is a
preparation patch which makes sure we can detect whether a pin is
initialized by the pinmux subsystem or not. This is required since
the gpio_request_enable/gpio_set_direction function need to know
if the pins mux register offsets are valid or not.

To get valid mux register offset for a pin, we need to have a pinmux
group configuration in the device tree. For the SD card detection pin
this is within the esdhc pingroup. Since the pingroup is used by the
driver, the mux and configuration register gets applied by the pinmux
subsystem.

However, the group does not need to be used anywhere, just the
configuration itself makes sure the mux register offset is known to
the pinmux driver. If a GPIO is requested then, the SoC's default pin
config is still in place. Is this the expected behaviour of a pinmux
driver? How can I make sure that a GPIO only pin (used by user-space
for instance) get a valid pin configuration applied?

To get the mux register offset, I also thought about calculating the
value in those two functions instead, however this seems not to be
consistent with the rest of the driver.

Other then that, the GPIO/IRQ chip driver is quite straight forward
and makes use of CONFIG_GPIOLIB_IRQCHIP. The only thing which is a
bit weird: There is no actual mask register for the GPIO interrupts.
There is only a configuration register, which, if configured, enables
the interrupt. Since the mask/unmask functions are requierd, the
driver writes/clears the configuration register in the mask/umask
functions.

Stefan Agner (4):
  pinctrl: imx: detect uninitialized pins
  pinctrl: imx: add gpio pinmux support for vf610
  gpio: vf610: add gpiolib/IRQ chip driver for Vybird
  ARM: dts: vf610: Use new GPIO support

 arch/arm/boot/dts/vf610-colibri.dtsi |   8 +
 arch/arm/boot/dts/vf610-twr.dts      |   1 +
 arch/arm/boot/dts/vf610.dtsi         |   1 +
 drivers/gpio/Kconfig                 |   7 +
 drivers/gpio/Makefile                |   1 +
 drivers/gpio/gpio-vf610.c            | 285 +++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-imx.c        |  63 +++++++-
 drivers/pinctrl/pinctrl-imx.h        |   8 +-
 drivers/pinctrl/pinctrl-vf610.c      |   4 +-
 9 files changed, 368 insertions(+), 10 deletions(-)
 create mode 100644 drivers/gpio/gpio-vf610.c

-- 
2.1.0

[PATCH 1/4] pinctrl: imx: detect uninitialized pins

[Stefan Agner]

The pinctrl driver initialized the register offsets for the pins
with 0. On Vybrid an offset of 0 is a valid offset for the pinctrl
mux register. So far, this was solved using the ZERO_OFFSET_VALID
flag which allowed offsets of 0. However, this does not allow to
verify whether a pins struct imx_pmx_func was initialized or not.

Use signed offset values for register offsets and initialize those
with -1 in order to detect uninitialized offset values reliable.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/pinctrl/pinctrl-imx.c   | 9 +++++----
 drivers/pinctrl/pinctrl-imx.h   | 7 +++----
 drivers/pinctrl/pinctrl-vf610.c | 2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c
index 946d594..0d4558b 100644
--- a/drivers/pinctrl/pinctrl-imx.c
+++ b/drivers/pinctrl/pinctrl-imx.c
@@ -204,7 +204,7 @@ static int imx_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
 		pin_id = pin->pin;
 		pin_reg = &info->pin_regs[pin_id];
 
-		if (!(info->flags & ZERO_OFFSET_VALID) && !pin_reg->mux_reg) {
+		if (pin_reg->mux_reg == -1) {
 			dev_err(ipctl->dev, "Pin(%s) does not support mux function\n",
 				info->pins[pin_id].name);
 			return -EINVAL;
@@ -308,7 +308,7 @@ static int imx_pinconf_get(struct pinctrl_dev *pctldev,
 	const struct imx_pinctrl_soc_info *info = ipctl->info;
 	const struct imx_pin_reg *pin_reg = &info->pin_regs[pin_id];
 
-	if (!(info->flags & ZERO_OFFSET_VALID) && !pin_reg->conf_reg) {
+	if (pin_reg->conf_reg == -1) {
 		dev_err(info->dev, "Pin(%s) does not support config function\n",
 			info->pins[pin_id].name);
 		return -EINVAL;
@@ -331,7 +331,7 @@ static int imx_pinconf_set(struct pinctrl_dev *pctldev,
 	const struct imx_pin_reg *pin_reg = &info->pin_regs[pin_id];
 	int i;
 
-	if (!(info->flags & ZERO_OFFSET_VALID) && !pin_reg->conf_reg) {
+	if (pin_reg->conf_reg == -1) {
 		dev_err(info->dev, "Pin(%s) does not support config function\n",
 			info->pins[pin_id].name);
 		return -EINVAL;
@@ -586,10 +586,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
 	if (!ipctl)
 		return -ENOMEM;
 
-	info->pin_regs = devm_kzalloc(&pdev->dev, sizeof(*info->pin_regs) *
+	info->pin_regs = devm_kmalloc(&pdev->dev, sizeof(*info->pin_regs) *
 				      info->npins, GFP_KERNEL);
 	if (!info->pin_regs)
 		return -ENOMEM;
+	memset(info->pin_regs, 0xff, sizeof(*info->pin_regs) * info->npins);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ipctl->base = devm_ioremap_resource(&pdev->dev, res);
diff --git a/drivers/pinctrl/pinctrl-imx.h b/drivers/pinctrl/pinctrl-imx.h
index db408b0..49e55d3 100644
--- a/drivers/pinctrl/pinctrl-imx.h
+++ b/drivers/pinctrl/pinctrl-imx.h
@@ -67,8 +67,8 @@ struct imx_pmx_func {
  * @conf_reg: config register offset
  */
 struct imx_pin_reg {
-	u16 mux_reg;
-	u16 conf_reg;
+	s16 mux_reg;
+	s16 conf_reg;
 };
 
 struct imx_pinctrl_soc_info {
@@ -83,8 +83,7 @@ struct imx_pinctrl_soc_info {
 	unsigned int flags;
 };
 
-#define ZERO_OFFSET_VALID	0x1
-#define SHARE_MUX_CONF_REG	0x2
+#define SHARE_MUX_CONF_REG	0x1
 
 #define NO_MUX		0x0
 #define NO_PAD		0x0
diff --git a/drivers/pinctrl/pinctrl-vf610.c b/drivers/pinctrl/pinctrl-vf610.c
index bddd913..b788e15 100644
--- a/drivers/pinctrl/pinctrl-vf610.c
+++ b/drivers/pinctrl/pinctrl-vf610.c
@@ -299,7 +299,7 @@ static const struct pinctrl_pin_desc vf610_pinctrl_pads[] = {
 static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
 	.pins = vf610_pinctrl_pads,
 	.npins = ARRAY_SIZE(vf610_pinctrl_pads),
-	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
+	.flags = SHARE_MUX_CONF_REG,
 };
 
 static struct of_device_id vf610_pinctrl_of_match[] = {
-- 
2.1.0

Re: [PATCH 1/4] pinctrl: imx: detect uninitialized pins

[Linus Walleij]

On Sat, Sep 6, 2014 at 6:25 PM, Stefan Agner <stefan@agner.ch> wrote:

> The pinctrl driver initialized the register offsets for the pins
> with 0. On Vybrid an offset of 0 is a valid offset for the pinctrl
> mux register. So far, this was solved using the ZERO_OFFSET_VALID
> flag which allowed offsets of 0. However, this does not allow to
> verify whether a pins struct imx_pmx_func was initialized or not.
>
> Use signed offset values for register offsets and initialize those
> with -1 in order to detect uninitialized offset values reliable.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>

Patch applied.

Yours,
Linus Walleij

[PATCH 2/4] pinctrl: imx: add gpio pinmux support for vf610

[Stefan Agner]

Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller.
This is needed since direction configuration is not part of the
GPIO module in Vybrid.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/pinctrl/pinctrl-imx.c   | 54 +++++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-imx.h   |  1 +
 drivers/pinctrl/pinctrl-vf610.c |  4 +--
 3 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c
index 0d4558b..64d1b59 100644
--- a/drivers/pinctrl/pinctrl-imx.c
+++ b/drivers/pinctrl/pinctrl-imx.c
@@ -294,10 +294,59 @@ static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
 	return 0;
 }
 
+static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct imx_pinctrl_soc_info *info = ipctl->info;
+	const struct imx_pin_reg *pin_reg;
+	u32 reg;
+
+	if (!(info->flags & GPIO_CONTROL))
+		return -EINVAL;
+
+	pin_reg = &info->pin_regs[offset];
+	if (pin_reg->mux_reg == -1)
+		return -EINVAL;
+
+	reg = readl(ipctl->base + pin_reg->mux_reg);
+	reg &= ~(0x7 << 20);
+	writel(reg, ipctl->base + pin_reg->mux_reg);
+
+	return 0;
+}
+
+static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+	   struct pinctrl_gpio_range *range, unsigned offset, bool input)
+{
+	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct imx_pinctrl_soc_info *info = ipctl->info;
+	const struct imx_pin_reg *pin_reg;
+	u32 reg;
+
+	if (!(info->flags & GPIO_CONTROL))
+		return -EINVAL;
+
+	pin_reg = &info->pin_regs[offset];
+	if (pin_reg->mux_reg == -1)
+		return -EINVAL;
+
+	reg = readl(ipctl->base + pin_reg->mux_reg);
+	if (input)
+		reg &= ~0x2;
+	else
+		reg |= 0x2;
+	writel(reg, ipctl->base + pin_reg->mux_reg);
+
+	return 0;
+}
+
 static const struct pinmux_ops imx_pmx_ops = {
 	.get_functions_count = imx_pmx_get_funcs_count,
 	.get_function_name = imx_pmx_get_func_name,
 	.get_function_groups = imx_pmx_get_groups,
+	.gpio_request_enable = imx_pmx_gpio_request_enable,
+	.gpio_set_direction = imx_pmx_gpio_set_direction,
 	.enable = imx_pmx_enable,
 };
 
@@ -579,6 +628,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
 		dev_err(&pdev->dev, "wrong pinctrl info\n");
 		return -EINVAL;
 	}
+
+	/* GPIO control functions only intended for shared mux/conf register */
+	if (info->flags & GPIO_CONTROL)
+		BUG_ON(!(info->flags & SHARE_MUX_CONF_REG));
+
 	info->dev = &pdev->dev;
 
 	/* Create state holders etc for this driver */
diff --git a/drivers/pinctrl/pinctrl-imx.h b/drivers/pinctrl/pinctrl-imx.h
index 49e55d3..8f37ca4 100644
--- a/drivers/pinctrl/pinctrl-imx.h
+++ b/drivers/pinctrl/pinctrl-imx.h
@@ -84,6 +84,7 @@ struct imx_pinctrl_soc_info {
 };
 
 #define SHARE_MUX_CONF_REG	0x1
+#define GPIO_CONTROL		0x2
 
 #define NO_MUX		0x0
 #define NO_PAD		0x0
diff --git a/drivers/pinctrl/pinctrl-vf610.c b/drivers/pinctrl/pinctrl-vf610.c
index b788e15..d34a3ac 100644
--- a/drivers/pinctrl/pinctrl-vf610.c
+++ b/drivers/pinctrl/pinctrl-vf610.c
@@ -299,7 +299,7 @@ static const struct pinctrl_pin_desc vf610_pinctrl_pads[] = {
 static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
 	.pins = vf610_pinctrl_pads,
 	.npins = ARRAY_SIZE(vf610_pinctrl_pads),
-	.flags = SHARE_MUX_CONF_REG,
+	.flags = SHARE_MUX_CONF_REG | GPIO_CONTROL,
 };
 
 static struct of_device_id vf610_pinctrl_of_match[] = {
@@ -326,7 +326,7 @@ static int __init vf610_pinctrl_init(void)
 {
 	return platform_driver_register(&vf610_pinctrl_driver);
 }
-arch_initcall(vf610_pinctrl_init);
+postcore_initcall(vf610_pinctrl_init);
 
 static void __exit vf610_pinctrl_exit(void)
 {
-- 
2.1.0

Re: [PATCH 2/4] pinctrl: imx: add gpio pinmux support for vf610

[Linus Walleij]

On Sat, Sep 6, 2014 at 6:25 PM, Stefan Agner <stefan@agner.ch> wrote:

> Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller.
> This is needed since direction configuration is not part of the
> GPIO module in Vybrid.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
(...)

> -arch_initcall(vf610_pinctrl_init);
> +postcore_initcall(vf610_pinctrl_init);

Why is this necessary? You should be able to rely on deferred
probing to do its work here. I think this should be module_init()
or driver_initcall() really.

Yours,
Linus Walleij

Re: [PATCH 2/4] pinctrl: imx: add gpio pinmux support for vf610

[Stefan Agner]

Am 2014-09-23 11:48, schrieb Linus Walleij:
> On Sat, Sep 6, 2014 at 6:25 PM, Stefan Agner <stefan@agner.ch> wrote:
> 
>> Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller.
>> This is needed since direction configuration is not part of the
>> GPIO module in Vybrid.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
> (...)
> 
>> -arch_initcall(vf610_pinctrl_init);
>> +postcore_initcall(vf610_pinctrl_init);
> 
> Why is this necessary? You should be able to rely on deferred
> probing to do its work here. I think this should be module_init()
> or driver_initcall() really.

Currently deferred probe doesn't work for gpiolib drivers which try to
add gpio-ranges from device tree:
gpiochip_add calls of_gpiochip_add_pin_range (through of_gpiochip_add).
This function tries to get the pinctrl driver, which is not registred at
that time. Currently the driver does not defer probing but fails
silently...

We would need to alter the return values of those two functions
(of_gpiochip_add_pin_range/of_gpiochip_add) and honor the return value
in gpiochip_add. 

Currently, it seems that we quite often use an early initcall to get the
pinctrl loaded early:
$ grep -h -o ".*_initcall" drivers/pinctrl/*.c | sort | uniq -c
     18 arch_initcall
      3 core_initcall
      3 postcore_initcall
      1 subsys_initcall

IMHO for those core services, it also makes sense to have them
initialized early. I'd rather prefer this hard coded than having dozens
of "requests probe deferral" messages...

--
Stefan

[PATCH 3/4] gpio: vf610: add gpiolib/IRQ chip driver for Vybird

[Stefan Agner]

Add a gpiolib and IRQ chip driver for Vybrid ARM SoC using the
Vybrid's GPIO and PORT module. The driver is instanced once per
each GPIO/PORT module pair and handles 32 GPIO's.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpio/Kconfig      |   7 ++
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/gpio-vf610.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 293 insertions(+)
 create mode 100644 drivers/gpio/gpio-vf610.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515..82b38f5 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -334,6 +334,13 @@ config GPIO_TZ1090_PDC
 	help
 	  Say yes here to support Toumaz Xenif TZ1090 PDC GPIOs.
 
+config GPIO_VF610
+	def_bool y
+	depends on ARCH_MXC && SOC_VF610
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support Vybrid vf610 GPIOs.
+
 config GPIO_XILINX
 	bool "Xilinx GPIO support"
 	depends on PPC_OF || MICROBLAZE || ARCH_ZYNQ
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 5d024e3..9893d4c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_GPIO_TWL6040)	+= gpio-twl6040.o
 obj-$(CONFIG_GPIO_TZ1090)	+= gpio-tz1090.o
 obj-$(CONFIG_GPIO_TZ1090_PDC)	+= gpio-tz1090-pdc.o
 obj-$(CONFIG_GPIO_UCB1400)	+= gpio-ucb1400.o
+obj-$(CONFIG_GPIO_VF610)	+= gpio-vf610.o
 obj-$(CONFIG_GPIO_VIPERBOARD)	+= gpio-viperboard.o
 obj-$(CONFIG_GPIO_VR41XX)	+= gpio-vr41xx.o
 obj-$(CONFIG_GPIO_VX855)	+= gpio-vx855.o
diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
new file mode 100644
index 0000000..5f59424
--- /dev/null
+++ b/drivers/gpio/gpio-vf610.c
@@ -0,0 +1,285 @@
+/*
+ * vf610 GPIO support through PORT and GPIO module
+ *
+ * Copyright (c) 2014 Toradex AG.
+ *
+ * Author: Stefan Agner <stefan@agner.ch>.
+ *
+ * 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, or (at your option) any later version.
+ * 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.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/gpio.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <asm-generic/bug.h>
+
+
+#define GPIO_PER_PORT		32
+
+struct vf610_gpio_port {
+	struct gpio_chip gc;
+	void __iomem *base;
+	void __iomem *gpio_base;
+	u8 irqc[GPIO_PER_PORT];
+	int irq;
+};
+
+#define GPIO_PDOR		0x00
+#define GPIO_PSOR		0x04
+#define GPIO_PCOR		0x08
+#define GPIO_PTOR		0x0c
+#define GPIO_PDIR		0x10
+
+#define PORT_PCR(n)		(n * 0x4)
+#define PORT_PCR_IRQC_OFFSET	16
+
+#define PORT_ISFR		0xa0
+#define PORT_DFER		0xc0
+#define PORT_DFCR		0xc4
+#define PORT_DFWR		0xc8
+
+#define PORT_INT_OFF		0x0
+#define PORT_INT_LOGIC_ZERO	0x8
+#define PORT_INT_RISING_EDGE	0x9
+#define PORT_INT_FALLING_EDGE	0xa
+#define PORT_INT_EITHER_EDGE	0xb
+#define PORT_INT_LOGIC_ONE	0xc
+
+
+static const struct of_device_id vf610_gpio_dt_ids[] = {
+	{ .compatible = "fsl,vf610-gpio" },
+	{ /* sentinel */ }
+};
+
+static inline void vf610_gpio_writel(u32 val, void __iomem *reg)
+{
+	__raw_writel(val, reg);
+}
+
+static inline u32 vf610_gpio_readl(void __iomem *reg)
+{
+	return __raw_readl(reg);
+}
+
+static int vf610_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	return pinctrl_request_gpio(chip->base + offset);
+}
+
+static void vf610_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	pinctrl_free_gpio(chip->base + offset);
+}
+
+static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct vf610_gpio_port *port =
+		container_of(gc, struct vf610_gpio_port, gc);
+
+	return !!(vf610_gpio_readl(port->gpio_base + GPIO_PDIR) & 1 << gpio);
+}
+
+static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct vf610_gpio_port *port =
+		container_of(gc, struct vf610_gpio_port, gc);
+	unsigned long mask = 1 << gpio;
+
+	if (val)
+		vf610_gpio_writel(mask, port->gpio_base + GPIO_PSOR);
+	else
+		vf610_gpio_writel(mask, port->gpio_base + GPIO_PCOR);
+}
+
+static int vf610_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
+{
+	return pinctrl_gpio_direction_input(chip->base + gpio);
+}
+
+static int vf610_gpio_direction_output(struct gpio_chip *chip, unsigned gpio,
+				       int value)
+{
+	vf610_gpio_set(chip, gpio, value);
+
+	return pinctrl_gpio_direction_output(chip->base + gpio);
+}
+
+static void vf610_gpio_irq_handler(u32 irq, struct irq_desc *desc)
+{
+	struct vf610_gpio_port *port = irq_get_handler_data(irq);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int pin;
+	unsigned long irq_isfr;
+
+	chained_irq_enter(chip, desc);
+
+	irq_isfr = vf610_gpio_readl(port->base + PORT_ISFR);
+
+	for_each_set_bit(pin, &irq_isfr, GPIO_PER_PORT) {
+		vf610_gpio_writel(1 << pin, port->base + PORT_ISFR);
+
+		generic_handle_irq(irq_find_mapping(port->gc.irqdomain, pin));
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+
+static void vf610_gpio_irq_ack(struct irq_data *d)
+{
+	struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d);
+	int gpio = d->hwirq;
+
+	vf610_gpio_writel(1 << gpio, port->base + PORT_ISFR);
+}
+
+static int vf610_gpio_irq_set_type(struct irq_data *d, u32 type)
+{
+	struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d);
+	u8 irqc;
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		irqc = PORT_INT_RISING_EDGE;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		irqc = PORT_INT_FALLING_EDGE;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		irqc = PORT_INT_EITHER_EDGE;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		irqc = PORT_INT_LOGIC_ZERO;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		irqc = PORT_INT_LOGIC_ONE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	port->irqc[d->hwirq] = irqc;
+
+	return 0;
+}
+
+static void vf610_gpio_irq_mask(struct irq_data *d)
+{
+	struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d);
+	void __iomem *pcr_base = port->base + PORT_PCR(d->hwirq);
+
+	vf610_gpio_writel(0, pcr_base);
+}
+
+static void vf610_gpio_irq_unmask(struct irq_data *d)
+{
+	struct vf610_gpio_port *port = irq_data_get_irq_chip_data(d);
+	void __iomem *pcr_base = port->base + PORT_PCR(d->hwirq);
+
+	vf610_gpio_writel(port->irqc[d->hwirq] << PORT_PCR_IRQC_OFFSET,
+			  pcr_base);
+}
+
+static struct irq_chip vf610_gpio_irq_chip = {
+	.name		= "gpio-vf610",
+	.irq_ack	= vf610_gpio_irq_ack,
+	.irq_mask	= vf610_gpio_irq_mask,
+	.irq_unmask	= vf610_gpio_irq_unmask,
+	.irq_set_type	= vf610_gpio_irq_set_type,
+};
+
+static int vf610_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct vf610_gpio_port *port;
+	struct resource *iores;
+	struct gpio_chip *gc;
+	int ret;
+
+	port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	port->base = devm_ioremap_resource(dev, iores);
+	if (IS_ERR(port->base))
+		return PTR_ERR(port->base);
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	port->gpio_base = devm_ioremap_resource(dev, iores);
+	if (IS_ERR(port->gpio_base))
+		return PTR_ERR(port->gpio_base);
+
+	port->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	if (port->irq == NO_IRQ)
+		return -ENODEV;
+
+	gc = &port->gc;
+	gc->of_node = np;
+	gc->dev = dev;
+	gc->label = "vf610-gpio",
+	gc->ngpio = GPIO_PER_PORT,
+	gc->base = of_alias_get_id(np, "gpio") * GPIO_PER_PORT;
+
+	gc->request = vf610_gpio_request,
+	gc->free = vf610_gpio_free,
+	gc->direction_input = vf610_gpio_direction_input,
+	gc->get = vf610_gpio_get,
+	gc->direction_output = vf610_gpio_direction_output,
+	gc->set = vf610_gpio_set,
+
+	ret = gpiochip_add(gc);
+	if (ret < 0)
+		return ret;
+
+	/* Clear the interrupt status register for all GPIO's */
+	vf610_gpio_writel(~0, port->base + PORT_ISFR);
+
+	ret = gpiochip_irqchip_add(gc, &vf610_gpio_irq_chip, 0,
+				   handle_simple_irq, IRQ_TYPE_NONE);
+	if (ret) {
+		dev_err(dev, "failed to add irqchip\n");
+		gpiochip_remove(gc);
+		return ret;
+	}
+	gpiochip_set_chained_irqchip(gc, &vf610_gpio_irq_chip, port->irq,
+				     vf610_gpio_irq_handler);
+
+	return 0;
+}
+
+static struct platform_driver vf610_gpio_driver = {
+	.driver		= {
+		.name	= "gpio-vf610",
+		.owner	= THIS_MODULE,
+		.of_match_table = vf610_gpio_dt_ids,
+	},
+	.probe		= vf610_gpio_probe,
+};
+
+static int __init gpio_vf610_init(void)
+{
+	return platform_driver_register(&vf610_gpio_driver);
+}
+postcore_initcall(gpio_vf610_init);
+
+MODULE_AUTHOR("Stefan Agner <stefan@agner.ch>");
+MODULE_DESCRIPTION("Freescale VF610 GPIO");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

Re: [PATCH 3/4] gpio: vf610: add gpiolib/IRQ chip driver for Vybird

[Linus Walleij]

On Sat, Sep 6, 2014 at 6:25 PM, Stefan Agner <stefan@agner.ch> wrote:

> Add a gpiolib and IRQ chip driver for Vybrid ARM SoC using the
> Vybrid's GPIO and PORT module. The driver is instanced once per
> each GPIO/PORT module pair and handles 32 GPIO's.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
(...)
>  obj-$(CONFIG_GPIO_UCB1400)     += gpio-ucb1400.o
> +obj-$(CONFIG_GPIO_VF610)       += gpio-vf610.o

Some like to keep GPIOs tightly associated with a pin controller
in a file next to the pin controller.

I.e. in drivers/pinctrl/freescale/gpio-vf610.c

But this works too. Any preference?

> +#define GPIO_PER_PORT          32

Very generic define. VF610_GPIOS_PER_PORT?

> +struct vf610_gpio_port {
> +       struct gpio_chip gc;
> +       void __iomem *base;
> +       void __iomem *gpio_base;
> +       u8 irqc[GPIO_PER_PORT];
> +       int irq;

irq? Why do you need to keep this around?

> +static const struct of_device_id vf610_gpio_dt_ids[] = {
> +       { .compatible = "fsl,vf610-gpio" },
> +       { /* sentinel */ }
> +};
> +
> +static inline void vf610_gpio_writel(u32 val, void __iomem *reg)
> +{
> +       __raw_writel(val, reg);

Use writel_relaxed() instead unless you can explain why you want this.

(Same for all occurences.)

> +static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> +       struct vf610_gpio_port *port =
> +               container_of(gc, struct vf610_gpio_port, gc);
> +
> +       return !!(vf610_gpio_readl(port->gpio_base + GPIO_PDIR) & 1 << gpio);

#include <linux/bitops.h>

... & BIT(gpio)

> +static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> +       struct vf610_gpio_port *port =
> +               container_of(gc, struct vf610_gpio_port, gc);
> +       unsigned long mask = 1 << gpio;

= BIT(gpio);

> +static void vf610_gpio_irq_handler(u32 irq, struct irq_desc *desc)
> +{
> +       struct vf610_gpio_port *port = irq_get_handler_data(irq);
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +       int pin;
> +       unsigned long irq_isfr;
> +
> +       chained_irq_enter(chip, desc);
> +
> +       irq_isfr = vf610_gpio_readl(port->base + PORT_ISFR);
> +
> +       for_each_set_bit(pin, &irq_isfr, GPIO_PER_PORT) {
> +               vf610_gpio_writel(1 << pin, port->base + PORT_ISFR);

BIT(pin)

(etc)

> +       port->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +       if (port->irq == NO_IRQ)
> +               return -ENODEV;

Can't you just use a local int irq; variable for this?

> +static int __init gpio_vf610_init(void)
> +{
> +       return platform_driver_register(&vf610_gpio_driver);
> +}
> +postcore_initcall(gpio_vf610_init);

postcore again. I don't like this, can you get rid of it?

Overall the driver looks very nice except for these nitty gritty details.

Yours,
Linus Walleij

Re: [PATCH 3/4] gpio: vf610: add gpiolib/IRQ chip driver for Vybird

[Stefan Agner]

Hi Linus,

Thanks for your review!

Am 2014-09-23 11:58, schrieb Linus Walleij:
> On Sat, Sep 6, 2014 at 6:25 PM, Stefan Agner <stefan@agner.ch> wrote:
> 
>> Add a gpiolib and IRQ chip driver for Vybrid ARM SoC using the
>> Vybrid's GPIO and PORT module. The driver is instanced once per
>> each GPIO/PORT module pair and handles 32 GPIO's.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
> (...)
>>  obj-$(CONFIG_GPIO_UCB1400)     += gpio-ucb1400.o
>> +obj-$(CONFIG_GPIO_VF610)       += gpio-vf610.o
> 
> Some like to keep GPIOs tightly associated with a pin controller
> in a file next to the pin controller.
> 
> I.e. in drivers/pinctrl/freescale/gpio-vf610.c
> 
> But this works too. Any preference?

The other Freescale GPIO drivers (e.g. gpio-mxs.c/gpio-mxc.c) are
located under drivers/gpio/ hence I would prefer to leave them there,
even we use pinctrl here. Unless someone at Freescale has another
opinion on this?


> 
>> +#define GPIO_PER_PORT          32
> 
> Very generic define. VF610_GPIOS_PER_PORT?

Agreed

> 
>> +struct vf610_gpio_port {
>> +       struct gpio_chip gc;
>> +       void __iomem *base;
>> +       void __iomem *gpio_base;
>> +       u8 irqc[GPIO_PER_PORT];
>> +       int irq;
> 
> irq? Why do you need to keep this around?
> 
>> +static const struct of_device_id vf610_gpio_dt_ids[] = {
>> +       { .compatible = "fsl,vf610-gpio" },
>> +       { /* sentinel */ }
>> +};
>> +
>> +static inline void vf610_gpio_writel(u32 val, void __iomem *reg)
>> +{
>> +       __raw_writel(val, reg);
> 
> Use writel_relaxed() instead unless you can explain why you want this.
> 
> (Same for all occurences.)

Agreed, I have don't know why I used the __raw variant here. I think its
because copied this two stubs from gpio-tegra.c.

> 
>> +static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>> +{
>> +       struct vf610_gpio_port *port =
>> +               container_of(gc, struct vf610_gpio_port, gc);
>> +
>> +       return !!(vf610_gpio_readl(port->gpio_base + GPIO_PDIR) & 1 << gpio);
> 
> #include <linux/bitops.h>
> 
> ... & BIT(gpio)
> 
>> +static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>> +{
>> +       struct vf610_gpio_port *port =
>> +               container_of(gc, struct vf610_gpio_port, gc);
>> +       unsigned long mask = 1 << gpio;
> 
> = BIT(gpio);
> 
>> +static void vf610_gpio_irq_handler(u32 irq, struct irq_desc *desc)
>> +{
>> +       struct vf610_gpio_port *port = irq_get_handler_data(irq);
>> +       struct irq_chip *chip = irq_desc_get_chip(desc);
>> +       int pin;
>> +       unsigned long irq_isfr;
>> +
>> +       chained_irq_enter(chip, desc);
>> +
>> +       irq_isfr = vf610_gpio_readl(port->base + PORT_ISFR);
>> +
>> +       for_each_set_bit(pin, &irq_isfr, GPIO_PER_PORT) {
>> +               vf610_gpio_writel(1 << pin, port->base + PORT_ISFR);
> 
> BIT(pin)
> 
> (etc)

Ok, will replace those bit operations.

> 
>> +       port->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>> +       if (port->irq == NO_IRQ)
>> +               return -ENODEV;
> 
> Can't you just use a local int irq; variable for this?
> 
>> +static int __init gpio_vf610_init(void)
>> +{
>> +       return platform_driver_register(&vf610_gpio_driver);
>> +}
>> +postcore_initcall(gpio_vf610_init);
> 
> postcore again. I don't like this, can you get rid of it?

I guess we could load this driver easily a bit later. IMHO, since lots
of other driver use GPIO's, we should it load before all the drivers
gets loaded (before device_initcall).

Most GPIO driver do this, some statistic again:
$ grep -h -o ".*_initcall" drivers/gpio/*.c | sort | uniq -c | sort -n
-r
     33 subsys_initcall
     14 postcore_initcall
      2 device_initcall
      2 arch_initcall
      1 late_initcall
      1 core_initcall

My proposal:
Use subsys_initcall (which is called after arch_initcall) in this GPIO
driver and leave the pinctrl driver as arch_initcall. This way we are
absolutely sure that the GPIO driver gets loaded after the pinctrl and
also leave the pinctrl at its current value.

--
Stefan


> 
> Overall the driver looks very nice except for these nitty gritty details.
> 
> Yours,
> Linus Walleij

Re: [PATCH 3/4] gpio: vf610: add gpiolib/IRQ chip driver for Vybird

[Linus Walleij]

On Tue, Sep 23, 2014 at 1:51 PM, Stefan Agner <stefan@agner.ch> wrote:
> [Me]
>> postcore again. I don't like this, can you get rid of it?
>
> I guess we could load this driver easily a bit later. IMHO, since lots
> of other driver use GPIO's, we should it load before all the drivers
> gets loaded (before device_initcall).

Nope. We use deferred probing to control that today. Ideally
all drivers should be device_initcall() and deferred probe be used
to order things, not by playing around with initcalls.

> Most GPIO driver do this, some statistic again:
> $ grep -h -o ".*_initcall" drivers/gpio/*.c | sort | uniq -c | sort -n
> -r
>      33 subsys_initcall
>      14 postcore_initcall
>       2 device_initcall
>       2 arch_initcall
>       1 late_initcall
>       1 core_initcall

Yeah old legacy. There are patch attacks to get rid of this.

The reason we can't just change them is because sometimes
dependent drivers do not handle the errorpath very well can can't
defer cleanly.

With a new driver I expect deferred probe to be used.

Yours,
Linus Walleij

Re: [PATCH 3/4] gpio: vf610: add gpiolib/IRQ chip driver for Vybird

[Stefan Agner]

Am 2014-09-24 13:10, schrieb Linus Walleij:
> On Tue, Sep 23, 2014 at 1:51 PM, Stefan Agner <stefan@agner.ch> wrote:
>> [Me]
>>> postcore again. I don't like this, can you get rid of it?
>>
>> I guess we could load this driver easily a bit later. IMHO, since lots
>> of other driver use GPIO's, we should it load before all the drivers
>> gets loaded (before device_initcall).
> 
> Nope. We use deferred probing to control that today. Ideally
> all drivers should be device_initcall() and deferred probe be used
> to order things, not by playing around with initcalls.
> 

You can not "nope" my humble opinion, this is not possible, its write
protected! :-)

I fully understand the deferred probe approach, and I also think its
really a good approach for almost all drivers. The system by itself
makes sure the drivers are loaded in correct order. But if all drivers
use pinctrl, and the pinctrl happens to be the last initialized driver,
I first get 30 messages with probe deferred before the pinctrl driver
finally gets initialized. IMHO, this is a waste of resources... Giving
the system some hint what we need early (e.g. pinctrl or even GPIO
drivers) is just sensible. But maybe I miss something here...

Fact is, currently, without touching GPIO infrastructure code, the GPIO
driver can not be deferred when the pinctrl driver is missing. The
of_gpiochip_add_pin_range function does not handle the missing pinctrl
driver. Hence as of now, the pinctrl still needs an earlier initcall.
But anyway, currently the pinctrl driver is already using arch_initcall,
this patch set is no longer touching that.

>> Most GPIO driver do this, some statistic again:
>> $ grep -h -o ".*_initcall" drivers/gpio/*.c | sort | uniq -c | sort -n
>> -r
>>      33 subsys_initcall
>>      14 postcore_initcall
>>       2 device_initcall
>>       2 arch_initcall
>>       1 late_initcall
>>       1 core_initcall
> 
> Yeah old legacy. There are patch attacks to get rid of this.
> 
> The reason we can't just change them is because sometimes
> dependent drivers do not handle the errorpath very well can can't
> defer cleanly.
> 
> With a new driver I expect deferred probe to be used.
> 

Actually, the esdhc driver gets the cd-gpio directly and does not handle
EPROBE_DEFER properly, hence Vybrid would be affected too. But I
understand that we need to start somewhere. I will change the GPIO
driver to device_initcall and going to fix esdhc driver.

--
Stefan

[PATCH 4/4] ARM: dts: vf610: Use new GPIO support

[Stefan Agner]

Use GPIO support by adding SD card detection configuration and
GPIO pinmux for Colibri's standard GPIO pins.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/vf610-colibri.dtsi | 8 ++++++++
 arch/arm/boot/dts/vf610-twr.dts      | 1 +
 arch/arm/boot/dts/vf610.dtsi         | 1 +
 3 files changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/vf610-colibri.dtsi b/arch/arm/boot/dts/vf610-colibri.dtsi
index 0cd8343..afe7cdc 100644
--- a/arch/arm/boot/dts/vf610-colibri.dtsi
+++ b/arch/arm/boot/dts/vf610-colibri.dtsi
@@ -31,6 +31,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_esdhc1>;
 	bus-width = <4>;
+	cd-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
 };
 
 &fec1 {
@@ -71,6 +72,13 @@
 
 &iomuxc {
 	vf610-colibri {
+		pinctrl_gpio: gpios {
+			fsl,pins = <
+				VF610_PAD_PTD9__GPIO_88		0x219d
+				VF610_PAD_PTD10__GPIO_89	0x219d
+			>;
+		};
+
 		pinctrl_esdhc1: esdhc1grp {
 			fsl,pins = <
 				VF610_PAD_PTA24__ESDHC1_CLK	0x31ef
diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index 189b697..3fe8a8f 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -116,6 +116,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_esdhc1>;
 	bus-width = <4>;
+	cd-gpios = <&gpio5 6 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
 
diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index 4d2ec32..467c97e 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -11,6 +11,7 @@
 #include "vf610-pinfunc.h"
 #include <dt-bindings/clock/vf610-clock.h>
 #include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	aliases {
-- 
2.1.0

Re: [PATCH 4/4] ARM: dts: vf610: Use new GPIO support

[Linus Walleij]

On Sat, Sep 6, 2014 at 6:25 PM, Stefan Agner <stefan@agner.ch> wrote:

> Use GPIO support by adding SD card detection configuration and
> GPIO pinmux for Colibri's standard GPIO pins.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

I guess you need the rest to be merged first though?

Yours,
Linus Walleij

Re: [PATCH 0/4] vf610: Add GPIO support

[Linus Walleij]

On Sat, Sep 6, 2014 at 9:25 AM, Stefan Agner <stefan@agner.ch> wrote:

> This patchset adds GPIO support for Vybrid.

Can I get some review from the i.MX driver maintainers for this
patchset?

Yours,
Linus Walleij

Re: [PATCH 0/4] vf610: Add GPIO support

[Linus Walleij]

On Fri, Sep 19, 2014 at 7:36 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Sep 6, 2014 at 9:25 AM, Stefan Agner <stefan@agner.ch> wrote:
>
>> This patchset adds GPIO support for Vybrid.
>
> Can I get some review from the i.MX driver maintainers for this
> patchset?

I will now just apply the patches.

Yours,
Linus Walleij