* [PATCH v9 0/2] gpio: add DT support for memory-mapped GPIOs
@ 2016-05-11 9:34 ` Christian Lamparter
0 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-11 9:34 UTC (permalink / raw)
To: linux-gpio, linux-kernel, linux-arm-kernel
Cc: Christian Lamparter, Álvaro Fernández Rojas,
Alexander Shiyan, Alexandre Courbot, Linus Walleij,
Andy Shevchenko, Joachim Eastwood
This patch series adds device tree support for generic memory-mapped GPIOs.
The GPIO library already allows drivers and architecture support code to
reuse generic code for managing 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). His generic approach patch allowed
me to easily get the GPIO (and the connected LEDs, buttons, gpiohogs)
up and running. Even thought, Mr. Fernandez initially developed it
for his work on the brcm63xx [2].
The third patch (which can be applied at a later time) moves gpio-clps711x,
gpio-ge, gpio-moxart and gpio-ts4800 into gpio-mmio.c. The old driver files
and makefile entries have been removed. The Kconfig entries for these
drivers are kept for compatibility reasons.
And finally, the most important stat about the series:
>>> 328 insertions(+), 380 deletions(-) <<<
It still removes more lines than it adds!
(This also includes the dt binding)
Thanks!
[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>
changelog:
v8 -> v9:
- made dt match table grep-able
- reorganized bgpio_dt_parse
- dropped already applied patch (dt binding) from the series
v7 -> v8:
- removed ngpio property parser and most flags parsers
- converted clps711x to switch case for GPIO
- retained original Kconfig syms which now select GPIO_GENERIC_PLATFORM
v6 -> v7:
- finally made a PATCH series (based on linux-gpio devel branch)
- added Rob Herring's ACK to the dt bindings
- cc'd clps711x, gpio-ge, moxart and ts4800 authors for driver
changes.
v5 -> v6:
- rewrote bindings and driver patch to fit the wd,mbl-gpio
- unified parser code for gpio-ge, gpio-moxart and gpio-ts4800
- fixed gpio chip's base being overwritten with bogus "0"
- fixed compat driver crash when reload gpio-generic.ko module
- dropped already applied patches from the series
- rebased code on linus' devel tree
- moved dt bindings patch to the top of the series
v4 -> v5:
- reverted rename of gpio-mmio.c back to gpio-generic.c
- fixed Andy Shevchenko's comments
- consolidated changes from clps711x, gpio-ge, gpio-moxart and
gpio-ts4800 into one patch.
v3 -> v4:
- renamed gpio-generic.c to gpio-mmio.c
- changed compat. string to "linux,gpio-mmio"
- integrated Cirrus clps711x driver
- integrated GE FGPA gpio-ge driver
- integrated MOXA ART GPIO driver
- integrated TS4800 gpio driver
- reshuffled patches, reworded commits, fixed spelling errors, etc.
Christian Lamparter (1):
gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
Álvaro Fernández Rojas (1):
gpio: mmio: add DT support for memory-mapped GPIOs
drivers/gpio/Kconfig | 16 ++-
drivers/gpio/Makefile | 4 -
drivers/gpio/gpio-clps711x.c | 91 --------------
drivers/gpio/gpio-ge.c | 114 ------------------
drivers/gpio/gpio-mmio.c | 280 ++++++++++++++++++++++++++++++++++++++++++-
drivers/gpio/gpio-moxart.c | 84 -------------
drivers/gpio/gpio-ts4800.c | 81 -------------
7 files changed, 290 insertions(+), 380 deletions(-)
delete mode 100644 drivers/gpio/gpio-clps711x.c
delete mode 100644 drivers/gpio/gpio-ge.c
delete mode 100644 drivers/gpio/gpio-moxart.c
delete mode 100644 drivers/gpio/gpio-ts4800.c
--
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 0/2] gpio: add DT support for memory-mapped GPIOs
@ 2016-05-11 9:34 ` Christian Lamparter
0 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-11 9:34 UTC (permalink / raw)
To: linux-gpio, linux-kernel, linux-arm-kernel
Cc: Christian Lamparter, Álvaro Fernández Rojas,
Alexander Shiyan, Alexandre Courbot, Linus Walleij,
Andy Shevchenko, Joachim Eastwood
This patch series adds device tree support for generic memory-mapped GPIOs.
The GPIO library already allows drivers and architecture support code to
reuse generic code for managing 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). His generic approach patch allowed
me to easily get the GPIO (and the connected LEDs, buttons, gpiohogs)
up and running. Even thought, Mr. Fernandez initially developed it
for his work on the brcm63xx [2].
The third patch (which can be applied at a later time) moves gpio-clps711x,
gpio-ge, gpio-moxart and gpio-ts4800 into gpio-mmio.c. The old driver files
and makefile entries have been removed. The Kconfig entries for these
drivers are kept for compatibility reasons.
And finally, the most important stat about the series:
>>> 328 insertions(+), 380 deletions(-) <<<
It still removes more lines than it adds!
(This also includes the dt binding)
Thanks!
[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>
changelog:
v8 -> v9:
- made dt match table grep-able
- reorganized bgpio_dt_parse
- dropped already applied patch (dt binding) from the series
v7 -> v8:
- removed ngpio property parser and most flags parsers
- converted clps711x to switch case for GPIO
- retained original Kconfig syms which now select GPIO_GENERIC_PLATFORM
v6 -> v7:
- finally made a PATCH series (based on linux-gpio devel branch)
- added Rob Herring's ACK to the dt bindings
- cc'd clps711x, gpio-ge, moxart and ts4800 authors for driver
changes.
v5 -> v6:
- rewrote bindings and driver patch to fit the wd,mbl-gpio
- unified parser code for gpio-ge, gpio-moxart and gpio-ts4800
- fixed gpio chip's base being overwritten with bogus "0"
- fixed compat driver crash when reload gpio-generic.ko module
- dropped already applied patches from the series
- rebased code on linus' devel tree
- moved dt bindings patch to the top of the series
v4 -> v5:
- reverted rename of gpio-mmio.c back to gpio-generic.c
- fixed Andy Shevchenko's comments
- consolidated changes from clps711x, gpio-ge, gpio-moxart and
gpio-ts4800 into one patch.
v3 -> v4:
- renamed gpio-generic.c to gpio-mmio.c
- changed compat. string to "linux,gpio-mmio"
- integrated Cirrus clps711x driver
- integrated GE FGPA gpio-ge driver
- integrated MOXA ART GPIO driver
- integrated TS4800 gpio driver
- reshuffled patches, reworded commits, fixed spelling errors, etc.
Christian Lamparter (1):
gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
Álvaro Fernández Rojas (1):
gpio: mmio: add DT support for memory-mapped GPIOs
drivers/gpio/Kconfig | 16 ++-
drivers/gpio/Makefile | 4 -
drivers/gpio/gpio-clps711x.c | 91 --------------
drivers/gpio/gpio-ge.c | 114 ------------------
drivers/gpio/gpio-mmio.c | 280 ++++++++++++++++++++++++++++++++++++++++++-
drivers/gpio/gpio-moxart.c | 84 -------------
drivers/gpio/gpio-ts4800.c | 81 -------------
7 files changed, 290 insertions(+), 380 deletions(-)
delete mode 100644 drivers/gpio/gpio-clps711x.c
delete mode 100644 drivers/gpio/gpio-ge.c
delete mode 100644 drivers/gpio/gpio-moxart.c
delete mode 100644 drivers/gpio/gpio-ts4800.c
--
2.8.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 0/2] gpio: add DT support for memory-mapped GPIOs
@ 2016-05-11 9:34 ` Christian Lamparter
0 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-11 9:34 UTC (permalink / raw)
To: linux-arm-kernel
This patch series adds device tree support for generic memory-mapped GPIOs.
The GPIO library already allows drivers and architecture support code to
reuse generic code for managing 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). His generic approach patch allowed
me to easily get the GPIO (and the connected LEDs, buttons, gpiohogs)
up and running. Even thought, Mr. Fernandez initially developed it
for his work on the brcm63xx [2].
The third patch (which can be applied at a later time) moves gpio-clps711x,
gpio-ge, gpio-moxart and gpio-ts4800 into gpio-mmio.c. The old driver files
and makefile entries have been removed. The Kconfig entries for these
drivers are kept for compatibility reasons.
And finally, the most important stat about the series:
>>> 328 insertions(+), 380 deletions(-) <<<
It still removes more lines than it adds!
(This also includes the dt binding)
Thanks!
[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>
changelog:
v8 -> v9:
- made dt match table grep-able
- reorganized bgpio_dt_parse
- dropped already applied patch (dt binding) from the series
v7 -> v8:
- removed ngpio property parser and most flags parsers
- converted clps711x to switch case for GPIO
- retained original Kconfig syms which now select GPIO_GENERIC_PLATFORM
v6 -> v7:
- finally made a PATCH series (based on linux-gpio devel branch)
- added Rob Herring's ACK to the dt bindings
- cc'd clps711x, gpio-ge, moxart and ts4800 authors for driver
changes.
v5 -> v6:
- rewrote bindings and driver patch to fit the wd,mbl-gpio
- unified parser code for gpio-ge, gpio-moxart and gpio-ts4800
- fixed gpio chip's base being overwritten with bogus "0"
- fixed compat driver crash when reload gpio-generic.ko module
- dropped already applied patches from the series
- rebased code on linus' devel tree
- moved dt bindings patch to the top of the series
v4 -> v5:
- reverted rename of gpio-mmio.c back to gpio-generic.c
- fixed Andy Shevchenko's comments
- consolidated changes from clps711x, gpio-ge, gpio-moxart and
gpio-ts4800 into one patch.
v3 -> v4:
- renamed gpio-generic.c to gpio-mmio.c
- changed compat. string to "linux,gpio-mmio"
- integrated Cirrus clps711x driver
- integrated GE FGPA gpio-ge driver
- integrated MOXA ART GPIO driver
- integrated TS4800 gpio driver
- reshuffled patches, reworded commits, fixed spelling errors, etc.
Christian Lamparter (1):
gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
?lvaro Fern?ndez Rojas (1):
gpio: mmio: add DT support for memory-mapped GPIOs
drivers/gpio/Kconfig | 16 ++-
drivers/gpio/Makefile | 4 -
drivers/gpio/gpio-clps711x.c | 91 --------------
drivers/gpio/gpio-ge.c | 114 ------------------
drivers/gpio/gpio-mmio.c | 280 ++++++++++++++++++++++++++++++++++++++++++-
drivers/gpio/gpio-moxart.c | 84 -------------
drivers/gpio/gpio-ts4800.c | 81 -------------
7 files changed, 290 insertions(+), 380 deletions(-)
delete mode 100644 drivers/gpio/gpio-clps711x.c
delete mode 100644 drivers/gpio/gpio-ge.c
delete mode 100644 drivers/gpio/gpio-moxart.c
delete mode 100644 drivers/gpio/gpio-ts4800.c
--
2.8.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 1/2] gpio: mmio: add DT support for memory-mapped GPIOs
2016-05-11 9:34 ` Christian Lamparter
@ 2016-05-11 9:34 ` Christian Lamparter
-1 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-11 9:34 UTC (permalink / raw)
To: linux-gpio, linux-kernel, linux-arm-kernel
Cc: Álvaro Fernández Rojas, Alexander Shiyan,
Alexandre Courbot, Linus Walleij, Andy Shevchenko,
Joachim Eastwood, Christian Lamparter
From: Álvaro Fernández Rojas <noltari@gmail.com>
This patch adds support for defining memory-mapped GPIOs which
are compatible with the existing gpio-mmio interface. The generic
library provides support for many memory-mapped GPIO controllers
that are found in various on-board FPGA and ASIC solutions that
are used to control board's switches, LEDs, chip-selects,
Ethernet/USB PHY power, etc.
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").
The configuration is detected by which resources are present.
For the single output register, this drives a 1 by setting a bit
and a zero by clearing a bit. For the set clr pair, this drives
a 1 by setting a bit in the set register and clears it by setting
a bit in the clear register. The configuration is detected by
which resources are present.
For setting the GPIO direction, there are three configurations:
a. simple bidirectional GPIOs 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.
The first user for this binding is "wd,mbl-gpio".
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
drivers/gpio/gpio-mmio.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 66 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 6c1cb3b..f72e40e 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.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,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
return devm_ioremap_resource(&pdev->dev, r);
}
+#ifdef CONFIG_OF
+static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ struct device *dev = &pdev->dev;
+
+ pdata->base = -1;
+
+ if (of_property_read_bool(dev->of_node, "no-output"))
+ *flags |= BGPIOF_NO_OUTPUT;
+
+ return 0;
+}
+
+static const struct of_device_id bgpio_of_match[] = {
+ { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bgpio_of_match);
+
+static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
+ unsigned long *flags)
+{
+ const int (*parse_dt)(struct platform_device *,
+ struct bgpio_pdata *, unsigned long *);
+ struct bgpio_pdata *pdata;
+ int err;
+
+ parse_dt = of_device_get_match_data(&pdev->dev);
+ if (!parse_dt)
+ return NULL;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
+ GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+
+ err = parse_dt(pdev, pdata, flags);
+ if (err)
+ return ERR_PTR(err);
+
+ return pdata;
+}
+#else
+static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
+ unsigned long *flags)
+{
+ return NULL;
+}
+#endif /* CONFIG_OF */
+
static int bgpio_pdev_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -579,10 +633,19 @@ 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(pdev, &flags);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
+
+ 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,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
static struct platform_driver bgpio_driver = {
.driver = {
.name = "basic-mmio-gpio",
+ .of_match_table = of_match_ptr(bgpio_of_match),
},
.id_table = bgpio_id_table,
.probe = bgpio_pdev_probe,
--
2.8.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v9 1/2] gpio: mmio: add DT support for memory-mapped GPIOs
@ 2016-05-11 9:34 ` Christian Lamparter
0 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-11 9:34 UTC (permalink / raw)
To: linux-arm-kernel
From: ?lvaro Fern?ndez Rojas <noltari@gmail.com>
This patch adds support for defining memory-mapped GPIOs which
are compatible with the existing gpio-mmio interface. The generic
library provides support for many memory-mapped GPIO controllers
that are found in various on-board FPGA and ASIC solutions that
are used to control board's switches, LEDs, chip-selects,
Ethernet/USB PHY power, etc.
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").
The configuration is detected by which resources are present.
For the single output register, this drives a 1 by setting a bit
and a zero by clearing a bit. For the set clr pair, this drives
a 1 by setting a bit in the set register and clears it by setting
a bit in the clear register. The configuration is detected by
which resources are present.
For setting the GPIO direction, there are three configurations:
a. simple bidirectional GPIOs 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.
The first user for this binding is "wd,mbl-gpio".
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: ?lvaro Fern?ndez Rojas <noltari@gmail.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
drivers/gpio/gpio-mmio.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 66 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 6c1cb3b..f72e40e 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.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,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
return devm_ioremap_resource(&pdev->dev, r);
}
+#ifdef CONFIG_OF
+static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ struct device *dev = &pdev->dev;
+
+ pdata->base = -1;
+
+ if (of_property_read_bool(dev->of_node, "no-output"))
+ *flags |= BGPIOF_NO_OUTPUT;
+
+ return 0;
+}
+
+static const struct of_device_id bgpio_of_match[] = {
+ { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bgpio_of_match);
+
+static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
+ unsigned long *flags)
+{
+ const int (*parse_dt)(struct platform_device *,
+ struct bgpio_pdata *, unsigned long *);
+ struct bgpio_pdata *pdata;
+ int err;
+
+ parse_dt = of_device_get_match_data(&pdev->dev);
+ if (!parse_dt)
+ return NULL;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
+ GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+
+ err = parse_dt(pdev, pdata, flags);
+ if (err)
+ return ERR_PTR(err);
+
+ return pdata;
+}
+#else
+static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
+ unsigned long *flags)
+{
+ return NULL;
+}
+#endif /* CONFIG_OF */
+
static int bgpio_pdev_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -579,10 +633,19 @@ 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(pdev, &flags);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
+
+ 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,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
static struct platform_driver bgpio_driver = {
.driver = {
.name = "basic-mmio-gpio",
+ .of_match_table = of_match_ptr(bgpio_of_match),
},
.id_table = bgpio_id_table,
.probe = bgpio_pdev_probe,
--
2.8.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
2016-05-11 9:34 ` Christian Lamparter
@ 2016-05-11 9:34 ` Christian Lamparter
-1 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-11 9:34 UTC (permalink / raw)
To: linux-gpio, linux-kernel, linux-arm-kernel
Cc: Christian Lamparter, Álvaro Fernández Rojas,
Alexander Shiyan, Alexandre Courbot, Linus Walleij,
Andy Shevchenko, Joachim Eastwood, Julien Grossholtz,
Martyn Welch, Jonas Jensen
This patch integrates the GPIO drivers for the following
boards, SoCs, etc. into gpio-mmio:
- CLPS711X SoCs
- MOXA ART SoC
- TS-4800 FPGA DIO blocks and compatibles
- GPIO controllers found on some GE Single Board Computers
Cc: Alexander Shiyan <shc_work@mail.ru>
Cc: Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>
Cc: Martyn Welch <martyn.welch@ge.com>
Cc: Jonas Jensen <jonas.jensen@gmail.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
drivers/gpio/Kconfig | 16 +++-
drivers/gpio/Makefile | 4 -
drivers/gpio/gpio-clps711x.c | 91 -------------------
drivers/gpio/gpio-ge.c | 114 -----------------------
drivers/gpio/gpio-mmio.c | 212 +++++++++++++++++++++++++++++++++++++++++++
drivers/gpio/gpio-moxart.c | 84 -----------------
drivers/gpio/gpio-ts4800.c | 81 -----------------
7 files changed, 224 insertions(+), 378 deletions(-)
delete mode 100644 drivers/gpio/gpio-clps711x.c
delete mode 100644 drivers/gpio/gpio-ge.c
delete mode 100644 drivers/gpio/gpio-moxart.c
delete mode 100644 drivers/gpio/gpio-ts4800.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a68d838..e4d1065 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -154,7 +154,7 @@ config GPIO_BRCMSTB
config GPIO_CLPS711X
tristate "CLPS711X GPIO support"
depends on ARCH_CLPS711X || COMPILE_TEST
- select GPIO_GENERIC
+ select GPIO_GENERIC_PLATFORM
help
Say yes here to support GPIO on CLPS711X SoCs.
@@ -196,7 +196,7 @@ config GPIO_ETRAXFS
config GPIO_GE_FPGA
bool "GE FPGA based GPIO"
depends on GE_FPGA
- select GPIO_GENERIC
+ select GPIO_GENERIC_PLATFORM
help
Support for common GPIO functionality provided on some GE Single Board
Computers.
@@ -209,6 +209,14 @@ config GPIO_GENERIC_PLATFORM
tristate "Generic memory-mapped GPIO controller support (MMIO platform device)"
select GPIO_GENERIC
help
+ Select this to support many generic memory-mapped GPIO controllers.
+
+ This driver also includes support for the following GPIOs:
+ CLPS711X SoCs
+ MOXA ART SoC
+ TS-4800 FPGA DIO blocks and compatibles.
+ GPIOs found on some GE Single Board Computers.
+
Say yes here to support basic platform_device memory-mapped GPIO controllers.
config GPIO_GRGPIO
@@ -288,7 +296,7 @@ config GPIO_MM_LANTIQ
config GPIO_MOXART
bool "MOXART GPIO support"
depends on ARCH_MOXART || COMPILE_TEST
- select GPIO_GENERIC
+ select GPIO_GENERIC_PLATFORM
help
Select this option to enable GPIO driver for
MOXA ART SoC devices.
@@ -408,7 +416,7 @@ config GPIO_TS4800
tristate "TS-4800 DIO blocks and compatibles"
depends on OF_GPIO
depends on SOC_IMX51 || COMPILE_TEST
- select GPIO_GENERIC
+ select GPIO_GENERIC_PLATFORM
help
This driver support TS-4800 FPGA GPIO controllers.
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 991598e..d8d63ae 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -31,7 +31,6 @@ obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o
obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
-obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o
obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
@@ -43,7 +42,6 @@ obj-$(CONFIG_GPIO_EM) += gpio-em.o
obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
obj-$(CONFIG_GPIO_ETRAXFS) += gpio-etraxfs.o
obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
-obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
@@ -68,7 +66,6 @@ obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
obj-$(CONFIG_GPIO_MCP23S08) += gpio-mcp23s08.o
obj-$(CONFIG_GPIO_ML_IOH) += gpio-ml-ioh.o
obj-$(CONFIG_GPIO_MM_LANTIQ) += gpio-mm-lantiq.o
-obj-$(CONFIG_GPIO_MOXART) += gpio-moxart.o
obj-$(CONFIG_GPIO_MPC5200) += gpio-mpc5200.o
obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o
obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o
@@ -107,7 +104,6 @@ obj-$(CONFIG_GPIO_TPS65218) += gpio-tps65218.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_TS4800) += gpio-ts4800.o
obj-$(CONFIG_GPIO_TS5500) += gpio-ts5500.o
obj-$(CONFIG_GPIO_TWL4030) += gpio-twl4030.o
obj-$(CONFIG_GPIO_TWL6040) += gpio-twl6040.o
diff --git a/drivers/gpio/gpio-clps711x.c b/drivers/gpio/gpio-clps711x.c
deleted file mode 100644
index 5a69025..0000000
--- a/drivers/gpio/gpio-clps711x.c
+++ /dev/null
@@ -1,91 +0,0 @@
-/*
- * CLPS711X GPIO driver
- *
- * Copyright (C) 2012,2013 Alexander Shiyan <shc_work@mail.ru>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- */
-
-#include <linux/err.h>
-#include <linux/module.h>
-#include <linux/gpio/driver.h>
-#include <linux/platform_device.h>
-
-static int clps711x_gpio_probe(struct platform_device *pdev)
-{
- struct device_node *np = pdev->dev.of_node;
- void __iomem *dat, *dir;
- struct gpio_chip *gc;
- struct resource *res;
- int err, id = np ? of_alias_get_id(np, "gpio") : pdev->id;
-
- if ((id < 0) || (id > 4))
- return -ENODEV;
-
- gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
- if (!gc)
- return -ENOMEM;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- dat = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(dat))
- return PTR_ERR(dat);
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
- dir = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(dir))
- return PTR_ERR(dir);
-
- switch (id) {
- case 3:
- /* PORTD is inverted logic for direction register */
- err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
- NULL, dir, 0);
- break;
- default:
- err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
- dir, NULL, 0);
- break;
- }
-
- if (err)
- return err;
-
- switch (id) {
- case 4:
- /* PORTE is 3 lines only */
- gc->ngpio = 3;
- break;
- default:
- break;
- }
-
- gc->base = id * 8;
- gc->owner = THIS_MODULE;
- platform_set_drvdata(pdev, gc);
-
- return devm_gpiochip_add_data(&pdev->dev, gc, NULL);
-}
-
-static const struct of_device_id __maybe_unused clps711x_gpio_ids[] = {
- { .compatible = "cirrus,clps711x-gpio" },
- { }
-};
-MODULE_DEVICE_TABLE(of, clps711x_gpio_ids);
-
-static struct platform_driver clps711x_gpio_driver = {
- .driver = {
- .name = "clps711x-gpio",
- .of_match_table = of_match_ptr(clps711x_gpio_ids),
- },
- .probe = clps711x_gpio_probe,
-};
-module_platform_driver(clps711x_gpio_driver);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
-MODULE_DESCRIPTION("CLPS711X GPIO driver");
-MODULE_ALIAS("platform:clps711x-gpio");
diff --git a/drivers/gpio/gpio-ge.c b/drivers/gpio/gpio-ge.c
deleted file mode 100644
index 8650b29..0000000
--- a/drivers/gpio/gpio-ge.c
+++ /dev/null
@@ -1,114 +0,0 @@
-/*
- * Driver for GE FPGA based GPIO
- *
- * Author: Martyn Welch <martyn.welch@ge.com>
- *
- * 2008 (c) GE Intelligent Platforms Embedded Systems, Inc.
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2. This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
- */
-
-/* TODO
- *
- * Configuration of output modes (totem-pole/open-drain)
- * Interrupt configuration - interrupts are always generated the FPGA relies on
- * the I/O interrupt controllers mask to stop them propergating
- */
-
-#include <linux/kernel.h>
-#include <linux/io.h>
-#include <linux/slab.h>
-#include <linux/of_device.h>
-#include <linux/of_gpio.h>
-#include <linux/of_address.h>
-#include <linux/module.h>
-#include <linux/gpio/driver.h>
-
-#define GEF_GPIO_DIRECT 0x00
-#define GEF_GPIO_IN 0x04
-#define GEF_GPIO_OUT 0x08
-#define GEF_GPIO_TRIG 0x0C
-#define GEF_GPIO_POLAR_A 0x10
-#define GEF_GPIO_POLAR_B 0x14
-#define GEF_GPIO_INT_STAT 0x18
-#define GEF_GPIO_OVERRUN 0x1C
-#define GEF_GPIO_MODE 0x20
-
-static const struct of_device_id gef_gpio_ids[] = {
- {
- .compatible = "gef,sbc610-gpio",
- .data = (void *)19,
- }, {
- .compatible = "gef,sbc310-gpio",
- .data = (void *)6,
- }, {
- .compatible = "ge,imp3a-gpio",
- .data = (void *)16,
- },
- { }
-};
-MODULE_DEVICE_TABLE(of, gef_gpio_ids);
-
-static int __init gef_gpio_probe(struct platform_device *pdev)
-{
- const struct of_device_id *of_id =
- of_match_device(gef_gpio_ids, &pdev->dev);
- struct gpio_chip *gc;
- void __iomem *regs;
- int ret;
-
- gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
- if (!gc)
- return -ENOMEM;
-
- regs = of_iomap(pdev->dev.of_node, 0);
- if (!regs)
- return -ENOMEM;
-
- ret = bgpio_init(gc, &pdev->dev, 4, regs + GEF_GPIO_IN,
- regs + GEF_GPIO_OUT, NULL, NULL,
- regs + GEF_GPIO_DIRECT, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
- if (ret) {
- dev_err(&pdev->dev, "bgpio_init failed\n");
- goto err0;
- }
-
- /* Setup pointers to chip functions */
- gc->label = devm_kstrdup(&pdev->dev, pdev->dev.of_node->full_name,
- GFP_KERNEL);
- if (!gc->label) {
- ret = -ENOMEM;
- goto err0;
- }
-
- gc->base = -1;
- gc->ngpio = (u16)(uintptr_t)of_id->data;
- gc->of_gpio_n_cells = 2;
- gc->of_node = pdev->dev.of_node;
-
- /* This function adds a memory mapped GPIO chip */
- ret = devm_gpiochip_add_data(&pdev->dev, gc, NULL);
- if (ret)
- goto err0;
-
- return 0;
-err0:
- iounmap(regs);
- pr_err("%s: GPIO chip registration failed\n",
- pdev->dev.of_node->full_name);
- return ret;
-};
-
-static struct platform_driver gef_gpio_driver = {
- .driver = {
- .name = "gef-gpio",
- .of_match_table = gef_gpio_ids,
- },
-};
-module_platform_driver_probe(gef_gpio_driver, gef_gpio_probe);
-
-MODULE_DESCRIPTION("GE I/O FPGA GPIO driver");
-MODULE_AUTHOR("Martyn Welch <martyn.welch@ge.com");
-MODULE_LICENSE("GPL");
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index f72e40e..1de9172 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -586,8 +586,220 @@ static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
return 0;
}
+static int clps711x_parse_dt(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct resource *res;
+ const char *dir_reg_name;
+ int id = np ? of_alias_get_id(np, "gpio") : pdev->id;
+
+ switch (id) {
+ case 0:
+ case 1:
+ case 2:
+ pdata->ngpio = 0; /* determined by register width */
+ dir_reg_name = "dirout";
+ break;
+ case 3:
+ pdata->ngpio = 0; /* determined by register width */
+ /* PORTD is inverted logic for direction register */
+ dir_reg_name = "dirin";
+ break;
+ case 4:
+ pdata->ngpio = 3; /* PORTE is 3 lines only */
+ dir_reg_name = "dirout";
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ pdata->base = id * 8;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+ if (!res->name || strcmp("dat", res->name))
+ res->name = devm_kstrdup(&pdev->dev, "dat", GFP_KERNEL);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res)
+ return -EINVAL;
+ if (!res->name || strcmp(dir_reg_name, res->name))
+ res->name = devm_kstrdup(&pdev->dev, dir_reg_name, GFP_KERNEL);
+
+ return 0;
+}
+
+static int ge_dt_cb(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ struct device_node *np = pdev->dev.of_node;
+
+ pdata->label = devm_kstrdup(&pdev->dev, np->full_name, GFP_KERNEL);
+ if (!pdata->label)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static int moxart_dt_cb(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ pdata->base = 0;
+ pdata->label = "moxart-gpio";
+ return 0;
+}
+
+
+static int ts4800_dt_cb(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ int err;
+
+ err = of_property_read_u32(pdev->dev.of_node, "ngpios", &pdata->ngpio);
+ if (err == -EINVAL) {
+ pdata->ngpio = 16;
+ return 0;
+ }
+
+ return err;
+}
+
+/*
+ * bgpio_init() needs up to five named mmio register resources.
+ * These currently are dat, set, clr, [dirout | dirin]. There
+ * is no particular order or predefined place for the entries.
+ * However, dirout and dirin are mutually exclusive.
+ */
+#define __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES 4
+
+struct compat_gpio_device_data {
+ unsigned int expected_resource_size;
+ unsigned int ngpio;
+ resource_size_t register_width;
+ unsigned long flags;
+ int (*call_back)(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags);
+ struct resource_replacement {
+ resource_size_t start_offset;
+ const char *name;
+ } resources[__NUM_COMPAT_OVERRIDE_MMIO_RESOURCES];
+};
+#undef __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES
+
+#define ADD_COMPAT_REGISTER(_name, _offset) \
+ { .name = (_name), .start_offset = (_offset) }
+
+#define ADD_COMPAT_GPIO(_comp, _sz, _ngpio, _width, _cb, _f, _res...) \
+ { \
+ .compatible = (_comp), \
+ .data = &(struct compat_gpio_device_data) { \
+ .expected_resource_size = (_sz), \
+ .ngpio = (_ngpio), \
+ .register_width = (_width), \
+ .flags = (_f), \
+ .call_back = (_cb), \
+ .resources = { _res }, \
+ } \
+}
+
+#define ADD_COMPAT_GE_GPIO(_name, _ngpio) \
+ ADD_COMPAT_GPIO(_name, 0x24, _ngpio, 0x4, ge_dt_cb, \
+ BGPIOF_BIG_ENDIAN_BYTE_ORDER, \
+ ADD_COMPAT_REGISTER("dat", 0x04), \
+ ADD_COMPAT_REGISTER("set", 0x08), \
+ ADD_COMPAT_REGISTER("dirin", 0x00)) \
+
+static const struct of_device_id compat_gpio_devices[] = {
+ ADD_COMPAT_GE_GPIO("ge,imp3a-gpio", 16),
+ ADD_COMPAT_GE_GPIO("gef,sbc310-gpio", 6),
+ ADD_COMPAT_GE_GPIO("gef,sbc610-gpio", 19),
+ ADD_COMPAT_GPIO("moxa,moxart-gpio", 0xc, 0, 0x4, moxart_dt_cb,
+ BGPIOF_READ_OUTPUT_REG_SET,
+ ADD_COMPAT_REGISTER("dat", 0x04),
+ ADD_COMPAT_REGISTER("set", 0x00),
+ ADD_COMPAT_REGISTER("dirout", 0x08)),
+ ADD_COMPAT_GPIO("technologic,ts4800-gpio", 0x6, 16, 0x2, ts4800_dt_cb,
+ 0, ADD_COMPAT_REGISTER("dat", 0x00),
+ ADD_COMPAT_REGISTER("set", 0x02),
+ ADD_COMPAT_REGISTER("dirout", 0x04)),
+};
+
+#undef ADD_COMPAT_GE_GPIO
+#undef ADD_COMPAT_GPIO
+#undef ADD_COMPAT_REGISTER
+
+static int compat_parse_dt(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ const struct device_node *node = pdev->dev.of_node;
+ const struct compat_gpio_device_data *entry;
+ const struct of_device_id *of_id;
+ struct resource *res;
+ int err;
+
+ of_id = of_match_node(compat_gpio_devices, node);
+ if (!of_id)
+ return -ENODEV;
+
+ entry = of_id->data;
+ if (!entry || !entry->resources[0].name)
+ return -EINVAL;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+
+ if (!res->name || strcmp(entry->resources[0].name, res->name)) {
+ struct resource nres[ARRAY_SIZE(entry->resources)];
+ size_t i;
+
+ if (resource_size(res) != entry->expected_resource_size)
+ return -EINVAL;
+
+ for (i = 0; i < ARRAY_SIZE(entry->resources); i++) {
+ if (!entry->resources[i].name)
+ continue;
+
+ nres[i].name = devm_kstrdup(&pdev->dev,
+ entry->resources[i].name, GFP_KERNEL);
+ nres[i].start = res->start +
+ entry->resources[i].start_offset;
+ nres[i].end = nres[i].start +
+ entry->register_width - 1;
+ nres[i].flags = IORESOURCE_MEM;
+ }
+
+ err = platform_device_add_resources(pdev, nres, i);
+ if (err)
+ return err;
+ }
+
+ pdata->base = -1;
+ pdata->ngpio = entry->ngpio;
+ *flags = entry->flags;
+
+ if (entry->call_back)
+ err = entry->call_back(pdev, pdata, flags);
+
+ return err;
+}
+
static const struct of_device_id bgpio_of_match[] = {
{ .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
+ { .compatible = "cirrus,clps711x-gpio", .data = clps711x_parse_dt },
+ { .compatible = "ge,imp3a-gpio", .data = compat_parse_dt },
+ { .compatible = "gef,sbc310-gpio", .data = compat_parse_dt },
+ { .compatible = "gef,sbc610-gpio", .data = compat_parse_dt },
+ { .compatible = "moxa,moxart-gpio", .data = compat_parse_dt },
+ { .compatible = "technologic,ts4800-gpio", .data = compat_parse_dt },
{ }
};
MODULE_DEVICE_TABLE(of, bgpio_of_match);
diff --git a/drivers/gpio/gpio-moxart.c b/drivers/gpio/gpio-moxart.c
deleted file mode 100644
index d58d389..0000000
--- a/drivers/gpio/gpio-moxart.c
+++ /dev/null
@@ -1,84 +0,0 @@
-/*
- * MOXA ART SoCs GPIO driver.
- *
- * Copyright (C) 2013 Jonas Jensen
- *
- * Jonas Jensen <jonas.jensen@gmail.com>
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
- */
-
-#include <linux/err.h>
-#include <linux/init.h>
-#include <linux/irq.h>
-#include <linux/io.h>
-#include <linux/platform_device.h>
-#include <linux/of_address.h>
-#include <linux/of_gpio.h>
-#include <linux/pinctrl/consumer.h>
-#include <linux/delay.h>
-#include <linux/timer.h>
-#include <linux/bitops.h>
-#include <linux/gpio/driver.h>
-
-#define GPIO_DATA_OUT 0x00
-#define GPIO_DATA_IN 0x04
-#define GPIO_PIN_DIRECTION 0x08
-
-static int moxart_gpio_probe(struct platform_device *pdev)
-{
- struct device *dev = &pdev->dev;
- struct resource *res;
- struct gpio_chip *gc;
- void __iomem *base;
- int ret;
-
- gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
- if (!gc)
- return -ENOMEM;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- base = devm_ioremap_resource(dev, res);
- if (IS_ERR(base))
- return PTR_ERR(base);
-
- ret = bgpio_init(gc, dev, 4, base + GPIO_DATA_IN,
- base + GPIO_DATA_OUT, NULL,
- base + GPIO_PIN_DIRECTION, NULL,
- BGPIOF_READ_OUTPUT_REG_SET);
- if (ret) {
- dev_err(&pdev->dev, "bgpio_init failed\n");
- return ret;
- }
-
- gc->label = "moxart-gpio";
- gc->request = gpiochip_generic_request;
- gc->free = gpiochip_generic_free;
- gc->base = 0;
- gc->owner = THIS_MODULE;
-
- ret = devm_gpiochip_add_data(dev, gc, NULL);
- if (ret) {
- dev_err(dev, "%s: gpiochip_add failed\n",
- dev->of_node->full_name);
- return ret;
- }
-
- return ret;
-}
-
-static const struct of_device_id moxart_gpio_match[] = {
- { .compatible = "moxa,moxart-gpio" },
- { }
-};
-
-static struct platform_driver moxart_gpio_driver = {
- .driver = {
- .name = "moxart-gpio",
- .of_match_table = moxart_gpio_match,
- },
- .probe = moxart_gpio_probe,
-};
-builtin_platform_driver(moxart_gpio_driver);
diff --git a/drivers/gpio/gpio-ts4800.c b/drivers/gpio/gpio-ts4800.c
deleted file mode 100644
index 0c144a7..0000000
--- a/drivers/gpio/gpio-ts4800.c
+++ /dev/null
@@ -1,81 +0,0 @@
-/*
- * GPIO driver for the TS-4800 board
- *
- * Copyright (c) 2016 - Savoir-faire Linux
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
- */
-
-#include <linux/gpio/driver.h>
-#include <linux/of_address.h>
-#include <linux/of_device.h>
-#include <linux/platform_device.h>
-
-#define DEFAULT_PIN_NUMBER 16
-#define INPUT_REG_OFFSET 0x00
-#define OUTPUT_REG_OFFSET 0x02
-#define DIRECTION_REG_OFFSET 0x04
-
-static int ts4800_gpio_probe(struct platform_device *pdev)
-{
- struct device_node *node;
- struct gpio_chip *chip;
- struct resource *res;
- void __iomem *base_addr;
- int retval;
- u32 ngpios;
-
- chip = devm_kzalloc(&pdev->dev, sizeof(struct gpio_chip), GFP_KERNEL);
- if (!chip)
- return -ENOMEM;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- base_addr = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(base_addr))
- return PTR_ERR(base_addr);
-
- node = pdev->dev.of_node;
- if (!node)
- return -EINVAL;
-
- retval = of_property_read_u32(node, "ngpios", &ngpios);
- if (retval == -EINVAL)
- ngpios = DEFAULT_PIN_NUMBER;
- else if (retval)
- return retval;
-
- retval = bgpio_init(chip, &pdev->dev, 2, base_addr + INPUT_REG_OFFSET,
- base_addr + OUTPUT_REG_OFFSET, NULL,
- base_addr + DIRECTION_REG_OFFSET, NULL, 0);
- if (retval) {
- dev_err(&pdev->dev, "bgpio_init failed\n");
- return retval;
- }
-
- chip->ngpio = ngpios;
-
- platform_set_drvdata(pdev, chip);
-
- return devm_gpiochip_add_data(&pdev->dev, chip, NULL);
-}
-
-static const struct of_device_id ts4800_gpio_of_match[] = {
- { .compatible = "technologic,ts4800-gpio", },
- {},
-};
-
-static struct platform_driver ts4800_gpio_driver = {
- .driver = {
- .name = "ts4800-gpio",
- .of_match_table = ts4800_gpio_of_match,
- },
- .probe = ts4800_gpio_probe,
-};
-
-module_platform_driver_probe(ts4800_gpio_driver, ts4800_gpio_probe);
-
-MODULE_AUTHOR("Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>");
-MODULE_DESCRIPTION("TS4800 FPGA GPIO driver");
-MODULE_LICENSE("GPL v2");
--
2.8.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
@ 2016-05-11 9:34 ` Christian Lamparter
0 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-11 9:34 UTC (permalink / raw)
To: linux-arm-kernel
This patch integrates the GPIO drivers for the following
boards, SoCs, etc. into gpio-mmio:
- CLPS711X SoCs
- MOXA ART SoC
- TS-4800 FPGA DIO blocks and compatibles
- GPIO controllers found on some GE Single Board Computers
Cc: Alexander Shiyan <shc_work@mail.ru>
Cc: Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>
Cc: Martyn Welch <martyn.welch@ge.com>
Cc: Jonas Jensen <jonas.jensen@gmail.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
drivers/gpio/Kconfig | 16 +++-
drivers/gpio/Makefile | 4 -
drivers/gpio/gpio-clps711x.c | 91 -------------------
drivers/gpio/gpio-ge.c | 114 -----------------------
drivers/gpio/gpio-mmio.c | 212 +++++++++++++++++++++++++++++++++++++++++++
drivers/gpio/gpio-moxart.c | 84 -----------------
drivers/gpio/gpio-ts4800.c | 81 -----------------
7 files changed, 224 insertions(+), 378 deletions(-)
delete mode 100644 drivers/gpio/gpio-clps711x.c
delete mode 100644 drivers/gpio/gpio-ge.c
delete mode 100644 drivers/gpio/gpio-moxart.c
delete mode 100644 drivers/gpio/gpio-ts4800.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a68d838..e4d1065 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -154,7 +154,7 @@ config GPIO_BRCMSTB
config GPIO_CLPS711X
tristate "CLPS711X GPIO support"
depends on ARCH_CLPS711X || COMPILE_TEST
- select GPIO_GENERIC
+ select GPIO_GENERIC_PLATFORM
help
Say yes here to support GPIO on CLPS711X SoCs.
@@ -196,7 +196,7 @@ config GPIO_ETRAXFS
config GPIO_GE_FPGA
bool "GE FPGA based GPIO"
depends on GE_FPGA
- select GPIO_GENERIC
+ select GPIO_GENERIC_PLATFORM
help
Support for common GPIO functionality provided on some GE Single Board
Computers.
@@ -209,6 +209,14 @@ config GPIO_GENERIC_PLATFORM
tristate "Generic memory-mapped GPIO controller support (MMIO platform device)"
select GPIO_GENERIC
help
+ Select this to support many generic memory-mapped GPIO controllers.
+
+ This driver also includes support for the following GPIOs:
+ CLPS711X SoCs
+ MOXA ART SoC
+ TS-4800 FPGA DIO blocks and compatibles.
+ GPIOs found on some GE Single Board Computers.
+
Say yes here to support basic platform_device memory-mapped GPIO controllers.
config GPIO_GRGPIO
@@ -288,7 +296,7 @@ config GPIO_MM_LANTIQ
config GPIO_MOXART
bool "MOXART GPIO support"
depends on ARCH_MOXART || COMPILE_TEST
- select GPIO_GENERIC
+ select GPIO_GENERIC_PLATFORM
help
Select this option to enable GPIO driver for
MOXA ART SoC devices.
@@ -408,7 +416,7 @@ config GPIO_TS4800
tristate "TS-4800 DIO blocks and compatibles"
depends on OF_GPIO
depends on SOC_IMX51 || COMPILE_TEST
- select GPIO_GENERIC
+ select GPIO_GENERIC_PLATFORM
help
This driver support TS-4800 FPGA GPIO controllers.
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 991598e..d8d63ae 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -31,7 +31,6 @@ obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o
obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
-obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o
obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
@@ -43,7 +42,6 @@ obj-$(CONFIG_GPIO_EM) += gpio-em.o
obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
obj-$(CONFIG_GPIO_ETRAXFS) += gpio-etraxfs.o
obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
-obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
@@ -68,7 +66,6 @@ obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
obj-$(CONFIG_GPIO_MCP23S08) += gpio-mcp23s08.o
obj-$(CONFIG_GPIO_ML_IOH) += gpio-ml-ioh.o
obj-$(CONFIG_GPIO_MM_LANTIQ) += gpio-mm-lantiq.o
-obj-$(CONFIG_GPIO_MOXART) += gpio-moxart.o
obj-$(CONFIG_GPIO_MPC5200) += gpio-mpc5200.o
obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o
obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o
@@ -107,7 +104,6 @@ obj-$(CONFIG_GPIO_TPS65218) += gpio-tps65218.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_TS4800) += gpio-ts4800.o
obj-$(CONFIG_GPIO_TS5500) += gpio-ts5500.o
obj-$(CONFIG_GPIO_TWL4030) += gpio-twl4030.o
obj-$(CONFIG_GPIO_TWL6040) += gpio-twl6040.o
diff --git a/drivers/gpio/gpio-clps711x.c b/drivers/gpio/gpio-clps711x.c
deleted file mode 100644
index 5a69025..0000000
--- a/drivers/gpio/gpio-clps711x.c
+++ /dev/null
@@ -1,91 +0,0 @@
-/*
- * CLPS711X GPIO driver
- *
- * Copyright (C) 2012,2013 Alexander Shiyan <shc_work@mail.ru>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- */
-
-#include <linux/err.h>
-#include <linux/module.h>
-#include <linux/gpio/driver.h>
-#include <linux/platform_device.h>
-
-static int clps711x_gpio_probe(struct platform_device *pdev)
-{
- struct device_node *np = pdev->dev.of_node;
- void __iomem *dat, *dir;
- struct gpio_chip *gc;
- struct resource *res;
- int err, id = np ? of_alias_get_id(np, "gpio") : pdev->id;
-
- if ((id < 0) || (id > 4))
- return -ENODEV;
-
- gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
- if (!gc)
- return -ENOMEM;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- dat = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(dat))
- return PTR_ERR(dat);
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
- dir = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(dir))
- return PTR_ERR(dir);
-
- switch (id) {
- case 3:
- /* PORTD is inverted logic for direction register */
- err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
- NULL, dir, 0);
- break;
- default:
- err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
- dir, NULL, 0);
- break;
- }
-
- if (err)
- return err;
-
- switch (id) {
- case 4:
- /* PORTE is 3 lines only */
- gc->ngpio = 3;
- break;
- default:
- break;
- }
-
- gc->base = id * 8;
- gc->owner = THIS_MODULE;
- platform_set_drvdata(pdev, gc);
-
- return devm_gpiochip_add_data(&pdev->dev, gc, NULL);
-}
-
-static const struct of_device_id __maybe_unused clps711x_gpio_ids[] = {
- { .compatible = "cirrus,clps711x-gpio" },
- { }
-};
-MODULE_DEVICE_TABLE(of, clps711x_gpio_ids);
-
-static struct platform_driver clps711x_gpio_driver = {
- .driver = {
- .name = "clps711x-gpio",
- .of_match_table = of_match_ptr(clps711x_gpio_ids),
- },
- .probe = clps711x_gpio_probe,
-};
-module_platform_driver(clps711x_gpio_driver);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
-MODULE_DESCRIPTION("CLPS711X GPIO driver");
-MODULE_ALIAS("platform:clps711x-gpio");
diff --git a/drivers/gpio/gpio-ge.c b/drivers/gpio/gpio-ge.c
deleted file mode 100644
index 8650b29..0000000
--- a/drivers/gpio/gpio-ge.c
+++ /dev/null
@@ -1,114 +0,0 @@
-/*
- * Driver for GE FPGA based GPIO
- *
- * Author: Martyn Welch <martyn.welch@ge.com>
- *
- * 2008 (c) GE Intelligent Platforms Embedded Systems, Inc.
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2. This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
- */
-
-/* TODO
- *
- * Configuration of output modes (totem-pole/open-drain)
- * Interrupt configuration - interrupts are always generated the FPGA relies on
- * the I/O interrupt controllers mask to stop them propergating
- */
-
-#include <linux/kernel.h>
-#include <linux/io.h>
-#include <linux/slab.h>
-#include <linux/of_device.h>
-#include <linux/of_gpio.h>
-#include <linux/of_address.h>
-#include <linux/module.h>
-#include <linux/gpio/driver.h>
-
-#define GEF_GPIO_DIRECT 0x00
-#define GEF_GPIO_IN 0x04
-#define GEF_GPIO_OUT 0x08
-#define GEF_GPIO_TRIG 0x0C
-#define GEF_GPIO_POLAR_A 0x10
-#define GEF_GPIO_POLAR_B 0x14
-#define GEF_GPIO_INT_STAT 0x18
-#define GEF_GPIO_OVERRUN 0x1C
-#define GEF_GPIO_MODE 0x20
-
-static const struct of_device_id gef_gpio_ids[] = {
- {
- .compatible = "gef,sbc610-gpio",
- .data = (void *)19,
- }, {
- .compatible = "gef,sbc310-gpio",
- .data = (void *)6,
- }, {
- .compatible = "ge,imp3a-gpio",
- .data = (void *)16,
- },
- { }
-};
-MODULE_DEVICE_TABLE(of, gef_gpio_ids);
-
-static int __init gef_gpio_probe(struct platform_device *pdev)
-{
- const struct of_device_id *of_id =
- of_match_device(gef_gpio_ids, &pdev->dev);
- struct gpio_chip *gc;
- void __iomem *regs;
- int ret;
-
- gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
- if (!gc)
- return -ENOMEM;
-
- regs = of_iomap(pdev->dev.of_node, 0);
- if (!regs)
- return -ENOMEM;
-
- ret = bgpio_init(gc, &pdev->dev, 4, regs + GEF_GPIO_IN,
- regs + GEF_GPIO_OUT, NULL, NULL,
- regs + GEF_GPIO_DIRECT, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
- if (ret) {
- dev_err(&pdev->dev, "bgpio_init failed\n");
- goto err0;
- }
-
- /* Setup pointers to chip functions */
- gc->label = devm_kstrdup(&pdev->dev, pdev->dev.of_node->full_name,
- GFP_KERNEL);
- if (!gc->label) {
- ret = -ENOMEM;
- goto err0;
- }
-
- gc->base = -1;
- gc->ngpio = (u16)(uintptr_t)of_id->data;
- gc->of_gpio_n_cells = 2;
- gc->of_node = pdev->dev.of_node;
-
- /* This function adds a memory mapped GPIO chip */
- ret = devm_gpiochip_add_data(&pdev->dev, gc, NULL);
- if (ret)
- goto err0;
-
- return 0;
-err0:
- iounmap(regs);
- pr_err("%s: GPIO chip registration failed\n",
- pdev->dev.of_node->full_name);
- return ret;
-};
-
-static struct platform_driver gef_gpio_driver = {
- .driver = {
- .name = "gef-gpio",
- .of_match_table = gef_gpio_ids,
- },
-};
-module_platform_driver_probe(gef_gpio_driver, gef_gpio_probe);
-
-MODULE_DESCRIPTION("GE I/O FPGA GPIO driver");
-MODULE_AUTHOR("Martyn Welch <martyn.welch@ge.com");
-MODULE_LICENSE("GPL");
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index f72e40e..1de9172 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -586,8 +586,220 @@ static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
return 0;
}
+static int clps711x_parse_dt(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct resource *res;
+ const char *dir_reg_name;
+ int id = np ? of_alias_get_id(np, "gpio") : pdev->id;
+
+ switch (id) {
+ case 0:
+ case 1:
+ case 2:
+ pdata->ngpio = 0; /* determined by register width */
+ dir_reg_name = "dirout";
+ break;
+ case 3:
+ pdata->ngpio = 0; /* determined by register width */
+ /* PORTD is inverted logic for direction register */
+ dir_reg_name = "dirin";
+ break;
+ case 4:
+ pdata->ngpio = 3; /* PORTE is 3 lines only */
+ dir_reg_name = "dirout";
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ pdata->base = id * 8;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+ if (!res->name || strcmp("dat", res->name))
+ res->name = devm_kstrdup(&pdev->dev, "dat", GFP_KERNEL);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res)
+ return -EINVAL;
+ if (!res->name || strcmp(dir_reg_name, res->name))
+ res->name = devm_kstrdup(&pdev->dev, dir_reg_name, GFP_KERNEL);
+
+ return 0;
+}
+
+static int ge_dt_cb(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ struct device_node *np = pdev->dev.of_node;
+
+ pdata->label = devm_kstrdup(&pdev->dev, np->full_name, GFP_KERNEL);
+ if (!pdata->label)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static int moxart_dt_cb(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ pdata->base = 0;
+ pdata->label = "moxart-gpio";
+ return 0;
+}
+
+
+static int ts4800_dt_cb(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ int err;
+
+ err = of_property_read_u32(pdev->dev.of_node, "ngpios", &pdata->ngpio);
+ if (err == -EINVAL) {
+ pdata->ngpio = 16;
+ return 0;
+ }
+
+ return err;
+}
+
+/*
+ * bgpio_init() needs up to five named mmio register resources.
+ * These currently are dat, set, clr, [dirout | dirin]. There
+ * is no particular order or predefined place for the entries.
+ * However, dirout and dirin are mutually exclusive.
+ */
+#define __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES 4
+
+struct compat_gpio_device_data {
+ unsigned int expected_resource_size;
+ unsigned int ngpio;
+ resource_size_t register_width;
+ unsigned long flags;
+ int (*call_back)(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags);
+ struct resource_replacement {
+ resource_size_t start_offset;
+ const char *name;
+ } resources[__NUM_COMPAT_OVERRIDE_MMIO_RESOURCES];
+};
+#undef __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES
+
+#define ADD_COMPAT_REGISTER(_name, _offset) \
+ { .name = (_name), .start_offset = (_offset) }
+
+#define ADD_COMPAT_GPIO(_comp, _sz, _ngpio, _width, _cb, _f, _res...) \
+ { \
+ .compatible = (_comp), \
+ .data = &(struct compat_gpio_device_data) { \
+ .expected_resource_size = (_sz), \
+ .ngpio = (_ngpio), \
+ .register_width = (_width), \
+ .flags = (_f), \
+ .call_back = (_cb), \
+ .resources = { _res }, \
+ } \
+}
+
+#define ADD_COMPAT_GE_GPIO(_name, _ngpio) \
+ ADD_COMPAT_GPIO(_name, 0x24, _ngpio, 0x4, ge_dt_cb, \
+ BGPIOF_BIG_ENDIAN_BYTE_ORDER, \
+ ADD_COMPAT_REGISTER("dat", 0x04), \
+ ADD_COMPAT_REGISTER("set", 0x08), \
+ ADD_COMPAT_REGISTER("dirin", 0x00)) \
+
+static const struct of_device_id compat_gpio_devices[] = {
+ ADD_COMPAT_GE_GPIO("ge,imp3a-gpio", 16),
+ ADD_COMPAT_GE_GPIO("gef,sbc310-gpio", 6),
+ ADD_COMPAT_GE_GPIO("gef,sbc610-gpio", 19),
+ ADD_COMPAT_GPIO("moxa,moxart-gpio", 0xc, 0, 0x4, moxart_dt_cb,
+ BGPIOF_READ_OUTPUT_REG_SET,
+ ADD_COMPAT_REGISTER("dat", 0x04),
+ ADD_COMPAT_REGISTER("set", 0x00),
+ ADD_COMPAT_REGISTER("dirout", 0x08)),
+ ADD_COMPAT_GPIO("technologic,ts4800-gpio", 0x6, 16, 0x2, ts4800_dt_cb,
+ 0, ADD_COMPAT_REGISTER("dat", 0x00),
+ ADD_COMPAT_REGISTER("set", 0x02),
+ ADD_COMPAT_REGISTER("dirout", 0x04)),
+};
+
+#undef ADD_COMPAT_GE_GPIO
+#undef ADD_COMPAT_GPIO
+#undef ADD_COMPAT_REGISTER
+
+static int compat_parse_dt(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ const struct device_node *node = pdev->dev.of_node;
+ const struct compat_gpio_device_data *entry;
+ const struct of_device_id *of_id;
+ struct resource *res;
+ int err;
+
+ of_id = of_match_node(compat_gpio_devices, node);
+ if (!of_id)
+ return -ENODEV;
+
+ entry = of_id->data;
+ if (!entry || !entry->resources[0].name)
+ return -EINVAL;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+
+ if (!res->name || strcmp(entry->resources[0].name, res->name)) {
+ struct resource nres[ARRAY_SIZE(entry->resources)];
+ size_t i;
+
+ if (resource_size(res) != entry->expected_resource_size)
+ return -EINVAL;
+
+ for (i = 0; i < ARRAY_SIZE(entry->resources); i++) {
+ if (!entry->resources[i].name)
+ continue;
+
+ nres[i].name = devm_kstrdup(&pdev->dev,
+ entry->resources[i].name, GFP_KERNEL);
+ nres[i].start = res->start +
+ entry->resources[i].start_offset;
+ nres[i].end = nres[i].start +
+ entry->register_width - 1;
+ nres[i].flags = IORESOURCE_MEM;
+ }
+
+ err = platform_device_add_resources(pdev, nres, i);
+ if (err)
+ return err;
+ }
+
+ pdata->base = -1;
+ pdata->ngpio = entry->ngpio;
+ *flags = entry->flags;
+
+ if (entry->call_back)
+ err = entry->call_back(pdev, pdata, flags);
+
+ return err;
+}
+
static const struct of_device_id bgpio_of_match[] = {
{ .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
+ { .compatible = "cirrus,clps711x-gpio", .data = clps711x_parse_dt },
+ { .compatible = "ge,imp3a-gpio", .data = compat_parse_dt },
+ { .compatible = "gef,sbc310-gpio", .data = compat_parse_dt },
+ { .compatible = "gef,sbc610-gpio", .data = compat_parse_dt },
+ { .compatible = "moxa,moxart-gpio", .data = compat_parse_dt },
+ { .compatible = "technologic,ts4800-gpio", .data = compat_parse_dt },
{ }
};
MODULE_DEVICE_TABLE(of, bgpio_of_match);
diff --git a/drivers/gpio/gpio-moxart.c b/drivers/gpio/gpio-moxart.c
deleted file mode 100644
index d58d389..0000000
--- a/drivers/gpio/gpio-moxart.c
+++ /dev/null
@@ -1,84 +0,0 @@
-/*
- * MOXA ART SoCs GPIO driver.
- *
- * Copyright (C) 2013 Jonas Jensen
- *
- * Jonas Jensen <jonas.jensen@gmail.com>
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
- */
-
-#include <linux/err.h>
-#include <linux/init.h>
-#include <linux/irq.h>
-#include <linux/io.h>
-#include <linux/platform_device.h>
-#include <linux/of_address.h>
-#include <linux/of_gpio.h>
-#include <linux/pinctrl/consumer.h>
-#include <linux/delay.h>
-#include <linux/timer.h>
-#include <linux/bitops.h>
-#include <linux/gpio/driver.h>
-
-#define GPIO_DATA_OUT 0x00
-#define GPIO_DATA_IN 0x04
-#define GPIO_PIN_DIRECTION 0x08
-
-static int moxart_gpio_probe(struct platform_device *pdev)
-{
- struct device *dev = &pdev->dev;
- struct resource *res;
- struct gpio_chip *gc;
- void __iomem *base;
- int ret;
-
- gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
- if (!gc)
- return -ENOMEM;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- base = devm_ioremap_resource(dev, res);
- if (IS_ERR(base))
- return PTR_ERR(base);
-
- ret = bgpio_init(gc, dev, 4, base + GPIO_DATA_IN,
- base + GPIO_DATA_OUT, NULL,
- base + GPIO_PIN_DIRECTION, NULL,
- BGPIOF_READ_OUTPUT_REG_SET);
- if (ret) {
- dev_err(&pdev->dev, "bgpio_init failed\n");
- return ret;
- }
-
- gc->label = "moxart-gpio";
- gc->request = gpiochip_generic_request;
- gc->free = gpiochip_generic_free;
- gc->base = 0;
- gc->owner = THIS_MODULE;
-
- ret = devm_gpiochip_add_data(dev, gc, NULL);
- if (ret) {
- dev_err(dev, "%s: gpiochip_add failed\n",
- dev->of_node->full_name);
- return ret;
- }
-
- return ret;
-}
-
-static const struct of_device_id moxart_gpio_match[] = {
- { .compatible = "moxa,moxart-gpio" },
- { }
-};
-
-static struct platform_driver moxart_gpio_driver = {
- .driver = {
- .name = "moxart-gpio",
- .of_match_table = moxart_gpio_match,
- },
- .probe = moxart_gpio_probe,
-};
-builtin_platform_driver(moxart_gpio_driver);
diff --git a/drivers/gpio/gpio-ts4800.c b/drivers/gpio/gpio-ts4800.c
deleted file mode 100644
index 0c144a7..0000000
--- a/drivers/gpio/gpio-ts4800.c
+++ /dev/null
@@ -1,81 +0,0 @@
-/*
- * GPIO driver for the TS-4800 board
- *
- * Copyright (c) 2016 - Savoir-faire Linux
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
- */
-
-#include <linux/gpio/driver.h>
-#include <linux/of_address.h>
-#include <linux/of_device.h>
-#include <linux/platform_device.h>
-
-#define DEFAULT_PIN_NUMBER 16
-#define INPUT_REG_OFFSET 0x00
-#define OUTPUT_REG_OFFSET 0x02
-#define DIRECTION_REG_OFFSET 0x04
-
-static int ts4800_gpio_probe(struct platform_device *pdev)
-{
- struct device_node *node;
- struct gpio_chip *chip;
- struct resource *res;
- void __iomem *base_addr;
- int retval;
- u32 ngpios;
-
- chip = devm_kzalloc(&pdev->dev, sizeof(struct gpio_chip), GFP_KERNEL);
- if (!chip)
- return -ENOMEM;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- base_addr = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(base_addr))
- return PTR_ERR(base_addr);
-
- node = pdev->dev.of_node;
- if (!node)
- return -EINVAL;
-
- retval = of_property_read_u32(node, "ngpios", &ngpios);
- if (retval == -EINVAL)
- ngpios = DEFAULT_PIN_NUMBER;
- else if (retval)
- return retval;
-
- retval = bgpio_init(chip, &pdev->dev, 2, base_addr + INPUT_REG_OFFSET,
- base_addr + OUTPUT_REG_OFFSET, NULL,
- base_addr + DIRECTION_REG_OFFSET, NULL, 0);
- if (retval) {
- dev_err(&pdev->dev, "bgpio_init failed\n");
- return retval;
- }
-
- chip->ngpio = ngpios;
-
- platform_set_drvdata(pdev, chip);
-
- return devm_gpiochip_add_data(&pdev->dev, chip, NULL);
-}
-
-static const struct of_device_id ts4800_gpio_of_match[] = {
- { .compatible = "technologic,ts4800-gpio", },
- {},
-};
-
-static struct platform_driver ts4800_gpio_driver = {
- .driver = {
- .name = "ts4800-gpio",
- .of_match_table = ts4800_gpio_of_match,
- },
- .probe = ts4800_gpio_probe,
-};
-
-module_platform_driver_probe(ts4800_gpio_driver, ts4800_gpio_probe);
-
-MODULE_AUTHOR("Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>");
-MODULE_DESCRIPTION("TS4800 FPGA GPIO driver");
-MODULE_LICENSE("GPL v2");
--
2.8.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
2016-05-11 9:34 ` Christian Lamparter
@ 2016-05-11 12:19 ` Andy Shevchenko
-1 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2016-05-11 12:19 UTC (permalink / raw)
To: Christian Lamparter
Cc: linux-gpio, linux-kernel, linux-arm Mailing List,
Álvaro Fernández Rojas, Alexander Shiyan,
Alexandre Courbot, Linus Walleij, Joachim Eastwood,
Julien Grossholtz, Martyn Welch, Jonas Jensen
On Wed, May 11, 2016 at 12:34 PM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> This patch integrates the GPIO drivers for the following
> boards, SoCs, etc. into gpio-mmio:
> - CLPS711X SoCs
> - MOXA ART SoC
> - TS-4800 FPGA DIO blocks and compatibles
> - GPIO controllers found on some GE Single Board Computers
>
> Cc: Alexander Shiyan <shc_work@mail.ru>
> Cc: Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>
> Cc: Martyn Welch <martyn.welch@ge.com>
> Cc: Jonas Jensen <jonas.jensen@gmail.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> drivers/gpio/Kconfig | 16 +++-
> drivers/gpio/Makefile | 4 -
> drivers/gpio/gpio-clps711x.c | 91 -------------------
> drivers/gpio/gpio-ge.c | 114 -----------------------
> drivers/gpio/gpio-mmio.c | 212 +++++++++++++++++++++++++++++++++++++++++++
> drivers/gpio/gpio-moxart.c | 84 -----------------
> drivers/gpio/gpio-ts4800.c | 81 -----------------
> 7 files changed, 224 insertions(+), 378 deletions(-)
> delete mode 100644 drivers/gpio/gpio-clps711x.c
> delete mode 100644 drivers/gpio/gpio-ge.c
> delete mode 100644 drivers/gpio/gpio-moxart.c
> delete mode 100644 drivers/gpio/gpio-ts4800.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a68d838..e4d1065 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -154,7 +154,7 @@ config GPIO_BRCMSTB
> config GPIO_CLPS711X
> tristate "CLPS711X GPIO support"
> depends on ARCH_CLPS711X || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Say yes here to support GPIO on CLPS711X SoCs.
>
> @@ -196,7 +196,7 @@ config GPIO_ETRAXFS
> config GPIO_GE_FPGA
> bool "GE FPGA based GPIO"
> depends on GE_FPGA
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Support for common GPIO functionality provided on some GE Single Board
> Computers.
> @@ -209,6 +209,14 @@ config GPIO_GENERIC_PLATFORM
> tristate "Generic memory-mapped GPIO controller support (MMIO platform device)"
> select GPIO_GENERIC
> help
> + Select this to support many generic memory-mapped GPIO controllers.
> +
> + This driver also includes support for the following GPIOs:
> + CLPS711X SoCs
> + MOXA ART SoC
> + TS-4800 FPGA DIO blocks and compatibles.
> + GPIOs found on some GE Single Board Computers.
> +
> Say yes here to support basic platform_device memory-mapped GPIO controllers.
>
> config GPIO_GRGPIO
> @@ -288,7 +296,7 @@ config GPIO_MM_LANTIQ
> config GPIO_MOXART
> bool "MOXART GPIO support"
> depends on ARCH_MOXART || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Select this option to enable GPIO driver for
> MOXA ART SoC devices.
> @@ -408,7 +416,7 @@ config GPIO_TS4800
> tristate "TS-4800 DIO blocks and compatibles"
> depends on OF_GPIO
> depends on SOC_IMX51 || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> This driver support TS-4800 FPGA GPIO controllers.
>
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 991598e..d8d63ae 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -31,7 +31,6 @@ obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o
> obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
> obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o
> obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
> -obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
> obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
> obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o
> obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
> @@ -43,7 +42,6 @@ obj-$(CONFIG_GPIO_EM) += gpio-em.o
> obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
> obj-$(CONFIG_GPIO_ETRAXFS) += gpio-etraxfs.o
> obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
> -obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
> obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
> obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
> obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
> @@ -68,7 +66,6 @@ obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
> obj-$(CONFIG_GPIO_MCP23S08) += gpio-mcp23s08.o
> obj-$(CONFIG_GPIO_ML_IOH) += gpio-ml-ioh.o
> obj-$(CONFIG_GPIO_MM_LANTIQ) += gpio-mm-lantiq.o
> -obj-$(CONFIG_GPIO_MOXART) += gpio-moxart.o
> obj-$(CONFIG_GPIO_MPC5200) += gpio-mpc5200.o
> obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o
> obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o
> @@ -107,7 +104,6 @@ obj-$(CONFIG_GPIO_TPS65218) += gpio-tps65218.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_TS4800) += gpio-ts4800.o
> obj-$(CONFIG_GPIO_TS5500) += gpio-ts5500.o
> obj-$(CONFIG_GPIO_TWL4030) += gpio-twl4030.o
> obj-$(CONFIG_GPIO_TWL6040) += gpio-twl6040.o
> diff --git a/drivers/gpio/gpio-clps711x.c b/drivers/gpio/gpio-clps711x.c
> deleted file mode 100644
> index 5a69025..0000000
> --- a/drivers/gpio/gpio-clps711x.c
> +++ /dev/null
> @@ -1,91 +0,0 @@
> -/*
> - * CLPS711X GPIO driver
> - *
> - * Copyright (C) 2012,2013 Alexander Shiyan <shc_work@mail.ru>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - */
> -
> -#include <linux/err.h>
> -#include <linux/module.h>
> -#include <linux/gpio/driver.h>
> -#include <linux/platform_device.h>
> -
> -static int clps711x_gpio_probe(struct platform_device *pdev)
> -{
> - struct device_node *np = pdev->dev.of_node;
> - void __iomem *dat, *dir;
> - struct gpio_chip *gc;
> - struct resource *res;
> - int err, id = np ? of_alias_get_id(np, "gpio") : pdev->id;
> -
> - if ((id < 0) || (id > 4))
> - return -ENODEV;
> -
> - gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
> - if (!gc)
> - return -ENOMEM;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - dat = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(dat))
> - return PTR_ERR(dat);
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - dir = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(dir))
> - return PTR_ERR(dir);
> -
> - switch (id) {
> - case 3:
> - /* PORTD is inverted logic for direction register */
> - err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
> - NULL, dir, 0);
> - break;
> - default:
> - err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
> - dir, NULL, 0);
> - break;
> - }
> -
> - if (err)
> - return err;
> -
> - switch (id) {
> - case 4:
> - /* PORTE is 3 lines only */
> - gc->ngpio = 3;
> - break;
> - default:
> - break;
> - }
> -
> - gc->base = id * 8;
> - gc->owner = THIS_MODULE;
> - platform_set_drvdata(pdev, gc);
> -
> - return devm_gpiochip_add_data(&pdev->dev, gc, NULL);
> -}
> -
> -static const struct of_device_id __maybe_unused clps711x_gpio_ids[] = {
> - { .compatible = "cirrus,clps711x-gpio" },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, clps711x_gpio_ids);
> -
> -static struct platform_driver clps711x_gpio_driver = {
> - .driver = {
> - .name = "clps711x-gpio",
> - .of_match_table = of_match_ptr(clps711x_gpio_ids),
> - },
> - .probe = clps711x_gpio_probe,
> -};
> -module_platform_driver(clps711x_gpio_driver);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
> -MODULE_DESCRIPTION("CLPS711X GPIO driver");
> -MODULE_ALIAS("platform:clps711x-gpio");
> diff --git a/drivers/gpio/gpio-ge.c b/drivers/gpio/gpio-ge.c
> deleted file mode 100644
> index 8650b29..0000000
> --- a/drivers/gpio/gpio-ge.c
> +++ /dev/null
> @@ -1,114 +0,0 @@
> -/*
> - * Driver for GE FPGA based GPIO
> - *
> - * Author: Martyn Welch <martyn.welch@ge.com>
> - *
> - * 2008 (c) GE Intelligent Platforms Embedded Systems, Inc.
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
> - */
> -
> -/* TODO
> - *
> - * Configuration of output modes (totem-pole/open-drain)
> - * Interrupt configuration - interrupts are always generated the FPGA relies on
> - * the I/O interrupt controllers mask to stop them propergating
> - */
> -
> -#include <linux/kernel.h>
> -#include <linux/io.h>
> -#include <linux/slab.h>
> -#include <linux/of_device.h>
> -#include <linux/of_gpio.h>
> -#include <linux/of_address.h>
> -#include <linux/module.h>
> -#include <linux/gpio/driver.h>
> -
> -#define GEF_GPIO_DIRECT 0x00
> -#define GEF_GPIO_IN 0x04
> -#define GEF_GPIO_OUT 0x08
> -#define GEF_GPIO_TRIG 0x0C
> -#define GEF_GPIO_POLAR_A 0x10
> -#define GEF_GPIO_POLAR_B 0x14
> -#define GEF_GPIO_INT_STAT 0x18
> -#define GEF_GPIO_OVERRUN 0x1C
> -#define GEF_GPIO_MODE 0x20
> -
> -static const struct of_device_id gef_gpio_ids[] = {
> - {
> - .compatible = "gef,sbc610-gpio",
> - .data = (void *)19,
> - }, {
> - .compatible = "gef,sbc310-gpio",
> - .data = (void *)6,
> - }, {
> - .compatible = "ge,imp3a-gpio",
> - .data = (void *)16,
> - },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, gef_gpio_ids);
> -
> -static int __init gef_gpio_probe(struct platform_device *pdev)
> -{
> - const struct of_device_id *of_id =
> - of_match_device(gef_gpio_ids, &pdev->dev);
> - struct gpio_chip *gc;
> - void __iomem *regs;
> - int ret;
> -
> - gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
> - if (!gc)
> - return -ENOMEM;
> -
> - regs = of_iomap(pdev->dev.of_node, 0);
> - if (!regs)
> - return -ENOMEM;
> -
> - ret = bgpio_init(gc, &pdev->dev, 4, regs + GEF_GPIO_IN,
> - regs + GEF_GPIO_OUT, NULL, NULL,
> - regs + GEF_GPIO_DIRECT, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
> - if (ret) {
> - dev_err(&pdev->dev, "bgpio_init failed\n");
> - goto err0;
> - }
> -
> - /* Setup pointers to chip functions */
> - gc->label = devm_kstrdup(&pdev->dev, pdev->dev.of_node->full_name,
> - GFP_KERNEL);
> - if (!gc->label) {
> - ret = -ENOMEM;
> - goto err0;
> - }
> -
> - gc->base = -1;
> - gc->ngpio = (u16)(uintptr_t)of_id->data;
> - gc->of_gpio_n_cells = 2;
> - gc->of_node = pdev->dev.of_node;
> -
> - /* This function adds a memory mapped GPIO chip */
> - ret = devm_gpiochip_add_data(&pdev->dev, gc, NULL);
> - if (ret)
> - goto err0;
> -
> - return 0;
> -err0:
> - iounmap(regs);
> - pr_err("%s: GPIO chip registration failed\n",
> - pdev->dev.of_node->full_name);
> - return ret;
> -};
> -
> -static struct platform_driver gef_gpio_driver = {
> - .driver = {
> - .name = "gef-gpio",
> - .of_match_table = gef_gpio_ids,
> - },
> -};
> -module_platform_driver_probe(gef_gpio_driver, gef_gpio_probe);
> -
> -MODULE_DESCRIPTION("GE I/O FPGA GPIO driver");
> -MODULE_AUTHOR("Martyn Welch <martyn.welch@ge.com");
> -MODULE_LICENSE("GPL");
> diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> index f72e40e..1de9172 100644
> --- a/drivers/gpio/gpio-mmio.c
> +++ b/drivers/gpio/gpio-mmio.c
> @@ -586,8 +586,220 @@ static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> return 0;
> }
>
> +static int clps711x_parse_dt(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + const char *dir_reg_name;
> + int id = np ? of_alias_get_id(np, "gpio") : pdev->id;
> +
> + switch (id) {
> + case 0:
> + case 1:
> + case 2:
> + pdata->ngpio = 0; /* determined by register width */
> + dir_reg_name = "dirout";
> + break;
> + case 3:
> + pdata->ngpio = 0; /* determined by register width */
> + /* PORTD is inverted logic for direction register */
> + dir_reg_name = "dirin";
> + break;
> + case 4:
> + pdata->ngpio = 3; /* PORTE is 3 lines only */
> + dir_reg_name = "dirout";
> + break;
> + default:
> + return -ENODEV;
> + }
> +
> + pdata->base = id * 8;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> + if (!res->name || strcmp("dat", res->name))
> + res->name = devm_kstrdup(&pdev->dev, "dat", GFP_KERNEL);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res)
> + return -EINVAL;
> + if (!res->name || strcmp(dir_reg_name, res->name))
> + res->name = devm_kstrdup(&pdev->dev, dir_reg_name, GFP_KERNEL);
> +
> + return 0;
> +}
> +
> +static int ge_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + struct device_node *np = pdev->dev.of_node;
> +
> + pdata->label = devm_kstrdup(&pdev->dev, np->full_name, GFP_KERNEL);
> + if (!pdata->label)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int moxart_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + pdata->base = 0;
> + pdata->label = "moxart-gpio";
> + return 0;
> +}
> +
> +
> +static int ts4800_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + int err;
> +
> + err = of_property_read_u32(pdev->dev.of_node, "ngpios", &pdata->ngpio);
> + if (err == -EINVAL) {
> + pdata->ngpio = 16;
> + return 0;
> + }
> +
> + return err;
> +}
> +
> +/*
> + * bgpio_init() needs up to five named mmio register resources.
> + * These currently are dat, set, clr, [dirout | dirin]. There
> + * is no particular order or predefined place for the entries.
> + * However, dirout and dirin are mutually exclusive.
> + */
> +#define __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES 4
> +
> +struct compat_gpio_device_data {
> + unsigned int expected_resource_size;
> + unsigned int ngpio;
> + resource_size_t register_width;
> + unsigned long flags;
> + int (*call_back)(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags);
> + struct resource_replacement {
> + resource_size_t start_offset;
> + const char *name;
> + } resources[__NUM_COMPAT_OVERRIDE_MMIO_RESOURCES];
> +};
> +#undef __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES
> +
> +#define ADD_COMPAT_REGISTER(_name, _offset) \
> + { .name = (_name), .start_offset = (_offset) }
> +
> +#define ADD_COMPAT_GPIO(_comp, _sz, _ngpio, _width, _cb, _f, _res...) \
> + { \
> + .compatible = (_comp), \
> + .data = &(struct compat_gpio_device_data) { \
> + .expected_resource_size = (_sz), \
> + .ngpio = (_ngpio), \
> + .register_width = (_width), \
> + .flags = (_f), \
> + .call_back = (_cb), \
> + .resources = { _res }, \
> + } \
> +}
> +
> +#define ADD_COMPAT_GE_GPIO(_name, _ngpio) \
> + ADD_COMPAT_GPIO(_name, 0x24, _ngpio, 0x4, ge_dt_cb, \
> + BGPIOF_BIG_ENDIAN_BYTE_ORDER, \
> + ADD_COMPAT_REGISTER("dat", 0x04), \
> + ADD_COMPAT_REGISTER("set", 0x08), \
> + ADD_COMPAT_REGISTER("dirin", 0x00)) \
> +
> +static const struct of_device_id compat_gpio_devices[] = {
> + ADD_COMPAT_GE_GPIO("ge,imp3a-gpio", 16),
> + ADD_COMPAT_GE_GPIO("gef,sbc310-gpio", 6),
> + ADD_COMPAT_GE_GPIO("gef,sbc610-gpio", 19),
> + ADD_COMPAT_GPIO("moxa,moxart-gpio", 0xc, 0, 0x4, moxart_dt_cb,
> + BGPIOF_READ_OUTPUT_REG_SET,
> + ADD_COMPAT_REGISTER("dat", 0x04),
> + ADD_COMPAT_REGISTER("set", 0x00),
> + ADD_COMPAT_REGISTER("dirout", 0x08)),
> + ADD_COMPAT_GPIO("technologic,ts4800-gpio", 0x6, 16, 0x2, ts4800_dt_cb,
> + 0, ADD_COMPAT_REGISTER("dat", 0x00),
> + ADD_COMPAT_REGISTER("set", 0x02),
> + ADD_COMPAT_REGISTER("dirout", 0x04)),
> +};
> +
> +#undef ADD_COMPAT_GE_GPIO
> +#undef ADD_COMPAT_GPIO
> +#undef ADD_COMPAT_REGISTER
> +
> +static int compat_parse_dt(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + const struct device_node *node = pdev->dev.of_node;
> + const struct compat_gpio_device_data *entry;
> + const struct of_device_id *of_id;
> + struct resource *res;
> + int err;
> +
> + of_id = of_match_node(compat_gpio_devices, node);
> + if (!of_id)
> + return -ENODEV;
> +
> + entry = of_id->data;
> + if (!entry || !entry->resources[0].name)
> + return -EINVAL;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + if (!res->name || strcmp(entry->resources[0].name, res->name)) {
> + struct resource nres[ARRAY_SIZE(entry->resources)];
> + size_t i;
> +
> + if (resource_size(res) != entry->expected_resource_size)
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(entry->resources); i++) {
> + if (!entry->resources[i].name)
> + continue;
> +
> + nres[i].name = devm_kstrdup(&pdev->dev,
> + entry->resources[i].name, GFP_KERNEL);
> + nres[i].start = res->start +
> + entry->resources[i].start_offset;
> + nres[i].end = nres[i].start +
> + entry->register_width - 1;
> + nres[i].flags = IORESOURCE_MEM;
> + }
> +
> + err = platform_device_add_resources(pdev, nres, i);
> + if (err)
> + return err;
> + }
> +
> + pdata->base = -1;
> + pdata->ngpio = entry->ngpio;
> + *flags = entry->flags;
> +
> + if (entry->call_back)
> + err = entry->call_back(pdev, pdata, flags);
> +
> + return err;
> +}
> +
> static const struct of_device_id bgpio_of_match[] = {
> { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
> + { .compatible = "cirrus,clps711x-gpio", .data = clps711x_parse_dt },
> + { .compatible = "ge,imp3a-gpio", .data = compat_parse_dt },
> + { .compatible = "gef,sbc310-gpio", .data = compat_parse_dt },
> + { .compatible = "gef,sbc610-gpio", .data = compat_parse_dt },
> + { .compatible = "moxa,moxart-gpio", .data = compat_parse_dt },
> + { .compatible = "technologic,ts4800-gpio", .data = compat_parse_dt },
> { }
> };
> MODULE_DEVICE_TABLE(of, bgpio_of_match);
> diff --git a/drivers/gpio/gpio-moxart.c b/drivers/gpio/gpio-moxart.c
> deleted file mode 100644
> index d58d389..0000000
> --- a/drivers/gpio/gpio-moxart.c
> +++ /dev/null
> @@ -1,84 +0,0 @@
> -/*
> - * MOXA ART SoCs GPIO driver.
> - *
> - * Copyright (C) 2013 Jonas Jensen
> - *
> - * Jonas Jensen <jonas.jensen@gmail.com>
> - *
> - * This file is licensed under the terms of the GNU General Public
> - * License version 2. This program is licensed "as is" without any
> - * warranty of any kind, whether express or implied.
> - */
> -
> -#include <linux/err.h>
> -#include <linux/init.h>
> -#include <linux/irq.h>
> -#include <linux/io.h>
> -#include <linux/platform_device.h>
> -#include <linux/of_address.h>
> -#include <linux/of_gpio.h>
> -#include <linux/pinctrl/consumer.h>
> -#include <linux/delay.h>
> -#include <linux/timer.h>
> -#include <linux/bitops.h>
> -#include <linux/gpio/driver.h>
> -
> -#define GPIO_DATA_OUT 0x00
> -#define GPIO_DATA_IN 0x04
> -#define GPIO_PIN_DIRECTION 0x08
> -
> -static int moxart_gpio_probe(struct platform_device *pdev)
> -{
> - struct device *dev = &pdev->dev;
> - struct resource *res;
> - struct gpio_chip *gc;
> - void __iomem *base;
> - int ret;
> -
> - gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> - if (!gc)
> - return -ENOMEM;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - base = devm_ioremap_resource(dev, res);
> - if (IS_ERR(base))
> - return PTR_ERR(base);
> -
> - ret = bgpio_init(gc, dev, 4, base + GPIO_DATA_IN,
> - base + GPIO_DATA_OUT, NULL,
> - base + GPIO_PIN_DIRECTION, NULL,
> - BGPIOF_READ_OUTPUT_REG_SET);
> - if (ret) {
> - dev_err(&pdev->dev, "bgpio_init failed\n");
> - return ret;
> - }
> -
> - gc->label = "moxart-gpio";
> - gc->request = gpiochip_generic_request;
> - gc->free = gpiochip_generic_free;
> - gc->base = 0;
> - gc->owner = THIS_MODULE;
> -
> - ret = devm_gpiochip_add_data(dev, gc, NULL);
> - if (ret) {
> - dev_err(dev, "%s: gpiochip_add failed\n",
> - dev->of_node->full_name);
> - return ret;
> - }
> -
> - return ret;
> -}
> -
> -static const struct of_device_id moxart_gpio_match[] = {
> - { .compatible = "moxa,moxart-gpio" },
> - { }
> -};
> -
> -static struct platform_driver moxart_gpio_driver = {
> - .driver = {
> - .name = "moxart-gpio",
> - .of_match_table = moxart_gpio_match,
> - },
> - .probe = moxart_gpio_probe,
> -};
> -builtin_platform_driver(moxart_gpio_driver);
> diff --git a/drivers/gpio/gpio-ts4800.c b/drivers/gpio/gpio-ts4800.c
> deleted file mode 100644
> index 0c144a7..0000000
> --- a/drivers/gpio/gpio-ts4800.c
> +++ /dev/null
> @@ -1,81 +0,0 @@
> -/*
> - * GPIO driver for the TS-4800 board
> - *
> - * Copyright (c) 2016 - Savoir-faire Linux
> - *
> - * This file is licensed under the terms of the GNU General Public
> - * License version 2. This program is licensed "as is" without any
> - * warranty of any kind, whether express or implied.
> - */
> -
> -#include <linux/gpio/driver.h>
> -#include <linux/of_address.h>
> -#include <linux/of_device.h>
> -#include <linux/platform_device.h>
> -
> -#define DEFAULT_PIN_NUMBER 16
> -#define INPUT_REG_OFFSET 0x00
> -#define OUTPUT_REG_OFFSET 0x02
> -#define DIRECTION_REG_OFFSET 0x04
> -
> -static int ts4800_gpio_probe(struct platform_device *pdev)
> -{
> - struct device_node *node;
> - struct gpio_chip *chip;
> - struct resource *res;
> - void __iomem *base_addr;
> - int retval;
> - u32 ngpios;
> -
> - chip = devm_kzalloc(&pdev->dev, sizeof(struct gpio_chip), GFP_KERNEL);
> - if (!chip)
> - return -ENOMEM;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - base_addr = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(base_addr))
> - return PTR_ERR(base_addr);
> -
> - node = pdev->dev.of_node;
> - if (!node)
> - return -EINVAL;
> -
> - retval = of_property_read_u32(node, "ngpios", &ngpios);
> - if (retval == -EINVAL)
> - ngpios = DEFAULT_PIN_NUMBER;
> - else if (retval)
> - return retval;
> -
> - retval = bgpio_init(chip, &pdev->dev, 2, base_addr + INPUT_REG_OFFSET,
> - base_addr + OUTPUT_REG_OFFSET, NULL,
> - base_addr + DIRECTION_REG_OFFSET, NULL, 0);
> - if (retval) {
> - dev_err(&pdev->dev, "bgpio_init failed\n");
> - return retval;
> - }
> -
> - chip->ngpio = ngpios;
> -
> - platform_set_drvdata(pdev, chip);
> -
> - return devm_gpiochip_add_data(&pdev->dev, chip, NULL);
> -}
> -
> -static const struct of_device_id ts4800_gpio_of_match[] = {
> - { .compatible = "technologic,ts4800-gpio", },
> - {},
> -};
> -
> -static struct platform_driver ts4800_gpio_driver = {
> - .driver = {
> - .name = "ts4800-gpio",
> - .of_match_table = ts4800_gpio_of_match,
> - },
> - .probe = ts4800_gpio_probe,
> -};
> -
> -module_platform_driver_probe(ts4800_gpio_driver, ts4800_gpio_probe);
> -
> -MODULE_AUTHOR("Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>");
> -MODULE_DESCRIPTION("TS4800 FPGA GPIO driver");
> -MODULE_LICENSE("GPL v2");
> --
> 2.8.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
@ 2016-05-11 12:19 ` Andy Shevchenko
0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2016-05-11 12:19 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, May 11, 2016 at 12:34 PM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> This patch integrates the GPIO drivers for the following
> boards, SoCs, etc. into gpio-mmio:
> - CLPS711X SoCs
> - MOXA ART SoC
> - TS-4800 FPGA DIO blocks and compatibles
> - GPIO controllers found on some GE Single Board Computers
>
> Cc: Alexander Shiyan <shc_work@mail.ru>
> Cc: Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>
> Cc: Martyn Welch <martyn.welch@ge.com>
> Cc: Jonas Jensen <jonas.jensen@gmail.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> drivers/gpio/Kconfig | 16 +++-
> drivers/gpio/Makefile | 4 -
> drivers/gpio/gpio-clps711x.c | 91 -------------------
> drivers/gpio/gpio-ge.c | 114 -----------------------
> drivers/gpio/gpio-mmio.c | 212 +++++++++++++++++++++++++++++++++++++++++++
> drivers/gpio/gpio-moxart.c | 84 -----------------
> drivers/gpio/gpio-ts4800.c | 81 -----------------
> 7 files changed, 224 insertions(+), 378 deletions(-)
> delete mode 100644 drivers/gpio/gpio-clps711x.c
> delete mode 100644 drivers/gpio/gpio-ge.c
> delete mode 100644 drivers/gpio/gpio-moxart.c
> delete mode 100644 drivers/gpio/gpio-ts4800.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a68d838..e4d1065 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -154,7 +154,7 @@ config GPIO_BRCMSTB
> config GPIO_CLPS711X
> tristate "CLPS711X GPIO support"
> depends on ARCH_CLPS711X || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Say yes here to support GPIO on CLPS711X SoCs.
>
> @@ -196,7 +196,7 @@ config GPIO_ETRAXFS
> config GPIO_GE_FPGA
> bool "GE FPGA based GPIO"
> depends on GE_FPGA
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Support for common GPIO functionality provided on some GE Single Board
> Computers.
> @@ -209,6 +209,14 @@ config GPIO_GENERIC_PLATFORM
> tristate "Generic memory-mapped GPIO controller support (MMIO platform device)"
> select GPIO_GENERIC
> help
> + Select this to support many generic memory-mapped GPIO controllers.
> +
> + This driver also includes support for the following GPIOs:
> + CLPS711X SoCs
> + MOXA ART SoC
> + TS-4800 FPGA DIO blocks and compatibles.
> + GPIOs found on some GE Single Board Computers.
> +
> Say yes here to support basic platform_device memory-mapped GPIO controllers.
>
> config GPIO_GRGPIO
> @@ -288,7 +296,7 @@ config GPIO_MM_LANTIQ
> config GPIO_MOXART
> bool "MOXART GPIO support"
> depends on ARCH_MOXART || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Select this option to enable GPIO driver for
> MOXA ART SoC devices.
> @@ -408,7 +416,7 @@ config GPIO_TS4800
> tristate "TS-4800 DIO blocks and compatibles"
> depends on OF_GPIO
> depends on SOC_IMX51 || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> This driver support TS-4800 FPGA GPIO controllers.
>
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 991598e..d8d63ae 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -31,7 +31,6 @@ obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o
> obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
> obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o
> obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
> -obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
> obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
> obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o
> obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
> @@ -43,7 +42,6 @@ obj-$(CONFIG_GPIO_EM) += gpio-em.o
> obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
> obj-$(CONFIG_GPIO_ETRAXFS) += gpio-etraxfs.o
> obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
> -obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
> obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
> obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
> obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
> @@ -68,7 +66,6 @@ obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
> obj-$(CONFIG_GPIO_MCP23S08) += gpio-mcp23s08.o
> obj-$(CONFIG_GPIO_ML_IOH) += gpio-ml-ioh.o
> obj-$(CONFIG_GPIO_MM_LANTIQ) += gpio-mm-lantiq.o
> -obj-$(CONFIG_GPIO_MOXART) += gpio-moxart.o
> obj-$(CONFIG_GPIO_MPC5200) += gpio-mpc5200.o
> obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o
> obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o
> @@ -107,7 +104,6 @@ obj-$(CONFIG_GPIO_TPS65218) += gpio-tps65218.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_TS4800) += gpio-ts4800.o
> obj-$(CONFIG_GPIO_TS5500) += gpio-ts5500.o
> obj-$(CONFIG_GPIO_TWL4030) += gpio-twl4030.o
> obj-$(CONFIG_GPIO_TWL6040) += gpio-twl6040.o
> diff --git a/drivers/gpio/gpio-clps711x.c b/drivers/gpio/gpio-clps711x.c
> deleted file mode 100644
> index 5a69025..0000000
> --- a/drivers/gpio/gpio-clps711x.c
> +++ /dev/null
> @@ -1,91 +0,0 @@
> -/*
> - * CLPS711X GPIO driver
> - *
> - * Copyright (C) 2012,2013 Alexander Shiyan <shc_work@mail.ru>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - */
> -
> -#include <linux/err.h>
> -#include <linux/module.h>
> -#include <linux/gpio/driver.h>
> -#include <linux/platform_device.h>
> -
> -static int clps711x_gpio_probe(struct platform_device *pdev)
> -{
> - struct device_node *np = pdev->dev.of_node;
> - void __iomem *dat, *dir;
> - struct gpio_chip *gc;
> - struct resource *res;
> - int err, id = np ? of_alias_get_id(np, "gpio") : pdev->id;
> -
> - if ((id < 0) || (id > 4))
> - return -ENODEV;
> -
> - gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
> - if (!gc)
> - return -ENOMEM;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - dat = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(dat))
> - return PTR_ERR(dat);
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - dir = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(dir))
> - return PTR_ERR(dir);
> -
> - switch (id) {
> - case 3:
> - /* PORTD is inverted logic for direction register */
> - err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
> - NULL, dir, 0);
> - break;
> - default:
> - err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
> - dir, NULL, 0);
> - break;
> - }
> -
> - if (err)
> - return err;
> -
> - switch (id) {
> - case 4:
> - /* PORTE is 3 lines only */
> - gc->ngpio = 3;
> - break;
> - default:
> - break;
> - }
> -
> - gc->base = id * 8;
> - gc->owner = THIS_MODULE;
> - platform_set_drvdata(pdev, gc);
> -
> - return devm_gpiochip_add_data(&pdev->dev, gc, NULL);
> -}
> -
> -static const struct of_device_id __maybe_unused clps711x_gpio_ids[] = {
> - { .compatible = "cirrus,clps711x-gpio" },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, clps711x_gpio_ids);
> -
> -static struct platform_driver clps711x_gpio_driver = {
> - .driver = {
> - .name = "clps711x-gpio",
> - .of_match_table = of_match_ptr(clps711x_gpio_ids),
> - },
> - .probe = clps711x_gpio_probe,
> -};
> -module_platform_driver(clps711x_gpio_driver);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
> -MODULE_DESCRIPTION("CLPS711X GPIO driver");
> -MODULE_ALIAS("platform:clps711x-gpio");
> diff --git a/drivers/gpio/gpio-ge.c b/drivers/gpio/gpio-ge.c
> deleted file mode 100644
> index 8650b29..0000000
> --- a/drivers/gpio/gpio-ge.c
> +++ /dev/null
> @@ -1,114 +0,0 @@
> -/*
> - * Driver for GE FPGA based GPIO
> - *
> - * Author: Martyn Welch <martyn.welch@ge.com>
> - *
> - * 2008 (c) GE Intelligent Platforms Embedded Systems, Inc.
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
> - */
> -
> -/* TODO
> - *
> - * Configuration of output modes (totem-pole/open-drain)
> - * Interrupt configuration - interrupts are always generated the FPGA relies on
> - * the I/O interrupt controllers mask to stop them propergating
> - */
> -
> -#include <linux/kernel.h>
> -#include <linux/io.h>
> -#include <linux/slab.h>
> -#include <linux/of_device.h>
> -#include <linux/of_gpio.h>
> -#include <linux/of_address.h>
> -#include <linux/module.h>
> -#include <linux/gpio/driver.h>
> -
> -#define GEF_GPIO_DIRECT 0x00
> -#define GEF_GPIO_IN 0x04
> -#define GEF_GPIO_OUT 0x08
> -#define GEF_GPIO_TRIG 0x0C
> -#define GEF_GPIO_POLAR_A 0x10
> -#define GEF_GPIO_POLAR_B 0x14
> -#define GEF_GPIO_INT_STAT 0x18
> -#define GEF_GPIO_OVERRUN 0x1C
> -#define GEF_GPIO_MODE 0x20
> -
> -static const struct of_device_id gef_gpio_ids[] = {
> - {
> - .compatible = "gef,sbc610-gpio",
> - .data = (void *)19,
> - }, {
> - .compatible = "gef,sbc310-gpio",
> - .data = (void *)6,
> - }, {
> - .compatible = "ge,imp3a-gpio",
> - .data = (void *)16,
> - },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, gef_gpio_ids);
> -
> -static int __init gef_gpio_probe(struct platform_device *pdev)
> -{
> - const struct of_device_id *of_id =
> - of_match_device(gef_gpio_ids, &pdev->dev);
> - struct gpio_chip *gc;
> - void __iomem *regs;
> - int ret;
> -
> - gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
> - if (!gc)
> - return -ENOMEM;
> -
> - regs = of_iomap(pdev->dev.of_node, 0);
> - if (!regs)
> - return -ENOMEM;
> -
> - ret = bgpio_init(gc, &pdev->dev, 4, regs + GEF_GPIO_IN,
> - regs + GEF_GPIO_OUT, NULL, NULL,
> - regs + GEF_GPIO_DIRECT, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
> - if (ret) {
> - dev_err(&pdev->dev, "bgpio_init failed\n");
> - goto err0;
> - }
> -
> - /* Setup pointers to chip functions */
> - gc->label = devm_kstrdup(&pdev->dev, pdev->dev.of_node->full_name,
> - GFP_KERNEL);
> - if (!gc->label) {
> - ret = -ENOMEM;
> - goto err0;
> - }
> -
> - gc->base = -1;
> - gc->ngpio = (u16)(uintptr_t)of_id->data;
> - gc->of_gpio_n_cells = 2;
> - gc->of_node = pdev->dev.of_node;
> -
> - /* This function adds a memory mapped GPIO chip */
> - ret = devm_gpiochip_add_data(&pdev->dev, gc, NULL);
> - if (ret)
> - goto err0;
> -
> - return 0;
> -err0:
> - iounmap(regs);
> - pr_err("%s: GPIO chip registration failed\n",
> - pdev->dev.of_node->full_name);
> - return ret;
> -};
> -
> -static struct platform_driver gef_gpio_driver = {
> - .driver = {
> - .name = "gef-gpio",
> - .of_match_table = gef_gpio_ids,
> - },
> -};
> -module_platform_driver_probe(gef_gpio_driver, gef_gpio_probe);
> -
> -MODULE_DESCRIPTION("GE I/O FPGA GPIO driver");
> -MODULE_AUTHOR("Martyn Welch <martyn.welch@ge.com");
> -MODULE_LICENSE("GPL");
> diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> index f72e40e..1de9172 100644
> --- a/drivers/gpio/gpio-mmio.c
> +++ b/drivers/gpio/gpio-mmio.c
> @@ -586,8 +586,220 @@ static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> return 0;
> }
>
> +static int clps711x_parse_dt(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + const char *dir_reg_name;
> + int id = np ? of_alias_get_id(np, "gpio") : pdev->id;
> +
> + switch (id) {
> + case 0:
> + case 1:
> + case 2:
> + pdata->ngpio = 0; /* determined by register width */
> + dir_reg_name = "dirout";
> + break;
> + case 3:
> + pdata->ngpio = 0; /* determined by register width */
> + /* PORTD is inverted logic for direction register */
> + dir_reg_name = "dirin";
> + break;
> + case 4:
> + pdata->ngpio = 3; /* PORTE is 3 lines only */
> + dir_reg_name = "dirout";
> + break;
> + default:
> + return -ENODEV;
> + }
> +
> + pdata->base = id * 8;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> + if (!res->name || strcmp("dat", res->name))
> + res->name = devm_kstrdup(&pdev->dev, "dat", GFP_KERNEL);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res)
> + return -EINVAL;
> + if (!res->name || strcmp(dir_reg_name, res->name))
> + res->name = devm_kstrdup(&pdev->dev, dir_reg_name, GFP_KERNEL);
> +
> + return 0;
> +}
> +
> +static int ge_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + struct device_node *np = pdev->dev.of_node;
> +
> + pdata->label = devm_kstrdup(&pdev->dev, np->full_name, GFP_KERNEL);
> + if (!pdata->label)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int moxart_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + pdata->base = 0;
> + pdata->label = "moxart-gpio";
> + return 0;
> +}
> +
> +
> +static int ts4800_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + int err;
> +
> + err = of_property_read_u32(pdev->dev.of_node, "ngpios", &pdata->ngpio);
> + if (err == -EINVAL) {
> + pdata->ngpio = 16;
> + return 0;
> + }
> +
> + return err;
> +}
> +
> +/*
> + * bgpio_init() needs up to five named mmio register resources.
> + * These currently are dat, set, clr, [dirout | dirin]. There
> + * is no particular order or predefined place for the entries.
> + * However, dirout and dirin are mutually exclusive.
> + */
> +#define __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES 4
> +
> +struct compat_gpio_device_data {
> + unsigned int expected_resource_size;
> + unsigned int ngpio;
> + resource_size_t register_width;
> + unsigned long flags;
> + int (*call_back)(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags);
> + struct resource_replacement {
> + resource_size_t start_offset;
> + const char *name;
> + } resources[__NUM_COMPAT_OVERRIDE_MMIO_RESOURCES];
> +};
> +#undef __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES
> +
> +#define ADD_COMPAT_REGISTER(_name, _offset) \
> + { .name = (_name), .start_offset = (_offset) }
> +
> +#define ADD_COMPAT_GPIO(_comp, _sz, _ngpio, _width, _cb, _f, _res...) \
> + { \
> + .compatible = (_comp), \
> + .data = &(struct compat_gpio_device_data) { \
> + .expected_resource_size = (_sz), \
> + .ngpio = (_ngpio), \
> + .register_width = (_width), \
> + .flags = (_f), \
> + .call_back = (_cb), \
> + .resources = { _res }, \
> + } \
> +}
> +
> +#define ADD_COMPAT_GE_GPIO(_name, _ngpio) \
> + ADD_COMPAT_GPIO(_name, 0x24, _ngpio, 0x4, ge_dt_cb, \
> + BGPIOF_BIG_ENDIAN_BYTE_ORDER, \
> + ADD_COMPAT_REGISTER("dat", 0x04), \
> + ADD_COMPAT_REGISTER("set", 0x08), \
> + ADD_COMPAT_REGISTER("dirin", 0x00)) \
> +
> +static const struct of_device_id compat_gpio_devices[] = {
> + ADD_COMPAT_GE_GPIO("ge,imp3a-gpio", 16),
> + ADD_COMPAT_GE_GPIO("gef,sbc310-gpio", 6),
> + ADD_COMPAT_GE_GPIO("gef,sbc610-gpio", 19),
> + ADD_COMPAT_GPIO("moxa,moxart-gpio", 0xc, 0, 0x4, moxart_dt_cb,
> + BGPIOF_READ_OUTPUT_REG_SET,
> + ADD_COMPAT_REGISTER("dat", 0x04),
> + ADD_COMPAT_REGISTER("set", 0x00),
> + ADD_COMPAT_REGISTER("dirout", 0x08)),
> + ADD_COMPAT_GPIO("technologic,ts4800-gpio", 0x6, 16, 0x2, ts4800_dt_cb,
> + 0, ADD_COMPAT_REGISTER("dat", 0x00),
> + ADD_COMPAT_REGISTER("set", 0x02),
> + ADD_COMPAT_REGISTER("dirout", 0x04)),
> +};
> +
> +#undef ADD_COMPAT_GE_GPIO
> +#undef ADD_COMPAT_GPIO
> +#undef ADD_COMPAT_REGISTER
> +
> +static int compat_parse_dt(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + const struct device_node *node = pdev->dev.of_node;
> + const struct compat_gpio_device_data *entry;
> + const struct of_device_id *of_id;
> + struct resource *res;
> + int err;
> +
> + of_id = of_match_node(compat_gpio_devices, node);
> + if (!of_id)
> + return -ENODEV;
> +
> + entry = of_id->data;
> + if (!entry || !entry->resources[0].name)
> + return -EINVAL;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + if (!res->name || strcmp(entry->resources[0].name, res->name)) {
> + struct resource nres[ARRAY_SIZE(entry->resources)];
> + size_t i;
> +
> + if (resource_size(res) != entry->expected_resource_size)
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(entry->resources); i++) {
> + if (!entry->resources[i].name)
> + continue;
> +
> + nres[i].name = devm_kstrdup(&pdev->dev,
> + entry->resources[i].name, GFP_KERNEL);
> + nres[i].start = res->start +
> + entry->resources[i].start_offset;
> + nres[i].end = nres[i].start +
> + entry->register_width - 1;
> + nres[i].flags = IORESOURCE_MEM;
> + }
> +
> + err = platform_device_add_resources(pdev, nres, i);
> + if (err)
> + return err;
> + }
> +
> + pdata->base = -1;
> + pdata->ngpio = entry->ngpio;
> + *flags = entry->flags;
> +
> + if (entry->call_back)
> + err = entry->call_back(pdev, pdata, flags);
> +
> + return err;
> +}
> +
> static const struct of_device_id bgpio_of_match[] = {
> { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
> + { .compatible = "cirrus,clps711x-gpio", .data = clps711x_parse_dt },
> + { .compatible = "ge,imp3a-gpio", .data = compat_parse_dt },
> + { .compatible = "gef,sbc310-gpio", .data = compat_parse_dt },
> + { .compatible = "gef,sbc610-gpio", .data = compat_parse_dt },
> + { .compatible = "moxa,moxart-gpio", .data = compat_parse_dt },
> + { .compatible = "technologic,ts4800-gpio", .data = compat_parse_dt },
> { }
> };
> MODULE_DEVICE_TABLE(of, bgpio_of_match);
> diff --git a/drivers/gpio/gpio-moxart.c b/drivers/gpio/gpio-moxart.c
> deleted file mode 100644
> index d58d389..0000000
> --- a/drivers/gpio/gpio-moxart.c
> +++ /dev/null
> @@ -1,84 +0,0 @@
> -/*
> - * MOXA ART SoCs GPIO driver.
> - *
> - * Copyright (C) 2013 Jonas Jensen
> - *
> - * Jonas Jensen <jonas.jensen@gmail.com>
> - *
> - * This file is licensed under the terms of the GNU General Public
> - * License version 2. This program is licensed "as is" without any
> - * warranty of any kind, whether express or implied.
> - */
> -
> -#include <linux/err.h>
> -#include <linux/init.h>
> -#include <linux/irq.h>
> -#include <linux/io.h>
> -#include <linux/platform_device.h>
> -#include <linux/of_address.h>
> -#include <linux/of_gpio.h>
> -#include <linux/pinctrl/consumer.h>
> -#include <linux/delay.h>
> -#include <linux/timer.h>
> -#include <linux/bitops.h>
> -#include <linux/gpio/driver.h>
> -
> -#define GPIO_DATA_OUT 0x00
> -#define GPIO_DATA_IN 0x04
> -#define GPIO_PIN_DIRECTION 0x08
> -
> -static int moxart_gpio_probe(struct platform_device *pdev)
> -{
> - struct device *dev = &pdev->dev;
> - struct resource *res;
> - struct gpio_chip *gc;
> - void __iomem *base;
> - int ret;
> -
> - gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> - if (!gc)
> - return -ENOMEM;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - base = devm_ioremap_resource(dev, res);
> - if (IS_ERR(base))
> - return PTR_ERR(base);
> -
> - ret = bgpio_init(gc, dev, 4, base + GPIO_DATA_IN,
> - base + GPIO_DATA_OUT, NULL,
> - base + GPIO_PIN_DIRECTION, NULL,
> - BGPIOF_READ_OUTPUT_REG_SET);
> - if (ret) {
> - dev_err(&pdev->dev, "bgpio_init failed\n");
> - return ret;
> - }
> -
> - gc->label = "moxart-gpio";
> - gc->request = gpiochip_generic_request;
> - gc->free = gpiochip_generic_free;
> - gc->base = 0;
> - gc->owner = THIS_MODULE;
> -
> - ret = devm_gpiochip_add_data(dev, gc, NULL);
> - if (ret) {
> - dev_err(dev, "%s: gpiochip_add failed\n",
> - dev->of_node->full_name);
> - return ret;
> - }
> -
> - return ret;
> -}
> -
> -static const struct of_device_id moxart_gpio_match[] = {
> - { .compatible = "moxa,moxart-gpio" },
> - { }
> -};
> -
> -static struct platform_driver moxart_gpio_driver = {
> - .driver = {
> - .name = "moxart-gpio",
> - .of_match_table = moxart_gpio_match,
> - },
> - .probe = moxart_gpio_probe,
> -};
> -builtin_platform_driver(moxart_gpio_driver);
> diff --git a/drivers/gpio/gpio-ts4800.c b/drivers/gpio/gpio-ts4800.c
> deleted file mode 100644
> index 0c144a7..0000000
> --- a/drivers/gpio/gpio-ts4800.c
> +++ /dev/null
> @@ -1,81 +0,0 @@
> -/*
> - * GPIO driver for the TS-4800 board
> - *
> - * Copyright (c) 2016 - Savoir-faire Linux
> - *
> - * This file is licensed under the terms of the GNU General Public
> - * License version 2. This program is licensed "as is" without any
> - * warranty of any kind, whether express or implied.
> - */
> -
> -#include <linux/gpio/driver.h>
> -#include <linux/of_address.h>
> -#include <linux/of_device.h>
> -#include <linux/platform_device.h>
> -
> -#define DEFAULT_PIN_NUMBER 16
> -#define INPUT_REG_OFFSET 0x00
> -#define OUTPUT_REG_OFFSET 0x02
> -#define DIRECTION_REG_OFFSET 0x04
> -
> -static int ts4800_gpio_probe(struct platform_device *pdev)
> -{
> - struct device_node *node;
> - struct gpio_chip *chip;
> - struct resource *res;
> - void __iomem *base_addr;
> - int retval;
> - u32 ngpios;
> -
> - chip = devm_kzalloc(&pdev->dev, sizeof(struct gpio_chip), GFP_KERNEL);
> - if (!chip)
> - return -ENOMEM;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - base_addr = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(base_addr))
> - return PTR_ERR(base_addr);
> -
> - node = pdev->dev.of_node;
> - if (!node)
> - return -EINVAL;
> -
> - retval = of_property_read_u32(node, "ngpios", &ngpios);
> - if (retval == -EINVAL)
> - ngpios = DEFAULT_PIN_NUMBER;
> - else if (retval)
> - return retval;
> -
> - retval = bgpio_init(chip, &pdev->dev, 2, base_addr + INPUT_REG_OFFSET,
> - base_addr + OUTPUT_REG_OFFSET, NULL,
> - base_addr + DIRECTION_REG_OFFSET, NULL, 0);
> - if (retval) {
> - dev_err(&pdev->dev, "bgpio_init failed\n");
> - return retval;
> - }
> -
> - chip->ngpio = ngpios;
> -
> - platform_set_drvdata(pdev, chip);
> -
> - return devm_gpiochip_add_data(&pdev->dev, chip, NULL);
> -}
> -
> -static const struct of_device_id ts4800_gpio_of_match[] = {
> - { .compatible = "technologic,ts4800-gpio", },
> - {},
> -};
> -
> -static struct platform_driver ts4800_gpio_driver = {
> - .driver = {
> - .name = "ts4800-gpio",
> - .of_match_table = ts4800_gpio_of_match,
> - },
> - .probe = ts4800_gpio_probe,
> -};
> -
> -module_platform_driver_probe(ts4800_gpio_driver, ts4800_gpio_probe);
> -
> -MODULE_AUTHOR("Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>");
> -MODULE_DESCRIPTION("TS4800 FPGA GPIO driver");
> -MODULE_LICENSE("GPL v2");
> --
> 2.8.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/2] gpio: mmio: add DT support for memory-mapped GPIOs
2016-05-11 9:34 ` Christian Lamparter
@ 2016-05-12 10:26 ` Alexandre Courbot
-1 siblings, 0 replies; 26+ messages in thread
From: Alexandre Courbot @ 2016-05-12 10:26 UTC (permalink / raw)
To: Christian Lamparter
Cc: linux-gpio, linux-kernel, linux-arm-kernel,
Álvaro Fernández Rojas, Alexander Shiyan,
Alexandre Courbot, Linus Walleij, Andy Shevchenko,
Joachim Eastwood
On Wednesday, May 11, 2016 6:34:34 PM JST, Christian Lamparter wrote:
> From: Álvaro Fernández Rojas <noltari@gmail.com>
>
> This patch adds support for defining memory-mapped GPIOs which
> are compatible with the existing gpio-mmio interface. The generic
> library provides support for many memory-mapped GPIO controllers
> that are found in various on-board FPGA and ASIC solutions that
> are used to control board's switches, LEDs, chip-selects,
> Ethernet/USB PHY power, etc.
>
> For setting GPIO's there are three configurations:
s/GPIO's/GPIOs
> 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").
>
> The configuration is detected by which resources are present.
> For the single output register, this drives a 1 by setting a bit
> and a zero by clearing a bit. For the set clr pair, this drives
> a 1 by setting a bit in the set register and clears it by setting
> a bit in the clear register. The configuration is detected by
> which resources are present.
The last sentence of this paragraph repeats for first one.
>
> For setting the GPIO direction, there are three configurations:
> a. simple bidirectional GPIOs 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.
>
> The first user for this binding is "wd,mbl-gpio".
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
> drivers/gpio/gpio-mmio.c | 68
> ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> index 6c1cb3b..f72e40e 100644
> --- a/drivers/gpio/gpio-mmio.c
> +++ b/drivers/gpio/gpio-mmio.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,58 @@ static void __iomem *bgpio_map(struct
> platform_device *pdev,
> return devm_ioremap_resource(&pdev->dev, r);
> }
>
> +#ifdef CONFIG_OF
> +static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + struct device *dev = &pdev->dev;
> +
> + pdata->base = -1;
> +
> + if (of_property_read_bool(dev->of_node, "no-output"))
> + *flags |= BGPIOF_NO_OUTPUT;
I don't think it is a good idea to add "generic" properties. Whether a
controller is capable of output or not should be determined by its
compatible string only, and not a vague property.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id bgpio_of_match[] = {
> + { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
Mmm cannot you determine whether your controller is capable of output or
not just from the compatible property here? If so, the
bgpio_basic_mmio_parse_dt seems to be unneeded. If not, then this is
dependent on the wd,mbl-gpio binding and should be renamed accordingly.
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, bgpio_of_match);
> +
> +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
> + unsigned long *flags)
> +{
> + const int (*parse_dt)(struct platform_device *,
> + struct bgpio_pdata *, unsigned long *);
> + struct bgpio_pdata *pdata;
> + int err;
> +
> + parse_dt = of_device_get_match_data(&pdev->dev);
> + if (!parse_dt)
> + return NULL;
> +
> + pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
> + GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + err = parse_dt(pdev, pdata, flags);
> + if (err)
> + return ERR_PTR(err);
> +
> + return pdata;
> +}
> +#else
> +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
> + unsigned long *flags)
> +{
> + return NULL;
> +}
> +#endif /* CONFIG_OF */
> +
> static int bgpio_pdev_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -579,10 +633,19 @@ 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(pdev, &flags);
> + if (IS_ERR(pdata))
> + return PTR_ERR(pdata);
> +
> + 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,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
> static struct platform_driver bgpio_driver = {
> .driver = {
> .name = "basic-mmio-gpio",
> + .of_match_table = of_match_ptr(bgpio_of_match),
> },
> .id_table = bgpio_id_table,
> .probe = bgpio_pdev_probe,
It seems to me that this patch does two things:
1) Add code to support device tree lookup
2) Add support for "wd,mbl-gpio".
If true, these two things should be in their own patches. You should also
have another patch that adds the DT bindings for "wd,mbl-gpio", so I would
do things in that order:
1/3: DT support for basic-mmio-gpio
2/3: DT bindings for "wd,mbl-gpio" (and have them validated by the DT
people - e.g. do you really need a "reg" property or is it here just to fit
with what bgpio_pdev_probe expects? More about this on 2/2)
3/3: Support for "wd,mbl-gpio" in basic-mmio-gpio
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 1/2] gpio: mmio: add DT support for memory-mapped GPIOs
@ 2016-05-12 10:26 ` Alexandre Courbot
0 siblings, 0 replies; 26+ messages in thread
From: Alexandre Courbot @ 2016-05-12 10:26 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday, May 11, 2016 6:34:34 PM JST, Christian Lamparter wrote:
> From: ?lvaro Fern?ndez Rojas <noltari@gmail.com>
>
> This patch adds support for defining memory-mapped GPIOs which
> are compatible with the existing gpio-mmio interface. The generic
> library provides support for many memory-mapped GPIO controllers
> that are found in various on-board FPGA and ASIC solutions that
> are used to control board's switches, LEDs, chip-selects,
> Ethernet/USB PHY power, etc.
>
> For setting GPIO's there are three configurations:
s/GPIO's/GPIOs
> 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").
>
> The configuration is detected by which resources are present.
> For the single output register, this drives a 1 by setting a bit
> and a zero by clearing a bit. For the set clr pair, this drives
> a 1 by setting a bit in the set register and clears it by setting
> a bit in the clear register. The configuration is detected by
> which resources are present.
The last sentence of this paragraph repeats for first one.
>
> For setting the GPIO direction, there are three configurations:
> a. simple bidirectional GPIOs 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.
>
> The first user for this binding is "wd,mbl-gpio".
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: ?lvaro Fern?ndez Rojas <noltari@gmail.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
> drivers/gpio/gpio-mmio.c | 68
> ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> index 6c1cb3b..f72e40e 100644
> --- a/drivers/gpio/gpio-mmio.c
> +++ b/drivers/gpio/gpio-mmio.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,58 @@ static void __iomem *bgpio_map(struct
> platform_device *pdev,
> return devm_ioremap_resource(&pdev->dev, r);
> }
>
> +#ifdef CONFIG_OF
> +static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + struct device *dev = &pdev->dev;
> +
> + pdata->base = -1;
> +
> + if (of_property_read_bool(dev->of_node, "no-output"))
> + *flags |= BGPIOF_NO_OUTPUT;
I don't think it is a good idea to add "generic" properties. Whether a
controller is capable of output or not should be determined by its
compatible string only, and not a vague property.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id bgpio_of_match[] = {
> + { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
Mmm cannot you determine whether your controller is capable of output or
not just from the compatible property here? If so, the
bgpio_basic_mmio_parse_dt seems to be unneeded. If not, then this is
dependent on the wd,mbl-gpio binding and should be renamed accordingly.
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, bgpio_of_match);
> +
> +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
> + unsigned long *flags)
> +{
> + const int (*parse_dt)(struct platform_device *,
> + struct bgpio_pdata *, unsigned long *);
> + struct bgpio_pdata *pdata;
> + int err;
> +
> + parse_dt = of_device_get_match_data(&pdev->dev);
> + if (!parse_dt)
> + return NULL;
> +
> + pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
> + GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + err = parse_dt(pdev, pdata, flags);
> + if (err)
> + return ERR_PTR(err);
> +
> + return pdata;
> +}
> +#else
> +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
> + unsigned long *flags)
> +{
> + return NULL;
> +}
> +#endif /* CONFIG_OF */
> +
> static int bgpio_pdev_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -579,10 +633,19 @@ 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(pdev, &flags);
> + if (IS_ERR(pdata))
> + return PTR_ERR(pdata);
> +
> + 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,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
> static struct platform_driver bgpio_driver = {
> .driver = {
> .name = "basic-mmio-gpio",
> + .of_match_table = of_match_ptr(bgpio_of_match),
> },
> .id_table = bgpio_id_table,
> .probe = bgpio_pdev_probe,
It seems to me that this patch does two things:
1) Add code to support device tree lookup
2) Add support for "wd,mbl-gpio".
If true, these two things should be in their own patches. You should also
have another patch that adds the DT bindings for "wd,mbl-gpio", so I would
do things in that order:
1/3: DT support for basic-mmio-gpio
2/3: DT bindings for "wd,mbl-gpio" (and have them validated by the DT
people - e.g. do you really need a "reg" property or is it here just to fit
with what bgpio_pdev_probe expects? More about this on 2/2)
3/3: Support for "wd,mbl-gpio" in basic-mmio-gpio
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
2016-05-11 9:34 ` Christian Lamparter
@ 2016-05-12 10:30 ` Alexandre Courbot
-1 siblings, 0 replies; 26+ messages in thread
From: Alexandre Courbot @ 2016-05-12 10:30 UTC (permalink / raw)
To: Christian Lamparter
Cc: linux-gpio, linux-kernel, linux-arm-kernel,
Álvaro Fernández Rojas, Alexander Shiyan,
Alexandre Courbot, Linus Walleij, Andy Shevchenko,
Joachim Eastwood, Julien Grossholtz, Martyn Welch, Jonas Jensen
On Wednesday, May 11, 2016 6:34:35 PM JST, Christian Lamparter wrote:
> This patch integrates the GPIO drivers for the following
> boards, SoCs, etc. into gpio-mmio:
> - CLPS711X SoCs
> - MOXA ART SoC
> - TS-4800 FPGA DIO blocks and compatibles
> - GPIO controllers found on some GE Single Board Computers
>
> Cc: Alexander Shiyan <shc_work@mail.ru>
> Cc: Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>
> Cc: Martyn Welch <martyn.welch@ge.com>
> Cc: Jonas Jensen <jonas.jensen@gmail.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
> drivers/gpio/Kconfig | 16 +++-
> drivers/gpio/Makefile | 4 -
> drivers/gpio/gpio-clps711x.c | 91 -------------------
> drivers/gpio/gpio-ge.c | 114 -----------------------
> drivers/gpio/gpio-mmio.c | 212
> +++++++++++++++++++++++++++++++++++++++++++
> drivers/gpio/gpio-moxart.c | 84 -----------------
> drivers/gpio/gpio-ts4800.c | 81 -----------------
> 7 files changed, 224 insertions(+), 378 deletions(-)
> delete mode 100644 drivers/gpio/gpio-clps711x.c
> delete mode 100644 drivers/gpio/gpio-ge.c
> delete mode 100644 drivers/gpio/gpio-moxart.c
> delete mode 100644 drivers/gpio/gpio-ts4800.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a68d838..e4d1065 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -154,7 +154,7 @@ config GPIO_BRCMSTB
> config GPIO_CLPS711X
> tristate "CLPS711X GPIO support"
> depends on ARCH_CLPS711X || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Say yes here to support GPIO on CLPS711X SoCs.
>
> @@ -196,7 +196,7 @@ config GPIO_ETRAXFS
> config GPIO_GE_FPGA
> bool "GE FPGA based GPIO"
> depends on GE_FPGA
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Support for common GPIO functionality provided on some GE Single Board
> Computers.
> @@ -209,6 +209,14 @@ config GPIO_GENERIC_PLATFORM
> tristate "Generic memory-mapped GPIO controller support (MMIO
> platform device)"
> select GPIO_GENERIC
> help
> + Select this to support many generic memory-mapped GPIO controllers.
> +
> + This driver also includes support for the following GPIOs:
> + CLPS711X SoCs
> + MOXA ART SoC
> + TS-4800 FPGA DIO blocks and compatibles.
> + GPIOs found on some GE Single Board Computers.
> +
> Say yes here to support basic platform_device memory-mapped
> GPIO controllers.
>
> config GPIO_GRGPIO
> @@ -288,7 +296,7 @@ config GPIO_MM_LANTIQ
> config GPIO_MOXART
> bool "MOXART GPIO support"
> depends on ARCH_MOXART || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Select this option to enable GPIO driver for
> MOXA ART SoC devices.
> @@ -408,7 +416,7 @@ config GPIO_TS4800
> tristate "TS-4800 DIO blocks and compatibles"
> depends on OF_GPIO
> depends on SOC_IMX51 || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> This driver support TS-4800 FPGA GPIO controllers.
>
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 991598e..d8d63ae 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -31,7 +31,6 @@ obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o
> obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
> obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o
> obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
> -obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
> obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
> obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o
> obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
> @@ -43,7 +42,6 @@ obj-$(CONFIG_GPIO_EM) += gpio-em.o
> obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
> obj-$(CONFIG_GPIO_ETRAXFS) += gpio-etraxfs.o
> obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
> -obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
> obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
> obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
> obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
> @@ -68,7 +66,6 @@ obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
> obj-$(CONFIG_GPIO_MCP23S08) += gpio-mcp23s08.o
> obj-$(CONFIG_GPIO_ML_IOH) += gpio-ml-ioh.o
> obj-$(CONFIG_GPIO_MM_LANTIQ) += gpio-mm-lantiq.o
> -obj-$(CONFIG_GPIO_MOXART) += gpio-moxart.o
> obj-$(CONFIG_GPIO_MPC5200) += gpio-mpc5200.o
> obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o
> obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o
> @@ -107,7 +104,6 @@ obj-$(CONFIG_GPIO_TPS65218) += gpio-tps65218.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_TS4800) += gpio-ts4800.o
> obj-$(CONFIG_GPIO_TS5500) += gpio-ts5500.o
> obj-$(CONFIG_GPIO_TWL4030) += gpio-twl4030.o
> obj-$(CONFIG_GPIO_TWL6040) += gpio-twl6040.o
> diff --git a/drivers/gpio/gpio-clps711x.c b/drivers/gpio/gpio-clps711x.c
> deleted file mode 100644
> index 5a69025..0000000
> --- a/drivers/gpio/gpio-clps711x.c
> +++ /dev/null
> @@ -1,91 +0,0 @@
> -/*
> - * CLPS711X GPIO driver
> - *
> - * Copyright (C) 2012,2013 Alexander Shiyan <shc_work@mail.ru>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - */
> -
> -#include <linux/err.h>
> -#include <linux/module.h>
> -#include <linux/gpio/driver.h>
> -#include <linux/platform_device.h>
> -
> -static int clps711x_gpio_probe(struct platform_device *pdev)
> -{
> - struct device_node *np = pdev->dev.of_node;
> - void __iomem *dat, *dir;
> - struct gpio_chip *gc;
> - struct resource *res;
> - int err, id = np ? of_alias_get_id(np, "gpio") : pdev->id;
> -
> - if ((id < 0) || (id > 4))
> - return -ENODEV;
> -
> - gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
> - if (!gc)
> - return -ENOMEM;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - dat = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(dat))
> - return PTR_ERR(dat);
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - dir = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(dir))
> - return PTR_ERR(dir);
> -
> - switch (id) {
> - case 3:
> - /* PORTD is inverted logic for direction register */
> - err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
> - NULL, dir, 0);
> - break;
> - default:
> - err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
> - dir, NULL, 0);
> - break;
> - }
> -
> - if (err)
> - return err;
> -
> - switch (id) {
> - case 4:
> - /* PORTE is 3 lines only */
> - gc->ngpio = 3;
> - break;
> - default:
> - break;
> - }
> -
> - gc->base = id * 8;
> - gc->owner = THIS_MODULE;
> - platform_set_drvdata(pdev, gc);
> -
> - return devm_gpiochip_add_data(&pdev->dev, gc, NULL);
> -}
> -
> -static const struct of_device_id __maybe_unused clps711x_gpio_ids[] = {
> - { .compatible = "cirrus,clps711x-gpio" },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, clps711x_gpio_ids);
> -
> -static struct platform_driver clps711x_gpio_driver = {
> - .driver = {
> - .name = "clps711x-gpio",
> - .of_match_table = of_match_ptr(clps711x_gpio_ids),
> - },
> - .probe = clps711x_gpio_probe,
> -};
> -module_platform_driver(clps711x_gpio_driver);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
> -MODULE_DESCRIPTION("CLPS711X GPIO driver");
> -MODULE_ALIAS("platform:clps711x-gpio");
> diff --git a/drivers/gpio/gpio-ge.c b/drivers/gpio/gpio-ge.c
> deleted file mode 100644
> index 8650b29..0000000
> --- a/drivers/gpio/gpio-ge.c
> +++ /dev/null
> @@ -1,114 +0,0 @@
> -/*
> - * Driver for GE FPGA based GPIO
> - *
> - * Author: Martyn Welch <martyn.welch@ge.com>
> - *
> - * 2008 (c) GE Intelligent Platforms Embedded Systems, Inc.
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
> - */
> -
> -/* TODO
> - *
> - * Configuration of output modes (totem-pole/open-drain)
> - * Interrupt configuration - interrupts are always generated
> the FPGA relies on
> - * the I/O interrupt controllers mask to stop them propergating
> - */
> -
> -#include <linux/kernel.h>
> -#include <linux/io.h>
> -#include <linux/slab.h>
> -#include <linux/of_device.h>
> -#include <linux/of_gpio.h>
> -#include <linux/of_address.h>
> -#include <linux/module.h>
> -#include <linux/gpio/driver.h>
> -
> -#define GEF_GPIO_DIRECT 0x00
> -#define GEF_GPIO_IN 0x04
> -#define GEF_GPIO_OUT 0x08
> -#define GEF_GPIO_TRIG 0x0C
> -#define GEF_GPIO_POLAR_A 0x10
> -#define GEF_GPIO_POLAR_B 0x14
> -#define GEF_GPIO_INT_STAT 0x18
> -#define GEF_GPIO_OVERRUN 0x1C
> -#define GEF_GPIO_MODE 0x20
> -
> -static const struct of_device_id gef_gpio_ids[] = {
> - {
> - .compatible = "gef,sbc610-gpio",
> - .data = (void *)19,
> - }, {
> - .compatible = "gef,sbc310-gpio",
> - .data = (void *)6,
> - }, {
> - .compatible = "ge,imp3a-gpio",
> - .data = (void *)16,
> - },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, gef_gpio_ids);
> -
> -static int __init gef_gpio_probe(struct platform_device *pdev)
> -{
> - const struct of_device_id *of_id =
> - of_match_device(gef_gpio_ids, &pdev->dev);
> - struct gpio_chip *gc;
> - void __iomem *regs;
> - int ret;
> -
> - gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
> - if (!gc)
> - return -ENOMEM;
> -
> - regs = of_iomap(pdev->dev.of_node, 0);
> - if (!regs)
> - return -ENOMEM;
> -
> - ret = bgpio_init(gc, &pdev->dev, 4, regs + GEF_GPIO_IN,
> - regs + GEF_GPIO_OUT, NULL, NULL,
> - regs + GEF_GPIO_DIRECT, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
> - if (ret) {
> - dev_err(&pdev->dev, "bgpio_init failed\n");
> - goto err0;
> - }
> -
> - /* Setup pointers to chip functions */
> - gc->label = devm_kstrdup(&pdev->dev, pdev->dev.of_node->full_name,
> - GFP_KERNEL);
> - if (!gc->label) {
> - ret = -ENOMEM;
> - goto err0;
> - }
> -
> - gc->base = -1;
> - gc->ngpio = (u16)(uintptr_t)of_id->data;
> - gc->of_gpio_n_cells = 2;
> - gc->of_node = pdev->dev.of_node;
> -
> - /* This function adds a memory mapped GPIO chip */
> - ret = devm_gpiochip_add_data(&pdev->dev, gc, NULL);
> - if (ret)
> - goto err0;
> -
> - return 0;
> -err0:
> - iounmap(regs);
> - pr_err("%s: GPIO chip registration failed\n",
> - pdev->dev.of_node->full_name);
> - return ret;
> -};
> -
> -static struct platform_driver gef_gpio_driver = {
> - .driver = {
> - .name = "gef-gpio",
> - .of_match_table = gef_gpio_ids,
> - },
> -};
> -module_platform_driver_probe(gef_gpio_driver, gef_gpio_probe);
> -
> -MODULE_DESCRIPTION("GE I/O FPGA GPIO driver");
> -MODULE_AUTHOR("Martyn Welch <martyn.welch@ge.com");
> -MODULE_LICENSE("GPL");
> diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> index f72e40e..1de9172 100644
> --- a/drivers/gpio/gpio-mmio.c
> +++ b/drivers/gpio/gpio-mmio.c
> @@ -586,8 +586,220 @@ static int
> bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> return 0;
> }
>
> +static int clps711x_parse_dt(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + const char *dir_reg_name;
> + int id = np ? of_alias_get_id(np, "gpio") : pdev->id;
> +
> + switch (id) {
> + case 0:
> + case 1:
> + case 2:
> + pdata->ngpio = 0; /* determined by register width */
> + dir_reg_name = "dirout";
> + break;
> + case 3:
> + pdata->ngpio = 0; /* determined by register width */
> + /* PORTD is inverted logic for direction register */
> + dir_reg_name = "dirin";
> + break;
> + case 4:
> + pdata->ngpio = 3; /* PORTE is 3 lines only */
> + dir_reg_name = "dirout";
> + break;
> + default:
> + return -ENODEV;
> + }
> +
> + pdata->base = id * 8;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> + if (!res->name || strcmp("dat", res->name))
> + res->name = devm_kstrdup(&pdev->dev, "dat", GFP_KERNEL);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res)
> + return -EINVAL;
> + if (!res->name || strcmp(dir_reg_name, res->name))
> + res->name = devm_kstrdup(&pdev->dev, dir_reg_name, GFP_KERNEL);
Ouch, I don't like this. Also the probability that the "if (!res->name ||
...)" condition is not met is so small that I would not bother with it and
do the kstrdup unconditionally. But maybe we can remove this altogether -
see below...
> +
> + return 0;
> +}
> +
> +static int ge_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + struct device_node *np = pdev->dev.of_node;
> +
> + pdata->label = devm_kstrdup(&pdev->dev, np->full_name, GFP_KERNEL);
> + if (!pdata->label)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int moxart_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + pdata->base = 0;
> + pdata->label = "moxart-gpio";
> + return 0;
> +}
> +
> +
> +static int ts4800_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + int err;
> +
> + err = of_property_read_u32(pdev->dev.of_node, "ngpios", &pdata->ngpio);
> + if (err == -EINVAL) {
> + pdata->ngpio = 16;
> + return 0;
> + }
> +
> + return err;
> +}
> +
> +/*
> + * bgpio_init() needs up to five named mmio register resources.
> + * These currently are dat, set, clr, [dirout | dirin]. There
> + * is no particular order or predefined place for the entries.
> + * However, dirout and dirin are mutually exclusive.
> + */
> +#define __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES 4
> +
> +struct compat_gpio_device_data {
> + unsigned int expected_resource_size;
> + unsigned int ngpio;
> + resource_size_t register_width;
> + unsigned long flags;
> + int (*call_back)(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags);
> + struct resource_replacement {
> + resource_size_t start_offset;
> + const char *name;
> + } resources[__NUM_COMPAT_OVERRIDE_MMIO_RESOURCES];
> +};
> +#undef __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES
> +
> +#define ADD_COMPAT_REGISTER(_name, _offset) \
> + { .name = (_name), .start_offset = (_offset) }
> +
> +#define ADD_COMPAT_GPIO(_comp, _sz, _ngpio, _width, _cb, _f, _res...) \
> + { \
> + .compatible = (_comp), \
> + .data = &(struct compat_gpio_device_data) { \
> + .expected_resource_size = (_sz), \
> + .ngpio = (_ngpio), \
> + .register_width = (_width), \
> + .flags = (_f), \
> + .call_back = (_cb), \
> + .resources = { _res }, \
> + } \
> +}
> +
> +#define ADD_COMPAT_GE_GPIO(_name, _ngpio) \
> + ADD_COMPAT_GPIO(_name, 0x24, _ngpio, 0x4, ge_dt_cb, \
> + BGPIOF_BIG_ENDIAN_BYTE_ORDER, \
> + ADD_COMPAT_REGISTER("dat", 0x04), \
> + ADD_COMPAT_REGISTER("set", 0x08), \
> + ADD_COMPAT_REGISTER("dirin", 0x00)) \
> +
> +static const struct of_device_id compat_gpio_devices[] = {
> + ADD_COMPAT_GE_GPIO("ge,imp3a-gpio", 16),
> + ADD_COMPAT_GE_GPIO("gef,sbc310-gpio", 6),
> + ADD_COMPAT_GE_GPIO("gef,sbc610-gpio", 19),
> + ADD_COMPAT_GPIO("moxa,moxart-gpio", 0xc, 0, 0x4, moxart_dt_cb,
> + BGPIOF_READ_OUTPUT_REG_SET,
> + ADD_COMPAT_REGISTER("dat", 0x04),
> + ADD_COMPAT_REGISTER("set", 0x00),
> + ADD_COMPAT_REGISTER("dirout", 0x08)),
> + ADD_COMPAT_GPIO("technologic,ts4800-gpio", 0x6, 16, 0x2, ts4800_dt_cb,
> + 0, ADD_COMPAT_REGISTER("dat", 0x00),
> + ADD_COMPAT_REGISTER("set", 0x02),
> + ADD_COMPAT_REGISTER("dirout", 0x04)),
> +};
> +
> +#undef ADD_COMPAT_GE_GPIO
> +#undef ADD_COMPAT_GPIO
> +#undef ADD_COMPAT_REGISTER
> +
> +static int compat_parse_dt(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + const struct device_node *node = pdev->dev.of_node;
> + const struct compat_gpio_device_data *entry;
> + const struct of_device_id *of_id;
> + struct resource *res;
> + int err;
> +
> + of_id = of_match_node(compat_gpio_devices, node);
> + if (!of_id)
> + return -ENODEV;
> +
> + entry = of_id->data;
> + if (!entry || !entry->resources[0].name)
> + return -EINVAL;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + if (!res->name || strcmp(entry->resources[0].name, res->name)) {
> + struct resource nres[ARRAY_SIZE(entry->resources)];
> + size_t i;
> +
> + if (resource_size(res) != entry->expected_resource_size)
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(entry->resources); i++) {
> + if (!entry->resources[i].name)
> + continue;
> +
> + nres[i].name = devm_kstrdup(&pdev->dev,
> + entry->resources[i].name, GFP_KERNEL);
> + nres[i].start = res->start +
> + entry->resources[i].start_offset;
> + nres[i].end = nres[i].start +
> + entry->register_width - 1;
> + nres[i].flags = IORESOURCE_MEM;
> + }
> +
> + err = platform_device_add_resources(pdev, nres, i);
> + if (err)
> + return err;
> + }
> +
> + pdata->base = -1;
> + pdata->ngpio = entry->ngpio;
> + *flags = entry->flags;
> +
> + if (entry->call_back)
> + err = entry->call_back(pdev, pdata, flags);
> +
> + return err;
> +}
Ok, so this is getting quite complex, with two of_device_id tables and two
levels of hooks, mainly because you want to use the bgpio_pdev_probe()
function which relies on named memory resources.
This function is here for basic platform devices, and you are not obliged
to rely on it for DT. Feel free to write your own function (or rather split
bgpio_pdev_probe() in two code paths) if it can reduce the amount of black
magic you need to do (especially the renaming of resources on-the-fly).
At the end of the day what you want is a function that calls bgpio_init()
with the right parameters. These parameters should be inferred from the
compatible property whenever possible (which is most of the time, and why I
am suspiscious of using "reg" properties for these devices since I don't
think it can change much?), and passed as-is to bgpio_init(). Your
compat_gpio_device_data struct basically encodes this, so this will take
care of all the "compat" devices.
For the other devices, you should be able to fill in such a structure from
the DT properties. Then you just use it for the parameters of bgpio_init().
It will be more elegant that adding resources, and probably shorter as
well.
> +
> static const struct of_device_id bgpio_of_match[] = {
> { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
> + { .compatible = "cirrus,clps711x-gpio", .data = clps711x_parse_dt },
> + { .compatible = "ge,imp3a-gpio", .data = compat_parse_dt },
> + { .compatible = "gef,sbc310-gpio", .data = compat_parse_dt },
> + { .compatible = "gef,sbc610-gpio", .data = compat_parse_dt },
> + { .compatible = "moxa,moxart-gpio", .data = compat_parse_dt },
> + { .compatible = "technologic,ts4800-gpio", .data = compat_parse_dt },
If you implement my idea, the function referred to by .data should return
that structure I was talking about, and bgpio_init() can be called directly
on it after that. The current path of bgpio_pdev_probe() should only be
taken if bgpio_parse_dt() returned NULL.
I believe this will reduce the number of lines in your patch some more, and
it will also make the code easier to read.
Also this should be split into several patches, like the previous one: the
first one adds the required infrastructure to gpio-mmio, and the subsequent
patches each move one device into gpio-mmio.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
@ 2016-05-12 10:30 ` Alexandre Courbot
0 siblings, 0 replies; 26+ messages in thread
From: Alexandre Courbot @ 2016-05-12 10:30 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday, May 11, 2016 6:34:35 PM JST, Christian Lamparter wrote:
> This patch integrates the GPIO drivers for the following
> boards, SoCs, etc. into gpio-mmio:
> - CLPS711X SoCs
> - MOXA ART SoC
> - TS-4800 FPGA DIO blocks and compatibles
> - GPIO controllers found on some GE Single Board Computers
>
> Cc: Alexander Shiyan <shc_work@mail.ru>
> Cc: Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>
> Cc: Martyn Welch <martyn.welch@ge.com>
> Cc: Jonas Jensen <jonas.jensen@gmail.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
> drivers/gpio/Kconfig | 16 +++-
> drivers/gpio/Makefile | 4 -
> drivers/gpio/gpio-clps711x.c | 91 -------------------
> drivers/gpio/gpio-ge.c | 114 -----------------------
> drivers/gpio/gpio-mmio.c | 212
> +++++++++++++++++++++++++++++++++++++++++++
> drivers/gpio/gpio-moxart.c | 84 -----------------
> drivers/gpio/gpio-ts4800.c | 81 -----------------
> 7 files changed, 224 insertions(+), 378 deletions(-)
> delete mode 100644 drivers/gpio/gpio-clps711x.c
> delete mode 100644 drivers/gpio/gpio-ge.c
> delete mode 100644 drivers/gpio/gpio-moxart.c
> delete mode 100644 drivers/gpio/gpio-ts4800.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a68d838..e4d1065 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -154,7 +154,7 @@ config GPIO_BRCMSTB
> config GPIO_CLPS711X
> tristate "CLPS711X GPIO support"
> depends on ARCH_CLPS711X || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Say yes here to support GPIO on CLPS711X SoCs.
>
> @@ -196,7 +196,7 @@ config GPIO_ETRAXFS
> config GPIO_GE_FPGA
> bool "GE FPGA based GPIO"
> depends on GE_FPGA
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Support for common GPIO functionality provided on some GE Single Board
> Computers.
> @@ -209,6 +209,14 @@ config GPIO_GENERIC_PLATFORM
> tristate "Generic memory-mapped GPIO controller support (MMIO
> platform device)"
> select GPIO_GENERIC
> help
> + Select this to support many generic memory-mapped GPIO controllers.
> +
> + This driver also includes support for the following GPIOs:
> + CLPS711X SoCs
> + MOXA ART SoC
> + TS-4800 FPGA DIO blocks and compatibles.
> + GPIOs found on some GE Single Board Computers.
> +
> Say yes here to support basic platform_device memory-mapped
> GPIO controllers.
>
> config GPIO_GRGPIO
> @@ -288,7 +296,7 @@ config GPIO_MM_LANTIQ
> config GPIO_MOXART
> bool "MOXART GPIO support"
> depends on ARCH_MOXART || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> Select this option to enable GPIO driver for
> MOXA ART SoC devices.
> @@ -408,7 +416,7 @@ config GPIO_TS4800
> tristate "TS-4800 DIO blocks and compatibles"
> depends on OF_GPIO
> depends on SOC_IMX51 || COMPILE_TEST
> - select GPIO_GENERIC
> + select GPIO_GENERIC_PLATFORM
> help
> This driver support TS-4800 FPGA GPIO controllers.
>
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 991598e..d8d63ae 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -31,7 +31,6 @@ obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o
> obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
> obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o
> obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
> -obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
> obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
> obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o
> obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
> @@ -43,7 +42,6 @@ obj-$(CONFIG_GPIO_EM) += gpio-em.o
> obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
> obj-$(CONFIG_GPIO_ETRAXFS) += gpio-etraxfs.o
> obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o
> -obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
> obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
> obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
> obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
> @@ -68,7 +66,6 @@ obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
> obj-$(CONFIG_GPIO_MCP23S08) += gpio-mcp23s08.o
> obj-$(CONFIG_GPIO_ML_IOH) += gpio-ml-ioh.o
> obj-$(CONFIG_GPIO_MM_LANTIQ) += gpio-mm-lantiq.o
> -obj-$(CONFIG_GPIO_MOXART) += gpio-moxart.o
> obj-$(CONFIG_GPIO_MPC5200) += gpio-mpc5200.o
> obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o
> obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o
> @@ -107,7 +104,6 @@ obj-$(CONFIG_GPIO_TPS65218) += gpio-tps65218.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_TS4800) += gpio-ts4800.o
> obj-$(CONFIG_GPIO_TS5500) += gpio-ts5500.o
> obj-$(CONFIG_GPIO_TWL4030) += gpio-twl4030.o
> obj-$(CONFIG_GPIO_TWL6040) += gpio-twl6040.o
> diff --git a/drivers/gpio/gpio-clps711x.c b/drivers/gpio/gpio-clps711x.c
> deleted file mode 100644
> index 5a69025..0000000
> --- a/drivers/gpio/gpio-clps711x.c
> +++ /dev/null
> @@ -1,91 +0,0 @@
> -/*
> - * CLPS711X GPIO driver
> - *
> - * Copyright (C) 2012,2013 Alexander Shiyan <shc_work@mail.ru>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - */
> -
> -#include <linux/err.h>
> -#include <linux/module.h>
> -#include <linux/gpio/driver.h>
> -#include <linux/platform_device.h>
> -
> -static int clps711x_gpio_probe(struct platform_device *pdev)
> -{
> - struct device_node *np = pdev->dev.of_node;
> - void __iomem *dat, *dir;
> - struct gpio_chip *gc;
> - struct resource *res;
> - int err, id = np ? of_alias_get_id(np, "gpio") : pdev->id;
> -
> - if ((id < 0) || (id > 4))
> - return -ENODEV;
> -
> - gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
> - if (!gc)
> - return -ENOMEM;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - dat = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(dat))
> - return PTR_ERR(dat);
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - dir = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(dir))
> - return PTR_ERR(dir);
> -
> - switch (id) {
> - case 3:
> - /* PORTD is inverted logic for direction register */
> - err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
> - NULL, dir, 0);
> - break;
> - default:
> - err = bgpio_init(gc, &pdev->dev, 1, dat, NULL, NULL,
> - dir, NULL, 0);
> - break;
> - }
> -
> - if (err)
> - return err;
> -
> - switch (id) {
> - case 4:
> - /* PORTE is 3 lines only */
> - gc->ngpio = 3;
> - break;
> - default:
> - break;
> - }
> -
> - gc->base = id * 8;
> - gc->owner = THIS_MODULE;
> - platform_set_drvdata(pdev, gc);
> -
> - return devm_gpiochip_add_data(&pdev->dev, gc, NULL);
> -}
> -
> -static const struct of_device_id __maybe_unused clps711x_gpio_ids[] = {
> - { .compatible = "cirrus,clps711x-gpio" },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, clps711x_gpio_ids);
> -
> -static struct platform_driver clps711x_gpio_driver = {
> - .driver = {
> - .name = "clps711x-gpio",
> - .of_match_table = of_match_ptr(clps711x_gpio_ids),
> - },
> - .probe = clps711x_gpio_probe,
> -};
> -module_platform_driver(clps711x_gpio_driver);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
> -MODULE_DESCRIPTION("CLPS711X GPIO driver");
> -MODULE_ALIAS("platform:clps711x-gpio");
> diff --git a/drivers/gpio/gpio-ge.c b/drivers/gpio/gpio-ge.c
> deleted file mode 100644
> index 8650b29..0000000
> --- a/drivers/gpio/gpio-ge.c
> +++ /dev/null
> @@ -1,114 +0,0 @@
> -/*
> - * Driver for GE FPGA based GPIO
> - *
> - * Author: Martyn Welch <martyn.welch@ge.com>
> - *
> - * 2008 (c) GE Intelligent Platforms Embedded Systems, Inc.
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
> - */
> -
> -/* TODO
> - *
> - * Configuration of output modes (totem-pole/open-drain)
> - * Interrupt configuration - interrupts are always generated
> the FPGA relies on
> - * the I/O interrupt controllers mask to stop them propergating
> - */
> -
> -#include <linux/kernel.h>
> -#include <linux/io.h>
> -#include <linux/slab.h>
> -#include <linux/of_device.h>
> -#include <linux/of_gpio.h>
> -#include <linux/of_address.h>
> -#include <linux/module.h>
> -#include <linux/gpio/driver.h>
> -
> -#define GEF_GPIO_DIRECT 0x00
> -#define GEF_GPIO_IN 0x04
> -#define GEF_GPIO_OUT 0x08
> -#define GEF_GPIO_TRIG 0x0C
> -#define GEF_GPIO_POLAR_A 0x10
> -#define GEF_GPIO_POLAR_B 0x14
> -#define GEF_GPIO_INT_STAT 0x18
> -#define GEF_GPIO_OVERRUN 0x1C
> -#define GEF_GPIO_MODE 0x20
> -
> -static const struct of_device_id gef_gpio_ids[] = {
> - {
> - .compatible = "gef,sbc610-gpio",
> - .data = (void *)19,
> - }, {
> - .compatible = "gef,sbc310-gpio",
> - .data = (void *)6,
> - }, {
> - .compatible = "ge,imp3a-gpio",
> - .data = (void *)16,
> - },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, gef_gpio_ids);
> -
> -static int __init gef_gpio_probe(struct platform_device *pdev)
> -{
> - const struct of_device_id *of_id =
> - of_match_device(gef_gpio_ids, &pdev->dev);
> - struct gpio_chip *gc;
> - void __iomem *regs;
> - int ret;
> -
> - gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
> - if (!gc)
> - return -ENOMEM;
> -
> - regs = of_iomap(pdev->dev.of_node, 0);
> - if (!regs)
> - return -ENOMEM;
> -
> - ret = bgpio_init(gc, &pdev->dev, 4, regs + GEF_GPIO_IN,
> - regs + GEF_GPIO_OUT, NULL, NULL,
> - regs + GEF_GPIO_DIRECT, BGPIOF_BIG_ENDIAN_BYTE_ORDER);
> - if (ret) {
> - dev_err(&pdev->dev, "bgpio_init failed\n");
> - goto err0;
> - }
> -
> - /* Setup pointers to chip functions */
> - gc->label = devm_kstrdup(&pdev->dev, pdev->dev.of_node->full_name,
> - GFP_KERNEL);
> - if (!gc->label) {
> - ret = -ENOMEM;
> - goto err0;
> - }
> -
> - gc->base = -1;
> - gc->ngpio = (u16)(uintptr_t)of_id->data;
> - gc->of_gpio_n_cells = 2;
> - gc->of_node = pdev->dev.of_node;
> -
> - /* This function adds a memory mapped GPIO chip */
> - ret = devm_gpiochip_add_data(&pdev->dev, gc, NULL);
> - if (ret)
> - goto err0;
> -
> - return 0;
> -err0:
> - iounmap(regs);
> - pr_err("%s: GPIO chip registration failed\n",
> - pdev->dev.of_node->full_name);
> - return ret;
> -};
> -
> -static struct platform_driver gef_gpio_driver = {
> - .driver = {
> - .name = "gef-gpio",
> - .of_match_table = gef_gpio_ids,
> - },
> -};
> -module_platform_driver_probe(gef_gpio_driver, gef_gpio_probe);
> -
> -MODULE_DESCRIPTION("GE I/O FPGA GPIO driver");
> -MODULE_AUTHOR("Martyn Welch <martyn.welch@ge.com");
> -MODULE_LICENSE("GPL");
> diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> index f72e40e..1de9172 100644
> --- a/drivers/gpio/gpio-mmio.c
> +++ b/drivers/gpio/gpio-mmio.c
> @@ -586,8 +586,220 @@ static int
> bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> return 0;
> }
>
> +static int clps711x_parse_dt(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + const char *dir_reg_name;
> + int id = np ? of_alias_get_id(np, "gpio") : pdev->id;
> +
> + switch (id) {
> + case 0:
> + case 1:
> + case 2:
> + pdata->ngpio = 0; /* determined by register width */
> + dir_reg_name = "dirout";
> + break;
> + case 3:
> + pdata->ngpio = 0; /* determined by register width */
> + /* PORTD is inverted logic for direction register */
> + dir_reg_name = "dirin";
> + break;
> + case 4:
> + pdata->ngpio = 3; /* PORTE is 3 lines only */
> + dir_reg_name = "dirout";
> + break;
> + default:
> + return -ENODEV;
> + }
> +
> + pdata->base = id * 8;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> + if (!res->name || strcmp("dat", res->name))
> + res->name = devm_kstrdup(&pdev->dev, "dat", GFP_KERNEL);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res)
> + return -EINVAL;
> + if (!res->name || strcmp(dir_reg_name, res->name))
> + res->name = devm_kstrdup(&pdev->dev, dir_reg_name, GFP_KERNEL);
Ouch, I don't like this. Also the probability that the "if (!res->name ||
...)" condition is not met is so small that I would not bother with it and
do the kstrdup unconditionally. But maybe we can remove this altogether -
see below...
> +
> + return 0;
> +}
> +
> +static int ge_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + struct device_node *np = pdev->dev.of_node;
> +
> + pdata->label = devm_kstrdup(&pdev->dev, np->full_name, GFP_KERNEL);
> + if (!pdata->label)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int moxart_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + pdata->base = 0;
> + pdata->label = "moxart-gpio";
> + return 0;
> +}
> +
> +
> +static int ts4800_dt_cb(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + int err;
> +
> + err = of_property_read_u32(pdev->dev.of_node, "ngpios", &pdata->ngpio);
> + if (err == -EINVAL) {
> + pdata->ngpio = 16;
> + return 0;
> + }
> +
> + return err;
> +}
> +
> +/*
> + * bgpio_init() needs up to five named mmio register resources.
> + * These currently are dat, set, clr, [dirout | dirin]. There
> + * is no particular order or predefined place for the entries.
> + * However, dirout and dirin are mutually exclusive.
> + */
> +#define __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES 4
> +
> +struct compat_gpio_device_data {
> + unsigned int expected_resource_size;
> + unsigned int ngpio;
> + resource_size_t register_width;
> + unsigned long flags;
> + int (*call_back)(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags);
> + struct resource_replacement {
> + resource_size_t start_offset;
> + const char *name;
> + } resources[__NUM_COMPAT_OVERRIDE_MMIO_RESOURCES];
> +};
> +#undef __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES
> +
> +#define ADD_COMPAT_REGISTER(_name, _offset) \
> + { .name = (_name), .start_offset = (_offset) }
> +
> +#define ADD_COMPAT_GPIO(_comp, _sz, _ngpio, _width, _cb, _f, _res...) \
> + { \
> + .compatible = (_comp), \
> + .data = &(struct compat_gpio_device_data) { \
> + .expected_resource_size = (_sz), \
> + .ngpio = (_ngpio), \
> + .register_width = (_width), \
> + .flags = (_f), \
> + .call_back = (_cb), \
> + .resources = { _res }, \
> + } \
> +}
> +
> +#define ADD_COMPAT_GE_GPIO(_name, _ngpio) \
> + ADD_COMPAT_GPIO(_name, 0x24, _ngpio, 0x4, ge_dt_cb, \
> + BGPIOF_BIG_ENDIAN_BYTE_ORDER, \
> + ADD_COMPAT_REGISTER("dat", 0x04), \
> + ADD_COMPAT_REGISTER("set", 0x08), \
> + ADD_COMPAT_REGISTER("dirin", 0x00)) \
> +
> +static const struct of_device_id compat_gpio_devices[] = {
> + ADD_COMPAT_GE_GPIO("ge,imp3a-gpio", 16),
> + ADD_COMPAT_GE_GPIO("gef,sbc310-gpio", 6),
> + ADD_COMPAT_GE_GPIO("gef,sbc610-gpio", 19),
> + ADD_COMPAT_GPIO("moxa,moxart-gpio", 0xc, 0, 0x4, moxart_dt_cb,
> + BGPIOF_READ_OUTPUT_REG_SET,
> + ADD_COMPAT_REGISTER("dat", 0x04),
> + ADD_COMPAT_REGISTER("set", 0x00),
> + ADD_COMPAT_REGISTER("dirout", 0x08)),
> + ADD_COMPAT_GPIO("technologic,ts4800-gpio", 0x6, 16, 0x2, ts4800_dt_cb,
> + 0, ADD_COMPAT_REGISTER("dat", 0x00),
> + ADD_COMPAT_REGISTER("set", 0x02),
> + ADD_COMPAT_REGISTER("dirout", 0x04)),
> +};
> +
> +#undef ADD_COMPAT_GE_GPIO
> +#undef ADD_COMPAT_GPIO
> +#undef ADD_COMPAT_REGISTER
> +
> +static int compat_parse_dt(struct platform_device *pdev,
> + struct bgpio_pdata *pdata,
> + unsigned long *flags)
> +{
> + const struct device_node *node = pdev->dev.of_node;
> + const struct compat_gpio_device_data *entry;
> + const struct of_device_id *of_id;
> + struct resource *res;
> + int err;
> +
> + of_id = of_match_node(compat_gpio_devices, node);
> + if (!of_id)
> + return -ENODEV;
> +
> + entry = of_id->data;
> + if (!entry || !entry->resources[0].name)
> + return -EINVAL;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + if (!res->name || strcmp(entry->resources[0].name, res->name)) {
> + struct resource nres[ARRAY_SIZE(entry->resources)];
> + size_t i;
> +
> + if (resource_size(res) != entry->expected_resource_size)
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(entry->resources); i++) {
> + if (!entry->resources[i].name)
> + continue;
> +
> + nres[i].name = devm_kstrdup(&pdev->dev,
> + entry->resources[i].name, GFP_KERNEL);
> + nres[i].start = res->start +
> + entry->resources[i].start_offset;
> + nres[i].end = nres[i].start +
> + entry->register_width - 1;
> + nres[i].flags = IORESOURCE_MEM;
> + }
> +
> + err = platform_device_add_resources(pdev, nres, i);
> + if (err)
> + return err;
> + }
> +
> + pdata->base = -1;
> + pdata->ngpio = entry->ngpio;
> + *flags = entry->flags;
> +
> + if (entry->call_back)
> + err = entry->call_back(pdev, pdata, flags);
> +
> + return err;
> +}
Ok, so this is getting quite complex, with two of_device_id tables and two
levels of hooks, mainly because you want to use the bgpio_pdev_probe()
function which relies on named memory resources.
This function is here for basic platform devices, and you are not obliged
to rely on it for DT. Feel free to write your own function (or rather split
bgpio_pdev_probe() in two code paths) if it can reduce the amount of black
magic you need to do (especially the renaming of resources on-the-fly).
At the end of the day what you want is a function that calls bgpio_init()
with the right parameters. These parameters should be inferred from the
compatible property whenever possible (which is most of the time, and why I
am suspiscious of using "reg" properties for these devices since I don't
think it can change much?), and passed as-is to bgpio_init(). Your
compat_gpio_device_data struct basically encodes this, so this will take
care of all the "compat" devices.
For the other devices, you should be able to fill in such a structure from
the DT properties. Then you just use it for the parameters of bgpio_init().
It will be more elegant that adding resources, and probably shorter as
well.
> +
> static const struct of_device_id bgpio_of_match[] = {
> { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
> + { .compatible = "cirrus,clps711x-gpio", .data = clps711x_parse_dt },
> + { .compatible = "ge,imp3a-gpio", .data = compat_parse_dt },
> + { .compatible = "gef,sbc310-gpio", .data = compat_parse_dt },
> + { .compatible = "gef,sbc610-gpio", .data = compat_parse_dt },
> + { .compatible = "moxa,moxart-gpio", .data = compat_parse_dt },
> + { .compatible = "technologic,ts4800-gpio", .data = compat_parse_dt },
If you implement my idea, the function referred to by .data should return
that structure I was talking about, and bgpio_init() can be called directly
on it after that. The current path of bgpio_pdev_probe() should only be
taken if bgpio_parse_dt() returned NULL.
I believe this will reduce the number of lines in your patch some more, and
it will also make the code easier to read.
Also this should be split into several patches, like the previous one: the
first one adds the required infrastructure to gpio-mmio, and the subsequent
patches each move one device into gpio-mmio.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/2] gpio: mmio: add DT support for memory-mapped GPIOs
2016-05-12 10:26 ` Alexandre Courbot
(?)
@ 2016-05-12 12:07 ` Christian Lamparter
-1 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-12 12:07 UTC (permalink / raw)
To: Alexandre Courbot
Cc: linux-gpio, linux-kernel, linux-arm-kernel,
Álvaro Fernández Rojas, Alexander Shiyan,
Linus Walleij, Andy Shevchenko, Joachim Eastwood
On Thursday, May 12, 2016 07:26:32 PM Alexandre Courbot wrote:
> On Wednesday, May 11, 2016 6:34:34 PM JST, Christian Lamparter wrote:
> > From: Álvaro Fernández Rojas <noltari@gmail.com>
> >
> > This patch adds support for defining memory-mapped GPIOs which
> > are compatible with the existing gpio-mmio interface. The generic
> > library provides support for many memory-mapped GPIO controllers
> > that are found in various on-board FPGA and ASIC solutions that
> > are used to control board's switches, LEDs, chip-selects,
> > Ethernet/USB PHY power, etc.
> >
> > For setting GPIO's there are three configurations:
>
> s/GPIO's/GPIOs
OK
> > 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").
> >
> > The configuration is detected by which resources are present.
> > For the single output register, this drives a 1 by setting a bit
> > and a zero by clearing a bit. For the set clr pair, this drives
> > a 1 by setting a bit in the set register and clears it by setting
> > a bit in the clear register. The configuration is detected by
> > which resources are present.
>
> The last sentence of this paragraph repeats for first one.
Ok
> >
> > For setting the GPIO direction, there are three configurations:
> > a. simple bidirectional GPIOs 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.
> >
> > The first user for this binding is "wd,mbl-gpio".
> >
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> > static void bgpio_write8(void __iomem *reg, unsigned long data)
> > {
> > @@ -569,6 +571,58 @@ static void __iomem *bgpio_map(struct
> > platform_device *pdev,
> > return devm_ioremap_resource(&pdev->dev, r);
> > }
> >
> > +#ifdef CONFIG_OF
> > +static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> > + struct bgpio_pdata *pdata,
> > + unsigned long *flags)
> > +{
> > + struct device *dev = &pdev->dev;
> > +
> > + pdata->base = -1;
> > +
> > + if (of_property_read_bool(dev->of_node, "no-output"))
> > + *flags |= BGPIOF_NO_OUTPUT;
>
> I don't think it is a good idea to add "generic" properties. Whether a
> controller is capable of output or not should be determined by its
> compatible string only, and not a vague property.
Well, meet the gpios on the MBL. If you want to figure out how WD wired
up these two GPIOs (one is input, the other output), I can sent you a
built-yourself MBL kit. It just needs a 3,5" drive and a 12V 2A power
plug with a standard 5.5mm plug.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id bgpio_of_match[] = {
> > + { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
>
> Mmm cannot you determine whether your controller is capable of output or
> not just from the compatible property here? If so, the
> bgpio_basic_mmio_parse_dt seems to be unneeded. If not, then this is
> dependent on the wd,mbl-gpio binding and should be renamed accordingly.
Sadly I don't know of any method. The device has two GPIOs one at 0x4e0000000.
The other one is at 0x4e0100000. The address tells me that there are two
external chips connected to the EBC (memory bank - RAM, ROM and DMA chips
go here according to IBM's documentations). Which is not the place you
would expect peripherals.
> > @@ -646,6 +709,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
> > static struct platform_driver bgpio_driver = {
> > .driver = {
> > .name = "basic-mmio-gpio",
> > + .of_match_table = of_match_ptr(bgpio_of_match),
> > },
> > .id_table = bgpio_id_table,
> > .probe = bgpio_pdev_probe,
>
> It seems to me that this patch does two things:
>
> 1) Add code to support device tree lookup
> 2) Add support for "wd,mbl-gpio".
>
> If true, these two things should be in their own patches. You should also
> have another patch that adds the DT bindings for "wd,mbl-gpio", so I would
> do things in that order:
The DT bindings have been merged. That's why I dropped it from the rebase.
> 1/3: DT support for basic-mmio-gpio
Sadly, adding the "basic-mmio-gpio" binding is not possible without a ACK from
the device tree maintainers. They have voiced their concerns. I think this was
your post of the discussion on it:
<http://www.spinics.net/lists/devicetree/msg124613.html>
That's why the series was updated around v5 and v6 to use the "wd,mbl-gpio"
binding.
So yes, I wanted to go this route in the beginning as well. But no go.
If we find more devices we could have a "basic-mmio-gpio" class. But
for now, we have to start somewhere.
> 2/3: DT bindings for "wd,mbl-gpio" (and have them validated by the DT
> people - e.g. do you really need a "reg" property or is it here just to fit
> with what bgpio_pdev_probe expects? More about this on 2/2)
> 3/3: Support for "wd,mbl-gpio" in basic-mmio-gpio
Yes, that would have been nice. And I agree it was the way to do it. But
without the wd,mbl-gpio mapping I would add the bgpio_parse_dt function
without any caller. (As I can't add the compatible = "basic-mmio-gpio", ...)
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/2] gpio: mmio: add DT support for memory-mapped GPIOs
@ 2016-05-12 12:07 ` Christian Lamparter
0 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-12 12:07 UTC (permalink / raw)
To: Alexandre Courbot
Cc: linux-gpio, linux-kernel, linux-arm-kernel,
Álvaro Fernández Rojas, Alexander Shiyan,
Linus Walleij, Andy Shevchenko, Joachim Eastwood
On Thursday, May 12, 2016 07:26:32 PM Alexandre Courbot wrote:
> On Wednesday, May 11, 2016 6:34:34 PM JST, Christian Lamparter wrote:
> > From: Álvaro Fernández Rojas <noltari@gmail.com>
> >
> > This patch adds support for defining memory-mapped GPIOs which
> > are compatible with the existing gpio-mmio interface. The generic
> > library provides support for many memory-mapped GPIO controllers
> > that are found in various on-board FPGA and ASIC solutions that
> > are used to control board's switches, LEDs, chip-selects,
> > Ethernet/USB PHY power, etc.
> >
> > For setting GPIO's there are three configurations:
>
> s/GPIO's/GPIOs
OK
> > 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").
> >
> > The configuration is detected by which resources are present.
> > For the single output register, this drives a 1 by setting a bit
> > and a zero by clearing a bit. For the set clr pair, this drives
> > a 1 by setting a bit in the set register and clears it by setting
> > a bit in the clear register. The configuration is detected by
> > which resources are present.
>
> The last sentence of this paragraph repeats for first one.
Ok
> >
> > For setting the GPIO direction, there are three configurations:
> > a. simple bidirectional GPIOs 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.
> >
> > The first user for this binding is "wd,mbl-gpio".
> >
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> > static void bgpio_write8(void __iomem *reg, unsigned long data)
> > {
> > @@ -569,6 +571,58 @@ static void __iomem *bgpio_map(struct
> > platform_device *pdev,
> > return devm_ioremap_resource(&pdev->dev, r);
> > }
> >
> > +#ifdef CONFIG_OF
> > +static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> > + struct bgpio_pdata *pdata,
> > + unsigned long *flags)
> > +{
> > + struct device *dev = &pdev->dev;
> > +
> > + pdata->base = -1;
> > +
> > + if (of_property_read_bool(dev->of_node, "no-output"))
> > + *flags |= BGPIOF_NO_OUTPUT;
>
> I don't think it is a good idea to add "generic" properties. Whether a
> controller is capable of output or not should be determined by its
> compatible string only, and not a vague property.
Well, meet the gpios on the MBL. If you want to figure out how WD wired
up these two GPIOs (one is input, the other output), I can sent you a
built-yourself MBL kit. It just needs a 3,5" drive and a 12V 2A power
plug with a standard 5.5mm plug.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id bgpio_of_match[] = {
> > + { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
>
> Mmm cannot you determine whether your controller is capable of output or
> not just from the compatible property here? If so, the
> bgpio_basic_mmio_parse_dt seems to be unneeded. If not, then this is
> dependent on the wd,mbl-gpio binding and should be renamed accordingly.
Sadly I don't know of any method. The device has two GPIOs one at 0x4e0000000.
The other one is at 0x4e0100000. The address tells me that there are two
external chips connected to the EBC (memory bank - RAM, ROM and DMA chips
go here according to IBM's documentations). Which is not the place you
would expect peripherals.
> > @@ -646,6 +709,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
> > static struct platform_driver bgpio_driver = {
> > .driver = {
> > .name = "basic-mmio-gpio",
> > + .of_match_table = of_match_ptr(bgpio_of_match),
> > },
> > .id_table = bgpio_id_table,
> > .probe = bgpio_pdev_probe,
>
> It seems to me that this patch does two things:
>
> 1) Add code to support device tree lookup
> 2) Add support for "wd,mbl-gpio".
>
> If true, these two things should be in their own patches. You should also
> have another patch that adds the DT bindings for "wd,mbl-gpio", so I would
> do things in that order:
The DT bindings have been merged. That's why I dropped it from the rebase.
> 1/3: DT support for basic-mmio-gpio
Sadly, adding the "basic-mmio-gpio" binding is not possible without a ACK from
the device tree maintainers. They have voiced their concerns. I think this was
your post of the discussion on it:
<http://www.spinics.net/lists/devicetree/msg124613.html>
That's why the series was updated around v5 and v6 to use the "wd,mbl-gpio"
binding.
So yes, I wanted to go this route in the beginning as well. But no go.
If we find more devices we could have a "basic-mmio-gpio" class. But
for now, we have to start somewhere.
> 2/3: DT bindings for "wd,mbl-gpio" (and have them validated by the DT
> people - e.g. do you really need a "reg" property or is it here just to fit
> with what bgpio_pdev_probe expects? More about this on 2/2)
> 3/3: Support for "wd,mbl-gpio" in basic-mmio-gpio
Yes, that would have been nice. And I agree it was the way to do it. But
without the wd,mbl-gpio mapping I would add the bgpio_parse_dt function
without any caller. (As I can't add the compatible = "basic-mmio-gpio", ...)
Regards,
Christian
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 1/2] gpio: mmio: add DT support for memory-mapped GPIOs
@ 2016-05-12 12:07 ` Christian Lamparter
0 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-12 12:07 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, May 12, 2016 07:26:32 PM Alexandre Courbot wrote:
> On Wednesday, May 11, 2016 6:34:34 PM JST, Christian Lamparter wrote:
> > From: ?lvaro Fern?ndez Rojas <noltari@gmail.com>
> >
> > This patch adds support for defining memory-mapped GPIOs which
> > are compatible with the existing gpio-mmio interface. The generic
> > library provides support for many memory-mapped GPIO controllers
> > that are found in various on-board FPGA and ASIC solutions that
> > are used to control board's switches, LEDs, chip-selects,
> > Ethernet/USB PHY power, etc.
> >
> > For setting GPIO's there are three configurations:
>
> s/GPIO's/GPIOs
OK
> > 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").
> >
> > The configuration is detected by which resources are present.
> > For the single output register, this drives a 1 by setting a bit
> > and a zero by clearing a bit. For the set clr pair, this drives
> > a 1 by setting a bit in the set register and clears it by setting
> > a bit in the clear register. The configuration is detected by
> > which resources are present.
>
> The last sentence of this paragraph repeats for first one.
Ok
> >
> > For setting the GPIO direction, there are three configurations:
> > a. simple bidirectional GPIOs 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.
> >
> > The first user for this binding is "wd,mbl-gpio".
> >
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: ?lvaro Fern?ndez Rojas <noltari@gmail.com>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> > static void bgpio_write8(void __iomem *reg, unsigned long data)
> > {
> > @@ -569,6 +571,58 @@ static void __iomem *bgpio_map(struct
> > platform_device *pdev,
> > return devm_ioremap_resource(&pdev->dev, r);
> > }
> >
> > +#ifdef CONFIG_OF
> > +static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> > + struct bgpio_pdata *pdata,
> > + unsigned long *flags)
> > +{
> > + struct device *dev = &pdev->dev;
> > +
> > + pdata->base = -1;
> > +
> > + if (of_property_read_bool(dev->of_node, "no-output"))
> > + *flags |= BGPIOF_NO_OUTPUT;
>
> I don't think it is a good idea to add "generic" properties. Whether a
> controller is capable of output or not should be determined by its
> compatible string only, and not a vague property.
Well, meet the gpios on the MBL. If you want to figure out how WD wired
up these two GPIOs (one is input, the other output), I can sent you a
built-yourself MBL kit. It just needs a 3,5" drive and a 12V 2A power
plug with a standard 5.5mm plug.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id bgpio_of_match[] = {
> > + { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
>
> Mmm cannot you determine whether your controller is capable of output or
> not just from the compatible property here? If so, the
> bgpio_basic_mmio_parse_dt seems to be unneeded. If not, then this is
> dependent on the wd,mbl-gpio binding and should be renamed accordingly.
Sadly I don't know of any method. The device has two GPIOs one at 0x4e0000000.
The other one is at 0x4e0100000. The address tells me that there are two
external chips connected to the EBC (memory bank - RAM, ROM and DMA chips
go here according to IBM's documentations). Which is not the place you
would expect peripherals.
> > @@ -646,6 +709,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
> > static struct platform_driver bgpio_driver = {
> > .driver = {
> > .name = "basic-mmio-gpio",
> > + .of_match_table = of_match_ptr(bgpio_of_match),
> > },
> > .id_table = bgpio_id_table,
> > .probe = bgpio_pdev_probe,
>
> It seems to me that this patch does two things:
>
> 1) Add code to support device tree lookup
> 2) Add support for "wd,mbl-gpio".
>
> If true, these two things should be in their own patches. You should also
> have another patch that adds the DT bindings for "wd,mbl-gpio", so I would
> do things in that order:
The DT bindings have been merged. That's why I dropped it from the rebase.
> 1/3: DT support for basic-mmio-gpio
Sadly, adding the "basic-mmio-gpio" binding is not possible without a ACK from
the device tree maintainers. They have voiced their concerns. I think this was
your post of the discussion on it:
<http://www.spinics.net/lists/devicetree/msg124613.html>
That's why the series was updated around v5 and v6 to use the "wd,mbl-gpio"
binding.
So yes, I wanted to go this route in the beginning as well. But no go.
If we find more devices we could have a "basic-mmio-gpio" class. But
for now, we have to start somewhere.
> 2/3: DT bindings for "wd,mbl-gpio" (and have them validated by the DT
> people - e.g. do you really need a "reg" property or is it here just to fit
> with what bgpio_pdev_probe expects? More about this on 2/2)
> 3/3: Support for "wd,mbl-gpio" in basic-mmio-gpio
Yes, that would have been nice. And I agree it was the way to do it. But
without the wd,mbl-gpio mapping I would add the bgpio_parse_dt function
without any caller. (As I can't add the compatible = "basic-mmio-gpio", ...)
Regards,
Christian
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
2016-05-12 10:30 ` Alexandre Courbot
@ 2016-05-12 13:07 ` Christian Lamparter
-1 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-12 13:07 UTC (permalink / raw)
To: Alexandre Courbot
Cc: linux-gpio, linux-kernel, linux-arm-kernel,
Álvaro Fernández Rojas, Alexander Shiyan,
Linus Walleij, Andy Shevchenko, Joachim Eastwood,
Julien Grossholtz, Martyn Welch, Jonas Jensen
On Thursday, May 12, 2016 07:30:05 PM Alexandre Courbot wrote:
> On Wednesday, May 11, 2016 6:34:35 PM JST, Christian Lamparter wrote:
> > This patch integrates the GPIO drivers for the following
> > boards, SoCs, etc. into gpio-mmio:
> > - CLPS711X SoCs
> > - MOXA ART SoC
> > - TS-4800 FPGA DIO blocks and compatibles
> > - GPIO controllers found on some GE Single Board Computers
> >
> > Cc: Alexander Shiyan <shc_work@mail.ru>
> > Cc: Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>
> > Cc: Martyn Welch <martyn.welch@ge.com>
> > Cc: Jonas Jensen <jonas.jensen@gmail.com>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
[...]
> > diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> > index f72e40e..1de9172 100644
> > --- a/drivers/gpio/gpio-mmio.c
> > +++ b/drivers/gpio/gpio-mmio.c
> > @@ -586,8 +586,220 @@ static int
> > bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> > return 0;
> > }
> >
> > +static int clps711x_parse_dt(struct platform_device *pdev,
> > + struct bgpio_pdata *pdata,
> > + unsigned long *flags)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct resource *res;
> > + const char *dir_reg_name;
> > + int id = np ? of_alias_get_id(np, "gpio") : pdev->id;
> > +
> > + switch (id) {
> > + case 0:
> > + case 1:
> > + case 2:
> > + pdata->ngpio = 0; /* determined by register width */
> > + dir_reg_name = "dirout";
> > + break;
> > + case 3:
> > + pdata->ngpio = 0; /* determined by register width */
> > + /* PORTD is inverted logic for direction register */
> > + dir_reg_name = "dirin";
> > + break;
> > + case 4:
> > + pdata->ngpio = 3; /* PORTE is 3 lines only */
> > + dir_reg_name = "dirout";
> > + break;
> > + default:
> > + return -ENODEV;
> > + }
> > +
> > + pdata->base = id * 8;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -EINVAL;
> > + if (!res->name || strcmp("dat", res->name))
> > + res->name = devm_kstrdup(&pdev->dev, "dat", GFP_KERNEL);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!res)
> > + return -EINVAL;
> > + if (!res->name || strcmp(dir_reg_name, res->name))
> > + res->name = devm_kstrdup(&pdev->dev, dir_reg_name, GFP_KERNEL);
>
> Ouch, I don't like this. Also the probability that the "if (!res->name ||
> ...)" condition is not met is so small that I would not bother with it and
> do the kstrdup unconditionally. But maybe we can remove this altogether -
> see below...
The !res->name check is simply there because the kernel's strcmp doesn't
check for NULL... the res->name will be coming from the device tree.
Yes it should always be initialized but again: it's not within the functions
control so it better be prepared for it. The "dir_reg_name" is part of the
function so we can guarantee that it's not NULL or a invalid string.
About the leakage: I use devm_kstrdup unlike the dump strdup, it does bind
the dupped string to the device. This way the resource is not leaked even
if the module is loaded and unloaded, it stays with the device. (Until
it is destroyed).
Note: The strcmp is there to check if resource name was already updated
so even if the module was loaded multiple times it will dup the string
just once.
Doing a direct assignment like I did in the earlier versions would lead
to issues since the device resources are part of the the device and not
with the driver module.
> > +/*
> > + * bgpio_init() needs up to five named mmio register resources.
> > + * These currently are dat, set, clr, [dirout | dirin]. There
> > + * is no particular order or predefined place for the entries.
> > + * However, dirout and dirin are mutually exclusive.
> > + */
> > +#define __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES 4
> > +
> > +struct compat_gpio_device_data {
> > + unsigned int expected_resource_size;
> > + unsigned int ngpio;
> > + resource_size_t register_width;
> > + unsigned long flags;
> > + int (*call_back)(struct platform_device *pdev,
> > + struct bgpio_pdata *pdata,
> > + unsigned long *flags);
> > + struct resource_replacement {
> > + resource_size_t start_offset;
> > + const char *name;
> > + } resources[__NUM_COMPAT_OVERRIDE_MMIO_RESOURCES];
> > +};
> > +#undef __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES
> > +
> > +#define ADD_COMPAT_REGISTER(_name, _offset) \
> > + { .name = (_name), .start_offset = (_offset) }
> > +
> > +#define ADD_COMPAT_GPIO(_comp, _sz, _ngpio, _width, _cb, _f, _res...) \
> > + { \
> > + .compatible = (_comp), \
> > + .data = &(struct compat_gpio_device_data) { \
> > + .expected_resource_size = (_sz), \
> > + .ngpio = (_ngpio), \
> > + .register_width = (_width), \
> > + .flags = (_f), \
> > + .call_back = (_cb), \
> > + .resources = { _res }, \
> > + } \
> > +}
> > +
> > +#define ADD_COMPAT_GE_GPIO(_name, _ngpio) \
> > + ADD_COMPAT_GPIO(_name, 0x24, _ngpio, 0x4, ge_dt_cb, \
> > + BGPIOF_BIG_ENDIAN_BYTE_ORDER, \
> > + ADD_COMPAT_REGISTER("dat", 0x04), \
> > + ADD_COMPAT_REGISTER("set", 0x08), \
> > + ADD_COMPAT_REGISTER("dirin", 0x00)) \
> > +
> > +static const struct of_device_id compat_gpio_devices[] = {
> > + ADD_COMPAT_GE_GPIO("ge,imp3a-gpio", 16),
> > + ADD_COMPAT_GE_GPIO("gef,sbc310-gpio", 6),
> > + ADD_COMPAT_GE_GPIO("gef,sbc610-gpio", 19),
> > + ADD_COMPAT_GPIO("moxa,moxart-gpio", 0xc, 0, 0x4, moxart_dt_cb,
> > + BGPIOF_READ_OUTPUT_REG_SET,
> > + ADD_COMPAT_REGISTER("dat", 0x04),
> > + ADD_COMPAT_REGISTER("set", 0x00),
> > + ADD_COMPAT_REGISTER("dirout", 0x08)),
> > + ADD_COMPAT_GPIO("technologic,ts4800-gpio", 0x6, 16, 0x2, ts4800_dt_cb,
> > + 0, ADD_COMPAT_REGISTER("dat", 0x00),
> > + ADD_COMPAT_REGISTER("set", 0x02),
> > + ADD_COMPAT_REGISTER("dirout", 0x04)),
> > +};
> > +
> > +#undef ADD_COMPAT_GE_GPIO
> > +#undef ADD_COMPAT_GPIO
> > +#undef ADD_COMPAT_REGISTER
> > +
> > +static int compat_parse_dt(struct platform_device *pdev,
> > + struct bgpio_pdata *pdata,
> > + unsigned long *flags)
> > +{
> > + const struct device_node *node = pdev->dev.of_node;
> > + const struct compat_gpio_device_data *entry;
> > + const struct of_device_id *of_id;
> > + struct resource *res;
> > + int err;
> > +
> > + of_id = of_match_node(compat_gpio_devices, node);
> > + if (!of_id)
> > + return -ENODEV;
> > +
> > + entry = of_id->data;
> > + if (!entry || !entry->resources[0].name)
> > + return -EINVAL;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -EINVAL;
> > +
> > + if (!res->name || strcmp(entry->resources[0].name, res->name)) {
> > + struct resource nres[ARRAY_SIZE(entry->resources)];
> > + size_t i;
> > +
> > + if (resource_size(res) != entry->expected_resource_size)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < ARRAY_SIZE(entry->resources); i++) {
> > + if (!entry->resources[i].name)
> > + continue;
> > +
> > + nres[i].name = devm_kstrdup(&pdev->dev,
> > + entry->resources[i].name, GFP_KERNEL);
> > + nres[i].start = res->start +
> > + entry->resources[i].start_offset;
> > + nres[i].end = nres[i].start +
> > + entry->register_width - 1;
> > + nres[i].flags = IORESOURCE_MEM;
> > + }
> > +
> > + err = platform_device_add_resources(pdev, nres, i);
> > + if (err)
> > + return err;
> > + }
> > +
> > + pdata->base = -1;
> > + pdata->ngpio = entry->ngpio;
> > + *flags = entry->flags;
> > +
> > + if (entry->call_back)
> > + err = entry->call_back(pdev, pdata, flags);
> > +
> > + return err;
> > +}
>
> Ok, so this is getting quite complex, with two of_device_id tables and two
> levels of hooks, mainly because you want to use the bgpio_pdev_probe()
> function which relies on named memory resources.
I think I need more here. The bgpio_pdev_probe() also sets up the bgpio_maps()
(there are 6 and I'm using 4 at the moment for all compat chips).
The bgpio_pdev_probe() also deals with devm_gpiochip_add_data() and allocates
the memory for the gpiochip structure... as well as and the error handling.
etc...
That's why I reused the existing code as much as possible.
(But let's get to the main issue here:)
> This function is here for basic platform devices, and you are not obliged
> to rely on it for DT. Feel free to write your own function (or rather split
> bgpio_pdev_probe() in two code paths) if it can reduce the amount of black
> magic you need to do (especially the renaming of resources on-the-fly).
>
> At the end of the day what you want is a function that calls bgpio_init()
> with the right parameters. These parameters should be inferred from the
> compatible property whenever possible (which is most of the time, and why I
> am suspiscious of using "reg" properties for these devices since I don't
> think it can change much?), and passed as-is to bgpio_init(). Your
> compat_gpio_device_data struct basically encodes this, so this will take
> care of all the "compat" devices.
>
> For the other devices, you should be able to fill in such a structure from
> the DT properties. Then you just use it for the parameters of bgpio_init().
> It will be more elegant that adding resources, and probably shorter as
> well.
>
> > +
> > static const struct of_device_id bgpio_of_match[] = {
> > { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
> > + { .compatible = "cirrus,clps711x-gpio", .data = clps711x_parse_dt },
> > + { .compatible = "ge,imp3a-gpio", .data = compat_parse_dt },
> > + { .compatible = "gef,sbc310-gpio", .data = compat_parse_dt },
> > + { .compatible = "gef,sbc610-gpio", .data = compat_parse_dt },
> > + { .compatible = "moxa,moxart-gpio", .data = compat_parse_dt },
> > + { .compatible = "technologic,ts4800-gpio", .data = compat_parse_dt },
>
> If you implement my idea, the function referred to by .data should return
> that structure I was talking about, and bgpio_init() can be called directly
> on it after that. The current path of bgpio_pdev_probe() should only be
> taken if bgpio_parse_dt() returned NULL.
>
> I believe this will reduce the number of lines in your patch some more, and
> it will also make the code easier to read.
>
> Also this should be split into several patches, like the previous one: the
> first one adds the required infrastructure to gpio-mmio, and the subsequent
> patches each move one device into gpio-mmio.
The main issue why this got so complicated was keeping compatibility with
the existing drivers.
If you look at "clps711x_parse_dt". You will see that it supports 5 GPIOs.
The thing that makes it really ugly is that it uses the dt alias as a way
to identify each port (A-E). The Port is then used to encode the number of
gpios the controller has and whenever it is dirin or dirout...
I argued that this information should be put in the dts and I made patch
for it. But updating the dts was not the way to go. as Rob explained
in <http://www.spinics.net/lists/devicetree/msg124538.html>.
This means that unless you can make a single structure that would encode
everything these compat drivers do, (look at the call_back functions of
the other drivers, each of the compat drivers also has a little bit of
extra code associated to setup stuff like a label or have an extra ngpios
property parser, etc...). I cannot see how you could make this any better
without breaking compatibility. This was the sole reason why there are
these individual parsers. (In RFC v4? I had that parser in their original
files - I don't know if this would have worked any better, but feedback
indicated that everything should be moved to the gpio-mmio.c).
So, what's your take on it?
Note: This patch was added to this series as a request of Rob to demonstrate
that having a device tree binding for the gpio-mmio.c driver is something we
want. It isn't necessary to apply it right away as it also hinges on the ACKs
of people who use these devices. And so far, nobody replied. We can add this
individual patch at a later time.
Regards,
Christian
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
@ 2016-05-12 13:07 ` Christian Lamparter
0 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-12 13:07 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, May 12, 2016 07:30:05 PM Alexandre Courbot wrote:
> On Wednesday, May 11, 2016 6:34:35 PM JST, Christian Lamparter wrote:
> > This patch integrates the GPIO drivers for the following
> > boards, SoCs, etc. into gpio-mmio:
> > - CLPS711X SoCs
> > - MOXA ART SoC
> > - TS-4800 FPGA DIO blocks and compatibles
> > - GPIO controllers found on some GE Single Board Computers
> >
> > Cc: Alexander Shiyan <shc_work@mail.ru>
> > Cc: Julien Grossholtz <julien.grossholtz@savoirfairelinux.com>
> > Cc: Martyn Welch <martyn.welch@ge.com>
> > Cc: Jonas Jensen <jonas.jensen@gmail.com>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
[...]
> > diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> > index f72e40e..1de9172 100644
> > --- a/drivers/gpio/gpio-mmio.c
> > +++ b/drivers/gpio/gpio-mmio.c
> > @@ -586,8 +586,220 @@ static int
> > bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
> > return 0;
> > }
> >
> > +static int clps711x_parse_dt(struct platform_device *pdev,
> > + struct bgpio_pdata *pdata,
> > + unsigned long *flags)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct resource *res;
> > + const char *dir_reg_name;
> > + int id = np ? of_alias_get_id(np, "gpio") : pdev->id;
> > +
> > + switch (id) {
> > + case 0:
> > + case 1:
> > + case 2:
> > + pdata->ngpio = 0; /* determined by register width */
> > + dir_reg_name = "dirout";
> > + break;
> > + case 3:
> > + pdata->ngpio = 0; /* determined by register width */
> > + /* PORTD is inverted logic for direction register */
> > + dir_reg_name = "dirin";
> > + break;
> > + case 4:
> > + pdata->ngpio = 3; /* PORTE is 3 lines only */
> > + dir_reg_name = "dirout";
> > + break;
> > + default:
> > + return -ENODEV;
> > + }
> > +
> > + pdata->base = id * 8;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -EINVAL;
> > + if (!res->name || strcmp("dat", res->name))
> > + res->name = devm_kstrdup(&pdev->dev, "dat", GFP_KERNEL);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!res)
> > + return -EINVAL;
> > + if (!res->name || strcmp(dir_reg_name, res->name))
> > + res->name = devm_kstrdup(&pdev->dev, dir_reg_name, GFP_KERNEL);
>
> Ouch, I don't like this. Also the probability that the "if (!res->name ||
> ...)" condition is not met is so small that I would not bother with it and
> do the kstrdup unconditionally. But maybe we can remove this altogether -
> see below...
The !res->name check is simply there because the kernel's strcmp doesn't
check for NULL... the res->name will be coming from the device tree.
Yes it should always be initialized but again: it's not within the functions
control so it better be prepared for it. The "dir_reg_name" is part of the
function so we can guarantee that it's not NULL or a invalid string.
About the leakage: I use devm_kstrdup unlike the dump strdup, it does bind
the dupped string to the device. This way the resource is not leaked even
if the module is loaded and unloaded, it stays with the device. (Until
it is destroyed).
Note: The strcmp is there to check if resource name was already updated
so even if the module was loaded multiple times it will dup the string
just once.
Doing a direct assignment like I did in the earlier versions would lead
to issues since the device resources are part of the the device and not
with the driver module.
> > +/*
> > + * bgpio_init() needs up to five named mmio register resources.
> > + * These currently are dat, set, clr, [dirout | dirin]. There
> > + * is no particular order or predefined place for the entries.
> > + * However, dirout and dirin are mutually exclusive.
> > + */
> > +#define __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES 4
> > +
> > +struct compat_gpio_device_data {
> > + unsigned int expected_resource_size;
> > + unsigned int ngpio;
> > + resource_size_t register_width;
> > + unsigned long flags;
> > + int (*call_back)(struct platform_device *pdev,
> > + struct bgpio_pdata *pdata,
> > + unsigned long *flags);
> > + struct resource_replacement {
> > + resource_size_t start_offset;
> > + const char *name;
> > + } resources[__NUM_COMPAT_OVERRIDE_MMIO_RESOURCES];
> > +};
> > +#undef __NUM_COMPAT_OVERRIDE_MMIO_RESOURCES
> > +
> > +#define ADD_COMPAT_REGISTER(_name, _offset) \
> > + { .name = (_name), .start_offset = (_offset) }
> > +
> > +#define ADD_COMPAT_GPIO(_comp, _sz, _ngpio, _width, _cb, _f, _res...) \
> > + { \
> > + .compatible = (_comp), \
> > + .data = &(struct compat_gpio_device_data) { \
> > + .expected_resource_size = (_sz), \
> > + .ngpio = (_ngpio), \
> > + .register_width = (_width), \
> > + .flags = (_f), \
> > + .call_back = (_cb), \
> > + .resources = { _res }, \
> > + } \
> > +}
> > +
> > +#define ADD_COMPAT_GE_GPIO(_name, _ngpio) \
> > + ADD_COMPAT_GPIO(_name, 0x24, _ngpio, 0x4, ge_dt_cb, \
> > + BGPIOF_BIG_ENDIAN_BYTE_ORDER, \
> > + ADD_COMPAT_REGISTER("dat", 0x04), \
> > + ADD_COMPAT_REGISTER("set", 0x08), \
> > + ADD_COMPAT_REGISTER("dirin", 0x00)) \
> > +
> > +static const struct of_device_id compat_gpio_devices[] = {
> > + ADD_COMPAT_GE_GPIO("ge,imp3a-gpio", 16),
> > + ADD_COMPAT_GE_GPIO("gef,sbc310-gpio", 6),
> > + ADD_COMPAT_GE_GPIO("gef,sbc610-gpio", 19),
> > + ADD_COMPAT_GPIO("moxa,moxart-gpio", 0xc, 0, 0x4, moxart_dt_cb,
> > + BGPIOF_READ_OUTPUT_REG_SET,
> > + ADD_COMPAT_REGISTER("dat", 0x04),
> > + ADD_COMPAT_REGISTER("set", 0x00),
> > + ADD_COMPAT_REGISTER("dirout", 0x08)),
> > + ADD_COMPAT_GPIO("technologic,ts4800-gpio", 0x6, 16, 0x2, ts4800_dt_cb,
> > + 0, ADD_COMPAT_REGISTER("dat", 0x00),
> > + ADD_COMPAT_REGISTER("set", 0x02),
> > + ADD_COMPAT_REGISTER("dirout", 0x04)),
> > +};
> > +
> > +#undef ADD_COMPAT_GE_GPIO
> > +#undef ADD_COMPAT_GPIO
> > +#undef ADD_COMPAT_REGISTER
> > +
> > +static int compat_parse_dt(struct platform_device *pdev,
> > + struct bgpio_pdata *pdata,
> > + unsigned long *flags)
> > +{
> > + const struct device_node *node = pdev->dev.of_node;
> > + const struct compat_gpio_device_data *entry;
> > + const struct of_device_id *of_id;
> > + struct resource *res;
> > + int err;
> > +
> > + of_id = of_match_node(compat_gpio_devices, node);
> > + if (!of_id)
> > + return -ENODEV;
> > +
> > + entry = of_id->data;
> > + if (!entry || !entry->resources[0].name)
> > + return -EINVAL;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -EINVAL;
> > +
> > + if (!res->name || strcmp(entry->resources[0].name, res->name)) {
> > + struct resource nres[ARRAY_SIZE(entry->resources)];
> > + size_t i;
> > +
> > + if (resource_size(res) != entry->expected_resource_size)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < ARRAY_SIZE(entry->resources); i++) {
> > + if (!entry->resources[i].name)
> > + continue;
> > +
> > + nres[i].name = devm_kstrdup(&pdev->dev,
> > + entry->resources[i].name, GFP_KERNEL);
> > + nres[i].start = res->start +
> > + entry->resources[i].start_offset;
> > + nres[i].end = nres[i].start +
> > + entry->register_width - 1;
> > + nres[i].flags = IORESOURCE_MEM;
> > + }
> > +
> > + err = platform_device_add_resources(pdev, nres, i);
> > + if (err)
> > + return err;
> > + }
> > +
> > + pdata->base = -1;
> > + pdata->ngpio = entry->ngpio;
> > + *flags = entry->flags;
> > +
> > + if (entry->call_back)
> > + err = entry->call_back(pdev, pdata, flags);
> > +
> > + return err;
> > +}
>
> Ok, so this is getting quite complex, with two of_device_id tables and two
> levels of hooks, mainly because you want to use the bgpio_pdev_probe()
> function which relies on named memory resources.
I think I need more here. The bgpio_pdev_probe() also sets up the bgpio_maps()
(there are 6 and I'm using 4 at the moment for all compat chips).
The bgpio_pdev_probe() also deals with devm_gpiochip_add_data() and allocates
the memory for the gpiochip structure... as well as and the error handling.
etc...
That's why I reused the existing code as much as possible.
(But let's get to the main issue here:)
> This function is here for basic platform devices, and you are not obliged
> to rely on it for DT. Feel free to write your own function (or rather split
> bgpio_pdev_probe() in two code paths) if it can reduce the amount of black
> magic you need to do (especially the renaming of resources on-the-fly).
>
> At the end of the day what you want is a function that calls bgpio_init()
> with the right parameters. These parameters should be inferred from the
> compatible property whenever possible (which is most of the time, and why I
> am suspiscious of using "reg" properties for these devices since I don't
> think it can change much?), and passed as-is to bgpio_init(). Your
> compat_gpio_device_data struct basically encodes this, so this will take
> care of all the "compat" devices.
>
> For the other devices, you should be able to fill in such a structure from
> the DT properties. Then you just use it for the parameters of bgpio_init().
> It will be more elegant that adding resources, and probably shorter as
> well.
>
> > +
> > static const struct of_device_id bgpio_of_match[] = {
> > { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
> > + { .compatible = "cirrus,clps711x-gpio", .data = clps711x_parse_dt },
> > + { .compatible = "ge,imp3a-gpio", .data = compat_parse_dt },
> > + { .compatible = "gef,sbc310-gpio", .data = compat_parse_dt },
> > + { .compatible = "gef,sbc610-gpio", .data = compat_parse_dt },
> > + { .compatible = "moxa,moxart-gpio", .data = compat_parse_dt },
> > + { .compatible = "technologic,ts4800-gpio", .data = compat_parse_dt },
>
> If you implement my idea, the function referred to by .data should return
> that structure I was talking about, and bgpio_init() can be called directly
> on it after that. The current path of bgpio_pdev_probe() should only be
> taken if bgpio_parse_dt() returned NULL.
>
> I believe this will reduce the number of lines in your patch some more, and
> it will also make the code easier to read.
>
> Also this should be split into several patches, like the previous one: the
> first one adds the required infrastructure to gpio-mmio, and the subsequent
> patches each move one device into gpio-mmio.
The main issue why this got so complicated was keeping compatibility with
the existing drivers.
If you look at "clps711x_parse_dt". You will see that it supports 5 GPIOs.
The thing that makes it really ugly is that it uses the dt alias as a way
to identify each port (A-E). The Port is then used to encode the number of
gpios the controller has and whenever it is dirin or dirout...
I argued that this information should be put in the dts and I made patch
for it. But updating the dts was not the way to go. as Rob explained
in <http://www.spinics.net/lists/devicetree/msg124538.html>.
This means that unless you can make a single structure that would encode
everything these compat drivers do, (look at the call_back functions of
the other drivers, each of the compat drivers also has a little bit of
extra code associated to setup stuff like a label or have an extra ngpios
property parser, etc...). I cannot see how you could make this any better
without breaking compatibility. This was the sole reason why there are
these individual parsers. (In RFC v4? I had that parser in their original
files - I don't know if this would have worked any better, but feedback
indicated that everything should be moved to the gpio-mmio.c).
So, what's your take on it?
Note: This patch was added to this series as a request of Rob to demonstrate
that having a device tree binding for the gpio-mmio.c driver is something we
want. It isn't necessary to apply it right away as it also hinges on the ACKs
of people who use these devices. And so far, nobody replied. We can add this
individual patch at a later time.
Regards,
Christian
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9.1] gpio: mmio: add DT support for memory-mapped GPIOs
2016-05-12 10:26 ` Alexandre Courbot
(?)
@ 2016-05-12 19:28 ` Christian Lamparter
-1 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-12 19:28 UTC (permalink / raw)
To: linux-gpio, linux-kernel, linux-arm-kernel
Cc: Álvaro Fernández Rojas, Alexander Shiyan,
Alexandre Courbot, Linus Walleij, Andy Shevchenko,
Joachim Eastwood, Christian Lamparter
From: Álvaro Fernández Rojas <noltari@gmail.com>
This patch adds support for defining memory-mapped GPIOs which
are compatible with the existing gpio-mmio interface. The generic
library provides support for many memory-mapped GPIO controllers
that are found in various on-board FPGA and ASIC solutions that
are used to control board's switches, LEDs, chip-selects,
Ethernet/USB PHY power, etc.
For setting GPIOs 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").
The configuration is detected by which resources are present.
For the single output register, this drives a 1 by setting a bit
and a zero by clearing a bit. For the set clr pair, this drives
a 1 by setting a bit in the set register and clears it by setting
a bit in the clear register.
For setting the GPIO direction, there are three configurations:
a. simple bidirectional GPIOs 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.
The first user for this binding is "wd,mbl-gpio".
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
drivers/gpio/gpio-mmio.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 66 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 6c1cb3b..f72e40e 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.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,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
return devm_ioremap_resource(&pdev->dev, r);
}
+#ifdef CONFIG_OF
+static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ struct device *dev = &pdev->dev;
+
+ pdata->base = -1;
+
+ if (of_property_read_bool(dev->of_node, "no-output"))
+ *flags |= BGPIOF_NO_OUTPUT;
+
+ return 0;
+}
+
+static const struct of_device_id bgpio_of_match[] = {
+ { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bgpio_of_match);
+
+static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
+ unsigned long *flags)
+{
+ const int (*parse_dt)(struct platform_device *,
+ struct bgpio_pdata *, unsigned long *);
+ struct bgpio_pdata *pdata;
+ int err;
+
+ parse_dt = of_device_get_match_data(&pdev->dev);
+ if (!parse_dt)
+ return NULL;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
+ GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+
+ err = parse_dt(pdev, pdata, flags);
+ if (err)
+ return ERR_PTR(err);
+
+ return pdata;
+}
+#else
+static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
+ unsigned long *flags)
+{
+ return NULL;
+}
+#endif /* CONFIG_OF */
+
static int bgpio_pdev_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -579,10 +633,19 @@ 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(pdev, &flags);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
+
+ 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,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
static struct platform_driver bgpio_driver = {
.driver = {
.name = "basic-mmio-gpio",
+ .of_match_table = of_match_ptr(bgpio_of_match),
},
.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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v9.1] gpio: mmio: add DT support for memory-mapped GPIOs
@ 2016-05-12 19:28 ` Christian Lamparter
0 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-12 19:28 UTC (permalink / raw)
To: linux-gpio, linux-kernel, linux-arm-kernel
Cc: Álvaro Fernández Rojas, Alexander Shiyan,
Alexandre Courbot, Linus Walleij, Andy Shevchenko,
Joachim Eastwood, Christian Lamparter
From: Álvaro Fernández Rojas <noltari@gmail.com>
This patch adds support for defining memory-mapped GPIOs which
are compatible with the existing gpio-mmio interface. The generic
library provides support for many memory-mapped GPIO controllers
that are found in various on-board FPGA and ASIC solutions that
are used to control board's switches, LEDs, chip-selects,
Ethernet/USB PHY power, etc.
For setting GPIOs 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").
The configuration is detected by which resources are present.
For the single output register, this drives a 1 by setting a bit
and a zero by clearing a bit. For the set clr pair, this drives
a 1 by setting a bit in the set register and clears it by setting
a bit in the clear register.
For setting the GPIO direction, there are three configurations:
a. simple bidirectional GPIOs 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.
The first user for this binding is "wd,mbl-gpio".
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
drivers/gpio/gpio-mmio.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 66 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 6c1cb3b..f72e40e 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.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,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
return devm_ioremap_resource(&pdev->dev, r);
}
+#ifdef CONFIG_OF
+static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ struct device *dev = &pdev->dev;
+
+ pdata->base = -1;
+
+ if (of_property_read_bool(dev->of_node, "no-output"))
+ *flags |= BGPIOF_NO_OUTPUT;
+
+ return 0;
+}
+
+static const struct of_device_id bgpio_of_match[] = {
+ { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bgpio_of_match);
+
+static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
+ unsigned long *flags)
+{
+ const int (*parse_dt)(struct platform_device *,
+ struct bgpio_pdata *, unsigned long *);
+ struct bgpio_pdata *pdata;
+ int err;
+
+ parse_dt = of_device_get_match_data(&pdev->dev);
+ if (!parse_dt)
+ return NULL;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
+ GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+
+ err = parse_dt(pdev, pdata, flags);
+ if (err)
+ return ERR_PTR(err);
+
+ return pdata;
+}
+#else
+static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
+ unsigned long *flags)
+{
+ return NULL;
+}
+#endif /* CONFIG_OF */
+
static int bgpio_pdev_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -579,10 +633,19 @@ 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(pdev, &flags);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
+
+ 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,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
static struct platform_driver bgpio_driver = {
.driver = {
.name = "basic-mmio-gpio",
+ .of_match_table = of_match_ptr(bgpio_of_match),
},
.id_table = bgpio_id_table,
.probe = bgpio_pdev_probe,
--
2.8.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v9.1] gpio: mmio: add DT support for memory-mapped GPIOs
@ 2016-05-12 19:28 ` Christian Lamparter
0 siblings, 0 replies; 26+ messages in thread
From: Christian Lamparter @ 2016-05-12 19:28 UTC (permalink / raw)
To: linux-arm-kernel
From: ?lvaro Fern?ndez Rojas <noltari@gmail.com>
This patch adds support for defining memory-mapped GPIOs which
are compatible with the existing gpio-mmio interface. The generic
library provides support for many memory-mapped GPIO controllers
that are found in various on-board FPGA and ASIC solutions that
are used to control board's switches, LEDs, chip-selects,
Ethernet/USB PHY power, etc.
For setting GPIOs 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").
The configuration is detected by which resources are present.
For the single output register, this drives a 1 by setting a bit
and a zero by clearing a bit. For the set clr pair, this drives
a 1 by setting a bit in the set register and clears it by setting
a bit in the clear register.
For setting the GPIO direction, there are three configurations:
a. simple bidirectional GPIOs 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.
The first user for this binding is "wd,mbl-gpio".
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: ?lvaro Fern?ndez Rojas <noltari@gmail.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
drivers/gpio/gpio-mmio.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 66 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 6c1cb3b..f72e40e 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.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,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
return devm_ioremap_resource(&pdev->dev, r);
}
+#ifdef CONFIG_OF
+static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
+ struct bgpio_pdata *pdata,
+ unsigned long *flags)
+{
+ struct device *dev = &pdev->dev;
+
+ pdata->base = -1;
+
+ if (of_property_read_bool(dev->of_node, "no-output"))
+ *flags |= BGPIOF_NO_OUTPUT;
+
+ return 0;
+}
+
+static const struct of_device_id bgpio_of_match[] = {
+ { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bgpio_of_match);
+
+static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
+ unsigned long *flags)
+{
+ const int (*parse_dt)(struct platform_device *,
+ struct bgpio_pdata *, unsigned long *);
+ struct bgpio_pdata *pdata;
+ int err;
+
+ parse_dt = of_device_get_match_data(&pdev->dev);
+ if (!parse_dt)
+ return NULL;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
+ GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+
+ err = parse_dt(pdev, pdata, flags);
+ if (err)
+ return ERR_PTR(err);
+
+ return pdata;
+}
+#else
+static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
+ unsigned long *flags)
+{
+ return NULL;
+}
+#endif /* CONFIG_OF */
+
static int bgpio_pdev_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -579,10 +633,19 @@ 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(pdev, &flags);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
+
+ 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,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
static struct platform_driver bgpio_driver = {
.driver = {
.name = "basic-mmio-gpio",
+ .of_match_table = of_match_ptr(bgpio_of_match),
},
.id_table = bgpio_id_table,
.probe = bgpio_pdev_probe,
--
2.8.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/2] gpio: mmio: add DT support for memory-mapped GPIOs
2016-05-12 12:07 ` Christian Lamparter
(?)
@ 2016-05-13 10:59 ` Alexandre Courbot
-1 siblings, 0 replies; 26+ messages in thread
From: Alexandre Courbot @ 2016-05-13 10:59 UTC (permalink / raw)
To: Christian Lamparter
Cc: linux-gpio, Linux Kernel Mailing List, linux-arm-kernel,
Álvaro Fernández, Alexander Shiyan, Linus Walleij,
Andy Shevchenko, Joachim Eastwood
On Thu, May 12, 2016 at 9:07 PM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> On Thursday, May 12, 2016 07:26:32 PM Alexandre Courbot wrote:
>> On Wednesday, May 11, 2016 6:34:34 PM JST, Christian Lamparter wrote:
>> > From: Álvaro Fernández Rojas <noltari@gmail.com>
>> >
>> > This patch adds support for defining memory-mapped GPIOs which
>> > are compatible with the existing gpio-mmio interface. The generic
>> > library provides support for many memory-mapped GPIO controllers
>> > that are found in various on-board FPGA and ASIC solutions that
>> > are used to control board's switches, LEDs, chip-selects,
>> > Ethernet/USB PHY power, etc.
>> >
>> > For setting GPIO's there are three configurations:
>>
>> s/GPIO's/GPIOs
> OK
>
>> > 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").
>> >
>> > The configuration is detected by which resources are present.
>> > For the single output register, this drives a 1 by setting a bit
>> > and a zero by clearing a bit. For the set clr pair, this drives
>> > a 1 by setting a bit in the set register and clears it by setting
>> > a bit in the clear register. The configuration is detected by
>> > which resources are present.
>>
>> The last sentence of this paragraph repeats for first one.
> Ok
>
>> >
>> > For setting the GPIO direction, there are three configurations:
>> > a. simple bidirectional GPIOs 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.
>> >
>> > The first user for this binding is "wd,mbl-gpio".
>> >
>> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
>> > ---
>> > static void bgpio_write8(void __iomem *reg, unsigned long data)
>> > {
>> > @@ -569,6 +571,58 @@ static void __iomem *bgpio_map(struct
>> > platform_device *pdev,
>> > return devm_ioremap_resource(&pdev->dev, r);
>> > }
>> >
>> > +#ifdef CONFIG_OF
>> > +static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
>> > + struct bgpio_pdata *pdata,
>> > + unsigned long *flags)
>> > +{
>> > + struct device *dev = &pdev->dev;
>> > +
>> > + pdata->base = -1;
>> > +
>> > + if (of_property_read_bool(dev->of_node, "no-output"))
>> > + *flags |= BGPIOF_NO_OUTPUT;
>>
>> I don't think it is a good idea to add "generic" properties. Whether a
>> controller is capable of output or not should be determined by its
>> compatible string only, and not a vague property.
>
> Well, meet the gpios on the MBL. If you want to figure out how WD wired
> up these two GPIOs (one is input, the other output), I can sent you a
> built-yourself MBL kit. It just needs a 3,5" drive and a 12V 2A power
> plug with a standard 5.5mm plug.
>
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static const struct of_device_id bgpio_of_match[] = {
>> > + { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
>>
>> Mmm cannot you determine whether your controller is capable of output or
>> not just from the compatible property here? If so, the
>> bgpio_basic_mmio_parse_dt seems to be unneeded. If not, then this is
>> dependent on the wd,mbl-gpio binding and should be renamed accordingly.
> Sadly I don't know of any method. The device has two GPIOs one at 0x4e0000000.
> The other one is at 0x4e0100000. The address tells me that there are two
> external chips connected to the EBC (memory bank - RAM, ROM and DMA chips
> go here according to IBM's documentations). Which is not the place you
> would expect peripherals.
Ok, if you have no way of figuring that out then this is legit.
>
>> > @@ -646,6 +709,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
>> > static struct platform_driver bgpio_driver = {
>> > .driver = {
>> > .name = "basic-mmio-gpio",
>> > + .of_match_table = of_match_ptr(bgpio_of_match),
>> > },
>> > .id_table = bgpio_id_table,
>> > .probe = bgpio_pdev_probe,
>>
>> It seems to me that this patch does two things:
>>
>> 1) Add code to support device tree lookup
>> 2) Add support for "wd,mbl-gpio".
>>
>> If true, these two things should be in their own patches. You should also
>> have another patch that adds the DT bindings for "wd,mbl-gpio", so I would
>> do things in that order:
>
> The DT bindings have been merged. That's why I dropped it from the rebase.
>
>> 1/3: DT support for basic-mmio-gpio
> Sadly, adding the "basic-mmio-gpio" binding is not possible without a ACK from
> the device tree maintainers. They have voiced their concerns. I think this was
> your post of the discussion on it:
> <http://www.spinics.net/lists/devicetree/msg124613.html>
>
> That's why the series was updated around v5 and v6 to use the "wd,mbl-gpio"
> binding.
>
> So yes, I wanted to go this route in the beginning as well. But no go.
> If we find more devices we could have a "basic-mmio-gpio" class. But
> for now, we have to start somewhere.
Ah, I was not suggesting that you add a "basic-mmio-gpio" compatible
property, but that the first patch of the series should add the
infrastructure code required for "wd,mbl-gpio" and the drivers you are
moving to use it in 2/2. That way you separate the addition of support
code to the actual device support.
>> 2/3: DT bindings for "wd,mbl-gpio" (and have them validated by the DT
>> people - e.g. do you really need a "reg" property or is it here just to fit
>> with what bgpio_pdev_probe expects? More about this on 2/2)
>> 3/3: Support for "wd,mbl-gpio" in basic-mmio-gpio
> Yes, that would have been nice. And I agree it was the way to do it. But
> without the wd,mbl-gpio mapping I would add the bgpio_parse_dt function
> without any caller. (As I can't add the compatible = "basic-mmio-gpio", ...)
bgpio_parse_dt() is called from bgpio_pdev_probe(), so whether you
have the mapping or not should not matter, does it?
It's just that there is no reason to add support for WD-MBL in this
patch, and convert other drivers in the next - both are equal users of
the infrastructure code you are adding.
Note that my comments on the next patch suggest deeper changes to this
one - you may want to read them as well (I will ignore v9.1 for now).
--
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/2] gpio: mmio: add DT support for memory-mapped GPIOs
@ 2016-05-13 10:59 ` Alexandre Courbot
0 siblings, 0 replies; 26+ messages in thread
From: Alexandre Courbot @ 2016-05-13 10:59 UTC (permalink / raw)
To: Christian Lamparter
Cc: linux-gpio, Linux Kernel Mailing List, linux-arm-kernel,
Álvaro Fernández, Alexander Shiyan, Linus Walleij,
Andy Shevchenko, Joachim Eastwood
On Thu, May 12, 2016 at 9:07 PM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> On Thursday, May 12, 2016 07:26:32 PM Alexandre Courbot wrote:
>> On Wednesday, May 11, 2016 6:34:34 PM JST, Christian Lamparter wrote:
>> > From: Álvaro Fernández Rojas <noltari@gmail.com>
>> >
>> > This patch adds support for defining memory-mapped GPIOs which
>> > are compatible with the existing gpio-mmio interface. The generic
>> > library provides support for many memory-mapped GPIO controllers
>> > that are found in various on-board FPGA and ASIC solutions that
>> > are used to control board's switches, LEDs, chip-selects,
>> > Ethernet/USB PHY power, etc.
>> >
>> > For setting GPIO's there are three configurations:
>>
>> s/GPIO's/GPIOs
> OK
>
>> > 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").
>> >
>> > The configuration is detected by which resources are present.
>> > For the single output register, this drives a 1 by setting a bit
>> > and a zero by clearing a bit. For the set clr pair, this drives
>> > a 1 by setting a bit in the set register and clears it by setting
>> > a bit in the clear register. The configuration is detected by
>> > which resources are present.
>>
>> The last sentence of this paragraph repeats for first one.
> Ok
>
>> >
>> > For setting the GPIO direction, there are three configurations:
>> > a. simple bidirectional GPIOs 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.
>> >
>> > The first user for this binding is "wd,mbl-gpio".
>> >
>> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
>> > ---
>> > static void bgpio_write8(void __iomem *reg, unsigned long data)
>> > {
>> > @@ -569,6 +571,58 @@ static void __iomem *bgpio_map(struct
>> > platform_device *pdev,
>> > return devm_ioremap_resource(&pdev->dev, r);
>> > }
>> >
>> > +#ifdef CONFIG_OF
>> > +static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
>> > + struct bgpio_pdata *pdata,
>> > + unsigned long *flags)
>> > +{
>> > + struct device *dev = &pdev->dev;
>> > +
>> > + pdata->base = -1;
>> > +
>> > + if (of_property_read_bool(dev->of_node, "no-output"))
>> > + *flags |= BGPIOF_NO_OUTPUT;
>>
>> I don't think it is a good idea to add "generic" properties. Whether a
>> controller is capable of output or not should be determined by its
>> compatible string only, and not a vague property.
>
> Well, meet the gpios on the MBL. If you want to figure out how WD wired
> up these two GPIOs (one is input, the other output), I can sent you a
> built-yourself MBL kit. It just needs a 3,5" drive and a 12V 2A power
> plug with a standard 5.5mm plug.
>
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static const struct of_device_id bgpio_of_match[] = {
>> > + { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
>>
>> Mmm cannot you determine whether your controller is capable of output or
>> not just from the compatible property here? If so, the
>> bgpio_basic_mmio_parse_dt seems to be unneeded. If not, then this is
>> dependent on the wd,mbl-gpio binding and should be renamed accordingly.
> Sadly I don't know of any method. The device has two GPIOs one at 0x4e0000000.
> The other one is at 0x4e0100000. The address tells me that there are two
> external chips connected to the EBC (memory bank - RAM, ROM and DMA chips
> go here according to IBM's documentations). Which is not the place you
> would expect peripherals.
Ok, if you have no way of figuring that out then this is legit.
>
>> > @@ -646,6 +709,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
>> > static struct platform_driver bgpio_driver = {
>> > .driver = {
>> > .name = "basic-mmio-gpio",
>> > + .of_match_table = of_match_ptr(bgpio_of_match),
>> > },
>> > .id_table = bgpio_id_table,
>> > .probe = bgpio_pdev_probe,
>>
>> It seems to me that this patch does two things:
>>
>> 1) Add code to support device tree lookup
>> 2) Add support for "wd,mbl-gpio".
>>
>> If true, these two things should be in their own patches. You should also
>> have another patch that adds the DT bindings for "wd,mbl-gpio", so I would
>> do things in that order:
>
> The DT bindings have been merged. That's why I dropped it from the rebase.
>
>> 1/3: DT support for basic-mmio-gpio
> Sadly, adding the "basic-mmio-gpio" binding is not possible without a ACK from
> the device tree maintainers. They have voiced their concerns. I think this was
> your post of the discussion on it:
> <http://www.spinics.net/lists/devicetree/msg124613.html>
>
> That's why the series was updated around v5 and v6 to use the "wd,mbl-gpio"
> binding.
>
> So yes, I wanted to go this route in the beginning as well. But no go.
> If we find more devices we could have a "basic-mmio-gpio" class. But
> for now, we have to start somewhere.
Ah, I was not suggesting that you add a "basic-mmio-gpio" compatible
property, but that the first patch of the series should add the
infrastructure code required for "wd,mbl-gpio" and the drivers you are
moving to use it in 2/2. That way you separate the addition of support
code to the actual device support.
>> 2/3: DT bindings for "wd,mbl-gpio" (and have them validated by the DT
>> people - e.g. do you really need a "reg" property or is it here just to fit
>> with what bgpio_pdev_probe expects? More about this on 2/2)
>> 3/3: Support for "wd,mbl-gpio" in basic-mmio-gpio
> Yes, that would have been nice. And I agree it was the way to do it. But
> without the wd,mbl-gpio mapping I would add the bgpio_parse_dt function
> without any caller. (As I can't add the compatible = "basic-mmio-gpio", ...)
bgpio_parse_dt() is called from bgpio_pdev_probe(), so whether you
have the mapping or not should not matter, does it?
It's just that there is no reason to add support for WD-MBL in this
patch, and convert other drivers in the next - both are equal users of
the infrastructure code you are adding.
Note that my comments on the next patch suggest deeper changes to this
one - you may want to read them as well (I will ignore v9.1 for now).
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 1/2] gpio: mmio: add DT support for memory-mapped GPIOs
@ 2016-05-13 10:59 ` Alexandre Courbot
0 siblings, 0 replies; 26+ messages in thread
From: Alexandre Courbot @ 2016-05-13 10:59 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 12, 2016 at 9:07 PM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> On Thursday, May 12, 2016 07:26:32 PM Alexandre Courbot wrote:
>> On Wednesday, May 11, 2016 6:34:34 PM JST, Christian Lamparter wrote:
>> > From: ?lvaro Fern?ndez Rojas <noltari@gmail.com>
>> >
>> > This patch adds support for defining memory-mapped GPIOs which
>> > are compatible with the existing gpio-mmio interface. The generic
>> > library provides support for many memory-mapped GPIO controllers
>> > that are found in various on-board FPGA and ASIC solutions that
>> > are used to control board's switches, LEDs, chip-selects,
>> > Ethernet/USB PHY power, etc.
>> >
>> > For setting GPIO's there are three configurations:
>>
>> s/GPIO's/GPIOs
> OK
>
>> > 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").
>> >
>> > The configuration is detected by which resources are present.
>> > For the single output register, this drives a 1 by setting a bit
>> > and a zero by clearing a bit. For the set clr pair, this drives
>> > a 1 by setting a bit in the set register and clears it by setting
>> > a bit in the clear register. The configuration is detected by
>> > which resources are present.
>>
>> The last sentence of this paragraph repeats for first one.
> Ok
>
>> >
>> > For setting the GPIO direction, there are three configurations:
>> > a. simple bidirectional GPIOs 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.
>> >
>> > The first user for this binding is "wd,mbl-gpio".
>> >
>> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> > Signed-off-by: ?lvaro Fern?ndez Rojas <noltari@gmail.com>
>> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
>> > ---
>> > static void bgpio_write8(void __iomem *reg, unsigned long data)
>> > {
>> > @@ -569,6 +571,58 @@ static void __iomem *bgpio_map(struct
>> > platform_device *pdev,
>> > return devm_ioremap_resource(&pdev->dev, r);
>> > }
>> >
>> > +#ifdef CONFIG_OF
>> > +static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev,
>> > + struct bgpio_pdata *pdata,
>> > + unsigned long *flags)
>> > +{
>> > + struct device *dev = &pdev->dev;
>> > +
>> > + pdata->base = -1;
>> > +
>> > + if (of_property_read_bool(dev->of_node, "no-output"))
>> > + *flags |= BGPIOF_NO_OUTPUT;
>>
>> I don't think it is a good idea to add "generic" properties. Whether a
>> controller is capable of output or not should be determined by its
>> compatible string only, and not a vague property.
>
> Well, meet the gpios on the MBL. If you want to figure out how WD wired
> up these two GPIOs (one is input, the other output), I can sent you a
> built-yourself MBL kit. It just needs a 3,5" drive and a 12V 2A power
> plug with a standard 5.5mm plug.
>
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static const struct of_device_id bgpio_of_match[] = {
>> > + { .compatible = "wd,mbl-gpio", .data = bgpio_basic_mmio_parse_dt },
>>
>> Mmm cannot you determine whether your controller is capable of output or
>> not just from the compatible property here? If so, the
>> bgpio_basic_mmio_parse_dt seems to be unneeded. If not, then this is
>> dependent on the wd,mbl-gpio binding and should be renamed accordingly.
> Sadly I don't know of any method. The device has two GPIOs one at 0x4e0000000.
> The other one is at 0x4e0100000. The address tells me that there are two
> external chips connected to the EBC (memory bank - RAM, ROM and DMA chips
> go here according to IBM's documentations). Which is not the place you
> would expect peripherals.
Ok, if you have no way of figuring that out then this is legit.
>
>> > @@ -646,6 +709,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
>> > static struct platform_driver bgpio_driver = {
>> > .driver = {
>> > .name = "basic-mmio-gpio",
>> > + .of_match_table = of_match_ptr(bgpio_of_match),
>> > },
>> > .id_table = bgpio_id_table,
>> > .probe = bgpio_pdev_probe,
>>
>> It seems to me that this patch does two things:
>>
>> 1) Add code to support device tree lookup
>> 2) Add support for "wd,mbl-gpio".
>>
>> If true, these two things should be in their own patches. You should also
>> have another patch that adds the DT bindings for "wd,mbl-gpio", so I would
>> do things in that order:
>
> The DT bindings have been merged. That's why I dropped it from the rebase.
>
>> 1/3: DT support for basic-mmio-gpio
> Sadly, adding the "basic-mmio-gpio" binding is not possible without a ACK from
> the device tree maintainers. They have voiced their concerns. I think this was
> your post of the discussion on it:
> <http://www.spinics.net/lists/devicetree/msg124613.html>
>
> That's why the series was updated around v5 and v6 to use the "wd,mbl-gpio"
> binding.
>
> So yes, I wanted to go this route in the beginning as well. But no go.
> If we find more devices we could have a "basic-mmio-gpio" class. But
> for now, we have to start somewhere.
Ah, I was not suggesting that you add a "basic-mmio-gpio" compatible
property, but that the first patch of the series should add the
infrastructure code required for "wd,mbl-gpio" and the drivers you are
moving to use it in 2/2. That way you separate the addition of support
code to the actual device support.
>> 2/3: DT bindings for "wd,mbl-gpio" (and have them validated by the DT
>> people - e.g. do you really need a "reg" property or is it here just to fit
>> with what bgpio_pdev_probe expects? More about this on 2/2)
>> 3/3: Support for "wd,mbl-gpio" in basic-mmio-gpio
> Yes, that would have been nice. And I agree it was the way to do it. But
> without the wd,mbl-gpio mapping I would add the bgpio_parse_dt function
> without any caller. (As I can't add the compatible = "basic-mmio-gpio", ...)
bgpio_parse_dt() is called from bgpio_pdev_probe(), so whether you
have the mapping or not should not matter, does it?
It's just that there is no reason to add support for WD-MBL in this
patch, and convert other drivers in the next - both are equal users of
the infrastructure code you are adding.
Note that my comments on the next patch suggest deeper changes to this
one - you may want to read them as well (I will ignore v9.1 for now).
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
2016-05-12 13:07 ` Christian Lamparter
@ 2016-05-16 8:34 ` Alexandre Courbot
-1 siblings, 0 replies; 26+ messages in thread
From: Alexandre Courbot @ 2016-05-16 8:34 UTC (permalink / raw)
To: Christian Lamparter
Cc: linux-gpio, linux-kernel, linux-arm-kernel,
Álvaro Fernández, Alexander Shiyan, Linus Walleij,
Andy Shevchenko, Joachim Eastwood, Julien Grossholtz,
Martyn Welch, Jonas Jensen
On Thursday, May 12, 2016 10:07:06 PM JST, Christian Lamparter wrote:
> On Thursday, May 12, 2016 07:30:05 PM Alexandre Courbot wrote:
>> On Wednesday, May 11, 2016 6:34:35 PM JST, Christian Lamparter wrote: ...
> [...]
>>> ...
>> Ouch, I don't like this. Also the probability that the "if (!res->name ||
>> ...)" condition is not met is so small that I would not bother
>> with it and
>> do the kstrdup unconditionally. But maybe we can remove this altogether -
>> see below... ...
>
> The !res->name check is simply there because the kernel's strcmp doesn't
> check for NULL... the res->name will be coming from the device tree.
> Yes it should always be initialized but again: it's not within
> the functions
> control so it better be prepared for it. The "dir_reg_name" is part of the
> function so we can guarantee that it's not NULL or a invalid string.
>
> About the leakage: I use devm_kstrdup unlike the dump strdup, it does bind
> the dupped string to the device. This way the resource is not leaked even
> if the module is loaded and unloaded, it stays with the device. (Until
> it is destroyed).
What I wanted to point out is that it is probably ok to just duplicate the
string at all times, since the probability that res->name will not be NULL
*and* the string will already match what we expect is very low (and could
only come from a device tree that is aware of this patch).
Leakage is not a concern thanks to devm, as you have pointed out.
>
> Note: The strcmp is there to check if resource name was already updated
> so even if the module was loaded multiple times it will dup the string
> just once.
Mmm, is this how things really work? My understanding is that
devm-allocated resources will be freed after the driver's remove function
is called (see slides 11/12 of
http://haifux.org/lectures/323/haifux-devres.pdf). I.e. when the module is
unloaded, your string will be destroyed. So unless I am missing something
you would still need to duplicate it the next time the device is probed.
So here it seems to me that to be on the safe side you either need to
restore the previous value of res->name on device destruction, or always
duplicate the string when probing it. But with this later solution there is
still the risk that if you then load *another* driver to probe this device,
which will then find a valid pointer on res->name, only to find invalid
content when following it... So in the end I'd recommend against messing
with resources like that - it just creates potential for invalid states.
Hopefully is my following suggestion is valid, you won't need to.
>
> Doing a direct assignment like I did in the earlier versions would lead
> to issues since the device resources are part of the the device and not
> with the driver module.
>
>>> ...
>> Ok, so this is getting quite complex, with two of_device_id
>> tables and two
>> levels of hooks, mainly because you want to use the bgpio_pdev_probe()
>> function which relies on named memory resources. ...
> I think I need more here. The bgpio_pdev_probe() also sets up
> the bgpio_maps()
> (there are 6 and I'm using 4 at the moment for all compat chips).
> The bgpio_pdev_probe() also deals with devm_gpiochip_add_data()
> and allocates
> the memory for the gpiochip structure... as well as and the error handling.
> etc...
>
> That's why I reused the existing code as much as possible.
> (But let's get to the main issue here:)
>> This function is here for basic platform devices, and you are not obliged
>> to rely on it for DT. Feel free to write your own function (or
>> rather split
>> bgpio_pdev_probe() in two code paths) if it can reduce the
>> amount of black
>> magic you need to do (especially the renaming of resources on-the-fly).
>>
>> At the end of the day what you want is a function that calls
>> bgpio_init() ...
>
> The main issue why this got so complicated was keeping compatibility with
> the existing drivers.
>
> If you look at "clps711x_parse_dt". You will see that it supports 5 GPIOs.
> The thing that makes it really ugly is that it uses the dt alias as a way
> to identify each port (A-E). The Port is then used to encode the number of
> gpios the controller has and whenever it is dirin or dirout...
> I argued that this information should be put in the dts and I made patch
> for it. But updating the dts was not the way to go. as Rob explained
> in <http://www.spinics.net/lists/devicetree/msg124538.html>.
>
> This means that unless you can make a single structure that would encode
> everything these compat drivers do, (look at the call_back functions of
> the other drivers, each of the compat drivers also has a little bit of
> extra code associated to setup stuff like a label or have an extra ngpios
> property parser, etc...). I cannot see how you could make this any better
> without breaking compatibility. This was the sole reason why there are
> these individual parsers. (In RFC v4? I had that parser in their original
> files - I don't know if this would have worked any better, but feedback
> indicated that everything should be moved to the gpio-mmio.c).
>
> So, what's your take on it?
I think your problem can be summarized as follows: for each device, you
need to call bgpio_init() with the proper parameters. Your goal is to infer
the right parameters for a variety a devices that make a different use of
the device tree.
Because of this, you *will* need device-specific DT-parsing functions, and
your current patch already contains them (like clps711x_parse_dt()). My
suggestion is to not rely on bgpio_pdev_probe() for these specific devices,
as this function expects resources, and forces you to so some unclear
hacking. Instead, how about something like the following (warning, this is
dirty and just to let you get the idea):
/* Params to bgpio_init() */
struct bgpio_params {
unsigned long sz;
void __iomem *dat;
void __iomem *set;
void __iomem *clr;
void __iomem *dirout;
void __iomem *dirin;
unsigned long flags;
struct bgpio_pdata pdata;
};
static int clps711x_parse_dt(struct platform_device *pdev,
struct bgpio_params *params)
{
struct device_node *np = pdev->dev.of_node;
struct resource *res;
void __iomem *mem;
int id = np ? of_alias_get_id(np, "gpio") : pdev->id;
params->pdata.base = id * 8;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res)
return -EINVAL;
params->dat = devm_ioremap_resource(&pdev->dev, res);
res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
if (!res)
return -EINVAL;
mem = devm_ioremap_resource(&pdev->dev, res);
switch (id) {
case 0:
case 1:
case 2:
pdata->ngpio = 0; /* determined by register width */
params->dirout = mem;
break;
case 3:
pdata->ngpio = 0; /* determined by register width */
/* PORTD is inverted logic for direction register */
params->dirin = mem;
break;
case 4:
pdata->ngpio = 3; /* PORTE is 3 lines only */
params->dirout = mem;
break;
default:
return -ENODEV;
}
return 0;
}
static int bgpio_pdev_probe(struct platform_device *pdev)
{
/* clear params out */
struct bgpio_params params = { 0 };
const int (*parse_dt)(struct platform_device *,
struct bgpio_params *);
parse_dt = of_device_get_match_data(&pdev->dev);
/* We are not instanciated from the DT, do regular probe using
resources */
if (!parse_dt) {
struct resource *r;
r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
if (!r)
return -EINVAL;
params.sz = resource_size(r);
params.dat = bgpio_map(pdev, "dat", sz);
if (IS_ERR(params.dat))
return PTR_ERR(params.dat);
/* etc, fill in params by doing what bgpio_pdev_probe initially did
*/
} else {
/* instanciated from DT, fill in params with the device-specific
function */
parse_dt(pdev, ¶ms);
pdata = ¶ms.pdata;
}
....
err = bgpio_init(gc, dev, params.sz, params.dat, params.set,
params.clr, params.dirout, params.dirin, params.flags);
....
}
... or something like that, it is probably possible to make this cleaner by
refactoring the code a bit (bgpio_pdata and my new params structure can
probably be merged, for instance).
You get the same result, but without messing with resources in dangerous
ways, and also removing one level of callback (and some hard-to-follow
macros). Of course parse_dt functions can be shared across devices when
they are compatible. Feel also free to adopt another approach (like
returning the struct resource in bgpio_params instead of the mapped
resource so you can call a modified bgpio_map() on them in
bgpio_pdev_probe()) if you find this more elegant. The idea I want to
convey is that you don't need to rely on the named resources at all times.
Hope this clarifies!
Alex.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio
@ 2016-05-16 8:34 ` Alexandre Courbot
0 siblings, 0 replies; 26+ messages in thread
From: Alexandre Courbot @ 2016-05-16 8:34 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, May 12, 2016 10:07:06 PM JST, Christian Lamparter wrote:
> On Thursday, May 12, 2016 07:30:05 PM Alexandre Courbot wrote:
>> On Wednesday, May 11, 2016 6:34:35 PM JST, Christian Lamparter wrote: ...
> [...]
>>> ...
>> Ouch, I don't like this. Also the probability that the "if (!res->name ||
>> ...)" condition is not met is so small that I would not bother
>> with it and
>> do the kstrdup unconditionally. But maybe we can remove this altogether -
>> see below... ...
>
> The !res->name check is simply there because the kernel's strcmp doesn't
> check for NULL... the res->name will be coming from the device tree.
> Yes it should always be initialized but again: it's not within
> the functions
> control so it better be prepared for it. The "dir_reg_name" is part of the
> function so we can guarantee that it's not NULL or a invalid string.
>
> About the leakage: I use devm_kstrdup unlike the dump strdup, it does bind
> the dupped string to the device. This way the resource is not leaked even
> if the module is loaded and unloaded, it stays with the device. (Until
> it is destroyed).
What I wanted to point out is that it is probably ok to just duplicate the
string at all times, since the probability that res->name will not be NULL
*and* the string will already match what we expect is very low (and could
only come from a device tree that is aware of this patch).
Leakage is not a concern thanks to devm, as you have pointed out.
>
> Note: The strcmp is there to check if resource name was already updated
> so even if the module was loaded multiple times it will dup the string
> just once.
Mmm, is this how things really work? My understanding is that
devm-allocated resources will be freed after the driver's remove function
is called (see slides 11/12 of
http://haifux.org/lectures/323/haifux-devres.pdf). I.e. when the module is
unloaded, your string will be destroyed. So unless I am missing something
you would still need to duplicate it the next time the device is probed.
So here it seems to me that to be on the safe side you either need to
restore the previous value of res->name on device destruction, or always
duplicate the string when probing it. But with this later solution there is
still the risk that if you then load *another* driver to probe this device,
which will then find a valid pointer on res->name, only to find invalid
content when following it... So in the end I'd recommend against messing
with resources like that - it just creates potential for invalid states.
Hopefully is my following suggestion is valid, you won't need to.
>
> Doing a direct assignment like I did in the earlier versions would lead
> to issues since the device resources are part of the the device and not
> with the driver module.
>
>>> ...
>> Ok, so this is getting quite complex, with two of_device_id
>> tables and two
>> levels of hooks, mainly because you want to use the bgpio_pdev_probe()
>> function which relies on named memory resources. ...
> I think I need more here. The bgpio_pdev_probe() also sets up
> the bgpio_maps()
> (there are 6 and I'm using 4 at the moment for all compat chips).
> The bgpio_pdev_probe() also deals with devm_gpiochip_add_data()
> and allocates
> the memory for the gpiochip structure... as well as and the error handling.
> etc...
>
> That's why I reused the existing code as much as possible.
> (But let's get to the main issue here:)
>> This function is here for basic platform devices, and you are not obliged
>> to rely on it for DT. Feel free to write your own function (or
>> rather split
>> bgpio_pdev_probe() in two code paths) if it can reduce the
>> amount of black
>> magic you need to do (especially the renaming of resources on-the-fly).
>>
>> At the end of the day what you want is a function that calls
>> bgpio_init() ...
>
> The main issue why this got so complicated was keeping compatibility with
> the existing drivers.
>
> If you look at "clps711x_parse_dt". You will see that it supports 5 GPIOs.
> The thing that makes it really ugly is that it uses the dt alias as a way
> to identify each port (A-E). The Port is then used to encode the number of
> gpios the controller has and whenever it is dirin or dirout...
> I argued that this information should be put in the dts and I made patch
> for it. But updating the dts was not the way to go. as Rob explained
> in <http://www.spinics.net/lists/devicetree/msg124538.html>.
>
> This means that unless you can make a single structure that would encode
> everything these compat drivers do, (look at the call_back functions of
> the other drivers, each of the compat drivers also has a little bit of
> extra code associated to setup stuff like a label or have an extra ngpios
> property parser, etc...). I cannot see how you could make this any better
> without breaking compatibility. This was the sole reason why there are
> these individual parsers. (In RFC v4? I had that parser in their original
> files - I don't know if this would have worked any better, but feedback
> indicated that everything should be moved to the gpio-mmio.c).
>
> So, what's your take on it?
I think your problem can be summarized as follows: for each device, you
need to call bgpio_init() with the proper parameters. Your goal is to infer
the right parameters for a variety a devices that make a different use of
the device tree.
Because of this, you *will* need device-specific DT-parsing functions, and
your current patch already contains them (like clps711x_parse_dt()). My
suggestion is to not rely on bgpio_pdev_probe() for these specific devices,
as this function expects resources, and forces you to so some unclear
hacking. Instead, how about something like the following (warning, this is
dirty and just to let you get the idea):
/* Params to bgpio_init() */
struct bgpio_params {
unsigned long sz;
void __iomem *dat;
void __iomem *set;
void __iomem *clr;
void __iomem *dirout;
void __iomem *dirin;
unsigned long flags;
struct bgpio_pdata pdata;
};
static int clps711x_parse_dt(struct platform_device *pdev,
struct bgpio_params *params)
{
struct device_node *np = pdev->dev.of_node;
struct resource *res;
void __iomem *mem;
int id = np ? of_alias_get_id(np, "gpio") : pdev->id;
params->pdata.base = id * 8;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res)
return -EINVAL;
params->dat = devm_ioremap_resource(&pdev->dev, res);
res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
if (!res)
return -EINVAL;
mem = devm_ioremap_resource(&pdev->dev, res);
switch (id) {
case 0:
case 1:
case 2:
pdata->ngpio = 0; /* determined by register width */
params->dirout = mem;
break;
case 3:
pdata->ngpio = 0; /* determined by register width */
/* PORTD is inverted logic for direction register */
params->dirin = mem;
break;
case 4:
pdata->ngpio = 3; /* PORTE is 3 lines only */
params->dirout = mem;
break;
default:
return -ENODEV;
}
return 0;
}
static int bgpio_pdev_probe(struct platform_device *pdev)
{
/* clear params out */
struct bgpio_params params = { 0 };
const int (*parse_dt)(struct platform_device *,
struct bgpio_params *);
parse_dt = of_device_get_match_data(&pdev->dev);
/* We are not instanciated from the DT, do regular probe using
resources */
if (!parse_dt) {
struct resource *r;
r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
if (!r)
return -EINVAL;
params.sz = resource_size(r);
params.dat = bgpio_map(pdev, "dat", sz);
if (IS_ERR(params.dat))
return PTR_ERR(params.dat);
/* etc, fill in params by doing what bgpio_pdev_probe initially did
*/
} else {
/* instanciated from DT, fill in params with the device-specific
function */
parse_dt(pdev, ¶ms);
pdata = ¶ms.pdata;
}
....
err = bgpio_init(gc, dev, params.sz, params.dat, params.set,
params.clr, params.dirout, params.dirin, params.flags);
....
}
... or something like that, it is probably possible to make this cleaner by
refactoring the code a bit (bgpio_pdata and my new params structure can
probably be merged, for instance).
You get the same result, but without messing with resources in dangerous
ways, and also removing one level of callback (and some hard-to-follow
macros). Of course parse_dt functions can be shared across devices when
they are compatible. Feel also free to adopt another approach (like
returning the struct resource in bgpio_params instead of the mapped
resource so you can call a modified bgpio_map() on them in
bgpio_pdev_probe()) if you find this more elegant. The idea I want to
convey is that you don't need to rely on the named resources at all times.
Hope this clarifies!
Alex.
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2016-05-16 8:34 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-11 9:34 [PATCH v9 0/2] gpio: add DT support for memory-mapped GPIOs Christian Lamparter
2016-05-11 9:34 ` Christian Lamparter
2016-05-11 9:34 ` Christian Lamparter
2016-05-11 9:34 ` [PATCH v9 1/2] gpio: mmio: " Christian Lamparter
2016-05-11 9:34 ` Christian Lamparter
2016-05-12 10:26 ` Alexandre Courbot
2016-05-12 10:26 ` Alexandre Courbot
2016-05-12 12:07 ` Christian Lamparter
2016-05-12 12:07 ` Christian Lamparter
2016-05-12 12:07 ` Christian Lamparter
2016-05-13 10:59 ` Alexandre Courbot
2016-05-13 10:59 ` Alexandre Courbot
2016-05-13 10:59 ` Alexandre Courbot
2016-05-12 19:28 ` [PATCH v9.1] " Christian Lamparter
2016-05-12 19:28 ` Christian Lamparter
2016-05-12 19:28 ` Christian Lamparter
2016-05-11 9:34 ` [PATCH v9 2/2] gpio: move clps711x, moxart, ts4800 and gpio-ge into gpio-mmio Christian Lamparter
2016-05-11 9:34 ` Christian Lamparter
2016-05-11 12:19 ` Andy Shevchenko
2016-05-11 12:19 ` Andy Shevchenko
2016-05-12 10:30 ` Alexandre Courbot
2016-05-12 10:30 ` Alexandre Courbot
2016-05-12 13:07 ` Christian Lamparter
2016-05-12 13:07 ` Christian Lamparter
2016-05-16 8:34 ` Alexandre Courbot
2016-05-16 8:34 ` Alexandre Courbot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.