Hi Uwe,

On Fri, Mar 24, 2017 at 9:39 AM, Uwe Kleine-K?nig
[off-list ref] wrote:

On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote:

quoted

On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-K?nig
[off-list ref] wrote:

quoted

From: Uwe Kleine-K?nig <redacted>
Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled

People disagree if gpiod_get_optional should return NULL or
ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that
the person who decided to disable GPIOLIB is assumed to know that there
is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might
introduce hard to debug problems if that decision is wrong.

So this patch introduces a compromise and let gpiod_get_optional (and
its variants) return NULL if the device in question cannot have an
associated GPIO because it is neither instantiated by a device tree nor
by ACPI.

This should handle most cases that are argued about.

Signed-off-by: Uwe Kleine-K?nig <redacted>
---
 include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index fb0fde686cb1..0ca29889290d 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h

@@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev,
        return ERR_PTR(-ENOSYS);
 }

-static inline struct gpio_desc *__must_check
-gpiod_get_optional(struct device *dev, const char *con_id,
-                  enum gpiod_flags flags)
+static inline bool __gpiod_no_optional_possible(struct device *dev)
 {
-       return ERR_PTR(-ENOSYS);
+       /*
+        * gpiod_get_optional et al can only provide a GPIO if at least one of
+        * the backends for specifing a GPIO is available. These are device
+        * tree, ACPI and gpiolib's lookup tables. The latter isn't available if
+        * GPIOLIB is disabled (which is the case here).
+        * So if the provided device is unrelated to device tree and ACPI, we
+        * can be sure that there is no optional GPIO and let gpiod_get_optional
+        * safely return NULL.
+        * Otherwise there is still a chance that there is no GPIO but we cannot
+        * be sure without having to enable a part of GPIOLIB (i.e. the lookup
+        * part). So lets play safe and return an error. (Though there are also
+        * arguments that returning NULL then would be beneficial.)
+        */
+
+       if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
+               return false;

At first sight, I though this was OK:

  1. On ARM with DT, we can assume CONFIG_GPIOLOB=y.

  2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y,
     and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do
     not use DT (yet), the check for dev->of_node (false) should handle
     that.

  3. However, I managed to do the same for h8300, which does use DT. Hence
     if mctrl_gpio would start relying on gpiod_get_optional(), this would
     break the sh-sci driver on h8300 :-(
     Note that h8300 doesn't have any GPIO drivers (yet?), so
     CONFIG_GPIPOLIB=n makes perfect sense!

Thanks for your efforts.

You're welcome.

quoted

So I'm afraid the only option is to always return NULL, and put the
responsability on the shoulders of the system integrator...

The gpio lines could be provided by an i2c gpio adapter, right? So IMHO
you don't need platform gpios to justify -ENODEV. So I guess that's a
case where we don't come to an agreement.

While you can enable I2C without further dependencies, no I2C GPIO expander
will be offered... unless you have enabled CONFIG_GPIOLIB first.

quoted

quoted

 static inline struct gpio_desc *__must_check
 gpiod_get_index_optional(struct device *dev, const char *con_id,
                         unsigned int index, enum gpiod_flags flags)
 {
+       if (__gpiod_no_optional_possible(dev))
+               return NULL;
+
        return ERR_PTR(-ENOSYS);

Regardless of the above, given you use the exact same construct in four
locations, what about letting __gpiod_no_optional_possible() return the NULL
or ERR_PTR itself, and renaming it to e.g. __gpiod_no_optional_return_value()?

I thought about that but didn't find a good name and so considered it
more clear this way. Another optimisation would be to unconditionally
define get_optional in terms of get_index_optional which would simplify
my patch a bit.

I'd consider __gpiod_optional_return_value a better name than
__gpiod_no_optional_return_value but I'm still not convinced.

No hard feelings about the name from my side.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds