Re: [patch 2.6.20-rc1 1/6] GPIO core
Hi! > > > Should it instead say that's an (obviously unchecked) error? > > > > Saying it is an error would be okay by me. (Or "Behaviour of these calls for > > GPIOs that can't be safely accessed without sleeping is undefined."). > > See the appended doc patch ... better? Yes, thanks. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.20-rc1 1/6] GPIO core
Hi! Should it instead say that's an (obviously unchecked) error? Saying it is an error would be okay by me. (Or Behaviour of these calls for GPIOs that can't be safely accessed without sleeping is undefined.). See the appended doc patch ... better? Yes, thanks. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.20-rc1 1/6] GPIO core
On Monday 01 January 2007 12:55 pm, Pavel Machek wrote: > > Think of it as "cookies represented by integers" if you like. > > typedef int gpio_t would hurt, and would serve as a useful > documentation hint. Yes, I agree that such needless obfuscation hurts. ;) Plus, such a typedef would disagree with Documentation/CodingStyle which says "... the rule should basically be to NEVER EVER use a typedef" (with some exceptions not matched here). > > Should it instead say that's an (obviously unchecked) error? > > Saying it is an error would be okay by me. (Or "Behaviour of these calls for > GPIOs that can't be safely accessed without sleeping is undefined."). See the appended doc patch ... better? - Dave = CUT HERE Index: at91/Documentation/gpio.txt === --- at91.orig/Documentation/gpio.txt2006-12-29 00:00:28.0 -0800 +++ at91/Documentation/gpio.txt 2006-12-29 15:47:18.0 -0800 @@ -78,7 +78,8 @@ Identifying GPIOs - GPIOs are identified by unsigned integers in the range 0..MAX_INT. That reserves "negative" numbers for other purposes like marking signals as -"not available on this board", or indicating faults. +"not available on this board", or indicating faults. Code that doesn't +touch the underlying hardware treats these integers as opaque cookies. Platforms define how they use those integers, and usually #define symbols for the GPIO lines so that board-specific setup code directly corresponds @@ -139,8 +140,8 @@ issues including wire-OR and output late 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 wihtout sleeping (see below). +return zero. Also, using these calls for GPIOs that can't safely be accessed +without sleeping (see below) is an error. Platform-specific implementations are encouraged to optimise the two calls to access the GPIO value in cases where the GPIO number (and for @@ -239,7 +240,8 @@ options are part of the IRQ interface, e system wakeup capabilities. Non-error values returned from irq_to_gpio() would most commonly be used -with gpio_get_value(). +with gpio_get_value(), for example to initialize or update driver state +when the IRQ is edge-triggered. @@ -262,7 +264,8 @@ like the aforementioned options for inpu Hardware may support reading or writing GPIOs in gangs, but that's usually configuration dependednt: for GPIOs sharing the same bank. (GPIOs are commonly grouped in banks of 16 or 32, with a given SOC having several such -banks.) Code relying on such mechanisms will necessarily be nonportable. +banks.) Some systems can trigger IRQs from output GPIOs. Code relying on +such mechanisms will necessarily be nonportable. Dynamic definition of GPIOs is not currently supported; for example, as a side effect of configuring an add-on board with some GPIO expanders. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.20-rc1 1/6] GPIO core
Hi! > > 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. typedef int gpio_t would hurt, and would serve as a useful documentation hint. Furthermore, you could switch it to enum on platform where it makes sense. > > 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 ...it adds exactly one line. > > > > > +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? Saying it is an error would be okay by me. (Or "Behaviour of these calls for GPIOs that can't be safely accessed without sleeping is undefined."). > You are perverting what _I_ said. (As you've done before; stop >that.) Sorry. > In terms of API specs, emitting any warning is traditionally > out-of-scope. Ok, I mis-read your specs as trying to push warnings into the scope. > > > > > +... 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 Ok, "debug behaviour is out of scope, again". What about "Behavour of reading a GPIO that hasn't been marked as an input using gpio_set_direction() is implementation-defined"? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.20-rc1 1/6] GPIO core
Hi! 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. typedef int gpio_t would hurt, and would serve as a useful documentation hint. Furthermore, you could switch it to enum on platform where it makes sense. 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 ...it adds exactly one line. +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? Saying it is an error would be okay by me. (Or Behaviour of these calls for GPIOs that can't be safely accessed without sleeping is undefined.). You are perverting what _I_ said. (As you've done before; stop that.) Sorry. In terms of API specs, emitting any warning is traditionally out-of-scope. Ok, I mis-read your specs as trying to push warnings into the scope. +... 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 Ok, debug behaviour is out of scope, again. What about Behavour of reading a GPIO that hasn't been marked as an input using gpio_set_direction() is implementation-defined? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.20-rc1 1/6] GPIO core
On Monday 01 January 2007 12:55 pm, Pavel Machek wrote: Think of it as cookies represented by integers if you like. typedef int gpio_t would hurt, and would serve as a useful documentation hint. Yes, I agree that such needless obfuscation hurts. ;) Plus, such a typedef would disagree with Documentation/CodingStyle which says ... the rule should basically be to NEVER EVER use a typedef (with some exceptions not matched here). Should it instead say that's an (obviously unchecked) error? Saying it is an error would be okay by me. (Or Behaviour of these calls for GPIOs that can't be safely accessed without sleeping is undefined.). See the appended doc patch ... better? - Dave = CUT HERE Index: at91/Documentation/gpio.txt === --- at91.orig/Documentation/gpio.txt2006-12-29 00:00:28.0 -0800 +++ at91/Documentation/gpio.txt 2006-12-29 15:47:18.0 -0800 @@ -78,7 +78,8 @@ Identifying GPIOs - GPIOs are identified by unsigned integers in the range 0..MAX_INT. That reserves negative numbers for other purposes like marking signals as -not available on this board, or indicating faults. +not available on this board, or indicating faults. Code that doesn't +touch the underlying hardware treats these integers as opaque cookies. Platforms define how they use those integers, and usually #define symbols for the GPIO lines so that board-specific setup code directly corresponds @@ -139,8 +140,8 @@ issues including wire-OR and output late 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 wihtout sleeping (see below). +return zero. Also, using these calls for GPIOs that can't safely be accessed +without sleeping (see below) is an error. Platform-specific implementations are encouraged to optimise the two calls to access the GPIO value in cases where the GPIO number (and for @@ -239,7 +240,8 @@ options are part of the IRQ interface, e system wakeup capabilities. Non-error values returned from irq_to_gpio() would most commonly be used -with gpio_get_value(). +with gpio_get_value(), for example to initialize or update driver state +when the IRQ is edge-triggered. @@ -262,7 +264,8 @@ like the aforementioned options for inpu Hardware may support reading or writing GPIOs in gangs, but that's usually configuration dependednt: for GPIOs sharing the same bank. (GPIOs are commonly grouped in banks of 16 or 32, with a given SOC having several such -banks.) Code relying on such mechanisms will necessarily be nonportable. +banks.) Some systems can trigger IRQs from output GPIOs. Code relying on +such mechanisms will necessarily be nonportable. Dynamic definition of GPIOs is not currently supported; for example, as a side effect of configuring an add-on board with some GPIO expanders. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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
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
Re: [patch 2.6.20-rc1 1/6] GPIO core
> Good afternoon. :)
Good after-midnight :-).
> > > +Identifying GPIOs
> > > +-
> > > +GPIOs are identified by unsigned integers in the range 0..MAX_INT. That
> > > +reserves "negative" numbers for other purposes like marking signals as
> > > +"not available on this board", or indicating faults.
> > > +
> > > +Platforms define how they use those integers, and usually #define symbols
> > > +for the GPIO lines so that board-specific setup code directly corresponds
> > > +to the relevant schematics. In contrast, drivers should only use GPIO
> >
> > Perhaps these should not be integers, then?
>
> Thing is, the platforms **DO** identify them as integers.
> Go through OMAP, PXA, StrongARM chip docs ... what you see is
> references to GPIO numbers, 0..MAX, and oh by the way those map to
> bit offsets within gpio controller banks.
Well. when you see (something) = gpio_number + 5 ... you most likely
have an error. That means that they are close to integers, but not
quite. I'd use
typedef int gpio_t;
...it also serves as a nice documentation.
> When they don't identify them as integers, as with AT91, AVR32, and
> S3C -- all of which present GPIOs as a lettered bank plus a bit offset
> within that bank, isn't that structure familiar -- then the Linux
> platform support already #defines them as integers. The naming matches
Great, except that mechanism should not #define them but "enum gpio {
} " them...
> > inside, allows you to typecheck, and allows expansion in (unlikely) case
> > where
> > more than int is needed? ...hotpluggable gpio pins?
>
> If some system wants that kind of infrastructure, it can easily
> implement it behind this API. There's the IDR infrastructure, for
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.
> > > +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.
> 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().
> > > +Those return either the corresponding number in the other namespace, or
> > > +else a negative errno code if the mapping can't be done. (For example,
> > > +some GPIOs can't used as IRQs.) 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
I believe your text suggests it _is_ incompatible. Plus that seems to
mean that architecture must not check for that error...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.20-rc1 1/6] GPIO core
On Wednesday 27 December 2006 9:49 am, Pavel Machek wrote:
> Hi!
Good afternoon. :)
> > +Identifying GPIOs
> > +-
> > +GPIOs are identified by unsigned integers in the range 0..MAX_INT. That
> > +reserves "negative" numbers for other purposes like marking signals as
> > +"not available on this board", or indicating faults.
> > +
> > +Platforms define how they use those integers, and usually #define symbols
> > +for the GPIO lines so that board-specific setup code directly corresponds
> > +to the relevant schematics. In contrast, drivers should only use GPIO
>
> Perhaps these should not be integers, then?
Thing is, the platforms **DO** identify them as integers.
Go through OMAP, PXA, StrongARM chip docs ... what you see is
references to GPIO numbers, 0..MAX, and oh by the way those map to
bit offsets within gpio controller banks.
When they don't identify them as integers, as with AT91, AVR32, and
S3C -- all of which present GPIOs as a lettered bank plus a bit offset
within that bank, isn't that structure familiar -- then the Linux
platform support already #defines them as integers. The naming matches
chip docs, the _numbering_ works much the same as on OMAP, PXA, etc.
Fortunately, only OMAP1 seems to have that ugly many-to-many mapping
between chip pins and GPIOs. On other current platforms, once you
know the GPIO number you know the pin, and vice versa. (That does
not matter to drivers at all -- only board setup code talks "pins",
everything else talks about functions such as "GPIO" -- but hairy
board setup code can be a frelling HUGE time sink/waste.)
> typedef struct { int mydata } pin_t; prevents people from looking
> inside, allows you to typecheck, and allows expansion in (unlikely) case where
> more than int is needed? ...hotpluggable gpio pins?
If some system wants that kind of infrastructure, it can easily
implement it behind this API. There's the IDR infrastructure, for
example, to allocate integers and map them to "more than int" data
for internal uses.
I could very easily imagine platforms that support the spinlock-safe
accessors for the SOC-integrated GPIOs, e.g. numbered below N, and
have a more dynamic "plug in your own external GPIO controller" way
to support GPIOs on i2c expanders, multifunction chips, and suchlike;
with that plugin stuff built over IDR and structures with ops vectors.
> > +Spinlock-Safe GPIO access
> > +-
> > ...
> > +
> > + /* GPIO INPUT: return zero or nonzero */
> > + int gpio_get_value(unsigned gpio);
> > +
> > + /* GPIO OUTPUT */
> > + void gpio_set_value(unsigned gpio, int value);
> > +
> > ...
> > +
> > +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.
And per Linus' rule about BUG(), "silently ignored" is clearly better
than needlessly stopping the whole system.
And for spinlock-unsafe cases, like I2C based GPIO expanders;
> > +Platforms that support this type of GPIO distinguish them from other GPIOs
> > +by returning nonzero from this call:
> > +
> > + int gpio_cansleep(unsigned gpio);
>
> This is ugly :-(. But I don't see easy way around...
Me either. Folk said earlier they want an API that can get/set
both types of GPIO, mostly to support userspace tweaking/config APIs,
but the only way I can see to have one of those is to include a
predicate like gpio_cansleep() to distinguish the two types.
> > +GPIOs mapped to IRQs
> > +
> > +GPIO numbers are unsigned integers; so are IRQ numbers. These make up
> > +two logically distinct namespaces (GPIO 0 need not use IRQ 0). You can
> > +map between them using calls like:
> > +
> > + /* map GPIO numbers to IRQ numbers */
> > + int gpio_to_irq(unsigned gpio);
> > +
> > + /* map IRQ numbers to GPIO numbers */
> > + int irq_to_gpio(unsigned irq);
>
> . Don't we have irq_t already?
Nope. Such a type would likely get in the way of lowlevel IRQ
dispatch code too ...
> > +Those return either the corresponding number in the other namespace, or
> > +else a negative errno code if the mapping can't be done. (For example,
> > +some GPIOs can't used as IRQs.) 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
Re: [patch 2.6.20-rc1 1/6] GPIO core
Hi!
> +Identifying GPIOs
> +-
> +GPIOs are identified by unsigned integers in the range 0..MAX_INT. That
> +reserves "negative" numbers for other purposes like marking signals as
> +"not available on this board", or indicating faults.
> +
> +Platforms define how they use those integers, and usually #define symbols
> +for the GPIO lines so that board-specific setup code directly corresponds
> +to the relevant schematics. In contrast, drivers should only use GPIO
Perhaps these should not be integers, then?
typedef struct { int mydata } pin_t; prevents people from looking
inside, allows you to typecheck, and allows expansion in (unlikely) case where
more than int is needed? ...hotpluggable gpio pins?
> +Spinlock-Safe GPIO access
> +-
> +Most GPIO controllers can be accessed with memory read/write instructions.
> +That doesn't need to sleep, and can safely be done from inside IRQ handlers.
> +
> +Use these calls to access such GPIOs:
> +
> + /* GPIO INPUT: return zero or nonzero */
> + int gpio_get_value(unsigned gpio);
> +
> + /* GPIO OUTPUT */
> + void gpio_set_value(unsigned gpio, int value);
> +
> +The values are boolean, zero for low, nonzero for high. When reading the
> +value of an output pin, the value returned should be what's seen on the
> +pin ... that won't always match the specified output value, because of
> +issues including wire-OR and output latencies.
> +
> +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 wihtout sleeping (see below).
'Silently ignored' is ugly. BUG() would be okay there.
> +Platforms that support this type of GPIO distinguish them from other GPIOs
> +by returning nonzero from this call:
> +
> + int gpio_cansleep(unsigned gpio);
This is ugly :-(. But I don't see easy way around...
> +GPIOs mapped to IRQs
> +
> +GPIO numbers are unsigned integers; so are IRQ numbers. These make up
> +two logically distinct namespaces (GPIO 0 need not use IRQ 0). You can
> +map between them using calls like:
> +
> + /* map GPIO numbers to IRQ numbers */
> + int gpio_to_irq(unsigned gpio);
> +
> + /* map IRQ numbers to GPIO numbers */
> + int irq_to_gpio(unsigned irq);
. Don't we have irq_t already?
> +Those return either the corresponding number in the other namespace, or
> +else a negative errno code if the mapping can't be done. (For example,
> +some GPIOs can't used as IRQs.) 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, if those outputs are really
tristates (wire-or or how you call it?)?
Pavel
--
Thanks for all the (sleeping) penguins.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.20-rc1 1/6] GPIO core
Good afternoon. :)
Good after-midnight :-).
+Identifying GPIOs
+-
+GPIOs are identified by unsigned integers in the range 0..MAX_INT. That
+reserves negative numbers for other purposes like marking signals as
+not available on this board, or indicating faults.
+
+Platforms define how they use those integers, and usually #define symbols
+for the GPIO lines so that board-specific setup code directly corresponds
+to the relevant schematics. In contrast, drivers should only use GPIO
Perhaps these should not be integers, then?
Thing is, the platforms **DO** identify them as integers.
Go through OMAP, PXA, StrongARM chip docs ... what you see is
references to GPIO numbers, 0..MAX, and oh by the way those map to
bit offsets within gpio controller banks.
Well. when you see (something) = gpio_number + 5 ... you most likely
have an error. That means that they are close to integers, but not
quite. I'd use
typedef int gpio_t;
...it also serves as a nice documentation.
When they don't identify them as integers, as with AT91, AVR32, and
S3C -- all of which present GPIOs as a lettered bank plus a bit offset
within that bank, isn't that structure familiar -- then the Linux
platform support already #defines them as integers. The naming matches
Great, except that mechanism should not #define them but enum gpio {
} them...
inside, allows you to typecheck, and allows expansion in (unlikely) case
where
more than int is needed? ...hotpluggable gpio pins?
If some system wants that kind of infrastructure, it can easily
implement it behind this API. There's the IDR infrastructure, for
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.
+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.
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().
+Those return either the corresponding number in the other namespace, or
+else a negative errno code if the mapping can't be done. (For example,
+some GPIOs can't used as IRQs.) 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
I believe your text suggests it _is_ incompatible. Plus that seems to
mean that architecture must not check for that error...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.20-rc1 1/6] GPIO core
Hi!
+Identifying GPIOs
+-
+GPIOs are identified by unsigned integers in the range 0..MAX_INT. That
+reserves negative numbers for other purposes like marking signals as
+not available on this board, or indicating faults.
+
+Platforms define how they use those integers, and usually #define symbols
+for the GPIO lines so that board-specific setup code directly corresponds
+to the relevant schematics. In contrast, drivers should only use GPIO
Perhaps these should not be integers, then?
typedef struct { int mydata } pin_t; prevents people from looking
inside, allows you to typecheck, and allows expansion in (unlikely) case where
more than int is needed? ...hotpluggable gpio pins?
+Spinlock-Safe GPIO access
+-
+Most GPIO controllers can be accessed with memory read/write instructions.
+That doesn't need to sleep, and can safely be done from inside IRQ handlers.
+
+Use these calls to access such GPIOs:
+
+ /* GPIO INPUT: return zero or nonzero */
+ int gpio_get_value(unsigned gpio);
+
+ /* GPIO OUTPUT */
+ void gpio_set_value(unsigned gpio, int value);
+
+The values are boolean, zero for low, nonzero for high. When reading the
+value of an output pin, the value returned should be what's seen on the
+pin ... that won't always match the specified output value, because of
+issues including wire-OR and output latencies.
+
+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 wihtout sleeping (see below).
'Silently ignored' is ugly. BUG() would be okay there.
+Platforms that support this type of GPIO distinguish them from other GPIOs
+by returning nonzero from this call:
+
+ int gpio_cansleep(unsigned gpio);
This is ugly :-(. But I don't see easy way around...
+GPIOs mapped to IRQs
+
+GPIO numbers are unsigned integers; so are IRQ numbers. These make up
+two logically distinct namespaces (GPIO 0 need not use IRQ 0). You can
+map between them using calls like:
+
+ /* map GPIO numbers to IRQ numbers */
+ int gpio_to_irq(unsigned gpio);
+
+ /* map IRQ numbers to GPIO numbers */
+ int irq_to_gpio(unsigned irq);
. Don't we have irq_t already?
+Those return either the corresponding number in the other namespace, or
+else a negative errno code if the mapping can't be done. (For example,
+some GPIOs can't used as IRQs.) 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, if those outputs are really
tristates (wire-or or how you call it?)?
Pavel
--
Thanks for all the (sleeping) penguins.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.20-rc1 1/6] GPIO core
On Wednesday 27 December 2006 9:49 am, Pavel Machek wrote:
Hi!
Good afternoon. :)
+Identifying GPIOs
+-
+GPIOs are identified by unsigned integers in the range 0..MAX_INT. That
+reserves negative numbers for other purposes like marking signals as
+not available on this board, or indicating faults.
+
+Platforms define how they use those integers, and usually #define symbols
+for the GPIO lines so that board-specific setup code directly corresponds
+to the relevant schematics. In contrast, drivers should only use GPIO
Perhaps these should not be integers, then?
Thing is, the platforms **DO** identify them as integers.
Go through OMAP, PXA, StrongARM chip docs ... what you see is
references to GPIO numbers, 0..MAX, and oh by the way those map to
bit offsets within gpio controller banks.
When they don't identify them as integers, as with AT91, AVR32, and
S3C -- all of which present GPIOs as a lettered bank plus a bit offset
within that bank, isn't that structure familiar -- then the Linux
platform support already #defines them as integers. The naming matches
chip docs, the _numbering_ works much the same as on OMAP, PXA, etc.
Fortunately, only OMAP1 seems to have that ugly many-to-many mapping
between chip pins and GPIOs. On other current platforms, once you
know the GPIO number you know the pin, and vice versa. (That does
not matter to drivers at all -- only board setup code talks pins,
everything else talks about functions such as GPIO -- but hairy
board setup code can be a frelling HUGE time sink/waste.)
typedef struct { int mydata } pin_t; prevents people from looking
inside, allows you to typecheck, and allows expansion in (unlikely) case where
more than int is needed? ...hotpluggable gpio pins?
If some system wants that kind of infrastructure, it can easily
implement it behind this API. There's the IDR infrastructure, for
example, to allocate integers and map them to more than int data
for internal uses.
I could very easily imagine platforms that support the spinlock-safe
accessors for the SOC-integrated GPIOs, e.g. numbered below N, and
have a more dynamic plug in your own external GPIO controller way
to support GPIOs on i2c expanders, multifunction chips, and suchlike;
with that plugin stuff built over IDR and structures with ops vectors.
+Spinlock-Safe GPIO access
+-
...
+
+ /* GPIO INPUT: return zero or nonzero */
+ int gpio_get_value(unsigned gpio);
+
+ /* GPIO OUTPUT */
+ void gpio_set_value(unsigned gpio, int value);
+
...
+
+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.
And per Linus' rule about BUG(), silently ignored is clearly better
than needlessly stopping the whole system.
And for spinlock-unsafe cases, like I2C based GPIO expanders;
+Platforms that support this type of GPIO distinguish them from other GPIOs
+by returning nonzero from this call:
+
+ int gpio_cansleep(unsigned gpio);
This is ugly :-(. But I don't see easy way around...
Me either. Folk said earlier they want an API that can get/set
both types of GPIO, mostly to support userspace tweaking/config APIs,
but the only way I can see to have one of those is to include a
predicate like gpio_cansleep() to distinguish the two types.
+GPIOs mapped to IRQs
+
+GPIO numbers are unsigned integers; so are IRQ numbers. These make up
+two logically distinct namespaces (GPIO 0 need not use IRQ 0). You can
+map between them using calls like:
+
+ /* map GPIO numbers to IRQ numbers */
+ int gpio_to_irq(unsigned gpio);
+
+ /* map IRQ numbers to GPIO numbers */
+ int irq_to_gpio(unsigned irq);
. Don't we have irq_t already?
Nope. Such a type would likely get in the way of lowlevel IRQ
dispatch code too ...
+Those return either the corresponding number in the other namespace, or
+else a negative errno code if the mapping can't be done. (For example,
+some GPIOs can't used as IRQs.) 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
