Re: [PATCH v2 1/4] gpiolib: Add "unknown" direction support

[Eric Miao]

On Thu, Feb 17, 2011 at 8:56 AM, Peter Tyser <ptyser@xxxxxxxxxxx> wrote:
> Previously, gpiolib would unconditionally flag all GPIO pins as inputs,
> regardless of their true state. ÂThis resulted in all GPIO output pins
> initially being incorrectly identified as "input" in the GPIO sysfs.
>
> Since the direction of GPIOs is not known prior to having their
> direction set, instead set the default direction to "unknown" to prevent
> user confusion. ÂA pin with an "unknown" direction can not be written or
> read via sysfs; it must first be configured as an input or output before
> it can be used.
>

Hrm... that's why I don't like the original definition of gpio_request()
which is vague on the pin configurations. The pin configuration should
be clear upon requesting, otherwise it's a potential issue.

Anyway, this unknown state looks to be a good mitigation to this
underlying problem. I'm good with it.

> While we're playing with the direction flag in/out defines, rename them to
> a more descriptive FLAG_DIR_* format.
>
> Cc: Alek Du <alek.du@xxxxxxxxx>
> Cc: Samuel Ortiz <sameo@xxxxxxxxxxxxxxx>
> Cc: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
> Cc: Eric Miao <eric.y.miao@xxxxxxxxx>
> Cc: Uwe Kleine-K?nig <u.kleine-koenig@xxxxxxxxxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> Cc: Joe Perches <joe@xxxxxxxxxxx>
> Cc: Alan Cox <alan@xxxxxxxxxxxxxxxxxxx>
> Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
> Signed-off-by: Peter Tyser <ptyser@xxxxxxxxxxx>
> ---
> Change since V1:
> - This patch is new and is based on feedback from Alan to support a new
> Â"unknown" direction.
> - Rename FLAG_IS_OUT to FLAG_DIR_OUT, and add FLAG_DIR_IN
>
> Âdrivers/gpio/gpiolib.c | Â 82 +++++++++++++++++++++++++++--------------------
> Â1 files changed, 47 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 649550e..0113c10 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -49,13 +49,14 @@ struct gpio_desc {
>    Âunsigned long      flags;
> Â/* flag symbols are bit numbers */
> Â#define FLAG_REQUESTED 0
> -#define FLAG_IS_OUT Â Â1
> -#define FLAG_RESERVED Â2
> -#define FLAG_EXPORT Â Â3 Â Â Â /* protected by sysfs_lock */
> -#define FLAG_SYSFS Â Â 4 Â Â Â /* exported via /sys/class/gpio/control */
> -#define FLAG_TRIG_FALL 5 Â Â Â /* trigger on falling edge */
> -#define FLAG_TRIG_RISE 6 Â Â Â /* trigger on rising edge */
> -#define FLAG_ACTIVE_LOW Â Â Â Â7 Â Â Â /* sysfs value has active low */
> +#define FLAG_DIR_OUT Â 1 Â Â Â /* pin is an output */
> +#define FLAG_DIR_IN Â Â2 Â Â Â /* pin is an input */
> +#define FLAG_RESERVED Â3
> +#define FLAG_EXPORT Â Â4 Â Â Â /* protected by sysfs_lock */
> +#define FLAG_SYSFS Â Â 5 Â Â Â /* exported via /sys/class/gpio/control */
> +#define FLAG_TRIG_FALL 6 Â Â Â /* trigger on falling edge */
> +#define FLAG_TRIG_RISE 7 Â Â Â /* trigger on rising edge */
> +#define FLAG_ACTIVE_LOW Â Â Â Â8 Â Â Â /* sysfs value has active low */
>
> Â#define ID_SHIFT Â Â Â 16 Â Â Â/* add new flags before this one */
>
> @@ -220,15 +221,22 @@ static ssize_t gpio_direction_show(struct device *dev,
> Â{
> Â Â Â Âconst struct gpio_desc Â*desc = dev_get_drvdata(dev);
>    Âssize_t         status;
> +    const char       Â*dir_str;
>
> Â Â Â Âmutex_lock(&sysfs_lock);
>
> - Â Â Â if (!test_bit(FLAG_EXPORT, &desc->flags))
> + Â Â Â if (!test_bit(FLAG_EXPORT, &desc->flags)) {
> Â Â Â Â Â Â Â Âstatus = -EIO;
> - Â Â Â else
> - Â Â Â Â Â Â Â status = sprintf(buf, "%s\n",
> - Â Â Â Â Â Â Â Â Â Â Â test_bit(FLAG_IS_OUT, &desc->flags)
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ? "out" : "in");
> + Â Â Â } else {
> + Â Â Â Â Â Â Â if (test_bit(FLAG_DIR_OUT, &desc->flags))
> + Â Â Â Â Â Â Â Â Â Â Â dir_str = "out";
> + Â Â Â Â Â Â Â else if (test_bit(FLAG_DIR_IN, &desc->flags))
> + Â Â Â Â Â Â Â Â Â Â Â dir_str = "in";
> + Â Â Â Â Â Â Â else
> + Â Â Â Â Â Â Â Â Â Â Â dir_str = "unknown";
> +
> + Â Â Â Â Â Â Â status = sprintf(buf, "%s\n", dir_str);
> + Â Â Â }
>
> Â Â Â Âmutex_unlock(&sysfs_lock);
> Â Â Â Âreturn status;
> @@ -272,6 +280,9 @@ static ssize_t gpio_value_show(struct device *dev,
>
> Â Â Â Âif (!test_bit(FLAG_EXPORT, &desc->flags)) {
> Â Â Â Â Â Â Â Âstatus = -EIO;
> + Â Â Â } else if ((!test_bit(FLAG_DIR_OUT, &desc->flags)) &&
> + Â Â Â Â Â Â Â Â Â Â Â (!test_bit(FLAG_DIR_IN, &desc->flags))) {
> + Â Â Â Â Â Â Â status = -EPERM;
> Â Â Â Â} else {
> Â Â Â Â Â Â Â Âint value;
>
> @@ -297,7 +308,7 @@ static ssize_t gpio_value_store(struct device *dev,
>
> Â Â Â Âif (!test_bit(FLAG_EXPORT, &desc->flags))
> Â Â Â Â Â Â Â Âstatus = -EIO;
> - Â Â Â else if (!test_bit(FLAG_IS_OUT, &desc->flags))
> + Â Â Â else if (!test_bit(FLAG_DIR_OUT, &desc->flags))
> Â Â Â Â Â Â Â Âstatus = -EPERM;
> Â Â Â Âelse {
>        Âlong      Âvalue;
> @@ -741,7 +752,7 @@ int gpio_export(unsigned gpio, bool direction_may_change)
>
> Â Â Â Â Â Â Â Â Â Â Â Âif (!status && gpio_to_irq(gpio) >= 0
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&& (direction_may_change
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â || !test_bit(FLAG_IS_OUT,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â || test_bit(FLAG_DIR_IN,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&desc->flags)))
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstatus = device_create_file(dev,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&dev_attr_edge);
> @@ -1059,22 +1070,10 @@ int gpiochip_add(struct gpio_chip *chip)
> Â Â Â Â Â Â Â Â Â Â Â Âbreak;
> Â Â Â Â Â Â Â Â}
> Â Â Â Â}
> - Â Â Â if (status == 0) {
> - Â Â Â Â Â Â Â for (id = base; id < base + chip->ngpio; id++) {
> + Â Â Â if (status == 0)
> + Â Â Â Â Â Â Â for (id = base; id < base + chip->ngpio; id++)
> Â Â Â Â Â Â Â Â Â Â Â Âgpio_desc[id].chip = chip;
>
> - Â Â Â Â Â Â Â Â Â Â Â /* REVISIT: Âmost hardware initializes GPIOs as
> - Â Â Â Â Â Â Â Â Â Â Â Â* inputs (often with pullups enabled) so power
> - Â Â Â Â Â Â Â Â Â Â Â Â* usage is minimized. ÂLinux code should set the
> - Â Â Â Â Â Â Â Â Â Â Â Â* gpio direction first thing; but until it does,
> - Â Â Â Â Â Â Â Â Â Â Â Â* we may expose the wrong direction in sysfs.
> - Â Â Â Â Â Â Â Â Â Â Â Â*/
> - Â Â Â Â Â Â Â Â Â Â Â gpio_desc[id].flags = !chip->direction_input
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ? (1 << FLAG_IS_OUT)
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â : 0;
> - Â Â Â Â Â Â Â }
> - Â Â Â }
> -
> Â Â Â Âof_gpiochip_add(chip);
>
> Âunlock:
> @@ -1402,8 +1401,10 @@ int gpio_direction_input(unsigned gpio)
> Â Â Â Â}
>
> Â Â Â Âstatus = chip->direction_input(chip, gpio);
> - Â Â Â if (status == 0)
> - Â Â Â Â Â Â Â clear_bit(FLAG_IS_OUT, &desc->flags);
> + Â Â Â if (status == 0) {
> + Â Â Â Â Â Â Â clear_bit(FLAG_DIR_OUT, &desc->flags);
> + Â Â Â Â Â Â Â set_bit(FLAG_DIR_IN, &desc->flags);
> + Â Â Â }
> Âlose:
> Â Â Â Âreturn status;
> Âfail:
> @@ -1455,8 +1456,10 @@ int gpio_direction_output(unsigned gpio, int value)
> Â Â Â Â}
>
> Â Â Â Âstatus = chip->direction_output(chip, gpio, value);
> - Â Â Â if (status == 0)
> - Â Â Â Â Â Â Â set_bit(FLAG_IS_OUT, &desc->flags);
> + Â Â Â if (status == 0) {
> + Â Â Â Â Â Â Â clear_bit(FLAG_DIR_IN, &desc->flags);
> + Â Â Â Â Â Â Â set_bit(FLAG_DIR_OUT, &desc->flags);
> + Â Â Â }
> Âlose:
> Â Â Â Âreturn status;
> Âfail:
> @@ -1643,21 +1646,27 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>    Âunsigned        Âi;
>    Âunsigned        Âgpio = chip->base;
>    Âstruct gpio_desc    Â*gdesc = &gpio_desc[gpio];
> -    int           is_out;
> +    const char       Â*dir_str;
> +    int           unknown = 0;
>
> Â Â Â Âfor (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
> Â Â Â Â Â Â Â Âif (!test_bit(FLAG_REQUESTED, &gdesc->flags))
> Â Â Â Â Â Â Â Â Â Â Â Âcontinue;
>
> - Â Â Â Â Â Â Â is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
> - Â Â Â Â Â Â Â seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
> - Â Â Â Â Â Â Â Â Â Â Â gpio, gdesc->label,
> - Â Â Â Â Â Â Â Â Â Â Â is_out ? "out" : "in ",
> - Â Â Â Â Â Â Â Â Â Â Â chip->get
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ? (chip->get(chip, i) ? "hi" : "lo")
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â : "? Â");
> + Â Â Â Â Â Â Â if (test_bit(FLAG_DIR_IN, &gdesc->flags)) {
> + Â Â Â Â Â Â Â Â Â Â Â dir_str = "in ";
> + Â Â Â Â Â Â Â } else if (test_bit(FLAG_DIR_OUT, &gdesc->flags)) {
> + Â Â Â Â Â Â Â Â Â Â Â dir_str = "out";
> + Â Â Â Â Â Â Â } else {
> + Â Â Â Â Â Â Â Â Â Â Â dir_str = "? Â";
> + Â Â Â Â Â Â Â Â Â Â Â unknown = 1;
> + Â Â Â Â Â Â Â }
> +
> + Â Â Â Â Â Â Â seq_printf(s, " gpio-%-3d (%-20.20s) %s %s", gpio, gdesc->label,
> + Â Â Â Â Â Â Â Â Â Â Â dir_str, (chip->get && !unknown) ?
> + Â Â Â Â Â Â Â Â Â Â Â (chip->get(chip, i) ? "hi" : "lo") : "?");
>
> - Â Â Â Â Â Â Â if (!is_out) {
> + Â Â Â Â Â Â Â if (test_bit(FLAG_DIR_IN, &gdesc->flags)) {
>            Âint       irq = gpio_to_irq(gpio);
> Â Â Â Â Â Â Â Â Â Â Â Âstruct irq_desc *desc = irq_to_desc(irq);
>
> --
> 1.7.0.4
>
>
N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i