[PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case

[PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case

[Anton Vorontsov]

With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
so the following pops up on PowerPC:

  cc1: warnings being treated as errors
  In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
  include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
                              inside parameter list
  include/linux/of_gpio.h:74: warning: its scope is only this definition
                              or declaration, which is probably not what
			      you want
  include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
                              inside parameter list
  make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1

This patch fixes the issue by providing the proper forward declaration.

Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
---

On Tue, Aug 24, 2010 at 04:26:08PM +1000, Benjamin Herrenschmidt wrote:
> I get that with my current stuff:
> 
> cc1: warnings being treated as errors
> In file included from [..]/mpc52xx_common.c:19:
> of_gpio.h:74: error: ‘struct gpio_chip’ declared inside parameter list
[...]
> make[3]: *** Waiting for unfinished jobs....

That's because with GPIOCHIP=n no one declares struct gpio_chip.

It should be either of_gpio.h or gpio.h. Let's make it gpio.h, as
this feels more generic, and we already have some !GPIOLIB handling
in there.

 include/linux/gpio.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 03f616b..85207d2 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -15,6 +15,12 @@
 struct device;
 
 /*
+ * Some code might rely on the declaration. Still, it is illegal
+ * to dereference it for !GPIOLIB case.
+ */
+struct gpio_chip;
+
+/*
  * Some platforms don't support the GPIO programming interface.
  *
  * In case some driver uses it anyway (it should normally have
-- 
1.7.0.5

Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case

[Grant Likely]

On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
> With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
> so the following pops up on PowerPC:
> 
>   cc1: warnings being treated as errors
>   In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
>   include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
>                               inside parameter list
>   include/linux/of_gpio.h:74: warning: its scope is only this definition
>                               or declaration, which is probably not what
> 			      you want
>   include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
>                               inside parameter list
>   make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1
> 
> This patch fixes the issue by providing the proper forward declaration.
> 
> Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>

This doesn't actually solve the problem, and gpiochip should remain undefined when CONFIG_GPIOLIB=n to catch exactly these build failures.  The real problem is that I merged a change into the mpc5200 code that required CONFIG_GPIOLIB be enabled without reflecting that requirement in Kconfig.

g.

> ---
> 
> On Tue, Aug 24, 2010 at 04:26:08PM +1000, Benjamin Herrenschmidt wrote:
> > I get that with my current stuff:
> > 
> > cc1: warnings being treated as errors
> > In file included from [..]/mpc52xx_common.c:19:
> > of_gpio.h:74: error: ‘struct gpio_chip’ declared inside parameter list
> [...]
> > make[3]: *** Waiting for unfinished jobs....
> 
> That's because with GPIOCHIP=n no one declares struct gpio_chip.
> 
> It should be either of_gpio.h or gpio.h. Let's make it gpio.h, as
> this feels more generic, and we already have some !GPIOLIB handling
> in there.
> 
>  include/linux/gpio.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index 03f616b..85207d2 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -15,6 +15,12 @@
>  struct device;
>  
>  /*
> + * Some code might rely on the declaration. Still, it is illegal
> + * to dereference it for !GPIOLIB case.
> + */
> +struct gpio_chip;
> +
> +/*
>   * Some platforms don't support the GPIO programming interface.
>   *
>   * In case some driver uses it anyway (it should normally have
> -- 
> 1.7.0.5
> 

Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case

[Anton Vorontsov]

On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote:
> On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
> > With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
> > so the following pops up on PowerPC:
> > 
> >   cc1: warnings being treated as errors
> >   In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
> >   include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
> >                               inside parameter list
> >   include/linux/of_gpio.h:74: warning: its scope is only this definition
> >                               or declaration, which is probably not what
> > 			      you want
> >   include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
> >                               inside parameter list
> >   make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1
> > 
> > This patch fixes the issue by providing the proper forward declaration.
> > 
> > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> 
> This doesn't actually solve the problem, and gpiochip should 
> remain undefined when CONFIG_GPIOLIB=n to catch exactly these
> build failures.  The real problem is that I merged a change
> into the mpc5200 code that required CONFIG_GPIOLIB be enabled
> without reflecting that requirement in Kconfig.

No, look closer. The error is in of_gpio.h, and it's perfectly
fine to include it w/o GPIOLIB=y.

> > ---
> > 
> > On Tue, Aug 24, 2010 at 04:26:08PM +1000, Benjamin Herrenschmidt wrote:
> > > I get that with my current stuff:
> > > 
> > > cc1: warnings being treated as errors
> > > In file included from [..]/mpc52xx_common.c:19:
> > > of_gpio.h:74: error: ‘struct gpio_chip’ declared inside parameter list
> > [...]
> > > make[3]: *** Waiting for unfinished jobs....
> > 
> > That's because with GPIOCHIP=n no one declares struct gpio_chip.
> > 
> > It should be either of_gpio.h or gpio.h. Let's make it gpio.h, as
> > this feels more generic, and we already have some !GPIOLIB handling
> > in there.
> > 
> >  include/linux/gpio.h |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> > index 03f616b..85207d2 100644
> > --- a/include/linux/gpio.h
> > +++ b/include/linux/gpio.h
> > @@ -15,6 +15,12 @@
> >  struct device;
> >  
> >  /*
> > + * Some code might rely on the declaration. Still, it is illegal
> > + * to dereference it for !GPIOLIB case.
> > + */
> > +struct gpio_chip;
> > +
> > +/*
> >   * Some platforms don't support the GPIO programming interface.
> >   *
> >   * In case some driver uses it anyway (it should normally have
> > -- 
> > 1.7.0.5
> > 

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case

[Grant Likely]

On Tue, Aug 31, 2010 at 2:37 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote:
>> On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
>> > With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
>> > so the following pops up on PowerPC:
>> >
>> >   cc1: warnings being treated as errors
>> >   In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
>> >   include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
>> >                               inside parameter list
>> >   include/linux/of_gpio.h:74: warning: its scope is only this definition
>> >                               or declaration, which is probably not what
>> >                           you want
>> >   include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
>> >                               inside parameter list
>> >   make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1
>> >
>> > This patch fixes the issue by providing the proper forward declaration.
>> >
>> > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
>>
>> This doesn't actually solve the problem, and gpiochip should
>> remain undefined when CONFIG_GPIOLIB=n to catch exactly these
>> build failures.  The real problem is that I merged a change
>> into the mpc5200 code that required CONFIG_GPIOLIB be enabled
>> without reflecting that requirement in Kconfig.
>
> No, look closer. The error is in of_gpio.h, and it's perfectly
> fine to include it w/o GPIOLIB=y.

Looking even closer, we're both wrong.  You're right I didn't look
carefully enough, and the error is in of_gpio.h, not the .c file.

However, it is not okay to get the definitions from of_gpio.h when
CONFIG_GPIOLIB=n.  If GPIOLIB, or more specifically OF_GPIO isn't set,
then the of_gpio.h definitions should either not be defined, or should
be defined as empty stubs (where appropriate).

So, instead of adding a forward declarations of the struct, the
correct thing I think to do is to #ifdef out the contents of of_gpio.h
when GPIOLIB isn't selected so that extra #includes are completely
benign.  I've been doing the same thing on the other linux/of*.h
headers.  I've got a patch in my tree that I'm testing right now and
I'll post later today.

g.

>
>> > ---
>> >
>> > On Tue, Aug 24, 2010 at 04:26:08PM +1000, Benjamin Herrenschmidt wrote:
>> > > I get that with my current stuff:
>> > >
>> > > cc1: warnings being treated as errors
>> > > In file included from [..]/mpc52xx_common.c:19:
>> > > of_gpio.h:74: error: ‘struct gpio_chip’ declared inside parameter list
>> > [...]
>> > > make[3]: *** Waiting for unfinished jobs....
>> >
>> > That's because with GPIOCHIP=n no one declares struct gpio_chip.
>> >
>> > It should be either of_gpio.h or gpio.h. Let's make it gpio.h, as
>> > this feels more generic, and we already have some !GPIOLIB handling
>> > in there.
>> >
>> >  include/linux/gpio.h |    6 ++++++
>> >  1 files changed, 6 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/include/linux/gpio.h b/include/linux/gpio.h
>> > index 03f616b..85207d2 100644
>> > --- a/include/linux/gpio.h
>> > +++ b/include/linux/gpio.h
>> > @@ -15,6 +15,12 @@
>> >  struct device;
>> >
>> >  /*
>> > + * Some code might rely on the declaration. Still, it is illegal
>> > + * to dereference it for !GPIOLIB case.
>> > + */
>> > +struct gpio_chip;
>> > +
>> > +/*
>> >   * Some platforms don't support the GPIO programming interface.
>> >   *
>> >   * In case some driver uses it anyway (it should normally have
>> > --
>> > 1.7.0.5
>> >
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case

[Anton Vorontsov]

On Tue, Aug 31, 2010 at 10:44:14AM -0600, Grant Likely wrote:
> On Tue, Aug 31, 2010 at 2:37 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote:
> >> On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
> >> > With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
> >> > so the following pops up on PowerPC:
> >> >
> >> >   cc1: warnings being treated as errors
> >> >   In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
> >> >   include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
> >> >                               inside parameter list
> >> >   include/linux/of_gpio.h:74: warning: its scope is only this definition
> >> >                               or declaration, which is probably not what
> >> >                           you want
> >> >   include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
> >> >                               inside parameter list
> >> >   make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1
> >> >
> >> > This patch fixes the issue by providing the proper forward declaration.
> >> >
> >> > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> >>
> >> This doesn't actually solve the problem, and gpiochip should
> >> remain undefined when CONFIG_GPIOLIB=n to catch exactly these
> >> build failures.  The real problem is that I merged a change
> >> into the mpc5200 code that required CONFIG_GPIOLIB be enabled
> >> without reflecting that requirement in Kconfig.
> >
> > No, look closer. The error is in of_gpio.h, and it's perfectly
> > fine to include it w/o GPIOLIB=y.
> 
> Looking even closer, we're both wrong.  You're right I didn't look
> carefully enough, and the error is in of_gpio.h, not the .c file.
> 
> However, it is not okay to get the definitions from of_gpio.h when
> CONFIG_GPIOLIB=n.  If GPIOLIB, or more specifically OF_GPIO isn't set,
> then the of_gpio.h definitions should either not be defined, or should
> be defined as empty stubs (where appropriate).

Grrr. Grant, look again, even closer than you did.

They are stubs!

#else /* CONFIG_OF_GPIO */  <<<<<< !OF_GPIO (or !GPIOLIB) case.

/* Drivers may not strictly depend on the GPIO support, so let them link. */
static inline int of_get_gpio_flags(struct device_node *np, int index,
                                    enum of_gpio_flags *flags)
{
        return -ENOSYS;
}

static inline unsigned int of_gpio_count(struct device_node *np)
{
        return 0;
}

static inline void of_gpiochip_add(struct gpio_chip *gc) { }
static inline void of_gpiochip_remove(struct gpio_chip *gc) { }

#endif /* CONFIG_OF_GPIO */

The errors are triggered by the of_gpiochip_*() stubs, which
are needed by the drivers/gpio/gpiolib.c.

Do you want to add another #ifdef CONFIG_GPIOLIB around
of_gpiochip_*()? That would be ugly.

There's nothing wrong in providing the forward decls, you
can't dereference it anyway (so you would still catch the
bogus users). And at the same time it's will work great
for these stubs.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case

[Grant Likely]

On Tue, Aug 31, 2010 at 09:07:56PM +0400, Anton Vorontsov wrote:
> On Tue, Aug 31, 2010 at 10:44:14AM -0600, Grant Likely wrote:
> > On Tue, Aug 31, 2010 at 2:37 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > > On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote:
> > >> On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
> > >> > With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
> > >> > so the following pops up on PowerPC:
> > >> >
> > >> >   cc1: warnings being treated as errors
> > >> >   In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
> > >> >   include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
> > >> >                               inside parameter list
> > >> >   include/linux/of_gpio.h:74: warning: its scope is only this definition
> > >> >                               or declaration, which is probably not what
> > >> >                           you want
> > >> >   include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
> > >> >                               inside parameter list
> > >> >   make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1
> > >> >
> > >> > This patch fixes the issue by providing the proper forward declaration.
> > >> >
> > >> > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> > >>
> > >> This doesn't actually solve the problem, and gpiochip should
> > >> remain undefined when CONFIG_GPIOLIB=n to catch exactly these
> > >> build failures.  The real problem is that I merged a change
> > >> into the mpc5200 code that required CONFIG_GPIOLIB be enabled
> > >> without reflecting that requirement in Kconfig.
> > >
> > > No, look closer. The error is in of_gpio.h, and it's perfectly
> > > fine to include it w/o GPIOLIB=y.
> > 
> > Looking even closer, we're both wrong.  You're right I didn't look
> > carefully enough, and the error is in of_gpio.h, not the .c file.
> > 
> > However, it is not okay to get the definitions from of_gpio.h when
> > CONFIG_GPIOLIB=n.  If GPIOLIB, or more specifically OF_GPIO isn't set,
> > then the of_gpio.h definitions should either not be defined, or should
> > be defined as empty stubs (where appropriate).
> 
> Grrr. Grant, look again, even closer than you did.

Gah.  Very bad day yesterday.  Applied and sorry for the noise.

g.