Re: [PATCH 6/7] drivers/gpio: Add and export gpiod_lookup[_index]

From: Linus Walleij
Date: Thu Jan 26 2017 - 10:24:35 EST


On Wed, Jan 25, 2017 at 1:06 AM, Furquan Shaikh <furquan@xxxxxxxxxxxx> wrote:

> Sometimes (as the case with fixed regulator) we only want to look up,
> but not necessarily reserve right away, GPIO. gpiod_lookup() and
> gpiod_lookup_by_index() allow us doing just that.
>
> Signed-off-by: Dmitry Torokhov <dtor@xxxxxxxxxxxx>
> Signed-off-by: Furquan Shaikh <furquan@xxxxxxxxxxxx>

Overall if this solves the GPIO regulator mess, I'd be happy
to apply it.

But overall it is a bit scary with looking up something struct gpiod *
that does not at all have the same semantics as a gpiod *
you get from gpiod_get().

We need to make sure that a gpiod looked up this way does not
work and tick and cannot perform operations as one obtained
with a proper gpiod_get().

> /**
> + * gpiod_lookup - look up a GPIO for a given GPIO function
> + * @dev: GPIO consumer, can be NULL for system-global GPIOs
> + * @con_id: function within the GPIO consumer
> + *
> + * Return the GPIO descriptor corresponding to the function con_id of device
> + * dev, -ENOENT if no GPIO has been assigned to the requested function, or
> + * another IS_ERR() code.
> + */
> +struct gpio_desc *__must_check gpiod_lookup(struct device *dev,
> + const char *con_id)
> +{
> + return gpiod_lookup_index(dev, con_id, 0);
> +}
> +EXPORT_SYMBOL_GPL(gpiod_lookup);

Make this a static inline in the <linux/gpio/consumer.h> header instead.
Saves a lot of trouble.

> /**
> - * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
> + * gpiod_lookup_index - look up a GPIO from a multi-index GPIO function
> * @dev: GPIO consumer, can be NULL for system-global GPIOs
> * @con_id: function within the GPIO consumer
> * @idx: index of the GPIO to obtain in the consumer
> - * @flags: optional GPIO initialization flags
> - *
> - * This variant of gpiod_get() allows to access GPIOs other than the first
> - * defined one for functions that define several GPIOs.
> *
> - * Return a valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
> - * requested function and/or index, or another IS_ERR() code if an error
> - * occurred while trying to acquire the GPIO.
> + * Return a valid GPIO descriptor, or -ENOENT if no GPIO has been assigned to
> + * the requested function and/or index, or another IS_ERR() code.
> */

What a mess in the patch. Not your fault but format-patch created this
mess.0

> -struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
> - const char *con_id,
> - unsigned int idx,
> - enum gpiod_flags flags)
> +struct gpio_desc *__must_check gpiod_lookup_index(struct device *dev,
> + const char *con_id,
> + unsigned int idx)
> {
> struct gpio_desc *desc = NULL;
> - int status;
> enum gpio_lookup_flags lookupflags = 0;
>
> dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
> @@ -3285,16 +3294,50 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
> desc = gpiod_find(dev, con_id, idx, &lookupflags);
> }
>
> - if (IS_ERR(desc)) {
> + if (IS_ERR(desc))
> dev_dbg(dev, "lookup for GPIO %s failed\n", con_id);
> +
> + /*
> + * Configure static flags based on lookup data (such as
> + * "active low", "open drain", etc.)
> + */
> + gpiod_configure_flags(desc, con_id, lookupflags, 0);

So what about setting some special flag from
gpiolib.h like
set_bit(FLAG_ONLY_LOOKUP, &desc->flags);
here numbing all
operations on the returned gpiod, by a check in
VALIDATE_DESC() and VALIDATE_DESC_VOID()
so that no real operations fall through?

Then that flag will just be forcibly cleared by
gpiod_get[_index]() later.

Yours,
Linus Walleij