[PATCH] gpio: add TS-5500 DIO headers support

[PATCH] gpio: add TS-5500 DIO headers support

[Vivien Didelot]

The Technologic Systems TS-5500 platform provides 3 digital I/O headers:
DIO1, DIO2, and the LCD port, that may be used as a DIO header.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
---
 drivers/gpio/Kconfig                      |   8 +
 drivers/gpio/Makefile                     |   1 +
 drivers/gpio/gpio-ts5500.c                | 344 ++++++++++++++++++++++++++++++
 include/linux/platform_data/gpio-ts5500.h |  34 +++
 4 files changed, 387 insertions(+)
 create mode 100644 drivers/gpio/gpio-ts5500.c
 create mode 100644 include/linux/platform_data/gpio-ts5500.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ba7926f5..f8bf8e1 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -395,6 +395,14 @@ config GPIO_TPS65912
 	help
 	  This driver supports TPS65912 gpio chip
 
+config GPIO_TS5500
+	tristate "TS-5500 DIO Headers"
+	depends on TS5500
+	help
+	  This driver supports the 3 Digital I/O headers of the Technologic
+	  Systems TS-5500 platform: DIO1, DIO2, and the LCD port which may be
+	  used as a DIO header.
+
 config GPIO_TWL4030
 	tristate "TWL4030, TWL5030, and TPS659x0 GPIOs"
 	depends on TWL4030_CORE
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 153cace..48e8670 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_ARCH_DAVINCI_TNETV107X) += gpio-tnetv107x.o
 obj-$(CONFIG_GPIO_TPS6586X)	+= gpio-tps6586x.o
 obj-$(CONFIG_GPIO_TPS65910)	+= gpio-tps65910.o
 obj-$(CONFIG_GPIO_TPS65912)	+= gpio-tps65912.o
+obj-$(CONFIG_GPIO_TS5500)	+= gpio-ts5500.o
 obj-$(CONFIG_GPIO_TWL4030)	+= gpio-twl4030.o
 obj-$(CONFIG_GPIO_UCB1400)	+= gpio-ucb1400.o
 obj-$(CONFIG_GPIO_VR41XX)	+= gpio-vr41xx.o
diff --git a/drivers/gpio/gpio-ts5500.c b/drivers/gpio/gpio-ts5500.c
new file mode 100644
index 0000000..d91fee9
--- /dev/null
+++ b/drivers/gpio/gpio-ts5500.c
@@ -0,0 +1,344 @@
+/*
+ * GPIO (DIO) driver for Technologic Systems TS-5500
+ *
+ * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
+ *	Vivien Didelot <vivien.didelot@savoirfairelinux.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * The TS-5500 platform has 38 Digital Input/Output lines (DIO), exposed by 3
+ * DIO headers: DIO1, DIO2, and the LCD port which may be used as a DIO header.
+ *
+ * The datasheet is available at:
+ * http://embeddedx86.com/documentation/ts-5500-manual.pdf.
+ * See section 6 "Digital I/O" for details about the pinout.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_data/gpio-ts5500.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/*
+ * This array describes the names of the DIO lines, but also the mapping between
+ * the datasheet, and corresponding offsets exposed by the driver.
+ */
+static const char * const ts5500_pinout[38] = {
+	/* DIO1 Header (offset 0-13) */
+	[0]  = "DIO1_0",  /* pin 1  */
+	[1]  = "DIO1_1",  /* pin 3  */
+	[2]  = "DIO1_2",  /* pin 5  */
+	[3]  = "DIO1_3",  /* pin 7  */
+	[4]  = "DIO1_4",  /* pin 9  */
+	[5]  = "DIO1_5",  /* pin 11 */
+	[6]  = "DIO1_6",  /* pin 13 */
+	[7]  = "DIO1_7",  /* pin 15 */
+	[8]  = "DIO1_8",  /* pin 4  */
+	[9]  = "DIO1_9",  /* pin 6  */
+	[10] = "DIO1_10", /* pin 8  */
+	[11] = "DIO1_11", /* pin 10 */
+	[12] = "DIO1_12", /* pin 12 */
+	[13] = "DIO1_13", /* pin 14 */
+
+	/* DIO2 Header (offset 14-26) */
+	[14] = "DIO2_0",  /* pin 1  */
+	[15] = "DIO2_1",  /* pin 3  */
+	[16] = "DIO2_2",  /* pin 5  */
+	[17] = "DIO2_3",  /* pin 7  */
+	[18] = "DIO2_4",  /* pin 9  */
+	[19] = "DIO2_5",  /* pin 11 */
+	[20] = "DIO2_6",  /* pin 13 */
+	[21] = "DIO2_7",  /* pin 15 */
+	[22] = "DIO2_8",  /* pin 4  */
+	[23] = "DIO2_9",  /* pin 6  */
+	[24] = "DIO2_10", /* pin 8  */
+	[25] = "DIO2_11", /* pin 10 */
+	[26] = "DIO2_13", /* pin 14 */
+
+	/* LCD Port as DIO (offset 27-37) */
+	[27] = "LCD_0",   /* pin 8  */
+	[28] = "LCD_1",   /* pin 7  */
+	[29] = "LCD_2",   /* pin 10 */
+	[30] = "LCD_3",   /* pin 9  */
+	[31] = "LCD_4",   /* pin 12 */
+	[32] = "LCD_5",   /* pin 11 */
+	[33] = "LCD_6",   /* pin 14 */
+	[34] = "LCD_7",   /* pin 13 */
+	[35] = "LCD_EN",  /* pin 5  */
+	[36] = "LCD_WR",  /* pin 6  */
+	[37] = "LCD_RS",  /* pin 3  */
+};
+
+#define IN	(1 << 0)
+#define OUT	(1 << 1)
+#ifndef NO_IRQ
+#define NO_IRQ	-1
+#endif
+
+/*
+ * This structure is used to describe capabilities of DIO lines,
+ * such as available directions, and mapped IRQ (if any).
+ */
+struct ts5500_dio {
+	const unsigned long value_addr;
+	const int value_bit;
+	const unsigned long control_addr;
+	const int control_bit;
+	const int irq;
+	const int direction;
+};
+
+static const struct ts5500_dio ts5500_dios[] = {
+	/* DIO1 Header (offset 0-13) */
+	[0]  = { 0x7b, 0, 0x7a, 0,  NO_IRQ, IN | OUT },
+	[1]  = { 0x7b, 1, 0x7a, 0,  NO_IRQ, IN | OUT },
+	[2]  = { 0x7b, 2, 0x7a, 0,  NO_IRQ, IN | OUT },
+	[3]  = { 0x7b, 3, 0x7a, 0,  NO_IRQ, IN | OUT },
+	[4]  = { 0x7b, 4, 0x7a, 1,  NO_IRQ, IN | OUT },
+	[5]  = { 0x7b, 5, 0x7a, 1,  NO_IRQ, IN | OUT },
+	[6]  = { 0x7b, 6, 0x7a, 1,  NO_IRQ, IN | OUT },
+	[7]  = { 0x7b, 7, 0x7a, 1,  NO_IRQ, IN | OUT },
+	[8]  = { 0x7c, 0, 0x7a, 5,  NO_IRQ, IN | OUT },
+	[9]  = { 0x7c, 1, 0x7a, 5,  NO_IRQ, IN | OUT },
+	[10] = { 0x7c, 2, 0x7a, 5,  NO_IRQ, IN | OUT },
+	[11] = { 0x7c, 3, 0x7a, 5,  NO_IRQ, IN | OUT },
+	[12] = { 0x7c, 4, 0,    -1, NO_IRQ, IN       },
+	[13] = { 0x7c, 5, 0,    -1, 7,      IN       },
+
+	/* DIO2 Header (offset 14-26) */
+	[14] = { 0x7e, 0, 0x7d, 0,  NO_IRQ, IN | OUT },
+	[15] = { 0x7e, 1, 0x7d, 0,  NO_IRQ, IN | OUT },
+	[16] = { 0x7e, 2, 0x7d, 0,  NO_IRQ, IN | OUT },
+	[17] = { 0x7e, 3, 0x7d, 0,  NO_IRQ, IN | OUT },
+	[18] = { 0x7e, 4, 0x7d, 1,  NO_IRQ, IN | OUT },
+	[19] = { 0x7e, 5, 0x7d, 1,  NO_IRQ, IN | OUT },
+	[20] = { 0x7e, 6, 0x7d, 1,  NO_IRQ, IN | OUT },
+	[21] = { 0x7e, 7, 0x7d, 1,  NO_IRQ, IN | OUT },
+	[22] = { 0x7f, 0, 0x7d, 5,  NO_IRQ, IN | OUT },
+	[23] = { 0x7f, 1, 0x7d, 5,  NO_IRQ, IN | OUT },
+	[24] = { 0x7f, 2, 0x7d, 5,  NO_IRQ, IN | OUT },
+	[25] = { 0x7f, 3, 0x7d, 5,  NO_IRQ, IN | OUT },
+	[26] = { 0x7f, 4, 0,    -1, 6,      IN       },
+
+	/* LCD Port as DIO (offset 27-37) */
+	[27] = { 0x72, 0, 0x7d, 2,  NO_IRQ, IN | OUT },
+	[28] = { 0x72, 1, 0x7d, 2,  NO_IRQ, IN | OUT },
+	[29] = { 0x72, 2, 0x7d, 2,  NO_IRQ, IN | OUT },
+	[30] = { 0x72, 3, 0x7d, 2,  NO_IRQ, IN | OUT },
+	[31] = { 0x72, 4, 0x7d, 3,  NO_IRQ, IN | OUT },
+	[32] = { 0x72, 5, 0x7d, 3,  NO_IRQ, IN | OUT },
+	[33] = { 0x72, 6, 0x7d, 3,  NO_IRQ, IN | OUT },
+	[34] = { 0x72, 7, 0x7d, 3,  NO_IRQ, IN | OUT },
+	[35] = { 0x73, 0, 0,    -1, NO_IRQ,      OUT },
+	[36] = { 0x73, 6, 0,    -1, NO_IRQ, IN       },
+	[37] = { 0x73, 7, 0,    -1, 1,      IN       },
+};
+
+static bool lcd_dio;
+static bool lcd_irq;
+static bool dio1_irq;
+static bool dio2_irq;
+
+static DEFINE_SPINLOCK(lock);
+
+static inline void io_set_bit(int bit, unsigned long addr)
+{
+	unsigned long val = inb(addr);
+	__set_bit(bit, &val);
+	outb(val, addr);
+}
+
+static inline void io_clear_bit(int bit, unsigned long addr)
+{
+	unsigned long val = inb(addr);
+	__clear_bit(bit, &val);
+	outb(val, addr);
+}
+
+static int ts5500_gpio_input(struct gpio_chip *chip, unsigned offset)
+{
+	unsigned long flags;
+	const struct ts5500_dio line = ts5500_dios[offset];
+
+	/* Some lines cannot be configured as input */
+	if (!(line.direction & IN))
+		return -ENXIO;
+
+	/* Some others are input only */
+	if (line.direction & OUT) {
+		spin_lock_irqsave(&lock, flags);
+		io_clear_bit(line.control_bit, line.control_addr);
+		spin_unlock_irqrestore(&lock, flags);
+	}
+
+	return 0;
+}
+
+static int ts5500_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	const struct ts5500_dio line = ts5500_dios[offset];
+
+	return (inb(line.value_addr) >> line.value_bit) & 1;
+}
+
+static int ts5500_gpio_output(struct gpio_chip *chip, unsigned offset, int val)
+{
+	unsigned long flags;
+	const struct ts5500_dio line = ts5500_dios[offset];
+
+	/* Some lines cannot be configured as output */
+	if (!(line.direction & OUT))
+		return -ENXIO;
+
+	spin_lock_irqsave(&lock, flags);
+	/* Some lines are output only */
+	if (line.direction & IN)
+		io_set_bit(line.control_bit, line.control_addr);
+
+	if (val)
+		io_set_bit(line.value_bit, line.value_addr);
+	else
+		io_clear_bit(line.value_bit, line.value_addr);
+	spin_unlock_irqrestore(&lock, flags);
+
+	return 0;
+}
+
+static void ts5500_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
+{
+	unsigned long flags;
+	const struct ts5500_dio line = ts5500_dios[offset];
+
+	spin_lock_irqsave(&lock, flags);
+	if (val)
+		io_set_bit(line.value_bit, line.value_addr);
+	else
+		io_clear_bit(line.value_bit, line.value_addr);
+	spin_unlock_irqrestore(&lock, flags);
+}
+
+static int ts5500_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	const struct ts5500_dio line = ts5500_dios[offset];
+
+	/* Only a few lines are IRQ-capable */
+	if (line.irq != NO_IRQ)
+		return line.irq;
+
+	/* This allows to bridge a line with the IRQ line of the same header */
+	if (dio1_irq && offset < 13)
+		return ts5500_dios[13].irq;
+	if (dio2_irq && offset > 13 && offset < 26)
+		return ts5500_dios[26].irq;
+	if (lcd_irq && offset > 26 && offset < 37)
+		return ts5500_dios[37].irq;
+
+	return -ENXIO;
+}
+
+static struct gpio_chip ts5500_gc = {
+	.label = "TS-5500 DIO Headers",
+	.owner = THIS_MODULE,
+	.direction_input = ts5500_gpio_input,
+	.get = ts5500_gpio_get,
+	.direction_output = ts5500_gpio_output,
+	.set = ts5500_gpio_set,
+	.to_irq = ts5500_gpio_to_irq,
+	.names = ts5500_pinout,
+	.ngpio = 27,
+	.base = -1,
+};
+
+static int __devinit ts5500_gpio_probe(struct platform_device *pdev)
+{
+	int ret;
+	unsigned long flags;
+	struct ts5500_gpio_platform_data *pdata = pdev->dev.platform_data;
+
+	if (pdata) {
+		ts5500_gc.base = pdata->base;
+		dio1_irq = pdata->dio1_irq;
+		dio2_irq = pdata->dio2_irq;
+		if (pdata->lcd_dio) {
+			lcd_dio = true;
+			lcd_irq = pdata->lcd_irq;
+			ts5500_gc.ngpio = 38;
+		}
+	}
+
+	if (!devm_request_region(&pdev->dev, 0x7a, 3, "DIO1 Header")) {
+		dev_err(&pdev->dev, "failed to request DIO1 ports (0x7a-7c)\n");
+		return -EBUSY;
+	}
+
+	if (!devm_request_region(&pdev->dev, 0x7d, 3, "DIO2 Header")) {
+		dev_err(&pdev->dev, "failed to request DIO2 ports (0x7d-7f)\n");
+		return -EBUSY;
+	}
+
+	if (lcd_dio &&
+	    !devm_request_region(&pdev->dev, 0x72, 2, "LCD Port as DIO")) {
+		dev_err(&pdev->dev, "failed to request LCD ports (0x72-73)\n");
+		return -EBUSY;
+	}
+
+	platform_set_drvdata(pdev, &ts5500_gc);
+
+	ret = gpiochip_add(&ts5500_gc);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register the gpio chip\n");
+		return ret;
+	}
+
+	/* Enable IRQ generation */
+	spin_lock_irqsave(&lock, flags);
+	io_set_bit(7, 0x7a); /* DIO1_13 on IRQ7 */
+	io_set_bit(7, 0x7d); /* DIO2_13 on IRQ6 */
+	if (lcd_dio) {
+		io_clear_bit(4, 0x7d); /* LCD Header usage as DIO */
+		io_set_bit(6, 0x7d); /* LCD_RS on IRQ1 */
+	}
+	spin_unlock_irqrestore(&lock, flags);
+
+	return 0;
+}
+
+static int __devexit ts5500_gpio_remove(struct platform_device *pdev)
+{
+	int ret;
+	unsigned long flags;
+
+	/* Disable IRQ generation */
+	spin_lock_irqsave(&lock, flags);
+	io_clear_bit(7, 0x7a);
+	io_clear_bit(7, 0x7d);
+	if (lcd_dio)
+		io_clear_bit(6, 0x7d);
+	spin_unlock_irqrestore(&lock, flags);
+
+	ret = gpiochip_remove(&ts5500_gc);
+	if (ret)
+		dev_err(&pdev->dev, "failed to remove the gpio chip\n");
+
+	return ret;
+}
+
+static struct platform_driver ts5500_gpio_driver = {
+	.driver = {
+		.name = "gpio-ts5500",
+		.owner = THIS_MODULE,
+	},
+	.probe = ts5500_gpio_probe,
+	.remove = __devexit_p(ts5500_gpio_remove),
+};
+
+module_platform_driver(ts5500_gpio_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Savoir-faire Linux Inc. <kernel@savoirfairelinux.com>");
+MODULE_DESCRIPTION("Technologic Systems TS-5500 Digital I/O driver");
diff --git a/include/linux/platform_data/gpio-ts5500.h b/include/linux/platform_data/gpio-ts5500.h
new file mode 100644
index 0000000..9efd4c3
--- /dev/null
+++ b/include/linux/platform_data/gpio-ts5500.h
@@ -0,0 +1,34 @@
+/*
+ * GPIO (DIO) header for Technologic Systems TS-5500
+ *
+ * Copyright (c) 2012 Savoir-faire Linux Inc.
+ *	Vivien Didelot <vivien.didelot@savoirfairelinux.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * The TS-5500 board has 38 Digital Input/Output lines, referred to as DIO in
+ * the product's literature. See section 6 "Digital I/O" of the datasheet:
+ * http://embeddedx86.com/documentation/ts-5500-manual.pdf
+ *
+ * As each IRQable line is input only, the platform data has an option for each
+ * header to return the corresponding IRQ line on request. This might be useful
+ * to bind a bidirectional line with the IRQ line of the same header.
+ */
+
+/**
+ * struct ts5500_gpio_platform_data - TS-5500 GPIO configuration
+ * @base:	The GPIO base number to use.
+ * @lcd_dio:	Use the LCD port as 11 additional digital I/O lines.
+ * @lcd_irq:	Return IRQ1 for every line of LCD DIO header.
+ * @dio1_irq:	Return IRQ7 for every line of DIO1 header.
+ * @dio2_irq:	Return IRQ6 for every line of DIO2 header.
+ */
+struct ts5500_gpio_platform_data {
+	int base;
+	bool lcd_dio;
+	bool lcd_irq;
+	bool dio1_irq;
+	bool dio2_irq;
+};
-- 
1.7.11.4

Re: [PATCH] gpio: add TS-5500 DIO headers support

[Vivien Didelot]

Hi,

I think the Kconfig patch is wrong, please do not consider this
patchset. I'll send a v2 very soon.

On Tue, 2012-09-25 at 20:42 -0400, Vivien Didelot wrote:
> The Technologic Systems TS-5500 platform provides 3 digital I/O headers:
> DIO1, DIO2, and the LCD port, that may be used as a DIO header.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> ---
>  drivers/gpio/Kconfig                      |   8 +
>  drivers/gpio/Makefile                     |   1 +
>  drivers/gpio/gpio-ts5500.c                | 344 ++++++++++++++++++++++++++++++
>  include/linux/platform_data/gpio-ts5500.h |  34 +++
>  4 files changed, 387 insertions(+)
>  create mode 100644 drivers/gpio/gpio-ts5500.c
>  create mode 100644 include/linux/platform_data/gpio-ts5500.h
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index ba7926f5..f8bf8e1 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -395,6 +395,14 @@ config GPIO_TPS65912
>  	help
>  	  This driver supports TPS65912 gpio chip
>  
> +config GPIO_TS5500
> +	tristate "TS-5500 DIO Headers"
> +	depends on TS5500
> +	help
> +	  This driver supports the 3 Digital I/O headers of the Technologic
> +	  Systems TS-5500 platform: DIO1, DIO2, and the LCD port which may be
> +	  used as a DIO header.

I moved the GPIO_TS5500 entry at the good place in the "Memory mapped
GPIO drivers" section. But I'm still worried about select/depends on.
I removed the TS5500 symbol dependency, which is not defined yet (will
be done in the future board support patch). Does the following entry
seem good?

    config GPIO_TS5500
            tristate "TS-5500 DIO Headers support"
            help
              This driver supports the 3 Digital I/O headers of the
              Technologic Systems TS-5500 platform: DIO1, DIO2, and the
              LCD port which may be used as a DIO header.

Regards,
Vivien


> +
>  config GPIO_TWL4030
>  	tristate "TWL4030, TWL5030, and TPS659x0 GPIOs"
>  	depends on TWL4030_CORE
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 153cace..48e8670 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_ARCH_DAVINCI_TNETV107X) += gpio-tnetv107x.o
>  obj-$(CONFIG_GPIO_TPS6586X)	+= gpio-tps6586x.o
>  obj-$(CONFIG_GPIO_TPS65910)	+= gpio-tps65910.o
>  obj-$(CONFIG_GPIO_TPS65912)	+= gpio-tps65912.o
> +obj-$(CONFIG_GPIO_TS5500)	+= gpio-ts5500.o
>  obj-$(CONFIG_GPIO_TWL4030)	+= gpio-twl4030.o
>  obj-$(CONFIG_GPIO_UCB1400)	+= gpio-ucb1400.o
>  obj-$(CONFIG_GPIO_VR41XX)	+= gpio-vr41xx.o
> diff --git a/drivers/gpio/gpio-ts5500.c b/drivers/gpio/gpio-ts5500.c
> new file mode 100644
> index 0000000..d91fee9
> --- /dev/null
> +++ b/drivers/gpio/gpio-ts5500.c
> @@ -0,0 +1,344 @@
> +/*
> + * GPIO (DIO) driver for Technologic Systems TS-5500
> + *
> + * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
> + *	Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * The TS-5500 platform has 38 Digital Input/Output lines (DIO), exposed by 3
> + * DIO headers: DIO1, DIO2, and the LCD port which may be used as a DIO header.
> + *
> + * The datasheet is available at:
> + * http://embeddedx86.com/documentation/ts-5500-manual.pdf.
> + * See section 6 "Digital I/O" for details about the pinout.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/gpio-ts5500.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/*
> + * This array describes the names of the DIO lines, but also the mapping between
> + * the datasheet, and corresponding offsets exposed by the driver.
> + */
> +static const char * const ts5500_pinout[38] = {
> +	/* DIO1 Header (offset 0-13) */
> +	[0]  = "DIO1_0",  /* pin 1  */
> +	[1]  = "DIO1_1",  /* pin 3  */
> +	[2]  = "DIO1_2",  /* pin 5  */
> +	[3]  = "DIO1_3",  /* pin 7  */
> +	[4]  = "DIO1_4",  /* pin 9  */
> +	[5]  = "DIO1_5",  /* pin 11 */
> +	[6]  = "DIO1_6",  /* pin 13 */
> +	[7]  = "DIO1_7",  /* pin 15 */
> +	[8]  = "DIO1_8",  /* pin 4  */
> +	[9]  = "DIO1_9",  /* pin 6  */
> +	[10] = "DIO1_10", /* pin 8  */
> +	[11] = "DIO1_11", /* pin 10 */
> +	[12] = "DIO1_12", /* pin 12 */
> +	[13] = "DIO1_13", /* pin 14 */
> +
> +	/* DIO2 Header (offset 14-26) */
> +	[14] = "DIO2_0",  /* pin 1  */
> +	[15] = "DIO2_1",  /* pin 3  */
> +	[16] = "DIO2_2",  /* pin 5  */
> +	[17] = "DIO2_3",  /* pin 7  */
> +	[18] = "DIO2_4",  /* pin 9  */
> +	[19] = "DIO2_5",  /* pin 11 */
> +	[20] = "DIO2_6",  /* pin 13 */
> +	[21] = "DIO2_7",  /* pin 15 */
> +	[22] = "DIO2_8",  /* pin 4  */
> +	[23] = "DIO2_9",  /* pin 6  */
> +	[24] = "DIO2_10", /* pin 8  */
> +	[25] = "DIO2_11", /* pin 10 */
> +	[26] = "DIO2_13", /* pin 14 */
> +
> +	/* LCD Port as DIO (offset 27-37) */
> +	[27] = "LCD_0",   /* pin 8  */
> +	[28] = "LCD_1",   /* pin 7  */
> +	[29] = "LCD_2",   /* pin 10 */
> +	[30] = "LCD_3",   /* pin 9  */
> +	[31] = "LCD_4",   /* pin 12 */
> +	[32] = "LCD_5",   /* pin 11 */
> +	[33] = "LCD_6",   /* pin 14 */
> +	[34] = "LCD_7",   /* pin 13 */
> +	[35] = "LCD_EN",  /* pin 5  */
> +	[36] = "LCD_WR",  /* pin 6  */
> +	[37] = "LCD_RS",  /* pin 3  */
> +};
> +
> +#define IN	(1 << 0)
> +#define OUT	(1 << 1)
> +#ifndef NO_IRQ
> +#define NO_IRQ	-1
> +#endif
> +
> +/*
> + * This structure is used to describe capabilities of DIO lines,
> + * such as available directions, and mapped IRQ (if any).
> + */
> +struct ts5500_dio {
> +	const unsigned long value_addr;
> +	const int value_bit;
> +	const unsigned long control_addr;
> +	const int control_bit;
> +	const int irq;
> +	const int direction;
> +};
> +
> +static const struct ts5500_dio ts5500_dios[] = {
> +	/* DIO1 Header (offset 0-13) */
> +	[0]  = { 0x7b, 0, 0x7a, 0,  NO_IRQ, IN | OUT },
> +	[1]  = { 0x7b, 1, 0x7a, 0,  NO_IRQ, IN | OUT },
> +	[2]  = { 0x7b, 2, 0x7a, 0,  NO_IRQ, IN | OUT },
> +	[3]  = { 0x7b, 3, 0x7a, 0,  NO_IRQ, IN | OUT },
> +	[4]  = { 0x7b, 4, 0x7a, 1,  NO_IRQ, IN | OUT },
> +	[5]  = { 0x7b, 5, 0x7a, 1,  NO_IRQ, IN | OUT },
> +	[6]  = { 0x7b, 6, 0x7a, 1,  NO_IRQ, IN | OUT },
> +	[7]  = { 0x7b, 7, 0x7a, 1,  NO_IRQ, IN | OUT },
> +	[8]  = { 0x7c, 0, 0x7a, 5,  NO_IRQ, IN | OUT },
> +	[9]  = { 0x7c, 1, 0x7a, 5,  NO_IRQ, IN | OUT },
> +	[10] = { 0x7c, 2, 0x7a, 5,  NO_IRQ, IN | OUT },
> +	[11] = { 0x7c, 3, 0x7a, 5,  NO_IRQ, IN | OUT },
> +	[12] = { 0x7c, 4, 0,    -1, NO_IRQ, IN       },
> +	[13] = { 0x7c, 5, 0,    -1, 7,      IN       },
> +
> +	/* DIO2 Header (offset 14-26) */
> +	[14] = { 0x7e, 0, 0x7d, 0,  NO_IRQ, IN | OUT },
> +	[15] = { 0x7e, 1, 0x7d, 0,  NO_IRQ, IN | OUT },
> +	[16] = { 0x7e, 2, 0x7d, 0,  NO_IRQ, IN | OUT },
> +	[17] = { 0x7e, 3, 0x7d, 0,  NO_IRQ, IN | OUT },
> +	[18] = { 0x7e, 4, 0x7d, 1,  NO_IRQ, IN | OUT },
> +	[19] = { 0x7e, 5, 0x7d, 1,  NO_IRQ, IN | OUT },
> +	[20] = { 0x7e, 6, 0x7d, 1,  NO_IRQ, IN | OUT },
> +	[21] = { 0x7e, 7, 0x7d, 1,  NO_IRQ, IN | OUT },
> +	[22] = { 0x7f, 0, 0x7d, 5,  NO_IRQ, IN | OUT },
> +	[23] = { 0x7f, 1, 0x7d, 5,  NO_IRQ, IN | OUT },
> +	[24] = { 0x7f, 2, 0x7d, 5,  NO_IRQ, IN | OUT },
> +	[25] = { 0x7f, 3, 0x7d, 5,  NO_IRQ, IN | OUT },
> +	[26] = { 0x7f, 4, 0,    -1, 6,      IN       },
> +
> +	/* LCD Port as DIO (offset 27-37) */
> +	[27] = { 0x72, 0, 0x7d, 2,  NO_IRQ, IN | OUT },
> +	[28] = { 0x72, 1, 0x7d, 2,  NO_IRQ, IN | OUT },
> +	[29] = { 0x72, 2, 0x7d, 2,  NO_IRQ, IN | OUT },
> +	[30] = { 0x72, 3, 0x7d, 2,  NO_IRQ, IN | OUT },
> +	[31] = { 0x72, 4, 0x7d, 3,  NO_IRQ, IN | OUT },
> +	[32] = { 0x72, 5, 0x7d, 3,  NO_IRQ, IN | OUT },
> +	[33] = { 0x72, 6, 0x7d, 3,  NO_IRQ, IN | OUT },
> +	[34] = { 0x72, 7, 0x7d, 3,  NO_IRQ, IN | OUT },
> +	[35] = { 0x73, 0, 0,    -1, NO_IRQ,      OUT },
> +	[36] = { 0x73, 6, 0,    -1, NO_IRQ, IN       },
> +	[37] = { 0x73, 7, 0,    -1, 1,      IN       },
> +};
> +
> +static bool lcd_dio;
> +static bool lcd_irq;
> +static bool dio1_irq;
> +static bool dio2_irq;
> +
> +static DEFINE_SPINLOCK(lock);
> +
> +static inline void io_set_bit(int bit, unsigned long addr)
> +{
> +	unsigned long val = inb(addr);
> +	__set_bit(bit, &val);
> +	outb(val, addr);
> +}
> +
> +static inline void io_clear_bit(int bit, unsigned long addr)
> +{
> +	unsigned long val = inb(addr);
> +	__clear_bit(bit, &val);
> +	outb(val, addr);
> +}
> +
> +static int ts5500_gpio_input(struct gpio_chip *chip, unsigned offset)
> +{
> +	unsigned long flags;
> +	const struct ts5500_dio line = ts5500_dios[offset];
> +
> +	/* Some lines cannot be configured as input */
> +	if (!(line.direction & IN))
> +		return -ENXIO;
> +
> +	/* Some others are input only */
> +	if (line.direction & OUT) {
> +		spin_lock_irqsave(&lock, flags);
> +		io_clear_bit(line.control_bit, line.control_addr);
> +		spin_unlock_irqrestore(&lock, flags);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ts5500_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +	const struct ts5500_dio line = ts5500_dios[offset];
> +
> +	return (inb(line.value_addr) >> line.value_bit) & 1;
> +}
> +
> +static int ts5500_gpio_output(struct gpio_chip *chip, unsigned offset, int val)
> +{
> +	unsigned long flags;
> +	const struct ts5500_dio line = ts5500_dios[offset];
> +
> +	/* Some lines cannot be configured as output */
> +	if (!(line.direction & OUT))
> +		return -ENXIO;
> +
> +	spin_lock_irqsave(&lock, flags);
> +	/* Some lines are output only */
> +	if (line.direction & IN)
> +		io_set_bit(line.control_bit, line.control_addr);
> +
> +	if (val)
> +		io_set_bit(line.value_bit, line.value_addr);
> +	else
> +		io_clear_bit(line.value_bit, line.value_addr);
> +	spin_unlock_irqrestore(&lock, flags);
> +
> +	return 0;
> +}
> +
> +static void ts5500_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
> +{
> +	unsigned long flags;
> +	const struct ts5500_dio line = ts5500_dios[offset];
> +
> +	spin_lock_irqsave(&lock, flags);
> +	if (val)
> +		io_set_bit(line.value_bit, line.value_addr);
> +	else
> +		io_clear_bit(line.value_bit, line.value_addr);
> +	spin_unlock_irqrestore(&lock, flags);
> +}
> +
> +static int ts5500_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +	const struct ts5500_dio line = ts5500_dios[offset];
> +
> +	/* Only a few lines are IRQ-capable */
> +	if (line.irq != NO_IRQ)
> +		return line.irq;
> +
> +	/* This allows to bridge a line with the IRQ line of the same header */
> +	if (dio1_irq && offset < 13)
> +		return ts5500_dios[13].irq;
> +	if (dio2_irq && offset > 13 && offset < 26)
> +		return ts5500_dios[26].irq;
> +	if (lcd_irq && offset > 26 && offset < 37)
> +		return ts5500_dios[37].irq;
> +
> +	return -ENXIO;
> +}
> +
> +static struct gpio_chip ts5500_gc = {
> +	.label = "TS-5500 DIO Headers",
> +	.owner = THIS_MODULE,
> +	.direction_input = ts5500_gpio_input,
> +	.get = ts5500_gpio_get,
> +	.direction_output = ts5500_gpio_output,
> +	.set = ts5500_gpio_set,
> +	.to_irq = ts5500_gpio_to_irq,
> +	.names = ts5500_pinout,
> +	.ngpio = 27,
> +	.base = -1,
> +};
> +
> +static int __devinit ts5500_gpio_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	unsigned long flags;
> +	struct ts5500_gpio_platform_data *pdata = pdev->dev.platform_data;
> +
> +	if (pdata) {
> +		ts5500_gc.base = pdata->base;
> +		dio1_irq = pdata->dio1_irq;
> +		dio2_irq = pdata->dio2_irq;
> +		if (pdata->lcd_dio) {
> +			lcd_dio = true;
> +			lcd_irq = pdata->lcd_irq;
> +			ts5500_gc.ngpio = 38;
> +		}
> +	}
> +
> +	if (!devm_request_region(&pdev->dev, 0x7a, 3, "DIO1 Header")) {
> +		dev_err(&pdev->dev, "failed to request DIO1 ports (0x7a-7c)\n");
> +		return -EBUSY;
> +	}
> +
> +	if (!devm_request_region(&pdev->dev, 0x7d, 3, "DIO2 Header")) {
> +		dev_err(&pdev->dev, "failed to request DIO2 ports (0x7d-7f)\n");
> +		return -EBUSY;
> +	}
> +
> +	if (lcd_dio &&
> +	    !devm_request_region(&pdev->dev, 0x72, 2, "LCD Port as DIO")) {
> +		dev_err(&pdev->dev, "failed to request LCD ports (0x72-73)\n");
> +		return -EBUSY;
> +	}
> +
> +	platform_set_drvdata(pdev, &ts5500_gc);
> +
> +	ret = gpiochip_add(&ts5500_gc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register the gpio chip\n");
> +		return ret;
> +	}
> +
> +	/* Enable IRQ generation */
> +	spin_lock_irqsave(&lock, flags);
> +	io_set_bit(7, 0x7a); /* DIO1_13 on IRQ7 */
> +	io_set_bit(7, 0x7d); /* DIO2_13 on IRQ6 */
> +	if (lcd_dio) {
> +		io_clear_bit(4, 0x7d); /* LCD Header usage as DIO */
> +		io_set_bit(6, 0x7d); /* LCD_RS on IRQ1 */
> +	}
> +	spin_unlock_irqrestore(&lock, flags);
> +
> +	return 0;
> +}
> +
> +static int __devexit ts5500_gpio_remove(struct platform_device *pdev)
> +{
> +	int ret;
> +	unsigned long flags;
> +
> +	/* Disable IRQ generation */
> +	spin_lock_irqsave(&lock, flags);
> +	io_clear_bit(7, 0x7a);
> +	io_clear_bit(7, 0x7d);
> +	if (lcd_dio)
> +		io_clear_bit(6, 0x7d);
> +	spin_unlock_irqrestore(&lock, flags);
> +
> +	ret = gpiochip_remove(&ts5500_gc);
> +	if (ret)
> +		dev_err(&pdev->dev, "failed to remove the gpio chip\n");
> +
> +	return ret;
> +}
> +
> +static struct platform_driver ts5500_gpio_driver = {
> +	.driver = {
> +		.name = "gpio-ts5500",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = ts5500_gpio_probe,
> +	.remove = __devexit_p(ts5500_gpio_remove),
> +};
> +
> +module_platform_driver(ts5500_gpio_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Savoir-faire Linux Inc. <kernel@savoirfairelinux.com>");
> +MODULE_DESCRIPTION("Technologic Systems TS-5500 Digital I/O driver");
> diff --git a/include/linux/platform_data/gpio-ts5500.h b/include/linux/platform_data/gpio-ts5500.h
> new file mode 100644
> index 0000000..9efd4c3
> --- /dev/null
> +++ b/include/linux/platform_data/gpio-ts5500.h
> @@ -0,0 +1,34 @@
> +/*
> + * GPIO (DIO) header for Technologic Systems TS-5500
> + *
> + * Copyright (c) 2012 Savoir-faire Linux Inc.
> + *	Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * The TS-5500 board has 38 Digital Input/Output lines, referred to as DIO in
> + * the product's literature. See section 6 "Digital I/O" of the datasheet:
> + * http://embeddedx86.com/documentation/ts-5500-manual.pdf
> + *
> + * As each IRQable line is input only, the platform data has an option for each
> + * header to return the corresponding IRQ line on request. This might be useful
> + * to bind a bidirectional line with the IRQ line of the same header.
> + */
> +
> +/**
> + * struct ts5500_gpio_platform_data - TS-5500 GPIO configuration
> + * @base:	The GPIO base number to use.
> + * @lcd_dio:	Use the LCD port as 11 additional digital I/O lines.
> + * @lcd_irq:	Return IRQ1 for every line of LCD DIO header.
> + * @dio1_irq:	Return IRQ7 for every line of DIO1 header.
> + * @dio2_irq:	Return IRQ6 for every line of DIO2 header.
> + */
> +struct ts5500_gpio_platform_data {
> +	int base;
> +	bool lcd_dio;
> +	bool lcd_irq;
> +	bool dio1_irq;
> +	bool dio2_irq;
> +};

Re: [PATCH] gpio: add TS-5500 DIO headers support

[Vivien Didelot]

Hi,

Grant, Linus, any feedback?

Thanks,
Vivien

On Wed, 2012-09-26 at 11:37 -0400, Vivien Didelot wrote:
> > +config GPIO_TS5500
> > +     tristate "TS-5500 DIO Headers"
> > +     depends on TS5500
> > +     help
> > +       This driver supports the 3 Digital I/O headers of the
> Technologic
> > +       Systems TS-5500 platform: DIO1, DIO2, and the LCD port which
> may be
> > +       used as a DIO header.
> 
> I moved the GPIO_TS5500 entry at the good place in the "Memory mapped
> GPIO drivers" section. But I'm still worried about select/depends on.
> I removed the TS5500 symbol dependency, which is not defined yet (will
> be done in the future board support patch). Does the following entry
> seem good?
> 
>     config GPIO_TS5500
>             tristate "TS-5500 DIO Headers support"
>             help
>               This driver supports the 3 Digital I/O headers of the
>               Technologic Systems TS-5500 platform: DIO1, DIO2, and
> the
>               LCD port which may be used as a DIO header.
> 
> Regards,
> Vivien 

Re: [PATCH] gpio: add TS-5500 DIO headers support

[Linus Walleij]

On Fri, Oct 5, 2012 at 1:18 AM, Vivien Didelot <vivien.didelot@gmail.com> wrote:

> Grant, Linus, any feedback?

On the entire driver or on the config fragment?

The latter looks OK, I'll look at the driver per se now.
(I was busy with the merge window you know...)

Yours,
Linus Walleij

Re: [PATCH] gpio: add TS-5500 DIO headers support

[Linus Walleij]

On Wed, Sep 26, 2012 at 2:42 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:

> The Technologic Systems TS-5500 platform provides 3 digital I/O headers:
> DIO1, DIO2, and the LCD port, that may be used as a DIO header.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
(...)
> + * The TS-5500 platform has 38 Digital Input/Output lines (DIO), exposed by 3
> + * DIO headers: DIO1, DIO2, and the LCD port which may be used as a DIO header.

Header? Is that like a socket or something?

The ability to swith the LCD port into DIO is a pin control property
and if this gets implemented the driver should technically be
in drivers/pinctrl. (It can implement GPIO too, no problem.)

Part of me dislike that you create one single driver for all
three blocks instead of abstracting the driver to handle one
block, and register one platform device each, 2-3 times.
But given that this is very tied to this one chip it could be the
simplest and most readable design so OK...

> + * The datasheet is available at:
> + * http://embeddedx86.com/documentation/ts-5500-manual.pdf.

Drop the period after the URL. It makes it hard to browse...

> +/*
> + * This array describes the names of the DIO lines, but also the mapping between
> + * the datasheet, and corresponding offsets exposed by the driver.
> + */
> +static const char * const ts5500_pinout[38] = {
> +       /* DIO1 Header (offset 0-13) */
> +       [0]  = "DIO1_0",  /* pin 1  */
> +       [1]  = "DIO1_1",  /* pin 3  */

Pins pins pins. The pinctrl subsystem has a framework for keeping track of
pins and giving them names, which is another reason to convert this to a
pinctrl driver.

Please consult Documentation/pinctrl.txt

> +#define IN     (1 << 0)
> +#define OUT    (1 << 1)

Why not use a bool named "out" then it's out if true
else input.

> +#ifndef NO_IRQ
> +#define NO_IRQ -1
> +#endif

No. We have very solidly decided that NO_IRQ == 0.

> +/*
> + * This structure is used to describe capabilities of DIO lines,
> + * such as available directions, and mapped IRQ (if any).
> + */
> +struct ts5500_dio {
> +       const unsigned long value_addr;
> +       const int value_bit;
> +       const unsigned long control_addr;
> +       const int control_bit;

All the above seem like they should be u8 rather than
const unsigned or const int.

> +       const int irq;

IRQ numbers shall not be hard-coded into drivers like
this, they shall be passed in as resources from the outside
just like you pass in other platform data.

> +       const int direction;

Use a bool out for this last thing?

> +};
> +
> +static const struct ts5500_dio ts5500_dios[] = {
> +       /* DIO1 Header (offset 0-13) */
> +       [0]  = { 0x7b, 0, 0x7a, 0,  NO_IRQ, IN | OUT },

Use C99 syntax and skip the [0] index (etc).

static const struct ts5500_dio ts5500_dios[] = {
    {
        .value_addr = 0x7b,
        .value_bit = 0,
        .control_addr = 0x7a,
        .control_bit = 0,
        .direction = OUT,
    },
    {
        .value_addr = 0x7b
....

Note absence of NO_IRQ - it's implicit zero in static
allocations.

> +static bool lcd_dio;
> +static bool lcd_irq;
> +static bool dio1_irq;
> +static bool dio2_irq;

Document what these are for with some /* comments */

But overall this has a singleton problem, this driver is not reentrant.
Usually that doesn't matter on a practical system, but we sure
prefer if you create a state container:

struct ts5500dio_state {
  bool lcd_dio;
  bool lcd_irq;
  bool dio1_irq;
  bool dio2_irq;
}

In .probe:

struct ts5500dio_state *my_state = devm_kzalloc(&dev, sizeof(struct
foo_state), GFP_KERNEL);

my_state->lcd_dio = ...

etc.

> +static DEFINE_SPINLOCK(lock);

This would also go into the state container.

> +static inline void io_set_bit(int bit, unsigned long addr)

io_set_bit() is too generic, use something like ts5500dio_set_bit()

> +{
> +       unsigned long val = inb(addr);

I'm not familiar with inb() and friends, I guess it's IO-space
accessors so I just buy into it...

> +       __set_bit(bit, &val);

Do you have to use the __ variants? Isn't just set_bit() working?
(Just checking...)

> +       outb(val, addr);
> +}
> +
> +static inline void io_clear_bit(int bit, unsigned long addr)
> +{
> +       unsigned long val = inb(addr);
> +       __clear_bit(bit, &val);
> +       outb(val, addr);
> +}

Same comments.

> +static int ts5500_gpio_input(struct gpio_chip *chip, unsigned offset)
> +{
> +       unsigned long flags;
> +       const struct ts5500_dio line = ts5500_dios[offset];
> +
> +       /* Some lines cannot be configured as input */
> +       if (!(line.direction & IN))
> +               return -ENXIO;

Why are you using that return code? (Probably OK, just want
a rationale...)

(...)
> +static int ts5500_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       const struct ts5500_dio line = ts5500_dios[offset];
> +
> +       return (inb(line.value_addr) >> line.value_bit) & 1;

Use this idiom:

return !!(inb(line.value_addr) & line.value_bit);

It's quite a common way to clamp a numeral to a bool int.

(...)
> +static int ts5500_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       const struct ts5500_dio line = ts5500_dios[offset];
> +
> +       /* Only a few lines are IRQ-capable */
> +       if (line.irq != NO_IRQ)
> +               return line.irq;
> +
> +       /* This allows to bridge a line with the IRQ line of the same header */
> +       if (dio1_irq && offset < 13)
> +               return ts5500_dios[13].irq;
> +       if (dio2_irq && offset > 13 && offset < 26)
> +               return ts5500_dios[26].irq;
> +       if (lcd_irq && offset > 26 && offset < 37)
> +               return ts5500_dios[37].irq;

Don't do this. Please use irqdomain for converting physical
IRQ numbers to Linux IRQ numbers. (Consult other GPIO
drivers for examples.)

These magic constants (13, 26, 37) are scary too.

You should not try to handle each block as a single
IRQ, instead instatiate a struct irq_chip in the driver
and let that use irqdomain do demux the IRQ and
register a range of Linux IRQ numbers, on per pin,
so the GPIO driver will handle the physical IRQ line,
then dispatch to a fan-out handler, so drivers that need
IRQs from the GPIO chip just register IRQ handlers like
anyone else.

(...)
> +static int __devinit ts5500_gpio_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       unsigned long flags;
> +       struct ts5500_gpio_platform_data *pdata = pdev->dev.platform_data;

This is where you should allocate you state container dynamically.

(...)
> +       /* Enable IRQ generation */
> +       spin_lock_irqsave(&lock, flags);
> +       io_set_bit(7, 0x7a); /* DIO1_13 on IRQ7 */
> +       io_set_bit(7, 0x7d); /* DIO2_13 on IRQ6 */
> +       if (lcd_dio) {
> +               io_clear_bit(4, 0x7d); /* LCD Header usage as DIO */
> +               io_set_bit(6, 0x7d); /* LCD_RS on IRQ1 */
> +       }

This is pincontrol ... but well. It's very very little after all.

> +/**
> + * struct ts5500_gpio_platform_data - TS-5500 GPIO configuration
> + * @base:      The GPIO base number to use.
> + * @lcd_dio:   Use the LCD port as 11 additional digital I/O lines.
> + * @lcd_irq:   Return IRQ1 for every line of LCD DIO header.
> + * @dio1_irq:  Return IRQ7 for every line of DIO1 header.
> + * @dio2_irq:  Return IRQ6 for every line of DIO2 header.
> + */
> +struct ts5500_gpio_platform_data {
> +       int base;
> +       bool lcd_dio;
> +       bool lcd_irq;
> +       bool dio1_irq;
> +       bool dio2_irq;
> +};

So I don't like how this hardcodes IRQ 1, 7, 6 to be used for this
purpose, it's better to pass that in as resources from the platform.

Yours,
Linus Walleij

Re: [PATCH] gpio: add TS-5500 DIO headers support

[Vivien Didelot]

Hi Linus,

On Mon, 2012-10-08 at 12:38 +0200, Linus Walleij wrote:
> On Wed, Sep 26, 2012 at 2:42 AM, Vivien Didelot
> <vivien.didelot@savoirfairelinux.com> wrote:
> 
> > The Technologic Systems TS-5500 platform provides 3 digital I/O headers:
> > DIO1, DIO2, and the LCD port, that may be used as a DIO header.
> >
> > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> > Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> (...)
> > + * The TS-5500 platform has 38 Digital Input/Output lines (DIO), exposed by 3
> > + * DIO headers: DIO1, DIO2, and the LCD port which may be used as a DIO header.
> 
> Header? Is that like a socket or something?

Digital Input/Output header is the term given in the platform datasheet
to describe a block. I can use "block" if it's more explicit.

> 
> The ability to swith the LCD port into DIO is a pin control property
> and if this gets implemented the driver should technically be
> in drivers/pinctrl. (It can implement GPIO too, no problem.)
> 
> Part of me dislike that you create one single driver for all
> three blocks instead of abstracting the driver to handle one
> block, and register one platform device each, 2-3 times.
> But given that this is very tied to this one chip it could be the
> simplest and most readable design so OK...

I agree with you. I thought about that too and it will make it easier to
support other similar platform DIO headers (such as the ones on the
TS-5600). But I'm not sure about the implementation, I'm thinking about
an array of struct ts5500_dio per block, described in a
MODULE_DEVICE_TABLE. Something like:

	static struct platform_device_id ts5500_device_ids[] = { 
		{ "ts5500-dio1", (kernel_ulong_t) ts5500_dio1 },
		{ "ts5500-dio2", (kernel_ulong_t) ts5500_dio2 },
		{ "ts5500-lcd", (kernel_ulong_t) ts5500_lcd },
		{ }
	};
	MODULE_DEVICE_TABLE(platform, ts5500_device_ids);

What do you think?

> 
> > + * The datasheet is available at:
> > + * http://embeddedx86.com/documentation/ts-5500-manual.pdf.
> 
> Drop the period after the URL. It makes it hard to browse...
> 
> > +/*
> > + * This array describes the names of the DIO lines, but also the mapping between
> > + * the datasheet, and corresponding offsets exposed by the driver.
> > + */
> > +static const char * const ts5500_pinout[38] = {
> > +       /* DIO1 Header (offset 0-13) */
> > +       [0]  = "DIO1_0",  /* pin 1  */
> > +       [1]  = "DIO1_1",  /* pin 3  */
> 
> Pins pins pins. The pinctrl subsystem has a framework for keeping track of
> pins and giving them names, which is another reason to convert this to a
> pinctrl driver.
> 
> Please consult Documentation/pinctrl.txt

Sounds better then, thanks.

> 
> > +#define IN     (1 << 0)
> > +#define OUT    (1 << 1)
> 
> Why not use a bool named "out" then it's out if true
> else input.

Because there are 3 possible cases: input-only, input-output and
output-only. Would it be better to have 2 booleans, "in" and "out"?

> 
> > +#ifndef NO_IRQ
> > +#define NO_IRQ -1
> > +#endif
> 
> No. We have very solidly decided that NO_IRQ == 0.

Ho ok, I didn't know that. Many implementations still use -1.
> 
> > +/*
> > + * This structure is used to describe capabilities of DIO lines,
> > + * such as available directions, and mapped IRQ (if any).
> > + */
> > +struct ts5500_dio {
> > +       const unsigned long value_addr;
> > +       const int value_bit;
> > +       const unsigned long control_addr;
> > +       const int control_bit;
> 
> All the above seem like they should be u8 rather than
> const unsigned or const int.

I used these types here to match {set,clear}_bit function prototypes.
But I can use const u8 for sure.

> 
> > +       const int irq;
> 
> IRQ numbers shall not be hard-coded into drivers like
> this, they shall be passed in as resources from the outside
> just like you pass in other platform data.

Just curious, what's the point of doing this, as IRQ lines are wired?

> 
> > +       const int direction;
> 
> Use a bool out for this last thing?

Or two bool in and out instead?

> 
> > +};
> > +
> > +static const struct ts5500_dio ts5500_dios[] = {
> > +       /* DIO1 Header (offset 0-13) */
> > +       [0]  = { 0x7b, 0, 0x7a, 0,  NO_IRQ, IN | OUT },
> 
> Use C99 syntax and skip the [0] index (etc).

I used this syntax because it is much more clearer to figure out what
Linux offset corresponds to the physical DIO, but well...

> 
> static const struct ts5500_dio ts5500_dios[] = {
>     {
>         .value_addr = 0x7b,
>         .value_bit = 0,
>         .control_addr = 0x7a,
>         .control_bit = 0,
>         .direction = OUT,
>     },
>     {
>         .value_addr = 0x7b
> ....
> 
> Note absence of NO_IRQ - it's implicit zero in static
> allocations.

This will make the code longer, but this is more explicit.
I should maybe group addr/bit pairs in a struct...

> 
> > +static bool lcd_dio;
> > +static bool lcd_irq;
> > +static bool dio1_irq;
> > +static bool dio2_irq;
> 
> Document what these are for with some /* comments */
> 
> But overall this has a singleton problem, this driver is not reentrant.
> Usually that doesn't matter on a practical system, but we sure
> prefer if you create a state container:
> 
> struct ts5500dio_state {
>   bool lcd_dio;
>   bool lcd_irq;
>   bool dio1_irq;
>   bool dio2_irq;
> }
> 
> In .probe:
> 
> struct ts5500dio_state *my_state = devm_kzalloc(&dev, sizeof(struct
> foo_state), GFP_KERNEL);
> 
> my_state->lcd_dio = ...
> 
> etc.

I agree with you about the singleton problem, but the issue here is that
gpio_chip_add() can be called before alloc functions exist, if the
module is built-in. As an usage example, here's my current
implementation of the TS-5500 platform code:

https://raw.github.com/v0n/linux-ts5500/ts5500/arch/x86/platform/ts5500/ts5500.c

With this module built-in with a kzalloc call, it totally crashes on
boot. Would it be the same problem with the pinctrl subsystem? Or is
there any other workaround? Maybe my platform code should try using a
later init call, instead of device_initcall().

> 
> > +static DEFINE_SPINLOCK(lock);
> 
> This would also go into the state container.
> 
> > +static inline void io_set_bit(int bit, unsigned long addr)
> 
> io_set_bit() is too generic, use something like ts5500dio_set_bit()
> 
> > +{
> > +       unsigned long val = inb(addr);
> 
> I'm not familiar with inb() and friends, I guess it's IO-space
> accessors so I just buy into it...
> 
> > +       __set_bit(bit, &val);
> 
> Do you have to use the __ variants? Isn't just set_bit() working?
> (Just checking...)

I used __ variants here because I didn't require it to be atomic, as I
protect the calls myself with a spinlock.

> 
> > +       outb(val, addr);
> > +}
> > +
> > +static inline void io_clear_bit(int bit, unsigned long addr)
> > +{
> > +       unsigned long val = inb(addr);
> > +       __clear_bit(bit, &val);
> > +       outb(val, addr);
> > +}
> 
> Same comments.
> 
> > +static int ts5500_gpio_input(struct gpio_chip *chip, unsigned offset)
> > +{
> > +       unsigned long flags;
> > +       const struct ts5500_dio line = ts5500_dios[offset];
> > +
> > +       /* Some lines cannot be configured as input */
> > +       if (!(line.direction & IN))
> > +               return -ENXIO;
> 
> Why are you using that return code? (Probably OK, just want
> a rationale...)

I took this return value from another driver. Is there a better one to
use here?

> 
> (...)
> > +static int ts5500_gpio_get(struct gpio_chip *chip, unsigned offset)
> > +{
> > +       const struct ts5500_dio line = ts5500_dios[offset];
> > +
> > +       return (inb(line.value_addr) >> line.value_bit) & 1;
> 
> Use this idiom:
> 
> return !!(inb(line.value_addr) & line.value_bit);
> 
> It's quite a common way to clamp a numeral to a bool int.

Ok.

> 
> (...)
> > +static int ts5500_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> > +{
> > +       const struct ts5500_dio line = ts5500_dios[offset];
> > +
> > +       /* Only a few lines are IRQ-capable */
> > +       if (line.irq != NO_IRQ)
> > +               return line.irq;
> > +
> > +       /* This allows to bridge a line with the IRQ line of the same header */
> > +       if (dio1_irq && offset < 13)
> > +               return ts5500_dios[13].irq;
> > +       if (dio2_irq && offset > 13 && offset < 26)
> > +               return ts5500_dios[26].irq;
> > +       if (lcd_irq && offset > 26 && offset < 37)
> > +               return ts5500_dios[37].irq;
> 
> Don't do this. Please use irqdomain for converting physical
> IRQ numbers to Linux IRQ numbers. (Consult other GPIO
> drivers for examples.)
> 
> These magic constants (13, 26, 37) are scary too.
> 
> You should not try to handle each block as a single
> IRQ, instead instatiate a struct irq_chip in the driver
> and let that use irqdomain do demux the IRQ and
> register a range of Linux IRQ numbers, on per pin,
> so the GPIO driver will handle the physical IRQ line,
> then dispatch to a fan-out handler, so drivers that need
> IRQs from the GPIO chip just register IRQ handlers like
> anyone else.

Do you mean that I should not return the same IRQ line for the same
header, but virtual ones? I'll try to find a good example for that.

> 
> (...)
> > +static int __devinit ts5500_gpio_probe(struct platform_device *pdev)
> > +{
> > +       int ret;
> > +       unsigned long flags;
> > +       struct ts5500_gpio_platform_data *pdata = pdev->dev.platform_data;
> 
> This is where you should allocate you state container dynamically.
> 
> (...)
> > +       /* Enable IRQ generation */
> > +       spin_lock_irqsave(&lock, flags);
> > +       io_set_bit(7, 0x7a); /* DIO1_13 on IRQ7 */
> > +       io_set_bit(7, 0x7d); /* DIO2_13 on IRQ6 */
> > +       if (lcd_dio) {
> > +               io_clear_bit(4, 0x7d); /* LCD Header usage as DIO */
> > +               io_set_bit(6, 0x7d); /* LCD_RS on IRQ1 */
> > +       }
> 
> This is pincontrol ... but well. It's very very little after all.
> 
> > +/**
> > + * struct ts5500_gpio_platform_data - TS-5500 GPIO configuration
> > + * @base:      The GPIO base number to use.
> > + * @lcd_dio:   Use the LCD port as 11 additional digital I/O lines.
> > + * @lcd_irq:   Return IRQ1 for every line of LCD DIO header.
> > + * @dio1_irq:  Return IRQ7 for every line of DIO1 header.
> > + * @dio2_irq:  Return IRQ6 for every line of DIO2 header.
> > + */
> > +struct ts5500_gpio_platform_data {
> > +       int base;
> > +       bool lcd_dio;
> > +       bool lcd_irq;
> > +       bool dio1_irq;
> > +       bool dio2_irq;
> > +};
> 
> So I don't like how this hardcodes IRQ 1, 7, 6 to be used for this
> purpose, it's better to pass that in as resources from the platform.
> 
> Yours,
> Linus Walleij

Thanks for your comments,
Vivien

Re: [PATCH] gpio: add TS-5500 DIO headers support

[Linus Walleij]

On Mon, Oct 8, 2012 at 8:20 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> On Mon, 2012-10-08 at 12:38 +0200, Linus Walleij wrote:
>> On Wed, Sep 26, 2012 at 2:42 AM, Vivien Didelot

>> <vivien.didelot@savoirfairelinux.com> wrote:
>> Part of me dislike that you create one single driver for all
>> three blocks instead of abstracting the driver to handle one
>> block, and register one platform device each, 2-3 times.
>> But given that this is very tied to this one chip it could be the
>> simplest and most readable design so OK...
>
> I agree with you. I thought about that too and it will make it easier to
> support other similar platform DIO headers (such as the ones on the
> TS-5600). But I'm not sure about the implementation, I'm thinking about
> an array of struct ts5500_dio per block, described in a
> MODULE_DEVICE_TABLE. Something like:
>
>         static struct platform_device_id ts5500_device_ids[] = {
>                 { "ts5500-dio1", (kernel_ulong_t) ts5500_dio1 },
>                 { "ts5500-dio2", (kernel_ulong_t) ts5500_dio2 },
>                 { "ts5500-lcd", (kernel_ulong_t) ts5500_lcd },
>                 { }
>         };
>         MODULE_DEVICE_TABLE(platform, ts5500_device_ids);
>
> What do you think?

It basically looks pretty nice, but can't tell until I see the entire
code...

>> > +static const char * const ts5500_pinout[38] = {
>> > +       /* DIO1 Header (offset 0-13) */
>> > +       [0]  = "DIO1_0",  /* pin 1  */
>> > +       [1]  = "DIO1_1",  /* pin 3  */
>>
>> Pins pins pins. The pinctrl subsystem has a framework for keeping track of
>> pins and giving them names, which is another reason to convert this to a
>> pinctrl driver.
>>
>> Please consult Documentation/pinctrl.txt
>
> Sounds better then, thanks.

Well that may also be a pretty big step if you just want to mux
one bank of GPIO. I'm a bit ambivalent. But if you want to tie
pin and gpio information together and name all pins, pinctrl
is what should suit you best.

In the GPIO world, things are opaque "gpios" not really pins.

>> > +#ifndef NO_IRQ
>> > +#define NO_IRQ -1
>> > +#endif
>>
>> No. We have very solidly decided that NO_IRQ == 0.
>
> Ho ok, I didn't know that. Many implementations still use -1.

They are bad examples not to be followed.
http://lwn.net/Articles/470820/

>> > +/*
>> > + * This structure is used to describe capabilities of DIO lines,
>> > + * such as available directions, and mapped IRQ (if any).
>> > + */
>> > +struct ts5500_dio {
>> > +       const unsigned long value_addr;
>> > +       const int value_bit;
>> > +       const unsigned long control_addr;
>> > +       const int control_bit;
>>
>> All the above seem like they should be u8 rather than
>> const unsigned or const int.
>
> I used these types here to match {set,clear}_bit function prototypes.
> But I can use const u8 for sure.

Hm hm yes that is true. These functions only work on longs,
as they access the entire word from memory.

If you access these as u8, use direct operators rather than
set/clear_bit & friends.

No big deal really. Do it as it works best for you.

>> > +       const int irq;
>>
>> IRQ numbers shall not be hard-coded into drivers like
>> this, they shall be passed in as resources from the outside
>> just like you pass in other platform data.
>
> Just curious, what's the point of doing this, as IRQ lines are wired?

It's a design pattern that only the platform knows about the IRQs
and they should be retrieved from there.

Resources tied to platform device is one way to get them, if they are
hardcoded.

Device Tree is another way on MIPS e.g.

And ACPI etc is used in some x86's I think.

Just that this kind of stuff which basically pertains to how the interrupt
controller is wired rather than this hardware block per se, means it
should be external.

>> > +       const int direction;
>>
>> Use a bool out for this last thing?
>
> Or two bool in and out instead?

It if can be in and out at the same time, sure.
I'm probably just not getting it.

>> > +};
>> > +
>> > +static const struct ts5500_dio ts5500_dios[] = {
>> > +       /* DIO1 Header (offset 0-13) */
>> > +       [0]  = { 0x7b, 0, 0x7a, 0,  NO_IRQ, IN | OUT },
>>
>> Use C99 syntax and skip the [0] index (etc).
>
> I used this syntax because it is much more clearer to figure out what
> Linux offset corresponds to the physical DIO, but well...

OK you can keep the offset if you like no big deal.

>> static const struct ts5500_dio ts5500_dios[] = {
>>     {
>>         .value_addr = 0x7b,
>>         .value_bit = 0,
>>         .control_addr = 0x7a,
>>         .control_bit = 0,
>>         .direction = OUT,
>>     },
>>     {
>>         .value_addr = 0x7b
>> ....
>>
>> Note absence of NO_IRQ - it's implicit zero in static
>> allocations.
>
> This will make the code longer, but this is more explicit.
> I should maybe group addr/bit pairs in a struct...

I really prefer if you do this because it makes the code way more
maintainable, because else, if you add a member to the struct
in the middle of the others, guess what happens.

>> > +static bool lcd_dio;
>> > +static bool lcd_irq;
>> > +static bool dio1_irq;
>> > +static bool dio2_irq;
>>
>> Document what these are for with some /* comments */
>>
>> But overall this has a singleton problem, this driver is not reentrant.
>> Usually that doesn't matter on a practical system, but we sure
>> prefer if you create a state container:
>>
>> struct ts5500dio_state {
>>   bool lcd_dio;
>>   bool lcd_irq;
>>   bool dio1_irq;
>>   bool dio2_irq;
>> }
>>
>> In .probe:
>>
>> struct ts5500dio_state *my_state = devm_kzalloc(&dev, sizeof(struct
>> foo_state), GFP_KERNEL);
>>
>> my_state->lcd_dio = ...
>>
>> etc.
>
> I agree with you about the singleton problem, but the issue here is that
> gpio_chip_add() can be called before alloc functions exist, if the
> module is built-in.

This sounds plain wrong, a module_platform_driver() is called at the
device init level and memory allocation is available.

> As an usage example, here's my current
> implementation of the TS-5500 platform code:
>
> https://raw.github.com/v0n/linux-ts5500/ts5500/arch/x86/platform/ts5500/ts5500.c
>
> With this module built-in with a kzalloc call, it totally crashes on
> boot.

Wicked, but probably an unrelated problem.
Please find out if it's really kzalloc() that fails and in that case why.

> Would it be the same problem with the pinctrl subsystem? Or is
> there any other workaround? Maybe my platform code should try using a
> later init call, instead of device_initcall().

You should be able to allocate memory at very early init calls,
this is done throughout the kernel and should not be a limitation,
you need to figure out why you can't allocate memory instead
of coding drivers that look odd and awkward just because you try
to avoid allocating memory.

>> > +       __set_bit(bit, &val);
>>
>> Do you have to use the __ variants? Isn't just set_bit() working?
>> (Just checking...)
>
> I used __ variants here because I didn't require it to be atomic, as I
> protect the calls myself with a spinlock.

OK true, that's fine.

And this is one of the calls that won't work if the thing you operate
on isn't a long, so I get this now.

The alternative is to do something explicit like:

#include <linux/bitops.h>

val |= BIT(bit);

which will work just fine also with u8 :-)

>> > +static inline void io_clear_bit(int bit, unsigned long addr)
>> > +{
>> > +       unsigned long val = inb(addr);
>> > +       __clear_bit(bit, &val);

Or

val &= ~BIT(bit);

in this case.

>> > +       /* Some lines cannot be configured as input */
>> > +       if (!(line.direction & IN))
>> > +               return -ENXIO;
>>
>> Why are you using that return code? (Probably OK, just want
>> a rationale...)
>
> I took this return value from another driver. Is there a better one to
> use here?

Nah it's fine as long as there is some rationale.

>> (...)
>> > +static int ts5500_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>> > +{
>> > +       const struct ts5500_dio line = ts5500_dios[offset];
>> > +
>> > +       /* Only a few lines are IRQ-capable */
>> > +       if (line.irq != NO_IRQ)
>> > +               return line.irq;
>> > +
>> > +       /* This allows to bridge a line with the IRQ line of the same header */
>> > +       if (dio1_irq && offset < 13)
>> > +               return ts5500_dios[13].irq;
>> > +       if (dio2_irq && offset > 13 && offset < 26)
>> > +               return ts5500_dios[26].irq;
>> > +       if (lcd_irq && offset > 26 && offset < 37)
>> > +               return ts5500_dios[37].irq;
>>
>> Don't do this. Please use irqdomain for converting physical
>> IRQ numbers to Linux IRQ numbers. (Consult other GPIO
>> drivers for examples.)
>>
>> These magic constants (13, 26, 37) are scary too.
>>
>> You should not try to handle each block as a single
>> IRQ, instead instatiate a struct irq_chip in the driver
>> and let that use irqdomain do demux the IRQ and
>> register a range of Linux IRQ numbers, on per pin,
>> so the GPIO driver will handle the physical IRQ line,
>> then dispatch to a fan-out handler, so drivers that need
>> IRQs from the GPIO chip just register IRQ handlers like
>> anyone else.
>
> Do you mean that I should not return the same IRQ line for the same
> header, but virtual ones? I'll try to find a good example for that.

Basically Linux IRQs (also sometimes called virtual IRQs) are
separate from the physical IRQ numbers of the system.

i.e. what you see in /proc/interrupts has no relation to the physical
interrupt lines.

Keep in mind that we're trying to disallow IRQ 0 altogether and some
platforms use that physical line for stuff.

So we need to use irqdomain to just grab an IRQ number to reference
the physical line. And we often do that for the IRQ controller.

The fact that sometimes the physical line number and the Linux
IRQ number correspond is just misleading...

In this case, since you have individual IRQs you want to check
for different lines, register something with e.g.
irq_domain_add_simple() to handle all these lines as IRQs.

It's a bit complex but pays off: all of a sudden you get statistics
in /proc/interrupts for exactly which GPIO IRQs were fired,
for example, and they get names if you provide that.

Look at the other GPIO drivers for many good examples of
how to do this. gpio-em.c is one example.

Yours,
Linus Walleij

Re: [PATCH] gpio: add TS-5500 DIO headers support

[Vivien Didelot]

Hi Linus,

On Fri, 2012-10-12 at 22:53 +0200, Linus Walleij wrote:
> Well that may also be a pretty big step if you just want to mux
> one bank of GPIO. I'm a bit ambivalent. But if you want to tie
> pin and gpio information together and name all pins, pinctrl
> is what should suit you best.
> 
> In the GPIO world, things are opaque "gpios" not really pins. 

Thanks a lot for all your comments. I think I'll stick with the GPIO
framework for the moment, trying to keep it as simple as possible.

About the generic driver (to allow registering one platform device per
DIO block), I think it won't be possible, because there are shared
regions, such as 0x7d, used by DIO2 and LCD DIO for direction...

Is there a way to share this, or does this mean that this driver should
handle the 3 blocks as it already does?

Thanks,
Vivien

Re: [PATCH] gpio: add TS-5500 DIO headers support

[Linus Walleij]

On Sat, Oct 13, 2012 at 12:04 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:

> About the generic driver (to allow registering one platform device per
> DIO block), I think it won't be possible, because there are shared
> regions, such as 0x7d, used by DIO2 and LCD DIO for direction...

Probably true.

> Is there a way to share this, or does this mean that this driver should
> handle the 3 blocks as it already does?

I think you should try to handle all 3 for the moment atleast.

Yours,
Linus Walleij

Re: [PATCH] gpio: add TS-5500 DIO headers support

[Vivien Didelot]

Hi Linus,

I rewrote some parts according to your comments, but I still have some
concerns.

On Fri, 2012-10-12 at 22:53 +0200, Linus Walleij wrote:
> >> (...)
> >> > +static int ts5500_gpio_to_irq(struct gpio_chip *chip, unsigned
> offset)
> >> > +{
> >> > +       const struct ts5500_dio line = ts5500_dios[offset];
> >> > +
> >> > +       /* Only a few lines are IRQ-capable */
> >> > +       if (line.irq != NO_IRQ)
> >> > +               return line.irq;
> >> > +
> >> > +       /* This allows to bridge a line with the IRQ line of the
> same header */
> >> > +       if (dio1_irq && offset < 13)
> >> > +               return ts5500_dios[13].irq;
> >> > +       if (dio2_irq && offset > 13 && offset < 26)
> >> > +               return ts5500_dios[26].irq;
> >> > +       if (lcd_irq && offset > 26 && offset < 37)
> >> > +               return ts5500_dios[37].irq;
> >>
> >> Don't do this. Please use irqdomain for converting physical
> >> IRQ numbers to Linux IRQ numbers. (Consult other GPIO
> >> drivers for examples.)
> >>
> >> These magic constants (13, 26, 37) are scary too.
> >>
> >> You should not try to handle each block as a single
> >> IRQ, instead instatiate a struct irq_chip in the driver
> >> and let that use irqdomain do demux the IRQ and
> >> register a range of Linux IRQ numbers, on per pin,
> >> so the GPIO driver will handle the physical IRQ line,
> >> then dispatch to a fan-out handler, so drivers that need
> >> IRQs from the GPIO chip just register IRQ handlers like
> >> anyone else.
> >
> > Do you mean that I should not return the same IRQ line for the same
> > header, but virtual ones? I'll try to find a good example for that.
> 
> Basically Linux IRQs (also sometimes called virtual IRQs) are
> separate from the physical IRQ numbers of the system.
> 
> i.e. what you see in /proc/interrupts has no relation to the physical
> interrupt lines.
> 
> Keep in mind that we're trying to disallow IRQ 0 altogether and some
> platforms use that physical line for stuff.
> 
> So we need to use irqdomain to just grab an IRQ number to reference
> the physical line. And we often do that for the IRQ controller.
> 
> The fact that sometimes the physical line number and the Linux
> IRQ number correspond is just misleading...
> 
> In this case, since you have individual IRQs you want to check
> for different lines, register something with e.g.
> irq_domain_add_simple() to handle all these lines as IRQs.
> 
> It's a bit complex but pays off: all of a sudden you get statistics
> in /proc/interrupts for exactly which GPIO IRQs were fired,
> for example, and they get names if you provide that.
> 
> Look at the other GPIO drivers for many good examples of
> how to do this. gpio-em.c is one example. 

I looked at some drivers and if I'm not mistaken, this case is
different. Technologic Systems platforms (such as the TS-5500) have
several pin blocks. Each block has input-only, input-output or
output-only pins. Only one pin per block is connected to an interrupt
line. But sadly, these interrupt-connected lines are input only.
Here are the details about the TS-5500 pin block "DIO1":

http://wiki.embeddedarm.com/wiki/TS-5500#DIO1_Header

Some GPIO devices need a bidirectional data line which can trigger an
IRQ. In this case, we use a bidirectional pin for data, that we strap to
the IRQ-able pin.

Basically, our setup looks like that:

    +---+ in-only+IRQ
    | D |-------------+  data +--------+
    | I | in/out pin  |-------|  GPIO  |
    | O |-------------+   clk | device |
    | 1 |---------------------|(SHT15) |
    +---+ in/out pin          +--------+

That's why I previously used a dio1_irq platform data field, to return
the interrupt connected to the IRQ-able pin for any GPIO on DIO1, in the
gpio_to_irq() implementation.

A Linux IRQ per pin doesn't seem to be possible because the
irq_create_mapping() documentation says that "Only one mapping per
hardware interrupt is permitted." Should I still implement the
irq_chip/irqdomain for a single IRQ per block? For each pin?
What do you think about this implementation?


Yours,
Vivien

Re: [PATCH] gpio: add TS-5500 DIO headers support

[Linus Walleij]

On Thu, Dec 6, 2012 at 4:49 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:

> I looked at some drivers and if I'm not mistaken, this case is
> different. Technologic Systems platforms (such as the TS-5500) have
> several pin blocks. Each block has input-only, input-output or
> output-only pins. Only one pin per block is connected to an interrupt
> line. But sadly, these interrupt-connected lines are input only.
> Here are the details about the TS-5500 pin block "DIO1":
>
> http://wiki.embeddedarm.com/wiki/TS-5500#DIO1_Header

Aha I get it now. How odd ... OK we need to support this too.

> That's why I previously used a dio1_irq platform data field, to return
> the interrupt connected to the IRQ-able pin for any GPIO on DIO1, in the
> gpio_to_irq() implementation.

OK I get it.

> A Linux IRQ per pin doesn't seem to be possible because the
> irq_create_mapping() documentation says that "Only one mapping per
> hardware interrupt is permitted." Should I still implement the
> irq_chip/irqdomain for a single IRQ per block? For each pin?
> What do you think about this implementation?

So basically there are in total three pins and yeah this is too
little and too strange to warrant an irqdomain so go ahead
with it as it looks and fix the other comments. I'm happy with
keeping this now.

Linus Walleij