Re: is there any generic GPIO chip framework like IRQ chips?

Re: is there any generic GPIO chip framework like IRQ chips?

[Paul Sokolovsky]

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@gmail.com

Re: is there any generic GPIO chip framework like IRQ chips?

[David Brownell]

On Tuesday 10 April 2007 4:11 pm, Paul Sokolovsky wrote:
> 
> >         /* 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.

Nope.  ARM (v4, v5, v6, etc) is a CPU architecture.  "Platform" is
fuzzily defined but it's more than the CPU.  It's a "family" notion
that probably ties better to SOC chip vendor and branding ... things
work differently on TI OMAP processors than on Intel IOP ones, or than
the PXA ones (originally from Intel).  For many folk, "platform" is a
board-level notion ... like "x86_PC with ACPI firmware".


>  	 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.

The thing to do with bugs is fix them.  If S3C were to have a failure
at that level, there'd be a lot more failures than just that one.


>   		 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.

Certainly not ... and the interface doesn't.  Any more than IRQs do.
Implementations most certainly can be platform-specific.  And they
usually are, of necessity (different controllers, etc).


So, talking about what an (optional) implementation framework might
look like (and which could handle the SOC, FPGA, I2C, and MFD cases
I've looked at):

> > 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 ... ?

See the next lines:

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

Each GPIO controller manages no more than a few pins at a time,
normally as many as fit into one register.


> >         }
> >                 
> []
> 
> > ... 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,

What's being bit-banged?  I2C isn't speed-critical.  SPI can be.

My opinion on the inlining is that the API should _allow_ that as
a source-compatible optimization, for cases where e.g. 1 Mbit/sec
isn't good enough but 2Mbit/sec is.  Luckily that optimization (to
make GPIO access into about three instructions, no subroutining, if
the particular GPIO is known at compile time) can be very easy.

GPIOs are a really light weight hardware mechanism, and should never
become fat'n'heavy.  Remember:  GPIO ~= Bit.  Keep it simple.

If a given implementation doesn't implement inline optimization,
some drivers may observe suckage but it probably won't matter for
most cases.  However, if an interface doesn't *ever* allow that
optimization, that's a sign of something wrong.


> 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 you're arguing about what N is.  That's obviously an implementation
concern, nothing to do with any decent interface.

It happens to be especially easy to ensure that if some GPIO controllers
are always there (e.g. ones built into the platform, like the ones on a
PXA SOC) then you can inline access to them .. no abstraction functions
necessary.   And it's impractical to inline when you can't guarantee the
same controller is always there.  (Absent altinstr style patching ...
which level of effort is not justifiable here.)


>   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.

By analogy, irq_chip is not part of the driver API, so gpio_chip
doesn't need to be either.  All that's *necessary* in the API is
a single level.

If there's to be a gpio_chip, it can go neatly _below_ the public
interface.  I think you didn't notice that what I sketched closely
resembles the innards of most GPIO implementations in the kernel,
even those not yet supporting the gpio_*() interfaces...


>   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.

No; for N GPIOs, it's the difference of N*8 additional bytes per
GPIO versus zero additonal bytes.  (It's fair to ignore costs of
the handles, since everyone pays that same cost.)

Note also that your stuff has been omitting large chunks of the
interface ... IRQ support, directionality, request/free, etc.

Handling an incoming GPIO IRQ (identified by an integer) involves
operations which are direct analogues of the ones you suggest are
too costly when applied to GPIOs not being viewed as IRQ sources.
That "costly" argument thus looks rather dubious to me ...


>   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.

Let me rephrase that in a way I think clarifies things ... and adds
some of the context you've omitted:

 - What I sketched was (a) a "gpio controller" abstraction, plus
   mapping of GPIO numbers to (b) controller, e.g. "yyy[gpio >> 5]"
   and (c) controller-specific gpio index, e.g. "gpio & 0x1f";

   That's how people have managed GPIOs on Linux for many years now.
   It plays well with the very common use of GPIOs as IRQ sources.

   Usually the "gpio controller" is a register bank on a SOC, but as
   I've pointed out, that could be generalized through the classic
   "another level of indirection" technology (not patentable) at
   the cost of a new mandatory indirect function call.

 - What you sketched essentially says "let's save results (b) and (c)
   together in a structure and use that instead of GPIO numbers",
   with the mandatory indirect function call.  Also "let's define
   a new programming interface in terms of that struct".

   And you've not discussed the IRQ side of the equation, or
   the cansleep/irqsafe issues ... your example code built on
   top of code that uses the integer IDs.

Note that I'm giving you the benefit of the doubt in several respects
there.  Your original (a) was quite bizarre, and your updated one isn't
a lot better because it still needs odd conversions.


>   Sorry, but what's less expensive here?

In terms of data space required, clearly the scheme I described; it
doesn't need the 8*N bytes per GPIO.  In terms of instruction count,
they're comparable ... I think what I described clearly has an edge
because of all the conversions needed in yours, but that'll probably
clean up so it's not different enough to matter (given they both
need indirect function calls).  In terms of API churn, again the
scheme I described:  no new API needed by drivers, and things can
easily be made to match chip specs (which mostly use integers).


> > 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.

See above.  The code implementing the GPIO controllers would more or
less be the same in both cases.  

> 
>   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?

Because you're constructing a straw man, to make what I'm saying
look less like a codification of existing practice than it is.  :)
(Or so it seems to me.)


> 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. 

Remember that IRQs do in fact "stick out of the whole system, all lined
up in a row".  And oddly enough, so must GPIOs used as IRQs...  Strange,
eh?  Go figure.

- Dave

Re: is there any generic GPIO chip framework like IRQ chips?

[Paul Sokolovsky]

Hello David,

Wednesday, April 11, 2007, 7:52:20 AM, you wrote:

> On Tuesday 10 April 2007 4:11 pm, Paul Sokolovsky wrote:
>> 
>> >         /* 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.

> Nope.  ARM (v4, v5, v6, etc) is a CPU architecture.  "Platform" is
> fuzzily defined but it's more than the CPU.  It's a "family" notion
> that probably ties better to SOC chip vendor and branding ... things
> work differently on TI OMAP processors than on Intel IOP ones, or than
> the PXA ones (originally from Intel).  For many folk, "platform" is a
> board-level notion ... like "x86_PC with ACPI firmware".


>>        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.

> The thing to do with bugs is fix them.  If S3C were to have a failure
> at that level, there'd be a lot more failures than just that one.

  Programmers fix bugs. System designers make systems without
inclination to have bugs at random places. So, let's try to do the
former ;-) (in the sense, let's try to make a framework which will be
devoid of possibility for such variations; note that this doesn't mean
that framework which allows them, but has other benefits, is *not*
outruled by this).

>>                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.

> Certainly not ... and the interface doesn't.  Any more than IRQs do.
> Implementations most certainly can be platform-specific.  And they
> usually are, of necessity (different controllers, etc).


> So, talking about what an (optional) implementation framework might
> look like (and which could handle the SOC, FPGA, I2C, and MFD cases
> I've looked at):

>> > 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 ... ?

> See the next lines:

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

> Each GPIO controller manages no more than a few pins at a time,
> normally as many as fit into one register.

  Thanks for describing this once again. But I really looked into your
January's code and this code already. And I fully agree that this will
work! And your code could have been long ago in git, but is not, so I
assume that your January's code was an RFC, asking what people think,
and if there're any alternatives. So, we here present exactly an
alternative, and it has its technical merits, I hope it's visible.


>> >         }
>> >                 
>> []
>> 
>> > ... 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,

> What's being bit-banged?  I2C isn't speed-critical.  SPI can be.

  Well, not me started to spoke about bit-banging. It's obviously just
one of usecases for GPIO API.

> My opinion on the inlining is that the API should _allow_ that as
> a source-compatible optimization, for cases where e.g. 1 Mbit/sec
> isn't good enough but 2Mbit/sec is.

  Exactly! For runtime-extensible GPIO API, speed is apparently not
top-level goal, as - well, extensibility is. But if can shave few
cycles off by ditching table lookups and subtraction, let's do that.
For someone, being able to bit-bang on 1.2MHz instead of 1MHz can be
real life-saver.

> Luckily that optimization (to
> make GPIO access into about three instructions, no subroutining, if
> the particular GPIO is known at compile time) can be very easy.

> GPIOs are a really light weight hardware mechanism, and should never
> become fat'n'heavy.  Remember:  GPIO ~= Bit.  Keep it simple.

> If a given implementation doesn't implement inline optimization,
> some drivers may observe suckage but it probably won't matter for
> most cases.  However, if an interface doesn't *ever* allow that
> optimization, that's a sign of something wrong.

  Also fully agree with these passages! But let's exactly keep it
simple and untangled. If your API motivates people to do adhoc
optimizations (by creating per-platform header-file based
inline implementations for example), let's keep it that way, clean and
nice, and don't try to mix it with *runtime* extensibility. As was
pointed out, these two requirement are opposite to some extent, and
trying to merry them can just lead to mess.

>> 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 you're arguing about what N is.  That's obviously an implementation
> concern, nothing to do with any decent interface.

  Yeah! We've been talking about implementation for quite some time
now ;-). And my point in writing the above was that it's not going to be
too easy and clean to stuff all the requirements into one implementation.

  That's why, again, GPIODEV does *not* try to supercede Generic GPIO
API, but instead extends (on the meta-interface level, i.e. method
signature change (but in consistent manner), but semantics stays).

> It happens to be especially easy to ensure that if some GPIO controllers
> are always there (e.g. ones built into the platform, like the ones on a
> PXA SOC) then you can inline access to them .. no abstraction functions
> necessary.   And it's impractical to inline when you can't guarantee the
> same controller is always there.  (Absent altinstr style patching ...
> which level of effort is not justifiable here.)

  Yes, and let that's something which can be entailed from your
approach: you argue that it's extensible interface, but kernel is
exactly not C++ classlib, there must be implementation. And that
implementation: a) will contain inline access for CPU SOC GPIO
controllers, just because "they are always there"; b) will not contain
anything else, just because "it's not always there". That's exactly
what implementation will go into 2.6.21.

  Yes, you argue that anyone who needs to extend Generic GPIO API to
their case can do that, and in such way that he won't lose single tick
comparing to using direct low-level methods. That's trait is *much*
appreciated. Actually, so much, that I don't even try to mess up
beauty of it by stuffing implementation for other requirement right
into it.

  But let's face it - most people will not hack mainline-provided
inlinable headers to add efficient handling for their boards'
additional GPIO, or create a new platform, to do that right. Many
people just need to plug in their adhoc GPIOs into system in such way
that it will be used by a generic driver. And they don't want to hack
sacred kernel headers for that. They are not caught by phrases "you
can do it this way, or you can do it that way - just bother to do it".
They need single, and not needlessly inefficient API to register their
GPIO chips at runtime and have them run right away, without thinking
how to snatch extra CPU cycle by constant optimization.

  So, we here think how to give them such ability, and yet not burden
the people with extra cycles than needed (even if those are few
cycles).


>>   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.

> By analogy, irq_chip is not part of the driver API, so gpio_chip
> doesn't need to be either.

  Wonderful. 2 things out of this however:

1) Just consider someone submits non irq_chip implementation of
multiplexed IRQ handling. I bet you will be first to teach one to
stop disfollowing patterns ;-). But we obviously know that few
people will bother anyway - irq_chip *is* implementation pattern,
and it makes no sense to not follow it.

2) Thankfully, we don't have gpio_chip yet. The temptation to clone
irq_chip, irq_desc, and stuff is high. So, what we talk here about is
not only how to implement extensible GPIO, but whether we finally can
break away from need to use in-kernel bureaucracies for handling each
and every (low-level) object. Proposed GPIODEV implementation shows
that it's possible. You say that irq_chip is not part of driver
interface. GPIODEV doesn't have gpio_chip/gpio_desc even as part
of the implementation!


>   All that's *necessary* in the API is
> a single level.

> If there's to be a gpio_chip, it can go neatly _below_ the public
> interface.  I think you didn't notice that what I sketched closely
> resembles the innards of most GPIO implementations in the kernel,
> even those not yet supporting the gpio_*() interfaces...

  Taking into account that GPIO is terribly simple thing, no wonder
all implementations are similar. What we lack is exactly common
interfaces. As argued, apparently, interfaces of different levels,
serving different requirements.


>>   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.

> No; for N GPIOs, it's the difference of N*8 additional bytes per
> GPIO versus zero additonal bytes.  (It's fair to ignore costs of
> the handles, since everyone pays that same cost.)

  Well, that's well-known time vs space tradeoff. And yes,
for GPIODEV, after the first requirement of runtime-extensibility,
the second requirement is speed.

> Note also that your stuff has been omitting large chunks of the
> interface ... IRQ support, directionality, request/free, etc.

  IRQ suport is there - "to_irq" method. Also, as GPIODEV is
implemented via pattern/meta-API (virtual methods), and new operation
can be added easily and cleanly, to be handled only for those devices,
which need it.

> Handling an incoming GPIO IRQ (identified by an integer) involves
> operations which are direct analogues of the ones you suggest are
> too costly when applied to GPIOs not being viewed as IRQ sources.
> That "costly" argument thus looks rather dubious to me ...

  I propose GPIO implementation here. IRQs are a bit different things,
after all. I don't touch their implementation here. But argument "IRQs
do extra operations, so let's do the same for GPIOs" is rather dubious
either.


>>   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.

> Let me rephrase that in a way I think clarifies things ... and adds
> some of the context you've omitted:

>  - What I sketched was (a) a "gpio controller" abstraction, plus
>    mapping of GPIO numbers to (b) controller, e.g. "yyy[gpio >> 5]"
>    and (c) controller-specific gpio index, e.g. "gpio & 0x1f";

>    That's how people have managed GPIOs on Linux for many years now.
>    It plays well with the very common use of GPIOs as IRQ sources.

>    Usually the "gpio controller" is a register bank on a SOC, but as
>    I've pointed out, that could be generalized through the classic
>    "another level of indirection" technology (not patentable) at
>    the cost of a new mandatory indirect function call.

>  - What you sketched essentially says "let's save results (b) and (c)
>    together in a structure and use that instead of GPIO numbers",
>    with the mandatory indirect function call.

     Yes. Except not "save", but use from the beginning, as "master"
identifiers.

>  Also "let's define
>    a new programming interface in terms of that struct".

     As was pointed already, nothing new in Linux kernel. Method
tables in the form of *_ops structures are used since the beginning of
times. It's just my IMHO, that they underused to solve some kind of
problems, so I make noise about that, but it's all mundane under the
hood.

>    And you've not discussed the IRQ side of the equation,

     There's an to_irq() method which accepts GPIO identifier in GPIO
namespace (struct gpio) and returns IRQ identifier in IRQ namespace
(integer). What can be more easy?

>  or
>    the cansleep/irqsafe issues ...

     If they are handled in your implementation, they can be handled
in GPIODEV too. They can be either fully delegated to GPIO device
drivers (i.e. there're gpiodev_*_cansleep() which just call method in
gpiodev_ops directly, or if you insist that there's some commonality,
gpiodev_*_cansleep() can do something themselves).

> your example code built on
>    top of code that uses the integer IDs.

> Note that I'm giving you the benefit of the doubt in several respects
> there.  Your original (a) was quite bizarre, and your updated one isn't
> a lot better because it still needs odd conversions.

  Well, that's why I used macro initially - to assign some sense to
those low-level typecasts. You and other people told macros must get
lost, so they are, and I guess, it's unfair now to say that typecast
sores you eye - I tried to save everyone from that. But in the end,
it's C, not me, or GPIODEV. I can rewrite it in asm, and there won't
be typecasts. But we know why we don't write in asm ;-).


>>   Sorry, but what's less expensive here?

> In terms of data space required, clearly the scheme I described; it
> doesn't need the 8*N bytes per GPIO.

  Well, but it needs some external table. Unless you want to burden
clients with management of that table, it needs to be allocated
conservatively big enough. Then, on typical not GPIO-abound system it
will be sparse, and large part of what you win in shorted GPIO ids
will be lost in this sparsity.

> In terms of instruction count,
> they're comparable ... I think what I described clearly has an edge
> because of all the conversions needed in yours, but that'll probably
> clean up so it's not different enough to matter

  Not probably, but sure - there's no machine code behind casting
pointer of one type to another type (oh, I just expect examples of
braindead archs for which this is not true ;-) ).

  Also, let me take a pun on "conversion" - so, both your and my
approach needs conversion, mine just uses static typecast, and yours
employs the whole lookup table for that ;-).

> (given they both
> need indirect function calls).  In terms of API churn, again the
> scheme I described:  no new API needed by drivers,

  That's why I prefer term "API level". GPIODEV introduces only new
level of API, not the whole new API. Users will choose level to use
based on requirements. For example, truly generic drivers will use
GPIODEV level, platform-specific drivers will use GPIO level, and
finally, drivers which have some critical requirements, will still use
"API of zeroth level", i.e. direct access to platform registers.

>  and things can
> easily be made to match chip specs (which mostly use integers).

  Let's don't warp reality here ;-). GPIODEV *does* use integers for
GPIO numbers. It just doesn't try to pretend that next spec continues
numbering where previous spec left ;-).


>> > 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.

> See above.  The code implementing the GPIO controllers would more or
> less be the same in both cases.

  Yes! The difference is how client calls GPIO methods.

>> 
>>   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?

> Because you're constructing a straw man, to make what I'm saying
> look less like a codification of existing practice than it is.  :)
> (Or so it seems to me.)

  No, I do not. Again, you proposed code to handle extra chips yet in
January, supposedly, as RFC. Yet in January other handhelds.org
developer discussed with you idea of using structured namespace for
GPIO. It wasn't random, we had some chore with flat namespaces for
quite some time before that, so to that time understood that there
handling (including need to write support code each time) doesn't
scale.

  We still didn't proceed to implement anything just then. And only
when we faced another problem (I mentioned it in original vtable RFC),
we saw similarities and when for coding solution. So, we had:

1. Case that we use different ADC/TS controller chips, they use
different command set and interface, but the data they produce are
essentially the same in the nature - stream of noisy coordinates,
ranging in some bounds. So, single touchscreen driver would be enough.

2. This GPIO stuff.

  They were then easily identifiable as cases of polymorphism, and we
solved them just like that, and using well-known pattern of inderect
function pointers. Combining that with structured identifiers allowed
to peal shell of the fruit, and decide, what is really required to
handle GPIOs is runtume-extensible manner, and what's only extraneous
bookkeeping code.

  And now we present this solution not only as the implementation of
extensible GPIO handling, but as a reusable solution for other similar
kind of problems. And I don't have ambition to say that it
"introduces" such pattern - at most, reinforces and popularizes.


>> 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. 

> Remember that IRQs do in fact "stick out of the whole system, all lined
> up in a row".  And oddly enough, so must GPIOs used as IRQs...  Strange,
> eh?  Go figure.

  A bit strange, indeed. But I may try to imagine why IRQs were
handled that way. IRQs are special things - they are external signals,
generated and pass thru different chips, but only CPU(s) handle them!
So, it would natural next step trying to line up them all in one table
for CPU's view.

  Note that looking deeper, IRQs numbers are not set in stone (one can
easily offset it), and for multiplexed IRQs, they are outright
virtual, as demultiplexing happens in software and any mapping can be
used (even though mostly same offsetting is). That means, that it
would be quite possible to stop thing of IRQ space as flat, but
instead think as structured, with nodes belonging to those
IRQ-processing chip, which gets real interrupt, not multiplexed one.

  But NO, I don't call for rewriting IRQ subsystem in such manner, as
proposed for GPIO! ;-) Let it rest in peace for some time after
Generic IRQ refactor which solved bunch of problems, so there's pretty
idyll in IRQ space now. But who knows, maybe in couple of years, when
dozen of CPUs in desktop machine will be norm, an there will be
multi-sensory systems with thousands of IRQs, maybe idea of structured
IRQ space won't be so off-shot.

  But that's clearly a trend. Systems became more complex, and
not using/ignoring structurization just would give more burden.
As an example of the trend, who would imagine something more flat,
single-dimensional, as RAM? Well, we have NUMA now. And kernel doesn't
try to pretend it's not there - there're API calls which accepts NUMA
node references, etc.

> - Dave



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

Re: is there any generic GPIO chip framework like IRQ chips?

[David Brownell]

On Thursday 12 April 2007 6:57 am, Paul Sokolovsky wrote:
> Wednesday, April 11, 2007, 7:52:20 AM, you wrote:

> > So, talking about what an (optional) implementation framework might
> > look like (and which could handle the SOC, FPGA, I2C, and MFD cases
> > I've looked at):

See patches in following messages ... a preliminary "gpio_chip" core
for such a framework, plus example support for one SOC family's GPIOs,
and then updating one board's handling of GPIOs, including over I2C.


>   Thanks for describing this once again. But I really looked into your
> January's code and this code already. And I fully agree that this will
> work! And your code could have been long ago in git, but is not, ...

Actually, no ... there was a pressing need to adopt some simple basic
interfaces, but no such need for implementation infrastructure to
support fancier features (until the basic interface was in use).
Plus, that code was barely proof-of-concept.

The fact that there are now three simple generic drivers (i2c-gpio,
leds-gpio, gpio_keys) and various additional drivers using those same
interfaces -- working already on multiple platforms! -- illustrates
that anything more than a programming interface, with support on a
handful of "seed" platforms, wasn't required initially.


>   Yes, and let that's something which can be entailed from your
> approach: you argue that it's extensible interface, but kernel is
> exactly not C++ classlib, there must be implementation.

I said "implementation choice" not "extensible interface"; those
are two very different approaches.

You keep bringing C++ into this.  You may not know that doesn't
win points in Linux.  If the point is solid, it doesn't need to
rely on parallels to languages that won't be used in the kernel.
(Plus, why would you imply C++ class libraries don't need to
have implementations?)


> And that 
> implementation: a) will contain inline access for CPU SOC GPIO
> controllers, just because "they are always there";

Not what I said.  You said "will", I said "can"; there's a big
difference.  Consider that for example the OMAP infrastructure
for GPIOs already has a fair amount of indirection, while most
other platforms started more simply ... some of them only
included inlines, others just had simple accessor functions.

My "can" allows all those; your "will" disallows most of them.

And none of them unified that model with non-SOC GPIOs.


> b) will not contain 
> anything else, just because "it's not always there". That's exactly
> what implementation will go into 2.6.21.

Again, not what I said.  The implementations there today just
wrap pre-existing GPIO code which mostly addressed SOC support
since that's the first thing that's needed.  But extensibility
has always been on the agenda.  It's just *details* of how to
extend that have been deferred ... not just by this iteration
of the interface, but by all the preceding platform-specific
ones too.


>   Yes, you argue that anyone who needs to extend Generic GPIO API to
> their case can do that, and in such way that he won't lose single tick
> comparing to using direct low-level methods. 

Again, not what I said ... I've discouraged changing/extending the
programming interface.  But I've certainly pointed out ways that
existing performance expectations can be preserved without changing
that programming interface.


>   But let's face it - most people will not hack mainline-provided
> inlinable headers to add efficient handling for their boards'
> additional GPIO,

Many folk working with embedded hardware will be more than willing
to do exactly that, should performance be a factor.  Especially if the
platform code is written so it's straightforward ... 


> 2) Thankfully, we don't have gpio_chip yet. The temptation to clone
> irq_chip, irq_desc, and stuff is high. So, what we talk here about is
> not only how to implement extensible GPIO, but whether we finally can
> break away from need to use in-kernel bureaucracies for handling each
> and every (low-level) object.

By "bureaucracy" I presume you mean the existence of opaque cookies?
Or do you mean the implied resource tracking?  (Remember that one of
the most basic tasks of an operating system is resource management...)


> > Let me rephrase that in a way I think clarifies things ... and adds
> > some of the context you've omitted:
> 
> >  - What I sketched was (a) a "gpio controller" abstraction, plus
> >    mapping of GPIO numbers to (b) controller, e.g. "yyy[gpio >> 5]"
> >    and (c) controller-specific gpio index, e.g. "gpio & 0x1f";
> 
> >    That's how people have managed GPIOs on Linux for many years now.
> >    It plays well with the very common use of GPIOs as IRQ sources.
> 
> >    Usually the "gpio controller" is a register bank on a SOC, but as
> >    I've pointed out, that could be generalized through the classic
> >    "another level of indirection" technology (not patentable) at
> >    the cost of a new mandatory indirect function call.
> 
> >  - What you sketched essentially says "let's save results (b) and (c)
> >    together in a structure and use that instead of GPIO numbers",
> >    with the mandatory indirect function call.
> 
> Yes. Except not "save", but use from the beginning, as "master"
> identifiers.
>
> >  						Also "let's define
> >    a new programming interface in terms of that struct".

So you're agreeing that, at a technical level, what I described
could be augmented by a "caching" facility ... giving a programming
interface with all the characteristics of your "GPIODEV" thingie.

All you're really disagreeing with is bootstrapping issues; and
whether there is in fact a need for such a layer.  The only argument
I could possibly buy is that it avoids the lookup of (b) ... but
that doesn't seem to matter in most cases I've looked at.


Consider the following /sys/kernel/debug/gpio dump, associated
with one system running the patches I'm posting in other followup
messages:

Controller: MPUIO, 192..207, irqs
  GPIO 193 ( 1): in  hi (  tps65010), IRQ 353/falling
  GPIO 194 ( 2): in  lo (    wakeup), IRQ 354/rising wakeup
  GPIO 196 ( 4): out hi (  idle_led)

Controller: GPIO, 0..15, irqs
  GPIO   0 ( 0): in  lo (   smc_irq), IRQ 160/rising
  GPIO   2 ( 2): out hi (  lcd_pwdn)
  GPIO   3 ( 3): out lo ( timer_led)
  GPIO   4 ( 4): in  hi (         ?), IRQ 164/falling
  GPIO   6 ( 6): in  lo (         ?)
  GPIO  11 (11): out lo (  cam_pwdn)

Controller: GPIO, 32..47, irqs
  GPIO  37 ( 5): in  lo (         ?), IRQ 197/rising wakeup

Controller: GPIO, 48..63, irqs
  GPIO  62 (14): in  hi (    cf_irq), IRQ 222/falling

Controller: tps65010, 208..213, can sleep
  GPIO 208 ( 0): out lo (  vbus_dis)
  GPIO 209 ( 1): out lo (  d3:green)
  GPIO 210 ( 2): out lo ( lan_reset)
  GPIO 211 ( 3): out lo (dsp_pwr_en)
  GPIO 212 ( 4): out lo (  d9:green)
  GPIO 213 ( 5): out lo (  d2:green)

Most of those don't get looked up in GPIO tables even once
a week after the system boots ... the exceptions being the
various LEDs, which obviously aren't performance-critical.
IRQs go another path, and the various power controls don't
change often after boot either.

FWIW, GPIO 37 is ttyS0.RXD used to wake up from sleep,
IRQ-164 is the touchscreen IRQ, and GPIO-6 is a busy signal
from the touchscreen that's not actually used.  They don't
yet use gpio_request(), which is why they're not labeled.


> > Note that I'm giving you the benefit of the doubt in several respects
> > there.  Your original (a) was quite bizarre, and your updated one isn't
> > a lot better because it still needs odd conversions.
> 
>   Well, that's why I used macro initially - to assign some sense to
> those low-level typecasts. You and other people told macros must get
> lost, so they are, and I guess, it's unfair now to say that typecast
> sores you eye - I tried to save everyone from that. But in the end,
> it's C, not me, or GPIODEV. I can rewrite it in asm, and there won't
> be typecasts. But we know why we don't write in asm ;-).

The issue I was referring to was abuse of "void *" pointers when
you should have been using typesafe ones.  And the association
with devices was wierd too.

- Dave

Re: is there any generic GPIO chip framework like IRQ chips?

[David Brownell]

This patch shows some almost-mergeable code matching $SUBJECT.

========	CUT HERE
Provide some implementation infrastructure that platforms may choose
to use when implementing the GPIO programming interface.

As previously discussed on LKML, this can be very desirable on some
platforms -- with GPIOs provided by non-SOC controllers -- but such
implementation choices are transparent to the driver interface.

When debugfs is available this provides an /sys/debug/gpio file showing
what GPIOs are available, and how the ones in use are configured.

Note that both OMAP and AVR32 implementations of the gpio_request() and
gpio_free() calls have additional features, which may be worth stealing.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 Documentation/gpio.txt     |    4 
 include/asm-generic/gpio.h |   96 +++++++++++
 lib/Kconfig                |    6 
 lib/Makefile               |    2 
 lib/gpiolib.c              |  364 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 472 insertions(+), 2 deletions(-)

--- g26.orig/Documentation/gpio.txt	2007-04-14 19:51:00.000000000 -0700
+++ g26/Documentation/gpio.txt	2007-04-14 19:51:12.000000000 -0700
@@ -63,7 +63,9 @@ and that can be critical for glue logic.
 Plus, this doesn't define an implementation framework, just an interface.
 One platform might implement it as simple inline functions accessing chip
 registers; another might implement it by delegating through abstractions
-used for several very different kinds of GPIO controller.
+used for several very different kinds of GPIO controller.  (There is some
+library code supporting such an implementation strategy, but drivers using
+the interface should not care if that's how the interface is implemented.)
 
 That said, if the convention is supported on their platform, drivers should
 use it when possible:
--- g26.orig/lib/Kconfig	2007-04-14 19:51:00.000000000 -0700
+++ g26/lib/Kconfig	2007-04-14 19:51:12.000000000 -0700
@@ -111,4 +111,10 @@ config HAS_IOPORT
 	depends on HAS_IOMEM && !NO_IOPORT
 	default y
 
+#
+# gpiolib is selected if needed
+#
+config GPIO_LIB
+	boolean
+
 endmenu
--- g26.orig/include/asm-generic/gpio.h	2007-04-14 19:51:00.000000000 -0700
+++ g26/include/asm-generic/gpio.h	2007-04-15 10:58:19.000000000 -0700
@@ -1,6 +1,101 @@
 #ifndef _ASM_GENERIC_GPIO_H
 #define _ASM_GENERIC_GPIO_H
 
+#ifdef CONFIG_GPIO_LIB
+
+/* Platforms may implement their GPIO interface with library code,
+ * at the cost of performance for non-inlined operations.
+ */
+
+#ifndef ARCH_GPIOS_PER_CHIP
+#define ARCH_GPIOS_PER_CHIP	BITS_PER_LONG
+#endif
+
+/**
+ * struct gpio_chip - abstract a GPIO controller
+ * @get: retrieves value for a given gpio signal
+ * @set: assigns value for a given gpio signal
+ * @direction_input: assigns signal as input, or returns error code
+ * @direction_output: assigns assigns signal as output, or returns error code
+ * @base: identifies the first GPIO number handled by this chip
+ * @name: for use in diagnostics
+ * @irqs: to report optional coupling to the IRQ framework
+ * @irq_base: identifies the first IRQ handled by this chip
+ * @ngpio: the number of GPIOs handled by this controller; the value must
+ *	be at most ARCH_GPIOS_PER_CHIP, so the last GPIO handled is
+ *	(base + ngpio - 1).
+ * @can_sleep: flag must be set iff get()/set() methods sleep, as they
+ *	must while accessing GPIO expander chips over I2C or SPI
+ *
+ * A gpio_chip can help platforms abstract various sources of GPIOs so
+ * they can all be accessed through a common programing interface.
+ * Example sources would be SOC controllers, FPGAs, multifunction
+ * chips, dedicated GPIO expanders, and so on.
+ */
+struct gpio_chip {
+	int			(*get)(struct gpio_chip *chip,
+						unsigned offset);
+	void			(*set)(struct gpio_chip *chip,
+						unsigned offset, int value);
+	int			(*direction_input)(struct gpio_chip *chip,
+						unsigned offset);
+	int			(*direction_output)(struct gpio_chip *chip,
+						unsigned offset, int value);
+	unsigned		base;
+	const char		*name;
+	struct irq_desc		*irqs;
+	unsigned		irq_base;
+	u16			ngpio;
+	unsigned		can_sleep:1;
+
+	/* other fields are for the gpio library only */
+	struct list_head	chips;
+	unsigned long		is_out[DIV_ROUND_UP(ARCH_GPIOS_PER_CHIP,
+					BITS_PER_LONG)];
+#ifdef CONFIG_DEBUG_FS
+	/* fat bits */
+	const char		*requested[ARCH_GPIOS_PER_CHIP];
+#else
+	/* thin bits */
+	unsigned long		requested[DIV_ROUND_UP(ARCH_GPIOS_PER_CHIP,
+					BITS_PER_LONG)];
+#endif
+};
+
+/* Platform init code must declare each gpio chip. */
+extern int gpio_chip_declare(struct gpio_chip *chip);
+
+/* Platform may assign gpio_to_chip() hook to get something faster
+ * than the default (e.g. matching IRQ assignments).
+ */
+extern struct gpio_chip *(*gpio_to_chip)(unsigned gpio);
+
+
+/* Always use the library code for GPIO management calls,
+ * or when sleeping may be involved.
+ */
+extern int gpio_request(unsigned gpio, const char *label);
+extern void gpio_free(unsigned gpio);
+
+extern int gpio_direction_input(unsigned gpio);
+extern int gpio_direction_output(unsigned gpio, int value);
+
+extern int gpio_get_value_cansleep(unsigned gpio);
+extern void gpio_set_value_cansleep(unsigned gpio, int value);
+
+
+/* A platform's <asm/gpio.h> code may want to inline the I/O calls when
+ * the GPIO is constant and refers to some always-present controller,
+ * giving direct access to chip registers and tight bitbanging loops.
+ */
+extern int __gpio_get_value(unsigned gpio);
+extern void __gpio_set_value(unsigned gpio, int value);
+
+extern int __gpio_cansleep(unsigned gpio);
+
+
+#else
+
 /* platforms that don't directly support access to GPIOs through I2C, SPI,
  * or other blocking infrastructure can use these wrappers.
  */
@@ -22,4 +117,6 @@ static inline void gpio_set_value_cansle
 	gpio_set_value(gpio, value);
 }
 
+#endif
+
 #endif /* _ASM_GENERIC_GPIO_H */
--- g26.orig/lib/Makefile	2007-04-14 19:51:00.000000000 -0700
+++ g26/lib/Makefile	2007-04-14 19:51:12.000000000 -0700
@@ -60,6 +60,8 @@ obj-$(CONFIG_FAULT_INJECTION) += fault-i
 
 lib-$(CONFIG_GENERIC_BUG) += bug.o
 
+lib-$(CONFIG_GPIO_LIB) += gpiolib.o
+
 hostprogs-y	:= gen_crc32table
 clean-files	:= crc32table.h
 
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ g26/lib/gpiolib.c	2007-04-15 10:58:57.000000000 -0700
@@ -0,0 +1,364 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/irq.h>
+
+#include <asm-generic/gpio.h>
+
+
+/*
+ * Platform code calls __gpio_*() routines when it can't inline a given
+ * GPIO call, and is using the gpio library infrastructure.  It must declare
+ * each gpio_chip during platform initialization, and may also choose to
+ * provide an optimized gpio_to_chip hook (e.g. the same linear mapping used
+ * to hook GPIOs into the IRQ framework)
+ */
+
+static DEFINE_SPINLOCK(chip_list_lock);
+static LIST_HEAD(chip_list);
+
+static struct gpio_chip *simple_to_chip(unsigned gpio)
+{
+	struct gpio_chip	*chip;
+
+	list_for_each_entry(chip, &chip_list, chips) {
+		if (gpio < chip->base || gpio >= (chip->base + chip->ngpio))
+			continue;
+		return chip;
+	}
+	return NULL;
+}
+
+static struct gpio_chip *default_to_chip(unsigned gpio)
+{
+	struct gpio_chip	*chip;
+	unsigned long		flags;
+
+	spin_lock_irqsave(&chip_list_lock, flags);
+	chip = simple_to_chip(gpio);
+	spin_unlock_irqrestore(&chip_list_lock, flags);
+	return chip;
+}
+
+struct gpio_chip *(*gpio_to_chip)(unsigned gpio) = default_to_chip;
+
+
+/**
+ * gpio_chip_declare - publish a gpio_chip to the infrastructure
+ * @chip: the chip
+ *
+ * Each GPIO controller needs to be published.  If it supports input
+ * pins, get() and direction_input() methods must be provided.  If it
+ * supports output pins, set() and direction_output() must be provided.
+ *
+ * Once declared, gpio chips are always available.
+ */
+int gpio_chip_declare(struct gpio_chip *chip)
+{
+	unsigned long		flags;
+
+	if (!chip || !(chip->set || chip->get) || !chip->ngpio)
+		return -EINVAL;
+	if (chip->set && !chip->direction_output)
+		return -EINVAL;
+	if (chip->get && !chip->direction_input)
+		return -EINVAL;
+	if (!chip->name)
+		chip->name = "generic";
+
+	spin_lock_irqsave(&chip_list_lock, flags);
+	if (simple_to_chip(chip->base + chip->ngpio - 1)
+			|| simple_to_chip(chip->base)) {
+		printk(KERN_ERR "GPIO: collision for '%s', %u..%u\n",
+				chip->name, chip->base,
+				chip->base + chip->ngpio - 1);
+		chip = NULL;
+	} else
+		list_add_tail(&chip->chips, &chip_list);
+	spin_unlock_irqrestore(&chip_list_lock, flags);
+
+	if (!chip)
+		return -EBUSY;
+
+	pr_debug("GPIO: added '%s' for %u..%u\n", chip->name,
+			chip->base, chip->base + chip->ngpio - 1);
+	return 0;
+}
+
+
+/* These "optional" allocation calls help prevent drivers from stomping
+ * on each other, and help provide better diagnostics in debugfs.
+ */
+int gpio_request(unsigned gpio, const char *label)
+{
+	struct gpio_chip	*chip = gpio_to_chip(gpio);
+	unsigned long		flags;
+	int			status = 0;
+
+	if (!chip || gpio < chip->base)
+		return -EINVAL;
+
+	gpio -= chip->base;
+	if (gpio >= chip->ngpio)
+		return -EINVAL;
+
+	if (!label)
+		label = "?";
+
+	spin_lock_irqsave(&chip_list_lock, flags);
+#ifdef CONFIG_DEBUG_FS
+	if (chip->requested[gpio])
+		status = -EBUSY;
+	else
+		chip->requested[gpio] = label;
+#else
+	if (test_and_set_bit(gpio, chip->requested))
+		status = -EBUSY;
+#endif
+	spin_unlock_irqrestore(&chip_list_lock, flags);
+	return status;
+}
+EXPORT_SYMBOL(gpio_request);
+
+void gpio_free(unsigned gpio)
+{
+	struct gpio_chip	*chip = gpio_to_chip(gpio);
+	unsigned long		flags;
+
+	if (!chip || gpio < chip->base)
+		return;
+	gpio -= chip->base;
+	if (gpio >= chip->ngpio)
+		return;
+
+	spin_lock_irqsave(&chip_list_lock, flags);
+#ifdef CONFIG_DEBUG_FS
+	if (!chip->requested[gpio])
+		chip = NULL;
+	else
+		chip->requested[gpio] = NULL;
+#else
+	if (!test_and_clear_bit(gpio, chip->requested))
+		chip = NULL;
+#endif
+	spin_unlock_irqrestore(&chip_list_lock, flags);
+	WARN_ON(chip == NULL);
+}
+EXPORT_SYMBOL(gpio_free);
+
+
+/* Drivers MUST make configuration calls before get/set calls */
+
+int gpio_direction_input(unsigned gpio)
+{
+	struct gpio_chip	*chip = gpio_to_chip(gpio);
+	int			status;
+
+	might_sleep();
+	if (!chip || !chip->direction_input || !chip->get || gpio < chip->base)
+		return -EINVAL;
+	gpio -= chip->base;
+	if (gpio >= chip->ngpio)
+		return -EINVAL;
+
+	status = chip->direction_input(chip, gpio);
+	if (status == 0)
+		clear_bit(gpio, chip->is_out);
+	return status;
+}
+EXPORT_SYMBOL(gpio_direction_input);
+
+int gpio_direction_output(unsigned gpio, int value)
+{
+	struct gpio_chip	*chip = gpio_to_chip(gpio);
+	int			status;
+
+	might_sleep();
+	if (!chip || !chip->direction_output || !chip->set || gpio < chip->base)
+		return -EINVAL;
+	gpio -= chip->base;
+	if (gpio >= chip->ngpio)
+		return -EINVAL;
+
+	status = chip->direction_output(chip, gpio, value);
+	if (status == 0)
+		set_bit(gpio, chip->is_out);
+	return status;
+}
+EXPORT_SYMBOL(gpio_direction_output);
+
+
+/* I/O calls are only valid after configuration completed;
+ * the relevant error checks have already been done.
+ *
+ * "Get" operations are often inlinable as reading a pin value register,
+ * and masking the relevant bit in that register.
+ *
+ * When "set" operations are inlinable, they involve writing that mask to
+ * one register to set a low value, or a different register to set it high.
+ * Otherwise a spinlock is needed, and there's little value to inlining.
+ */
+int __gpio_get_value(unsigned gpio)
+{
+	struct gpio_chip *chip = gpio_to_chip(gpio);
+
+	WARN_ON(chip->can_sleep != 0);
+	return chip->get(chip, gpio - chip->base);
+}
+EXPORT_SYMBOL(__gpio_get_value);
+
+void __gpio_set_value(unsigned gpio, int value)
+{
+	struct gpio_chip *chip = gpio_to_chip(gpio);
+
+	WARN_ON(chip->can_sleep != 0);
+	return chip->set(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL(__gpio_set_value);
+
+int __gpio_cansleep(unsigned gpio)
+{
+	struct gpio_chip *chip = gpio_to_chip(gpio);
+
+	return chip->can_sleep;
+}
+EXPORT_SYMBOL(__gpio_cansleep);
+
+
+/* There's no value in inlining GPIO calls that may sleep */
+
+int gpio_get_value_cansleep(unsigned gpio)
+{
+	struct gpio_chip *chip = gpio_to_chip(gpio);
+
+	might_sleep();
+	return chip->get(chip, gpio - chip->base);
+}
+EXPORT_SYMBOL(gpio_get_value_cansleep);
+
+void gpio_set_value_cansleep(unsigned gpio, int value)
+{
+	struct gpio_chip *chip = gpio_to_chip(gpio);
+
+	might_sleep();
+	return chip->set(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL(gpio_set_value_cansleep);
+
+
+#ifdef CONFIG_DEBUG_FS
+
+/*
+ * When debugfs is available, use it to dump a summary of the
+ * GPIO signals currently used, and their status.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+static int dbg_gpio_show(struct seq_file *s, void *unused)
+{
+	struct gpio_chip	*chip;
+
+	list_for_each_entry(chip, &chip_list, chips) {
+		unsigned	offset;
+		const char	*name = NULL;
+
+		for (offset = 0; offset < chip->ngpio; offset++) {
+			unsigned	gpio;
+			unsigned	value, is_out;
+
+			if (!chip->requested[offset])
+				continue;
+
+			/* Show each irq bank separately */
+			if (!name) {
+				name = chip->name;
+				seq_printf(s,
+					"Controller: %s, %u..%d%s%s\n",
+					name, chip->base,
+					chip->base + chip->ngpio - 1,
+					chip->irqs ? ", irqs" : "",
+					chip->can_sleep ? ", can sleep" : "");
+			}
+
+			/* Dump basic per-GPIO status */
+			gpio = chip->base + offset;
+			is_out = test_bit(offset, chip->is_out);
+			value = chip->get ? chip->get(chip, offset) : 0;
+			seq_printf(s, "  GPIO %3u (%2u): %s %s (%10s)",
+					gpio, offset,
+					is_out ? "out" : "in ",
+					value  ? "hi"  : "lo",
+					chip->requested[offset]);
+
+			/* For GPIOs used as IRQs, dump more status.  Wakeup
+			 * is often set only between suspend() and resume().
+			 */
+			if (!is_out && chip->irqs) {
+				struct irq_desc	*irq = chip->irqs + offset;
+
+				if (irq->action) {
+					char		*trigger;
+					unsigned	stat;
+
+					stat = irq->status;
+					switch (stat & IRQ_TYPE_SENSE_MASK) {
+					case IRQ_TYPE_EDGE_FALLING:
+						trigger = "falling";
+						break;
+					case IRQ_TYPE_EDGE_RISING:
+						trigger = "rising";
+						break;
+					case IRQ_TYPE_EDGE_BOTH:
+						trigger = "bothedge";
+						break;
+					case IRQ_TYPE_LEVEL_LOW:
+						trigger = "low";
+						break;
+					case IRQ_TYPE_LEVEL_HIGH:
+						trigger = "high";
+						break;
+					default:
+						trigger = "?";
+						break;
+					}
+					seq_printf(s,
+						", IRQ %3d/%s%s",
+						chip->irq_base + offset,
+						trigger,
+						(stat & IRQ_WAKEUP)
+							? " wakeup"
+							: "");
+				}
+			}
+
+			seq_printf(s, "\n");
+		}
+
+		if (name)
+			seq_printf(s, "\n");
+	}
+	return 0;
+}
+
+static int dbg_gpio_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, dbg_gpio_show, &inode->i_private);
+}
+
+static const struct file_operations debug_fops = {
+	.open		= dbg_gpio_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init gpiolib_debuginit(void)
+{
+	(void) debugfs_create_file("gpio", S_IRUGO, NULL, NULL, &debug_fops);
+	return 0;
+}
+subsys_initcall(gpiolib_debuginit);
+
+#endif
+

Re: is there any generic GPIO chip framework like IRQ chips?

[David Brownell]

Makes OMAP use the infrastructure in the preceding patch; slightly
slows down the code, because it adds an indirect function call and
uses a non-table lookup, but GPIOs aren't accessed on critical paths.

===============	CUT HERE
Update OMAP to use the new GPIO implementation framework.  This is just a
quick'n'dirty update; more code could now be removed.  Plus, it'd really
be better to shift the entire OMAP GPIO infrastructure over to cleaner
code, this all sort of "just growed" over time.

Note:  against current Linux-OMAP tree.  The code at the end of gpio.c
isn't yet upstream; remove that segment and it may apply against kernel.org
trees too.

---
 arch/arm/plat-omap/Kconfig       |    2 
 arch/arm/plat-omap/gpio.c        |  201 ++++++++++++++-------------------------
 include/asm-arm/arch-omap/gpio.h |   52 ++--------
 3 files changed, 88 insertions(+), 167 deletions(-)

--- osk2.orig/arch/arm/plat-omap/Kconfig	2007-04-15 10:59:51.000000000 -0700
+++ osk2/arch/arm/plat-omap/Kconfig	2007-04-15 11:00:40.000000000 -0700
@@ -11,9 +11,11 @@ choice
 
 config ARCH_OMAP1
 	bool "TI OMAP1"
+	select GPIO_LIB
 
 config ARCH_OMAP2
 	bool "TI OMAP2"
+	select GPIO_LIB
 
 endchoice
 
--- osk2.orig/arch/arm/plat-omap/gpio.c	2007-04-15 10:59:51.000000000 -0700
+++ osk2/arch/arm/plat-omap/gpio.c	2007-04-15 11:02:54.000000000 -0700
@@ -137,6 +137,7 @@ struct gpio_bank {
 	u32 saved_risingdetect;
 #endif
 	spinlock_t lock;
+	struct gpio_chip chip;
 };
 
 #define METHOD_MPUIO		0
@@ -838,17 +839,23 @@ static int gpio_wake_enable(unsigned int
 int omap_request_gpio(int gpio)
 {
 	struct gpio_bank *bank;
+	int status;
 
 	if (check_gpio(gpio) < 0)
 		return -EINVAL;
 
+	status = gpio_request(gpio, NULL);
+	if (status < 0)
+		return status;
+
 	bank = get_gpio_bank(gpio);
 	spin_lock(&bank->lock);
 	if (unlikely(bank->reserved_map & (1 << get_gpio_index(gpio)))) {
 		printk(KERN_ERR "omap-gpio: GPIO %d is already reserved!\n", gpio);
 		dump_stack();
 		spin_unlock(&bank->lock);
-		return -1;
+		gpio_free(gpio);
+		return -EBUSY;
 	}
 	bank->reserved_map |= (1 << get_gpio_index(gpio));
 
@@ -902,6 +909,7 @@ void omap_free_gpio(int gpio)
 	bank->reserved_map &= ~(1 << get_gpio_index(gpio));
 	_reset_gpio(bank, gpio);
 	spin_unlock(&bank->lock);
+	gpio_free(gpio);
 }
 
 /*
@@ -1199,6 +1207,50 @@ static inline void mpuio_init(void) {}
 
 /*---------------------------------------------------------------------*/
 
+/* REVISIT these are stupid implementations!  replace by ones that
+ * don't switch on METHOD_* and which mostly avoid spinlocks
+ */
+
+static int gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	return omap_get_gpio_datain(chip->base + offset);
+}
+
+static void gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct gpio_bank *bank;
+
+	bank = container_of(chip, struct gpio_bank, chip);
+	spin_lock(&bank->lock);
+	_set_gpio_dataout(bank, offset, value);
+	spin_unlock(&bank->lock);
+}
+
+static int gpio_input(struct gpio_chip *chip, unsigned offset)
+{
+	struct gpio_bank *bank;
+
+	bank = container_of(chip, struct gpio_bank, chip);
+	spin_lock(&bank->lock);
+	_set_gpio_direction(bank, offset, 1);
+	spin_unlock(&bank->lock);
+	return 0;
+}
+
+static int gpio_output(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct gpio_bank *bank;
+
+	bank = container_of(chip, struct gpio_bank, chip);
+	spin_lock(&bank->lock);
+	_set_gpio_dataout(bank, offset, value);
+	_set_gpio_direction(bank, offset, 0);
+	spin_unlock(&bank->lock);
+	return 0;
+}
+
+/*---------------------------------------------------------------------*/
+
 static int initialized;
 static struct clk * gpio_ick;
 static struct clk * gpio_fck;
@@ -1212,6 +1264,7 @@ static int __init _omap_gpio_init(void)
 {
 	int i;
 	struct gpio_bank *bank;
+	int gpio = 0;
 
 	initialized = 1;
 
@@ -1346,6 +1399,27 @@ static int __init _omap_gpio_init(void)
 			gpio_count = 32;
 		}
 #endif
+		/* REVISIT just switch from OMAP-specific gpio
+		 * interfaces over to the generic ones
+		 */
+		bank->chip.get = gpio_get;
+		bank->chip.set = gpio_set;
+		bank->chip.direction_input = gpio_input;
+		bank->chip.direction_output = gpio_output;
+		if (bank_is_mpuio(bank)) {
+			bank->chip.base = OMAP_MPUIO(0);
+			bank->chip.name = "MPUIO";
+		} else {
+			bank->chip.base = gpio;
+			bank->chip.name = "GPIO";
+			gpio += gpio_count;
+		}
+		bank->chip.irqs = irq_desc + bank->virtual_irq_start;
+		bank->chip.irq_base = bank->virtual_irq_start;
+		bank->chip.ngpio = gpio_count;
+
+		gpio_chip_declare(&bank->chip);
+
 		for (j = bank->virtual_irq_start;
 		     j < bank->virtual_irq_start + gpio_count; j++) {
 			set_irq_chip_data(j, bank);
@@ -1582,128 +1656,3 @@ EXPORT_SYMBOL(omap_set_gpio_dataout);
 EXPORT_SYMBOL(omap_get_gpio_datain);
 
 arch_initcall(omap_gpio_sysinit);
-
-
-#ifdef	CONFIG_DEBUG_FS
-
-#include <linux/debugfs.h>
-#include <linux/seq_file.h>
-
-static int gpio_is_input(struct gpio_bank *bank, int mask)
-{
-	void __iomem *reg = bank->base;
-
-	switch (bank->method) {
-	case METHOD_MPUIO:
-		reg += OMAP_MPUIO_IO_CNTL;
-		break;
-	case METHOD_GPIO_1510:
-		reg += OMAP1510_GPIO_DIR_CONTROL;
-		break;
-	case METHOD_GPIO_1610:
-		reg += OMAP1610_GPIO_DIRECTION;
-		break;
-	case METHOD_GPIO_730:
-		reg += OMAP730_GPIO_DIR_CONTROL;
-		break;
-	case METHOD_GPIO_24XX:
-		reg += OMAP24XX_GPIO_OE;
-		break;
-	}
-	return __raw_readl(reg) & mask;
-}
-
-
-static int dbg_gpio_show(struct seq_file *s, void *unused)
-{
-	unsigned	i, j, gpio;
-
-	for (i = 0, gpio = 0; i < gpio_bank_count; i++) {
-		struct gpio_bank	*bank = gpio_bank + i;
-		unsigned		bankwidth = 16;
-		u32			mask = 1;
-
-		if (bank_is_mpuio(bank))
-			gpio = OMAP_MPUIO(0);
-		else if (cpu_is_omap24xx() || cpu_is_omap730())
-			bankwidth = 32;
-
-		for (j = 0; j < bankwidth; j++, gpio++, mask <<= 1) {
-			unsigned	irq, value, is_in, irqstat;
-
-			if (!(bank->reserved_map & mask))
-				continue;
-
-			irq = bank->virtual_irq_start + j;
-			value = omap_get_gpio_datain(gpio);
-			is_in = gpio_is_input(bank, mask);
-
-			if (bank_is_mpuio(bank))
-				seq_printf(s, "MPUIO %2d: ", j);
-			else
-				seq_printf(s, "GPIO %3d: ", gpio);
-			seq_printf(s, "%s %s",
-					is_in ? "in " : "out",
-					value ? "hi"  : "lo");
-
-			irqstat = irq_desc[irq].status;
-			if (is_in && ((bank->suspend_wakeup & mask)
-					|| irqstat & IRQ_TYPE_SENSE_MASK)) {
-				char	*trigger = NULL;
-
-				switch (irqstat & IRQ_TYPE_SENSE_MASK) {
-				case IRQ_TYPE_EDGE_FALLING:
-					trigger = "falling";
-					break;
-				case IRQ_TYPE_EDGE_RISING:
-					trigger = "rising";
-					break;
-				case IRQ_TYPE_EDGE_BOTH:
-					trigger = "bothedge";
-					break;
-				case IRQ_TYPE_LEVEL_LOW:
-					trigger = "low";
-					break;
-				case IRQ_TYPE_LEVEL_HIGH:
-					trigger = "high";
-					break;
-				case IRQ_TYPE_NONE:
-					trigger = "(unspecified)";
-					break;
-				}
-				seq_printf(s, ", irq-%d %s%s",
-						irq, trigger,
-						(bank->suspend_wakeup & mask)
-							? " wakeup" : "");
-			}
-			seq_printf(s, "\n");
-		}
-
-		if (bank_is_mpuio(bank)) {
-			seq_printf(s, "\n");
-			gpio = 0;
-		}
-	}
-	return 0;
-}
-
-static int dbg_gpio_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, dbg_gpio_show, &inode->i_private);
-}
-
-static const struct file_operations debug_fops = {
-	.open		= dbg_gpio_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
-
-static int __init omap_gpio_debuginit(void)
-{
-	(void) debugfs_create_file("omap_gpio", S_IRUGO,
-					NULL, NULL, &debug_fops);
-	return 0;
-}
-late_initcall(omap_gpio_debuginit);
-#endif
--- osk2.orig/include/asm-arm/arch-omap/gpio.h	2007-04-15 10:59:51.000000000 -0700
+++ osk2/include/asm-arm/arch-omap/gpio.h	2007-04-15 11:00:40.000000000 -0700
@@ -78,58 +78,28 @@ extern int omap_get_gpio_datain(int gpio
 
 /*-------------------------------------------------------------------------*/
 
-/* wrappers for "new style" GPIO calls. the old OMAP-specfic ones should
- * eventually be removed (along with this errno.h inclusion), and maybe
- * gpios should put MPUIOs last too.
+/* Wrappers for "new style" GPIO calls, using the new infrastructure
+ * which lets us plug in FPGA, I2C, and other implementations.
+ * *
+ * The original OMAP-specfic calls should eventually be removed.
  */
 
-#include <asm/errno.h>
-
-static inline int gpio_request(unsigned gpio, const char *label)
-{
-	return omap_request_gpio(gpio);
-}
-
-static inline void gpio_free(unsigned gpio)
-{
-	omap_free_gpio(gpio);
-}
-
-static inline int __gpio_set_direction(unsigned gpio, int is_input)
-{
-	if (cpu_class_is_omap2()) {
-		if (gpio > OMAP_MAX_GPIO_LINES)
-			return -EINVAL;
-	} else {
-		if (gpio > (OMAP_MAX_GPIO_LINES + 16 /* MPUIO */))
-			return -EINVAL;
-	}
-	omap_set_gpio_direction(gpio, is_input);
-	return 0;
-}
-
-static inline int gpio_direction_input(unsigned gpio)
-{
-	return __gpio_set_direction(gpio, 1);
-}
-
-static inline int gpio_direction_output(unsigned gpio, int value)
-{
-	omap_set_gpio_dataout(gpio, value);
-	return __gpio_set_direction(gpio, 0);
-}
+#include <asm-generic/gpio.h>
 
 static inline int gpio_get_value(unsigned gpio)
 {
-	return omap_get_gpio_datain(gpio);
+	return __gpio_get_value(gpio);
 }
 
 static inline void gpio_set_value(unsigned gpio, int value)
 {
-	omap_set_gpio_dataout(gpio, value);
+	__gpio_set_value(gpio, value);
 }
 
-#include <asm-generic/gpio.h>		/* cansleep wrappers */
+static inline int gpio_cansleep(unsigned gpio)
+{
+	return __gpio_cansleep(gpio);
+}
 
 static inline int gpio_to_irq(unsigned gpio)
 {

Re: is there any generic GPIO chip framework like IRQ chips?

[David Brownell]

To merge, this would be split apart ... what's interesting here is
the way the new leds-gpio driver can be made to talk to leds that
sit across an I2C link.


==========	CUT HERE
This presumes various patches updating:

 - LED framework to support generic GPIOs (in MM and LED queue)

 - LED framework's generic GPIO support to understand that some GPIOs
   can't be directly accessed in timer callbacks (patch submitted)

 - I2C to support "new style" drivers (driver model conformance, in MM
   and i2c queues);

 - Kernel.org to match current OMAP trees (in the works, huge)

 - OMAP to use the "struct gpio_chip" infrastructure for its GPIOs (a
   previous patch in this series)

 - TPS65010 to be such a "new style" I2C driver (blocked waiting for OMAP
   and I2C patches to merge);

Given those prerequisites, this patch:

 * Exposes tps65010 GPIOs and LEDs through the GPIO framework (gaining an
   activity light for CF access)
 
 * Converts the OSK board to use that framework (not quite everywhere; at
   least the ads7846 driver and serial line wakeup still need updating)

 * Removes "old style" OSK led support, except with the Mistral board
   whose red-or-green LED isn't really supported by the new framework

Note that this is the first example of the "I2C GPIO expander" case,
where the GPIOs can't be accessed in non-sleeping contexts.

---
 arch/arm/mach-omap1/board-osk.c       |   66 +++++++++++++++++-------
 arch/arm/mach-omap1/leds-osk.c        |   92 ----------------------------------
 arch/arm/mach-omap1/leds.c            |   15 ++---
 drivers/i2c/chips/tps65010.c          |   59 +++++++++++++++++++++
 include/asm-arm/arch-omap/board-osk.h |   11 ++++
 include/asm-arm/arch-omap/tps65010.h  |   17 ++++++
 6 files changed, 141 insertions(+), 119 deletions(-)

--- osk2.orig/include/asm-arm/arch-omap/tps65010.h	2007-04-15 11:09:12.000000000 -0700
+++ osk2/include/asm-arm/arch-omap/tps65010.h	2007-04-15 11:09:15.000000000 -0700
@@ -152,5 +152,22 @@ extern int tps65010_config_vregs1(unsign
  */
 extern int tps65013_set_low_pwr(unsigned mode);
 
+
+/**
+ * struct tps65010_board - packages GPIO and LED lines
+ * @base: the GPIO number to assign to GPIO-1
+ * @outmask: bit (N-1) is set to allow GPIO-N to be used
+ *	as an (open drain) output
+ *
+ * Board data may be used to package the GPIO (and LED) lines
+ * for use in by the generic GPIO and LED frameworks.  The first
+ * four GPIOs starting at gpio_base are GPIO1..GPIO4; the next
+ * two are LED1..LED2.
+ */
+struct tps65010_board {
+	int			base;
+	unsigned		outmask;
+};
+
 #endif /*  __ASM_ARCH_TPS65010_H */
 
--- osk2.orig/drivers/i2c/chips/tps65010.c	2007-04-15 11:09:12.000000000 -0700
+++ osk2/drivers/i2c/chips/tps65010.c	2007-04-15 11:09:15.000000000 -0700
@@ -34,9 +34,9 @@
 #include <linux/mutex.h>
 
 #include <asm/irq.h>
+#include <asm/gpio.h>
 #include <asm/mach-types.h>
 
-#include <asm/arch/gpio.h>
 #include <asm/arch/mux.h>
 #include <asm/arch/tps65010.h>
 
@@ -91,7 +91,8 @@ struct tps65010 {
 	u8			chgstatus, regstatus, chgconf;
 	u8			nmask1, nmask2;
 
-	/* not currently tracking GPIO state */
+	u8			outmask;
+	struct gpio_chip	chip;
 };
 
 #define	POWER_POLL_DELAY	msecs_to_jiffies(5000)
@@ -454,6 +455,38 @@ static irqreturn_t tps65010_irq(int irq,
 
 /*-------------------------------------------------------------------------*/
 
+/* offsets 0..3 == GPIO1..GPIO4
+ * offsets 4..5 == LED1..LED2 (we set one of the non-BLINK modes)
+ */
+static void
+tps65010_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	if (offset < 4)
+		tps65010_set_gpio_out_value(offset + 1, value);
+	else
+		tps65010_set_led(offset - 3, value ? ON : OFF);
+}
+
+static int
+tps65010_output(struct gpio_chip *chip, unsigned offset, int value)
+{
+	/* GPIOs may be input-only */
+	if (offset < 4) {
+		struct tps65010		*tps;
+
+		tps = container_of(chip, struct tps65010, chip);
+		if (!(tps->outmask & (1 << offset)))
+			return -EINVAL;
+		tps65010_set_gpio_out_value(offset + 1, value);
+	} else
+		tps65010_set_led(offset - 3, value ? ON : OFF);
+
+	return 0;
+}
+
+
+/*-------------------------------------------------------------------------*/
+
 static struct tps65010 *the_tps;
 
 static int __exit tps65010_remove(struct i2c_client *client)
@@ -475,6 +508,7 @@ static int tps65010_probe(struct i2c_cli
 	struct tps65010		*tps;
 	int			status;
 	unsigned long		irqflags;
+	struct tps65010_board	*board = client->dev.platform_data;
 
 	if (the_tps) {
 		dev_dbg(&client->dev, "only one %s for now\n", DRIVER_NAME);
@@ -585,6 +619,27 @@ static int tps65010_probe(struct i2c_cli
 
 	tps->file = debugfs_create_file(DRIVER_NAME, S_IRUGO, NULL,
 				tps, DEBUG_FOPS);
+
+	/* optionally register GPIOs (and leds) */
+	if (board && board->base > 0) {
+		tps->outmask = board->outmask;
+
+		/* tps->chip.get = tps65010_gpio_get; */
+		tps->chip.set = tps65010_gpio_set;
+		/* tps->chip.direction_input = tps65010_input; */
+		tps->chip.direction_output = tps65010_output;
+		tps->chip.base = board->base;
+		tps->chip.name = client->name;
+		/* we don't handle GPIO input irqs (yet?) ... */
+		tps->chip.ngpio = 6;
+		tps->chip.can_sleep = 1;
+
+		status = gpio_chip_declare(&tps->chip);
+		if (status < 0)
+			dev_dbg(&client->dev, "can't declare GPIOs, err %d\n",
+					status);
+	}
+
 	return 0;
 fail1:
 	kfree(tps);
--- osk2.orig/include/asm-arm/arch-omap/board-osk.h	2007-04-15 11:09:12.000000000 -0700
+++ osk2/include/asm-arm/arch-omap/board-osk.h	2007-04-15 11:09:15.000000000 -0700
@@ -32,5 +32,16 @@
 /* At OMAP5912 OSK the Ethernet is directly connected to CS1 */
 #define OMAP_OSK_ETHR_START		0x04800300
 
+/* TPS65010 has four GPIOs.  nPG and LED2 can be treated like GPIOs with
+ * alternate pin configurations for hardware-controlled blinking.
+ */
+#define OSK_TPS_GPIO_BASE		(OMAP_MAX_GPIO_LINES + 16 /* MPUIO */)
+#	define OSK_TPS_GPIO_USB_PWR_EN	(OSK_TPS_GPIO_BASE + 0)
+#	define OSK_TPS_GPIO_LED_D3	(OSK_TPS_GPIO_BASE + 1)
+#	define OSK_TPS_GPIO_LAN_RESET	(OSK_TPS_GPIO_BASE + 2)
+#	define OSK_TPS_GPIO_DSP_PWR_EN	(OSK_TPS_GPIO_BASE + 3)
+#	define OSK_TPS_GPIO_LED_D9	(OSK_TPS_GPIO_BASE + 4)
+#	define OSK_TPS_GPIO_LED_D2	(OSK_TPS_GPIO_BASE + 5)
+
 #endif /*  __ASM_ARCH_OMAP_OSK_H */
 
--- osk2.orig/arch/arm/mach-omap1/board-osk.c	2007-04-15 11:09:12.000000000 -0700
+++ osk2/arch/arm/mach-omap1/board-osk.c	2007-04-15 11:09:15.000000000 -0700
@@ -33,6 +33,7 @@
 #include <linux/irq.h>
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
+#include <linux/leds.h>
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
@@ -182,11 +183,17 @@ static struct platform_device *osk5912_d
 	&osk5912_mcbsp1_device,
 };
 
+static struct tps65010_board tps_board = {
+	.base		= OSK_TPS_GPIO_BASE,
+	.outmask	= 0x0f,
+};
+
 static struct i2c_board_info __initdata osk_i2c_board_info[] = {
 {
 	I2C_BOARD_INFO("tps65010", 0x48),
 	.type		= "tps65010",
 	.irq		= OMAP_GPIO_IRQ(OMAP_MPUIO(1)),
+	.platform_data	= &tps_board,
 },
 #ifdef	CONFIG_OMAP_OSK_MISTRAL
 {
@@ -202,7 +209,7 @@ static struct i2c_board_info __initdata 
 
 static void __init osk_init_smc91x(void)
 {
-	if ((omap_request_gpio(0)) < 0) {
+	if ((gpio_request(0, "smc_irq")) < 0) {
 		printk("Error requesting gpio 0 for smc91x irq\n");
 		return;
 	}
@@ -214,7 +221,7 @@ static void __init osk_init_smc91x(void)
 static void __init osk_init_cf(void)
 {
 	omap_cfg_reg(M7_1610_GPIO62);
-	if ((omap_request_gpio(62)) < 0) {
+	if ((gpio_request(62, "cf_irq")) < 0) {
 		printk("Error requesting gpio 62 for CF irq\n");
 		return;
 	}
@@ -338,7 +345,7 @@ static struct platform_device *mistral_d
 
 static int mistral_get_pendown_state(void)
 {
-	return !omap_get_gpio_datain(4);
+	return !gpio_get_value(4);
 }
 
 static const struct ads7846_platform_data mistral_ts_info = {
@@ -400,10 +407,9 @@ static void __init osk_mistral_init(void
 	omap_cfg_reg(W14_1610_CCP_DATAP);
 
 	/* CAM_PWDN */
-	if (omap_request_gpio(11) == 0) {
+	if (gpio_request(11, "cam_pwdn") == 0) {
 		omap_cfg_reg(N20_1610_GPIO11);
-		omap_set_gpio_direction(11, 0 /* out */);
-		omap_set_gpio_dataout(11, 0 /* off */);
+		gpio_direction_output(11, 0 /* off */);
 	} else
 		pr_debug("OSK+Mistral: CAM_PWDN is awol\n");
 
@@ -416,9 +422,9 @@ static void __init osk_mistral_init(void
 
 	/* the sideways button (SW1) is for use as a "wakeup" button */
 	omap_cfg_reg(N15_1610_MPUIO2);
-	if (omap_request_gpio(OMAP_MPUIO(2)) == 0) {
+	if (gpio_request(OMAP_MPUIO(2), "wakeup") == 0) {
 		int ret = 0;
-		omap_set_gpio_direction(OMAP_MPUIO(2), 1);
+		gpio_direction_input(OMAP_MPUIO(2));
 		set_irq_type(OMAP_GPIO_IRQ(OMAP_MPUIO(2)), IRQT_RISING);
 #ifdef	CONFIG_PM
 		/* share the IRQ in case someone wants to use the
@@ -429,7 +435,7 @@ static void __init osk_mistral_init(void
 				IRQF_SHARED, "mistral_wakeup",
 				&osk_mistral_wake_interrupt);
 		if (ret != 0) {
-			omap_free_gpio(OMAP_MPUIO(2));
+			gpio_free(OMAP_MPUIO(2));
 			printk(KERN_ERR "OSK+Mistral: no wakeup irq, %d?\n",
 				ret);
 		} else
@@ -442,10 +448,8 @@ static void __init osk_mistral_init(void
 	 * board, like the touchscreen, EEPROM, and wakeup (!) switch.
 	 */
 	omap_cfg_reg(PWL);
-	if (omap_request_gpio(2) == 0) {
-		omap_set_gpio_direction(2, 0 /* out */);
-		omap_set_gpio_dataout(2, 1 /* on */);
-	}
+	if (gpio_request(2, "lcd_pwdn") == 0)
+		gpio_direction_output(2, 1 /* on */);
 
 	platform_add_devices(mistral_devices, ARRAY_SIZE(mistral_devices));
 }
@@ -474,8 +478,8 @@ static void __init osk_init(void)
 
 	/* irq for tps65010 chip */
 	// omap_cfg_reg(U19_1610_MPUIO1);
-	omap_request_gpio(OMAP_MPUIO(1));
-	omap_set_gpio_direction(OMAP_MPUIO(1), 1);
+	if (gpio_request(OMAP_MPUIO(1), "tps65010"))
+		gpio_direction_input(OMAP_MPUIO(1));
 
 	i2c_register_board_info(1, osk_i2c_board_info,
 			ARRAY_SIZE(osk_i2c_board_info));
@@ -490,6 +494,26 @@ static void __init osk_map_io(void)
 }
 
 #ifdef CONFIG_TPS65010
+
+static struct gpio_led tps_leds[] = {
+	{ .gpio = OSK_TPS_GPIO_LED_D9, .name = "d9:green",
+			.default_trigger = "ide-disk", /* CF */ },
+	{ .gpio = OSK_TPS_GPIO_LED_D2, .name = "d2:green", },
+	{ .gpio = OSK_TPS_GPIO_LED_D3, .name = "d3:green", .active_low = 1,
+			.default_trigger = "heartbeat", },
+};
+
+static struct gpio_led_platform_data tps_led_data = {
+	.num_leds	= ARRAY_SIZE(tps_leds),
+	.leds		= tps_leds,
+};
+
+static struct platform_device tps_led_dev = {
+	.name			= "leds-gpio",
+	.id			= -1,
+	.dev.platform_data	= &tps_led_data,
+};
+
 static int __init osk_tps_init(void)
 {
 	if (!machine_is_omap_osk())
@@ -504,16 +528,20 @@ static int __init osk_tps_init(void)
 	/* Set GPIO 1 HIGH to disable VBUS power supply;
 	 * OHCI driver powers it up/down as needed.
 	 */
-	tps65010_set_gpio_out_value(GPIO1, HIGH);
+	if (gpio_request(OSK_TPS_GPIO_USB_PWR_EN, "vbus_dis") == 0)
+		gpio_direction_output(OSK_TPS_GPIO_USB_PWR_EN, 1);
 
 	/* Set GPIO 2 low to turn on LED D3 */
 	tps65010_set_gpio_out_value(GPIO2, HIGH);
 
 	/* Set GPIO 3 low to take ethernet out of reset */
-	tps65010_set_gpio_out_value(GPIO3, LOW);
+	if (gpio_request(OSK_TPS_GPIO_LAN_RESET, "lan_reset") == 0)
+		gpio_direction_output(OSK_TPS_GPIO_LAN_RESET, 0);
 
 	/* gpio4 for VDD_DSP */
-	// FIXME power DSP iff it's configured
+	/* FIXME power DSP iff it's configured */
+	if (gpio_request(OSK_TPS_GPIO_DSP_PWR_EN, "dsp_pwr_en") == 0)
+		gpio_direction_output(OSK_TPS_GPIO_DSP_PWR_EN, 1);
 
 	/* Enable LOW_PWR */
 	tps65010_set_low_pwr(ON);
@@ -522,7 +550,7 @@ static int __init osk_tps_init(void)
 	tps65010_config_vregs1(TPS_LDO2_ENABLE | TPS_VLDO2_3_0V
 			| TPS_LDO1_ENABLE);
 
-	return 0;
+	return platform_device_register(&tps_led_dev);
 }
 fs_initcall(osk_tps_init);
 #endif
--- osk2.orig/arch/arm/mach-omap1/leds-osk.c	2007-04-15 11:09:12.000000000 -0700
+++ osk2/arch/arm/mach-omap1/leds-osk.c	2007-04-15 11:09:15.000000000 -0700
@@ -1,7 +1,7 @@
 /*
  * linux/arch/arm/mach-omap1/leds-osk.c
  *
- * LED driver for OSK, and optionally Mistral QVGA, boards
+ * LED driver for OSK's optional Mistral QVGA board
  */
 #include <linux/init.h>
 #include <linux/workqueue.h>
@@ -11,7 +11,6 @@
 #include <asm/system.h>
 
 #include <asm/arch/gpio.h>
-#include <asm/arch/tps65010.h>
 
 #include "leds.h"
 
@@ -20,51 +19,11 @@
 #define LED_STATE_CLAIMED	(1 << 1)
 static u8 led_state;
 
-#define	GREEN_LED		(1 << 0)	/* TPS65010 LED1 */
-#define	AMBER_LED		(1 << 1)	/* TPS65010 LED2 */
-#define	RED_LED			(1 << 2)	/* TPS65010 GPIO2 */
 #define	TIMER_LED		(1 << 3)	/* Mistral board */
 #define	IDLE_LED		(1 << 4)	/* Mistral board */
 static u8 hw_led_state;
 
 
-/* TPS65010 leds are changed using i2c -- from a task context.
- * Using one of these for the "idle" LED would be impractical...
- */
-#define	TPS_LEDS	(GREEN_LED | RED_LED | AMBER_LED)
-
-static u8 tps_leds_change;
-
-static void tps_work(struct work_struct *unused)
-{
-	for (;;) {
-		u8	leds;
-
-		local_irq_disable();
-		leds = tps_leds_change;
-		tps_leds_change = 0;
-		local_irq_enable();
-
-		if (!leds)
-			break;
-
-		/* careful:  the set_led() value is on/off/blink */
-		if (leds & GREEN_LED)
-			tps65010_set_led(LED1, !!(hw_led_state & GREEN_LED));
-		if (leds & AMBER_LED)
-			tps65010_set_led(LED2, !!(hw_led_state & AMBER_LED));
-
-		/* the gpio led doesn't have that issue */
-		if (leds & RED_LED)
-			tps65010_set_gpio_out_value(GPIO2,
-					!(hw_led_state & RED_LED));
-	}
-}
-
-static DECLARE_WORK(work, tps_work);
-
-#ifdef	CONFIG_OMAP_OSK_MISTRAL
-
 /* For now, all system indicators require the Mistral board, since that
  * LED can be manipulated without a task context.  This LED is either red,
  * or green, but not both; it can't give the full "disco led" effect.
@@ -88,24 +47,19 @@ static void mistral_setled(void)
 	omap_set_gpio_dataout(GPIO_LED_RED, red);
 }
 
-#endif
-
 void osk_leds_event(led_event_t evt)
 {
 	unsigned long	flags;
-	u16		leds;
 
 	local_irq_save(flags);
 
 	if (!(led_state & LED_STATE_ENABLED) && evt != led_start)
 		goto done;
 
-	leds = hw_led_state;
 	switch (evt) {
 	case led_start:
 		led_state |= LED_STATE_ENABLED;
 		hw_led_state = 0;
-		leds = ~0;
 		break;
 
 	case led_halted:
@@ -118,7 +72,6 @@ void osk_leds_event(led_event_t evt)
 	case led_claim:
 		led_state |= LED_STATE_CLAIMED;
 		hw_led_state = 0;
-		leds = ~0;
 		break;
 
 	case led_release:
@@ -126,8 +79,6 @@ void osk_leds_event(led_event_t evt)
 		hw_led_state = 0;
 		break;
 
-#ifdef	CONFIG_OMAP_OSK_MISTRAL
-
 	case led_timer:
 		hw_led_state ^= TIMER_LED;
 		mistral_setled();
@@ -143,51 +94,10 @@ void osk_leds_event(led_event_t evt)
 		mistral_setled();
 		break;
 
-#endif	/* CONFIG_OMAP_OSK_MISTRAL */
-
-	/* "green" == tps LED1 (leftmost, normally power-good)
-	 * works only with DC adapter, not on battery power!
-	 */
-	case led_green_on:
-		if (led_state & LED_STATE_CLAIMED)
-			hw_led_state |= GREEN_LED;
-		break;
-	case led_green_off:
-		if (led_state & LED_STATE_CLAIMED)
-			hw_led_state &= ~GREEN_LED;
-		break;
-
-	/* "amber" == tps LED2 (middle) */
-	case led_amber_on:
-		if (led_state & LED_STATE_CLAIMED)
-			hw_led_state |= AMBER_LED;
-		break;
-	case led_amber_off:
-		if (led_state & LED_STATE_CLAIMED)
-			hw_led_state &= ~AMBER_LED;
-		break;
-
-	/* "red" == LED on tps gpio3 (rightmost) */
-	case led_red_on:
-		if (led_state & LED_STATE_CLAIMED)
-			hw_led_state |= RED_LED;
-		break;
-	case led_red_off:
-		if (led_state & LED_STATE_CLAIMED)
-			hw_led_state &= ~RED_LED;
-		break;
-
 	default:
 		break;
 	}
 
-	leds ^= hw_led_state;
-	leds &= TPS_LEDS;
-	if (leds && (led_state & LED_STATE_CLAIMED)) {
-		tps_leds_change |= leds;
-		schedule_work(&work);
-	}
-
 done:
 	local_irq_restore(flags);
 }
--- osk2.orig/arch/arm/mach-omap1/leds.c	2007-04-15 11:09:12.000000000 -0700
+++ osk2/arch/arm/mach-omap1/leds.c	2007-04-15 11:09:15.000000000 -0700
@@ -5,11 +5,12 @@
  */
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/list.h>
 
 #include <asm/leds.h>
+#include <asm/gpio.h>
 #include <asm/mach-types.h>
 
-#include <asm/arch/gpio.h>
 #include <asm/arch/mux.h>
 
 #include "leds.h"
@@ -25,17 +26,17 @@ omap_leds_init(void)
 			|| machine_is_omap_perseus2())
 		leds_event = h2p2_dbg_leds_event;
 
+#ifdef	CONFIG_OMAP_OSK_MISTRAL
 	else if (machine_is_omap_osk())
 		leds_event = osk_leds_event;
+#endif
 
 	else
 		return -1;
 
 	if (machine_is_omap_h2()
 			|| machine_is_omap_h3()
-#ifdef	CONFIG_OMAP_OSK_MISTRAL
 			|| machine_is_omap_osk()
-#endif
 			) {
 
 		/* LED1/LED2 pins can be used as GPIO (as done here), or by
@@ -47,14 +48,14 @@ omap_leds_init(void)
 		 * that's a different kind of LED (just one color at a time).
 		 */
 		omap_cfg_reg(P18_1610_GPIO3);
-		if (omap_request_gpio(3) == 0)
-			omap_set_gpio_direction(3, 0);
+		if (gpio_request(3, "timer_led") == 0)
+			gpio_direction_output(3, 0);
 		else
 			printk(KERN_WARNING "LED: can't get GPIO3/red?\n");
 
 		omap_cfg_reg(MPUIO4);
-		if (omap_request_gpio(OMAP_MPUIO(4)) == 0)
-			omap_set_gpio_direction(OMAP_MPUIO(4), 0);
+		if (gpio_request(OMAP_MPUIO(4), "idle_led") == 0)
+			gpio_direction_output(OMAP_MPUIO(4), 0);
 		else
 			printk(KERN_WARNING "LED: can't get MPUIO4/green?\n");
 	}

Re: is there any generic GPIO chip framework like IRQ chips?

[Paul Sokolovsky]

Hello David,

Sunday, April 15, 2007, 10:47:57 PM, you wrote:

> On Thursday 12 April 2007 6:57 am, Paul Sokolovsky wrote:
>> Wednesday, April 11, 2007, 7:52:20 AM, you wrote:

>> > So, talking about what an (optional) implementation framework might
>> > look like (and which could handle the SOC, FPGA, I2C, and MFD cases
>> > I've looked at):

> See patches in following messages ... a preliminary "gpio_chip" core
> for such a framework, plus example support for one SOC family's GPIOs,
> and then updating one board's handling of GPIOs, including over I2C.

        Just to compare, diffstats for GPIODEV:

Core API, 0 bytes of dedicated support code:

diffstat 0001-gpiodev-api.patch
 gpiodev.h |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Adding GPIODEV for GPIOs without dedicated device (would be less if
there was struct device already):

diffstat 0002-gpiodev-for-pxa.patch
 generic.c |   37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)


        While it's understood that your code contains refactors,
extensive error checking, debugging support, etc. it's clear that
gpiolib.c has some weight to drop into kernel.


        But I'm a bit stumbled by that code. What's that? Is it just
example of implementation of your approach? Or is it code to go into
kernel? In this case, it needs work - it doesn't adhere to your own
optimization scheme by using lookup table instead of list.

        Or is it again example of what should be done by folks who
want to use extensible GPIO at all - specifically, hacking platform
header to plug there such framework?

        In the latter case, we again speak about different things, or
about different levels of such things - you speak about constructor
parts which "anyone" can use to construct whatever GPIO API they like,
whereas I'm speaking about exact API implementation which can be used
right away.

[]

> The fact that there are now three simple generic drivers (i2c-gpio,
> leds-gpio, gpio_keys) and various additional drivers using those same
> interfaces -- working already on multiple platforms! -- illustrates
> that anything more than a programming interface, with support on a
> handful of "seed" platforms, wasn't required initially.

        Well, besides gpio_keys we here have asic3_keys, samcop_keys,
etc. - all that duplication just because the current GPIO API doesn't
allow extensibility to more chips.

>>   Yes, and let that's something which can be entailed from your
>> approach: you argue that it's extensible interface, but kernel is
>> exactly not C++ classlib, there must be implementation.

> I said "implementation choice" not "extensible interface"; those
> are two very different approaches.

> You keep bringing C++ into this. 

        Sorry, how could I? I didn't post a single line of C++ code,
vice-versa, posted patches in plain C, which work for any
platform/architecture/etc. Words like "interface" are used only on
design stage. After that, there's just C code ready for calls. There's
no "one implementation strategy is", "another implementation strategy
is", etc.

> You may not know that doesn't win points in Linux.  

        Why, I believe I know local taboos and labels. ;-)

> If the point is solid, it doesn't need to
> rely on parallels to languages that won't be used in the kernel.
> (Plus, why would you imply C++ class libraries don't need to
> have implementations?)


>> And that 
>> implementation: a) will contain inline access for CPU SOC GPIO
>> controllers, just because "they are always there";

> Not what I said.  You said "will", I said "can"; there's a big
> difference.  Consider that for example the OMAP infrastructure
> for GPIOs already has a fair amount of indirection, while most
> other platforms started more simply ... some of them only
> included inlines, others just had simple accessor functions.

> My "can" allows all those; your "will" disallows most of them.

        Yes, in the same sense as some wheel-producing machine
disallows production of square and hexagon wheels.

        As I told, that was one of the reason I decided to submit
my patches as patches, not just RFC, and argue for them - there're
already bunch of square pegs in ARM Linux, and allowing only round
(or square) ones instead of both seems like refreshing change.

> And none of them unified that model with non-SOC GPIOs.

[]

>>   Yes, you argue that anyone who needs to extend Generic GPIO API to
>> their case can do that, and in such way that he won't lose single tick
>> comparing to using direct low-level methods. 

> Again, not what I said ... I've discouraged changing/extending the
> programming interface.  But I've certainly pointed out ways that
> existing performance expectations can be preserved without changing
> that programming interface.

        Ok, so this, IMHO, keeps to be the most valid point in favor
of continuing to use flat GPIO namespace. Challenging that would mean
to challenge original requirements for API, and that was done in
January, and I don't want to return to that.

[]

>> 2) Thankfully, we don't have gpio_chip yet. The temptation to clone
>> irq_chip, irq_desc, and stuff is high. So, what we talk here about is
>> not only how to implement extensible GPIO, but whether we finally can
>> break away from need to use in-kernel bureaucracies for handling each
>> and every (low-level) object.

> By "bureaucracy" I presume you mean the existence of opaque cookies?
> Or do you mean the implied resource tracking? 

        No, just requirement to do superflous operations, like, first
adding something to something, then storing what was added somewhere in
the table, just to substract it later then.
 
> (Remember that one of
> the most basic tasks of an operating system is resource management...)

        Yes, and simple resources can be even managed efficiently, as
GPIODEV API shows ;-).

[]

> So you're agreeing that, at a technical level, what I described
> could be augmented by a "caching" facility ... giving a programming
> interface with all the characteristics of your "GPIODEV" thingie.

> All you're really disagreeing with is bootstrapping issues; and
> whether there is in fact a need for such a layer.  The only argument
> I could possibly buy is that it avoids the lookup of (b) ... but
> that doesn't seem to matter in most cases I've looked at.

        So, yes, I disagree that using scalar identifiers is the most
efficient method. I however hardly can disagree that it's still a
suitable method. My other arguments are of wider scope than GPIO API
per se.

        One thing I obviously agree is that we need standard extensible
GPIO API, no matter what it is. And of course, you, as the API maintainer,
have better insight into this topic, and need to preserve its local
consistency (even if that, per my view, follows things which deserve
nomination of anti-patterns).

        So, now the most important question is what we all would get
with your approach in the end.

        So, if you could make sure gpiolib.c doesn't contain inefficient
implementation, and make such extensible implementation available by
default for ARM PXA/S3Cxxx/OMAP, then it's for sure cover Handhelds.org's,
and many other peoples' usecases, and that would be highly
appreciated.

        If you could do it for 2.6.22 merge window, that would
straight ideal.


[]

> Most of those don't get looked up in GPIO tables even once
> a week after the system boots ... the exceptions being the
> various LEDs, which obviously aren't performance-critical.
> IRQs go another path, and the various power controls don't
> change often after boot either.

        I would agree with this from pragmatical approach -
our usecase for handhelds.org is the same, we don't do
bitbanging on GPIOs. Other thing is when people would need
it, but ... - everything was said in that respect already.

> FWIW, GPIO 37 is ttyS0.RXD used to wake up from sleep,
> IRQ-164 is the touchscreen IRQ, and GPIO-6 is a busy signal
> from the touchscreen that's not actually used.  They don't
> yet use gpio_request(), which is why they're not labeled.


>> > Note that I'm giving you the benefit of the doubt in several respects
>> > there.  Your original (a) was quite bizarre, and your updated one isn't
>> > a lot better because it still needs odd conversions.
>> 
>>   Well, that's why I used macro initially - to assign some sense to
>> those low-level typecasts. You and other people told macros must get
>> lost, so they are, and I guess, it's unfair now to say that typecast
>> sores you eye - I tried to save everyone from that. But in the end,
>> it's C, not me, or GPIODEV. I can rewrite it in asm, and there won't
>> be typecasts. But we know why we don't write in asm ;-).

> The issue I was referring to was abuse of "void *" pointers when
> you should have been using typesafe ones. 

        My argument stays the same - it's C, so that's why it's
written that way. If that was Perl or Java, I would use hash to
convert from identifier to reference, but with C, it's optional.

>  And the association
> with devices was wierd too.

        Why? There's already abstraction of a device - struct
device. A device provides GPIO operations, so the operations are
attached to struct device. Otherwise, for how long there will be
parallel structure proliferation? Almost all devices are already
struct device, but some of them also require separately maintained
structure - irq_chip, gpio_chip, etc. Merging of classdev vs dev
structures and their trees is on TODO, but in the meantime we'll
spawn bunch more parallel structures...

> - Dave



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

Re: is there any generic GPIO chip framework like IRQ chips?

[David Brownell]

> >> > So, talking about what an (optional) implementation framework might
> >> > look like (and which could handle the SOC, FPGA, I2C, and MFD cases
> >> > I've looked at):
> 
> > See patches in following messages ... a preliminary "gpio_chip" core
> > for such a framework, plus example support for one SOC family's GPIOs,
> > and then updating one board's handling of GPIOs, including over I2C.
> 
>         Just to compare, diffstats for GPIODEV:

Now, if they were functionally equivalent, such a comparison
would be less of an apples/oranges thing!

The most useful comparison would focus on technical aspects of
the gpio_chip abstraction itself (i.e. $SUBJECT).


> 	it needs work - it doesn't adhere to your own 
> optimization scheme by using lookup table instead of list.

I thought it was more important to address the $SUBJECT first:
get a working gpio_chip abstraction which covers all the needed
functionality.  The patch had a hook for implementing such tweaks,
but it wasn't used.

The next version you'll see lets the platform code use its own
existing lookup code, as part of slimming things down a bit.
I also decided to take out the debugfs support.


>         	 you speak about constructor
> parts which "anyone" can use to construct whatever GPIO API they like,
> whereas I'm speaking about exact API implementation which can be used
> right away.

I most certainly did not speak about "whatever GPIO API they like"!!

Quite the contrary, in fact.  Please don't put words in my mouth.
(You've been doing it quite extensively in this thread; it's rude.)

And that "core" patch I posted was clearly usable "right away";
otherwise the two examples _using_ it couldn't have worked.


>         Well, besides gpio_keys we here have asic3_keys, samcop_keys,
> etc. - all that duplication just because the current GPIO API doesn't
> allow extensibility to more chips.

When I get tired of repeating myself, just remember:  the current
programming interface *DOES* allow such extensibility.  That's what it
means to be an "interface", rather than an implementation:  it defines
inputs and outputs, allowing any process that conforms to both.

In fact, the patches I sent demonstrated exactly that extensibility.
Same interface, additional chips; different implementation inside.


> > So you're agreeing that, at a technical level, what I described
> > could be augmented by a "caching" facility ... giving a programming
> > interface with all the characteristics of your "GPIODEV" thingie.
> 
> > All you're really disagreeing with is bootstrapping issues; and
> > whether there is in fact a need for such a layer.  The only argument
> > I could possibly buy is that it avoids the lookup of (b) ... but
> > that doesn't seem to matter in most cases I've looked at.


> So, now the most important question is what we all would get
> with your approach in the end.
> 
> So, if you could make sure gpiolib.c doesn't contain inefficient
> implementation,

I can make it comparable to existing implementations that work
the same way ... e.g. AT91 and OMAP code.  Of course, it's not
possible to get away from the cost of function indirection, with
a generic gpio_chip abstraction.  Or those lookup costs; but as
you agreed, those costs don't seem to matter much.  And if they
ever do matter, caching support would be easy to add.


> and make such extensible implementation available by default
> for ARM PXA/S3Cxxx/OMAP, then it's for sure cover Handhelds.org's,
> and many other peoples' usecases, and that would be highly
> appreciated.
> 
> If you could do it for 2.6.22 merge window, that would
> straight ideal.

I think having an optional gpio_chip, not unlike what was in
that one patch, should be reasonable; also, making it work on
some platforms that I use.  But I don't think there's much
overlap between those platforms and what hh.org uses.

- Dave

Re: is there any generic GPIO chip framework like IRQ chips?

[Paul Sokolovsky]

Hello David,

Thursday, April 19, 2007, 5:22:44 AM, you wrote:

>> >> > So, talking about what an (optional) implementation framework might
>> >> > look like (and which could handle the SOC, FPGA, I2C, and MFD cases
>> >> > I've looked at):
>> 
>> > See patches in following messages ... a preliminary "gpio_chip" core
>> > for such a framework, plus example support for one SOC family's GPIOs,
>> > and then updating one board's handling of GPIOs, including over I2C.
>> 
>>         Just to compare, diffstats for GPIODEV:

> Now, if they were functionally equivalent, such a comparison
> would be less of an apples/oranges thing!

        But of course they are functionally equivalent! They do the
same thing - manage GPIOs. They even do it in very similar ways. To
remind, the only differences we (I) come to, are:

1. Structured GPIO identifiers instead of scalar.
2. No explicit adhoc structures similar to irq_desc & irq_chip.
3. Lesser (even though slightly) latency.
4. Not tied to not really directly related notions like "platform".

        Any optimization which you apply to your implementation can be
applied to GPIODEV. GPIODEV was submitted in that form because its
first requirement was to replace simple chip-adhoc GPIO accessors, so
there's no additional things like reporting GPIO info to userspace
(but again, that can be added, even based on your implementation).


> The most useful comparison would focus on technical aspects of
> the gpio_chip abstraction itself (i.e. $SUBJECT).

        Nice. But $SUBJECT is a specific question, and it was answered
in one of the first messages of the thread: No, there's no GPIO chip
framework like IRQ chips [in the mainline], and there're concerns if a
framework in such form ("like IRQ chips") is needed at all, and
alternative implementation was proposed.

>>       it needs work - it doesn't adhere to your own
>> optimization scheme by using lookup table instead of list.

> I thought it was more important to address the $SUBJECT first:

        Well, is this your last argument why your implementation is
better: GPIO chip framework is needed just because there's some random
mail subject which mentions it?

        Anyway, the discussion happens under this subject just because
current of discussion followed in this way. The one of topics of
the discussion was presentation of alternative to gpio_chip
implementation, so please don't tell I do offtopic here.

> get a working gpio_chip abstraction which covers all the needed
> functionality.  The patch had a hook for implementing such tweaks,
> but it wasn't used.

> The next version you'll see lets the platform code use its own
> existing lookup code, as part of slimming things down a bit.
> I also decided to take out the debugfs support.


>>                you speak about constructor
>> parts which "anyone" can use to construct whatever GPIO API they like,
>> whereas I'm speaking about exact API implementation which can be used
>> right away.

> I most certainly did not speak about "whatever GPIO API they like"!!

> Quite the contrary, in fact.  Please don't put words in my mouth.
> (You've been doing it quite extensively in this thread; it's rude.)

        I kindly apologize if heat of discussion led me so far. But
maybe information you supplied was a bit scarce, and that made me
imagine too much?

        Let's recollect with what the discussion started: proposal
from myself and other HH.org developers of the alternative implementation,
and you quickly verdicted that without specific patches, you stop the
discussion. They were posted, technical aspects of them were
discussed, and compared to your proposed implementation. In the end,
you posted your patches, which doesn't even correspond with the
implementation you yourself presented, and adapted only for OMAP.

        And we both understand well why we can't reach agreement here:
we represent different communities with different needs. You
represent "platform hacking" community, and it's all sunny in your
realm, while we're machine-hacking guys, and we'd rather exactly hack
machines, but regularly bump into different "platform" and
"architecture" issues which we'd be glad not to deal with at all. But
we have to, and then based on this painful experience we design new
things which are not unnecessarily tied to specific platform or
architecture, at the same time trying to alleviate burden on
platform on architecture maintainers.

> And that "core" patch I posted was clearly usable "right away";
> otherwise the two examples _using_ it couldn't have worked.

        It works "right away" only on OMAP. For other (sub)archs,
header patching is required and that's clearly not "right away" for
the scope of mainline Linux, which we discuss here (because it makes
little sense to discuss feudalistic trees at all - in them, anything
can be done in any way without my or your guidelines how to do that).

        Now that other (sub)archs need cut'n'paste patches, it's quite
another task to push them back to mainline, as again, that would go
thru overbusy platform and architecture maintainers, and their
platforms and architectures oftentimes have more priority stuff to be
done, or have quite another vision at all, etc. etc.


>>         Well, besides gpio_keys we here have asic3_keys, samcop_keys,
>> etc. - all that duplication just because the current GPIO API doesn't
>> allow extensibility to more chips.

> When I get tired of repeating myself, just remember:  the current
> programming interface *DOES* allow such extensibility.

        Well, I tirelessly keep agreeing to that, like I did
thruout the whole thread. It's just way of such extensibility is
challenged, and arguably better way is proposed.

>   That's what it
> means to be an "interface", rather than an implementation:  it defines
> inputs and outputs, allowing any process that conforms to both.

> In fact, the patches I sent demonstrated exactly that extensibility.
> Same interface, additional chips; different implementation inside.


>> > So you're agreeing that, at a technical level, what I described
>> > could be augmented by a "caching" facility ... giving a programming
>> > interface with all the characteristics of your "GPIODEV" thingie.
>> 
>> > All you're really disagreeing with is bootstrapping issues; and
>> > whether there is in fact a need for such a layer.  The only argument
>> > I could possibly buy is that it avoids the lookup of (b) ... but
>> > that doesn't seem to matter in most cases I've looked at.


>> So, now the most important question is what we all would get
>> with your approach in the end.
>> 
>> So, if you could make sure gpiolib.c doesn't contain inefficient
>> implementation,

> I can make it comparable to existing implementations that work
> the same way ... e.g. AT91 and OMAP code.

        Why would you compare it to such implementations? Please
compare it to the patches that we submitted, which address the same
problem as yours, not to some random implementation for specific
architecture.

>   Of course, it's not
> possible to get away from the cost of function indirection, with
> a generic gpio_chip abstraction.  Or those lookup costs; but as
> you agreed, those costs don't seem to matter much.  And if they
> ever do matter, caching support would be easy to add.


>> and make such extensible implementation available by default
>> for ARM PXA/S3Cxxx/OMAP, then it's for sure cover Handhelds.org's,
>> and many other peoples' usecases, and that would be highly
>> appreciated.
>> 
>> If you could do it for 2.6.22 merge window, that would
>> straight ideal.

> I think having an optional gpio_chip, not unlike what was in
> that one patch, should be reasonable; also, making it work on
> some platforms that I use.  But I don't think there's much
> overlap between those platforms and what hh.org uses.

        Pity to hear. Especially taking into account that we proposed
the implementation which works on any platform and even architecture
at all (in the sense that it doesn't require special assistance from
platform to allow extensible GPIO handling in the first place).

        I also hear shade of encouragement to keep elaborating
and submitting GPIODEV in your words, but unfortunately that's not
really what I'd like. The whole argument was about having one good
runtime-extensible framework, not two. And I can only accept your
primacy in this question. For us it is after all just one of very many
good patches we'd like to share with community, so I'd better move to
next of them.


> - Dave



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

Re: is there any generic GPIO chip framework like IRQ chips?

[David Brownell]

On Monday 23 April 2007, Paul Sokolovsky wrote:
> Hello David,
> 
> Thursday, April 19, 2007, 5:22:44 AM, you wrote:
> 
> >> >> > So, talking about what an (optional) implementation framework might
> >> >> > look like (and which could handle the SOC, FPGA, I2C, and MFD cases
> >> >> > I've looked at):
> >> 
> >> > See patches in following messages ... a preliminary "gpio_chip" core
> >> > for such a framework, plus example support for one SOC family's GPIOs,
> >> > and then updating one board's handling of GPIOs, including over I2C.
> >> 
> >>         Just to compare, diffstats for GPIODEV:
>
> > Now, if they were functionally equivalent, such a comparison
> > would be less of an apples/oranges thing!
> 
>         But of course they are functionally equivalent! They do the
> same thing - manage GPIOs. They even do it in very similar ways.

Functional equivalence means handling all the capabilities
defined in Documentation/gpio.txt ... they are defined
because without them, the programming interface wouldn't
cover significant capabilities folk asked for.

You omitted notions of:  spinlock safe/unsafe operation,
as needed to handle e.g. register access over I2C/SPI
versus spinlock-safe access; request/reserve, minimizing
inter-driver conflicts; direction setting, since it's most
often mutable.

It's no surprise when less-functional code is smaller...


> >>       it needs work - it doesn't adhere to your own
> >> optimization scheme by using lookup table instead of list.
> 
> > I thought it was more important to address the $SUBJECT first:
> 
>         Well, is this your last argument why your implementation is
> better: GPIO chip framework is needed just because there's some random
> mail subject which mentions it?

I certainly pointed out that something like a gpio_chip would
be easy enough to do, back when the programming interfaces were
first discussed ... in the context of supporting more than the
initial implementations of platform-level SOC style GPIOs, after
things are under way.

Working on that was what I've called a "phase III" issue ... where
"phase I" was a programming interface that could be be easily and
widely adopted, and "phase II" was the initial adoption (platforms
and drivers).  With "phase II" well under way, broadening the scope
of implementations now make sense.


>         Let's recollect with what the discussion started: 

Depends which discussion you mean.  I was continuing previous
ones; you seemed to want to start a new one.


>         proposal 
> from myself and other HH.org developers of the alternative implementation,

No, that original proposal was a *REPLACEMENT INTERFACE* along
with a fairly nasty C++ish implementation.  You then reposted that
same code to a wider audience after ignoring the initial feedback,
then later a revised version with minor changes.  (After my latest
updates were working on multiple boards, but before I posted them.)


>         And we both understand well why we can't reach agreement here:
> we represent different communities with different needs. 

That's not my understanding at all.  Though you're right to
imply that the HH.org community seems to **feel** (for reasons
opaque to most people) that its embedded needs are for some
reason different from those of folk working more closely with
the kernel.org codebase.

(IMO it's a microcosm of a classic embedded product approach:
hack a bunch of stuff so it works, try throwing it upstream,
and only then notice that merging takes real work, with thought
for what other systems with related problems need.)


> >> So, now the most important question is what we all would get
> >> with your approach in the end.
> >> 
> >> So, if you could make sure gpiolib.c doesn't contain inefficient
> >> implementation,
> 
> > I can make it comparable to existing implementations that work
> > the same way ... e.g. AT91 and OMAP code.
> 
>         Why would you compare it to such implementations?

Because the alternatives were *MUCH* lighter weight (much less
flexible), so any comparisons would be unfair!  :)

 
>         I also hear shade of encouragement to keep elaborating
> and submitting GPIODEV in your words, but unfortunately that's not 
> really what I'd like. The whole argument was about having one good
> runtime-extensible framework, not two.

The thing that's potentially interesting in what you've said is
what an API using {gpio_chip, index} addressing would look like.
The extensibility would be through gpio_chip.

The notion has come up before (even from me!), and I recently
noticed some CRIS GPIO code looking something like that.

It's not clear such a thing is needed very often ... but as I've
structured things, it would be easy to add such a layer, should
it turn out to be necessary.  That's why "struct gpio_chip" is
such a relevant next step.

- Dave