Re: [PATCH net-next v7 5/5] net: phy: add Rust Asix PHY driver

Alice Ryhl
2023-11-17T09:39:20Z

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> 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 <benno.lossin@proton.me>

Overall looks reasonable. I found various nits below:

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

> +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.

> + // 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:

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

> + 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?

Alice