Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[Benno Lossin <benno.lossin@proton.me>]

On 16.04.24 15:21, FUJITA Tomonori wrote:
> Hi,
> 
> On Tue, 16 Apr 2024 14:38:11 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>> If I correctly understand the original driver code, C45 bus protocol
>>> is used. Adding functions for C45 bus protocol read/write would be
>>> enough for this driver, I guess.
>>
>> Now i've read more of the patches, i can see that the MDIO bus master
>> is C45 only. At least, that is all that is implemented in the
>> driver. So for this combination of MAC and PHY, forcing C45 register
>> access using C45 bus protocol will work.
> 
> Thanks a lot!
> 
>> However, can you combine this PHY with some other MDIO bus master,
>> which does not support C45? Then C45 over C22 would need to be used?
>> Looking at the data sheet i found online, there is no suggestion it
>> does support C22 bus protocol. All the diagrams/tables only show C45
>> bus protocol.
> 
> qt2025_ds3014.pdf?
> 
>> So this PHY is a special case. So you can use wrapper methods which
>> force C45 bus protocol, and ignore phylib support for performing C45
>> over C22 when needed. However, while doing this:
>>
>> 1: Clearly document that these helpers are not generic, they force C45
>>     register access using C45 bus protocol, and should only by used PHY
>>     drivers which know the PHY device does not support C45 over C22
>>
>> 2: Think about naming. At some point we are going to add the generic
>>     helpers for accessing C45 registers which leave the core to decide
>>     if to perform a C45 bus protocol access or C45 over C22. Those
>>     generic helpers need to have the natural name for accessing a C45
>>     register since 99% of drivers will be using them. The helpers you
>>     add now need to use a less common name.
> 
> Sounds like we should save the names of c45_read and c45_write.
> 
> read_with_c45_bus_protocol and write_with_c45_bus_protocol?
> 
> They call mdiobus_c45_*, right?

I think we could also do a more rusty solution. For example we could
have these generic functions for `phy::Device`:

     fn read_register<R: Register>(&mut self, which: R::Index) -> Result<R::Value>;
     fn write_register<R: Register>(&mut self, which: R::Index, value: R::Value) -> Result;

That way we can support many different registers without polluting the
namespace. We can then have a `C45` register and a `C22` register and a
`C45OrC22` (maybe we should use `C45_OrC22` instead, since I can read
that better, but let's bikeshed when we have the actual patch).

Calling those functions would look like this:

     let value = dev.read_register::<C45>(reg1)?;
     dev.write_register::<C45>(reg2, value)?;

-- 
Cheers,
Benno