On 09.10.23 15:15, Andrew Lunn wrote:

quoted

quoted

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

Maybe refactor to only have one `dev.set_speed` call?

This is a common pattern in the C code. This is basically a
re-implementation of

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L2432

because this PHY is broken. Being one of the maintainers of the PHY
subsystem, it helps me review this code if it happens to look like the
existing code it is adding a workaround to.

Is there a Rust reason to only have one call?

My reason was consistency, since the call to `set_duplex`
below that was changed to only have one call:

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

I think it should be consistent, I chose to reduce the number of
function calls, since it is immediately obvious that only the argument
is depending on the condition. But if you think it should mirror the C
side, then maybe change the duplex back to calling twice?

-- 
Cheers,
Benno