[PATCH] pinctrl: sunxi: set pin function as input on export

[PATCH] pinctrl: sunxi: set pin function as input on export

[Krzysztof Adamski]

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

Re: [PATCH] pinctrl: sunxi: set pin function as input on export

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]

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-++hxYGjEMp0AvxtiuMwx3w@public.gmane.org>
> ---
>  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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] pinctrl: sunxi: set pin function as input on export

[Linus Walleij]

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

Re: [PATCH] pinctrl: sunxi: set pin function as input on export

[Krzysztof Adamski]

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-++hxYGjEMp0AvxtiuMwx3w@public.gmane.org> 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-++hxYGjEMp0AvxtiuMwx3w@public.gmane.org>
>
>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

Re: [PATCH] pinctrl: sunxi: set pin function as input on export

[Linus Walleij]

On Tue, Feb 16, 2016 at 9:00 AM, Krzysztof Adamski
<krzysztof.adamski-++hxYGjEMp0AvxtiuMwx3w@public.gmane.org> 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