On Tue, Aug 19, 2014 at 1:56 AM, Mika Westerberg
[off-list ref] wrote:

On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:

quoted

On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
[off-list ref] wrote:

quoted

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 2ebc9071e354..e6c2413a6fbf 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c

@@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
        return desc;
 }

+struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx,
+                                     enum gpio_lookup_flags *flags)
+{
+       struct gpio_desc *desc = ERR_PTR(-ENOENT);
+
+       if (!dev || !flags)
+               return ERR_PTR(-EINVAL);
+
+       /* Using device tree? */
+       if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+               desc = of_find_gpio(dev, NULL, idx, flags);
+       else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev))
+               desc = acpi_get_gpiod_flags(dev, idx, flags);
+
+       return desc;
+}
+EXPORT_SYMBOL(dev_get_gpiod_flags);

Putting aside the fact that this function is clearly ACPI-centric (no
con_id parameter and no handling of the platform interface), I have
two big problems with it and it ending up in the consumer interface:

1) The returned descriptor is not requested by gpiolib, which means no
check is made about whether the GPIO has already been requested by
someone else, and another driver can very well request the same GPIO
later and obtain it. Any descriptor returned by a function in
consumer.h *must* be properly requested. Furthermore the 1:1 mapping
between GPIO descriptors and GPIO numbers is not something we can take
for granted (since it will likely change soon), so this practice is
definitely to ban.

My bad, somehow I missed the part that it never requested the GPIO.
Thanks for pointing it out.

quoted

2) It exposes the GPIO flags, while they are supposed to be opaque to consumers.

And this, of course we should be using gpiod_is_active_low() and similar
functions that work with descriptors.

Yes, although if you convert the driver to use descriptors you should
not even have to worry about active_low status.

For drivers that still need to handle GPIO numbers for compatibility
reasons, it might be nice if gpiolib provided a gpio_to_desc() variant
that accepts an ACTIVE_LOW flag, so you don't have to worry about the
active low status once you have converted your GPIO number to a
descriptor. Actually for these cases we may be better with a function
that does what gpio_to_desc() does, but also requests the GPIO and
allows some flags to be specified so the integer-handling part of
drivers can be completely dropped afterwards. That's another problem
though. :)

quoted

These two points would somehow be acceptable if this function was
gpiolib-private, but here it is clearly not the case and this allows
pretty nasty thing to happen. Basically you are using it to take
advantage of the gpiod lookup mechanism and then quickly fall back to
the legacy integer interface. That's really not something to encourage
- these drivers should be converted to use gpiod internally (while
preserving integer-based lookup for compatiblity, if needed).

In patch 8 you say:

"this can be solved by adding a new field of type
struct gpio_desc but then there is another problem: the devm_gpiod_get
needs to operate on the button device instead of its parent device that
has the driver binded, so when the driver is unloaded, the resources for
the gpio will not get freed automatically."

I'd very much prefer that you use the non-devm variant of gpiod_get()
and free the resources manually when the driver is unloaded than this
workaround that introduces an loophole in the gpiod consumer lookup
functions.

I agree and we are going to rework this and the consumer patches to do
exactly what you say.

Great, thanks to you and Aaron for your understanding!

Alex.