Re: [RFC PATCH v1] rust: net::phy support to C45 registers access

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

Hi,
Thanks for reviewing the patch!

On Thu, 30 May 2024 08:02:35 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>> After the discussion on C45 register support, I implemented a more rusty
>> solution based on Benno's idea (hopefully, I understand correctly):
> 
> Yes this looks similar to what I imagined.

Great.

>> https://lore.kernel.org/all/20240415104701.4772-3-fujita.tomonori@gmail.com/
>> 
>> I'm not sure so I sent a RFC patch to rust-for-linux only. Once I have
>> something that we could agree on, I'll send non RFC patch to netdev.
>> 
>> 
>> Adds support for C45 registers access. C45 registers can be accessed
>> in two ways: either C45 bus protocol or C45 over C22. Normally, a PHY
>> driver shouldn't care how to access. PHYLIB chooses the appropriate
>> one. But there is an exception; PHY hardware supporting only C45 bus
>> protocol.
>> 
>> This adds two ways to access C45; RegC45 and RegC45Direct. Normally, a
>> driver should use the former; let the PHYLIB take care of how to
>> access (by calling phy_read_mmd/phy_write_mmd). The latter calls
>> mdiobus_c45_read/mdiobus_c45_write; C45 over C22 isn't used.
>> 
>> The API to access to C22 registers is also updated to align with the
>> above changes.
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>  drivers/net/phy/ax88796b_rust.rs |   7 +-
>>  rust/kernel/net/phy.rs           | 218 +++++++++++++++++++++++++++----
>>  2 files changed, 198 insertions(+), 27 deletions(-)

(snip)

>> -    /// Writes a given C22 PHY register.
>> -    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
>> -        let phydev = self.0.get();
>> -        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> -        // So it's just an FFI call, open code of `phy_write()` with a valid `phy_device` pointer
>> -        // `phydev`.
>> -        to_result(unsafe {
>> -            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
>> -        })
>> +    /// Writes a PHY register.
>> +    pub fn write<R: RegAccess>(&mut self, reg: R, val: u16) -> Result {
>> +        reg.write(self, val)
>>      }
>> 
>>      /// Reads a paged register.
>> @@ -302,6 +285,195 @@ pub fn genphy_read_abilities(&mut self) -> Result {
>>      }
>>  }
>> 
>> +/// Reads and writes C22 and C45 registers.
> 
> You used "PHY register" below, so you should probably use it here as
> well.

Yeah, fixed.

>> +///
>> +/// This trait is used to implement multiple ways to read and write
>> +/// PHY registers.
>> +pub trait RegAccess {
>> +    /// Reads a PHY register.
>> +    fn read(&self, dev: &mut Device) -> Result<u16>;
> 
> Do all registers have a size of exactly `u16`? If there are also

Yes.

> registers with `u8`, then I would prefer an associated type in the trait
> that you then use as the return type.
> 
>> +
>> +    /// Writes a PHY register.
>> +    fn write(&self, dev: &mut Device, val: u16) -> Result;
>> +}
>> +
>> +/// Accesses to C22 registers.
>> +#[derive(Clone, Copy)]
>> +pub enum RegC22 {
> 
> I think we could just name this `C22`.
> It might also make sense to move all registers into a `reg` module. What
> do you think?

Makes sense. I did both and like them.


>> +impl RegAccess for RegC22 {
>> +    /// Reads a given C22 PHY register.
>> +    fn read(&self, dev: &mut Device) -> Result<u16> {
>> +        let phydev = dev.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
>> +        // So it's just an FFI call, open code of `phy_read()` with a valid `phy_device` pointer
>> +        // `phydev`.
>> +        let ret = unsafe {
>> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, *self as u32)
>> +        };
>> +        if ret < 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(ret as u16)
>> +        }
>> +    }
>> +
>> +    /// Writes a given C22 PHY register.
>> +    fn write(&self, dev: &mut Device, val: u16) -> Result {
>> +        let phydev = dev.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
>> +        // So it's just an FFI call, open code of `phy_write()` with a valid `phy_device` pointer
>> +        // `phydev`.
>> +        to_result(unsafe {
>> +            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, *self as u32, val)
>> +        })
>> +    }
>> +}
>> +
>> +/// Accesses to C45 registers.
>> +///
>> +/// C45 registers can be accessed in two ways: either C45 bus protocol or
>> +/// C45 over C22 bus protocol. Normally, a PHY driver shouldn't care how to
>> +/// access. `PHYLIB` chooses the appropriate one. `RegC45` is preferred over
>> +/// `RegC45Direct`.
>> +pub struct RegC45 {
> 
> Same here, this could just be named `C45`.

Changed.

>> +    devad: u8,
>> +    regnum: u16,
>> +}
>> +
>> +impl RegC45 {
>> +    /// Creates a new instance of `RegC45`.
>> +    pub fn new(devad: u8, regnum: u16) -> Self {
> 
> Are there really no common values that you would use as the parameters
> here? If you want to add any, you can do so via associated constants:
> 
>     const COMMON_REGISTER: RegC45 = RegC45::new(/* ... */);

I think that there are combinations that are often used. But they
aren't defined in such way, I guess. I suppose that the developpers
are familier with a way to access a register with devad and regnum.