rust: i2c: Add basic I2C driver abstractions

Implement the core abstractions needed for I2C drivers, including:

* i2c::Driver — the trait drivers must implement, including probe

* i2c::Device — a safe wrapper around struct i2c_client

* i2c::Adapter — implements driver::RegistrationOps to hook into the generic driver::Registration machinery

* i2c::DeviceId — a RawDeviceId implementation for I2C device IDs

Signed-off-by Igor Korotin <[email protected]>

---
 MAINTAINERS                     |   2 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/i2c.c              |  15 ++
 rust/kernel/i2c.rs              | 391 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   2 +
 6 files changed, 412 insertions(+)
 create mode 100644 rust/helpers/i2c.c
 create mode 100644 rust/kernel/i2c.rs

MAINTAINERS +0 -0

index 7f8ddeec3b17..688a0ff23e69 100644

MAINTAINERS +0 -0

 I2C SUBSYSTEM HOST DRIVERS
 M:	Andi Shyti <[email protected]>

rust/bindings/bindings_helper.h +1 -0

index 7e8f22850647..efc9be4d9a6e 100644
@@ -53,6 +53,7 @@
 #include <linux/file.h>
 #include <linux/firmware.h>
 #include <linux/fs.h>
+#include <linux/i2c.h>
 #include <linux/jiffies.h>
 #include <linux/jump_label.h>
 #include <linux/mdio.h>

rust/helpers/helpers.c +1 -0

index 0b09bd0e3561..be34554b3fab 100644
@@ -23,6 +23,7 @@
 #include "drm.c"
 #include "err.c"
 #include "fs.c"
+#include "i2c.c"
 #include "io.c"
 #include "jump_label.c"
 #include "kunit.c"

rust/helpers/i2c.c new file +3 -0

new file mode 100644
index 000000000000..5f384f8f560e
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/i2c.h>

rust/kernel/i2c.rs new file +52 -0

new file mode 100644
index 000000000000..4f2f3c378153
@@ -0,0 +1,391 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! I2C Driver subsystem
+
+// I2C Driver abstractions.
+use crate::{
+    acpi, container_of, device,
+    device_id::RawDeviceId,
+    driver,
+    error::*,
+    of,
+    prelude::*,
+    types::{ForeignOwnable, Opaque},
+};
+
+use core::{
+    marker::PhantomData,
+    ptr::{addr_of_mut, NonNull},
+};
+
+/// An I2C device id table.
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+pub struct DeviceId(bindings::i2c_device_id);
+
+impl DeviceId {
+    const I2C_NAME_SIZE: usize = 20;
+
+    /// Create a new device id from an I2C 'id' string.
+    #[inline(always)]
+    pub const fn new(id: &'static CStr) -> Self {
+        build_assert!(
+            id.len_with_nul() <= Self::I2C_NAME_SIZE,
+            "ID exceeds 20 bytes"
+        );
+        let src = id.as_bytes_with_nul();
+        // Replace with `bindings::acpi_device_id::default()` once stabilized for `const`.
+        // SAFETY: FFI type is valid to be zero-initialized.
+        let mut i2c: bindings::i2c_device_id = unsafe { core::mem::zeroed() };
+        let mut i = 0;
+        while i < src.len() {
+            i2c.name[i] = src[i];
+            i += 1;
+        }
+
+        Self(i2c)
+    }
+}
+
+// SAFETY:
+// * `DeviceId` is a `#[repr(transparent)` wrapper of `i2c_device_id` and does not add
+//   additional invariants, so it's safe to transmute to `RawType`.

rust/kernel/i2c.rs new file +73 -0

+/// IdTable type for I2C
+pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
+
+/// Create a I2C `IdTable` with its alias for modpost.
+#[macro_export]
+macro_rules! i2c_device_table {
+    ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => {
+        const $table_name: $crate::device_id::IdArray<
+            $crate::i2c::DeviceId,
+            $id_info_type,
+            { $table_data.len() },
+        > = $crate::device_id::IdArray::new($table_data);
+
+        $crate::module_device_table!("i2c", $module_table_name, $table_name);
+    };
+}
+
+/// An adapter for the registration of I2C drivers.
+pub struct Adapter<T: Driver>(T);
+
+// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// a preceding call to `register` has been successful.
+unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
+    type RegType = bindings::i2c_driver;
+
+    unsafe fn register(
+        pdrv: &Opaque<Self::RegType>,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result {
+        let i2c_table = match T::I2C_ID_TABLE {
+            Some(table) => table.as_ptr(),
+            None => core::ptr::null(),
+        };
+
+        let of_table = match T::OF_ID_TABLE {
+            Some(table) => table.as_ptr(),
+            None => core::ptr::null(),
+        };
+
+        let acpi_table = match T::ACPI_ID_TABLE {
+            Some(table) => table.as_ptr(),
+            None => core::ptr::null(),
+        };
+
+        // SAFETY: It's safe to set the fields of `struct i2c_client` on initialization.
+        unsafe {
+            (*pdrv.get()).driver.name = name.as_char_ptr();
+            (*pdrv.get()).probe = Some(Self::probe_callback);
+            (*pdrv.get()).remove = Some(Self::remove_callback);
+            (*pdrv.get()).shutdown = Some(Self::shutdown_callback);
+            (*pdrv.get()).id_table = i2c_table;
+            (*pdrv.get()).driver.of_match_table = of_table;
+            (*pdrv.get()).driver.acpi_match_table = acpi_table;
+        }
+
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        to_result(unsafe { bindings::i2c_register_driver(module.0, pdrv.get()) })
+    }
+
+    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        unsafe { bindings::i2c_del_driver(pdrv.get()) }
+    }
+}
+
+impl<T: Driver + 'static> Adapter<T> {
+    extern "C" fn probe_callback(pdev: *mut bindings::i2c_client) -> kernel::ffi::c_int {
+        // SAFETY: The I2C bus only ever calls the probe callback with a valid pointer to a
+        // `struct i2c_client`.
+        //
+        // INVARIANT: `pdev` is valid for the duration of `probe_callback()`.
+        let pdev = unsafe { &*pdev.cast::<Device<device::Core>>() };

rust/kernel/i2c.rs new file +18 -0

+    }
+
+    extern "C" fn shutdown_callback(pdev: *mut bindings::i2c_client) {
+        let pdev = unsafe { &*pdev.cast::<Device<device::Core>>() };
+
+        T::shutdown(pdev);
+    }
+
+    /// The [`i2c::IdTable`] of the corresponding driver.
+    fn i2c_id_table() -> Option<IdTable<<Self as driver::Adapter>::IdInfo>> {
+        T::I2C_ID_TABLE
+    }
+
+    /// Returns the driver's private data from the matching entry in the [`i2c::IdTable`], if any.
+    ///
+    /// If this returns `None`, it means there is no match with an entry in the [`i2c::IdTable`].
+    fn i2c_id_info(dev: &Device) -> Option<&'static <Self as driver::Adapter>::IdInfo> {
+        let table = Self::i2c_id_table()?;

rust/kernel/i2c.rs new file +113 -0

+            // does not add additional invariants, so it's safe to transmute.
+            let id = unsafe { &*raw_id.cast::<DeviceId>() };
+
+            Some(table.info(<DeviceId as crate::device_id::RawDeviceId>::index(id)))
+        }
+    }
+}
+
+impl<T: Driver + 'static> driver::Adapter for Adapter<T> {
+    type IdInfo = T::IdInfo;
+
+    fn of_id_table() -> Option<of::IdTable<Self::IdInfo>> {
+        T::OF_ID_TABLE
+    }
+
+    fn acpi_id_table() -> Option<acpi::IdTable<Self::IdInfo>> {
+        T::ACPI_ID_TABLE
+    }
+}
+
+/// Declares a kernel module that exposes a single i2c driver.
+///
+/// # Examples
+///
+/// ```ignore
+/// kernel::module_i2c_driver! {
+///     type: MyDriver,
+///     name: "Module name",
+///     authors: ["Author name"],
+///     description: "Description",
+///     license: "GPL v2",
+/// }
+/// ```
+#[macro_export]
+macro_rules! module_i2c_driver {
+    ($($f:tt)*) => {
+        $crate::module_driver!(<T>, $crate::i2c::Adapter<T>, { $($f)* });
+    };
+}
+
+/// The i2c driver trait.
+///
+/// Drivers must implement this trait in order to get a i2c driver registered.
+///
+/// # Example
+///
+///```
+/// # use kernel::{acpi, bindings, c_str, device::Core, i2c, of};
+///
+/// struct MyDriver;
+///
+/// kernel::acpi_device_table!(
+///     ACPI_TABLE,
+///     MODULE_ACPI_TABLE,
+///     <MyDriver as i2c::Driver>::IdInfo,
+///     [
+///         (acpi::DeviceId::new(c_str!("LNUXBEEF")), ())
+///     ]
+/// );
+///
+/// kernel::i2c_device_table!(
+///     I2C_TABLE,
+///     MODULE_I2C_TABLE,
+///     <MyDriver as i2c::Driver>::IdInfo,
+///     [
+///          (i2c::DeviceId::new(c_str!("rust_driver_i2c")), ())
+///     ]
+/// );
+///
+/// kernel::of_device_table!(
+///     OF_TABLE,
+///     MODULE_OF_TABLE,
+///     <MyDriver as i2c::Driver>::IdInfo,
+///     [
+///         (of::DeviceId::new(c_str!("test,device")), ())
+///     ]
+/// );
+///
+/// impl i2c::Driver for MyDriver {
+///     type IdInfo = ();
+///     const I2C_ID_TABLE: Option<i2c::IdTable<Self::IdInfo>> = Some(&I2C_TABLE);
+///     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+///     const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
+///
+///     fn probe(
+///         _pdev: &i2c::Device<Core>,
+///         _id_info: Option<&Self::IdInfo>,
+///     ) -> Result<Pin<KBox<Self>>> {
+///         Err(ENODEV)
+///     }
+///
+///     fn shutdown(_pdev: &i2c::Device<Core>) {
+///     }
+/// }
+///```
+pub trait Driver: Send {
+    /// The type holding information about each device id supported by the driver.
+    // TODO: Use `associated_type_defaults` once stabilized:
+    //
+    // ```
+    // type IdInfo: 'static = ();
+    // ```
+    type IdInfo: 'static;
+
+    /// The table of device ids supported by the driver.
+    const I2C_ID_TABLE: Option<IdTable<Self::IdInfo>> = None;
+
+    /// The table of OF device ids supported by the driver.
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
+
+    /// The table of ACPI device ids supported by the driver.
+    const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = None;
+

rust/kernel/i2c.rs new file +82 -0

+    fn shutdown(dev: &Device<device::Core>) {
+        let _ = dev;
+    }
+}
+
+/// The i2c client representation.
+///
+/// This structure represents the Rust abstraction for a C `struct i2c_client`. The
+/// implementation abstracts the usage of an already existing C `struct i2c_client` within Rust
+/// code that we get passed from the C side.
+///
+/// # Invariants
+///
+/// A [`Device`] instance represents a valid `struct i2c_client` created by the C portion of
+/// the kernel.
+#[repr(transparent)]
+pub struct Device<Ctx: device::DeviceContext = device::Normal>(
+    Opaque<bindings::i2c_client>,
+    PhantomData<Ctx>,
+);
+
+impl<Ctx: device::DeviceContext> Device<Ctx> {
+    fn as_raw(&self) -> *mut bindings::i2c_client {
+        self.0.get()
+    }
+}
+
+// SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic
+// argument.
+kernel::impl_device_context_deref!(unsafe { Device });
+kernel::impl_device_context_into_aref!(Device);
+
+// SAFETY: Instances of `Device` are always reference-counted.
+unsafe impl crate::types::AlwaysRefCounted for Device {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
+        unsafe { bindings::get_device(self.as_ref().as_raw()) };
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::put_device(addr_of_mut!((*obj.as_ref().as_raw()).dev)) }
+    }
+}
+
+impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
+    fn as_ref(&self) -> &device::Device<Ctx> {
+        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+        // `struct i2c_client`.
+        let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
+
+        // SAFETY: `dev` points to a valid `struct device`.
+        unsafe { device::Device::as_ref(dev) }
+    }
+}
+
+impl<Ctx: device::DeviceContext> TryFrom<&device::Device<Ctx>> for &Device<Ctx> {
+    type Error = kernel::error::Error;
+
+    fn try_from(dev: &device::Device<Ctx>) -> Result<Self, Self::Error> {
+        // SAFETY: By the type invariant of `Device`, `dev.as_raw()` is a valid pointer to a
+        // `struct device`.
+        if unsafe { bindings::i2c_verify_client(dev.as_raw()).is_null() } {
+            return Err(EINVAL);
+        }
+
+        // SAFETY: We've just verified that the type of `dev` equals to
+        // `bindings::i2c_client_type`, hence `dev` must be embedded in a valid
+        // `struct i2c_client` as guaranteed by the corresponding C code.
+        let pdev = unsafe { container_of!(dev.as_raw(), bindings::i2c_client, dev) };
+
+        // SAFETY: `pdev` is a valid pointer to a `struct i2c_client`.
+        Ok(unsafe { &*pdev.cast() })
+    }
+}
+
+// SAFETY: A `Device` is always reference-counted and can be released from any thread.
+unsafe impl Send for Device {}
+
+// SAFETY: `Device` can be shared among threads because all methods of `Device`
+// (i.e. `Device<Normal>) are thread safe.
+unsafe impl Sync for Device {}

rust/kernel/lib.rs +0 -0

index 5bbf3627212f..ee1233e44a0f 100644

rust/kernel/lib.rs +0 -0

 pub mod init;
 pub mod io;
 pub mod ioctl;

Signature · 1 line

2.43.0

Quoted 28 lines · Igor Korotin

Implement the core abstractions needed for I2C drivers, including:

* i2c::Driver — the trait drivers must implement, including probe

* i2c::Device — a safe wrapper around struct i2c_client

* i2c::Adapter — implements driver::RegistrationOps to hook into the generic driver::Registration machinery

* i2c::DeviceId — a RawDeviceId implementation for I2C device IDs

Signed-off-by Igor Korotin <[email protected]>

---
 MAINTAINERS                     |   2 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/i2c.c              |  15 ++
 rust/kernel/i2c.rs              | 391 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   2 +
 6 files changed, 412 insertions(+)
 create mode 100644 rust/helpers/i2c.c
 create mode 100644 rust/kernel/i2c.rs

MAINTAINERS +0 -0

index 7f8ddeec3b17..688a0ff23e69 100644

MAINTAINERS F: include/linux/i2c-smbus.h · line 11367

Quoted 5 lines

MAINTAINERS +2 -0

@@ -11363,6 +11363,8 @@ F:	include/linux/i2c-smbus.h
 F:	include/linux/i2c.h
 F:	include/uapi/linux/i2c-*.h
 F:	include/uapi/linux/i2c.h
+F:	rust/helpers/i2c.c
+F:	rust/kernel/i2c.rs

Is this agreed with the maintainer?

There are multiple possible options, for instance:

1) Maintain the Rust abstractions as part of the existing MAINTAINERS entry. Optionally, the author can be added as another maintainer or reviewer.

2) Add a separate MAINTAINERS entry; patches still go through the same subsystem tree.

3) Add a separate MAINTAINERS entry; patches don't go through the subsystem tree (e.g. because the subsystem maintainers don't want to deal with it).

I usually recommend (1), which is what is proposes here, but that's of course up to you and the I2C maintainer.

@Wolfram: In case there's no agreement yet, what's your preference of maintainership for this?

Quoted 18 lines

MAINTAINERS +18 -0

+    }
+
+    extern "C" fn shutdown_callback(pdev: *mut bindings::i2c_client) {
+        let pdev = unsafe { &*pdev.cast::<Device<device::Core>>() };
+
+        T::shutdown(pdev);
+    }
+
+    /// The [`i2c::IdTable`] of the corresponding driver.
+    fn i2c_id_table() -> Option<IdTable<<Self as driver::Adapter>::IdInfo>> {
+        T::I2C_ID_TABLE
+    }
+
+    /// Returns the driver's private data from the matching entry in the [`i2c::IdTable`], if any.
+    ///
+    /// If this returns `None`, it means there is no match with an entry in the [`i2c::IdTable`].
+    fn i2c_id_info(dev: &Device) -> Option<&'static <Self as driver::Adapter>::IdInfo> {
+        let table = Self::i2c_id_table()?;

MAINTAINERS F: include/linux/i2c-smbus.h

Quoted 10 lines

MAINTAINERS +10 -0

+
+        // SAFETY:
+        // - `table` has static lifetime, hence it's valid for read,
+        // - `dev` is guaranteed to be valid while it's alive, and so is `pdev.as_ref().as_raw()`.
+        let raw_id = unsafe { bindings::i2c_match_id(table.as_ptr(), dev.as_raw()) };
+
+        if raw_id.is_null() {
+            None
+        } else {
+            // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct i2c_device_id` and

Nit: Missing ']', probably a copy-paste mistake from other busses.

Yes? :)

Just a note, this won't be needed in the future, I have patches in the

that let you use generic accessors from the generic Device type. [1]

[1] https://lore.kernel.org/lkml/[email protected]/

Just a heads-up, this trait will be split up. [2]

[2]

Better use from_result() here.

I like to do that as well, but I usually get asked to use drop()

do that here as well. :)

For now I think you have to ensure that I2C is built-in.

As for being added as a reviewer or co-maintainer, I’m not yet confident in my Rust skills. I’m learning Rust from scratch and, given my extensive C-kernel background, I thought I’d start by contributing something useful to the Rust side.

Quoted 5 lines · Igor Korotin

As for being added as a reviewer or co-maintainer, I’m not yet confident in my Rust skills. I’m learning Rust from scratch and, given my extensive C-kernel background, I thought I’d start by contributing something useful to the Rust side.

At the moment, for any given subsystem, it is possible that maintainers have even less Rust experience than you do :)

In general, it never hurts to offer to help with maintenance -- it shows you are committed to the code you want to add etc.

Quoted 7 lines

rust/kernel/i2c.rs new file +7 -0

+
+        0
+    }
+
+    extern "C" fn remove_callback(pdev: *mut bindings::i2c_client) {
+        // SAFETY: `pdev` is a valid pointer to a `struct i2c_client`.
+        let ptr = unsafe { bindings::i2c_get_clientdata(pdev) }.cast();

rust/kernel/i2c.rs line 158

Quoted 5 lines

rust/kernel/i2c.rs new file +5 -0

+
+        // SAFETY: `remove_callback` is only ever called after a successful call to
+        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
+        // `KBox<T>` pointer created through `KBox::into_foreign`.
+        let _ = unsafe { KBox::<T>::from_foreign(ptr) };

I like to do that as well, but I usually get asked to use drop() instead, let's do that here as well. :)

Quoted 7 lines · Danilo Krummrich

Quoted 7 lines

rust/kernel/i2c.rs new file +7 -0

+
+        0
+    }
+
+    extern "C" fn remove_callback(pdev: *mut bindings::i2c_client) {
+        // SAFETY: `pdev` is a valid pointer to a `struct i2c_client`.
+        let ptr = unsafe { bindings::i2c_get_clientdata(pdev) }.cast();

Quoted 9 lines

rust/kernel/i2c.rs line 158

Quoted 5 lines

rust/kernel/i2c.rs new file +5 -0

+
+        // SAFETY: `remove_callback` is only ever called after a successful call to
+        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
+        // `KBox<T>` pointer created through `KBox::into_foreign`.
+        let _ = unsafe { KBox::<T>::from_foreign(ptr) };

I like to do that as well, but I usually get asked to use drop() instead, let's do that here as well. :)

Danilo, could you, please, explain this one a little bit more? I see the same pattern for all the implemented subsystems so far: auxiliary, platform, pci.
If this pattern for these subsystems is to be changed, then I'm fine.

rust/kernel/i2c.rs fn shutdown(dev: &Device<device::Core>) { · line 426

Quoted 6 lines · Igor Korotin

rust/kernel/i2c.rs +2 -1

-pub struct Device<Ctx: device::DeviceContext = device::Normal>(
+pub struct Device<Ctx: device::DeviceContext = device::Normal, State: DeviceState = state::Borrowed>(
     Opaque<bindings::i2c_client>,
     PhantomData<Ctx>,
+    PhantomData<State>,
 );

I see what you're doing here, but I think you're thinking this way too complicated.

I recommend not to reuse the Device type to register a new I2C client device, it's adding too much complexity without any real value.

You also don't want the DeviceContext types for a device registration, since the registration will never have any other DeviceContext than device::Normal (see also my comment on the sample module).

DeviceContext types are only useful for &Device (i.e. references) given out for a specific scope, such as probe(), remove(), etc.

The only thing you really want to do is to register a new I2C client device, get a i2c::Registration instance and call i2c_unregister_device() when the i2c::Registration is dropped.

This is exactly the same use-case as we have in the auxiliary bus. I highly recommend looking at what auxiliary::Registration does [1].

Also note that if you want a reference to the device in the i2c::Registration, you can also add a i2c::Registration::device() method that returns an &i2c::Device, which through into() you can obtain an ARef<i2c::Device> from.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/auxiliary.rs?h=v6.16-rc4#n299

rust/kernel/i2c.rs fn shutdown(dev: &Device<device::Core>) { · line 426

Quoted 6 lines · Igor Korotin

rust/kernel/i2c.rs +2 -1

-pub struct Device<Ctx: device::DeviceContext = device::Normal>(
+pub struct Device<Ctx: device::DeviceContext = device::Normal, State: DeviceState = state::Borrowed>(
     Opaque<bindings::i2c_client>,
     PhantomData<Ctx>,
+    PhantomData<State>,
 );

I see what you're doing here, but I think you're thinking this way too complicated.

I recommend not to reuse the Device type to register a new I2C client device, it's adding too much complexity without any real value.

You also don't want the DeviceContext types for a device registration, since the registration will never have any other DeviceContext than device::Normal (see also my comment on the sample module).

DeviceContext types are only useful for &Device (i.e. references) given out for a specific scope, such as probe(), remove(), etc.

The only thing you really want to do is to register a new I2C client device, get a i2c::Registration instance and call i2c_unregister_device() when the i2c::Registration is dropped.

This is exactly the same use-case as we have in the auxiliary bus. I highly recommend looking at what auxiliary::Registration does [1].

Also note that if you want a reference to the device in the i2c::Registration, you can also add a i2c::Registration::device() method that returns an &i2c::Device, which through into() you can obtain an ARef<i2c::Device> from.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/auxiliary.rs?h=v6.16-rc4#n299

Quoted 32 lines · Danilo Krummrich

rust/kernel/i2c.rs fn shutdown(dev: &Device<device::Core>) { · line 426

Quoted 6 lines · Igor Korotin

rust/kernel/i2c.rs +2 -1

-pub struct Device<Ctx: device::DeviceContext = device::Normal>(
+pub struct Device<Ctx: device::DeviceContext = device::Normal, State: DeviceState = state::Borrowed>(
     Opaque<bindings::i2c_client>,
     PhantomData<Ctx>,
+    PhantomData<State>,
 );

I see what you're doing here, but I think you're thinking this way too complicated.

I recommend not to reuse the Device type to register a new I2C client device, it's adding too much complexity without any real value.

You also don't want the DeviceContext types for a device registration, since the registration will never have any other DeviceContext than device::Normal (see also my comment on the sample module).

DeviceContext types are only useful for &Device (i.e. references) given out for a specific scope, such as probe(), remove(), etc.

The only thing you really want to do is to register a new I2C client device, get a i2c::Registration instance and call i2c_unregister_device() when the i2c::Registration is dropped.

This is exactly the same use-case as we have in the auxiliary bus. I highly recommend looking at what auxiliary::Registration does [1].

Also note that if you want a reference to the device in the i2c::Registration, you can also add a i2c::Registration::device() method that returns an &i2c::Device, which through into() you can obtain an ARef<i2c::Device> from.

Quoted 1 line

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/auxiliary.rs?h=v6.16-rc4#n299

I took a quick look at the auxiliary Registration abstraction and I see that it is not applicable for I2C subsystem. The issue here is that I2C C code doesn't provide with an API that can registers an I2C client from already existing struct i2c_client.

Quoted 35 lines · Igor Korotin

Quoted 32 lines · Danilo Krummrich

rust/kernel/i2c.rs fn shutdown(dev: &Device<device::Core>) { · line 426

Quoted 6 lines · Igor Korotin

rust/kernel/i2c.rs +2 -1

-pub struct Device<Ctx: device::DeviceContext = device::Normal>(
+pub struct Device<Ctx: device::DeviceContext = device::Normal, State: DeviceState = state::Borrowed>(
     Opaque<bindings::i2c_client>,
     PhantomData<Ctx>,
+    PhantomData<State>,
 );

I see what you're doing here, but I think you're thinking this way too complicated.

I recommend not to reuse the Device type to register a new I2C client device, it's adding too much complexity without any real value.

You also don't want the DeviceContext types for a device registration, since the registration will never have any other DeviceContext than device::Normal (see also my comment on the sample module).

DeviceContext types are only useful for &Device (i.e. references) given out for a specific scope, such as probe(), remove(), etc.

The only thing you really want to do is to register a new I2C client device, get a i2c::Registration instance and call i2c_unregister_device() when the i2c::Registration is dropped.

This is exactly the same use-case as we have in the auxiliary bus. I highly recommend looking at what auxiliary::Registration does [1].

Also note that if you want a reference to the device in the i2c::Registration, you can also add a i2c::Registration::device() method that returns an &i2c::Device, which through into() you can obtain an ARef<i2c::Device> from.

Quoted 6 lines

Quoted 1 line

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/auxiliary.rs?h=v6.16-rc4#n299

I took a quick look at the auxiliary Registration abstraction and I see that it is not applicable for I2C subsystem. The issue here is that I2C C code doesn't provide with an API that can registers an I2C client from already existing struct i2c_client.

I don't see why the following wouldn't work:

	struct Registration(NonNull<bindings::i2c_client>);

	impl Registration {
	   pub fn new(adp: &I2cAdapterRef, info: &I2cBoardInfo) -> Result<Self> {
	      // SAFETY: [...]
	      let cli = unsafe { bindings::i2c_new_client_device(adp.as_raw(), info.as_raw()) };
	
	      // Handle ERR_PTR()
	
	      Self(NonNull::new(cli))
	   }
	}
	
	impl Drop for Registration {
	   fn drop(&mut self) {
	      // SAFETY: [...]
	      unsafe { bindings::i2c_unregister_device(self.as_ptr()) };
	   }
	}

And in you sample driver you can still the exactly the same as you did before:

	struct SampleDriver {
	   _reg: i2c::Registration,
	}
	
	impl kernel::Module for SampleDriver {
	    fn init(_module: &'static ThisModule) -> Result<Self> {
	        let adapter = i2c::I2cAdapterRef::get(0).ok_or(EINVAL)?;
	
	        let reg = i2c::Registration::new(&adapter, &BOARD_INFO)?;
	
	        Ok(Self { _reg: reg })
	    }
	}

Quoted 67 lines · Danilo Krummrich

Quoted 35 lines · Igor Korotin

Quoted 32 lines · Danilo Krummrich

rust/kernel/i2c.rs fn shutdown(dev: &Device<device::Core>) { · line 426

Quoted 6 lines · Igor Korotin

rust/kernel/i2c.rs +2 -1

-pub struct Device<Ctx: device::DeviceContext = device::Normal>(
+pub struct Device<Ctx: device::DeviceContext = device::Normal, State: DeviceState = state::Borrowed>(
      Opaque<bindings::i2c_client>,
      PhantomData<Ctx>,
+    PhantomData<State>,
  );

I see what you're doing here, but I think you're thinking this way too complicated.

I recommend not to reuse the Device type to register a new I2C client device, it's adding too much complexity without any real value.

You also don't want the DeviceContext types for a device registration, since the registration will never have any other DeviceContext than device::Normal (see also my comment on the sample module).

DeviceContext types are only useful for &Device (i.e. references) given out for a specific scope, such as probe(), remove(), etc.

The only thing you really want to do is to register a new I2C client device, get a i2c::Registration instance and call i2c_unregister_device() when the i2c::Registration is dropped.

This is exactly the same use-case as we have in the auxiliary bus. I highly recommend looking at what auxiliary::Registration does [1].

Also note that if you want a reference to the device in the i2c::Registration, you can also add a i2c::Registration::device() method that returns an &i2c::Device, which through into() you can obtain an ARef<i2c::Device> from.

Quoted 6 lines

Quoted 1 line

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/auxiliary.rs?h=v6.16-rc4#n299

I took a quick look at the auxiliary Registration abstraction and I see that it is not applicable for I2C subsystem. The issue here is that I2C C code doesn't provide with an API that can registers an I2C client from already existing struct i2c_client.

I don't see why the following wouldn't work:

	struct Registration(NonNull<bindings::i2c_client>);

	impl Registration {
	   pub fn new(adp: &I2cAdapterRef, info: &I2cBoardInfo) -> Result<Self> {
	      // SAFETY: [...]
	      let cli = unsafe { bindings::i2c_new_client_device(adp.as_raw(), info.as_raw()) };
	
	      // Handle ERR_PTR()
	
	      Self(NonNull::new(cli))
	   }
	}
	
	impl Drop for Registration {
	   fn drop(&mut self) {
	      // SAFETY: [...]
	      unsafe { bindings::i2c_unregister_device(self.as_ptr()) };
	   }
	}

And in you sample driver you can still the exactly the same as you did before:

Quoted 14 lines

	struct SampleDriver {
	   _reg: i2c::Registration,
	}
	
	impl kernel::Module for SampleDriver {
	    fn init(_module: &'static ThisModule) -> Result<Self> {
	        let adapter = i2c::I2cAdapterRef::get(0).ok_or(EINVAL)?;
	
	        let reg = i2c::Registration::new(&adapter, &BOARD_INFO)?;
	
	        Ok(Self { _reg: reg })
	    }
	}

Ok, I think I've caught the idea. In general I worried that if one has link to an i2c::Device which is a transparent representation of struct i2c_client he could somehow cast that i2c::Device to Registration(NonNull<bindings::i2c_client>). After some experiments, I found out that this is not possible due to scope of the i2c::Device::as_raw(). So the simple NonNull implementation is as safe as my "super-puper secured" DeviceOwned implementation.

samples/rust/rust_device_i2c.rs new file +3 -0

+        Ok(Self { _owned: device })
+    }
+}