* Re: is there any generic GPIO chip framework like IRQ chips?
2007-04-12 13:57 ` Paul Sokolovsky
@ 2007-04-15 19:47 ` David Brownell
2007-04-17 16:05 ` Paul Sokolovsky
2007-04-15 19:53 ` David Brownell
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: David Brownell @ 2007-04-15 19:47 UTC (permalink / raw)
To: Paul Sokolovsky; +Cc: linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: is there any generic GPIO chip framework like IRQ chips?
2007-04-15 19:47 ` David Brownell
@ 2007-04-17 16:05 ` Paul Sokolovsky
2007-04-19 2:22 ` David Brownell
0 siblings, 1 reply; 11+ messages in thread
From: Paul Sokolovsky @ 2007-04-17 16:05 UTC (permalink / raw)
To: David Brownell; +Cc: linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: is there any generic GPIO chip framework like IRQ chips?
2007-04-17 16:05 ` Paul Sokolovsky
@ 2007-04-19 2:22 ` David Brownell
2007-04-23 8:45 ` Paul Sokolovsky
0 siblings, 1 reply; 11+ messages in thread
From: David Brownell @ 2007-04-19 2:22 UTC (permalink / raw)
To: Paul Sokolovsky; +Cc: linux-arm-kernel, linux-kernel
> >> > 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: is there any generic GPIO chip framework like IRQ chips?
2007-04-19 2:22 ` David Brownell
@ 2007-04-23 8:45 ` Paul Sokolovsky
2007-04-24 4:52 ` David Brownell
0 siblings, 1 reply; 11+ messages in thread
From: Paul Sokolovsky @ 2007-04-23 8:45 UTC (permalink / raw)
To: David Brownell; +Cc: linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: is there any generic GPIO chip framework like IRQ chips?
2007-04-23 8:45 ` Paul Sokolovsky
@ 2007-04-24 4:52 ` David Brownell
0 siblings, 0 replies; 11+ messages in thread
From: David Brownell @ 2007-04-24 4:52 UTC (permalink / raw)
To: Paul Sokolovsky; +Cc: linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: is there any generic GPIO chip framework like IRQ chips?
2007-04-12 13:57 ` Paul Sokolovsky
2007-04-15 19:47 ` David Brownell
@ 2007-04-15 19:53 ` David Brownell
2007-04-15 19:58 ` David Brownell
2007-04-15 20:03 ` David Brownell
3 siblings, 0 replies; 11+ messages in thread
From: David Brownell @ 2007-04-15 19:53 UTC (permalink / raw)
To: Paul Sokolovsky; +Cc: linux-arm-kernel, linux-kernel
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
+
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: is there any generic GPIO chip framework like IRQ chips?
2007-04-12 13:57 ` Paul Sokolovsky
2007-04-15 19:47 ` David Brownell
2007-04-15 19:53 ` David Brownell
@ 2007-04-15 19:58 ` David Brownell
2007-04-15 20:03 ` David Brownell
3 siblings, 0 replies; 11+ messages in thread
From: David Brownell @ 2007-04-15 19:58 UTC (permalink / raw)
To: Paul Sokolovsky; +Cc: linux-arm-kernel, linux-kernel
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)
{
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: is there any generic GPIO chip framework like IRQ chips?
2007-04-12 13:57 ` Paul Sokolovsky
` (2 preceding siblings ...)
2007-04-15 19:58 ` David Brownell
@ 2007-04-15 20:03 ` David Brownell
3 siblings, 0 replies; 11+ messages in thread
From: David Brownell @ 2007-04-15 20:03 UTC (permalink / raw)
To: Paul Sokolovsky; +Cc: linux-arm-kernel, linux-kernel
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");
}
^ permalink raw reply [flat|nested] 11+ messages in thread