On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
[off-list ref] wrote:

Maybe want to link to the C version, just for the crossref?

+    fn read_status(dev: &mut phy::Device) -> Result<u16> {
+        dev.genphy_update_link()?;
+        if !dev.get_link() {
+            return Ok(0);
+        }

Looking at this usage, I think `get_link()` should be renamed to just
`link()`. `get_link` makes me think that it is performing an action
like calling `genphy_update_link`, just `link()` sounds more like a
static accessor.

Or maybe it's worth replacing `get_link` with a `get_updated_link`
that calls `genphy_update_link` and then returns `link`, the user can
store it if they need to reuse it. This seems somewhat less accident
prone than someone calling `.link()`/`.get_link()` repeatedly and
wondering why their phy isn't coming up.

In any case, please make the docs clear about what behavior is
executed and what the preconditions are, it should be clear what's
going to wait for the bus vs. simple field access.

+        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
+            dev.set_speed(100);
+        } else {
+            dev.set_speed(10);
+        }

Speed should probably actually be an enum since it has defined values.
Something like

    #[non_exhaustive]
    enum Speed {
        Speed10M,
        Speed100M,
        Speed1000M,
        // 2.5G, 5G, 10G, 25G?
    }

    impl Speed {
        fn as_mb(self) -> u32;
    }

+        let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
+            phy::DuplexMode::Full
+        } else {
+            phy::DuplexMode::Half
+        };

BMCR_x and MII_x are generated as `u32` but that's just a bindgen
thing. It seems we should reexport them as the correct types so users
don't need to cast all over:

    pub MII_BMCR: u8 = bindings::MII_BMCR as u8;
    pub BMCR_RESV: u16 = bindings::BMCR_RESV as u16; ...
    // (I'd just make a macro for this)

But I'm not sure how to handle that since the uapi crate exposes its
bindings directly. We're probably going to run into this issue with
other uapi items at some point, any thoughts Miguel?

+        dev.genphy_read_lpa()?;

Same question as with the `genphy_update_link`

+    fn link_change_notify(dev: &mut phy::Device) {
+        // Reset PHY, otherwise MII_LPA will provide outdated information.
+        // This issue is reproducible only with some link partner PHYs.
+        if dev.state() == phy::DeviceState::NoLink {
+            let _ = dev.init_hw();
+            let _ = dev.start_aneg();
+        }
+    }
+}

Is it worth doing anything with these errors? I know that the C driver doesn't.

---

The overall driver looks correct to me, most of these comments are
actually about [1/3]

- Trevor

[1]: https://lore.kernel.org/rust-for-linux/baa4cc4b-bcde-4b02-9286-c923404db972@lunn.ch/ (local)