[PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[Arnd Bergmann]

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.
 
-- 
2.18.0

Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[Hans de Goede]

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.
>   
> 

Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[Arnd Bergmann]

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

Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[Arnd Bergmann]

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

Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[Arnd Bergmann]

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

Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[Rafael J. Wysocki]

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?

Re: [PATCH] ACPI / PMIC: xpower: fix IOSF_MBI dependency

[Hans de Goede]

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