gpio: Add helpers to determin direction of gpiods

Message ID 20171210002340.18460-1-linus.walleij@linaro.org
State New
Headers show
Series
  • gpio: Add helpers to determin direction of gpiods
Related show

Commit Message

Linus Walleij Dec. 10, 2017, 12:23 a.m.
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(-)

-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Wolfram Sang Dec. 12, 2017, 10:42 p.m. | #1
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
Geert Uytterhoeven Dec. 13, 2017, 10:15 a.m. | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Dec. 13, 2017, 2:54 p.m. | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Dec. 13, 2017, 3:03 p.m. | #4
> > 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.

Patch

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,