[PATCH RFC v2 1/2] pinctrl: add pinctrl_add_gpio_ranges function

[PATCH RFC v2 1/2] pinctrl: add pinctrl_add_gpio_ranges function

[Dong Aisheng]

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

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 drivers/pinctrl/core.c          |   22 ++++++++++++++++++++++
 include/linux/pinctrl/pinctrl.h |    6 ++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c3b331b..916ed49 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -346,6 +346,28 @@ void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 }
 EXPORT_SYMBOL_GPL(pinctrl_remove_gpio_range);
 
+void pinctrl_add_gpio_ranges(struct pinctrl_dev *pctldev,
+			     struct pinctrl_gpio_range *ranges,
+			     unsigned nranges)
+{
+	int i;
+
+	for (i = 0; i < nranges; i++)
+		pinctrl_add_gpio_range(pctldev, &ranges[i]);
+}
+EXPORT_SYMBOL_GPL(pinctrl_add_gpio_ranges);
+
+void pinctrl_remove_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct pinctrl_gpio_range *ranges,
+				unsigned nranges)
+{
+	int i;
+
+	for (i = 0; i < nranges; i++)
+		pinctrl_remove_gpio_range(pctldev, &ranges[i]);
+}
+EXPORT_SYMBOL_GPL(pinctrl_remove_gpio_ranges);
+
 /**
  * pinctrl_get_group_selector() - returns the group selector for a group
  * @pctldev: the pin controller handling the group
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 3b894a6..6a29965 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -133,6 +133,12 @@ extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
 				struct pinctrl_gpio_range *range);
 extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 				struct pinctrl_gpio_range *range);
+extern void pinctrl_add_gpio_ranges(struct pinctrl_dev *pctldev,
+			     struct pinctrl_gpio_range *ranges,
+			     unsigned nranges);
+extern void pinctrl_remove_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct pinctrl_gpio_range *ranges,
+				unsigned nranges);
 extern const char *pinctrl_dev_get_name(struct pinctrl_dev *pctldev);
 extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
 #else
-- 
1.7.0.4

[PATCH RFC v2 1/2] pinctrl: add pinctrl_add_gpio_ranges function

[Dong Aisheng]

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

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 drivers/pinctrl/core.c          |   22 ++++++++++++++++++++++
 include/linux/pinctrl/pinctrl.h |    6 ++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c3b331b..916ed49 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -346,6 +346,28 @@ void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 }
 EXPORT_SYMBOL_GPL(pinctrl_remove_gpio_range);
 
+void pinctrl_add_gpio_ranges(struct pinctrl_dev *pctldev,
+			     struct pinctrl_gpio_range *ranges,
+			     unsigned nranges)
+{
+	int i;
+
+	for (i = 0; i < nranges; i++)
+		pinctrl_add_gpio_range(pctldev, &ranges[i]);
+}
+EXPORT_SYMBOL_GPL(pinctrl_add_gpio_ranges);
+
+void pinctrl_remove_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct pinctrl_gpio_range *ranges,
+				unsigned nranges)
+{
+	int i;
+
+	for (i = 0; i < nranges; i++)
+		pinctrl_remove_gpio_range(pctldev, &ranges[i]);
+}
+EXPORT_SYMBOL_GPL(pinctrl_remove_gpio_ranges);
+
 /**
  * pinctrl_get_group_selector() - returns the group selector for a group
  * @pctldev: the pin controller handling the group
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 3b894a6..6a29965 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -133,6 +133,12 @@ extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
 				struct pinctrl_gpio_range *range);
 extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 				struct pinctrl_gpio_range *range);
+extern void pinctrl_add_gpio_ranges(struct pinctrl_dev *pctldev,
+			     struct pinctrl_gpio_range *ranges,
+			     unsigned nranges);
+extern void pinctrl_remove_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct pinctrl_gpio_range *ranges,
+				unsigned nranges);
 extern const char *pinctrl_dev_get_name(struct pinctrl_dev *pctldev);
 extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
 #else
-- 
1.7.0.4

[PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

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

The gpio ranges standard dt binding format is
<&gpio $gpio_offset $pin_offset $npin>

The core will parse and register the pinctrl gpio ranges
from device tree.

TODO: add binding doc.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

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

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fcb1de4..c1b87e6 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 property @propname
+ * @propname: a list of phandles of gpios and corresponding data.
+ *  The format is: <&gpio0 $gpio_offset $pin_offset $count>
+ */
+int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct device_node *np,
+				const char *propname)
+{
+	struct pinctrl_gpio_range *ranges;
+	unsigned int gpio_offset, pin_offset, npins;
+	struct device_node *np_gpio;
+	struct property *prop;
+	const __be32 *list;
+	phandle phandle;
+	int nranges;
+	int size;
+	int i;
+
+	if (!np || !propname) {
+		dev_err(pctldev->dev, "no device node or gpios propname\n");
+		return -EINVAL;
+	}
+
+	prop = of_find_property(np, propname, &size);
+	if (!prop)
+		return -ENODEV;
+
+	list = prop->value;
+	size /= sizeof(*list);
+	if (size % 4) {
+		dev_err(pctldev->dev, "wrong gpio range table format\n");
+		return -EINVAL;
+	}
+
+	nranges = size /4;
+	ranges = devm_kzalloc(pctldev->dev, nranges * sizeof(*ranges),
+				GFP_KERNEL);
+	for (i = 0; i < nranges; i++) {
+			phandle = be32_to_cpup(list++);
+			np_gpio = of_find_node_by_phandle(phandle);
+			if (!np_gpio) {
+				dev_err(pctldev->dev,
+					"failed to find gpio node(%s)\n",
+					np_gpio->name);
+				return -ENODEV;
+			}
+
+			ranges[i].gc = of_node_to_gpiochip(np_gpio);
+			if (!ranges[i].gc) {
+				dev_err(pctldev->dev,
+					"can not find gpio chip of node(%s)\n",
+					np->name);
+				of_node_put(np_gpio);
+				return -ENODEV;
+			}
+
+			gpio_offset = be32_to_cpu(*list++);
+			pin_offset = be32_to_cpu(*list++);
+			npins = be32_to_cpu(*list++);
+
+			if (gpio_offset < 0 ||
+			    gpio_offset > ranges[i].gc->ngpio ||
+			    pin_offset < 0 || npins < 0) {
+				dev_err(pctldev->dev,
+					"wrong data in the gpio range table\n");
+				of_node_put(np_gpio);
+				return -ENODEV;
+			}
+
+			ranges[i].name = dev_name(pctldev->dev);
+			ranges[i].base = ranges[i].gc->base + gpio_offset;
+			ranges[i].pin_base = pin_offset;
+			ranges[i].npins = npins;
+			of_node_put(np_gpio);
+	}
+
+	pinctrl_add_gpio_ranges(pctldev, ranges, nranges);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_dt_add_gpio_ranges);
+
+void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
+				   struct pinctrl_gpio_range *ranges,
+				   unsigned nranges)
+{
+	pinctrl_remove_gpio_ranges(pctldev, ranges, nranges);
+}
+EXPORT_SYMBOL_GPL(pinctrl_dt_remove_gpio_ranges);
diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
index 760bc49..9f79657 100644
--- a/drivers/pinctrl/devicetree.h
+++ b/drivers/pinctrl/devicetree.h
@@ -20,9 +20,7 @@
 
 void pinctrl_dt_free_maps(struct pinctrl *p);
 int pinctrl_dt_to_map(struct pinctrl *p);
-
 #else
-
 static inline int pinctrl_dt_to_map(struct pinctrl *p)
 {
 	return 0;
@@ -31,5 +29,4 @@ static inline int pinctrl_dt_to_map(struct pinctrl *p)
 static inline void pinctrl_dt_free_maps(struct pinctrl *p)
 {
 }
-
 #endif
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 6a29965..98d77b4 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -141,6 +141,28 @@ extern void pinctrl_remove_gpio_ranges(struct pinctrl_dev *pctldev,
 				unsigned nranges);
 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,
+				const char *propname);
+extern void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
+				   struct pinctrl_gpio_range *ranges,
+				   unsigned nranges);
+#else
+static inline int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct device_node *np,
+				const char *propname)
+{
+	return 0;
+}
+static void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
+				   struct pinctrl_gpio_range *ranges,
+				   unsigned nranges)
+{
+}
+#endif	/* !CONFIG_OF */
+
 #else
 
 struct pinctrl_dev;
-- 
1.7.0.4

[PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

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

The gpio ranges standard dt binding format is
<&gpio $gpio_offset $pin_offset $npin>

The core will parse and register the pinctrl gpio ranges
from device tree.

TODO: add binding doc.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

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

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fcb1de4..c1b87e6 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 property @propname
+ * @propname: a list of phandles of gpios and corresponding data.
+ *  The format is: <&gpio0 $gpio_offset $pin_offset $count>
+ */
+int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct device_node *np,
+				const char *propname)
+{
+	struct pinctrl_gpio_range *ranges;
+	unsigned int gpio_offset, pin_offset, npins;
+	struct device_node *np_gpio;
+	struct property *prop;
+	const __be32 *list;
+	phandle phandle;
+	int nranges;
+	int size;
+	int i;
+
+	if (!np || !propname) {
+		dev_err(pctldev->dev, "no device node or gpios propname\n");
+		return -EINVAL;
+	}
+
+	prop = of_find_property(np, propname, &size);
+	if (!prop)
+		return -ENODEV;
+
+	list = prop->value;
+	size /= sizeof(*list);
+	if (size % 4) {
+		dev_err(pctldev->dev, "wrong gpio range table format\n");
+		return -EINVAL;
+	}
+
+	nranges = size /4;
+	ranges = devm_kzalloc(pctldev->dev, nranges * sizeof(*ranges),
+				GFP_KERNEL);
+	for (i = 0; i < nranges; i++) {
+			phandle = be32_to_cpup(list++);
+			np_gpio = of_find_node_by_phandle(phandle);
+			if (!np_gpio) {
+				dev_err(pctldev->dev,
+					"failed to find gpio node(%s)\n",
+					np_gpio->name);
+				return -ENODEV;
+			}
+
+			ranges[i].gc = of_node_to_gpiochip(np_gpio);
+			if (!ranges[i].gc) {
+				dev_err(pctldev->dev,
+					"can not find gpio chip of node(%s)\n",
+					np->name);
+				of_node_put(np_gpio);
+				return -ENODEV;
+			}
+
+			gpio_offset = be32_to_cpu(*list++);
+			pin_offset = be32_to_cpu(*list++);
+			npins = be32_to_cpu(*list++);
+
+			if (gpio_offset < 0 ||
+			    gpio_offset > ranges[i].gc->ngpio ||
+			    pin_offset < 0 || npins < 0) {
+				dev_err(pctldev->dev,
+					"wrong data in the gpio range table\n");
+				of_node_put(np_gpio);
+				return -ENODEV;
+			}
+
+			ranges[i].name = dev_name(pctldev->dev);
+			ranges[i].base = ranges[i].gc->base + gpio_offset;
+			ranges[i].pin_base = pin_offset;
+			ranges[i].npins = npins;
+			of_node_put(np_gpio);
+	}
+
+	pinctrl_add_gpio_ranges(pctldev, ranges, nranges);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_dt_add_gpio_ranges);
+
+void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
+				   struct pinctrl_gpio_range *ranges,
+				   unsigned nranges)
+{
+	pinctrl_remove_gpio_ranges(pctldev, ranges, nranges);
+}
+EXPORT_SYMBOL_GPL(pinctrl_dt_remove_gpio_ranges);
diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
index 760bc49..9f79657 100644
--- a/drivers/pinctrl/devicetree.h
+++ b/drivers/pinctrl/devicetree.h
@@ -20,9 +20,7 @@
 
 void pinctrl_dt_free_maps(struct pinctrl *p);
 int pinctrl_dt_to_map(struct pinctrl *p);
-
 #else
-
 static inline int pinctrl_dt_to_map(struct pinctrl *p)
 {
 	return 0;
@@ -31,5 +29,4 @@ static inline int pinctrl_dt_to_map(struct pinctrl *p)
 static inline void pinctrl_dt_free_maps(struct pinctrl *p)
 {
 }
-
 #endif
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 6a29965..98d77b4 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -141,6 +141,28 @@ extern void pinctrl_remove_gpio_ranges(struct pinctrl_dev *pctldev,
 				unsigned nranges);
 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,
+				const char *propname);
+extern void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
+				   struct pinctrl_gpio_range *ranges,
+				   unsigned nranges);
+#else
+static inline int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
+				struct device_node *np,
+				const char *propname)
+{
+	return 0;
+}
+static void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
+				   struct pinctrl_gpio_range *ranges,
+				   unsigned nranges)
+{
+}
+#endif	/* !CONFIG_OF */
+
 #else
 
 struct pinctrl_dev;
-- 
1.7.0.4

Re: [PATCH RFC v2 1/2] pinctrl: add pinctrl_add_gpio_ranges function

[Stephen Warren]

On 05/18/2012 07:12 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

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

[PATCH RFC v2 1/2] pinctrl: add pinctrl_add_gpio_ranges function

[Stephen Warren]

On 05/18/2012 07:12 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

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

Re: [PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/18/2012 07:12 AM, Dong Aisheng wrote:
> The gpio ranges standard dt binding format is
> <&gpio $gpio_offset $pin_offset $npin>
> 
> The core will parse and register the pinctrl gpio ranges
> from device tree.

Overall this conceptually makes sense, and looks good. Some comments though:

> +/*
> + * 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 property @propname
> + * @propname: a list of phandles of gpios and corresponding data.
> + *  The format is: <&gpio0 $gpio_offset $pin_offset $count>
> + */
> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
> +				struct device_node *np,
> +				const char *propname)

If this will be fully standardized, perhaps we should just choose a
standard property name, rather than allowing different drivers to do
different things.

Perhaps "gpio-map"?

> +	nranges = size /4;

Add a space after the /

> +	ranges = devm_kzalloc(pctldev->dev, nranges * sizeof(*ranges),
> +				GFP_KERNEL);
> +	for (i = 0; i < nranges; i++) {
> +			phandle = be32_to_cpup(list++);

Looks like double indentation througout this block?

> +			np_gpio = of_find_node_by_phandle(phandle);
> +			if (!np_gpio) {
> +				dev_err(pctldev->dev,
> +					"failed to find gpio node(%s)\n",
> +					np_gpio->name);

Perhaps devm_kfree(ranges) here so that if this is called multiple times
due to deferred probe, the allocations from the failed attempts don't
accumulate. Same for other error paths.

> +				return -ENODEV;
> +			}
> +
> +			ranges[i].gc = of_node_to_gpiochip(np_gpio);

Of course, there is the outstanding question of whether this API will
exist and how it will work.

I think you can of_node_put(np_gpio) here, before the error-check for
ranges[i].gc, to avoid having to do it in all the error paths below.

Do you need to xxx_get(ranges[i].gc) to prevent it going away, and put()
it when removing the ranges?

> +			if (gpio_offset < 0 ||
> +			    gpio_offset > ranges[i].gc->ngpio ||
> +			    pin_offset < 0 || npins < 0) {

These are all unsigned, so most of those conditions can never be true.

Perhaps replace the remaining check with:

	gpio_offset + npins > ranges[i].gc->ngpio

to make sure the range isn't too long either

> +	pinctrl_add_gpio_ranges(pctldev, ranges, nranges);

I think we should also store ranges somewhere separate in pctldev ...

> +void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
> +				   struct pinctrl_gpio_range *ranges,
> +				   unsigned nranges)

... so that this function doesn't need ranges/nranges passed to it; it
can just get them out of pctldec.

Or better yet, what if pinctrl_unregister automatically removed any
ranges registered by pinctrl_dt_add_gpio_ranges, so this function
doesn't even need to exist?

[PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/18/2012 07:12 AM, Dong Aisheng wrote:
> The gpio ranges standard dt binding format is
> <&gpio $gpio_offset $pin_offset $npin>
> 
> The core will parse and register the pinctrl gpio ranges
> from device tree.

Overall this conceptually makes sense, and looks good. Some comments though:

> +/*
> + * 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 property @propname
> + * @propname: a list of phandles of gpios and corresponding data.
> + *  The format is: <&gpio0 $gpio_offset $pin_offset $count>
> + */
> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
> +				struct device_node *np,
> +				const char *propname)

If this will be fully standardized, perhaps we should just choose a
standard property name, rather than allowing different drivers to do
different things.

Perhaps "gpio-map"?

> +	nranges = size /4;

Add a space after the /

> +	ranges = devm_kzalloc(pctldev->dev, nranges * sizeof(*ranges),
> +				GFP_KERNEL);
> +	for (i = 0; i < nranges; i++) {
> +			phandle = be32_to_cpup(list++);

Looks like double indentation througout this block?

> +			np_gpio = of_find_node_by_phandle(phandle);
> +			if (!np_gpio) {
> +				dev_err(pctldev->dev,
> +					"failed to find gpio node(%s)\n",
> +					np_gpio->name);

Perhaps devm_kfree(ranges) here so that if this is called multiple times
due to deferred probe, the allocations from the failed attempts don't
accumulate. Same for other error paths.

> +				return -ENODEV;
> +			}
> +
> +			ranges[i].gc = of_node_to_gpiochip(np_gpio);

Of course, there is the outstanding question of whether this API will
exist and how it will work.

I think you can of_node_put(np_gpio) here, before the error-check for
ranges[i].gc, to avoid having to do it in all the error paths below.

Do you need to xxx_get(ranges[i].gc) to prevent it going away, and put()
it when removing the ranges?

> +			if (gpio_offset < 0 ||
> +			    gpio_offset > ranges[i].gc->ngpio ||
> +			    pin_offset < 0 || npins < 0) {

These are all unsigned, so most of those conditions can never be true.

Perhaps replace the remaining check with:

	gpio_offset + npins > ranges[i].gc->ngpio

to make sure the range isn't too long either

> +	pinctrl_add_gpio_ranges(pctldev, ranges, nranges);

I think we should also store ranges somewhere separate in pctldev ...

> +void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
> +				   struct pinctrl_gpio_range *ranges,
> +				   unsigned nranges)

... so that this function doesn't need ranges/nranges passed to it; it
can just get them out of pctldec.

Or better yet, what if pinctrl_unregister automatically removed any
ranges registered by pinctrl_dt_add_gpio_ranges, so this function
doesn't even need to exist?

Re: [PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
> On 05/18/2012 07:12 AM, Dong Aisheng wrote:
> > The gpio ranges standard dt binding format is
> > <&gpio $gpio_offset $pin_offset $npin>
> > 
> > The core will parse and register the pinctrl gpio ranges
> > from device tree.
> 
> Overall this conceptually makes sense, and looks good. Some comments though:
> 
Great, thanks Stephen.

> > +/*
> > + * 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 property @propname
> > + * @propname: a list of phandles of gpios and corresponding data.
> > + *  The format is: <&gpio0 $gpio_offset $pin_offset $count>
> > + */
> > +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
> > +				struct device_node *np,
> > +				const char *propname)
> 
> If this will be fully standardized, perhaps we should just choose a
> standard property name, rather than allowing different drivers to do
> different things.
> 
> Perhaps "gpio-map"?
> 
Well, correct, this name is ok to me.

> > +	nranges = size /4;
> 
> Add a space after the /
> 
> > +	ranges = devm_kzalloc(pctldev->dev, nranges * sizeof(*ranges),
> > +				GFP_KERNEL);
> > +	for (i = 0; i < nranges; i++) {
> > +			phandle = be32_to_cpup(list++);
> 
> Looks like double indentation througout this block?
> 
Correct, will fix them in a formal patch.

> > +			np_gpio = of_find_node_by_phandle(phandle);
> > +			if (!np_gpio) {
> > +				dev_err(pctldev->dev,
> > +					"failed to find gpio node(%s)\n",
> > +					np_gpio->name);
> 
> Perhaps devm_kfree(ranges) here so that if this is called multiple times
> due to deferred probe, the allocations from the failed attempts don't
> accumulate. Same for other error paths.
> 
Correct.
Will add it.

> > +				return -ENODEV;
> > +			}
> > +
> > +			ranges[i].gc = of_node_to_gpiochip(np_gpio);
> 
> Of course, there is the outstanding question of whether this API will
> exist and how it will work.
> 
> I think you can of_node_put(np_gpio) here, before the error-check for
> ranges[i].gc, to avoid having to do it in all the error paths below.
> 
Good idea. Will do it.

> Do you need to xxx_get(ranges[i].gc) to prevent it going away, and put()
> it when removing the ranges?
> 
How would you suggest to implement xxx_get(ranges[i].gc)?
Since the parameter is a struct gpiochip, my first sense is that it may be
provided by gpio subsystem, but i did not find such a function.
Looking at gpio subsystem, i also can't see it should provide such function.

I wonder if we need to implement it, if gpiochip is gone way,
the error will be detected in the higher gpio layer and will not pass
down to pinctrl.

> > +			if (gpio_offset < 0 ||
> > +			    gpio_offset > ranges[i].gc->ngpio ||
> > +			    pin_offset < 0 || npins < 0) {
> 
> These are all unsigned, so most of those conditions can never be true.
> 
Correct, will remove negative value checking.

> Perhaps replace the remaining check with:
> 
> 	gpio_offset + npins > ranges[i].gc->ngpio
> 
Good idea, will replace above checking with this one.

> to make sure the range isn't too long either
> 
> > +	pinctrl_add_gpio_ranges(pctldev, ranges, nranges);
> 
> I think we should also store ranges somewhere separate in pctldev ...
> 
Hmm, i'm not quite understand,
What do you mean separate in pctldev?
We already save the ranges in pctldev's gpio_ranges list.

> > +void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
> > +				   struct pinctrl_gpio_range *ranges,
> > +				   unsigned nranges)
> 
> ... so that this function doesn't need ranges/nranges passed to it; it
> can just get them out of pctldec.
> 
For remove ranges/nranges, maybe we can use list_for_each_entry_safe
plus list_del like:
list_for_each_entry_safe(range, tmp, &pctldev->gpio_ranges, node)
    list_del(range->node);

I just follow the original way here and if change we may need another
patch.

> Or better yet, what if pinctrl_unregister automatically removed any
> ranges registered by pinctrl_dt_add_gpio_ranges, so this function
> doesn't even need to exist?
> 
Yes, it seems this can also work for non-dt registered gpio ranges added by
pinctrl_add_gpio_ranges, e.g, remove need pinctrl_remove_gpio_range too,
It seems it should be in a separate patch which is not related to gpio ranges
binding.

Do you want me to work on that patch first for this series to rebase or
we can do it later?

Regards
Dong Aisheng

[PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
> On 05/18/2012 07:12 AM, Dong Aisheng wrote:
> > The gpio ranges standard dt binding format is
> > <&gpio $gpio_offset $pin_offset $npin>
> > 
> > The core will parse and register the pinctrl gpio ranges
> > from device tree.
> 
> Overall this conceptually makes sense, and looks good. Some comments though:
> 
Great, thanks Stephen.

> > +/*
> > + * 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 property @propname
> > + * @propname: a list of phandles of gpios and corresponding data.
> > + *  The format is: <&gpio0 $gpio_offset $pin_offset $count>
> > + */
> > +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
> > +				struct device_node *np,
> > +				const char *propname)
> 
> If this will be fully standardized, perhaps we should just choose a
> standard property name, rather than allowing different drivers to do
> different things.
> 
> Perhaps "gpio-map"?
> 
Well, correct, this name is ok to me.

> > +	nranges = size /4;
> 
> Add a space after the /
> 
> > +	ranges = devm_kzalloc(pctldev->dev, nranges * sizeof(*ranges),
> > +				GFP_KERNEL);
> > +	for (i = 0; i < nranges; i++) {
> > +			phandle = be32_to_cpup(list++);
> 
> Looks like double indentation througout this block?
> 
Correct, will fix them in a formal patch.

> > +			np_gpio = of_find_node_by_phandle(phandle);
> > +			if (!np_gpio) {
> > +				dev_err(pctldev->dev,
> > +					"failed to find gpio node(%s)\n",
> > +					np_gpio->name);
> 
> Perhaps devm_kfree(ranges) here so that if this is called multiple times
> due to deferred probe, the allocations from the failed attempts don't
> accumulate. Same for other error paths.
> 
Correct.
Will add it.

> > +				return -ENODEV;
> > +			}
> > +
> > +			ranges[i].gc = of_node_to_gpiochip(np_gpio);
> 
> Of course, there is the outstanding question of whether this API will
> exist and how it will work.
> 
> I think you can of_node_put(np_gpio) here, before the error-check for
> ranges[i].gc, to avoid having to do it in all the error paths below.
> 
Good idea. Will do it.

> Do you need to xxx_get(ranges[i].gc) to prevent it going away, and put()
> it when removing the ranges?
> 
How would you suggest to implement xxx_get(ranges[i].gc)?
Since the parameter is a struct gpiochip, my first sense is that it may be
provided by gpio subsystem, but i did not find such a function.
Looking at gpio subsystem, i also can't see it should provide such function.

I wonder if we need to implement it, if gpiochip is gone way,
the error will be detected in the higher gpio layer and will not pass
down to pinctrl.

> > +			if (gpio_offset < 0 ||
> > +			    gpio_offset > ranges[i].gc->ngpio ||
> > +			    pin_offset < 0 || npins < 0) {
> 
> These are all unsigned, so most of those conditions can never be true.
> 
Correct, will remove negative value checking.

> Perhaps replace the remaining check with:
> 
> 	gpio_offset + npins > ranges[i].gc->ngpio
> 
Good idea, will replace above checking with this one.

> to make sure the range isn't too long either
> 
> > +	pinctrl_add_gpio_ranges(pctldev, ranges, nranges);
> 
> I think we should also store ranges somewhere separate in pctldev ...
> 
Hmm, i'm not quite understand,
What do you mean separate in pctldev?
We already save the ranges in pctldev's gpio_ranges list.

> > +void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
> > +				   struct pinctrl_gpio_range *ranges,
> > +				   unsigned nranges)
> 
> ... so that this function doesn't need ranges/nranges passed to it; it
> can just get them out of pctldec.
> 
For remove ranges/nranges, maybe we can use list_for_each_entry_safe
plus list_del like:
list_for_each_entry_safe(range, tmp, &pctldev->gpio_ranges, node)
    list_del(range->node);

I just follow the original way here and if change we may need another
patch.

> Or better yet, what if pinctrl_unregister automatically removed any
> ranges registered by pinctrl_dt_add_gpio_ranges, so this function
> doesn't even need to exist?
> 
Yes, it seems this can also work for non-dt registered gpio ranges added by
pinctrl_add_gpio_ranges, e.g, remove need pinctrl_remove_gpio_range too,
It seems it should be in a separate patch which is not related to gpio ranges
binding.

Do you want me to work on that patch first for this series to rebase or
we can do it later?

Regards
Dong Aisheng

Re: [PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/21/2012 06:39 AM, Dong Aisheng wrote:
> On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
>> On 05/18/2012 07:12 AM, Dong Aisheng wrote:
>>> The gpio ranges standard dt binding format is
>>> <&gpio $gpio_offset $pin_offset $npin>
>>>
>>> The core will parse and register the pinctrl gpio ranges
>>> from device tree.
...
>>> +	pinctrl_add_gpio_ranges(pctldev, ranges, nranges);
>>
>> I think we should also store ranges somewhere separate in pctldev ...
>
> Hmm, i'm not quite understand,
> What do you mean separate in pctldev?
> We already save the ranges in pctldev's gpio_ranges list.
>
>>> +void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
>>> +				   struct pinctrl_gpio_range *ranges,
>>> +				   unsigned nranges)
>>
>> ... so that this function doesn't need ranges/nranges passed to it; it
>> can just get them out of pctldec.
>
> For remove ranges/nranges, maybe we can use list_for_each_entry_safe
> plus list_del like:
> list_for_each_entry_safe(range, tmp, &pctldev->gpio_ranges, node)
>     list_del(range->node);

If some GPIO ranges are added using pinctrl_dt_add_gpio_ranges() and
some are added using pinctrl_add_gpio_range(), how will
pinctrl_dt_remove_gpio_ranges() know which ones to remove, unless

a) The list of ranges is passed to pinctrl_dt_remove_gpio_ranges() as
above (but then where does the caller find the list to pass in)

or:

b) The list of ranges added by pinctrl_dt_add_gpio_ranges() is stored
separately from the overall gpio_ranges list, so the code knows which
were added by that API.

Perhaps the simplest way to implement this is to add a new Boolean field
to struct gpio_range that says whether pinctrl_dt_add_gpio_ranges()
added it, and set it to true/false in  pinctrl_dt_add_gpio_ranges() and
 pinctrl_add_gpio_ranges().

> I just follow the original way here and if change we may need another
> patch.
> 
>> Or better yet, what if pinctrl_unregister automatically removed any
>> ranges registered by pinctrl_dt_add_gpio_ranges, so this function
>> doesn't even need to exist?
>
> Yes, it seems this can also work for non-dt registered gpio ranges added by
> pinctrl_add_gpio_ranges, e.g, remove need pinctrl_remove_gpio_range too,
> It seems it should be in a separate patch which is not related to gpio ranges
> binding.
>
> Do you want me to work on that patch first for this series to rebase or
> we can do it later?

Maybe we should just delete the functions pinctrl_remove_gpio_range()
and pinctrl_dt_remove_gpio_range() completely. They only remove the gpio
range from the list in the pctldev, and since the pctldev is being
unregistered anyway, that doesn't seem very useful. The ranges don't
need to be free()'d since pinctrl_dt_add_gpio_ranges() allocates them
using devm_kzalloc() anyway, and in the non-DT case, if they need
free-ing, the driver already had to do that itself.

[PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/21/2012 06:39 AM, Dong Aisheng wrote:
> On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
>> On 05/18/2012 07:12 AM, Dong Aisheng wrote:
>>> The gpio ranges standard dt binding format is
>>> <&gpio $gpio_offset $pin_offset $npin>
>>>
>>> The core will parse and register the pinctrl gpio ranges
>>> from device tree.
...
>>> +	pinctrl_add_gpio_ranges(pctldev, ranges, nranges);
>>
>> I think we should also store ranges somewhere separate in pctldev ...
>
> Hmm, i'm not quite understand,
> What do you mean separate in pctldev?
> We already save the ranges in pctldev's gpio_ranges list.
>
>>> +void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
>>> +				   struct pinctrl_gpio_range *ranges,
>>> +				   unsigned nranges)
>>
>> ... so that this function doesn't need ranges/nranges passed to it; it
>> can just get them out of pctldec.
>
> For remove ranges/nranges, maybe we can use list_for_each_entry_safe
> plus list_del like:
> list_for_each_entry_safe(range, tmp, &pctldev->gpio_ranges, node)
>     list_del(range->node);

If some GPIO ranges are added using pinctrl_dt_add_gpio_ranges() and
some are added using pinctrl_add_gpio_range(), how will
pinctrl_dt_remove_gpio_ranges() know which ones to remove, unless

a) The list of ranges is passed to pinctrl_dt_remove_gpio_ranges() as
above (but then where does the caller find the list to pass in)

or:

b) The list of ranges added by pinctrl_dt_add_gpio_ranges() is stored
separately from the overall gpio_ranges list, so the code knows which
were added by that API.

Perhaps the simplest way to implement this is to add a new Boolean field
to struct gpio_range that says whether pinctrl_dt_add_gpio_ranges()
added it, and set it to true/false in  pinctrl_dt_add_gpio_ranges() and
 pinctrl_add_gpio_ranges().

> I just follow the original way here and if change we may need another
> patch.
> 
>> Or better yet, what if pinctrl_unregister automatically removed any
>> ranges registered by pinctrl_dt_add_gpio_ranges, so this function
>> doesn't even need to exist?
>
> Yes, it seems this can also work for non-dt registered gpio ranges added by
> pinctrl_add_gpio_ranges, e.g, remove need pinctrl_remove_gpio_range too,
> It seems it should be in a separate patch which is not related to gpio ranges
> binding.
>
> Do you want me to work on that patch first for this series to rebase or
> we can do it later?

Maybe we should just delete the functions pinctrl_remove_gpio_range()
and pinctrl_dt_remove_gpio_range() completely. They only remove the gpio
range from the list in the pctldev, and since the pctldev is being
unregistered anyway, that doesn't seem very useful. The ranges don't
need to be free()'d since pinctrl_dt_add_gpio_ranges() allocates them
using devm_kzalloc() anyway, and in the non-DT case, if they need
free-ing, the driver already had to do that itself.

Re: [PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/21/2012 06:39 AM, Dong Aisheng wrote:
> On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
>> On 05/18/2012 07:12 AM, Dong Aisheng wrote:
>>> The gpio ranges standard dt binding format is
>>> <&gpio $gpio_offset $pin_offset $npin>
>>>
>>> The core will parse and register the pinctrl gpio ranges
>>> from device tree.
...
>> Do you need to xxx_get(ranges[i].gc) to prevent it going away, and put()
>> it when removing the ranges?
>
> How would you suggest to implement xxx_get(ranges[i].gc)?
> Since the parameter is a struct gpiochip, my first sense is that it may be
> provided by gpio subsystem, but i did not find such a function.
> Looking at gpio subsystem, i also can't see it should provide such function.
> 
> I wonder if we need to implement it, if gpiochip is gone way,
> the error will be detected in the higher gpio layer and will not pass
> down to pinctrl.

Yes, it looks like we should add new APIs for this; we need to
try_module_get() on the module containing the GPIO chip so it doesn't
disappear, similar to what gpio_request() does.

[PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/21/2012 06:39 AM, Dong Aisheng wrote:
> On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
>> On 05/18/2012 07:12 AM, Dong Aisheng wrote:
>>> The gpio ranges standard dt binding format is
>>> <&gpio $gpio_offset $pin_offset $npin>
>>>
>>> The core will parse and register the pinctrl gpio ranges
>>> from device tree.
...
>> Do you need to xxx_get(ranges[i].gc) to prevent it going away, and put()
>> it when removing the ranges?
>
> How would you suggest to implement xxx_get(ranges[i].gc)?
> Since the parameter is a struct gpiochip, my first sense is that it may be
> provided by gpio subsystem, but i did not find such a function.
> Looking at gpio subsystem, i also can't see it should provide such function.
> 
> I wonder if we need to implement it, if gpiochip is gone way,
> the error will be detected in the higher gpio layer and will not pass
> down to pinctrl.

Yes, it looks like we should add new APIs for this; we need to
try_module_get() on the module containing the GPIO chip so it doesn't
disappear, similar to what gpio_request() does.

Re: [PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Tue, May 22, 2012 at 1:09 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/21/2012 06:39 AM, Dong Aisheng wrote:
>> On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
>>> On 05/18/2012 07:12 AM, Dong Aisheng wrote:
>>>> The gpio ranges standard dt binding format is
>>>> <&gpio $gpio_offset $pin_offset $npin>
>>>>
>>>> The core will parse and register the pinctrl gpio ranges
>>>> from device tree.
> ...
>>>> +   pinctrl_add_gpio_ranges(pctldev, ranges, nranges);
>>>
>>> I think we should also store ranges somewhere separate in pctldev ...
>>
>> Hmm, i'm not quite understand,
>> What do you mean separate in pctldev?
>> We already save the ranges in pctldev's gpio_ranges list.
>>
>>>> +void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
>>>> +                              struct pinctrl_gpio_range *ranges,
>>>> +                              unsigned nranges)
>>>
>>> ... so that this function doesn't need ranges/nranges passed to it; it
>>> can just get them out of pctldec.
>>
>> For remove ranges/nranges, maybe we can use list_for_each_entry_safe
>> plus list_del like:
>> list_for_each_entry_safe(range, tmp, &pctldev->gpio_ranges, node)
>>     list_del(range->node);
>
> If some GPIO ranges are added using pinctrl_dt_add_gpio_ranges() and
> some are added using pinctrl_add_gpio_range(), how will
> pinctrl_dt_remove_gpio_ranges() know which ones to remove, unless
>
Yes, you're right.

> a) The list of ranges is passed to pinctrl_dt_remove_gpio_ranges() as
> above (but then where does the caller find the list to pass in)
>
> or:
>
> b) The list of ranges added by pinctrl_dt_add_gpio_ranges() is stored
> separately from the overall gpio_ranges list, so the code knows which
> were added by that API.
>
> Perhaps the simplest way to implement this is to add a new Boolean field
> to struct gpio_range that says whether pinctrl_dt_add_gpio_ranges()
> added it, and set it to true/false in  pinctrl_dt_add_gpio_ranges() and
>  pinctrl_add_gpio_ranges().
>
Maybe we should not allow people to use pinctrl_add_gpio_ranges for dt
and i can not see it helps too much.
Then we do not have two case.

>> I just follow the original way here and if change we may need another
>> patch.
>>
>>> Or better yet, what if pinctrl_unregister automatically removed any
>>> ranges registered by pinctrl_dt_add_gpio_ranges, so this function
>>> doesn't even need to exist?
>>
>> Yes, it seems this can also work for non-dt registered gpio ranges added by
>> pinctrl_add_gpio_ranges, e.g, remove need pinctrl_remove_gpio_range too,
>> It seems it should be in a separate patch which is not related to gpio ranges
>> binding.
>>
>> Do you want me to work on that patch first for this series to rebase or
>> we can do it later?
>
> Maybe we should just delete the functions pinctrl_remove_gpio_range()
> and pinctrl_dt_remove_gpio_range() completely. They only remove the gpio
> range from the list in the pctldev, and since the pctldev is being
> unregistered anyway, that doesn't seem very useful. The ranges don't
> need to be free()'d since pinctrl_dt_add_gpio_ranges() allocates them
> using devm_kzalloc() anyway, and in the non-DT case, if they need
> free-ing, the driver already had to do that itself.
>
Yes, i agree with this one.
I will re-order the patch series to remove pinctrl_remove_gpio_range first.

Regards
Dong Aisheng

[PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Tue, May 22, 2012 at 1:09 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/21/2012 06:39 AM, Dong Aisheng wrote:
>> On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
>>> On 05/18/2012 07:12 AM, Dong Aisheng wrote:
>>>> The gpio ranges standard dt binding format is
>>>> <&gpio $gpio_offset $pin_offset $npin>
>>>>
>>>> The core will parse and register the pinctrl gpio ranges
>>>> from device tree.
> ...
>>>> + ? pinctrl_add_gpio_ranges(pctldev, ranges, nranges);
>>>
>>> I think we should also store ranges somewhere separate in pctldev ...
>>
>> Hmm, i'm not quite understand,
>> What do you mean separate in pctldev?
>> We already save the ranges in pctldev's gpio_ranges list.
>>
>>>> +void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct pinctrl_gpio_range *ranges,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned nranges)
>>>
>>> ... so that this function doesn't need ranges/nranges passed to it; it
>>> can just get them out of pctldec.
>>
>> For remove ranges/nranges, maybe we can use list_for_each_entry_safe
>> plus list_del like:
>> list_for_each_entry_safe(range, tmp, &pctldev->gpio_ranges, node)
>> ? ? list_del(range->node);
>
> If some GPIO ranges are added using pinctrl_dt_add_gpio_ranges() and
> some are added using pinctrl_add_gpio_range(), how will
> pinctrl_dt_remove_gpio_ranges() know which ones to remove, unless
>
Yes, you're right.

> a) The list of ranges is passed to pinctrl_dt_remove_gpio_ranges() as
> above (but then where does the caller find the list to pass in)
>
> or:
>
> b) The list of ranges added by pinctrl_dt_add_gpio_ranges() is stored
> separately from the overall gpio_ranges list, so the code knows which
> were added by that API.
>
> Perhaps the simplest way to implement this is to add a new Boolean field
> to struct gpio_range that says whether pinctrl_dt_add_gpio_ranges()
> added it, and set it to true/false in ?pinctrl_dt_add_gpio_ranges() and
> ?pinctrl_add_gpio_ranges().
>
Maybe we should not allow people to use pinctrl_add_gpio_ranges for dt
and i can not see it helps too much.
Then we do not have two case.

>> I just follow the original way here and if change we may need another
>> patch.
>>
>>> Or better yet, what if pinctrl_unregister automatically removed any
>>> ranges registered by pinctrl_dt_add_gpio_ranges, so this function
>>> doesn't even need to exist?
>>
>> Yes, it seems this can also work for non-dt registered gpio ranges added by
>> pinctrl_add_gpio_ranges, e.g, remove need pinctrl_remove_gpio_range too,
>> It seems it should be in a separate patch which is not related to gpio ranges
>> binding.
>>
>> Do you want me to work on that patch first for this series to rebase or
>> we can do it later?
>
> Maybe we should just delete the functions pinctrl_remove_gpio_range()
> and pinctrl_dt_remove_gpio_range() completely. They only remove the gpio
> range from the list in the pctldev, and since the pctldev is being
> unregistered anyway, that doesn't seem very useful. The ranges don't
> need to be free()'d since pinctrl_dt_add_gpio_ranges() allocates them
> using devm_kzalloc() anyway, and in the non-DT case, if they need
> free-ing, the driver already had to do that itself.
>
Yes, i agree with this one.
I will re-order the patch series to remove pinctrl_remove_gpio_range first.

Regards
Dong Aisheng

Re: [PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Tue, May 22, 2012 at 1:11 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/21/2012 06:39 AM, Dong Aisheng wrote:
>> On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
>>> On 05/18/2012 07:12 AM, Dong Aisheng wrote:
>>>> The gpio ranges standard dt binding format is
>>>> <&gpio $gpio_offset $pin_offset $npin>
>>>>
>>>> The core will parse and register the pinctrl gpio ranges
>>>> from device tree.
> ...
>>> Do you need to xxx_get(ranges[i].gc) to prevent it going away, and put()
>>> it when removing the ranges?
>>
>> How would you suggest to implement xxx_get(ranges[i].gc)?
>> Since the parameter is a struct gpiochip, my first sense is that it may be
>> provided by gpio subsystem, but i did not find such a function.
>> Looking at gpio subsystem, i also can't see it should provide such function.
>>
>> I wonder if we need to implement it, if gpiochip is gone way,
>> the error will be detected in the higher gpio layer and will not pass
>> down to pinctrl.
>
> Yes, it looks like we should add new APIs for this; we need to
> try_module_get() on the module containing the GPIO chip so it doesn't
> disappear, similar to what gpio_request() does.
yes, i checked the gpio request code.
It looks to me try_module_get is just ok like:
if (!try_module_get(ranges[i].gc.owner))
    goto done;
Will add it.
Thanks for the info.

Regards
Dong Aisheng

[PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Tue, May 22, 2012 at 1:11 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/21/2012 06:39 AM, Dong Aisheng wrote:
>> On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
>>> On 05/18/2012 07:12 AM, Dong Aisheng wrote:
>>>> The gpio ranges standard dt binding format is
>>>> <&gpio $gpio_offset $pin_offset $npin>
>>>>
>>>> The core will parse and register the pinctrl gpio ranges
>>>> from device tree.
> ...
>>> Do you need to xxx_get(ranges[i].gc) to prevent it going away, and put()
>>> it when removing the ranges?
>>
>> How would you suggest to implement xxx_get(ranges[i].gc)?
>> Since the parameter is a struct gpiochip, my first sense is that it may be
>> provided by gpio subsystem, but i did not find such a function.
>> Looking at gpio subsystem, i also can't see it should provide such function.
>>
>> I wonder if we need to implement it, if gpiochip is gone way,
>> the error will be detected in the higher gpio layer and will not pass
>> down to pinctrl.
>
> Yes, it looks like we should add new APIs for this; we need to
> try_module_get() on the module containing the GPIO chip so it doesn't
> disappear, similar to what gpio_request() does.
yes, i checked the gpio request code.
It looks to me try_module_get is just ok like:
if (!try_module_get(ranges[i].gc.owner))
    goto done;
Will add it.
Thanks for the info.

Regards
Dong Aisheng

Re: [PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
...
> > +			np_gpio = of_find_node_by_phandle(phandle);
> > +			if (!np_gpio) {
> > +				dev_err(pctldev->dev,
> > +					"failed to find gpio node(%s)\n",
> > +					np_gpio->name);
> 
> Perhaps devm_kfree(ranges) here so that if this is called multiple times
> due to deferred probe, the allocations from the failed attempts don't
> accumulate. Same for other error paths.
> 
I checked a bit more, it seems the resource will be removed first if there's
a deffer probe error.
static int really_probe(struct device *dev, struct device_driver *drv)
{
.....
probe_failed:
        devres_release_all(dev);
        driver_sysfs_remove(dev);
        dev->driver = NULL;

        if (ret == -EPROBE_DEFER) {
                /* Driver requested deferred probing */
                dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
                driver_deferred_probe_add(dev);
		...
	}
So we may not need devm_kfree for the EPROBE_DEFER error here.
It looks to me reasonable that managed resource covers the defer probe
error.

Regards
Dong Aisheng

[PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
...
> > +			np_gpio = of_find_node_by_phandle(phandle);
> > +			if (!np_gpio) {
> > +				dev_err(pctldev->dev,
> > +					"failed to find gpio node(%s)\n",
> > +					np_gpio->name);
> 
> Perhaps devm_kfree(ranges) here so that if this is called multiple times
> due to deferred probe, the allocations from the failed attempts don't
> accumulate. Same for other error paths.
> 
I checked a bit more, it seems the resource will be removed first if there's
a deffer probe error.
static int really_probe(struct device *dev, struct device_driver *drv)
{
.....
probe_failed:
        devres_release_all(dev);
        driver_sysfs_remove(dev);
        dev->driver = NULL;

        if (ret == -EPROBE_DEFER) {
                /* Driver requested deferred probing */
                dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
                driver_deferred_probe_add(dev);
		...
	}
So we may not need devm_kfree for the EPROBE_DEFER error here.
It looks to me reasonable that managed resource covers the defer probe
error.

Regards
Dong Aisheng

Re: [PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/21/2012 09:35 PM, Dong Aisheng wrote:
> On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
> ...
>>> +			np_gpio = of_find_node_by_phandle(phandle);
>>> +			if (!np_gpio) {
>>> +				dev_err(pctldev->dev,
>>> +					"failed to find gpio node(%s)\n",
>>> +					np_gpio->name);
>>
>> Perhaps devm_kfree(ranges) here so that if this is called multiple times
>> due to deferred probe, the allocations from the failed attempts don't
>> accumulate. Same for other error paths.
>
> I checked a bit more, it seems the resource will be removed first if there's
> a deffer probe error.

Oh yes of course - that makes sense.

[PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/21/2012 09:35 PM, Dong Aisheng wrote:
> On Sat, May 19, 2012 at 04:05:46AM +0800, Stephen Warren wrote:
> ...
>>> +			np_gpio = of_find_node_by_phandle(phandle);
>>> +			if (!np_gpio) {
>>> +				dev_err(pctldev->dev,
>>> +					"failed to find gpio node(%s)\n",
>>> +					np_gpio->name);
>>
>> Perhaps devm_kfree(ranges) here so that if this is called multiple times
>> due to deferred probe, the allocations from the failed attempts don't
>> accumulate. Same for other error paths.
>
> I checked a bit more, it seems the resource will be removed first if there's
> a deffer probe error.

Oh yes of course - that makes sense.

Re: [PATCH RFC v2 1/2] pinctrl: add pinctrl_add_gpio_ranges function

[Linus Walleij]

On Fri, May 18, 2012 at 3:12 PM, Dong Aisheng <b29396@freescale.com> wrote:

> From: Dong Aisheng <dong.aisheng@linaro.org>
>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

Applied with Stephens ACK and a blurb telling what it's good for :-)

Thanks,
Linus Walleij

[PATCH RFC v2 1/2] pinctrl: add pinctrl_add_gpio_ranges function

[Linus Walleij]

On Fri, May 18, 2012 at 3:12 PM, Dong Aisheng <b29396@freescale.com> wrote:

> From: Dong Aisheng <dong.aisheng@linaro.org>
>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

Applied with Stephens ACK and a blurb telling what it's good for :-)

Thanks,
Linus Walleij

Re: [PATCH RFC v2 1/2] pinctrl: add pinctrl_add_gpio_ranges function

[Linus Walleij]

On Thu, May 24, 2012 at 4:29 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, May 18, 2012 at 3:12 PM, Dong Aisheng <b29396@freescale.com> wrote:
>
>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>
>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>
> Applied with Stephens ACK and a blurb telling what it's good for :-)

Oh. there was a later version, forget this, I'll look at v2/v3.

Yours,
Linus Walleij

[PATCH RFC v2 1/2] pinctrl: add pinctrl_add_gpio_ranges function

[Linus Walleij]

On Thu, May 24, 2012 at 4:29 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, May 18, 2012 at 3:12 PM, Dong Aisheng <b29396@freescale.com> wrote:
>
>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>
>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>
> Applied with Stephens ACK and a blurb telling what it's good for :-)

Oh. there was a later version, forget this, I'll look at v2/v3.

Yours,
Linus Walleij