Re: [PATCH v2 3/3] net: phy: add Rust Asix PHY driver

FUJITA Tomonori
2023-10-06T15:26:13Z

On Fri, 6 Oct 2023 16:35:28 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> The Kconfig file would be like the following. AX88796B_RUST_PHY
>> depends on AX88796B_PHY so the description of AX88796B_PHY is enough?
>> I'll add the name of the module.
>>
>>
>> config AX88796B_PHY
>> tristate "Asix PHYs"
>> help
>> Currently supports the Asix Electronics PHY found in the X-Surf 100
>> AX88796B package.
>
> I _think_ you can add
>
> depends on !AX88796B_RUST_PHY
>
>> config AX88796B_RUST_PHY
>> bool "Rust reference driver"
>> depends on RUST && AX88796B_PHY
>
> And then this becomes
>
> depends on RUST && !AX88796B_PHY
>
>> default n
>> help
>> Uses the Rust version driver for Asix PHYs.
>
> You then express the mutual exclusion in Kconfig, so that only one of
> AX88796B_PHY and AX88796B_RUST_PHY is ever enabled.
>
> I've not actually tried this, so it might not work. Ideally you need
> to be able disable both, so that you can enable one.

This doesn't work.

ubuntu@ip-172-30-47-114:~/git/linux$ make LLVM=1 -j 32 menuconfig
drivers/net/phy/Kconfig:111:error: recursive dependency detected!
drivers/net/phy/Kconfig:111: symbol AX88796B_RUST_PHY depends on AX88796B_PHY
drivers/net/phy/Kconfig:104: symbol AX88796B_PHY depends on AX88796B_RUST_P

The following gurantees that only one is built but we hit the `select
AX88796B_PHY` problem in my previous mail.

config AX88796B_PHY
tristate "Asix PHYs"
help
Currently supports the Asix Electronics PHY found in the X-Surf 100
AX88796B package.

config AX88796B_RUST_PHY
bool "Rust reference driver"
depends on RUST && AX88796B_PHY=n
help
Uses the Rust version driver for Asix PHYs.

Greg, Sorry. I messed up copy-and-paste in the previous mail. I think that you meant the above.

> There is good documentation in
>
> Documentation/kbuild/kconfig-language.rst
>
>> >> +ifdef CONFIG_AX88796B_RUST_PHY
>> >> + obj-$(CONFIG_AX88796B_PHY) += ax88796b_rust.o
>> >> +else
>> >> + obj-$(CONFIG_AX88796B_PHY) += ax88796b.o
>> >> +endif
>> >
>> > This can be expressed in Kconfig, no need to put this here, right?
>>
>> Not sure. Is it possible? If we allow both modules to be built, I
>> guess it's possible though.
>
> If what i suggested above works, you don't need the ifdef, just list
> the two drivers are normal and let Kconfig only enable one at most.
> Or go back to your idea of using choice. Maybe something like
>
> choice
> tristate "AX88796B PHY driver"
>
> config CONFIG_AX88796B_PHY
> bool "C driver"
>
> config CONFIG_AX88796B_RUST_PHY
> bool "Rust driver"
> depends on RUST
> endchoice
>
> totally untested....

Now I'm thinking that this is the best option. Kconfig would be the following:

config AX88796B_PHY
tristate "Asix PHYs"
help
Currently supports the Asix Electronics PHY found in the X-Surf 100
AX88796B package.

choice
prompt "Implementation options"
depends on AX88796B_PHY
help
There are two implementations for a driver for Asix PHYs; C and Rust.
If not sure, choose C.

config AX88796B_C_PHY
bool "The C version driver for Asix PHYs"

config AX88796B_RUST_PHY
bool "The Rust version driver for Asix PHYs"
depends on RUST

endchoice

No hack in Makefile:

obj-$(CONFIG_AX88796B_C_PHY) += ax88796b.o
obj-$(CONFIG_AX88796B_RUST_PHY) += ax88796b_rust.o