| Message ID | 1396811227-8851-1-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Wolfram, On Sun, 2014-04-06 at 09:07PM +0200, Wolfram Sang wrote: > During development, the driver first really needed to depend on > COMMON_CLK only. Later, it was switched to writel_relaxed, but it was > forgotten to update the dependencies, so build errors occured: > > config: make ARCH=i386 allyesconfig > > All error/warnings: > > drivers/i2c/busses/i2c-cadence.c: In function 'cdns_i2c_clear_bus_hold': > >> drivers/i2c/busses/i2c-cadence.c:168:3: error: implicit declaration > >> of function 'writel_relaxed' [-Werror=implicit-function-declaration] > > Use a very safe dependency for now. Thanks for taking care of this. Looks like the '_relaxed' IO helpers are only available on ARM. I'll give that a try and might send a patch eventually. I suspect 'ARM && COMMON_CLK' would work and be a little less restrictive. Thanks, Sören
Hello, On Sun, Apr 06, 2014 at 09:07:07PM +0200, Wolfram Sang wrote: > During development, the driver first really needed to depend on > COMMON_CLK only. Later, it was switched to writel_relaxed, but it was > forgotten to update the dependencies, so build errors occured: > > config: make ARCH=i386 allyesconfig > > All error/warnings: > > drivers/i2c/busses/i2c-cadence.c: In function 'cdns_i2c_clear_bus_hold': > >> drivers/i2c/busses/i2c-cadence.c:168:3: error: implicit declaration > >> of function 'writel_relaxed' [-Werror=implicit-function-declaration] > > Use a very safe dependency for now. > > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > --- > drivers/i2c/busses/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index 93165ff453ab..3d3b9b3577c5 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -378,7 +378,7 @@ config I2C_BLACKFIN_TWI_CLK_KHZ > > config I2C_CADENCE > tristate "Cadence I2C Controller" > - depends on COMMON_CLK > + depends on ARCH_ZYNQ I'd suggest: depends on ARM && (ARCH_ZYNC || COMPILE_TEST) This way it's available for compile testing but still it doesn't appear when configuring a kernel for a real machine. Best regards Uwe > help > Say yes here to select Cadence I2C Host Controller. This controller is > e.g. used by Xilinx Zynq. > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Mon, 7 Apr 2014 08:31:00 +0200 ?? Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > Hello, > > On Sun, Apr 06, 2014 at 09:07:07PM +0200, Wolfram Sang wrote: > > During development, the driver first really needed to depend on > > COMMON_CLK only. Later, it was switched to writel_relaxed, but it was > > forgotten to update the dependencies, so build errors occured: > > > > config: make ARCH=i386 allyesconfig > > > > All error/warnings: > > > > drivers/i2c/busses/i2c-cadence.c: In function 'cdns_i2c_clear_bus_hold': > > >> drivers/i2c/busses/i2c-cadence.c:168:3: error: implicit declaration > > >> of function 'writel_relaxed' [-Werror=implicit-function-declaration] > > > > Use a very safe dependency for now. > > > > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > > --- > > drivers/i2c/busses/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > index 93165ff453ab..3d3b9b3577c5 100644 > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -378,7 +378,7 @@ config I2C_BLACKFIN_TWI_CLK_KHZ > > > > config I2C_CADENCE > > tristate "Cadence I2C Controller" > > - depends on COMMON_CLK > > + depends on ARCH_ZYNQ > I'd suggest: > > depends on ARM && (ARCH_ZYNC || COMPILE_TEST) ARCH_ZYNC || (ARM && COMPILE_TEST) Same, but more clear. ---
On 04/07/2014 08:31 AM, Uwe Kleine-König wrote: > Hello, > > On Sun, Apr 06, 2014 at 09:07:07PM +0200, Wolfram Sang wrote: >> During development, the driver first really needed to depend on >> COMMON_CLK only. Later, it was switched to writel_relaxed, but it was >> forgotten to update the dependencies, so build errors occured: >> >> config: make ARCH=i386 allyesconfig >> >> All error/warnings: >> >> drivers/i2c/busses/i2c-cadence.c: In function 'cdns_i2c_clear_bus_hold': >>>> drivers/i2c/busses/i2c-cadence.c:168:3: error: implicit declaration >>>> of function 'writel_relaxed' [-Werror=implicit-function-declaration] >> >> Use a very safe dependency for now. >> >> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> >> --- >> drivers/i2c/busses/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index 93165ff453ab..3d3b9b3577c5 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -378,7 +378,7 @@ config I2C_BLACKFIN_TWI_CLK_KHZ >> >> config I2C_CADENCE >> tristate "Cadence I2C Controller" >> - depends on COMMON_CLK >> + depends on ARCH_ZYNQ > I'd suggest: > > depends on ARM && (ARCH_ZYNC || COMPILE_TEST) > > This way it's available for compile testing but still it doesn't appear > when configuring a kernel for a real machine. I would suggest to add relaxed IO helper function to asm-generic/io.h to be available for all archs. Then we can avoid this type of errors. Thanks, Michal
> I would suggest to add relaxed IO helper function to asm-generic/io.h > to be available for all archs. Then we can avoid this type of errors. I favour this, too. For the cadence driver, I needed a quick solution before linux-next breaks, though.
Hello, On Mon, Apr 07, 2014 at 10:37:36AM +0400, Alexander Shiyan wrote: > Mon, 7 Apr 2014 08:31:00 +0200 ?? Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > > Hello, > > > > On Sun, Apr 06, 2014 at 09:07:07PM +0200, Wolfram Sang wrote: > > > During development, the driver first really needed to depend on > > > COMMON_CLK only. Later, it was switched to writel_relaxed, but it was > > > forgotten to update the dependencies, so build errors occured: > > > > > > config: make ARCH=i386 allyesconfig > > > > > > All error/warnings: > > > > > > drivers/i2c/busses/i2c-cadence.c: In function 'cdns_i2c_clear_bus_hold': > > > >> drivers/i2c/busses/i2c-cadence.c:168:3: error: implicit declaration > > > >> of function 'writel_relaxed' [-Werror=implicit-function-declaration] > > > > > > Use a very safe dependency for now. > > > > > > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > > > --- > > > drivers/i2c/busses/Kconfig | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > > index 93165ff453ab..3d3b9b3577c5 100644 > > > --- a/drivers/i2c/busses/Kconfig > > > +++ b/drivers/i2c/busses/Kconfig > > > @@ -378,7 +378,7 @@ config I2C_BLACKFIN_TWI_CLK_KHZ > > > > > > config I2C_CADENCE > > > tristate "Cadence I2C Controller" > > > - depends on COMMON_CLK > > > + depends on ARCH_ZYNQ > > I'd suggest: > > > > depends on ARM && (ARCH_ZYNC || COMPILE_TEST) > > ARCH_ZYNC || (ARM && COMPILE_TEST) > > Same, but more clear. "more clear" might be subjective, at least for me I don't see a benefit from one over the other. hmm, considering a (hypothetical) driver that depends on I2C and is available on Zync but compiles on ARM, it would need either: ARM && I2C && (ZYNC || COMPILE_TEST) or ZYNC && I2C || (ARM && I2C && COMPILE_TEST) or I2C && (ARCH_ZYNC || (ARM && COMPILE_TEST)) In this case the first variant is the best, isn't it? Is this a reason to stick to this scheme even if there in the simpler case that is in question here? Probably not?! But anyhow, my main concern was the introduction of COMPILE_TEST, I don't care much about the actual expression used. So use whatever you want. Best regards Uwe
On 04/07/2014 08:56 AM, Wolfram Sang wrote: > >> I would suggest to add relaxed IO helper function to asm-generic/io.h >> to be available for all archs. Then we can avoid this type of errors. > > I favour this, too. For the cadence driver, I needed a quick solution > before linux-next breaks, though. Sure. That's understandable. Before my comment I have sent this to Russell too. http://www.spinics.net/lists/arm-kernel/msg320285.html Thanks, Michal
Mon, 7 Apr 2014 08:56:57 +0200 ?? Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > Hello, ... > > > On Sun, Apr 06, 2014 at 09:07:07PM +0200, Wolfram Sang wrote: > > > > During development, the driver first really needed to depend on > > > > COMMON_CLK only. Later, it was switched to writel_relaxed, but it was > > > > forgotten to update the dependencies, so build errors occured: > > > > > > > > config: make ARCH=i386 allyesconfig > > > > > > > > All error/warnings: > > > > > > > > drivers/i2c/busses/i2c-cadence.c: In function 'cdns_i2c_clear_bus_hold': > > > > >> drivers/i2c/busses/i2c-cadence.c:168:3: error: implicit declaration > > > > >> of function 'writel_relaxed' [-Werror=implicit-function-declaration] > > > > > > > > Use a very safe dependency for now. > > > > > > > > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > > > > --- > > > > drivers/i2c/busses/Kconfig | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > > > index 93165ff453ab..3d3b9b3577c5 100644 > > > > --- a/drivers/i2c/busses/Kconfig > > > > +++ b/drivers/i2c/busses/Kconfig > > > > @@ -378,7 +378,7 @@ config I2C_BLACKFIN_TWI_CLK_KHZ > > > > > > > > config I2C_CADENCE > > > > tristate "Cadence I2C Controller" > > > > - depends on COMMON_CLK > > > > + depends on ARCH_ZYNQ > > > I'd suggest: > > > > > > depends on ARM && (ARCH_ZYNC || COMPILE_TEST) > > > > ARCH_ZYNC || (ARM && COMPILE_TEST) > > > > Same, but more clear. > "more clear" might be subjective, at least for me I don't see a benefit > from one over the other. > > hmm, considering a (hypothetical) driver that depends on I2C and is > available on Zync but compiles on ARM, it would need either: No. Look at drivers/i2c/Kconfig: "source drivers/i2c/busses/Kconfig" is under "if I2C" already. ---
On Mon, 2014-04-07 at 10:37AM +0400, Alexander Shiyan wrote: > Mon, 7 Apr 2014 08:31:00 +0200 ?? Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > > Hello, > > > > On Sun, Apr 06, 2014 at 09:07:07PM +0200, Wolfram Sang wrote: > > > During development, the driver first really needed to depend on > > > COMMON_CLK only. Later, it was switched to writel_relaxed, but it was > > > forgotten to update the dependencies, so build errors occured: > > > > > > config: make ARCH=i386 allyesconfig > > > > > > All error/warnings: > > > > > > drivers/i2c/busses/i2c-cadence.c: In function 'cdns_i2c_clear_bus_hold': > > > >> drivers/i2c/busses/i2c-cadence.c:168:3: error: implicit declaration > > > >> of function 'writel_relaxed' [-Werror=implicit-function-declaration] > > > > > > Use a very safe dependency for now. > > > > > > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > > > --- > > > drivers/i2c/busses/Kconfig | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > > index 93165ff453ab..3d3b9b3577c5 100644 > > > --- a/drivers/i2c/busses/Kconfig > > > +++ b/drivers/i2c/busses/Kconfig > > > @@ -378,7 +378,7 @@ config I2C_BLACKFIN_TWI_CLK_KHZ > > > > > > config I2C_CADENCE > > > tristate "Cadence I2C Controller" > > > - depends on COMMON_CLK > > > + depends on ARCH_ZYNQ > > I'd suggest: > > > > depends on ARM && (ARCH_ZYNC || COMPILE_TEST) > > ARCH_ZYNC || (ARM && COMPILE_TEST) Why would you add Zynq to the dependencies? This driver should work on all ARM platforms (more or less, given that the IP is implemented similarly). I think ARM, if it selects COMMON_CLK, is enough. Otherwise ARM && COMMON_CLK. I don't see a good reason to restrict the driver to Zynq only. Sören
On Mon, 2014-04-07 at 08:39AM +0200, Michal Simek wrote: > On 04/07/2014 08:31 AM, Uwe Kleine-König wrote: > > Hello, > > > > On Sun, Apr 06, 2014 at 09:07:07PM +0200, Wolfram Sang wrote: > >> During development, the driver first really needed to depend on > >> COMMON_CLK only. Later, it was switched to writel_relaxed, but it was > >> forgotten to update the dependencies, so build errors occured: > >> > >> config: make ARCH=i386 allyesconfig > >> > >> All error/warnings: > >> > >> drivers/i2c/busses/i2c-cadence.c: In function 'cdns_i2c_clear_bus_hold': > >>>> drivers/i2c/busses/i2c-cadence.c:168:3: error: implicit declaration > >>>> of function 'writel_relaxed' [-Werror=implicit-function-declaration] > >> > >> Use a very safe dependency for now. > >> > >> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > >> --- > >> drivers/i2c/busses/Kconfig | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > >> index 93165ff453ab..3d3b9b3577c5 100644 > >> --- a/drivers/i2c/busses/Kconfig > >> +++ b/drivers/i2c/busses/Kconfig > >> @@ -378,7 +378,7 @@ config I2C_BLACKFIN_TWI_CLK_KHZ > >> > >> config I2C_CADENCE > >> tristate "Cadence I2C Controller" > >> - depends on COMMON_CLK > >> + depends on ARCH_ZYNQ > > I'd suggest: > > > > depends on ARM && (ARCH_ZYNC || COMPILE_TEST) > > > > This way it's available for compile testing but still it doesn't appear > > when configuring a kernel for a real machine. > > I would suggest to add relaxed IO helper function to asm-generic/io.h > to be available for all archs. Then we can avoid this type of errors. ACK Sören
On Mon, 2014-04-07 at 08:31AM +0200, Uwe Kleine-König wrote: > Hello, > > On Sun, Apr 06, 2014 at 09:07:07PM +0200, Wolfram Sang wrote: > > During development, the driver first really needed to depend on > > COMMON_CLK only. Later, it was switched to writel_relaxed, but it was > > forgotten to update the dependencies, so build errors occured: > > > > config: make ARCH=i386 allyesconfig > > > > All error/warnings: > > > > drivers/i2c/busses/i2c-cadence.c: In function 'cdns_i2c_clear_bus_hold': > > >> drivers/i2c/busses/i2c-cadence.c:168:3: error: implicit declaration > > >> of function 'writel_relaxed' [-Werror=implicit-function-declaration] > > > > Use a very safe dependency for now. > > > > Signed-off-by: Wolfram Sang <wsa@the-dreams.de> > > --- > > drivers/i2c/busses/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > index 93165ff453ab..3d3b9b3577c5 100644 > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -378,7 +378,7 @@ config I2C_BLACKFIN_TWI_CLK_KHZ > > > > config I2C_CADENCE > > tristate "Cadence I2C Controller" > > - depends on COMMON_CLK > > + depends on ARCH_ZYNQ > I'd suggest: > > depends on ARM && (ARCH_ZYNC || COMPILE_TEST) This misses COMMON_CLK in the COMPILE_TEST case. Sören
Hello Sören, On Mon, Apr 07, 2014 at 08:36:29AM -0700, Sören Brinkmann wrote: > On Mon, 2014-04-07 at 10:37AM +0400, Alexander Shiyan wrote: > > ARCH_ZYNC || (ARM && COMPILE_TEST) > > Why would you add Zynq to the dependencies? This driver should work on > all ARM platforms (more or less, given that the IP is implemented > similarly). I think ARM, if it selects COMMON_CLK, is enough. Otherwise > ARM && COMMON_CLK. I don't see a good reason to restrict the driver to > Zynq only. The idea is that if COMPILE_TEST is off you can only enable the driver for machines where the device in question really exists to simplify kernel configuration. Sometimes this isn't really possible, e.g. for i2c devices that can be put on more or less every machine. I had the impression that the cadence i2c bus device only exists on Zync? Ok, you could implement this device on every SoC that has some programmable hardware, but that is not the point. Best regards Uwe
On 04/07/2014 08:56 AM, Wolfram Sang wrote: > >> I would suggest to add relaxed IO helper function to asm-generic/io.h >> to be available for all archs. Then we can avoid this type of errors. > > I favour this, too. For the cadence driver, I needed a quick solution > before linux-next breaks, though. > FYI: Here is the patch http://www.spinics.net/lists/kernel/msg1718606.html Thanks, Michal
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 93165ff453ab..3d3b9b3577c5 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -378,7 +378,7 @@ config I2C_BLACKFIN_TWI_CLK_KHZ config I2C_CADENCE tristate "Cadence I2C Controller" - depends on COMMON_CLK + depends on ARCH_ZYNQ help Say yes here to select Cadence I2C Host Controller. This controller is e.g. used by Xilinx Zynq.