| Message ID | 20250410100410.348-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,v2,1/2] net: phy: mediatek: permit to compile test GE SOC PHY driver | expand |
On Thu, Apr 10, 2025, at 12:04, Christian Marangi wrote: > When commit 462a3daad679 ("net: phy: mediatek: fix compile-test > dependencies") fixed the dependency, it should have also introduced > an or on COMPILE_TEST to permit this driver to be compile-tested even if > NVMEM_MTK_EFUSE wasn't selected Why does this matter? NVMEM_MTK_EFUSE can be enabled for both allmodconfig and randconfig builds on any architecture, so you get build coverage either way, it's just a little less likely to be enabled in randconfig I guess? > diff --git a/drivers/net/phy/mediatek/Kconfig b/drivers/net/phy/mediatek/Kconfig > index 2a8ac5aed0f8..6a4c2b328c41 100644 > --- a/drivers/net/phy/mediatek/Kconfig > +++ b/drivers/net/phy/mediatek/Kconfig > @@ -15,8 +15,7 @@ config MEDIATEK_GE_PHY > > config MEDIATEK_GE_SOC_PHY > tristate "MediaTek SoC Ethernet PHYs" > - depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST > - depends on NVMEM_MTK_EFUSE > + depends on (ARM64 && ARCH_MEDIATEK && NVMEM_MTK_EFUSE) || COMPILE_TEST > select MTK_NET_PHYLIB > help > Supports MediaTek SoC built-in Gigabit Ethernet PHYs. > -- I would expect this to break the build with CONFIG_NVMEM=m and MEDIATEK_GE_SOC_PHY=y. The normal thing here would be to have a dependency on CONFIG_NVMEM in place of the NVMEM_MTK_EFUSE dependency, or possible on 'NVMEM || !NVMEM' if you want to make it more likely to be enabled in randconfig builds. Arnd
On Thu, Apr 10, 2025 at 12:31:05PM +0200, Arnd Bergmann wrote: > On Thu, Apr 10, 2025, at 12:04, Christian Marangi wrote: > > When commit 462a3daad679 ("net: phy: mediatek: fix compile-test > > dependencies") fixed the dependency, it should have also introduced > > an or on COMPILE_TEST to permit this driver to be compile-tested even if > > NVMEM_MTK_EFUSE wasn't selected > > Why does this matter? NVMEM_MTK_EFUSE can be enabled for both > allmodconfig and randconfig builds on any architecture, so you > get build coverage either way, it's just a little less likely > to be enabled in randconfig I guess? > If we base stuff on the fact that everything is selected or that a random config by luck selects it, then COMPILE_TEST doesn't make sense at all. For my personal test, I wanted to test the driver on a simple x86 build without having to depend on ARCH or having to cross compile. Won't happen on real world scenario? Totally. I should be able to compile it? Yes. > > diff --git a/drivers/net/phy/mediatek/Kconfig b/drivers/net/phy/mediatek/Kconfig > > index 2a8ac5aed0f8..6a4c2b328c41 100644 > > --- a/drivers/net/phy/mediatek/Kconfig > > +++ b/drivers/net/phy/mediatek/Kconfig > > @@ -15,8 +15,7 @@ config MEDIATEK_GE_PHY > > > > config MEDIATEK_GE_SOC_PHY > > tristate "MediaTek SoC Ethernet PHYs" > > - depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST > > - depends on NVMEM_MTK_EFUSE > > + depends on (ARM64 && ARCH_MEDIATEK && NVMEM_MTK_EFUSE) || COMPILE_TEST > > select MTK_NET_PHYLIB > > help > > Supports MediaTek SoC built-in Gigabit Ethernet PHYs. > > -- > > I would expect this to break the build with CONFIG_NVMEM=m > and MEDIATEK_GE_SOC_PHY=y. > > The normal thing here would be to have a dependency on > CONFIG_NVMEM in place of the NVMEM_MTK_EFUSE dependency, > or possible on 'NVMEM || !NVMEM' if you want to make it > more likely to be enabled in randconfig builds. > The big idea of these dependency is that... In MTK the internal PHY of the switch needs calibration or it won't work hence it doesn't make sense to select the PHY as it won't ever work without the NVMEM driver. But from a compile test view where we only evaluate if the driver have compilation error or other kind of warning, we should not care...
On Thu, Apr 10, 2025 at 12:40:15PM +0200, Christian Marangi wrote: > On Thu, Apr 10, 2025 at 12:31:05PM +0200, Arnd Bergmann wrote: > > On Thu, Apr 10, 2025, at 12:04, Christian Marangi wrote: > > > When commit 462a3daad679 ("net: phy: mediatek: fix compile-test > > > dependencies") fixed the dependency, it should have also introduced > > > an or on COMPILE_TEST to permit this driver to be compile-tested even if > > > NVMEM_MTK_EFUSE wasn't selected > > > > Why does this matter? NVMEM_MTK_EFUSE can be enabled for both > > allmodconfig and randconfig builds on any architecture, so you > > get build coverage either way, it's just a little less likely > > to be enabled in randconfig I guess? > > > > If we base stuff on the fact that everything is selected or that a > random config by luck selects it, then COMPILE_TEST doesn't make sense > at all. > > For my personal test, I wanted to test the driver on a simple x86 build > without having to depend on ARCH or having to cross compile. Won't > happen on real world scenario? Totally. I should be able to compile it? > Yes. > > > > diff --git a/drivers/net/phy/mediatek/Kconfig b/drivers/net/phy/mediatek/Kconfig > > > index 2a8ac5aed0f8..6a4c2b328c41 100644 > > > --- a/drivers/net/phy/mediatek/Kconfig > > > +++ b/drivers/net/phy/mediatek/Kconfig > > > @@ -15,8 +15,7 @@ config MEDIATEK_GE_PHY > > > > > > config MEDIATEK_GE_SOC_PHY > > > tristate "MediaTek SoC Ethernet PHYs" > > > - depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST > > > - depends on NVMEM_MTK_EFUSE > > > + depends on (ARM64 && ARCH_MEDIATEK && NVMEM_MTK_EFUSE) || COMPILE_TEST > > > select MTK_NET_PHYLIB > > > help > > > Supports MediaTek SoC built-in Gigabit Ethernet PHYs. > > > -- > > > > I would expect this to break the build with CONFIG_NVMEM=m > > and MEDIATEK_GE_SOC_PHY=y. > > > > The normal thing here would be to have a dependency on > > CONFIG_NVMEM in place of the NVMEM_MTK_EFUSE dependency, > > or possible on 'NVMEM || !NVMEM' if you want to make it > > more likely to be enabled in randconfig builds. > > > > The big idea of these dependency is that... In MTK the internal PHY of > the switch needs calibration or it won't work hence it doesn't make > sense to select the PHY as it won't ever work without the NVMEM driver. > > But from a compile test view where we only evaluate if the driver have > compilation error or other kind of warning, we should not care... > Also 99% I could be wrong but from what I can see in NVMEM kconfig, NVMEM is not tristate but only bool? So NVMEM=m is not a thing?
On Thu, Apr 10, 2025, at 12:44, Christian Marangi wrote: > On Thu, Apr 10, 2025 at 12:40:15PM +0200, Christian Marangi wrote: > > Also 99% I could be wrong but from what I can see in NVMEM kconfig, > NVMEM is not tristate but only bool? So NVMEM=m is not a thing? Ah, I missed that. In this case your patches are both fine. Acked-by: Arnd Bergmann <arnd@arndb.de>
On Thu, Apr 10, 2025, at 12:40, Christian Marangi wrote: > On Thu, Apr 10, 2025 at 12:31:05PM +0200, Arnd Bergmann wrote: >> On Thu, Apr 10, 2025, at 12:04, Christian Marangi wrote: >> > When commit 462a3daad679 ("net: phy: mediatek: fix compile-test >> > dependencies") fixed the dependency, it should have also introduced >> > an or on COMPILE_TEST to permit this driver to be compile-tested even if >> > NVMEM_MTK_EFUSE wasn't selected >> >> Why does this matter? NVMEM_MTK_EFUSE can be enabled for both >> allmodconfig and randconfig builds on any architecture, so you >> get build coverage either way, it's just a little less likely >> to be enabled in randconfig I guess? >> > > If we base stuff on the fact that everything is selected or that a > random config by luck selects it, then COMPILE_TEST doesn't make sense > at all. > > For my personal test, I wanted to test the driver on a simple x86 build > without having to depend on ARCH or having to cross compile. Won't > happen on real world scenario? Totally. I should be able to compile it? > Yes. Being able to compile test the driver yourself is clearly a good idea, I just don't think that requires the dependency to be conditional here. >> I would expect this to break the build with CONFIG_NVMEM=m >> and MEDIATEK_GE_SOC_PHY=y. >> >> The normal thing here would be to have a dependency on >> CONFIG_NVMEM in place of the NVMEM_MTK_EFUSE dependency, >> or possible on 'NVMEM || !NVMEM' if you want to make it >> more likely to be enabled in randconfig builds. >> > > The big idea of these dependency is that... In MTK the internal PHY of > the switch needs calibration or it won't work hence it doesn't make > sense to select the PHY as it won't ever work without the NVMEM driver. That's not how dependencies normally work: the driver also fails to work correctly if you are missing the correct pinctrl or led driver, or the syscon, yet you don't list them explicitly because you just need them for the machine to work correctly. The driver dependencies should just list what you need to build, regardless of whether you are compile-testing or building a driver that is going to be used. Arnd
Hello: This series was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 10 Apr 2025 12:04:03 +0200 you wrote: > When commit 462a3daad679 ("net: phy: mediatek: fix compile-test > dependencies") fixed the dependency, it should have also introduced > an or on COMPILE_TEST to permit this driver to be compile-tested even if > NVMEM_MTK_EFUSE wasn't selected. The driver makes use of NVMEM API that > are always compiled (return error) so the driver can actually be > compiled even without that config. > > [...] Here is the summary with links: - [net-next,v2,1/2] net: phy: mediatek: permit to compile test GE SOC PHY driver https://git.kernel.org/netdev/net-next/c/e5566162af8b - [net-next,v2,2/2] net: phy: mediatek: add Airoha PHY ID to SoC driver https://git.kernel.org/netdev/net-next/c/6a325aed130b You are awesome, thank you!
diff --git a/drivers/net/phy/mediatek/Kconfig b/drivers/net/phy/mediatek/Kconfig index 2a8ac5aed0f8..6a4c2b328c41 100644 --- a/drivers/net/phy/mediatek/Kconfig +++ b/drivers/net/phy/mediatek/Kconfig @@ -15,8 +15,7 @@ config MEDIATEK_GE_PHY config MEDIATEK_GE_SOC_PHY tristate "MediaTek SoC Ethernet PHYs" - depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST - depends on NVMEM_MTK_EFUSE + depends on (ARM64 && ARCH_MEDIATEK && NVMEM_MTK_EFUSE) || COMPILE_TEST select MTK_NET_PHYLIB help Supports MediaTek SoC built-in Gigabit Ethernet PHYs.
When commit 462a3daad679 ("net: phy: mediatek: fix compile-test dependencies") fixed the dependency, it should have also introduced an or on COMPILE_TEST to permit this driver to be compile-tested even if NVMEM_MTK_EFUSE wasn't selected. The driver makes use of NVMEM API that are always compiled (return error) so the driver can actually be compiled even without that config. Fix and simplify the dependency condition of this kernel config. Fixes: 462a3daad679 ("net: phy: mediatek: fix compile-test dependencies") Acked-by: Daniel Golle <daniel@makrotopia.org> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- Changes v2: - Add Ack and Reviewed-by tag - Address suggested dependency from Russell drivers/net/phy/mediatek/Kconfig | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)