Message-Id: <200612291718.34494.david-b@pacbell.net>
Date:	Fri, 29 Dec 2006 17:18:33 -0800
From:	David Brownell <david-b@...bell.net>
To:	Pavel Machek <pavel@....cz>
Cc:	Linux Kernel list <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...l.org>,
	Andrew Victor <andrew@...people.com>,
	Bill Gatliff <bgat@...lgatliff.com>,
	Haavard Skinnemoen <hskinnemoen@...el.com>, jamey.hicks@...com,
	Kevin Hilman <khilman@...sta.com>,
	Nicolas Pitre <nico@....org>,
	Russell King <rmk@....linux.org.uk>,
	Tony Lindgren <tony@...mide.com>,
	pHilipp Zabel <philipp.zabel@...il.com>
Subject: Re: [patch 2.6.20-rc1 1/6] GPIO core

On Thursday 28 December 2006 4:27 pm, Pavel Machek wrote:
> 
> > > > +GPIOs are identified by unsigned integers in the range 0..MAX_INT....
> > > 
> > > Perhaps these should not be integers, then?
> > 
> > Thing is, the platforms **DO** identify them as integers.
> > ...
> 
> Well. when you see (something) = gpio_number + 5 ... you most likely
> have an error.

One could surely apply that argument to hundreds of places throughout
the kernel ... that doesn't make it a good one.  One of the downfalls
of many "object oriented programming" efforts was this same desire to
encapsulate things that don't need it; it's lose, not a don't-care.

Think of it as "cookies represented by integers" if you like.


> No, that's a wrong way. I want you to admit that gpio numbers are
> opaque cookies noone should look at, and use (something like)
> gpio_t... so that we can teach sparse to check them.

You're welcome to dream on.  :)

The goal here is not to create new complexity, it's to wrap the
current widely used abstraction (gpios are integers, with get/set
primitives and a direction) in a neutral programming interface
that's very easy to map to/from the current arch-specific ones.

So that various drivers can get on with the business of being
generally useful, rather than arch-specific; or at least being
easier to read.  See the example PXA patch I recently posted;
it's a code shrink, and *direct* translation from the current
GPIO interface (which uses integers).


> > > > +The get/set calls have no error returns because "invalid GPIO" should have
> > > > +been reported earlier in gpio_set_direction().  However, note that not all
> > > > +platforms can read the value of output pins; those that can't should always
> > > > +return zero.  Also, these calls will be ignored for GPIOs that can't safely
> > > > +be accessed without sleeping (see below).
> > > 
> > > 'Silently ignored' is ugly. BUG() would be okay there.
> > 
> > The reason for "silently ignored" is that we really don't want to be
> > cluttering up the code (source or object) with logic to test for this
> > kind of "can't happen" failure, especially since there's not going to
> > be any way to _resolve_ such failures cleanly.
> 
> You may not want to clutter up code for one arch, but for some of them
> maybe it is okay and welcome. Please do not document "silently
> ignored" into API.

Those words were yours; so you can consider that already done.
Should it instead say that's an (obviously unchecked) error?

The "no error returns" was an explicit request from several folk
during earlier API discussions.  People actually _using_ GPIOs have
no use for faults on those known-valid get/set calls.  Seriously,
exactly how could you ever recover from register access no longer
working correctly?  The chip is in the process of exploding, or
being crushed; what could software possibly do to recover?


> > And per Linus' rule about BUG(), "silently ignored" is clearly better
> > than needlessly stopping the whole system.
> 
> You are perverting what Linus said. "Do not bother detecting errors"
> is not what he had in mind.. but perhaps it should be WARN() not
> BUG().

You are perverting what _I_ said.  (As you've done before; stop that.)

It's very clear that I was talking about a tradeoff ("better than"), and
pointing out how Linus' rule made it clear that your proposal was on the
wrong end of things.  (His rule being that BUG should not be used unless
the system really can't continue operating.)

In terms of API specs, emitting any warning is traditionally out-of-scope.
Because "of course" it's legit for debug modes to do all kinds of things,
including emitting warnings; and likewise it's legit for non-debug modes
to do nothing not absolutely required.  And programming interface specs
have no business in "quality of implementation" issues like whether any
implementation even _has_ a debug mode, much less what it covers.


> > > > +... It is an unchecked error to use a GPIO
> > > > +number that hasn't been marked as an input using gpio_set_direction(), or
> > > 
> > > It should be valid to do irqs on outputs,
> > 
> > Good point -- it _might_ be valid to do that, on some platforms.
> > Such things have been used as a software IRQ trigger, which can
> > make bootstrapping some things easier.
> > 
> > That's not incompatible with it being an error for portable code to
> > try that, or with refusing to check it so that those platforms don't
> > needlessly cause trouble!
> 
> I believe your text suggests it _is_ incompatible. Plus that seems to
> mean that  architecture must not check for that error...

Which -- that portable code mustn't try such things?  That seems clearly
wrong; that's what the "is an error" phrase means.  Or that code should
not need an obscure API for nonportable tricks like that?  That seems
wrong too; that's one of the reasons to specify things as "unchecked".
Or that implementations shouldn't be required to guard against that
behavior in layers above?  Same comment about "unchecked".

I think you must have missed the class in "how to write API specs" which
discussed how to accomodate nonportable behaviors; it's a given that if
there are more than two implementations of the interface, such things are
going to exist.  One of the options is to just declare such things as
clearly out-of-scope for the API, and just wash your hands of what some
implementaiton will be doing (regardless of what you write down).  In
this case, that's what happened.

And again, remember that if your implementation (not architecture!) wants
to have a (debug) mode that warns about marginal might-be-trouble cases,
that's always legit.

- Dave

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

Powered by blists - more mailing lists