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

2017-12-13 Thread Wolfram Sang 

> > 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 .
> 
> It has the upside that we can also check the return value
> for errors.

Sounds good, in deed.



signature.asc
Description: PGP signature

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

2017-12-13 Thread Linus Walleij 
On Tue, Dec 12, 2017 at 11:42 PM, Wolfram Sang  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 .

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

2017-12-12 Thread Wolfram Sang 
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 
> Suggested-by: Wolfram Sang 
> Signed-off-by: Linus Walleij 
> ---
>  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



signature.asc
Description: PGP signature