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 <[email protected]>
---
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
<[email protected]> 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 <[email protected]>
Thanks! ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
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 <[email protected]>
> Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> 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
>
--
With Best Regards,
Andy Shevchenko
On Tue, Apr 13, 2021 at 02:48:15PM +0200, Geert Uytterhoeven wrote:
> On Tue, Apr 13, 2021 at 2:37 PM Andy Shevchenko
> <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Tue, Apr 13, 2021 at 4:41 PM Andy Shevchenko
<[email protected]> 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
> > <[email protected]> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Apr 13, 2021 at 04:44:33PM +0200, Geert Uytterhoeven wrote:
> On Tue, Apr 13, 2021 at 4:41 PM Andy Shevchenko
> <[email protected]> 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
> > > <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Andy,
On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko
<[email protected]> 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 <[email protected]> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <[email protected]> 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?
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko
> <[email protected]> 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 <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko
<[email protected]> 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
> > <[email protected]> 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 <[email protected]> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 2021/4/15 2:06, Geert Uytterhoeven wrote:
> Hi Yicong,
>
> On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]> wrote:
> On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko
> <[email protected]> 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
> > > <[email protected]> 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 <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
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 <[email protected]> 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/[email protected]/
>>
>> Thanks again!
>>
>>>> Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller")
>>>> Signed-off-by: Geert Uytterhoeven <[email protected]>
>>>> ---
>>>> 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 <[email protected]> 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/[email protected]/
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.
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven <[email protected]> wrote:
> On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <[email protected]> wrote:
> > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko
> > > <[email protected]> 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
> > > > > <[email protected]> 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 <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko
<[email protected]> wrote:
> On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko
> > <[email protected]> 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
> > > > <[email protected]> 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 <[email protected]> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Andy,
On Mon, Apr 19, 2021 at 3:35 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko
> > > > <[email protected]> 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
> > > > > > <[email protected]> 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 <[email protected]> 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/[email protected]/,
there is no dependency on ARCH_HISI, so moving the ACPI dependency
up won't help.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Andy,
On Mon, Apr 19, 2021 at 4:14 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Apr 19, 2021 at 4:58 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven <[email protected]> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Andy,
On Mon, Apr 19, 2021 at 3:58 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Mon, Apr 19, 2021 at 3:35 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko
> > > > <[email protected]> wrote:
> > > > > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko
> > > > > > <[email protected]> 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
> > > > > > > > <[email protected]> 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 <[email protected]> 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/[email protected]/,
> > 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven <[email protected]> wrote:
> On Mon, Apr 19, 2021 at 3:35 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven <[email protected]> wrote:
> > > On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko
> > > > > <[email protected]> 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
> > > > > > > <[email protected]> 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 <[email protected]> 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/[email protected]/,
> 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!
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 19, 2021 at 4:58 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 19, 2021 at 5:18 PM Geert Uytterhoeven <[email protected]> wrote:
> On Mon, Apr 19, 2021 at 4:14 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Apr 19, 2021 at 4:58 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 19, 2021 at 5:15 PM Geert Uytterhoeven <[email protected]> wrote:
> On Mon, Apr 19, 2021 at 3:58 PM Andy Shevchenko
> <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko