[PATCH] GPIOD, OF: parse flags into gpiod

[PATCH] GPIOD, OF: parse flags into gpiod

[Robert ABEL]

GPIO descriptions were assumed to be initialized with correct flags
by GPIO chips. Hence, DT flags were being ignored.
Translate flags from DTS flags to GPIOD flags when of_get_*gpiod_* is
called.

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 drivers/gpio/gpiolib-of.c     | 12 ++++++++----
 drivers/gpio/gpiolib.c        | 12 ++++++++++++
 include/linux/gpio/consumer.h |  4 ++++
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 2024d45..03f261c 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -66,18 +66,21 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
 struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 		     const char *propname, int index, enum of_gpio_flags *flags)
 {
+
+	enum of_gpio_flags tmp_flags;
+	enum of_gpio_flags* priv_flags = flags ? : &tmp_flags;
+
 	/* Return -EPROBE_DEFER to support probe() functions to be called
 	 * later when the GPIO actually becomes available
 	 */
 	struct gg_data gg_data = {
-		.flags = flags,
+		.flags = priv_flags,
 		.out_gpio = ERR_PTR(-EPROBE_DEFER)
 	};
 	int ret;
 
 	/* .of_xlate might decide to not fill in the flags, so clear it. */
-	if (flags)
-		*flags = 0;
+	*priv_flags = 0;
 
 	ret = of_parse_phandle_with_args(np, propname, "#gpio-cells", index,
 					 &gg_data.gpiospec);
@@ -87,7 +90,8 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 		return ERR_PTR(ret);
 	}
 
-	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
+	if (gpiochip_find(&gg_data, of_gpiochip_find_and_xlate))
+		gpiod_translate_flags(gg_data.out_gpio, gg_data.flags);
 
 	of_node_put(gg_data.gpiospec.np);
 	pr_debug("%s exited with status %d\n", __func__,
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f48817d..8bd4dbb 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -187,6 +187,18 @@ int desc_to_gpio(const struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(desc_to_gpio);
 
+#if defined(CONFIG_OF)
+void gpiod_translate_flags(struct gpio_desc *desc, enum of_gpio_flags *flags)
+{
+
+	desc->flags = 0;
+
+	if (*flags & OF_GPIO_ACTIVE_LOW)
+		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+
+}
+EXPORT_SYMBOL_GPL(gpiod_translate_flags);
+#endif /* CONFIG_OF */
 
 /* Warn when drivers omit gpio_request() calls -- legal but ill-advised
  * when setting direction, and otherwise illegal.  Until board setup code
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index bed128e..4d120dc 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -59,6 +59,10 @@ int gpiod_to_irq(const struct gpio_desc *desc);
 /* Convert between the old gpio_ and new gpiod_ interfaces */
 struct gpio_desc *gpio_to_desc(unsigned gpio);
 int desc_to_gpio(const struct gpio_desc *desc);
+#if defined(CONFIG_OF)
+enum of_gpio_flags;
+void gpiod_translate_flags(struct gpio_desc *desc, enum of_gpio_flags *flags);
+#endif /* CONFIG_OF */
 
 #else /* CONFIG_GPIOLIB */
 
-- 
1.9.2

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Alexandre Courbot]

On Tue, Apr 29, 2014 at 9:38 PM, Robert ABEL
<rabel@cit-ec.uni-bielefeld.de> wrote:
> GPIO descriptions were assumed to be initialized with correct flags
> by GPIO chips. Hence, DT flags were being ignored.
> Translate flags from DTS flags to GPIOD flags when of_get_*gpiod_* is
> called.

This considerably changes the behavior of of_get_named_gpiod_flags(),
and makes it apply DT flags to the GPIO descriptor no matter what. In
effect, it makes the flags argument completely unneeded.

of_get_named_gpiod_flags() ought to be gpiolib-private actually (I
still need to send a patch fixing that) since its only user is
of_find_gpio(), which correctly applies the flags.

We have users of of_get_named_gpio_flags() that could at first sight
benefit from your change. However, your change will return them a GPIO
which will behave differently from what they expect since the
active_low property will be set on its GPIO descriptor. Callers of
of_get_named_gpio_flags(), working on integer GPIOs, typically manage
that property themselves. This will result in the active_low property
being applied twice, effectively canceling its effect.

So the real solution to this issue is to make
of_get_named_gpiod_flags() private and encourage GPIO users to switch
to the descriptor interface. I'm afraid we cannot accept this patch
for the reasons given above.

Also, for future contributions could you use "gpiod:of: " in your
patch subject to keep the style consistent with existing practices in
drivers/gpio?

Short review follows.

>
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/gpio/gpiolib-of.c     | 12 ++++++++----
>  drivers/gpio/gpiolib.c        | 12 ++++++++++++
>  include/linux/gpio/consumer.h |  4 ++++
>  3 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 2024d45..03f261c 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -66,18 +66,21 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
>  struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
>                      const char *propname, int index, enum of_gpio_flags *flags)
>  {
> +
> +       enum of_gpio_flags tmp_flags;
> +       enum of_gpio_flags* priv_flags = flags ? : &tmp_flags;
> +
>         /* Return -EPROBE_DEFER to support probe() functions to be called
>          * later when the GPIO actually becomes available
>          */
>         struct gg_data gg_data = {
> -               .flags = flags,
> +               .flags = priv_flags,
>                 .out_gpio = ERR_PTR(-EPROBE_DEFER)
>         };
>         int ret;
>
>         /* .of_xlate might decide to not fill in the flags, so clear it. */
> -       if (flags)
> -               *flags = 0;
> +       *priv_flags = 0;
>
>         ret = of_parse_phandle_with_args(np, propname, "#gpio-cells", index,
>                                          &gg_data.gpiospec);
> @@ -87,7 +90,8 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
>                 return ERR_PTR(ret);
>         }
>
> -       gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> +       if (gpiochip_find(&gg_data, of_gpiochip_find_and_xlate))
> +               gpiod_translate_flags(gg_data.out_gpio, gg_data.flags);
>
>         of_node_put(gg_data.gpiospec.np);
>         pr_debug("%s exited with status %d\n", __func__,
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index f48817d..8bd4dbb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -187,6 +187,18 @@ int desc_to_gpio(const struct gpio_desc *desc)
>  }
>  EXPORT_SYMBOL_GPL(desc_to_gpio);
>
> +#if defined(CONFIG_OF)
> +void gpiod_translate_flags(struct gpio_desc *desc, enum of_gpio_flags *flags)

This function translates device tree flags - not just any tag. Its
name should reflect that.

> +{
> +
> +       desc->flags = 0;
> +
> +       if (*flags & OF_GPIO_ACTIVE_LOW)
> +               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +
> +}

You could also use this function to set the flags in of_find_gpio(),
since the code is the same.

> +EXPORT_SYMBOL_GPL(gpiod_translate_flags);

I don't think this function should have been exported. It is only to
be used in gpiolib-of.o, which will always be linked to gpiolib.o
anyway.

> +#endif /* CONFIG_OF */
>
>  /* Warn when drivers omit gpio_request() calls -- legal but ill-advised
>   * when setting direction, and otherwise illegal.  Until board setup code
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index bed128e..4d120dc 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -59,6 +59,10 @@ int gpiod_to_irq(const struct gpio_desc *desc);
>  /* Convert between the old gpio_ and new gpiod_ interfaces */
>  struct gpio_desc *gpio_to_desc(unsigned gpio);
>  int desc_to_gpio(const struct gpio_desc *desc);
> +#if defined(CONFIG_OF)
> +enum of_gpio_flags;
> +void gpiod_translate_flags(struct gpio_desc *desc, enum of_gpio_flags *flags);
> +#endif /* CONFIG_OF */

Again, this should not be part of the public API. These declarations
should have be moved into the gpiolib.h header.

>
>  #else /* CONFIG_GPIOLIB */
>
> --
> 1.9.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Robert Abel]

On 01.05.2014 08:24, Alexandre Courbot wrote:
> This considerably changes the behavior of of_get_named_gpiod_flags(), 
> and makes it apply DT flags to the GPIO descriptor no matter what.
Which should be done anyway, IMHO.

> In effect, it makes the flags argument completely unneeded.
Except for drivers who use the old integer interface and call 
desc_to_gpio themselves. Which is why I kept it.
> of_get_named_gpiod_flags() ought to be gpiolib-private actually (I 
> still need to send a patch fixing that) since its only user is 
> of_find_gpio(), which correctly applies the flags.
of_find_gpio does _not_ apply the flags correctly. I'm looking at 
dd34c37aa3e81715a1ed8da61fa438072428e188 here (head of for-next branch):

> static struct gpio_desc *of_find_gpio(struct device *dev, const char 
> *con_id,
>                       unsigned int idx,
>                       enum gpio_lookup_flags *flags)
> {
>     ...
>     enum of_gpio_flags of_flags;
>     struct gpio_desc *desc;
>
>     ...
>
>         desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
>                         &of_flags);
>     ...
>
>     if (of_flags & OF_GPIO_ACTIVE_LOW)
>         *flags |= GPIO_ACTIVE_LOW;
>
>     return desc;
> }

It just translates the parsed of_gpio_flags to gpio_lookup_flags for the 
passed flags argument --which btw must not be NULL, which it can be for 
many other exported functions. So the descriptor is strill _wrong_, i.e. 
desc->flags not set, and any subsequent call to set the gpio will behave 
incorrectly when flags were applied in the DT.
Which is why I fixed it in of_get_named_gpiod_flags to begin with.
IMHO gpio_desc should be a welcomed change to get away from drivers 
having to manage their I/O polarity themselves. But right now it's 
royally broken.

> We have users of of_get_named_gpio_flags() that could at first sight 
> benefit from your change. However, your change will return them a GPIO 
> which will behave differently from what they expect since the 
> active_low property will be set on its GPIO descriptor.
They shouldn't expect anything. gpio_desc is an opaque type for exactly 
that reason. Drivers should neither know nor care about the gpio_desc 
flags. It's an API inconsistency to actually let them see the flags in 
the first place -- but that's for historical reasons, apparently.

> Callers of of_get_named_gpio_flags(), working on integer GPIOs, 
> typically manage that property themselves. This will result in the 
> active_low property being applied twice, effectively canceling its effect.
The integer GPIO use-case is to convert gpio_desc to integers and call 
the appropriate integer functions (gpio_*) depending on the flags. All 
integer functions gpio_* call their gpiod_*_raw_* counterpart. The flags 
are _not_ applied twice. They're applied once if the driver is aware of 
them or not at all if the driver ignores them.

Any driver that mixes gpio_* and gpiod_* calls should not be expected to 
be stable and work anyways, correct? The flags might be applied twice if 
the driver parses them, is aware of them, but calls gpiod_* functions 
depending on the parsed flags. However, this is incorrect driver 
behavior. Though, to be fair, this distinction is less than ideally 
documented...

> Also, for future contributions could you use "gpiod:of: " in your 
> patch subject to keep the style consistent with existing practices in 
> drivers/gpio?
I didn't see any such usage, but OK.
>> ---
>> +#if defined(CONFIG_OF)
>> +void gpiod_translate_flags(struct gpio_desc *desc, enum of_gpio_flags *flags)
> This function translates device tree flags - not just any tag. Its
> name should reflect that.
I had gpiod_translate_of_flags at first, but that sounded awkward, as 
anything using the of abbreviation.
>> +{
>> +
>> +       desc->flags = 0;
>> +
>> +       if (*flags & OF_GPIO_ACTIVE_LOW)
>> +               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
>> +
>> +}
> You could also use this function to set the flags in of_find_gpio(),
> since the code is the same.
That would be ideal.
>> +EXPORT_SYMBOL_GPL(gpiod_translate_flags);
> I don't think this function should have been exported. It is only to
> be used in gpiolib-of.o, which will always be linked to gpiolib.o
> anyway.
I had trouble compiling without the export. I'll try again.

> Again, this should not be part of the public API. These declarations
> should have be moved into the gpiolib.h header.
OK.
>
>>   #else /* CONFIG_GPIOLIB */
>>
>> --
>> 1.9.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Robert Abel]

On 01.05.2014 08:24, Alexandre Courbot wrote:
> This considerably changes the behavior of of_get_named_gpiod_flags(), 
> and makes it apply DT flags to the GPIO descriptor no matter what.
Which should be done anyway, IMHO. I think this is correct and expected 
behavior.
Users (drivers, gpiolib, ...) expect to call of_get_named_gpiod_flags 
and get a usable and _correct_ gpio_desc back. However, right now, users 
_have_ to use of_get_named_gpiod_flags with the flags argument as there 
is no way to recover the lost flags when calling 
of_get_named_gpiod_flags(..., NULL).
I think correctness should start at the function level. 
of_get_named_gpiod_flags should be correct in and of itself. Even if 
private. Other functions should not have to be aware of the need to 
actively parse the flags to correct the gpio_desc->flags.

> In effect, it makes the flags argument completely unneeded.
Except for drivers who use the old integer interface and call 
desc_to_gpio themselves. Which is why I kept it.

> of_get_named_gpiod_flags() ought to be gpiolib-private actually (I 
> still need to send a patch fixing that)
Well, a few lines above you complained how my patch considerably changed 
this function. Yet you want to take away the ability to have a DT node 
with gpio references in fields not named "gpios" or "%s-gpio(s|)"...
Surely this is a much bigger breakage -- not just of the API?

> since its only user is of_find_gpio(), which correctly applies the flags.
of_find_gpio does _not_ apply the flags correctly. I'm looking at 
dd34c37aa3e81715a1ed8da61fa438072428e188 here (head of for-next branch):

> static struct gpio_desc *of_find_gpio(struct device *dev, const char 
> *con_id,
>                       unsigned int idx,
>                       enum gpio_lookup_flags *flags)
> {
>     ...
>     enum of_gpio_flags of_flags;
>     struct gpio_desc *desc;
>
>     ...
>
>         desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
>                         &of_flags);
>     ...
>
>     if (of_flags & OF_GPIO_ACTIVE_LOW)
>         *flags |= GPIO_ACTIVE_LOW;
>
>     return desc;
> }

It just translates the parsed of_gpio_flags to gpio_lookup_flags for the 
passed flags argument --which btw must not be NULL, which it can be for 
many other exported functions.
So the descriptor is strill _wrong_, i.e. desc->flags not set, and any 
subsequent call to set the gpio will behave incorrectly when flags were 
applied in the DT. Which is why I fixed it in of_get_named_gpiod_flags 
to begin with.
gpio_desc should be a welcomed change to get away from drivers having to 
manage their I/O polarity themselves. But right now it's royally broken.

> We have users of of_get_named_gpio_flags() that could at first sight 
> benefit from your change. However, your change will return them a GPIO 
> which will behave differently from what they expect since the 
> active_low property will be set on its GPIO descriptor.
They shouldn't expect anything. gpio_desc is an opaque type for exactly 
that reason. Drivers should neither know nor care about the gpio_desc 
flags. It's an API inconsistency to actually let them see the flags in 
the first place -- but that's for historical reasons, apparently.

> Callers of of_get_named_gpio_flags(), working on integer GPIOs, 
> typically manage that property themselves. This will result in the 
> active_low property being applied twice, effectively canceling its effect.
The integer GPIO use-case is to convert gpio_desc to integers and call 
the appropriate integer functions gpio_* depending on the flags. All 
integer functions gpio_* call their gpiod_*_raw_* counterpart. The flags 
are _not_ applied twice. They're applied once if the driver is aware of 
them or not at all if the driver ignores them.

Any driver that mixes gpio_* and gpiod_* API at a whim might not remain 
stable, granted.
The flags might be applied twice if the driver parses them, is aware of 
them, but calls gpiod_* functions depending on the parsed flags. 
However, this is incorrect driver behavior and drivers that rely on this 
API duality should be fixed.
Though, to be fair, this distinction might be less than ideally 
documented...

> Also, for future contributions could you use "gpiod:of: " in your 
> patch subject to keep the style consistent with existing practices in 
> drivers/gpio?
I didn't see any such usage, but OK.
>> ---
>> +#if defined(CONFIG_OF)
>> +void gpiod_translate_flags(struct gpio_desc *desc, enum of_gpio_flags *flags)
> This function translates device tree flags - not just any tag. Its
> name should reflect that.
I had gpiod_translate_of_flags at first, but that sounded awkward, as 
anything using the of abbreviation. <-- See what I did there?
>> +{
>> +
>> +       desc->flags = 0;
>> +
>> +       if (*flags & OF_GPIO_ACTIVE_LOW)
>> +               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
>> +
>> +}
> You could also use this function to set the flags in of_find_gpio(),
> since the code is the same.
That would be ideal if it turns out this will be necessary in the future.
>> +EXPORT_SYMBOL_GPL(gpiod_translate_flags);
> I don't think this function should have been exported. It is only to
> be used in gpiolib-of.o, which will always be linked to gpiolib.o
> anyway.
I had trouble compiling without the export. I'll try again.

>> +#if defined(CONFIG_OF)
>> +enum of_gpio_flags;
>> +void gpiod_translate_flags(struct gpio_desc *desc, enum of_gpio_flags *flags);
>> +#endif /* CONFIG_OF */
> Again, this should not be part of the public API. These declarations
> should have be moved into the gpiolib.h header.
OK.

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Alexandre Courbot]

On Mon, May 5, 2014 at 7:27 PM, Robert Abel
<rabel@cit-ec.uni-bielefeld.de> wrote:
> On 01.05.2014 08:24, Alexandre Courbot wrote:
>>
>> This considerably changes the behavior of of_get_named_gpiod_flags(), and
>> makes it apply DT flags to the GPIO descriptor no matter what.
>
> Which should be done anyway, IMHO. I think this is correct and expected
> behavior.

If you do that you will have to update the hundreds of direct and
indirect users of of_get_named_gpio(), or face their wrath because
their code will stop working all of a sudden. More detailed
explanation later in this mail.

> Users (drivers, gpiolib, ...) expect to call of_get_named_gpiod_flags and
> get a usable and _correct_ gpio_desc back. However, right now, users _have_
> to use of_get_named_gpiod_flags with the flags argument as there is no way
> to recover the lost flags when calling of_get_named_gpiod_flags(..., NULL).
> I think correctness should start at the function level.
> of_get_named_gpiod_flags should be correct in and of itself. Even if
> private. Other functions should not have to be aware of the need to actively
> parse the flags to correct the gpio_desc->flags.

As explainer earlier, of_get_named_gpiod_flags() should never have
been public in the first place. This is being addressed by
https://lkml.org/lkml/2014/5/3/155 .

Now that it is private, if you take a look at how it is used, you will
see that its current design actually makes sense.

The first user of this function is of_get_named_gpio_flags() and its
siblings, which all implement the legacy (but still dominant in
user-code) integer-based GPIO interface. For this interface, handling
of GPIO properties is up to the consumer and done through the flags
parameter. I fully agree that this is not optimal ; however that's how
the code is used and changing this cannot be done without a lot of
refactoring and/or angry users.

gpiod has been introduced as a saner, easier-to-use interface for
GPIOs. For this interface, we *do* want GPIO properties to be handled
transparently. That's why the only way of requesting a GPIO is through
gpiod_get(), which takes care of this.

>> In effect, it makes the flags argument completely unneeded.
>
> Except for drivers who use the old integer interface and call desc_to_gpio
> themselves. Which is why I kept it.

Exactly. But if you also set the flag in their back in
of_get_named_gpiod_flags(), they are not going to be happy about it...

>> of_get_named_gpiod_flags() ought to be gpiolib-private actually (I still
>> need to send a patch fixing that)
>
> Well, a few lines above you complained how my patch considerably changed
> this function. Yet you want to take away the ability to have a DT node with
> gpio references in fields not named "gpios" or "%s-gpio(s|)"...
> Surely this is a much bigger breakage -- not just of the API?

No API is broken by this. GPIOs in the DT are to be defined using the
-gpios suffix (Documentation/gpio/board.txt ). Just like regulators
are defined with the -supply suffix. This is enforced by the gpiod API
and therefore direct access to of_get_named_gpiod_flags() is unneeded,
and actually dangerous.

There might be some DT users that did not follow that rule and got
away with it because the old GPIO API is more permissive. These will
not be able to update to gpiod easily (examples welcome if this is
what motivated your patch), but this patch is not the correct way of
addressing this issue.

>> since its only user is of_find_gpio(), which correctly applies the flags.
>
> of_find_gpio does _not_ apply the flags correctly. I'm looking at
> dd34c37aa3e81715a1ed8da61fa438072428e188 here (head of for-next branch):

That's correct - because the flags are actually applied one level
higher by the only caller of of_find_gpio(), namely gpiod_get(). The
reason why this is done there and not earlier is because there are
other GPIO providers (ACPI, platform data) and we want the flags code
to be handled in a single place.

>
>> static struct gpio_desc *of_find_gpio(struct device *dev, const char
>> *con_id,
>>                       unsigned int idx,
>>                       enum gpio_lookup_flags *flags)
>> {
>>     ...
>>     enum of_gpio_flags of_flags;
>>     struct gpio_desc *desc;
>>
>>     ...
>>
>>         desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
>>                         &of_flags);
>>     ...
>>
>>     if (of_flags & OF_GPIO_ACTIVE_LOW)
>>         *flags |= GPIO_ACTIVE_LOW;
>>
>>     return desc;
>> }
>
>
> It just translates the parsed of_gpio_flags to gpio_lookup_flags for the
> passed flags argument --which btw must not be NULL, which it can be for many
> other exported functions.
> So the descriptor is strill _wrong_, i.e. desc->flags not set, and any
> subsequent call to set the gpio will behave incorrectly when flags were
> applied in the DT. Which is why I fixed it in of_get_named_gpiod_flags to
> begin with.
> gpio_desc should be a welcomed change to get away from drivers having to
> manage their I/O polarity themselves. But right now it's royally broken.

Considering that gpiod_get() is now the entry point to all the
functions named above in the gpiod API, and that it *does* handle the
flags correctly, it doesn't seem broken to me.

>
>> We have users of of_get_named_gpio_flags() that could at first sight
>> benefit from your change. However, your change will return them a GPIO which
>> will behave differently from what they expect since the active_low property
>> will be set on its GPIO descriptor.
>
> They shouldn't expect anything. gpio_desc is an opaque type for exactly that
> reason. Drivers should neither know nor care about the gpio_desc flags. It's
> an API inconsistency to actually let them see the flags in the first place
> -- but that's for historical reasons, apparently.

For the integer-based GPIO interface, there is nothing we can do about it.

For the gpiod interface, you are right in that the flags should be
completely hidden to user-code. That's why GPIOs can now only be
acquired through gpiod_get() which takes care of this.

>
>> Callers of of_get_named_gpio_flags(), working on integer GPIOs, typically
>> manage that property themselves. This will result in the active_low property
>> being applied twice, effectively canceling its effect.
>
> The integer GPIO use-case is to convert gpio_desc to integers and call the
> appropriate integer functions gpio_* depending on the flags. All integer
> functions gpio_* call their gpiod_*_raw_* counterpart. The flags are _not_
> applied twice. They're applied once if the driver is aware of them or not at
> all if the driver ignores them.

Consider the following code in pwm_bl.c:

    data->enable_gpio = of_get_named_gpio_flags(node, "enable-gpios", 0,
                            &flags);

    if (gpio_is_valid(data->enable_gpio) && (flags & OF_GPIO_ACTIVE_LOW))
        data->enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW;

    ....

        if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
            gpio_set_value(pb->enable_gpio, 0);
        else
            gpio_set_value(pb->enable_gpio, 1);

User code of the legacy GPIO API handles the active_low property by
itself because of_get_named_gpio_flags() did not apply that property
on the descriptor. If you change this, the signal on an active-low
GPIO will become the inverse of what it used to be. That's the reason
why this patch cannot be accepted.

> Any driver that mixes gpio_* and gpiod_* API at a whim might not remain
> stable, granted.
> The flags might be applied twice if the driver parses them, is aware of
> them, but calls gpiod_* functions depending on the parsed flags. However,
> this is incorrect driver behavior and drivers that rely on this API duality
> should be fixed.
> Though, to be fair, this distinction might be less than ideally
> documented...
>
>
>> Also, for future contributions could you use "gpiod:of: " in your patch
>> subject to keep the style consistent with existing practices in
>> drivers/gpio?
>
> I didn't see any such usage, but OK.

git log -- drivers/gpio

Should be "gpio: of:" actually.

This is a common convention everywhere in the kernel.

Cheers,
Alex.

>>>
>>> ---
>>>
>>> +#if defined(CONFIG_OF)
>>> +void gpiod_translate_flags(struct gpio_desc *desc, enum of_gpio_flags
>>> *flags)
>>
>> This function translates device tree flags - not just any tag. Its
>> name should reflect that.
>
> I had gpiod_translate_of_flags at first, but that sounded awkward, as
> anything using the of abbreviation. <-- See what I did there?
>
>>> +{
>>> +
>>> +       desc->flags = 0;
>>> +
>>> +       if (*flags & OF_GPIO_ACTIVE_LOW)
>>> +               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
>>> +
>>> +}
>>
>> You could also use this function to set the flags in of_find_gpio(),
>> since the code is the same.
>
> That would be ideal if it turns out this will be necessary in the future.
>
>>> +EXPORT_SYMBOL_GPL(gpiod_translate_flags);
>>
>> I don't think this function should have been exported. It is only to
>> be used in gpiolib-of.o, which will always be linked to gpiolib.o
>> anyway.
>
> I had trouble compiling without the export. I'll try again.
>
>>> +#if defined(CONFIG_OF)
>>> +enum of_gpio_flags;
>>> +void gpiod_translate_flags(struct gpio_desc *desc, enum of_gpio_flags
>>> *flags);
>>> +#endif /* CONFIG_OF */
>>
>> Again, this should not be part of the public API. These declarations
>> should have be moved into the gpiolib.h header.
>
> OK.
>

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Robert ABEL]

On 05 May 2014 15:10, Alexandre Courbot wrote:
> Consider the following code in pwm_bl.c:
>
>      data->enable_gpio = of_get_named_gpio_flags(node, "enable-gpios", 0,
>                              &flags);
>
>      if (gpio_is_valid(data->enable_gpio) && (flags & OF_GPIO_ACTIVE_LOW))
>          data->enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW;
>
>      ....
>
>          if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
>              gpio_set_value(pb->enable_gpio, 0);
>          else
>              gpio_set_value(pb->enable_gpio, 1);
>
> User code of the legacy GPIO API handles the active_low property by
> itself because of_get_named_gpio_flags() did not apply that property
> on the descriptor. If you change this, the signal on an active-low
> GPIO will become the inverse of what it used to be. That's the reason
> why this patch cannot be accepted.
Let me expand your own example for you by expanding all functions:

     data->enable_gpio = of_get_named_gpio_flags(node, "enable-gpios", 0,
                             &flags); /* assume gpio_desc.flags is 
populated here */

     if (gpio_is_valid(data->enable_gpio) && (flags & OF_GPIO_ACTIVE_LOW))
         data->enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW;

     ....

         if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
             gpio_set_value(pb->enable_gpio, 0);
             => __gpio_set_value(pb->enable_gpio, 0);
             => gpiod_set_raw_value(gpio_to_desc(pb->enable_gpio), 0)
             => _gpiod_set_raw_value(gpio_to_desc(pb->enable_gpio), 0) 
/* gpio_desc.flags is ignored here */
         else
             gpio_set_value(pb->enable_gpio, 1);
             => __gpio_set_value(pb->enable_gpio, 1);
             => gpiod_set_raw_value(gpio_to_desc(pb->enable_gpio), 1)
             => _gpiod_set_raw_value(gpio_to_desc(pb->enable_gpio), 1) 
/* gpio_desc.flags is ignored here */

As you see, gpio_to_desc(pb->enable_gpio).flags are ignored along the way.
Thus, this change will *not* break integer usage.
And because gpio_desc are opaque, this will not affect drivers using gpiod,
because they should not have cared about the flags in the first place, 
because
they cannot see them anyway.


> If you do that you will have to update the hundreds of direct and
> indirect users of of_get_named_gpio(), or face their wrath because
> their code will stop working all of a sudden. More detailed
> explanation later in this mail.
See above.
> As explainer earlier, of_get_named_gpiod_flags() should never have
> been public in the first place. This is being addressed by
> https://lkml.org/lkml/2014/5/3/155 .
>
> Now that it is private, if you take a look at how it is used, you will
> see that its current design actually makes sense.
I dispute that, see below.
> There might be some DT users that did not follow that rule and got
> away with it because the old GPIO API is more permissive. These will
> not be able to update to gpiod easily [...].
This is exactly what I meant with breakage.
> That's correct - because the flags are actually applied one level
> higher by the only caller of of_find_gpio(), namely gpiod_get(). The
> reason why this is done there and not earlier is because there are
> other GPIO providers (ACPI, platform data) and we want the flags code
> to be handled in a single place.
Let's consider my use case for a minute here. I use a table to get gpios 
from a dt node:

     /* parse GPIOs */
     for (; *pin_desc; pin_desc++, gpios++) {

         index = of_property_match_string(np, "gpio-names", 
(*pin_desc)->name);
         if (index >= 0)
             *gpios = of_get_gpiod_flags(np, index, NULL);

         [...]

     }

Now I had no way of knowing that I'm not supposed to use 
of_get_gpiod_flags. Because every other driver uses of_get_gpio(_flags), 
I used the similarly named *public* gpiolib functions, because I like 
the concept of opaque gpio descriptors.

Now, you might argue I'm not supposed to do it that way and whatnot all 
you want. Your of_get_gpiod_flags function is *broken*, because it does 
not give me the correct gpio_desc. You can now go ahead and make *all* 
functions except for gpiod_get etc. private, because they're all 
apparently not supposed to be used, although they are public and not a 
word is spent explaining that they will not return a valid gpio_desc 
that has been parsed from DT.

Or you could go the route and actually make all these functions work 
correctly. They all ought to return a correct gpio_desc that has all its 
fields fully populated.

How you can claim this is all good and well and how it quote "doesn't 
seem broken to [you]" is frankly beyond me!!

These functions should all just be correct on their own. And they would 
be, if the gpio_desc fields are correctly populated at the _lowest_ 
level of the API instead of at some arbitrary higher level that might 
change in the future. All these functions should return a correctly 
parsed and valid gpio_desc for the same input. Right now some function 
(like gpiod_get) return a correct gpio_desc, while of_get_gpiod_flags 
returns an incorrect gpio_desc.

It's time to fix this issue. I have already demonstrated how this does 
not affect integer drivers. This will only affect gpiod drivers which 
parse the flags and invert logic levels internally due to incorrect 
gpio_desc returned to them in the first place.
Don't weaken the whole concept of gpio_desc by making half the API 
unusable or private. Don't pretend this state is A-Okay either, please!

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Alexandre Courbot]

On Mon, May 5, 2014 at 11:08 PM, Robert ABEL
<rabel@cit-ec.uni-bielefeld.de> wrote:
> On 05 May 2014 15:10, Alexandre Courbot wrote:
>>
>> Consider the following code in pwm_bl.c:
>>
>>      data->enable_gpio = of_get_named_gpio_flags(node, "enable-gpios", 0,
>>                              &flags);
>>
>>      if (gpio_is_valid(data->enable_gpio) && (flags & OF_GPIO_ACTIVE_LOW))
>>          data->enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW;
>>
>>      ....
>>
>>          if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
>>              gpio_set_value(pb->enable_gpio, 0);
>>          else
>>              gpio_set_value(pb->enable_gpio, 1);
>>
>> User code of the legacy GPIO API handles the active_low property by
>> itself because of_get_named_gpio_flags() did not apply that property
>> on the descriptor. If you change this, the signal on an active-low
>> GPIO will become the inverse of what it used to be. That's the reason
>> why this patch cannot be accepted.
>
> Let me expand your own example for you by expanding all functions:
>
>
>     data->enable_gpio = of_get_named_gpio_flags(node, "enable-gpios", 0,
>                             &flags); /* assume gpio_desc.flags is populated
> here */
>
>
>     if (gpio_is_valid(data->enable_gpio) && (flags & OF_GPIO_ACTIVE_LOW))
>         data->enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW;
>
>     ....
>
>         if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
>             gpio_set_value(pb->enable_gpio, 0);
>             => __gpio_set_value(pb->enable_gpio, 0);
>             => gpiod_set_raw_value(gpio_to_desc(pb->enable_gpio), 0)
>             => _gpiod_set_raw_value(gpio_to_desc(pb->enable_gpio), 0) /*
> gpio_desc.flags is ignored here */
>         else
>             gpio_set_value(pb->enable_gpio, 1);
>             => __gpio_set_value(pb->enable_gpio, 1);
>             => gpiod_set_raw_value(gpio_to_desc(pb->enable_gpio), 1)
>             => _gpiod_set_raw_value(gpio_to_desc(pb->enable_gpio), 1) /*
> gpio_desc.flags is ignored here */
>
> As you see, gpio_to_desc(pb->enable_gpio).flags are ignored along the way.
> Thus, this change will *not* break integer usage.

Ok, you are obviously correct here.

> And because gpio_desc are opaque, this will not affect drivers using gpiod,
> because they should not have cared about the flags in the first place,
> because
> they cannot see them anyway.
>
>
>
>> If you do that you will have to update the hundreds of direct and
>> indirect users of of_get_named_gpio(), or face their wrath because
>> their code will stop working all of a sudden. More detailed
>> explanation later in this mail.
>
> See above.
>
>> As explainer earlier, of_get_named_gpiod_flags() should never have
>> been public in the first place. This is being addressed by
>> https://lkml.org/lkml/2014/5/3/155 .
>>
>> Now that it is private, if you take a look at how it is used, you will
>> see that its current design actually makes sense.
>
> I dispute that, see below.
>>
>> There might be some DT users that did not follow that rule and got
>> away with it because the old GPIO API is more permissive. These will
>> not be able to update to gpiod easily [...].
>
> This is exactly what I meant with breakage.
>
>> That's correct - because the flags are actually applied one level
>> higher by the only caller of of_find_gpio(), namely gpiod_get(). The
>> reason why this is done there and not earlier is because there are
>> other GPIO providers (ACPI, platform data) and we want the flags code
>> to be handled in a single place.
>
> Let's consider my use case for a minute here. I use a table to get gpios
> from a dt node:
>
>     /* parse GPIOs */
>     for (; *pin_desc; pin_desc++, gpios++) {
>
>         index = of_property_match_string(np, "gpio-names",
> (*pin_desc)->name);
>         if (index >= 0)
>             *gpios = of_get_gpiod_flags(np, index, NULL);
>
>         [...]
>
>     }
>
> Now I had no way of knowing that I'm not supposed to use of_get_gpiod_flags.
> Because every other driver uses of_get_gpio(_flags), I used the similarly
> named *public* gpiolib functions, because I like the concept of opaque gpio
> descriptors.

I am not seeing this "gpio-names" property being used anywhere in
mainline to lookup GPIOs, nor do I see usages of of_get_gpiod_flags()
outside of gpiolib, so I had no way of knowing you decided to use it.

Besides I don't have a high-level view on your code, but (at first
sight) couldn't your call to of_get_gpiod_flags() simply be replaced
by gpiod_get()?

> Now, you might argue I'm not supposed to do it that way and whatnot all you
> want. Your of_get_gpiod_flags function is *broken*, because it does not give
> me the correct gpio_desc.

of_get_gpiod_flags() works the same way as of_get_gpio_flags() which
it mirrors and is correct in that respect. Returning a flags parameter
for the caller to apply while having applied these flags already is
what makes no sense to me. Taste and colours...

If you really need a function that will return GPIO descriptor from an
arbitrary DT property (or maybe a version of gpiod_get() working on a
DT node and not a device instance), that's another story and we can
discuss that if it is well motivated. But if we go that way, then
let's craft a proper function for this and not use that horror
inherited from the dark ages.

Also, clearly and calmly explaining your needs will serve you much
better than screaming around and arbitrarily calling things "broken".

> You can now go ahead and make *all* functions
> except for gpiod_get etc. private, because they're all apparently not
> supposed to be used, although they are public and not a word is spent
> explaining that they will not return a valid gpio_desc that has been parsed
> from DT.

Well, that was a mistake that went through the review process. We
wanted to make a perfect GPIO API but unfortunately you were not
around to advise us.

>
> Or you could go the route and actually make all these functions work
> correctly. They all ought to return a correct gpio_desc that has all its
> fields fully populated.

Good news, once of_get_gpiod_flags() becomes private all public gpiod_
functions should return a correct gpio_desc.

>
> How you can claim this is all good and well and how it quote "doesn't seem
> broken to [you]" is frankly beyond me!!

And here we go again...

>
> These functions should all just be correct on their own. And they would be,
> if the gpio_desc fields are correctly populated at the _lowest_ level of the
> API instead of at some arbitrary higher level that might change in the
> future. All these functions should return a correctly parsed and valid
> gpio_desc for the same input. Right now some function (like gpiod_get)
> return a correct gpio_desc, while of_get_gpiod_flags returns an incorrect
> gpio_desc.

Again, the issue was of_get_gpiod_flags() being public in the first place.

As for where to set the descriptor's properties, having it done in
gpiod_get() allows us to handle this in a single place, without having
to rely on lower-level functions to remember doing it themselves. Can
this design be challenged? Sure. It can also go on and on for months
if no solution is clearly superior to the other, and that's likely the
path it will take.

> It's time to fix this issue. I have already demonstrated how this does not
> affect integer drivers. This will only affect gpiod drivers which parse the
> flags and invert logic levels internally due to incorrect gpio_desc returned
> to them in the first place.
> Don't weaken the whole concept of gpio_desc by making half the API unusable
> or private. Don't pretend this state is A-Okay either, please!

To make things clear, here is my stance on the question:

1) of_get_gpiod_flags() being public was a mistake that is being addressed.
2) There are no user of of_get_gpiod_flags() in mainline, therefore
removing it from the public API is perfectly ok. We cannot keep track
of all the external trees to see who is using what, and the kernel has
a long history of much more drastic API changes.
3) If the accepted way of looking up GPIOs does not cover your needs,
please explain clearly how the current gpiod_get() function is
insufficient and we can work something out. But if all it takes to fix
your issue is some small changes in out-of-mainline code, then I will
advocate this rather than adding functions to the gpiod API.

And yeah, the gpiod API is young and we are still dealing with the
(hopefully) last issues of this kind, so please try to be more
tolerant towards your fellow developers.

Alex.

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Alexandre Courbot]

On Tue, May 6, 2014 at 12:14 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> Let's consider my use case for a minute here. I use a table to get gpios
>> from a dt node:
>>
>>     /* parse GPIOs */
>>     for (; *pin_desc; pin_desc++, gpios++) {
>>
>>         index = of_property_match_string(np, "gpio-names",
>> (*pin_desc)->name);
>>         if (index >= 0)
>>             *gpios = of_get_gpiod_flags(np, index, NULL);
>>
>>         [...]
>>
>>     }
>>
>> Now I had no way of knowing that I'm not supposed to use of_get_gpiod_flags.
>> Because every other driver uses of_get_gpio(_flags), I used the similarly
>> named *public* gpiolib functions, because I like the concept of opaque gpio
>> descriptors.
>
> I am not seeing this "gpio-names" property being used anywhere in
> mainline to lookup GPIOs, nor do I see usages of of_get_gpiod_flags()
> outside of gpiolib, so I had no way of knowing you decided to use it.
>
> Besides I don't have a high-level view on your code, but (at first
> sight) couldn't your call to of_get_gpiod_flags() simply be replaced
> by gpiod_get()?

That was meant to read gpiod_get_index(), sorry for the confusion.

Alex.

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Robert ABEL]

On 05 May 2014 17:14, Alexandre Courbot wrote:
> Ok, you are obviously correct here. 
But you won't accept my patch, because you don't want to. Not because 
I'm correct, right? Good, thought so.
> of_get_gpiod_flags() works the same way as of_get_gpio_flags() which
> it mirrors and is correct in that respect. Returning a flags parameter
> for the caller to apply while having applied these flags already is
> what makes no sense to me. Taste and colours...
No, it's definitely _not_ a matter of taste. My version fixes this 
mistake at the earliest possible moment (when the fully parsed gpio is 
returned from the call to the chip), the root cause of the problem. Your 
version applies this fix at on arbitrary point -- too bad for all other 
functions which assume a valid gpio_desc is returned from the lower 
levels. Because it isn't.
But of course, that's just my taste that I like non-broken functions. If 
you like broken functions, you're free to like them.
> If you really need a function that will return GPIO descriptor from an
> arbitrary DT property (or maybe a version of gpiod_get() working on a
> DT node and not a device instance), that's another story and we can
> discuss that if it is well motivated.
Good news :) Such a function was already implemented and available to 
everybody and his dog!
Bad news :( You just made that function private because you wouldn't 
accept my patch to root out this flag application problem.

> Also, clearly and calmly explaining your needs will serve you much
> better than screaming around and arbitrarily calling things "broken".
I'll call broken code *broken*. If you don't like it, that's fine. But I 
won't call broken code *working and in order* just because you get your 
panties in a twist otherwise.
> Well, that was a mistake that went through the review process. We
> wanted to make a perfect GPIO API but unfortunately you were not
> around to advise us.
A real shame indeed.
> Good news, once of_get_gpiod_flags() becomes private all public gpiod_
> functions should return a correct gpio_desc.
Wrong. First of all, every new gpiod_* function that might be added in 
the future will probably need two revisions, because the authors might 
not be aware that of_find_gpio returns an invalid gpio_desc. gpiod_find 
remains broken as well, as it implicitly relies on gpio_desc stored per 
chip to have valid gpio_desc.flags.

> As for where to set the descriptor's properties, having it done in
> gpiod_get() allows us to handle this in a single place, without having
> to rely on lower-level functions to remember doing it themselves. Can
> this design be challenged? Sure. It can also go on and on for months
> if no solution is clearly superior to the other, and that's likely the
> path it will take.
The solution to parse the flags when the gpio_desc is fist used is 
clearly superior, because all other functions depending on that function 
will /just work/.
Fixing things at some arbitrary branch of the caller tree while leaving 
all other branches to fend on their own is poor, dare I say *broken*, 
design.

The single place where things need to be handled at is the lowest common 
denominator, which all other functions can rely on. If higher level 
functions cannot rely on a low-level function, because it lies and does 
not return a valid gpio_desc, then they will in turn return an invalid 
gpio_desc. And so on.

> To make things clear, here is my stance on the question:
>
> 1) of_get_gpiod_flags() being public was a mistake that is being addressed.
No, it was useful and now you broke my and possibly many other 
programmer's code in making it private. Already by name it was supposed 
to be the gpiod_ equivalent to be called when using DT nodes. Thanks 
again for breaking this and possibly getting it into mainline.
Now I need two patches in my private tree: one to fix the flag issue and 
one to re-export that function.
> 2) There are no user of of_get_gpiod_flags() in mainline, therefore
> removing it from the public API is perfectly ok. We cannot keep track
> of all the external trees to see who is using what, and the kernel has
> a long history of much more drastic API changes.
Removing something from a *public* API is never ok post hoc. Never. When 
things get to API level there are certain guarantees that have to be 
met. One would be that functions don't just vanish from revision to 
revision.
> 3) If the accepted way of looking up GPIOs does not cover your needs,
> please explain clearly how the current gpiod_get() function is
> insufficient and we can work something out. But if all it takes to fix
> your issue is some small changes in out-of-mainline code, then I will
> advocate this rather than adding functions to the gpiod API.
How come your way is now the accepted way? I thought the accepted way 
was that gpio_desc were an opaque type whose flags are set in low-level 
code and who can be relied upon to /just work/.
Which comes back to this statement, which seems to be the fundamental 
problem here:
> Returning a flags parameter
> for the caller to apply while having applied these flags already is
> what makes no sense to me.
The flags parameter is not returned to be applied by the user.
First of all, gpio_desc is an opaque type, so the user cannot (and 
indeed must not) apply these flags to gpio_desc.flags.
However, I think with apply you mean this:
The flags are returned so the user can implement his own logic for 
inverting/open draining signals etc.
However, the whole point of the exercise is precisely that no external 
user-logic is required for open drain, open source, low-active, 
high-active etc. However, this will duplicate all the code that's 
already in gpiolib in the user code. This cannot be!

As I see it, the flags are returned as a piece of information to the 
user code. This is for historic reasons, because integer gpios rely upon 
these flags and because gpio_desc is an opaque type, so the flags cannot 
be /easily/ acquired from somewhere else for the current gpio.
However, user code _must not_ change its behavior depending on the 
flags, and neither must gpiolib code. Users must be able to trust that 
the gpio_desc returned to them is valid. They must be able to trust that 
the Right Thing™ happens when they call gpiod_set_value(&my_desc, 1) on 
their gpio: it is asserted -- irrespective of what asserted means in the DT.

Your approach catapults us back into the real dark ages. With your 
approach, we are back to *driver-level* support for active-low, 
open-drain, etc. And that's why the flags *have* to be applied by 
gpiolib, else the whole design is *broken*. Deal with it.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Javier Martinez Canillas]

Hello Robert,

On Tue, May 13, 2014 at 2:19 PM, Robert ABEL
<rabel@cit-ec.uni-bielefeld.de> wrote:
> On 05 May 2014 17:14, Alexandre Courbot wrote:
>>
>> Ok, you are obviously correct here.
>
> But you won't accept my patch, because you don't want to. Not because I'm
> correct, right? Good, thought so.
>
>> of_get_gpiod_flags() works the same way as of_get_gpio_flags() which
>> it mirrors and is correct in that respect. Returning a flags parameter
>> for the caller to apply while having applied these flags already is
>> what makes no sense to me. Taste and colours...
>
> No, it's definitely _not_ a matter of taste. My version fixes this mistake
> at the earliest possible moment (when the fully parsed gpio is returned from
> the call to the chip), the root cause of the problem. Your version applies
> this fix at on arbitrary point -- too bad for all other functions which
> assume a valid gpio_desc is returned from the lower levels. Because it
> isn't.
> But of course, that's just my taste that I like non-broken functions. If you
> like broken functions, you're free to like them.
>
>> If you really need a function that will return GPIO descriptor from an
>> arbitrary DT property (or maybe a version of gpiod_get() working on a
>> DT node and not a device instance), that's another story and we can
>> discuss that if it is well motivated.
>
> Good news :) Such a function was already implemented and available to
> everybody and his dog!
> Bad news :( You just made that function private because you wouldn't accept
> my patch to root out this flag application problem.
>
>
>> Also, clearly and calmly explaining your needs will serve you much
>> better than screaming around and arbitrarily calling things "broken".
>
> I'll call broken code *broken*. If you don't like it, that's fine. But I
> won't call broken code *working and in order* just because you get your
> panties in a twist otherwise.
>
>> Well, that was a mistake that went through the review process. We
>> wanted to make a perfect GPIO API but unfortunately you were not
>> around to advise us.
>
> A real shame indeed.
>
>> Good news, once of_get_gpiod_flags() becomes private all public gpiod_
>> functions should return a correct gpio_desc.
>
> Wrong. First of all, every new gpiod_* function that might be added in the
> future will probably need two revisions, because the authors might not be
> aware that of_find_gpio returns an invalid gpio_desc. gpiod_find remains
> broken as well, as it implicitly relies on gpio_desc stored per chip to have
> valid gpio_desc.flags.
>
>
>> As for where to set the descriptor's properties, having it done in
>> gpiod_get() allows us to handle this in a single place, without having
>> to rely on lower-level functions to remember doing it themselves. Can
>> this design be challenged? Sure. It can also go on and on for months
>> if no solution is clearly superior to the other, and that's likely the
>> path it will take.
>
> The solution to parse the flags when the gpio_desc is fist used is clearly
> superior, because all other functions depending on that function will /just
> work/.
> Fixing things at some arbitrary branch of the caller tree while leaving all
> other branches to fend on their own is poor, dare I say *broken*, design.
>

Well is not an arbitrary place but in the function that GPIO consumers
are supposed to use to get a properly configured GPIO.

On the other hand it seems that making of_get_named_gpiod_flags()
private broke a lot of drivers according to Linus although I wonder
why since I see no user of of_get_gpiod_flags() in mainline.

So it may be possible that gpiod_get_index() is not the only function
called by consumers. But if we return a proper filled gpio_desc with
flags then what's the point of the flags parameter passed to
of_get_named_gpiod_flags()?

In that case we should just remove that parameter and fill the flags
passed to of_get_named_gpio_flags() from the returned gpio_desc by
right of_get_named_gpiod_flags().

> The single place where things need to be handled at is the lowest common
> denominator, which all other functions can rely on. If higher level
> functions cannot rely on a low-level function, because it lies and does not
> return a valid gpio_desc, then they will in turn return an invalid
> gpio_desc. And so on.
>
>
>> To make things clear, here is my stance on the question:
>>
>> 1) of_get_gpiod_flags() being public was a mistake that is being
>> addressed.
>
> No, it was useful and now you broke my and possibly many other programmer's
> code in making it private. Already by name it was supposed to be the gpiod_
> equivalent to be called when using DT nodes. Thanks again for breaking this
> and possibly getting it into mainline.
> Now I need two patches in my private tree: one to fix the flag issue and one
> to re-export that function.
>

That's a disadvantage of having code out of the mainline kernel and
one of the reasons why is much better to work upstream.

>> 2) There are no user of of_get_gpiod_flags() in mainline, therefore
>> removing it from the public API is perfectly ok. We cannot keep track
>> of all the external trees to see who is using what, and the kernel has
>> a long history of much more drastic API changes.
>
> Removing something from a *public* API is never ok post hoc. Never. When
> things get to API level there are certain guarantees that have to be met.
> One would be that functions don't just vanish from revision to revision.
>

That's certainly not true in the kernel, in fact we don't have an
stable internal API, only the external ABI with user-space i stable.
This allows to keep improving the kernel and to correct previous
mistakes (like the one you are pointing out on of_get_gpiod_flags).
Please read Documentation/stable_api_nonsense.txt in the kernel source
directory.

In fact if there are no mainline users of an API this will almost
certainly be removed.

>> 3) If the accepted way of looking up GPIOs does not cover your needs,
>> please explain clearly how the current gpiod_get() function is
>> insufficient and we can work something out. But if all it takes to fix
>> your issue is some small changes in out-of-mainline code, then I will
>> advocate this rather than adding functions to the gpiod API.
>
> How come your way is now the accepted way? I thought the accepted way was
> that gpio_desc were an opaque type whose flags are set in low-level code and
> who can be relied upon to /just work/.
> Which comes back to this statement, which seems to be the fundamental
> problem here:
>
>> Returning a flags parameter
>> for the caller to apply while having applied these flags already is
>> what makes no sense to me.
>
> The flags parameter is not returned to be applied by the user.
> First of all, gpio_desc is an opaque type, so the user cannot (and indeed
> must not) apply these flags to gpio_desc.flags.
> However, I think with apply you mean this:
> The flags are returned so the user can implement his own logic for
> inverting/open draining signals etc.
> However, the whole point of the exercise is precisely that no external
> user-logic is required for open drain, open source, low-active, high-active
> etc. However, this will duplicate all the code that's already in gpiolib in
> the user code. This cannot be!
>
> As I see it, the flags are returned as a piece of information to the user
> code. This is for historic reasons, because integer gpios rely upon these
> flags and because gpio_desc is an opaque type, so the flags cannot be
> /easily/ acquired from somewhere else for the current gpio.
> However, user code _must not_ change its behavior depending on the flags,
> and neither must gpiolib code. Users must be able to trust that the
> gpio_desc returned to them is valid. They must be able to trust that the
> Right Thing™ happens when they call gpiod_set_value(&my_desc, 1) on their
> gpio: it is asserted -- irrespective of what asserted means in the DT.
>
> Your approach catapults us back into the real dark ages. With your approach,
> we are back to *driver-level* support for active-low, open-drain, etc. And
> that's why the flags *have* to be applied by gpiolib, else the whole design
> is *broken*. Deal with it.
>

Please try to keep you tone polite, we all try to make things better
but using an aggressive language does not help in any way.

> Regards,
>
> Robert
>

Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Robert Abel]

Hi Javier, Hi Alexandre,

I'm going to mix and match and reorder your two e-mails. Sorry about the 
big mid-section.

On 13.05.2014 15:38, Javier Martinez Canillas wrote:
> Well is not an arbitrary place but in the function that GPIO consumers
> are supposed to use to get a properly configured GPIO.
Well, that's not very obvious. I just looked for the gpiod equivalent of 
the integer gpio function and wound up with of_get_named_gpiod_flags. 
This seemed the logical place to start for me. Too bad it returns 
invalid gpio_desc.

On 13.05.2014 15:38, Javier Martinez Canillas wrote:
> In that case we should just remove that parameter and fill the flags
> passed to of_get_named_gpio_flags() from the returned gpio_desc by
> right of_get_named_gpiod_flags().
On 13.05.2014 16:24, Alexandre Courbot wrote:
> I don't want this function to be public because it leaks a flags
> parameter that the outside does not need to see.
On 13.05.2014 16:24, Alexandre Courbot wrote:
> Which is why we don't leak a descriptor's flags to users of
> the gpiod API.

But those are enum of_gpio_flags -- not the gpio_desc.flags. So there is 
no "leak" so to speak and user code cannot mess with the opaque 
gpio_desc that way.
 From what I see, the flags argument is just there for the legacy 
integer gpio lib, i.e. of_get_named_gpio_flags calls 
of_get_named_gpiod_flags. That's their only point as far as I'm concerned.
IMHO, the argument should be kept for now and be removed after all 
mainline drivers transitioned to gpiod.

On 13.05.2014 16:24, Alexandre Courbot wrote:
> I feel quite strongly that this whole
> issue could easily be fixed on your private tree, but you are not very
> verbose about your use case and it doesn't seem like this is a
> solution you are looking for here anyway.
Oh believe you me, it is fixed in my tree -- using my patch. However, I 
felt the general need to bring this to mainline attention, because it 
breaks gpiod drivers who rely on DT for polarity.
I literally posted my code for getting gpios in one of the other emails, 
so I'm not sure what about my use case is so unclear:
Acquire gpios from a DT node to use in my driver.

On 13.05.2014 15:38, Javier Martinez Canillas wrote:
> That's a disadvantage of having code out of the mainline kernel and
> one of the reasons why is much better to work upstream.
Which is why I sent my patch upstream, because I thought other people 
might very well run into the same trouble as I did.

On 13.05.2014 15:38, Javier Martinez Canillas wrote:
>> Removing something from a *public* API is never ok post hoc.
>
> That's certainly not true in the kernel, in fact we don't have an
> stable internal API, only the external ABI with user-space i stable.
> This allows to keep improving the kernel and to correct previous
> mistakes (like the one you are pointing out on of_get_gpiod_flags).
> Please read Documentation/stable_api_nonsense.txt in the kernel source
> directory.
On 13.05.2014 16:24, Alexandre Courbot wrote:

> http://www.mjmwired.net/kernel/Documentation/stable_api_nonsense.txt
Well, I should have said /removing it on a whim/, but I concede I did not know about this policy (nor necessarily agree completely with it).

On 13.05.2014 16:24, Alexandre Courbot wrote:
> Blah blah, sticks and stones...
You might not care about function-level correctness, but I do. And I 
hold it's not a matter of taste whether functions return valid or 
invalid results. Use correct functions as building blocks for 
higher-level functions and you're all set. Use functions that return 
correct results four out of five times and higher-level functions will 
break every so often.

On 13.05.2014 16:24, Alexandre Courbot wrote:
> I explained to you
> that we could consider another function that does not return that
> flag, but you decided to ignore it.
I decided not to have a solution that reinvents the wheel. The current 
function is perfectly fine once the flags are fixed. Why would there be 
need for *of_get_named_gpiod_flags* as well as 
*of_get_named_gpiod_flags_actually_return_proper_gpio_desc*? There 
isn't. Let's fix of_get_named_gpiod_flags and move on.

On 13.05.2014 16:24, Alexandre Courbot wrote:

> Do you understand that your solution would require to handle the flags
> in gpiod_find(), of_find_gpio() and acpi_find_gpio(), while the
> solution that handles things in a single place (gpiod_get_index()) is
> the one that is currently implemented?
No, I'm actually not aware of this. I proposed a fix in 
of_get_named_gpiod_flags.
- of_find_gpio calls of_get_named_gpiod_flags. If the latter returns a 
valid gpio_desc, so will of_find_gpio. The point of my patch - it 
handles the DT case in a single place. All other functions relying on 
of_get_named_gpiod_flags will return valid gpio_desc by design. Without 
specific implementation-dependent knowledge.
=> of_find_gpio works; does not interfere with integer gpio API.

- acpi_get_gpiod_by_index does not handle flag parsing itself.
It relies upon acpi_get_gpiod, which in turn implicitly relies upon the 
gpio_desc.flags being correctly set in the gpiochip descriptor list. 
acpi_find_gpio currently translates the flags from an acpi_gpio_info 
structure after the fact (coming from the gpiochip) anyway. So this does 
not seem like a big concern. Translate to gpio_desc.flags there right 
away, you even save a level of translation in the end. The implicit 
reliance on the gpiochip desc table might be a point of contention for 
another day.
=> no interference by of_get_named_gpiod_flags; a level of translation 
anyway.

- gpiod_find has the same reliance on the gpiochip descriptor table as 
acpi_get_gpiod_by_index, but does no translation of its own. It relies 
upon valid descriptors being in the gpiochip descriptor table
=> no interference by of_get_named_gpiod_flags; total reliance on 
gpiochip platform(?) data as you pointed out.

Needless to say, the gpio chips don't usually set up these descriptor 
tables, so individual descriptors are likely incorrect before first use 
anyway. That's why the acpi calls add another level of translation using 
the acpi_gpio_info structure.

I expect gpiod_find, acpi_get_gpiod_by_index, as well as of_find_gpio to 
return a valid gpio_desc, because they promise to do just that by merit 
of their function signature.
I knew that of_find_gpio was breaking this promise. That's why I went to 
the lowest common denominator function and fixed it.

I suspect gpiod_find lies about the gpio_desc it returns (because it 
does not translation anywhere). But I'm currently not in a position to 
test that hypothesis.

I propose acpi_get_gpiod_by_index should not apply flags to gpio_desc 
and should do no translation of its own:
Currently acpi uses ACPI_* flags, which it -- as a crutch -- translates 
to enum gpio_lookup_flags which in turn gpiolib -- knowing the flags 
were not applied -- translates enum gpio_lookup_flags to gpiod_flags in 
gpiod_get.

Now apply function-level correctness:
acpi_get_gpiod_by_index promises to return a valid gpio_desc. => _Make 
it so_. Take out a level of re-translation and you end up with functions 
that perform correctly on their own.
In general, translation of flags has to be applied anyways, because 
gpio-supplier-specific flags are there to stay. However, translation 
logic should be in gpio-supplier code, not in code built atop 
gpio-supplier functions that /happens to undocumentedly(!) know/ that 
the functions lie and return invalid results.

To recap:

On 13.05.2014 16:24, Alexandre Courbot wrote:

> Do you understand that your solution would require to handle the flags
> in gpiod_find(), of_find_gpio() and acpi_find_gpio(), while the
> solution that handles things in a single place (gpiod_get_index()) is
> the one that is currently implemented?
I understand that the flags are already being handled/translated all 
over the place. I propose:
- to move gpio-supplier-specific logic into gpio-supplier code, where it 
belongs,
- have all gpio-suppliers adhere to the gpiod interface, i.e. return 
valid gpio_desc with appropriate flags set,
- remove translation of enum gpio_lookup_flags to gpio_desc.flags in 
gpiolib, and thereby
- decouple gpio-supplier code from gpiolib, especially mitigate 
higher-level functions doing jobs which their lower-level callees 
neglected to do

On 13.05.2014 16:24, Alexandre Courbot wrote:

> Why would authors of gpiod_ functions use functions of the old integer API?
Because gpiod_get_index uses it:
> struct gpio_desc *__must_check gpiod_get_index(...)
> {
>     ...
>
>     /* Using device tree? */
>     if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) {
>         ...
>         desc = of_find_gpio(dev, con_id, idx, &flags);
>     }...

On 13.05.2014 16:24, Alexandre Courbot wrote:

> gpiod_find()'s sole purpose is to be called by gpiod_get_index() to
> lookup for platform-defined GPIOs. [...]
> Where in its name or prototype is it hinted that it
> should set the flags itself?

Well, right here:

> static struct gpio_desc *gpiod_find(...) ^^^^^^^^^^^^^^^^
If I cannot rely upon gpiod_find to return a valid gpio_desc, then I have to /know/ that. This implies that all programmers have to be familiar with what you just said. Which ultimately implies that all users have to work around gpiod_find not returning a valid gpio_desc. Let's fix that, have function-level correctness and less headache while improving gpiolib and gpiolib-acpi.

Regards,

Robert

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Alexandre Courbot]

I promised to myself that I would not contribute to this thread
anymore, it seems like we are becoming more civil, let's hope it
lasts...

On Wed, May 14, 2014 at 9:44 AM, Robert Abel
<rabel@cit-ec.uni-bielefeld.de> wrote:
> Hi Javier, Hi Alexandre,
>
> I'm going to mix and match and reorder your two e-mails. Sorry about the big
> mid-section.
>
>
> On 13.05.2014 15:38, Javier Martinez Canillas wrote:
>>
>> Well is not an arbitrary place but in the function that GPIO consumers
>> are supposed to use to get a properly configured GPIO.
>
> Well, that's not very obvious. I just looked for the gpiod equivalent of the
> integer gpio function and wound up with of_get_named_gpiod_flags. This
> seemed the logical place to start for me. Too bad it returns invalid
> gpio_desc.
>
>
> On 13.05.2014 15:38, Javier Martinez Canillas wrote:
>>
>> In that case we should just remove that parameter and fill the flags
>> passed to of_get_named_gpio_flags() from the returned gpio_desc by
>> right of_get_named_gpiod_flags().
>
> On 13.05.2014 16:24, Alexandre Courbot wrote:
>>
>> I don't want this function to be public because it leaks a flags
>> parameter that the outside does not need to see.
>
> On 13.05.2014 16:24, Alexandre Courbot wrote:
>>
>> Which is why we don't leak a descriptor's flags to users of
>> the gpiod API.
>
>
> But those are enum of_gpio_flags -- not the gpio_desc.flags. So there is no
> "leak" so to speak and user code cannot mess with the opaque gpio_desc that
> way.
> From what I see, the flags argument is just there for the legacy integer
> gpio lib, i.e. of_get_named_gpio_flags calls of_get_named_gpiod_flags.
> That's their only point as far as I'm concerned.
> IMHO, the argument should be kept for now and be removed after all mainline
> drivers transitioned to gpiod.

While I would love to see all mainline drivers transitioned to gpiod,
I am not seeing it happening anytime soon. In any case, gpiod and the
legacy integer API are two different APIs and there is no reason to
expect "mirror" functions here.

Here are a couple more reasons why of_get_named_gpiod_flags() should
*not* be public:
- We currently have 3 different GPIO providers: DT, platform and ACPI.
of_get_named_gpiod_flags() only treats the DT case. Are you going to
make equivalent functions for ACPI and platform public as well? When
we add more GPIO providers, are you willing to handle them as well?
- Contrary to the old integer API, GPIO consumers that use the gpiod
interface should not have to worry about who their GPIO provider is.
They just call gpiod_get*(), and who actually provides the GPIO is
handled by gpiolib. A public of_get_named_gpiod_flags() allows
consumer to circumvent that policy.

>
>
> On 13.05.2014 16:24, Alexandre Courbot wrote:
>>
>> I feel quite strongly that this whole
>> issue could easily be fixed on your private tree, but you are not very
>> verbose about your use case and it doesn't seem like this is a
>> solution you are looking for here anyway.
>
> Oh believe you me, it is fixed in my tree -- using my patch. However, I felt
> the general need to bring this to mainline attention, because it breaks
> gpiod drivers who rely on DT for polarity.

Not those that use the indented function, gpiod_get*(). Which is
everybody in mainline so far.

> I literally posted my code for getting gpios in one of the other emails, so
> I'm not sure what about my use case is so unclear:
> Acquire gpios from a DT node to use in my driver.

I asked you why your problem could not be solved by using gpiod_get*()
but you did not reply. Having a link to your private tree would also
be helpful to see the bigger picture.

For some reason you seem to believe that your problem can *only* be
solved by a public of_get_named_gpiod_flags(). Unless you get rid of
that state of mind there is no way we are going forward.

>
>
> On 13.05.2014 15:38, Javier Martinez Canillas wrote:
>>
>> That's a disadvantage of having code out of the mainline kernel and
>> one of the reasons why is much better to work upstream.
>
> Which is why I sent my patch upstream, because I thought other people might
> very well run into the same trouble as I did.
>
>
> On 13.05.2014 15:38, Javier Martinez Canillas wrote:
>>>
>>> Removing something from a *public* API is never ok post hoc.
>>
>>
>> That's certainly not true in the kernel, in fact we don't have an
>> stable internal API, only the external ABI with user-space i stable.
>> This allows to keep improving the kernel and to correct previous
>> mistakes (like the one you are pointing out on of_get_gpiod_flags).
>> Please read Documentation/stable_api_nonsense.txt in the kernel source
>> directory.
>
> On 13.05.2014 16:24, Alexandre Courbot wrote:
>
>> http://www.mjmwired.net/kernel/Documentation/stable_api_nonsense.txt
>
> Well, I should have said /removing it on a whim/, but I concede I did not
> know about this policy (nor necessarily agree completely with it).

A whim now, seriously? There are plenty of good reasons for now having
this function available to GPIO consumers and I listed them already.

>
>
> On 13.05.2014 16:24, Alexandre Courbot wrote:
>>
>> Blah blah, sticks and stones...
>
> You might not care about function-level correctness, but I do.

You should really stop making clueless judgemental assertions about
what people care or do not care about before they get tired of you for
good.

> And I hold
> it's not a matter of taste whether functions return valid or invalid
> results. Use correct functions as building blocks for higher-level functions
> and you're all set. Use functions that return correct results four out of
> five times and higher-level functions will break every so often.
>
>
> On 13.05.2014 16:24, Alexandre Courbot wrote:
>>
>> I explained to you
>> that we could consider another function that does not return that
>> flag, but you decided to ignore it.
>
> I decided not to have a solution that reinvents the wheel. The current
> function is perfectly fine once the flags are fixed. Why would there be need
> for *of_get_named_gpiod_flags* as well as
> *of_get_named_gpiod_flags_actually_return_proper_gpio_desc*? There isn't.
> Let's fix of_get_named_gpiod_flags and move on.

Again, I see no reason why this function should be visible. I see no
reason why it should return a useless flag to its users. And I see no
reason why gpiod_get_*() cannot fit your needs.

Stop focusing on this function in particular, as it will probably be
gone soon anyway. Focus on your problem and what is the most elegant
way to address it.

The second part of your mail is more interesting.

>
>
> On 13.05.2014 16:24, Alexandre Courbot wrote:
>
>> Do you understand that your solution would require to handle the flags
>> in gpiod_find(), of_find_gpio() and acpi_find_gpio(), while the
>> solution that handles things in a single place (gpiod_get_index()) is
>> the one that is currently implemented?
>
> No, I'm actually not aware of this. I proposed a fix in
> of_get_named_gpiod_flags.
> - of_find_gpio calls of_get_named_gpiod_flags. If the latter returns a valid
> gpio_desc, so will of_find_gpio. The point of my patch - it handles the DT
> case in a single place. All other functions relying on
> of_get_named_gpiod_flags will return valid gpio_desc by design. Without
> specific implementation-dependent knowledge.
> => of_find_gpio works; does not interfere with integer gpio API.
>
> - acpi_get_gpiod_by_index does not handle flag parsing itself.
> It relies upon acpi_get_gpiod, which in turn implicitly relies upon the
> gpio_desc.flags being correctly set in the gpiochip descriptor list.
> acpi_find_gpio currently translates the flags from an acpi_gpio_info
> structure after the fact (coming from the gpiochip) anyway. So this does not
> seem like a big concern. Translate to gpio_desc.flags there right away, you
> even save a level of translation in the end. The implicit reliance on the
> gpiochip desc table might be a point of contention for another day.
> => no interference by of_get_named_gpiod_flags; a level of translation
> anyway.
>
> - gpiod_find has the same reliance on the gpiochip descriptor table as
> acpi_get_gpiod_by_index, but does no translation of its own. It relies upon
> valid descriptors being in the gpiochip descriptor table
> => no interference by of_get_named_gpiod_flags; total reliance on gpiochip
> platform(?) data as you pointed out.
>
> Needless to say, the gpio chips don't usually set up these descriptor
> tables, so individual descriptors are likely incorrect before first use
> anyway. That's why the acpi calls add another level of translation using the
> acpi_gpio_info structure.
>
> I expect gpiod_find, acpi_get_gpiod_by_index, as well as of_find_gpio to
> return a valid gpio_desc, because they promise to do just that by merit of
> their function signature.
> I knew that of_find_gpio was breaking this promise. That's why I went to the
> lowest common denominator function and fixed it.
>
> I suspect gpiod_find lies about the gpio_desc it returns (because it does
> not translation anywhere). But I'm currently not in a position to test that
> hypothesis.
>
> I propose acpi_get_gpiod_by_index should not apply flags to gpio_desc and
> should do no translation of its own:
> Currently acpi uses ACPI_* flags, which it -- as a crutch -- translates to
> enum gpio_lookup_flags which in turn gpiolib -- knowing the flags were not
> applied -- translates enum gpio_lookup_flags to gpiod_flags in gpiod_get.
>
> Now apply function-level correctness:
> acpi_get_gpiod_by_index promises to return a valid gpio_desc. => _Make it
> so_. Take out a level of re-translation and you end up with functions that
> perform correctly on their own.
> In general, translation of flags has to be applied anyways, because
> gpio-supplier-specific flags are there to stay. However, translation logic
> should be in gpio-supplier code, not in code built atop gpio-supplier
> functions that /happens to undocumentedly(!) know/ that the functions lie
> and return invalid results.
>
> To recap:
>
>
> On 13.05.2014 16:24, Alexandre Courbot wrote:
>
>> Do you understand that your solution would require to handle the flags
>> in gpiod_find(), of_find_gpio() and acpi_find_gpio(), while the
>> solution that handles things in a single place (gpiod_get_index()) is
>> the one that is currently implemented?
>
> I understand that the flags are already being handled/translated all over
> the place. I propose:
> - to move gpio-supplier-specific logic into gpio-supplier code, where it
> belongs,
> - have all gpio-suppliers adhere to the gpiod interface, i.e. return valid
> gpio_desc with appropriate flags set,
> - remove translation of enum gpio_lookup_flags to gpio_desc.flags in
> gpiolib, and thereby
> - decouple gpio-supplier code from gpiolib, especially mitigate higher-level
> functions doing jobs which their lower-level callees neglected to do

I'm sure it's clear already, but of_find_gpio(), acpi_find_gpio() and
gpiod_find() are gpiolib-private functions, solely used by
gpiod_get_index(). As their name states, they "find" a GPIO using a
given provider. You seem to absolutely want them to set the flags up
as well for "correctness", but well, that's just not what is asked of
them. Any discussion on the topic is philosophical, no matter how
right you believe you are on this, and how many instances of "stupid"
you employ in your argument.

Now it is true that the functions (with the exception of gpiod_find())
already process the flags into some intermediate form that is returned
to gpiod_get_index(), which then processes it again into the final
flags of the descriptor. We could certainly save a step by setting the
descriptor flags directly in of_find_gpio(), acpi_find_gpio() and
gpiod_find() (or even lower) and by getting rid of the global code in
gpiod_get_index(). While we are at it we would remove the now useless
output flags argument, and rename them all to
(acpi|of|platform)_gpiod_get_index() for consistency. I don't know
whether it is worth doing so, but if it can make the code simpler I
would probably welcome such a patch.

Doing the same in of_get_named_gpiod_flags() is more delicate as it is
also used by the integer API. It is true that the active_low property
does not change the behavior of gpio_* functions. But we may decide in
the future to allow open_drain/open_source to be specified in the DT
(as it is for the platform provider currently). Also we can imagine a
scenario where an integer GPIO is obtained by of_find_gpio() and
turned into a descriptor by gpio_to_desc() - here whether the flags
have been applied or not during lookup will matter. In short I don't
want the legacy API to be affected by this, even it is seems like it
won't at the moment.

In any case it does not change anything to the issue of whether
of_get_named_gpiod_flags() should be public or not.

>
>
> On 13.05.2014 16:24, Alexandre Courbot wrote:
>
>> Why would authors of gpiod_ functions use functions of the old integer
>> API?
>
> Because gpiod_get_index uses it:
>>
>> struct gpio_desc *__must_check gpiod_get_index(...)
>> {
>>     ...
>>
>>     /* Using device tree? */
>>     if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) {
>>         ...
>>         desc = of_find_gpio(dev, con_id, idx, &flags);
>>     }...

of_find_gpio() is not part of the integer API - it is private to
gpiolib.c is called from gpiod_get_index() only.

>
>
> On 13.05.2014 16:24, Alexandre Courbot wrote:
>
>> gpiod_find()'s sole purpose is to be called by gpiod_get_index() to
>> lookup for platform-defined GPIOs. [...]
>>
>> Where in its name or prototype is it hinted that it
>> should set the flags itself?
>
>
> Well, right here:
>
>> static struct gpio_desc *gpiod_find(...) ^^^^^^^^^^^^^^^^
>
> If I cannot rely upon gpiod_find to return a valid gpio_desc, then I have to
> /know/ that. This implies that all programmers have to be familiar with what
> you just said. Which ultimately implies that all users have to work around
> gpiod_find not returning a valid gpio_desc. Let's fix that, have
> function-level correctness and less headache while improving gpiolib and
> gpiolib-acpi.

As a GPIO user you don't need to (and as a matter of fact, cannot)
call gpiod_find() since it is gpiolib-private.

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Alexandre Courbot]

On Tue, May 13, 2014 at 9:19 PM, Robert ABEL
<rabel@cit-ec.uni-bielefeld.de> wrote:
> On 05 May 2014 17:14, Alexandre Courbot wrote:
>>
>> Ok, you are obviously correct here.
>
> But you won't accept my patch, because you don't want to. Not because I'm
> correct, right? Good, thought so.

This example has nothing to do with the validity of having this or
that function public.

Besides I have no authority to accept your patch - that's Linus' job.
If he likes your patch nothing that I say will prevent him from
merging it.

>
>> of_get_gpiod_flags() works the same way as of_get_gpio_flags() which
>> it mirrors and is correct in that respect. Returning a flags parameter
>> for the caller to apply while having applied these flags already is
>> what makes no sense to me. Taste and colours...
>
> No, it's definitely _not_ a matter of taste. My version fixes this mistake
> at the earliest possible moment (when the fully parsed gpio is returned from
> the call to the chip), the root cause of the problem. Your version applies
> this fix at on arbitrary point -- too bad for all other functions which
> assume a valid gpio_desc is returned from the lower levels. Because it
> isn't.
> But of course, that's just my taste that I like non-broken functions. If you
> like broken functions, you're free to like them.

Blah blah, sticks and stones...

>
>> If you really need a function that will return GPIO descriptor from an
>> arbitrary DT property (or maybe a version of gpiod_get() working on a
>> DT node and not a device instance), that's another story and we can
>> discuss that if it is well motivated.
>
> Good news :) Such a function was already implemented and available to
> everybody and his dog!
> Bad news :( You just made that function private because you wouldn't accept
> my patch to root out this flag application problem.

I don't want this function to be public because it leaks a flags
parameter that the outside does not need to see. I explained to you
that we could consider another function that does not return that
flag, but you decided to ignore it.

Not that we should even feel obliged to consider this solution if
mainline does not need it - your private tree is, well, your tree.
"Deal with it".

>
>
>> Also, clearly and calmly explaining your needs will serve you much
>> better than screaming around and arbitrarily calling things "broken".
>
> I'll call broken code *broken*. If you don't like it, that's fine. But I
> won't call broken code *working and in order* just because you get your
> panties in a twist otherwise.
>
>> Well, that was a mistake that went through the review process. We
>> wanted to make a perfect GPIO API but unfortunately you were not
>> around to advise us.
>
> A real shame indeed.
>
>> Good news, once of_get_gpiod_flags() becomes private all public gpiod_
>> functions should return a correct gpio_desc.
>
> Wrong. First of all, every new gpiod_* function that might be added in the
> future will probably need two revisions, because the authors might not be
> aware that of_find_gpio returns an invalid gpio_desc.

Why would authors of gpiod_ functions use functions of the old integer API?

> gpiod_find remains
> broken as well, as it implicitly relies on gpio_desc stored per chip to have
> valid gpio_desc.flags.

gpiod_find()'s sole purpose is to be called by gpiod_get_index() to
lookup for platform-defined GPIOs. The flags are set in the platform
lookup table and returned to gpiod_find_index() which treats the flags
in one place for descriptors returned by DT, ACPI and platform lookup.

That's the sole use of gpiod_find(). It is not exported. It is static
to gpiolib.c. Where in its name or prototype is it hinted that it
should set the flags itself?

>
>
>> As for where to set the descriptor's properties, having it done in
>> gpiod_get() allows us to handle this in a single place, without having
>> to rely on lower-level functions to remember doing it themselves. Can
>> this design be challenged? Sure. It can also go on and on for months
>> if no solution is clearly superior to the other, and that's likely the
>> path it will take.
>
> The solution to parse the flags when the gpio_desc is fist used is clearly
> superior, because all other functions depending on that function will /just
> work/.
> Fixing things at some arbitrary branch of the caller tree while leaving all
> other branches to fend on their own is poor, dare I say *broken*, design.
>
> The single place where things need to be handled at is the lowest common
> denominator, which all other functions can rely on. If higher level
> functions cannot rely on a low-level function, because it lies and does not
> return a valid gpio_desc, then they will in turn return an invalid
> gpio_desc. And so on.

Do you understand that your solution would require to handle the flags
in gpiod_find(), of_find_gpio() and acpi_find_gpio(), while the
solution that handles things in a single place (gpiod_get_index()) is
the one that is currently implemented?

>
>
>> To make things clear, here is my stance on the question:
>>
>> 1) of_get_gpiod_flags() being public was a mistake that is being
>> addressed.
>
> No, it was useful and now you broke my and possibly many other programmer's
> code in making it private. Already by name it was supposed to be the gpiod_
> equivalent to be called when using DT nodes. Thanks again for breaking this
> and possibly getting it into mainline.
> Now I need two patches in my private tree: one to fix the flag issue and one
> to re-export that function.
>
>> 2) There are no user of of_get_gpiod_flags() in mainline, therefore
>> removing it from the public API is perfectly ok. We cannot keep track
>> of all the external trees to see who is using what, and the kernel has
>> a long history of much more drastic API changes.
>
> Removing something from a *public* API is never ok post hoc. Never. When
> things get to API level there are certain guarantees that have to be met.
> One would be that functions don't just vanish from revision to revision.

http://www.mjmwired.net/kernel/Documentation/stable_api_nonsense.txt

>
>> 3) If the accepted way of looking up GPIOs does not cover your needs,
>> please explain clearly how the current gpiod_get() function is
>> insufficient and we can work something out. But if all it takes to fix
>> your issue is some small changes in out-of-mainline code, then I will
>> advocate this rather than adding functions to the gpiod API.
>
> How come your way is now the accepted way? I thought the accepted way was
> that gpio_desc were an opaque type whose flags are set in low-level code and
> who can be relied upon to /just work/.

Exactly. Which is why we don't leak a descriptor's flags to users of
the gpiod API.

> Which comes back to this statement, which seems to be the fundamental
> problem here:
>
>> Returning a flags parameter
>> for the caller to apply while having applied these flags already is
>> what makes no sense to me.
>
> The flags parameter is not returned to be applied by the user.
> First of all, gpio_desc is an opaque type, so the user cannot (and indeed
> must not) apply these flags to gpio_desc.flags.
> However, I think with apply you mean this:
> The flags are returned so the user can implement his own logic for
> inverting/open draining signals etc.
> However, the whole point of the exercise is precisely that no external
> user-logic is required for open drain, open source, low-active, high-active
> etc. However, this will duplicate all the code that's already in gpiolib in
> the user code. This cannot be!
>
> As I see it, the flags are returned as a piece of information to the user
> code. This is for historic reasons, because integer gpios rely upon these
> flags and because gpio_desc is an opaque type, so the flags cannot be
> /easily/ acquired from somewhere else for the current gpio.
> However, user code _must not_ change its behavior depending on the flags,
> and neither must gpiolib code. Users must be able to trust that the
> gpio_desc returned to them is valid. They must be able to trust that the
> Right Thing™ happens when they call gpiod_set_value(&my_desc, 1) on their
> gpio: it is asserted -- irrespective of what asserted means in the DT.
>
> Your approach catapults us back into the real dark ages. With your approach,
> we are back to *driver-level* support for active-low, open-drain, etc. And
> that's why the flags *have* to be applied by gpiolib, else the whole design
> is *broken*. Deal with it.

Like I said Linus is the one who accepts or rejects patches
ultimately. I am trying to help said patches getting in shape but
apparently you are not very appreciative of the initiative nor are you
willing to consider my attempts to reach a common ground through
another, less leaking function. I feel quite strongly that this whole
issue could easily be fixed on your private tree, but you are not very
verbose about your use case and it doesn't seem like this is a
solution you are looking for here anyway.

Finally, since the tone and vocabulary of your messages make it clear
that to your eyes I am not in a position to help you with this, I will
let you settle this issue with Linus.

Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Linus Walleij]

On Tue, May 13, 2014 at 4:24 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, May 13, 2014 at 9:19 PM, Robert ABEL
> <rabel@cit-ec.uni-bielefeld.de> wrote:

>> But you won't accept my patch, because you don't want to. Not because I'm
>> correct, right? Good, thought so.
>
> This example has nothing to do with the validity of having this or
> that function public.
>
> Besides I have no authority to accept your patch - that's Linus' job.
> If he likes your patch nothing that I say will prevent him from
> merging it.

I will not merge a patch which the GPIO co-maintainer has doubts
about, as happens to be the case.

Right now I'm more thinking about how we get your patch removing
the function into shape so we can apply it and resolve the whole issue,
as I trust you to be doing the right thing around the gpiod_* functions
for sure.

Yours,
Linus Walleij

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Linus Walleij]

On Tue, Apr 29, 2014 at 2:38 PM, Robert ABEL
<rabel@cit-ec.uni-bielefeld.de> wrote:

> GPIO descriptions were assumed to be initialized with correct flags
> by GPIO chips. Hence, DT flags were being ignored.
> Translate flags from DTS flags to GPIOD flags when of_get_*gpiod_* is
> called.
>
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>

Since I've merged Alexandre's other patch making the function private
I'm dropping this assuming you have sorted things out...

Yours,
Linus Walleij

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Robert ABEL]

Hi Linus,

On 13 May 2014 10:42, Linus Walleij wrote:
> Since I've merged Alexandre's other patch making the function private 
> I'm dropping this assuming you have sorted things out...
No, I just stopped responding, because I grew tired of his stupid argument.
You want broken functions, you get broken functions.

Alexandre's claim that

On 05 May 2014 17:14, Alexandre Courbot wrote:
> Good news, once of_get_gpiod_flags() becomes private all public gpiod_
> functions should return a correct gpio_desc.
is wrong, of course. And ridiculous, I might add.
None of the gpiod_* functions return valid gpio_desc with the singular 
exception of gpiod_get, which happens to be aware of the shortcomings of 
all other public functions and corrects their mistakes after the fact.

Regards,

Robert

Re: [PATCH] GPIOD, OF: parse flags into gpiod

[Linus Walleij]

On Tue, May 13, 2014 at 1:33 PM, Robert ABEL
<rabel@cit-ec.uni-bielefeld.de> wrote:
> On 13 May 2014 10:42, Linus Walleij wrote:
>>
>> Since I've merged Alexandre's other patch making the function private I'm
>> dropping this assuming you have sorted things out...
>
> No, I just stopped responding, because I grew tired of his stupid argument.
> You want broken functions, you get broken functions.

Yeah well applying that patch removing the call broke a lot of drivers
so I had to take it out again and you can sort this out.

Please keep a civil tone even if I see you're both passionately
discussing this now, let's all be friends and figure out what to do
now OK?

Yours,
Linus Walleij