Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752591Ab1BQHeG (ORCPT ); Thu, 17 Feb 2011 02:34:06 -0500 Received: from mail-iy0-f174.google.com ([209.85.210.174]:63142 "EHLO mail-iy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752072Ab1BQHeD (ORCPT ); Thu, 17 Feb 2011 02:34:03 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; b=FiSJyjIJk/fbcxsFWAKLNSszgijqXQ2IFREC2d0rziuVnJofWeZ/nTFlf3GxSQBbO1 pvXLXuIAulTdtypRjuWc+iRixnjv799mbMHg5lUWSVf8DBh1+PY++tBFUUOe1v+6jMAF Bl+/hZhOsO5lx7wgOV4yggP2rCCsyiq0XJSM0= MIME-Version: 1.0 In-Reply-To: <1297904216-15219-1-git-send-email-ptyser@xes-inc.com> References: <1297904216-15219-1-git-send-email-ptyser@xes-inc.com> From: Eric Miao Date: Thu, 17 Feb 2011 15:33:42 +0800 Message-ID: Subject: Re: [PATCH v2 1/4] gpiolib: Add "unknown" direction support To: Peter Tyser Cc: linux-kernel@vger.kernel.org, Alek Du , Samuel Ortiz , David Brownell , "Uwe Kleine-K?nig" , Mark Brown , Joe Perches , Alan Cox , Grant Likely Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id p1H7YGYp007980 Content-Length: 10210 Lines: 216 On Thu, Feb 17, 2011 at 8:56 AM, Peter Tyser 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 > Cc: Samuel Ortiz > Cc: David Brownell > Cc: Eric Miao > Cc: Uwe Kleine-K?nig > Cc: Mark Brown > Cc: Joe Perches > Cc: Alan Cox > Cc: Grant Likely > Signed-off-by: Peter Tyser > --- > 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?