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

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

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