Re: [RFC PATCH v2 1/2] rust: net::phy support for C45 register access

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

On 02.06.24 09:24, FUJITA Tomonori wrote:
> On Sat, 01 Jun 2024 12:21:14 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> On 01.06.24 06:35, FUJITA Tomonori wrote:
>>> Adds support for C45 register access. Instead of adding read/write_c45
>>> methods, this unifies the API to access C22 and C45 registers with
>>> trait.
>>
>> The short commit message only mentions C45, should it also include C22?
> 
> How about the following?
> 
> Adds support for C45 register access. The abstractions support access
> to only C22 registers now. Instead of adding read/write_c45 methods
> specifically for C45, a new reg module supports the unified API to
> access to C22 and C45 registers with trait, by calling an appropriate
> phylib functions.

Sure you can use that, but my comment above was about the short message
(ie the first line/the email title. That one only mentions C22).

[...]

>>> +    /// MDIO manageable devices.
>>> +    pub struct Mmd(u8);
>>> +
>>> +    impl Mmd {
>>> +        /// Physical Medium Attachment/Dependent.
>>> +        pub const PMAPMD: Self = Mmd(uapi::MDIO_MMD_PMAPMD as u8);
>>> +        /// WAN interface sublayer.
>>> +        pub const WIS: Self = Mmd(uapi::MDIO_MMD_WIS as u8);
>>> +        /// Physical coding sublayer.
>>> +        pub const PCS: Self = Mmd(uapi::MDIO_MMD_PCS as u8);
>>> +        /// PHY Extender sublayer.
>>> +        pub const PHYXS: Self = Mmd(uapi::MDIO_MMD_PHYXS as u8);
>>> +        /// DTE Extender sublayer.
>>> +        pub const DTEXS: Self = Mmd(uapi::MDIO_MMD_DTEXS as u8);
>>> +        /// Transmission convergence.
>>> +        pub const TC: Self = Mmd(uapi::MDIO_MMD_TC as u8);
>>> +        /// Auto negotiation.
>>> +        pub const AN: Self = Mmd(uapi::MDIO_MMD_AN as u8);
>>> +        /// Clause 22 extension.
>>> +        pub const C22_EXT: Self = Mmd(uapi::MDIO_MMD_C22EXT as u8);
>>> +        /// Vendor specific 1.
>>> +        pub const VEND1: Self = Mmd(uapi::MDIO_MMD_VEND1 as u8);
>>> +        /// Vendor specific 2.
>>> +        pub const VEND2: Self = Mmd(uapi::MDIO_MMD_VEND2 as u8);
>>> +
>>> +        /// Creates a new instance of `Mmd` with a specific device address.
>>> +        pub fn new(devad: u8) -> Self {
>>> +            Mmd(devad)
>>
>> Is there also a range that makes sense to exclude (similar to C22)?
> 
> Not, I think. C22 saves 0x10..0x1f for vendor specific registers. But
> C45 has VEND1 and VEND2 for the same purpose.
> 
> Andrew, we need to allow a driver to use reserved addresses (14-28) in
> C45? If not, we don't new() func to allow a driver to use them.

You can also just leave it out and if some driver needs it at a later
point, you can just add it at that time.

> I overlooked some addresses in 802. I'll add const for all the
> addresses in v3.
> 
> 
>>> +        }
>>> +    }
>>> +
>>> +    /// C45 registers.
>>> +    pub struct C45 {
>>> +        devad: Mmd,
>>> +        regnum: u16,
>>> +    }
>>> +
>>> +    impl C45 {
>>> +        /// Creates a new instance of `C45`.
>>> +        pub fn new(devad: Mmd, regnum: u16) -> Self {
>>
>> I really like that the first argument has a proper type now, do you
>> think that the same is also possible for `regnum`? Or are there no
> 
> I think that it's difficult.
> 
>> common values for `regnum` (that also have the same meaning across
>> devices/drivers)? In that case, keep the `u16`.
> 
> Many are defined in the specification. Drivers might use registers
> undefined in the spec for some reasons, I assume.

I see, well then just use the `u16` and the drivers can define their own
constants.

---
Cheers,
Benno