Re: [PATCH v3 1/2] rust: net::phy unified read/write API for C22 and C45 registers

[Trevor Gross <tmgross@umich.edu>]

On Sun, Jun 2, 2024 at 6:18 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add the unified read/write API for C22 and C45 register. The

s/register/registers

> abstractions support access to only C22 registers now. Instead of
> adding read/write_c45 methods specifically for C45, a new reg module
> supports the unified API to access C22 and C45 registers with trait,
> by calling an appropriate phylib functions.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Sorry for the delays in reviews, this looks pretty good to me so far.
I like the cleanup of moving register logic to its own module.

> diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
> new file mode 100644
> index 000000000000..53a83e69b200
> --- /dev/null
> +++ b/rust/kernel/net/phy/reg.rs
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 FUJITA Tomonori <fujita.tomonori@gmail.com>
> +
> +//! PHY registers.
> +//!
> +//! This module provides an interface for PHY registers defined in IEEE 802.3
> +//! Clause 22 and 45.

Docs suggestion:

    PHY register interfaces.

    This module provides support for accessing PHY registers via Ethernet
    management interface clause 22 and 45, as defined in IEEE 802.3.

> +use super::Device;
> +use crate::build_assert;
> +use crate::error::*;
> +use crate::uapi;
> +
> +/// Accesses PHY registers.
> +///
> +/// This trait is used to implement the unified interface to access
> +/// C22 and C45 PHY registers.
> +pub trait Register {

Since we don't use this anywhere could it be sealed?

This might not even need to be public since it should probably only be
used as a method on `Device` (i.e. `my_device.read(C22::BMCR)` rather
than importing the trait and using `C22::BCMR::read(my_device)`).

> +    /// Reads a PHY register.
> +    fn read(&self, dev: &mut Device) -> Result<u16>;
> +
> +    /// Writes a PHY register.
> +    fn write(&self, dev: &mut Device, val: u16) -> Result;
> +}
> +

> +/// C22 registers.
> +pub struct C22(u8);

Docs suggestion:

    A single MDIO clause 22 register address (5 bits).

> +/// MDIO manageable devices.
> +pub struct Mmd(u8);

> +/// C45 registers.

Docs suggestion

    A single MDIO clause 45 register device and address.

    Clause 45 uses a 5-bit device address to access a specific MMD within a
    port, then a 16-bit register address to access a location within
that device.
    `C45` represents this by storing a [`Mmd`] and a register number.

> +pub struct C45 {
> +    devad: Mmd,
> +    regnum: u16,
> +}

- Trevor