From: Thierry Reding <[email protected]>
of_find_gpio() is always called under an IS_ENABLED(CONFIG_OF), so the
dummy implementation provided for !OF configurations is not needed.
Signed-off-by: Thierry Reding <[email protected]>
---
drivers/gpio/gpiolib.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ee1819fdcf35..7a0b97076374 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2590,7 +2590,6 @@ void gpiod_add_lookup_table(struct gpiod_lookup_table *table)
mutex_unlock(&gpio_lookup_lock);
}
-#ifdef CONFIG_OF
static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
unsigned int idx,
enum gpio_lookup_flags *flags)
@@ -2615,14 +2614,6 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
return desc;
}
-#else
-static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
- unsigned int idx,
- enum gpio_lookup_flags *flags)
-{
- return ERR_PTR(-ENODEV);
-}
-#endif
static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
unsigned int idx,
--
1.9.2
From: Thierry Reding <[email protected]>
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 <[email protected]>
---
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]);
- desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
- &of_flags);
+ desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
+ &of_flags);
+ if (!IS_ERR(desc))
+ break;
+ }
if (IS_ERR(desc))
return desc;
--
1.9.2
On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
<[email protected]> wrote:
> From: Thierry Reding <[email protected]>
>
> of_find_gpio() is always called under an IS_ENABLED(CONFIG_OF), so the
> dummy implementation provided for !OF configurations is not needed.
>
> Signed-off-by: Thierry Reding <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
<[email protected]> wrote:
> From: Thierry Reding <[email protected]>
>
> 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 <[email protected]>
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?
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...
> 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]);
>
> - desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
> - &of_flags);
> + desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
> + &of_flags);
> + if (!IS_ERR(desc))
> + break;
> + }
Code snippet left for reference on devicetree ML.
Yours,
Linus Walleij
On Thu, Apr 24, 2014 at 7:47 AM, Linus Walleij <[email protected]> wrote:
> On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
> <[email protected]> wrote:
>
>> From: Thierry Reding <[email protected]>
>>
>> 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 <[email protected]>
>
> 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.
>
> 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. 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).
>> 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. 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.
Rob
On Thu, Apr 24, 2014 at 09:06:24AM -0500, Rob Herring wrote:
> On Thu, Apr 24, 2014 at 7:47 AM, Linus Walleij <[email protected]> wrote:
> > On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
> > <[email protected]> wrote:
> >
> >> From: Thierry Reding <[email protected]>
> >>
> >> 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 <[email protected]>
> >
> > 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
On Thu, Apr 24, 2014 at 11:06 PM, Rob Herring <[email protected]> wrote:
> On Thu, Apr 24, 2014 at 7:47 AM, Linus Walleij <[email protected]> wrote:
>> On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
>> <[email protected]> wrote:
>>
>>> From: Thierry Reding <[email protected]>
>>>
>>> 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 <[email protected]>
>>
>> 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.
In an idea world of_get_named_gpio() would be gpiolib-private so
people cannot come with their own custom-named DT GPIO properties.
Given its broad usage this is not possible, but maybe we can at least
do it for of_get_named_gpiod().
>
>>
>> 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. 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).
It seems like we have to support that use-case indeed (many instances
in arch/arm). The incentive for handling this in that function vs.
user code is that having support here would allow drivers to directly
use gpiod_get() and having it automatically handle GPIO properties
like active-low instead of requiring user code to handle it by itself
every time.
Without this many drivers for devices using "-gpio" properties could
not switch to the new gpiod interface.
So as far as I'm concerned this code makes GPIO user-code easier. This
is not to say that we should allow that "-gpio" suffix for new
bindings.
Acked-by: Alexandre Courbot <[email protected]>
On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
<[email protected]> wrote:
> From: Thierry Reding <[email protected]>
>
> 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 <[email protected]>
It appears this can save quite a lot of code in drivers, work that
I trust Thierry to persue based on this to some extent so patch is
tentatively applied unless something comes up.
Yours,
Linus Walleij
On 04/24/2014 12:22 PM, Thierry Reding wrote:
...
> 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.
Perhaps rather than making the existing gpiod API automatically search
for both -gpios and -gpio, we could make a new API for the other suffix,
so that driver indicate explicitly which property name they want. That
way, someone can't accidentally write -gpio in the DT and have it still
work. Or, add a parameter to the existing API, but that's probably a lot
more churn.
* Linus Walleij <[email protected]> [140425 00:53]:
> On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
> <[email protected]> wrote:
>
> > From: Thierry Reding <[email protected]>
> >
> > 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 <[email protected]>
>
> It appears this can save quite a lot of code in drivers, work that
> I trust Thierry to persue based on this to some extent so patch is
> tentatively applied unless something comes up.
Looks like this patch causes a regression where GPIOs on I2C will
no longer return -EPROBE_DEFER but seem to return -ENOENT instead.
This breaks drivers using things like devm_gpiod_get_index()
on a GPIO that's on a I2C bus not probed yet.
Reverting commit dd34c37aa3e (gpio: of: Allow -gpio suffix for
property names) fixes things.
Regards,
Tony
* Tony Lindgren <[email protected]> [140602 16:06]:
> * Linus Walleij <[email protected]> [140425 00:53]:
> > On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
> > <[email protected]> wrote:
> >
> > > From: Thierry Reding <[email protected]>
> > >
> > > 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 <[email protected]>
> >
> > It appears this can save quite a lot of code in drivers, work that
> > I trust Thierry to persue based on this to some extent so patch is
> > tentatively applied unless something comes up.
>
> Looks like this patch causes a regression where GPIOs on I2C will
> no longer return -EPROBE_DEFER but seem to return -ENOENT instead.
>
> This breaks drivers using things like devm_gpiod_get_index()
> on a GPIO that's on a I2C bus not probed yet.
>
> Reverting commit dd34c37aa3e (gpio: of: Allow -gpio suffix for
> property names) fixes things.
Looks like something like below fixes the issue.
Regards,
Tony
8< -----------------------
From: Tony Lindgren <[email protected]>
Date: Mon, 2 Jun 2014 16:13:46 -0700
Subject: [PATCH] gpio: of: Fix handling for deferred probe for -gpio suffix
Commit dd34c37aa3e (gpio: of: Allow -gpio suffix for property names)
added parsing for both -gpio and -gpios suffix but also changed
the handling for deferred probe unintentionally. Because of the
looping the second name will now return -ENOENT instead of
-EPROBE_DEFER. Fix the issue by breaking out of the loop if
-EPROBE_DEFER is encountered.
Signed-off-by: Tony Lindgren <[email protected]>
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2614,7 +2614,7 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
&of_flags);
- if (!IS_ERR(desc))
+ if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER))
break;
}
On Mon, Jun 02, 2014 at 04:14:23PM -0700, Tony Lindgren wrote:
> * Tony Lindgren <[email protected]> [140602 16:06]:
> > * Linus Walleij <[email protected]> [140425 00:53]:
> > > On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
> > > <[email protected]> wrote:
> > >
> > > > From: Thierry Reding <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > >
> > > It appears this can save quite a lot of code in drivers, work that
> > > I trust Thierry to persue based on this to some extent so patch is
> > > tentatively applied unless something comes up.
> >
> > Looks like this patch causes a regression where GPIOs on I2C will
> > no longer return -EPROBE_DEFER but seem to return -ENOENT instead.
> >
> > This breaks drivers using things like devm_gpiod_get_index()
> > on a GPIO that's on a I2C bus not probed yet.
> >
> > Reverting commit dd34c37aa3e (gpio: of: Allow -gpio suffix for
> > property names) fixes things.
>
> Looks like something like below fixes the issue.
>
> Regards,
>
> Tony
>
> 8< -----------------------
> From: Tony Lindgren <[email protected]>
> Date: Mon, 2 Jun 2014 16:13:46 -0700
> Subject: [PATCH] gpio: of: Fix handling for deferred probe for -gpio suffix
>
> Commit dd34c37aa3e (gpio: of: Allow -gpio suffix for property names)
> added parsing for both -gpio and -gpios suffix but also changed
> the handling for deferred probe unintentionally. Because of the
> looping the second name will now return -ENOENT instead of
> -EPROBE_DEFER. Fix the issue by breaking out of the loop if
> -EPROBE_DEFER is encountered.
>
> Signed-off-by: Tony Lindgren <[email protected]>
>
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2614,7 +2614,7 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>
> desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
> &of_flags);
> - if (!IS_ERR(desc))
> + if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER))
> break;
> }
This looks good to me:
Reviewed-by: Thierry Reding <[email protected]>
On Tue, Jun 3, 2014 at 1:14 AM, Tony Lindgren <[email protected]> wrote:
> Looks like something like below fixes the issue.
>
> Regards,
>
> Tony
>
> 8< -----------------------
> From: Tony Lindgren <[email protected]>
> Date: Mon, 2 Jun 2014 16:13:46 -0700
> Subject: [PATCH] gpio: of: Fix handling for deferred probe for -gpio suffix
>
> Commit dd34c37aa3e (gpio: of: Allow -gpio suffix for property names)
> added parsing for both -gpio and -gpios suffix but also changed
> the handling for deferred probe unintentionally. Because of the
> looping the second name will now return -ENOENT instead of
> -EPROBE_DEFER. Fix the issue by breaking out of the loop if
> -EPROBE_DEFER is encountered.
>
> Signed-off-by: Tony Lindgren <[email protected]>
Patch applied for fixes with Thierry's Reveiwed-by tag.
Yours,
Linus Walleij