Re: [RFC 2.6.27 1/1] gpiolib: add support for batch set of pins

On Sunday 30 November 2008, Jaya Kumar wrote:

quoted

The gpio_*() interfaces are how system glue code and drivers
access GPIOs, unless they roll their own chip-specific calls.

Whereas gpiolib (and gpio_chip) are an *optional* framework
for implementing those calls.  Platforms can (and do!) use
other frameworks ... maybe they don't want to pay its costs,
and don't need the various GPIO expander drivers.

So to repeat:  don't assume the interface is implemented in
one particular way (using gpiolib).  It has to make sense
with other implementation strategies too.  Standard split
between interface and implementation, that's all.  (Some folk
have been heard to talk about "gpiolib" as the interface to
drivers ... it's not, it's a provider-only interface library.)

Okay, I read above several times and I read Documentation/gpio.txt.
But... I'm not confident I've understood your meanings above in terms
of how it should change the code. What I know so far is:

a)
arch/arm/mach-pxa/include/mach/gpio.h is where the pxa GPIO api
interface is defined.

It's actually where the *implementation* is *declared*.  The
API is defined primarily in Documentation/gpio.txt ... though
in this case the implementation mostly punts to gpiolib.  And
as you note below, the implementation is *defined* elsewhere.

So that's where I will put the 
gpio_get/set_values_batch functions. This will match the existing
gpio_set/get_value code there.

Those functions just punt to gpiolib, unlesss they happen
to fit in the "constant parameters" case where they can
expend to two or three instructions inline, rather than a
more expensive function call.

In this case, I don't see any benefit to supporting that
"inline constant parameters" case.

 

b)
arch/arm/mach-pxa/gpio.c is where the implementation is.
So I'll put the pxa_gpio_direction_input/output_batch and
pxa_gpio_set/get_batch there.

This is where the PXA implementation is *defined*, yes.

 

c)
drivers/gpio/gpiolib_batch.c

This is where I'd put the generic platform independent implementations
of __gpio_set/get_values_batch

Does this sound consistent with your recommendation? If not, I need
more help to understand what changes you are recommending.

More or less, yes.  I'd have to see actual code.  :)

quoted

Doesn't necessarily need to be *you* doing that, but if it only
works on older gumstix boards it'd not be general enough.  Which
is part of why I say to get the lowest level proposal out there
first.  If done right, it'll be easy to support on other chips.

Yes, I completely agree that it must work on a wide range of
architectures. But I don't understand what you mean when you say get
the lowest level proposal out there first.

Provide a patch/RFD with

 (0) methods you want to add to "struct gpio_chip".

Nothing else.  That should be the easiest part of this change.

Other patches should probably include:

 (1) one with the proposed driver level interface ... needs
     discussion, so beginning with a patch may not be best;

 (2) one *implementing* that interface using current calls;

 (3) another *optimizing* that interface to use the new low
     level methods (0), on chips where they're available;

 (4) gpio_chip implementations for those low level ops.

Plus at least one example of how to use the interfaces in (1),
by the time everything starts to become solid, and implementation
patches are worth while.

(I'm not keen on starting with code to implement interfaces, except
when the interfaces are trivial or obvious.  Interfaces are hard to
change.  It's best to spend some time up-front improving them, while
they're easy to change/fix.)

I see the RFC code that I 
posted as being the lowest level proposal (albeit needing better name
and bitmask support) and one that is sufficiently general that it can
be implemented on other architectures. 

To repeat:  what you've sent so far is mixing layers.  It doesn't split
out the programming interface from its implementation at all.

Yet the whole *point* of having gpiolib is to make that split easy.

Which is why I want to see patches that reflect that architectural
split, instead of a hard-to-review megapatch that crosses all the
existing boundaries and doesn't show how non-PXA hardware could
implement these patches (or benefit from them).

- Dave