Re: randconfig build error with next-20141001, in drivers/i2c/algos/i2c-algo-bit.c

Re: randconfig build error with next-20141001, in drivers/i2c/algos/i2c-algo-bit.c

[Randy Dunlap]

On 10/01/14 14:37, Jim Davis wrote:
> Building with the attached random configuration file,

Also:
warning: (CAN_PEAK_PCIEC && SFC && IGB && VIDEO_TW68 && DRM && FB_DDC && FB_VIA) selects I2C_ALGOBIT which has unmet direct dependencies (I2C)

> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_bus’:
> drivers/i2c/algos/i2c-algo-bit.c:658:33: error: ‘i2c_add_adapter’
> undeclared (first use in this function)
>   return __i2c_bit_add_bus(adap, i2c_add_adapter);
>                                  ^
> drivers/i2c/algos/i2c-algo-bit.c:658:33: note: each undeclared
> identifier is reported only once for each function it appears in
> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_numbered_bus’:
> drivers/i2c/algos/i2c-algo-bit.c:664:33: error:
> ‘i2c_add_numbered_adapter’ undeclared (first use in this function)
>   return __i2c_bit_add_bus(adap, i2c_add_numbered_adapter);
>                                  ^
>   CC      net/openvswitch/actions.o
> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_bus’:
> drivers/i2c/algos/i2c-algo-bit.c:659:1: warning: control reaches end of non-void
>  function [-Wreturn-type]
>  }
>  ^
> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_numbered_bus’:
> drivers/i2c/algos/i2c-algo-bit.c:665:1: warning: control reaches end of non-void
>  function [-Wreturn-type]
>  }
>  ^
> make[3]: *** [drivers/i2c/algos/i2c-algo-bit.o] Error 1

In drivers/media/pci/tw68/Kconfig, VIDEO_TW68 should depend on I2C in order
to make it safe to select I2C_ALGOBIT.

In drivers/net/can/sja1000/Kconfig, CAN_PEAK_PCIEC should depend on I2C
instead of selecting I2C (and change the help text).


-- 
~Randy

Re: randconfig build error with next-20141001, in drivers/i2c/algos/i2c-algo-bit.c

[Oliver Hartkopp]

Hello all,

just to get it right:

So far it looks like this in linux/drivers/net/can/sja1000/Kconfig

config CAN_PEAK_PCIEC
        bool "PEAK PCAN-ExpressCard Cards"
        depends on CAN_PEAK_PCI
        select I2C
        select I2C_ALGOBIT

If one would change the

        select I2C

into

        depends on I2C

IMHO the CAN_PEAK_PCIEC hardware would *only* be visible and selectable when
I2C was selected before (from anyone else?).

So what it wrong on the current Kconfig entry?
Is 'select' deprecated?

Or did randconfig generate a configuration that would not be possible by
properly generating the config file with 'make menuconfig' ??

Please explain.

Thanks,
Oliver

On 10/02/2014 01:26 AM, Randy Dunlap wrote:
> On 10/01/14 14:37, Jim Davis wrote:
>> Building with the attached random configuration file,
> 
> Also:
> warning: (CAN_PEAK_PCIEC && SFC && IGB && VIDEO_TW68 && DRM && FB_DDC && FB_VIA) selects I2C_ALGOBIT which has unmet direct dependencies (I2C)
> 
>> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_bus’:
>> drivers/i2c/algos/i2c-algo-bit.c:658:33: error: ‘i2c_add_adapter’
>> undeclared (first use in this function)
>>   return __i2c_bit_add_bus(adap, i2c_add_adapter);
>>                                  ^
>> drivers/i2c/algos/i2c-algo-bit.c:658:33: note: each undeclared
>> identifier is reported only once for each function it appears in
>> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_numbered_bus’:
>> drivers/i2c/algos/i2c-algo-bit.c:664:33: error:
>> ‘i2c_add_numbered_adapter’ undeclared (first use in this function)
>>   return __i2c_bit_add_bus(adap, i2c_add_numbered_adapter);
>>                                  ^
>>   CC      net/openvswitch/actions.o
>> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_bus’:
>> drivers/i2c/algos/i2c-algo-bit.c:659:1: warning: control reaches end of non-void
>>  function [-Wreturn-type]
>>  }
>>  ^
>> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_numbered_bus’:
>> drivers/i2c/algos/i2c-algo-bit.c:665:1: warning: control reaches end of non-void
>>  function [-Wreturn-type]
>>  }
>>  ^
>> make[3]: *** [drivers/i2c/algos/i2c-algo-bit.o] Error 1
> 
> In drivers/media/pci/tw68/Kconfig, VIDEO_TW68 should depend on I2C in order
> to make it safe to select I2C_ALGOBIT.
> 
> In drivers/net/can/sja1000/Kconfig, CAN_PEAK_PCIEC should depend on I2C
> instead of selecting I2C (and change the help text).
> 
> 

Re: randconfig build error with next-20141001, in drivers/i2c/algos/i2c-algo-bit.c

[Randy Dunlap]

On 10/06/14 01:06, Oliver Hartkopp wrote:
> Hello all,
> 
> just to get it right:
> 
> So far it looks like this in linux/drivers/net/can/sja1000/Kconfig
> 
> config CAN_PEAK_PCIEC
>         bool "PEAK PCAN-ExpressCard Cards"
>         depends on CAN_PEAK_PCI
>         select I2C
>         select I2C_ALGOBIT
> 
> If one would change the
> 
>         select I2C
> 
> into
> 
>         depends on I2C
> 
> IMHO the CAN_PEAK_PCIEC hardware would *only* be visible and selectable when
> I2C was selected before (from anyone else?).

That is correct.

> So what it wrong on the current Kconfig entry?
> Is 'select' deprecated?

No, it's not deprecated.  It's just dangerous.  and driver configs should not
enable entire subsystems via 'select'.

> Or did randconfig generate a configuration that would not be possible by
> properly generating the config file with 'make menuconfig' ??

randconfig generated a config for another driver which causes a build error,
not for a CAN driver.  The CAN driver does not have a build error AFAIK.
Its Kconfig is just doing something with a very big & ugly stick.


> Please explain.
> 
> Thanks,
> Oliver
> 
> On 10/02/2014 01:26 AM, Randy Dunlap wrote:
>> On 10/01/14 14:37, Jim Davis wrote:
>>> Building with the attached random configuration file,
>>
>> Also:
>> warning: (CAN_PEAK_PCIEC && SFC && IGB && VIDEO_TW68 && DRM && FB_DDC && FB_VIA) selects I2C_ALGOBIT which has unmet direct dependencies (I2C)
>>
>>> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_bus’:
>>> drivers/i2c/algos/i2c-algo-bit.c:658:33: error: ‘i2c_add_adapter’
>>> undeclared (first use in this function)
>>>   return __i2c_bit_add_bus(adap, i2c_add_adapter);
>>>                                  ^
>>> drivers/i2c/algos/i2c-algo-bit.c:658:33: note: each undeclared
>>> identifier is reported only once for each function it appears in
>>> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_numbered_bus’:
>>> drivers/i2c/algos/i2c-algo-bit.c:664:33: error:
>>> ‘i2c_add_numbered_adapter’ undeclared (first use in this function)
>>>   return __i2c_bit_add_bus(adap, i2c_add_numbered_adapter);
>>>                                  ^
>>>   CC      net/openvswitch/actions.o
>>> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_bus’:
>>> drivers/i2c/algos/i2c-algo-bit.c:659:1: warning: control reaches end of non-void
>>>  function [-Wreturn-type]
>>>  }
>>>  ^
>>> drivers/i2c/algos/i2c-algo-bit.c: In function ‘i2c_bit_add_numbered_bus’:
>>> drivers/i2c/algos/i2c-algo-bit.c:665:1: warning: control reaches end of non-void
>>>  function [-Wreturn-type]
>>>  }
>>>  ^
>>> make[3]: *** [drivers/i2c/algos/i2c-algo-bit.o] Error 1
>>
>> In drivers/media/pci/tw68/Kconfig, VIDEO_TW68 should depend on I2C in order
>> to make it safe to select I2C_ALGOBIT.
>>
>> In drivers/net/can/sja1000/Kconfig, CAN_PEAK_PCIEC should depend on I2C
>> instead of selecting I2C (and change the help text).


-- 
~Randy

Re: randconfig build error with next-20141001, in drivers/i2c/algos/i2c-algo-bit.c

[Oliver Hartkopp]

On 10/06/2014 06:52 PM, Randy Dunlap wrote:
> On 10/06/14 01:06, Oliver Hartkopp wrote:
>> Hello all,
>>
>> just to get it right:
>>
>> So far it looks like this in linux/drivers/net/can/sja1000/Kconfig
>>
>> config CAN_PEAK_PCIEC
>>         bool "PEAK PCAN-ExpressCard Cards"
>>         depends on CAN_PEAK_PCI
>>         select I2C
>>         select I2C_ALGOBIT
>>
>> If one would change the
>>
>>         select I2C
>>
>> into
>>
>>         depends on I2C
>>
>> IMHO the CAN_PEAK_PCIEC hardware would *only* be visible and selectable when
>> I2C was selected before (from anyone else?).
> 
> That is correct.
> 
>> So what it wrong on the current Kconfig entry?
>> Is 'select' deprecated?
> 
> No, it's not deprecated.  It's just dangerous.  and driver configs should not
> enable entire subsystems via 'select'.
> 
>> Or did randconfig generate a configuration that would not be possible by
>> properly generating the config file with 'make menuconfig' ??
> 
> randconfig generated a config for another driver which causes a build error,
> not for a CAN driver.  The CAN driver does not have a build error AFAIK.
> Its Kconfig is just doing something with a very big & ugly stick.

But when it is not done like this, we might have an invisible config option in
the corner case that I2C is not enabled by anyone else.

So what would you propose then?

AFAICS there is 'just' a style problem as 'configs should not enable entire
subsystems'. But it finally is a correct and valid Kconfig, right?

When I2C is already enabled - fine. If (unlikely) I2C is not enabled, we need
to pull the ugly stick. So what is dangerous on this? Was there any misuse of
select statements before?

Best regards,
Oliver

Re: randconfig build error with next-20141001, in drivers/i2c/algos/i2c-algo-bit.c

[Randy Dunlap]

On 10/06/14 10:39, Oliver Hartkopp wrote:
> 
> 
> On 10/06/2014 06:52 PM, Randy Dunlap wrote:
>> On 10/06/14 01:06, Oliver Hartkopp wrote:
>>> Hello all,
>>>
>>> just to get it right:
>>>
>>> So far it looks like this in linux/drivers/net/can/sja1000/Kconfig
>>>
>>> config CAN_PEAK_PCIEC
>>>         bool "PEAK PCAN-ExpressCard Cards"
>>>         depends on CAN_PEAK_PCI
>>>         select I2C
>>>         select I2C_ALGOBIT
>>>
>>> If one would change the
>>>
>>>         select I2C
>>>
>>> into
>>>
>>>         depends on I2C
>>>
>>> IMHO the CAN_PEAK_PCIEC hardware would *only* be visible and selectable when
>>> I2C was selected before (from anyone else?).
>>
>> That is correct.
>>
>>> So what it wrong on the current Kconfig entry?
>>> Is 'select' deprecated?
>>
>> No, it's not deprecated.  It's just dangerous.  and driver configs should not
>> enable entire subsystems via 'select'.
>>
>>> Or did randconfig generate a configuration that would not be possible by
>>> properly generating the config file with 'make menuconfig' ??
>>
>> randconfig generated a config for another driver which causes a build error,
>> not for a CAN driver.  The CAN driver does not have a build error AFAIK.
>> Its Kconfig is just doing something with a very big & ugly stick.
> 
> But when it is not done like this, we might have an invisible config option in
> the corner case that I2C is not enabled by anyone else.
> 
> So what would you propose then?
> 
> AFAICS there is 'just' a style problem as 'configs should not enable entire
> subsystems'. But it finally is a correct and valid Kconfig, right?

Yes, right.

> When I2C is already enabled - fine. If (unlikely) I2C is not enabled, we need
> to pull the ugly stick. So what is dangerous on this? Was there any misuse of
> select statements before?

No syntactic misuse, more of a style thing, like you say.

The danger is in select being a big stick that does not check for symbol
dependencies.

In the unlikely case that I2C is not enabled, the user should have to enable
it instead of a solitary driver enabling it.  IOW, if a subsystem is disabled,
the user probably wanted it that way and a single driver should not override
that setting.


-- 
~Randy

Re: randconfig build error with next-20141001, in drivers/i2c/algos/i2c-algo-bit.c

[Oliver Hartkopp]

On 10/06/2014 08:09 PM, Randy Dunlap wrote:
> On 10/06/14 10:39, Oliver Hartkopp wrote:

>> AFAICS there is 'just' a style problem as 'configs should not enable entire
>> subsystems'. But it finally is a correct and valid Kconfig, right?
> 
> Yes, right.

(..)

> In the unlikely case that I2C is not enabled, the user should have to enable
> it instead of a solitary driver enabling it.  IOW, if a subsystem is disabled,
> the user probably wanted it that way and a single driver should not override
> that setting.

Due to the fact that a change to 'depends on I2C' would make the config option
invisible (and therefore not selectable) in the case I2C was (unlikely)
disabled I would finally vote to leave it as-is.

The current Kconfig entry already contains a description that points to the
requirement to have I2C and I2C_ALGOBIT to be enabled to compile this driver:

config CAN_PEAK_PCIEC
	bool "PEAK PCAN-ExpressCard Cards"
	depends on CAN_PEAK_PCI
	select I2C
	select I2C_ALGOBIT
	default y
	---help---
	  Say Y here if you want to use a PCAN-ExpressCard from PEAK-System
	  Technik. This will also automatically select I2C and I2C_ALGO
	  configuration options.

AFAIK the PEAK PCAN-ExpressCard is usually used in x86 architecture Laptops,
so it's near to an academic discussion as x86 usually selects I2C ;-)

@Stephane: When updating the help text to introduce the PCAN-ExpressCard 34
support anyway you might probably add some more information *why* the I2C
support is needed (for CAN transceiver settings and status LED).

And /s/I2C_ALGO/I2C_ALGOBIT/ :-)

Tnx & best regards,
Oliver

Re: randconfig build error with next-20141001, in drivers/i2c/algos/i2c-algo-bit.c

[Stephane Grosjean]

Le 07/10/2014 10:58, Oliver Hartkopp a écrit :
> On 10/06/2014 08:09 PM, Randy Dunlap wrote:
>> On 10/06/14 10:39, Oliver Hartkopp wrote:
>>> AFAICS there is 'just' a style problem as 'configs should not enable entire
>>> subsystems'. But it finally is a correct and valid Kconfig, right?
>> Yes, right.
> (..)
>
>> In the unlikely case that I2C is not enabled, the user should have to enable
>> it instead of a solitary driver enabling it.  IOW, if a subsystem is disabled,
>> the user probably wanted it that way and a single driver should not override
>> that setting.
> Due to the fact that a change to 'depends on I2C' would make the config option
> invisible (and therefore not selectable) in the case I2C was (unlikely)
> disabled I would finally vote to leave it as-is.
>
> The current Kconfig entry already contains a description that points to the
> requirement to have I2C and I2C_ALGOBIT to be enabled to compile this driver:
>
> config CAN_PEAK_PCIEC
> 	bool "PEAK PCAN-ExpressCard Cards"
> 	depends on CAN_PEAK_PCI
> 	select I2C
> 	select I2C_ALGOBIT
> 	default y
> 	---help---
> 	  Say Y here if you want to use a PCAN-ExpressCard from PEAK-System
> 	  Technik. This will also automatically select I2C and I2C_ALGO
> 	  configuration options.
>
> AFAIK the PEAK PCAN-ExpressCard is usually used in x86 architecture Laptops,
> so it's near to an academic discussion as x86 usually selects I2C ;-)
>
> @Stephane: When updating the help text to introduce the PCAN-ExpressCard 34
> support anyway you might probably add some more information *why* the I2C
> support is needed (for CAN transceiver settings and status LED).
>
> And /s/I2C_ALGO/I2C_ALGOBIT/ :-)

Ok! (FYI, I had already prepared the help text for introducing the 
PCIEC34. I will subst I2C_ALGO as well. I'll prepare the patch asap...)

Regards,

Stéphane
> Tnx & best regards,
> Oliver

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--

Re: randconfig build error with next-20141001, in drivers/i2c/algos/i2c-algo-bit.c

[Randy Dunlap]

On 10/07/14 01:58, Oliver Hartkopp wrote:
> On 10/06/2014 08:09 PM, Randy Dunlap wrote:
>> On 10/06/14 10:39, Oliver Hartkopp wrote:
> 
>>> AFAICS there is 'just' a style problem as 'configs should not enable entire
>>> subsystems'. But it finally is a correct and valid Kconfig, right?
>>
>> Yes, right.
> 
> (..)
> 
>> In the unlikely case that I2C is not enabled, the user should have to enable
>> it instead of a solitary driver enabling it.  IOW, if a subsystem is disabled,
>> the user probably wanted it that way and a single driver should not override
>> that setting.
> 
> Due to the fact that a change to 'depends on I2C' would make the config option
> invisible (and therefore not selectable) in the case I2C was (unlikely)
> disabled I would finally vote to leave it as-is.
> 
> The current Kconfig entry already contains a description that points to the
> requirement to have I2C and I2C_ALGOBIT to be enabled to compile this driver:
> 
> config CAN_PEAK_PCIEC
> 	bool "PEAK PCAN-ExpressCard Cards"
> 	depends on CAN_PEAK_PCI
> 	select I2C
> 	select I2C_ALGOBIT
> 	default y
> 	---help---
> 	  Say Y here if you want to use a PCAN-ExpressCard from PEAK-System
> 	  Technik. This will also automatically select I2C and I2C_ALGO
> 	  configuration options.
> 
> AFAIK the PEAK PCAN-ExpressCard is usually used in x86 architecture Laptops,
> so it's near to an academic discussion as x86 usually selects I2C ;-)

Pray tell where does x86 usually select I2C?
Thanks.

> @Stephane: When updating the help text to introduce the PCAN-ExpressCard 34
> support anyway you might probably add some more information *why* the I2C
> support is needed (for CAN transceiver settings and status LED).
> 
> And /s/I2C_ALGO/I2C_ALGOBIT/ :-)


-- 
~Randy