[PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Bin Meng]

The Intel SPI-NOR driver is dependent on LPC_ICH to get the platform
data. Select it in the Kconfig.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi-nor/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index bfdfb1e..e998800 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -93,6 +93,7 @@ config SPI_INTEL_SPI_PLATFORM
 	tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
 	depends on X86
 	select SPI_INTEL_SPI
+	select LPC_ICH
 	help
 	  This enables platform support for the Intel PCH/PCU SPI
 	  controller in master mode. This controller is present in modern
-- 
2.9.2

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Mika Westerberg]

On Fri, Aug 25, 2017 at 01:12:51AM -0700, Bin Meng wrote:
> The Intel SPI-NOR driver is dependent on LPC_ICH to get the platform
> data. Select it in the Kconfig.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  drivers/mtd/spi-nor/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index bfdfb1e..e998800 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -93,6 +93,7 @@ config SPI_INTEL_SPI_PLATFORM
>  	tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
>  	depends on X86
>  	select SPI_INTEL_SPI
> +	select LPC_ICH

How about 

	depends on X86 && LPC_ICH

instead?

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Bin Meng]

Hi Mika,

On Fri, Aug 25, 2017 at 6:40 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Aug 25, 2017 at 01:12:51AM -0700, Bin Meng wrote:
>> The Intel SPI-NOR driver is dependent on LPC_ICH to get the platform
>> data. Select it in the Kconfig.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  drivers/mtd/spi-nor/Kconfig | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index bfdfb1e..e998800 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -93,6 +93,7 @@ config SPI_INTEL_SPI_PLATFORM
>>       tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
>>       depends on X86
>>       select SPI_INTEL_SPI
>> +     select LPC_ICH
>
> How about
>
>         depends on X86 && LPC_ICH
>
> instead?

Other LPC_ICH users (like gpio, watchdog) use the 'select' logic, so I
would prefer to keep things consistent.

Regards,
Bin

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Stefan Roese]

On 25.08.2017 12:40, Mika Westerberg wrote:
> On Fri, Aug 25, 2017 at 01:12:51AM -0700, Bin Meng wrote:
>> The Intel SPI-NOR driver is dependent on LPC_ICH to get the platform
>> data. Select it in the Kconfig.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>   drivers/mtd/spi-nor/Kconfig | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index bfdfb1e..e998800 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -93,6 +93,7 @@ config SPI_INTEL_SPI_PLATFORM
>>   	tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
>>   	depends on X86
>>   	select SPI_INTEL_SPI
>> +	select LPC_ICH
> 
> How about
> 
> 	depends on X86 && LPC_ICH
> 
> instead?

I prefer Bin's version, as with your patch, the MTD SPI driver will
not be "seen" / selectable, as long as the LPC_ICH support is not
enabled. I've been hunting such dependencies myself a few times and
find them unnecessary burden.

Thanks,
Stefan

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Mika Westerberg]

On Fri, Aug 25, 2017 at 08:11:54PM +0800, Bin Meng wrote:
> Other LPC_ICH users (like gpio, watchdog) use the 'select' logic, so I
> would prefer to keep things consistent.

Fair enough.

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Cyrille Pitchen]

Le 25/08/2017 à 10:12, Bin Meng a écrit :
> The Intel SPI-NOR driver is dependent on LPC_ICH to get the platform
> data. Select it in the Kconfig.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Applied to the spi-nor/next branch of l2-mtd

Thanks!
> ---
> 
>  drivers/mtd/spi-nor/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index bfdfb1e..e998800 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -93,6 +93,7 @@ config SPI_INTEL_SPI_PLATFORM
>  	tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
>  	depends on X86
>  	select SPI_INTEL_SPI
> +	select LPC_ICH
>  	help
>  	  This enables platform support for the Intel PCH/PCU SPI
>  	  controller in master mode. This controller is present in modern
> 

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Arnd Bergmann]

On Wed, Oct 11, 2017 at 10:03 AM, Cyrille Pitchen
<cyrille.pitchen@wedev4u.fr> wrote:
> Le 25/08/2017 à 10:12, Bin Meng a écrit :
>> The Intel SPI-NOR driver is dependent on LPC_ICH to get the platform
>> data. Select it in the Kconfig.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> Applied to the spi-nor/next branch of l2-mtd

This causes a build error now:

warning: (SPI_INTEL_SPI_PLATFORM && ITCO_WDT) selects LPC_ICH which
has unmet direct dependencies (HAS_IOMEM && PCI)
drivers/mfd/lpc_ich.c: In function 'lpc_ich_init_spi':
drivers/mfd/lpc_ich.c:1137:3: error: implicit declaration of function
'pci_bus_write_config_byte'; did you mean 'pci_write_config_byte'?
[-Werror=implicit-function-declaration]

Generally speaking, using 'select' to force-enable another
driver is a bad idea and causes endless problems, but if you
insist on doing that, please make sure you get the right
dependencies.

Also, the 'depends on EXPERT' statement looks misplaced,
enabling EXPERT should only be there to allow you to turn
extra things *off*, not to hide device drivers.

How about this:

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 19bcb63a1ce7..b81d9b4dae7b 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -90,7 +90,7 @@ config SPI_INTEL_SPI
        tristate

 config SPI_INTEL_SPI_PCI
-       tristate "Intel PCH/PCU SPI flash PCI driver" if EXPERT
+       tristate "Intel PCH/PCU SPI flash PCI driver"
        depends on X86 && PCI
        select SPI_INTEL_SPI
        help
@@ -106,10 +106,10 @@ config SPI_INTEL_SPI_PCI
          will be called intel-spi-pci.

 config SPI_INTEL_SPI_PLATFORM
-       tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
-       depends on X86
+       tristate "Intel PCH/PCU SPI flash platform driver"
+       depends on X86 && (PCI || COMPILE_TEST)
        select SPI_INTEL_SPI
-       select LPC_ICH
+       select LPC_ICH if PCI
        help
          This enables platform support for the Intel PCH/PCU SPI
          controller in master mode. This controller is present in modern

       Arnd

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Cyrille Pitchen]

Hi Arnd,

Le 13/10/2017 à 13:15, Arnd Bergmann a écrit :
> On Wed, Oct 11, 2017 at 10:03 AM, Cyrille Pitchen
> <cyrille.pitchen@wedev4u.fr> wrote:
>> Le 25/08/2017 à 10:12, Bin Meng a écrit :
>>> The Intel SPI-NOR driver is dependent on LPC_ICH to get the platform
>>> data. Select it in the Kconfig.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>
>> Applied to the spi-nor/next branch of l2-mtd
> 
> This causes a build error now:
> 
> warning: (SPI_INTEL_SPI_PLATFORM && ITCO_WDT) selects LPC_ICH which
> has unmet direct dependencies (HAS_IOMEM && PCI)
> drivers/mfd/lpc_ich.c: In function 'lpc_ich_init_spi':
> drivers/mfd/lpc_ich.c:1137:3: error: implicit declaration of function
> 'pci_bus_write_config_byte'; did you mean 'pci_write_config_byte'?
> [-Werror=implicit-function-declaration]
> 
> Generally speaking, using 'select' to force-enable another
> driver is a bad idea and causes endless problems, but if you
> insist on doing that, please make sure you get the right
> dependencies.
> 
> Also, the 'depends on EXPERT' statement looks misplaced,
> enabling EXPERT should only be there to allow you to turn
> extra things *off*, not to hide device drivers.
> 
> How about this:
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 19bcb63a1ce7..b81d9b4dae7b 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -90,7 +90,7 @@ config SPI_INTEL_SPI
>         tristate
> 
>  config SPI_INTEL_SPI_PCI
> -       tristate "Intel PCH/PCU SPI flash PCI driver" if EXPERT
> +       tristate "Intel PCH/PCU SPI flash PCI driver"
>         depends on X86 && PCI
>         select SPI_INTEL_SPI
>         help
> @@ -106,10 +106,10 @@ config SPI_INTEL_SPI_PCI
>           will be called intel-spi-pci.
> 
>  config SPI_INTEL_SPI_PLATFORM
> -       tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
> -       depends on X86
> +       tristate "Intel PCH/PCU SPI flash platform driver"
> +       depends on X86 && (PCI || COMPILE_TEST)
>         select SPI_INTEL_SPI
> -       select LPC_ICH
> +       select LPC_ICH if PCI
>         help
>           This enables platform support for the Intel PCH/PCU SPI
>           controller in master mode. This controller is present in modern
> 
>        Arnd
> 

The patch has been removed from spi-nor/next and updated to 'Rejected'
in patchwork.

Thanks for your report.

Bin, you may have to submit another patch with the correct dependencies
or maybe adopt the "depends LPC_ICH" approach as suggested by Mika.

Best regards,

Cyrille

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Bin Meng]

Hi Arnd,

On Fri, Oct 13, 2017 at 7:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Oct 11, 2017 at 10:03 AM, Cyrille Pitchen
> <cyrille.pitchen@wedev4u.fr> wrote:
>> Le 25/08/2017 à 10:12, Bin Meng a écrit :
>>> The Intel SPI-NOR driver is dependent on LPC_ICH to get the platform
>>> data. Select it in the Kconfig.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>
>> Applied to the spi-nor/next branch of l2-mtd
>
> This causes a build error now:
>
> warning: (SPI_INTEL_SPI_PLATFORM && ITCO_WDT) selects LPC_ICH which
> has unmet direct dependencies (HAS_IOMEM && PCI)
> drivers/mfd/lpc_ich.c: In function 'lpc_ich_init_spi':
> drivers/mfd/lpc_ich.c:1137:3: error: implicit declaration of function
> 'pci_bus_write_config_byte'; did you mean 'pci_write_config_byte'?
> [-Werror=implicit-function-declaration]
>

Thanks for your testing. However, I believe this build error cannot be
seen in a real system due to x86 always has PCI on. But I see you were
doing build testing with COMPILE_TEST.

> Generally speaking, using 'select' to force-enable another
> driver is a bad idea and causes endless problems, but if you
> insist on doing that, please make sure you get the right
> dependencies.
>

Sure.

> Also, the 'depends on EXPERT' statement looks misplaced,
> enabling EXPERT should only be there to allow you to turn
> extra things *off*, not to hide device drivers.
>

I will leave this to Mika to comment the "EXPERT" usage. If we remove
this, I think that should be another patch and the documentation of
this driver should be updated too.

> How about this:
>
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 19bcb63a1ce7..b81d9b4dae7b 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -90,7 +90,7 @@ config SPI_INTEL_SPI
>         tristate
>
>  config SPI_INTEL_SPI_PCI
> -       tristate "Intel PCH/PCU SPI flash PCI driver" if EXPERT
> +       tristate "Intel PCH/PCU SPI flash PCI driver"
>         depends on X86 && PCI
>         select SPI_INTEL_SPI
>         help
> @@ -106,10 +106,10 @@ config SPI_INTEL_SPI_PCI
>           will be called intel-spi-pci.
>
>  config SPI_INTEL_SPI_PLATFORM
> -       tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
> -       depends on X86
> +       tristate "Intel PCH/PCU SPI flash platform driver"
> +       depends on X86 && (PCI || COMPILE_TEST)
>         select SPI_INTEL_SPI
> -       select LPC_ICH
> +       select LPC_ICH if PCI

I think we just need do:

depends on X86 && PCI

then

select LPC_ICH

>         help
>           This enables platform support for the Intel PCH/PCU SPI
>           controller in master mode. This controller is present in modern
>

Regards,
Bin

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Arnd Bergmann]

On Sun, Oct 15, 2017 at 3:38 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> On Fri, Oct 13, 2017 at 7:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:>> On Wed, Oct 11, 2017 at 10:03 AM, Cyrille Pitchen
>> warning: (SPI_INTEL_SPI_PLATFORM && ITCO_WDT) selects LPC_ICH which
>> has unmet direct dependencies (HAS_IOMEM && PCI)
>> drivers/mfd/lpc_ich.c: In function 'lpc_ich_init_spi':
>> drivers/mfd/lpc_ich.c:1137:3: error: implicit declaration of function
>> 'pci_bus_write_config_byte'; did you mean 'pci_write_config_byte'?
>> [-Werror=implicit-function-declaration]
>>
>
> Thanks for your testing. However, I believe this build error cannot be
> seen in a real system due to x86 always has PCI on. But I see you were
> doing build testing with COMPILE_TEST.

I don't think there is a general dependency on PCI for x86, other
than the fact that most device drivers require it and you'd normally
want to enable it.

>> Also, the 'depends on EXPERT' statement looks misplaced,
>> enabling EXPERT should only be there to allow you to turn
>> extra things *off*, not to hide device drivers.
>>
>
> I will leave this to Mika to comment the "EXPERT" usage. If we remove
> this, I think that should be another patch and the documentation of
> this driver should be updated too.

Ok.

>> How about this:
>>
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 19bcb63a1ce7..b81d9b4dae7b 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -90,7 +90,7 @@ config SPI_INTEL_SPI
>>         tristate
>>
>>  config SPI_INTEL_SPI_PCI
>> -       tristate "Intel PCH/PCU SPI flash PCI driver" if EXPERT
>> +       tristate "Intel PCH/PCU SPI flash PCI driver"
>>         depends on X86 && PCI
>>         select SPI_INTEL_SPI
>>         help
>> @@ -106,10 +106,10 @@ config SPI_INTEL_SPI_PCI
>>           will be called intel-spi-pci.
>>
>>  config SPI_INTEL_SPI_PLATFORM
>> -       tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
>> -       depends on X86
>> +       tristate "Intel PCH/PCU SPI flash platform driver"
>> +       depends on X86 && (PCI || COMPILE_TEST)
>>         select SPI_INTEL_SPI
>> -       select LPC_ICH
>> +       select LPC_ICH if PCI
>
> I think we just need do:
>
> depends on X86 && PCI
>
> then
>
> select LPC_ICH

Right, that would solve this particular build issue, just not
allow the compile-test alternative.

We could also try to fix this more broadly, to allow compile-testing
independent of X86: It is bad style to have a 'select' statement for
a user-visible symbol (both in my version above, and yours).

The only connection I see between LPC_ICH and
SPI_INTEL_SPI_PCI is that at the moment that MFD driver is the
only thing that provides a platform device named "intel-spi", but I
see no reason why the spi-nor driver needs to know about this.

In similar cases on ARM, I tend to recommend something like

config SPI_INTEL_SPI_PLATFORM
       tristate "Intel PCH/PCU SPI flash platform driver" if LPC_ICH
|| COMPILE_TEST
       select SPI_INTEL_SPI

and then change all the other 'select LPC_ICH' to 'depends on
as well. If we leave the 'select' in place here, it might be good to
remove the prompt from the LPC_ICH option and have it only
selected by the other drivers.

       Arnd

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Mika Westerberg]

On Sun, Oct 15, 2017 at 09:38:57PM +0800, Bin Meng wrote:
> > Also, the 'depends on EXPERT' statement looks misplaced,
> > enabling EXPERT should only be there to allow you to turn
> > extra things *off*, not to hide device drivers.
> >
> 
> I will leave this to Mika to comment the "EXPERT" usage. If we remove
> this, I think that should be another patch and the documentation of
> this driver should be updated too.

Yeah, I guess we can remove that EXPERT dependency. It was added there
exactly because I did not want ordinary users playing with the device
and inadvertently overwrite their BIOSes (if it is not protected). I
also agree it should be a separate patch. Do you want to do that or
should I?

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Bin Meng]

Hi Mika,

On Mon, Oct 23, 2017 at 8:12 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Sun, Oct 15, 2017 at 09:38:57PM +0800, Bin Meng wrote:
>> > Also, the 'depends on EXPERT' statement looks misplaced,
>> > enabling EXPERT should only be there to allow you to turn
>> > extra things *off*, not to hide device drivers.
>> >
>>
>> I will leave this to Mika to comment the "EXPERT" usage. If we remove
>> this, I think that should be another patch and the documentation of
>> this driver should be updated too.
>
> Yeah, I guess we can remove that EXPERT dependency. It was added there
> exactly because I did not want ordinary users playing with the device
> and inadvertently overwrite their BIOSes (if it is not protected). I
> also agree it should be a separate patch. Do you want to do that or
> should I?

Thanks. I can prepare a patch to remove EXPERT.

Regards,
Bin