Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 9 Nov 2007 11:36:19 -0800 David Brownell <david-b@pacbell.net> wrote:

> 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).
> 
> ...
>
> +
> +/* 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.

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

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

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

> +postcore_initcall(gpiolib_debugfs_init);

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