Re: [PATCH RFC v3 3/3] pinctrl: add pinctrl gpio binding support

On 05/23/2012 07:42 PM, Dong Aisheng wrote:

On Thu, May 24, 2012 at 4:44 AM, Stephen Warren [off-list ref] wrote:

quoted

On 05/23/2012 07:22 AM, Dong Aisheng wrote:

quoted

From: Dong Aisheng <redacted>

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 <redacted>

This is mostly good. Just a few comments:

quoted

+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.

quoted

quoted

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

quoted

+int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,

The locking I was talking about before is between the following line:

quoted

+             ranges[i].gc = of_node_to_gpiochip(np_gpio);

and this code:

quoted

+             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.

quoted

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.

quoted

Finally, just a minor nit:

quoted

+             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.