Re: [PATCH net-next v3 4/6] rust: net::phy unified read/write API for C22 and C45 registers

[Andrew Lunn <andrew@lunn.ch>]

> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 FUJITA Tomonori <fujita.tomonori@gmail.com>
> +
> +//! PHY register interfaces.
> +//!
> +//! This module provides support for accessing PHY registers via Ethernet
> +//! management interface clause 22 and 45, as defined in IEEE 802.3.

Here we need to be very careful. The word `via` in the sentence above
means we are talking about the access mechanism, c22 transfers, or c45
transfers. A PHY driver should not care about the transfer mechanism,
it should just care about the register in the C22 or C45 register
namespace.

> +impl Register for C22 {
> +    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.0.into())
> +        };
> +        to_result(ret)?;
> +        Ok(ret as u16)
> +    }
> +
> +    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.0.into(), val)
> +        })

These two are O.K. You have to use C22 bus transfers to access the C22
register namespace.

> +impl Register for C45 {
> +    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.
> +        let ret =
> +            unsafe { bindings::phy_read_mmd(phydev, self.devad.0.into(), self.regnum.into()) };
> +        to_result(ret)?;
> +        Ok(ret as u16)
> +    }
> +
> +    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.
> +        to_result(unsafe {
> +            bindings::phy_write_mmd(phydev, self.devad.0.into(), self.regnum.into(), val)
> +        })
> +    }

And these are also O.K. There are two mechanisms to access the C45
register namespace. By calling phy_write_mmd()/phy_write_mmd() you are
leaving it upto the core to decide which mechanism to use. The driver
itself does not care.

So the problem is with the comment above. It would be better to say
something like:

This module provides support for accessing PHY registers in the
Ethernet management interface clauses 22 and 45 register namespaces, as
defined in IEEE 802.3.

Dropping the via, and adding register namespace should make it clear
we are talking about the registers themselves, not how you access
them.


    Andrew

---
pw-bot: cr