As discussed on i2c mailing list with David Brownell, and number
outside of the 0...MAX_INT range is invalid as a GPIO number.
Define a macro, similar to NO_IRQ, to be used as a deliberate
invalid GPIO, rather than defining a is_valid_gpio() macro.
Signed-off-by: Guennadi Liakhovetski <[email protected]>
---
As gpiolib doesn't seem to have an own mailing list, sending it directly
to LKML.
include/asm-generic/gpio.h | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index f29a502..806b86c 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -16,6 +16,10 @@
#define ARCH_NR_GPIOS 256
#endif
+#ifndef NO_GPIO
+#define NO_GPIO ((unsigned int)-1)
+#endif
+
struct seq_file;
/**
--
1.5.3.4
On Thu, 31 Jan 2008, Guennadi Liakhovetski wrote:
> As discussed on i2c mailing list with David Brownell, and number
> outside of the 0...MAX_INT range is invalid as a GPIO number.
> Define a macro, similar to NO_IRQ, to be used as a deliberate
> invalid GPIO, rather than defining a is_valid_gpio() macro.
David, I think, your ack is required on this one. Can we get it in,
please? My soc-camera patches are accepted by the v4l maintainer, and
without this one the series would be incomplete.
Thanks
Guennadi
> Signed-off-by: Guennadi Liakhovetski <[email protected]>
>
> ---
>
> As gpiolib doesn't seem to have an own mailing list, sending it directly
> to LKML.
>
> include/asm-generic/gpio.h | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index f29a502..806b86c 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -16,6 +16,10 @@
> #define ARCH_NR_GPIOS 256
> #endif
>
> +#ifndef NO_GPIO
> +#define NO_GPIO ((unsigned int)-1)
> +#endif
> +
> struct seq_file;
>
> /**
> --
> 1.5.3.4
>
---
Guennadi Liakhovetski
On Thursday 31 January 2008, Guennadi Liakhovetski wrote:
> As discussed on i2c mailing list with David Brownell, and number
> outside of the 0...MAX_INT range is invalid as a GPIO number.
> Define a macro, similar to NO_IRQ, to be used as a deliberate
> invalid GPIO, rather than defining a is_valid_gpio() macro.
Actually I thought that what you needed was an is_valid_gpio();
your motivation was that you needed a predicate.
The problem I have with a #define for a single such invalid GPIO
number is that people will inevitably start to assume it's the
only such number. In particular "if (gpio == NO_GPIO) ..."
is by definition incorrect.
So I'd really rather see a predicate like is_valid_gpio().
If you want to designate one value for use as an initializer,
then I'd rather see a simple
#define NO_GPIO (-EINVAL)
without any option for arch-specific overrides ... along with a
comment that this is only *one* of the numerous values which
will fail is_valid_gpio().
- Dave
>
> Signed-off-by: Guennadi Liakhovetski <[email protected]>
>
> ---
>
> As gpiolib doesn't seem to have an own mailing list, sending it directly
> to LKML.
>
> include/asm-generic/gpio.h | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index f29a502..806b86c 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -16,6 +16,10 @@
> #define ARCH_NR_GPIOS 256
> #endif
>
> +#ifndef NO_GPIO
> +#define NO_GPIO ((unsigned int)-1)
> +#endif
> +
> struct seq_file;
>
> /**
> --
> 1.5.3.4
>
On Fri, 8 Feb 2008, David Brownell wrote:
> On Thursday 31 January 2008, Guennadi Liakhovetski wrote:
> > As discussed on i2c mailing list with David Brownell, and number
> > outside of the 0...MAX_INT range is invalid as a GPIO number.
> > Define a macro, similar to NO_IRQ, to be used as a deliberate
> > invalid GPIO, rather than defining a is_valid_gpio() macro.
>
> Actually I thought that what you needed was an is_valid_gpio();
> your motivation was that you needed a predicate.
>
> The problem I have with a #define for a single such invalid GPIO
> number is that people will inevitably start to assume it's the
> only such number. In particular "if (gpio == NO_GPIO) ..."
> is by definition incorrect.
>
> So I'd really rather see a predicate like is_valid_gpio().
>
> If you want to designate one value for use as an initializer,
> then I'd rather see a simple
>
> #define NO_GPIO (-EINVAL)
>
> without any option for arch-specific overrides ... along with a
> comment that this is only *one* of the numerous values which
> will fail is_valid_gpio().
I was thinking about irq numbers and trying to avoid as early as possible
their problem: namely that each and every platform has its own idea of
which irq numbers are valid and which are not, some use 0 as invalid irq,
some -1, some 256, etc. And when those platforms share drivers, problems
arise. And the simple and efficient NO_IRQ notion, that would fis those
problems nicely, cannot seem to establish itself.
The disadvantages I see in your suggestions are:
1. two accessors (is_valid_gpio() and NO_GPIO) instead of one
2. have to include errno.h
3. it doesn't seem very logical to me to define a gpio number in terms of
an error code
4. "confusing freedom" - NO_GPIO is the invalid gpio number, but, in fact,
you can use just any negative number
Advantages of my proposal:
1. simplicity - only one macro, and "well-definedness" - use this and only
this as invalid gpio number. The rest are either valid, or undefined.
2. overridable by platforms - though I don't have any examples at hand, I
can imagine, that some platforms would prefer some specific "natural"
for them numbers.
But, this is not something I would spend too much energy arguing about,
and this is your code in the end:-) So, if you still disagree, I'll do it
the way you suggest. I might well be wrong too:-)
Thanks
Guennadi
---
Guennadi Liakhovetski
On Saturday 09 February 2008, Guennadi Liakhovetski wrote:
> On Fri, 8 Feb 2008, David Brownell wrote:
>
> > Actually I thought that what you needed was an is_valid_gpio();
> > your motivation was that you needed a predicate.
> >
> > The problem I have with a #define for a single such invalid GPIO
> > number is that people will inevitably start to assume it's the
> > only such number. In particular "if (gpio == NO_GPIO) ..."
> > is by definition incorrect.
> >
> > So I'd really rather see a predicate like is_valid_gpio().
> >
> > If you want to designate one value for use as an initializer,
> > then I'd rather see a simple
> >
> > #define NO_GPIO (-EINVAL)
> >
> > without any option for arch-specific overrides ... along with a
> > comment that this is only *one* of the numerous values which
> > will fail is_valid_gpio().
>
> I was thinking about irq numbers and trying to avoid as early as possible
> their problem: namely that each and every platform has its own idea of
> which irq numbers are valid and which are not, some use 0 as invalid irq,
> some -1, some 256, etc.
That problem came about mostly because the definition was not
part of the original interface definition. Not unlike DMA
addressing ... for the longest time it was impossible to
report DMA mapping failures.
Whereas there's *never* been any question about whether
negative numbers are invalid GPIO numbers. (They aren't.)
> And when those platforms share drivers, problems
> arise. And the simple and efficient NO_IRQ notion, that would fis those
> problems nicely, cannot seem to establish itself.
Inertia is one of the problems there ... plus, the only
obvious advantage of "#define NO_IRQ 0" is that it makes
it easier to be lazy about initialization.
Plus, changing platforms to use that convention means they
mostly need to adopt an *unnatural* step of mapping from the
hardware IRQ numbers (which often start at zero, as they do
on one system I just ssh'd into) to some "logical" ID.
Even if you believe that's worthwhile, it's work; and it
could easily break something.
> The disadvantages I see in your suggestions are:
>
> 1. two accessors (is_valid_gpio() and NO_GPIO) instead of one
Neither of those is an "accessor". One is a "predicate"; and
the other is an "initializer". (A better initializer name might
be more like INIT_GPIO_INVALID.)
The "accessor" scenario is actually a natural place to rely
on errno values. Accessors are like
int gpio = foo_get_gpio(foo_ptr);
And the normal kernel convention there is to return negative
errno values that characterize the different fault modes.
(Ditto allocators: foo_alloc_gpio etc.)
> 2. have to include errno.h
Which most code already does. And you'd certainly want to
do that if you were using an accessor to get GPIOs...
> 3. it doesn't seem very logical to me to define a gpio number in terms of
> an error code
It's not a GPIO number though; it's specifically designated as
NOT being a GPIO. So why not have it be a magic number which
has meaning in multiple contexts? Do you object to "ssize_t",
or in general the "return negative errno on fault" conventions?
> 4. "confusing freedom" - NO_GPIO is the invalid gpio number, but, in fact,
> you can use just any negative number
I don't see any reason to change the API to disallow using
other negative values there. What good would come from that?
(Remember, the *CURRENT* definition covers this situation
by saying no negative number is a valid GPIO number.)
At the machine instruction level, comparing against "-1" or
any other single currently-defined-as-invalid number is more
expensive than checking "is it negative".
And at a higher level, you'd prevent normal accessor (or
allocator, etc) idioms. I can't see any value to preventing
such usage.
> Advantages of my proposal:
>
> 1. simplicity - only one macro, and "well-definedness" - use this and only
> this as invalid gpio number. The rest are either valid, or undefined.
It's currently simple and well defined; negative numbers
are not GPIOs. You want a *different* model, which is in
fact more complex ... it adds that "undefined" notion.
> 2. overridable by platforms - though I don't have any examples at hand, I
> can imagine, that some platforms would prefer some specific "natural"
> for them numbers.
They can already pick any positive number. I don't know
about you, but I *shudder* to think of anyone who's
seriously trying to manage more than 2 Gbits of GPIOs
one bit at a time!
> But, this is not something I would spend too much energy arguing about,
> and this is your code in the end:-) So, if you still disagree, I'll do it
> the way you suggest. I might well be wrong too:-)
Well, you've not convinced me there's any reason to change
the current rules to prevent accessor/allocator idioms from
returning fault codes that could be meaningful.
- Dave
David, you convinced me:-) I'll redo the patch. Just one comment:
On Sat, 9 Feb 2008, David Brownell wrote:
> On Saturday 09 February 2008, Guennadi Liakhovetski wrote:
>
> > And when those platforms share drivers, problems
> > arise. And the simple and efficient NO_IRQ notion, that would fis those
> > problems nicely, cannot seem to establish itself.
>
> Inertia is one of the problems there ... plus, the only
> obvious advantage of "#define NO_IRQ 0" is that it makes
> it easier to be lazy about initialization.
>
> Plus, changing platforms to use that convention means they
> mostly need to adopt an *unnatural* step of mapping from the
> hardware IRQ numbers (which often start at zero, as they do
> on one system I just ssh'd into) to some "logical" ID.
> Even if you believe that's worthwhile, it's work; and it
> could easily break something.
NO_IRQ doesn't have to be 0. Platforms, where 0 is a valid number can use
-1, or 256, or whatever they want:-)
Thanks
Guennadi
---
Guennadi Liakhovetski