Message-ID: <357046193.20070411021107@gmail.com>
Date:	Wed, 11 Apr 2007 02:11:07 +0300
From:	Paul Sokolovsky <pmiscml@...il.com>
To:	David Brownell <david-b@...bell.net>
CC:	linux-arm-kernel@...ts.arm.linux.org.uk,
	linux-kernel@...r.kernel.org
Subject: Re: is there any generic GPIO chip framework like IRQ chips?

Hello David,

Monday, April 9, 2007, 11:22:25 PM, you wrote:

> On Monday 09 April 2007 10:18 am, Paul Sokolovsky wrote:

>> > Nobody who really wants to have such an implementation framework has yet
>> > ponied up and done the work to make one.
>> 
>>   Well, sorry if it wasn't made clear, but we (handhelds.org) made such
>> an implementation. It is in turn based on low-overhead and flexible
>> patterns of object-oriented virtual methods. It is presented here:
>> http://lkml.org/lkml/2007/3/27/63

> There's a lot of precedent for how to abstract methods in kernel code,
> and unfortunately that approach didn't follow *ANY* of it.  Get rid of
> the VTABLE() and METHOD() macros.  Stop thinking in C++ for kernel code!

  Fixed.


>> 1. Extended API header (uses "gpio_dev" (to be renamed to "gpiodev")
>> prefix instead of "gpio"; such API is intended to be used side by side
>> with your implementation, as the latter has important feature of
>> reducing to direct GPIo register access in case of constant GPIO#).
>> http://handhelds.org/cgi-bin/cvsweb.cgi/linux/kernel26/include/linux/gpio_dev.h?rev=1.2&content-type=text/x-cvsweb-markup

> VTABLE() and METHOD() ... you were already told no way to such
> "crappage".  That's makes reviewing the rest pointless...

  Fixed. Thanks for not stopping on the first parse error ;-)

> Now, translated to more traditional approaches, and looking
> more like I was first thinking about rather than the IDR-based
> thing in

>         http://lkml.org/lkml/2007/1/1/87

[]

>         /* defined by the platform using array, if/else/..., IDR, or whatever */
>         struct gpio_yyy *gpio_to_yyy(unsigned gpio);

   I assume by "platform" you mean CPU architecture. So, well... It
indeed must be very flexible, scalable, maintainable, and completely
not error-prone to let each platform reimplement wheel in its own
special way. For example, say, ASIC3 appears in both PXA and S3Cxxx
devices. Suppose driver which uses ASIC3 works well on PXA, but acts up
on S3Cxxx. Reason? Just because PXA uses "IDR" for its "gpio_to_yyy",
while S3Cxxx uses "whatever".

   But here's a point which made me to cleanup patches and resubmit
them again, and continue this discussion (because I really don't feel
like fighting against the tide) - I read "Re: Any possible to split
the pxa-regs.h?" thread, and see some fresh trend in it. So, let me
continue this and ask: how GPIO handling relates to CPU architecture
at all (and that's what I assume you mean by "platform")? The answer:
it does not.

   We for long time took for granted that the whole system world spins
around plastic package with label "CPU". Apparently, that's wrong, it
spins only around the CPU core which happens to be in that plastic.
Anything else in that plastic is just that - external devices. So sorry,
why do we need to define external device access in terms of what CPU
controls them? Maybe it makes sense to define that access just in the same
way as for any other external device - using a driver and a framework
a driver should follow? (And yes, plain vanilla LDM is not enough here,
a minuscule extension needs to be applied.)

> then you could do something like this (maybe passing "private" not "yyy"):

>         int gpio_get_value(unsigned gpio)
>         {
>                 struct gpio_yyy *yyy = gpio_to_yyy(gpio);

>                 return yyy->get(yyy, gpio - base);

                        ^^^ this subtraction makes me wonder what
exactly it does here - anything useful, or ... ?

>                 /* where get(yyy, offset) might look like:
>                  *      u32 mask = 1 << offset;
>                  *      return mask & __raw_readl(yyy->private + GPIO_PIN_READ);
>                  */
>         }
>                 
[]

> ... of course, those two might also be wrapped to understand that 
> the first N gpios could become inlined accesses to the relevant
> platform registers when "gpio" is constant ...

  Why first N could be inlined? What if I need second N inlined?
Here, PXA GPIOs have really low-frequency stuff, while ASIC3 does
bitbanging, so that's what should be inlined. And on system X, GPIO
chips #1, #3, #4, #7 needs to be inlined, while the rest could be not.

  So, why don't we handle this in following way: have two different
levels of API: one which is concerned with inlinability (as noone
disagrees this is important feature), and another - with generality
and extensibility? That's why gpiodev API proposed to be on top of
gpio API.


[]

> Fair enough?  You were making a few other implementation choices
> too,

  Yes, that's true. There're always some choices and compromises
among different requirements, but I and other handhelds.org developers
argue that the proposed "gpiodev" is more scalable.

> especially for the "gpiodev" thingies which I dropped here
> in the interest of having exactly ONE struct gpio_yyy per controller,
> instead of one per GPIO,

  This is not correct presentation. Comparing your per-contoller
struct's with my per-gpio structs are comparing apples and oranges.
GPIODEV just uses structured id's instead of scalar, that's all.
And they have it because that allows to automagically solve many
issues your approach outlined above solves "manually".

> since that's not only less expensive but

  In GPIO API, GPIO id's are integers, 4 bytes on 32bit platform,
passed by value. In GPIODEV API, GPIO id's are fixed-size structures,
8 bytes, passed by reference, 4 bytes, so the same overhead.

  In GPIODEV API, to execute operation, you take one member of ID,
treat that as structure, take pointer from it, and call function by
that pointer, passing second member as is.

  With your approach above, you do lookups or even expensive linear
searches just to get idea of what code should process the operation,
then offset gpio id by some value before passing it to that code.

  Sorry, but what's less expensive here?

> it's also a direct match to what the underlying code needs.

  Apparently just the opposite - GPIODEV's data structures have what
the underlying code needs right away, while you need to do some magic
on it.


  So, the code you propose goes to strange, and as was shown,
superfluous things like first requiring adding some value to gpio#,
then looping/looking up value to subtract from it, etc., etc. Why?
There doesn't seem to be any technical reason for that. The reason it
seems to be doing that is paradigmatic: to pretend that the world is
flat, and that GPIO's stick not out of specific chips, but out of the
whole system, all lined up in a row. Proposal: let's leave such outlook
behind, accept that world is 3D, and have bunch of problems solved for
us automagically (and what's not less important, such new outlook can
be reused for few other problems too).


>> 4. Example of a driver taking advantage of gpiodev API: simple shim to
>> extend pxa2xx_udc driver to arbitrary GPIOs (not only those builtin to
>> PXA):
>> http://handhelds.org/cgi-bin/cvsweb.cgi/linux/kernel26/drivers/usb/gadget/pxa2xx_udc_gpio.c?rev=1.1&content-type=text/x-cvsweb-markup&f=h
>> (note that it makes use of simple superstructure on top of gpiodev API,
>> and abstraction of power-controlling GPIO:
>> http://handhelds.org/cgi-bin/cvsweb.cgi/linux/kernel26/include/linux/dpm.h?rev=1.3&content-type=text/x-cvsweb-markup&f=h
>> )

> But there are already patches in the queue to update that UDC to use the
> generic GPIO interface ...

  Wonderful. It only means that patches for generic GPIODEV interface
can be done a bit later ;-).

> the delay merging the asm-arm/arch-ixp4xx code
> seems to be the main holdup.  At this point I don't much want to muddy
> those waters any furher, it's taken long enough to integrate the ixp4xx
> fixes.  (And if I'm getting impatient, I hate to think about what Milan
> must be feeling!)

  I of course have no intention to delay already existing patches, or
anything. I'd be glad if there was any runtime-extensible GPIO API at
all. I just happen to get a feeling that there can be gained momentum
to resolve some issues of ARM Linux (and few of Linux at all), so just
hope that discussion and proposed implementation can be found useful.

> - Dave



-- 
Best regards,
 Paul                            mailto:pmiscml@...il.com

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists