Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Mon, Aug 18, 2014 at 09:24:48AM -0700, Alexandre Courbot wrote:
> On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > 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.

> 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.

> 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.