[PATCH 1/1] gpio: pisosr: Don't use magic numbers

[PATCH 1/1] gpio: pisosr: Don't use magic numbers

[Alexander Stein]

At first view I thought this function returned an error, but actually
it is the input direction. Use the define for input which makes reading
the code much easier.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/gpio/gpio-pisosr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-pisosr.c b/drivers/gpio/gpio-pisosr.c
index 58ea08d..068de54 100644
--- a/drivers/gpio/gpio-pisosr.c
+++ b/drivers/gpio/gpio-pisosr.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
@@ -65,7 +66,7 @@ static int pisosr_gpio_get_direction(struct gpio_chip *chip,
 				     unsigned offset)
 {
 	/* This device always input */
-	return 1;
+	return GPIOF_DIR_IN;
 }
 
 static int pisosr_gpio_direction_input(struct gpio_chip *chip,
-- 
2.4.10

Re: [PATCH 1/1] gpio: pisosr: Don't use magic numbers

[Linus Walleij]

On Mon, Feb 15, 2016 at 10:14 AM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:

> At first view I thought this function returned an error, but actually
> it is the input direction. Use the define for input which makes reading
> the code much easier.
>
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

NACK that flag is for consumers, not drivers.
Drivers have their own API and should ideally
return a bool true/false, but that would be another major
refactoring....

Yours,
Linus Walleij

Re: [PATCH 1/1] gpio: pisosr: Don't use magic numbers

[Alexander Stein]

On Tuesday 16 February 2016 15:55:47, Linus Walleij wrote:
> On Mon, Feb 15, 2016 at 10:14 AM, Alexander Stein
> <alexander.stein@systec-electronic.com> wrote:
> 
> > At first view I thought this function returned an error, but actually
> > it is the input direction. Use the define for input which makes reading
> > the code much easier.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> 
> NACK that flag is for consumers, not drivers.
> Drivers have their own API and should ideally
> return a bool true/false, but that would be another major
> refactoring....

Well, having a callback get_direction returning a bool would seem really strange. Actually the comments on gpiod_get_direction explicitly state GPIOF_DIR_IN and GPIOF_DIR_OUT as return values which aren't used in that function itself, they come from the callback. Also other dirvers like e.g. gpio-ich return (in ichx_gpio_get_direction) GPIOF_DIR_IN or GPIOF_DIR_OUT.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Re: [PATCH 1/1] gpio: pisosr: Don't use magic numbers

[Linus Walleij]

On Tue, Feb 16, 2016 at 4:35 PM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:

> Well, having a callback get_direction returning a bool would seem really strange.

Well maybe an enum would be better. Patches accepted.

> Actually the comments on gpiod_get_direction explicitly state GPIOF_DIR_IN and GPIOF_DIR_OUT
> as return values which aren't used in that function itself, they come from the callback.

Yeah so it is the gpiolib core that need to make sure the right value is
propagated back to the API, not the driver. It exploits the fact that
driver return a suiting 0/1 that happen to correspond to that.

> Also other
> dirvers like e.g. gpio-ich return (in ichx_gpio_get_direction) GPIOF_DIR_IN or GPIOF_DIR_OUT.

Two wrong doesn't make one right. Patch gpio-ich to return 0/1 if
you like, or even better: add a direction enum to <linux/gpio/driver.h>
and start patching the callback and drivers to use that.

Overall this is a thing that "just works" mostly, but I have had
many problems with the driver and consumer API being mixed
up (mostly by everyone using <linux/gpio.h> and I don't want
to encourage that mess.

Drivers should use <linux/gpio/driver.h> and nothing else.
Consumers should use <linus/gpio/consumer.h> and nothing else.

The latter require that they switch to GPIO descriptors though.
I think.

Yours,
Linus Walleij