On Thu, Apr 24, 2014 at 09:06:24AM -0500, Rob Herring wrote:
> On Thu, Apr 24, 2014 at 7:47 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> > On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
> > <thierry.reding@xxxxxxxxx> wrote:
> >
> >> From: Thierry Reding <treding@xxxxxxxxxx>
> >>
> >> Many bindings use the -gpio suffix in property names. Support this in
> >> addition to the -gpios suffix when requesting GPIOs using the new
> >> descriptor-based API.
> >>
> >> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> >
> > Are the DT bindings really full of such ambiguity between
> > singular and plural? Examples?
> >
> > What happens in affected drivers today? It just doesn't work?
>
> They mostly seem to use of_get_named_gpio.
Indeed. That has the downside of requiring manual parsing and handling
of the GPIO polarity, though.
> > Does that mean these bindings are not actively used by any
> > drivers yet so we could augment the bindings instead, or are
> > they already deployed so we must implement this?
> >
> > Would like a word from the DT people here...
>
> Interestingly, there is not a single occurrence of '-gpio ' in
> powerpc, but a bunch in ARM. In hindsight, we should have perhaps
> enforced using -gpios only, but that doesn't really matter now. We
> have -gpio in use, so we need to support it.
I think I also saw a proposal only recently to add support for a
gpios/gpio-names type of binding
> That doesn't necessarily mean this function has to support it. For
> example, this function could a legacy method and some other function
> should be used instead (of_get_named_gpio perhaps).
The reason why I posted this is precisely because I wanted to convert
over some drivers to use the new helpers (because they make things like
polarity handling much easier). My first attempt was to fix the binding
because I was under the impression that -gpio usage was discouraged, but
people didn't like that because, you know, DT bindings being a stable
ABI and all that.
The downside of not allowing the gpiod API to support the -gpio suffix
is that we'll never be able to convert drivers that use such a binding
and will forever have a hodgepodge of GPIO APIs that we need to support.
> >> drivers/gpio/gpiolib.c | 18 ++++++++++++------
> >> 1 file changed, 12 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >> index 7a0b97076374..b991462c22fb 100644
> >> --- a/drivers/gpio/gpiolib.c
> >> +++ b/drivers/gpio/gpiolib.c
> >> @@ -2594,17 +2594,23 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
> >> unsigned int idx,
> >> enum gpio_lookup_flags *flags)
> >> {
> >> + static const char *suffixes[] = { "gpios", "gpio" };
> >> char prop_name[32]; /* 32 is max size of property name */
> >> enum of_gpio_flags of_flags;
> >> struct gpio_desc *desc;
> >> + unsigned int i;
> >>
> >> - if (con_id)
> >> - snprintf(prop_name, 32, "%s-gpios", con_id);
> >> - else
> >> - snprintf(prop_name, 32, "gpios");
> >> + for (i = 0; i < ARRAY_SIZE(suffixes); i++) {
> >> + if (con_id)
> >> + snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]);
> >> + else
> >> + snprintf(prop_name, 32, "%s", suffixes[i]);
>
> This has the side effect of searching for "gpio" as property name
> which I don't think we want to allow.
Why don't we want to allow a "gpio" property when we already allow
"gpios"?
> It also allows a DT with either suffix to work. While I don't
> necessarily think the kernel's job should be DT validation, we don't
> have any other validation today and I don't see how this change
> simplifies the code. Between stricter DT checking (in the kernel) and
> simpler code, I'd pick the latter.
I had briefly considered adding more validation here as well, such as
refusing to hand out any GPIO with idx > 0 for the -gpio suffix, but
then opted not to do that in favour of code simplicity.
Thierry
Attachment:
pgpuonFgp_0te.pgp
Description: PGP signature
