[PATCH] gpio: exynos4: Add device tree support

[PATCH] gpio: exynos4: Add device tree support

[Thomas Abraham]

As gpio chips get registered, a device tree node which represents the
gpio chip is searched and attached to it. A translate function is also
provided to convert the gpio specifier into actual platform settings
for pin function selection, pull up/down and drive strength settings.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
This patch addes device tree support for exynos4 with minimal changes to
existing code. All the gpio chips that are registered find a corresponding
gpio-controller in the device tree. An example of such a node is:

	gpd1: gpio-controller@114000C0 {
		compatible = "samsung,exynos4-gpio-gpd1", "samsung,exynos4-gpio";
		#gpio-cells = <4>;
		gpio-controller;
	};

Four cells are required to describe a gpio and all its properties. The pin
function selection, pull up/down and drive strength settings are considered
the properties of the gpio pin. This fits well with the hardware organization
of gpio, pinmux, pull up/down and drive strength for exynos4.

The format of the gpio specifier is
<[phandle] [gpio_pin] [function] [pull] [drive_strength]>;

- gpio_pin: Pin number within a gpio-controller.
- function: Pinmux function number (as per the SoC spec).
- pull: Pull up/down setting value (as per the SoC spec).
- drive_strength: Pin Driver strength setting (as per the SoC spec).

Example: A i2c device node which uses two gpio lines is listed below.

	i2c@13860000 {
		compatible = "samsung,s3c2440-i2c";
		reg = <0x13860000 0x100>;
		interrupts = <122>;
		gpios = <&gpd1 0 2 3 0
			 &gpd1 1 2 3 0>;
	};

A driver or any other portion of the code that is looking up a gpio number
from a device tree node would first get the gpio number using the of_get_gpio()
function. And then use the gpio_request() function on that gpio number.

This approach has been tested with i2c driver for exynos4.

 drivers/gpio/gpio-exynos4.c |   53 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpio-exynos4.c b/drivers/gpio/gpio-exynos4.c
index d24b337..aa2d9fb 100644
--- a/drivers/gpio/gpio-exynos4.c
+++ b/drivers/gpio/gpio-exynos4.c
@@ -13,6 +13,10 @@
 #include <linux/irq.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
+#ifdef CONFIG_OF
+#include <linux/of.h>
+#include <linux/slab.h>
+#endif
 
 #include <mach/map.h>
 
@@ -320,6 +324,52 @@ static struct s3c_gpio_chip exynos4_gpio_part3_4bit[] = {
 	},
 };
 
+#ifdef CONFIG_OF
+int exynos4_gpio_xlate(struct gpio_chip *gc, struct device_node *np,
+			 const void *gpio_spec, u32 *flags)
+{
+	const __be32 *gpio = gpio_spec;
+	const u32 n = be32_to_cpup(gpio);
+	unsigned int pin = gc->base + be32_to_cpu(gpio[0]);
+
+	if (gc->of_gpio_n_cells < 4) {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	if (n > gc->ngpio)
+		return -EINVAL;
+
+	s3c_gpio_cfgpin(pin, S3C_GPIO_SFN(be32_to_cpu(gpio[1])));
+	s3c_gpio_setpull(pin, be32_to_cpu(gpio[2]));
+	s5p_gpio_set_drvstr(pin, be32_to_cpu(gpio[3]));
+	return n;
+}
+
+static __init void exynos4_gpiolib_attach_ofnode(struct gpio_chip *gc)
+{
+	const char exynos4_gpio_compat_base[] = "samsung,exynos4-gpio-";
+	char *exynos4_gpio_compat;
+
+	exynos4_gpio_compat = kzalloc(strlen(exynos4_gpio_compat_base) +
+				strlen(gc->label), GFP_KERNEL);
+	if (!exynos4_gpio_compat)
+		return;
+
+	strcpy(exynos4_gpio_compat, exynos4_gpio_compat_base);
+	strcat(exynos4_gpio_compat, gc->label);
+	gc->of_node = of_find_compatible_node(NULL, NULL, exynos4_gpio_compat);
+	gc->of_gpio_n_cells = 4;
+	gc->of_xlate = exynos4_gpio_xlate;
+	kfree(exynos4_gpio_compat);
+}
+#else
+static __init void exynos4_gpiolib_attach_ofnode(struct gpio_chip *chip)
+{
+	return;
+}
+#endif
+
 static __init int exynos4_gpiolib_init(void)
 {
 	struct s3c_gpio_chip *chip;
@@ -340,6 +390,7 @@ static __init int exynos4_gpiolib_init(void)
 		}
 		if (chip->base == NULL)
 			chip->base = S5P_VA_GPIO1 + (i) * 0x20;
+		exynos4_gpiolib_attach_ofnode(&chip->chip);
 	}
 
 	samsung_gpiolib_add_4bit_chips(exynos4_gpio_part1_4bit, nr_chips);
@@ -357,6 +408,7 @@ static __init int exynos4_gpiolib_init(void)
 		}
 		if (chip->base == NULL)
 			chip->base = S5P_VA_GPIO2 + (i) * 0x20;
+		exynos4_gpiolib_attach_ofnode(&chip->chip);
 	}
 
 	samsung_gpiolib_add_4bit_chips(exynos4_gpio_part2_4bit, nr_chips);
@@ -374,6 +426,7 @@ static __init int exynos4_gpiolib_init(void)
 		}
 		if (chip->base == NULL)
 			chip->base = S5P_VA_GPIO3 + (i) * 0x20;
+		exynos4_gpiolib_attach_ofnode(&chip->chip);
 	}
 
 	samsung_gpiolib_add_4bit_chips(exynos4_gpio_part3_4bit, nr_chips);
-- 
1.6.6.rc2

[PATCH] gpio: exynos4: Add device tree support

[Thomas Abraham]

As gpio chips get registered, a device tree node which represents the
gpio chip is searched and attached to it. A translate function is also
provided to convert the gpio specifier into actual platform settings
for pin function selection, pull up/down and driver strength settings.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
This patch is based on the latest consolidated Samsung GPIO driver available
in the following tree:
  https://github.com/kgene/linux-samsung.git  branch: for-next

 .../devicetree/bindings/gpio/gpio-samsung.txt      |   30 +++++++++++
 drivers/gpio/gpio-samsung.c                        |   53 ++++++++++++++++++++
 2 files changed, 83 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-samsung.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
new file mode 100644
index 0000000..883faeb
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
@@ -0,0 +1,30 @@
+Samsung Exynos4 GPIO Controller
+
+Required properties:
+- compatible: Format of compatible property value should be
+  "samsung,exynos4-gpio-<controller_name>". Example: For GPA0 controller, the
+  compatible property value should be "samsung,exynos4-gpio-gpa0".
+
+- reg: Physical base address of the controller and length of memory mapped region.
+
+- #gpio-cells: Should be 4. The syntax of the gpio specifier used by client nodes
+  should be the following with values derived from the SoC user manual.
+     <[phandle of the gpio controller node] <pin number within the gpio controller]
+      [mux function] [pull up/down] [drive strength]>
+
+- gpio-controller: Specifies that the node is a gpio controller.
+
+- #address-cells: should be 1.
+
+- #size-cells: should be 1. 
+
+Example:
+
+	gpa0: gpio-controller@11400000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "samsung,exynos4-gpio-gpa0";
+		reg = <0x11400000 0x20>;
+		#gpio-cells = <4>;
+		gpio-controller;
+	};
diff --git a/drivers/gpio/gpio-samsung.c b/drivers/gpio/gpio-samsung.c
index b6be77a..037d3bb 100644
--- a/drivers/gpio/gpio-samsung.c
+++ b/drivers/gpio/gpio-samsung.c
@@ -24,6 +24,10 @@
 #include <linux/interrupt.h>
 #include <linux/sysdev.h>
 #include <linux/ioport.h>
+#ifdef CONFIG_OF
+#include <linux/of.h>
+#include <linux/slab.h>
+#endif
 
 #include <asm/irq.h>
 
@@ -2353,6 +2357,52 @@ static struct samsung_gpio_chip exynos4_gpios_3[] = {
 #endif
 };
 
+#if defined(CONFIG_ARCH_EXYNOS4) && defined(CONFIG_OF)
+int exynos4_gpio_xlate(struct gpio_chip *gc, struct device_node *np,
+			 const void *gpio_spec, u32 *flags)
+{
+	const __be32 *gpio = gpio_spec;
+	const u32 n = be32_to_cpup(gpio);
+	unsigned int pin = gc->base + be32_to_cpu(gpio[0]);
+
+	if (gc->of_gpio_n_cells < 4) {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	if (n > gc->ngpio)
+		return -EINVAL;
+
+	s3c_gpio_cfgpin(pin, S3C_GPIO_SFN(be32_to_cpu(gpio[1])));
+	s3c_gpio_setpull(pin, be32_to_cpu(gpio[2]));
+	s5p_gpio_set_drvstr(pin, be32_to_cpu(gpio[3]));
+	return n;
+}
+
+static __init void exynos4_gpiolib_attach_ofnode(struct gpio_chip *gc)
+{
+	const char exynos4_gpio_compat_base[] = "samsung,exynos4-gpio-";
+	char *exynos4_gpio_compat;
+
+	exynos4_gpio_compat = kzalloc(strlen(exynos4_gpio_compat_base) +
+				strlen(gc->label), GFP_KERNEL);
+	if (!exynos4_gpio_compat)
+		return;
+
+	strcpy(exynos4_gpio_compat, exynos4_gpio_compat_base);
+	strcat(exynos4_gpio_compat, gc->label);
+	gc->of_node = of_find_compatible_node(NULL, NULL, exynos4_gpio_compat);
+	gc->of_gpio_n_cells = 4;
+	gc->of_xlate = exynos4_gpio_xlate;
+	kfree(exynos4_gpio_compat);
+}
+#else
+static __init void exynos4_gpiolib_attach_ofnode(struct gpio_chip *chip)
+{
+	return;
+}
+#endif /* defined(CONFIG_ARCH_EXYNOS4) && defined(CONFIG_OF) */
+
 /* TODO: cleanup soc_is_* */
 static __init int samsung_gpiolib_init(void)
 {
@@ -2434,6 +2484,7 @@ static __init int samsung_gpiolib_init(void)
 				chip->config = &exynos4_gpio_cfg;
 				chip->group = group++;
 			}
+			exynos4_gpiolib_attach_ofnode(&chip->chip);
 		}
 		samsung_gpiolib_add_4bit_chips(exynos4_gpios_1, nr_chips, S5P_VA_GPIO1);
 
@@ -2446,6 +2497,7 @@ static __init int samsung_gpiolib_init(void)
 				chip->config = &exynos4_gpio_cfg;
 				chip->group = group++;
 			}
+			exynos4_gpiolib_attach_ofnode(&chip->chip);
 		}
 		samsung_gpiolib_add_4bit_chips(exynos4_gpios_2, nr_chips, S5P_VA_GPIO2);
 
@@ -2458,6 +2510,7 @@ static __init int samsung_gpiolib_init(void)
 				chip->config = &exynos4_gpio_cfg;
 				chip->group = group++;
 			}
+			exynos4_gpiolib_attach_ofnode(&chip->chip);
 		}
 		samsung_gpiolib_add_4bit_chips(exynos4_gpios_3, nr_chips, S5P_VA_GPIO3);
 
-- 
1.6.6.rc2

Re: [PATCH] gpio: exynos4: Add device tree support

[Rob Herring]

Thomas,

On 10/11/2011 03:16 AM, Thomas Abraham wrote:
> As gpio chips get registered, a device tree node which represents the
> gpio chip is searched and attached to it. A translate function is also
> provided to convert the gpio specifier into actual platform settings
> for pin function selection, pull up/down and driver strength settings.
> 
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
> This patch is based on the latest consolidated Samsung GPIO driver available
> in the following tree:
>   https://github.com/kgene/linux-samsung.git  branch: for-next
> 
>  .../devicetree/bindings/gpio/gpio-samsung.txt      |   30 +++++++++++
>  drivers/gpio/gpio-samsung.c                        |   53 ++++++++++++++++++++
>  2 files changed, 83 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> new file mode 100644
> index 0000000..883faeb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> @@ -0,0 +1,30 @@
> +Samsung Exynos4 GPIO Controller
> +
> +Required properties:
> +- compatible: Format of compatible property value should be
> +  "samsung,exynos4-gpio-<controller_name>". Example: For GPA0 controller, the
> +  compatible property value should be "samsung,exynos4-gpio-gpa0".

Isn't gpa0 an instance of the h/w, not a version?

> +
> +- reg: Physical base address of the controller and length of memory mapped region.
> +
> +- #gpio-cells: Should be 4. The syntax of the gpio specifier used by client nodes
> +  should be the following with values derived from the SoC user manual.
> +     <[phandle of the gpio controller node] <pin number within the gpio controller]
> +      [mux function] [pull up/down] [drive strength]>

It would be better to list out the values here.

> +
> +- gpio-controller: Specifies that the node is a gpio controller.
> +
> +- #address-cells: should be 1.
> +
> +- #size-cells: should be 1. 
> +
> +Example:
> +
> +	gpa0: gpio-controller@11400000 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "samsung,exynos4-gpio-gpa0";
> +		reg = <0x11400000 0x20>;
> +		#gpio-cells = <4>;
> +		gpio-controller;
> +	};
> diff --git a/drivers/gpio/gpio-samsung.c b/drivers/gpio/gpio-samsung.c
> index b6be77a..037d3bb 100644
> --- a/drivers/gpio/gpio-samsung.c
> +++ b/drivers/gpio/gpio-samsung.c
> @@ -24,6 +24,10 @@
>  #include <linux/interrupt.h>
>  #include <linux/sysdev.h>
>  #include <linux/ioport.h>
> +#ifdef CONFIG_OF
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#endif

Don't need the ifdef here.

>  
>  #include <asm/irq.h>
>  
> @@ -2353,6 +2357,52 @@ static struct samsung_gpio_chip exynos4_gpios_3[] = {
>  #endif
>  };
>  
> +#if defined(CONFIG_ARCH_EXYNOS4) && defined(CONFIG_OF)
> +int exynos4_gpio_xlate(struct gpio_chip *gc, struct device_node *np,

static

> +			 const void *gpio_spec, u32 *flags)
> +{
> +	const __be32 *gpio = gpio_spec;
> +	const u32 n = be32_to_cpup(gpio);
> +	unsigned int pin = gc->base + be32_to_cpu(gpio[0]);
> +
> +	if (gc->of_gpio_n_cells < 4) {
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	if (n > gc->ngpio)
> +		return -EINVAL;
> +
> +	s3c_gpio_cfgpin(pin, S3C_GPIO_SFN(be32_to_cpu(gpio[1])));
> +	s3c_gpio_setpull(pin, be32_to_cpu(gpio[2]));
> +	s5p_gpio_set_drvstr(pin, be32_to_cpu(gpio[3]));
> +	return n;
> +}
> +
> +static __init void exynos4_gpiolib_attach_ofnode(struct gpio_chip *gc)
> +{
> +	const char exynos4_gpio_compat_base[] = "samsung,exynos4-gpio-";
> +	char *exynos4_gpio_compat;
> +
> +	exynos4_gpio_compat = kzalloc(strlen(exynos4_gpio_compat_base) +
> +				strlen(gc->label), GFP_KERNEL);
> +	if (!exynos4_gpio_compat)
> +		return;
> +
> +	strcpy(exynos4_gpio_compat, exynos4_gpio_compat_base);
> +	strcat(exynos4_gpio_compat, gc->label);
> +	gc->of_node = of_find_compatible_node(NULL, NULL, exynos4_gpio_compat);
> +	gc->of_gpio_n_cells = 4;
> +	gc->of_xlate = exynos4_gpio_xlate;
> +	kfree(exynos4_gpio_compat);
> +}
> +#else
> +static __init void exynos4_gpiolib_attach_ofnode(struct gpio_chip *chip)
> +{
> +	return;
> +}
> +#endif /* defined(CONFIG_ARCH_EXYNOS4) && defined(CONFIG_OF) */
> +
>  /* TODO: cleanup soc_is_* */
>  static __init int samsung_gpiolib_init(void)
>  {
> @@ -2434,6 +2484,7 @@ static __init int samsung_gpiolib_init(void)
>  				chip->config = &exynos4_gpio_cfg;
>  				chip->group = group++;
>  			}
> +			exynos4_gpiolib_attach_ofnode(&chip->chip);
>  		}
>  		samsung_gpiolib_add_4bit_chips(exynos4_gpios_1, nr_chips, S5P_VA_GPIO1);
>  
> @@ -2446,6 +2497,7 @@ static __init int samsung_gpiolib_init(void)
>  				chip->config = &exynos4_gpio_cfg;
>  				chip->group = group++;
>  			}
> +			exynos4_gpiolib_attach_ofnode(&chip->chip);
>  		}
>  		samsung_gpiolib_add_4bit_chips(exynos4_gpios_2, nr_chips, S5P_VA_GPIO2);
>  
> @@ -2458,6 +2510,7 @@ static __init int samsung_gpiolib_init(void)
>  				chip->config = &exynos4_gpio_cfg;
>  				chip->group = group++;
>  			}
> +			exynos4_gpiolib_attach_ofnode(&chip->chip);
>  		}
>  		samsung_gpiolib_add_4bit_chips(exynos4_gpios_3, nr_chips, S5P_VA_GPIO3);
>  

This code is really ugly, but I guess you inherited it. Converting to a
platform driver and using id table would be much cleaner.

Rob

Re: [PATCH] gpio: exynos4: Add device tree support

[Thomas Abraham]

Hi Rob,

On 11 October 2011 20:41, Rob Herring <robherring2@gmail.com> wrote:
> Thomas,
>
> On 10/11/2011 03:16 AM, Thomas Abraham wrote:
>> As gpio chips get registered, a device tree node which represents the
>> gpio chip is searched and attached to it. A translate function is also
>> provided to convert the gpio specifier into actual platform settings
>> for pin function selection, pull up/down and driver strength settings.
>>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>> This patch is based on the latest consolidated Samsung GPIO driver available
>> in the following tree:
>>   https://github.com/kgene/linux-samsung.git  branch: for-next
>>
>>  .../devicetree/bindings/gpio/gpio-samsung.txt      |   30 +++++++++++
>>  drivers/gpio/gpio-samsung.c                        |   53 ++++++++++++++++++++
>>  2 files changed, 83 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>> new file mode 100644
>> index 0000000..883faeb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>> @@ -0,0 +1,30 @@
>> +Samsung Exynos4 GPIO Controller
>> +
>> +Required properties:
>> +- compatible: Format of compatible property value should be
>> +  "samsung,exynos4-gpio-<controller_name>". Example: For GPA0 controller, the
>> +  compatible property value should be "samsung,exynos4-gpio-gpa0".
>
> Isn't gpa0 an instance of the h/w, not a version?

GPA0 is a instance of the gpio controller. There are several such
instances and there could be differences in those instances such as
the number of GPIO lines managed by that gpio controller instance.

>
>> +
>> +- reg: Physical base address of the controller and length of memory mapped region.
>> +
>> +- #gpio-cells: Should be 4. The syntax of the gpio specifier used by client nodes
>> +  should be the following with values derived from the SoC user manual.
>> +     <[phandle of the gpio controller node] <pin number within the gpio controller]
>> +      [mux function] [pull up/down] [drive strength]>
>
> It would be better to list out the values here.

Ok. I will do that in the next version of the patch.

>
>> +
>> +- gpio-controller: Specifies that the node is a gpio controller.
>> +
>> +- #address-cells: should be 1.
>> +
>> +- #size-cells: should be 1.
>> +
>> +Example:
>> +
>> +     gpa0: gpio-controller@11400000 {
>> +             #address-cells = <1>;
>> +             #size-cells = <1>;
>> +             compatible = "samsung,exynos4-gpio-gpa0";
>> +             reg = <0x11400000 0x20>;
>> +             #gpio-cells = <4>;
>> +             gpio-controller;
>> +     };
>> diff --git a/drivers/gpio/gpio-samsung.c b/drivers/gpio/gpio-samsung.c
>> index b6be77a..037d3bb 100644
>> --- a/drivers/gpio/gpio-samsung.c
>> +++ b/drivers/gpio/gpio-samsung.c
>> @@ -24,6 +24,10 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/sysdev.h>
>>  #include <linux/ioport.h>
>> +#ifdef CONFIG_OF
>> +#include <linux/of.h>
>> +#include <linux/slab.h>
>> +#endif
>
> Don't need the ifdef here.

Ok. I will remove it.

>
>>
>>  #include <asm/irq.h>
>>
>> @@ -2353,6 +2357,52 @@ static struct samsung_gpio_chip exynos4_gpios_3[] = {
>>  #endif
>>  };
>>
>> +#if defined(CONFIG_ARCH_EXYNOS4) && defined(CONFIG_OF)
>> +int exynos4_gpio_xlate(struct gpio_chip *gc, struct device_node *np,
>
> static

Ok. I will add it.

>
>> +                      const void *gpio_spec, u32 *flags)
>> +{
>> +     const __be32 *gpio = gpio_spec;
>> +     const u32 n = be32_to_cpup(gpio);
>> +     unsigned int pin = gc->base + be32_to_cpu(gpio[0]);
>> +
>> +     if (gc->of_gpio_n_cells < 4) {
>> +             WARN_ON(1);
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (n > gc->ngpio)
>> +             return -EINVAL;
>> +
>> +     s3c_gpio_cfgpin(pin, S3C_GPIO_SFN(be32_to_cpu(gpio[1])));
>> +     s3c_gpio_setpull(pin, be32_to_cpu(gpio[2]));
>> +     s5p_gpio_set_drvstr(pin, be32_to_cpu(gpio[3]));
>> +     return n;
>> +}
>> +
>> +static __init void exynos4_gpiolib_attach_ofnode(struct gpio_chip *gc)
>> +{
>> +     const char exynos4_gpio_compat_base[] = "samsung,exynos4-gpio-";
>> +     char *exynos4_gpio_compat;
>> +
>> +     exynos4_gpio_compat = kzalloc(strlen(exynos4_gpio_compat_base) +
>> +                             strlen(gc->label), GFP_KERNEL);
>> +     if (!exynos4_gpio_compat)
>> +             return;
>> +
>> +     strcpy(exynos4_gpio_compat, exynos4_gpio_compat_base);
>> +     strcat(exynos4_gpio_compat, gc->label);
>> +     gc->of_node = of_find_compatible_node(NULL, NULL, exynos4_gpio_compat);
>> +     gc->of_gpio_n_cells = 4;
>> +     gc->of_xlate = exynos4_gpio_xlate;
>> +     kfree(exynos4_gpio_compat);
>> +}
>> +#else
>> +static __init void exynos4_gpiolib_attach_ofnode(struct gpio_chip *chip)
>> +{
>> +     return;
>> +}
>> +#endif /* defined(CONFIG_ARCH_EXYNOS4) && defined(CONFIG_OF) */
>> +
>>  /* TODO: cleanup soc_is_* */
>>  static __init int samsung_gpiolib_init(void)
>>  {
>> @@ -2434,6 +2484,7 @@ static __init int samsung_gpiolib_init(void)
>>                               chip->config = &exynos4_gpio_cfg;
>>                               chip->group = group++;
>>                       }
>> +                     exynos4_gpiolib_attach_ofnode(&chip->chip);
>>               }
>>               samsung_gpiolib_add_4bit_chips(exynos4_gpios_1, nr_chips, S5P_VA_GPIO1);
>>
>> @@ -2446,6 +2497,7 @@ static __init int samsung_gpiolib_init(void)
>>                               chip->config = &exynos4_gpio_cfg;
>>                               chip->group = group++;
>>                       }
>> +                     exynos4_gpiolib_attach_ofnode(&chip->chip);
>>               }
>>               samsung_gpiolib_add_4bit_chips(exynos4_gpios_2, nr_chips, S5P_VA_GPIO2);
>>
>> @@ -2458,6 +2510,7 @@ static __init int samsung_gpiolib_init(void)
>>                               chip->config = &exynos4_gpio_cfg;
>>                               chip->group = group++;
>>                       }
>> +                     exynos4_gpiolib_attach_ofnode(&chip->chip);
>>               }
>>               samsung_gpiolib_add_4bit_chips(exynos4_gpios_3, nr_chips, S5P_VA_GPIO3);
>>
>
> This code is really ugly, but I guess you inherited it. Converting to a
> platform driver and using id table would be much cleaner.

Ok. I wanted to get the initial dt support for gpio controller on
Exynos started because dt support for other controllers depend on
this. I will revisit this as per your suggestion during the 3.2 cycle.

Thanks for reviewing this patch.

Regards,
Thomas

>
> Rob
>

Re: [PATCH] gpio: exynos4: Add device tree support

[Rob Herring]

On 10/11/2011 10:19 AM, Thomas Abraham wrote:
> Hi Rob,
> 
> On 11 October 2011 20:41, Rob Herring <robherring2@gmail.com> wrote:
>> Thomas,
>>
>> On 10/11/2011 03:16 AM, Thomas Abraham wrote:
>>> As gpio chips get registered, a device tree node which represents the
>>> gpio chip is searched and attached to it. A translate function is also
>>> provided to convert the gpio specifier into actual platform settings
>>> for pin function selection, pull up/down and driver strength settings.
>>>
>>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>>> ---
>>> This patch is based on the latest consolidated Samsung GPIO driver available
>>> in the following tree:
>>>   https://github.com/kgene/linux-samsung.git  branch: for-next
>>>
>>>  .../devicetree/bindings/gpio/gpio-samsung.txt      |   30 +++++++++++
>>>  drivers/gpio/gpio-samsung.c                        |   53 ++++++++++++++++++++
>>>  2 files changed, 83 insertions(+), 0 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>> new file mode 100644
>>> index 0000000..883faeb
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>> @@ -0,0 +1,30 @@
>>> +Samsung Exynos4 GPIO Controller
>>> +
>>> +Required properties:
>>> +- compatible: Format of compatible property value should be
>>> +  "samsung,exynos4-gpio-<controller_name>". Example: For GPA0 controller, the
>>> +  compatible property value should be "samsung,exynos4-gpio-gpa0".
>>
>> Isn't gpa0 an instance of the h/w, not a version?
> 
> GPA0 is a instance of the gpio controller. There are several such
> instances and there could be differences in those instances such as
> the number of GPIO lines managed by that gpio controller instance.
> 

That doesn't seem like a reason to have different compatible strings.
Does that affect the programming model of the controller? Unused lines
whether at the board level or SOC level shouldn't really matter.

Rob

Re: [PATCH] gpio: exynos4: Add device tree support

[Thomas Abraham]

On 11 October 2011 21:00, Rob Herring <robherring2@gmail.com> wrote:
> On 10/11/2011 10:19 AM, Thomas Abraham wrote:
>> Hi Rob,
>>
>> On 11 October 2011 20:41, Rob Herring <robherring2@gmail.com> wrote:
>>> Thomas,
>>>
>>> On 10/11/2011 03:16 AM, Thomas Abraham wrote:
>>>> As gpio chips get registered, a device tree node which represents the
>>>> gpio chip is searched and attached to it. A translate function is also
>>>> provided to convert the gpio specifier into actual platform settings
>>>> for pin function selection, pull up/down and driver strength settings.
>>>>
>>>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>>>> ---
>>>> This patch is based on the latest consolidated Samsung GPIO driver available
>>>> in the following tree:
>>>>   https://github.com/kgene/linux-samsung.git  branch: for-next
>>>>
>>>>  .../devicetree/bindings/gpio/gpio-samsung.txt      |   30 +++++++++++
>>>>  drivers/gpio/gpio-samsung.c                        |   53 ++++++++++++++++++++
>>>>  2 files changed, 83 insertions(+), 0 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>>> new file mode 100644
>>>> index 0000000..883faeb
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>>> @@ -0,0 +1,30 @@
>>>> +Samsung Exynos4 GPIO Controller
>>>> +
>>>> +Required properties:
>>>> +- compatible: Format of compatible property value should be
>>>> +  "samsung,exynos4-gpio-<controller_name>". Example: For GPA0 controller, the
>>>> +  compatible property value should be "samsung,exynos4-gpio-gpa0".
>>>
>>> Isn't gpa0 an instance of the h/w, not a version?
>>
>> GPA0 is a instance of the gpio controller. There are several such
>> instances and there could be differences in those instances such as
>> the number of GPIO lines managed by that gpio controller instance.
>>
>
> That doesn't seem like a reason to have different compatible strings.
> Does that affect the programming model of the controller? Unused lines
> whether at the board level or SOC level shouldn't really matter.


No, that does not affect the programming of the controller. The reason
for the instance name extension in compatible string is to match the
gpio_chip with a gpio controller node. When matched, the of_node
pointer of the gpio_chip is set to point to that controller node.

This might not be the best possible implementation but the device tree
support code in this patch is dictated by the structure of the
existing gpio driver code. As you suggested previously, I will look at
reworking the gpio driver a little later but for now, there was a need
for working gpio dt solution to make progress on dt support for other
controllers.

Thanks,
Thomas.

>
> Rob
>
>

Re: [PATCH] gpio: exynos4: Add device tree support

[Rob Herring]

On 10/11/2011 11:06 AM, Thomas Abraham wrote:
> On 11 October 2011 21:00, Rob Herring <robherring2@gmail.com> wrote:
>> On 10/11/2011 10:19 AM, Thomas Abraham wrote:
>>> Hi Rob,
>>>
>>> On 11 October 2011 20:41, Rob Herring <robherring2@gmail.com> wrote:
>>>> Thomas,
>>>>
>>>> On 10/11/2011 03:16 AM, Thomas Abraham wrote:
>>>>> As gpio chips get registered, a device tree node which represents the
>>>>> gpio chip is searched and attached to it. A translate function is also
>>>>> provided to convert the gpio specifier into actual platform settings
>>>>> for pin function selection, pull up/down and driver strength settings.
>>>>>
>>>>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>>>>> ---
>>>>> This patch is based on the latest consolidated Samsung GPIO driver available
>>>>> in the following tree:
>>>>>   https://github.com/kgene/linux-samsung.git  branch: for-next
>>>>>
>>>>>  .../devicetree/bindings/gpio/gpio-samsung.txt      |   30 +++++++++++
>>>>>  drivers/gpio/gpio-samsung.c                        |   53 ++++++++++++++++++++
>>>>>  2 files changed, 83 insertions(+), 0 deletions(-)
>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>>>> new file mode 100644
>>>>> index 0000000..883faeb
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>>>> @@ -0,0 +1,30 @@
>>>>> +Samsung Exynos4 GPIO Controller
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: Format of compatible property value should be
>>>>> +  "samsung,exynos4-gpio-<controller_name>". Example: For GPA0 controller, the
>>>>> +  compatible property value should be "samsung,exynos4-gpio-gpa0".
>>>>
>>>> Isn't gpa0 an instance of the h/w, not a version?
>>>
>>> GPA0 is a instance of the gpio controller. There are several such
>>> instances and there could be differences in those instances such as
>>> the number of GPIO lines managed by that gpio controller instance.
>>>
>>
>> That doesn't seem like a reason to have different compatible strings.
>> Does that affect the programming model of the controller? Unused lines
>> whether at the board level or SOC level shouldn't really matter.
> 
> 
> No, that does not affect the programming of the controller. The reason
> for the instance name extension in compatible string is to match the
> gpio_chip with a gpio controller node. When matched, the of_node
> pointer of the gpio_chip is set to point to that controller node.
> 
> This might not be the best possible implementation but the device tree
> support code in this patch is dictated by the structure of the
> existing gpio driver code. As you suggested previously, I will look at
> reworking the gpio driver a little later but for now, there was a need
> for working gpio dt solution to make progress on dt support for other
> controllers.

Linux should provide clues about what's needed in a binding, but the
binding should not be defined based on current Linux code. Doing the
binding one way and changing it later is not a good plan.

I think you need to convert all users of gpio over as well so you can
move to dynamic gpio_chip creation and gpio numbering. Or maybe you can
match based on base address? This is going to be a common problem as
gpio is converted over to DT. Perhaps Grant or others have suggestions
on the approach to use.

Rob

Re: [PATCH] gpio: exynos4: Add device tree support

[Thomas Abraham]

On 12 October 2011 20:41, Rob Herring <robherring2@gmail.com> wrote:
> On 10/11/2011 11:06 AM, Thomas Abraham wrote:
>> On 11 October 2011 21:00, Rob Herring <robherring2@gmail.com> wrote:
>>> On 10/11/2011 10:19 AM, Thomas Abraham wrote:
>>>> Hi Rob,
>>>>
>>>> On 11 October 2011 20:41, Rob Herring <robherring2@gmail.com> wrote:
>>>>> Thomas,
>>>>>
>>>>> On 10/11/2011 03:16 AM, Thomas Abraham wrote:
>>>>>> As gpio chips get registered, a device tree node which represents the
>>>>>> gpio chip is searched and attached to it. A translate function is also
>>>>>> provided to convert the gpio specifier into actual platform settings
>>>>>> for pin function selection, pull up/down and driver strength settings.
>>>>>>
>>>>>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>>>>>> ---
>>>>>> This patch is based on the latest consolidated Samsung GPIO driver available
>>>>>> in the following tree:
>>>>>>   https://github.com/kgene/linux-samsung.git  branch: for-next
>>>>>>
>>>>>>  .../devicetree/bindings/gpio/gpio-samsung.txt      |   30 +++++++++++
>>>>>>  drivers/gpio/gpio-samsung.c                        |   53 ++++++++++++++++++++
>>>>>>  2 files changed, 83 insertions(+), 0 deletions(-)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..883faeb
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>>>>> @@ -0,0 +1,30 @@
>>>>>> +Samsung Exynos4 GPIO Controller
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: Format of compatible property value should be
>>>>>> +  "samsung,exynos4-gpio-<controller_name>". Example: For GPA0 controller, the
>>>>>> +  compatible property value should be "samsung,exynos4-gpio-gpa0".
>>>>>
>>>>> Isn't gpa0 an instance of the h/w, not a version?
>>>>
>>>> GPA0 is a instance of the gpio controller. There are several such
>>>> instances and there could be differences in those instances such as
>>>> the number of GPIO lines managed by that gpio controller instance.
>>>>
>>>
>>> That doesn't seem like a reason to have different compatible strings.
>>> Does that affect the programming model of the controller? Unused lines
>>> whether at the board level or SOC level shouldn't really matter.
>>
>>
>> No, that does not affect the programming of the controller. The reason
>> for the instance name extension in compatible string is to match the
>> gpio_chip with a gpio controller node. When matched, the of_node
>> pointer of the gpio_chip is set to point to that controller node.
>>
>> This might not be the best possible implementation but the device tree
>> support code in this patch is dictated by the structure of the
>> existing gpio driver code. As you suggested previously, I will look at
>> reworking the gpio driver a little later but for now, there was a need
>> for working gpio dt solution to make progress on dt support for other
>> controllers.
>
> Linux should provide clues about what's needed in a binding, but the
> binding should not be defined based on current Linux code. Doing the
> binding one way and changing it later is not a good plan.

Ok. When starting on this, two compatible values where used for the
gpio controllers, one was "samsung,exynos4-gpio" and another was
"samsung,exynos4-gpio-<ctrl_name>". And when the gpio dt support would
mature, the second compatible value could be dropped. Non-linux
platforms could always use the generic "samsung,exynos4-gpio"
compatible value to match. I moved to using only
"samsung,exynos4-gpio-<ctrl_name>" just before sending this patch
because I was not sure of the right approach.

>
> I think you need to convert all users of gpio over as well so you can
> move to dynamic gpio_chip creation and gpio numbering. Or maybe you can
> match based on base address? This is going to be a common problem as
> gpio is converted over to DT. Perhaps Grant or others have suggestions
> on the approach to use.

Yes, I agree with you about the dynamic gpio_chip creation and gpio
numbering. Probably, I should have focussed more on this before moving
to dt support for other controllers.

But matching based on base address is still an option if that is
better than matching with compatible values as defined currently. The
'struct samsung_gpio_chip' which encapsulates 'struct gpio_chip' has
information about the base address of the gpio controller. The match
of gpio device node with 'struct gpio_chip' can be quickly moved over
to base address matching. Would this be better than the current
approach?

Thank you for your comments Rob.

Regards,
Thomas.

>
> Rob
>

Re: [PATCH] gpio: exynos4: Add device tree support

[Grant Likely]

On Wed, Oct 12, 2011 at 09:45:25PM +0530, Thomas Abraham wrote:
> On 12 October 2011 20:41, Rob Herring <robherring2@gmail.com> wrote:
> > On 10/11/2011 11:06 AM, Thomas Abraham wrote:
> >> On 11 October 2011 21:00, Rob Herring <robherring2@gmail.com> wrote:
> >>> On 10/11/2011 10:19 AM, Thomas Abraham wrote:
> >>>> Hi Rob,
> >>>>
> >>>> On 11 October 2011 20:41, Rob Herring <robherring2@gmail.com> wrote:
> >>>>> Thomas,
> >>>>>
> >>>>> On 10/11/2011 03:16 AM, Thomas Abraham wrote:
> >>>>>> As gpio chips get registered, a device tree node which represents the
> >>>>>> gpio chip is searched and attached to it. A translate function is also
> >>>>>> provided to convert the gpio specifier into actual platform settings
> >>>>>> for pin function selection, pull up/down and driver strength settings.
> >>>>>>
> >>>>>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> >>>>>> ---
> >>>>>> This patch is based on the latest consolidated Samsung GPIO driver available
> >>>>>> in the following tree:
> >>>>>>   https://github.com/kgene/linux-samsung.git  branch: for-next
> >>>>>>
> >>>>>>  .../devicetree/bindings/gpio/gpio-samsung.txt      |   30 +++++++++++
> >>>>>>  drivers/gpio/gpio-samsung.c                        |   53 ++++++++++++++++++++
> >>>>>>  2 files changed, 83 insertions(+), 0 deletions(-)
> >>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> >>>>>> new file mode 100644
> >>>>>> index 0000000..883faeb
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> >>>>>> @@ -0,0 +1,30 @@
> >>>>>> +Samsung Exynos4 GPIO Controller
> >>>>>> +
> >>>>>> +Required properties:
> >>>>>> +- compatible: Format of compatible property value should be
> >>>>>> +  "samsung,exynos4-gpio-<controller_name>". Example: For GPA0 controller, the
> >>>>>> +  compatible property value should be "samsung,exynos4-gpio-gpa0".
> >>>>>
> >>>>> Isn't gpa0 an instance of the h/w, not a version?
> >>>>
> >>>> GPA0 is a instance of the gpio controller. There are several such
> >>>> instances and there could be differences in those instances such as
> >>>> the number of GPIO lines managed by that gpio controller instance.
> >>>>
> >>>
> >>> That doesn't seem like a reason to have different compatible strings.
> >>> Does that affect the programming model of the controller? Unused lines
> >>> whether at the board level or SOC level shouldn't really matter.
> >>
> >>
> >> No, that does not affect the programming of the controller. The reason
> >> for the instance name extension in compatible string is to match the
> >> gpio_chip with a gpio controller node. When matched, the of_node
> >> pointer of the gpio_chip is set to point to that controller node.
> >>
> >> This might not be the best possible implementation but the device tree
> >> support code in this patch is dictated by the structure of the
> >> existing gpio driver code. As you suggested previously, I will look at
> >> reworking the gpio driver a little later but for now, there was a need
> >> for working gpio dt solution to make progress on dt support for other
> >> controllers.
> >
> > Linux should provide clues about what's needed in a binding, but the
> > binding should not be defined based on current Linux code. Doing the
> > binding one way and changing it later is not a good plan.
> 
> Ok. When starting on this, two compatible values where used for the
> gpio controllers, one was "samsung,exynos4-gpio" and another was
> "samsung,exynos4-gpio-<ctrl_name>". And when the gpio dt support would
> mature, the second compatible value could be dropped. Non-linux
> platforms could always use the generic "samsung,exynos4-gpio"
> compatible value to match. I moved to using only
> "samsung,exynos4-gpio-<ctrl_name>" just before sending this patch
> because I was not sure of the right approach.
> 
> >
> > I think you need to convert all users of gpio over as well so you can
> > move to dynamic gpio_chip creation and gpio numbering. Or maybe you can
> > match based on base address? This is going to be a common problem as
> > gpio is converted over to DT. Perhaps Grant or others have suggestions
> > on the approach to use.
> 
> Yes, I agree with you about the dynamic gpio_chip creation and gpio
> numbering. Probably, I should have focussed more on this before moving
> to dt support for other controllers.
> 
> But matching based on base address is still an option if that is
> better than matching with compatible values as defined currently. The
> 'struct samsung_gpio_chip' which encapsulates 'struct gpio_chip' has
> information about the base address of the gpio controller. The match
> of gpio device node with 'struct gpio_chip' can be quickly moved over
> to base address matching. Would this be better than the current
> approach?

That's what I would do.

g.

Re: [PATCH] gpio: exynos4: Add device tree support

[Thomas Abraham]

On 13 October 2011 06:31, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Oct 12, 2011 at 09:45:25PM +0530, Thomas Abraham wrote:
>> On 12 October 2011 20:41, Rob Herring <robherring2@gmail.com> wrote:
>> > On 10/11/2011 11:06 AM, Thomas Abraham wrote:
>> >> On 11 October 2011 21:00, Rob Herring <robherring2@gmail.com> wrote:
>> >>> On 10/11/2011 10:19 AM, Thomas Abraham wrote:
>> >>>> Hi Rob,
>> >>>>
>> >>>> On 11 October 2011 20:41, Rob Herring <robherring2@gmail.com> wrote:
>> >>>>> Thomas,
>> >>>>>
>> >>>>> On 10/11/2011 03:16 AM, Thomas Abraham wrote:
>> >>>>>> As gpio chips get registered, a device tree node which represents the
>> >>>>>> gpio chip is searched and attached to it. A translate function is also
>> >>>>>> provided to convert the gpio specifier into actual platform settings
>> >>>>>> for pin function selection, pull up/down and driver strength settings.
>> >>>>>>
>> >>>>>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> >>>>>> ---
>> >>>>>> This patch is based on the latest consolidated Samsung GPIO driver available
>> >>>>>> in the following tree:
>> >>>>>>   https://github.com/kgene/linux-samsung.git  branch: for-next
>> >>>>>>
>> >>>>>>  .../devicetree/bindings/gpio/gpio-samsung.txt      |   30 +++++++++++
>> >>>>>>  drivers/gpio/gpio-samsung.c                        |   53 ++++++++++++++++++++
>> >>>>>>  2 files changed, 83 insertions(+), 0 deletions(-)
>> >>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>> >>>>>>
>> >>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>> >>>>>> new file mode 100644
>> >>>>>> index 0000000..883faeb
>> >>>>>> --- /dev/null
>> >>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>> >>>>>> @@ -0,0 +1,30 @@
>> >>>>>> +Samsung Exynos4 GPIO Controller
>> >>>>>> +
>> >>>>>> +Required properties:
>> >>>>>> +- compatible: Format of compatible property value should be
>> >>>>>> +  "samsung,exynos4-gpio-<controller_name>". Example: For GPA0 controller, the
>> >>>>>> +  compatible property value should be "samsung,exynos4-gpio-gpa0".
>> >>>>>
>> >>>>> Isn't gpa0 an instance of the h/w, not a version?
>> >>>>
>> >>>> GPA0 is a instance of the gpio controller. There are several such
>> >>>> instances and there could be differences in those instances such as
>> >>>> the number of GPIO lines managed by that gpio controller instance.
>> >>>>
>> >>>
>> >>> That doesn't seem like a reason to have different compatible strings.
>> >>> Does that affect the programming model of the controller? Unused lines
>> >>> whether at the board level or SOC level shouldn't really matter.
>> >>
>> >>
>> >> No, that does not affect the programming of the controller. The reason
>> >> for the instance name extension in compatible string is to match the
>> >> gpio_chip with a gpio controller node. When matched, the of_node
>> >> pointer of the gpio_chip is set to point to that controller node.
>> >>
>> >> This might not be the best possible implementation but the device tree
>> >> support code in this patch is dictated by the structure of the
>> >> existing gpio driver code. As you suggested previously, I will look at
>> >> reworking the gpio driver a little later but for now, there was a need
>> >> for working gpio dt solution to make progress on dt support for other
>> >> controllers.
>> >
>> > Linux should provide clues about what's needed in a binding, but the
>> > binding should not be defined based on current Linux code. Doing the
>> > binding one way and changing it later is not a good plan.
>>
>> Ok. When starting on this, two compatible values where used for the
>> gpio controllers, one was "samsung,exynos4-gpio" and another was
>> "samsung,exynos4-gpio-<ctrl_name>". And when the gpio dt support would
>> mature, the second compatible value could be dropped. Non-linux
>> platforms could always use the generic "samsung,exynos4-gpio"
>> compatible value to match. I moved to using only
>> "samsung,exynos4-gpio-<ctrl_name>" just before sending this patch
>> because I was not sure of the right approach.
>>
>> >
>> > I think you need to convert all users of gpio over as well so you can
>> > move to dynamic gpio_chip creation and gpio numbering. Or maybe you can
>> > match based on base address? This is going to be a common problem as
>> > gpio is converted over to DT. Perhaps Grant or others have suggestions
>> > on the approach to use.
>>
>> Yes, I agree with you about the dynamic gpio_chip creation and gpio
>> numbering. Probably, I should have focussed more on this before moving
>> to dt support for other controllers.
>>
>> But matching based on base address is still an option if that is
>> better than matching with compatible values as defined currently. The
>> 'struct samsung_gpio_chip' which encapsulates 'struct gpio_chip' has
>> information about the base address of the gpio controller. The match
>> of gpio device node with 'struct gpio_chip' can be quickly moved over
>> to base address matching. Would this be better than the current
>> approach?
>
> That's what I would do.

Thanks Grant. I will modify the patch to match against the base
address of the controller.

Regards,
Thomas.

>
> g.
>
>

RE: [PATCH] gpio: exynos4: Add device tree support

[Kukjin Kim]

Thomas Abraham wrote:
> 
> As gpio chips get registered, a device tree node which represents the
> gpio chip is searched and attached to it. A translate function is also
> provided to convert the gpio specifier into actual platform settings
> for pin function selection, pull up/down and driver strength settings.
> 
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
> This patch is based on the latest consolidated Samsung GPIO driver
available
> in the following tree:
>   https://github.com/kgene/linux-samsung.git  branch: for-next
> 
>  .../devicetree/bindings/gpio/gpio-samsung.txt      |   30 +++++++++++
>  drivers/gpio/gpio-samsung.c                        |   53
> ++++++++++++++++++++
>  2 files changed, 83 insertions(+), 0 deletions(-)
>  create mode 100644
Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> new file mode 100644
> index 0000000..883faeb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> @@ -0,0 +1,30 @@
> +Samsung Exynos4 GPIO Controller
> +
> +Required properties:
> +- compatible: Format of compatible property value should be
> +  "samsung,exynos4-gpio-<controller_name>". Example: For GPA0 controller,
> the
> +  compatible property value should be "samsung,exynos4-gpio-gpa0".
> +
> +- reg: Physical base address of the controller and length of memory
mapped
> region.
> +
> +- #gpio-cells: Should be 4. The syntax of the gpio specifier used by
client nodes
> +  should be the following with values derived from the SoC user manual.
> +     <[phandle of the gpio controller node] <pin number within the gpio
> controller]
> +      [mux function] [pull up/down] [drive strength]>
> +
> +- gpio-controller: Specifies that the node is a gpio controller.
> +
> +- #address-cells: should be 1.
> +
> +- #size-cells: should be 1.
> +
> +Example:
> +
> +	gpa0: gpio-controller@11400000 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "samsung,exynos4-gpio-gpa0";
> +		reg = <0x11400000 0x20>;
> +		#gpio-cells = <4>;
> +		gpio-controller;
> +	};
> diff --git a/drivers/gpio/gpio-samsung.c b/drivers/gpio/gpio-samsung.c
> index b6be77a..037d3bb 100644
> --- a/drivers/gpio/gpio-samsung.c
> +++ b/drivers/gpio/gpio-samsung.c
> @@ -24,6 +24,10 @@
>  #include <linux/interrupt.h>
>  #include <linux/sysdev.h>
>  #include <linux/ioport.h>
> +#ifdef CONFIG_OF
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#endif
> 
>  #include <asm/irq.h>
> 
> @@ -2353,6 +2357,52 @@ static struct samsung_gpio_chip exynos4_gpios_3[] =
> {
>  #endif
>  };
> 
> +#if defined(CONFIG_ARCH_EXYNOS4) && defined(CONFIG_OF)
> +int exynos4_gpio_xlate(struct gpio_chip *gc, struct device_node *np,
> +			 const void *gpio_spec, u32 *flags)
> +{
> +	const __be32 *gpio = gpio_spec;
> +	const u32 n = be32_to_cpup(gpio);
> +	unsigned int pin = gc->base + be32_to_cpu(gpio[0]);
> +
> +	if (gc->of_gpio_n_cells < 4) {
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	if (n > gc->ngpio)
> +		return -EINVAL;
> +
> +	s3c_gpio_cfgpin(pin, S3C_GPIO_SFN(be32_to_cpu(gpio[1])));
> +	s3c_gpio_setpull(pin, be32_to_cpu(gpio[2]));
> +	s5p_gpio_set_drvstr(pin, be32_to_cpu(gpio[3]));
> +	return n;
> +}
> +
> +static __init void exynos4_gpiolib_attach_ofnode(struct gpio_chip *gc)
> +{
> +	const char exynos4_gpio_compat_base[] = "samsung,exynos4-gpio-";
> +	char *exynos4_gpio_compat;
> +
> +	exynos4_gpio_compat = kzalloc(strlen(exynos4_gpio_compat_base) +
> +				strlen(gc->label), GFP_KERNEL);
> +	if (!exynos4_gpio_compat)
> +		return;
> +
> +	strcpy(exynos4_gpio_compat, exynos4_gpio_compat_base);
> +	strcat(exynos4_gpio_compat, gc->label);
> +	gc->of_node = of_find_compatible_node(NULL, NULL,
> exynos4_gpio_compat);
> +	gc->of_gpio_n_cells = 4;
> +	gc->of_xlate = exynos4_gpio_xlate;
> +	kfree(exynos4_gpio_compat);
> +}
> +#else
> +static __init void exynos4_gpiolib_attach_ofnode(struct gpio_chip *chip)
> +{
> +	return;
> +}
> +#endif /* defined(CONFIG_ARCH_EXYNOS4) && defined(CONFIG_OF) */
> +
>  /* TODO: cleanup soc_is_* */
>  static __init int samsung_gpiolib_init(void)
>  {
> @@ -2434,6 +2484,7 @@ static __init int samsung_gpiolib_init(void)
>  				chip->config = &exynos4_gpio_cfg;
>  				chip->group = group++;
>  			}
> +			exynos4_gpiolib_attach_ofnode(&chip->chip);
>  		}
>  		samsung_gpiolib_add_4bit_chips(exynos4_gpios_1, nr_chips,
> S5P_VA_GPIO1);
> 
> @@ -2446,6 +2497,7 @@ static __init int samsung_gpiolib_init(void)
>  				chip->config = &exynos4_gpio_cfg;
>  				chip->group = group++;
>  			}
> +			exynos4_gpiolib_attach_ofnode(&chip->chip);
>  		}
>  		samsung_gpiolib_add_4bit_chips(exynos4_gpios_2, nr_chips,
> S5P_VA_GPIO2);
> 
> @@ -2458,6 +2510,7 @@ static __init int samsung_gpiolib_init(void)
>  				chip->config = &exynos4_gpio_cfg;
>  				chip->group = group++;
>  			}
> +			exynos4_gpiolib_attach_ofnode(&chip->chip);
>  		}
>  		samsung_gpiolib_add_4bit_chips(exynos4_gpios_3, nr_chips,
> S5P_VA_GPIO3);
> 
> --
> 1.6.6.rc2

Hi Thomas,

You got some comments from Rob, so could you please address from him?
Others, looks ok to me.

And I need to get the ack from Grant to upstream via Samsung tree for
upcoming merge window.
Grant, is this ok for you?

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

Re: [PATCH] gpio: exynos4: Add device tree support

[Grant Likely]

On Wed, Oct 12, 2011 at 10:33:48PM +0900, Kukjin Kim wrote:
> Thomas Abraham wrote:
> > 
> > As gpio chips get registered, a device tree node which represents the
> > gpio chip is searched and attached to it. A translate function is also
> > provided to convert the gpio specifier into actual platform settings
> > for pin function selection, pull up/down and driver strength settings.
> > 
> > Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> 
> Hi Thomas,
> 
> You got some comments from Rob, so could you please address from him?
> Others, looks ok to me.
> 
> And I need to get the ack from Grant to upstream via Samsung tree for
> upcoming merge window.
> Grant, is this ok for you?

Yes, go ahead and merge it with my:

Acked-by: Grant Likely <grant.likely@secretlab.ca>

g.