| Message ID | 0a1fe4365ef599adde42396f0bb735c8623f679c.1572945757.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/62] gpio: Add definition for GPIO direction | expand |
On Tue, Nov 05, 2019 at 12:16:03PM +0200, Matti Vaittinen wrote: > It's hard for occasional GPIO code reader/writer to know if values 0/1 > equal to IN or OUT. Use defined GPIO_LINE_DIRECTION_IN and > GPIO_LINE_DIRECTION_OUT to help them out. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > drivers/gpio/gpio-f7188x.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c > index fdc639f856f1..cadd02993539 100644 > --- a/drivers/gpio/gpio-f7188x.c > +++ b/drivers/gpio/gpio-f7188x.c > @@ -250,7 +250,10 @@ static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset) > > superio_exit(sio->addr); > > - return !(dir & 1 << offset); > + if (dir & 1 << offset) > + return GPIO_LINE_DIRECTION_OUT; > + > + return GPIO_LINE_DIRECTION_IN Hi Matti, I am probably missing something but I can't find GPIO_LINE_DIRECTION_IN and GPIO_LINE_DIRECTION_OUT defined anywhere. Besides I am an occasional code reader/writer and I find the original code easy to understand. Simon
Morning Simon, On Wed, 2019-11-06 at 06:34 +0100, Simon Guinot wrote: > On Tue, Nov 05, 2019 at 12:16:03PM +0200, Matti Vaittinen wrote: > > It's hard for occasional GPIO code reader/writer to know if values > > 0/1 > > equal to IN or OUT. Use defined GPIO_LINE_DIRECTION_IN and > > GPIO_LINE_DIRECTION_OUT to help them out. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > drivers/gpio/gpio-f7188x.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio- > > f7188x.c > > index fdc639f856f1..cadd02993539 100644 > > --- a/drivers/gpio/gpio-f7188x.c > > +++ b/drivers/gpio/gpio-f7188x.c > > @@ -250,7 +250,10 @@ static int f7188x_gpio_get_direction(struct > > gpio_chip *chip, unsigned offset) > > > > superio_exit(sio->addr); > > > > - return !(dir & 1 << offset); > > + if (dir & 1 << offset) > > + return GPIO_LINE_DIRECTION_OUT; > > + > > + return GPIO_LINE_DIRECTION_IN > > Hi Matti, > > I am probably missing something but I can't find > GPIO_LINE_DIRECTION_IN > and GPIO_LINE_DIRECTION_OUT defined anywhere. Sorry. I accidentally sent the patch 01/62 to limited audience - and also messed up the message-ID from the series so threading messages is probably not working :( I did resend the patch adding defines to all reviewers yesterday - title should be "[RESEND PATCH 01/62] gpio: Add definition for GPIO direction". > Besides I am an occasional code reader/writer and I find the original > code easy to understand. Glad to hear that. When I read code: return !(dir & 1 << offset); It's impossible for me to tell if dir having bit at offset 'offset' set means IN or OUT - I know the meaning of code, it checks this bit for in/out - but which dir value is IN and which is OUT? When this is written as: if (dir & 1 << offset) return GPIO_LINE_DIRECTION_OUT; return GPIO_LINE_DIRECTION_IN it get's quite obvious even for me that having the matching bit set means direction to be OUT. Best Regards Matti Vaittinen
Hi Matti, On Wed, Nov 6, 2019 at 7:45 AM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > On Wed, 2019-11-06 at 06:34 +0100, Simon Guinot wrote: > > On Tue, Nov 05, 2019 at 12:16:03PM +0200, Matti Vaittinen wrote: > > > It's hard for occasional GPIO code reader/writer to know if values > > > 0/1 > > > equal to IN or OUT. Use defined GPIO_LINE_DIRECTION_IN and > > > GPIO_LINE_DIRECTION_OUT to help them out. > > > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > --- > > > drivers/gpio/gpio-f7188x.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio- > > > f7188x.c > > > index fdc639f856f1..cadd02993539 100644 > > > --- a/drivers/gpio/gpio-f7188x.c > > > +++ b/drivers/gpio/gpio-f7188x.c > > > @@ -250,7 +250,10 @@ static int f7188x_gpio_get_direction(struct > > > gpio_chip *chip, unsigned offset) > > > > > > superio_exit(sio->addr); > > > > > > - return !(dir & 1 << offset); > > > + if (dir & 1 << offset) > > > + return GPIO_LINE_DIRECTION_OUT; > > > + > > > + return GPIO_LINE_DIRECTION_IN > > > > Hi Matti, > > > > I am probably missing something but I can't find > > GPIO_LINE_DIRECTION_IN > > and GPIO_LINE_DIRECTION_OUT defined anywhere. > > Sorry. I accidentally sent the patch 01/62 to limited audience - and > also messed up the message-ID from the series so threading messages is > probably not working :( I did resend the patch adding defines to all > reviewers yesterday - title should be "[RESEND PATCH 01/62] gpio: Add > definition for GPIO direction". > > > Besides I am an occasional code reader/writer and I find the original > > code easy to understand. > > Glad to hear that. When I read code: > > return !(dir & 1 << offset); > > It's impossible for me to tell if dir having bit at offset 'offset' set > means IN or OUT - I know the meaning of code, it checks this bit for > in/out - but which dir value is IN and which is OUT? > > When this is written as: > > if (dir & 1 << offset) > return GPIO_LINE_DIRECTION_OUT; > > return GPIO_LINE_DIRECTION_IN > > it get's quite obvious even for me that having the matching bit set > means direction to be OUT. "suggest parentheses around... " warning? if (dir & BIT(offset)) ... Gr{oetje,eeting}s, Geert
Hello Geert, On Wed, 2019-11-06 at 13:38 +0100, Geert Uytterhoeven wrote: > Hi Matti, > > On Wed, Nov 6, 2019 at 7:45 AM Vaittinen, Matti > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > On Wed, 2019-11-06 at 06:34 +0100, Simon Guinot wrote: > > > On Tue, Nov 05, 2019 at 12:16:03PM +0200, Matti Vaittinen wrote: > > > > It's hard for occasional GPIO code reader/writer to know if > > > > values > > > > 0/1 > > > > equal to IN or OUT. Use defined GPIO_LINE_DIRECTION_IN and > > > > GPIO_LINE_DIRECTION_OUT to help them out. > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > matti.vaittinen@fi.rohmeurope.com> > > > > --- > > > > drivers/gpio/gpio-f7188x.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio- > > > > f7188x.c > > > > index fdc639f856f1..cadd02993539 100644 > > > > --- a/drivers/gpio/gpio-f7188x.c > > > > +++ b/drivers/gpio/gpio-f7188x.c > > > > @@ -250,7 +250,10 @@ static int > > > > f7188x_gpio_get_direction(struct > > > > gpio_chip *chip, unsigned offset) > > > > > > > > superio_exit(sio->addr); > > > > > > > > - return !(dir & 1 << offset); > > > > + if (dir & 1 << offset) > > > > + return GPIO_LINE_DIRECTION_OUT; > > > > + > > > > + return GPIO_LINE_DIRECTION_IN > > > > > > Hi Matti, > > > > > > I am probably missing something but I can't find > > > GPIO_LINE_DIRECTION_IN > > > and GPIO_LINE_DIRECTION_OUT defined anywhere. > > > > Sorry. I accidentally sent the patch 01/62 to limited audience - > > and > > also messed up the message-ID from the series so threading messages > > is > > probably not working :( I did resend the patch adding defines to > > all > > reviewers yesterday - title should be "[RESEND PATCH 01/62] gpio: > > Add > > definition for GPIO direction". > > > > > Besides I am an occasional code reader/writer and I find the > > > original > > > code easy to understand. > > > > Glad to hear that. When I read code: > > > > return !(dir & 1 << offset); > > > > It's impossible for me to tell if dir having bit at offset 'offset' > > set > > means IN or OUT - I know the meaning of code, it checks this bit > > for > > in/out - but which dir value is IN and which is OUT? > > > > When this is written as: > > > > if (dir & 1 << offset) > > return GPIO_LINE_DIRECTION_OUT; > > > > return GPIO_LINE_DIRECTION_IN > > > > it get's quite obvious even for me that having the matching bit set > > means direction to be OUT. > > "suggest parentheses around... " warning? I don't think I saw that. Also, simple test: #include <stdio.h> int main(int argc, char *argv[]) { int offset = argc; if (4 & 1 << offset) printf("foo\n"); printf("%d", !(4 & 1 << offset)); return !(4 & 1 << offset); } and building with gcc -Wall does not show any warnings. nor did compilation for the driver: [mvaittin@localhost linux]$ make ARCH=arm CROSS_COMPILE=${CC} LOADADDR=0x80008000 CALL scripts/checksyscalls.sh CALL scripts/atomic/check-atomics.sh CHK include/generated/compile.h CC [M] drivers/gpio/gpio-f7188x.o Kernel: arch/arm/boot/Image is ready Kernel: arch/arm/boot/zImage is ready Building modules, stage 2. MODPOST 1282 modules LD [M] drivers/gpio/gpio-f7188x.ko [mvaittin@localhost linux]$ In any case, my intention was to keep the logic exactly same - except for the documented change for cases where I changed bit-position specific return value to 1. Br, Matti Vaittinen
diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c index fdc639f856f1..cadd02993539 100644 --- a/drivers/gpio/gpio-f7188x.c +++ b/drivers/gpio/gpio-f7188x.c @@ -250,7 +250,10 @@ static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset) superio_exit(sio->addr); - return !(dir & 1 << offset); + if (dir & 1 << offset) + return GPIO_LINE_DIRECTION_OUT; + + return GPIO_LINE_DIRECTION_IN; } static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
It's hard for occasional GPIO code reader/writer to know if values 0/1 equal to IN or OUT. Use defined GPIO_LINE_DIRECTION_IN and GPIO_LINE_DIRECTION_OUT to help them out. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/gpio/gpio-f7188x.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)