| Message ID | 1503648771-17514-1-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Cyrille Pitchen |
| Headers | show |
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?
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
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
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>
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 >
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
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
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
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
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?
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
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
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(+)