[PATCH v4 1/6] gpio: fix a typo of comment message

[PATCH v4 1/6] gpio: fix a typo of comment message

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 drivers/gpio/gpiolib-of.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index d18068a..8389d4a 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -21,7 +21,7 @@
 #include <linux/of_gpio.h>
 #include <linux/slab.h>
 
-/* Private data structure for of_gpiochip_is_match */
+/* Private data structure for of_gpiochip_find_and_xlate */
 struct gg_data {
 	enum of_gpio_flags *flags;
 	struct of_phandle_args gpiospec;
-- 
1.7.0.4

[PATCH v4 2/6] gpio: re-add of_node_to_gpiochip function

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 drivers/gpio/gpiolib-of.c |   11 +++++++++++
 include/linux/of_gpio.h   |    5 +++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 8389d4a..b8010a9 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -234,3 +234,14 @@ void of_gpiochip_remove(struct gpio_chip *chip)
 	if (chip->of_node)
 		of_node_put(chip->of_node);
 }
+
+/* Private function for resolving node pointer to gpio_chip */
+static int of_gpiochip_is_match(struct gpio_chip *chip, void *data)
+{
+	return chip->of_node == data;
+}
+
+struct gpio_chip *of_node_to_gpiochip(struct device_node *np)
+{
+	return gpiochip_find(np, of_gpiochip_is_match);
+}
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index c454f57..880783b 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -61,6 +61,7 @@ extern void of_gpiochip_remove(struct gpio_chip *gc);
 extern int of_gpio_simple_xlate(struct gpio_chip *gc,
 				const struct of_phandle_args *gpiospec,
 				u32 *flags);
+extern struct gpio_chip *of_node_to_gpiochip(struct device_node *np);
 
 #else /* CONFIG_OF_GPIO */
 
@@ -84,6 +85,10 @@ static inline int of_gpio_simple_xlate(struct gpio_chip *gc,
 	return -ENOSYS;
 }
 
+static struct gpio_chip *of_node_to_gpiochip(struct device_node *np)
+{
+	return NULL;
+}
 static inline void of_gpiochip_add(struct gpio_chip *gc) { }
 static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
 
-- 
1.7.0.4

[PATCH v4 2/6] gpio: re-add of_node_to_gpiochip function

[Grant Likely]

On Fri, 25 May 2012 21:36:16 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

Nack.  Several problems here.  First and foremost there isn't any
description of *why* this change is needed or when it was removed.
Any patch beyond the most trivial change needs to have a commit
message the describes it.

Second, of_node_to_gpiochip() is no longer a valid API because gpiolib
now allows multiple gpiochips to use the same device tree node.  See
commit 3d0f7cf0f "gpio: Adjust of_xlate API to support multiple GPIO
chips" to see a description of why this was changed.

g.

> ---
>  drivers/gpio/gpiolib-of.c |   11 +++++++++++
>  include/linux/of_gpio.h   |    5 +++++
>  2 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 8389d4a..b8010a9 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -234,3 +234,14 @@ void of_gpiochip_remove(struct gpio_chip *chip)
>  	if (chip->of_node)
>  		of_node_put(chip->of_node);
>  }
> +
> +/* Private function for resolving node pointer to gpio_chip */
> +static int of_gpiochip_is_match(struct gpio_chip *chip, void *data)
> +{
> +	return chip->of_node == data;
> +}
> +
> +struct gpio_chip *of_node_to_gpiochip(struct device_node *np)
> +{
> +	return gpiochip_find(np, of_gpiochip_is_match);
> +}
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index c454f57..880783b 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -61,6 +61,7 @@ extern void of_gpiochip_remove(struct gpio_chip *gc);
>  extern int of_gpio_simple_xlate(struct gpio_chip *gc,
>  				const struct of_phandle_args *gpiospec,
>  				u32 *flags);
> +extern struct gpio_chip *of_node_to_gpiochip(struct device_node *np);
>  
>  #else /* CONFIG_OF_GPIO */
>  
> @@ -84,6 +85,10 @@ static inline int of_gpio_simple_xlate(struct gpio_chip *gc,
>  	return -ENOSYS;
>  }
>  
> +static struct gpio_chip *of_node_to_gpiochip(struct device_node *np)
> +{
> +	return NULL;
> +}
>  static inline void of_gpiochip_add(struct gpio_chip *gc) { }
>  static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
>  
> -- 
> 1.7.0.4
> 
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

[PATCH v4 2/6] gpio: re-add of_node_to_gpiochip function

[Dong Aisheng]

On Sat, May 26, 2012 at 8:01 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, 25 May 2012 21:36:16 +0800, Dong Aisheng <b29396@freescale.com> wrote:
>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>
>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>
> Nack. ?Several problems here. ?First and foremost there isn't any
> description of *why* this change is needed or when it was removed.
Sorry i sent it too fast to eager to see people's comments on the main
issue of patch 6.
It should have description on what you pointed out.
It's needed by the patch 6 in this series that it wants to find gpiochip from
node then calculate the gpio number from device tree in a standard way.

> Any patch beyond the most trivial change needs to have a commit
> message the describes it.
>
> Second, of_node_to_gpiochip() is no longer a valid API because gpiolib
> now allows multiple gpiochips to use the same device tree node. ?See
> commit 3d0f7cf0f "gpio: Adjust of_xlate API to support multiple GPIO
> chips" to see a description of why this was changed.
>
I noticed that of_node_to_gpiochip() is removed by that patch.
But i did not realized that this API was broken before.
After looking a bit more on the sample code of an banked gpio,
https://lkml.org/lkml/2012/5/18/51
it seems besides the gpio node, we also depend on controller specific
gpio .of_xlate function
to find the correct gpiochip in the same bank for the same node.
So it's correct that of_node_to_gpiochip() is broken now.

I may change it if we finally still find this API is needed.

Thanks for the review.

Regards
Dong Aisheng

>> ---
>> ?drivers/gpio/gpiolib-of.c | ? 11 +++++++++++
>> ?include/linux/of_gpio.h ? | ? ?5 +++++
>> ?2 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> index 8389d4a..b8010a9 100644
>> --- a/drivers/gpio/gpiolib-of.c
>> +++ b/drivers/gpio/gpiolib-of.c
>> @@ -234,3 +234,14 @@ void of_gpiochip_remove(struct gpio_chip *chip)
>> ? ? ? if (chip->of_node)
>> ? ? ? ? ? ? ? of_node_put(chip->of_node);
>> ?}
>> +
>> +/* Private function for resolving node pointer to gpio_chip */
>> +static int of_gpiochip_is_match(struct gpio_chip *chip, void *data)
>> +{
>> + ? ? return chip->of_node == data;
>> +}
>> +
>> +struct gpio_chip *of_node_to_gpiochip(struct device_node *np)
>> +{
>> + ? ? return gpiochip_find(np, of_gpiochip_is_match);
>> +}
>> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
>> index c454f57..880783b 100644
>> --- a/include/linux/of_gpio.h
>> +++ b/include/linux/of_gpio.h
>> @@ -61,6 +61,7 @@ extern void of_gpiochip_remove(struct gpio_chip *gc);
>> ?extern int of_gpio_simple_xlate(struct gpio_chip *gc,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const struct of_phandle_args *gpiospec,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? u32 *flags);
>> +extern struct gpio_chip *of_node_to_gpiochip(struct device_node *np);
>>
>> ?#else /* CONFIG_OF_GPIO */
>>
>> @@ -84,6 +85,10 @@ static inline int of_gpio_simple_xlate(struct gpio_chip *gc,
>> ? ? ? return -ENOSYS;
>> ?}
>>
>> +static struct gpio_chip *of_node_to_gpiochip(struct device_node *np)
>> +{
>> + ? ? return NULL;
>> +}
>> ?static inline void of_gpiochip_add(struct gpio_chip *gc) { }
>> ?static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
>>
>> --
>> 1.7.0.4
>>
>>
>
> --
> Grant Likely, B.Sc, P.Eng.
> Secret Lab Technologies, Ltd.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/

[PATCH v4 3/6] of: release node fix for of_parse_phandle_with_args

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

Since this API requires user to call of_node_put for the node
it returned via outargs, if no outargs provided, user have no
chance to release it, so release it internally if no outargs
provided.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 drivers/of/base.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d9bfd49..91c1fb4 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -961,6 +961,8 @@ int of_parse_phandle_with_args(struct device_node *np, const char *list_name,
 				out_args->args_count = count;
 				for (i = 0; i < count; i++)
 					out_args->args[i] = be32_to_cpup(list++);
+			} else {
+				of_node_put(node);
 			}
 			return 0;
 		}
-- 
1.7.0.4

[PATCH v4 3/6] of: release node fix for of_parse_phandle_with_args

[Grant Likely]

On Fri, 25 May 2012 21:36:17 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Since this API requires user to call of_node_put for the node
> it returned via outargs, if no outargs provided, user have no
> chance to release it, so release it internally if no outargs
> provided.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

Applied, thanks.

g.

> ---
>  drivers/of/base.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d9bfd49..91c1fb4 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -961,6 +961,8 @@ int of_parse_phandle_with_args(struct device_node *np, const char *list_name,
>  				out_args->args_count = count;
>  				for (i = 0; i < count; i++)
>  					out_args->args[i] = be32_to_cpup(list++);
> +			} else {
> +				of_node_put(node);
>  			}
>  			return 0;
>  		}
> -- 
> 1.7.0.4
> 
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

[PATCH v4 4/6] gpio: introduce lock mechanism for gpiochip_find

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

The module lock will be automatically claimed for gpiochip_find function
in case the gpio module is removed during the using of gpiochip instance.
Users are responsible to call gpiochip_put to release the lock after
the using.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 drivers/gpio/gpiolib-of.c  |    5 ++++-
 drivers/gpio/gpiolib.c     |   17 +++++++++++++++++
 include/asm-generic/gpio.h |    2 +-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index b8010a9..d521452 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -63,6 +63,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
                            int index, enum of_gpio_flags *flags)
 {
 	struct gg_data gg_data = { .flags = flags, .out_gpio = -ENODEV };
+	struct gpio_chip *chip;
 	int ret;
 
 	/* .of_xlate might decide to not fill in the flags, so clear it. */
@@ -76,7 +77,9 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
 		return -EINVAL;
 	}
 
-	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
+	chip = gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
+	if (chip)
+		gpiochip_put(chip);
 
 	of_node_put(gg_data.gpiospec.np);
 	pr_debug("%s exited with status %d\n", __func__, ret);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 120b2a0..6453d43 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1144,6 +1144,18 @@ int gpiochip_remove(struct gpio_chip *chip)
 EXPORT_SYMBOL_GPL(gpiochip_remove);
 
 /**
+ * gpiochip_put() - release a gpio_chip
+ * @chip: the chip to release
+ */
+inline void gpiochip_put(struct gpio_chip *chip)
+{
+	BUG_ON(!chip);
+
+	module_put(chip->owner);
+}
+EXPORT_SYMBOL_GPL(gpiochip_put);
+
+/**
  * gpiochip_find() - iterator for locating a specific gpio_chip
  * @data: data to pass to match function
  * @callback: Callback function to check gpio_chip
@@ -1153,6 +1165,9 @@ EXPORT_SYMBOL_GPL(gpiochip_remove);
  * 0 if the device doesn't match and non-zero if it does.  If the callback is
  * non-zero, this function will return to the caller and not iterate over any
  * more gpio_chips.
+ *
+ * Note the gpio_chip is returned with the module locked, users are responsible
+ * to release the lock with gpiochip_put(chip) after using it.
  */
 struct gpio_chip *gpiochip_find(void *data,
 				int (*match)(struct gpio_chip *chip,
@@ -1169,6 +1184,8 @@ struct gpio_chip *gpiochip_find(void *data,
 
 		if (match(gpio_desc[i].chip, data)) {
 			chip = gpio_desc[i].chip;
+			if (!try_module_get(chip->owner))
+				chip = NULL;
 			break;
 		}
 	}
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 365ea09..af372be 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -145,7 +145,7 @@ extern int __must_check gpiochip_remove(struct gpio_chip *chip);
 extern struct gpio_chip *gpiochip_find(void *data,
 					int (*match)(struct gpio_chip *chip,
 						     void *data));
-
+extern void gpiochip_put(struct gpio_chip *chip);
 
 /* Always use the library code for GPIO management calls,
  * or when sleeping may be involved.
-- 
1.7.0.4

[PATCH v4 4/6] gpio: introduce lock mechanism for gpiochip_find

[Grant Likely]

On Fri, 25 May 2012 21:36:18 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> The module lock will be automatically claimed for gpiochip_find function
> in case the gpio module is removed during the using of gpiochip instance.
> Users are responsible to call gpiochip_put to release the lock after
> the using.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

Hey Dong,

Does this actually protect anything?  From what I can see the gpio_lock
is held for the entire time of iterating over gpio_chips when the
match function is called and the gpio number is fully resolved before
gpiochip_find returns when called from of_get_named_gpio_flags().

Also, it doesn't do anything to protect against the gpio_chip being
removed after the gpio number is resolved, which means the gpio number
may no longer be valid, or may no longer point to the same gpio chip.
It looks like the locking protection needs to be wider to be useful.

Or is this patch to fix something in the mach-mx35-3ds.c, which is the
only other use of gpiochip_find().

g.

> ---
>  drivers/gpio/gpiolib-of.c  |    5 ++++-
>  drivers/gpio/gpiolib.c     |   17 +++++++++++++++++
>  include/asm-generic/gpio.h |    2 +-
>  3 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index b8010a9..d521452 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -63,6 +63,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>                             int index, enum of_gpio_flags *flags)
>  {
>  	struct gg_data gg_data = { .flags = flags, .out_gpio = -ENODEV };
> +	struct gpio_chip *chip;
>  	int ret;
>  
>  	/* .of_xlate might decide to not fill in the flags, so clear it. */
> @@ -76,7 +77,9 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>  		return -EINVAL;
>  	}
>  
> -	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> +	chip = gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> +	if (chip)
> +		gpiochip_put(chip);
>  
>  	of_node_put(gg_data.gpiospec.np);
>  	pr_debug("%s exited with status %d\n", __func__, ret);
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 120b2a0..6453d43 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1144,6 +1144,18 @@ int gpiochip_remove(struct gpio_chip *chip)
>  EXPORT_SYMBOL_GPL(gpiochip_remove);
>  
>  /**
> + * gpiochip_put() - release a gpio_chip
> + * @chip: the chip to release
> + */
> +inline void gpiochip_put(struct gpio_chip *chip)
> +{
> +	BUG_ON(!chip);
> +
> +	module_put(chip->owner);
> +}
> +EXPORT_SYMBOL_GPL(gpiochip_put);
> +
> +/**
>   * gpiochip_find() - iterator for locating a specific gpio_chip
>   * @data: data to pass to match function
>   * @callback: Callback function to check gpio_chip
> @@ -1153,6 +1165,9 @@ EXPORT_SYMBOL_GPL(gpiochip_remove);
>   * 0 if the device doesn't match and non-zero if it does.  If the callback is
>   * non-zero, this function will return to the caller and not iterate over any
>   * more gpio_chips.
> + *
> + * Note the gpio_chip is returned with the module locked, users are responsible
> + * to release the lock with gpiochip_put(chip) after using it.
>   */
>  struct gpio_chip *gpiochip_find(void *data,
>  				int (*match)(struct gpio_chip *chip,
> @@ -1169,6 +1184,8 @@ struct gpio_chip *gpiochip_find(void *data,
>  
>  		if (match(gpio_desc[i].chip, data)) {
>  			chip = gpio_desc[i].chip;
> +			if (!try_module_get(chip->owner))
> +				chip = NULL;
>  			break;
>  		}
>  	}
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index 365ea09..af372be 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -145,7 +145,7 @@ extern int __must_check gpiochip_remove(struct gpio_chip *chip);
>  extern struct gpio_chip *gpiochip_find(void *data,
>  					int (*match)(struct gpio_chip *chip,
>  						     void *data));
> -
> +extern void gpiochip_put(struct gpio_chip *chip);
>  
>  /* Always use the library code for GPIO management calls,
>   * or when sleeping may be involved.
> -- 
> 1.7.0.4
> 
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

[PATCH v4 4/6] gpio: introduce lock mechanism for gpiochip_find

[Dong Aisheng]

On Fri, May 25, 2012 at 5:25 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, 25 May 2012 21:36:18 +0800, Dong Aisheng <b29396@freescale.com> wrote:
>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>
>> The module lock will be automatically claimed for gpiochip_find function
>> in case the gpio module is removed during the using of gpiochip instance.
>> Users are responsible to call gpiochip_put to release the lock after
>> the using.
>>
>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>
> Hey Dong,
>
> Does this actually protect anything? ?From what I can see the gpio_lock
> is held for the entire time of iterating over gpio_chips when the
> match function is called and the gpio number is fully resolved before
> gpiochip_find returns when called from of_get_named_gpio_flags().
>
Correct, the of_get_named_gpio_flags() has no issues.
The problem is the gpiochip pointer is returned and user may use it
with no lock which may possible already been released.
I got this suggestion from Stephen Warren originally and i agreed with him.

> Also, it doesn't do anything to protect against the gpio_chip being
> removed after the gpio number is resolved, which means the gpio number
> may no longer be valid, or may no longer point to the same gpio chip.
Can you help explain this case a bit more?
The module lock used here is only prevent the gpio module to be
removed which causes
the gpiochip to be release during the using.
You mean except module remove, the gpio_chip can also be removed by other case?

> It looks like the locking protection needs to be wider to be useful.
>
> Or is this patch to fix something in the mach-mx35-3ds.c, which is the
> only other use of gpiochip_find().
>
Formerly it's used for patch 6 since it used the gpiochip_find function.
But yes, mach-mx35-3ds.c should also need the lock if this is an issue.

Regards
Dong Aisheng
>
>> ---
>> ?drivers/gpio/gpiolib-of.c ?| ? ?5 ++++-
>> ?drivers/gpio/gpiolib.c ? ? | ? 17 +++++++++++++++++
>> ?include/asm-generic/gpio.h | ? ?2 +-
>> ?3 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> index b8010a9..d521452 100644
>> --- a/drivers/gpio/gpiolib-of.c
>> +++ b/drivers/gpio/gpiolib-of.c
>> @@ -63,6 +63,7 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? int index, enum of_gpio_flags *flags)
>> ?{
>> ? ? ? struct gg_data gg_data = { .flags = flags, .out_gpio = -ENODEV };
>> + ? ? struct gpio_chip *chip;
>> ? ? ? int ret;
>>
>> ? ? ? /* .of_xlate might decide to not fill in the flags, so clear it. */
>> @@ -76,7 +77,9 @@ int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>> ? ? ? ? ? ? ? return -EINVAL;
>> ? ? ? }
>>
>> - ? ? gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
>> + ? ? chip = gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
>> + ? ? if (chip)
>> + ? ? ? ? ? ? gpiochip_put(chip);
>>
>> ? ? ? of_node_put(gg_data.gpiospec.np);
>> ? ? ? pr_debug("%s exited with status %d\n", __func__, ret);
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 120b2a0..6453d43 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1144,6 +1144,18 @@ int gpiochip_remove(struct gpio_chip *chip)
>> ?EXPORT_SYMBOL_GPL(gpiochip_remove);
>>
>> ?/**
>> + * gpiochip_put() - release a gpio_chip
>> + * @chip: the chip to release
>> + */
>> +inline void gpiochip_put(struct gpio_chip *chip)
>> +{
>> + ? ? BUG_ON(!chip);
>> +
>> + ? ? module_put(chip->owner);
>> +}
>> +EXPORT_SYMBOL_GPL(gpiochip_put);
>> +
>> +/**
>> ? * gpiochip_find() - iterator for locating a specific gpio_chip
>> ? * @data: data to pass to match function
>> ? * @callback: Callback function to check gpio_chip
>> @@ -1153,6 +1165,9 @@ EXPORT_SYMBOL_GPL(gpiochip_remove);
>> ? * 0 if the device doesn't match and non-zero if it does. ?If the callback is
>> ? * non-zero, this function will return to the caller and not iterate over any
>> ? * more gpio_chips.
>> + *
>> + * Note the gpio_chip is returned with the module locked, users are responsible
>> + * to release the lock with gpiochip_put(chip) after using it.
>> ? */
>> ?struct gpio_chip *gpiochip_find(void *data,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int (*match)(struct gpio_chip *chip,
>> @@ -1169,6 +1184,8 @@ struct gpio_chip *gpiochip_find(void *data,
>>
>> ? ? ? ? ? ? ? if (match(gpio_desc[i].chip, data)) {
>> ? ? ? ? ? ? ? ? ? ? ? chip = gpio_desc[i].chip;
>> + ? ? ? ? ? ? ? ? ? ? if (!try_module_get(chip->owner))
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? chip = NULL;
>> ? ? ? ? ? ? ? ? ? ? ? break;
>> ? ? ? ? ? ? ? }
>> ? ? ? }
>> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
>> index 365ea09..af372be 100644
>> --- a/include/asm-generic/gpio.h
>> +++ b/include/asm-generic/gpio.h
>> @@ -145,7 +145,7 @@ extern int __must_check gpiochip_remove(struct gpio_chip *chip);
>> ?extern struct gpio_chip *gpiochip_find(void *data,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int (*match)(struct gpio_chip *chip,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *data));
>> -
>> +extern void gpiochip_put(struct gpio_chip *chip);
>>
>> ?/* Always use the library code for GPIO management calls,
>> ? * or when sleeping may be involved.
>> --
>> 1.7.0.4
>>
>>
>
> --
> Grant Likely, B.Sc, P.Eng.
> Secret Lab Technologies, Ltd.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/

[PATCH v4 4/6] gpio: introduce lock mechanism for gpiochip_find

[Dong Aisheng]

Hi Grant,

On Fri, May 25, 2012 at 06:25:00PM -0600, Grant Likely wrote:
> On Fri, 25 May 2012 21:36:18 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > The module lock will be automatically claimed for gpiochip_find function
> > in case the gpio module is removed during the using of gpiochip instance.
> > Users are responsible to call gpiochip_put to release the lock after
> > the using.
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> 
...
> Also, it doesn't do anything to protect against the gpio_chip being
> removed after the gpio number is resolved, which means the gpio number
> may no longer be valid, or may no longer point to the same gpio chip.
> It looks like the locking protection needs to be wider to be useful.
> 
I understand the issue now.
It's correct that we did not lock gpio_chip before calling gpio_request
after the gpio number is resolved.

I thought about adding a new API called of_gpio_request to hide the lock
to users like:
int of_gpio_request(..)
{
	spin_lock_irqsave(&gpio_lock, flags);
	ret = of_get_named_gpio(..);
	if (ret < 0)
		do_err..
	ret = gpio_request(..)

	spin_unlock_irqrestore(&gpio_lock, flags);
	return ret;
}
But it seems it does not work since the gpio_request may sleep and we may
need a new sleepable lock rather using the exist gpio_lock.

In the same time, i'm also thinking about a question that do we really
need to do this to protect gpio_chip being removed afer gpio number is
resolved?
My doubts is that gpio lib really does not block the gpiochip to be removed
before calling gpio_request, so why we need to do that for dt?
Maybe just let gpio_request to detect if gpio number is valid is already ok
for dt.

what's your suggestion on it?

Regards
Dong Aisheng

[PATCH v4 4/6] gpio: introduce lock mechanism for gpiochip_find

[Grant Likely]

On Wed, 30 May 2012 12:10:58 +0800, Dong Aisheng <dongas86@gmail.com> wrote:
> Hi Grant,
> 
> On Fri, May 25, 2012 at 06:25:00PM -0600, Grant Likely wrote:
> > On Fri, 25 May 2012 21:36:18 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > The module lock will be automatically claimed for gpiochip_find function
> > > in case the gpio module is removed during the using of gpiochip instance.
> > > Users are responsible to call gpiochip_put to release the lock after
> > > the using.
> > > 
> > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> ...
> > Also, it doesn't do anything to protect against the gpio_chip being
> > removed after the gpio number is resolved, which means the gpio number
> > may no longer be valid, or may no longer point to the same gpio chip.
> > It looks like the locking protection needs to be wider to be useful.
> > 
> I understand the issue now.
> It's correct that we did not lock gpio_chip before calling gpio_request
> after the gpio number is resolved.
> 
> I thought about adding a new API called of_gpio_request to hide the lock
> to users like:
> int of_gpio_request(..)
> {
> 	spin_lock_irqsave(&gpio_lock, flags);
> 	ret = of_get_named_gpio(..);
> 	if (ret < 0)
> 		do_err..
> 	ret = gpio_request(..)
> 
> 	spin_unlock_irqrestore(&gpio_lock, flags);
> 	return ret;
> }
> But it seems it does not work since the gpio_request may sleep and we may
> need a new sleepable lock rather using the exist gpio_lock.
> 
> In the same time, i'm also thinking about a question that do we really
> need to do this to protect gpio_chip being removed afer gpio number is
> resolved?

Probably not (for this series) because it shouldn't cause an oops if
it happens.  Ultimately however it would be good to have proper
reference counting on gpio chips to prevent any problems.

g.

[PATCH v4 5/6] of: add of_parse_phandle_with_args_ext function

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

The api is used when user wants to get some more extended args except ones
indicated by the cells_name.
For example, gpio cells is 2.
list = <&gpio 1 2 3 4>;
If users want to get extended args including 3, 4, he can call:
of_parse_phandle_with_args_ext(np, "gpios", "#gpio-cells", index, 2, &outargs);

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 drivers/of/base.c  |   25 ++++++++++++++++++++++++-
 include/linux/of.h |    5 ++++-
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 91c1fb4..e51ee5f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -893,6 +893,28 @@ int of_parse_phandle_with_args(struct device_node *np, const char *list_name,
 				const char *cells_name, int index,
 				struct of_phandle_args *out_args)
 {
+	return of_parse_phandle_with_args_ext(np, list_name, cells_name,
+					index, 0, out_args);
+}
+EXPORT_SYMBOL(of_parse_phandle_with_args);
+
+/*
+ * of_parse_phandle_with_args_ext() - Find a node pointed by phandle in a list
+ * with extended args.
+ * @np:		pointer to a device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cells_name:	property name that specifies phandles' arguments count
+ * @index:	index of a phandle to parse out
+ * @cells_ext:	property name that specifies the extended args count except
+ *		phandles' arguments count
+ * @out_args:	optional pointer to output arguments structure (will be filled)
+ */
+int of_parse_phandle_with_args_ext(struct device_node *np,
+				const char *list_name,
+				const char *cells_name, int index,
+				unsigned int cells_ext,
+				struct of_phandle_args *out_args)
+{
 	const __be32 *list, *list_end;
 	int size, cur_index = 0;
 	uint32_t count = 0;
@@ -936,6 +958,7 @@ int of_parse_phandle_with_args(struct device_node *np, const char *list_name,
 			 * Make sure that the arguments actually fit in the
 			 * remaining property data length
 			 */
+			count += cells_ext;
 			if (list + count > list_end) {
 				pr_err("%s: arguments longer than property\n",
 					 np->full_name);
@@ -978,7 +1001,7 @@ int of_parse_phandle_with_args(struct device_node *np, const char *list_name,
 		of_node_put(node);
 	return -EINVAL;
 }
-EXPORT_SYMBOL(of_parse_phandle_with_args);
+EXPORT_SYMBOL(of_parse_phandle_with_args_ext);
 
 /**
  * prom_add_property - Add a property to a node
diff --git a/include/linux/of.h b/include/linux/of.h
index 2ec1083..27b4d67 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -251,7 +251,10 @@ extern struct device_node *of_parse_phandle(struct device_node *np,
 extern int of_parse_phandle_with_args(struct device_node *np,
 	const char *list_name, const char *cells_name, int index,
 	struct of_phandle_args *out_args);
-
+extern int of_parse_phandle_with_args_ext(struct device_node *np,
+	const char *list_name, const char *cells_name, int index,
+	unsigned int cells_ext,
+	struct of_phandle_args *out_args);
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
-- 
1.7.0.4

[PATCH v4 5/6] of: add of_parse_phandle_with_args_ext function

[Stephen Warren]

On 05/25/2012 07:36 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> The api is used when user wants to get some more extended args except ones
> indicated by the cells_name.
> For example, gpio cells is 2.
> list = <&gpio 1 2 3 4>;

I assume either that property should be named "gpios", or ...

> If users want to get extended args including 3, 4, he can call:
> of_parse_phandle_with_args_ext(np, "gpios", "#gpio-cells", index, 2, &outargs);

The second parameter above should be "list".

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

This patch implements a standard common binding for pinctrl gpio ranges.
Each SoC can add gpio ranges through device tree by adding a gpio-maps property
under their pinctrl devices node with the format:
<&gpio $gpio-specifier $pin_offset $count>
while the gpio phandle and gpio-specifier are the standard approach
to represent a gpio in device tree.
Then we can cooperate it with the gpio xlate function to get the gpio number
from device tree to set up the gpio ranges map.

Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
to parse and register the gpio ranges from device tree.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
Personally i'm not very satisfied with current solution due to a few reasons:
1) i can not user standard gpio api to get gpio number
2) i need to reinvent a new api of_parse_phandles_with_args_ext which i'm not
sure if it can be accepted by DT maintainer.
If i did not invent that API, i need to rewrite a lot of duplicated code
with slight differences with the exist functions like of_get_named_gpio_flags
and of_parse_phandle_with_args for the special pinctrl gpio maps format.

So i just sent it out first to see people's comment and if any better solution.

One alternative solution is that that the gpio-maps into two parts:
pinctrl-gpios = <&gpio_phandle gpio-specifier ..>
pinctrl-gpio-maps = <pin_id count ..>
Then we can reuse the standard gpio api altough it's not better than the
original one.

Comments are welcome!

ChangeLog v3->v4:
* using standard gpio parsing approach to get the gpio number from device
  tree. The gpio-maps becomes a little slightly different as before:
  <&gpio $gpio_specifier $pin_offset $npin>.
  The $gpio_specifier length is controller dependent which is specified by
  '#gpio-cells' in in gpio controller node.

ChangeLog v2->v3:
* standardise the gpio ranges node name to 'gpio-maps'. Each SoC should use this
node name to define gpio ranges.
* defer probe if can not get gpiochip from node in case gpio driver is still
not loaded.
* some other minor fixes suggested by Stephen Warren.

ChangeLog v1->v2:
* introduce standard binding for gpio range.
---
 .../bindings/pinctrl/pinctrl-bindings.txt          |   22 +++++
 drivers/pinctrl/devicetree.c                       |   95 ++++++++++++++++++++
 include/linux/pinctrl/pinctrl.h                    |   11 +++
 3 files changed, 128 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index c95ea82..e999be5 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -126,3 +126,25 @@ device; they may be grandchildren, for example. Whether this is legal, and
 whether there is any interaction between the child and intermediate parent
 nodes, is again defined entirely by the binding for the individual pin
 controller device.
+
+=== pinctrl gpio ranges ===
+Some pins in pinctrl device can also be multiplexed as gpio. With a gpio range
+map, user can know which pin in the range can be used as gpio.
+
+Required properties:
+gpio-maps: integers array, each entry in the array represents a gpio range
+with the format: <&gpio $gpio-specifier $pin_offset $count>
+- gpio: phandle pointing at gpio device node
+- gpio-specifier: array of #gpio-cells specifying specific gpio, the length is
+  controller specific. Usually it may be two cells for simple gpio.
+- pin_offset: integer, the pin offset or pin id
+- npins: integer, the gpio ranges starting from pin_offset
+
+For example:
+pincontroller {
+	/* gpio-specifier is two cells */
+	gpio-maps = <&gpio1 0 0 0 32
+		     &gpio2 0 0 0 30
+		     &gpio3 1 0 100 1
+		     ....>;
+};
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fcb1de4..bc5f0f8 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -17,7 +17,9 @@
  */
 
 #include <linux/device.h>
+#include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/slab.h>
 
@@ -247,3 +249,96 @@ err:
 	pinctrl_dt_free_maps(p);
 	return ret;
 }
+
+/*
+ * pinctrl_dt_add_gpio_range() - parse and register GPIO ranges from device tree
+ * @pctldev: pin controller device to add the range to
+ * @np: the device node contains the gpio-maps node
+ *  The format of gpio-maps should be:
+ *  <&gpio $gpio-specifier $pin_offset $count>
+ *  The gpio-specifier length is controller dependent which is specified by
+ *  #gpio-cells in in gpio controller node.
+ */
+int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct device_node *np)
+{
+	struct pinctrl_gpio_range *ranges;
+	unsigned int gpio_offset, pin_offset, npins;
+	struct device_node *np_gpio;
+	struct of_phandle_args gpiospec;
+	struct property *prop;
+	const __be32 *list;
+	phandle phandle;
+	int nranges = 0;
+	int size;
+	int i;
+	int ret;
+
+	if (!np) {
+		dev_err(pctldev->dev, "no device node\n");
+		return -EINVAL;
+	}
+
+	/* count gpio ranges */
+	do {
+		ret = of_parse_phandle_with_args_ext(np, "gpio-maps", "#gpio-cells",
+						 nranges, 2, NULL);
+		if (ret)
+			break;
+	} while (++nranges);
+
+	if (!nranges) {
+		dev_err(pctldev->dev, "no gpio ranges found\n");
+		return -ENODEV;
+	}
+
+	/* setup gpio ranges table */
+	ranges = devm_kzalloc(pctldev->dev, nranges * sizeof(*ranges),
+				GFP_KERNEL);
+	for (i = 0; i < nranges; i++) {
+		ret = of_parse_phandle_with_args_ext(np, "gpio-maps", "#gpio-cells",
+						 i, 2, &gpiospec);
+		if (ret)
+			return ret;
+
+		ranges[i].gc = of_node_to_gpiochip(gpiospec.np);
+		if (!ranges[i].gc) {
+			dev_err(pctldev->dev,
+				"can not find gpio chip of node(%s)\n",
+				gpiospec.np->full_name);
+			ret = -EPROBE_DEFER;
+			goto out;
+		}
+
+		if (!ranges[i].gc->of_xlate) {
+			dev_err(pctldev->dev,
+				"no of_xlate function found for gpio(%s)\n",
+				gpiospec.np->full_name);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		ranges[i].name = dev_name(pctldev->dev);
+		ranges[i].base = ranges[i].gc->of_xlate(ranges[i].gc, &gpiospec, NULL);
+		if (ranges[i].base < 0) {
+			ret = -EINVAL;
+			goto out;
+		}
+		ranges[i].base += ranges[i].gc->base;
+		ranges[i].pin_base = gpiospec.args[gpiospec.args_count - 2];
+		ranges[i].npins = gpiospec.args[gpiospec.args_count - 1];
+
+		gpiochip_put(ranges[i].gc);
+		of_node_put(gpiospec.np);
+	}
+
+	pinctrl_add_gpio_ranges(pctldev, ranges, nranges);
+
+	return 0;
+
+out:
+	of_node_put(gpiospec.np);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pinctrl_dt_add_gpio_ranges);
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 3b894a6..947ab9f 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -135,6 +135,17 @@ extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 				struct pinctrl_gpio_range *range);
 extern const char *pinctrl_dev_get_name(struct pinctrl_dev *pctldev);
 extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
+#ifdef CONFIG_OF
+extern int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct device_node *np);
+#else
+static inline int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct device_node *np);
+{
+	return 0;
+}
+#endif	/* !CONFIG_OF */
+
 #else
 
 struct pinctrl_dev;
-- 
1.7.0.4

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/25/2012 07:36 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> This patch implements a standard common binding for pinctrl gpio ranges.
> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
> under their pinctrl devices node with the format:
> <&gpio $gpio-specifier $pin_offset $count>
> while the gpio phandle and gpio-specifier are the standard approach
> to represent a gpio in device tree.
> Then we can cooperate it with the gpio xlate function to get the gpio number
> from device tree to set up the gpio ranges map.
> 
> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
> to parse and register the gpio ranges from device tree.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
> Personally i'm not very satisfied with current solution due to a few reasons:
> 1) i can not user standard gpio api to get gpio number
> 2) i need to reinvent a new api of_parse_phandles_with_args_ext which i'm not
> sure if it can be accepted by DT maintainer.
> If i did not invent that API, i need to rewrite a lot of duplicated code
> with slight differences with the exist functions like of_get_named_gpio_flags
> and of_parse_phandle_with_args for the special pinctrl gpio maps format.
> 
> So i just sent it out first to see people's comment and if any better solution.
> 
> One alternative solution is that that the gpio-maps into two parts:
> pinctrl-gpios = <&gpio_phandle gpio-specifier ..>
> pinctrl-gpio-maps = <pin_id count ..>
> Then we can reuse the standard gpio api altough it's not better than the
> original one.

The problem I see with that is that it splits what is essentially a
single array with phandle+specifier+pin-id+count into two separate
arrays. Anyone reading/editing the DT needs to fully understand this,
and keep the entries in the two properties in the same order. Putting
everything into a single property makes this much more obvious to me. I
personally don't see any issue with the
of_parse_phandles_with_args_ext() function; it seems pretty clean to me.

> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c

> +	if (!nranges) {
> +		dev_err(pctldev->dev, "no gpio ranges found\n");
> +		return -ENODEV;
> +	}

In the case of a generic pinctrl IP block that can support an external
GPIO controller but happens not to be hooked up to one within a
particular SoC, that might not be an error. However, that situation is
pretty unlikely, so I think it's find to call dev_err() for now, and we
can change it later if we need.

> +		ranges[i].base = ranges[i].gc->of_xlate(ranges[i].gc, &gpiospec, NULL);

I believe Grant wants to change the of_xlate prototype in order to be
able to return a different gc value, so this will probably need slight
rework work with that change, once they're both approved. Still, I think
this is fine for now.

> +		if (ranges[i].base < 0) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		ranges[i].base += ranges[i].gc->base;
> +		ranges[i].pin_base = gpiospec.args[gpiospec.args_count - 2];
> +		ranges[i].npins = gpiospec.args[gpiospec.args_count - 1];
> +
> +		gpiochip_put(ranges[i].gc);

I wonder if this shouldn't happen until the pinctrl device is free'd,
and all the GPIO ranges are removed from it?

If we don't do that, I would argue that we shouldn't store ranges[i].gc,
since it might become invalid - I believe the only use of it is within
this function?

> +		of_node_put(gpiospec.np);
> +	}

Aside from the comments I've made, this series all seems reasonable.
There certainly are alternative ways of doing some of it, but I don't
see any other approach having any particular advantage over this one.
So, the series,

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Fri, May 25, 2012 at 10:03 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/25/2012 07:36 AM, Dong Aisheng wrote:
>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>
>> This patch implements a standard common binding for pinctrl gpio ranges.
>> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
>> under their pinctrl devices node with the format:
>> <&gpio $gpio-specifier $pin_offset $count>
>> while the gpio phandle and gpio-specifier are the standard approach
>> to represent a gpio in device tree.
>> Then we can cooperate it with the gpio xlate function to get the gpio number
>> from device tree to set up the gpio ranges map.
>>
>> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
>> to parse and register the gpio ranges from device tree.
>>
>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>> ---
>> Personally i'm not very satisfied with current solution due to a few reasons:
>> 1) i can not user standard gpio api to get gpio number
>> 2) i need to reinvent a new api of_parse_phandles_with_args_ext which i'm not
>> sure if it can be accepted by DT maintainer.
>> If i did not invent that API, i need to rewrite a lot of duplicated code
>> with slight differences with the exist functions like of_get_named_gpio_flags
>> and of_parse_phandle_with_args for the special pinctrl gpio maps format.
>>
>> So i just sent it out first to see people's comment and if any better solution.
>>
>> One alternative solution is that that the gpio-maps into two parts:
>> pinctrl-gpios = <&gpio_phandle gpio-specifier ..>
>> pinctrl-gpio-maps = <pin_id count ..>
>> Then we can reuse the standard gpio api altough it's not better than the
>> original one.
>
> The problem I see with that is that it splits what is essentially a
> single array with phandle+specifier+pin-id+count into two separate
> arrays. Anyone reading/editing the DT needs to fully understand this,
> and keep the entries in the two properties in the same order. Putting
> everything into a single property makes this much more obvious to me. I
Yes, i agree with you.
That's why i insisted to send this format first.

> personally don't see any issue with the
> of_parse_phandles_with_args_ext() function; it seems pretty clean to me.
>
Thanks, you gave me some confidence on it.

>> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
>
>> + ? ? if (!nranges) {
>> + ? ? ? ? ? ? dev_err(pctldev->dev, "no gpio ranges found\n");
>> + ? ? ? ? ? ? return -ENODEV;
>> + ? ? }
>
> In the case of a generic pinctrl IP block that can support an external
> GPIO controller but happens not to be hooked up to one within a
> particular SoC, that might not be an error. However, that situation is
> pretty unlikely, so I think it's find to call dev_err() for now, and we
> can change it later if we need.
>
>> + ? ? ? ? ? ? ranges[i].base = ranges[i].gc->of_xlate(ranges[i].gc, &gpiospec, NULL);
>
> I believe Grant wants to change the of_xlate prototype in order to be
> able to return a different gc value, so this will probably need slight
> rework work with that change, once they're both approved. Still, I think
> this is fine for now.
>
I looked Grant's commit 3d0f7cf0f "gpio: Adjust of_xlate API to
support multiple GPIO
chips", it seemed i need make some changes here since
of_node_to_gpiochip is broken now
after support banked gpio. Thus the gc got here may not correct for
some special gpio
controllers.

>> + ? ? ? ? ? ? if (ranges[i].base < 0) {
>> + ? ? ? ? ? ? ? ? ? ? ret = -EINVAL;
>> + ? ? ? ? ? ? ? ? ? ? goto out;
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ranges[i].base += ranges[i].gc->base;
>> + ? ? ? ? ? ? ranges[i].pin_base = gpiospec.args[gpiospec.args_count - 2];
>> + ? ? ? ? ? ? ranges[i].npins = gpiospec.args[gpiospec.args_count - 1];
>> +
>> + ? ? ? ? ? ? gpiochip_put(ranges[i].gc);
>
> I wonder if this shouldn't happen until the pinctrl device is free'd,
> and all the GPIO ranges are removed from it?
>
Hmm, that may bring some complexities since non-dt case also needs
to be covered if we do that...

> If we don't do that, I would argue that we shouldn't store ranges[i].gc,
> since it might become invalid - I believe the only use of it is within
> this function?
>
In my option, i think it's ok to store it since they're just some data
to describe
hw properties. The gpio function may become invalid but not data.
Is it reasonable to you?

>> + ? ? ? ? ? ? of_node_put(gpiospec.np);
>> + ? ? }
>
> Aside from the comments I've made, this series all seems reasonable.
> There certainly are alternative ways of doing some of it, but I don't
> see any other approach having any particular advantage over this one.
> So, the series,
>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
Thanks.

Regards
Dong Aisheng

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/26/2012 10:52 AM, Dong Aisheng wrote:
> On Fri, May 25, 2012 at 10:03 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 05/25/2012 07:36 AM, Dong Aisheng wrote:
...
>> If we don't do that, [lock ranges[i].gc] I would argue that we
>> shouldn't store ranges[i].gc, since it might become invalid - I
>> believe the only use of it is withinthis function?
>>
> In my option, i think it's ok to store it since they're just some data
> to describe
> hw properties. The gpio function may become invalid but not data.
> Is it reasonable to you?

The problem is that if someone tries to dereference the gc field, and
it's no longer valid, which could cause an OOPS. Perhaps we can get away
just with a comment in the struct definition indicating that this field
should only be used by drivers that provided the gc field directly
rather than having it set up by DT, but then why even store it when
creating the ranges from DT in that case?

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Sun, May 27, 2012 at 09:39:19AM -0600, Stephen Warren wrote:
> On 05/26/2012 10:52 AM, Dong Aisheng wrote:
> > On Fri, May 25, 2012 at 10:03 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >> On 05/25/2012 07:36 AM, Dong Aisheng wrote:
> ...
> >> If we don't do that, [lock ranges[i].gc] I would argue that we
> >> shouldn't store ranges[i].gc, since it might become invalid - I
> >> believe the only use of it is withinthis function?
> >>
> > In my option, i think it's ok to store it since they're just some data
> > to describe
> > hw properties. The gpio function may become invalid but not data.
> > Is it reasonable to you?
> 
> The problem is that if someone tries to dereference the gc field, and
> it's no longer valid, which could cause an OOPS. Perhaps we can get away
> just with a comment in the struct definition indicating that this field
> should only be used by drivers that provided the gc field directly
> rather than having it set up by DT, but then why even store it when
> creating the ranges from DT in that case?
Yes, you're right.
Maybe we could both not store the gc filed for DT (currently we did not see
the need to store it for dt, right?) and add a comment in the struct definition
as you said. For non-dt users the driver owner should manage that field
correctly with lock since it's provided directly by driver.
Is that ok?

Regards
Dong Aisheng

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/29/2012 09:01 PM, Dong Aisheng wrote:
> On Sun, May 27, 2012 at 09:39:19AM -0600, Stephen Warren wrote:
>> On 05/26/2012 10:52 AM, Dong Aisheng wrote:
>>> On Fri, May 25, 2012 at 10:03 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 05/25/2012 07:36 AM, Dong Aisheng wrote:
>> ...
>>>> If we don't do that, [lock ranges[i].gc] I would argue that we
>>>> shouldn't store ranges[i].gc, since it might become invalid - I
>>>> believe the only use of it is withinthis function?
>>>>
>>> In my option, i think it's ok to store it since they're just some data
>>> to describe
>>> hw properties. The gpio function may become invalid but not data.
>>> Is it reasonable to you?
>>
>> The problem is that if someone tries to dereference the gc field, and
>> it's no longer valid, which could cause an OOPS. Perhaps we can get away
>> just with a comment in the struct definition indicating that this field
>> should only be used by drivers that provided the gc field directly
>> rather than having it set up by DT, but then why even store it when
>> creating the ranges from DT in that case?
>
> Yes, you're right.
> Maybe we could both not store the gc filed for DT (currently we did not see
> the need to store it for dt, right?) and add a comment in the struct definition
> as you said. For non-dt users the driver owner should manage that field
> correctly with lock since it's provided directly by driver.
> Is that ok?

Yes, that makes sense to me. Thanks.

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Grant Likely]

On Fri, 25 May 2012 11:03:48 -0600, Stephen Warren <swarren@wwwdotorg.org> wrote:
> I believe Grant wants to change the of_xlate prototype in order to be
> able to return a different gc value, so this will probably need slight
> rework work with that change, once they're both approved. Still, I think
> this is fine for now.

I took a different approach.  Now multiple gpio_chips can use the same
of node, and of_xlate needs to match on the entire gpio specifier, not
just the node.  This code is now in mainline.

g.

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Grant Likely]

On Fri, 25 May 2012 21:36:20 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> This patch implements a standard common binding for pinctrl gpio ranges.
> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
> under their pinctrl devices node with the format:
> <&gpio $gpio-specifier $pin_offset $count>
> while the gpio phandle and gpio-specifier are the standard approach
> to represent a gpio in device tree.
> Then we can cooperate it with the gpio xlate function to get the gpio number
> from device tree to set up the gpio ranges map.
> 
> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
> to parse and register the gpio ranges from device tree.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
> Personally i'm not very satisfied with current solution due to a few reasons:
> 1) i can not user standard gpio api to get gpio number
> 2) i need to reinvent a new api of_parse_phandles_with_args_ext which i'm not
> sure if it can be accepted by DT maintainer.

Right, as mentioned in my other email, doing it this way completely
breaks the way the phandle-with-args pattern works.  That pattern
depends on the phandle node to have a #-cells property telling it how
many cells to process for the binding.  Adding additional data cells
means the kernel is no longer able to parse multiple entries in the
gpios property.

> If i did not invent that API, i need to rewrite a lot of duplicated code
> with slight differences with the exist functions like of_get_named_gpio_flags
> and of_parse_phandle_with_args for the special pinctrl gpio maps format.
> 
> So i just sent it out first to see people's comment and if any better solution.
> 
> One alternative solution is that that the gpio-maps into two parts:
> pinctrl-gpios = <&gpio_phandle gpio-specifier ..>
> pinctrl-gpio-maps = <pin_id count ..>
> Then we can reuse the standard gpio api altough it's not better than the
> original one.
> 
> Comments are welcome!
> 
> ChangeLog v3->v4:
> * using standard gpio parsing approach to get the gpio number from device
>   tree. The gpio-maps becomes a little slightly different as before:
>   <&gpio $gpio_specifier $pin_offset $npin>.
>   The $gpio_specifier length is controller dependent which is specified by
>   '#gpio-cells' in in gpio controller node.
> 
> ChangeLog v2->v3:
> * standardise the gpio ranges node name to 'gpio-maps'. Each SoC should use this
> node name to define gpio ranges.
> * defer probe if can not get gpiochip from node in case gpio driver is still
> not loaded.
> * some other minor fixes suggested by Stephen Warren.
> 
> ChangeLog v1->v2:
> * introduce standard binding for gpio range.
> ---
>  .../bindings/pinctrl/pinctrl-bindings.txt          |   22 +++++
>  drivers/pinctrl/devicetree.c                       |   95 ++++++++++++++++++++
>  include/linux/pinctrl/pinctrl.h                    |   11 +++
>  3 files changed, 128 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> index c95ea82..e999be5 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> @@ -126,3 +126,25 @@ device; they may be grandchildren, for example. Whether this is legal, and
>  whether there is any interaction between the child and intermediate parent
>  nodes, is again defined entirely by the binding for the individual pin
>  controller device.
> +
> +=== pinctrl gpio ranges ===
> +Some pins in pinctrl device can also be multiplexed as gpio. With a gpio range
> +map, user can know which pin in the range can be used as gpio.
> +
> +Required properties:
> +gpio-maps: integers array, each entry in the array represents a gpio range
> +with the format: <&gpio $gpio-specifier $pin_offset $count>
> +- gpio: phandle pointing at gpio device node
> +- gpio-specifier: array of #gpio-cells specifying specific gpio, the length is
> +  controller specific. Usually it may be two cells for simple gpio.
> +- pin_offset: integer, the pin offset or pin id
> +- npins: integer, the gpio ranges starting from pin_offset
> +
> +For example:
> +pincontroller {
> +	/* gpio-specifier is two cells */
> +	gpio-maps = <&gpio1 0 0 0 32
> +		     &gpio2 0 0 0 30
> +		     &gpio3 1 0 100 1
> +		     ....>;

Hmmm.... I need more information about this gpio-maps property.  How
is it arranged?  What kind of data is in it.  Can you give some
specific examples of how hardware would be described with a gpio-maps
property?

g.

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Fri, May 25, 2012 at 5:29 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, 25 May 2012 21:36:20 +0800, Dong Aisheng <b29396@freescale.com> wrote:
>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>
>> This patch implements a standard common binding for pinctrl gpio ranges.
>> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
>> under their pinctrl devices node with the format:
>> <&gpio $gpio-specifier $pin_offset $count>
>> while the gpio phandle and gpio-specifier are the standard approach
>> to represent a gpio in device tree.
>> Then we can cooperate it with the gpio xlate function to get the gpio number
>> from device tree to set up the gpio ranges map.
>>
>> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
>> to parse and register the gpio ranges from device tree.
>>
>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>> ---
>> Personally i'm not very satisfied with current solution due to a few reasons:
>> 1) i can not user standard gpio api to get gpio number
>> 2) i need to reinvent a new api of_parse_phandles_with_args_ext which i'm not
>> sure if it can be accepted by DT maintainer.
>
> Right, as mentioned in my other email, doing it this way completely
> breaks the way the phandle-with-args pattern works. ?That pattern
> depends on the phandle node to have a #-cells property telling it how
> many cells to process for the binding. ?Adding additional data cells
> means the kernel is no longer able to parse multiple entries in the
> gpios property.
Hmm, it can still parse multiple entries in the gpios property except
that it adds two args although it's not related to gpio, but it is useful
for users for special case like pinctrl gpio ranges map.

>
>> If i did not invent that API, i need to rewrite a lot of duplicated code
>> with slight differences with the exist functions like of_get_named_gpio_flags
>> and of_parse_phandle_with_args for the special pinctrl gpio maps format.
>>
>> So i just sent it out first to see people's comment and if any better solution.
>>
>> One alternative solution is that that the gpio-maps into two parts:
>> pinctrl-gpios = <&gpio_phandle gpio-specifier ..>
>> pinctrl-gpio-maps = <pin_id count ..>
>> Then we can reuse the standard gpio api altough it's not better than the
>> original one.
>>
>> Comments are welcome!
>>
>> ChangeLog v3->v4:
>> * using standard gpio parsing approach to get the gpio number from device
>> ? tree. The gpio-maps becomes a little slightly different as before:
>> ? <&gpio $gpio_specifier $pin_offset $npin>.
>> ? The $gpio_specifier length is controller dependent which is specified by
>> ? '#gpio-cells' in in gpio controller node.
>>
>> ChangeLog v2->v3:
>> * standardise the gpio ranges node name to 'gpio-maps'. Each SoC should use this
>> node name to define gpio ranges.
>> * defer probe if can not get gpiochip from node in case gpio driver is still
>> not loaded.
>> * some other minor fixes suggested by Stephen Warren.
>>
>> ChangeLog v1->v2:
>> * introduce standard binding for gpio range.
>> ---
>> ?.../bindings/pinctrl/pinctrl-bindings.txt ? ? ? ? ?| ? 22 +++++
>> ?drivers/pinctrl/devicetree.c ? ? ? ? ? ? ? ? ? ? ? | ? 95 ++++++++++++++++++++
>> ?include/linux/pinctrl/pinctrl.h ? ? ? ? ? ? ? ? ? ?| ? 11 +++
>> ?3 files changed, 128 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> index c95ea82..e999be5 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> @@ -126,3 +126,25 @@ device; they may be grandchildren, for example. Whether this is legal, and
>> ?whether there is any interaction between the child and intermediate parent
>> ?nodes, is again defined entirely by the binding for the individual pin
>> ?controller device.
>> +
>> +=== pinctrl gpio ranges ===
>> +Some pins in pinctrl device can also be multiplexed as gpio. With a gpio range
>> +map, user can know which pin in the range can be used as gpio.
>> +
>> +Required properties:
>> +gpio-maps: integers array, each entry in the array represents a gpio range
>> +with the format: <&gpio $gpio-specifier $pin_offset $count>
>> +- gpio: phandle pointing at gpio device node
>> +- gpio-specifier: array of #gpio-cells specifying specific gpio, the length is
>> + ?controller specific. Usually it may be two cells for simple gpio.
>> +- pin_offset: integer, the pin offset or pin id
>> +- npins: integer, the gpio ranges starting from pin_offset
>> +
>> +For example:
>> +pincontroller {
>> + ? ? /* gpio-specifier is two cells */
>> + ? ? gpio-maps = <&gpio1 0 0 0 32
>> + ? ? ? ? ? ? ? ? ?&gpio2 0 0 0 30
>> + ? ? ? ? ? ? ? ? ?&gpio3 1 0 100 1
>> + ? ? ? ? ? ? ? ? ?....>;
>
> Hmmm.... I need more information about this gpio-maps property. ?How
> is it arranged? ?What kind of data is in it. ?Can you give some
> specific examples of how hardware would be described with a gpio-maps
> property?
>
For exampe:
MX6Q_PAD_SD2_DAT2__GPIO_1_13 means MX6Q_PAD_SD2_DAT2 can be used as GPIO_1_13,
For reference gpio1,13, we usually do: xx-gpios = <gpio1 13 0> in device tree.
Here we want to create a pin map of gpio1,13 to MX6Q_PAD_SD2_DAT2 for
pinctrl gpio ranges map,
the format should be <GPIO_NUMBER PIN_ID NPINS>, then the pinctrl core
can automatically mux
the PIN_ID to gpio function by refer to this map.
For GPIO_NUMBER, we want to use the standard gpio dt represent way
since the gpio base may
be dynamically.
Assume MX6Q_PAD_SD2_DAT2 pin id is 1 and only one pin starting from it
can be used as gpio.
Then the gpio-maps for MX6Q_PAD_SD2_DAT2 can be:
gpio-maps = <gpio1 13 0 1 1>

We may have several pins can be used as gpio on mx6q.
Then the gpio-maps may becomes:
gpio-maps = <gpio1 13 0 1 1>,
                   <gpio1 14 0 5 1>,
                   <gpio2 0 0 20 1>,
                   ................

Since the format is a little different from the standard gpio
represent way, so i can not use the standard gpio
api to parse the gpio number. That's why i need to invent
of_parse_phandle_args_ext function for this special
format.

we still did not find any better way to do that.
Do you have any suggestion for this special case?

Regards
Dong Aisheng

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Grant Likely]

On Sat, 26 May 2012 09:58:06 -0700, Dong Aisheng <dongas86@gmail.com> wrote:
> On Fri, May 25, 2012 at 5:29 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Fri, 25 May 2012 21:36:20 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> >> From: Dong Aisheng <dong.aisheng@linaro.org>
> >>
> >> This patch implements a standard common binding for pinctrl gpio ranges.
> >> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
> >> under their pinctrl devices node with the format:
> >> <&gpio $gpio-specifier $pin_offset $count>
> >> while the gpio phandle and gpio-specifier are the standard approach
> >> to represent a gpio in device tree.
> >> Then we can cooperate it with the gpio xlate function to get the gpio number
> >> from device tree to set up the gpio ranges map.
> >>
> >> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
> >> to parse and register the gpio ranges from device tree.
> >>
> >> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> >> ---
> >> Personally i'm not very satisfied with current solution due to a few reasons:
> >> 1) i can not user standard gpio api to get gpio number
> >> 2) i need to reinvent a new api of_parse_phandles_with_args_ext which i'm not
> >> sure if it can be accepted by DT maintainer.
> >
> > Right, as mentioned in my other email, doing it this way completely
> > breaks the way the phandle-with-args pattern works. ??That pattern
> > depends on the phandle node to have a #-cells property telling it how
> > many cells to process for the binding. ??Adding additional data cells
> > means the kernel is no longer able to parse multiple entries in the
> > gpios property.
> Hmm, it can still parse multiple entries in the gpios property except
> that it adds two args although it's not related to gpio, but it is useful
> for users for special case like pinctrl gpio ranges map.

Really?  How exactly does it know that each record is longer than
#gpio-cells specifies (I'm speaking from the binding level; not having
custom code that just "knows" the the records have additional
padding).

I have no interest in creating exceptions to the phandle-with-args
pattern since it adds yet more implicit knowledge about how to parse.
For example, the common gpio code can no longer parse a gpios property
that is padded out because the common code doesn't know anything about
padding.

g.

> > Hmmm.... I need more information about this gpio-maps property. ??How
> > is it arranged? ??What kind of data is in it. ??Can you give some
> > specific examples of how hardware would be described with a gpio-maps
> > property?
> >
> For exampe:
> MX6Q_PAD_SD2_DAT2__GPIO_1_13 means MX6Q_PAD_SD2_DAT2 can be used as GPIO_1_13,
> For reference gpio1,13, we usually do: xx-gpios = <gpio1 13 0> in device tree.
> Here we want to create a pin map of gpio1,13 to MX6Q_PAD_SD2_DAT2 for
> pinctrl gpio ranges map,
> the format should be <GPIO_NUMBER PIN_ID NPINS>, then the pinctrl core
> can automatically mux
> the PIN_ID to gpio function by refer to this map.
> For GPIO_NUMBER, we want to use the standard gpio dt represent way
> since the gpio base may
> be dynamically.
> Assume MX6Q_PAD_SD2_DAT2 pin id is 1 and only one pin starting from it
> can be used as gpio.
> Then the gpio-maps for MX6Q_PAD_SD2_DAT2 can be:
> gpio-maps = <gpio1 13 0 1 1>
> 
> We may have several pins can be used as gpio on mx6q.
> Then the gpio-maps may becomes:
> gpio-maps = <gpio1 13 0 1 1>,
>                    <gpio1 14 0 5 1>,
>                    <gpio2 0 0 20 1>,
>                    ................
> 
> Since the format is a little different from the standard gpio
> represent way, so i can not use the standard gpio
> api to parse the gpio number. That's why i need to invent
> of_parse_phandle_args_ext function for this special
> format.
> 
> we still did not find any better way to do that.
> Do you have any suggestion for this special case?

Oh, I see....  Does this gpio-maps property sit beside a normal
"gpios" property?  Or is it in a completely separate node?  If it sits
beside a normal "gpios" property and lines up with the gpio properties
there, then I would just make it a tuple for each gpio.  Ie:

gpios = <&gpio1 13 0>, <&gpio1 14 0>, <&gpio2 0 0>;
gpio-pinmux = <1 1>,   <5 1>,         <20 1>;

g.

[PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Wed, May 30, 2012 at 02:46:41PM +0800, Grant Likely wrote:
> On Sat, 26 May 2012 09:58:06 -0700, Dong Aisheng <dongas86@gmail.com> wrote:
> > On Fri, May 25, 2012 at 5:29 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > > On Fri, 25 May 2012 21:36:20 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> > >> From: Dong Aisheng <dong.aisheng@linaro.org>
> > >>
> > >> This patch implements a standard common binding for pinctrl gpio ranges.
> > >> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
> > >> under their pinctrl devices node with the format:
> > >> <&gpio $gpio-specifier $pin_offset $count>
> > >> while the gpio phandle and gpio-specifier are the standard approach
> > >> to represent a gpio in device tree.
> > >> Then we can cooperate it with the gpio xlate function to get the gpio number
> > >> from device tree to set up the gpio ranges map.
> > >>
> > >> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
> > >> to parse and register the gpio ranges from device tree.
> > >>
> > >> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > >> ---
> > >> Personally i'm not very satisfied with current solution due to a few reasons:
> > >> 1) i can not user standard gpio api to get gpio number
> > >> 2) i need to reinvent a new api of_parse_phandles_with_args_ext which i'm not
> > >> sure if it can be accepted by DT maintainer.
> > >
> > > Right, as mentioned in my other email, doing it this way completely
> > > breaks the way the phandle-with-args pattern works. ?That pattern
> > > depends on the phandle node to have a #-cells property telling it how
> > > many cells to process for the binding. ?Adding additional data cells
> > > means the kernel is no longer able to parse multiple entries in the
> > > gpios property.
> > Hmm, it can still parse multiple entries in the gpios property except
> > that it adds two args although it's not related to gpio, but it is useful
> > for users for special case like pinctrl gpio ranges map.
> 
> Really?  How exactly does it know that each record is longer than
> #gpio-cells specifies (I'm speaking from the binding level; not having
> custom code that just "knows" the the records have additional
> padding).
> 
I'm not sure i understood your question correctly,
but yes, binding level does not know it.
Customer should call the correct of_parse_phandle_with_args or
of_parse_phandles_with_args_ext to do parsing.

> I have no interest in creating exceptions to the phandle-with-args
> pattern since it adds yet more implicit knowledge about how to parse.
Yes, i admit that's a problem.
But i did not know if any better solution.

> For example, the common gpio code can no longer parse a gpios property
> that is padded out because the common code doesn't know anything about
> padding.
> 
I guess the common gpio code won't break since we already have the
standard gpio format for dt.
Users shouldn't use the wrong format to represent gpios.

> g.
> 
> > > Hmmm.... I need more information about this gpio-maps property. ?How
> > > is it arranged? ?What kind of data is in it. ?Can you give some
> > > specific examples of how hardware would be described with a gpio-maps
> > > property?
> > >
> > For exampe:
> > MX6Q_PAD_SD2_DAT2__GPIO_1_13 means MX6Q_PAD_SD2_DAT2 can be used as GPIO_1_13,
> > For reference gpio1,13, we usually do: xx-gpios = <gpio1 13 0> in device tree.
> > Here we want to create a pin map of gpio1,13 to MX6Q_PAD_SD2_DAT2 for
> > pinctrl gpio ranges map,
> > the format should be <GPIO_NUMBER PIN_ID NPINS>, then the pinctrl core
> > can automatically mux
> > the PIN_ID to gpio function by refer to this map.
> > For GPIO_NUMBER, we want to use the standard gpio dt represent way
> > since the gpio base may
> > be dynamically.
> > Assume MX6Q_PAD_SD2_DAT2 pin id is 1 and only one pin starting from it
> > can be used as gpio.
> > Then the gpio-maps for MX6Q_PAD_SD2_DAT2 can be:
> > gpio-maps = <gpio1 13 0 1 1>
> > 
> > We may have several pins can be used as gpio on mx6q.
> > Then the gpio-maps may becomes:
> > gpio-maps = <gpio1 13 0 1 1>,
> >                    <gpio1 14 0 5 1>,
> >                    <gpio2 0 0 20 1>,
> >                    ................
> > 
> > Since the format is a little different from the standard gpio
> > represent way, so i can not use the standard gpio
> > api to parse the gpio number. That's why i need to invent
> > of_parse_phandle_args_ext function for this special
> > format.
> > 
> > we still did not find any better way to do that.
> > Do you have any suggestion for this special case?
> 
> Oh, I see....  Does this gpio-maps property sit beside a normal
> "gpios" property?  Or is it in a completely separate node?  If it sits
no, currently it is in a completely separate node and is recommended
to be under pinctrl device node since pinctrl driver needs to use it.

> beside a normal "gpios" property and lines up with the gpio properties
> there, then I would just make it a tuple for each gpio.  Ie:
> 
> gpios = <&gpio1 13 0>, <&gpio1 14 0>, <&gpio2 0 0>;
> gpio-pinmux = <1 1>,   <5 1>,         <20 1>;
> 
I've had the same idea before but seemed Stephen did not like that since
it's nature to put them at the same line with the format of
<GPIO PIN_ID COUNT>. (Here the GPIO is standard gpio dt format)
See:
http://www.spinics.net/lists/arm-kernel/msg176830.html

And above format may be hard to read if the tuples get big.

Regards
Dong Aisheng

[PATCH v4 1/6] gpio: fix a typo of comment message

[Grant Likely]

On Fri, 25 May 2012 21:36:15 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

Applied, thanks.

g.

> ---
>  drivers/gpio/gpiolib-of.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index d18068a..8389d4a 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -21,7 +21,7 @@
>  #include <linux/of_gpio.h>
>  #include <linux/slab.h>
>  
> -/* Private data structure for of_gpiochip_is_match */
> +/* Private data structure for of_gpiochip_find_and_xlate */
>  struct gg_data {
>  	enum of_gpio_flags *flags;
>  	struct of_phandle_args gpiospec;
> -- 
> 1.7.0.4
> 
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.