| Message ID | 20210721151330.2176653-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: accel: fxls8962af: fix i2c dependency | expand |
On Wed, Jul 21, 2021 at 6:13 PM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > With CONFIG_SPI=y and CONFIG_I2C=m, building fxls8962af into vmlinux > causes a link error against the I2C module: > > aarch64-linux-ld: drivers/iio/accel/fxls8962af-core.o: in function `fxls8962af_fifo_flush': > fxls8962af-core.c:(.text+0x3a0): undefined reference to `i2c_verify_client' > > Work around it by adding a Kconfig dependency that forces the SPI driver > to be a loadable module whenever I2C is a module. ... > config FXLS8962AF > tristate > + depends on I2C || !I2C # cannot be built-in for modular I2C Can you enlighten me how this will not be a no-op?
On Wed, Jul 21, 2021 at 5:52 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Jul 21, 2021 at 6:13 PM Arnd Bergmann <arnd@kernel.org> wrote: > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > With CONFIG_SPI=y and CONFIG_I2C=m, building fxls8962af into vmlinux > > causes a link error against the I2C module: > > > > aarch64-linux-ld: drivers/iio/accel/fxls8962af-core.o: in function `fxls8962af_fifo_flush': > > fxls8962af-core.c:(.text+0x3a0): undefined reference to `i2c_verify_client' > > > > Work around it by adding a Kconfig dependency that forces the SPI driver > > to be a loadable module whenever I2C is a module. > > ... > > > config FXLS8962AF > > tristate > > + depends on I2C || !I2C # cannot be built-in for modular I2C > > Can you enlighten me how this will not be a no-op? This part does nothing, it only causes a warning when FXLS8962AF gets selected =y when I2C=m. The important bit is the other hunk that adds the same dependency to the FXLS8962AF_SPI symbol, which enforces that either I2C is completely disabled, or treated as a dependency that prevents the user from setting FXLS8962AF_SPI=y when that would cause a link failure. The effect is similar to a 'depends on SND_SOC_I2C_AND_SPI', except we only need it on the SPI symbol here because the SPI core cannot be in a module itself. Arnd
On Wed, Jul 21, 2021 at 7:12 PM Arnd Bergmann <arnd@kernel.org> wrote: > On Wed, Jul 21, 2021 at 5:52 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Jul 21, 2021 at 6:13 PM Arnd Bergmann <arnd@kernel.org> wrote: ... > > > config FXLS8962AF > > > tristate > > > + depends on I2C || !I2C # cannot be built-in for modular I2C > > > > Can you enlighten me how this will not be a no-op? > > This part does nothing, it only causes a warning when FXLS8962AF > gets selected =y when I2C=m. This is something new to me. But shouldn't the other chunk guarantee that warning won't happen? > The important bit is the other hunk that adds the same dependency > to the FXLS8962AF_SPI symbol, which enforces that either I2C > is completely disabled, or treated as a dependency that prevents > the user from setting FXLS8962AF_SPI=y when that would cause > a link failure. This part I understand and neither object to nor comment on. > The effect is similar to a 'depends on SND_SOC_I2C_AND_SPI', > except we only need it on the SPI symbol here because the SPI > core cannot be in a module itself. I see. Thanks for elaboration.
On Wed, Jul 21, 2021 at 7:34 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Jul 21, 2021 at 7:12 PM Arnd Bergmann <arnd@kernel.org> wrote: > > On Wed, Jul 21, 2021 at 5:52 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Wed, Jul 21, 2021 at 6:13 PM Arnd Bergmann <arnd@kernel.org> wrote: > > ... > > > > > config FXLS8962AF > > > > tristate > > > > + depends on I2C || !I2C # cannot be built-in for modular I2C > > > > > > Can you enlighten me how this will not be a no-op? > > > > This part does nothing, it only causes a warning when FXLS8962AF > > gets selected =y when I2C=m. > > This is something new to me. But shouldn't the other chunk guarantee > that warning won't happen? Correct, it works without that, but if that fails after something changes, this version would provide better diagnostics than the FXLS8962AF core driver causing a link failure, and I found it documents better why the other driver needs the dependency. Let me know if you prefer me to resend the patch without this hunk. Arnd
On Wed, 21 Jul 2021 20:40:30 +0200 Arnd Bergmann <arnd@kernel.org> wrote: > On Wed, Jul 21, 2021 at 7:34 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Jul 21, 2021 at 7:12 PM Arnd Bergmann <arnd@kernel.org> wrote: > > > On Wed, Jul 21, 2021 at 5:52 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Wed, Jul 21, 2021 at 6:13 PM Arnd Bergmann <arnd@kernel.org> wrote: > > > > ... > > > > > > > config FXLS8962AF > > > > > tristate > > > > > + depends on I2C || !I2C # cannot be built-in for modular I2C > > > > > > > > Can you enlighten me how this will not be a no-op? > > > > > > This part does nothing, it only causes a warning when FXLS8962AF > > > gets selected =y when I2C=m. > > > > This is something new to me. But shouldn't the other chunk guarantee > > that warning won't happen? > > Correct, it works without that, but if that fails after something changes, > this version would provide better diagnostics than the FXLS8962AF > core driver causing a link failure, and I found it documents better > why the other driver needs the dependency. > > Let me know if you prefer me to resend the patch without this hunk. > > Arnd Hi Arnd, I didn't think of this particularly combination when we dealt with last build issue the workaround brought in. I've applied this to the fixes-togreg branch of iio.git as an immediately solution, but longer term we should think about just using a function pointer to allow us to move this into the i2c specific module. If we do that we can drop this complex build logic later. Thanks, Jonathan
On Sat, Jul 24, 2021 at 5:15 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Wed, 21 Jul 2021 20:40:30 +0200 Arnd Bergmann <arnd@kernel.org> wrote: > I didn't think of this particularly combination when we dealt with > last build issue the workaround brought in. I've applied this to the > fixes-togreg branch of iio.git as an immediately solution, but longer > term we should think about just using a function pointer to allow us > to move this into the i2c specific module. If we do that we can > drop this complex build logic later. Ok, that sounds good to me, thanks! Arnd
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index 0e56ace61103..8d8b1ba42ff8 100644 --- a/drivers/iio/accel/Kconfig +++ b/drivers/iio/accel/Kconfig @@ -231,6 +231,7 @@ config DMARD10 config FXLS8962AF tristate + depends on I2C || !I2C # cannot be built-in for modular I2C config FXLS8962AF_I2C tristate "NXP FXLS8962AF/FXLS8964AF Accelerometer I2C Driver" @@ -247,6 +248,7 @@ config FXLS8962AF_I2C config FXLS8962AF_SPI tristate "NXP FXLS8962AF/FXLS8964AF Accelerometer SPI Driver" depends on SPI + depends on I2C || !I2C select FXLS8962AF select REGMAP_SPI help