diff mbox

[v8,2/3] gpio: mmio: add DT support for memory-mapped GPIOs

Message ID 535f785bf6116c0fb6f46afbb77e6d4bd3ef5f60.1462543458.git.chunkeey@googlemail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Christian Lamparter May 8, 2016, 1:08 p.m. UTC
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".

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

Comments

Joachim Eastwood May 8, 2016, 5:17 p.m. UTC | #1
Hi Christian,

On 8 May 2016 at 15:08, Christian Lamparter <chunkeey@googlemail.com> 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:
>         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".
>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---

> +#define ADD(_name, _func) { .compatible = _name, .data = _func }

I don't see the point in having a macro for such a simple data
structure, but since this v8 and Linus hasn't complained I guess it's
fine.

Using a macro here makes it impossible to grep for 'compatible'. Doing
'git grep compatible drivers/gpio/' is sometimes very useful to see
which hardware the driver actually supports.

> +static const struct of_device_id bgpio_of_match[] = {
> +       ADD("wd,mbl-gpio", bgpio_basic_mmio_parse_dt),
> +       { }
> +};
> +#undef ADD
> +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 *);
> +       const struct device_node *node = pdev->dev.of_node;
> +       const struct of_device_id *of_id;
> +       struct bgpio_pdata *pdata;
> +       int err = -ENODEV;
> +
> +       of_id = of_match_node(bgpio_of_match, node);
> +       if (!of_id)
> +               return NULL;
> +
> +       pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
> +                            GFP_KERNEL);
> +       if (!pdata)
> +               return ERR_PTR(-ENOMEM);
> +
> +       parse_dt = (const void *)of_id->data;

You can retrieve OF match data using of_device_get_match_data(). Saves
you a couple of lines and better explains what your doing.


> +       if (parse_dt)
> +               err = parse_dt(pdev, pdata, flags);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       return pdata;
> +}


regards,
Joachim Eastwood
Christian Lamparter May 8, 2016, 6:27 p.m. UTC | #2
On Sunday, May 08, 2016 07:17:13 PM Joachim Eastwood wrote:
> > +#define ADD(_name, _func) { .compatible = _name, .data = _func }
> 
> I don't see the point in having a macro for such a simple data
> structure, but since this v8 and Linus hasn't complained I guess it's
> fine.
>
> Using a macro here makes it impossible to grep for 'compatible'. Doing
> 'git grep compatible drivers/gpio/' is sometimes very useful to see
> which hardware the driver actually supports.

Ok. I'll definitely picking it up. I'll wait until Tuesday/Wednesday
for more comments and then release a new series.
 
> > +static const struct of_device_id bgpio_of_match[] = {
> > +       ADD("wd,mbl-gpio", bgpio_basic_mmio_parse_dt),
> > +       { }
> > +};
> > +#undef ADD
> > +MODULE_DEVICE_TABLE(of, bgpio_of_match);
> > +
> > +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
> > +                                         unsigned long *flags)
> > +{
[...]
> > +       of_id = of_match_node(bgpio_of_match, node);
> > +       if (!of_id)
> > +               return NULL;
[...]
> You can retrieve OF match data using of_device_get_match_data().
> Saves you a couple of lines and better explains what your doing.
Yes thanks that's useful too since I don't need the of_id variable
anymore.

Both improvements save a total of 8 lines. so it's 328 insertions(+)
vs 380 deletions(-).

Regards,
Christian
Linus Walleij May 10, 2016, 12:08 p.m. UTC | #3
On Sun, May 8, 2016 at 3:08 PM, Christian Lamparter
<chunkeey@googlemail.com> 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.

Overall very nice, just waiting for the next version.

> The first user for this binding is "wd,mbl-gpio".

And that binding defines that we have a register named "dat".

> +       if (of_property_read_bool(dev->of_node, "no-output"))

And then this too.

Do we want these generic MMIO bindings (dat, no-output)
in a special document like
Documentation/devicetree/bindings/gpio/gpio-mmio.txt?

Going forward?

This patch set mainly deals with refactorings, but in the
long run we want to slim things down a bit and use standard
bindings I think.

Yours,
Linus Walleij
Christian Lamparter May 10, 2016, 7:55 p.m. UTC | #4
On Tuesday, May 10, 2016 02:08:45 PM Linus Walleij wrote:
> On Sun, May 8, 2016 at 3:08 PM, Christian Lamparter
> <chunkeey@googlemail.com> 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.
> 
> Overall very nice, just waiting for the next version.

K, will deliver. I noticed that you sent a mail in which you stated
that you applied the dt binding already. Can you update your devel
branch on git.kernel.org's linux-gpio? Then, I'll simply rebase my
series and sent the remaining two patches.
(unless you tell me otherwise).
 
> > The first user for this binding is "wd,mbl-gpio".
> 
> And that binding defines that we have a register named "dat".

Yeah, I had to remove all non wd related bits. But since this 
series was posted (over and over :D) on a public mailing-list
the original  "generic" linux-mmio binding is available for
everybody to perusal[0] and study. I think what we can make
would be something like a devicetree template out of it. This
way people can remove unused flags and regnames for their
compatible device tree binding. (But first: need to finish
that ppc-gpio.txt).

> > +       if (of_property_read_bool(dev->of_node, "no-output"))
> 
> And then this too.
> 
> Do we want these generic MMIO bindings (dat, no-output)
> in a special document like
> Documentation/devicetree/bindings/gpio/gpio-mmio.txt?
> 
> Going forward?
Ah, I was thinking about Documentation/gpio... Since there's no
way it would go in the devicetree/bindings without having a compatible?
(And there's technically none). As far as I know the problem here is not
that it would be impossible to do that (updating a .dts file is "easy"...),
but updating .dtb to a tiny flash-rom on the device might not be. So we
have to make every effort to preserve compatibility for those devices
(and old/incomplete/broken dtbs) as long as the device is supported.

About adding new device:
This will work in the following way: 
 1. new drives will need to supply their hardware-specific devicetree
    binding file to the dt maintainers (This "vendor,device.txt" file
    will be like the wd,mbl-gpio.txt - but modified for the hardware
    (this is where the template would be handy)

 2. Make a one-liner patch which adds a compatible string to
    gpio-mmio.c's bgpio_of_match table:

+	{ .compatible = "vendor,device", .data = bgpio_basic_mmio_parse_dt },

(Of course, not having parses for the "ngpio" property and the flags like
big-endian, reg-output-reg,set, unreadable-reg-dir, ... properties from
the get-go is sad, these can add back once a driver/binding needs it).

I think brcm63xx will be following shortly. So we can test the
procedure.
> This patch set mainly deals with refactorings, but in the
> long run we want to slim things down a bit and use standard
> bindings I think.
Well, to do that, I think we need to collect enough devices to
make it a real "class" of devices first.

Regards,
Christian

[0] <https://lkml.org/lkml/2016/4/28/921>
diff mbox

Patch

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 6c1cb3b..4b7b74c 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,66 @@  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;
+	int err;
+
+	pdata->base = -1;
+
+	if (of_property_read_bool(dev->of_node, "no-output"))
+		*flags |= BGPIOF_NO_OUTPUT;
+
+	return 0;
+}
+
+#define ADD(_name, _func) { .compatible = _name, .data = _func }
+
+static const struct of_device_id bgpio_of_match[] = {
+	ADD("wd,mbl-gpio", bgpio_basic_mmio_parse_dt),
+	{ }
+};
+#undef ADD
+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 *);
+	const struct device_node *node = pdev->dev.of_node;
+	const struct of_device_id *of_id;
+	struct bgpio_pdata *pdata;
+	int err = -ENODEV;
+
+	of_id = of_match_node(bgpio_of_match, node);
+	if (!of_id)
+		return NULL;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
+			     GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	parse_dt = (const void *)of_id->data;
+	if (parse_dt)
+		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 +641,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 +717,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,