Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

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

On 09.10.23 15:15, Andrew Lunn wrote:
>>> +        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