[PATCH v2] gpio/samsung: Add device tree support for Exynos4

[PATCH v2] gpio/samsung: Add device tree support for Exynos4

[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>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
---
Changes since v1:
- As suggested by Rob and Grant, the gpio controller node lookup is based
  on the base address of the gpio controller instead of the unique
  per-controller compatible property value.

This patch is based on the following tree and branch.
git://git.linaro.org/git/people/arnd/arm-soc.git  branch: for-next

 .../devicetree/bindings/gpio/gpio-samsung.txt      |   40 ++++++++++++
 drivers/gpio/gpio-samsung.c                        |   66 ++++++++++++++++++++
 2 files changed, 106 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..c143058
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
@@ -0,0 +1,40 @@
+Samsung Exynos4 GPIO Controller
+
+Required properties:
+- compatible: Compatible property value should be "samsung,exynos4-gpio>".
+
+- 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]>
+
+  Values for gpio specifier:
+  - Pin number: is a value between 0 to 7.
+  - Pull Up/Down: 0 - Pull Up/Down Disabled.
+                  1 - Pull Down Enabled.
+                  3 - Pull Up Enabled.
+  - Drive Strength: 0 - 1x,
+                    1 - 3x,
+                    2 - 2x,
+                    3 - 4x
+
+- gpio-controller: Specifies that the node is a gpio controller.
+- #address-cells: should be 1.
+- #size-cells: should be 1. 
+
+Example:
+
+	gpa0: gpio-controller at 11400000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "samsung,exynos4-gpio";
+		reg = <0x11400000 0x20>;
+		#gpio-cells = <4>;
+		gpio-controller;
+	};
diff --git a/drivers/gpio/gpio-samsung.c b/drivers/gpio/gpio-samsung.c
index 8662518..0140756 100644
--- a/drivers/gpio/gpio-samsung.c
+++ b/drivers/gpio/gpio-samsung.c
@@ -24,6 +24,9 @@
 #include <linux/interrupt.h>
 #include <linux/sysdev.h>
 #include <linux/ioport.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
 
 #include <asm/irq.h>
 
@@ -2374,6 +2377,63 @@ 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 const struct of_device_id exynos4_gpio_dt_match[] __initdata = {
+	{ .compatible = "samsung,exynos4-gpio", },
+	{}
+};
+
+static __init void exynos4_gpiolib_attach_ofnode(struct samsung_gpio_chip *chip,
+						u64 base, u64 offset)
+{
+	struct gpio_chip *gc =  &chip->chip;
+	u64 address;
+
+	if (!of_have_populated_dt())
+		return;
+
+	address = (chip->base) ? (base + ((u32)chip->base & 0xfff)) :
+			(base + offset);
+
+	gc->of_node = of_find_matching_node_by_address(NULL,
+			exynos4_gpio_dt_match, address);
+	if (!gc->of_node) {
+		pr_info("gpio: device tree node not found for gpio controller"
+			" with base address %08llx\n", address);
+		return;
+	}
+	gc->of_gpio_n_cells = 4;
+	gc->of_xlate = exynos4_gpio_xlate;
+}
+#else
+static __init void exynos4_gpiolib_attach_ofnode(struct samsung_gpio_chip *chip,
+						u64 base, u64 offset)
+{
+	return;
+}
+#endif /* defined(CONFIG_ARCH_EXYNOS4) && defined(CONFIG_OF) */
+
 /* TODO: cleanup soc_is_* */
 static __init int samsung_gpiolib_init(void)
 {
@@ -2455,6 +2515,8 @@ static __init int samsung_gpiolib_init(void)
 				chip->config = &exynos4_gpio_cfg;
 				chip->group = group++;
 			}
+			exynos4_gpiolib_attach_ofnode(chip,
+					EXYNOS4_PA_GPIO1, i * 0x20);
 		}
 		samsung_gpiolib_add_4bit_chips(exynos4_gpios_1, nr_chips, S5P_VA_GPIO1);
 
@@ -2467,6 +2529,8 @@ static __init int samsung_gpiolib_init(void)
 				chip->config = &exynos4_gpio_cfg;
 				chip->group = group++;
 			}
+			exynos4_gpiolib_attach_ofnode(chip,
+					EXYNOS4_PA_GPIO2, i * 0x20);
 		}
 		samsung_gpiolib_add_4bit_chips(exynos4_gpios_2, nr_chips, S5P_VA_GPIO2);
 
@@ -2479,6 +2543,8 @@ static __init int samsung_gpiolib_init(void)
 				chip->config = &exynos4_gpio_cfg;
 				chip->group = group++;
 			}
+			exynos4_gpiolib_attach_ofnode(chip,
+					EXYNOS4_PA_GPIO3, i * 0x20);
 		}
 		samsung_gpiolib_add_4bit_chips(exynos4_gpios_3, nr_chips, S5P_VA_GPIO3);
 
-- 
1.7.4.4

[PATCH v2] gpio/samsung: Add device tree support for Exynos4

[Sylwester Nawrocki]

Hi Thomas,

thanks for your work on this.

On 11/01/2011 01:43 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>
> Acked-by: Grant Likely<grant.likely@secretlab.ca>
> ---
> Changes since v1:
> - As suggested by Rob and Grant, the gpio controller node lookup is based
>    on the base address of the gpio controller instead of the unique
>    per-controller compatible property value.
> 
> This patch is based on the following tree and branch.
> git://git.linaro.org/git/people/arnd/arm-soc.git  branch: for-next
> 
>   .../devicetree/bindings/gpio/gpio-samsung.txt      |   40 ++++++++++++
>   drivers/gpio/gpio-samsung.c                        |   66 ++++++++++++++++++++
>   2 files changed, 106 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..c143058
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> @@ -0,0 +1,40 @@
> +Samsung Exynos4 GPIO Controller
> +
> +Required properties:
> +- compatible: Compatible property value should be "samsung,exynos4-gpio>".
> +
> +- 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]>
> +
> +  Values for gpio specifier:
> +  - Pin number: is a value between 0 to 7.
> +  - Pull Up/Down: 0 - Pull Up/Down Disabled.
> +                  1 - Pull Down Enabled.
> +                  3 - Pull Up Enabled.
> +  - Drive Strength: 0 - 1x,
> +                    1 - 3x,
> +                    2 - 2x,
> +                    3 - 4x

I wonder whether it's worth to have more regular mapping, i.e.
*)	0 - 1x, 
	1 - 2x, 
	2 - 3x, 
	3 - 4x

It doesn't give as much advantage, and introduces an overhead of doing 
an additional remapping. However I find current mapping of the DT specifier
values to real driver strength slightly confusing. 
Perhaps unlikely, the future SoCs could have different meaning of the
register values.

> +
> +- gpio-controller: Specifies that the node is a gpio controller.
> +- #address-cells: should be 1.
> +- #size-cells: should be 1.
> +
> +Example:
> +
> +	gpa0: gpio-controller at 11400000 {
> +		#address-cells =<1>;
> +		#size-cells =<1>;
> +		compatible = "samsung,exynos4-gpio";
> +		reg =<0x11400000 0x20>;
> +		#gpio-cells =<4>;
> +		gpio-controller;
> +	};
> diff --git a/drivers/gpio/gpio-samsung.c b/drivers/gpio/gpio-samsung.c
> index 8662518..0140756 100644
> --- a/drivers/gpio/gpio-samsung.c
> +++ b/drivers/gpio/gpio-samsung.c
> @@ -24,6 +24,9 @@
>   #include<linux/interrupt.h>
>   #include<linux/sysdev.h>
>   #include<linux/ioport.h>
> +#include<linux/of.h>
> +#include<linux/slab.h>
> +#include<linux/of_address.h>
> 
>   #include<asm/irq.h>
> 
> @@ -2374,6 +2377,63 @@ 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;
> +	}

nit: Could be simplified to:

	if (WARN_ON(gc->of_gpio_n_cells < 4))
		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]));

The above functions can fail and IMHO ignoring the return value here
makes the system harder to debug. 

Assuming GPIO drive strength specifier mapping *) the following code could
do the remapping (not tested):

	unsigned int tmp = be32_to_cpu(gpio[3]);
	u32 drvstr = ((tmp >> 1) ^ tmp) & 1 ? ~tmp & 3 : tmp;
	
	s5p_gpio_set_drvstr(pin, drvstr);

> +	return n;
> +}
> +
> +static const struct of_device_id exynos4_gpio_dt_match[] __initdata = {
> +	{ .compatible = "samsung,exynos4-gpio", },
> +	{}
> +};
> +
> +static __init void exynos4_gpiolib_attach_ofnode(struct samsung_gpio_chip *chip,
> +						u64 base, u64 offset)
> +{
> +	struct gpio_chip *gc =&chip->chip;
> +	u64 address;
> +
> +	if (!of_have_populated_dt())
> +		return;
> +
> +	address = (chip->base) ? (base + ((u32)chip->base&  0xfff)) :
> +			(base + offset);

Could the extra parentheses be dropped ?

--
Regards,
Sylwester

[PATCH v2] gpio/samsung: Add device tree support for Exynos4

[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>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> Changes since v1:
> - As suggested by Rob and Grant, the gpio controller node lookup is based
>   on the base address of the gpio controller instead of the unique
>   per-controller compatible property value.
> 
> This patch is based on the following tree and branch.
> git://git.linaro.org/git/people/arnd/arm-soc.git  branch: for-next
> 
>  .../devicetree/bindings/gpio/gpio-samsung.txt      |   40 ++++++++++++
>  drivers/gpio/gpio-samsung.c                        |   66
> ++++++++++++++++++++
>  2 files changed, 106 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..c143058
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> @@ -0,0 +1,40 @@
> +Samsung Exynos4 GPIO Controller
> +
> +Required properties:
> +- compatible: Compatible property value should be
"samsung,exynos4-gpio>".
> +
> +- 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]>
> +
> +  Values for gpio specifier:
> +  - Pin number: is a value between 0 to 7.
> +  - Pull Up/Down: 0 - Pull Up/Down Disabled.
> +                  1 - Pull Down Enabled.
> +                  3 - Pull Up Enabled.
> +  - Drive Strength: 0 - 1x,
> +                    1 - 3x,
> +                    2 - 2x,
> +                    3 - 4x
> +
> +- gpio-controller: Specifies that the node is a gpio controller.
> +- #address-cells: should be 1.
> +- #size-cells: should be 1.
> +
> +Example:
> +
> +	gpa0: gpio-controller at 11400000 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "samsung,exynos4-gpio";
> +		reg = <0x11400000 0x20>;
> +		#gpio-cells = <4>;
> +		gpio-controller;
> +	};
> diff --git a/drivers/gpio/gpio-samsung.c b/drivers/gpio/gpio-samsung.c
> index 8662518..0140756 100644
> --- a/drivers/gpio/gpio-samsung.c
> +++ b/drivers/gpio/gpio-samsung.c
> @@ -24,6 +24,9 @@
>  #include <linux/interrupt.h>
>  #include <linux/sysdev.h>
>  #include <linux/ioport.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> 
>  #include <asm/irq.h>
> 
> @@ -2374,6 +2377,63 @@ 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 const struct of_device_id exynos4_gpio_dt_match[] __initdata = {
> +	{ .compatible = "samsung,exynos4-gpio", },
> +	{}
> +};
> +
> +static __init void exynos4_gpiolib_attach_ofnode(struct samsung_gpio_chip
> *chip,
> +						u64 base, u64 offset)
> +{
> +	struct gpio_chip *gc =  &chip->chip;
> +	u64 address;
> +
> +	if (!of_have_populated_dt())
> +		return;
> +
> +	address = (chip->base) ? (base + ((u32)chip->base & 0xfff)) :
> +			(base + offset);
> +
> +	gc->of_node = of_find_matching_node_by_address(NULL,
> +			exynos4_gpio_dt_match, address);
> +	if (!gc->of_node) {
> +		pr_info("gpio: device tree node not found for gpio
controller"
> +			" with base address %08llx\n", address);
> +		return;
> +	}
> +	gc->of_gpio_n_cells = 4;
> +	gc->of_xlate = exynos4_gpio_xlate;
> +}
> +#else
> +static __init void exynos4_gpiolib_attach_ofnode(struct samsung_gpio_chip
> *chip,
> +						u64 base, u64 offset)
> +{
> +	return;
> +}
> +#endif /* defined(CONFIG_ARCH_EXYNOS4) && defined(CONFIG_OF) */
> +
>  /* TODO: cleanup soc_is_* */
>  static __init int samsung_gpiolib_init(void)
>  {
> @@ -2455,6 +2515,8 @@ static __init int samsung_gpiolib_init(void)
>  				chip->config = &exynos4_gpio_cfg;
>  				chip->group = group++;
>  			}
> +			exynos4_gpiolib_attach_ofnode(chip,
> +					EXYNOS4_PA_GPIO1, i * 0x20);
>  		}
>  		samsung_gpiolib_add_4bit_chips(exynos4_gpios_1, nr_chips,
> S5P_VA_GPIO1);
> 
> @@ -2467,6 +2529,8 @@ static __init int samsung_gpiolib_init(void)
>  				chip->config = &exynos4_gpio_cfg;
>  				chip->group = group++;
>  			}
> +			exynos4_gpiolib_attach_ofnode(chip,
> +					EXYNOS4_PA_GPIO2, i * 0x20);
>  		}
>  		samsung_gpiolib_add_4bit_chips(exynos4_gpios_2, nr_chips,
> S5P_VA_GPIO2);
> 
> @@ -2479,6 +2543,8 @@ static __init int samsung_gpiolib_init(void)
>  				chip->config = &exynos4_gpio_cfg;
>  				chip->group = group++;
>  			}
> +			exynos4_gpiolib_attach_ofnode(chip,
> +					EXYNOS4_PA_GPIO3, i * 0x20);
>  		}
>  		samsung_gpiolib_add_4bit_chips(exynos4_gpios_3, nr_chips,
> S5P_VA_GPIO3);
> 
> --
> 1.7.4.4

Thomas,

Happens following build error.

drivers/gpio/gpio-samsung.c: In function 'samsung_gpiolib_init':
drivers/gpio/gpio-samsung.c:2519: error: 'EXYNOS4_PA_GPIO1' undeclared
(first use in this function)
drivers/gpio/gpio-samsung.c:2519: error: (Each undeclared identifier is
reported only once
drivers/gpio/gpio-samsung.c:2519: error: for each function it appears in.)
drivers/gpio/gpio-samsung.c:2533: error: 'EXYNOS4_PA_GPIO2' undeclared
(first use in this function)
drivers/gpio/gpio-samsung.c:2547: error: 'EXYNOS4_PA_GPIO3' undeclared
(first use in this function)

Thanks.

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

[PATCH v2] gpio/samsung: Add device tree support for Exynos4

[Thomas Abraham]

Hi Sylwester,

On 1 November 2011 13:52, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> Hi Thomas,
>
> thanks for your work on this.
>
> On 11/01/2011 01:43 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>
>> Acked-by: Grant Likely<grant.likely@secretlab.ca>
>> ---
>> Changes since v1:
>> - As suggested by Rob and Grant, the gpio controller node lookup is based
>> ? ?on the base address of the gpio controller instead of the unique
>> ? ?per-controller compatible property value.
>>
>> This patch is based on the following tree and branch.
>> git://git.linaro.org/git/people/arnd/arm-soc.git ?branch: for-next
>>
>> ? .../devicetree/bindings/gpio/gpio-samsung.txt ? ? ?| ? 40 ++++++++++++
>> ? drivers/gpio/gpio-samsung.c ? ? ? ? ? ? ? ? ? ? ? ?| ? 66 ++++++++++++++++++++
>> ? 2 files changed, 106 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..c143058
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>> @@ -0,0 +1,40 @@
>> +Samsung Exynos4 GPIO Controller
>> +
>> +Required properties:
>> +- compatible: Compatible property value should be "samsung,exynos4-gpio>".
>> +
>> +- 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]>
>> +
>> + ?Values for gpio specifier:
>> + ?- Pin number: is a value between 0 to 7.
>> + ?- Pull Up/Down: 0 - Pull Up/Down Disabled.
>> + ? ? ? ? ? ? ? ? ?1 - Pull Down Enabled.
>> + ? ? ? ? ? ? ? ? ?3 - Pull Up Enabled.
>> + ?- Drive Strength: 0 - 1x,
>> + ? ? ? ? ? ? ? ? ? ?1 - 3x,
>> + ? ? ? ? ? ? ? ? ? ?2 - 2x,
>> + ? ? ? ? ? ? ? ? ? ?3 - 4x
>
> I wonder whether it's worth to have more regular mapping, i.e.
> *) ? ? ?0 - 1x,
> ? ? ? ?1 - 2x,
> ? ? ? ?2 - 3x,
> ? ? ? ?3 - 4x
>
> It doesn't give as much advantage, and introduces an overhead of doing
> an additional remapping. However I find current mapping of the DT specifier
> values to real driver strength slightly confusing.
> Perhaps unlikely, the future SoCs could have different meaning of the
> register values.

The dts file describes the hardware and the drive strength values
listed above are as per the Exynos4 SoC user manual. I would prefer to
do it the way you suggested, but that would mean dts is not exactly
matching the hardware manual.

[...]

>> + ? ? };
>> diff --git a/drivers/gpio/gpio-samsung.c b/drivers/gpio/gpio-samsung.c
>> index 8662518..0140756 100644
>> --- a/drivers/gpio/gpio-samsung.c
>> +++ b/drivers/gpio/gpio-samsung.c

[...]

>> + ? ? if (gc->of_gpio_n_cells< ?4) {
>> + ? ? ? ? ? ? WARN_ON(1);
>> + ? ? ? ? ? ? return -EINVAL;
>> + ? ? }
>
> nit: Could be simplified to:
>
> ? ? ? ?if (WARN_ON(gc->of_gpio_n_cells < 4))
> ? ? ? ? ? ? ? ?return -EINVAL;

Ok.

>
>> +
>> + ? ? 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]));
>
> The above functions can fail and IMHO ignoring the return value here
> makes the system harder to debug.

Ok.

>
> Assuming GPIO drive strength specifier mapping *) the following code could
> do the remapping (not tested):
>
> ? ? ? ?unsigned int tmp = be32_to_cpu(gpio[3]);
> ? ? ? ?u32 drvstr = ((tmp >> 1) ^ tmp) & 1 ? ~tmp & 3 : tmp;
>
> ? ? ? ?s5p_gpio_set_drvstr(pin, drvstr);
>
>> + ? ? return n;
>> +}
>> +
>> +static const struct of_device_id exynos4_gpio_dt_match[] __initdata = {
>> + ? ? { .compatible = "samsung,exynos4-gpio", },
>> + ? ? {}
>> +};
>> +
>> +static __init void exynos4_gpiolib_attach_ofnode(struct samsung_gpio_chip *chip,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? u64 base, u64 offset)
>> +{
>> + ? ? struct gpio_chip *gc =&chip->chip;
>> + ? ? u64 address;
>> +
>> + ? ? if (!of_have_populated_dt())
>> + ? ? ? ? ? ? return;
>> +
>> + ? ? address = (chip->base) ? (base + ((u32)chip->base& ?0xfff)) :
>> + ? ? ? ? ? ? ? ? ? ? (base + offset);
>
> Could the extra parentheses be dropped ?

Can I postpone these changes for now so that this patch gets merged in
next-samsung-dt branch. I believe these are required changes but could
be done a little later.

Thanks for your review and comments on this patch.

Regards,
Thomas.

>
> --
> Regards,
> Sylwester
>

[PATCH v2] gpio/samsung: Add device tree support for Exynos4

[Thomas Abraham]

Hi Mr. Kim,

On 2 November 2011 17:25, Kukjin Kim <kgene.kim@samsung.com> 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>
>> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>> ---
>> Changes since v1:
>> - As suggested by Rob and Grant, the gpio controller node lookup is based
>> ? on the base address of the gpio controller instead of the unique
>> ? per-controller compatible property value.
>>
>> This patch is based on the following tree and branch.
>> git://git.linaro.org/git/people/arnd/arm-soc.git ?branch: for-next
>>
>> ?.../devicetree/bindings/gpio/gpio-samsung.txt ? ? ?| ? 40 ++++++++++++
>> ?drivers/gpio/gpio-samsung.c ? ? ? ? ? ? ? ? ? ? ? ?| ? 66
>> ++++++++++++++++++++
>> ?2 files changed, 106 insertions(+), 0 deletions(-)
>> ?create mode 100644

[...]

>> diff --git a/drivers/gpio/gpio-samsung.c b/drivers/gpio/gpio-samsung.c
>> index 8662518..0140756 100644
>> --- a/drivers/gpio/gpio-samsung.c
>> +++ b/drivers/gpio/gpio-samsung.c

[...]

> Thomas,
>
> Happens following build error.
>
> drivers/gpio/gpio-samsung.c: In function 'samsung_gpiolib_init':
> drivers/gpio/gpio-samsung.c:2519: error: 'EXYNOS4_PA_GPIO1' undeclared
> (first use in this function)
> drivers/gpio/gpio-samsung.c:2519: error: (Each undeclared identifier is
> reported only once
> drivers/gpio/gpio-samsung.c:2519: error: for each function it appears in.)
> drivers/gpio/gpio-samsung.c:2533: error: 'EXYNOS4_PA_GPIO2' undeclared
> (first use in this function)
> drivers/gpio/gpio-samsung.c:2547: error: 'EXYNOS4_PA_GPIO3' undeclared
> (first use in this function)

Can this be fixed while you apply this patch. If required, I can
submit an updated patch for this.

Thanks,
Thomas.

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

[PATCH v2] gpio/samsung: Add device tree support for Exynos4

[Mark Brown]

On Wed, Nov 02, 2011 at 06:35:05PM +0530, Thomas Abraham wrote:
> On 1 November 2011 13:52, Sylwester Nawrocki <snjw23@gmail.com> wrote:

> > It doesn't give as much advantage, and introduces an overhead of doing
> > an additional remapping. However I find current mapping of the DT specifier
> > values to real driver strength slightly confusing.
> > Perhaps unlikely, the future SoCs could have different meaning of the
> > register values.

> The dts file describes the hardware and the drive strength values
> listed above are as per the Exynos4 SoC user manual. I would prefer to
> do it the way you suggested, but that would mean dts is not exactly
> matching the hardware manual.

On the other hand it does mean that the user can directly read the
values - there's nothing that obviously tells the user that the binding
is using datasheet register values rather than the actual driver
strength numbers and given that software is supposed to provide an
abstraction to make things easier it seems reasonably natural to assume
the latter.

[PATCH v2] gpio/samsung: Add device tree support for Exynos4

[Sylwester Nawrocki]

On 11/02/2011 02:05 PM, Thomas Abraham wrote:
> On 1 November 2011 13:52, Sylwester Nawrocki <snjw23@gmail.com> wrote:
>> On 11/01/2011 01:43 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>
>>> Acked-by: Grant Likely<grant.likely@secretlab.ca>
>>> ---
>>> Changes since v1:
>>> - As suggested by Rob and Grant, the gpio controller node lookup is based
>>>    on the base address of the gpio controller instead of the unique
>>>    per-controller compatible property value.
>>>
>>> This patch is based on the following tree and branch.
>>> git://git.linaro.org/git/people/arnd/arm-soc.git  branch: for-next
>>>
>>>   .../devicetree/bindings/gpio/gpio-samsung.txt      |   40 ++++++++++++
>>>   drivers/gpio/gpio-samsung.c                        |   66 ++++++++++++++++++++
>>>   2 files changed, 106 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..c143058
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
>>> @@ -0,0 +1,40 @@
>>> +Samsung Exynos4 GPIO Controller
>>> +
>>> +Required properties:
>>> +- compatible: Compatible property value should be "samsung,exynos4-gpio>".
>>> +
>>> +- 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]>
>>> +
>>> +  Values for gpio specifier:
>>> +  - Pin number: is a value between 0 to 7.
>>> +  - Pull Up/Down: 0 - Pull Up/Down Disabled.
>>> +                  1 - Pull Down Enabled.
>>> +                  3 - Pull Up Enabled.
>>> +  - Drive Strength: 0 - 1x,
>>> +                    1 - 3x,
>>> +                    2 - 2x,
>>> +                    3 - 4x
>>
>> I wonder whether it's worth to have more regular mapping, i.e.
>> *)      0 - 1x,
>>        1 - 2x,
>>        2 - 3x,
>>        3 - 4x
>>
>> It doesn't give as much advantage, and introduces an overhead of doing
>> an additional remapping. However I find current mapping of the DT specifier
>> values to real driver strength slightly confusing.
>> Perhaps unlikely, the future SoCs could have different meaning of the
>> register values.
> 
> The dts file describes the hardware and the drive strength values
> listed above are as per the Exynos4 SoC user manual. I would prefer to
> do it the way you suggested, but that would mean dts is not exactly
> matching the hardware manual.

Sorry, I somehow missed these bindings are for Exynos4 only.

Anyway, there is rather little chance to have uniform drive strength
representation across all Samsung SoC. It's sometimes expressed in mA,
sometimes has no units. Perhaps it's best to stick with HW reg. values,
however it is some times confusing as hell...


For instance, for exynos4 we have:

driver strength | 1x   | 2x   | 3x   | 4x
---------------------------------------------
register value  | 0x00 | 0x2  | 0x01 | 0x03


for s3c6410:

driver strength | 2mA  | 4mA  | 7 mA | 9 mA
---------------------------------------------
register value  | 0x00 | 0x2  | 0x01 | 0x03


Nevertheless, I think, for exynos4 the effort to have actual drive strength
directly expressed in DT description, rather than using not entirely sane
HW values, is rather small.
And it could save a bit of frustration for all these poor engineers trying
to use the API..

...

> 
> Can I postpone these changes for now so that this patch gets merged in
> next-samsung-dt branch. I believe these are required changes but could
> be done a little later.

Sure, that's understood. It's fine for me to get those things fixed any time
for 3.2 stable release.

--
Regards,
Sylwester