From: Dong Aisheng <[email protected]>
The gpio ranges will be automatically removed when the pinctrl
driver is unregistered.
Signed-off-by: Dong Aisheng <[email protected]>
---
I greped the lastes torvalds tree, it seems no other pinctrl drivers
are using pinctrl_remove_gpio_range. It may be safe to merge this
through pinctrl tree.
---
drivers/pinctrl/core.c | 19 +++++--------------
drivers/pinctrl/pinctrl-tegra.c | 1 -
drivers/pinctrl/pinctrl-u300.c | 2 --
include/linux/pinctrl/pinctrl.h | 2 --
4 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c3b331b..e16529e 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -333,20 +333,6 @@ void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
EXPORT_SYMBOL_GPL(pinctrl_add_gpio_range);
/**
- * pinctrl_remove_gpio_range() - remove a range of GPIOs fro a pin controller
- * @pctldev: pin controller device to remove the range from
- * @range: the GPIO range to remove
- */
-void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
- struct pinctrl_gpio_range *range)
-{
- mutex_lock(&pinctrl_mutex);
- list_del(&range->node);
- mutex_unlock(&pinctrl_mutex);
-}
-EXPORT_SYMBOL_GPL(pinctrl_remove_gpio_range);
-
-/**
* pinctrl_get_group_selector() - returns the group selector for a group
* @pctldev: the pin controller handling the group
* @pin_group: the pin group to look up
@@ -1485,6 +1471,7 @@ EXPORT_SYMBOL_GPL(pinctrl_register);
*/
void pinctrl_unregister(struct pinctrl_dev *pctldev)
{
+ struct pinctrl_gpio_range *range, *n;
if (pctldev == NULL)
return;
@@ -1500,6 +1487,10 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
/* Destroy descriptor tree */
pinctrl_free_pindescs(pctldev, pctldev->desc->pins,
pctldev->desc->npins);
+ /* remove gpio ranges map */
+ list_for_each_entry_safe(range, n, &pctldev->gpio_ranges, node)
+ list_del(&range->node);
+
kfree(pctldev);
mutex_unlock(&pinctrl_mutex);
diff --git a/drivers/pinctrl/pinctrl-tegra.c b/drivers/pinctrl/pinctrl-tegra.c
index 2c98fba..47bd452 100644
--- a/drivers/pinctrl/pinctrl-tegra.c
+++ b/drivers/pinctrl/pinctrl-tegra.c
@@ -702,7 +702,6 @@ static int __devexit tegra_pinctrl_remove(struct platform_device *pdev)
{
struct tegra_pmx *pmx = platform_get_drvdata(pdev);
- pinctrl_remove_gpio_range(pmx->pctl, &tegra_pinctrl_gpio_range);
pinctrl_unregister(pmx->pctl);
return 0;
diff --git a/drivers/pinctrl/pinctrl-u300.c b/drivers/pinctrl/pinctrl-u300.c
index 05d0299..10de43c 100644
--- a/drivers/pinctrl/pinctrl-u300.c
+++ b/drivers/pinctrl/pinctrl-u300.c
@@ -1177,8 +1177,6 @@ static int __devexit u300_pmx_remove(struct platform_device *pdev)
struct u300_pmx *upmx = platform_get_drvdata(pdev);
int i;
- for (i = 0; i < ARRAY_SIZE(u300_gpio_ranges); i++)
- pinctrl_remove_gpio_range(upmx->pctl, &u300_gpio_ranges[i]);
pinctrl_unregister(upmx->pctl);
iounmap(upmx->virtbase);
release_mem_region(upmx->phybase, upmx->physize);
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 3b894a6..170a588 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -131,8 +131,6 @@ extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin);
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 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
From: Dong Aisheng <[email protected]>
Acked-by: Stephen Warren <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
ChangeLog v2->v3:
* remove pinctrl_remove_gpio_ranges function.
---
drivers/pinctrl/core.c | 11 +++++++++++
include/linux/pinctrl/pinctrl.h | 3 +++
2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index e16529e..f6c51c4 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -332,6 +332,17 @@ void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
}
EXPORT_SYMBOL_GPL(pinctrl_add_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);
+
/**
* 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 170a588..69393a6 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -131,6 +131,9 @@ extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin);
extern void pinctrl_add_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 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
From: Dong Aisheng <[email protected]>
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_offset $pin_offset $npin>.
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 <[email protected]>
---
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 | 20 +++++
drivers/pinctrl/devicetree.c | 83 ++++++++++++++++++++
include/linux/pinctrl/pinctrl.h | 11 +++
3 files changed, 114 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index c95ea82..113c11e 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -126,3 +126,23 @@ 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: 4 integers array, each entry in the array represents a gpio
+range with the format: <&gpio $gpio_offset $pin_offset $count>
+- gpio: phandle pointing at gpio device node
+- gpio_offset: integer, the local offset of $gpio
+- pin_offset: integer, the pin offset or pin id
+- npins: integer, the gpio ranges starting from pin_offset
+
+For example:
+pincontroller {
+ gpio-maps = <&gpio1 0 0 32
+ &gpio2 0 0 30
+ &gpio3 1 100 1
+ ....>;
+};
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fcb1de4..7979ec1 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,84 @@ 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:
+ * <&gpio0 $gpio_offset $pin_offset $count>
+ */
+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 property *prop;
+ const __be32 *list;
+ phandle phandle;
+ int nranges;
+ int size;
+ int i;
+
+ if (!np) {
+ dev_err(pctldev->dev, "no device node\n");
+ return -EINVAL;
+ }
+
+ prop = of_find_property(np, "gpio-maps", &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_gpio->name);
+ of_node_put(np_gpio);
+ return -EPROBE_DEFER;
+ }
+
+ of_node_put(np_gpio);
+
+ gpio_offset = be32_to_cpu(*list++);
+ pin_offset = be32_to_cpu(*list++);
+ npins = be32_to_cpu(*list++);
+
+ if (gpio_offset + npins > ranges[i].gc->ngpio) {
+ dev_err(pctldev->dev, "exceed the gpio range\n");
+ 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;
+ }
+
+ pinctrl_add_gpio_ranges(pctldev, ranges, nranges);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_dt_add_gpio_ranges);
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 69393a6..07c7763 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -136,6 +136,17 @@ extern void pinctrl_add_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);
+#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
Hi Stephen,
On Wed, May 23, 2012 at 09:22:42PM +0800, Dong Aisheng wrote:
> From: Dong Aisheng <[email protected]>
>
> 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_offset $pin_offset $npin>.
>
> 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 <[email protected]>
> ---
> 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.
>
I still did not use try_module_get for gpio ranges to prevent gpio modules from going
away after a bit more thinking.
A few reasons:
1) gpio ranges are only raw data which does not depend on gpio module
2) if the gpio chip is going away, the gpio request will fail without touch pinctrl
stuff. So it seems doesn't matter if gpio module is going away for pinctrl.
3) For non-dt, the gpiochip parameter of gpio ranges is also optional and does not
require gpio chip to be exist. It looks reasonable to me to be like that.
So i'm wondering if we really need to add this checking.
Even if we need to do that, the right place may be pinctrl_add_gpio_range which not
related to this series like:
int pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
struct pinctrl_gpio_range *range)
{
...
if (range->gc) {
if (!try_module_get(range->gc->owner))
return -ENODEV;
}
...
}
So i sent this series first and let that issue to be discussed later.
Regards
Dong Aisheng
On 05/23/2012 07:22 AM, Dong Aisheng wrote:
> From: Dong Aisheng <[email protected]>
>
> The gpio ranges will be automatically removed when the pinctrl
> driver is unregistered.
>
> Signed-off-by: Dong Aisheng <[email protected]>
Acked-by: Stephen Warren <[email protected]>
On 05/23/2012 07:22 AM, Dong Aisheng wrote:
> From: Dong Aisheng <[email protected]>
>
> 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_offset $pin_offset $npin>.
>
> 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 <[email protected]>
This is mostly good. Just a few comments:
> +gpio-maps: 4 integers array, each entry in the array represents a gpio
> +range with the format: <&gpio $gpio_offset $pin_offset $count>
> +- gpio: phandle pointing at gpio device node
> +- gpio_offset: integer, the local offset of $gpio
> +- pin_offset: integer, the pin offset or pin id
> +- npins: integer, the gpio ranges starting from pin_offset
This uses a single cell to represent a GPIO ID within a GPIO controller.
The standard GPIO bindings use #gpio-cells, where that's a property in
the GPIO controller's node. I wonder if we shouldn't do the same here,
and call into the GPIO driver to parse #gpio-cells and give back the
Linux GPIO ID, just like of_get_named_gpio_flags() does. This would also
make this code able to cope with the GPIO of_xlate function returning a
different GPIO chip, which Grant put in place for banked GPIO controllers.
> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
The locking I was talking about before is between the following line:
> + ranges[i].gc = of_node_to_gpiochip(np_gpio);
and this code:
> + 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;
If of_node_to_gpiochip() doesn't mark the GPIO chip as "in use", then
the module that provides that device could be unloaded between the two
blocks of code above.
Re: your locking comments in your other email: ranges[i].gc doesn't
appear to be used anywhere else in pinctrl, so I think it's OK not to
lock the GPIO chip for any more time than between the above two blocks
of code.
Finally, just a minor nit:
> + 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_gpio->name);
> + of_node_put(np_gpio);
> + return -EPROBE_DEFER;
> + }
> +
> + of_node_put(np_gpio);
could be slightly simpler:
+ ranges[i].gc = of_node_to_gpiochip(np_gpio);
+ of_node_put(np_gpio); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
+ if (!ranges[i].gc) {
+ dev_err(pctldev->dev,
+ "can not find gpio chip of node(%s)\n",
+ np_gpio->name);
+ return -EPROBE_DEFER;
+ }
On Thu, May 24, 2012 at 4:44 AM, Stephen Warren <[email protected]> wrote:
> On 05/23/2012 07:22 AM, Dong Aisheng wrote:
>> From: Dong Aisheng <[email protected]>
>>
>> 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_offset $pin_offset $npin>.
>>
>> 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 <[email protected]>
>
> This is mostly good. Just a few comments:
>
>> +gpio-maps: 4 integers array, each entry in the array represents a gpio
>> +range with the format: <&gpio $gpio_offset $pin_offset $count>
>> +- gpio: phandle pointing at gpio device node
>> +- gpio_offset: integer, the local offset of $gpio
>> +- pin_offset: integer, the pin offset or pin id
>> +- npins: integer, the gpio ranges starting from pin_offset
>
> This uses a single cell to represent a GPIO ID within a GPIO controller.
> The standard GPIO bindings use #gpio-cells, where that's a property in
> the GPIO controller's node. I wonder if we shouldn't do the same here,
> and call into the GPIO driver to parse #gpio-cells and give back the
> Linux GPIO ID, just like of_get_named_gpio_flags() does. This would also
> make this code able to cope with the GPIO of_xlate function returning a
> different GPIO chip, which Grant put in place for banked GPIO controllers.
>
I checked the code, the second cell only represents gpio flag in
of_gpio_simple_xlate which seems meaningless to pinctrl, so it looks
increase overhead to pinctrl gpio ranges map.
However, it seems i may have to agree that we need keep align with the
exist of gpio design to use the standard way to get gpio number via
of_xlate function rather than do it privately in pinctrl driver.
One disadvantage is that i can not reuse of_get_named_gpio_flags due
to different format
for gpio-maps, i may have to write a slightly different one as
of_get_named_gpio_flags
for gpio-maps.
>> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
>
>> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
>
> The locking I was talking about before is between the following line:
>
>> + ? ? ? ? ? ? ranges[i].gc = of_node_to_gpiochip(np_gpio);
>
> and this code:
>
>> + ? ? ? ? ? ? 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;
>
> If of_node_to_gpiochip() doesn't mark the GPIO chip as "in use", then
> the module that provides that device could be unloaded between the two
> blocks of code above.
>
Correct.
> Re: your locking comments in your other email: ranges[i].gc doesn't
> appear to be used anywhere else in pinctrl, so I think it's OK not to
> lock the GPIO chip for any more time than between the above two blocks
> of code.
>
So i will add lock between them like:
ranges[i].gc = of_node_to_gpiochip(np_gpio);
if (!try_module_get(ranges[i].gc->owner))
err...
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;
module_put(ranges[i].gc->owner)
If anything wrong please let me know.
> Finally, just a minor nit:
>
>> + ? ? ? ? ? ? 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_gpio->name);
>> + ? ? ? ? ? ? ? ? ? ? of_node_put(np_gpio);
>> + ? ? ? ? ? ? ? ? ? ? return -EPROBE_DEFER;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? of_node_put(np_gpio);
>
> could be slightly simpler:
>
> + ? ? ? ? ? ? ? ranges[i].gc = of_node_to_gpiochip(np_gpio);
> + ? ? ? ? ? ? ? of_node_put(np_gpio); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> + ? ? ? ? ? ? ? if (!ranges[i].gc) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(pctldev->dev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "can not find gpio chip of node(%s)\n",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? np_gpio->name);
Because here still uese np_gpio, Can i still use it after of_node_put?
> + ? ? ? ? ? ? ? ? ? ? ? return -EPROBE_DEFER;
> + ? ? ? ? ? ? ? }
Regards
Dong Aisheng
On 05/23/2012 07:42 PM, Dong Aisheng wrote:
> On Thu, May 24, 2012 at 4:44 AM, Stephen Warren <[email protected]> wrote:
>> On 05/23/2012 07:22 AM, Dong Aisheng wrote:
>>> From: Dong Aisheng <[email protected]>
>>>
>>> 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_offset $pin_offset $npin>.
>>>
>>> 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 <[email protected]>
>>
>> This is mostly good. Just a few comments:
>>
>>> +gpio-maps: 4 integers array, each entry in the array represents a gpio
>>> +range with the format: <&gpio $gpio_offset $pin_offset $count>
>>> +- gpio: phandle pointing at gpio device node
>>> +- gpio_offset: integer, the local offset of $gpio
>>> +- pin_offset: integer, the pin offset or pin id
>>> +- npins: integer, the gpio ranges starting from pin_offset
>>
>> This uses a single cell to represent a GPIO ID within a GPIO controller.
>> The standard GPIO bindings use #gpio-cells, where that's a property in
>> the GPIO controller's node. I wonder if we shouldn't do the same here,
>> and call into the GPIO driver to parse #gpio-cells and give back the
>> Linux GPIO ID, just like of_get_named_gpio_flags() does. This would also
>> make this code able to cope with the GPIO of_xlate function returning a
>> different GPIO chip, which Grant put in place for banked GPIO controllers.
>>
> I checked the code, the second cell only represents gpio flag in
> of_gpio_simple_xlate which seems meaningless to pinctrl, so it looks
> increase overhead to pinctrl gpio ranges map.
With the simple translation function, yes it's just flags. However, not
all GPIO controllers use the simple translation function; I think I
recall the Exynos binding having 4 or 5 cells. In other words, the
format is defined by each individual GPIO controller, even if many/most
do happen to follow the same format.
> However, it seems i may have to agree that we need keep align with the
> exist of gpio design to use the standard way to get gpio number via
> of_xlate function rather than do it privately in pinctrl driver.
>
> One disadvantage is that i can not reuse of_get_named_gpio_flags due
> to different format
> for gpio-maps, i may have to write a slightly different one as
> of_get_named_gpio_flags
> for gpio-maps.
>
>>> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
>>
>>> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
>>
>> The locking I was talking about before is between the following line:
>>
>>> + ranges[i].gc = of_node_to_gpiochip(np_gpio);
>>
>> and this code:
>>
>>> + 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;
>>
>> If of_node_to_gpiochip() doesn't mark the GPIO chip as "in use", then
>> the module that provides that device could be unloaded between the two
>> blocks of code above.
>>
> Correct.
>
>> Re: your locking comments in your other email: ranges[i].gc doesn't
>> appear to be used anywhere else in pinctrl, so I think it's OK not to
>> lock the GPIO chip for any more time than between the above two blocks
>> of code.
>>
> So i will add lock between them like:
> ranges[i].gc = of_node_to_gpiochip(np_gpio);
> if (!try_module_get(ranges[i].gc->owner))
> err...
I think that module_get() needs to happen inside of_node_to_gpiochip(),
so that it executes inside any lock that function takes.
> 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;
> module_put(ranges[i].gc->owner)
> If anything wrong please let me know.
>
>> Finally, just a minor nit:
>>
>>> + 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_gpio->name);
>>> + of_node_put(np_gpio);
>>> + return -EPROBE_DEFER;
>>> + }
>>> +
>>> + of_node_put(np_gpio);
>>
>> could be slightly simpler:
>>
>> + ranges[i].gc = of_node_to_gpiochip(np_gpio);
>> + of_node_put(np_gpio); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>> + if (!ranges[i].gc) {
>> + dev_err(pctldev->dev,
>> + "can not find gpio chip of node(%s)\n",
>> + np_gpio->name);
> Because here still uese np_gpio, Can i still use it after of_node_put?
Oh right, that makes sense, yes.
On Thu, May 24, 2012 at 12:42:19PM +0800, Stephen Warren wrote:
> On 05/23/2012 07:42 PM, Dong Aisheng wrote:
> > On Thu, May 24, 2012 at 4:44 AM, Stephen Warren <[email protected]> wrote:
> >> On 05/23/2012 07:22 AM, Dong Aisheng wrote:
> >>> From: Dong Aisheng <[email protected]>
> >>>
> >>> 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_offset $pin_offset $npin>.
> >>>
> >>> 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 <[email protected]>
> >>
> >> This is mostly good. Just a few comments:
> >>
> >>> +gpio-maps: 4 integers array, each entry in the array represents a gpio
> >>> +range with the format: <&gpio $gpio_offset $pin_offset $count>
> >>> +- gpio: phandle pointing at gpio device node
> >>> +- gpio_offset: integer, the local offset of $gpio
> >>> +- pin_offset: integer, the pin offset or pin id
> >>> +- npins: integer, the gpio ranges starting from pin_offset
> >>
> >> This uses a single cell to represent a GPIO ID within a GPIO controller.
> >> The standard GPIO bindings use #gpio-cells, where that's a property in
> >> the GPIO controller's node. I wonder if we shouldn't do the same here,
> >> and call into the GPIO driver to parse #gpio-cells and give back the
> >> Linux GPIO ID, just like of_get_named_gpio_flags() does. This would also
> >> make this code able to cope with the GPIO of_xlate function returning a
> >> different GPIO chip, which Grant put in place for banked GPIO controllers.
> >>
> > I checked the code, the second cell only represents gpio flag in
> > of_gpio_simple_xlate which seems meaningless to pinctrl, so it looks
> > increase overhead to pinctrl gpio ranges map.
>
> With the simple translation function, yes it's just flags. However, not
> all GPIO controllers use the simple translation function; I think I
> recall the Exynos binding having 4 or 5 cells. In other words, the
> format is defined by each individual GPIO controller, even if many/most
> do happen to follow the same format.
>
Correct, it should be SoC dependent.
> > However, it seems i may have to agree that we need keep align with the
> > exist of gpio design to use the standard way to get gpio number via
> > of_xlate function rather than do it privately in pinctrl driver.
> >
> > One disadvantage is that i can not reuse of_get_named_gpio_flags due
> > to different format
> > for gpio-maps, i may have to write a slightly different one as
> > of_get_named_gpio_flags
> > for gpio-maps.
> >
> >>> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
> >>
> >>> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
> >>
> >> The locking I was talking about before is between the following line:
> >>
> >>> + ranges[i].gc = of_node_to_gpiochip(np_gpio);
> >>
> >> and this code:
> >>
> >>> + 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;
> >>
> >> If of_node_to_gpiochip() doesn't mark the GPIO chip as "in use", then
> >> the module that provides that device could be unloaded between the two
> >> blocks of code above.
> >>
> > Correct.
> >
> >> Re: your locking comments in your other email: ranges[i].gc doesn't
> >> appear to be used anywhere else in pinctrl, so I think it's OK not to
> >> lock the GPIO chip for any more time than between the above two blocks
> >> of code.
> >>
> > So i will add lock between them like:
> > ranges[i].gc = of_node_to_gpiochip(np_gpio);
> > if (!try_module_get(ranges[i].gc->owner))
> > err...
>
> I think that module_get() needs to happen inside of_node_to_gpiochip(),
> so that it executes inside any lock that function takes.
Can you please help explain a bit more?
I did not quite understand.
It looks to me of_node_to_gpiochip is only convert the gpio node to gpio chip.
Why need get the module inside this function?
For gpio_request function, it also calls try_module_get(gc) after find the gpio
chip.
>
> > 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;
> > module_put(ranges[i].gc->owner)
> > If anything wrong please let me know.
> >
> >> Finally, just a minor nit:
> >>
> >>> + 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_gpio->name);
> >>> + of_node_put(np_gpio);
> >>> + return -EPROBE_DEFER;
> >>> + }
> >>> +
> >>> + of_node_put(np_gpio);
> >>
> >> could be slightly simpler:
> >>
> >> + ranges[i].gc = of_node_to_gpiochip(np_gpio);
> >> + of_node_put(np_gpio); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> >> + if (!ranges[i].gc) {
> >> + dev_err(pctldev->dev,
> >> + "can not find gpio chip of node(%s)\n",
> >> + np_gpio->name);
> > Because here still uese np_gpio, Can i still use it after of_node_put?
>
> Oh right, that makes sense, yes.
>
I guess you mean no(can not use the node after of_node_put), right?
Regards
Dong Aisheng
On Wed, May 23, 2012 at 3:22 PM, Dong Aisheng <[email protected]> wrote:
> From: Dong Aisheng <[email protected]>
>
> Acked-by: Stephen Warren <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
> ChangeLog v2->v3:
> * remove pinctrl_remove_gpio_ranges function.
Applied this v3 version of both patches as well.
Yours,
Linus Walleij
On 05/23/2012 11:19 PM, Dong Aisheng wrote:
> On Thu, May 24, 2012 at 12:42:19PM +0800, Stephen Warren wrote:
>> On 05/23/2012 07:42 PM, Dong Aisheng wrote:
>>> On Thu, May 24, 2012 at 4:44 AM, Stephen Warren <[email protected]> wrote:
>>>> On 05/23/2012 07:22 AM, Dong Aisheng wrote:
>>>>> From: Dong Aisheng <[email protected]>
>>>>>
>>>>> 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_offset $pin_offset $npin>.
>>>>>
>>>>> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
>>>>> to parse and register the gpio ranges from device tree.
...
>>>> Re: your locking comments in your other email: ranges[i].gc doesn't
>>>> appear to be used anywhere else in pinctrl, so I think it's OK not to
>>>> lock the GPIO chip for any more time than between the above two blocks
>>>> of code.
>>>
>>> So i will add lock between them like:
>>> ranges[i].gc = of_node_to_gpiochip(np_gpio);
>>> if (!try_module_get(ranges[i].gc->owner))
>>> err...
>>
>> I think that module_get() needs to happen inside of_node_to_gpiochip(),
>> so that it executes inside any lock that function takes.
>
> Can you please help explain a bit more?
> I did not quite understand.
> It looks to me of_node_to_gpiochip is only convert the gpio node to gpio chip.
> Why need get the module inside this function?
> For gpio_request function, it also calls try_module_get(gc) after find the gpio
> chip.
The problem is this:
Thread 1: Call of_node_to_gpiochip(), returns a gpio_chip.
Thread 2: Unregisters the same gpio_chip that was returned above.
Thread 1: Accesses the now unregistered (and possibly free'd) gpio_chip
-> at best, bad data, at worst, OOPS.
In order to prevent this, of_node_to_gpiochip() should take measures to
prevent another thread from unregistering the gpio_chip until thread 1
has completed its step above.
The existing of_get_named_gpio_flags() is safe from this, since
gpiochip_find() acquires the GPIO lock, and all accesses to the fouond
gpio chip occur with that lock held, inside the match function. Perhaps
a similar approach could be used here.
>>>> Finally, just a minor nit:
...
>>>> could be slightly simpler:
...
>>> Because here still uese np_gpio, Can i still use it after of_node_put?
>>
>> Oh right, that makes sense, yes.
>>
> I guess you mean no(can not use the node after of_node_put), right?
I mean the original code in your patch is fine.
On Thu, May 24, 2012 at 11:22:13PM +0800, Stephen Warren wrote:
> On 05/23/2012 11:19 PM, Dong Aisheng wrote:
> > On Thu, May 24, 2012 at 12:42:19PM +0800, Stephen Warren wrote:
> >> On 05/23/2012 07:42 PM, Dong Aisheng wrote:
> >>> On Thu, May 24, 2012 at 4:44 AM, Stephen Warren <[email protected]> wrote:
> >>>> On 05/23/2012 07:22 AM, Dong Aisheng wrote:
> >>>>> From: Dong Aisheng <[email protected]>
> >>>>>
> >>>>> 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_offset $pin_offset $npin>.
> >>>>>
> >>>>> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
> >>>>> to parse and register the gpio ranges from device tree.
> ...
> >>>> Re: your locking comments in your other email: ranges[i].gc doesn't
> >>>> appear to be used anywhere else in pinctrl, so I think it's OK not to
> >>>> lock the GPIO chip for any more time than between the above two blocks
> >>>> of code.
> >>>
> >>> So i will add lock between them like:
> >>> ranges[i].gc = of_node_to_gpiochip(np_gpio);
> >>> if (!try_module_get(ranges[i].gc->owner))
> >>> err...
> >>
> >> I think that module_get() needs to happen inside of_node_to_gpiochip(),
> >> so that it executes inside any lock that function takes.
> >
> > Can you please help explain a bit more?
> > I did not quite understand.
> > It looks to me of_node_to_gpiochip is only convert the gpio node to gpio chip.
> > Why need get the module inside this function?
> > For gpio_request function, it also calls try_module_get(gc) after find the gpio
> > chip.
>
> The problem is this:
>
> Thread 1: Call of_node_to_gpiochip(), returns a gpio_chip.
> Thread 2: Unregisters the same gpio_chip that was returned above.
> Thread 1: Accesses the now unregistered (and possibly free'd) gpio_chip
> -> at best, bad data, at worst, OOPS.
>
Correct. We did have this issue.
Thanks for clarify.
> In order to prevent this, of_node_to_gpiochip() should take measures to
> prevent another thread from unregistering the gpio_chip until thread 1
> has completed its step above.
>
> The existing of_get_named_gpio_flags() is safe from this, since
> gpiochip_find() acquires the GPIO lock, and all accesses to the fouond
> gpio chip occur with that lock held, inside the match function. Perhaps
> a similar approach could be used here.
Why it looks to me of_get_named_gpio_flags has the same issue and also not safe?
For of_node_to_gpiochip itself called in of_get_named_gpio_flags, it's safe.
But after that, i'm suspecting it has the same issue as you described above, right?
For example:
int of_get_named_gpio_flags(struct device_node *np, const char *propname,
int index, enum of_gpio_flags *flags)
{
...
gc = of_node_to_gpiochip(gpiospec.np);
if (!gc) {
pr_debug("%s: gpio controller %s isn't registered\n",
np->full_name, gpiospec.np->full_name);
ret = -ENODEV;
goto err1;
}
===> the gc may be unregistered here by another thread and
even already have been freed, right?
ret = gc->of_xlate(gc, &gpiospec, flags);
...
}
Maybe we need get the lock in of_node_to_gpiochip and release it by calling
of_gpio_put(..) after using?
Regards
Dong Aisheng
On 05/24/2012 09:22 PM, Dong Aisheng wrote:
> On Thu, May 24, 2012 at 11:22:13PM +0800, Stephen Warren wrote:
...
>> The problem is this:
>>
>> Thread 1: Call of_node_to_gpiochip(), returns a gpio_chip.
>> Thread 2: Unregisters the same gpio_chip that was returned above.
>> Thread 1: Accesses the now unregistered (and possibly free'd) gpio_chip
>> -> at best, bad data, at worst, OOPS.
>>
> Correct. We did have this issue.
> Thanks for clarify.
>
>> In order to prevent this, of_node_to_gpiochip() should take measures to
>> prevent another thread from unregistering the gpio_chip until thread 1
>> has completed its step above.
>>
>> The existing of_get_named_gpio_flags() is safe from this, since
>> gpiochip_find() acquires the GPIO lock, and all accesses to the fouond
>> gpio chip occur with that lock held, inside the match function. Perhaps
>> a similar approach could be used here.
>
> Why it looks to me of_get_named_gpio_flags has the same issue and also not safe?
> For of_node_to_gpiochip itself called in of_get_named_gpio_flags, it's safe.
Uggh. Yes, I meant that of_node_to_gpiochip() itself doesn't have this
issue, but you're right, it looks like of_get_named_gpio_flags() does.
> But after that, i'm suspecting it has the same issue as you described above, right?
>
> For example:
> int of_get_named_gpio_flags(struct device_node *np, const char *propname,
> int index, enum of_gpio_flags *flags)
> {
> ...
> gc = of_node_to_gpiochip(gpiospec.np);
> if (!gc) {
> pr_debug("%s: gpio controller %s isn't registered\n",
> np->full_name, gpiospec.np->full_name);
> ret = -ENODEV;
> goto err1;
> }
>
> ===> the gc may be unregistered here by another thread and
> even already have been freed, right?
>
> ret = gc->of_xlate(gc, &gpiospec, flags);
> ...
> }
>
> Maybe we need get the lock in of_node_to_gpiochip and release it by calling
> of_gpio_put(..) after using?
Yes, something like that; it should take the module lock, not the gpio lock.
On Fri, May 25, 2012 at 12:59:47PM +0800, Stephen Warren wrote:
> On 05/24/2012 09:22 PM, Dong Aisheng wrote:
> > On Thu, May 24, 2012 at 11:22:13PM +0800, Stephen Warren wrote:
> ...
> >> The problem is this:
> >>
> >> Thread 1: Call of_node_to_gpiochip(), returns a gpio_chip.
> >> Thread 2: Unregisters the same gpio_chip that was returned above.
> >> Thread 1: Accesses the now unregistered (and possibly free'd) gpio_chip
> >> -> at best, bad data, at worst, OOPS.
> >>
> > Correct. We did have this issue.
> > Thanks for clarify.
> >
> >> In order to prevent this, of_node_to_gpiochip() should take measures to
> >> prevent another thread from unregistering the gpio_chip until thread 1
> >> has completed its step above.
> >>
> >> The existing of_get_named_gpio_flags() is safe from this, since
> >> gpiochip_find() acquires the GPIO lock, and all accesses to the fouond
> >> gpio chip occur with that lock held, inside the match function. Perhaps
> >> a similar approach could be used here.
> >
> > Why it looks to me of_get_named_gpio_flags has the same issue and also not safe?
> > For of_node_to_gpiochip itself called in of_get_named_gpio_flags, it's safe.
>
> Uggh. Yes, I meant that of_node_to_gpiochip() itself doesn't have this
> issue, but you're right, it looks like of_get_named_gpio_flags() does.
>
> > But after that, i'm suspecting it has the same issue as you described above, right?
> >
> > For example:
> > int of_get_named_gpio_flags(struct device_node *np, const char *propname,
> > int index, enum of_gpio_flags *flags)
> > {
> > ...
> > gc = of_node_to_gpiochip(gpiospec.np);
> > if (!gc) {
> > pr_debug("%s: gpio controller %s isn't registered\n",
> > np->full_name, gpiospec.np->full_name);
> > ret = -ENODEV;
> > goto err1;
> > }
> >
> > ===> the gc may be unregistered here by another thread and
> > even already have been freed, right?
> >
> > ret = gc->of_xlate(gc, &gpiospec, flags);
> > ...
> > }
> >
> > Maybe we need get the lock in of_node_to_gpiochip and release it by calling
> > of_gpio_put(..) after using?
>
> Yes, something like that; it should take the module lock, not the gpio lock.
>
Okay, i will try to add it.
Regards
Dong Aisheng