[PATCH] gpio: Add helpers to determin direction of gpiods

[PATCH] gpio: Add helpers to determin direction of gpiods

[Linus Walleij]

The gpiod_get_direction() returns 1 for input and 0 for output
but it's pretty hard to remember which one is which and generally
unintuitive and messy to provide #defines so let's simply add
two static inlines to do the job.

Cc: Wolfram Sang <wsa@the-dreams.de>
Suggested-by: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/gpio/consumer.txt | 14 +++++++++++---
 include/linux/gpio/consumer.h   | 25 +++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index 63e1bd1d88e3..c4b8e3d8c29f 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -162,8 +162,8 @@ The device-managed variants are, unsurprisingly:
 Using GPIOs
 ===========
 
-Setting Direction
------------------
+Setting and Getting Direction
+-----------------------------
 The first thing a driver must do with a GPIO is setting its direction. If no
 direction-setting flags have been given to gpiod_get*(), this is done by
 invoking one of the gpiod_direction_*() functions:
@@ -184,7 +184,15 @@ A driver can also query the current direction of a GPIO:
 
 	int gpiod_get_direction(const struct gpio_desc *desc)
 
-This function will return either GPIOF_DIR_IN or GPIOF_DIR_OUT.
+This function will return either 1 (input) or 0 (output). There are also
+these convenience helpers:
+
+	bool gpiod_is_input(const struct gpio_desc *desc)
+	bool gpiod_is_output(const struct gpio_desc *desc)
+
+These will not provide the same level of error fallbacks: if they fail to
+obtain the direction, they will print an error and report as input since this
+is usually safest.
 
 Be aware that there is no default direction for GPIOs. Therefore, **using a GPIO
 without setting its direction first is illegal and will result in undefined
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 7447d85dbe2f..4e5ee3ec1913 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -487,6 +487,31 @@ struct gpio_desc *devm_fwnode_get_index_gpiod_from_child(struct device *dev,
 
 #endif /* CONFIG_GPIOLIB */
 
+/*
+ * Helpers that quickly tell whether a line is input or output.
+ */
+static inline bool gpiod_is_input(struct gpio_desc *desc)
+{
+	int ret = gpiod_get_direction(desc);
+	if (ret < 0) {
+		pr_err("GPIO: failed to get direction\n");
+		/* It is usually safest to assume we are input */
+		return true;
+	}
+	return !!ret;
+}
+
+static inline bool gpiod_is_output(struct gpio_desc *desc)
+{
+	int ret = gpiod_get_direction(desc);
+	if (ret < 0) {
+		pr_err("GPIO: failed to get direction\n");
+		/* It is usually safest to assume we are input */
+		return false;
+	}
+	return !ret;
+}
+
 static inline
 struct gpio_desc *devm_fwnode_get_gpiod_from_child(struct device *dev,
 						   const char *con_id,
-- 
2.14.3

Re: [PATCH] gpio: Add helpers to determin direction of gpiods

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

Hey Linus,

thanks for doing this.

On Sun, Dec 10, 2017 at 01:23:40AM +0100, Linus Walleij wrote:
> The gpiod_get_direction() returns 1 for input and 0 for output
> but it's pretty hard to remember which one is which and generally
> unintuitive and messy to provide #defines so let's simply add
> two static inlines to do the job.
> 
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Suggested-by: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/gpio/consumer.txt | 14 +++++++++++---
>  include/linux/gpio/consumer.h   | 25 +++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 3 deletions(-)

You need to fix the kerneldoc for 'gpiod_get_direction", too. It still
mentions GPIOF_DIR_*.

> +static inline bool gpiod_is_input(struct gpio_desc *desc)
> +{
> +	int ret = gpiod_get_direction(desc);
> +	if (ret < 0) {
> +		pr_err("GPIO: failed to get direction\n");

Is that really helpful for the user if we don't say which GPIO failed?

> +		/* It is usually safest to assume we are input */
> +		return true;
> +	}
> +	return !!ret;
> +}

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gpio: Add helpers to determin direction of gpiods

[Linus Walleij]

On Tue, Dec 12, 2017 at 11:42 PM, Wolfram Sang <wsa@the-dreams.de> wrote:

>>  Documentation/gpio/consumer.txt | 14 +++++++++++---
>
> You need to fix the kerneldoc for 'gpiod_get_direction", too. It still
> mentions GPIOF_DIR_*.

More places? I can't find them!

I thought the patch removed that here:

-This function will return either GPIOF_DIR_IN or GPIOF_DIR_OUT.
+This function will return either 1 (input) or 0 (output). There are also
+these convenience helpers:

I can only find it in Documentation/gpio/gpio-legacy.txt which is
fine.

>> +static inline bool gpiod_is_input(struct gpio_desc *desc)
>> +{
>> +     int ret = gpiod_get_direction(desc);
>> +     if (ret < 0) {
>> +             pr_err("GPIO: failed to get direction\n");
>
> Is that really helpful for the user if we don't say which GPIO failed?

I would have to add yet more APIs to do that.

Hm.

Together with Geert's comments I start to lean towards actually
just creating two new #defines in <linux/consumer.h>.

It has the upside that we can also check the return value
for errors.

Yours,
Linus Walleij

Re: [PATCH] gpio: Add helpers to determin direction of gpiods

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]


> > You need to fix the kerneldoc for 'gpiod_get_direction", too. It still
> > mentions GPIOF_DIR_*.
> 
> More places? I can't find them!

drivers/gpio/gpiolib.c

Line 198 in both 4.15-rc3 and linux-next.

Or as I said, the kerneldoc of gpiod_get_direction ;)

> Together with Geert's comments I start to lean towards actually
> just creating two new #defines in <linux/consumer.h>.
> 
> It has the upside that we can also check the return value
> for errors.

Sounds good, in deed.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gpio: Add helpers to determin direction of gpiods

[Geert Uytterhoeven]

Hi Linus,

On Sun, Dec 10, 2017 at 1:23 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> The gpiod_get_direction() returns 1 for input and 0 for output
> but it's pretty hard to remember which one is which and generally
> unintuitive and messy to provide #defines so let's simply add
> two static inlines to do the job.
>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Suggested-by: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/gpio/consumer.txt | 14 +++++++++++---
>  include/linux/gpio/consumer.h   | 25 +++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
> index 63e1bd1d88e3..c4b8e3d8c29f 100644
> --- a/Documentation/gpio/consumer.txt
> +++ b/Documentation/gpio/consumer.txt

> @@ -184,7 +184,15 @@ A driver can also query the current direction of a GPIO:
>
>         int gpiod_get_direction(const struct gpio_desc *desc)
>
> -This function will return either GPIOF_DIR_IN or GPIOF_DIR_OUT.
> +This function will return either 1 (input) or 0 (output). There are also
> +these convenience helpers:
> +
> +       bool gpiod_is_input(const struct gpio_desc *desc)
> +       bool gpiod_is_output(const struct gpio_desc *desc)
> +
> +These will not provide the same level of error fallbacks: if they fail to
> +obtain the direction, they will print an error and report as input since this
> +is usually safest.

Leaving a pin configured as input is indeed the safest, from a hardware
point of view.
But I don't know if reporting to software that it is input is safe ;-)

Can this actually fail?

> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -487,6 +487,31 @@ struct gpio_desc *devm_fwnode_get_index_gpiod_from_child(struct device *dev,
>
>  #endif /* CONFIG_GPIOLIB */
>
> +/*
> + * Helpers that quickly tell whether a line is input or output.
> + */
> +static inline bool gpiod_is_input(struct gpio_desc *desc)
> +{
> +       int ret = gpiod_get_direction(desc);
> +       if (ret < 0) {
> +               pr_err("GPIO: failed to get direction\n");

Do we really want a copy of this string in every module using this function?
If you want to keep the error message, the function should be moved to
the GPIO code, I think.

> +               /* It is usually safest to assume we are input */
> +               return true;
> +       }
> +       return !!ret;
> +}
> +
> +static inline bool gpiod_is_output(struct gpio_desc *desc)
> +{
> +       int ret = gpiod_get_direction(desc);
> +       if (ret < 0) {
> +               pr_err("GPIO: failed to get direction\n");

Likewise.

> +               /* It is usually safest to assume we are input */
> +               return false;
> +       }
> +       return !ret;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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