Re: [RFC PATCH v2 2/2] rust: net::phy support for C45 genphy helpers

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

On 01.06.24 06:35, FUJITA Tomonori wrote:
> Add support for genphy_c45_* helper functions. Instead of adding
> genphy_c45_ methods, this unifies the API for genphy functions with
> trait.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/net/phy.rs | 49 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 10 deletions(-)
> 
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 286fd5a7ec91..aabd714ae3f6 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -248,16 +248,8 @@ pub fn genphy_suspend(&mut self) -> Result {
>      }
> 
>      /// Checks the link status and updates current link state.
> -    pub fn genphy_read_status(&mut self) -> Result<u16> {
> -        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.
> -        let ret = unsafe { bindings::genphy_read_status(phydev) };
> -        if ret < 0 {
> -            Err(Error::from_errno(ret))
> -        } else {
> -            Ok(ret as u16)
> -        }
> +    pub fn genphy_read_status<G: reg::GenPhyOps>(&mut self) -> Result<u16> {

With this change, don't you also need a change in the ax88796b Rust
driver?

> +        G::read_status(self)
>      }
> 
>      /// Updates the link status.
> @@ -446,6 +438,43 @@ fn write(&self, dev: &mut Device, val: u16) -> Result {
>              })
>          }
>      }
> +
> +    /// PHY helper functions.
> +    ///
> +    /// This trait is used to implement some helper functions executed either
> +    /// via C22 or C45 registers.
> +    pub trait GenPhyOps {

Why does this need another trait? Can't it be part of the `Register`
(currently named `Access`) trait?

---
Cheers,
Benno

> +        /// Checks the link status and updates current link state.
> +        fn read_status(dev: &mut Device) -> Result<u16>;
> +    }
> +
> +    impl GenPhyOps for C22 {
> +        fn read_status(dev: &mut Device) -> Result<u16> {
> +            let phydev = dev.0.get();
> +            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +            // So it's just an FFI call.
> +            let ret = unsafe { bindings::genphy_read_status(phydev) };
> +            if ret < 0 {
> +                Err(Error::from_errno(ret))
> +            } else {
> +                Ok(ret as u16)
> +            }
> +        }
> +    }
> +
> +    impl GenPhyOps for C45 {
> +        fn read_status(dev: &mut Device) -> Result<u16> {
> +            let phydev = dev.0.get();
> +            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +            // So it's just an FFI call.
> +            let ret = unsafe { bindings::genphy_c45_read_status(phydev) };
> +            if ret < 0 {
> +                Err(Error::from_errno(ret))
> +            } else {
> +                Ok(ret as u16)
> +            }
> +        }
> +    }
>  }
> 
>  /// Defines certain other features this PHY supports (like interrupts).
> --
> 2.34.1
>