Re: [PATCH v5 2/4] gpiolib: Add ability to get GPIO direction

[Peter Tyser <ptyser@xes-inc.com>]

On Mon, 2011-03-07 at 00:08 -0700, Grant Likely wrote:
> On Sun, Mar 06, 2011 at 09:07:21PM -0600, Peter Tyser wrote:
> > On Sun, 2011-03-06 at 00:30 -0700, Grant Likely wrote:
> > > On Tue, Mar 01, 2011 at 05:28:18PM -0600, Peter Tyser wrote:
> > > > Add a new get_direction() function to the gpio_chip structure.  This is
> > > > useful so that the direction of a GPIO can be determined when its
> > > > initially exported.  Previously, the direction defaulted to "unknown"
> > > > regardless of the actual configuration of the GPIO.
> > > > 
> > > > If a GPIO driver implements get_direction(), it is called in
> > > > gpio_request() to set the initial direction of the GPIO accurately.
> > > 
> > > I don't like this approach.  It effectively creates two methods for
> > > determining the direction of a gpio; either ask the driver, or read
> > > the flags value.  Currently, only gpio_request() actually uses the
> > > first option, but it still leaves the ambiguity.
> > > 
> > > Instead, the driver should be able to preload the direction flag at or
> > > shortly after gpiochip_add() time.  No need for a new callback hook
> > > from what I can see.
> > 
> > The callback hook was used because the gpio_desc[] structure was local
> > to gpiolib.c.  The code would have to be restructured a bit to allow
> > drivers to set the flags themselves.  I can do that if you'd prefer, but
> > don't think it'd be all that much cleaner.
> 
> Make it part of the gpiochip_add function, or add a function for
> explicitly setting gpio directions.  Should not require any
> reorganization at all.
> 
> > Also, I thought it made sense to read the direction of the GPIO in
> > gpio_request() because that's the point that the GPIO comes under the
> > GPIO subsystem's control.  Prior to that, there's a chance the direction
> > could be changed by platform code, or another driver, etc, so reading
> > the direction in gpiochip_add() may result in out-of-date directions.
> 
> That's the problem with gpiolib having sole responsibility of the
> cache.  It doesn't give the driver any recourse for changing things if
> it needs to.  The best solution might very well be to eliminate the
> direction state flag entirely and force all gpiochip drivers to
> populate a gpio_get_direction() callback, but that is a lot more work.
> 
> > The reading of the direction could also be put in chip drivers'
> > request() function.  That would get rid of the callback and still ensure
> > the direction is up-to-date.
> 
> I don't object to a callback hook.  My objection is how it is bodged
> on to work around limitations to the direction being cached in the
> flags variable.  I want to see a solution that either depends entirely
> on the callback, or completely fixes the problems with the cached
> value by allowing the driver to update it.

I agree it'd be nice to have each driver implement a get_direction()
function and remove the FLAG_DIR_* flags altogether, but didn't want to
do the work to 20 drivers which I don't use...  Would you accept a
series that:
- Added "unknown" direction support (eg patch 1)
- Added ability to get GPIO direction (eg patch 2 in this series)
- Sequence of patches to add get_direction() to all GPIO drivers (most
of which I couldn't test).
- Replace checking of FLAG_DIR_* in gpiolib.c with calls to new
get_direction() function and remove FLAG_DIR_* flags all together.


It should be relatively easy to add get_direction() to most drivers, and
I'm willing to do it if all parties would agree it was the proper
direction to take.  Or do you have any other suggestions about an easier
migration path?

Best,
Peter