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

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

On 17.04.24 00:30, Andrew Lunn wrote:
>> 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)?;
> 
> I don't know how well that will work out in practice. The addressing
> schemes for C22 and C45 are different.
> 
> C22 simply has 32 registers, numbered 0-31.
> 
> C45 has 32 MDIO manageable devices (MMD) each with 65536 registers.
> 
> All of the 32 C22 registers have well defined names, which are listed
> in mii.h. Ideally we want to keep the same names. The most of the MMD
> also have defined names, listed in mdio.h. Many of the registers are
> also named in mdio.h, although vendors do tend to add more vendor
> proprietary registers.
> 
> Your R::Index would need to be a single value for C22 and a tuple of
> two values for C45.

Yes that was my idea:

    enum C22Index {
        // The unique 32 names of the C22 registers...
    }

    impl Register for C22 {
        type Index = C22Index;
        type Value = u16;
    }

    impl Register for C45 {
        type Index = (u8, u16); // We could also create a newtype that wraps this and give it a better name.
        type Value = u16;
    }

You can then do:

    let val = dev.read_register::<C45>((4, 406))?;
    dev.write_register::<C22>(4, val)?;

If you only have these two registers types, then I would say this is
overkill, but I assumed that there are more.

> There is nothing like `C45OrC22`. A register is either in C22
> namespace, or in the C45 namespace.

I see, I got this idea from:

> [...] 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. [...]

Is this not accessing a C45 register via C22 and letting phylib decide?

> Where it gets interesting is that there are two methods to access the
> C45 register namespace. The PHY driver should not care about this, it
> is the MDIO bus driver and the PHYLIB core which handles the two
> access mechanisms.

If the driver shouldn't be concerned with how the access gets handled,
why do we even have a naming problem?

-- 
Cheers,
Benno