* [PATCH v3 0/3] rust: i2c: Add basic I2C driver abstractions
@ 2025-08-01 15:37 Igor Korotin
2025-08-01 15:40 ` [PATCH v3 1/3] rust: i2c: add basic I2C device and " Igor Korotin
` (3 more replies)
0 siblings, 4 replies; 39+ messages in thread
From: Igor Korotin @ 2025-08-01 15:37 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
rust-for-linux
This patch series lays the groundwork for writing Linux I2C drivers in
Rust by:
1. Core abstractions
Introduce `i2c::Device`, `i2c::Driver` and `i2c::Adapter` built on
the existing `struct i2c_client` and `struct i2c_driver`, with safe
Rust wrappers around probe, transfer, and teardown logic.
2. Manual device creation
Provide an API to register an I2C device at runtime from Rust using
`I2cBoardInfo`, including automatic cleanup when the driver unloads.
3. Sample driver (legacy table, OF & ACPI)
Add `rust_driver_i2c`, a sample that:
- creates an I2C client device using `i2c::Registration::new()`
- binds to an I2C client via:
- legacy I2C-ID table,
- Open Firmware (device-tree) compatible strings, or
- ACPI IDs.
- destroyes the I2C client device on exit.
Together, these three patches:
- Establish the essential Rust traits and types for I2C drivers.
- Enable driver binding via legacy ID table, device-tree (OF), or ACPI
- Enable manual device creation at runtime.
- Ship a samples showing typical usage
Igor Korotin (3):
rust: i2c: add basic I2C device and driver abstractions
rust: i2c: add manual I2C device creation abstractions
samples: rust: add Rust I2C sample driver
Changelog
---------
v3:
- removed unnecessary i2c_get_clientdata and i2c_set_clientdata rust
helpers. Using generic accessors implemented in [1] instead.
- Reimplemented i2c::DeviceId based on changes in [2].
- Using from_result in i2c::Adapter::probe_callback
- Using explicit drop() for i2c client private data in
`i2c::Adapter::remove_callback`
- replaced device::Device::as_ref() with device::Device::from_raw in
`i2c::Device::as_ref()`. It is renamed in device::Device.
- Build Rust I2C only if I2C is built-in
- Reimplement overcomplicated trait i2c::DeviceOwned the same way it is
implemented in auxiliary [3].
- Merge rust_device_i2c and rust_driver_i2c samples. Resulting
rust_driver_i2c creates pined i2c_client using i2c::Registration::new
and probes newly created i2c_client.
- Created a new entry in MAINTAINERS file containing i2c.rs and
rust_driver_i2c.rs in it.
- Link to v2: [4]
[1] https://lore.kernel.org/lkml/20250621195118.124245-3-dakr@kernel.org/
[2] https://lore.kernel.org/rust-for-linux/20250711040947.1252162-1-fujita.tomonori@gmail.com/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/auxiliary.rs?h=v6.16-rc4#n299
[4] https://lore.kernel.org/rust-for-linux/20250704153332.1193214-1-igor.korotin.linux@gmail.com/
v2:
- Merged separated ACPI support patches since ACPI-table support is
merged into driver-core-next.
- Added I2cAdapterRef and I2cBoardInfo abstractions
- Added DeviceState generic parameter which is used for `i2c::Device`
as a sign if the device is created manually
- Added `DeviceOwned` abstraction which is a safe reference to a
manually created `i2c::Device<Ctx, state::Owned>`.
- Added Rust manual I2C device creation sample
- Link to v1: https://lore.kernel.org/rust-for-linux/20250626174623.904917-1-igor.korotin.linux@gmail.com/
MAINTAINERS | 8 +
rust/bindings/bindings_helper.h | 1 +
rust/kernel/i2c.rs | 498 ++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
samples/rust/Kconfig | 11 +
samples/rust/Makefile | 1 +
samples/rust/rust_driver_i2c.rs | 106 +++++++
7 files changed, 627 insertions(+)
create mode 100644 rust/kernel/i2c.rs
create mode 100644 samples/rust/rust_driver_i2c.rs
base-commit: 260f6f4fda93c8485c8037865c941b42b9cba5d2
--
2.43.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-01 15:37 [PATCH v3 0/3] rust: i2c: Add basic I2C driver abstractions Igor Korotin
@ 2025-08-01 15:40 ` Igor Korotin
2025-08-01 17:14 ` Daniel Almeida
2025-08-02 0:00 ` Danilo Krummrich
2025-08-01 15:44 ` [PATCH v3 2/3] rust: i2c: add manual I2C device creation abstractions Igor Korotin
` (2 subsequent siblings)
3 siblings, 2 replies; 39+ messages in thread
From: Igor Korotin @ 2025-08-01 15:40 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
rust-for-linux
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 `RawDeviceIdIndex` implementation for I2C device IDs
Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
---
MAINTAINERS | 7 +
rust/bindings/bindings_helper.h | 1 +
rust/kernel/i2c.rs | 391 ++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
4 files changed, 401 insertions(+)
create mode 100644 rust/kernel/i2c.rs
diff --git a/MAINTAINERS b/MAINTAINERS
index 4f03e230f3c5..767beaa0f7c2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11514,6 +11514,13 @@ F: include/linux/i2c.h
F: include/uapi/linux/i2c-*.h
F: include/uapi/linux/i2c.h
+I2C SUBSYSTEM [RUST]
+M: Igor Korotin <igor.korotin.linux@gmail.com>
+R: Danilo Krummrich <dakr@kernel.org>
+L: rust-for-linux@vger.kernel.org
+S: Maintained
+F: rust/kernel/i2c.rs
+
I2C SUBSYSTEM HOST DRIVERS
M: Andi Shyti <andi.shyti@kernel.org>
L: linux-i2c@vger.kernel.org
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 84d60635e8a9..81796d5e16e8 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -53,6 +53,7 @@
#include <linux/file.h>
#include <linux/firmware.h>
#include <linux/fs.h>
+#include <linux/i2c.h>
#include <linux/ioport.h>
#include <linux/jiffies.h>
#include <linux/jump_label.h>
diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
new file mode 100644
index 000000000000..fafcae92cfdb
--- /dev/null
+++ b/rust/kernel/i2c.rs
@@ -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, RawDeviceIdIndex},
+ driver,
+ error::*,
+ of,
+ prelude::*,
+ types::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`.
+unsafe impl RawDeviceId for DeviceId {
+ type RawType = bindings::i2c_device_id;
+}
+
+// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
+unsafe impl RawDeviceIdIndex for DeviceId {
+ const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::i2c_device_id, driver_data);
+
+ fn index(&self) -> usize {
+ self.0.driver_data as _
+ }
+}
+
+/// 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::CoreInternal>>() };
+
+ let info =
+ Self::i2c_id_info(pdev).or_else(|| <Self as driver::Adapter>::id_info(pdev.as_ref()));
+
+
+ from_result(|| {
+ let data = T::probe(pdev, info)?;
+
+ pdev.as_ref().set_drvdata(data);
+ Ok(0)
+ })
+ }
+
+ extern "C" fn remove_callback(pdev: *mut bindings::i2c_client) {
+ // SAFETY: `pdev` is a valid pointer to a `struct i2c_client`.
+ let pdev = unsafe { &*pdev.cast::<Device<device::CoreInternal>>() };
+
+ // SAFETY: `remove_callback` is only ever called after a successful call to
+ // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
+ // and stored a `Pin<KBox<T>>`.
+ drop(unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() });
+ }
+
+ 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()?;
+
+ // 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
+ // 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::RawDeviceIdIndex>::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;
+
+ /// I2C driver probe.
+ ///
+ /// Called when a new i2c device is added or discovered.
+ /// Implementers should attempt to initialize the device here.
+ fn probe(dev: &Device<device::Core>, id_info: Option<&Self::IdInfo>)
+ -> Result<Pin<KBox<Self>>>;
+
+ /// I2C driver shutdown
+ ///
+ /// Called when
+ 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::from_raw(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 {}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c2d1b9375205..0780c3e746fc 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -86,6 +86,8 @@
#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
pub mod firmware;
pub mod fs;
+#[cfg(CONFIG_I2C = "y")]
+pub mod i2c;
pub mod init;
pub mod io;
pub mod ioctl;
--
2.43.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v3 2/3] rust: i2c: add manual I2C device creation abstractions
2025-08-01 15:37 [PATCH v3 0/3] rust: i2c: Add basic I2C driver abstractions Igor Korotin
2025-08-01 15:40 ` [PATCH v3 1/3] rust: i2c: add basic I2C device and " Igor Korotin
@ 2025-08-01 15:44 ` Igor Korotin
2025-08-01 17:59 ` Daniel Almeida
2025-08-02 0:12 ` Danilo Krummrich
2025-08-01 15:45 ` [PATCH v3 3/3] samples: rust: add Rust I2C sample driver Igor Korotin
2025-08-04 15:07 ` [PATCH v3 0/3] rust: i2c: Add basic I2C driver abstractions Daniel Almeida
3 siblings, 2 replies; 39+ messages in thread
From: Igor Korotin @ 2025-08-01 15:44 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
rust-for-linux
In addition to the basic I2C device support, added 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::I2cAdapterRef` — a safe reference around `struct i2c_adapter`
* `i2c::I2cBoardInfo` — a safe wrapper around `struct i2c_board_info`
Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
---
rust/kernel/i2c.rs | 111 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 109 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index fafcae92cfdb..bbdc41124fd8 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -139,7 +139,6 @@ extern "C" fn probe_callback(pdev: *mut bindings::i2c_client) -> kernel::ffi::c_
let info =
Self::i2c_id_info(pdev).or_else(|| <Self as driver::Adapter>::id_info(pdev.as_ref()));
-
from_result(|| {
let data = T::probe(pdev, info)?;
@@ -312,6 +311,77 @@ fn shutdown(dev: &Device<device::Core>) {
}
}
+/// The i2c adapter reference
+///
+/// This represents the Rust abstraction for a reference to an existing
+/// C `struct i2c_adapter`.
+///
+/// # Invariants
+///
+/// A [`I2cAdapterRef`] instance represents a valid `struct i2c_adapter` created by the C portion of
+/// the kernel.
+#[repr(transparent)]
+pub struct I2cAdapterRef(NonNull<bindings::i2c_adapter>);
+
+impl I2cAdapterRef {
+ fn as_raw(&self) -> *mut bindings::i2c_adapter {
+ self.0.as_ptr()
+ }
+
+ /// Gets pointer to an `i2c_adapter` by index.
+ pub fn get(index: i32) -> Option<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) })?;
+ Some(Self(adapter))
+ }
+}
+
+impl Drop for I2cAdapterRef {
+ fn drop(&mut self) {
+ // SAFETY: This `I2cAdapterRef` was obtained from `i2c_get_adapter`,
+ // and calling `i2c_put_adapter` exactly once will correctly release
+ // the reference count in the I2C core. It is safe to call from any context
+ unsafe { bindings::i2c_put_adapter(self.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 board‐info 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::acpi_device_id::default()` once stabilized for `const`.
+ // SAFETY: FFI type is valid to be zero-initialized.
+ let mut i2c_board_info: bindings::i2c_board_info = unsafe { core::mem::zeroed() };
+ 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 {
+ &self.0 as *const _
+ }
+}
+
/// The i2c client representation.
///
/// This structure represents the Rust abstraction for a C `struct i2c_client`. The
@@ -340,7 +410,7 @@ fn as_raw(&self) -> *mut bindings::i2c_client {
kernel::impl_device_context_into_aref!(Device);
// SAFETY: Instances of `Device` are always reference-counted.
-unsafe impl crate::types::AlwaysRefCounted for Device {
+unsafe impl<Ctx: device::DeviceContext> crate::types::AlwaysRefCounted for Device<Ctx> {
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()) };
@@ -389,3 +459,40 @@ 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 {}
+
+/// 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(i2c_adapter: &I2cAdapterRef, i2c_board_info: &I2cBoardInfo) -> Result<Self> {
+ // SAFETY: the kernel guarantees that `i2c_new_client_device()` returns either a valid
+ // pointer or NULL. `NonNull::new` guarantees the correct check.
+ let raw_dev = from_err_ptr(unsafe {
+ bindings::i2c_new_client_device(i2c_adapter.as_raw(), i2c_board_info.as_raw())
+ })?;
+
+ Ok(Self(NonNull::new(raw_dev).unwrap()))
+ }
+}
+
+impl Drop for Registration {
+ fn drop(&mut self) {
+ unsafe { bindings::i2c_unregister_device(self.0.as_ptr()) }
+ }
+}
+
+// SAFETY: A `Device` is always reference-counted and can be released from any thread.
+unsafe impl Send for Registration {}
+
+// SAFETY: A `Device` is always reference-counted and can be released from any thread.
+unsafe impl Sync for Registration {}
--
2.43.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v3 3/3] samples: rust: add Rust I2C sample driver
2025-08-01 15:37 [PATCH v3 0/3] rust: i2c: Add basic I2C driver abstractions Igor Korotin
2025-08-01 15:40 ` [PATCH v3 1/3] rust: i2c: add basic I2C device and " Igor Korotin
2025-08-01 15:44 ` [PATCH v3 2/3] rust: i2c: add manual I2C device creation abstractions Igor Korotin
@ 2025-08-01 15:45 ` Igor Korotin
2025-08-01 18:09 ` Daniel Almeida
` (2 more replies)
2025-08-04 15:07 ` [PATCH v3 0/3] rust: i2c: Add basic I2C driver abstractions Daniel Almeida
3 siblings, 3 replies; 39+ messages in thread
From: Igor Korotin @ 2025-08-01 15:45 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
rust-for-linux
Add a new `rust_driver_i2c` sample, showing how to create a new
i2c client using `i2c::Registration` and bind a driver to it
via legacy I2C-ID table.
Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
---
MAINTAINERS | 1 +
samples/rust/Kconfig | 11 ++++
samples/rust/Makefile | 1 +
samples/rust/rust_driver_i2c.rs | 106 ++++++++++++++++++++++++++++++++
4 files changed, 119 insertions(+)
create mode 100644 samples/rust/rust_driver_i2c.rs
diff --git a/MAINTAINERS b/MAINTAINERS
index 767beaa0f7c2..69312298ea01 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11520,6 +11520,7 @@ R: Danilo Krummrich <dakr@kernel.org>
L: rust-for-linux@vger.kernel.org
S: Maintained
F: rust/kernel/i2c.rs
+F: samples/rust/rust_driver_i2c.rs
I2C SUBSYSTEM HOST DRIVERS
M: Andi Shyti <andi.shyti@kernel.org>
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 7f7371a004ee..28dae070b365 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -62,6 +62,17 @@ config SAMPLE_RUST_DMA
If unsure, say N.
+config SAMPLE_RUST_DRIVER_I2C
+ tristate "I2C Driver"
+ depends on I2C=y
+ help
+ This option builds the Rust I2C driver sample.
+
+ To compile this as a module, choose M here:
+ the module will be called rust_driver_i2c.
+
+ If unsure, say N.
+
config SAMPLE_RUST_DRIVER_PCI
tristate "PCI Driver"
depends on PCI
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index bd2faad63b4f..141d8f078248 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_SAMPLE_RUST_MINIMAL) += rust_minimal.o
obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE) += rust_misc_device.o
obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o
obj-$(CONFIG_SAMPLE_RUST_DMA) += rust_dma.o
+obj-$(CONFIG_SAMPLE_RUST_DRIVER_I2C) += rust_driver_i2c.o
obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o
obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o
obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o
diff --git a/samples/rust/rust_driver_i2c.rs b/samples/rust/rust_driver_i2c.rs
new file mode 100644
index 000000000000..20815efc933e
--- /dev/null
+++ b/samples/rust/rust_driver_i2c.rs
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust I2C driver sample.
+//!
+//! This module shows how to:
+//! 1. Manually create an `i2c_client` at address `SAMPLE_I2C_CLIENT_ADDR`
+//! on the adapter with index `SAMPLE_I2C_ADAPTER_INDEX`.
+//! 2. Register a matching Rust-based I2C driver for that client.
+//!
+//! # Requirements
+//! - The target system must expose an I2C adapter at index
+//! `SAMPLE_I2C_ADAPTER_INDEX`.
+//! - To emulate an adapter for testing, you can load the
+//! `i2c-stub` kernel module.
+//!
+
+use kernel::{acpi, c_str, device::Core, i2c, of, prelude::*, types::ARef};
+
+const SAMPLE_I2C_CLIENT_ADDR: u16 = 0x30;
+const SAMPLE_I2C_ADAPTER_INDEX: i32 = 0;
+const BOARD_INFO: i2c::I2cBoardInfo =
+ i2c::I2cBoardInfo::new(c_str!("rust_driver_i2c"), SAMPLE_I2C_CLIENT_ADDR);
+
+struct SampleDriver {
+ pdev: ARef<i2c::Device>,
+}
+
+kernel::acpi_device_table! {
+ ACPI_TABLE,
+ MODULE_ACPI_TABLE,
+ <SampleDriver as i2c::Driver>::IdInfo,
+ [(acpi::DeviceId::new(c_str!("LNUXBEEF")), 0)]
+}
+
+kernel::i2c_device_table! {
+ I2C_TABLE,
+ MODULE_I2C_TABLE,
+ <SampleDriver as i2c::Driver>::IdInfo,
+ [(i2c::DeviceId::new(c_str!("rust_driver_i2c")), 0)]
+}
+
+kernel::of_device_table! {
+ OF_TABLE,
+ MODULE_OF_TABLE,
+ <SampleDriver as i2c::Driver>::IdInfo,
+ [(of::DeviceId::new(c_str!("test,rust_driver_i2c")), 0)]
+}
+
+impl i2c::Driver for SampleDriver {
+ type IdInfo = u32;
+
+ const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
+ const I2C_ID_TABLE: Option<i2c::IdTable<Self::IdInfo>> = Some(&I2C_TABLE);
+ const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+ fn probe(pdev: &i2c::Device<Core>, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
+ let dev = pdev.as_ref();
+
+ dev_dbg!(dev, "Probe Rust I2C driver sample.\n");
+
+ if let Some(info) = info {
+ dev_info!(dev, "Probed with info: '{}'.\n", info);
+ }
+
+ let drvdata = KBox::new(Self { pdev: pdev.into() }, GFP_KERNEL)?;
+
+ Ok(drvdata.into())
+ }
+ fn shutdown(pdev: &i2c::Device<Core>) {
+ dev_dbg!(pdev.as_ref(), "Shutdown Rust I2C driver sample.\n");
+ }
+}
+
+impl Drop for SampleDriver {
+ fn drop(&mut self) {
+ dev_dbg!(self.pdev.as_ref(), "Remove Rust I2C driver sample.\n");
+ }
+}
+
+// NOTE: The code below is expanded macro module_i2c_driver. It is not used here
+// because we need to manually create an I2C client in `init()`. The macro
+// hides `init()`, so to demo client creation on adapter SAMPLE_I2C_ADAPTER_INDEX
+// we expand it by hand.
+type Ops<T> = kernel::i2c::Adapter<T>;
+#[kernel::prelude::pin_data]
+struct DriverModule {
+ #[pin]
+ _driver: kernel::driver::Registration<Ops<SampleDriver>>,
+ _reg: i2c::Registration,
+}
+
+impl kernel::InPlaceModule for DriverModule {
+ fn init(
+ module: &'static kernel::ThisModule,
+ ) -> impl ::pin_init::PinInit<Self, kernel::error::Error> {
+ let adapter = i2c::I2cAdapterRef::get(SAMPLE_I2C_ADAPTER_INDEX);
+
+ kernel::try_pin_init!(Self {
+ _reg <- i2c::Registration::new(&adapter.unwrap(), &BOARD_INFO),
+ _driver<-kernel::driver::Registration::new(<Self as kernel::ModuleMetadata>::NAME,module,),
+ })
+ }
+}
+kernel::prelude::module! {
+ type:DriverModule,name:"rust_driver_i2c",authors:["Igor Korotin"],description:"Rust I2C driver",license:"GPL v2",
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-01 15:40 ` [PATCH v3 1/3] rust: i2c: add basic I2C device and " Igor Korotin
@ 2025-08-01 17:14 ` Daniel Almeida
2025-08-02 0:22 ` Danilo Krummrich
2025-08-04 14:16 ` Igor Korotin
2025-08-02 0:00 ` Danilo Krummrich
1 sibling, 2 replies; 39+ messages in thread
From: Daniel Almeida @ 2025-08-01 17:14 UTC (permalink / raw)
To: Igor Korotin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, rust-for-linux
Hi Igor, this is looking good AFAICT :)
Some minor comments inline.
> On 1 Aug 2025, at 12:40, Igor Korotin <igor.korotin.linux@gmail.com> wrote:
>
> 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 `RawDeviceIdIndex` implementation for I2C device IDs
>
> Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
> ---
> MAINTAINERS | 7 +
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/i2c.rs | 391 ++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 2 +
> 4 files changed, 401 insertions(+)
> create mode 100644 rust/kernel/i2c.rs
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4f03e230f3c5..767beaa0f7c2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11514,6 +11514,13 @@ F: include/linux/i2c.h
> F: include/uapi/linux/i2c-*.h
> F: include/uapi/linux/i2c.h
>
> +I2C SUBSYSTEM [RUST]
> +M: Igor Korotin <igor.korotin.linux@gmail.com>
> +R: Danilo Krummrich <dakr@kernel.org>
> +L: rust-for-linux@vger.kernel.org
> +S: Maintained
> +F: rust/kernel/i2c.rs
> +
> I2C SUBSYSTEM HOST DRIVERS
> M: Andi Shyti <andi.shyti@kernel.org>
> L: linux-i2c@vger.kernel.org
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 84d60635e8a9..81796d5e16e8 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -53,6 +53,7 @@
> #include <linux/file.h>
> #include <linux/firmware.h>
> #include <linux/fs.h>
> +#include <linux/i2c.h>
> #include <linux/ioport.h>
> #include <linux/jiffies.h>
> #include <linux/jump_label.h>
> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> new file mode 100644
> index 000000000000..fafcae92cfdb
> --- /dev/null
> +++ b/rust/kernel/i2c.rs
> @@ -0,0 +1,391 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! I2C Driver subsystem
Missing period?
> +
> +// I2C Driver abstractions.
> +use crate::{
> + acpi, container_of, device,
> + device_id::{RawDeviceId, RawDeviceIdIndex},
> + driver,
> + error::*,
> + of,
> + prelude::*,
> + types::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;
> + }
Nit:
for i in 0..src.len() {
}
Saves you from manually keeping track of the loop variable.
> +
> + 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`.
> +unsafe impl RawDeviceId for DeviceId {
> + type RawType = bindings::i2c_device_id;
> +}
> +
> +// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
> +unsafe impl RawDeviceIdIndex for DeviceId {
> + const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::i2c_device_id, driver_data);
> +
> + fn index(&self) -> usize {
> + self.0.driver_data as _
> + }
> +}
> +
> +/// 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(),
> + };
I wonder what happens if these are all None?
> +
> + // 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 {
Nit: throughout the file: I think that pdev comes either from pci or platform
devices? Maybe a different variable name should be used?
> + // 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::CoreInternal>>() };
> +
> + let info =
> + Self::i2c_id_info(pdev).or_else(|| <Self as driver::Adapter>::id_info(pdev.as_ref()));
> +
> +
> + from_result(|| {
> + let data = T::probe(pdev, info)?;
> +
> + pdev.as_ref().set_drvdata(data);
> + Ok(0)
> + })
> + }
> +
> + extern "C" fn remove_callback(pdev: *mut bindings::i2c_client) {
> + // SAFETY: `pdev` is a valid pointer to a `struct i2c_client`.
> + let pdev = unsafe { &*pdev.cast::<Device<device::CoreInternal>>() };
> +
> + // SAFETY: `remove_callback` is only ever called after a successful call to
> + // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
> + // and stored a `Pin<KBox<T>>`.
> + drop(unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() });
> + }
> +
> + extern "C" fn shutdown_callback(pdev: *mut bindings::i2c_client) {
> + let pdev = unsafe { &*pdev.cast::<Device<device::Core>>() };
Missing SAFETY comment. There’s a lint for this now, thankfully :)
> +
> + 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()?;
> +
> + // SAFETY:
> + // - `table` has static lifetime, hence it's valid for read,
nit: for reads? Or perhaps a native English speaker can chime in.
> + // - `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()) };
Wait, where’s `pdev.as_ref().as_raw()` here?
> +
> + if raw_id.is_null() {
> + None
> + } else {
> + // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct i2c_device_id` and
> + // 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::RawDeviceIdIndex>::index(id)))
> + }
This could perhaps benefit from an early return to reduce the indentation?
> + }
> +}
> +
> +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;
> +
> + /// I2C driver probe.
> + ///
> + /// Called when a new i2c device is added or discovered.
> + /// Implementers should attempt to initialize the device here.
> + fn probe(dev: &Device<device::Core>, id_info: Option<&Self::IdInfo>)
> + -> Result<Pin<KBox<Self>>>;
> +
> + /// I2C driver shutdown
> + ///
> + /// Called when
> + 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.
nit: "An existing C `struct i2c_client` that gets passed from the C side” reads a bit better, perhaps?
> +///
> +/// # 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) };
&raw mut
I also wonder whether this unsafe block can be made shorter, as only the
dereference is unsafe.
> +
> + // SAFETY: `dev` points to a valid `struct device`.
> + unsafe { device::Device::from_raw(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 {}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index c2d1b9375205..0780c3e746fc 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -86,6 +86,8 @@
> #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
> pub mod firmware;
> pub mod fs;
> +#[cfg(CONFIG_I2C = "y")]
> +pub mod i2c;
> pub mod init;
> pub mod io;
> pub mod ioctl;
> --
> 2.43.0
>
>
— Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 2/3] rust: i2c: add manual I2C device creation abstractions
2025-08-01 15:44 ` [PATCH v3 2/3] rust: i2c: add manual I2C device creation abstractions Igor Korotin
@ 2025-08-01 17:59 ` Daniel Almeida
2025-08-02 0:26 ` Danilo Krummrich
2025-08-04 14:38 ` Igor Korotin
2025-08-02 0:12 ` Danilo Krummrich
1 sibling, 2 replies; 39+ messages in thread
From: Daniel Almeida @ 2025-08-01 17:59 UTC (permalink / raw)
To: Igor Korotin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, rust-for-linux
Hi Igor,
> On 1 Aug 2025, at 12:44, Igor Korotin <igor.korotin.linux@gmail.com> wrote:
>
> In addition to the basic I2C device support, added 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::I2cAdapterRef` — a safe reference around `struct i2c_adapter`
>
> * `i2c::I2cBoardInfo` — a safe wrapper around `struct i2c_board_info`
>
> Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
> ---
> rust/kernel/i2c.rs | 111 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 109 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> index fafcae92cfdb..bbdc41124fd8 100644
> --- a/rust/kernel/i2c.rs
> +++ b/rust/kernel/i2c.rs
> @@ -139,7 +139,6 @@ extern "C" fn probe_callback(pdev: *mut bindings::i2c_client) -> kernel::ffi::c_
> let info =
> Self::i2c_id_info(pdev).or_else(|| <Self as driver::Adapter>::id_info(pdev.as_ref()));
>
> -
Unrelated change?
> from_result(|| {
> let data = T::probe(pdev, info)?;
>
> @@ -312,6 +311,77 @@ fn shutdown(dev: &Device<device::Core>) {
> }
> }
>
> +/// The i2c adapter reference
> +///
> +/// This represents the Rust abstraction for a reference to an existing
> +/// C `struct i2c_adapter`.
> +///
> +/// # Invariants
> +///
> +/// A [`I2cAdapterRef`] instance represents a valid `struct i2c_adapter` created by the C portion of
> +/// the kernel.
> +#[repr(transparent)]
> +pub struct I2cAdapterRef(NonNull<bindings::i2c_adapter>);
Is there a plan for an owned version? Otherwise I’d personally drop the
“Ref” nomenclature.
> +
> +impl I2cAdapterRef {
> + fn as_raw(&self) -> *mut bindings::i2c_adapter {
> + self.0.as_ptr()
> + }
> +
> + /// Gets pointer to an `i2c_adapter` by index.
> + pub fn get(index: i32) -> Option<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) })?;
> + Some(Self(adapter))
> + }
> +}
> +
> +impl Drop for I2cAdapterRef {
> + fn drop(&mut self) {
> + // SAFETY: This `I2cAdapterRef` was obtained from `i2c_get_adapter`,
> + // and calling `i2c_put_adapter` exactly once will correctly release
> + // the reference count in the I2C core. It is safe to call from any context
> + unsafe { bindings::i2c_put_adapter(self.as_raw()) }
Should this implement AlwasyRefCounted?
> + }
> +}
> +
> +/// 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 board‐info 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::acpi_device_id::default()` once stabilized for `const`.
> + // SAFETY: FFI type is valid to be zero-initialized.
> + let mut i2c_board_info: bindings::i2c_board_info = unsafe { core::mem::zeroed() };
> + let mut i: usize = 0;
> + while i < src.len() {
> + i2c_board_info.type_[i] = src[i];
> + i += 1;
> + }
Same comment as the last patch.
> +
> + i2c_board_info.addr = addr;
> + Self(i2c_board_info)
> + }
> +
> + fn as_raw(&self) -> *const bindings::i2c_board_info {
> + &self.0 as *const _
> + }
> +}
> +
> /// The i2c client representation.
> ///
> /// This structure represents the Rust abstraction for a C `struct i2c_client`. The
> @@ -340,7 +410,7 @@ fn as_raw(&self) -> *mut bindings::i2c_client {
> kernel::impl_device_context_into_aref!(Device);
>
> // SAFETY: Instances of `Device` are always reference-counted.
> -unsafe impl crate::types::AlwaysRefCounted for Device {
> +unsafe impl<Ctx: device::DeviceContext> crate::types::AlwaysRefCounted for Device<Ctx> {
> 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()) };
> @@ -389,3 +459,40 @@ 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 {}
> +
> +/// 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(i2c_adapter: &I2cAdapterRef, i2c_board_info: &I2cBoardInfo) -> Result<Self> {
> + // SAFETY: the kernel guarantees that `i2c_new_client_device()` returns either a valid
> + // pointer or NULL. `NonNull::new` guarantees the correct check.
> + let raw_dev = from_err_ptr(unsafe {
> + bindings::i2c_new_client_device(i2c_adapter.as_raw(), i2c_board_info.as_raw())
> + })?;
> +
> + Ok(Self(NonNull::new(raw_dev).unwrap()))
> + }
> +}
> +
> +impl Drop for Registration {
> + fn drop(&mut self) {
> + unsafe { bindings::i2c_unregister_device(self.0.as_ptr()) }
> + }
> +}
> +
> +// SAFETY: A `Device` is always reference-counted and can be released from any thread.
> +unsafe impl Send for Registration {}
> +
> +// SAFETY: A `Device` is always reference-counted and can be released from any thread.
> +unsafe impl Sync for Registration {}
A `Device` ?
> --
> 2.43.0
>
>
— Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/3] samples: rust: add Rust I2C sample driver
2025-08-01 15:45 ` [PATCH v3 3/3] samples: rust: add Rust I2C sample driver Igor Korotin
@ 2025-08-01 18:09 ` Daniel Almeida
2025-08-04 14:43 ` Igor Korotin
2025-08-02 0:18 ` Danilo Krummrich
2025-08-07 8:42 ` kernel test robot
2 siblings, 1 reply; 39+ messages in thread
From: Daniel Almeida @ 2025-08-01 18:09 UTC (permalink / raw)
To: Igor Korotin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, rust-for-linux
Hi Igor,
Overall looks good, some minor comments below.
> On 1 Aug 2025, at 12:45, Igor Korotin <igor.korotin.linux@gmail.com> wrote:
>
> Add a new `rust_driver_i2c` sample, showing how to create a new
> i2c client using `i2c::Registration` and bind a driver to it
> via legacy I2C-ID table.
>
> Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
> ---
> MAINTAINERS | 1 +
> samples/rust/Kconfig | 11 ++++
> samples/rust/Makefile | 1 +
> samples/rust/rust_driver_i2c.rs | 106 ++++++++++++++++++++++++++++++++
> 4 files changed, 119 insertions(+)
> create mode 100644 samples/rust/rust_driver_i2c.rs
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 767beaa0f7c2..69312298ea01 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11520,6 +11520,7 @@ R: Danilo Krummrich <dakr@kernel.org>
> L: rust-for-linux@vger.kernel.org
> S: Maintained
> F: rust/kernel/i2c.rs
> +F: samples/rust/rust_driver_i2c.rs
>
> I2C SUBSYSTEM HOST DRIVERS
> M: Andi Shyti <andi.shyti@kernel.org>
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index 7f7371a004ee..28dae070b365 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -62,6 +62,17 @@ config SAMPLE_RUST_DMA
>
> If unsure, say N.
>
> +config SAMPLE_RUST_DRIVER_I2C
> + tristate "I2C Driver"
> + depends on I2C=y
> + help
> + This option builds the Rust I2C driver sample.
> +
> + To compile this as a module, choose M here:
> + the module will be called rust_driver_i2c.
> +
> + If unsure, say N.
> +
> config SAMPLE_RUST_DRIVER_PCI
> tristate "PCI Driver"
> depends on PCI
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index bd2faad63b4f..141d8f078248 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_SAMPLE_RUST_MINIMAL) += rust_minimal.o
> obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE) += rust_misc_device.o
> obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o
> obj-$(CONFIG_SAMPLE_RUST_DMA) += rust_dma.o
> +obj-$(CONFIG_SAMPLE_RUST_DRIVER_I2C) += rust_driver_i2c.o
> obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o
> obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o
> obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o
> diff --git a/samples/rust/rust_driver_i2c.rs b/samples/rust/rust_driver_i2c.rs
> new file mode 100644
> index 000000000000..20815efc933e
> --- /dev/null
> +++ b/samples/rust/rust_driver_i2c.rs
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Rust I2C driver sample.
> +//!
> +//! This module shows how to:
Blank line here?
> +//! 1. Manually create an `i2c_client` at address `SAMPLE_I2C_CLIENT_ADDR`
> +//! on the adapter with index `SAMPLE_I2C_ADAPTER_INDEX`.
> +//! 2. Register a matching Rust-based I2C driver for that client.
> +//!
> +//! # Requirements
Blank line here?
> +//! - The target system must expose an I2C adapter at index
> +//! `SAMPLE_I2C_ADAPTER_INDEX`.
> +//! - To emulate an adapter for testing, you can load the
> +//! `i2c-stub` kernel module.
Anything else needs to be done?
> +//!
> +
> +use kernel::{acpi, c_str, device::Core, i2c, of, prelude::*, types::ARef};
> +
> +const SAMPLE_I2C_CLIENT_ADDR: u16 = 0x30;
> +const SAMPLE_I2C_ADAPTER_INDEX: i32 = 0;
> +const BOARD_INFO: i2c::I2cBoardInfo =
> + i2c::I2cBoardInfo::new(c_str!("rust_driver_i2c"), SAMPLE_I2C_CLIENT_ADDR);
> +
> +struct SampleDriver {
> + pdev: ARef<i2c::Device>,
> +}
> +
> +kernel::acpi_device_table! {
> + ACPI_TABLE,
> + MODULE_ACPI_TABLE,
> + <SampleDriver as i2c::Driver>::IdInfo,
> + [(acpi::DeviceId::new(c_str!("LNUXBEEF")), 0)]
> +}
> +
> +kernel::i2c_device_table! {
> + I2C_TABLE,
> + MODULE_I2C_TABLE,
> + <SampleDriver as i2c::Driver>::IdInfo,
> + [(i2c::DeviceId::new(c_str!("rust_driver_i2c")), 0)]
> +}
> +
> +kernel::of_device_table! {
> + OF_TABLE,
> + MODULE_OF_TABLE,
> + <SampleDriver as i2c::Driver>::IdInfo,
> + [(of::DeviceId::new(c_str!("test,rust_driver_i2c")), 0)]
> +}
> +
> +impl i2c::Driver for SampleDriver {
> + type IdInfo = u32;
> +
> + const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
> + const I2C_ID_TABLE: Option<i2c::IdTable<Self::IdInfo>> = Some(&I2C_TABLE);
> + const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> + fn probe(pdev: &i2c::Device<Core>, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
> + let dev = pdev.as_ref();
> +
> + dev_dbg!(dev, "Probe Rust I2C driver sample.\n");
> +
> + if let Some(info) = info {
> + dev_info!(dev, "Probed with info: '{}'.\n", info);
> + }
> +
> + let drvdata = KBox::new(Self { pdev: pdev.into() }, GFP_KERNEL)?;
> +
> + Ok(drvdata.into())
> + }
Blank line here.
> + fn shutdown(pdev: &i2c::Device<Core>) {
> + dev_dbg!(pdev.as_ref(), "Shutdown Rust I2C driver sample.\n");
> + }
> +}
> +
> +impl Drop for SampleDriver {
> + fn drop(&mut self) {
> + dev_dbg!(self.pdev.as_ref(), "Remove Rust I2C driver sample.\n");
> + }
> +}
> +
> +// NOTE: The code below is expanded macro module_i2c_driver. It is not used here
> +// because we need to manually create an I2C client in `init()`. The macro
> +// hides `init()`, so to demo client creation on adapter SAMPLE_I2C_ADAPTER_INDEX
> +// we expand it by hand.
> +type Ops<T> = kernel::i2c::Adapter<T>;
Blank here.
Also, just “pin_data” ?
> +#[kernel::prelude::pin_data]
> +struct DriverModule {
> + #[pin]
> + _driver: kernel::driver::Registration<Ops<SampleDriver>>,
> + _reg: i2c::Registration,
> +}
> +
> +impl kernel::InPlaceModule for DriverModule {
> + fn init(
> + module: &'static kernel::ThisModule,
> + ) -> impl ::pin_init::PinInit<Self, kernel::error::Error> {
> + let adapter = i2c::I2cAdapterRef::get(SAMPLE_I2C_ADAPTER_INDEX);
> +
> + kernel::try_pin_init!(Self {
> + _reg <- i2c::Registration::new(&adapter.unwrap(), &BOARD_INFO),
> + _driver<-kernel::driver::Registration::new(<Self as kernel::ModuleMetadata>::NAME,module,),
Missing space in “<-“
> + })
> + }
> +}
> +kernel::prelude::module! {
> + type:DriverModule,name:"rust_driver_i2c",authors:["Igor Korotin"],description:"Rust I2C driver",license:"GPL v2",
> +}
> --
> 2.43.0
>
>
This is usually broken into multiple lines?
— Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-01 15:40 ` [PATCH v3 1/3] rust: i2c: add basic I2C device and " Igor Korotin
2025-08-01 17:14 ` Daniel Almeida
@ 2025-08-02 0:00 ` Danilo Krummrich
2025-08-02 9:07 ` Wolfram Sang
` (2 more replies)
1 sibling, 3 replies; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-02 0:00 UTC (permalink / raw)
To: Igor Korotin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux, Wolfram Sang
(Cc: Wolfram)
On Fri Aug 1, 2025 at 5:40 PM CEST, Igor Korotin wrote:
> 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 `RawDeviceIdIndex` implementation for I2C device IDs
>
> Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
> ---
> MAINTAINERS | 7 +
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/i2c.rs | 391 ++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 2 +
> 4 files changed, 401 insertions(+)
> create mode 100644 rust/kernel/i2c.rs
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4f03e230f3c5..767beaa0f7c2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11514,6 +11514,13 @@ F: include/linux/i2c.h
> F: include/uapi/linux/i2c-*.h
> F: include/uapi/linux/i2c.h
>
> +I2C SUBSYSTEM [RUST]
Has this been agreed with Wolfram off-list? (I'm asking since I haven't seen a
reply from him in v2.)
In case you just did not receive an answer yet, it's fine to proceed with a
proposal as you do here. However, please make sure to keep Wolfram in the loop.
:)
In either case, which tree should patches flow through?
> +M: Igor Korotin <igor.korotin.linux@gmail.com>
> +R: Danilo Krummrich <dakr@kernel.org>
If you'd like me to keep an eye on this as well, this is fine with me.
> +L: rust-for-linux@vger.kernel.org
> +S: Maintained
> +F: rust/kernel/i2c.rs
> +
> I2C SUBSYSTEM HOST DRIVERS
> M: Andi Shyti <andi.shyti@kernel.org>
> L: linux-i2c@vger.kernel.org
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 84d60635e8a9..81796d5e16e8 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -53,6 +53,7 @@
> #include <linux/file.h>
> #include <linux/firmware.h>
> #include <linux/fs.h>
> +#include <linux/i2c.h>
> #include <linux/ioport.h>
> #include <linux/jiffies.h>
> #include <linux/jump_label.h>
> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> new file mode 100644
> index 000000000000..fafcae92cfdb
> --- /dev/null
> +++ b/rust/kernel/i2c.rs
> @@ -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, RawDeviceIdIndex},
> + driver,
> + error::*,
> + of,
> + prelude::*,
> + types::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`.
> +unsafe impl RawDeviceId for DeviceId {
> + type RawType = bindings::i2c_device_id;
> +}
> +
> +// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
> +unsafe impl RawDeviceIdIndex for DeviceId {
> + const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::i2c_device_id, driver_data);
> +
> + fn index(&self) -> usize {
> + self.0.driver_data as _
This cast shouldn't be needed.
> + }
> +}
> +
> +/// 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>,
Rather idrv or similar I think; same for various cases below.
> + 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(),
> + };
I suppose other than platform drivers, I2C drivers can't match by name, hence I
think we should fail if none of the above is provided.
> +
> + // 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::CoreInternal>>() };
> +
> + let info =
> + Self::i2c_id_info(pdev).or_else(|| <Self as driver::Adapter>::id_info(pdev.as_ref()));
> +
> +
> + from_result(|| {
> + let data = T::probe(pdev, info)?;
> +
> + pdev.as_ref().set_drvdata(data);
> + Ok(0)
> + })
> + }
> +
> + extern "C" fn remove_callback(pdev: *mut bindings::i2c_client) {
> + // SAFETY: `pdev` is a valid pointer to a `struct i2c_client`.
> + let pdev = unsafe { &*pdev.cast::<Device<device::CoreInternal>>() };
> +
> + // SAFETY: `remove_callback` is only ever called after a successful call to
> + // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
> + // and stored a `Pin<KBox<T>>`.
> + drop(unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() });
> + }
> +
> + extern "C" fn shutdown_callback(pdev: *mut bindings::i2c_client) {
> + let pdev = unsafe { &*pdev.cast::<Device<device::Core>>() };
Missing safety comment, please make sure to compile with CLIPPY=1. I think I
also saw some checkpatch.pl warnings.
> +
> + 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()?;
> +
> + // 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
> + // 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::RawDeviceIdIndex>::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;
> +
> + /// I2C driver probe.
> + ///
> + /// Called when a new i2c device is added or discovered.
> + /// Implementers should attempt to initialize the device here.
> + fn probe(dev: &Device<device::Core>, id_info: Option<&Self::IdInfo>)
> + -> Result<Pin<KBox<Self>>>;
> +
> + /// I2C driver shutdown
> + ///
> + /// Called when
Yes? :)
> + 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)) }
You can also use &raw mut.
> + }
> +}
> +
> +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) };
Same here.
> +
> + // SAFETY: `dev` points to a valid `struct device`.
> + unsafe { device::Device::from_raw(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 {}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index c2d1b9375205..0780c3e746fc 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -86,6 +86,8 @@
> #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
> pub mod firmware;
> pub mod fs;
> +#[cfg(CONFIG_I2C = "y")]
> +pub mod i2c;
> pub mod init;
> pub mod io;
> pub mod ioctl;
> --
> 2.43.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 2/3] rust: i2c: add manual I2C device creation abstractions
2025-08-01 15:44 ` [PATCH v3 2/3] rust: i2c: add manual I2C device creation abstractions Igor Korotin
2025-08-01 17:59 ` Daniel Almeida
@ 2025-08-02 0:12 ` Danilo Krummrich
1 sibling, 0 replies; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-02 0:12 UTC (permalink / raw)
To: Igor Korotin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
On Fri Aug 1, 2025 at 5:44 PM CEST, Igor Korotin wrote:
> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> index fafcae92cfdb..bbdc41124fd8 100644
> --- a/rust/kernel/i2c.rs
> +++ b/rust/kernel/i2c.rs
> @@ -139,7 +139,6 @@ extern "C" fn probe_callback(pdev: *mut bindings::i2c_client) -> kernel::ffi::c_
> let info =
> Self::i2c_id_info(pdev).or_else(|| <Self as driver::Adapter>::id_info(pdev.as_ref()));
>
> -
Seems like this hunk should be in patch 1.
> from_result(|| {
> let data = T::probe(pdev, info)?;
>
> @@ -312,6 +311,77 @@ fn shutdown(dev: &Device<device::Core>) {
> }
> }
>
> +/// The i2c adapter reference
> +///
> +/// This represents the Rust abstraction for a reference to an existing
> +/// C `struct i2c_adapter`.
> +///
> +/// # Invariants
> +///
> +/// A [`I2cAdapterRef`] instance represents a valid `struct i2c_adapter` created by the C portion of
> +/// the kernel.
> +#[repr(transparent)]
> +pub struct I2cAdapterRef(NonNull<bindings::i2c_adapter>);
> +
> +impl I2cAdapterRef {
> + fn as_raw(&self) -> *mut bindings::i2c_adapter {
> + self.0.as_ptr()
> + }
> +
> + /// Gets pointer to an `i2c_adapter` by index.
> + pub fn get(index: i32) -> Option<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) })?;
> + Some(Self(adapter))
> + }
I think this should just return Result<Self> and if i2c_get_adapter() is NULL it
should just return ENOENT.
> +}
> +
> +impl Drop for I2cAdapterRef {
> + fn drop(&mut self) {
> + // SAFETY: This `I2cAdapterRef` was obtained from `i2c_get_adapter`,
> + // and calling `i2c_put_adapter` exactly once will correctly release
> + // the reference count in the I2C core. It is safe to call from any context
> + unsafe { bindings::i2c_put_adapter(self.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 board‐info 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::acpi_device_id::default()` once stabilized for `const`.
> + // SAFETY: FFI type is valid to be zero-initialized.
> + let mut i2c_board_info: bindings::i2c_board_info = unsafe { core::mem::zeroed() };
> + 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 {
> + &self.0 as *const _
I think we don't need this cast.
> + }
> +}
> +
> /// The i2c client representation.
> ///
> /// This structure represents the Rust abstraction for a C `struct i2c_client`. The
> @@ -340,7 +410,7 @@ fn as_raw(&self) -> *mut bindings::i2c_client {
> kernel::impl_device_context_into_aref!(Device);
>
> // SAFETY: Instances of `Device` are always reference-counted.
> -unsafe impl crate::types::AlwaysRefCounted for Device {
> +unsafe impl<Ctx: device::DeviceContext> crate::types::AlwaysRefCounted for Device<Ctx> {
I think you forgot to remove this change.
In case it's intentional, it's wrong. We can't allow the existance of an
ARef<Device<Ctx>>, where Ctx is anything else than device::Normal. All device
contexts other than Normal are bound to a specific scope and must only ever
exist as reference, e.g. &Device<Bound>.
> 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()) };
> @@ -389,3 +459,40 @@ 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 {}
> +
> +/// 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(i2c_adapter: &I2cAdapterRef, i2c_board_info: &I2cBoardInfo) -> Result<Self> {
> + // SAFETY: the kernel guarantees that `i2c_new_client_device()` returns either a valid
> + // pointer or NULL. `NonNull::new` guarantees the correct check.
> + let raw_dev = from_err_ptr(unsafe {
> + bindings::i2c_new_client_device(i2c_adapter.as_raw(), i2c_board_info.as_raw())
> + })?;
> +
> + Ok(Self(NonNull::new(raw_dev).unwrap()))
> + }
> +}
> +
> +impl Drop for Registration {
> + fn drop(&mut self) {
> + unsafe { bindings::i2c_unregister_device(self.0.as_ptr()) }
> + }
> +}
> +
> +// SAFETY: A `Device` is always reference-counted and can be released from any thread.
> +unsafe impl Send for Registration {}
> +
> +// SAFETY: A `Device` is always reference-counted and can be released from any thread.
> +unsafe impl Sync for Registration {}
> --
> 2.43.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/3] samples: rust: add Rust I2C sample driver
2025-08-01 15:45 ` [PATCH v3 3/3] samples: rust: add Rust I2C sample driver Igor Korotin
2025-08-01 18:09 ` Daniel Almeida
@ 2025-08-02 0:18 ` Danilo Krummrich
2025-08-07 8:42 ` kernel test robot
2 siblings, 0 replies; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-02 0:18 UTC (permalink / raw)
To: Igor Korotin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
On Fri Aug 1, 2025 at 5:45 PM CEST, Igor Korotin wrote:
> +#[kernel::prelude::pin_data]
Can just be #[pin_data].
> +struct DriverModule {
> + #[pin]
> + _driver: kernel::driver::Registration<Ops<SampleDriver>>,
> + _reg: i2c::Registration,
> +}
> +
> +impl kernel::InPlaceModule for DriverModule {
> + fn init(
> + module: &'static kernel::ThisModule,
> + ) -> impl ::pin_init::PinInit<Self, kernel::error::Error> {
> + let adapter = i2c::I2cAdapterRef::get(SAMPLE_I2C_ADAPTER_INDEX);
> +
> + kernel::try_pin_init!(Self {
> + _reg <- i2c::Registration::new(&adapter.unwrap(), &BOARD_INFO),
Please don't use unwrap(), this panics the kernel in case of failure. Please see
below for a better solution.
> + _driver<-kernel::driver::Registration::new(<Self as kernel::ModuleMetadata>::NAME,module,),
> + })
> + }
> +}
> +kernel::prelude::module! {
> + type:DriverModule,name:"rust_driver_i2c",authors:["Igor Korotin"],description:"Rust I2C driver",license:"GPL v2",
> +}
I suggest to write this like this:
#[pin_data]
struct DriverModule {
#[pin]
_driver: kernel::driver::Registration<Ops<SampleDriver>>,
_reg: i2c::Registration,
}
impl kernel::InPlaceModule for DriverModule {
fn init(
module: &'static kernel::ThisModule,
) -> impl ::pin_init::PinInit<Self, Error> {
try_pin_init!(Self {
_reg <- {
let adapter = i2c::I2cAdapterRef::get(SAMPLE_I2C_ADAPTER_INDEX).ok_or(ENOENT)?;
i2c::Registration::new(&adapter, &BOARD_INFO)
},
_driver <- kernel::driver::Registration::new(<Self as kernel::ModuleMetadata>::NAME, module),
})
}
}
kernel::prelude::module! {
type:DriverModule,
name:"rust_driver_i2c",
authors:["Igor Korotin"],
description:"Rust I2C driver",
license:"GPL v2",
}
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-01 17:14 ` Daniel Almeida
@ 2025-08-02 0:22 ` Danilo Krummrich
2025-08-04 14:16 ` Igor Korotin
1 sibling, 0 replies; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-02 0:22 UTC (permalink / raw)
To: Daniel Almeida
Cc: Igor Korotin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
On Fri Aug 1, 2025 at 7:14 PM CEST, Daniel Almeida wrote:
>> +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;
>> + }
>
> Nit:
>
> for i in 0..src.len() {
>
> }
>
> Saves you from manually keeping track of the loop variable.
That would be nice, but we can't do this in cost context. :(
>> +// 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) };
>
> &raw mut
>
> I also wonder whether this unsafe block can be made shorter, as only the
> dereference is unsafe.
No, this would create a temporary value.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 2/3] rust: i2c: add manual I2C device creation abstractions
2025-08-01 17:59 ` Daniel Almeida
@ 2025-08-02 0:26 ` Danilo Krummrich
2025-08-04 14:38 ` Igor Korotin
1 sibling, 0 replies; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-02 0:26 UTC (permalink / raw)
To: Daniel Almeida
Cc: Igor Korotin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
On Fri Aug 1, 2025 at 7:59 PM CEST, Daniel Almeida wrote:
>> +/// The i2c adapter reference
>> +///
>> +/// This represents the Rust abstraction for a reference to an existing
>> +/// C `struct i2c_adapter`.
>> +///
>> +/// # Invariants
>> +///
>> +/// A [`I2cAdapterRef`] instance represents a valid `struct i2c_adapter` created by the C portion of
>> +/// the kernel.
>> +#[repr(transparent)]
>> +pub struct I2cAdapterRef(NonNull<bindings::i2c_adapter>);
>
> Is there a plan for an owned version? Otherwise I’d personally drop the
> “Ref” nomenclature.
>
>> +
>> +impl I2cAdapterRef {
>> + fn as_raw(&self) -> *mut bindings::i2c_adapter {
>> + self.0.as_ptr()
>> + }
>> +
>> + /// Gets pointer to an `i2c_adapter` by index.
>> + pub fn get(index: i32) -> Option<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) })?;
>> + Some(Self(adapter))
>> + }
>> +}
>> +
>> +impl Drop for I2cAdapterRef {
>> + fn drop(&mut self) {
>> + // SAFETY: This `I2cAdapterRef` was obtained from `i2c_get_adapter`,
>> + // and calling `i2c_put_adapter` exactly once will correctly release
>> + // the reference count in the I2C core. It is safe to call from any context
>> + unsafe { bindings::i2c_put_adapter(self.as_raw()) }
>
> Should this implement AlwasyRefCounted?
I don't think it's necessary given the context this is used in, but it's also
not much more code to do so, and ARef<I2cAdapter> turns out much cleaner I
think.
So, yes I agree this should implement AlwasyRefCounted.
>> + }
>> +}
>> +
>> +/// 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 board‐info 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::acpi_device_id::default()` once stabilized for `const`.
>> + // SAFETY: FFI type is valid to be zero-initialized.
>> + let mut i2c_board_info: bindings::i2c_board_info = unsafe { core::mem::zeroed() };
>> + let mut i: usize = 0;
>> + while i < src.len() {
>> + i2c_board_info.type_[i] = src[i];
>> + i += 1;
>> + }
>
> Same comment as the last patch.
Still not possible unfortunately. :(
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-02 0:00 ` Danilo Krummrich
@ 2025-08-02 9:07 ` Wolfram Sang
2025-08-02 10:51 ` Danilo Krummrich
2025-08-04 14:58 ` Igor Korotin
2025-08-15 15:40 ` Igor Korotin
2 siblings, 1 reply; 39+ messages in thread
From: Wolfram Sang @ 2025-08-02 9:07 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Igor Korotin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]
Hi all,
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 4f03e230f3c5..767beaa0f7c2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11514,6 +11514,13 @@ F: include/linux/i2c.h
> > F: include/uapi/linux/i2c-*.h
> > F: include/uapi/linux/i2c.h
> >
> > +I2C SUBSYSTEM [RUST]
>
> Has this been agreed with Wolfram off-list? (I'm asking since I haven't seen a
> reply from him in v2.)
>
> In case you just did not receive an answer yet, it's fine to proceed with a
> proposal as you do here. However, please make sure to keep Wolfram in the loop.
> :)
Thank you for pointing out this thread, much appreciated. I think (I
hope) I said previously that I am supportive of the bindings in general
but I can't help at all because I don't speak Rust. Given that, I am
more than happy to see this MAINTAINERS entry for Rust I2C. This part is
definitely:
Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> In either case, which tree should patches flow through?
As long as you don't touch generic I2C header files, I suggest some Rust
tree. I guess patches will have more dependencies on what is in that
tree.
> > +M: Igor Korotin <igor.korotin.linux@gmail.com>
> > +R: Danilo Krummrich <dakr@kernel.org>
>
> If you'd like me to keep an eye on this as well, this is fine with me.
>
> > +L: rust-for-linux@vger.kernel.org
I think it is worth adding the Linux I2C list here as well. Other people
there do speak Rust.
> > +S: Maintained
> > +F: rust/kernel/i2c.rs
Thanks,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-02 9:07 ` Wolfram Sang
@ 2025-08-02 10:51 ` Danilo Krummrich
2025-08-02 12:14 ` Wolfram Sang
0 siblings, 1 reply; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-02 10:51 UTC (permalink / raw)
To: Wolfram Sang
Cc: Igor Korotin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
On Sat Aug 2, 2025 at 11:07 AM CEST, Wolfram Sang wrote:
> Hi all,
>
>> > diff --git a/MAINTAINERS b/MAINTAINERS
>> > index 4f03e230f3c5..767beaa0f7c2 100644
>> > --- a/MAINTAINERS
>> > +++ b/MAINTAINERS
>> > @@ -11514,6 +11514,13 @@ F: include/linux/i2c.h
>> > F: include/uapi/linux/i2c-*.h
>> > F: include/uapi/linux/i2c.h
>> >
>> > +I2C SUBSYSTEM [RUST]
>>
>> Has this been agreed with Wolfram off-list? (I'm asking since I haven't seen a
>> reply from him in v2.)
>>
>> In case you just did not receive an answer yet, it's fine to proceed with a
>> proposal as you do here. However, please make sure to keep Wolfram in the loop.
>> :)
>
> Thank you for pointing out this thread, much appreciated. I think (I
> hope) I said previously that I am supportive of the bindings in general
> but I can't help at all because I don't speak Rust. Given that, I am
> more than happy to see this MAINTAINERS entry for Rust I2C. This part is
> definitely:
>
> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
>> In either case, which tree should patches flow through?
>
> As long as you don't touch generic I2C header files, I suggest some Rust
> tree. I guess patches will have more dependencies on what is in that
> tree.
Maybe it would also be a good option to take PRs from Igor? I think it could be
a great way to get in touch with some of the Rust work over time without having
to commit to maintaining it. (Of course, this is entirely up to you though.)
While I encourage the above, one fallback alternative could be to take patches
through the driver-core tree -- at least for now. Given that this is a bus
abstraction and hence most infrastructure used is generic device / driver
infrastructure, there's some synergy.
>> > +M: Igor Korotin <igor.korotin.linux@gmail.com>
>> > +R: Danilo Krummrich <dakr@kernel.org>
>>
>> If you'd like me to keep an eye on this as well, this is fine with me.
>>
>> > +L: rust-for-linux@vger.kernel.org
>
> I think it is worth adding the Linux I2C list here as well. Other people
> there do speak Rust.
Agreed, maybe it'd also be good for you to be listed as reviewer?
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-02 10:51 ` Danilo Krummrich
@ 2025-08-02 12:14 ` Wolfram Sang
2025-08-02 12:41 ` Danilo Krummrich
0 siblings, 1 reply; 39+ messages in thread
From: Wolfram Sang @ 2025-08-02 12:14 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Igor Korotin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]
Hi,
> Maybe it would also be a good option to take PRs from Igor? I think it could be
> a great way to get in touch with some of the Rust work over time without having
> to commit to maintaining it. (Of course, this is entirely up to you though.)
Makes sense...
> While I encourage the above, one fallback alternative could be to take patches
> through the driver-core tree -- at least for now. Given that this is a bus
> abstraction and hence most infrastructure used is generic device / driver
> infrastructure, there's some synergy.
... especially if the alternative is "only" the driver core tree. My
tree makes more sense then. I thought there might be a more fitting
"Rust" tree. If that's not the case, I can pull from Igor, no problem.
We will see how this goes dependency-wise.
> >> > +L: rust-for-linux@vger.kernel.org
> >
> > I think it is worth adding the Linux I2C list here as well. Other people
> > there do speak Rust.
>
> Agreed, maybe it'd also be good for you to be listed as reviewer?
Please no. I really can't review these patches. I will have a short look
at them when I pull from Igor. If you need my experience while
developing, just CC me in those (probably few) cases.
Have a nice weekend,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-02 12:14 ` Wolfram Sang
@ 2025-08-02 12:41 ` Danilo Krummrich
0 siblings, 0 replies; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-02 12:41 UTC (permalink / raw)
To: Wolfram Sang
Cc: Igor Korotin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
On Sat Aug 2, 2025 at 2:14 PM CEST, Wolfram Sang wrote:
> Hi,
>
>> Maybe it would also be a good option to take PRs from Igor? I think it could be
>> a great way to get in touch with some of the Rust work over time without having
>> to commit to maintaining it. (Of course, this is entirely up to you though.)
>
> Makes sense...
Great!
>> While I encourage the above, one fallback alternative could be to take patches
>> through the driver-core tree -- at least for now. Given that this is a bus
>> abstraction and hence most infrastructure used is generic device / driver
>> infrastructure, there's some synergy.
>
> ... especially if the alternative is "only" the driver core tree. My
> tree makes more sense then. I thought there might be a more fitting
> "Rust" tree. If that's not the case, I can pull from Igor, no problem.
> We will see how this goes dependency-wise.
There's of course also Miguel's Rust tree, but this is more for "core Rust
infrastructure".
Citing Miguel [1]:
"We try to reserve the global Rust tree fallback for cases where there
is really no other way, e.g. when a key subsystem doesn't want
anything to do with Rust at all and thus would block many others
otherwise."
[1] https://lore.kernel.org/all/CANiq72=+UUFVemSGHvzkX5FoOv-U5YRDKQkbKjYwqEfex-ey6w@mail.gmail.com/
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-01 17:14 ` Daniel Almeida
2025-08-02 0:22 ` Danilo Krummrich
@ 2025-08-04 14:16 ` Igor Korotin
1 sibling, 0 replies; 39+ messages in thread
From: Igor Korotin @ 2025-08-04 14:16 UTC (permalink / raw)
To: Daniel Almeida
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, rust-for-linux
Hello Daniel
Thank you for the review.
On 8/1/25 18:14, Daniel Almeida wrote:
>> +
>> + 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`.
>> +unsafe impl RawDeviceId for DeviceId {
>> + type RawType = bindings::i2c_device_id;
>> +}
>> +
>> +// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
>> +unsafe impl RawDeviceIdIndex for DeviceId {
>> + const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::i2c_device_id, driver_data);
>> +
>> + fn index(&self) -> usize {
>> + self.0.driver_data as _
>> + }
>> +}
>> +
>> +/// 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(),
>> + };
>
> I wonder what happens if these are all None?
>
C code in the i2c_register_driver function doesn't require id_table, OF
table, or ACPI table to be non-NULL. Technically, there's no issue
registering such an I2C driver. Though there's the following code in
i2c_device_probe:
/*
* An I2C ID table is not mandatory, if and only if, a suitable OF
* or ACPI ID table is supplied for the probing device.
*/
if (!driver->id_table &&
!acpi_driver_match_device(dev, dev->driver) &&
!i2c_of_match_device(dev->driver->of_match_table, client)) {
status = -ENODEV;
goto put_sync_adapter;
}
Based on that, maybe it makes sense to add some kind of
compile-time/build-time assertion to make sure the developer defines at
least one of those tables. If so, I'm happy to think about how to
implement something like that.
>> +
>> + // 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 {
>
> Nit: throughout the file: I think that pdev comes either from pci or platform
> devices? Maybe a different variable name should be used?
>
To be honest, I always thought pdev/pdrv/etc just meant "pointer to
device"/"pointer to driver" and not necessarily PCI/platform. If it
actually implies platform/PCI, then yeah, I agree — better to use
something like idev/idrv, as Danilo also pointed out.
>> + // - `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()) };
>
> Wait, where’s `pdev.as_ref().as_raw()` here?
>
Yeah, good catch. There was a significant rework between v2 and v3, and
I didn’t clean up all the comments properly. My bad.
Noted all the other comments too — I’ll fix those in the next drop.
Best Regards
Igor
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 2/3] rust: i2c: add manual I2C device creation abstractions
2025-08-01 17:59 ` Daniel Almeida
2025-08-02 0:26 ` Danilo Krummrich
@ 2025-08-04 14:38 ` Igor Korotin
1 sibling, 0 replies; 39+ messages in thread
From: Igor Korotin @ 2025-08-04 14:38 UTC (permalink / raw)
To: Daniel Almeida
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, rust-for-linux
Hello Daniel
On 8/1/25 18:59, Daniel Almeida wrote:
>> from_result(|| {
>> let data = T::probe(pdev, info)?;
>>
>> @@ -312,6 +311,77 @@ fn shutdown(dev: &Device<device::Core>) {
>> }
>> }
>>
>> +/// The i2c adapter reference
>> +///
>> +/// This represents the Rust abstraction for a reference to an existing
>> +/// C `struct i2c_adapter`.
>> +///
>> +/// # Invariants
>> +///
>> +/// A [`I2cAdapterRef`] instance represents a valid `struct i2c_adapter` created by the C portion of
>> +/// the kernel.
>> +#[repr(transparent)]
>> +pub struct I2cAdapterRef(NonNull<bindings::i2c_adapter>);
>
> Is there a plan for an owned version? Otherwise I’d personally drop the
> “Ref” nomenclature.
>
Not at the moment at least. I'll remove this Ref nomenclature in the
next drop
Thank you for the review!
Best Regards
Igor
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/3] samples: rust: add Rust I2C sample driver
2025-08-01 18:09 ` Daniel Almeida
@ 2025-08-04 14:43 ` Igor Korotin
2025-08-04 14:58 ` Daniel Almeida
0 siblings, 1 reply; 39+ messages in thread
From: Igor Korotin @ 2025-08-04 14:43 UTC (permalink / raw)
To: Daniel Almeida
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, rust-for-linux
Hello Daniel
On 8/1/25 19:09, Daniel Almeida wrote:
>> +//! - The target system must expose an I2C adapter at index
>> +//! `SAMPLE_I2C_ADAPTER_INDEX`.
>> +//! - To emulate an adapter for testing, you can load the
>> +//! `i2c-stub` kernel module.
>
> Anything else needs to be done?
>
Sorry, not sure I understood — can you clarify?
Thank you for the review.
Thanks
Igor
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/3] samples: rust: add Rust I2C sample driver
2025-08-04 14:43 ` Igor Korotin
@ 2025-08-04 14:58 ` Daniel Almeida
0 siblings, 0 replies; 39+ messages in thread
From: Daniel Almeida @ 2025-08-04 14:58 UTC (permalink / raw)
To: Igor Korotin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, rust-for-linux
Hi Igor,
> On 4 Aug 2025, at 11:43, Igor Korotin <igor.korotin.linux@gmail.com> wrote:
>
> Hello Daniel
>
> On 8/1/25 19:09, Daniel Almeida wrote:
>>> +//! - The target system must expose an I2C adapter at index
>>> +//! `SAMPLE_I2C_ADAPTER_INDEX`.
>>> +//! - To emulate an adapter for testing, you can load the
>>> +//! `i2c-stub` kernel module.
>>
>> Anything else needs to be done?
>>
>
> Sorry, not sure I understood — can you clarify?
>
> Thank you for the review.
>
> Thanks
> Igor
I was asking whether you have more details on how i2c-stub can be used to test
your code, as I'm unfamiliar with it. Are there any more parameters that I need to
set up?
You probably have this written down for your own testing, so if you can share
more instructions on how you're doing it, I can also test it for you and give
you a Tested-by tag.
— Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-02 0:00 ` Danilo Krummrich
2025-08-02 9:07 ` Wolfram Sang
@ 2025-08-04 14:58 ` Igor Korotin
2025-08-04 15:17 ` Danilo Krummrich
2025-08-15 15:40 ` Igor Korotin
2 siblings, 1 reply; 39+ messages in thread
From: Igor Korotin @ 2025-08-04 14:58 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux, Wolfram Sang
Hello Danilo
On 8/2/25 01:00, Danilo Krummrich wrote:
> (Cc: Wolfram)
>
> On Fri Aug 1, 2025 at 5:40 PM CEST, Igor Korotin wrote:
>> 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 `RawDeviceIdIndex` implementation for I2C device IDs
>>
>> Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
>> ---
>> MAINTAINERS | 7 +
>> rust/bindings/bindings_helper.h | 1 +
>> rust/kernel/i2c.rs | 391 ++++++++++++++++++++++++++++++++
>> rust/kernel/lib.rs | 2 +
>> 4 files changed, 401 insertions(+)
>> create mode 100644 rust/kernel/i2c.rs
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 4f03e230f3c5..767beaa0f7c2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11514,6 +11514,13 @@ F: include/linux/i2c.h
>> F: include/uapi/linux/i2c-*.h
>> F: include/uapi/linux/i2c.h
>>
>> +I2C SUBSYSTEM [RUST]
>
> Has this been agreed with Wolfram off-list? (I'm asking since I haven't seen a
> reply from him in v2.)
>
> In case you just did not receive an answer yet, it's fine to proceed with a
> proposal as you do here. However, please make sure to keep Wolfram in the loop.
> :)
>
> In either case, which tree should patches flow through?
>
>> +M: Igor Korotin <igor.korotin.linux@gmail.com>
>> +R: Danilo Krummrich <dakr@kernel.org>
>
> If you'd like me to keep an eye on this as well, this is fine with me.
>
Yeah. I really need your help if you don't mind. Thanks.
Could you also briefly tell me finally which tree for patches should I
use, how do I do it and if I need to represent it in the MAINTAINERS
file somehow?
Thanks a lot for your help and review.
Best Regards
Igor
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 0/3] rust: i2c: Add basic I2C driver abstractions
2025-08-01 15:37 [PATCH v3 0/3] rust: i2c: Add basic I2C driver abstractions Igor Korotin
` (2 preceding siblings ...)
2025-08-01 15:45 ` [PATCH v3 3/3] samples: rust: add Rust I2C sample driver Igor Korotin
@ 2025-08-04 15:07 ` Daniel Almeida
3 siblings, 0 replies; 39+ messages in thread
From: Daniel Almeida @ 2025-08-04 15:07 UTC (permalink / raw)
To: Igor Korotin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, rust-for-linux
Igor,
> On 1 Aug 2025, at 12:37, Igor Korotin <igor.korotin.linux@gmail.com> wrote:
>
> This patch series lays the groundwork for writing Linux I2C drivers in
> Rust by:
>
> 1. Core abstractions
> Introduce `i2c::Device`, `i2c::Driver` and `i2c::Adapter` built on
> the existing `struct i2c_client` and `struct i2c_driver`, with safe
> Rust wrappers around probe, transfer, and teardown logic.
>
> 2. Manual device creation
> Provide an API to register an I2C device at runtime from Rust using
> `I2cBoardInfo`, including automatic cleanup when the driver unloads.
>
> 3. Sample driver (legacy table, OF & ACPI)
> Add `rust_driver_i2c`, a sample that:
> - creates an I2C client device using `i2c::Registration::new()`
> - binds to an I2C client via:
> - legacy I2C-ID table,
> - Open Firmware (device-tree) compatible strings, or
> - ACPI IDs.
> - destroyes the I2C client device on exit.
>
> Together, these three patches:
>
> - Establish the essential Rust traits and types for I2C drivers.
> - Enable driver binding via legacy ID table, device-tree (OF), or ACPI
> - Enable manual device creation at runtime.
> - Ship a samples showing typical usage
>
> Igor Korotin (3):
> rust: i2c: add basic I2C device and driver abstractions
> rust: i2c: add manual I2C device creation abstractions
> samples: rust: add Rust I2C sample driver
>
> Changelog
> ---------
> v3:
> - removed unnecessary i2c_get_clientdata and i2c_set_clientdata rust
> helpers. Using generic accessors implemented in [1] instead.
> - Reimplemented i2c::DeviceId based on changes in [2].
> - Using from_result in i2c::Adapter::probe_callback
> - Using explicit drop() for i2c client private data in
> `i2c::Adapter::remove_callback`
> - replaced device::Device::as_ref() with device::Device::from_raw in
> `i2c::Device::as_ref()`. It is renamed in device::Device.
> - Build Rust I2C only if I2C is built-in
> - Reimplement overcomplicated trait i2c::DeviceOwned the same way it is
> implemented in auxiliary [3].
> - Merge rust_device_i2c and rust_driver_i2c samples. Resulting
> rust_driver_i2c creates pined i2c_client using i2c::Registration::new
> and probes newly created i2c_client.
> - Created a new entry in MAINTAINERS file containing i2c.rs and
> rust_driver_i2c.rs in it.
> - Link to v2: [4]
>
> [1] https://lore.kernel.org/lkml/20250621195118.124245-3-dakr@kernel.org/
> [2] https://lore.kernel.org/rust-for-linux/20250711040947.1252162-1-fujita.tomonori@gmail.com/
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/auxiliary.rs?h=v6.16-rc4#n299
> [4] https://lore.kernel.org/rust-for-linux/20250704153332.1193214-1-igor.korotin.linux@gmail.com/
>
> v2:
> - Merged separated ACPI support patches since ACPI-table support is
> merged into driver-core-next.
> - Added I2cAdapterRef and I2cBoardInfo abstractions
> - Added DeviceState generic parameter which is used for `i2c::Device`
> as a sign if the device is created manually
> - Added `DeviceOwned` abstraction which is a safe reference to a
> manually created `i2c::Device<Ctx, state::Owned>`.
> - Added Rust manual I2C device creation sample
> - Link to v1: https://lore.kernel.org/rust-for-linux/20250626174623.904917-1-igor.korotin.linux@gmail.com/
>
> MAINTAINERS | 8 +
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/i2c.rs | 498 ++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 2 +
> samples/rust/Kconfig | 11 +
> samples/rust/Makefile | 1 +
> samples/rust/rust_driver_i2c.rs | 106 +++++++
> 7 files changed, 627 insertions(+)
> create mode 100644 rust/kernel/i2c.rs
> create mode 100644 samples/rust/rust_driver_i2c.rs
>
>
> base-commit: 260f6f4fda93c8485c8037865c941b42b9cba5d2
> --
> 2.43.0
>
>
I’m not sure whether this has been mentioned yet: c_int and friends are in
the prelude. There are quite a few places where kernel::ffi::c_int shows up in
this series. That can be just “c_int” instead, for example.
— Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 14:58 ` Igor Korotin
@ 2025-08-04 15:17 ` Danilo Krummrich
2025-08-04 15:24 ` Miguel Ojeda
2025-08-04 15:40 ` Igor Korotin
0 siblings, 2 replies; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-04 15:17 UTC (permalink / raw)
To: Igor Korotin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux, Wolfram Sang
On Mon Aug 4, 2025 at 4:58 PM CEST, Igor Korotin wrote:
> On 8/2/25 01:00, Danilo Krummrich wrote:
>> On Fri Aug 1, 2025 at 5:40 PM CEST, Igor Korotin wrote:
>>> +M: Igor Korotin <igor.korotin.linux@gmail.com>
>>> +R: Danilo Krummrich <dakr@kernel.org>
>>
>> If you'd like me to keep an eye on this as well, this is fine with me.
>>
>
> Yeah. I really need your help if you don't mind. Thanks.
>
> Could you also briefly tell me finally which tree for patches should I
> use, how do I do it and if I need to represent it in the MAINTAINERS
> file somehow?
Given that Wolfram said that he prefers you to maintain the code under a
separate entry and send him PRs, you would need your own git tree where you
pick up patches, which you then send as a PR to Wolfram.
The link to the tree should go under "T:" in the MAINTAINERS file in the entry
your patch already contains.
I understand you haven't been a maintainer so far and haven't picked up patches
and send pull requests, so it can be a bit much in the beginning.
A good place to start is the "Kernel Maintainer Handbook" [1].
Additionally, if you're willing to pick up this responsibility, I'd happy to
offer you a call where we could go through the details of applying patches and
send PRs and clarify questions to ensure it's going to be a smooth experience
for everyone involved.
Once things are in place, you should clarify with Wolfram when in the release
cycle he expects you to send PRs, i.e. until which -rc before the merge window
for new patches, and if he has any additional expectations.
I know it may look a bit complicated at a first glance, but it's not as
complicated as it may appear either. :)
[1] https://docs.kernel.org/maintainer/index.html
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 15:17 ` Danilo Krummrich
@ 2025-08-04 15:24 ` Miguel Ojeda
2025-08-04 15:40 ` Igor Korotin
1 sibling, 0 replies; 39+ messages in thread
From: Miguel Ojeda @ 2025-08-04 15:24 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Igor Korotin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux, Wolfram Sang
On Mon, Aug 4, 2025 at 5:17 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Given that Wolfram said that he prefers you to maintain the code under a
> separate entry and send him PRs, you would need your own git tree where you
> pick up patches, which you then send as a PR to Wolfram.
>
> The link to the tree should go under "T:" in the MAINTAINERS file in the entry
> your patch already contains.
>
> I understand you haven't been a maintainer so far and haven't picked up patches
> and send pull requests, so it can be a bit much in the beginning.
>
> A good place to start is the "Kernel Maintainer Handbook" [1].
>
> Additionally, if you're willing to pick up this responsibility, I'd happy to
> offer you a call where we could go through the details of applying patches and
> send PRs and clarify questions to ensure it's going to be a smooth experience
> for everyone involved.
>
> Once things are in place, you should clarify with Wolfram when in the release
> cycle he expects you to send PRs, i.e. until which -rc before the merge window
> for new patches, and if he has any additional expectations.
>
> I know it may look a bit complicated at a first glance, but it's not as
> complicated as it may appear either. :)
+1 to all this and happy to help as well on learning how to be a maintainer.
Thanks Igor (and Wolfram)!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 15:17 ` Danilo Krummrich
2025-08-04 15:24 ` Miguel Ojeda
@ 2025-08-04 15:40 ` Igor Korotin
2025-08-04 16:11 ` Wolfram Sang
1 sibling, 1 reply; 39+ messages in thread
From: Igor Korotin @ 2025-08-04 15:40 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux, Wolfram Sang
Hello Danilo
On 8/4/25 16:17, Danilo Krummrich wrote:
> On Mon Aug 4, 2025 at 4:58 PM CEST, Igor Korotin wrote:
>> On 8/2/25 01:00, Danilo Krummrich wrote:
>>> On Fri Aug 1, 2025 at 5:40 PM CEST, Igor Korotin wrote:
>>>> +M: Igor Korotin <igor.korotin.linux@gmail.com>
>>>> +R: Danilo Krummrich <dakr@kernel.org>
>>>
>>> If you'd like me to keep an eye on this as well, this is fine with me.
>>>
>>
>> Yeah. I really need your help if you don't mind. Thanks.
>>
>> Could you also briefly tell me finally which tree for patches should I
>> use, how do I do it and if I need to represent it in the MAINTAINERS
>> file somehow?
>
> Given that Wolfram said that he prefers you to maintain the code under a
> separate entry and send him PRs, you would need your own git tree where you
> pick up patches, which you then send as a PR to Wolfram.
>
> The link to the tree should go under "T:" in the MAINTAINERS file in the entry
> your patch already contains.
>
> I understand you haven't been a maintainer so far and haven't picked up patches
> and send pull requests, so it can be a bit much in the beginning.
>
> A good place to start is the "Kernel Maintainer Handbook" [1].
>
> Additionally, if you're willing to pick up this responsibility, I'd happy to
> offer you a call where we could go through the details of applying patches and
> send PRs and clarify questions to ensure it's going to be a smooth experience
> for everyone involved.
>
> Once things are in place, you should clarify with Wolfram when in the release
> cycle he expects you to send PRs, i.e. until which -rc before the merge window
> for new patches, and if he has any additional expectations.
>
> I know it may look a bit complicated at a first glance, but it's not as
> complicated as it may appear either. :)
>
> [1] https://docs.kernel.org/maintainer/index.html
Wow, that’s a lot to take in — but thanks a lot for the detailed
explanation.
I’m a bit worried about it, to be honest, but I’ll give it a try at least.
I’ll start by going through [1] in the next few days to get the basics.
Thanks again for the info and for offering help.
Best regards,
Igor
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 15:40 ` Igor Korotin
@ 2025-08-04 16:11 ` Wolfram Sang
2025-08-04 17:15 ` Danilo Krummrich
2025-08-04 17:26 ` Igor Korotin
0 siblings, 2 replies; 39+ messages in thread
From: Wolfram Sang @ 2025-08-04 16:11 UTC (permalink / raw)
To: Igor Korotin
Cc: Danilo Krummrich, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
[-- Attachment #1: Type: text/plain, Size: 723 bytes --]
> Wow, that’s a lot to take in — but thanks a lot for the detailed
> explanation.
>
> I’m a bit worried about it, to be honest, but I’ll give it a try at least.
Then, we can also start simple.
If there is a Rust related patch on the i2c-list, I will not do anything
with it unless it got a Reviewed-by from Igor. If it got one, I will
apply it to my tree. If there any dependencies, it would be helpful if
this will be mentioned.
So, for starters, we can use my tree in the seperate I2C RUST entry. We
did a similar thing with the I2C MUX subsubsystem. By the time, we can
move to a dedicated tree, if there is a need. Thinking about it, there
will not be a lot of patches for I2C Rust, or?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 16:11 ` Wolfram Sang
@ 2025-08-04 17:15 ` Danilo Krummrich
2025-08-04 22:01 ` Wolfram Sang
2025-08-04 17:26 ` Igor Korotin
1 sibling, 1 reply; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-04 17:15 UTC (permalink / raw)
To: Wolfram Sang
Cc: Igor Korotin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
On Mon Aug 4, 2025 at 6:11 PM CEST, Wolfram Sang wrote:
> Thinking about it, there will not be a lot of patches for I2C Rust, or?
I think the changes to expect are mostly in three categories.
(1) I2C infrastructure changes. For instance, this series contains the bus
abstractions to probe an I2C (client) driver. So, I assume subsequent work
may contain infrastructure for I2C regmap, adapter drivers, xfer
algorithms, etc.
(2) driver-core infrastructure changes related to the device / driver
lifecycle.
(3) Common Rust infrastructure changes.
(2) and (3) may also go through the driver-core and Rust trees, the frequency is
not too high, but I'd expect a few patches every cycle.
As for (1) I know of at least two Rust I2C drivers people are working on,
DS90UB954 (FDP-Link deserializer) and NCV6336, which I think is a buck
converter.
Depending on what Igor works on (I thought I read it somewhere, but do not
remember), we probably get away without adapter drivers and stuff for now.
Besides the I2C infrastructure they require (which doesn't seem too much at a
first glance), I think we also have to consider how drivers should be routed
eventually.
- Danilo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 16:11 ` Wolfram Sang
2025-08-04 17:15 ` Danilo Krummrich
@ 2025-08-04 17:26 ` Igor Korotin
2025-08-04 17:46 ` Miguel Ojeda
2025-08-04 21:57 ` Wolfram Sang
1 sibling, 2 replies; 39+ messages in thread
From: Igor Korotin @ 2025-08-04 17:26 UTC (permalink / raw)
To: Wolfram Sang
Cc: Danilo Krummrich, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
Hello Wolfram
On 8/4/25 17:11, Wolfram Sang wrote:
> So, for starters, we can use my tree in the seperate I2C RUST entry. We
> did a similar thing with the I2C MUX subsubsystem. By the time, we can
> move to a dedicated tree, if there is a need. Thinking about it, there
> will not be a lot of patches for I2C Rust, or?
Can't say for sure. I don’t expect much from my side, but from time to
time there are Rust commits that change multiple subsystems at once —
kind of like a wildcard hit.
Anyway, as far as I see, we can always adjust the workflow later if
needed. For now, we just need to start somewhere.
Thanks,
Igor
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 17:26 ` Igor Korotin
@ 2025-08-04 17:46 ` Miguel Ojeda
2025-08-04 21:57 ` Wolfram Sang
1 sibling, 0 replies; 39+ messages in thread
From: Miguel Ojeda @ 2025-08-04 17:46 UTC (permalink / raw)
To: Igor Korotin
Cc: Wolfram Sang, Danilo Krummrich, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, rust-for-linux
On Mon, Aug 4, 2025 at 7:28 PM Igor Korotin
<igor.korotin.linux@gmail.com> wrote:
>
> Can't say for sure. I don’t expect much from my side, but from time to
> time there are Rust commits that change multiple subsystems at once —
> kind of like a wildcard hit.
Yeah, I think that should be fine, e.g. those would be expected from
time to time in C too. In Rust we may have a few more since things are
still evolving and we try to enable useful lints if possible (since
the early we do it the easier it is).
In some cases, we try to take those "global" cleanups through the Rust
tree with Acked-bys etc. to reduce churn, but it depends on the
particular change.
> Anyway, as far as I see, we can always adjust the workflow later if
> needed. For now, we just need to start somewhere.
Yeah, Wolfram's approach is also a good opportunity to learn on your
side with low stress, i.e. I would usually suggest that when he picks
a patch, then you can try to understand what he did and why, and
whether you can replicate the commit locally as if it were you picking
it up etc.
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 17:26 ` Igor Korotin
2025-08-04 17:46 ` Miguel Ojeda
@ 2025-08-04 21:57 ` Wolfram Sang
1 sibling, 0 replies; 39+ messages in thread
From: Wolfram Sang @ 2025-08-04 21:57 UTC (permalink / raw)
To: Igor Korotin
Cc: Danilo Krummrich, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
[-- Attachment #1: Type: text/plain, Size: 293 bytes --]
> Anyway, as far as I see, we can always adjust the workflow later if
> needed. For now, we just need to start somewhere.
Yes. So, if you send v4 of your patches, please include the linux-i2c
list, so patchwork will pick them up. Reviews from other Rust
maintainers are welcome, of course.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 17:15 ` Danilo Krummrich
@ 2025-08-04 22:01 ` Wolfram Sang
2025-08-05 8:37 ` Igor Korotin
2025-08-05 12:40 ` Daniel Almeida
0 siblings, 2 replies; 39+ messages in thread
From: Wolfram Sang @ 2025-08-04 22:01 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Igor Korotin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
[-- Attachment #1: Type: text/plain, Size: 571 bytes --]
> Besides the I2C infrastructure they require (which doesn't seem too much at a
> first glance), I think we also have to consider how drivers should be routed
> eventually.
Well, I2C client drivers go to their specific subsystem, of course. I2C
controller drivers will go to I2C. But as I can't review them, I need a
maintainer for them. Is Igor willing to do this? Review I2C controller
drivers written in Rust? Or is he only interested in the Rust I2C
bindings? Anything is fine with me, we should only make sure that the
MAINTATINERS file describes this correctly.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 22:01 ` Wolfram Sang
@ 2025-08-05 8:37 ` Igor Korotin
2025-08-05 9:28 ` Wolfram Sang
2025-08-05 12:40 ` Daniel Almeida
1 sibling, 1 reply; 39+ messages in thread
From: Igor Korotin @ 2025-08-05 8:37 UTC (permalink / raw)
To: Wolfram Sang, Danilo Krummrich
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
Hello Wolfram
On 8/4/25 23:01, Wolfram Sang wrote:
>
> Well, I2C client drivers go to their specific subsystem, of course. I2C
> controller drivers will go to I2C. But as I can't review them, I need a
> maintainer for them. Is Igor willing to do this? Review I2C controller
> drivers written in Rust? Or is he only interested in the Rust I2C
> bindings? Anything is fine with me, we should only make sure that the
> MAINTATINERS file describes this correctly.
I'm definitely interested, but going through a tough period right now,
so I’m not sure I can commit fully just yet. Happy to stay involved and
ramp up over time.
Thanks
Igor
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-05 8:37 ` Igor Korotin
@ 2025-08-05 9:28 ` Wolfram Sang
0 siblings, 0 replies; 39+ messages in thread
From: Wolfram Sang @ 2025-08-05 9:28 UTC (permalink / raw)
To: Igor Korotin
Cc: Danilo Krummrich, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux
[-- Attachment #1: Type: text/plain, Size: 903 bytes --]
> > Well, I2C client drivers go to their specific subsystem, of course. I2C
> > controller drivers will go to I2C. But as I can't review them, I need a
> > maintainer for them. Is Igor willing to do this? Review I2C controller
> > drivers written in Rust? Or is he only interested in the Rust I2C
> > bindings? Anything is fine with me, we should only make sure that the
> > MAINTATINERS file describes this correctly.
>
> I'm definitely interested, but going through a tough period right now,
> so I’m not sure I can commit fully just yet. Happy to stay involved and
> ramp up over time.
Perfectly understandable. Let's just deal with it when it happens.
Unlike client drivers, it might take a while until a controller driver
shows up (famous last words). We will figure a way then. Thanks for your
current commitment despite the tough period and all the best going
through it!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-04 22:01 ` Wolfram Sang
2025-08-05 8:37 ` Igor Korotin
@ 2025-08-05 12:40 ` Daniel Almeida
1 sibling, 0 replies; 39+ messages in thread
From: Daniel Almeida @ 2025-08-05 12:40 UTC (permalink / raw)
To: Wolfram Sang
Cc: Danilo Krummrich, Igor Korotin, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, rust-for-linux
Hi,
> On 4 Aug 2025, at 19:01, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
>
>
>> Besides the I2C infrastructure they require (which doesn't seem too much at a
>> first glance), I think we also have to consider how drivers should be routed
>> eventually.
>
> Well, I2C client drivers go to their specific subsystem, of course. I2C
> controller drivers will go to I2C. But as I can't review them, I need a
> maintainer for them. Is Igor willing to do this? Review I2C controller
> drivers written in Rust? Or is he only interested in the Rust I2C
> bindings? Anything is fine with me, we should only make sure that the
> MAINTATINERS file describes this correctly.
>
FWIW, I can help Igor and Danilo as a reviewer here, if needed.
I never wrote a I2C controller driver before, but as most things, it can be
learned. And the review part can fit into the overall bandwidth I have to
review Rust patches in general.
I guess it will make Igor more comfortable by having yet another person to rely
on.
— Daniel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/3] samples: rust: add Rust I2C sample driver
2025-08-01 15:45 ` [PATCH v3 3/3] samples: rust: add Rust I2C sample driver Igor Korotin
2025-08-01 18:09 ` Daniel Almeida
2025-08-02 0:18 ` Danilo Krummrich
@ 2025-08-07 8:42 ` kernel test robot
2 siblings, 0 replies; 39+ messages in thread
From: kernel test robot @ 2025-08-07 8:42 UTC (permalink / raw)
To: Igor Korotin
Cc: oe-lkp, lkp, linux-kernel, rust-for-linux, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, oliver.sang
Hello,
kernel test robot noticed "kernel_BUG_at_rust/helpers/bug.c" on:
commit: 65aee29875487338caa52e3a4ffc41165d4798ad ("[PATCH v3 3/3] samples: rust: add Rust I2C sample driver")
url: https://github.com/intel-lab-lkp/linux/commits/Igor-Korotin/rust-i2c-add-basic-I2C-device-and-driver-abstractions/20250801-234859
patch link: https://lore.kernel.org/all/20250801154506.14810-1-igor.korotin.linux@gmail.com/
patch subject: [PATCH v3 3/3] samples: rust: add Rust I2C sample driver
in testcase: boot
config: x86_64-rhel-9.4-rust
compiler: clang-20
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
+------------------------------------------+------------+------------+
| | 6c0f429b5b | 65aee29875 |
+------------------------------------------+------------+------------+
| boot_successes | 18 | 0 |
| boot_failures | 0 | 18 |
| kernel_BUG_at_rust/helpers/bug.c | 0 | 18 |
| Oops:invalid_opcode:#[##]SMP_PTI | 0 | 18 |
| RIP:rust_helper_BUG | 0 | 18 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 18 |
+------------------------------------------+------------+------------+
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202508071027.8981cbd4-lkp@intel.com
[ 17.294743][ T1] ------------[ cut here ]------------
[ 17.295458][ T1] kernel BUG at rust/helpers/bug.c:7!
[ 17.296172][ T1] Oops: invalid opcode: 0000 [#1] SMP PTI
[ 17.296925][ T1] CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.16.0-08688-g65aee2987548 #1 PREEMPT(voluntary)
[ 17.296974][ T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 17.296974][ T1] RIP: 0010:rust_helper_BUG (rust/helpers/bug.c:7)
[ 17.296974][ T1] Code: 00 00 00 00 00 0f 1f 44 00 00 48 8d 87 10 ff ff ff e9 cf 22 82 00 cc 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 <0f> 0b 66 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 e9 e6 00 0a 00 66
All code
========
0: 00 00 add %al,(%rax)
2: 00 00 add %al,(%rax)
4: 00 0f add %cl,(%rdi)
6: 1f (bad)
7: 44 00 00 add %r8b,(%rax)
a: 48 8d 87 10 ff ff ff lea -0xf0(%rdi),%rax
11: e9 cf 22 82 00 jmp 0x8222e5
16: cc int3
17: 66 66 66 66 66 2e 0f data16 data16 data16 data16 cs nopw 0x0(%rax,%rax,1)
1e: 1f 84 00 00 00 00 00
25: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
2a:* 0f 0b ud2 <-- trapping instruction
2c: 66 0f 1f 84 00 00 00 nopw 0x0(%rax,%rax,1)
33: 00 00
35: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
3a: e9 e6 00 0a 00 jmp 0xa0125
3f: 66 data16
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: 66 0f 1f 84 00 00 00 nopw 0x0(%rax,%rax,1)
9: 00 00
b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
10: e9 e6 00 0a 00 jmp 0xa00fb
15: 66 data16
[ 17.296974][ T1] RSP: 0000:ffffd25cc0013948 EFLAGS: 00010246
[ 17.296974][ T1] RAX: 0000000000000080 RBX: ffffffffb55c2ea0 RCX: 0a0c9e9b2c6f9900
[ 17.296974][ T1] RDX: 0000000000000002 RSI: 00000000ffff7fff RDI: ffffffffb651ce50
[ 17.296974][ T1] RBP: ffffd25cc0013e28 R08: 0000000000007fff R09: ffffffffb645cdf0
[ 17.296974][ T1] R10: 0000000000017ffd R11: 00000000ffff7fff R12: 0000000000000000
[ 17.296974][ T1] R13: 0000000000000000 R14: ffffffffb7172dd4 R15: 0000000000000000
[ 17.296974][ T1] FS: 0000000000000000(0000) GS:ffff8c5bb8b33000(0000) knlGS:0000000000000000
[ 17.296974][ T1] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 17.296974][ T1] CR2: 0000000000000000 CR3: 0000000048420000 CR4: 00000000000406f0
[ 17.296974][ T1] Call Trace:
[ 17.296974][ T1] <TASK>
[ 17.296974][ T1] __rustc::rust_begin_unwind (rust/kernel/lib.rs:213)
[ 17.296974][ T1] ? <&kernel::str::CStr as core::fmt::Display>::fmt (opt/cross/rustc-1.88.0-bindgen-0.72.0/rustup/toolchains/1.88.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2636)
[ 17.296974][ T1] ? work_grab_pending (kernel/workqueue.c:2064)
[ 17.296974][ T1] core::panicking::panic_fmt (opt/cross/rustc-1.88.0-bindgen-0.72.0/rustup/toolchains/1.88.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:75)
[ 17.296974][ T1] core::panicking::panic (opt/cross/rustc-1.88.0-bindgen-0.72.0/rustup/toolchains/1.88.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:145)
[ 17.296974][ T1] core::option::unwrap_failed (opt/cross/rustc-1.88.0-bindgen-0.72.0/rustup/toolchains/1.88.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:2040)
[ 17.296974][ T1] __rust_driver_i2c_init (samples/rust/rust_driver_i2c.rs:104)
[ 17.296974][ T1] ? blake2s_update (lib/crypto/blake2s.c:45)
[ 17.296974][ T1] ? add_device_randomness (drivers/char/random.c:952)
[ 17.296974][ T1] ? <rust_dma::DmaSampleDriver as core::ops::drop::Drop>::drop (rust/kernel/driver.rs:127)
[ 17.296974][ T1] ? <rust_driver_i2c::DriverModule as kernel::InPlaceModule>::init (samples/rust/rust_driver_i2c.rs:104 samples/rust/rust_driver_i2c.rs:104 samples/rust/rust_driver_i2c.rs:104)
[ 17.296974][ T1] do_one_initcall (init/main.c:1269)
[ 17.296974][ T1] ? crng_make_state (include/linux/spinlock.h:406 drivers/char/random.c:363)
[ 17.296974][ T1] ? get_random_u32 (include/linux/string.h:366 include/crypto/chacha.h:119 drivers/char/random.c:425 drivers/char/random.c:554)
[ 17.296974][ T1] ? __get_random_u32_below (drivers/char/random.c:568)
[ 17.296974][ T1] ? allocate_slab (mm/slub.c:2578)
[ 17.296974][ T1] ? sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1050)
[ 17.296974][ T1] ? asm_sysvec_apic_timer_interrupt (arch/x86/include/asm/idtentry.h:702)
[ 17.296974][ T1] ? parameq (kernel/params.c:81 kernel/params.c:91 kernel/params.c:99)
[ 17.296974][ T1] ? parameq (kernel/params.c:90 kernel/params.c:99)
[ 17.296974][ T1] ? do_initcall_level (init/main.c:1315)
[ 17.296974][ T1] ? parse_args (kernel/params.c:153)
[ 17.296974][ T1] do_initcall_level (init/main.c:1330)
[ 17.296974][ T1] do_initcalls (init/main.c:1344)
[ 17.296974][ T1] kernel_init_freeable (init/main.c:1581)
[ 17.296974][ T1] ? rest_init (init/main.c:1461)
[ 17.296974][ T1] kernel_init (init/main.c:1471)
[ 17.296974][ T1] ret_from_fork (arch/x86/kernel/process.c:154)
[ 17.296974][ T1] ? rest_init (init/main.c:1461)
[ 17.296974][ T1] ret_from_fork_asm (arch/x86/entry/entry_64.S:258)
[ 17.296974][ T1] </TASK>
[ 17.296974][ T1] Modules linked in:
[ 17.339320][ T1] ---[ end trace 0000000000000000 ]---
[ 17.340041][ T1] RIP: 0010:rust_helper_BUG (rust/helpers/bug.c:7)
[ 17.340745][ T1] Code: 00 00 00 00 00 0f 1f 44 00 00 48 8d 87 10 ff ff ff e9 cf 22 82 00 cc 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 <0f> 0b 66 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 e9 e6 00 0a 00 66
All code
========
0: 00 00 add %al,(%rax)
2: 00 00 add %al,(%rax)
4: 00 0f add %cl,(%rdi)
6: 1f (bad)
7: 44 00 00 add %r8b,(%rax)
a: 48 8d 87 10 ff ff ff lea -0xf0(%rdi),%rax
11: e9 cf 22 82 00 jmp 0x8222e5
16: cc int3
17: 66 66 66 66 66 2e 0f data16 data16 data16 data16 cs nopw 0x0(%rax,%rax,1)
1e: 1f 84 00 00 00 00 00
25: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
2a:* 0f 0b ud2 <-- trapping instruction
2c: 66 0f 1f 84 00 00 00 nopw 0x0(%rax,%rax,1)
33: 00 00
35: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
3a: e9 e6 00 0a 00 jmp 0xa0125
3f: 66 data16
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: 66 0f 1f 84 00 00 00 nopw 0x0(%rax,%rax,1)
9: 00 00
b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
10: e9 e6 00 0a 00 jmp 0xa00fb
15: 66 data16
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250807/202508071027.8981cbd4-lkp@intel.com
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-02 0:00 ` Danilo Krummrich
2025-08-02 9:07 ` Wolfram Sang
2025-08-04 14:58 ` Igor Korotin
@ 2025-08-15 15:40 ` Igor Korotin
2025-08-15 16:03 ` Danilo Krummrich
2 siblings, 1 reply; 39+ messages in thread
From: Igor Korotin @ 2025-08-15 15:40 UTC (permalink / raw)
To: Danilo Krummrich, Daniel Almeida
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux, Wolfram Sang
Hello Guys
Preparing v4, I keep thinking if for I2C I should rename `i2c::Device`
to `i2c::I2cClient` or `i2c::Client`. The reason why I made it `Device`
for the first iterations is so it would be the same code as for
PCI/Platform code for example.
But the original I2C C code has other terminology. Do you think it worth
keeping it in the Rust code?
Best Regards
Igor
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-15 15:40 ` Igor Korotin
@ 2025-08-15 16:03 ` Danilo Krummrich
2025-08-15 17:04 ` Greg KH
0 siblings, 1 reply; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-15 16:03 UTC (permalink / raw)
To: Igor Korotin
Cc: Daniel Almeida, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, rust-for-linux, Wolfram Sang
On 8/15/25 5:40 PM, Igor Korotin wrote:
> Hello Guys
>
> Preparing v4, I keep thinking if for I2C I should rename `i2c::Device`
> to `i2c::I2cClient` or `i2c::Client`. The reason why I made it `Device`
> for the first iterations is so it would be the same code as for
> PCI/Platform code for example.
> But the original I2C C code has other terminology. Do you think it worth
> keeping it in the Rust code?
I think the reason I2C names this i2c_client and i2c_adapter is that technically
both are I2C devices, hence just i2c_device would have been ambiguous.
What about i2c::ClientDevice and i2c::AdapterDevice?
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-15 16:03 ` Danilo Krummrich
@ 2025-08-15 17:04 ` Greg KH
2025-08-15 17:16 ` Danilo Krummrich
0 siblings, 1 reply; 39+ messages in thread
From: Greg KH @ 2025-08-15 17:04 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Igor Korotin, Daniel Almeida, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, rust-for-linux,
Wolfram Sang
On Fri, Aug 15, 2025 at 06:03:32PM +0200, Danilo Krummrich wrote:
> On 8/15/25 5:40 PM, Igor Korotin wrote:
> > Hello Guys
> >
> > Preparing v4, I keep thinking if for I2C I should rename `i2c::Device`
> > to `i2c::I2cClient` or `i2c::Client`. The reason why I made it `Device`
> > for the first iterations is so it would be the same code as for
> > PCI/Platform code for example.
> > But the original I2C C code has other terminology. Do you think it worth
> > keeping it in the Rust code?
>
> I think the reason I2C names this i2c_client and i2c_adapter is that technically
> both are I2C devices, hence just i2c_device would have been ambiguous.
Yes, when I did that code 20+ years ago, it was hard to pick out good
names. Also we were converting over from the existing non-driver-model
i2c code, which had those names at the time, so we kept them as much as
possible.
> What about i2c::ClientDevice and i2c::AdapterDevice?
I would try to keep them matching the same as the C side, as
i2c_client_device and i2c_adapter_device wouldn't make much sense there
either.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/3] rust: i2c: add basic I2C device and driver abstractions
2025-08-15 17:04 ` Greg KH
@ 2025-08-15 17:16 ` Danilo Krummrich
0 siblings, 0 replies; 39+ messages in thread
From: Danilo Krummrich @ 2025-08-15 17:16 UTC (permalink / raw)
To: Greg KH
Cc: Igor Korotin, Daniel Almeida, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, rust-for-linux,
Wolfram Sang
On 8/15/25 7:04 PM, Greg KH wrote:
> I would try to keep them matching the same as the C side, as
> i2c_client_device and i2c_adapter_device wouldn't make much sense there
> either.
Another option would be separate modules, i.e. i2c::client::Device and
i2c::adapter::Device.
But I guess just i2c::Client and i2c::Adapter are fine too.
The former would fit a bit better into the existing Rust naming scheme, the
latter matches a bit better with the C code.
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2025-08-15 17:16 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-01 15:37 [PATCH v3 0/3] rust: i2c: Add basic I2C driver abstractions Igor Korotin
2025-08-01 15:40 ` [PATCH v3 1/3] rust: i2c: add basic I2C device and " Igor Korotin
2025-08-01 17:14 ` Daniel Almeida
2025-08-02 0:22 ` Danilo Krummrich
2025-08-04 14:16 ` Igor Korotin
2025-08-02 0:00 ` Danilo Krummrich
2025-08-02 9:07 ` Wolfram Sang
2025-08-02 10:51 ` Danilo Krummrich
2025-08-02 12:14 ` Wolfram Sang
2025-08-02 12:41 ` Danilo Krummrich
2025-08-04 14:58 ` Igor Korotin
2025-08-04 15:17 ` Danilo Krummrich
2025-08-04 15:24 ` Miguel Ojeda
2025-08-04 15:40 ` Igor Korotin
2025-08-04 16:11 ` Wolfram Sang
2025-08-04 17:15 ` Danilo Krummrich
2025-08-04 22:01 ` Wolfram Sang
2025-08-05 8:37 ` Igor Korotin
2025-08-05 9:28 ` Wolfram Sang
2025-08-05 12:40 ` Daniel Almeida
2025-08-04 17:26 ` Igor Korotin
2025-08-04 17:46 ` Miguel Ojeda
2025-08-04 21:57 ` Wolfram Sang
2025-08-15 15:40 ` Igor Korotin
2025-08-15 16:03 ` Danilo Krummrich
2025-08-15 17:04 ` Greg KH
2025-08-15 17:16 ` Danilo Krummrich
2025-08-01 15:44 ` [PATCH v3 2/3] rust: i2c: add manual I2C device creation abstractions Igor Korotin
2025-08-01 17:59 ` Daniel Almeida
2025-08-02 0:26 ` Danilo Krummrich
2025-08-04 14:38 ` Igor Korotin
2025-08-02 0:12 ` Danilo Krummrich
2025-08-01 15:45 ` [PATCH v3 3/3] samples: rust: add Rust I2C sample driver Igor Korotin
2025-08-01 18:09 ` Daniel Almeida
2025-08-04 14:43 ` Igor Korotin
2025-08-04 14:58 ` Daniel Almeida
2025-08-02 0:18 ` Danilo Krummrich
2025-08-07 8:42 ` kernel test robot
2025-08-04 15:07 ` [PATCH v3 0/3] rust: i2c: Add basic I2C driver abstractions Daniel Almeida
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).