Discussion:
randconfig build error with next-20141001, in drivers/i2c/algos/i2c-algo-bit.c
Randy Dunlap
2014-10-01 23:26:11 UTC
Permalink
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:658:33: error: =91i2c_add_adapter=92
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 =91i2c_bit_add_numbered=
=91i2c_add_numbered_adapter=92 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:659:1: warning: control reaches end =
of non-void
function [-Wreturn-type]
}
^
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_numbered=
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 o=
rder
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).


--=20
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Oliver Hartkopp
2014-10-02 15:18:13 UTC
Permalink
Hello Stephane,

can you please send a short patch for that to Marc/linux-can ?
I'm currently out of office.

Tnx,
Oliver


-------- Forwarded Message --------
Subject: Re: randconfig build error with next-20141001, in
drivers/i2c/algos/i2c-algo-bit.c
Date: Wed, 01 Oct 2014 16:26:11 -0700
=46rom: Randy Dunlap <***@infradead.org>
To: Jim Davis <***@gmail.com>, Stephen Rothwell <***@canb.auug.or=
g.au>,
linux-next <linux-***@vger.kernel.org>, linux-kernel
<linux-***@vger.kernel.org>, ***@the-dreams.de, ***@linux-fr.org, =
Paul
Gortmaker <***@windriver.com>, linux-***@vger.kernel.org,
***@vger.kernel.org <***@vger.kernel.org>, linux-***@vger.kernel.=
org,
linux-media <linux-***@vger.kernel.org>, Hans Verkuil <***@xs4al=
l.nl>
Building with the attached random configuration file,
Also:
warning: (CAN_PEAK_PCIEC && SFC && IGB && VIDEO_TW68 && DRM && FB_DDC &=
&
=46B_VIA) selects I2C_ALGOBIT which has unmet direct dependencies (I2C)
drivers/i2c/algos/i2c-algo-bit.c:658:33: error: =91i2c_add_adapter=92
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 =91i2c_bit_add_numbered=
=91i2c_add_numbered_adapter=92 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:659:1: warning: control reaches end =
of non-void
function [-Wreturn-type]
}
^
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_numbered=
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 o=
rder
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).


--=20
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephane Grosjean
2014-10-06 07:52:51 UTC
Permalink
Hi Oliver,

Sorry I was not at the office last week too ... :-/

So, I send the patch to Marc asap ...

Regards,

St=E9phane
Post by Oliver Hartkopp
Hello Stephane,
can you please send a short patch for that to Marc/linux-can ?
I'm currently out of office.
Tnx,
Oliver
-------- Forwarded Message --------
Subject: Re: randconfig build error with next-20141001, in
drivers/i2c/algos/i2c-algo-bit.c
Date: Wed, 01 Oct 2014 16:26:11 -0700
org.au>,
, Paul
l.org,
all.nl>
Post by Oliver Hartkopp
Building with the attached random configuration file,
warning: (CAN_PEAK_PCIEC && SFC && IGB && VIDEO_TW68 && DRM && FB_DDC=
&&
Post by Oliver Hartkopp
FB_VIA) selects I2C_ALGOBIT which has unmet direct dependencies (I2C)
drivers/i2c/algos/i2c-algo-bit.c:658:33: error: =91i2c_add_adapter=92
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 =91i2c_bit_add_numbere=
=91i2c_add_numbered_adapter=92 undeclared (first use in this functio=
n)
Post by Oliver Hartkopp
return __i2c_bit_add_bus(adap, i2c_add_numbered_adapter);
^
CC net/openvswitch/actions.o
drivers/i2c/algos/i2c-algo-bit.c:659:1: warning: control reaches end=
of non-void
Post by Oliver Hartkopp
function [-Wreturn-type]
}
^
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_numbere=
drivers/i2c/algos/i2c-algo-bit.c:665:1: warning: control reaches end=
of non-void
Post by Oliver Hartkopp
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
Post by Oliver Hartkopp
to make it safe to select I2C_ALGOBIT.
In drivers/net/can/sja1000/Kconfig, CAN_PEAK_PCIEC should depend on I=
2C
Post by Oliver Hartkopp
instead of selecting I2C (and change the help text).
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183=20
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Oliver Hartkopp
2014-10-06 08:09:58 UTC
Permalink
Hi Stephane,

I'm back home.

But I wondered if this problem really is a problem with our current Kco=
nfig.

Let's first wait for the answer from the complaining guys.

Best regards,
Oliver
Post by Stephane Grosjean
Hi Oliver,
=20
Sorry I was not at the office last week too ... :-/
=20
So, I send the patch to Marc asap ...
=20
Regards,
=20
St=E9phane
=20
=20
=20
Post by Oliver Hartkopp
Hello Stephane,
can you please send a short patch for that to Marc/linux-can ?
I'm currently out of office.
Tnx,
Oliver
-------- Forwarded Message --------
Subject: Re: randconfig build error with next-20141001, in
drivers/i2c/algos/i2c-algo-bit.c
Date: Wed, 01 Oct 2014 16:26:11 -0700
=2Eorg.au>,
g, Paul
el.org,
4all.nl>
Post by Stephane Grosjean
Post by Oliver Hartkopp
Building with the attached random configuration file,
warning: (CAN_PEAK_PCIEC && SFC && IGB && VIDEO_TW68 && DRM && FB_DD=
C &&
Post by Stephane Grosjean
Post by Oliver Hartkopp
FB_VIA) selects I2C_ALGOBIT which has unmet direct dependencies (I2C=
)
Post by Stephane Grosjean
Post by Oliver Hartkopp
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_bus=92=
drivers/i2c/algos/i2c-algo-bit.c:658:33: error: =91i2c_add_adapter=92
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 =91i2c_bit_add_number=
=91i2c_add_numbered_adapter=92 undeclared (first use in this functi=
on)
Post by Stephane Grosjean
Post by Oliver Hartkopp
return __i2c_bit_add_bus(adap, i2c_add_numbered_adapter);
^
CC net/openvswitch/actions.o
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_bus=92=
drivers/i2c/algos/i2c-algo-bit.c:659:1: warning: control reaches en=
d of
Post by Stephane Grosjean
Post by Oliver Hartkopp
non-void
function [-Wreturn-type]
}
^
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_number=
drivers/i2c/algos/i2c-algo-bit.c:665:1: warning: control reaches en=
d of
Post by Stephane Grosjean
Post by Oliver Hartkopp
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 i=
n order
Post by Stephane Grosjean
Post by Oliver Hartkopp
to make it safe to select I2C_ALGOBIT.
In drivers/net/can/sja1000/Kconfig, CAN_PEAK_PCIEC should depend on =
I2C
Post by Stephane Grosjean
Post by Oliver Hartkopp
instead of selecting I2C (and change the help text).
=20
--=20
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 Geschaeftsfuehrung: Alexander Gach=
, Uwe
Post by Stephane Grosjean
Wilhelm
--=20
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephane Grosjean
2014-10-06 08:42:52 UTC
Permalink
Hi Oliver,
Post by Oliver Hartkopp
Hi Stephane,
I'm back home.
But I wondered if this problem really is a problem with our current K=
config.

Well, you're right when you say that the PEAK_PCIEC option won't be=20
visible if CONFIG_I2C is not selected.
AFAIR, my first version of Kconfig used the "depends on" keyword rathe=
r=20
then the "select" one.
Anyway, if we had to change this Kconfig, we could take the opportunity=
=20
to upgrade the help text, by adding the info of the support of the new=20
PCAN-ExpressCard 34.

Regards,

St=E9phane
Post by Oliver Hartkopp
Let's first wait for the answer from the complaining guys.
Best regards,
Oliver
Post by Stephane Grosjean
Hi Oliver,
Sorry I was not at the office last week too ... :-/
So, I send the patch to Marc asap ...
Regards,
St=E9phane
Post by Oliver Hartkopp
Hello Stephane,
can you please send a short patch for that to Marc/linux-can ?
I'm currently out of office.
Tnx,
Oliver
-------- Forwarded Message --------
Subject: Re: randconfig build error with next-20141001, in
drivers/i2c/algos/i2c-algo-bit.c
Date: Wed, 01 Oct 2014 16:26:11 -0700
g.org.au>,
rg, Paul
,
nel.org,
s4all.nl>
Post by Oliver Hartkopp
Post by Stephane Grosjean
Post by Oliver Hartkopp
Building with the attached random configuration file,
warning: (CAN_PEAK_PCIEC && SFC && IGB && VIDEO_TW68 && DRM && FB_D=
DC &&
Post by Oliver Hartkopp
Post by Stephane Grosjean
Post by Oliver Hartkopp
FB_VIA) selects I2C_ALGOBIT which has unmet direct dependencies (I2=
C)
Post by Oliver Hartkopp
Post by Stephane Grosjean
Post by Oliver Hartkopp
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_bus=92=
drivers/i2c/algos/i2c-algo-bit.c:658:33: error: =91i2c_add_adapter=
=92
Post by Oliver Hartkopp
Post by Stephane Grosjean
Post by Oliver Hartkopp
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 =91i2c_bit_add_numbe=
=91i2c_add_numbered_adapter=92 undeclared (first use in this funct=
ion)
Post by Oliver Hartkopp
Post by Stephane Grosjean
Post by Oliver Hartkopp
return __i2c_bit_add_bus(adap, i2c_add_numbered_adapter);
^
CC net/openvswitch/actions.o
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_bus=92=
drivers/i2c/algos/i2c-algo-bit.c:659:1: warning: control reaches e=
nd of
Post by Oliver Hartkopp
Post by Stephane Grosjean
Post by Oliver Hartkopp
non-void
function [-Wreturn-type]
}
^
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_numbe=
drivers/i2c/algos/i2c-algo-bit.c:665:1: warning: control reaches e=
nd of
Post by Oliver Hartkopp
Post by Stephane Grosjean
Post by Oliver Hartkopp
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
Post by Oliver Hartkopp
Post by Stephane Grosjean
Post by Oliver Hartkopp
to make it safe to select I2C_ALGOBIT.
In drivers/net/can/sja1000/Kconfig, CAN_PEAK_PCIEC should depend on=
I2C
Post by Oliver Hartkopp
Post by Stephane Grosjean
Post by Oliver Hartkopp
instead of selecting I2C (and change the help text).
--=20
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 Geschaeftsfuehrung: Alexander Gac=
h, Uwe
Post by Oliver Hartkopp
Post by Stephane Grosjean
Wilhelm
--=20
--
To unsubscribe from this list: send the line "unsubscribe linux-can" =
in
Post by Oliver Hartkopp
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183=20
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Oliver Hartkopp
2014-10-06 08:06:00 UTC
Permalink
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 b=
y
properly generating the config file with 'make menuconfig' ??

Please explain.

Thanks,
Oliver
Post by Oliver Hartkopp
Building with the attached random configuration file,
=20
warning: (CAN_PEAK_PCIEC && SFC && IGB && VIDEO_TW68 && DRM && FB_DDC=
&& FB_VIA) selects I2C_ALGOBIT which has unmet direct dependencies (I2=
C)
Post by Oliver Hartkopp
=20
drivers/i2c/algos/i2c-algo-bit.c:658:33: error: =91i2c_add_adapter=92
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 =91i2c_bit_add_numbere=
=91i2c_add_numbered_adapter=92 undeclared (first use in this functio=
n)
Post by Oliver Hartkopp
return __i2c_bit_add_bus(adap, i2c_add_numbered_adapter);
^
CC net/openvswitch/actions.o
drivers/i2c/algos/i2c-algo-bit.c:659:1: warning: control reaches end=
of non-void
Post by Oliver Hartkopp
function [-Wreturn-type]
}
^
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_numbere=
drivers/i2c/algos/i2c-algo-bit.c:665:1: warning: control reaches end=
of non-void
Post by Oliver Hartkopp
function [-Wreturn-type]
}
^
make[3]: *** [drivers/i2c/algos/i2c-algo-bit.o] Error 1
=20
In drivers/media/pci/tw68/Kconfig, VIDEO_TW68 should depend on I2C in=
order
Post by Oliver Hartkopp
to make it safe to select I2C_ALGOBIT.
=20
In drivers/net/can/sja1000/Kconfig, CAN_PEAK_PCIEC should depend on I=
2C
Post by Oliver Hartkopp
instead of selecting I2C (and change the help text).
=20
=20
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Randy Dunlap
2014-10-06 16:52:22 UTC
Permalink
Post by Oliver Hartkopp
Hello all,
=20
=20
So far it looks like this in linux/drivers/net/can/sja1000/Kconfig
=20
config CAN_PEAK_PCIEC
bool "PEAK PCAN-ExpressCard Cards"
depends on CAN_PEAK_PCI
select I2C
select I2C_ALGOBIT
=20
If one would change the
=20
select I2C
=20
into
=20
depends on I2C
=20
IMHO the CAN_PEAK_PCIEC hardware would *only* be visible and selectab=
le when
Post by Oliver Hartkopp
I2C was selected before (from anyone else?).
That is correct.
Post by Oliver Hartkopp
So what it wrong on the current Kconfig entry?
Is 'select' deprecated?
No, it's not deprecated. It's just dangerous. and driver configs shou=
ld not
enable entire subsystems via 'select'.
Post by Oliver Hartkopp
Or did randconfig generate a configuration that would not be possible=
by
Post by Oliver Hartkopp
properly generating the config file with 'make menuconfig' ??
randconfig generated a config for another driver which causes a build e=
rror,
not for a CAN driver. The CAN driver does not have a build error AFAIK=
=2E
Its Kconfig is just doing something with a very big & ugly stick.
Post by Oliver Hartkopp
Please explain.
=20
Thanks,
Oliver
=20
Post by Oliver Hartkopp
Building with the attached random configuration file,
warning: (CAN_PEAK_PCIEC && SFC && IGB && VIDEO_TW68 && DRM && FB_DD=
C && FB_VIA) selects I2C_ALGOBIT which has unmet direct dependencies (I=
2C)
Post by Oliver Hartkopp
Post by Oliver Hartkopp
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_bus=92=
drivers/i2c/algos/i2c-algo-bit.c:658:33: error: =91i2c_add_adapter=92
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 =91i2c_bit_add_number=
=91i2c_add_numbered_adapter=92 undeclared (first use in this functi=
on)
Post by Oliver Hartkopp
Post by Oliver Hartkopp
return __i2c_bit_add_bus(adap, i2c_add_numbered_adapter);
^
CC net/openvswitch/actions.o
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_bus=92=
drivers/i2c/algos/i2c-algo-bit.c:659:1: warning: control reaches en=
d of non-void
Post by Oliver Hartkopp
Post by Oliver Hartkopp
function [-Wreturn-type]
}
^
drivers/i2c/algos/i2c-algo-bit.c: In function =91i2c_bit_add_number=
drivers/i2c/algos/i2c-algo-bit.c:665:1: warning: control reaches en=
d of non-void
Post by Oliver Hartkopp
Post by Oliver Hartkopp
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 i=
n order
Post by Oliver Hartkopp
Post by Oliver Hartkopp
to make it safe to select I2C_ALGOBIT.
In drivers/net/can/sja1000/Kconfig, CAN_PEAK_PCIEC should depend on =
I2C
Post by Oliver Hartkopp
Post by Oliver Hartkopp
instead of selecting I2C (and change the help text).
--=20
~Randy
Oliver Hartkopp
2014-10-06 17:39:50 UTC
Permalink
Post by Randy Dunlap
Post by Oliver Hartkopp
Hello all,
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.
Post by Oliver Hartkopp
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'.
Post by Oliver Hartkopp
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
Randy Dunlap
2014-10-06 18:09:44 UTC
Permalink
Post by Oliver Hartkopp
Post by Randy Dunlap
Post by Oliver Hartkopp
Hello all,
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.
Post by Oliver Hartkopp
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'.
Post by Oliver Hartkopp
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.
Post by Oliver Hartkopp
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
Oliver Hartkopp
2014-10-07 08:58:25 UTC
Permalink
Post by Randy Dunlap
Post by Oliver Hartkopp
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.
(..)
Post by Randy Dunlap
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
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephane Grosjean
2014-10-07 12:37:30 UTC
Permalink
Post by Randy Dunlap
AFAICS there is 'just' a style problem as 'configs should not enabl=
e entire
Post by Randy Dunlap
subsystems'. But it finally is a correct and valid Kconfig, right?
Yes, right.
(..)
Post by Randy Dunlap
In the unlikely case that I2C is not enabled, the user should have t=
o enable
Post by Randy Dunlap
it instead of a solitary driver enabling it. IOW, if a subsystem is=
disabled,
Post by Randy Dunlap
the user probably wanted it that way and a single driver should not =
override
Post by Randy Dunlap
that setting.
Due to the fact that a change to 'depends on I2C' would make the conf=
ig option
invisible (and therefore not selectable) in the case I2C was (unlikel=
y)
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=
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 L=
aptops,
so it's near to an academic discussion as x86 usually selects I2C ;-)
@Stephane: When updating the help text to introduce the PCAN-ExpressC=
ard 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=20
PCIEC34. I will subst I2C_ALGO as well. I'll prepare the patch asap...)

Regards,

St=C3=A9phane
Tnx & best regards,
Oliver
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183=20
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Randy Dunlap
2014-10-07 17:03:29 UTC
Permalink
Post by Oliver Hartkopp
Post by Randy Dunlap
Post by Oliver Hartkopp
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.
(..)
Post by Randy Dunlap
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
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.
Post by Oliver Hartkopp
@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
Loading...