Re: [PATCH net-next v2 3/3] net: phy: add Rust reference driver for ET1011C

[Andrew Lunn]

What board are you testing this on? This is a pretty old PHY, and
nobody has done anything with it for a long time.

> +const BMCR_FULLDPLX: u16 = uapi::BMCR_FULLDPLX as u16;
> +const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;
> +const BMCR_SPEED1000: u16 = uapi::BMCR_SPEED1000 as u16;
> +const BMCR_ANENABLE: u16 = uapi::BMCR_ANENABLE as u16;
> +const BMCR_RESET: u16 = uapi::BMCR_RESET as u16;

ax88796b_rust.rs also has:

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

Can these be shared somehow?

> +impl Driver for PhyET1011C {
> +    const NAME: &'static CStr = c"ET1011C";
> +    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_model_mask(0x0282f014);
> +
> +    fn config_aneg(dev: &mut phy::Device) -> Result {
> +        let ctl = dev.read(C22::BMCR)?;
> +        let ctl = ctl & !(BMCR_FULLDPLX | BMCR_SPEED100 | BMCR_SPEED1000 | BMCR_ANENABLE);

It is pretty unusual to clear these bits in BMCR. BMCR_FULLDPLX,
BMCR_SPEED100, BMCR_SPEED1000 are only used when autoneg is
disabled. genphy_setup_forced() will set them, if
needed. BMCR_ANENABLE will be set when auto-neg is restarted.

Is there any hint in the datasheet why this is needed?


> +        dev.write(C22::BMCR, ctl | BMCR_RESET)?;

BMCR_RESET is a self clearly bit, and you should wait for it to
clear. genphy_soft_reset() does the polling. But before you can swap
to this, you need to understand why the other bits are being cleared.

Given how old this C driver is, i'm not sure it is using best
practices. So you really should be looking at every line, is it
correct in todays phylib framework? Should it be done differently in a
modern driver? We don't want a 1:1 copy in rust, we want something
which is up to date.

     Andrew