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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
