Message-ID: <e7e8c93ff4eb9c56eb412a9856947409a26eade1.camel@posteo.de>
Date: Mon, 17 Nov 2025 16:13:33 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Igor Korotin <igor.korotin.linux@...il.com>, Miguel Ojeda
 <ojeda@...nel.org>,  Alex Gaynor <alex.gaynor@...il.com>, Wolfram Sang
 <wsa+renesas@...g-engineering.com>
Cc: Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
 Björn Roy Baron	 <bjorn3_gh@...tonmail.com>, Benno
 Lossin <lossin@...nel.org>, Andreas Hindborg	 <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross	 <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...nel.org>, Greg Kroah-Hartman	
 <gregkh@...uxfoundation.org>, Viresh Kumar <viresh.kumar@...aro.org>, Asahi
 Lina <lina+kernel@...hilina.net>, Wedson Almeida Filho
 <wedsonaf@...il.com>, Alex Hung	 <alex.hung@....com>, Tamir Duberstein
 <tamird@...il.com>, Xiangfei Ding	 <dingxiangfei2009@...il.com>,
 linux-kernel@...r.kernel.org, 	rust-for-linux@...r.kernel.org,
 linux-i2c@...r.kernel.org
Subject: Re: [PATCH v8 2/4] rust: i2c: add manual I2C device creation
 abstractions

On Sun, 2025-11-16 at 16:21 +0000, Igor Korotin wrote:
> In addition to the basic I2C device support, add rust abstractions
> upon `i2c_new_client_device`/`i2c_unregister_device` C functions.
> 
> Implement the core abstractions needed for manual creation/deletion
> of I2C devices, including:
> 
>  * `i2c::Registration` — a NonNull pointer created by the function
>                           `i2c_new_client_device`
> 
>  * `i2c::I2cAdapter` — a ref counted wrapper around `struct i2c_adapter`
> 
>  * `i2c::I2cBoardInfo` — a safe wrapper around `struct i2c_board_info`
> 
> Signed-off-by: Igor Korotin <igor.korotin.linux@...il.com>
> ---
>  rust/kernel/i2c.rs | 155 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 154 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> index a5ad8213d9ac..3695b4adf436 100644
> --- a/rust/kernel/i2c.rs
> +++ b/rust/kernel/i2c.rs
> @@ -11,6 +11,7 @@
>          RawDeviceId,
>          RawDeviceIdIndex, //
>      },
> +    devres::Devres,
>      driver,
>      error::*,
>      of,
> @@ -23,9 +24,14 @@
>  
>  use core::{
>      marker::PhantomData,
> -    ptr::NonNull, //
> +    ptr::{
> +        from_ref,
> +        NonNull, //
> +    }, //
>  };
>  
> +use kernel::types::ARef;
> +
>  /// An I2C device id table.
>  #[repr(transparent)]
>  #[derive(Clone, Copy)]
> @@ -356,6 +362,102 @@ fn unbind(dev: &I2cClient<device::Core>, this: Pin<&Self>) {
>      }
>  }
>  
> +/// The i2c adapter representation.
> +///
> +/// This structure represents the Rust abstraction for a C `struct i2c_adapter`. The
> +/// implementation abstracts the usage of an existing C `struct i2c_adapter` that
> +/// gets passed from the C side
> +///
> +/// # Invariants
> +///
> +/// A [`I2cAdapter`] instance represents a valid `struct i2c_adapter` created by the C portion of
> +/// the kernel.
> +#[repr(transparent)]
> +pub struct I2cAdapter<Ctx: device::DeviceContext = device::Normal>(
> +    Opaque<bindings::i2c_adapter>,
> +    PhantomData<Ctx>,
> +);
> +
> +impl<Ctx: device::DeviceContext> I2cAdapter<Ctx> {
> +    fn as_raw(&self) -> *mut bindings::i2c_adapter {
> +        self.0.get()
> +    }
> +}
> +
> +impl I2cAdapter {
> +    /// Returns the I2C Adapter index.
> +    #[inline]
> +    pub fn index(&self) -> i32 {
> +        // SAFETY: `self.as_raw` is a valid pointer to a `struct i2c_adapter`.
> +        unsafe { (*self.as_raw()).nr }
> +    }
> +
> +    /// Gets pointer to an `i2c_adapter` by index.
> +    pub fn get(index: i32) -> Result<ARef<Self>> {
> +        // SAFETY: `index` must refer to a valid I2C adapter; the kernel
> +        // guarantees that `i2c_get_adapter(index)` returns either a valid
> +        // pointer or NULL. `NonNull::new` guarantees the correct check.
> +        let adapter = NonNull::new(unsafe { bindings::i2c_get_adapter(index) }).ok_or(ENODEV)?;
> +
> +        // SAFETY: `adapter` is non-null and points to a live `i2c_adapter`.
> +        // `I2cAdapter` is #[repr(transparent)], so this cast is valid.
> +        Ok(unsafe { (&*adapter.as_ptr().cast::<I2cAdapter<device::Normal>>()).into() })
> +    }
> +}
> +
> +// SAFETY: `I2cAdapter` is a transparent wrapper of a type that doesn't depend on
> +// `I2cAdapter`'s generic argument.
> +kernel::impl_device_context_deref!(unsafe { I2cAdapter });
> +kernel::impl_device_context_into_aref!(I2cAdapter);
> +
> +// SAFETY: Instances of `I2cAdapter` are always reference-counted.
> +unsafe impl crate::types::AlwaysRefCounted for I2cAdapter {
> +    fn inc_ref(&self) {
> +        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> +        unsafe { bindings::i2c_get_adapter(self.index()) };
> +    }
> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> +        unsafe { bindings::i2c_put_adapter(obj.as_ref().as_raw()) }
> +    }
> +}
> +
> +/// The i2c board info representation
> +///
> +/// This structure represents the Rust abstraction for a C `struct i2c_board_info` structure,
> +/// which is used for manual I2C client creation.
> +#[repr(transparent)]
> +pub struct I2cBoardInfo(bindings::i2c_board_info);
> +
> +impl I2cBoardInfo {
> +    const I2C_TYPE_SIZE: usize = 20;
> +    /// Create a new [`I2cBoardInfo`] for a kernel driver.
> +    #[inline(always)]
> +    pub const fn new(type_: &'static CStr, addr: u16) -> Self {
> +        build_assert!(
> +            type_.len_with_nul() <= Self::I2C_TYPE_SIZE,
> +            "Type exceeds 20 bytes"
> +        );
> +        let src = type_.as_bytes_with_nul();
> +        // Replace with `bindings::i2c_board_info::default()` once stabilized for `const`.
> +        // SAFETY: FFI type is valid to be zero-initialized.
> +        let mut i2c_board_info: bindings::i2c_board_info = pin_init::zeroed();
Same clippy error here:

error: statement has unnecessary safety comment
   --> rust/kernel/i2c.rs:446:9
    |
446 |         let mut i2c_board_info: bindings::i2c_board_info =
pin_init::zeroed();
    |        
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: consider removing the safety comment
   --> rust/kernel/i2c.rs:445:9
    |
445 |         // SAFETY: FFI type is valid to be zero-initialized.
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: for further information visit
https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#unnecessary_safety_comment

Thanks
- Markus Probst

> +        let mut i: usize = 0;
> +        while i < src.len() {
> +            i2c_board_info.type_[i] = src[i];
> +            i += 1;
> +        }
> +
> +        i2c_board_info.addr = addr;
> +        Self(i2c_board_info)
> +    }
> +
> +    fn as_raw(&self) -> *const bindings::i2c_board_info {
> +        from_ref(&self.0)
> +    }
> +}
> +
>  /// The i2c client representation.
>  ///
>  /// This structure represents the Rust abstraction for a C `struct i2c_client`. The
> @@ -434,3 +536,54 @@ unsafe impl Send for I2cClient {}
>  // SAFETY: `I2cClient` can be shared among threads because all methods of `I2cClient`
>  // (i.e. `I2cClient<Normal>) are thread safe.
>  unsafe impl Sync for I2cClient {}
> +
> +/// The registration of an i2c client device.
> +///
> +/// This type represents the registration of a [`struct i2c_client`]. When an instance of this
> +/// type is dropped, its respective i2c client device will be unregistered from the system.
> +///
> +/// # Invariants
> +///
> +/// `self.0` always holds a valid pointer to an initialized and registered
> +/// [`struct i2c_client`].
> +#[repr(transparent)]
> +pub struct Registration(NonNull<bindings::i2c_client>);
> +
> +impl Registration {
> +    /// The C `i2c_new_client_device` function wrapper for manual I2C client creation.
> +    pub fn new<'a>(
> +        i2c_adapter: &I2cAdapter,
> +        i2c_board_info: &I2cBoardInfo,
> +        parent_dev: &'a device::Device<device::Bound>,
> +    ) -> impl PinInit<Devres<Self>, Error> + 'a {
> +        Devres::new(parent_dev, Self::try_new(i2c_adapter, i2c_board_info))
> +    }
> +
> +    fn try_new(i2c_adapter: &I2cAdapter, i2c_board_info: &I2cBoardInfo) -> Result<Self> {
> +        // SAFETY: the kernel guarantees that `i2c_new_client_device()` returns either a valid
> +        // pointer or NULL. `from_err_ptr` separates errors. Following `NonNull::new`
> +        // checks for NULL.
> +        let raw_dev = from_err_ptr(unsafe {
> +            bindings::i2c_new_client_device(i2c_adapter.as_raw(), i2c_board_info.as_raw())
> +        })?;
> +
> +        let dev_ptr = NonNull::new(raw_dev).ok_or(ENODEV)?;
> +
> +        Ok(Self(dev_ptr))
> +    }
> +}
> +
> +impl Drop for Registration {
> +    fn drop(&mut self) {
> +        // SAFETY: `Drop` is only called for a valid `Registration`, which by invariant
> +        // always contains a non-null pointer to an `i2c_client`.
> +        unsafe { bindings::i2c_unregister_device(self.0.as_ptr()) }
> +    }
> +}
> +
> +// SAFETY: A `Registration` of a `struct i2c_client` can be released from any thread.
> +unsafe impl Send for Registration {}
> +
> +// SAFETY: `Registration` offers no interior mutability (no mutation through &self
> +// and no mutable access is exposed)
> +unsafe impl Sync for Registration {}

Powered by blists - more mailing lists