On Mon, 2 Oct 2023 17:24:17 +0200
Andrew Lunn [off-list ref] wrote:

quoted

+    /// Gets the id of the PHY.
+    pub fn id(&mut self) -> u32 {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).phy_id }
+    }

I somewhat agree with GregKH here. It will be easier to review and
maintain if the naming of well known things stay the same in the C and
Rust world. So phy_id. However....

phy_id() is fine by me.

The complete type name is `net::phy::Device` so I guess that the
method names usually don't start with `phy`. But we maintain both C
and Rust so I think that we need a balance between them.

quoted

+    /// Gets the state of the PHY.
+    pub fn state(&mut self) -> DeviceState {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let state = unsafe { (*phydev).state };
+        match state {
+            bindings::phy_state::PHY_DOWN => DeviceState::Down,
+            bindings::phy_state::PHY_READY => DeviceState::Ready,
+            bindings::phy_state::PHY_HALTED => DeviceState::Halted,
+            bindings::phy_state::PHY_ERROR => DeviceState::Error,
+            bindings::phy_state::PHY_UP => DeviceState::Up,
+            bindings::phy_state::PHY_RUNNING => DeviceState::Running,
+            bindings::phy_state::PHY_NOLINK => DeviceState::NoLink,
+            bindings::phy_state::PHY_CABLETEST => DeviceState::CableTest,
+        }
+    }
+
+    /// Returns true if the link is up.
+    pub fn get_link(&mut self) -> bool {
+        const LINK_IS_UP: u32 = 1;
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).link() == LINK_IS_UP }
+    }

Naming is hard.

This one is trickier and shows a difference between C and Rust. C just
does phydev->link and treats it as a boolean, setter/getters are not
needed. But Rust does seem to need setter/getters, and it is a lot
less clear what link() does. get_link() is a bit more
obvious. has_link() would also work. But as GregKH said, get_foo() and
put_foo() are often used to represent getting a reference on an object
and releasing it. I am however of the opinion that many driver writers
don't understand locking, so it is best to hide all the locking in the
core. I would not actually expect to see a PHY driver need to take a
reference on anything.

Since we forced into a world of getter/setter, the previous one
probably should be get_phy_id() and we want consistent set_ and get_
prefixes for plain accesses to members without further interpretation.

get/set_something names aren't commonly used in Rust, I guess. Some examples
follows in the standard library.

https://doc.rust-lang.org/stable/std/net/struct.TcpStream.html

there are set_linger(), set_nodelay(), set_read_timeout(),
set_write_timeout(). correspondingly, linger(), nodelay(),
read_timeout(), write_timeout() are provided.

https://doc.rust-lang.org/stable/std/io/struct.Cursor.html

There are set_position() and position().

As I wrote above, I don't think that we need to follow Rust naming
practices strictly, as long as there are patterns in Rust bindings.

quoted

+    /// Returns true if auto-negotiation is enabled.
+    pub fn is_autoneg_enabled(&mut self) -> bool {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
+    }

Should this maybe be get_autoneg_enabled()? I don't know.

I think that we can leave this name alone since tis_something() names
are used for OS related functions in Rust.