[PATCH] drivers/video: fix mb862xx_i2c depends issue build failure

[PATCH] drivers/video: fix mb862xx_i2c depends issue build failure

[Paul Gortmaker]

Any randconfig that sets I2C=m and FB_MB862XX_I2C=y will
encounter a final link failure that looks like this:

drivers/built-in.o: In function `mb862xx_i2c_init':
drivers/video/mb862xx/mb862xx-i2c.c:165: undefined reference to `i2c_add_adapter'
drivers/built-in.o: In function `mb862xx_i2c_exit':
drivers/video/mb862xx/mb862xx-i2c.c:176: undefined reference to `i2c_del_adapter'

Since FB_MB862XX_I2C is a bool and not tristate, simply
don't offer it at all if core I2C support is not built in.

Reported-by: Jim Davis <jim.epost@gmail.com>
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index dade5b7699bc..aefd1b9a3cbd 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2338,7 +2338,7 @@ endchoice
 
 config FB_MB862XX_I2C
 	bool "Support I2C bus on MB862XX GDC"
-	depends on FB_MB862XX && I2C
+	depends on FB_MB862XX && I2C=y
 	default y
 	help
 	  Selecting this option adds Coral-P(A)/Lime GDC I2C bus adapter
-- 
1.9.0

Re: [PATCH] drivers/video: fix mb862xx_i2c depends issue build failure

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

Hi,

On 20/03/14 17:16, Paul Gortmaker wrote:
> Any randconfig that sets I2C=m and FB_MB862XX_I2C=y will
> encounter a final link failure that looks like this:

It compiles fine with I2C=m, FB_MB862XX=m and FB_MB862XX_I2C=y.

> drivers/built-in.o: In function `mb862xx_i2c_init':
> drivers/video/mb862xx/mb862xx-i2c.c:165: undefined reference to `i2c_add_adapter'
> drivers/built-in.o: In function `mb862xx_i2c_exit':
> drivers/video/mb862xx/mb862xx-i2c.c:176: undefined reference to `i2c_del_adapter'
> 
> Since FB_MB862XX_I2C is a bool and not tristate, simply
> don't offer it at all if core I2C support is not built in.

FB_MB862XX_I2C is not a driver, it just adds the i2c support to
FB_MB862XX. The relevant thing is whether FB_MB862XX is m or y, so
compiling with:

I2C=m, FB_MB862XX=y and FB_MB862XX_I2C=y

will fail.

> Reported-by: Jim Davis <jim.epost@gmail.com>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index dade5b7699bc..aefd1b9a3cbd 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -2338,7 +2338,7 @@ endchoice
>  
>  config FB_MB862XX_I2C
>  	bool "Support I2C bus on MB862XX GDC"
> -	depends on FB_MB862XX && I2C
> +	depends on FB_MB862XX && I2C=y
>  	default y
>  	help
>  	  Selecting this option adds Coral-P(A)/Lime GDC I2C bus adapter

This fix is not correct, as it prevents the following, valid, config:

I2C=m, FB_MB862XX=m and FB_MB862XX_I2C=y

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH] drivers/video: fix mb862xx_i2c depends issue build failure

[Paul Gortmaker]

On 14-03-21 09:32 AM, Tomi Valkeinen wrote:
> Hi,
> 
> On 20/03/14 17:16, Paul Gortmaker wrote:
>> Any randconfig that sets I2C=m and FB_MB862XX_I2C=y will
>> encounter a final link failure that looks like this:
> 
> It compiles fine with I2C=m, FB_MB862XX=m and FB_MB862XX_I2C=y.
> 
>> drivers/built-in.o: In function `mb862xx_i2c_init':
>> drivers/video/mb862xx/mb862xx-i2c.c:165: undefined reference to `i2c_add_adapter'
>> drivers/built-in.o: In function `mb862xx_i2c_exit':
>> drivers/video/mb862xx/mb862xx-i2c.c:176: undefined reference to `i2c_del_adapter'
>>
>> Since FB_MB862XX_I2C is a bool and not tristate, simply
>> don't offer it at all if core I2C support is not built in.
> 
> FB_MB862XX_I2C is not a driver, it just adds the i2c support to
> FB_MB862XX. The relevant thing is whether FB_MB862XX is m or y, so
> compiling with:
> 
> I2C=m, FB_MB862XX=y and FB_MB862XX_I2C=y
> 
> will fail.

How would you suggest we fix it then?  Perhaps we could simplify the
Kconfig space and just get rid of FB_MB862XX_I2C entirely?  Is there
ever a reason why someone would want it turned off when I2C is present?

Paul.
--

> 
>> Reported-by: Jim Davis <jim.epost@gmail.com>
>> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
>> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>>
>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>> index dade5b7699bc..aefd1b9a3cbd 100644
>> --- a/drivers/video/Kconfig
>> +++ b/drivers/video/Kconfig
>> @@ -2338,7 +2338,7 @@ endchoice
>>  
>>  config FB_MB862XX_I2C
>>  	bool "Support I2C bus on MB862XX GDC"
>> -	depends on FB_MB862XX && I2C
>> +	depends on FB_MB862XX && I2C=y
>>  	default y
>>  	help
>>  	  Selecting this option adds Coral-P(A)/Lime GDC I2C bus adapter
> 
> This fix is not correct, as it prevents the following, valid, config:
> 
> I2C=m, FB_MB862XX=m and FB_MB862XX_I2C=y
> 
>  Tomi
> 
> 

Re: [PATCH] drivers/video: fix mb862xx_i2c depends issue build failure

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2241 bytes --]

On 21/03/14 15:53, Paul Gortmaker wrote:
> On 14-03-21 09:32 AM, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 20/03/14 17:16, Paul Gortmaker wrote:
>>> Any randconfig that sets I2C=m and FB_MB862XX_I2C=y will
>>> encounter a final link failure that looks like this:
>>
>> It compiles fine with I2C=m, FB_MB862XX=m and FB_MB862XX_I2C=y.
>>
>>> drivers/built-in.o: In function `mb862xx_i2c_init':
>>> drivers/video/mb862xx/mb862xx-i2c.c:165: undefined reference to `i2c_add_adapter'
>>> drivers/built-in.o: In function `mb862xx_i2c_exit':
>>> drivers/video/mb862xx/mb862xx-i2c.c:176: undefined reference to `i2c_del_adapter'
>>>
>>> Since FB_MB862XX_I2C is a bool and not tristate, simply
>>> don't offer it at all if core I2C support is not built in.
>>
>> FB_MB862XX_I2C is not a driver, it just adds the i2c support to
>> FB_MB862XX. The relevant thing is whether FB_MB862XX is m or y, so
>> compiling with:
>>
>> I2C=m, FB_MB862XX=y and FB_MB862XX_I2C=y
>>
>> will fail.
> 
> How would you suggest we fix it then?  Perhaps we could simplify the
> Kconfig space and just get rid of FB_MB862XX_I2C entirely?  Is there
> ever a reason why someone would want it turned off when I2C is present?

I'm not familiar with the driver and devices that use it, so I can't
really say. But you could probably have a board with the FB_MB862XX,
without i2c displays, while still you'd have I2C for other uses. So
there you could minimally reduce the kernel size by leaving out the
FB_MB862XX_I2C.

I'm fine with that solution, though. But how would it work in practice?
Did you mean that FB_MB862XX would depend on I2C? That's not a good
option, but how would you otherwise make the i2c dependency correct?

Actually, I'm fine with the original patch also, as I believe the case
where I2C=m is somewhat theoretical (correct me if I'm wrong). But the
patch description was totally wrong, and if that solution is preferred,
it should also clearly state that the patch prevents the I2C support
when I2C is built as a module. But I think your proposal in this mail is
better.

Maybe there are even better ways to handle it in the Kconfig, as such
"add feature X to the driver" sounds quite common to me.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH] drivers/video: fix mb862xx_i2c depends issue build failure

[Randy Dunlap]

On 03/21/2014 06:53 AM, Paul Gortmaker wrote:
> On 14-03-21 09:32 AM, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 20/03/14 17:16, Paul Gortmaker wrote:
>>> Any randconfig that sets I2C=m and FB_MB862XX_I2C=y will
>>> encounter a final link failure that looks like this:
>>
>> It compiles fine with I2C=m, FB_MB862XX=m and FB_MB862XX_I2C=y.
>>
>>> drivers/built-in.o: In function `mb862xx_i2c_init':
>>> drivers/video/mb862xx/mb862xx-i2c.c:165: undefined reference to `i2c_add_adapter'
>>> drivers/built-in.o: In function `mb862xx_i2c_exit':
>>> drivers/video/mb862xx/mb862xx-i2c.c:176: undefined reference to `i2c_del_adapter'
>>>
>>> Since FB_MB862XX_I2C is a bool and not tristate, simply
>>> don't offer it at all if core I2C support is not built in.
>>
>> FB_MB862XX_I2C is not a driver, it just adds the i2c support to
>> FB_MB862XX. The relevant thing is whether FB_MB862XX is m or y, so
>> compiling with:
>>
>> I2C=m, FB_MB862XX=y and FB_MB862XX_I2C=y
>>
>> will fail.
> 
> How would you suggest we fix it then?  Perhaps we could simplify the
> Kconfig space and just get rid of FB_MB862XX_I2C entirely?  Is there
> ever a reason why someone would want it turned off when I2C is present?
> 
> Paul.
> --
> 
>>
>>> Reported-by: Jim Davis <jim.epost@gmail.com>
>>> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
>>> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>>>
>>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>>> index dade5b7699bc..aefd1b9a3cbd 100644
>>> --- a/drivers/video/Kconfig
>>> +++ b/drivers/video/Kconfig
>>> @@ -2338,7 +2338,7 @@ endchoice
>>>  
>>>  config FB_MB862XX_I2C
>>>  	bool "Support I2C bus on MB862XX GDC"
>>> -	depends on FB_MB862XX && I2C
>>> +	depends on FB_MB862XX && I2C=y
>>>  	default y
>>>  	help
>>>  	  Selecting this option adds Coral-P(A)/Lime GDC I2C bus adapter
>>
>> This fix is not correct, as it prevents the following, valid, config:
>>
>> I2C=m, FB_MB862XX=m and FB_MB862XX_I2C=y

If I am following this correctly, this kconfig situation is often handled
by something like

	depends on I2C=y || I2Cû_MB862XX



-- 
~Randy