[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231129195145.69ff5cf6.gary@garyguo.net>
Date: Wed, 29 Nov 2023 19:51:45 +0000
From: Gary Guo <gary@...yguo.net>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch,
tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, benno.lossin@...ton.me,
wedsonaf@...il.com, aliceryhl@...gle.com, boqun.feng@...il.com
Subject: Re: [PATCH net-next v8 1/4] rust: core abstractions for network PHY
drivers
On Thu, 23 Nov 2023 14:04:09 +0900
FUJITA Tomonori <fujita.tomonori@...il.com> wrote:
> + /// Reads a given C22 PHY register.
> + // This function reads a hardware register and updates the stats so takes `&mut self`.
> + pub fn read(&mut self, regnum: u16) -> Result<u16> {
> + let phydev = self.0.get();
> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> + // So an FFI call with a valid pointer.
> + let ret = unsafe {
> + bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> + };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(ret as u16)
> + }
> + }
> +
> + /// Writes a given C22 PHY register.
> + pub fn write(&mut self, regnum: u16, val: u16) -> Result {
> + let phydev = self.0.get();
> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> + // So an FFI call with a valid pointer.
> + to_result(unsafe {
> + bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
> + })
> + }
`read` and `write` are not very distinctive names, especially when
`read_paged` exists.
Maybe `mdio_{read,write}`?
> +/// Defines certain other features this PHY supports (like interrupts).
> +///
> +/// These flag values are used in [`Driver::FLAGS`].
> +pub mod flags {
> + /// PHY is internal.
> + pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL;
> + /// PHY needs to be reset after the refclk is enabled.
> + pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN;
> + /// Polling is used to detect PHY status changes.
> + pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST;
> + /// Don't suspend.
> + pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND;
> +}
> +
> +/// An adapter for the registration of a PHY driver.
> +struct Adapter<T: Driver> {
> + _p: PhantomData<T>,
> +}
> +
> +impl<T: Driver> Adapter<T> {
> + /// # Safety
> + ///
> + /// `phydev` must be passed by the corresponding callback in `phy_driver`.
> + unsafe extern "C" fn soft_reset_callback(
> + phydev: *mut bindings::phy_device,
> + ) -> core::ffi::c_int {
> + from_result(|| {
> + // SAFETY: This callback is called only in contexts
> + // where we hold `phy_device->lock`, so the accessors on
> + // `Device` are okay to call.
> + let dev = unsafe { Device::from_raw(phydev) };
> + T::soft_reset(dev)?;
Usually we want type safety by to the callback typed access to the
device's driver-private data, rather than just give it an arbitrary
`Device`. Any reason not to similar things here?
> + Ok(0)
> + })
> + }
> +}
> +
Powered by blists - more mailing lists