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

FUJITA Tomonori
2023-11-19T09:57:37Z

On Fri, 17 Nov 2023 09:39:15 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

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

Oops, will fix.

>
>> +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,
};

?

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

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