On Fri, 17 Nov 2023 09:39:15 +0000
Alice Ryhl [off-list ref] wrote:

FUJITA Tomonori [off-list ref] writes:

quoted

This is the Rust implementation of drivers/net/phy/ax88796b.c. The
features are equivalent. You can choose C or Rust versionon kernel
configuration.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Benno Lossin <redacted>

Overall looks reasonable. I found various nits below:

There's a typo in your commit message: versionon.

Oops, will fix.

quoted

+use kernel::c_str;
+use kernel::net::phy::{self, DeviceId, Driver};
+use kernel::prelude::*;
+use kernel::uapi;

You used the other import style in other patches.

use kernel::{
    c_str,
    net::phy::{self, DeviceId, Driver},
    prelude::*,
    uapi,
};

?

quoted

+        // If MII_LPA is 0, phy_resolve_aneg_linkmode() will fail to resolve
+        // linkmode so use MII_BMCR as default values.
+        let ret = dev.read(uapi::MII_BMCR as u16)?;
+
+        if ret as u32 & uapi::BMCR_SPEED100 != 0 {

The `ret as u32` and `uapi::MII_BMCR as u16` casts make me think that
these constants are defined as the wrong type?

It's probably difficult to get bindgen to change the type, but you could
do this at the top of the function or file:

Yes, if we could specfy a type with bindgen, it's easier.

	const MII_BMCR: u16 = uapi::MII_BMCR as u16;
	const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;

I'll.

quoted

+            let _ = dev.init_hw();
+            let _ = dev.start_aneg();

Just to confirm: You want to call `start_aneg` even if `init_hw` returns
failure? And you want to ignore both errors?

Yeah, I tried to implement the exact same behavior in the original C driver.


Thanks!