| Message ID | 26db9291095c1dfd81c73b0f5f1434f9b399b1f5.1618316565.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: I2C_HISI should depend on ARCH_HISI && ACPI | expand |
On Tue, Apr 13, 2021 at 02:26:15PM +0200, Geert Uytterhoeven wrote: > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > about this driver when configuring a kernel without Hisilicon platform > or ACPI firmware support. I don't by the ACPI dependency, sorry. The driver is a pure platform driver that can be enumerated on ACPI enabled devices, but otherwise it can be used as a platform one. If you remove ACPI dependency, feel free to add my Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/i2c/busses/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index b5b4e0d0ff4dd0bc..3ead6d9e130b2ebc 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -647,7 +647,7 @@ config I2C_HIGHLANDER > > config I2C_HISI > tristate "HiSilicon I2C controller" > - depends on ARM64 || COMPILE_TEST > + depends on (ARM64 && ARCH_HISI && ACPI) || COMPILE_TEST > help > Say Y here if you want to have Hisilicon I2C controller support > available on the Kunpeng Server. > -- > 2.25.1 >
Hi Andy, On Tue, Apr 13, 2021 at 2:37 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, Apr 13, 2021 at 02:26:15PM +0200, Geert Uytterhoeven wrote: > > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > > about this driver when configuring a kernel without Hisilicon platform > > or ACPI firmware support. > > I don't by the ACPI dependency, sorry. > > The driver is a pure platform driver that can be enumerated on ACPI enabled > devices, but otherwise it can be used as a platform one. Sure, you can manually instantiate a platform device with a matching name, and set up the "clk_rate" device property. But would it make sense to do that? Would anyone ever do that? The corresponding SPI_HISI_KUNPENG depends on ACPI, too. > If you remove ACPI dependency, feel free to add my > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks! ;-) Gr{oetje,eeting}s, Geert
On Tue, Apr 13, 2021 at 02:48:15PM +0200, Geert Uytterhoeven wrote: > On Tue, Apr 13, 2021 at 2:37 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Apr 13, 2021 at 02:26:15PM +0200, Geert Uytterhoeven wrote: > > > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > > > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > > > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > > > about this driver when configuring a kernel without Hisilicon platform > > > or ACPI firmware support. > > > > I don't by the ACPI dependency, sorry. > > > > The driver is a pure platform driver that can be enumerated on ACPI enabled > > devices, but otherwise it can be used as a platform one. > > Sure, you can manually instantiate a platform device with a matching > name, and set up the "clk_rate" device property. > But would it make sense to do that? Would anyone ever do that? It will narrow down the possibility to have One Kernel for as many as possible platforms. > The corresponding SPI_HISI_KUNPENG depends on ACPI, too.
Hi Andy, On Tue, Apr 13, 2021 at 4:41 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, Apr 13, 2021 at 02:48:15PM +0200, Geert Uytterhoeven wrote: > > On Tue, Apr 13, 2021 at 2:37 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Tue, Apr 13, 2021 at 02:26:15PM +0200, Geert Uytterhoeven wrote: > > > > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > > > > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > > > > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > > > > about this driver when configuring a kernel without Hisilicon platform > > > > or ACPI firmware support. > > > > > > I don't by the ACPI dependency, sorry. > > > > > > The driver is a pure platform driver that can be enumerated on ACPI enabled > > > devices, but otherwise it can be used as a platform one. > > > > Sure, you can manually instantiate a platform device with a matching > > name, and set up the "clk_rate" device property. > > But would it make sense to do that? Would anyone ever do that? > > It will narrow down the possibility to have One Kernel for as many as possible > platforms. That One Kernel needs to have CONFIG_ACPI enabled to use I2C on the HiSilicon Kunpeng. If CONFIG_ACPI is disabled, it cannot be used, as there is no other code that creates "hisi-i2c" platform devices. Gr{oetje,eeting}s, Geert
On Tue, Apr 13, 2021 at 04:44:33PM +0200, Geert Uytterhoeven wrote: > On Tue, Apr 13, 2021 at 4:41 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Apr 13, 2021 at 02:48:15PM +0200, Geert Uytterhoeven wrote: > > > On Tue, Apr 13, 2021 at 2:37 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Tue, Apr 13, 2021 at 02:26:15PM +0200, Geert Uytterhoeven wrote: > > > > > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > > > > > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > > > > > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > > > > > about this driver when configuring a kernel without Hisilicon platform > > > > > or ACPI firmware support. > > > > > > > > I don't by the ACPI dependency, sorry. > > > > > > > > The driver is a pure platform driver that can be enumerated on ACPI enabled > > > > devices, but otherwise it can be used as a platform one. > > > > > > Sure, you can manually instantiate a platform device with a matching > > > name, and set up the "clk_rate" device property. > > > But would it make sense to do that? Would anyone ever do that? > > > > It will narrow down the possibility to have One Kernel for as many as possible > > platforms. > > That One Kernel needs to have CONFIG_ACPI enabled to use I2C on the > HiSilicon Kunpeng. If CONFIG_ACPI is disabled, it cannot be used, as there > is no other code that creates "hisi-i2c" platform devices. It is fine, but since you add a dependency to the ARCH variant, the ACPI should be added there, not here. Here is simply wrong place for this dependency as driver is *not* dependent on ACPI per se.
On 2021/4/13 20:26, Geert Uytterhoeven wrote: > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > about this driver when configuring a kernel without Hisilicon platform > or ACPI firmware support. > this is a public IP which doesn't specifically depend on ARCH_HISI. I'm not sure all the platform this IP on has ARCH_HISI configured. The driver will not be compiled by default config. This is not correct to have this dependence. > Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/i2c/busses/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index b5b4e0d0ff4dd0bc..3ead6d9e130b2ebc 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -647,7 +647,7 @@ config I2C_HIGHLANDER > > config I2C_HISI > tristate "HiSilicon I2C controller" > - depends on ARM64 || COMPILE_TEST > + depends on (ARM64 && ARCH_HISI && ACPI) || COMPILE_TEST > help > Say Y here if you want to have Hisilicon I2C controller support > available on the Kunpeng Server. >
Hi Yicong, On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > On 2021/4/13 20:26, Geert Uytterhoeven wrote: > > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > > about this driver when configuring a kernel without Hisilicon platform > > or ACPI firmware support. > > this is a public IP which doesn't specifically depend on ARCH_HISI. I'm > not sure all the platform this IP on has ARCH_HISI configured. The driver > will not be compiled by default config. This is not correct to have > this dependence. Thanks for your answer! I guess it's still fine to add a dependency on ACPI? Thanks again! > > Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller") > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > drivers/i2c/busses/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > index b5b4e0d0ff4dd0bc..3ead6d9e130b2ebc 100644 > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -647,7 +647,7 @@ config I2C_HIGHLANDER > > > > config I2C_HISI > > tristate "HiSilicon I2C controller" > > - depends on ARM64 || COMPILE_TEST > > + depends on (ARM64 && ARCH_HISI && ACPI) || COMPILE_TEST > > help > > Say Y here if you want to have Hisilicon I2C controller support > > available on the Kunpeng Server. \ Gr{oetje,eeting}s, Geert
On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > > On 2021/4/13 20:26, Geert Uytterhoeven wrote: > > > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > > > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > > > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > > > about this driver when configuring a kernel without Hisilicon platform > > > or ACPI firmware support. > > > > this is a public IP which doesn't specifically depend on ARCH_HISI. I'm > > not sure all the platform this IP on has ARCH_HISI configured. The driver > > will not be compiled by default config. This is not correct to have > > this dependence. > > Thanks for your answer! > > I guess it's still fine to add a dependency on ACPI? But why?
Hi Andy, On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > > > On 2021/4/13 20:26, Geert Uytterhoeven wrote: > > > > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > > > > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > > > > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > > > > about this driver when configuring a kernel without Hisilicon platform > > > > or ACPI firmware support. > > > > > > this is a public IP which doesn't specifically depend on ARCH_HISI. I'm > > > not sure all the platform this IP on has ARCH_HISI configured. The driver > > > will not be compiled by default config. This is not correct to have > > > this dependence. > > > > Thanks for your answer! > > > > I guess it's still fine to add a dependency on ACPI? > > But why? Please tell me how/when the driver is used when CONFIG_ACPI=n. Thanks! Gr{oetje,eeting}s, Geert
On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > > > > On 2021/4/13 20:26, Geert Uytterhoeven wrote: > > > > > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > > > > > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > > > > > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > > > > > about this driver when configuring a kernel without Hisilicon platform > > > > > or ACPI firmware support. > > > > > > > > this is a public IP which doesn't specifically depend on ARCH_HISI. I'm > > > > not sure all the platform this IP on has ARCH_HISI configured. The driver > > > > will not be compiled by default config. This is not correct to have > > > > this dependence. > > > > > > Thanks for your answer! > > > > > > I guess it's still fine to add a dependency on ACPI? > > > > But why? > > Please tell me how/when the driver is used when CONFIG_ACPI=n. I'm not using it at all. Ask the author :-) But if we follow your logic, then we need to mark all the _platform_ drivers for x86 world as ACPI dependent? This sounds ugly. So, if you are going to send a such patch, NAK here from me. Same here. NAK.
Hi Andy, On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > > > > > On 2021/4/13 20:26, Geert Uytterhoeven wrote: > > > > > > The HiSilicon Kunpeng I2C controller is only present on HiSilicon > > > > > > Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > > > > > > Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > > > > > > about this driver when configuring a kernel without Hisilicon platform > > > > > > or ACPI firmware support. > > > > > > > > > > this is a public IP which doesn't specifically depend on ARCH_HISI. I'm > > > > > not sure all the platform this IP on has ARCH_HISI configured. The driver > > > > > will not be compiled by default config. This is not correct to have > > > > > this dependence. > > > > > > > > Thanks for your answer! > > > > > > > > I guess it's still fine to add a dependency on ACPI? > > > > > > But why? > > > > Please tell me how/when the driver is used when CONFIG_ACPI=n. > > I'm not using it at all. Ask the author :-) > > But if we follow your logic, then we need to mark all the _platform_ drivers > for x86 world as ACPI dependent? This sounds ugly. Do all other x86 platform drivers have (1) an .acpi_match_table[] and (2) no other way of instantiating their devices? The first driver from the top of my memory I looked at is rtc-cmos: it has no .acpi_match_table[], and the rtc-cmos device is instantiated from arch/x86/kernel/rtc.c. For drivers with only an .of_match_table(), and no legacy users instantiating platform devices, we do have dependencies on OF. Gr{oetje,eeting}s, Geert
On 2021/4/15 2:06, Geert Uytterhoeven wrote: > Hi Yicong, > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: >> On 2021/4/13 20:26, Geert Uytterhoeven wrote: >>> The HiSilicon Kunpeng I2C controller is only present on HiSilicon >>> Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. >>> Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user >>> about this driver when configuring a kernel without Hisilicon platform >>> or ACPI firmware support. >> >> this is a public IP which doesn't specifically depend on ARCH_HISI. I'm >> not sure all the platform this IP on has ARCH_HISI configured. The driver >> will not be compiled by default config. This is not correct to have >> this dependence. > > Thanks for your answer! > > I guess it's still fine to add a dependency on ACPI? yes. currently we only use this driver through ACPI. So at least for this driver, it make sense to keep the dependency. > > Thanks again! > >>> Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller") >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> --- >>> drivers/i2c/busses/Kconfig | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >>> index b5b4e0d0ff4dd0bc..3ead6d9e130b2ebc 100644 >>> --- a/drivers/i2c/busses/Kconfig >>> +++ b/drivers/i2c/busses/Kconfig >>> @@ -647,7 +647,7 @@ config I2C_HIGHLANDER >>> >>> config I2C_HISI >>> tristate "HiSilicon I2C controller" >>> - depends on ARM64 || COMPILE_TEST >>> + depends on (ARM64 && ARCH_HISI && ACPI) || COMPILE_TEST >>> help >>> Say Y here if you want to have Hisilicon I2C controller support >>> available on the Kunpeng Server. > \ > Gr{oetje,eeting}s, > > Geert >
On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > > > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > > > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: ... > > > > > I guess it's still fine to add a dependency on ACPI? > > > > > > > > But why? > > > > > > Please tell me how/when the driver is used when CONFIG_ACPI=n. > > > > I'm not using it at all. Ask the author :-) > > > > But if we follow your logic, then we need to mark all the _platform_ drivers > > for x86 world as ACPI dependent? This sounds ugly. > > Do all other x86 platform drivers have (1) an .acpi_match_table[] and > (2) no other way of instantiating their devices? > The first driver from the top of my memory I looked at is rtc-cmos: > it has no .acpi_match_table[], and the rtc-cmos device is instantiated > from arch/x86/kernel/rtc.c. > > For drivers with only an .of_match_table(), and no legacy users > instantiating platform devices, we do have dependencies on OF. This is not true. Entire IIO subsystem is an example.
On 2021/4/15 16:18, Yicong Yang wrote: > On 2021/4/15 2:06, Geert Uytterhoeven wrote: >> Hi Yicong, >> >> On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: >>> On 2021/4/13 20:26, Geert Uytterhoeven wrote: >>>> The HiSilicon Kunpeng I2C controller is only present on HiSilicon >>>> Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. >>>> Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user >>>> about this driver when configuring a kernel without Hisilicon platform >>>> or ACPI firmware support. >>> >>> this is a public IP which doesn't specifically depend on ARCH_HISI. I'm >>> not sure all the platform this IP on has ARCH_HISI configured. The driver >>> will not be compiled by default config. This is not correct to have >>> this dependence. >> >> Thanks for your answer! >> >> I guess it's still fine to add a dependency on ACPI? > > yes. currently we only use this driver through ACPI. So at least > for this driver, it make sense to keep the dependency. > sorry. i was a little mess about this. I dropped this in [1]. so just keep it as is. [1] https://lore.kernel.org/linux-i2c/YGMntYT2iz72wgrd@smile.fi.intel.com/ >> >> Thanks again! >> >>>> Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller") >>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>>> --- >>>> drivers/i2c/busses/Kconfig | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >>>> index b5b4e0d0ff4dd0bc..3ead6d9e130b2ebc 100644 >>>> --- a/drivers/i2c/busses/Kconfig >>>> +++ b/drivers/i2c/busses/Kconfig >>>> @@ -647,7 +647,7 @@ config I2C_HIGHLANDER >>>> >>>> config I2C_HISI >>>> tristate "HiSilicon I2C controller" >>>> - depends on ARM64 || COMPILE_TEST >>>> + depends on (ARM64 && ARCH_HISI && ACPI) || COMPILE_TEST >>>> help >>>> Say Y here if you want to have Hisilicon I2C controller support >>>> available on the Kunpeng Server. >> \ >> Gr{oetje,eeting}s, >> >> Geert >> > > > . >
On Thu, Apr 15, 2021 at 05:04:39PM +0800, Yicong Yang wrote: > On 2021/4/15 16:18, Yicong Yang wrote: > > On 2021/4/15 2:06, Geert Uytterhoeven wrote: > >> On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > >>> On 2021/4/13 20:26, Geert Uytterhoeven wrote: > >>>> The HiSilicon Kunpeng I2C controller is only present on HiSilicon > >>>> Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. > >>>> Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user > >>>> about this driver when configuring a kernel without Hisilicon platform > >>>> or ACPI firmware support. > >>> > >>> this is a public IP which doesn't specifically depend on ARCH_HISI. I'm > >>> not sure all the platform this IP on has ARCH_HISI configured. The driver > >>> will not be compiled by default config. This is not correct to have > >>> this dependence. > >> > >> Thanks for your answer! > >> > >> I guess it's still fine to add a dependency on ACPI? > > > > yes. currently we only use this driver through ACPI. So at least > > for this driver, it make sense to keep the dependency. > > > > sorry. i was a little mess about this. I dropped this in [1]. > so just keep it as is. > > [1] https://lore.kernel.org/linux-i2c/YGMntYT2iz72wgrd@smile.fi.intel.com/ If you think that ACPI dependency is good to have there, go ahead, not my worries of the consequences. I just consider that as unneeded dependencies. The proper fix would be to have a split in Kbuild infra for compile dependencies and run-time dependencies. +Cc: Masahiro for the discussion, maybe it had already taken place and there is an impediment to do so.
Hi Andy, On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > > > > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > > > > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > > ... > > > > > > > I guess it's still fine to add a dependency on ACPI? > > > > > > > > > > But why? > > > > > > > > Please tell me how/when the driver is used when CONFIG_ACPI=n. > > > > > > I'm not using it at all. Ask the author :-) > > > > > > But if we follow your logic, then we need to mark all the _platform_ drivers > > > for x86 world as ACPI dependent? This sounds ugly. > > > > Do all other x86 platform drivers have (1) an .acpi_match_table[] and > > (2) no other way of instantiating their devices? > > The first driver from the top of my memory I looked at is rtc-cmos: > > it has no .acpi_match_table[], and the rtc-cmos device is instantiated > > from arch/x86/kernel/rtc.c. > > > > For drivers with only an .of_match_table(), and no legacy users > > instantiating platform devices, we do have dependencies on OF. > > This is not true. Entire IIO subsystem is an example. Do you care to elaborate? Three quarters of the IIO drivers are I2C and SPI drivers, and thus not subject to the above. Gr{oetje,eeting}s, Geert
On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > > > > > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > > > > > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: ... > > > > > > > I guess it's still fine to add a dependency on ACPI? > > > > > > > > > > > > But why? > > > > > > > > > > Please tell me how/when the driver is used when CONFIG_ACPI=n. > > > > > > > > I'm not using it at all. Ask the author :-) > > > > > > > > But if we follow your logic, then we need to mark all the _platform_ drivers > > > > for x86 world as ACPI dependent? This sounds ugly. > > > > > > Do all other x86 platform drivers have (1) an .acpi_match_table[] and > > > (2) no other way of instantiating their devices? > > > The first driver from the top of my memory I looked at is rtc-cmos: > > > it has no .acpi_match_table[], and the rtc-cmos device is instantiated > > > from arch/x86/kernel/rtc.c. > > > > > > For drivers with only an .of_match_table(), and no legacy users > > > instantiating platform devices, we do have dependencies on OF. > > > > This is not true. Entire IIO subsystem is an example. > > Do you care to elaborate? > Three quarters of the IIO drivers are I2C and SPI drivers, and thus not > subject to the above. It seems I missed that you are talking about platform device drivers. In any case it's not true. We have the platform drivers w/o legacy users that are not dependent on OF. They may _indirectly_ be dependent, but this is fine as I stated above when suggested to move ACPI dependency on ARCH_xxx level.
Hi Andy, On Mon, Apr 19, 2021 at 3:35 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > > > > > > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > > > > > > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > > ... > > > > > > > > > I guess it's still fine to add a dependency on ACPI? > > > > > > > > > > > > > > But why? > > > > > > > > > > > > Please tell me how/when the driver is used when CONFIG_ACPI=n. > > > > > > > > > > I'm not using it at all. Ask the author :-) > > > > > > > > > > But if we follow your logic, then we need to mark all the _platform_ drivers > > > > > for x86 world as ACPI dependent? This sounds ugly. > > > > > > > > Do all other x86 platform drivers have (1) an .acpi_match_table[] and > > > > (2) no other way of instantiating their devices? > > > > The first driver from the top of my memory I looked at is rtc-cmos: > > > > it has no .acpi_match_table[], and the rtc-cmos device is instantiated > > > > from arch/x86/kernel/rtc.c. > > > > > > > > For drivers with only an .of_match_table(), and no legacy users > > > > instantiating platform devices, we do have dependencies on OF. > > > > > > This is not true. Entire IIO subsystem is an example. > > > > Do you care to elaborate? > > Three quarters of the IIO drivers are I2C and SPI drivers, and thus not > > subject to the above. > > It seems I missed that you are talking about platform device drivers. OK. > In any case it's not true. We have the platform drivers w/o legacy > users that are not dependent on OF. Example? ;-) > They may _indirectly_ be dependent, but this is fine as I stated above > when suggested to move ACPI dependency on ARCH_xxx level. As per the response from the driver maintainer https://lore.kernel.org/linux-arm-kernel/bd8db435-24e1-5ab3-6b35-1d4d8a292a7e@hisilicon.com/, there is no dependency on ARCH_HISI, so moving the ACPI dependency up won't help. Gr{oetje,eeting}s, Geert
On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Apr 19, 2021 at 3:35 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > > > > > > > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > > > > ... > > > > > > > > > > > I guess it's still fine to add a dependency on ACPI? > > > > > > > > > > > > > > > > But why? > > > > > > > > > > > > > > Please tell me how/when the driver is used when CONFIG_ACPI=n. > > > > > > > > > > > > I'm not using it at all. Ask the author :-) > > > > > > > > > > > > But if we follow your logic, then we need to mark all the _platform_ drivers > > > > > > for x86 world as ACPI dependent? This sounds ugly. > > > > > > > > > > Do all other x86 platform drivers have (1) an .acpi_match_table[] and > > > > > (2) no other way of instantiating their devices? > > > > > The first driver from the top of my memory I looked at is rtc-cmos: > > > > > it has no .acpi_match_table[], and the rtc-cmos device is instantiated > > > > > from arch/x86/kernel/rtc.c. > > > > > > > > > > For drivers with only an .of_match_table(), and no legacy users > > > > > instantiating platform devices, we do have dependencies on OF. > > > > > > > > This is not true. Entire IIO subsystem is an example. > > > > > > Do you care to elaborate? > > > Three quarters of the IIO drivers are I2C and SPI drivers, and thus not > > > subject to the above. > > > > It seems I missed that you are talking about platform device drivers. > > OK. > > > In any case it's not true. We have the platform drivers w/o legacy > > users that are not dependent on OF. > > Example? ;-) i2c-owl.c > > They may _indirectly_ be dependent, but this is fine as I stated above > > when suggested to move ACPI dependency on ARCH_xxx level. > > As per the response from the driver maintainer > https://lore.kernel.org/linux-arm-kernel/bd8db435-24e1-5ab3-6b35-1d4d8a292a7e@hisilicon.com/, > there is no dependency on ARCH_HISI, so moving the ACPI dependency > up won't help. So, an ACPI dependency is simply not applicable here as it's a compile dependency as well, which is not a limitation for this driver. Again, talk to Masahiro how to handle this, but I don't see any good justification to have ACPI (compile time) dependency here. So, again NAK!
On Mon, Apr 19, 2021 at 4:58 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > In any case it's not true. We have the platform drivers w/o legacy > > > users that are not dependent on OF. > > > > Example? ;-) > > i2c-owl.c In case you want more sound/sparc/amd7930.c And I believe I can find zillions of them.
Hi Andy, On Mon, Apr 19, 2021 at 3:58 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Apr 19, 2021 at 3:35 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > > > > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > > > > > > > > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > > > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > > > > > > ... > > > > > > > > > > > > > I guess it's still fine to add a dependency on ACPI? > > > > > > > > > > > > > > > > > > But why? > > > > > > > > > > > > > > > > Please tell me how/when the driver is used when CONFIG_ACPI=n. > > > > > > > > > > > > > > I'm not using it at all. Ask the author :-) > > > > > > > > > > > > > > But if we follow your logic, then we need to mark all the _platform_ drivers > > > > > > > for x86 world as ACPI dependent? This sounds ugly. > > > > > > > > > > > > Do all other x86 platform drivers have (1) an .acpi_match_table[] and > > > > > > (2) no other way of instantiating their devices? > > > > > > The first driver from the top of my memory I looked at is rtc-cmos: > > > > > > it has no .acpi_match_table[], and the rtc-cmos device is instantiated > > > > > > from arch/x86/kernel/rtc.c. > > > > > > > > > > > > For drivers with only an .of_match_table(), and no legacy users > > > > > > instantiating platform devices, we do have dependencies on OF. > > > > > > > > > > This is not true. Entire IIO subsystem is an example. > > > > > > > > Do you care to elaborate? > > > > Three quarters of the IIO drivers are I2C and SPI drivers, and thus not > > > > subject to the above. > > > > > > It seems I missed that you are talking about platform device drivers. > > > > OK. > > > > > In any case it's not true. We have the platform drivers w/o legacy > > > users that are not dependent on OF. > > > > Example? ;-) > > i2c-owl.c I2C_OWL depends on ARCH_ACTIONS || COMPILE_TEST (arm32) ARCH_ACTIONS depends on ARCH_MULTI_V7 depends on ARCH_MULTIPLATFORM ARCH_MULTIPLATFORM selects USE_OF USE_OF selects OF ARCH_MULTI_V7 selects ARCH_MULTI_V6_V7 (arm64) ARM64 selects OF so we do have a dependency on OF, unless we're compile-testing. > > > They may _indirectly_ be dependent, but this is fine as I stated above > > > when suggested to move ACPI dependency on ARCH_xxx level. > > > > As per the response from the driver maintainer > > https://lore.kernel.org/linux-arm-kernel/bd8db435-24e1-5ab3-6b35-1d4d8a292a7e@hisilicon.com/, > > there is no dependency on ARCH_HISI, so moving the ACPI dependency > > up won't help. > > So, an ACPI dependency is simply not applicable here as it's a compile > dependency as well, which is not a limitation for this driver. Again, > talk to Masahiro how to handle this, but I don't see any good > justification to have ACPI (compile time) dependency here. So, again > NAK! Please tell me how this driver will be probed when CONFIG_ACPI is disabled (it cannot, as nothing instantiates platform devices of the right type, so there is no reason to bother the user with a question about this driver when configuring his kernel). Gr{oetje,eeting}s, Geert
Hi Andy, On Mon, Apr 19, 2021 at 4:14 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 19, 2021 at 4:58 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > In any case it's not true. We have the platform drivers w/o legacy > > > > users that are not dependent on OF. > > > > > > Example? ;-) > > > > i2c-owl.c > > In case you want more > sound/sparc/amd7930.c SND_SUN_AMD7930 depends on SND_SPARC && SBUS SND_SPARC depends on SPARC SPARC selects OF Hence, SND_SUN_AMD7930 depends on OF. Gr{oetje,eeting}s, Geert
On Mon, Apr 19, 2021 at 5:18 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Apr 19, 2021 at 4:14 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 19, 2021 at 4:58 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > > In any case it's not true. We have the platform drivers w/o legacy > > > > > users that are not dependent on OF. > > > > > > > > Example? ;-) > > > > > > i2c-owl.c > > > > In case you want more > > sound/sparc/amd7930.c > > SND_SUN_AMD7930 depends on SND_SPARC && SBUS > SND_SPARC depends on SPARC > SPARC selects OF > > Hence, SND_SUN_AMD7930 depends on OF. Exactly my point. Read back what I wrote. TL;DR: It's *fine* to have _indirect_ dependency like this.
On Mon, Apr 19, 2021 at 5:15 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Apr 19, 2021 at 3:58 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > Please tell me how this driver will be probed when CONFIG_ACPI > is disabled (it cannot, as nothing instantiates platform devices of the > right type, so there is no reason to bother the user with a question about > this driver when configuring his kernel). Go ahead with it in v2. I'll not block you.
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index b5b4e0d0ff4dd0bc..3ead6d9e130b2ebc 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -647,7 +647,7 @@ config I2C_HIGHLANDER config I2C_HISI tristate "HiSilicon I2C controller" - depends on ARM64 || COMPILE_TEST + depends on (ARM64 && ARCH_HISI && ACPI) || COMPILE_TEST help Say Y here if you want to have Hisilicon I2C controller support available on the Kunpeng Server.
The HiSilicon Kunpeng I2C controller is only present on HiSilicon Kunpeng SoCs, and its driver relies on ACPI to probe for its presence. Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user about this driver when configuring a kernel without Hisilicon platform or ACPI firmware support. Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/i2c/busses/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)