| Message ID | 20181102110653.118257-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | ACPI / PMIC: xpower: fix IOSF_MBI dependency | expand |
Hi, On 02-11-18 12:06, Arnd Bergmann wrote: > We still get a link failure with IOSF_MBI=m when the xpower driver > is built-in: > > drivers/acpi/pmic/intel_pmic_xpower.o: In function `intel_xpower_pmic_update_power': > intel_pmic_xpower.c:(.text+0x4f2): undefined reference to `iosf_mbi_block_punit_i2c_access' > intel_pmic_xpower.c:(.text+0x5e2): undefined reference to `iosf_mbi_unblock_punit_i2c_access' > > This makes the dependency stronger, so we can only build when IOSF_MBI > is built-in. > > Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to Kconfig entry") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Hmm, it is probably better to make IOSF_MBI a bool, it is selected by: X86_INTEL_QUARK and X86_INTEL_LPSS which are both bools themselves. Arguably it should also be hidden and only enabled through these selects. Does someone from Intel have an opinion on making it hidden? Regards, Hans > --- > drivers/acpi/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index 18851e7eedd5..31a3c4a03f61 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -514,7 +514,7 @@ config CRC_PMIC_OPREGION > > config XPOWER_PMIC_OPREGION > bool "ACPI operation region support for XPower AXP288 PMIC" > - depends on MFD_AXP20X_I2C && IOSF_MBI > + depends on MFD_AXP20X_I2C && IOSF_MBI=y > help > This config adds ACPI operation region support for XPower AXP288 PMIC. > >
On Fri, Nov 02, 2018 at 12:09:34PM +0100, Hans de Goede wrote: > Hi, > > On 02-11-18 12:06, Arnd Bergmann wrote: > > We still get a link failure with IOSF_MBI=m when the xpower driver > > is built-in: > > > > drivers/acpi/pmic/intel_pmic_xpower.o: In function `intel_xpower_pmic_update_power': > > intel_pmic_xpower.c:(.text+0x4f2): undefined reference to `iosf_mbi_block_punit_i2c_access' > > intel_pmic_xpower.c:(.text+0x5e2): undefined reference to `iosf_mbi_unblock_punit_i2c_access' > > > > This makes the dependency stronger, so we can only build when IOSF_MBI > > is built-in. > > > > Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to Kconfig entry") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Hmm, it is probably better to make IOSF_MBI a bool, it is selected by: > X86_INTEL_QUARK and X86_INTEL_LPSS which are both bools themselves. > > Arguably it should also be hidden and only enabled through these selects. > Does someone from Intel have an opinion on making it hidden? We have number of cases where tristate modules depend on or select it. So, I have not seen a good argument to make it boolean.
On Fri, Nov 02, 2018 at 12:06:43PM +0100, Arnd Bergmann wrote: > We still get a link failure with IOSF_MBI=m when the xpower driver > is built-in: > > drivers/acpi/pmic/intel_pmic_xpower.o: In function `intel_xpower_pmic_update_power': > intel_pmic_xpower.c:(.text+0x4f2): undefined reference to `iosf_mbi_block_punit_i2c_access' > intel_pmic_xpower.c:(.text+0x5e2): undefined reference to `iosf_mbi_unblock_punit_i2c_access' > > This makes the dependency stronger, so we can only build when IOSF_MBI > is built-in. > > Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to Kconfig entry") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/acpi/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index 18851e7eedd5..31a3c4a03f61 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -514,7 +514,7 @@ config CRC_PMIC_OPREGION > > config XPOWER_PMIC_OPREGION > bool "ACPI operation region support for XPower AXP288 PMIC" > - depends on MFD_AXP20X_I2C && IOSF_MBI > + depends on MFD_AXP20X_I2C && IOSF_MBI=y To me sounds like select IOSF_MBI would be more appropriate here. > help > This config adds ACPI operation region support for XPower AXP288 PMIC.
On 11/2/18, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Nov 02, 2018 at 12:06:43PM +0100, Arnd Bergmann wrote: >> We still get a link failure with IOSF_MBI=m when the xpower driver >> is built-in: >> >> drivers/acpi/pmic/intel_pmic_xpower.o: In function >> `intel_xpower_pmic_update_power': >> intel_pmic_xpower.c:(.text+0x4f2): undefined reference to >> `iosf_mbi_block_punit_i2c_access' >> intel_pmic_xpower.c:(.text+0x5e2): undefined reference to >> `iosf_mbi_unblock_punit_i2c_access' >> >> This makes the dependency stronger, so we can only build when IOSF_MBI >> is built-in. >> >> Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to >> Kconfig entry") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> drivers/acpi/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index 18851e7eedd5..31a3c4a03f61 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -514,7 +514,7 @@ config CRC_PMIC_OPREGION >> >> config XPOWER_PMIC_OPREGION >> bool "ACPI operation region support for XPower AXP288 PMIC" >> - depends on MFD_AXP20X_I2C && IOSF_MBI >> + depends on MFD_AXP20X_I2C && IOSF_MBI=y > > To me sounds like > > select IOSF_MBI would be more appropriate here. It looks like we have a mix of the two two, with most drivers using 'select' and only a few ones using 'depends on'. Mixing the two often leads to trouble, especially for user-visible symbols. Making it a hidden symbol that is always selected is probably fine, but then every driver selecting it must also use 'depends on X86 && PCI'. Arnd
On 11/2/18, Arnd Bergmann <arnd@arndb.de> wrote: > On 11/2/18, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> On Fri, Nov 02, 2018 at 12:06:43PM +0100, Arnd Bergmann wrote: >>> We still get a link failure with IOSF_MBI=m when the xpower driver >>> is built-in: >>> >>> drivers/acpi/pmic/intel_pmic_xpower.o: In function >>> `intel_xpower_pmic_update_power': >>> intel_pmic_xpower.c:(.text+0x4f2): undefined reference to >>> `iosf_mbi_block_punit_i2c_access' >>> intel_pmic_xpower.c:(.text+0x5e2): undefined reference to >>> `iosf_mbi_unblock_punit_i2c_access' >>> >>> This makes the dependency stronger, so we can only build when IOSF_MBI >>> is built-in. >>> >>> Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to >>> Kconfig entry") >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> --- >>> drivers/acpi/Kconfig | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >>> index 18851e7eedd5..31a3c4a03f61 100644 >>> --- a/drivers/acpi/Kconfig >>> +++ b/drivers/acpi/Kconfig >>> @@ -514,7 +514,7 @@ config CRC_PMIC_OPREGION >>> >>> config XPOWER_PMIC_OPREGION >>> bool "ACPI operation region support for XPower AXP288 PMIC" >>> - depends on MFD_AXP20X_I2C && IOSF_MBI >>> + depends on MFD_AXP20X_I2C && IOSF_MBI=y >> >> To me sounds like >> >> select IOSF_MBI would be more appropriate here. > > It looks like we have a mix of the two two, with most drivers > using 'select' and only a few ones using 'depends on'. Mixing > the two often leads to trouble, especially for user-visible > symbols. > > Making it a hidden symbol that is always selected is probably > fine, but then every driver selecting it must also use 'depends > on X86 && PCI'. Oh, and that also requires removing the #else section in arch/x86/include/asm/iosf_mbi.h, otherwise you might have a driver that can build with or without CONFIG_IOSF_MBI, but fails to be built-in when some other tristate symbol uses 'select IOSF_MBI'. Making it a 'bool' seems easier there. Arnd
On Fri, Nov 02, 2018 at 11:07:34PM +0100, Arnd Bergmann wrote: > On 11/2/18, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Nov 02, 2018 at 12:06:43PM +0100, Arnd Bergmann wrote: > >> - depends on MFD_AXP20X_I2C && IOSF_MBI > >> + depends on MFD_AXP20X_I2C && IOSF_MBI=y > > > > To me sounds like > > > > select IOSF_MBI would be more appropriate here. > > It looks like we have a mix of the two two, with most drivers > using 'select' and only a few ones using 'depends on'. Mixing > the two often leads to trouble, especially for user-visible > symbols. > > Making it a hidden symbol that is always selected is probably > fine, but then every driver selecting it must also use 'depends > on X86 && PCI'. I doubt every is a correct word here. Whenever driver uses IOSF_MBI it implies X86 and PCI (or should have those dependencies in mind already). But I agree that hide it and select as a library would make more sense than the current state of affairs.
On 11/5/18, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Nov 02, 2018 at 11:07:34PM +0100, Arnd Bergmann wrote: >> On 11/2/18, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> > On Fri, Nov 02, 2018 at 12:06:43PM +0100, Arnd Bergmann wrote: > >> >> - depends on MFD_AXP20X_I2C && IOSF_MBI >> >> + depends on MFD_AXP20X_I2C && IOSF_MBI=y >> > >> > To me sounds like >> > >> > select IOSF_MBI would be more appropriate here. >> >> It looks like we have a mix of the two two, with most drivers >> using 'select' and only a few ones using 'depends on'. Mixing >> the two often leads to trouble, especially for user-visible >> symbols. >> >> Making it a hidden symbol that is always selected is probably >> fine, but then every driver selecting it must also use 'depends >> on X86 && PCI'. > > I doubt every is a correct word here. Whenever driver uses IOSF_MBI it > implies X86 and PCI (or should have those dependencies in mind already). I mean it must depend on those two in some form. If a driver uses 'depends on IOSF_MBI' today, that is implied through that dependency. Changing it to 'select' means we have to add that dependency, like diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 56ccb1ea7da5..fb750a8a9b77 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -489,6 +489,7 @@ config I2C_DESIGNWARE_PLATFORM tristate "Synopsys DesignWare Platform" select I2C_DESIGNWARE_CORE depends on (ACPI && COMMON_CLK) || !ACPI + select IOSF_MBI if I2C_DESIGNWARE_BAYTRAIL help If you say yes to this option, support will be included for the Synopsys DesignWare I2C adapter. @@ -520,9 +521,8 @@ config I2C_DESIGNWARE_PCI config I2C_DESIGNWARE_BAYTRAIL bool "Intel Baytrail I2C semaphore support" - depends on ACPI - depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \ - (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y) + depends on ACPI && X86 && PCI + depends on I2C_DESIGNWARE_PLATFORM help This driver enables managed host access to the PMIC I2C bus on select Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows For anything that already has the dependency, nothing changes. Arnd
On Fri, Nov 2, 2018 at 12:07 PM Arnd Bergmann <arnd@arndb.de> wrote: > > We still get a link failure with IOSF_MBI=m when the xpower driver > is built-in: > > drivers/acpi/pmic/intel_pmic_xpower.o: In function `intel_xpower_pmic_update_power': > intel_pmic_xpower.c:(.text+0x4f2): undefined reference to `iosf_mbi_block_punit_i2c_access' > intel_pmic_xpower.c:(.text+0x5e2): undefined reference to `iosf_mbi_unblock_punit_i2c_access' > > This makes the dependency stronger, so we can only build when IOSF_MBI > is built-in. > > Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to Kconfig entry") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/acpi/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index 18851e7eedd5..31a3c4a03f61 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -514,7 +514,7 @@ config CRC_PMIC_OPREGION > > config XPOWER_PMIC_OPREGION > bool "ACPI operation region support for XPower AXP288 PMIC" > - depends on MFD_AXP20X_I2C && IOSF_MBI > + depends on MFD_AXP20X_I2C && IOSF_MBI=y > help > This config adds ACPI operation region support for XPower AXP288 PMIC. > > -- At this point I'm inclined to apply the patch as is as a short-term fix and improvements can be made on top of it. Any objections?
Hi, On 07-11-18 13:22, Rafael J. Wysocki wrote: > On Fri, Nov 2, 2018 at 12:07 PM Arnd Bergmann <arnd@arndb.de> wrote: >> >> We still get a link failure with IOSF_MBI=m when the xpower driver >> is built-in: >> >> drivers/acpi/pmic/intel_pmic_xpower.o: In function `intel_xpower_pmic_update_power': >> intel_pmic_xpower.c:(.text+0x4f2): undefined reference to `iosf_mbi_block_punit_i2c_access' >> intel_pmic_xpower.c:(.text+0x5e2): undefined reference to `iosf_mbi_unblock_punit_i2c_access' >> >> This makes the dependency stronger, so we can only build when IOSF_MBI >> is built-in. >> >> Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to Kconfig entry") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> drivers/acpi/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index 18851e7eedd5..31a3c4a03f61 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -514,7 +514,7 @@ config CRC_PMIC_OPREGION >> >> config XPOWER_PMIC_OPREGION >> bool "ACPI operation region support for XPower AXP288 PMIC" >> - depends on MFD_AXP20X_I2C && IOSF_MBI >> + depends on MFD_AXP20X_I2C && IOSF_MBI=y >> help >> This config adds ACPI operation region support for XPower AXP288 PMIC. >> >> -- > > At this point I'm inclined to apply the patch as is as a short-term > fix and improvements can be made on top of it. > > Any objections? Not from me, go for it :) Regards, Hans
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 18851e7eedd5..31a3c4a03f61 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -514,7 +514,7 @@ config CRC_PMIC_OPREGION config XPOWER_PMIC_OPREGION bool "ACPI operation region support for XPower AXP288 PMIC" - depends on MFD_AXP20X_I2C && IOSF_MBI + depends on MFD_AXP20X_I2C && IOSF_MBI=y help This config adds ACPI operation region support for XPower AXP288 PMIC.
We still get a link failure with IOSF_MBI=m when the xpower driver is built-in: drivers/acpi/pmic/intel_pmic_xpower.o: In function `intel_xpower_pmic_update_power': intel_pmic_xpower.c:(.text+0x4f2): undefined reference to `iosf_mbi_block_punit_i2c_access' intel_pmic_xpower.c:(.text+0x5e2): undefined reference to `iosf_mbi_unblock_punit_i2c_access' This makes the dependency stronger, so we can only build when IOSF_MBI is built-in. Fixes: 6a9b593d4b6f ("ACPI / PMIC: xpower: Add depends on IOSF_MBI to Kconfig entry") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/acpi/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)