On Fri, May 25, 2012 at 10:03 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 05/25/2012 07:36 AM, Dong Aisheng wrote:
>> From: Dong Aisheng <dong.aisheng at 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 at 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 at wwwdotorg.org>
Thanks.
Regards
Dong Aisheng