basic-mmio-gpio: add DT support

basic-mmio-gpio: add DT support

[Álvaro Fernández Rojas]

Add DT support while keeping legacy support.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index 16f6115..9792783 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -61,6 +61,9 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
 #include <linux/platform_device.h>
 #include <linux/mod_devicetable.h>
 #include <linux/basic_mmio_gpio.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
 
 static void bgpio_write8(void __iomem *reg, unsigned long data)
 {
@@ -488,8 +491,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
 	return ret;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id bgpio_dt_ids[] = {
+	{ .compatible = "basic-mmio-gpio" },
+};
+MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
+
+static int bgpio_probe_dt(struct platform_device *pdev)
+{
+	u32 tmp;
+	struct bgpio_pdata *pdata;
+	struct device_node *np;
+
+	np = pdev->dev.of_node;
+	if (!np)
+		return 0;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->label = dev_name(&pdev->dev);
+	pdata->base = -1;
+	if (of_find_property(np, "byte-be", NULL)) {
+		pdata->flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
+	}
+	if (of_find_property(np, "bit-be", NULL)) {
+		pdata->flags |= BGPIOF_BIG_ENDIAN;
+	}
+	if (of_find_property(np, "regset-nr", NULL)) {
+		pdata->flags |= BGPIOF_UNREADABLE_REG_SET;
+	}
+	if (of_find_property(np, "regdir-nr", NULL)) {
+		pdata->flags |= BGPIOF_UNREADABLE_REG_DIR;
+	}
+	if (!of_property_read_u32(np, "num-gpios", &tmp)) {
+		pdata->ngpio = tmp;
+	}
+
+	pdev->dev.platform_data = pdata;
+
+	return 1;
+}
+#else
+static inline int bgpio_probe_dt(struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+
 static int bgpio_pdev_probe(struct platform_device *pdev)
 {
+	int status;
 	struct device *dev = &pdev->dev;
 	struct resource *r;
 	void __iomem *dat;
@@ -498,10 +551,24 @@ 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;
 	int err;
 	struct bgpio_chip *bgc;
-	struct bgpio_pdata *pdata = dev_get_platdata(dev);
+	struct bgpio_pdata *pdata;
+	bool use_of = 0;
+
+	status = bgpio_probe_dt(pdev);
+	if (status < 0)
+		return status;
+	if (status > 0)
+		use_of = 1;
+
+	pdata = dev_get_platdata(dev);
+
+	if (!use_of)
+		flags = pdev->id_entry->driver_data;
+	else if (pdata)
+		flags = pdata->flags;
 
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
 	if (!r)
@@ -547,6 +614,9 @@ static int bgpio_pdev_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, bgc);
 
+	if (use_of)
+		of_gpiochip_add(&bgc->gc);
+
 	return gpiochip_add(&bgc->gc);
 }
 
@@ -572,6 +642,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
 static struct platform_driver bgpio_driver = {
 	.driver = {
 		.name = "basic-mmio-gpio",
+		.of_match_table = bgpio_dt_ids,
 	},
 	.id_table = bgpio_id_table,
 	.probe = bgpio_pdev_probe,
diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
index 0e97856..a35dffd 100644
--- a/include/linux/basic_mmio_gpio.h
+++ b/include/linux/basic_mmio_gpio.h
@@ -22,6 +22,7 @@ struct bgpio_pdata {
 	const char *label;
 	int base;
 	int ngpio;
+	unsigned long flags;
 };
 
 struct device;
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: basic-mmio-gpio: add DT support

[Alexandre Courbot]

On Wed, Dec 17, 2014 at 7:41 AM, Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
> Add DT support while keeping legacy support.
>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c

There is no documentation for these new bindings?

> index 16f6115..9792783 100644
> --- a/drivers/gpio/gpio-generic.c
> +++ b/drivers/gpio/gpio-generic.c
> @@ -61,6 +61,9 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>  #include <linux/platform_device.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/basic_mmio_gpio.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
>
>  static void bgpio_write8(void __iomem *reg, unsigned long data)
>  {
> @@ -488,8 +491,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
>         return ret;
>  }
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id bgpio_dt_ids[] = {
> +       { .compatible = "basic-mmio-gpio" },
> +};
> +MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
> +
> +static int bgpio_probe_dt(struct platform_device *pdev)
> +{
> +       u32 tmp;
> +       struct bgpio_pdata *pdata;
> +       struct device_node *np;
> +
> +       np = pdev->dev.of_node;
> +       if (!np)
> +               return 0;
> +
> +       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +       if (!pdata)
> +               return -ENOMEM;
> +
> +       pdata->label = dev_name(&pdev->dev);
> +       pdata->base = -1;
> +       if (of_find_property(np, "byte-be", NULL)) {
> +               pdata->flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
> +       }
> +       if (of_find_property(np, "bit-be", NULL)) {
> +               pdata->flags |= BGPIOF_BIG_ENDIAN;
> +       }
> +       if (of_find_property(np, "regset-nr", NULL)) {
> +               pdata->flags |= BGPIOF_UNREADABLE_REG_SET;
> +       }
> +       if (of_find_property(np, "regdir-nr", NULL)) {
> +               pdata->flags |= BGPIOF_UNREADABLE_REG_DIR;
> +       }
> +       if (!of_property_read_u32(np, "num-gpios", &tmp)) {
> +               pdata->ngpio = tmp;
> +       }

I don't think this is acceptable. gpio-generic is designed to be used
as a framework for other drivers to build upon. These drivers should
have their own compatible strings, which should be enough to infer all
the properties you defined here.

Device Tree identifies hardware precisely (vendor and model), and this
new binding is just not that. You *could* however have a very simple
driver that associates compatible strings to static tables containing
the values of the properties you wanted to see passed through the DT,
and have one single driver that covers many mmio-based GPIO devices.
But I'm afraid "basic-mmio-gpio" is *way* to vague to describe
hardware.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: basic-mmio-gpio: add DT support

[Álvaro Fernández Rojas]

El 14/01/2015 a las 6:32, Alexandre Courbot escribió:
> On Wed, Dec 17, 2014 at 7:41 AM, Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
>> Add DT support while keeping legacy support.
>>
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> ---
>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> 
> There is no documentation for these new bindings?

Actually, I was waiting for this patch (by kamlakant.patel@linaro.org) to be accepted before: "[PATCH v1 5/5] gpio: document basic mmio gpio library".
Anyway, I will add documentation on the next version of this patch.

> 
>> index 16f6115..9792783 100644
>> --- a/drivers/gpio/gpio-generic.c
>> +++ b/drivers/gpio/gpio-generic.c
>> @@ -61,6 +61,9 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>>  #include <linux/platform_device.h>
>>  #include <linux/mod_devicetable.h>
>>  #include <linux/basic_mmio_gpio.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_gpio.h>
>>
>>  static void bgpio_write8(void __iomem *reg, unsigned long data)
>>  {
>> @@ -488,8 +491,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
>>         return ret;
>>  }
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id bgpio_dt_ids[] = {
>> +       { .compatible = "basic-mmio-gpio" },
>> +};
>> +MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
>> +
>> +static int bgpio_probe_dt(struct platform_device *pdev)
>> +{
>> +       u32 tmp;
>> +       struct bgpio_pdata *pdata;
>> +       struct device_node *np;
>> +
>> +       np = pdev->dev.of_node;
>> +       if (!np)
>> +               return 0;
>> +
>> +       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>> +       if (!pdata)
>> +               return -ENOMEM;
>> +
>> +       pdata->label = dev_name(&pdev->dev);
>> +       pdata->base = -1;
>> +       if (of_find_property(np, "byte-be", NULL)) {
>> +               pdata->flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
>> +       }
>> +       if (of_find_property(np, "bit-be", NULL)) {
>> +               pdata->flags |= BGPIOF_BIG_ENDIAN;
>> +       }
>> +       if (of_find_property(np, "regset-nr", NULL)) {
>> +               pdata->flags |= BGPIOF_UNREADABLE_REG_SET;
>> +       }
>> +       if (of_find_property(np, "regdir-nr", NULL)) {
>> +               pdata->flags |= BGPIOF_UNREADABLE_REG_DIR;
>> +       }
>> +       if (!of_property_read_u32(np, "num-gpios", &tmp)) {
>> +               pdata->ngpio = tmp;
>> +       }
> 
> I don't think this is acceptable. gpio-generic is designed to be used
> as a framework for other drivers to build upon. These drivers should
> have their own compatible strings, which should be enough to infer all
> the properties you defined here.
> 

gpio-generic is not only designed as a framework for other drivers, it can also be used directly by registering the driver through platform device (basic-mmio-gpio/basic-mmio-gpio-be).
My intention is to make it DT compatible as a generic driver, which can be used for different hardware, by selecting different DT properties as configuration.

> Device Tree identifies hardware precisely (vendor and model), and this
> new binding is just not that. You *could* however have a very simple
> driver that associates compatible strings to static tables containing
> the values of the properties you wanted to see passed through the DT,
> and have one single driver that covers many mmio-based GPIO devices.
> But I'm afraid "basic-mmio-gpio" is *way* to vague to describe
> hardware.
> 

I don't think making a new driver mapping different compatible strings to static tables containing the values of the properties passed through DT is a sane way of doing it, since it will require multiple combinations to cover all the possibilites.

My plan is to be able to do something like this (btw, I actually tested this patch on BCM63xx and works perfectly):
gpio-controller@10000084 {
	compatible = "basic-mmio-gpio", "brcm,brcm6368";
	reg = <0x10000084 0x4>, <0x1000008c 0x4>;
	reg-names = "dirout", "dat";

	num-gpios = <32>;
	byte-be;

	gpio-controller;
	#gpio-cells = <2>;
};
And for other hardware you could do:
gpio-controller@10000180 {
	compatible = "basic-mmio-gpio", "foo,bar";
	reg = <0x10000180 0x4>, <0x10000184 0x4>, <0x10000188 0x4>;
	reg-names = "dirin", "dirout", "dat";

	num-gpios = <20>;
	bit-be;
	byte-be;
	regdir-nr;

	gpio-controller;
	#gpio-cells = <2>;
};
This way you wouldn't need to add a wrapper for each specific hardware using basic-mmio-gpio, and you would save time by using the generic driver.

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

Re: basic-mmio-gpio: add DT support

[Alexandre Courbot]

On Wed, Jan 14, 2015 at 8:20 PM, Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
> El 14/01/2015 a las 6:32, Alexandre Courbot escribió:
>> On Wed, Dec 17, 2014 at 7:41 AM, Álvaro Fernández Rojas
>> <noltari@gmail.com> wrote:
>>> Add DT support while keeping legacy support.
>>>
>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>> ---
>>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
>>
>> There is no documentation for these new bindings?
>
> Actually, I was waiting for this patch (by kamlakant.patel@linaro.org) to be accepted before: "[PATCH v1 5/5] gpio: document basic mmio gpio library".
> Anyway, I will add documentation on the next version of this patch.
>
>>
>>> index 16f6115..9792783 100644
>>> --- a/drivers/gpio/gpio-generic.c
>>> +++ b/drivers/gpio/gpio-generic.c
>>> @@ -61,6 +61,9 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>>>  #include <linux/platform_device.h>
>>>  #include <linux/mod_devicetable.h>
>>>  #include <linux/basic_mmio_gpio.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/of_gpio.h>
>>>
>>>  static void bgpio_write8(void __iomem *reg, unsigned long data)
>>>  {
>>> @@ -488,8 +491,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
>>>         return ret;
>>>  }
>>>
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id bgpio_dt_ids[] = {
>>> +       { .compatible = "basic-mmio-gpio" },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
>>> +
>>> +static int bgpio_probe_dt(struct platform_device *pdev)
>>> +{
>>> +       u32 tmp;
>>> +       struct bgpio_pdata *pdata;
>>> +       struct device_node *np;
>>> +
>>> +       np = pdev->dev.of_node;
>>> +       if (!np)
>>> +               return 0;
>>> +
>>> +       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>> +       if (!pdata)
>>> +               return -ENOMEM;
>>> +
>>> +       pdata->label = dev_name(&pdev->dev);
>>> +       pdata->base = -1;
>>> +       if (of_find_property(np, "byte-be", NULL)) {
>>> +               pdata->flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
>>> +       }
>>> +       if (of_find_property(np, "bit-be", NULL)) {
>>> +               pdata->flags |= BGPIOF_BIG_ENDIAN;
>>> +       }
>>> +       if (of_find_property(np, "regset-nr", NULL)) {
>>> +               pdata->flags |= BGPIOF_UNREADABLE_REG_SET;
>>> +       }
>>> +       if (of_find_property(np, "regdir-nr", NULL)) {
>>> +               pdata->flags |= BGPIOF_UNREADABLE_REG_DIR;
>>> +       }
>>> +       if (!of_property_read_u32(np, "num-gpios", &tmp)) {
>>> +               pdata->ngpio = tmp;
>>> +       }
>>
>> I don't think this is acceptable. gpio-generic is designed to be used
>> as a framework for other drivers to build upon. These drivers should
>> have their own compatible strings, which should be enough to infer all
>> the properties you defined here.
>>
>
> gpio-generic is not only designed as a framework for other drivers, it can also be used directly by registering the driver through platform device (basic-mmio-gpio/basic-mmio-gpio-be).

This works for platform drivers which stay confined to the kernel, but
DT is a firmware definition where such generic bindings are much less
desirable.

> My intention is to make it DT compatible as a generic driver, which can be used for different hardware, by selecting different DT properties as configuration.
>
>> Device Tree identifies hardware precisely (vendor and model), and this
>> new binding is just not that. You *could* however have a very simple
>> driver that associates compatible strings to static tables containing
>> the values of the properties you wanted to see passed through the DT,
>> and have one single driver that covers many mmio-based GPIO devices.
>> But I'm afraid "basic-mmio-gpio" is *way* to vague to describe
>> hardware.
>>
>
> I don't think making a new driver mapping different compatible strings to static tables containing the values of the properties passed through DT is a sane way of doing it, since it will require multiple combinations to cover all the possibilites.
>
> My plan is to be able to do something like this (btw, I actually tested this patch on BCM63xx and works perfectly):
> gpio-controller@10000084 {
>         compatible = "basic-mmio-gpio", "brcm,brcm6368";

Here your compatible string should be

        compatible = "brcm,brcm6368", "basic-mmio-gpio";

I.e. from more precise to less precise. A dedicated BRCM6368 driver
should take precedence over your generic one.

>         reg = <0x10000084 0x4>, <0x1000008c 0x4>;
>         reg-names = "dirout", "dat";
>
>         num-gpios = <32>;
>         byte-be;
>
>         gpio-controller;
>         #gpio-cells = <2>;
> };
> And for other hardware you could do:
> gpio-controller@10000180 {
>         compatible = "basic-mmio-gpio", "foo,bar";
>         reg = <0x10000180 0x4>, <0x10000184 0x4>, <0x10000188 0x4>;
>         reg-names = "dirin", "dirout", "dat";
>
>         num-gpios = <20>;
>         bit-be;
>         byte-be;
>         regdir-nr;
>
>         gpio-controller;
>         #gpio-cells = <2>;
> };
> This way you wouldn't need to add a wrapper for each specific hardware using basic-mmio-gpio, and you would save time by using the generic driver.

I understand the intent but IIUC the history of DT speaks  against
such generic bindings. On the other hand I can see some instances of
them like "simple-bus" for instance. Added the DT maintainers and
mailing list to get more informed opinions on the matter.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: basic-mmio-gpio: add DT support

[Mark Rutland]

On Mon, Jan 19, 2015 at 03:37:18AM +0000, Alexandre Courbot wrote:
> On Wed, Jan 14, 2015 at 8:20 PM, Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
> > El 14/01/2015 a las 6:32, Alexandre Courbot escribió:
> >> On Wed, Dec 17, 2014 at 7:41 AM, Álvaro Fernández Rojas
> >> <noltari@gmail.com> wrote:
> >>> Add DT support while keeping legacy support.
> >>>
> >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> >>> ---
> >>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> >>
> >> There is no documentation for these new bindings?
> >
> > Actually, I was waiting for this patch (by kamlakant.patel@linaro.org) to be accepted before: "[PATCH v1 5/5] gpio: document basic mmio gpio library".
> > Anyway, I will add documentation on the next version of this patch.
> >
> >>
> >>> index 16f6115..9792783 100644
> >>> --- a/drivers/gpio/gpio-generic.c
> >>> +++ b/drivers/gpio/gpio-generic.c
> >>> @@ -61,6 +61,9 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
> >>>  #include <linux/platform_device.h>
> >>>  #include <linux/mod_devicetable.h>
> >>>  #include <linux/basic_mmio_gpio.h>
> >>> +#include <linux/of.h>
> >>> +#include <linux/of_device.h>
> >>> +#include <linux/of_gpio.h>
> >>>
> >>>  static void bgpio_write8(void __iomem *reg, unsigned long data)
> >>>  {
> >>> @@ -488,8 +491,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
> >>>         return ret;
> >>>  }
> >>>
> >>> +#ifdef CONFIG_OF
> >>> +static const struct of_device_id bgpio_dt_ids[] = {
> >>> +       { .compatible = "basic-mmio-gpio" },
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
> >>> +
> >>> +static int bgpio_probe_dt(struct platform_device *pdev)
> >>> +{
> >>> +       u32 tmp;
> >>> +       struct bgpio_pdata *pdata;
> >>> +       struct device_node *np;
> >>> +
> >>> +       np = pdev->dev.of_node;
> >>> +       if (!np)
> >>> +               return 0;
> >>> +
> >>> +       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> >>> +       if (!pdata)
> >>> +               return -ENOMEM;
> >>> +
> >>> +       pdata->label = dev_name(&pdev->dev);
> >>> +       pdata->base = -1;
> >>> +       if (of_find_property(np, "byte-be", NULL)) {
> >>> +               pdata->flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
> >>> +       }
> >>> +       if (of_find_property(np, "bit-be", NULL)) {
> >>> +               pdata->flags |= BGPIOF_BIG_ENDIAN;
> >>> +       }
> >>> +       if (of_find_property(np, "regset-nr", NULL)) {
> >>> +               pdata->flags |= BGPIOF_UNREADABLE_REG_SET;
> >>> +       }
> >>> +       if (of_find_property(np, "regdir-nr", NULL)) {
> >>> +               pdata->flags |= BGPIOF_UNREADABLE_REG_DIR;
> >>> +       }
> >>> +       if (!of_property_read_u32(np, "num-gpios", &tmp)) {
> >>> +               pdata->ngpio = tmp;
> >>> +       }
> >>
> >> I don't think this is acceptable. gpio-generic is designed to be used
> >> as a framework for other drivers to build upon. These drivers should
> >> have their own compatible strings, which should be enough to infer all
> >> the properties you defined here.
> >>
> >
> > gpio-generic is not only designed as a framework for other drivers, it can also be used directly by registering the driver through platform device (basic-mmio-gpio/basic-mmio-gpio-be).
> 
> This works for platform drivers which stay confined to the kernel, but
> DT is a firmware definition where such generic bindings are much less
> desirable.
> 
> > My intention is to make it DT compatible as a generic driver, which can be used for different hardware, by selecting different DT properties as configuration.
> >
> >> Device Tree identifies hardware precisely (vendor and model), and this
> >> new binding is just not that. You *could* however have a very simple
> >> driver that associates compatible strings to static tables containing
> >> the values of the properties you wanted to see passed through the DT,
> >> and have one single driver that covers many mmio-based GPIO devices.
> >> But I'm afraid "basic-mmio-gpio" is *way* to vague to describe
> >> hardware.
> >>
> >
> > I don't think making a new driver mapping different compatible strings to static tables containing the values of the properties passed through DT is a sane way of doing it, since it will require multiple combinations to cover all the possibilites.
> >
> > My plan is to be able to do something like this (btw, I actually tested this patch on BCM63xx and works perfectly):
> > gpio-controller@10000084 {
> >         compatible = "basic-mmio-gpio", "brcm,brcm6368";
> 
> Here your compatible string should be
> 
>         compatible = "brcm,brcm6368", "basic-mmio-gpio";
> 
> I.e. from more precise to less precise. A dedicated BRCM6368 driver
> should take precedence over your generic one.
> 
> >         reg = <0x10000084 0x4>, <0x1000008c 0x4>;
> >         reg-names = "dirout", "dat";
> >
> >         num-gpios = <32>;
> >         byte-be;
> >
> >         gpio-controller;
> >         #gpio-cells = <2>;
> > };
> > And for other hardware you could do:
> > gpio-controller@10000180 {
> >         compatible = "basic-mmio-gpio", "foo,bar";
> >         reg = <0x10000180 0x4>, <0x10000184 0x4>, <0x10000188 0x4>;
> >         reg-names = "dirin", "dirout", "dat";
> >
> >         num-gpios = <20>;
> >         bit-be;
> >         byte-be;
> >         regdir-nr;
> >
> >         gpio-controller;
> >         #gpio-cells = <2>;
> > };
> > This way you wouldn't need to add a wrapper for each specific hardware using basic-mmio-gpio, and you would save time by using the generic driver.
> 
> I understand the intent but IIUC the history of DT speaks  against
> such generic bindings. On the other hand I can see some instances of
> them like "simple-bus" for instance. Added the DT maintainers and
> mailing list to get more informed opinions on the matter.

While generic bindings can be ok, they either need to be incredibly
simple (e.g. simple-bus, which has no configuration whatsoever), or need
to be rigorously specified (e.g. the generic PCI host controller
binding, which follows an existing specification).

From the context above, this looks relatively complex. At the least, a
full binding document is required along with a rationale, so that can be
reviewed.

Thanks,
Mark.
--
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

[PATCH v2 1/2] basic-mmio-gpio: add DT support

[Álvaro Fernández Rojas]

Add DT support while keeping legacy support.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
v2: rename regset-nr to regset-wo and regdir-nr to regdir-wo.
document DT bindings.

diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index 16f6115..c741abb 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -61,6 +61,9 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
 #include <linux/platform_device.h>
 #include <linux/mod_devicetable.h>
 #include <linux/basic_mmio_gpio.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
 
 static void bgpio_write8(void __iomem *reg, unsigned long data)
 {
@@ -488,8 +491,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
 	return ret;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id bgpio_dt_ids[] = {
+	{ .compatible = "basic-mmio-gpio" },
+};
+MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
+
+static int bgpio_probe_dt(struct platform_device *pdev)
+{
+	u32 tmp;
+	struct bgpio_pdata *pdata;
+	struct device_node *np;
+
+	np = pdev->dev.of_node;
+	if (!np)
+		return 0;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->label = dev_name(&pdev->dev);
+	pdata->base = -1;
+	if (of_find_property(np, "byte-be", NULL)) {
+		pdata->flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
+	}
+	if (of_find_property(np, "bit-be", NULL)) {
+		pdata->flags |= BGPIOF_BIG_ENDIAN;
+	}
+	if (of_find_property(np, "regset-wo", NULL)) {
+		pdata->flags |= BGPIOF_UNREADABLE_REG_SET;
+	}
+	if (of_find_property(np, "regdir-wo", NULL)) {
+		pdata->flags |= BGPIOF_UNREADABLE_REG_DIR;
+	}
+	if (!of_property_read_u32(np, "num-gpios", &tmp)) {
+		pdata->ngpio = tmp;
+	}
+
+	pdev->dev.platform_data = pdata;
+
+	return 1;
+}
+#else
+static inline int bgpio_probe_dt(struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+
 static int bgpio_pdev_probe(struct platform_device *pdev)
 {
+	int status;
 	struct device *dev = &pdev->dev;
 	struct resource *r;
 	void __iomem *dat;
@@ -498,10 +551,24 @@ 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;
 	int err;
 	struct bgpio_chip *bgc;
-	struct bgpio_pdata *pdata = dev_get_platdata(dev);
+	struct bgpio_pdata *pdata;
+	bool use_of = 0;
+
+	status = bgpio_probe_dt(pdev);
+	if (status < 0)
+		return status;
+	if (status > 0)
+		use_of = 1;
+
+	pdata = dev_get_platdata(dev);
+
+	if (!use_of)
+		flags = pdev->id_entry->driver_data;
+	else if (pdata)
+		flags = pdata->flags;
 
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
 	if (!r)
@@ -547,6 +614,9 @@ static int bgpio_pdev_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, bgc);
 
+	if (use_of)
+		of_gpiochip_add(&bgc->gc);
+
 	return gpiochip_add(&bgc->gc);
 }
 
@@ -572,6 +642,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
 static struct platform_driver bgpio_driver = {
 	.driver = {
 		.name = "basic-mmio-gpio",
+		.of_match_table = bgpio_dt_ids,
 	},
 	.id_table = bgpio_id_table,
 	.probe = bgpio_pdev_probe,
diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
index 0e97856..a35dffd 100644
--- a/include/linux/basic_mmio_gpio.h
+++ b/include/linux/basic_mmio_gpio.h
@@ -22,6 +22,7 @@ struct bgpio_pdata {
 	const char *label;
 	int base;
 	int ngpio;
+	unsigned long flags;
 };
 
 struct device;

--
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

[PATCH v2 2/2] basic-mmio-gpio: document DT bindings

[Álvaro Fernández Rojas]

Add DT support while keeping legacy support.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index b9bd1d6..c5d39c6 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -209,3 +209,75 @@ Example 2:
 Here, three GPIO ranges are defined wrt. two pin controllers. pinctrl1 GPIO
 ranges are defined using pin numbers whereas the GPIO ranges wrt. pinctrl2
 are named "foo" and "bar".
+
+3) Generic driver for memory-mapped GPIO controllers
+----------------------------------------------------
+The GPIO generic library provides support for basic platform_device
+memory-mapped GPIO controllers, which can be accessed by selecting Kconfig
+symbol GPIO_GENERIC and using library functions provided by GPIO generic
+driver (see drivers/gpio/gpio-generic.c).
+The simplest form of a GPIO controller that the driver support is just a
+single "data" register, where GPIO state can be read and/or written.
+
+The driver supports:
+- 8/16/32/64 bits registers. The number of GPIOs is determined by the width of
+  the registers.
+- GPIO controllers with clear/set registers.
+- GPIO controllers with a single "data" register.
+- Big endian bits/GPIOs ordering.
+
+For setting GPIO's there are three supported configurations:
+- single input/output register resource (named "dat").
+- set/clear pair (named "set" and "clr").
+- single output register resource and single input resource ("set" and dat").
+
+For setting the GPIO direction, there are three supported configurations:
+- simple bidirection GPIO that requires no configuration.
+- an output direction register (named "dirout") where a 1 bit indicates the
+  GPIO is an output.
+- an input direction register (named "dirin") where a 1 bit indicates the GPIO
+  is an input.
+
+Required properties:
+- compatible : Should be "basic-mmio-gpio".
+- reg : Address and length of the registers needed for the device.
+- reg-names : Names of the needed registers.
+- #gpio-cells : Should be two.  The first cell is the pin number and
+  the second cell is used to specify optional parameters (currently
+  unused).
+- gpio-controller : Marks the device node as a gpio controller.
+
+Optional properties:
+- num-gpios : Specify the number of configurable gpios.
+- bit-be : Use big endian bit order for pins.
+- byte-be : Use big endian byte order for registers.
+- regset-wo : set register is unreadable.
+- regdir-wo : dir register is unreadable.
+
+Examples:
+
+gpio0: gpio-controller@10000084 {
+	compatible = "brcm,brcm6368", "basic-mmio-gpio";
+	reg = <0x10000084 0x4>, <0x1000008c 0x4>;
+	reg-names = "dirout", "dat";
+
+	gpio-controller;
+	#gpio-cells = <2>;
+
+	num-gpios = <32>;
+	byte-be;
+};
+
+gpio1: gpio-controller@10000180 {
+	compatible = "foo,bar", "basic-mmio-gpio";
+	reg = <0x10000180 0x4>, <0x10000184 0x4>, <0x10000188 0x4>;
+	reg-names = "dirin", "dirout", "dat";
+
+	gpio-controller;
+	#gpio-cells = <2>;
+
+	num-gpios = <20>;
+	bit-be;
+	byte-be;
+	regdir-wo;
+};
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] basic-mmio-gpio: add DT support

[Alexandre Courbot]

On Mon, Jan 26, 2015 at 1:32 AM, Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
> Add DT support while keeping legacy support.
>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
> v2: rename regset-nr to regset-wo and regdir-nr to regdir-wo.
> document DT bindings.

As Mark explained, you need to get the bindings approved by the DT
people first, so please send them to devicetree@vger.kernel.org (and
also directly to the DT maintainers) with a rationale for this, and
get their approval. There is no point in reviewing this code until we
have confirmation that it is acceptable in the first place.
--
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