[RFC v4 0/2] gpio: add DT support for basic memory-mapped GPIOs

[RFC v4 0/2] gpio: add DT support for basic memory-mapped GPIOs

[Christian Lamparter]

This patch series adds device tree support for basic memory-mapped GPIOs.
The GPIO library already allows drivers and architecture support code to
reuse generic code for manageing a GPIO chip. Currently, a developer has
to create a platform device "basic-mmio-gpio" and attach a bgpio_pdata
platform data structure to make use of it. However, for architectures
which rely on the device tree to enumerate devices, creating custom
platform devices is another extra step that can be avoided by having
direct support via a device tree binding.

I initially came across this patch [0] from Álvaro Fernández Rojas,
while looking for an easy way to add support for the GPIO of my
WD MyBook Live [1] (APM82181 - ppc464). This generic approach patch
allowed me to easily get the GPIO (and the connected LEDs,
buttons, gpiohogs, etc.) up and running. Even tought, Mr. Fernandez
initially developed it for his work on the brcm63xx [2].

[0] <https://patchwork.ozlabs.org/patch/422121/>
[1] <https://github.com/chunkeey/MBL-openwrt>
[2] <https://wiki.openwrt.org/doc/hardware/soc/soc.broadcom.bcm63xx>

---
Note: Since it was such a long time ago, I had issues with getting
all the replies from lkml.org (the side simply browned-out some
replies and comments to the original, v2 and v3 series. So please
if you have already had a comment on the previous patch that wasn't
implemented in this RFC, let me know.

Thanks! (Please keep me in the CC)
---

Christian Lamparter (1):
  gpio: generic: fix GPIO_GENERIC_PLATFORM is set to module case

Álvaro Fernández Rojas (2):
  gpio: dt-bindings: add basic-mmio-gpio bindings
  gpio: generic: add DT support for basic memory-mapped GPIOs

 .../devicetree/bindings/gpio/basic-mmio-gpio.txt   | 73 ++++++++++++++++++++++
 drivers/gpio/gpio-generic.c                        | 72 ++++++++++++++++++++-
 2 files changed, 142 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/basic-mmio-gpio.txt

-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v3 1/3] gpio: dt-bindings: add basic-mmio-gpio bindings

[Christian Lamparter]

From: Álvaro Fernández Rojas <noltari-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

This patch adds the device tree bindings for the basic-mmio-gpio.
The basic-mmio-gpio is already part of a the GPIO generic library
and shares its compatible with the platform device.

Signed-off-by: Álvaro Fernández Rojas <noltari-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 .../devicetree/bindings/gpio/basic-mmio-gpio.txt   | 73 ++++++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/basic-mmio-gpio.txt

diff --git a/Documentation/devicetree/bindings/gpio/basic-mmio-gpio.txt b/Documentation/devicetree/bindings/gpio/basic-mmio-gpio.txt
new file mode 100644
index 0000000..5efb155
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/basic-mmio-gpio.txt
@@ -0,0 +1,73 @@
+Bindings for the generic driver for memory-mapped GPIO controllers.
+
+Required properties:
+	- compatible: should be "basic-mmio-gpio"
+	- reg-names: must contain
+		"dat" - data register
+		may contain
+		"set" - data set register
+		"clr" - data clear register
+		"dirout" - direction output register
+		"dirin" - direction input register
+	- reg: address + size pairs describing the GPIO register sets;
+		order must correspond with the order of entries in reg-names
+	- #gpio-cells = must be set to 2
+	- gpio-controller: Marks the device node as a gpio controller.
+
+Optional properties:
+	- ngpio: specifies the number of gpio mapped in the register.
+	- big-endian: force big endian register accesses.
+	- big-endian-byte-order: assign GPIOs in reverse order.
+	- unreadable-reg-set: data set register is not readable.
+	- read-output-reg-set: cache value set for reads.
+	- unreadable-reg-dir: dirout/dirin register is not readable.
+	- no-output: GPIOs are read-only.
+
+The GPIO generic library provides support for basic memory-mapped GPIO
+controllers. The configuration is detected by which resources are present.
+The simplest form of a GPIO controller that the driver support is just a
+single "dat" register, where GPIO state can be read and/or written.
+However, the driver supports far more:
+	- 8/16/32/64 bits registers. The number of GPIOs is automatically
+	  determined by the width of the registers.
+	- GPIO controllers with clear/set registers.
+	- GPIO controllers with a single "dat" register.
+	- Big endian bits/GPIOs ordering.
+
+For setting GPIO's there are three configurations:
+	1. single input/output register resource (named "dat"),
+	2. set/clear pair (named "set" and "clr"),
+	3. single output register resource and single input resource
+	   ("set" and dat").
+
+For setting the GPIO direction, there are three configurations:
+	a. simple bidirection GPIO that requires no configuration.
+	b. an output direction register (named "dirout")
+	   where a 1 bit indicates the GPIO is an output.
+	c. an input direction register (named "dirin")
+	   where a 1 bit indicates the GPIO is an input.
+
+Examples:
+
+	/* Configuration for single input/output register
+	 * for eight simple bidirection GPIOs.
+	 */
+	gpio_a_1 {
+		compatible = "basic-mmio-gpio";
+		reg = <0x18000000 0x1>;
+		reg-names = "dat";
+		#gpio-cells = <2>;
+		gpio-controller;
+	};
+
+	/* Configuration for set/clear pair registers with
+	 * 32 output direction register GPIOs.
+	 */
+	gpio_b_2 {
+		compatible = "basic-mmio-gpio";
+		reg = <0x18000000 0x4>, <0x18000010 0x4>,
+		      <0x18000004 0x4>, <0x18000008 0x4>;
+		reg-names = "dat", "set", "clr", "dirout";
+		#gpio-cells = <2>;
+		gpio-controller;
+	};
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v3 2/3] gpio: generic: add DT support for basic memory-mapped GPIOs

[Christian Lamparter]

From: Álvaro Fernández Rojas <noltari@gmail.com>

This patch adds support for defining basic memory-mapped
GPIOs within the device tree framework.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
 drivers/gpio/gpio-generic.c | 70 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index 54cddfa..f535299 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -61,6 +61,8 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
 #include <linux/bitops.h>
 #include <linux/platform_device.h>
 #include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 static void bgpio_write8(void __iomem *reg, unsigned long data)
 {
@@ -569,6 +571,59 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
 	return devm_ioremap_resource(&pdev->dev, r);
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id  bgpio_of_match[] = {
+	{ .compatible = "basic-mmio-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bgpio_of_match);
+
+static struct bgpio_pdata *bgpio_parse_dt(struct device *dev,
+					  unsigned long *flags)
+{
+	struct bgpio_pdata *pdata;
+	int err;
+
+	if (!of_match_node(bgpio_of_match, dev->of_node))
+		return NULL;
+
+	pdata = devm_kzalloc(dev, sizeof(struct bgpio_pdata),
+			     GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	err = of_property_read_u32(dev->of_node, "ngpio", &pdata->ngpio);
+	if (err && err != -EINVAL)
+		return ERR_PTR(err);
+
+	if (of_device_is_big_endian(dev->of_node))
+		*flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
+
+	if (of_property_read_bool(dev->of_node, "unreadable-reg-set"))
+		*flags |= BGPIOF_UNREADABLE_REG_SET;
+
+	if (of_property_read_bool(dev->of_node, "unreadable-reg-dir"))
+		*flags |= BGPIOF_UNREADABLE_REG_DIR;
+
+	if (of_property_read_bool(dev->of_node, "big-endian-byte-order"))
+		*flags |= BGPIOF_BIG_ENDIAN;
+
+	if (of_property_read_bool(dev->of_node, "read-output-reg-set"))
+		*flags |= BGPIOF_READ_OUTPUT_REG_SET;
+
+	if (of_property_read_bool(dev->of_node, "no-output"))
+		*flags |= BGPIOF_NO_OUTPUT;
+
+	return pdata;
+}
+#else
+static struct bgpio_data *bgpio_parse_dt(struct device *dev,
+					 unsigned long *flags)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF */
+
 static int bgpio_pdev_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -579,10 +634,18 @@ static int bgpio_pdev_probe(struct platform_device *pdev)
 	void __iomem *dirout;
 	void __iomem *dirin;
 	unsigned long sz;
-	unsigned long flags = pdev->id_entry->driver_data;
+	unsigned long flags = 0;
 	int err;
 	struct gpio_chip *gc;
-	struct bgpio_pdata *pdata = dev_get_platdata(dev);
+	struct bgpio_pdata *pdata;
+
+	pdata = bgpio_parse_dt(dev, &flags);
+	if (IS_ERR(pdata)) {
+		return PTR_ERR(pdata);
+	} else if (!pdata) {
+		pdata = dev_get_platdata(dev);
+		flags = pdev->id_entry->driver_data;
+	}
 
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
 	if (!r)
@@ -646,6 +709,9 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
 static struct platform_driver bgpio_driver = {
 	.driver = {
 		.name = "basic-mmio-gpio",
+#ifdef CONFIG_OF
+		.of_match_table = of_match_ptr(bgpio_of_match),
+#endif
 	},
 	.id_table = bgpio_id_table,
 	.probe = bgpio_pdev_probe,
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v3 3/3] gpio: generic: fix GPIO_GENERIC_PLATFORM is set to module case

[Christian Lamparter]

GPIO_GENERIC_PLATFORM is a tristate. If the module option is
selected the resulting gpio-generic.ko will lack most of the
module initialzation and probe code.

Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
 drivers/gpio/gpio-generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index f535299..b2170d0 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -551,7 +551,7 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 }
 EXPORT_SYMBOL_GPL(bgpio_init);
 
-#ifdef CONFIG_GPIO_GENERIC_PLATFORM
+#if IS_ENABLED(CONFIG_GPIO_GENERIC_PLATFORM)
 
 static void __iomem *bgpio_map(struct platform_device *pdev,
 			       const char *name,
-- 
2.8.1

[RFC v3.1 2/3] gpio: generic: add DT support for basic memory-mapped GPIOs

[Christian Lamparter]

From: Álvaro Fernández Rojas <noltari@gmail.com>

This patch adds support for defining basic memory-mapped
GPIOs within the device tree framework.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
fixed bgpio_>>P<<data typo.
---

 drivers/gpio/gpio-generic.c | 70 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index 54cddfa..f535299 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -61,6 +61,8 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
 #include <linux/bitops.h>
 #include <linux/platform_device.h>
 #include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 static void bgpio_write8(void __iomem *reg, unsigned long data)
 {
@@ -569,6 +571,59 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
 	return devm_ioremap_resource(&pdev->dev, r);
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id  bgpio_of_match[] = {
+	{ .compatible = "basic-mmio-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bgpio_of_match);
+
+static struct bgpio_pdata *bgpio_parse_dt(struct device *dev,
+					  unsigned long *flags)
+{
+	struct bgpio_pdata *pdata;
+	int err;
+
+	if (!of_match_node(bgpio_of_match, dev->of_node))
+		return NULL;
+
+	pdata = devm_kzalloc(dev, sizeof(struct bgpio_pdata),
+			     GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	err = of_property_read_u32(dev->of_node, "ngpio", &pdata->ngpio);
+	if (err && err != -EINVAL)
+		return ERR_PTR(err);
+
+	if (of_device_is_big_endian(dev->of_node))
+		*flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
+
+	if (of_property_read_bool(dev->of_node, "unreadable-reg-set"))
+		*flags |= BGPIOF_UNREADABLE_REG_SET;
+
+	if (of_property_read_bool(dev->of_node, "unreadable-reg-dir"))
+		*flags |= BGPIOF_UNREADABLE_REG_DIR;
+
+	if (of_property_read_bool(dev->of_node, "big-endian-byte-order"))
+		*flags |= BGPIOF_BIG_ENDIAN;
+
+	if (of_property_read_bool(dev->of_node, "read-output-reg-set"))
+		*flags |= BGPIOF_READ_OUTPUT_REG_SET;
+
+	if (of_property_read_bool(dev->of_node, "no-output"))
+		*flags |= BGPIOF_NO_OUTPUT;
+
+	return pdata;
+}
+#else
+static struct bgpio_pdata *bgpio_parse_dt(struct device *dev,
+					  unsigned long *flags)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF */
+
 static int bgpio_pdev_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -579,10 +634,18 @@ static int bgpio_pdev_probe(struct platform_device *pdev)
 	void __iomem *dirout;
 	void __iomem *dirin;
 	unsigned long sz;
-	unsigned long flags = pdev->id_entry->driver_data;
+	unsigned long flags = 0;
 	int err;
 	struct gpio_chip *gc;
-	struct bgpio_pdata *pdata = dev_get_platdata(dev);
+	struct bgpio_pdata *pdata;
+
+	pdata = bgpio_parse_dt(dev, &flags);
+	if (IS_ERR(pdata)) {
+		return PTR_ERR(pdata);
+	} else if (!pdata) {
+		pdata = dev_get_platdata(dev);
+		flags = pdev->id_entry->driver_data;
+	}
 
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
 	if (!r)
@@ -646,6 +709,9 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
 static struct platform_driver bgpio_driver = {
 	.driver = {
 		.name = "basic-mmio-gpio",
+#ifdef CONFIG_OF
+		.of_match_table = of_match_ptr(bgpio_of_match),
+#endif
 	},
 	.id_table = bgpio_id_table,
 	.probe = bgpio_pdev_probe,
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v3 1/3] gpio: dt-bindings: add basic-mmio-gpio bindings

[Rob Herring]

On Tue, Apr 26, 2016 at 5:51 PM, Christian Lamparter
<chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> From: Álvaro Fernández Rojas <noltari-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> This patch adds the device tree bindings for the basic-mmio-gpio.
> The basic-mmio-gpio is already part of a the GPIO generic library
> and shares its compatible with the platform device.

These things always start out "simple", "basic" or "generic". Then
people extend them a property or 2 at a time until they are no longer
basic. Make a bunch of GPIO drivers use a generic driver then maybe
I'll be convinced this is a good idea. The concepts of generic code
and generic bindings don't have to be coupled.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v3 1/3] gpio: dt-bindings: add basic-mmio-gpio bindings

[Christian Lamparter]

On Tuesday, April 26, 2016 09:06:14 PM Rob Herring wrote:
> On Tue, Apr 26, 2016 at 5:51 PM, Christian Lamparter
> <chunkeey@googlemail.com> wrote:
> > From: Álvaro Fernández Rojas <noltari@gmail.com>
> >
> > This patch adds the device tree bindings for the basic-mmio-gpio.
> > The basic-mmio-gpio is already part of a the GPIO generic library
> > and shares its compatible with the platform device.
> 
> These things always start out "simple", "basic" or "generic". Then
> people extend them a property or 2 at a time until they are no longer
> basic. Make a bunch of GPIO drivers use a generic driver then maybe
> I'll be convinced this is a good idea.
Well, I mentioned brcm63xx (MIPS) and MyBook Live (PPC). But OK, if you
want me to look at the existing code in the kernel. I found the following
candidates.

1. MOXA ART: "moxa,moxart-gpio" 
http://lxr.free-electrons.com/source/drivers/gpio/gpio-moxart.c
This driver is used by the:
arch/arm/boot/dts/moxart.dtsi

---
diff --git a/arch/arm/boot/dts/moxart.dtsi b/arch/arm/boot/dts/moxart.dtsi
index 1fd27ed..26707d0 100644
--- a/arch/arm/boot/dts/moxart.dtsi
+++ b/arch/arm/boot/dts/moxart.dtsi
@@ -66,8 +66,11 @@
 		gpio: gpio@98700000 {
 			gpio-controller;
 			#gpio-cells = <2>;
-			compatible = "moxa,moxart-gpio";
-			reg = <0x98700000 0xC>;
+			compatible = "basic-mmio-gpio";
+			reg = <0x98700000 0x4
+			       0x98700004 0x4
+			       0x98700008 0x4>;
+			reg-names = "set dat dirout";
 		};
 
 		rtc: rtc {
---

2. GE FPGA based GPIO: "ge,imp3a-gpio", "gef,sbc310-gpio", "gef,sbc610-gpio"
http://lxr.free-electrons.com/source/drivers/gpio/gpio-ge.c

This driver is used by the:
arch/powerpc/boot/dts/fsl/ge_imp3a.dts
arch/powerpc/boot/dts/fsl/gef_ppc9a.dts
arch/powerpc/boot/dts/fsl/gef_sbc610.dts
arch/powerpc/boot/dts/fsl/gef_sbc310.dts

---
diff --git a/arch/powerpc/boot/dts/fsl/gef_ppc9a.dts b/arch/powerpc/boot/dts/fsl/gef_ppc9a.dts
index 0424fc2..20dd084 100644
--- a/arch/powerpc/boot/dts/fsl/gef_ppc9a.dts
+++ b/arch/powerpc/boot/dts/fsl/gef_ppc9a.dts
@@ -112,9 +112,14 @@
 		};
 		gef_gpio: gpio@7,14000 {
 			#gpio-cells = <2>;
-			compatible = "gef,ppc9a-gpio", "gef,sbc610-gpio";
-			reg = <0x7 0x14000 0x24>;
+			compatible = "basic-mmio-gpio";
+			reg = <0x7 0x14000 0x4
+			       0x7 0x14004 0x4
+			       0x7 0x14008 0x4>;
+			reg-names = "dirout dat set";
+			ngpio = <19>;
 			gpio-controller;
+			big-endian;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/ge_imp3a.dts b/arch/powerpc/boot/dts/fsl/ge_imp3a.dts
index a2bb47f..ac5855a 100644
--- a/arch/powerpc/boot/dts/fsl/ge_imp3a.dts
+++ b/arch/powerpc/boot/dts/fsl/ge_imp3a.dts
@@ -93,9 +93,13 @@
 
 		gef_gpio: gpio@4,400 {
 			#gpio-cells = <2>;
-			compatible = "ge,imp3a-gpio";
-			reg = <0x4 0x400 0x24>;
+			compatible = "basic-mmio-gpio";
+			reg = <0x4 0x400 0x02
+			       0x4 0x404 0x02
+			       0x4 0x408 0x02>;
+			reg-names = "dirin dat set";
 			gpio-controller;
+			big-endian;
 		};
 
 		wdt@4,800 {
diff --git a/arch/powerpc/boot/dts/fsl/gef_sbc310.dts b/arch/powerpc/boot/dts/fsl/gef_sbc310.dts
index 84b3d38..f09528f 100644
--- a/arch/powerpc/boot/dts/fsl/gef_sbc310.dts
+++ b/arch/powerpc/boot/dts/fsl/gef_sbc310.dts
@@ -113,9 +113,14 @@
 		};
 		gef_gpio: gpio@4,8000 {
 			#gpio-cells = <2>;
-			compatible = "gef,sbc310-gpio";
-			reg = <0x4 0x8000 0x24>;
+			compatible = "basic-mmio-gpio";
+			reg = <0x4 0x8000 0x4
+			       0x4 0x8004 0x4
+			       0x4 0x8008 0x4>;
+			reg-names = "dirout dat set";
+			ngpio = <6>;
 			gpio-controller;
+			big-endian;
 		};
 	};
 
diff --git a/arch/powerpc/boot/dts/fsl/gef_sbc610.dts b/arch/powerpc/boot/dts/fsl/gef_sbc610.dts
index 974446a..a9d740a 100644
--- a/arch/powerpc/boot/dts/fsl/gef_sbc610.dts
+++ b/arch/powerpc/boot/dts/fsl/gef_sbc610.dts
@@ -110,9 +110,14 @@
 		};
 		gef_gpio: gpio@7,14000 {
 			#gpio-cells = <2>;
-			compatible = "gef,sbc610-gpio";
-			reg = <0x7 0x14000 0x24>;
+			compatible = "basic-mmio-gpio";
+			reg = <0x7 0x14000 0x4
+			       0x7 0x14004 0x4
+			       0x7 0x14008 0x4>;
+			reg-names = "dirout dat set";
+			ngpio = <19>;
 			gpio-controller;
+			big-endian;
 		};
 	};
---

3. CLPS711X GPIO driver: "cirrus,clps711x-gpio"
http://lxr.free-electrons.com/source/drivers/gpio/gpio-clps711x.c
(But there is no DTS which uses the in the kernel)

4. GPIO driver for the TS-4800 board: "technologic,ts4800-gpio"
(Again no DTS in kernel)

What do you think. There's a total of six devices that can make use
of the "basic-mmio-gpio". If this gives me your go-ahead, I'll include
them in the series as proper patches.

Regards,
Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v3 1/3] gpio: dt-bindings: add basic-mmio-gpio bindings

[Rob Herring]

On Wed, Apr 27, 2016 at 5:15 AM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> On Tuesday, April 26, 2016 09:06:14 PM Rob Herring wrote:
>> On Tue, Apr 26, 2016 at 5:51 PM, Christian Lamparter
>> <chunkeey@googlemail.com> wrote:
>> > From: Álvaro Fernández Rojas <noltari@gmail.com>
>> >
>> > This patch adds the device tree bindings for the basic-mmio-gpio.
>> > The basic-mmio-gpio is already part of a the GPIO generic library
>> > and shares its compatible with the platform device.
>>
>> These things always start out "simple", "basic" or "generic". Then
>> people extend them a property or 2 at a time until they are no longer
>> basic. Make a bunch of GPIO drivers use a generic driver then maybe
>> I'll be convinced this is a good idea.
> Well, I mentioned brcm63xx (MIPS) and MyBook Live (PPC). But OK, if you
> want me to look at the existing code in the kernel. I found the following
> candidates.

What I meant was convert the drivers, not the dts files to a generic
driver. There is no benefit here from a dts perspective. It is driver
consolidation that would make all this worthwhile.

> 1. MOXA ART: "moxa,moxart-gpio"
> http://lxr.free-electrons.com/source/drivers/gpio/gpio-moxart.c
> This driver is used by the:
> arch/arm/boot/dts/moxart.dtsi
>
> ---
> diff --git a/arch/arm/boot/dts/moxart.dtsi b/arch/arm/boot/dts/moxart.dtsi
> index 1fd27ed..26707d0 100644
> --- a/arch/arm/boot/dts/moxart.dtsi
> +++ b/arch/arm/boot/dts/moxart.dtsi
> @@ -66,8 +66,11 @@
>                 gpio: gpio@98700000 {
>                         gpio-controller;
>                         #gpio-cells = <2>;
> -                       compatible = "moxa,moxart-gpio";
> -                       reg = <0x98700000 0xC>;
> +                       compatible = "basic-mmio-gpio";
> +                       reg = <0x98700000 0x4
> +                              0x98700004 0x4
> +                              0x98700008 0x4>;
> +                       reg-names = "set dat dirout";

This is not a compatible change. You can't remove the old drivers as
they are needed to work with old dtbs, so there is no gain.

You would need to match on existing compatibles such as
moxa,moxart-gpio and provide a match data struct that has all the info
you are adding here (e.g. data register offset). Then additionally you
could add "basic-mmio-gpio" (I would drop "basic" part) and the
additional data associated with it. But it has to be new properties,
not changing properties. Changing the reg values doesn't work.


> 2. GE FPGA based GPIO: "ge,imp3a-gpio", "gef,sbc310-gpio", "gef,sbc610-gpio"
> 3. CLPS711X GPIO driver: "cirrus,clps711x-gpio"
> 4. GPIO driver for the TS-4800 board: "technologic,ts4800-gpio"
>
> What do you think. There's a total of six devices that can make use
> of the "basic-mmio-gpio". If this gives me your go-ahead, I'll include
> them in the series as proper patches.

Those are a good start. My other worry about this is next someone will
want to add interrupt registers which is probably a majority of GPIO
controllers.

Of course, I'm sure Linus has an opinion on all of this.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v3 1/3] gpio: dt-bindings: add basic-mmio-gpio bindings

[Alexandre Courbot]

On Thu, Apr 28, 2016 at 12:08 AM, Rob Herring <robh+dt@kernel.org> wrote:
> On Wed, Apr 27, 2016 at 5:15 AM, Christian Lamparter
> <chunkeey@googlemail.com> wrote:
>> On Tuesday, April 26, 2016 09:06:14 PM Rob Herring wrote:
>>> On Tue, Apr 26, 2016 at 5:51 PM, Christian Lamparter
>>> <chunkeey@googlemail.com> wrote:
>>> > From: Álvaro Fernández Rojas <noltari@gmail.com>
>>> >
>>> > This patch adds the device tree bindings for the basic-mmio-gpio.
>>> > The basic-mmio-gpio is already part of a the GPIO generic library
>>> > and shares its compatible with the platform device.
>>>
>>> These things always start out "simple", "basic" or "generic". Then
>>> people extend them a property or 2 at a time until they are no longer
>>> basic. Make a bunch of GPIO drivers use a generic driver then maybe
>>> I'll be convinced this is a good idea.
>> Well, I mentioned brcm63xx (MIPS) and MyBook Live (PPC). But OK, if you
>> want me to look at the existing code in the kernel. I found the following
>> candidates.
>
> What I meant was convert the drivers, not the dts files to a generic
> driver. There is no benefit here from a dts perspective. It is driver
> consolidation that would make all this worthwhile.
>
>> 1. MOXA ART: "moxa,moxart-gpio"
>> http://lxr.free-electrons.com/source/drivers/gpio/gpio-moxart.c
>> This driver is used by the:
>> arch/arm/boot/dts/moxart.dtsi
>>
>> ---
>> diff --git a/arch/arm/boot/dts/moxart.dtsi b/arch/arm/boot/dts/moxart.dtsi
>> index 1fd27ed..26707d0 100644
>> --- a/arch/arm/boot/dts/moxart.dtsi
>> +++ b/arch/arm/boot/dts/moxart.dtsi
>> @@ -66,8 +66,11 @@
>>                 gpio: gpio@98700000 {
>>                         gpio-controller;
>>                         #gpio-cells = <2>;
>> -                       compatible = "moxa,moxart-gpio";
>> -                       reg = <0x98700000 0xC>;
>> +                       compatible = "basic-mmio-gpio";
>> +                       reg = <0x98700000 0x4
>> +                              0x98700004 0x4
>> +                              0x98700008 0x4>;
>> +                       reg-names = "set dat dirout";
>
> This is not a compatible change. You can't remove the old drivers as
> they are needed to work with old dtbs, so there is no gain.
>
> You would need to match on existing compatibles such as
> moxa,moxart-gpio and provide a match data struct that has all the info
> you are adding here (e.g. data register offset). Then additionally you
> could add "basic-mmio-gpio" (I would drop "basic" part) and the
> additional data associated with it. But it has to be new properties,
> not changing properties. Changing the reg values doesn't work.
>
>
>> 2. GE FPGA based GPIO: "ge,imp3a-gpio", "gef,sbc310-gpio", "gef,sbc610-gpio"
>> 3. CLPS711X GPIO driver: "cirrus,clps711x-gpio"
>> 4. GPIO driver for the TS-4800 board: "technologic,ts4800-gpio"
>>
>> What do you think. There's a total of six devices that can make use
>> of the "basic-mmio-gpio". If this gives me your go-ahead, I'll include
>> them in the series as proper patches.
>
> Those are a good start. My other worry about this is next someone will
> want to add interrupt registers which is probably a majority of GPIO
> controllers.
>
> Of course, I'm sure Linus has an opinion on all of this.

I'm not Linus, but after a quick look at the drivers mentioned by
Christian I see little room for improvement (but still some - see
below). Like Rob mentioned we want the DT's compatible property to
describe the hardware as precisely as possible. "basic-mmio-gpio" is
not precise and not backward-compatible. The examples listed by
Christian are already all using the generic MMIO driver and are thus
already quite short.

However, they all repeat the same basic sequence to initialize the
generic IOMMU driver, and maybe this could be consolidated in a single
file, with each matching chip having the arguments passed to
bgpio_init() encoded as data (a bit like the simple-panel driver does
- see the platform_of_match member of
drivers/gpu/drm/panel/panel-simple.c)

This would reduce the amount of code that basically does the same
thing, and remove a few files from drivers/gpio. Even there I see that
gpio-clps711x.c has some specifics compared to the other drivers, but
maybe Christian can iron them out.

So without touching the DT I think consolidating the bgpio users in a
fashion similar to what simple-panel does would be worthwhile.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v3 1/3] gpio: dt-bindings: add basic-mmio-gpio bindings

[Christian Lamparter]

On Thursday, April 28, 2016 03:20:39 PM Alexandre Courbot wrote:
> On Thu, Apr 28, 2016 at 12:08 AM, Rob Herring <robh+dt@kernel.org> wrote:
> > On Wed, Apr 27, 2016 at 5:15 AM, Christian Lamparter
> > What I meant was convert the drivers, not the dts files to a generic
> > driver. There is no benefit here from a dts perspective. It is driver
> > consolidation that would make all this worthwhile.
> > [...]
> > My other worry about this is next someone will want to add interrupt
> > registers which is probably a majority of GPIO controllers.
> >
> > Of course, I'm sure Linus has an opinion on all of this.
> 
> I'm not Linus, but after a quick look at the drivers mentioned by
> Christian I see little room for improvement (but still some - see
> below). Like Rob mentioned we want the DT's compatible property to
> describe the hardware as precisely as possible. "basic-mmio-gpio" is
> not precise and not backward-compatible. The examples listed by
> Christian are already all using the generic MMIO driver and are thus
> already quite short.
> [...]
> So without touching the DT I think consolidating the bgpio users in a
> fashion similar to what simple-panel does would be worthwhile.
I'm getting ready to update my series with a patch like that. I would
like to consolidate the gpio drivers into a single file. But for the
v4 series at least the parsers are still residing in their original files
(I hoped this would make the transition from the _probe to the parser
less painless but the diff came out pretty much unreadable). 
I think this would be just a matter of what's preferred.