[PATCH] added device tree support to gpio-generic driver

[PATCH] added device tree support to gpio-generic driver

[Romain Baeriswyl]

---
 .../devicetree/bindings/gpio/gpio-generic.txt      |   19 +++++
 drivers/gpio/gpio-generic.c                        |   81 ++++++++++++++-----
 2 files changed, 78 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-generic.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-generic.txt b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
new file mode 100644
index 0000000..c2c4b98
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
@@ -0,0 +1,19 @@
+Bindings for gpio-generic
+
+Required properties:
+- compatible : "basic-mmio-gpio" for little endian register access or
+               "basic-mmio-gpio-be" for big endian register access
+- ngpios: Specifies the number of gpio mapped in the register. The value is
+          limited to the number of bits of the LONG type.
+
+Optional properties:
+- base: Allows to forces the gpio number base offset used to index the gpio in
+        the device. If it is not see then the driver search autonoumously for
+        valid index range.
+
+Examples:
+
+	gpio_a {
+		compatible = "basic-mmio-gpio";
+		ngpios = <32>;
+	};
diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index b92a690..9a4354c 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -15,11 +15,11 @@
  *  `.just a single "data" register, where GPIO state can be read and/or `
  *    `,..written. ,,..``~~~~ .....``.`.`.~~.```.`.........``````.```````
  *        `````````
-                                    ___
-_/~~|___/~|   . ```~~~~~~       ___/___\___     ,~.`.`.`.`````.~~...,,,,...
-__________|~$@~~~        %~    /o*o*o*o*o*o\   .. Implementing such a GPIO .
-o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
-                                                 `....trivial..'~`.```.```
+ *                                   ___
+ * _/~~|___/~|   . ```~~~~~~       ___/___\___     ,~.`.`.`.`````.~~...,,,,...
+ * __________|~$@~~~        %~    /o*o*o*o*o*o\   .. Implementing such a GPIO .
+ * o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
+ *                                                `....trivial..'~`.```.```
  *                                                    ```````
  *  .```````~~~~`..`.``.``.
  * .  The driver supports  `...       ,..```.`~~~```````````````....````.``,,
@@ -61,6 +61,8 @@ 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>
 
 static void bgpio_write8(void __iomem *reg, unsigned long data)
 {
@@ -375,10 +377,9 @@ static int bgpio_setup_accessors(struct device *dev,
 			dev_err(dev,
 				"64 bit big endian byte order unsupported\n");
 			return -EINVAL;
-		} else {
-			bgc->read_reg	= bgpio_read64;
-			bgc->write_reg	= bgpio_write64;
 		}
+		bgc->read_reg	= bgpio_read64;
+		bgc->write_reg	= bgpio_write64;
 		break;
 #endif /* BITS_PER_LONG >= 64 */
 	default:
@@ -564,6 +565,27 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
 	return ret;
 }
 
+static const struct platform_device_id bgpio_id_table[] = {
+	{ "basic-mmio-gpio",
+	  .driver_data	= 0,
+	}, { "basic-mmio-gpio-be",
+	  .driver_data	= BGPIOF_BIG_ENDIAN
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, bgpio_id_table);
+
+static const struct of_device_id  bgpio_dt_ids[] = {
+	{ .compatible = "basic-mmio-gpio",
+	  .data = bgpio_id_table + 0
+	},
+	{ .compatible = "basic-mmio-gpio-be",
+	  .data = bgpio_id_table + 1
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
+
 static int bgpio_pdev_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -574,10 +596,37 @@ 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;
+
+	if (of_have_populated_dt()) {
+		const struct of_device_id *of_id =
+			of_match_device(bgpio_dt_ids, dev);
+
+		pdata = devm_kzalloc(dev, sizeof(struct bgpio_pdata),
+				     GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+
+		if (of_property_read_u32(dev->of_node, "ngpio",
+					 &pdata->ngpio)) {
+			dev_err(dev, "Failed to get field ngpio");
+			return -EINVAL;
+		}
+		if (of_property_read_u32(dev->of_node, "base", &pdata->base))
+			pdata->base = -1;
+
+		dev->platform_data = pdata;
+
+		if (of_id)
+			pdev->id_entry = of_id->data;
+	}
+
+	pdata = dev_get_platdata(dev);
+
+	flags = pdev->id_entry->driver_data;
 
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
 	if (!r)
@@ -633,18 +682,6 @@ static int bgpio_pdev_remove(struct platform_device *pdev)
 	return bgpio_remove(bgc);
 }
 
-static const struct platform_device_id bgpio_id_table[] = {
-	{
-		.name		= "basic-mmio-gpio",
-		.driver_data	= 0,
-	}, {
-		.name		= "basic-mmio-gpio-be",
-		.driver_data	= BGPIOF_BIG_ENDIAN,
-	},
-	{ }
-};
-MODULE_DEVICE_TABLE(platform, bgpio_id_table);
-
 static struct platform_driver bgpio_driver = {
 	.driver = {
 		.name = "basic-mmio-gpio",
-- 
1.7.1

*********************************************************
This message contains information that may be confidential and/or privileged and is intended only for the individual or entity named in the body of email above. If this message has been received in error, your receipt of this message is not intended to waive any applicable privilege. No one else may disclose, copy, distribute or use the contents of this message. Unauthorized use, dissemination and duplication is strictly prohibited, and may be unlawful.

Re: [PATCH] added device tree support to gpio-generic driver

[Alexander Shiyan]

Пятница,  5 июня 2015, 8:51 +02:00 от Romain Baeriswyl <romain.baeriswyl@alitech.com>:
> ---
>  .../devicetree/bindings/gpio/gpio-generic.txt      |   19 +++++
>  drivers/gpio/gpio-generic.c                        |   81 ++++++++++++++-----
>  2 files changed, 78 insertions(+), 22 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-generic.txt

Hello.

...
> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
...
> +		if (of_property_read_u32(dev->of_node, "base", &pdata->base))
> +			pdata->base = -1;

The "base" property is excessive for DT.

---

Re: [PATCH] added device tree support to gpio-generic driver

[Alexandre Courbot]

On Fri, Jun 5, 2015 at 3:51 PM, Romain Baeriswyl
<romain.baeriswyl@alitech.com> wrote:
> ---

Your patch is missing a detailed commit message.

>  .../devicetree/bindings/gpio/gpio-generic.txt      |   19 +++++
>  drivers/gpio/gpio-generic.c                        |   81 ++++++++++++++-----
>  2 files changed, 78 insertions(+), 22 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-generic.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-generic.txt b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
> new file mode 100644
> index 0000000..c2c4b98
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
> @@ -0,0 +1,19 @@
> +Bindings for gpio-generic
> +
> +Required properties:
> +- compatible : "basic-mmio-gpio" for little endian register access or
> +               "basic-mmio-gpio-be" for big endian register access
> +- ngpios: Specifies the number of gpio mapped in the register. The value is
> +          limited to the number of bits of the LONG type.
> +
> +Optional properties:
> +- base: Allows to forces the gpio number base offset used to index the gpio in
> +        the device. If it is not see then the driver search autonoumously for
> +        valid index range.

A base property for GPIO drivers is frown upon nowadays. >:/

> +
> +Examples:
> +
> +       gpio_a {
> +               compatible = "basic-mmio-gpio";
> +               ngpios = <32>;
> +       };
> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> index b92a690..9a4354c 100644
> --- a/drivers/gpio/gpio-generic.c
> +++ b/drivers/gpio/gpio-generic.c
> @@ -15,11 +15,11 @@
>   *  `.just a single "data" register, where GPIO state can be read and/or `
>   *    `,..written. ,,..``~~~~ .....``.`.`.~~.```.`.........``````.```````
>   *        `````````
> -                                    ___
> -_/~~|___/~|   . ```~~~~~~       ___/___\___     ,~.`.`.`.`````.~~...,,,,...
> -__________|~$@~~~        %~    /o*o*o*o*o*o\   .. Implementing such a GPIO .
> -o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
> -                                                 `....trivial..'~`.```.```
> + *                                   ___
> + * _/~~|___/~|   . ```~~~~~~       ___/___\___     ,~.`.`.`.`````.~~...,,,,...
> + * __________|~$@~~~        %~    /o*o*o*o*o*o\   .. Implementing such a GPIO .
> + * o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
> + *                                                `....trivial..'~`.```.```

Comment fix? Why not, but this is not related to the subject of this
patch. Please do this in a separate patch.

>   *                                                    ```````
>   *  .```````~~~~`..`.``.``.
>   * .  The driver supports  `...       ,..```.`~~~```````````````....````.``,,
> @@ -61,6 +61,8 @@ 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>
>
>  static void bgpio_write8(void __iomem *reg, unsigned long data)
>  {
> @@ -375,10 +377,9 @@ static int bgpio_setup_accessors(struct device *dev,
>                         dev_err(dev,
>                                 "64 bit big endian byte order unsupported\n");
>                         return -EINVAL;
> -               } else {
> -                       bgc->read_reg   = bgpio_read64;
> -                       bgc->write_reg  = bgpio_write64;
>                 }
> +               bgc->read_reg   = bgpio_read64;
> +               bgc->write_reg  = bgpio_write64;

Why change this? This if/else sequence was consistent with the other
cases, I think it would be better to keep it that way.

>                 break;
>  #endif /* BITS_PER_LONG >= 64 */
>         default:
> @@ -564,6 +565,27 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
>         return ret;
>  }
>
> +static const struct platform_device_id bgpio_id_table[] = {
> +       { "basic-mmio-gpio",
> +         .driver_data  = 0,
> +       }, { "basic-mmio-gpio-be",
> +         .driver_data  = BGPIOF_BIG_ENDIAN
> +       },
> +       { },
> +};

Formatting is wrong here. Please keep the same formatting as the
original statement.

> +MODULE_DEVICE_TABLE(platform, bgpio_id_table);
> +
> +static const struct of_device_id  bgpio_dt_ids[] = {
> +       { .compatible = "basic-mmio-gpio",

Same remark about formatting.

> +         .data = bgpio_id_table + 0

I would probably prefer &bgpio_id_table[0], but your call.

> +       },
> +       { .compatible = "basic-mmio-gpio-be",
> +         .data = bgpio_id_table + 1
> +       },

Instead of having two different compatible strings, how about making
the big-endian option a boolean DT property?

> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
> +
>  static int bgpio_pdev_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -574,10 +596,37 @@ 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;
> +
> +       if (of_have_populated_dt()) {
> +               const struct of_device_id *of_id =
> +                       of_match_device(bgpio_dt_ids, dev);
> +
> +               pdata = devm_kzalloc(dev, sizeof(struct bgpio_pdata),
> +                                    GFP_KERNEL);
> +               if (!pdata)
> +                       return -ENOMEM;
> +
> +               if (of_property_read_u32(dev->of_node, "ngpio",
> +                                        &pdata->ngpio)) {
> +                       dev_err(dev, "Failed to get field ngpio");
> +                       return -EINVAL;
> +               }
> +               if (of_property_read_u32(dev->of_node, "base", &pdata->base))
> +                       pdata->base = -1;
> +
> +               dev->platform_data = pdata;
> +
> +               if (of_id)
> +                       pdev->id_entry = of_id->data;
> +       }

Could you move this to a bgpio_parse_dt() function to keep this
function short and clear?

> +
> +       pdata = dev_get_platdata(dev);
> +
> +       flags = pdev->id_entry->driver_data;
>
>         r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
>         if (!r)
> @@ -633,18 +682,6 @@ static int bgpio_pdev_remove(struct platform_device *pdev)
>         return bgpio_remove(bgc);
>  }
>
> -static const struct platform_device_id bgpio_id_table[] = {
> -       {
> -               .name           = "basic-mmio-gpio",
> -               .driver_data    = 0,
> -       }, {
> -               .name           = "basic-mmio-gpio-be",
> -               .driver_data    = BGPIOF_BIG_ENDIAN,
> -       },
> -       { }
> -};
> -MODULE_DEVICE_TABLE(platform, bgpio_id_table);
> -
>  static struct platform_driver bgpio_driver = {
>         .driver = {
>                 .name = "basic-mmio-gpio",
> --
> 1.7.1
>
> *********************************************************
> This message contains information that may be confidential and/or privileged and is intended only for the individual or entity named in the body of email above. If this message has been received in error, your receipt of this message is not intended to waive any applicable privilege. No one else may disclose, copy, distribute or use the contents of this message. Unauthorized use, dissemination and duplication is strictly prohibited, and may be unlawful.
>
>
>
>

Re: [PATCH] added device tree support to gpio-generic driver

[Romain Baeriswyl]

On 2015-06-08 06:19, Alexandre Courbot wrote:
> On Fri, Jun 5, 2015 at 3:51 PM, Romain Baeriswyl
> <romain.baeriswyl@alitech.com> wrote:
>> ---
> 
> Your patch is missing a detailed commit message.
> 
>>  .../devicetree/bindings/gpio/gpio-generic.txt      |   19 +++++
>>  drivers/gpio/gpio-generic.c                        |   81 ++++++++++++++-----
>>  2 files changed, 78 insertions(+), 22 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-generic.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-generic.txt b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
>> new file mode 100644
>> index 0000000..c2c4b98
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
>> @@ -0,0 +1,19 @@
>> +Bindings for gpio-generic
>> +
>> +Required properties:
>> +- compatible : "basic-mmio-gpio" for little endian register access or
>> +               "basic-mmio-gpio-be" for big endian register access
>> +- ngpios: Specifies the number of gpio mapped in the register. The value is
>> +          limited to the number of bits of the LONG type.
>> +
>> +Optional properties:
>> +- base: Allows to forces the gpio number base offset used to index the gpio in
>> +        the device. If it is not see then the driver search autonoumously for
>> +        valid index range.
> 
> A base property for GPIO drivers is frown upon nowadays. >:/
> 
OK, I will remove it.

>> +
>> +Examples:
>> +
>> +       gpio_a {
>> +               compatible = "basic-mmio-gpio";
>> +               ngpios = <32>;
>> +       };
>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
>> index b92a690..9a4354c 100644
>> --- a/drivers/gpio/gpio-generic.c
>> +++ b/drivers/gpio/gpio-generic.c
>> @@ -15,11 +15,11 @@
>>   *  `.just a single "data" register, where GPIO state can be read and/or `
>>   *    `,..written. ,,..``~~~~ .....``.`.`.~~.```.`.........``````.```````
>>   *        `````````
>> -                                    ___
>> -_/~~|___/~|   . ```~~~~~~       ___/___\___     ,~.`.`.`.`````.~~...,,,,...
>> -__________|~$@~~~        %~    /o*o*o*o*o*o\   .. Implementing such a GPIO .
>> -o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>> -                                                 `....trivial..'~`.```.```
>> + *                                   ___
>> + * _/~~|___/~|   . ```~~~~~~       ___/___\___     ,~.`.`.`.`````.~~...,,,,...
>> + * __________|~$@~~~        %~    /o*o*o*o*o*o\   .. Implementing such a GPIO .
>> + * o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>> + *                                                `....trivial..'~`.```.```
> 
> Comment fix? Why not, but this is not related to the subject of this
> patch. Please do this in a separate patch.
> 
I just added the '*' to have the checkpatch.pl passing.

>>   *                                                    ```````
>>   *  .```````~~~~`..`.``.``.
>>   * .  The driver supports  `...       ,..```.`~~~```````````````....````.``,,
>> @@ -61,6 +61,8 @@ 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>
>>
>>  static void bgpio_write8(void __iomem *reg, unsigned long data)
>>  {
>> @@ -375,10 +377,9 @@ static int bgpio_setup_accessors(struct device *dev,
>>                         dev_err(dev,
>>                                 "64 bit big endian byte order unsupported\n");
>>                         return -EINVAL;
>> -               } else {
>> -                       bgc->read_reg   = bgpio_read64;
>> -                       bgc->write_reg  = bgpio_write64;
>>                 }
>> +               bgc->read_reg   = bgpio_read64;
>> +               bgc->write_reg  = bgpio_write64;
> 
> Why change this? This if/else sequence was consistent with the other
> cases, I think it would be better to keep it that way.
> 
The else is actually not required as there is a return in the first
case. This change was also suggested by checkpatch.pl.

>>                 break;
>>  #endif /* BITS_PER_LONG >= 64 */
>>         default:
>> @@ -564,6 +565,27 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
>>         return ret;
>>  }
>>
>> +static const struct platform_device_id bgpio_id_table[] = {
>> +       { "basic-mmio-gpio",
>> +         .driver_data  = 0,
>> +       }, { "basic-mmio-gpio-be",
>> +         .driver_data  = BGPIOF_BIG_ENDIAN
>> +       },
>> +       { },
>> +};
> 
> Formatting is wrong here. Please keep the same formatting as the
> original statement.
> 
OK

>> +MODULE_DEVICE_TABLE(platform, bgpio_id_table);
>> +
>> +static const struct of_device_id  bgpio_dt_ids[] = {
>> +       { .compatible = "basic-mmio-gpio",
> 
> Same remark about formatting.
> 
>> +         .data = bgpio_id_table + 0
> 
> I would probably prefer &bgpio_id_table[0], but your call.
> 
>> +       },
>> +       { .compatible = "basic-mmio-gpio-be",
>> +         .data = bgpio_id_table + 1
>> +       },
> 
> Instead of having two different compatible strings, how about making
> the big-endian option a boolean DT property?
> 
I wanted to keep this device tree version aligned with the platform data
version.

>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
>> +
>>  static int bgpio_pdev_probe(struct platform_device *pdev)
>>  {
>>         struct device *dev = &pdev->dev;
>> @@ -574,10 +596,37 @@ 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;
>> +
>> +       if (of_have_populated_dt()) {
>> +               const struct of_device_id *of_id =
>> +                       of_match_device(bgpio_dt_ids, dev);
>> +
>> +               pdata = devm_kzalloc(dev, sizeof(struct bgpio_pdata),
>> +                                    GFP_KERNEL);
>> +               if (!pdata)
>> +                       return -ENOMEM;
>> +
>> +               if (of_property_read_u32(dev->of_node, "ngpio",
>> +                                        &pdata->ngpio)) {
>> +                       dev_err(dev, "Failed to get field ngpio");
>> +                       return -EINVAL;
>> +               }
>> +               if (of_property_read_u32(dev->of_node, "base", &pdata->base))
>> +                       pdata->base = -1;
>> +
>> +               dev->platform_data = pdata;
>> +
>> +               if (of_id)
>> +                       pdev->id_entry = of_id->data;
>> +       }
> 
> Could you move this to a bgpio_parse_dt() function to keep this
> function short and clear?
> 
OK

>> +
>> +       pdata = dev_get_platdata(dev);
>> +
>> +       flags = pdev->id_entry->driver_data;
>>
>>         r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
>>         if (!r)
>> @@ -633,18 +682,6 @@ static int bgpio_pdev_remove(struct platform_device *pdev)
>>         return bgpio_remove(bgc);
>>  }
>>
>> -static const struct platform_device_id bgpio_id_table[] = {
>> -       {
>> -               .name           = "basic-mmio-gpio",
>> -               .driver_data    = 0,
>> -       }, {
>> -               .name           = "basic-mmio-gpio-be",
>> -               .driver_data    = BGPIOF_BIG_ENDIAN,
>> -       },
>> -       { }
>> -};
>> -MODULE_DEVICE_TABLE(platform, bgpio_id_table);
>> -
>>  static struct platform_driver bgpio_driver = {
>>         .driver = {
>>                 .name = "basic-mmio-gpio",
>> --
>> 1.7.1
>>
>> *********************************************************
>> This message contains information that may be confidential and/or privileged and is intended only for the individual or entity named in the body of email above. If this message has been received in error, your receipt of this message is not intended to waive any applicable privilege. No one else may disclose, copy, distribute or use the contents of this message. Unauthorized use, dissemination and duplication is strictly prohibited, and may be unlawful.
>>
>>
>>
>>

Re: [PATCH] added device tree support to gpio-generic driver

[Alexandre Courbot]

On Mon, Jun 8, 2015 at 3:26 PM, Romain Baeriswyl
<romain.baeriswyl@alitech.com> wrote:
>
>
> On 2015-06-08 06:19, Alexandre Courbot wrote:
>> On Fri, Jun 5, 2015 at 3:51 PM, Romain Baeriswyl
>> <romain.baeriswyl@alitech.com> wrote:
>>> ---
>>
>> Your patch is missing a detailed commit message.
>>
>>>  .../devicetree/bindings/gpio/gpio-generic.txt      |   19 +++++
>>>  drivers/gpio/gpio-generic.c                        |   81 ++++++++++++++-----
>>>  2 files changed, 78 insertions(+), 22 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-generic.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-generic.txt b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
>>> new file mode 100644
>>> index 0000000..c2c4b98
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
>>> @@ -0,0 +1,19 @@
>>> +Bindings for gpio-generic
>>> +
>>> +Required properties:
>>> +- compatible : "basic-mmio-gpio" for little endian register access or
>>> +               "basic-mmio-gpio-be" for big endian register access
>>> +- ngpios: Specifies the number of gpio mapped in the register. The value is
>>> +          limited to the number of bits of the LONG type.
>>> +
>>> +Optional properties:
>>> +- base: Allows to forces the gpio number base offset used to index the gpio in
>>> +        the device. If it is not see then the driver search autonoumously for
>>> +        valid index range.
>>
>> A base property for GPIO drivers is frown upon nowadays. >:/
>>
> OK, I will remove it.
>
>>> +
>>> +Examples:
>>> +
>>> +       gpio_a {
>>> +               compatible = "basic-mmio-gpio";
>>> +               ngpios = <32>;
>>> +       };
>>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
>>> index b92a690..9a4354c 100644
>>> --- a/drivers/gpio/gpio-generic.c
>>> +++ b/drivers/gpio/gpio-generic.c
>>> @@ -15,11 +15,11 @@
>>>   *  `.just a single "data" register, where GPIO state can be read and/or `
>>>   *    `,..written. ,,..``~~~~ .....``.`.`.~~.```.`.........``````.```````
>>>   *        `````````
>>> -                                    ___
>>> -_/~~|___/~|   . ```~~~~~~       ___/___\___     ,~.`.`.`.`````.~~...,,,,...
>>> -__________|~$@~~~        %~    /o*o*o*o*o*o\   .. Implementing such a GPIO .
>>> -o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>>> -                                                 `....trivial..'~`.```.```
>>> + *                                   ___
>>> + * _/~~|___/~|   . ```~~~~~~       ___/___\___     ,~.`.`.`.`````.~~...,,,,...
>>> + * __________|~$@~~~        %~    /o*o*o*o*o*o\   .. Implementing such a GPIO .
>>> + * o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>>> + *                                                `....trivial..'~`.```.```
>>
>> Comment fix? Why not, but this is not related to the subject of this
>> patch. Please do this in a separate patch.
>>
> I just added the '*' to have the checkpatch.pl passing.

Would be great as the first patch of your series then. :P

>
>>>   *                                                    ```````
>>>   *  .```````~~~~`..`.``.``.
>>>   * .  The driver supports  `...       ,..```.`~~~```````````````....````.``,,
>>> @@ -61,6 +61,8 @@ 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>
>>>
>>>  static void bgpio_write8(void __iomem *reg, unsigned long data)
>>>  {
>>> @@ -375,10 +377,9 @@ static int bgpio_setup_accessors(struct device *dev,
>>>                         dev_err(dev,
>>>                                 "64 bit big endian byte order unsupported\n");
>>>                         return -EINVAL;
>>> -               } else {
>>> -                       bgc->read_reg   = bgpio_read64;
>>> -                       bgc->write_reg  = bgpio_write64;
>>>                 }
>>> +               bgc->read_reg   = bgpio_read64;
>>> +               bgc->write_reg  = bgpio_write64;
>>
>> Why change this? This if/else sequence was consistent with the other
>> cases, I think it would be better to keep it that way.
>>
> The else is actually not required as there is a return in the first
> case. This change was also suggested by checkpatch.pl.

checkpatch is a useful script, but at the end of the day you still
should apply your judgment to know whether what it suggests actually
makes sense or not. In this case, I would favor code consistency over
arbitrary rules.

>
>>>                 break;
>>>  #endif /* BITS_PER_LONG >= 64 */
>>>         default:
>>> @@ -564,6 +565,27 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
>>>         return ret;
>>>  }
>>>
>>> +static const struct platform_device_id bgpio_id_table[] = {
>>> +       { "basic-mmio-gpio",
>>> +         .driver_data  = 0,
>>> +       }, { "basic-mmio-gpio-be",
>>> +         .driver_data  = BGPIOF_BIG_ENDIAN
>>> +       },
>>> +       { },
>>> +};
>>
>> Formatting is wrong here. Please keep the same formatting as the
>> original statement.
>>
> OK
>
>>> +MODULE_DEVICE_TABLE(platform, bgpio_id_table);
>>> +
>>> +static const struct of_device_id  bgpio_dt_ids[] = {
>>> +       { .compatible = "basic-mmio-gpio",
>>
>> Same remark about formatting.
>>
>>> +         .data = bgpio_id_table + 0
>>
>> I would probably prefer &bgpio_id_table[0], but your call.
>>
>>> +       },
>>> +       { .compatible = "basic-mmio-gpio-be",
>>> +         .data = bgpio_id_table + 1
>>> +       },
>>
>> Instead of having two different compatible strings, how about making
>> the big-endian option a boolean DT property?
>>
> I wanted to keep this device tree version aligned with the platform data
> version.

Mmm makes sense, but in this case I think the platform got it wrong.
The BIG_ENDIAN flag should be part of the platform data, not selected
from the driver's name. I'm open to be refuted, of course.

Re: [PATCH] added device tree support to gpio-generic driver

[Romain Baeriswyl]

>> ---
> 
> Your patch is missing a detailed commit message.
> 
>>  .../devicetree/bindings/gpio/gpio-generic.txt      |   19 +++++
>>  drivers/gpio/gpio-generic.c                        |   81 ++++++++++++++-----
>>  2 files changed, 78 insertions(+), 22 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-generic.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-generic.txt b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
>> new file mode 100644
>> index 0000000..c2c4b98
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
>> @@ -0,0 +1,19 @@
>> +Bindings for gpio-generic
>> +
>> +Required properties:
>> +- compatible : "basic-mmio-gpio" for little endian register access or
>> +               "basic-mmio-gpio-be" for big endian register access
>> +- ngpios: Specifies the number of gpio mapped in the register. The value is
>> +          limited to the number of bits of the LONG type.
>> +
>> +Optional properties:
>> +- base: Allows to forces the gpio number base offset used to index the gpio in
>> +        the device. If it is not see then the driver search autonoumously for
>> +        valid index range.
> 
> A base property for GPIO drivers is frown upon nowadays. >:/

With the gpio index base managed by the gpio framework, how can we know
the gpio index to be used with the gpio sysfs interface (export, import)?
The base may change when adding/removing one instance of this driver.

> 
>> +
>> +Examples:
>> +
>> +       gpio_a {
>> +               compatible = "basic-mmio-gpio";
>> +               ngpios = <32>;
>> +       };
>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
>> index b92a690..9a4354c 100644
>> --- a/drivers/gpio/gpio-generic.c
>> +++ b/drivers/gpio/gpio-generic.c
>> @@ -15,11 +15,11 @@
>>   *  `.just a single "data" register, where GPIO state can be read and/or `
>>   *    `,..written. ,,..``~~~~ .....``.`.`.~~.```.`.........``````.```````
>>   *        `````````
>> -                                    ___
>> -_/~~|___/~|   . ```~~~~~~       ___/___\___     ,~.`.`.`.`````.~~...,,,,...
>> -__________|~$@~~~        %~    /o*o*o*o*o*o\   .. Implementing such a GPIO .
>> -o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>> -                                                 `....trivial..'~`.```.```
>> + *                                   ___
>> + * _/~~|___/~|   . ```~~~~~~       ___/___\___     ,~.`.`.`.`````.~~...,,,,...
>> + * __________|~$@~~~        %~    /o*o*o*o*o*o\   .. Implementing such a GPIO .
>> + * o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>> + *                                                `....trivial..'~`.```.```
> 
> Comment fix? Why not, but this is not related to the subject of this
> patch. Please do this in a separate patch.
> 
>>   *                                                    ```````
>>   *  .```````~~~~`..`.``.``.
>>   * .  The driver supports  `...       ,..```.`~~~```````````````....````.``,,
>> @@ -61,6 +61,8 @@ 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>
>>
>>  static void bgpio_write8(void __iomem *reg, unsigned long data)
>>  {
>> @@ -375,10 +377,9 @@ static int bgpio_setup_accessors(struct device *dev,
>>                         dev_err(dev,
>>                                 "64 bit big endian byte order unsupported\n");
>>                         return -EINVAL;
>> -               } else {
>> -                       bgc->read_reg   = bgpio_read64;
>> -                       bgc->write_reg  = bgpio_write64;
>>                 }
>> +               bgc->read_reg   = bgpio_read64;
>> +               bgc->write_reg  = bgpio_write64;
> 
> Why change this? This if/else sequence was consistent with the other
> cases, I think it would be better to keep it that way.
> 
>>                 break;
>>  #endif /* BITS_PER_LONG >= 64 */
>>         default:
>> @@ -564,6 +565,27 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
>>         return ret;
>>  }
>>
>> +static const struct platform_device_id bgpio_id_table[] = {
>> +       { "basic-mmio-gpio",
>> +         .driver_data  = 0,
>> +       }, { "basic-mmio-gpio-be",
>> +         .driver_data  = BGPIOF_BIG_ENDIAN
>> +       },
>> +       { },
>> +};
> 
> Formatting is wrong here. Please keep the same formatting as the
> original statement.
> 
>> +MODULE_DEVICE_TABLE(platform, bgpio_id_table);
>> +
>> +static const struct of_device_id  bgpio_dt_ids[] = {
>> +       { .compatible = "basic-mmio-gpio",
> 
> Same remark about formatting.
> 
>> +         .data = bgpio_id_table + 0
> 
> I would probably prefer &bgpio_id_table[0], but your call.
> 
>> +       },
>> +       { .compatible = "basic-mmio-gpio-be",
>> +         .data = bgpio_id_table + 1
>> +       },
> 
> Instead of having two different compatible strings, how about making
> the big-endian option a boolean DT property?
> 
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
>> +
>>  static int bgpio_pdev_probe(struct platform_device *pdev)
>>  {
>>         struct device *dev = &pdev->dev;
>> @@ -574,10 +596,37 @@ 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;
>> +
>> +       if (of_have_populated_dt()) {
>> +               const struct of_device_id *of_id =
>> +                       of_match_device(bgpio_dt_ids, dev);
>> +
>> +               pdata = devm_kzalloc(dev, sizeof(struct bgpio_pdata),
>> +                                    GFP_KERNEL);
>> +               if (!pdata)
>> +                       return -ENOMEM;
>> +
>> +               if (of_property_read_u32(dev->of_node, "ngpio",
>> +                                        &pdata->ngpio)) {
>> +                       dev_err(dev, "Failed to get field ngpio");
>> +                       return -EINVAL;
>> +               }
>> +               if (of_property_read_u32(dev->of_node, "base", &pdata->base))
>> +                       pdata->base = -1;
>> +
>> +               dev->platform_data = pdata;
>> +
>> +               if (of_id)
>> +                       pdev->id_entry = of_id->data;
>> +       }
> 
> Could you move this to a bgpio_parse_dt() function to keep this
> function short and clear?
> 
>> +
>> +       pdata = dev_get_platdata(dev);
>> +
>> +       flags = pdev->id_entry->driver_data;
>>
>>         r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
>>         if (!r)
>> @@ -633,18 +682,6 @@ static int bgpio_pdev_remove(struct platform_device *pdev)
>>         return bgpio_remove(bgc);
>>  }
>>
>> -static const struct platform_device_id bgpio_id_table[] = {
>> -       {
>> -               .name           = "basic-mmio-gpio",
>> -               .driver_data    = 0,
>> -       }, {
>> -               .name           = "basic-mmio-gpio-be",
>> -               .driver_data    = BGPIOF_BIG_ENDIAN,
>> -       },
>> -       { }
>> -};
>> -MODULE_DEVICE_TABLE(platform, bgpio_id_table);
>> -
>>  static struct platform_driver bgpio_driver = {
>>         .driver = {
>>                 .name = "basic-mmio-gpio",
>> --
>> 1.7.1
>>
>> *********************************************************
>> This message contains information that may be confidential and/or privileged and is intended only for the individual or entity named in the body of email above. If this message has been received in error, your receipt of this message is not intended to waive any applicable privilege. No one else may disclose, copy, distribute or use the contents of this message. Unauthorized use, dissemination and duplication is strictly prohibited, and may be unlawful.
>>
>>
>>
>>

Re: [PATCH] added device tree support to gpio-generic driver

[Linus Walleij]

This patch needs to go to the devicetree list too, see To: on this
mail.

On Fri, Jun 5, 2015 at 8:51 AM, Romain Baeriswyl
<romain.baeriswyl@alitech.com> wrote:

> +Required properties:
> +- compatible : "basic-mmio-gpio" for little endian register access or
> +               "basic-mmio-gpio-be" for big endian register access

Basic I don't know.

"single-register-gpio" is more to the point, don't you think?

> +- ngpios: Specifies the number of gpio mapped in the register. The value is
> +          limited to the number of bits of the LONG type.

So if it is 8 for a 32 bit register, does it mean bits 0..7 little endian or
big endian, or does it depend on endianness? Clarify this
in the binding.

> +
> +Optional properties:
> +- base: Allows to forces the gpio number base offset used to index the gpio in
> +        the device. If it is not see then the driver search autonoumously for
> +        valid index range.
> +
> +Examples:
> +
> +       gpio_a {
> +               compatible = "basic-mmio-gpio";
> +               ngpios = <32>;
> +       };

Yours,
Linus Walleij

[PATCH V2] added device tree support to gpio-generic driver

[Romain Baeriswyl]

This patch adds support of device tree to the gpio-generic driver.

Signed-off-by: Romain Baeriswyl <romain.baeriswyl@alitech.com> 
---
 .../devicetree/bindings/gpio/gpio-generic.txt      |   17 +++++
 drivers/gpio/gpio-generic.c                        |   72 +++++++++++++++----
 include/linux/basic_mmio_gpio.h                    |    1 +
 3 files changed, 75 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-generic.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-generic.txt b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
new file mode 100644
index 0000000..20e3727
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
@@ -0,0 +1,17 @@
+Bindings for gpio-generic
+
+Required properties:
+- compatible : should be "single-register-gpio"
+- ngpios: specifies the number of gpio mapped in the register from bit 0 to
+          bit ngpios-1. The value is limited to the number of bits of the
+	  LONG type.
+
+Optional property:
+- big-endian: tells that register is big endian.
+
+Examples:
+
+	gpio_a {
+		compatible = "single-register-gpio";
+		ngpios = <32>;
+	};
diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index b92a690..8143c10 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -61,6 +61,8 @@ 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>
 
 static void bgpio_write8(void __iomem *reg, unsigned long data)
 {
@@ -564,6 +566,46 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
 	return ret;
 }
 
+static const struct platform_device_id bgpio_id_table[] = {
+	{
+		"basic-mmio-gpio",
+		.driver_data	= 0,
+	}, {
+		"basic-mmio-gpio-be",
+		.driver_data	= BGPIOF_BIG_ENDIAN
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, bgpio_id_table);
+
+static const struct of_device_id  bgpio_dt_ids[] = {
+	{ .compatible = "single-register-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
+
+static int bgpio_parse_dt(struct device *dev)
+{
+	struct bgpio_pdata *pdata;
+
+	pdata = devm_kzalloc(dev, sizeof(struct bgpio_pdata),
+			     GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	if (of_property_read_u32(dev->of_node, "ngpio", &pdata->ngpio)) {
+		dev_err(dev, "Failed to get field ngpio");
+		return -EINVAL;
+	}
+	pdata->base = -1;
+
+	if (of_device_is_big_endian(dev->of_node))
+		pdata->flags = BGPIOF_BIG_ENDIAN;
+
+	dev->platform_data = pdata;
+	return 0;
+}
+
 static int bgpio_pdev_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -574,10 +616,21 @@ 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;
 	int err;
 	struct bgpio_chip *bgc;
-	struct bgpio_pdata *pdata = dev_get_platdata(dev);
+	struct bgpio_pdata *pdata;
+
+	if (of_have_populated_dt()) {
+		err = bgpio_parse_dt(dev);
+		if (err < 0) {
+			dev_err(dev, "Failed to get DT data\n");
+			return err;
+		}
+	}
+	pdata = dev_get_platdata(dev);
+
+	if (!of_have_populated_dt())
+		pdata->flags = pdev->id_entry->driver_data;
 
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
 	if (!r)
@@ -609,7 +662,8 @@ static int bgpio_pdev_probe(struct platform_device *pdev)
 	if (!bgc)
 		return -ENOMEM;
 
-	err = bgpio_init(bgc, dev, sz, dat, set, clr, dirout, dirin, flags);
+	err = bgpio_init(bgc, dev, sz, dat, set, clr, dirout, dirin,
+			 pdata->flags);
 	if (err)
 		return err;
 
@@ -633,18 +687,6 @@ static int bgpio_pdev_remove(struct platform_device *pdev)
 	return bgpio_remove(bgc);
 }
 
-static const struct platform_device_id bgpio_id_table[] = {
-	{
-		.name		= "basic-mmio-gpio",
-		.driver_data	= 0,
-	}, {
-		.name		= "basic-mmio-gpio-be",
-		.driver_data	= BGPIOF_BIG_ENDIAN,
-	},
-	{ }
-};
-MODULE_DEVICE_TABLE(platform, bgpio_id_table);
-
 static struct platform_driver bgpio_driver = {
 	.driver = {
 		.name = "basic-mmio-gpio",
diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
index 0e97856..d85d9ca 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;
+	int flags;
 };
 
 struct device;
-- 
1.7.1

*********************************************************
This message contains information that may be confidential and/or privileged and is intended only for the individual or entity named in the body of email above. If this message has been received in error, your receipt of this message is not intended to waive any applicable privilege. No one else may disclose, copy, distribute or use the contents of this message. Unauthorized use, dissemination and duplication is strictly prohibited, and may be unlawful.

Re: [PATCH V2] added device tree support to gpio-generic driver

[Alexander Shiyan]

Hello.

Четверг, 11 июня 2015, 12:32 +02:00 от Romain Baeriswyl <romain.baeriswyl@alitech.com>:
> This patch adds support of device tree to the gpio-generic driver.
> 
> Signed-off-by: Romain Baeriswyl <romain.baeriswyl@alitech.com> 
> ---

The only issue how the direction will be handled in this case?

Thanks.
---

Re: [PATCH V2] added device tree support to gpio-generic driver

[Sascha Hauer]

On Thu, Jun 11, 2015 at 01:42:16PM +0300, Alexander Shiyan wrote:
> Hello.
> 
> Четверг, 11 июня 2015, 12:32 +02:00 от Romain Baeriswyl <romain.baeriswyl@alitech.com>:
> > This patch adds support of device tree to the gpio-generic driver.
> > 
> > Signed-off-by: Romain Baeriswyl <romain.baeriswyl@alitech.com> 
> > ---
> 
> The only issue how the direction will be handled in this case?

The bgpio driver uses named resources for the different registers, like
"dat", "set", "clr", "dirout". That of course should be described in the
binding.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
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] added device tree support to gpio-generic driver

[Linus Walleij]

On Thu, Jun 11, 2015 at 12:32 PM, Romain Baeriswyl
<romain.baeriswyl@alitech.com> wrote:

> This patch adds support of device tree to the gpio-generic driver.
>
> Signed-off-by: Romain Baeriswyl <romain.baeriswyl@alitech.com>

I notriced this patch seems to miss compilation guards.
Please make sure it still compiles for platforms without CONFIG_OF and
CONFIG_OF_GPIO.

Yours,
Linus Walleij

[PATCH V3] added device tree support to gpio-generic driver

[Romain Baeriswyl]

This patch adds support of device tree to the gpio-generic driver.

Signed-off-by: Romain Baeriswyl <romain.baeriswyl@alitech.com> 
---
 .../devicetree/bindings/gpio/gpio-generic.txt      |   62 ++++++++++++++++++++
 drivers/gpio/gpio-generic.c                        |   60 ++++++++++++++++++-
 include/linux/basic_mmio_gpio.h                    |    1 +
 3 files changed, 120 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-generic.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-generic.txt b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
new file mode 100644
index 0000000..5259b2c
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-generic.txt
@@ -0,0 +1,62 @@
+Bindings for gpio-generic
+
+Required properties:
+- compatible : should be "single-register-gpio"
+- ngpios: specifies the number of gpio mapped in the register from bit 0 to
+          bit ngpios-1. The value is limited to the number of bits of the
+	  LONG type.
+- reg-names: must contain
+      "dat" - data register
+  may contain
+      "set" - data set register
+      "clr" - data clear register
+      "dirout" - direction output register
+      "dirin" - direction input register
+- reg: address + size pairs describing the GPIO register sets; order must
+       correspond with the order of entries in reg-names
+- #gpio-cells = must be set to 2
+
+Optional property:
+- big-endian: tells that register is big endian.
+
+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 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 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.
+
+Examples:
+
+	/* Configuration with single input/output register and
+	 * simple bidirection GPIO.
+	 */
+	gpio_a {
+		compatible = "basic-mmio-gpio";
+		ngpios = <32>;
+		reg = <0x18000000 0x4>;
+		reg-names = "dat";
+		#gpio-cells = <2>;
+	};
+
+	/* Configuration with set/clear pair registers and with an output
+	 * direction register
+	 */
+	gpio_b {
+		compatible = "single-register-gpio";
+		ngpios = <32>;
+		reg = <0x18000000 0x4>, <0x18000010 0x4>,
+		      <0x18000004 0x4>, <0x18000008 0x4>;
+		reg-names = "dat", "set", "clr", "dirout";
+		#gpio-cells = <2>;
+	};
diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index b92a690..da1408a 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -61,6 +61,8 @@ 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>
 
 static void bgpio_write8(void __iomem *reg, unsigned long data)
 {
@@ -564,6 +566,35 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
 	return ret;
 }
 
+#ifdef CONFIG_OF
+static int bgpio_parse_dt(struct device *dev)
+{
+	struct bgpio_pdata *pdata;
+
+	pdata = devm_kzalloc(dev, sizeof(struct bgpio_pdata),
+			     GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	if (of_property_read_u32(dev->of_node, "ngpio", &pdata->ngpio)) {
+		dev_err(dev, "Failed to get field ngpio");
+		return -EINVAL;
+	}
+	pdata->base = -1;
+
+	if (of_device_is_big_endian(dev->of_node))
+		pdata->flags = BGPIOF_BIG_ENDIAN;
+
+	dev->platform_data = pdata;
+	return 0;
+}
+#else
+static int bgpio_parse_dt(struct device *dev)
+{
+	return 0;
+}
+#endif /* CONFIG_OF */
+
 static int bgpio_pdev_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -574,10 +605,21 @@ 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;
 	int err;
 	struct bgpio_chip *bgc;
-	struct bgpio_pdata *pdata = dev_get_platdata(dev);
+	struct bgpio_pdata *pdata;
+
+	if (of_have_populated_dt()) {
+		err = bgpio_parse_dt(dev);
+		if (err < 0) {
+			dev_err(dev, "Failed to get DT data\n");
+			return err;
+		}
+	}
+	pdata = dev_get_platdata(dev);
+
+	if (!of_have_populated_dt())
+		pdata->flags = pdev->id_entry->driver_data;
 
 	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
 	if (!r)
@@ -609,7 +651,8 @@ static int bgpio_pdev_probe(struct platform_device *pdev)
 	if (!bgc)
 		return -ENOMEM;
 
-	err = bgpio_init(bgc, dev, sz, dat, set, clr, dirout, dirin, flags);
+	err = bgpio_init(bgc, dev, sz, dat, set, clr, dirout, dirin,
+			 pdata->flags);
 	if (err)
 		return err;
 
@@ -633,6 +676,14 @@ static int bgpio_pdev_remove(struct platform_device *pdev)
 	return bgpio_remove(bgc);
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id  bgpio_of_match[] = {
+	{ .compatible = "basic-mmio-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bgpio_of_match);
+#endif
+
 static const struct platform_device_id bgpio_id_table[] = {
 	{
 		.name		= "basic-mmio-gpio",
@@ -648,6 +699,9 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table);
 static struct platform_driver bgpio_driver = {
 	.driver = {
 		.name = "basic-mmio-gpio",
+#ifdef CONFIG_OF
+		.of_match_table = of_match_ptr(bgpio_of_match),
+#endif
 	},
 	.id_table = bgpio_id_table,
 	.probe = bgpio_pdev_probe,
diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
index 0e97856..d85d9ca 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;
+	int flags;
 };
 
 struct device;
-- 
1.7.1

*********************************************************
This message contains information that may be confidential and/or privileged and is intended only for the individual or entity named in the body of email above. If this message has been received in error, your receipt of this message is not intended to waive any applicable privilege. No one else may disclose, copy, distribute or use the contents of this message. Unauthorized use, dissemination and duplication is strictly prohibited, and may be unlawful.

Re: [PATCH V3] added device tree support to gpio-generic driver

[Masahiro Yamada]

Just cosmetic comments.

> added device tree support to gpio-generic driver

 - Please use the present tense in your commit subject
 - Please prefix the subject with a word describing which area your
patch is related to, in this case "gpio: "

So, your patch subject can be improved as  "gpio: add device tree
support to gpio-generic driver"




-- 
Best Regards
Masahiro Yamada

Re: [PATCH] added device tree support to gpio-generic driver

[Geert Uytterhoeven]

On Wed, Jun 10, 2015 at 10:42 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Fri, Jun 5, 2015 at 8:51 AM, Romain Baeriswyl
> <romain.baeriswyl@alitech.com> wrote:
>
>> +Required properties:
>> +- compatible : "basic-mmio-gpio" for little endian register access or
>> +               "basic-mmio-gpio-be" for big endian register access
>
> Basic I don't know.
>
> "single-register-gpio" is more to the point, don't you think?
>
>> +- ngpios: Specifies the number of gpio mapped in the register. The value is
>> +          limited to the number of bits of the LONG type.
>
> So if it is 8 for a 32 bit register, does it mean bits 0..7 little endian or
> big endian, or does it depend on endianness? Clarify this
> in the binding.

LONG can be 64-bit too.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds