generic gpio -- gpio_chip support

Provide new implementation infrastructure that platforms may choose to use when implementing the GPIO programming interface. Platforms can update their GPIO support to use this. In many cases the incremental cost to access a non-inlined GPIO should be on the order of a dozen instructions, so it won't normally be a problem. The upside is:

* Providing two features which were "want to have (but OK to defer)" when GPIO interfaces were first discussed in November 2006:

- A "struct gpio_chip" to plug in GPIOs that aren't directly supported
by SOC platforms, but come from FPGAs or other multifunction devices using conventional device registers (like UCB-1x00 or SM501 GPIOs, and southbridges in PCs with more open specs than usual).

- Full support for message-based GPIO expanders, where registers are
accessed through sleeping I/O calls. Previous support for these "cansleep" calls was just stubs. (One example: the widely used pcf8574 I2C chips, with 8 GPIOs each.)

* Including a non-stub implementation of the gpio_{request,free}() calls, making those calls much more useful. The diagnostic labels are also recorded given DEBUG_FS, so /sys/kernel/debug/gpio can show a snapshot of all GPIOs known to this infrastructure.

The driver programming interfaces introduced in 2.6.21 do not change at all; this infrastructure is entirely below those covers.

This opens the door to an augmented programming interface, addressing GPIOs by chip and index. That could be used as a performance tweak (lookup once then cache, avoiding locking and lookup overheads) or to support transient GPIOs not registered in the integer GPIO namespace (maybe a USB-to-GPIO adapter, or GPIOs coupled to some other type of add-on card).

...

+/* There's no value in inlining GPIO calls that may sleep.

+postcore_initcall(gpiolib_debugfs_init);

lib/gpiolib.c line 36

Quoted 11 lines · On Fri, 9 Nov 2007 11:36:19 -0800 David Brownell <[email protected]> wrote:

Provide new implementation infrastructure that platforms may choose to use when implementing the GPIO programming interface. ...

...

lib/gpiolib.c new file +6 -0

+/* gpio_lock protects the table of chips and to gpio_chip->requested.
+ * While any gpio is requested, its gpio_chip is not removable.  It's
+ * a raw spinlock to ensure safe access from hardirq contexts, and to
+ * shrink bitbang overhead:  per-bit preemption would be very wrong.
+ */
+static raw_spinlock_t gpio_lock = __RAW_SPIN_LOCK_UNLOCKED;

Well that's weird.

For starters, this initialisation will confound lockdep: it should use DEFINE_SPINLOCK.

And the rationale seems dubious. All you're saving here is a couple of accesses to task_struct at spin_unlock()-time. If the current task has a preemption pending then yes, we'll schedule away but that's a very rare thing and that's just what we're supposed to do.

So please tell us more about this. Perhaps there are performance problems with the current core preemption machinery.

lib/gpiolib.c line 101

Quoted 8 lines

lib/gpiolib.c new file +6 -0

+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
 
...
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+	return status;
+}

And of course if this code is converted to conventional locking, the above becomes spin_lock_irqsave()/spin_lock_irqrestore() in many places.

Quoted 1 line

+/* There's no value in inlining GPIO calls that may sleep.

There's no value in inlining anything, hardly ;)

Quoted 1 line

+postcore_initcall(gpiolib_debugfs_init);

postcore_initcall() is unusual, hence a comment describing why it was employed would be a good thing to have.

Quoted 16 lines · Andrew Morton

lib/gpiolib.c line 36

Quoted 11 lines · On Fri, 9 Nov 2007 11:36:19 -0800 David Brownell <[email protected]> wrote:

Provide new implementation infrastructure that platforms may choose to use when implementing the GPIO programming interface. ...

...

lib/gpiolib.c new file +6 -0

+/* gpio_lock protects the table of chips and to gpio_chip->requested.
+ * While any gpio is requested, its gpio_chip is not removable.  It's
+ * a raw spinlock to ensure safe access from hardirq contexts, and to
+ * shrink bitbang overhead:  per-bit preemption would be very wrong.
+ */
+static raw_spinlock_t gpio_lock = __RAW_SPIN_LOCK_UNLOCKED;

Well that's weird.

Quoted 2 lines

For starters, this initialisation will confound lockdep: it should use DEFINE_SPINLOCK.

Then it wouldn't be "raw"! It seems info on raw spinlocks is out of sync with current code. Allegedly it should be possible to just pass a raw_spinlock_t pointer to spin_lock_irqsave() and friends and have GCC sort out the right stuff ... but that didn't work.

I speculate that either the design has changed (without fanfare), or else that stuff is in RT kernels and has not yet gone upstream.

Quoted 5 lines

And the rationale seems dubious. All you're saving here is a couple of accesses to task_struct at spin_unlock()-time. If the current task has a preemption pending then yes, we'll schedule away but that's a very rare thing and that's just what we're supposed to do.

Unfortunately, that's not what I observed/measured.

http://marc.info/?l=linux-kernel&m=119429680220361&w=2

Plus, note the comment about hardirq context and RT environments. When that spinlock is really a mutex, and the non-hardirq contexts are tasks, non-raw spinlocks would wrongly prevent access to GPIOs from true hardirq contexts.

Quoted 3 lines

So please tell us more about this. Perhaps there are performance problems with the current core preemption machinery.

The above email summarizes significant slowdown I observed using just i2c-gpio, which happened to be easy to measure down to the microsecond level.

Quoted 11 lines

lib/gpiolib.c line 101

Quoted 8 lines

lib/gpiolib.c new file +6 -0

+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
 
...
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+	return status;
+}

And of course if this code is converted to conventional locking, the above becomes spin_lock_irqsave()/spin_lock_irqrestore() in many places.

See above. Allegedly we ought to be able to use those calls even with raw spinlocks ... but that didn't work when I tried it. I'd certainly prefer if it did work.

Quoted 4 lines

Quoted 2 lines

+/* There's no value in inlining GPIO calls that may sleep.

There's no value in inlining anything, hardly ;)

Remember that GPIOs do get used for bitbanging, and there's lots of kernel code to make bit ops stay fast!

The canonical reason to inline GPIO operations is so that when you bitbang some serial protocol, you're closer to one instruction per bit toggle than fifty ... at one point I measured a factor of nearly three speedup for byte-level bitbanging for SPI (ca. 70 Kbit with quick function calls, vs about 2 Mbits inlined, ISTR) ... with most of the overhead being to pass the next byte down to the bitbang loop. Using a bigger wordsize saw even more improvement.

Quoted 4 lines

Quoted 1 line

+postcore_initcall(gpiolib_debugfs_init);

postcore_initcall() is unusual, hence a comment describing why it was employed would be a good thing to have.

Actually that one could easily be later, fs_initcall or whatnot. I think that's debris from other versions.

- Dave

Quoted 7 lines

It seems info on raw spinlocks is out
of sync with current code. Allegedly it should be possible to just pass a raw_spinlock_t pointer to spin_lock_irqsave() and friends and have GCC sort out the right stuff ... but that didn't work.

I speculate that either the design has changed (without fanfare), or else that stuff is in RT kernels and has not yet gone upstream.

Well whatever. We shouldn't have to resort to caller-side party tricks like this to get acceptable performance.

Quoted 4 lines

Quoted 4 lines

And the rationale seems dubious. All you're saving here is a couple of accesses to task_struct at spin_unlock()-time. If the current task has a preemption pending then yes, we'll schedule away but that's a very rare thing and that's just what we're supposed to do.

Quoted 4 lines

Unfortunately, that's not what I observed/measured.

http://marc.info/?l=linux-kernel&m=119429680220361&w=2

There's something missing here. It went from 6.4 usec/bit up to 11.2 usec/bit. You seem to be saying that enabling preemption accounts for 1 usec. What caused the rest of the slowdown?

Quoted 4 lines

Plus, note the comment about hardirq context and RT environments. When that spinlock is really a mutex, and the non-hardirq contexts are tasks, non-raw spinlocks would wrongly prevent access to GPIOs from true hardirq contexts.

I don't understand all this about raw spinlocks and hardirq context. We use plain old spin_lock()/spin_unlock() in hardirq context all the time?

I'm still trying to understand what you've observed here. Is it the case that a single gpio operation went from 6.4 up to 11.2 usecs?

And that this
operation does a single spin_lock/spin_unlock? If so, something would have had to have gone seriously wrong for a spin_lock/unlock time to increase by 4.8 microseconds.

I assume these timings are from a reasonably fast machine?

Quoted 12 lines

Quoted 11 lines

lib/gpiolib.c line 101

Quoted 8 lines

lib/gpiolib.c new file +6 -0

+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
 
...
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+	return status;
+}

And of course if this code is converted to conventional locking, the above becomes spin_lock_irqsave()/spin_lock_irqrestore() in many places.

Quoted 3 lines

See above. Allegedly we ought to be able to use those calls even with raw spinlocks ... but that didn't work when I tried it. I'd certainly prefer if it did work.

It would of course be better to not use raw spinlocks at all. It's an internal implementation detail and drivers shouldn't go poking at it:

So can we please drill down and work out what went wrong and see if we can fix it all up properly?

Quoted 8 lines

Quoted 8 lines

Fortuntely it turns out those problems all go away if the gpiolib code uses a *raw* spinlock to guard its table lookups...

That's as it should be, since the only substantive changes were to grab and release a lock, do one table lookup a bit differently, and add one indirect function call ... changes which should not have any visible performance impact on per-bit codepaths, and one might expect to cost on the order of one dozen instructions.

Quoted 5 lines

Quoted 2 lines

I speculate that either the design has changed (without fanfare), or else that stuff is in RT kernels and has not yet gone upstream.

Well whatever. We shouldn't have to resort to caller-side party tricks like this to get acceptable performance.

I'd be happy if, as originally presented, it were possible to just pass a raw_spinlock_t to spin_lock_irqsave() and friends.

Quoted 3 lines · David Brownell

Quoted 3 lines

Quoted 2 lines

I speculate that either the design has changed (without fanfare), or else that stuff is in RT kernels and has not yet gone upstream.

Quoted 5 lines

Quoted 2 lines

Well whatever. We shouldn't have to resort to caller-side party tricks like this to get acceptable performance.

I'd be happy if, as originally presented, it were possible to just pass a raw_spinlock_t to spin_lock_irqsave() and friends.

that's a spinlock type abstraction of PREEMPT_RT, not of mainline.

Why do you want to use raw_spinlock_t?

Quoted 12 lines · Ingo Molnar

Quoted 8 lines · David Brownell

Quoted 5 lines

Quoted 2 lines

I speculate that either the design has changed (without fanfare), or else that stuff is in RT kernels and has not yet gone upstream.

Well whatever. We shouldn't have to resort to caller-side party tricks like this to get acceptable performance.

I'd be happy if, as originally presented, it were possible to just pass a raw_spinlock_t to spin_lock_irqsave() and friends.

Quoted 1 line

that's a spinlock type abstraction of PREEMPT_RT, not of mainline.

Any reason that stuff shouldn't move into mainline?

Quoted 1 line

Why do you want to use raw_spinlock_t?

Already answered elsewhere in this thread ...

I'll highlight the
point that such bitops shouldn't be preemption points.

Quoted 4 lines

Quoted 2 lines

     Why do you want to use raw_spinlock_t?

Already answered elsewhere in this thread ...

Can't say I really understood the answer.  I don't think we actually know where all of this extra time is being spent?

Quoted 2 lines

I'll highlight the
point that such bitops shouldn't be preemption points.

Disagree.  *everything* should be a preemption point.

For internal-implementation
details we do need to disable preemtion sometimes (to prevent deadlocks and to protect per-cpu resources).

But those
preemption-off periods should be minimised.

Quoted 4 lines · Andrew Morton

Quoted 2 lines

I'll highlight the
point that such bitops shouldn't be preemption points.

Disagree.  *everything* should be a preemption point.

So it's wrong that <asm-generic/bitops/atomic.h> uses the same calls to prevent *those* bitops from being prempted?

Thus, that code should switch over to normal spinlocks...

I believe that if I submitted patches to do that, there'd be a not-so-small riot. And the arguments would all boil down to much the same ones applying to *these* bitops...

Quoted 3 lines

For internal-implementation
details we do need to disable preemtion sometimes (to prevent deadlocks and to protect per-cpu resources).

You're certainly talking about "internal-implementation details" in this case. It's not like the lock is used outside of those routines. Or as if other implementations would even *need* such a lock. (Just like the IRQ table, if the entries can't be removed and are all set up very early, locking would be pointless.)

Quoted 5 lines · David Brownell

Quoted 3 lines · Andrew Morton

Quoted 2 lines

I'll highlight the
point that such bitops shouldn't be preemption points.

Quoted 4 lines

Quoted 1 line

Disagree.  *everything* should be a preemption point.

So it's wrong that <asm-generic/bitops/atomic.h> uses the same calls to prevent *those* bitops from being prempted?

Upstream, all spinlocks prevent preemption.

But these ones
are raw locks rather than normal locks probably because that they are trivially an innermost and correct lock.

Quoted 1 line · Nick Piggin

Upstream, all spinlocks prevent preemption.

I chose my wording carefully though. A preemption point is more than just a small region where preemption isn't allowed.

It's one of those where preemption is *INVITED* ...

Now, in the RT case, I believe the rationale for inviting preemption when dropping a lock is largely related to the way priority inversion is handled. When lock contention can block higher priority activities, dropping the lock must be able to trigger the relevant activity switch.

... and the raw spinlocks don't support that machinery, while "normal" spinlocks become inversion-aware mutexes.

Quoted 3 lines

But these ones
are raw locks rather than normal locks probably because that they are trivially an innermost and correct lock.

As in the $SUBJECT case, I'd say.

Although another point is related to "trivial": the data is being protected through an operation too trivial to be worth paying for any of that priority logic.

Quoted 9 lines

Quoted 3 lines

But these ones
are raw locks rather than normal locks probably because that they are trivially an innermost and correct lock.

As in the $SUBJECT case, I'd say.

Although another point is related to "trivial":  the data is being protected through an operation too trivial to be worth paying for any of that priority logic.

A driver shouldn't get to decide that, IMO.

And if there is
some policy in the -rt tree allowing these decisions, then it's exactly the kind of thing we don't want upsream.

Although another point is related to "trivial": the data is being protected through an operation too trivial to be worth paying for any of that priority logic.

Quoted 3 lines · On Wed, 14 Nov 2007 00:37:57 -0800 David Brownell <[email protected]> wrote:

Although another point is related to "trivial": the data is being protected through an operation too trivial to be worth paying for any of that priority logic.

But isn't there any way we can remove the lock from the fast path altogether? What is it really protecting?

Since this is the code that runs under the lock

(excluding the "extra checks" case):

(quoted diff) +4 -0

+static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
+{
+	return chips[gpio / ARCH_GPIOS_PER_CHIP];
+}

I'd say it protects against chips being removed in the middle of the operation. However, this comment says that chips cannot be removed while any gpio on it is requested:

+/* gpio_lock protects the table of chips and to gpio_chip->requested.
+ * While any gpio is requested, its gpio_chip is not removable. It's
+ * a raw spinlock to ensure safe access from hardirq contexts, and to
+ * shrink bitbang overhead: per-bit preemption would be very wrong.
+ */

And since we drop the lock before calling the actual get/set bit operation, we would be screwed anyway if the chip was removed during the call to __gpio_set_value(). So what does the lock really buy us?

Quoted 1 line

Since this is the code that runs under the lock

No, there's more than that. This is what runs under it in the hot paths, yes, but the gpio request/free paths do more work than this. (That includes direction setting, since that can be an implicit request.)

The get/set bit calls are the hot paths. Locking on those paths buys us a consistent locking policy, which is obviously correct. It's consistent with the request/free paths.

But I think what you're suggesting is that the "requested" flag is effectively a long-term lock, so grabbing the spinlock on those paths is not necessary. Right?

Quoted 4 lines · Ingo Molnar

Quoted 3 lines · David Brownell

Quoted 3 lines

Quoted 2 lines

I speculate that either the design has changed (without fanfare), or else that stuff is in RT kernels and has not yet gone upstream.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Well whatever. We shouldn't have to resort to caller-side party tricks like this to get acceptable performance.

I'd be happy if, as originally presented, it were possible to just pass a raw_spinlock_t to spin_lock_irqsave() and friends.

that's a spinlock type abstraction of PREEMPT_RT, not of mainline.

Any reason that stuff shouldn't move into mainline?

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

I'd be happy if, as originally presented, it were possible to just pass a raw_spinlock_t to spin_lock_irqsave() and friends.

that's a spinlock type abstraction of PREEMPT_RT, not of mainline.

Even when you're talking about the -rt tree, I suspect you really shouldn't be using raw spinlocks, right?

I mean, if you have a
timing critical operation, then you should ensure you have priorities set correctly so that you simply don't get preempted.

By using a raw_spinlock_t, you're saying that you're more important than anyone else (for the period of the critical section) including processes which the user has explicitly set to a higher priority.

Quoted 1 line

Any reason that stuff shouldn't move into mainline?

This sort of raw_spinlock_t arms race throughout drivers/ would be a huge reason not to move it into mainline.

Quoted 3 lines · Nick Piggin

I mean, if you have a
timing critical operation, then you should ensure you have priorities set correctly so that you simply don't get preempted.

Which is why bitops like <asm-generic/bitops/atomic.h> use normal spinlocks. Oh, wait, no they don't ...

Quoted 3 lines

By using a raw_spinlock_t, you're saying that you're more important than anyone else (for the period of the critical section) including processes which the user has explicitly set to a higher priority.

Nope. Just saying that the relevant instructions (three, in the hot path I looked at, and not a case where priority inversion scenarios should be a concern) shouldn't be forcibly morphed into preemption points. Any more than other bitops were (not).

If a higher priority task needs that CPU, nothing prevents it from being immediately descheduled. Ditto handling a hardirq.

All this does is prevent constant and needless checking for "do you want to preempt me now?" "now?" "now?" in "now?" the middle "now?" of "now?" i/o "now?" loops.

Quoted 4 lines

Quoted 1 line

Any reason that stuff shouldn't move into mainline?

This sort of raw_spinlock_t arms race throughout drivers/ would be a huge reason not to move it into mainline.

This isn't driver code...

I think you've just presented an argument why that stuff shouldn't really exist in -rt either... :)

Quoted 3 lines

All this does is prevent constant and needless checking for "do you want to preempt me now?" "now?" "now?" in "now?" the middle "now?" of "now?" i/o "now?" loops.

Actually that's wrong.

Quoted 5 lines · Nick Piggin

Quoted 3 lines

All this does is prevent constant and needless checking for "do you want to preempt me now?" "now?" "now?" in "now?" the middle "now?" of "now?" i/o "now?" loops.

Actually that's wrong.

Certainly it's right for the mainstream kernel. Dropping a lock (other than a raw spinlock) does that checking; when a loop needs to acquire then drop such a lock, that's exactly what's going on.

And in the RT kernel, it's got to do the same thing ... because dropping that lock may mean that a higher priority task should immediately run and grab the lock.

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

All this does is prevent constant and needless checking for "do you want to preempt me now?" "now?" "now?" in "now?" the middle "now?" of "now?" i/o "now?" loops.

Actually that's wrong.

Quoted 4 lines

Certainly it's right for the mainstream kernel.  Dropping a lock (other than a raw spinlock) does that checking; when a loop needs to acquire then drop such a lock, that's exactly what's going on.

Obviously a raw spinlock is no different from a regular spinlock upstream.

Quoted 11 lines · Nick Piggin

Quoted 10 lines

Quoted 5 lines

Quoted 3 lines

All this does is prevent constant and needless checking for "do you want to preempt me now?" "now?" "now?" in "now?" the middle "now?" of "now?" i/o "now?" loops.

Actually that's wrong.

Certainly it's right for the mainstream kernel.  Dropping a lock (other than a raw spinlock) does that checking; when a loop needs to acquire then drop such a lock, that's exactly what's going on.

Quoted 2 lines

Obviously a raw spinlock is no different from a regular spinlock upstream.

Erm, no. The raw ones don't have the extra logic when the lock gets dropped.

Quoted 15 lines · David Brownell

Quoted 13 lines · Nick Piggin

Quoted 10 lines

Quoted 5 lines

Quoted 3 lines

All this does is prevent constant and needless checking for "do you want to preempt me now?" "now?" "now?" in "now?" the middle "now?" of "now?" i/o "now?" loops.

Actually that's wrong.

Certainly it's right for the mainstream kernel.  Dropping a lock (other than a raw spinlock) does that checking; when a loop needs to acquire then drop such a lock, that's exactly what's going on.

Obviously a raw spinlock is no different from a regular spinlock upstream.

Quoted 2 lines

Erm, no. The raw ones don't have the extra logic when the lock gets dropped.

If you don't have preemption disabled already, then it is a bug to use raw spinlocks. If you do have preemption disabled, then a regular spinlock isn't going to check preemption after the unlock either.

Quoted 1 line

Why do you want to use raw_spinlock_t?

Already answered elsewhere in this thread ... I'll highlight the point that such bitops shouldn't be preemption points.

Quoted 2 lines

I'm still trying to understand what you've observed here. Is it the case that a single gpio operation went from 6.4 up to 11.2 usecs?

That was a single bitbanged I2C bit transfer, with embedded udelay()s. I believe that was four gpio operations, as summarized at the end of that email above. Enabling preempt + debug increased the cost of each GPIO call from whatever it was (reasonable) by 1.2 usecs.

Quoted 7 lines · David Brownell

Quoted 2 lines

I'm still trying to understand what you've observed here. Is it the case that a single gpio operation went from 6.4 up to 11.2 usecs?

That was a single bitbanged I2C bit transfer, with embedded udelay()s. I believe that was four gpio operations, as summarized at the end of that email above. Enabling preempt + debug increased the cost of each GPIO call from whatever it was (reasonable) by 1.2 usecs.

This raw lock change is just pampering over the design problem of the gpio lib:

There is no need to check for every single access to a GPIO pin, whether the pin has a valid number and the chip, which provides access to the pin, is still registered.

Each driver, which wants to access a pin, needs to make sure that

- the pin is available
- the pin is associated to this driver
- the chip reference count is incremented

_before_ it starts to do anything with the pin. Once this is done the access to the pin is completely lock free except for the protection of the chip hardware itself.

The protection of the chip list can be converted to a mutex and does not need to be a spinlock at all.

Quoted 2 lines

The protection of the chip list can be converted to a mutex and does not need to be a spinlock at all.

No, we still need to use a spinlock to protect table changes. The reason for that is briefly:

- gpio_request()/gpio_free() have so far been optional. Most platforms implement them as NOPs, not all drivers use them. (Having gpiolib in place should help change that ...)

- gpio_direction_input()/gpio_direction_output() implicitly request the pins, if they weren't already requested.

- Those input/output direction-setting calls may be called in IRQ contexts, which means (on non-RT kernels) no mutex.

So we're actually in good shape; just take out a bit of code (or turn it into debugging instrumentation) and I don't think anyone will complain about the locking any more.

Quoted 9 lines · David Brownell

Quoted 2 lines

The protection of the chip list can be converted to a mutex and does not need to be a spinlock at all.

No, we still need to use a spinlock to protect table changes. The reason for that is briefly:

- gpio_request()/gpio_free() have so far been optional. Most platforms implement them as NOPs, not all drivers use them. (Having gpiolib in place should help change that ...)

By magically doing the request of the pin ? See below.

Quoted 2 lines

- gpio_direction_input()/gpio_direction_output() implicitly request the pins, if they weren't already requested.

Eek, that's completely wrong. Allowing to access a resource _before_ it is assigned and then doing the assignment implicit is a really bad idea.

Quoted 2 lines

- Those input/output direction-setting calls may be called in IRQ contexts, which means (on non-RT kernels) no mutex.

There is no reason to do that if you actually have a useful reference to the chip _before_ accessing the pin.

Quoted 3 lines

So we're actually in good shape; just take out a bit of code (or turn it into debugging instrumentation) and I don't think anyone will complain about the locking any more.

This still does not solve the lookup, which is done for each operation on a pin (direction setting, read, write).

Quoted 6 lines

Quoted 2 lines

- gpio_direction_input()/gpio_direction_output() implicitly request the pins, if they weren't already requested.

Eek, that's completely wrong. Allowing to access a resource _before_ it is assigned and then doing the assignment implicit is a really bad idea.

This is an artifact of making the GPIO interface easy to adopt, by letting all the initial adopters wrap "legacy" SOC-specific GPIO interfaces instead of creating a bunch of new platform code.

Quoted 7 lines · On Thu, 15 Nov 2007 00:20:33 -0800 David Brownell <[email protected]> wrote:

Quoted 6 lines

Quoted 2 lines

- gpio_direction_input()/gpio_direction_output() implicitly request the pins, if they weren't already requested.

Eek, that's completely wrong. Allowing to access a resource _before_ it is assigned and then doing the assignment implicit is a really bad idea.

Quoted 3 lines

This is an artifact of making the GPIO interface easy to adopt, by letting all the initial adopters wrap "legacy" SOC-specific GPIO interfaces instead of creating a bunch of new platform code.

It's still ok for platforms that do not use gpiolib to provide NOP gpio_request() and gpio_free() functions if they don't care about tracking gpio usage.

That doesn't mean we shouldn't require all drivers
to use those calls -- if they are implemented as empty inlines, it won't cost anything to call them.

I'd rather speed up the gpio_direction_* functions a bit by removing the implicit gpio_request() since they may be called a lot more frequently and, as you pointed out, possibly from atomic context.

struct gpio_desc {
       struct gpio_chip *chip;
       int pinoffset;
};

Quoted 4 lines · Thomas Gleixner

struct gpio_desc {
	   struct gpio_chip *chip;
	   int pinoffset;
};

Eric Miao has a different notion of "gpio_desc" more analagous to the "irq_desc", so I'll not use that name for such a structure.

Let me call yours a "struct gpio_pin" instead -- purely for some discussion here.

There's been some desire for a "gpio_pin" struct for entirely different reasons ... as the identifiers passed into a slightly different type of GPIO programming interface. So for example

static inline void gpiopin_set_value(struct gpio_pin *p, int v)

  {
	p->chip->set(p->chip, p->pinoffset, v);

}

One reason to be interested in gpio_chip is that it it would easily support interoperation between traditional GPIO ID schemes (numbered to match the SOC at the heart of a given board) and more dynamic ones that might be needed to use curiousities like a set of USB-to-GPIO adapters.