Messages in this thread | | | Date | Sat, 26 May 2012 09:52:32 -0700 | Subject | Re: [PATCH v4 6/6] pinctrl: add pinctrl gpio binding support | From | Dong Aisheng <> |
| |
On Fri, May 25, 2012 at 10:03 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 05/25/2012 07:36 AM, Dong Aisheng wrote: >> From: Dong Aisheng <dong.aisheng@linaro.org> >> >> This patch implements a standard common binding for pinctrl gpio ranges. >> Each SoC can add gpio ranges through device tree by adding a gpio-maps property >> under their pinctrl devices node with the format: >> <&gpio $gpio-specifier $pin_offset $count> >> while the gpio phandle and gpio-specifier are the standard approach >> to represent a gpio in device tree. >> Then we can cooperate it with the gpio xlate function to get the gpio number >> from device tree to set up the gpio ranges map. >> >> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node) >> to parse and register the gpio ranges from device tree. >> >> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org> >> --- >> Personally i'm not very satisfied with current solution due to a few reasons: >> 1) i can not user standard gpio api to get gpio number >> 2) i need to reinvent a new api of_parse_phandles_with_args_ext which i'm not >> sure if it can be accepted by DT maintainer. >> If i did not invent that API, i need to rewrite a lot of duplicated code >> with slight differences with the exist functions like of_get_named_gpio_flags >> and of_parse_phandle_with_args for the special pinctrl gpio maps format. >> >> So i just sent it out first to see people's comment and if any better solution. >> >> One alternative solution is that that the gpio-maps into two parts: >> pinctrl-gpios = <&gpio_phandle gpio-specifier ..> >> pinctrl-gpio-maps = <pin_id count ..> >> Then we can reuse the standard gpio api altough it's not better than the >> original one. > > The problem I see with that is that it splits what is essentially a > single array with phandle+specifier+pin-id+count into two separate > arrays. Anyone reading/editing the DT needs to fully understand this, > and keep the entries in the two properties in the same order. Putting > everything into a single property makes this much more obvious to me. I Yes, i agree with you. That's why i insisted to send this format first.
> personally don't see any issue with the > of_parse_phandles_with_args_ext() function; it seems pretty clean to me. > Thanks, you gave me some confidence on it.
>> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c > >> + if (!nranges) { >> + dev_err(pctldev->dev, "no gpio ranges found\n"); >> + return -ENODEV; >> + } > > In the case of a generic pinctrl IP block that can support an external > GPIO controller but happens not to be hooked up to one within a > particular SoC, that might not be an error. However, that situation is > pretty unlikely, so I think it's find to call dev_err() for now, and we > can change it later if we need. > >> + ranges[i].base = ranges[i].gc->of_xlate(ranges[i].gc, &gpiospec, NULL); > > I believe Grant wants to change the of_xlate prototype in order to be > able to return a different gc value, so this will probably need slight > rework work with that change, once they're both approved. Still, I think > this is fine for now. > I looked Grant's commit 3d0f7cf0f "gpio: Adjust of_xlate API to support multiple GPIO chips", it seemed i need make some changes here since of_node_to_gpiochip is broken now after support banked gpio. Thus the gc got here may not correct for some special gpio controllers.
>> + if (ranges[i].base < 0) { >> + ret = -EINVAL; >> + goto out; >> + } >> + ranges[i].base += ranges[i].gc->base; >> + ranges[i].pin_base = gpiospec.args[gpiospec.args_count - 2]; >> + ranges[i].npins = gpiospec.args[gpiospec.args_count - 1]; >> + >> + gpiochip_put(ranges[i].gc); > > I wonder if this shouldn't happen until the pinctrl device is free'd, > and all the GPIO ranges are removed from it? > Hmm, that may bring some complexities since non-dt case also needs to be covered if we do that...
> If we don't do that, I would argue that we shouldn't store ranges[i].gc, > since it might become invalid - I believe the only use of it is within > this function? > In my option, i think it's ok to store it since they're just some data to describe hw properties. The gpio function may become invalid but not data. Is it reasonable to you?
>> + of_node_put(gpiospec.np); >> + } > > Aside from the comments I've made, this series all seems reasonable. > There certainly are alternative ways of doing some of it, but I don't > see any other approach having any particular advantage over this one. > So, the series, > > Acked-by: Stephen Warren <swarren@wwwdotorg.org> Thanks.
Regards Dong Aisheng -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |