2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Adrian Bunk]

Commit 8ffbc6559493c64d6194c92d856196fdaeb8a5fb causes the following 
compile error with CONFIG_VIDEO_DEV=y/m, CONFIG_I2C=n:

<--  snip  -->

...
  MODPOST 26 modules
ERROR: "i2c_attach_client" [drivers/media/video/v4l2-common.ko] undefined!
make[2]: *** [__modpost] Error 1

<--  snip  -->

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Mauro Carvalho Chehab]

Hi Adrian and Marcin,

On Sun, 27 Jan 2008 20:52:16 +0200
Adrian Bunk <bunk@kernel.org> wrote:

> Commit 8ffbc6559493c64d6194c92d856196fdaeb8a5fb causes the following 
> compile error with CONFIG_VIDEO_DEV=y/m, CONFIG_I2C=n:
> 
> <--  snip  -->
> 
> ...
>   MODPOST 26 modules
> ERROR: "i2c_attach_client" [drivers/media/video/v4l2-common.ko] undefined!
> make[2]: *** [__modpost] Error 1
> 
> <--  snip  -->


Thanks for getting this regression. Please test the enclosed patch.


Cheers,
Mauro

Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org>

diff -r b0815101889d linux/drivers/media/video/v4l2-common.c
--- a/linux/drivers/media/video/v4l2-common.c	Sun Jan 27 20:39:00 2008
-0200 +++ b/linux/drivers/media/video/v4l2-common.c	Sun Jan 27 22:23:25
2008 -0200 @@ -1585,6 +1585,7 @@ u32 v4l2_ctrl_next(const u32 * const * c
 	return **ctrl_classes;
 }
 
+#ifdef CONFIG_I2C
 int v4l2_chip_match_i2c_client(struct i2c_client *c, u32 match_type, u32
match_chip) {
 	switch (match_type) {
@@ -1596,6 +1597,7 @@ int v4l2_chip_match_i2c_client(struct i2
 		return 0;
 	}
 }
+EXPORT_SYMBOL(v4l2_chip_match_i2c_client);
 
 int v4l2_chip_ident_i2c_client(struct i2c_client *c, struct v4l2_chip_ident
*chip, u32 ident, u32 revision)
@@ -1612,6 +1614,7 @@ int v4l2_chip_ident_i2c_client(struct i2
 	}
 	return 0;
 }
+EXPORT_SYMBOL(v4l2_chip_ident_i2c_client);
 
 int v4l2_chip_match_host(u32 match_type, u32 match_chip)
 {
@@ -1622,6 +1625,7 @@ int v4l2_chip_match_host(u32 match_type,
 		return 0;
 	}
 }
+EXPORT_SYMBOL(v4l2_chip_match_host);
 
 /* ----------------------------------------------------------------- */
 
@@ -1656,6 +1660,8 @@ int v4l2_i2c_attach(struct i2c_adapter *
 	}
 	return err != -ENOMEM ? 0 : err;
 }
+EXPORT_SYMBOL(v4l2_i2c_attach);
+#endif
 
 /* ----------------------------------------------------------------- */
 
@@ -1679,15 +1685,3 @@ EXPORT_SYMBOL(v4l2_ctrl_query_menu);
 EXPORT_SYMBOL(v4l2_ctrl_query_menu);
 EXPORT_SYMBOL(v4l2_ctrl_query_fill);
 EXPORT_SYMBOL(v4l2_ctrl_query_fill_std);
-
-EXPORT_SYMBOL(v4l2_chip_match_i2c_client);
-EXPORT_SYMBOL(v4l2_chip_ident_i2c_client);
-EXPORT_SYMBOL(v4l2_chip_match_host);
-
-EXPORT_SYMBOL(v4l2_i2c_attach);
-
-/*
- * Local variables:
- * c-basic-offset: 8
- * End:
- */

Re: 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Adrian Bunk]

On Sun, Jan 27, 2008 at 10:33:34PM -0200, Mauro Carvalho Chehab wrote:
> Hi Adrian and Marcin,
> 
> On Sun, 27 Jan 2008 20:52:16 +0200
> Adrian Bunk <bunk@kernel.org> wrote:
> 
> > Commit 8ffbc6559493c64d6194c92d856196fdaeb8a5fb causes the following 
> > compile error with CONFIG_VIDEO_DEV=y/m, CONFIG_I2C=n:
> > 
> > <--  snip  -->
> > 
> > ...
> >   MODPOST 26 modules
> > ERROR: "i2c_attach_client" [drivers/media/video/v4l2-common.ko] undefined!
> > make[2]: *** [__modpost] Error 1
> > 
> > <--  snip  -->
> 
> 
> Thanks for getting this regression. Please test the enclosed patch.

It doesn't enable the code if CONFIG_VIDEO_DEV=m, CONFIG_I2C=m.

And what should happen if CONFIG_VIDEO_DEV=y, CONFIG_I2C=m?

If the latter should offer this code to modules we'll have to put it 
into an own module.

> Cheers,
> Mauro
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org>
> 
> diff -r b0815101889d linux/drivers/media/video/v4l2-common.c
> --- a/linux/drivers/media/video/v4l2-common.c	Sun Jan 27 20:39:00 2008
> -0200 +++ b/linux/drivers/media/video/v4l2-common.c	Sun Jan 27 22:23:25
> 2008 -0200 @@ -1585,6 +1585,7 @@ u32 v4l2_ctrl_next(const u32 * const * c
>  	return **ctrl_classes;
>  }
>  
> +#ifdef CONFIG_I2C
>  int v4l2_chip_match_i2c_client(struct i2c_client *c, u32 match_type, u32
> match_chip) {
>  	switch (match_type) {
> @@ -1596,6 +1597,7 @@ int v4l2_chip_match_i2c_client(struct i2
>  		return 0;
>  	}
>  }
> +EXPORT_SYMBOL(v4l2_chip_match_i2c_client);
>  
>  int v4l2_chip_ident_i2c_client(struct i2c_client *c, struct v4l2_chip_ident
> *chip, u32 ident, u32 revision)
> @@ -1612,6 +1614,7 @@ int v4l2_chip_ident_i2c_client(struct i2
>  	}
>  	return 0;
>  }
> +EXPORT_SYMBOL(v4l2_chip_ident_i2c_client);
>  
>  int v4l2_chip_match_host(u32 match_type, u32 match_chip)
>  {
> @@ -1622,6 +1625,7 @@ int v4l2_chip_match_host(u32 match_type,
>  		return 0;
>  	}
>  }
> +EXPORT_SYMBOL(v4l2_chip_match_host);
>  
>  /* ----------------------------------------------------------------- */
>  
> @@ -1656,6 +1660,8 @@ int v4l2_i2c_attach(struct i2c_adapter *
>  	}
>  	return err != -ENOMEM ? 0 : err;
>  }
> +EXPORT_SYMBOL(v4l2_i2c_attach);
> +#endif
>  
>  /* ----------------------------------------------------------------- */
>  
> @@ -1679,15 +1685,3 @@ EXPORT_SYMBOL(v4l2_ctrl_query_menu);
>  EXPORT_SYMBOL(v4l2_ctrl_query_menu);
>  EXPORT_SYMBOL(v4l2_ctrl_query_fill);
>  EXPORT_SYMBOL(v4l2_ctrl_query_fill_std);
> -
> -EXPORT_SYMBOL(v4l2_chip_match_i2c_client);
> -EXPORT_SYMBOL(v4l2_chip_ident_i2c_client);
> -EXPORT_SYMBOL(v4l2_chip_match_host);
> -
> -EXPORT_SYMBOL(v4l2_i2c_attach);
> -
> -/*
> - * Local variables:
> - * c-basic-offset: 8
> - * End:
> - */

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [v4l-dvb-maintainer] 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Trent Piepho]

On Mon, 28 Jan 2008, Adrian Bunk wrote:
> On Sun, Jan 27, 2008 at 10:33:34PM -0200, Mauro Carvalho Chehab wrote:
> > On Sun, 27 Jan 2008 20:52:16 +0200
> > Adrian Bunk <bunk@kernel.org> wrote:
> >
> > > Commit 8ffbc6559493c64d6194c92d856196fdaeb8a5fb causes the following
> > > compile error with CONFIG_VIDEO_DEV=y/m, CONFIG_I2C=n:
> > >
> > > ERROR: "i2c_attach_client" [drivers/media/video/v4l2-common.ko] undefined!

> > Thanks for getting this regression. Please test the enclosed patch.
>
> It doesn't enable the code if CONFIG_VIDEO_DEV=m, CONFIG_I2C=m.
>
> And what should happen if CONFIG_VIDEO_DEV=y, CONFIG_I2C=m?
>
> If the latter should offer this code to modules we'll have to put it
> into an own module.

Adrian beat me to it, but I was going to point out the same thing.  Also
only the function v4l2_i2c_attach() is a problem.  The other functions,
like v4l2_chip_match_i2c_client(), which the patch put inside an #ifdef
don't use any i2c symbols and don't need to be protected.
v4l2_chip_match_host() could even easily be used by a driver that doesn't
need I2C.

Maybe the kernel headers should provide a couple macros for testing
configs, since people get it wrong over and over again?

#define CONFIG_ON(x) (defined(CONFIG_##x) || defined(CONFIG_##x##_MODULE))
#define CONFIG_AVAIABLE(x) (defined(CONFIG_##x) || (defined(MODULE) && defined(CONFIG_##x##_MODULE)))

Would save typing too.

Not sure what to do about VIDEO_DEV=y, I2C=m.  It should be possible except
for this function.

> > diff -r b0815101889d linux/drivers/media/video/v4l2-common.c
> > --- a/linux/drivers/media/video/v4l2-common.c	Sun Jan 27 20:39:00 2008
> > -0200 +++ b/linux/drivers/media/video/v4l2-common.c	Sun Jan 27 22:23:25
> > 2008 -0200 @@ -1585,6 +1585,7 @@ u32 v4l2_ctrl_next(const u32 * const * c
> >  	return **ctrl_classes;
> >  }
> >
> > +#ifdef CONFIG_I2C
> >  int v4l2_chip_match_i2c_client(struct i2c_client *c, u32 match_type, u32
> > match_chip) {
> >  	switch (match_type) {
> > @@ -1596,6 +1597,7 @@ int v4l2_chip_match_i2c_client(struct i2
> >  		return 0;
> >  	}
> >  }
> > +EXPORT_SYMBOL(v4l2_chip_match_i2c_client);
> >
> >  int v4l2_chip_ident_i2c_client(struct i2c_client *c, struct v4l2_chip_ident
> > *chip, u32 ident, u32 revision)
> > @@ -1612,6 +1614,7 @@ int v4l2_chip_ident_i2c_client(struct i2
> >  	}
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL(v4l2_chip_ident_i2c_client);
> >
> >  int v4l2_chip_match_host(u32 match_type, u32 match_chip)
> >  {
> > @@ -1622,6 +1625,7 @@ int v4l2_chip_match_host(u32 match_type,
> >  		return 0;
> >  	}
> >  }
> > +EXPORT_SYMBOL(v4l2_chip_match_host);
> >
> >  /* ----------------------------------------------------------------- */
> >
> > @@ -1656,6 +1660,8 @@ int v4l2_i2c_attach(struct i2c_adapter *
> >  	}
> >  	return err != -ENOMEM ? 0 : err;
> >  }
> > +EXPORT_SYMBOL(v4l2_i2c_attach);
> > +#endif

Re: [v4l-dvb-maintainer] 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Mauro Carvalho Chehab]

> Maybe the kernel headers should provide a couple macros for testing
> configs, since people get it wrong over and over again?
> 
> #define CONFIG_ON(x) (defined(CONFIG_##x) || defined(CONFIG_##x##_MODULE))
> #define CONFIG_AVAIABLE(x) (defined(CONFIG_##x) || (defined(MODULE) && defined(CONFIG_##x##_MODULE)))

Seems a good idea to me.
> 
> Not sure what to do about VIDEO_DEV=y, I2C=m.  It should be possible except
> for this function.

I don't see much sense on allowing v4l2-common being in-kernel, while having
I2C as module. Also, creating a separate module for just a single function
seems to be overkill.

IMO, in this specific case, v4l2-common should also be a module. Not sure,
however,the better syntax on Kconfig. Once, someone suggested a very weird
syntax, like:

	depends on I2C if I2C


Cheers,
Mauro

Re: [v4l-dvb-maintainer] 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Adrian Bunk]

On Mon, Jan 28, 2008 at 09:49:12AM -0200, Mauro Carvalho Chehab wrote:
> > Maybe the kernel headers should provide a couple macros for testing
> > configs, since people get it wrong over and over again?
> > 
> > #define CONFIG_ON(x) (defined(CONFIG_##x) || defined(CONFIG_##x##_MODULE))
> > #define CONFIG_AVAIABLE(x) (defined(CONFIG_##x) || (defined(MODULE) && defined(CONFIG_##x##_MODULE)))
> 
> Seems a good idea to me.
> > 
> > Not sure what to do about VIDEO_DEV=y, I2C=m.  It should be possible except
> > for this function.
> 
> I don't see much sense on allowing v4l2-common being in-kernel, while having
> I2C as module. Also, creating a separate module for just a single function
> seems to be overkill.
> 
> IMO, in this specific case, v4l2-common should also be a module. Not sure,
> however,the better syntax on Kconfig. Once, someone suggested a very weird
> syntax, like:
> 
> 	depends on I2C if I2C

Why does anyone want to introduce such a weird syntax for something 
that already works with the simple

	depends on I2C || I2C=n

?

> Cheers,
> Mauro

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [v4l-dvb-maintainer] 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Jan Engelhardt]

On Jan 28 2008 01:05, Trent Piepho wrote:

>Maybe the kernel headers should provide a couple macros for testing
>configs, since people get it wrong over and over again?
>
>#define CONFIG_ON(x) (defined(CONFIG_##x) || defined(CONFIG_##x##_MODULE))
>#define CONFIG_AVAIABLE(x) (defined(CONFIG_##x) || (defined(MODULE) && defined(CONFIG_##x##_MODULE)))
               ^AVAILABLE(x)

What's the difference between these two?

CONFIG_x_MODULE will never be defined if MODULE is not, so defined(MODULE)
seems redundant.

>Would save typing too.

Yes please add it. (But don't try to convert any subsystems - deep dev phase)

Re: [v4l-dvb-maintainer] 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Mauro Carvalho Chehab]

On Mon, 28 Jan 2008 14:12:45 +0200
Adrian Bunk <bunk@kernel.org> wrote:

> On Mon, Jan 28, 2008 at 09:49:12AM -0200, Mauro Carvalho Chehab wrote:
> > > Maybe the kernel headers should provide a couple macros for testing
> > > configs, since people get it wrong over and over again?
> > > 
> > > #define CONFIG_ON(x) (defined(CONFIG_##x) || defined(CONFIG_##x##_MODULE))
> > > #define CONFIG_AVAIABLE(x) (defined(CONFIG_##x) || (defined(MODULE) && defined(CONFIG_##x##_MODULE)))
> > 
> > Seems a good idea to me.
> > > 
> > > Not sure what to do about VIDEO_DEV=y, I2C=m.  It should be possible except
> > > for this function.
> > 
> > I don't see much sense on allowing v4l2-common being in-kernel, while having
> > I2C as module. Also, creating a separate module for just a single function
> > seems to be overkill.
> > 
> > IMO, in this specific case, v4l2-common should also be a module. Not sure,
> > however,the better syntax on Kconfig. Once, someone suggested a very weird
> > syntax, like:
> > 
> > 	depends on I2C if I2C
> 
> Why does anyone want to introduce such a weird syntax for something 
> that already works with the simple
> 
> 	depends on I2C || I2C=n

On that time, I nacked the weird syntax. The one you've pointed seems good
enough.

Trent,

Could you please prepare the changeset for the macro, as suggested by Ian?

I'll rewrite my patch to use your macro, and add the "depends on" syntax as
suggested by Adrian.

Cheers,
Mauro

Re: [v4l-dvb-maintainer] 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Mauro Carvalho Chehab]

 
> Could you please prepare the changeset for the macro, as suggested by Ian?

In time:

s/as suggested by Ian/considering Jan's arguments/

Cheers,
Mauro

Re: [v4l-dvb-maintainer] 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Adrian Bunk]

On Mon, Jan 28, 2008 at 01:17:10PM +0100, Jan Engelhardt wrote:
> 
> On Jan 28 2008 01:05, Trent Piepho wrote:
> 
> >Maybe the kernel headers should provide a couple macros for testing
> >configs, since people get it wrong over and over again?
> >
> >#define CONFIG_ON(x) (defined(CONFIG_##x) || defined(CONFIG_##x##_MODULE))
> >#define CONFIG_AVAIABLE(x) (defined(CONFIG_##x) || (defined(MODULE) && defined(CONFIG_##x##_MODULE)))
>                ^AVAILABLE(x)
> 
> What's the difference between these two?
> 
> CONFIG_x_MODULE will never be defined if MODULE is not, so defined(MODULE)
> seems redundant.
>...

It's not redundant - x is the module you want to use something from, and 
MODULE is defined when the code you are working on gets compiled as a 
module.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [v4l-dvb-maintainer] 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Mauro Carvalho Chehab]

On Mon, 28 Jan 2008 14:12:45 +0200
Adrian Bunk <bunk@kernel.org> wrote:

>
> Why does anyone want to introduce such a weird syntax for something 
> that already works with the simple
> 
> 	depends on I2C || I2C=n
> 
This didn't work.

Something weird is happening here... It seems that "menuconfig I2C" is not
working properly, or maybe I'm blind ;)

I'm trying to generate the patch for fixing this issue. However, every time I
add a dependency for I2C, I got wrong results:

make oldconfig >/dev/null; grep CONFIG_VIDEO_DEV .config; grep CONFIG_I2C= .config; grep CONFIG_VIDEO_V4L2_COMMON .config
CONFIG_VIDEO_DEV=y
CONFIG_I2C=m
CONFIG_VIDEO_V4L2_COMMON=y

---

Trying to isolate the problem, I'm applying this patch to define
CONFIG_VIDEO_V4L2_COMMON:


diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
index 8f4a453..20a5d26 100644
--- a/drivers/media/Kconfig
+++ b/drivers/media/Kconfig
@@ -25,6 +25,11 @@ config VIDEO_DEV
          To compile this driver as a module, choose M here: the
          module will be called videodev.

+config VIDEO_V4L2_COMMON
+       tristate
+       depends on I2C
+       default y
+
 config VIDEO_V4L1
        bool "Enable Video For Linux API 1 (DEPRECATED)"
        depends on VIDEO_DEV

---

Shouldn't VIDEO_V4L2_COMMON be equal to 'm', due to I2C dependency?

Cheers,
Mauro

Re: [v4l-dvb-maintainer] 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Mauro Carvalho Chehab]

Hi Trent,

> Also  only the function v4l2_i2c_attach() is a problem.  The other functions,
> like v4l2_chip_match_i2c_client(), which the patch put inside an #ifdef
> don't use any i2c symbols and don't need to be protected.
>  could even easily be used by a driver that doesn't
> need I2C.

True, but, with the exception of v4l2_chip_match_host(), all the others have
struct i2c_client as an argument. I can't see any valid usage where someone would
fill this struct when i2c is not supported.

---

I'm committing right now a patch that should properly fix the regression. I've
checked it with -git tree and seems to work properly to me.

The Kconfig changes become this way:

diff -r ea8b678dd436 -r 3f704aa9d92e linux/drivers/media/Kconfig
--- a/linux/drivers/media/Kconfig       Mon Jan 28 22:11:19 2008 +0000
+++ b/linux/drivers/media/Kconfig       Tue Jan 29 16:32:35 2008 -0200
@@ -24,6 +24,11 @@ config VIDEO_DEV

          To compile this driver as a module, choose M here: the
          module will be called videodev.
+
+config VIDEO_V4L2_COMMON
+       tristate
+       depends on (I2C || I2C=n) && VIDEO_DEV
+       default (I2C || I2C=n) && VIDEO_DEV

The "depends on" clause is equal to default. Probably, it can be safely
removed, but this way seems clearer to my eyes.

Based on my tests, it seems that "depends on" only works fine for values that
are prompted. If the value is not prompted, it just uses whatever declared on
"default".

I'll push the complete changeset to -hg and my -git tree (at devel branch).

Cheers,
Mauro.

Re: [v4l-dvb-maintainer] 2.6.25 regression: VIDEO_DEV=y/m, I2C=n compile error

[Trent Piepho]

On Mon, 28 Jan 2008, Jan Engelhardt wrote:
> On Jan 28 2008 01:05, Trent Piepho wrote:
>
> >Maybe the kernel headers should provide a couple macros for testing
> >configs, since people get it wrong over and over again?
> >
> >#define CONFIG_ON(x) (defined(CONFIG_##x) || defined(CONFIG_##x##_MODULE))
> >#define CONFIG_AVAIABLE(x) (defined(CONFIG_##x) || (defined(MODULE) && defined(CONFIG_##x##_MODULE)))
>                ^AVAILABLE(x)
>
> What's the difference between these two?
>
> CONFIG_x_MODULE will never be defined if MODULE is not, so defined(MODULE)
> seems redundant.

You're probably thinking of CONFIG_MODULES, which indicates the kernel
supports module loading.  MODULE indicates the current code is being
compiled as a module.  The idea is that code compiled in the kernel can't
call modules.  Generally what you would see is:

#if CONFIG_ON(x)
code that will be _used by_ x
probably want x to depend on us in Kconfig
#endif
#if CONFIG_AVAILABLE(x)
code that wants _to use_ x
#endif