| Message ID | 1454942242-25690-1-git-send-email-krzysztof.adamski@tieto.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mon, Feb 08, 2016 at 03:37:22PM +0100, Krzysztof Adamski wrote: > Default function of a pin in sunxi SoCs is "disabled". By default gpios > exported by sysfs are set as input and indeed, when reading "direction" > file you will get "in". The "value" pin won't return proper value, > though, confusing user of this interface. > > This patch sets direction of a GPIO as input when exporting it. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com> > --- > drivers/pinctrl/sunxi/pinctrl-sunxi.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c > index 7a2465f..905a9fb 100644 > --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c > +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c > @@ -452,6 +452,16 @@ static int sunxi_pinctrl_gpio_direction_input(struct gpio_chip *chip, > return pinctrl_gpio_direction_input(chip->base + offset); > } > > +int sunxi_pinctrl_gpio_request(struct gpio_chip *chip, unsigned offset) > +{ > + int ret = pinctrl_gpio_direction_input(chip->base + offset); > + > + if (ret) > + return ret; > + > + return pinctrl_request_gpio(chip->base + offset); > +} > + > static int sunxi_pinctrl_gpio_get(struct gpio_chip *chip, unsigned offset) > { > struct sunxi_pinctrl *pctl = gpiochip_get_data(chip); > @@ -946,7 +956,7 @@ int sunxi_pinctrl_init(struct platform_device *pdev, > > last_pin = pctl->desc->pins[pctl->desc->npins - 1].pin.number; > pctl->chip->owner = THIS_MODULE; > - pctl->chip->request = gpiochip_generic_request, > + pctl->chip->request = sunxi_pinctrl_gpio_request, > pctl->chip->free = gpiochip_generic_free, > pctl->chip->direction_input = sunxi_pinctrl_gpio_direction_input, > pctl->chip->direction_output = sunxi_pinctrl_gpio_direction_output, > -- > 2.4.2 > It seems to me that it's something that should be enforced in the core, there's nothing sunxi specific here. Thanks! Maxime
On Mon, Feb 8, 2016 at 3:37 PM, Krzysztof Adamski <krzysztof.adamski@tieto.com> wrote: > Default function of a pin in sunxi SoCs is "disabled". By default gpios > exported by sysfs are set as input and indeed, when reading "direction" > file you will get "in". The "value" pin won't return proper value, > though, confusing user of this interface. > > This patch sets direction of a GPIO as input when exporting it. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com> As maxime says it's not a sunxi problem. So maybe it is wrong to set pins to either input or output when exporting them, I suspect they should be exported as is. Also: the sysfs ABI is deprecated. I suggest you invest your time in working on the new chardev ABI. Yours, Linus Walleij
On Mon, Feb 15, 2016 at 11:04:26PM +0100, Linus Walleij wrote: >On Mon, Feb 8, 2016 at 3:37 PM, Krzysztof Adamski ><krzysztof.adamski@tieto.com> wrote: > >> Default function of a pin in sunxi SoCs is "disabled". By default gpios >> exported by sysfs are set as input and indeed, when reading "direction" >> file you will get "in". The "value" pin won't return proper value, >> though, confusing user of this interface. >> >> This patch sets direction of a GPIO as input when exporting it. >> >> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com> > >As maxime says it's not a sunxi problem. > >So maybe it is wrong to set pins to either input or output when >exporting them, I suspect they should be exported as is. > >Also: the sysfs ABI is deprecated. I suggest you invest your time >in working on the new chardev ABI. While sysfs API may be very limited it does have it's strengths too. One of them is that it's trivially scriptable even from bash or other scripting languages and the other one is that it's there for so long that it's very popular and it will take much time before all the tutorials and existing applications are updated. So, well, I still find it useful to have good support for it. I find current behaviour of sysfs interface broken (it says that GPIO is in input mode when in reality it isn't) and still think it's a good idea to fix it. But putting sysfs issue aside, wouldn't it be safer to set some well known state of the pin when requesting it instead of leaving it at whatever was set before? The same question goes for freeing, actually, shouldn't the pin be disabled when we call gpio_free instead of leaving it at its last state? There are existing drivers that set gpio direction to input when requesting the pin (for example pinctrl-plgpio.c, pinctrl-sirf.c) so sunxi wouldn't be the only driver to do this. This does also show that indeed the problem is not sunxi specific, though. Best regards, Krzysztof Adamski
On Tue, Feb 16, 2016 at 9:00 AM, Krzysztof Adamski <krzysztof.adamski@tieto.com> wrote: > While sysfs API may be very limited it does have it's strengths too. One of > them is that it's trivially scriptable even from bash or other scripting > languages Feel free to contribute a tool to tools/gpio that perform what you need from scripts, using a chardev. But overall it is subject to the same issue as always: we open a chardev and hold it to keep references to it even if the hardware goes away. Scripting doesn't seem supergood at opening chardevs and holding them and are thus inherently fragile. With a chardev we can handle that gracefully. As it is right now, files in sysfs can just disappear while your script is running (if e.g. the GPIO is on a USB device or so) and then all hell breaks loose I think. So it is very fragile. > and the other one is that it's there for so long that it's very > popular and it will take much time before all the tutorials and existing > applications are updated. So, well, I still find it useful to have good > support for it. See commit fe95046e960b4b76e73dc1486955d93f47276134 it is deprecated but will be around until (at least) 2020. The chardev will be promoted as it is *always* available, whereas the sysfs has a Kconfig option you must switch on. > I find current behaviour of sysfs interface broken (it says that GPIO is in > input mode when in reality it isn't) and still think it's a good idea to fix > it. But putting sysfs issue aside, wouldn't it be safer to set some well > known state of the pin when requesting it instead of leaving it at whatever > was set before? We have this for in-kernel consumers (<linux/gpio/consumer.h>): /** * Optional flags that can be passed to one of gpiod_* to configure direction * and output value. These values cannot be OR'd. */ enum gpiod_flags { GPIOD_ASIS = 0, GPIOD_IN = GPIOD_FLAGS_BIT_DIR_SET, GPIOD_OUT_LOW = GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT, GPIOD_OUT_HIGH = GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT | GPIOD_FLAGS_BIT_DIR_VAL, }; We have not yet specified a proper ABI for usespace. Are these flags the same as you would want for userspace? My feeling is that unless specified ASIS is what you want. Especially if there is also an ABI to read the line value. Let's get the chardev right in this regard and use that. > The same question goes for freeing, actually, shouldn't the > pin be disabled when we call gpio_free instead of leaving it at its last > state? Well maybe we should have an ABI specifying what state we want it to be left in, if e.g. the userspace application either quit without saying anything, or more importantly, if it crashes. With the chardev we can do that. > There are existing drivers that set gpio direction to input when requesting > the pin (for example pinctrl-plgpio.c, pinctrl-sirf.c) so sunxi wouldn't be > the only driver to do this. This does also show that indeed the problem is > not sunxi specific, though. Drivers can do pretty much what they want, they are supposed to handle the hardware as well as they can. They might be doing this for a reason or not, better ask the driver writers and/or send them patches to augment the behaviour. Yours, Linus Walleij
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c index 7a2465f..905a9fb 100644 --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c @@ -452,6 +452,16 @@ static int sunxi_pinctrl_gpio_direction_input(struct gpio_chip *chip, return pinctrl_gpio_direction_input(chip->base + offset); } +int sunxi_pinctrl_gpio_request(struct gpio_chip *chip, unsigned offset) +{ + int ret = pinctrl_gpio_direction_input(chip->base + offset); + + if (ret) + return ret; + + return pinctrl_request_gpio(chip->base + offset); +} + static int sunxi_pinctrl_gpio_get(struct gpio_chip *chip, unsigned offset) { struct sunxi_pinctrl *pctl = gpiochip_get_data(chip); @@ -946,7 +956,7 @@ int sunxi_pinctrl_init(struct platform_device *pdev, last_pin = pctl->desc->pins[pctl->desc->npins - 1].pin.number; pctl->chip->owner = THIS_MODULE; - pctl->chip->request = gpiochip_generic_request, + pctl->chip->request = sunxi_pinctrl_gpio_request, pctl->chip->free = gpiochip_generic_free, pctl->chip->direction_input = sunxi_pinctrl_gpio_direction_input, pctl->chip->direction_output = sunxi_pinctrl_gpio_direction_output,
Default function of a pin in sunxi SoCs is "disabled". By default gpios exported by sysfs are set as input and indeed, when reading "direction" file you will get "in". The "value" pin won't return proper value, though, confusing user of this interface. This patch sets direction of a GPIO as input when exporting it. Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com> --- drivers/pinctrl/sunxi/pinctrl-sunxi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)