Question about non-boolean gpio sysfs values
Hello,
I'm now getting non-boolean values from gpio sysfs interface on 3.13, like this
root@wirenboard:~# cat /sys/class/gpio/gpio16/value 65536
(I'm working with imx23 soc which is handled by gpio-generic.c driver)
Looks like these weird non-boolean values appeared after this commit: https://github.com/torvalds/linux/commit/79a9becda8940deb2274b5aa4577c86d52ee7ecb .
1) Is it an expected behavior or not?
2) If it's not the case, which one of these should be fixed?
a) bgpio_get in gpio-generic.c to return only boolean values
or
b) gpio_value_show so only the sysfs interface will be affected
or
c) gpiod_get_value and gpiod_get_value_cansleep functions
--
Thanks,
Evgeny Boger
Quoted 8 lines · Evgeny Boger
Hello,
I'm now getting non-boolean values from gpio sysfs interface on 3.13, like this
root@wirenboard:~# cat /sys/class/gpio/gpio16/value 65536
Quoted 8 lines
(I'm working with imx23 soc which is handled by
gpio-generic.cdriver)Looks like these weird non-boolean values appeared after this commit:
https://github.com/torvalds/linux/commit/79a9becda8940deb2274b5aa4577c86d52ee7ecb
.1) Is it an expected behavior or not?
This seems wrong to me, a GPIO should always be 0 or 1.
Quoted 8 lines
2) If it's not the case, which one of these should be fixed?
a)bgpio_getingpio-generic.cto return only boolean values
or
b)gpio_value_showso only the sysfs interface will be affected
or
c)gpiod_get_valueandgpiod_get_value_cansleepfunctionsFrom what I understand of gpio-generic, this driver uses simple
register accesses and a bitmask to set/read GPIOs. Interestingly you are accessing GPIO 16, and the value returned is 65536, which happens to be (1 << 16).
As it turns out, bgpio_get() does the following:
return bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio);
So it seems like this is where your unexpected value comes from. As for where to fix it, the GPIO driver should return the value of the GPIO itself, and not some mask that indicates how it handles GPIOs internally. But I suppose it would also be nice to make gpiod_get_value*() more consistent so the whole subsystem gets fixed in one shot (I suspect a few other drivers are doing the same). I mean, we never expect a GPIO to be something else than 0 or 1, do we?
Alex.
Quoted 5 lines · Alexandre Courbot
Quoted 4 lines · Evgeny Boger
Hello,
I'm now getting non-boolean values from gpio sysfs interface on 3.13, like this
You didn't get this before?
Quoted 1 line
This seems wrong to me, a GPIO should always be 0 or 1.
Yup.
Quoted 3 lines
As it turns out,
bgpio_get()does the following:return bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio);
OK lemme patch this.
Quoted 4 lines
But I suppose it would also be nice to make gpiod_get_value*() more consistent so the whole subsystem gets fixed in one shot (I suspect a few other drivers are doing the same). I mean, we never expect a GPIO to be something else than 0 or 1, do we?
We can clamp the returned value in gpiod_get_value() for sure.
Maybe we should even print a warning there if the driver returns anything other that 0,1.
Maybe we should even retype the function to a bool, atleast in the driver-facing API.
Yours,
Linus Walleij
Quoted 24 lines · Linus Walleij
Quoted 5 lines · Alexandre Courbot
Quoted 4 lines · Evgeny Boger
Hello,
I'm now getting non-boolean values from gpio sysfs interface on 3.13, like this
You didn't get this before?
Quoted 1 line
This seems wrong to me, a GPIO should always be 0 or 1.
Yup.
Quoted 3 lines
As it turns out,
bgpio_get()does the following:return bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio);OK lemme patch this.
Quoted 4 lines
But I suppose it would also be nice to make gpiod_get_value*() more consistent so the whole subsystem gets fixed in one shot (I suspect a few other drivers are doing the same). I mean, we never expect a GPIO to be something else than 0 or 1, do we?
Quoted 7 lines
We can clamp the returned value in
gpiod_get_value()for sure.Maybe we should even print a warning there if the driver returns anything other that 0,1.
Maybe we should even retype the function to a bool, atleast in the driver-facing API.
Agree with the bool retyping, and while we are at it let's also change the driver function's prototype. That's probably the shortest way to fix this (yay to implicit type conversion!) and to make sure this problem isn't reintroduced in the future.
Shall I make a patch for this? It will change every driver around, but shouldn't be too intrusive.
Alex.
Quoted 5 lines · Alexandre Courbot
Quoted 2 lines · Linus Walleij
Maybe we should even retype the function to a bool, atleast in the driver-facing API.
Quoted 7 lines
Agree with the bool retyping, and while we are at it let's also change the driver function's prototype. That's probably the shortest way to fix this (yay to implicit type conversion!) and to make sure this problem isn't reintroduced in the future.
Shall I make a patch for this? It will change every driver around, but shouldn't be too intrusive.
Yeah hit me with it, I like that things are moving.
Yours,
Linus Walleij