Linux I2C development
 help / color / mirror / Atom feed
* [RFC PATCH v5 0/3] iio: position: add Rust driver for ams AS5600
@ 2026-08-22  6:26 Muchamad Coirul Anwar
  2026-08-22  6:26 ` [RFC PATCH v5 1/3] i2c: rust: implement SMBus access via IoBackend and FallibleIoCapable Muchamad Coirul Anwar
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Muchamad Coirul Anwar @ 2026-08-22  6:26 UTC (permalink / raw)
  To: jic23, lars
  Cc: linux-iio, linux-kernel, linux-i2c, rust-for-linux, andi.shyti,
	wsa+renesas, ojeda, dakr, igor.korotin, branstj, brucer42,
	Muchamad Coirul Anwar

This is v5 of the Rust driver for the ams AS5600 12-bit magnetic rotary
position sensor.

Link: https://lore.kernel.org/linux-iio/20260707151542.91997-1-muchamadcoirulanwar@gmail.com/

Base tree and dependencies:

  This series is based on driver-core-testing [1], not vanilla rust-next.
  It depends on Gary Guo's io_projection-v6 [2] for IoBackend, IoBase,
  Region, and KnownSize.

  FallibleIoCapable is included in patch 1/3 following Danilo's
  suggestion [3] to carry it as a prerequisite until it lands upstream.

  [1] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/log/?h=driver-core-testing
  [2] https://lore.kernel.org/driver-core/20260706-io_projection-v6-0-72cd5d055d54@garyguo.net/
  [3] https://lore.kernel.org/all/DJVQ852J7SOH.26YBIJTQ9B66G@kernel.org/

Changes since RFC v4:

  I2C abstraction:
  - Switch to FallibleIoCapable — IoCapable is infallible by design,
    but I2C can fail at transport level (Igor)
  - Restrict I/O ops to I2cClient<Bound> (Danilo)
  - Add smbus_read_word() and smbus_read_word_swapped() for odd offsets
  - Add FallibleIoCapable trait to io.rs with blanket impl
  - Add bit_usize() to bits.rs

  IIO abstraction:
  - Replace raw isize mask with IioChanInfo enum (Nuno)
  - Expand PinnedDrop SAFETY comment re: kernfs_drain() (Danilo)
  - build_device() takes modes parameter instead of hardcoding
  - channels() now returns &'static slice

  Driver:
  - Use smbus_read_word_swapped() instead of manual swap_bytes()
  - Use ARef<I2cClient<Bound>> (Danilo)
  - Drop pr_info! debug logging
  - Kconfig cleanup (Jonathan)

  Known limitations (unchanged):
  - No power management
  - No write_raw, buffer, or trigger support

Design notes:

  The IIO abstraction uses iio_device_alloc (not devm_*) so the Rust
  Drop controls cleanup ordering: unregister, drop driver data, then
  free iio_dev.

  iio_device_unregister() drains in-flight sysfs reads via kernfs_drain().
  This is sufficient for INDIO_DIRECT_MODE without buffer/trigger.
  Character device paths need separate analysis.

  Module ownership via __iio_device_register(), not iio_info.owner.

Muchamad Coirul Anwar (3):
  i2c: rust: implement SMBus access via IoBackend and FallibleIoCapable
  rust: add minimal IIO subsystem abstractions
  iio: position: add Rust driver for ams AS5600

 drivers/iio/position/Kconfig    |  11 +
 drivers/iio/position/Makefile   |   1 +
 drivers/iio/position/as5600.rs  | 189 +++++++++++++++++
 rust/bindings/bindings_helper.h |   2 +
 rust/kernel/bits.rs             |  29 +++
 rust/kernel/error.rs            |   1 +
 rust/kernel/i2c.rs              | 302 +++++++++++++++++++++++++++
 rust/kernel/iio.rs              | 384 +++++++++++++++++++++++++++++++++++
 rust/kernel/io.rs               |  66 ++++--
 rust/kernel/lib.rs              |   2 +
 10 files changed, 967 insertions(+), 20 deletions(-)
 create mode 100644 drivers/iio/position/as5600.rs
 create mode 100644 rust/kernel/iio.rs

---
Tested on BeagleBone Black (AM335x), kernel 7.2.0-rc1+,
AS5600 on i2c-2 (0x36) at 3.3V, 6mm diametric neodymium magnet.
Full test session: 2026-08-17.

Build: make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- modules
       (zero warnings, zero errors)

Functional tests:

  1. Probe and registration:
     $ sudo insmod as5600.ko
     $ echo "as5600 0x36" > /sys/bus/i2c/devices/i2c-2/new_device
     $ cat /sys/bus/iio/devices/iio:device0/name
     as5600
     $ ls /sys/bus/i2c/devices/2-0036/driver
     2-0036  bind  module  uevent  unbind

  2. Raw angle and scale (magnet present):
     $ cat /sys/bus/iio/devices/iio:device0/in_angl_raw
     3366
     $ cat /sys/bus/iio/devices/iio:device0/in_angl_scale
     0.001533981
     20 consecutive reads, all within 0-4095.
     Computed: 3366 * 0.001533981 = 5.163 rad (~295.9 degrees).

  3. Unbind/rebind lifecycle (PinnedDrop with ARef cleanup):
     $ echo "2-0036" > /sys/bus/i2c/devices/2-0036/driver/unbind
     $ ls /sys/bus/iio/devices/iio:device0 2>&1
     ls: cannot access '...': No such file or directory
     $ echo "2-0036" > /sys/bus/i2c/drivers/as5600/bind
     $ cat /sys/bus/iio/devices/iio:device0/name
     as5600
     $ dmesg | grep -i "oops\|panic\|bug:"
     (empty)

  4. Concurrent stress (Mutex serialization under contention):
     10 parallel readers hammering in_angl_raw for 5 seconds.
     Repeated as 20-cycle unbind/rebind loop with readers active
     throughout (8 seconds total).
     $ dmesg | grep -i "oops\|panic\|bug:\|rcu"
     (empty)

  5. Module removal under active I/O:
     a) rmmod while flood readers are running — no crash.
        iio_device_unregister() drains in-flight read_raw callbacks
        before PinnedDrop proceeds; subsequent reads return ENOENT.
     b) rmmod while a sysfs fd is held open — no crash.
        iio_dev kref not released until fd is closed.
     $ dmesg | grep -i "oops\|panic\|bug:"
     (empty)

  6. Lifecycle stress:
     50x rapid unbind/rebind — no crash, device functional after all
     cycles.
     50x insmod/rmmod — no crash.
     $ dmesg | grep -i "oops\|panic\|bug:"
     (empty)

  7. I2C bus disconnect:
     SCL/SDA physically pulled while read loop is running. Driver
     returns "Remote I/O error" immediately on each failed transfer;
     no hang, no internal retry loop. Cable reconnected — reads resume
     from the next iteration without rmmod (about 2 seconds downtime).
     rmmod issued while bus still in error state — exits cleanly.
     ARef<I2cClient> drop is put_device() only, no bus transaction.
     $ dmesg | grep -i "oops\|panic\|bug:"
     (empty)

  8. Memory and locking:
     dmesg contains no strings matching "KASAN:" or "possible deadlock".
     The test kernel was not built with CONFIG_KASAN or CONFIG_PROVE_LOCKING;
     the above is a pattern match against kernel log output, not sanitizer
     or lockdep instrumentation.
     kmemleak not available (CONFIG_DEBUG_KMEMLEAK not set).

-- 
2.50.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH v5 1/3] i2c: rust: implement SMBus access via IoBackend and FallibleIoCapable
  2026-08-22  6:26 [RFC PATCH v5 0/3] iio: position: add Rust driver for ams AS5600 Muchamad Coirul Anwar
@ 2026-08-22  6:26 ` Muchamad Coirul Anwar
  2026-08-23 23:41   ` Jonathan Cameron
  2026-08-22  6:26 ` [RFC PATCH v5 2/3] rust: add minimal IIO subsystem abstractions Muchamad Coirul Anwar
  2026-08-22  6:26 ` [RFC PATCH v5 3/3] iio: position: add Rust driver for ams AS5600 Muchamad Coirul Anwar
  2 siblings, 1 reply; 7+ messages in thread
From: Muchamad Coirul Anwar @ 2026-08-22  6:26 UTC (permalink / raw)
  To: jic23, lars
  Cc: linux-iio, linux-kernel, linux-i2c, rust-for-linux, andi.shyti,
	wsa+renesas, ojeda, dakr, igor.korotin, branstj, brucer42,
	Muchamad Coirul Anwar

Implement SMBus byte and word read/write operations for I2cClient using
the FallibleIoCapable trait from the generic I/O backend infrastructure.

I2cClient now exposes an I2cBackend that implements FallibleIoCapable<u8>
and FallibleIoCapable<u16>, replacing the previous IoCapable approach.
I2C/SMBus bus transactions are inherently fallible (NACK, arbitration
loss, timeout), so the infallible IoCapable is not appropriate here.
FallibleIoCapable carries the errno from i2c_smbus_read_byte_data and
i2c_smbus_read_word_data directly to the caller via Result<T>.

The implementation is restricted to I2cClient<Bound> as I/O operations
require a live device context.

I2cClient<Bound>::smbus_io() returns an I2cView handle for use with the
generic try_read8/try_read16 methods. Two standalone methods are also
provided for odd-offset word access that bypasses the alignment check
in the Io trait:

  smbus_read_word()        - CPU-native byte order (SMBus LE wire format)
  smbus_read_word_swapped() - byte-swapped result for big-endian devices

maxsize is 256, covering the SMBus command byte range 0x00-0xFF. This
is the command byte space, not the 7-bit device address which is handled
by the I2C core at adapter level.

Link: https://lore.kernel.org/rust-for-linux/20260131-i2c-adapter-v1-4-5a436e34cd1a@gmail.com/
Link: https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/commit/?h=driver-core-testing&id=121d87b28e1d9061d3aaa156c43a627d3cb5e620
Suggested-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
---
 rust/kernel/bits.rs |  29 +++++
 rust/kernel/i2c.rs  | 302 ++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/io.rs   |  66 +++++++---
 3 files changed, 377 insertions(+), 20 deletions(-)

diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
index 2daead125626..a6537a668dd6 100644
--- a/rust/kernel/bits.rs
+++ b/rust/kernel/bits.rs
@@ -41,6 +41,7 @@ pub const fn [<bit_ $ty>](n: u32) -> $ty {
 impl_bit_fn!(u32);
 impl_bit_fn!(u16);
 impl_bit_fn!(u8);
+impl_bit_fn!(usize);
 
 macro_rules! impl_genmask_fn {
     (
@@ -203,3 +204,31 @@ pub const fn [<genmask_ $ty>](range: RangeInclusive<u32>) -> $ty {
     /// assert_eq!(genmask_u8(0..=7), u8::MAX);
     /// ```
 );
+
+impl_genmask_fn!(
+    usize,
+    /// # Examples
+    ///
+    /// ```
+    /// # #![expect(clippy::reversed_empty_ranges)]
+    /// # use kernel::bits::genmask_checked_usize;
+    /// assert_eq!(genmask_checked_usize(0..=0), Some(0b1));
+    /// assert_eq!(genmask_checked_usize(0..=3), Some(0b1111));
+    /// assert_eq!(genmask_checked_usize(1..=3), Some(0b1110));
+    ///
+    /// // `200` is out of the supported bit range on all platforms.
+    /// assert_eq!(genmask_checked_usize(0..=200), None);
+    ///
+    /// // Invalid range where the start is bigger than the end.
+    /// assert_eq!(genmask_checked_usize(5..=2), None);
+    /// ```
+    ,
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::bits::genmask_usize;
+    /// assert_eq!(genmask_usize(0..=0), 0b1);
+    /// assert_eq!(genmask_usize(0..=3), 0b1111);
+    /// assert_eq!(genmask_usize(1..=3), 0b1110);
+    /// ```
+);
diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index 624b971ca8b0..f939907573a6 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -14,8 +14,15 @@
     devres::Devres,
     driver,
     error::*,
+    io::{
+        FallibleIoCapable,
+        IoBackend,
+        IoBase,
+        Region, //
+    },
     of,
     prelude::*,
+    ptr::KnownSize,
     sync::aref::{
         ARef,
         AlwaysRefCounted, //
@@ -601,3 +608,298 @@ unsafe impl Send for Registration {}
 // SAFETY: `Registration` offers no interior mutability (no mutation through &self
 // and no mutable access is exposed)
 unsafe impl Sync for Registration {}
+
+// SAFETY: `I2cClient<Bound>` wraps a kernel `struct i2c_client`. The I2C core
+// and bus locking mechanisms ensure that the underlying client structure can
+// be safely transferred between threads.
+unsafe impl Send for I2cClient<device::Bound> {}
+
+// SAFETY: `I2cClient<Bound>` wraps a kernel `struct i2c_client`. All methods
+// that access the client go through kernel I2C core functions that provide
+// their own synchronization. No &self method exposes interior mutability.
+unsafe impl Sync for I2cClient<device::Bound> {}
+
+// SAFETY: `I2cClient<Bound>` is always reference-counted via the embedded
+// `struct device`. `get_device`/`put_device` increment and decrement the
+// device refcount atomically. A separate impl is needed for `I2cClient<Bound>`
+// because `AlwaysRefCounted` is not implemented generically over all
+// `DeviceContext`s — only the specific contexts that are safe to refcount
+// from arbitrary threads.
+unsafe impl AlwaysRefCounted for I2cClient<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()) };
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::put_device(&raw mut (*obj.as_ref().as_raw()).dev) }
+    }
+}
+
+/// I/O backend for SMBus register access via I2C.
+///
+/// This backend implements only [`FallibleIoCapable`] and not [`IoCapable`],
+/// because I2C/SMBus bus transactions are inherently fallible — NACK,
+/// arbitration loss, and timeout can occur regardless of address validity.
+/// The infallible [`Io::read`], [`Io::write`], and [`Io::update`] methods
+/// are therefore compile-time unavailable for this backend.
+pub struct I2cBackend;
+
+/// View type for [`I2cBackend`], carrying a reference to an I2C client and
+/// a fake pointer that encodes the register offset and address-space size
+/// as fat-pointer metadata.
+///
+/// The pointer field is never dereferenced. After [`IoBackend::project_view`]
+/// projects an offset into the pointer, `addr()` yields that offset as the
+/// SMBus command byte. [`KnownSize::size()`] reads the fat-pointer metadata
+/// length (256 for the SMBus command space).
+///
+/// # Invariants
+///
+/// `ptr` is a non-dereferenceable fat pointer. Its address component encodes
+/// the SMBus register offset (0..=255) after [`IoBackend::project_view`]
+/// projection; its length metadata is 256 (the SMBus command byte address
+/// space). `client` points to a valid `I2cClient<Bound>` that remains live
+/// for `'a`.
+pub struct I2cView<'a, T: ?Sized> {
+    client: &'a I2cClient<device::Bound>,
+    ptr: *mut T,
+}
+
+impl<T: ?Sized> Copy for I2cView<'_, T> {}
+
+impl<T: ?Sized> Clone for I2cView<'_, T> {
+    #[inline]
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+
+impl IoBackend for I2cBackend {
+    type View<'a, T: ?Sized + KnownSize> = I2cView<'a, T>;
+
+    #[inline]
+    fn as_ptr<'a, T: ?Sized + KnownSize>(view: Self::View<'a, T>) -> *mut T {
+        view.ptr
+    }
+
+    #[inline]
+    unsafe fn project_view<'a, T: ?Sized + KnownSize, U: ?Sized + KnownSize>(
+        view: Self::View<'a, T>,
+        ptr: *mut U,
+    ) -> Self::View<'a, U> {
+        // INVARIANT: Per safety requirement.
+        I2cView {
+            client: view.client,
+            ptr,
+        }
+    }
+}
+
+impl FallibleIoCapable<u8> for I2cBackend {
+    #[inline]
+    fn io_try_read<'a>(view: I2cView<'a, u8>) -> Result<u8> {
+        // `io_view()` ensures `offset + 1 <= 256`, so `addr()` is at most 255;
+        // the `as u8` cast below is therefore lossless.
+        let reg = Self::as_ptr(view).addr() as u8;
+        // SAFETY: `view.client.as_raw()` returns a valid `*mut struct i2c_client`
+        // pointer as guaranteed by the type invariant of `I2cClient`.
+        // `i2c_smbus_read_byte_data` is safe to call with any valid client pointer
+        // and any u8 command byte.
+        let ret = unsafe { bindings::i2c_smbus_read_byte_data(view.client.as_raw(), reg) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u8)
+        }
+    }
+
+    #[inline]
+    fn io_try_write<'a>(view: I2cView<'a, u8>, value: u8) -> Result {
+        // `io_view()` ensures `offset + 1 <= 256`, so `addr()` is at most 255;
+        // the `as u8` cast below is therefore lossless.
+        let reg = Self::as_ptr(view).addr() as u8;
+        // SAFETY: `view.client.as_raw()` returns a valid `*mut struct i2c_client`
+        // pointer as guaranteed by the type invariant of `I2cClient`.
+        // `i2c_smbus_write_byte_data` is safe to call with any valid client pointer
+        // and any u8 command byte and value.
+        let ret = unsafe { bindings::i2c_smbus_write_byte_data(view.client.as_raw(), reg, value) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+}
+
+impl FallibleIoCapable<u16> for I2cBackend {
+    #[inline]
+    fn io_try_read<'a>(view: I2cView<'a, u16>) -> Result<u16> {
+        // `io_view()` ensures `offset + 2 <= 256`, so `addr()` is at most 254;
+        // the `as u8` cast below is therefore lossless.
+        let reg = Self::as_ptr(view).addr() as u8;
+        // SAFETY: `view.client.as_raw()` returns a valid `*mut struct i2c_client`
+        // pointer as guaranteed by the type invariant of `I2cClient`.
+        // `i2c_smbus_read_word_data` is safe to call with any valid client pointer
+        // and any u8 command byte.
+        let ret = unsafe { bindings::i2c_smbus_read_word_data(view.client.as_raw(), reg) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    #[inline]
+    fn io_try_write<'a>(view: I2cView<'a, u16>, value: u16) -> Result {
+        // `io_view()` ensures `offset + 2 <= 256`, so `addr()` is at most 254;
+        // the `as u8` cast below is therefore lossless.
+        let reg = Self::as_ptr(view).addr() as u8;
+        // SAFETY: `view.client.as_raw()` returns a valid `*mut struct i2c_client`
+        // pointer as guaranteed by the type invariant of `I2cClient`.
+        // `i2c_smbus_write_word_data` is safe to call with any valid client pointer
+        // and any u8 command byte and u16 value.
+        let ret = unsafe { bindings::i2c_smbus_write_word_data(view.client.as_raw(), reg, value) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(())
+        }
+    }
+}
+
+impl<'a, T: ?Sized + KnownSize> IoBase<'a> for I2cView<'a, T> {
+    type Backend = I2cBackend;
+    type Target = T;
+
+    #[inline]
+    fn as_view(self) -> I2cView<'a, T> {
+        self
+    }
+}
+
+// SAFETY: `I2cView` contains `&'a I2cClient<Bound>` (which is `Send` because
+// `I2cClient<Bound>: Sync`) and `*mut T`. The raw pointer is never
+// dereferenced — it only encodes the SMBus register offset as its address.
+// With `T: Sync`, moving the view to another thread cannot cause data races.
+unsafe impl<T: ?Sized + Sync> Send for I2cView<'_, T> {}
+
+// SAFETY: `I2cView` contains `&'a I2cClient<Bound>` (which is `Sync`) and
+// `*mut T`. The raw pointer is never dereferenced; sharing an `&I2cView`
+// across threads is equivalent to sharing `&I2cClient<Bound>` and a
+// read-only address value. `T: Sync` ensures the addressed data is
+// safe to access from multiple threads.
+unsafe impl<T: ?Sized + Sync> Sync for I2cView<'_, T> {}
+
+impl I2cClient<device::Bound> {
+    /// Returns an I/O handle for SMBus register access on this I2C client.
+    ///
+    /// The returned handle provides fallible read/write methods for the
+    /// 256-byte SMBus command address space (0x00–0xFF). This is the SMBus
+    /// command byte range, NOT the 7-bit device address, which is handled
+    /// by the I2C core at the adapter level.
+    ///
+    /// Note: [`Io::try_read16`] and [`Io::try_write16`] on the returned handle
+    /// reject odd offsets. The underlying [`Region`] base address is 0, so
+    /// [`offset_valid`] checks `(0 + offset) % 2 == 0` — only even offsets
+    /// pass. For word-sized access to odd-offset registers use
+    /// [`smbus_read_word`] or [`smbus_read_word_swapped`] instead.
+    ///
+    /// The underlying pointer in the returned [`I2cView`] is never
+    /// dereferenced; it encodes the register address space size as
+    /// fat-pointer metadata and the register offset as the pointer address.
+    ///
+    /// [`smbus_read_word`]: Self::smbus_read_word
+    /// [`smbus_read_word_swapped`]: Self::smbus_read_word_swapped
+    #[inline]
+    pub fn smbus_io(&self) -> I2cView<'_, Region<256>> {
+        // INVARIANT: `client` is `self`, a valid `I2cClient<Bound>`.
+        //
+        // `ptr` is a "fake pointer" — it is constructed solely to carry two
+        // pieces of metadata through the `IoBase` machinery:
+        //   - address component: 0 initially; after each `project_view` call,
+        //     this becomes the register offset (the SMBus command byte).
+        //   - length metadata: 256, encoding the SMBus command address space
+        //     size so `io_view()` can bounds-check offsets.
+        //
+        // `without_provenance_mut(0)` produces a pointer with no memory
+        // provenance — it cannot be used to read or write memory. This is safe
+        // because `I2cBackend::as_ptr()` extracts the address as a `usize`
+        // offset and passes it to `i2c_smbus_*` functions, never dereferencing
+        // the pointer itself. Using a provenance-free base avoids accidentally
+        // creating a pointer that appears to alias real memory.
+        I2cView {
+            client: self,
+            ptr: Region::<256>::ptr_from_raw_parts_mut(core::ptr::without_provenance_mut(0), 256),
+        }
+    }
+
+    /// Reads a 16-bit word from an SMBus register in CPU-native byte order.
+    ///
+    /// Wraps `i2c_smbus_read_word_data`. The `reg` parameter is the SMBus
+    /// command byte (0x00–0xFF) — an instruction sent to the device over the
+    /// serial bus, not a memory address. There is no alignment requirement:
+    /// any command byte value is valid regardless of whether it is odd or even.
+    ///
+    /// SMBus transmits the low byte first (little-endian on the wire), and this
+    /// method returns the value in CPU-native byte order without further
+    /// conversion. Use [`Self::smbus_read_word_swapped`] for devices that store
+    /// multi-byte registers in big-endian (MSB-first) format.
+    ///
+    /// Returns `Err` if the bus transaction fails (e.g. NACK, arbitration loss,
+    /// or timeout).
+    #[inline]
+    pub fn smbus_read_word(&self, reg: u8) -> Result<u16> {
+        // SAFETY: `self.as_raw()` returns a valid `*mut struct i2c_client`
+        // pointer as guaranteed by the type invariant of `I2cClient`.
+        // `i2c_smbus_read_word_data` is safe to call with any valid client
+        // pointer and any u8 command byte.
+        let ret = unsafe { bindings::i2c_smbus_read_word_data(self.as_raw(), reg) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Reads a 16-bit word from an SMBus register with bytes unconditionally
+    /// swapped.
+    ///
+    /// Wraps `i2c_smbus_read_word_data` and applies [`u16::swap_bytes`] to the
+    /// result. Use this for devices that store multi-byte registers in
+    /// big-endian (MSB-first) format, which is common among I2C sensors whose
+    /// datasheets do not reference the SMBus specification.
+    ///
+    /// The swap is **unconditional** — it is not equivalent to `be16_to_cpu`.
+    /// On a big-endian CPU, `be16_to_cpu` would be a no-op, but this method
+    /// still swaps. The reason: SMBus always transmits the low byte first, so
+    /// the driver always receives data in little-endian wire order regardless
+    /// of CPU endianness. The swap corrects for the device's wire-level byte
+    /// order, not the CPU's native order.
+    ///
+    /// The `reg` parameter is the SMBus command byte (0x00–0xFF). There is no
+    /// alignment requirement; any command byte value is valid.
+    ///
+    /// Returns `Err` if the bus transaction fails (e.g. NACK, arbitration loss,
+    /// or timeout).
+    ///
+    /// # Example
+    ///
+    /// ```ignore
+    /// // AS5600 stores the 12-bit raw angle big-endian at register 0x0C.
+    /// let raw = client.smbus_read_word_swapped(0x0C)?;
+    /// let angle = raw & 0x0FFF;
+    /// ```
+    #[inline]
+    pub fn smbus_read_word_swapped(&self, reg: u8) -> Result<u16> {
+        // SAFETY: `self.as_raw()` returns a valid `*mut struct i2c_client`
+        // pointer as guaranteed by the type invariant of `I2cClient`.
+        let ret = unsafe { bindings::i2c_smbus_read_word_data(self.as_raw(), reg) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok((ret as u16).swap_bytes())
+        }
+    }
+}
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 95f46bb75f9e..516895ca2082 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -276,6 +276,36 @@ pub trait IoCapable<T>: IoBackend {
     fn io_write<'a>(view: Self::View<'a, T>, value: T);
 }
 
+/// Fallible counterpart of [`IoCapable`] for I/O backends where operations can fail at the
+/// transport level (e.g. I2C, SPI).
+///
+/// Infallible backends ([`IoCapable`] implementors) get this for free via blanket implementation.
+/// Fallible-only backends implement this trait directly without implementing [`IoCapable`]; the
+/// infallible [`Io::read`], [`Io::write`], and [`Io::update`] methods will then be unavailable,
+/// enforcing that callers use the `try_*` variants instead.
+pub trait FallibleIoCapable<T>: IoBackend {
+    /// Performs an I/O read of type `T` at `view` and returns the result, or an error if the
+    /// transport-level operation fails.
+    fn io_try_read<'a>(view: Self::View<'a, T>) -> Result<T>;
+
+    /// Performs an I/O write of `value` at `view`, or returns an error if the transport-level
+    /// operation fails.
+    fn io_try_write<'a>(view: Self::View<'a, T>, value: T) -> Result;
+}
+
+impl<B: IoCapable<T>, T> FallibleIoCapable<T> for B {
+    #[inline(always)]
+    fn io_try_read<'a>(view: Self::View<'a, T>) -> Result<T> {
+        Ok(Self::io_read(view))
+    }
+
+    #[inline(always)]
+    fn io_try_write<'a>(view: Self::View<'a, T>, value: T) -> Result {
+        Self::io_write(view, value);
+        Ok(())
+    }
+}
+
 /// Trait indicating that an I/O backend supports memory copy operations.
 pub trait IoCopyable: IoBackend {
     /// Copy contents of `view` to `buffer`.
@@ -645,7 +675,7 @@ fn copy_to_slice(self, data: &mut [u8])
     fn try_read8(self, offset: usize) -> Result<u8>
     where
         usize: IoLoc<Self::Target, u8, IoType = u8>,
-        Self::Backend: IoCapable<u8>,
+        Self::Backend: FallibleIoCapable<u8>,
     {
         self.try_read(offset)
     }
@@ -655,7 +685,7 @@ fn try_read8(self, offset: usize) -> Result<u8>
     fn try_read16(self, offset: usize) -> Result<u16>
     where
         usize: IoLoc<Self::Target, u16, IoType = u16>,
-        Self::Backend: IoCapable<u16>,
+        Self::Backend: FallibleIoCapable<u16>,
     {
         self.try_read(offset)
     }
@@ -665,7 +695,7 @@ fn try_read16(self, offset: usize) -> Result<u16>
     fn try_read32(self, offset: usize) -> Result<u32>
     where
         usize: IoLoc<Self::Target, u32, IoType = u32>,
-        Self::Backend: IoCapable<u32>,
+        Self::Backend: FallibleIoCapable<u32>,
     {
         self.try_read(offset)
     }
@@ -675,7 +705,7 @@ fn try_read32(self, offset: usize) -> Result<u32>
     fn try_read64(self, offset: usize) -> Result<u64>
     where
         usize: IoLoc<Self::Target, u64, IoType = u64>,
-        Self::Backend: IoCapable<u64>,
+        Self::Backend: FallibleIoCapable<u64>,
     {
         self.try_read(offset)
     }
@@ -685,7 +715,7 @@ fn try_read64(self, offset: usize) -> Result<u64>
     fn try_write8(self, value: u8, offset: usize) -> Result
     where
         usize: IoLoc<Self::Target, u8, IoType = u8>,
-        Self::Backend: IoCapable<u8>,
+        Self::Backend: FallibleIoCapable<u8>,
     {
         self.try_write(offset, value)
     }
@@ -695,7 +725,7 @@ fn try_write8(self, value: u8, offset: usize) -> Result
     fn try_write16(self, value: u16, offset: usize) -> Result
     where
         usize: IoLoc<Self::Target, u16, IoType = u16>,
-        Self::Backend: IoCapable<u16>,
+        Self::Backend: FallibleIoCapable<u16>,
     {
         self.try_write(offset, value)
     }
@@ -705,7 +735,7 @@ fn try_write16(self, value: u16, offset: usize) -> Result
     fn try_write32(self, value: u32, offset: usize) -> Result
     where
         usize: IoLoc<Self::Target, u32, IoType = u32>,
-        Self::Backend: IoCapable<u32>,
+        Self::Backend: FallibleIoCapable<u32>,
     {
         self.try_write(offset, value)
     }
@@ -715,7 +745,7 @@ fn try_write32(self, value: u32, offset: usize) -> Result
     fn try_write64(self, value: u64, offset: usize) -> Result
     where
         usize: IoLoc<Self::Target, u64, IoType = u64>,
-        Self::Backend: IoCapable<u64>,
+        Self::Backend: FallibleIoCapable<u64>,
     {
         self.try_write(offset, value)
     }
@@ -827,10 +857,10 @@ fn write64(self, value: u64, offset: usize)
     fn try_read<T, L>(self, location: L) -> Result<T>
     where
         L: IoLoc<Self::Target, T>,
-        Self::Backend: IoCapable<L::IoType>,
+        Self::Backend: FallibleIoCapable<L::IoType>,
     {
         let view = io_view::<Self, L::IoType>(self, location.offset())?;
-        Ok(Self::Backend::io_read(view).into())
+        Ok(Self::Backend::io_try_read(view)?.into())
     }
 
     /// Generic fallible write with runtime bounds check.
@@ -860,12 +890,11 @@ fn try_read<T, L>(self, location: L) -> Result<T>
     fn try_write<T, L>(self, location: L, value: T) -> Result
     where
         L: IoLoc<Self::Target, T>,
-        Self::Backend: IoCapable<L::IoType>,
+        Self::Backend: FallibleIoCapable<L::IoType>,
     {
         let view = io_view::<Self, L::IoType>(self, location.offset())?;
         let io_value = value.into();
-        Self::Backend::io_write(view, io_value);
-        Ok(())
+        Self::Backend::io_try_write(view, io_value)
     }
 
     /// Generic fallible write of a fully-located register value.
@@ -905,7 +934,7 @@ fn try_write_reg<T, L, V>(self, value: V) -> Result
     where
         L: IoLoc<Self::Target, T>,
         V: LocatedRegister<Self::Target, Location = L, Value = T>,
-        Self::Backend: IoCapable<L::IoType>,
+        Self::Backend: FallibleIoCapable<L::IoType>,
     {
         let (location, value) = value.into_io_op();
 
@@ -938,16 +967,13 @@ fn try_write_reg<T, L, V>(self, value: V) -> Result
     fn try_update<T, L, F>(self, location: L, f: F) -> Result
     where
         L: IoLoc<Self::Target, T>,
-        Self::Backend: IoCapable<L::IoType>,
+        Self::Backend: FallibleIoCapable<L::IoType>,
         F: FnOnce(T) -> T,
     {
         let view = io_view::<Self, L::IoType>(self, location.offset())?;
-
-        let value: T = Self::Backend::io_read(view).into();
+        let value: T = Self::Backend::io_try_read(view)?.into();
         let io_value = f(value).into();
-        Self::Backend::io_write(view, io_value);
-
-        Ok(())
+        Self::Backend::io_try_write(view, io_value)
     }
 
     /// Generic infallible read with compile-time bounds check.
-- 
2.50.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC PATCH v5 2/3] rust: add minimal IIO subsystem abstractions
  2026-08-22  6:26 [RFC PATCH v5 0/3] iio: position: add Rust driver for ams AS5600 Muchamad Coirul Anwar
  2026-08-22  6:26 ` [RFC PATCH v5 1/3] i2c: rust: implement SMBus access via IoBackend and FallibleIoCapable Muchamad Coirul Anwar
@ 2026-08-22  6:26 ` Muchamad Coirul Anwar
  2026-08-24  0:07   ` Jonathan Cameron
  2026-08-22  6:26 ` [RFC PATCH v5 3/3] iio: position: add Rust driver for ams AS5600 Muchamad Coirul Anwar
  2 siblings, 1 reply; 7+ messages in thread
From: Muchamad Coirul Anwar @ 2026-08-22  6:26 UTC (permalink / raw)
  To: jic23, lars
  Cc: linux-iio, linux-kernel, linux-i2c, rust-for-linux, andi.shyti,
	wsa+renesas, ojeda, dakr, igor.korotin, branstj, brucer42,
	Muchamad Coirul Anwar

Add safe Rust wrappers for the Linux IIO (Industrial I/O) subsystem:

- IioChanInfo enum wrapping iio_chan_info_enum, with TryFrom<u32> for
  type-safe dispatch in read_raw. The compiler enforces match
  exhaustiveness, replacing the previous raw isize approach.
- IioVal enum with NonZeroI32 for division-by-zero prevention on
  IIO_VAL_FRACTIONAL.
- IioDriver trait with read_raw callback (requires Send + Sync).
- Device<T, State> with typestate (Unregistered -> Registered) to
  prevent double-registration at compile time.
- PinnedDrop for guaranteed cleanup sequence:
    iio_device_unregister -> drop_in_place(T) -> iio_device_free
  iio_device_unregister() calls cdev_device_del() which drains the
  kernfs workqueue before returning. All in-flight read_raw callbacks
  (which go through kernfs sysfs reads) complete before drop_in_place
  proceeds. This covers the sysfs read path used by this driver.
- Compile-time const VTABLE (iio_info).
- C-to-Rust FFI trampoline for read_raw dispatch.

The abstraction uses iio_device_alloc (not devm_*) so that the Rust
Drop implementation has full control over the cleanup sequence.
Module ownership is enforced via __iio_device_register(indio_dev, module).

Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
---
 rust/bindings/bindings_helper.h |   2 +
 rust/kernel/error.rs            |   1 +
 rust/kernel/iio.rs              | 384 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   2 +
 4 files changed, 389 insertions(+)
 create mode 100644 rust/kernel/iio.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 1124785e210b..f311959bab18 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -62,6 +62,8 @@
 #include <linux/firmware.h>
 #include <linux/fs.h>
 #include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
 #include <linux/interrupt.h>
 #include <linux/io-pgtable.h>
 #include <linux/ioport.h>
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index a56ba6309594..5dc917d92151 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -86,6 +86,7 @@ macro_rules! declare_err {
     declare_err!(EIOCBQUEUED, "iocb queued, will get completion event.");
     declare_err!(ERECALLCONFLICT, "Conflict with recalled state.");
     declare_err!(ENOGRACE, "NFS file lock reclaim refused.");
+    declare_err!(ENODATA, "No data available.");
 }
 
 /// Generic integer kernel error.
diff --git a/rust/kernel/iio.rs b/rust/kernel/iio.rs
new file mode 100644
index 000000000000..f1638160fed1
--- /dev/null
+++ b/rust/kernel/iio.rs
@@ -0,0 +1,384 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2026 Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
+//! IIO subsystem abstractions.
+//!
+//! Minimal safe Rust wrappers for the Linux IIO (Industrial I/O) subsystem.
+//! Provides [`Device`] for allocating and registering an IIO device, and the
+//! [`IioDriver`] trait for implementing `read_raw` callbacks in safe Rust.
+
+use crate::{
+    bindings::{
+        __iio_device_register,
+        iio_chan_spec,
+        iio_dev,
+        iio_device_alloc,
+        iio_device_free,
+        iio_device_unregister,
+        iio_info, //
+    },
+    device,
+    error::{
+        code::*,
+        to_result,
+        Result, //
+    },
+    prelude::*,
+    ThisModule, //
+};
+
+use core::{
+    ffi::c_int,
+    marker::PhantomData,
+    mem::{
+        forget,
+        size_of,
+        zeroed, //
+    },
+    num::NonZeroI32,
+    pin::Pin,
+    ptr::drop_in_place, //
+};
+
+use pin_init::{
+    pin_data,
+    pinned_drop, //
+};
+
+/// IIO value type: single integer (`IIO_VAL_INT`).
+pub const IIO_VAL_INT: c_int = crate::bindings::IIO_VAL_INT as c_int;
+/// IIO value type: integer plus micro part (`IIO_VAL_INT_PLUS_MICRO`).
+pub const IIO_VAL_INT_PLUS_MICRO: c_int = crate::bindings::IIO_VAL_INT_PLUS_MICRO as c_int;
+/// IIO value type: integer plus nano part (`IIO_VAL_INT_PLUS_NANO`).
+pub const IIO_VAL_INT_PLUS_NANO: c_int = crate::bindings::IIO_VAL_INT_PLUS_NANO as c_int;
+/// IIO value type: fractional (`IIO_VAL_FRACTIONAL`).
+pub const IIO_VAL_FRACTIONAL: c_int = crate::bindings::IIO_VAL_FRACTIONAL as c_int;
+
+/// Generates a Rust enum wrapper for C `enum iio_chan_info_enum`.
+///
+/// This macro creates a type-safe enum with automatic `TryFrom<u32>`
+/// conversion. Drivers match directly on `IioChanInfo` variants in
+/// `read_raw`, and the compiler enforces match exhaustiveness.
+/// Additional variants can be added as drivers require them.
+macro_rules! build_iio_enum {
+    (
+        $(
+            $(#[$meta:meta])*
+            $rust_name:ident = $c_const:ident
+        ),* $(,)?
+    ) => {
+        /// Channel info attribute selector for [`IioDriver::read_raw`].
+        ///
+        /// Wraps C `enum iio_chan_info_enum` values. The IIO core passes this
+        /// to `read_raw` to indicate which attribute userspace is reading
+        /// (e.g., raw value, scale factor, offset).
+        ///
+        /// Currently covers the subset needed by in-tree Rust drivers.
+        /// Additional variants from `include/linux/iio/types.h` can be
+        /// added as needed.
+        #[repr(u32)]
+        #[derive(Debug, Clone, Copy, PartialEq, Eq)]
+        pub enum IioChanInfo {
+            $(
+                $(#[$meta])*
+                $rust_name = bindings::$c_const,
+            )*
+        }
+        impl TryFrom<u32> for IioChanInfo {
+            type Error = Error;
+            fn try_from(value: u32) -> Result<Self, Self::Error> {
+                match value {
+                    $( bindings::$c_const => Ok(IioChanInfo::$rust_name), )*
+                    _ => Err(EINVAL),
+                }
+            }
+        }
+    };
+}
+
+build_iio_enum! {
+    /// Raw unprocessed value from the channel (`IIO_CHAN_INFO_RAW`).
+    ///
+    /// For sensors, this is typically the ADC reading or register value
+    /// before any scaling or offset correction.
+    Raw = iio_chan_info_enum_IIO_CHAN_INFO_RAW,
+    /// Scale factor to convert raw values to SI units (`IIO_CHAN_INFO_SCALE`).
+    ///
+    /// The processed value is `raw * scale`. The unit depends on the channel
+    /// type (e.g. V for voltage, m/s² for acceleration, rad for angle).
+    Scale = iio_chan_info_enum_IIO_CHAN_INFO_SCALE,
+}
+
+/// Represents the return value of a `read_raw` operation.
+///
+/// Each variant corresponds to an `IIO_VAL_*` constant and tells the
+/// IIO core how to format `val` and `val2` for userspace.
+pub enum IioVal {
+    /// A single integer value.
+    Int(i32),
+    /// A fractional value represented as `val / val2`.
+    /// The denominator is `NonZeroI32` to prevent division-by-zero in
+    /// `iio_format_value()`.
+    Fractional(i32, NonZeroI32),
+    /// An integer plus a micro (1e-6) fractional part: `val.val2`.
+    IntPlusMicro(i32, i32),
+    /// An integer plus a nano (1e-9) fractional part: `val.val2`.
+    IntPlusNano(i32, i32),
+}
+
+/// Trait to be implemented by IIO driver private data.
+///
+/// Implementors supply the `read_raw` callback invoked by the IIO core
+/// when userspace reads a channel attribute (e.g. `in_angl_raw`).
+///
+/// The `Send + Sync` bounds ensure the compiler rejects driver types with
+/// thread-unsafe interior mutability (e.g. `Cell`), since the IIO core may
+/// invoke `read_raw` concurrently from multiple sysfs readers.
+pub trait IioDriver: Send + Sync {
+    /// Called by the IIO core when userspace reads a channel attribute.
+    ///
+    /// `chan` is the channel being read; `info` selects the attribute
+    /// (e.g. `IIO_CHAN_INFO_RAW`, `IIO_CHAN_INFO_SCALE`).
+    fn read_raw(&self, chan: *const iio_chan_spec, info: IioChanInfo) -> Result<IioVal>;
+
+    /// Returns the channel specifications for this driver.
+    ///
+    /// The default implementation returns an empty slice.
+    fn channels(&self) -> &'static [iio_chan_spec] {
+        &[]
+    }
+}
+
+/// C-compatible trampoline for the `iio_info.read_raw` callback.
+///
+/// # Safety
+///
+/// This function is only called by the IIO core via the `read_raw` function
+/// pointer in `iio_info`. The IIO core guarantees:
+/// - `indio_dev` is a valid `iio_dev` allocated by `iio_device_alloc`.
+/// - `chan` points to a valid channel spec from the device's channel array.
+/// - `val` is a valid non-null pointer to a writable `int`.
+/// - `val2` is a valid non-null pointer to a writable `int`. The IIO core
+///   always passes stack-allocated storage for both, regardless of whether
+///   the driver uses `val2` (e.g. `IIO_VAL_INT` only writes `val`; `val2`
+///   is provided but left unread by the caller for that return type).
+unsafe extern "C" fn read_raw_callback<T: IioDriver>(
+    indio_dev: *mut iio_dev,
+    chan: *const iio_chan_spec,
+    val: *mut c_int,
+    val2: *mut c_int,
+    info: isize,
+) -> c_int {
+    // SAFETY: `indio_dev` is valid and was allocated with space for `T` in its
+    // private data area. The `priv_` field was initialized in `Device::build_device()`.
+    let priv_ptr = unsafe { (*indio_dev).priv_ as *mut T };
+    // SAFETY: `priv_ptr` points to a valid, initialized instance of `T` that
+    // lives as long as the `iio_dev` allocation.
+    let driver = unsafe { &*priv_ptr };
+
+    let info_enum = match IioChanInfo::try_from(info as u32) {
+        Ok(valid) => valid,
+        Err(e) => return e.to_errno(),
+    };
+
+    match driver.read_raw(chan, info_enum) {
+        Ok(IioVal::Int(v)) => {
+            // SAFETY: `val` is valid per the function's Safety contract above.
+            // `val2` is not written; `IIO_VAL_INT` signals to the IIO core
+            // that only `val` carries meaningful data.
+            unsafe {
+                *val = v;
+            }
+            IIO_VAL_INT
+        }
+        Ok(IioVal::Fractional(v, v2)) => {
+            // SAFETY: both `val` and `val2` are valid per the Safety contract.
+            unsafe {
+                *val = v;
+                *val2 = v2.get();
+            }
+            IIO_VAL_FRACTIONAL
+        }
+        Ok(IioVal::IntPlusMicro(v, v2)) => {
+            // SAFETY: both `val` and `val2` are valid per the Safety contract.
+            unsafe {
+                *val = v;
+                *val2 = v2;
+            }
+            IIO_VAL_INT_PLUS_MICRO
+        }
+        Ok(IioVal::IntPlusNano(v, v2)) => {
+            // SAFETY: both `val` and `val2` are valid per the Safety contract.
+            unsafe {
+                *val = v;
+                *val2 = v2;
+            }
+            IIO_VAL_INT_PLUS_NANO
+        }
+        Err(e) => e.to_errno(),
+    }
+}
+
+// Device<T, State>: IIO device wrapper with typestate.
+
+/// Marker type for an unregistered IIO device.
+pub struct Unregistered;
+/// Marker type for a registered IIO device.
+pub struct Registered;
+
+/// A wrapped IIO device managing its C `struct iio_dev` lifetime.
+///
+/// Uses `iio_device_alloc` for allocation (not devres) and manual cleanup
+/// via `Drop`: `iio_device_unregister` -> `drop_in_place` for `T` ->
+/// `iio_device_free`.
+///
+/// # Invariants
+///
+/// - `indio_dev` is a valid pointer to an `iio_dev` allocated by `iio_device_alloc`.
+/// - If `registered` is true, the device was successfully registered via
+///   `__iio_device_register`.
+#[pin_data(PinnedDrop)]
+pub struct Device<T: IioDriver, State = Unregistered> {
+    indio_dev: *mut iio_dev,
+    registered: bool,
+    _p: PhantomData<(T, State)>,
+}
+
+// SAFETY: `Device` only contains a raw pointer to a kernel-managed `iio_dev`.
+// The IIO core serializes access to the device, and `T` is required to be `Send`.
+unsafe impl<T: IioDriver, S> Send for Device<T, S> {}
+// SAFETY: All `&self` access to the `iio_dev` is read-only or goes through the
+// IIO core which provides its own synchronization. `T` is required to be `Sync`.
+unsafe impl<T: IioDriver, S> Sync for Device<T, S> {}
+
+#[pinned_drop]
+impl<T: IioDriver, S> PinnedDrop for Device<T, S> {
+    fn drop(self: Pin<&mut Self>) {
+        if self.registered {
+            // SAFETY: `__iio_device_register` succeeded.
+            //
+            // iio_device_unregister() removes sysfs entries via kernfs, which
+            // calls kernfs_drain() to wait for all in-flight sysfs attribute
+            // reads to complete before returning. Drivers that only use sysfs
+            // access paths (INDIO_DIRECT_MODE without buffer/trigger) are
+            // guaranteed that no read_raw callback is in flight after this.
+            //
+            // For drivers with buffer support, additional synchronization
+            // analysis is required for character device paths, which are NOT
+            // covered by kernfs_drain().
+            unsafe { iio_device_unregister(self.indio_dev) };
+        }
+
+        // SAFETY: `priv_` was fully initialized in `build_device` via
+        // `init.__pinned_init(priv_ptr)`. `drop_in_place` runs `T`'s destructor
+        // (including any pinned fields like Mutex). After that, `iio_device_free`
+        // calls `put_device` which decrements the kref. The underlying `iio_dev`
+        // memory is only freed when kref reaches 0.
+        unsafe {
+            let priv_ptr = (*self.indio_dev).priv_ as *mut T;
+            drop_in_place(priv_ptr);
+            iio_device_free(self.indio_dev);
+        }
+    }
+}
+
+impl<T: IioDriver> Device<T> {
+    // SAFETY:
+    // - `read_raw_callback::<T>` is a valid function pointer whose signature
+    //   matches the IIO core's `read_raw` contract.
+    // - All remaining fields are pointers or function pointers; zeroed values
+    //   are NULL, and the IIO core checks for NULL before invoking any optional
+    //   callback or dereferencing any optional attribute group.
+    const VTABLE: iio_info = iio_info {
+        read_raw: Some(read_raw_callback::<T>),
+        ..unsafe { zeroed() }
+    };
+
+    /// Allocates a new IIO device with the given driver data.
+    ///
+    /// Uses `iio_device_alloc` (not `devm_*`) so that the Rust `Drop`
+    /// implementation has full control over the cleanup sequence.
+    /// The device is not yet registered; call [`register`](Self::register)
+    /// to make it visible to userspace.
+    pub fn build_device<E>(
+        dev: &device::Device,
+        name: &'static CStr,
+        modes: u32,
+        init: impl PinInit<T, E>,
+    ) -> Result<Self>
+    where
+        Error: From<E>,
+    {
+        let priv_size = i32::try_from(size_of::<T>()).map_err(|_| EINVAL)?;
+
+        // SAFETY: `dev.as_raw()` returns a valid `struct device` pointer.
+        // `iio_device_alloc` allocates an `iio_dev` with `sizeof(T)` bytes of
+        // private data. Returns NULL on failure.
+        let indio_dev = unsafe { iio_device_alloc(dev.as_raw(), priv_size) };
+        if indio_dev.is_null() {
+            return Err(ENOMEM);
+        }
+
+        // SAFETY: `indio_dev` is valid and freshly allocated. `priv_` points to
+        // zeroed memory (kzalloc'd by iio_device_alloc). `PinInit::__pinned_init`
+        // overwrites it in place without reading previous contents.
+        let priv_ptr = unsafe { (*indio_dev).priv_ as *mut T };
+        let init_result = unsafe { init.__pinned_init(priv_ptr) };
+        if let Err(e) = init_result {
+            // SAFETY: `pin_init` guarantees partial-init rollback internally.
+            // `priv_` memory was not fully initialized, so we only free the
+            // container without running `T`'s destructor.
+            unsafe { iio_device_free(indio_dev) };
+            return Err(Error::from(e));
+        }
+
+        // SAFETY: `priv_ptr` is now fully initialized. We set up the IIO
+        // device fields:
+        // - `name` is a `'static` C string that outlives the device.
+        // - `VTABLE` is a `'static` const and outlives the device.
+        // - `channels()` is required to return a `'static` slice (trait
+        //   contract). The pointer stored in `indio_dev.channels` therefore
+        //   remains valid for the entire lifetime of the `iio_dev` allocation
+        //   (until `iio_device_free`), because static data outlives any
+        //   allocation.
+        // - `modes` is passed by the caller and stored as-is.
+        unsafe {
+            (*indio_dev).name = name.as_char_ptr();
+            (*indio_dev).info = &Self::VTABLE;
+
+            let chans = (*priv_ptr).channels();
+            (*indio_dev).channels = chans.as_ptr();
+            (*indio_dev).num_channels = chans.len() as _;
+            (*indio_dev).modes = modes as i32;
+        }
+
+        Ok(Self {
+            indio_dev,
+            registered: false,
+            _p: PhantomData,
+        })
+    }
+
+    /// Registers the IIO device, making it visible to userspace via sysfs.
+    ///
+    /// On success, channel attributes like `in_angl_raw` become readable.
+    /// On failure the device stays unregistered and will be freed when
+    /// this [`Device`] is dropped.
+    #[inline]
+    pub fn register(self, module: &'static ThisModule) -> Result<Device<T, Registered>> {
+        // SAFETY: `self.indio_dev` is a valid, fully initialized `iio_dev`.
+        // `module.as_ptr()` provides the module owner for proper refcounting.
+        let ret = unsafe { __iio_device_register(self.indio_dev, module.as_ptr()) };
+        to_result(ret)?;
+
+        let registered_dev = Device {
+            indio_dev: self.indio_dev,
+            registered: true,
+            _p: PhantomData,
+        };
+
+        // Prevent `self`'s Drop from running. Ownership of `indio_dev`
+        // has been transferred to `registered_dev`.
+        forget(self);
+        Ok(registered_dev)
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 68f4d9a3425d..726a23e2d579 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -81,6 +81,8 @@
 #[cfg(CONFIG_I2C = "y")]
 pub mod i2c;
 pub mod id_pool;
+#[cfg(CONFIG_IIO)]
+pub mod iio;
 #[doc(hidden)]
 pub mod impl_flags;
 pub mod init;
-- 
2.50.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC PATCH v5 3/3] iio: position: add Rust driver for ams AS5600
  2026-08-22  6:26 [RFC PATCH v5 0/3] iio: position: add Rust driver for ams AS5600 Muchamad Coirul Anwar
  2026-08-22  6:26 ` [RFC PATCH v5 1/3] i2c: rust: implement SMBus access via IoBackend and FallibleIoCapable Muchamad Coirul Anwar
  2026-08-22  6:26 ` [RFC PATCH v5 2/3] rust: add minimal IIO subsystem abstractions Muchamad Coirul Anwar
@ 2026-08-22  6:26 ` Muchamad Coirul Anwar
  2026-08-24  0:17   ` Jonathan Cameron
  2 siblings, 1 reply; 7+ messages in thread
From: Muchamad Coirul Anwar @ 2026-08-22  6:26 UTC (permalink / raw)
  To: jic23, lars
  Cc: linux-iio, linux-kernel, linux-i2c, rust-for-linux, andi.shyti,
	wsa+renesas, ojeda, dakr, igor.korotin, branstj, brucer42,
	Muchamad Coirul Anwar

Add a Rust driver for the ams AS5600 12-bit magnetic rotary position
sensor. The driver exposes in_angl_raw and in_angl_scale via the IIO
sysfs interface.

Features:
- ARef<I2cClient<Bound>> for safe refcounted I2C client access
- Mutex-serialized status + angle read sequence
- Static channel spec (module-level const)
- No magnet validation at probe (deferred to read_raw per IIO convention)
- Error propagation via ? operator (no recovery state machine)
- Type-safe IioChanInfo enum dispatch in read_raw

The AS5600 stores the 12-bit raw angle big-endian across registers
0x0C-0x0D. smbus_read_word_swapped() handles the byte swap: SMBus
always transmits the low byte first (little-endian wire), so an
unconditional byte swap recovers the correct value regardless of CPU
endianness. The long-term solution is regmap-rs where endianness is
configured once at the transport level.

This driver uses INDIO_DIRECT_MODE without buffer or trigger support.
All userspace access is through sysfs attributes, which ensures safe
cleanup via kernfs_drain() synchronization in the IIO abstraction's
PinnedDrop. See the module-level doc comment for details.

Tested on BeagleBone Black (AM335x) with AS5600 on i2c-2 (0x36).

Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
---
 drivers/iio/position/Kconfig   |  11 ++
 drivers/iio/position/Makefile  |   1 +
 drivers/iio/position/as5600.rs | 189 +++++++++++++++++++++++++++++++++
 3 files changed, 201 insertions(+)
 create mode 100644 drivers/iio/position/as5600.rs

diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig
index 1576a6380b53..ac4f19d61ff6 100644
--- a/drivers/iio/position/Kconfig
+++ b/drivers/iio/position/Kconfig
@@ -6,6 +6,17 @@
 
 menu "Linear and angular position sensors"
 
+config AS5600
+	tristate "ams AS5600 magnetic rotary position sensor"
+	depends on I2C && RUST
+	help
+	  Support for the ams OSRAM AS5600 12-bit magnetic rotary
+	  position sensor. Provides in_angl_raw (0-4095) and
+	  in_angl_scale (radians per LSB) via sysfs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called as5600.
+
 config IQS624_POS
 	tristate "Azoteq IQS624/625 angular position sensors"
 	depends on MFD_IQS62X || COMPILE_TEST
diff --git a/drivers/iio/position/Makefile b/drivers/iio/position/Makefile
index d70902f2979d..2d26f6d6ace3 100644
--- a/drivers/iio/position/Makefile
+++ b/drivers/iio/position/Makefile
@@ -4,5 +4,6 @@
 
 # When adding new entries keep the list in alphabetical order
 
+obj-$(CONFIG_AS5600) += as5600.o
 obj-$(CONFIG_HID_SENSOR_CUSTOM_INTEL_HINGE) += hid-sensor-custom-intel-hinge.o
 obj-$(CONFIG_IQS624_POS)	+= iqs624-pos.o
diff --git a/drivers/iio/position/as5600.rs b/drivers/iio/position/as5600.rs
new file mode 100644
index 000000000000..8f2ea20a0645
--- /dev/null
+++ b/drivers/iio/position/as5600.rs
@@ -0,0 +1,189 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (C) 2026 Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
+//! Driver for ams AS5600 12-bit magnetic rotary position sensor.
+//!
+//! This driver uses `INDIO_DIRECT_MODE` without buffer or trigger support.
+//! All userspace access is through sysfs attributes (`in_angl_raw`,
+//! `in_angl_scale`), which ensures safe cleanup via `kernfs_drain()`
+//! synchronization in the IIO abstraction's `PinnedDrop`.
+//!
+//! Datasheet: https://look.ams-osram.com/m/7059eac7531a86fd/original/AS5600-DS000365.pdf
+
+use kernel::{
+    bindings::{
+        iio_chan_info_enum_IIO_CHAN_INFO_RAW,
+        iio_chan_info_enum_IIO_CHAN_INFO_SCALE,
+        iio_chan_spec,
+        iio_chan_type_IIO_ANGL,
+        INDIO_DIRECT_MODE, //
+    },
+    bits::{
+        bit_u8,
+        bit_usize,
+        genmask_u16, //
+    },
+    device::{
+        Bound,
+        Core, //
+    },
+    error::code::ENODATA,
+    i2c::{
+        DeviceId,
+        Driver,
+        I2cClient,
+        IdTable, //
+    },
+    i2c_device_table,
+    iio::{
+        Device,
+        IioChanInfo,
+        IioDriver,
+        IioVal,
+        Registered, //
+    },
+    io::Io,
+    module_i2c_driver,
+    of,
+    of_device_table,
+    prelude::*,
+    sync::{
+        aref::ARef,
+        new_mutex,
+        Mutex, //
+    }, //
+};
+
+const AS5600_REG_STATUS: u8 = 0x0B;
+const AS5600_REG_RAW_ANGLE_H: u8 = 0x0C;
+
+const AS5600_STATUS_MD: u8 = bit_u8(5);
+const AS5600_RAW_ANGLE_MASK: u16 = genmask_u16(0..=11);
+
+module_i2c_driver! {
+    type: As5600,
+    name: "as5600",
+    authors: ["Muchamad Coirul Anwar"],
+    description: "I2C Driver for ams OSRAM AS5600 Magnetic Rotary Position Sensor",
+    license: "GPL",
+}
+
+i2c_device_table!(
+    I2C_TABLE,
+    MODULE_I2C_TABLE,
+    <As5600 as Driver>::IdInfo,
+    [(DeviceId::new(c"as5600"), ())]
+);
+
+of_device_table!(
+    OF_TABLE,
+    MODULE_OF_TABLE,
+    <As5600 as Driver>::IdInfo,
+    [(of::DeviceId::new(c"ams,as5600"), ())]
+);
+
+struct As5600Channels([iio_chan_spec; 1]);
+
+// SAFETY: `iio_chan_spec` is a plain C struct with no interior mutability.
+// All pointer fields (`event_spec`, `ext_info`, `extend_name`, etc.) are
+// NULL — set via `zeroed()` and never reassigned — so no shared mutable
+// state exists behind them. The static is a compile-time constant with no
+// `&mut` access path, making concurrent shared access safe.
+unsafe impl Sync for As5600Channels {}
+
+static AS5600_CHANNELS: As5600Channels = As5600Channels({
+    // SAFETY: `iio_chan_spec` is a repr(C) struct where all-zeroes is valid
+    // (integers default to 0, pointers to NULL).
+    let mut chan: iio_chan_spec = unsafe { core::mem::zeroed() };
+    chan.type_ = iio_chan_type_IIO_ANGL;
+    chan.info_mask_separate = bit_usize(iio_chan_info_enum_IIO_CHAN_INFO_RAW)
+        | bit_usize(iio_chan_info_enum_IIO_CHAN_INFO_SCALE);
+    [chan]
+});
+
+#[pin_data]
+struct As5600Priv {
+    #[pin]
+    io_lock: Mutex<As5600HwState>,
+}
+
+struct As5600HwState {
+    client: ARef<I2cClient<Bound>>,
+}
+
+impl IioDriver for As5600Priv {
+    fn read_raw(&self, _chan: *const iio_chan_spec, info: IioChanInfo) -> Result<IioVal> {
+        match info {
+            IioChanInfo::Raw => {
+                let hw = self.io_lock.lock();
+                let io = hw.client.smbus_io();
+                // Read status register to verify magnet presence before
+                // reading the angle.
+                let status = io.try_read8(AS5600_REG_STATUS as usize)?;
+
+                // Check magnet presence (MD bit). Without a magnet the angle
+                // register contains stale/invalid data.
+                if (status & AS5600_STATUS_MD) == 0 {
+                    return Err(ENODATA);
+                }
+
+                // Word read at register 0x0C returns big-endian data.
+                // smbus_read_word_swapped() handles the byte swap.
+                // Mutex ensures status + angle read is atomic.
+                let raw = hw.client.smbus_read_word_swapped(AS5600_REG_RAW_ANGLE_H)?;
+                let angle = raw & AS5600_RAW_ANGLE_MASK;
+                Ok(IioVal::Int(angle as i32))
+            }
+            // Scale factor: radians per LSB = 2*pi / 4096 ~= 0.001533981
+            IioChanInfo::Scale => Ok(IioVal::IntPlusNano(0, 1533981)),
+        }
+    }
+
+    fn channels(&self) -> &'static [iio_chan_spec] {
+        &AS5600_CHANNELS.0
+    }
+}
+
+#[pin_data]
+struct As5600 {
+    #[pin]
+    _iio_dev: Device<As5600Priv, Registered>,
+}
+
+impl Driver for As5600 {
+    type IdInfo = ();
+    type Data<'bound> = As5600;
+
+    const I2C_ID_TABLE: Option<IdTable<Self::IdInfo>> = Some(&I2C_TABLE);
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+    // `try_pin_init!` returns a concrete anonymous type that may expose more
+    // bounds than the trait signature declares (e.g. auto-traits like `Send`).
+    // This refinement of the RPITIT return type is intentional.
+    #[allow(refining_impl_trait)]
+    fn probe<'bound>(
+        dev: &'bound I2cClient<Core<'_>>,
+        _id_info: Option<&'bound Self::IdInfo>,
+    ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
+        try_pin_init!(As5600 {
+            _iio_dev: {
+                // Deref coercion: I2cClient<Core<'_>> -> I2cClient<Bound>.
+                // We capture the Bound context to call smbus_read_word_swapped()
+                // and try_read8(), which require Bound.
+                let bound: &I2cClient<Bound> = dev;
+                let client: ARef<I2cClient<Bound>> = ARef::from(bound);
+
+                let priv_init = pin_init!(As5600Priv {
+                    io_lock <- new_mutex!(As5600HwState {
+                       client
+                    }),
+                });
+
+                let iio_dev =
+                    Device::build_device(dev.as_ref(), c"as5600", INDIO_DIRECT_MODE, priv_init)?;
+                let registered = iio_dev.register(&crate::THIS_MODULE)?;
+                dev_dbg!(dev.as_ref(), "AS5600 magnetic position sensor ready\n");
+                registered
+            }
+        })
+    }
+}
-- 
2.50.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v5 1/3] i2c: rust: implement SMBus access via IoBackend and FallibleIoCapable
  2026-08-22  6:26 ` [RFC PATCH v5 1/3] i2c: rust: implement SMBus access via IoBackend and FallibleIoCapable Muchamad Coirul Anwar
@ 2026-08-23 23:41   ` Jonathan Cameron
  0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2026-08-23 23:41 UTC (permalink / raw)
  To: Muchamad Coirul Anwar, Mark Brown
  Cc: lars, linux-iio, linux-kernel, linux-i2c, rust-for-linux,
	andi.shyti, wsa+renesas, ojeda, dakr, igor.korotin, branstj,
	brucer42

On Sat, 22 Aug 2026 14:26:56 +0800
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> Implement SMBus byte and word read/write operations for I2cClient using
> the FallibleIoCapable trait from the generic I/O backend infrastructure.
> 
> I2cClient now exposes an I2cBackend that implements FallibleIoCapable<u8>
> and FallibleIoCapable<u16>, replacing the previous IoCapable approach.
> I2C/SMBus bus transactions are inherently fallible (NACK, arbitration
> loss, timeout), so the infallible IoCapable is not appropriate here.
> FallibleIoCapable carries the errno from i2c_smbus_read_byte_data and
> i2c_smbus_read_word_data directly to the caller via Result<T>.
> 
> The implementation is restricted to I2cClient<Bound> as I/O operations
> require a live device context.
> 
> I2cClient<Bound>::smbus_io() returns an I2cView handle for use with the
> generic try_read8/try_read16 methods. Two standalone methods are also
> provided for odd-offset word access that bypasses the alignment check
> in the Io trait:

Given some devices implement auto address increment and others decrement
even in aligned byte pairs it seems you will see things that 'smell' like
they are unaligned.

I'd forgotten this fun corner of smbus like i2c devices!

> 
>   smbus_read_word()        - CPU-native byte order (SMBus LE wire format)
>   smbus_read_word_swapped() - byte-swapped result for big-endian devices
> 
> maxsize is 256, covering the SMBus command byte range 0x00-0xFF. This
> is the command byte space, not the 7-bit device address which is handled
> by the I2C core at adapter level.
> 
> Link: https://lore.kernel.org/rust-for-linux/20260131-i2c-adapter-v1-4-5a436e34cd1a@gmail.com/
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/commit/?h=driver-core-testing&id=121d87b28e1d9061d3aaa156c43a627d3cb5e620
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>

Just to repeat myself (and I appreciate the challenges that exist for
rust support in general and that it may be easier to look at the
i2c layer) I think that if it we are looking at bindings that are
register like then regmap is the way to go.  The space of what you can
build that is register based and uses these i2c_smbus commands is a lot
richer than you might think.  Either you end up reinventing all the
infrastructure regmap has to handle these, or you just use regmap.

There are mixed devices where register stuff is used alongside other accesses,
however for those I'm not sure it is worth doing anything other than
wrapping the raw bus access functions.

A few more references to the real variations we have to cope with inline.

Thanks

Jonathan

p.s. One day the rust driver in IIO won't be the bottom of my 'to review'
list :(

> ---
>  rust/kernel/bits.rs |  29 +++++
>  rust/kernel/i2c.rs  | 302 ++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/io.rs   |  66 +++++++---
>  3 files changed, 377 insertions(+), 20 deletions(-)
> 
> diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
> index 2daead125626..a6537a668dd6 100644
> --- a/rust/kernel/bits.rs
> +++ b/rust/kernel/bits.rs
> @@ -41,6 +41,7 @@ pub const fn [<bit_ $ty>](n: u32) -> $ty {
>  impl_bit_fn!(u32);
>  impl_bit_fn!(u16);
>  impl_bit_fn!(u8);
> +impl_bit_fn!(usize);
>  
>  macro_rules! impl_genmask_fn {
>      (
> @@ -203,3 +204,31 @@ pub const fn [<genmask_ $ty>](range: RangeInclusive<u32>) -> $ty {
>      /// assert_eq!(genmask_u8(0..=7), u8::MAX);
>      /// ```
>  );
> +
> +impl_genmask_fn!(
> +    usize,
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # #![expect(clippy::reversed_empty_ranges)]
> +    /// # use kernel::bits::genmask_checked_usize;
> +    /// assert_eq!(genmask_checked_usize(0..=0), Some(0b1));
> +    /// assert_eq!(genmask_checked_usize(0..=3), Some(0b1111));
> +    /// assert_eq!(genmask_checked_usize(1..=3), Some(0b1110));
> +    ///
> +    /// // `200` is out of the supported bit range on all platforms.
> +    /// assert_eq!(genmask_checked_usize(0..=200), None);
> +    ///
> +    /// // Invalid range where the start is bigger than the end.
> +    /// assert_eq!(genmask_checked_usize(5..=2), None);
> +    /// ```
> +    ,
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::bits::genmask_usize;
> +    /// assert_eq!(genmask_usize(0..=0), 0b1);
> +    /// assert_eq!(genmask_usize(0..=3), 0b1111);
> +    /// assert_eq!(genmask_usize(1..=3), 0b1110);
> +    /// ```
> +);
> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> index 624b971ca8b0..f939907573a6 100644
> --- a/rust/kernel/i2c.rs
> +++ b/rust/kernel/i2c.rs
> @@ -14,8 +14,15 @@
>      devres::Devres,
>      driver,
>      error::*,
> +    io::{
> +        FallibleIoCapable,
> +        IoBackend,
> +        IoBase,
> +        Region, //
> +    },
>      of,
>      prelude::*,
> +    ptr::KnownSize,
>      sync::aref::{
>          ARef,
>          AlwaysRefCounted, //
> @@ -601,3 +608,298 @@ unsafe impl Send for Registration {}
>  // SAFETY: `Registration` offers no interior mutability (no mutation through &self
>  // and no mutable access is exposed)
>  unsafe impl Sync for Registration {}
> +
> +// SAFETY: `I2cClient<Bound>` wraps a kernel `struct i2c_client`. The I2C core
> +// and bus locking mechanisms ensure that the underlying client structure can
> +// be safely transferred between threads.
> +unsafe impl Send for I2cClient<device::Bound> {}
> +
> +// SAFETY: `I2cClient<Bound>` wraps a kernel `struct i2c_client`. All methods
> +// that access the client go through kernel I2C core functions that provide
> +// their own synchronization. No &self method exposes interior mutability.
> +unsafe impl Sync for I2cClient<device::Bound> {}
> +
> +// SAFETY: `I2cClient<Bound>` is always reference-counted via the embedded
> +// `struct device`. `get_device`/`put_device` increment and decrement the
> +// device refcount atomically. A separate impl is needed for `I2cClient<Bound>`
> +// because `AlwaysRefCounted` is not implemented generically over all
> +// `DeviceContext`s — only the specific contexts that are safe to refcount
> +// from arbitrary threads.
> +unsafe impl AlwaysRefCounted for I2cClient<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()) };
> +    }
> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> +        unsafe { bindings::put_device(&raw mut (*obj.as_ref().as_raw()).dev) }
> +    }
> +}
> +
> +/// I/O backend for SMBus register access via I2C.
> +///
> +/// This backend implements only [`FallibleIoCapable`] and not [`IoCapable`],
> +/// because I2C/SMBus bus transactions are inherently fallible — NACK,
> +/// arbitration loss, and timeout can occur regardless of address validity.
> +/// The infallible [`Io::read`], [`Io::write`], and [`Io::update`] methods
> +/// are therefore compile-time unavailable for this backend.
> +pub struct I2cBackend;
> +
> +/// View type for [`I2cBackend`], carrying a reference to an I2C client and
> +/// a fake pointer that encodes the register offset and address-space size
> +/// as fat-pointer metadata.
> +///
> +/// The pointer field is never dereferenced. After [`IoBackend::project_view`]
> +/// projects an offset into the pointer, `addr()` yields that offset as the
> +/// SMBus command byte. [`KnownSize::size()`] reads the fat-pointer metadata
> +/// length (256 for the SMBus command space).
> +///
> +/// # Invariants
> +///
> +/// `ptr` is a non-dereferenceable fat pointer. Its address component encodes
> +/// the SMBus register offset (0..=255) after [`IoBackend::project_view`]
> +/// projection; its length metadata is 256 (the SMBus command byte address
> +/// space). `client` points to a valid `I2cClient<Bound>` that remains live
> +/// for `'a`.
> +pub struct I2cView<'a, T: ?Sized> {
> +    client: &'a I2cClient<device::Bound>,
> +    ptr: *mut T,
> +}
> +
> +impl<T: ?Sized> Copy for I2cView<'_, T> {}
> +
> +impl<T: ?Sized> Clone for I2cView<'_, T> {
> +    #[inline]
> +    fn clone(&self) -> Self {
> +        *self
> +    }
> +}
> +
> +impl IoBackend for I2cBackend {
> +    type View<'a, T: ?Sized + KnownSize> = I2cView<'a, T>;
> +
> +    #[inline]
> +    fn as_ptr<'a, T: ?Sized + KnownSize>(view: Self::View<'a, T>) -> *mut T {
> +        view.ptr
> +    }
> +
> +    #[inline]
> +    unsafe fn project_view<'a, T: ?Sized + KnownSize, U: ?Sized + KnownSize>(
> +        view: Self::View<'a, T>,
> +        ptr: *mut U,
> +    ) -> Self::View<'a, U> {
> +        // INVARIANT: Per safety requirement.
> +        I2cView {
> +            client: view.client,
> +            ptr,
> +        }
> +    }
> +}
> +
> +impl FallibleIoCapable<u8> for I2cBackend {
> +    #[inline]
> +    fn io_try_read<'a>(view: I2cView<'a, u8>) -> Result<u8> {
> +        // `io_view()` ensures `offset + 1 <= 256`, so `addr()` is at most 255;
> +        // the `as u8` cast below is therefore lossless.
> +        let reg = Self::as_ptr(view).addr() as u8;
> +        // SAFETY: `view.client.as_raw()` returns a valid `*mut struct i2c_client`
> +        // pointer as guaranteed by the type invariant of `I2cClient`.
> +        // `i2c_smbus_read_byte_data` is safe to call with any valid client pointer
> +        // and any u8 command byte.
> +        let ret = unsafe { bindings::i2c_smbus_read_byte_data(view.client.as_raw(), reg) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u8)
> +        }
> +    }
> +
> +    #[inline]
> +    fn io_try_write<'a>(view: I2cView<'a, u8>, value: u8) -> Result {
> +        // `io_view()` ensures `offset + 1 <= 256`, so `addr()` is at most 255;
> +        // the `as u8` cast below is therefore lossless.
> +        let reg = Self::as_ptr(view).addr() as u8;
> +        // SAFETY: `view.client.as_raw()` returns a valid `*mut struct i2c_client`
> +        // pointer as guaranteed by the type invariant of `I2cClient`.
> +        // `i2c_smbus_write_byte_data` is safe to call with any valid client pointer
> +        // and any u8 command byte and value.
> +        let ret = unsafe { bindings::i2c_smbus_write_byte_data(view.client.as_raw(), reg, value) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +}
> +
> +impl FallibleIoCapable<u16> for I2cBackend {
> +    #[inline]
> +    fn io_try_read<'a>(view: I2cView<'a, u16>) -> Result<u16> {
> +        // `io_view()` ensures `offset + 2 <= 256`, so `addr()` is at most 254;
> +        // the `as u8` cast below is therefore lossless.
> +        let reg = Self::as_ptr(view).addr() as u8;
> +        // SAFETY: `view.client.as_raw()` returns a valid `*mut struct i2c_client`
> +        // pointer as guaranteed by the type invariant of `I2cClient`.
> +        // `i2c_smbus_read_word_data` is safe to call with any valid client pointer
> +        // and any u8 command byte.
> +        let ret = unsafe { bindings::i2c_smbus_read_word_data(view.client.as_raw(), reg) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }
> +
> +    #[inline]
> +    fn io_try_write<'a>(view: I2cView<'a, u16>, value: u16) -> Result {
> +        // `io_view()` ensures `offset + 2 <= 256`, so `addr()` is at most 254;

There are smbus devices that have fully 2 byte registers.  For those you'd
need to divide this by 2 and the range would go up to 510
E.g. drivers/light/cm32181.c (though that doesn't have that many registers).

> +        // the `as u8` cast below is therefore lossless.
> +        let reg = Self::as_ptr(view).addr() as u8;
> +        // SAFETY: `view.client.as_raw()` returns a valid `*mut struct i2c_client`
> +        // pointer as guaranteed by the type invariant of `I2cClient`.
> +        // `i2c_smbus_write_word_data` is safe to call with any valid client pointer
> +        // and any u8 command byte and u16 value.
> +        let ret = unsafe { bindings::i2c_smbus_write_word_data(view.client.as_raw(), reg, value) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +}
...

> +impl I2cClient<device::Bound> {
> +    /// Returns an I/O handle for SMBus register access on this I2C client.
> +    ///
> +    /// The returned handle provides fallible read/write methods for the
> +    /// 256-byte SMBus command address space (0x00–0xFF). This is the SMBus
> +    /// command byte range, NOT the 7-bit device address, which is handled
> +    /// by the I2C core at the adapter level.

That rather feels like you are correcting my confusion in the earlier version! 
I'd assume people are more awake than me and skip the NOT part ;)

> +    ///
> +    /// Note: [`Io::try_read16`] and [`Io::try_write16`] on the returned handle
> +    /// reject odd offsets. The underlying [`Region`] base address is 0, so
> +    /// [`offset_valid`] checks `(0 + offset) % 2 == 0` — only even offsets
> +    /// pass. For word-sized access to odd-offset registers use
> +    /// [`smbus_read_word`] or [`smbus_read_word_swapped`] instead.
> +    ///
> +    /// The underlying pointer in the returned [`I2cView`] is never
> +    /// dereferenced; it encodes the register address space size as
> +    /// fat-pointer metadata and the register offset as the pointer address.
> +    ///
> +    /// [`smbus_read_word`]: Self::smbus_read_word
> +    /// [`smbus_read_word_swapped`]: Self::smbus_read_word_swapped
> +    #[inline]
> +    pub fn smbus_io(&self) -> I2cView<'_, Region<256>> {
> +        // INVARIANT: `client` is `self`, a valid `I2cClient<Bound>`.
> +        //
> +        // `ptr` is a "fake pointer" — it is constructed solely to carry two
> +        // pieces of metadata through the `IoBase` machinery:
> +        //   - address component: 0 initially; after each `project_view` call,
> +        //     this becomes the register offset (the SMBus command byte).
> +        //   - length metadata: 256, encoding the SMBus command address space
> +        //     size so `io_view()` can bounds-check offsets.
> +        //
> +        // `without_provenance_mut(0)` produces a pointer with no memory
> +        // provenance — it cannot be used to read or write memory. This is safe
> +        // because `I2cBackend::as_ptr()` extracts the address as a `usize`
> +        // offset and passes it to `i2c_smbus_*` functions, never dereferencing
> +        // the pointer itself. Using a provenance-free base avoids accidentally
> +        // creating a pointer that appears to alias real memory.
> +        I2cView {
> +            client: self,
> +            ptr: Region::<256>::ptr_from_raw_parts_mut(core::ptr::without_provenance_mut(0), 256),
> +        }
> +    }
> +
> +    /// Reads a 16-bit word from an SMBus register in CPU-native byte order.
> +    ///
> +    /// Wraps `i2c_smbus_read_word_data`. The `reg` parameter is the SMBus
> +    /// command byte (0x00–0xFF) — an instruction sent to the device over the
> +    /// serial bus, not a memory address. There is no alignment requirement:
> +    /// any command byte value is valid regardless of whether it is odd or even.

It might be a memory address, could be almost anything.  Maybe 'not necessarily'
a memory address.
The kernel docs have it as:
"Command byte, a data byte which often selects a register on the device"

> +    ///
> +    /// SMBus transmits the low byte first (little-endian on the wire), and this
> +    /// method returns the value in CPU-native byte order without further
> +    /// conversion. Use [`Self::smbus_read_word_swapped`] for devices that store
> +    /// multi-byte registers in big-endian (MSB-first) format.
> +    ///
> +    /// Returns `Err` if the bus transaction fails (e.g. NACK, arbitration loss,
> +    /// or timeout).
> +    #[inline]
> +    pub fn smbus_read_word(&self, reg: u8) -> Result<u16> {
> +        // SAFETY: `self.as_raw()` returns a valid `*mut struct i2c_client`
> +        // pointer as guaranteed by the type invariant of `I2cClient`.
> +        // `i2c_smbus_read_word_data` is safe to call with any valid client
> +        // pointer and any u8 command byte.
> +        let ret = unsafe { bindings::i2c_smbus_read_word_data(self.as_raw(), reg) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }
> +
> +    /// Reads a 16-bit word from an SMBus register with bytes unconditionally
> +    /// swapped.
> +    ///
> +    /// Wraps `i2c_smbus_read_word_data` and applies [`u16::swap_bytes`] to the
> +    /// result. Use this for devices that store multi-byte registers in
> +    /// big-endian (MSB-first) format, which is common among I2C sensors whose
> +    /// datasheets do not reference the SMBus specification.

This last bit feels like cover letter, patch description material.
I wouldn't normally expect function documentation to justify how useful
a function is!

> +    ///
> +    /// The swap is **unconditional** — it is not equivalent to `be16_to_cpu`.
> +    /// On a big-endian CPU, `be16_to_cpu` would be a no-op, but this method
> +    /// still swaps. The reason: SMBus always transmits the low byte first, so
> +    /// the driver always receives data in little-endian wire order regardless
> +    /// of CPU endianness. The swap corrects for the device's wire-level byte
> +    /// order, not the CPU's native order.

This feels like we are justifying why it isn't a different implementation.
Can we rewrite to not need that reference to what else it isn't.
 
> +    ///
> +    /// The `reg` parameter is the SMBus command byte (0x00–0xFF). There is no
> +    /// alignment requirement; any command byte value is valid.

What would an alignment requirement mean here?

> +    ///
> +    /// Returns `Err` if the bus transaction fails (e.g. NACK, arbitration loss,
> +    /// or timeout).
> +    ///
> +    /// # Example
> +    ///
> +    /// ```ignore
> +    /// // AS5600 stores the 12-bit raw angle big-endian at register 0x0C.
> +    /// let raw = client.smbus_read_word_swapped(0x0C)?;
> +    /// let angle = raw & 0x0FFF;
> +    /// ```
> +    #[inline]
> +    pub fn smbus_read_word_swapped(&self, reg: u8) -> Result<u16> {
> +        // SAFETY: `self.as_raw()` returns a valid `*mut struct i2c_client`
> +        // pointer as guaranteed by the type invariant of `I2cClient`.
> +        let ret = unsafe { bindings::i2c_smbus_read_word_data(self.as_raw(), reg) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok((ret as u16).swap_bytes())
> +        }
> +    }
> +}
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 95f46bb75f9e..516895ca2082 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -276,6 +276,36 @@ pub trait IoCapable<T>: IoBackend {
>      fn io_write<'a>(view: Self::View<'a, T>, value: T);
>  }
>  
> +/// Fallible counterpart of [`IoCapable`] for I/O backends where operations can fail at the
> +/// transport level (e.g. I2C, SPI).
> +///

Why is this part in the patch adding the i2c specific use case?
I'd expect it to be a precursor patch.

> +/// Infallible backends ([`IoCapable`] implementors) get this for free via blanket implementation.
> +/// Fallible-only backends implement this trait directly without implementing [`IoCapable`]; the
> +/// infallible [`Io::read`], [`Io::write`], and [`Io::update`] methods will then be unavailable,
> +/// enforcing that callers use the `try_*` variants instead.
> +pub trait FallibleIoCapable<T>: IoBackend {
> +    /// Performs an I/O read of type `T` at `view` and returns the result, or an error if the
> +    /// transport-level operation fails.
> +    fn io_try_read<'a>(view: Self::View<'a, T>) -> Result<T>;
> +
> +    /// Performs an I/O write of `value` at `view`, or returns an error if the transport-level
> +    /// operation fails.
> +    fn io_try_write<'a>(view: Self::View<'a, T>, value: T) -> Result;
> +}
> +
> +impl<B: IoCapable<T>, T> FallibleIoCapable<T> for B {
> +    #[inline(always)]
> +    fn io_try_read<'a>(view: Self::View<'a, T>) -> Result<T> {
> +        Ok(Self::io_read(view))
> +    }
> +
> +    #[inline(always)]
> +    fn io_try_write<'a>(view: Self::View<'a, T>, value: T) -> Result {
> +        Self::io_write(view, value);
> +        Ok(())
> +    }
> +}
> +
>  /// Trait indicating that an I/O backend supports memory copy operations.
>  pub trait IoCopyable: IoBackend {
>      /// Copy contents of `view` to `buffer`.
> @@ -645,7 +675,7 @@ fn copy_to_slice(self, data: &mut [u8])
>      fn try_read8(self, offset: usize) -> Result<u8>
>      where
>          usize: IoLoc<Self::Target, u8, IoType = u8>,
> -        Self::Backend: IoCapable<u8>,
> +        Self::Backend: FallibleIoCapable<u8>,
>      {
>          self.try_read(offset)
>      }
> @@ -655,7 +685,7 @@ fn try_read8(self, offset: usize) -> Result<u8>
>      fn try_read16(self, offset: usize) -> Result<u16>
>      where
>          usize: IoLoc<Self::Target, u16, IoType = u16>,
> -        Self::Backend: IoCapable<u16>,
> +        Self::Backend: FallibleIoCapable<u16>,
>      {
>          self.try_read(offset)
>      }
> @@ -665,7 +695,7 @@ fn try_read16(self, offset: usize) -> Result<u16>
>      fn try_read32(self, offset: usize) -> Result<u32>
>      where
>          usize: IoLoc<Self::Target, u32, IoType = u32>,
> -        Self::Backend: IoCapable<u32>,
> +        Self::Backend: FallibleIoCapable<u32>,
>      {
>          self.try_read(offset)
>      }
> @@ -675,7 +705,7 @@ fn try_read32(self, offset: usize) -> Result<u32>
>      fn try_read64(self, offset: usize) -> Result<u64>
>      where
>          usize: IoLoc<Self::Target, u64, IoType = u64>,
> -        Self::Backend: IoCapable<u64>,
> +        Self::Backend: FallibleIoCapable<u64>,
>      {
>          self.try_read(offset)
>      }
> @@ -685,7 +715,7 @@ fn try_read64(self, offset: usize) -> Result<u64>
>      fn try_write8(self, value: u8, offset: usize) -> Result
>      where
>          usize: IoLoc<Self::Target, u8, IoType = u8>,
> -        Self::Backend: IoCapable<u8>,
> +        Self::Backend: FallibleIoCapable<u8>,
>      {
>          self.try_write(offset, value)
>      }
> @@ -695,7 +725,7 @@ fn try_write8(self, value: u8, offset: usize) -> Result
>      fn try_write16(self, value: u16, offset: usize) -> Result
>      where
>          usize: IoLoc<Self::Target, u16, IoType = u16>,
> -        Self::Backend: IoCapable<u16>,
> +        Self::Backend: FallibleIoCapable<u16>,
>      {
>          self.try_write(offset, value)
>      }
> @@ -705,7 +735,7 @@ fn try_write16(self, value: u16, offset: usize) -> Result
>      fn try_write32(self, value: u32, offset: usize) -> Result
>      where
>          usize: IoLoc<Self::Target, u32, IoType = u32>,
> -        Self::Backend: IoCapable<u32>,
> +        Self::Backend: FallibleIoCapable<u32>,
>      {
>          self.try_write(offset, value)
>      }
> @@ -715,7 +745,7 @@ fn try_write32(self, value: u32, offset: usize) -> Result
>      fn try_write64(self, value: u64, offset: usize) -> Result
>      where
>          usize: IoLoc<Self::Target, u64, IoType = u64>,
> -        Self::Backend: IoCapable<u64>,
> +        Self::Backend: FallibleIoCapable<u64>,
>      {
>          self.try_write(offset, value)
>      }
> @@ -827,10 +857,10 @@ fn write64(self, value: u64, offset: usize)
>      fn try_read<T, L>(self, location: L) -> Result<T>
>      where
>          L: IoLoc<Self::Target, T>,
> -        Self::Backend: IoCapable<L::IoType>,
> +        Self::Backend: FallibleIoCapable<L::IoType>,
>      {
>          let view = io_view::<Self, L::IoType>(self, location.offset())?;
> -        Ok(Self::Backend::io_read(view).into())
> +        Ok(Self::Backend::io_try_read(view)?.into())
>      }
>  
>      /// Generic fallible write with runtime bounds check.
> @@ -860,12 +890,11 @@ fn try_read<T, L>(self, location: L) -> Result<T>
>      fn try_write<T, L>(self, location: L, value: T) -> Result
>      where
>          L: IoLoc<Self::Target, T>,
> -        Self::Backend: IoCapable<L::IoType>,
> +        Self::Backend: FallibleIoCapable<L::IoType>,
>      {
>          let view = io_view::<Self, L::IoType>(self, location.offset())?;
>          let io_value = value.into();
> -        Self::Backend::io_write(view, io_value);
> -        Ok(())
> +        Self::Backend::io_try_write(view, io_value)
>      }
>  
>      /// Generic fallible write of a fully-located register value.
> @@ -905,7 +934,7 @@ fn try_write_reg<T, L, V>(self, value: V) -> Result
>      where
>          L: IoLoc<Self::Target, T>,
>          V: LocatedRegister<Self::Target, Location = L, Value = T>,
> -        Self::Backend: IoCapable<L::IoType>,
> +        Self::Backend: FallibleIoCapable<L::IoType>,
>      {
>          let (location, value) = value.into_io_op();
>  
> @@ -938,16 +967,13 @@ fn try_write_reg<T, L, V>(self, value: V) -> Result
>      fn try_update<T, L, F>(self, location: L, f: F) -> Result
>      where
>          L: IoLoc<Self::Target, T>,
> -        Self::Backend: IoCapable<L::IoType>,
> +        Self::Backend: FallibleIoCapable<L::IoType>,
>          F: FnOnce(T) -> T,
>      {
>          let view = io_view::<Self, L::IoType>(self, location.offset())?;
> -
> -        let value: T = Self::Backend::io_read(view).into();
> +        let value: T = Self::Backend::io_try_read(view)?.into();
>          let io_value = f(value).into();
> -        Self::Backend::io_write(view, io_value);
> -
> -        Ok(())
> +        Self::Backend::io_try_write(view, io_value)
>      }
>  
>      /// Generic infallible read with compile-time bounds check.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v5 2/3] rust: add minimal IIO subsystem abstractions
  2026-08-22  6:26 ` [RFC PATCH v5 2/3] rust: add minimal IIO subsystem abstractions Muchamad Coirul Anwar
@ 2026-08-24  0:07   ` Jonathan Cameron
  0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2026-08-24  0:07 UTC (permalink / raw)
  To: Muchamad Coirul Anwar
  Cc: lars, linux-iio, linux-kernel, linux-i2c, rust-for-linux,
	andi.shyti, wsa+renesas, ojeda, dakr, igor.korotin, branstj,
	brucer42

On Sat, 22 Aug 2026 14:26:57 +0800
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> Add safe Rust wrappers for the Linux IIO (Industrial I/O) subsystem:
> 
> - IioChanInfo enum wrapping iio_chan_info_enum, with TryFrom<u32> for
>   type-safe dispatch in read_raw. The compiler enforces match
>   exhaustiveness, replacing the previous raw isize approach.
> - IioVal enum with NonZeroI32 for division-by-zero prevention on
>   IIO_VAL_FRACTIONAL.
> - IioDriver trait with read_raw callback (requires Send + Sync).
> - Device<T, State> with typestate (Unregistered -> Registered) to
>   prevent double-registration at compile time.
> - PinnedDrop for guaranteed cleanup sequence:
>     iio_device_unregister -> drop_in_place(T) -> iio_device_free
>   iio_device_unregister() calls cdev_device_del() which drains the
>   kernfs workqueue before returning. All in-flight read_raw callbacks
>   (which go through kernfs sysfs reads) complete before drop_in_place
>   proceeds. This covers the sysfs read path used by this driver.
> - Compile-time const VTABLE (iio_info).
> - C-to-Rust FFI trampoline for read_raw dispatch.
> 
> The abstraction uses iio_device_alloc (not devm_*) so that the Rust
> Drop implementation has full control over the cleanup sequence.
> Module ownership is enforced via __iio_device_register(indio_dev, module).
> 
> Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>

Hi Muchamad

This looks fine to me subject to a few little things - see inline.

However I didn't take the time to decode every line of rust today so there were bits
I simply didn't understand yet.  So for this to be able to move forward I'm going
to need reviews from rust experts!

Jonathan

> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index a56ba6309594..5dc917d92151 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -86,6 +86,7 @@ macro_rules! declare_err {
>      declare_err!(EIOCBQUEUED, "iocb queued, will get completion event.");
>      declare_err!(ERECALLCONFLICT, "Conflict with recalled state.");
>      declare_err!(ENOGRACE, "NFS file lock reclaim refused.");
> +    declare_err!(ENODATA, "No data available.");

Do we have something says there must be a user in the same patch?
A really generic thing like this in C would definitely be a patch on its
own so that folk who care about maintaining a given file can easily see
it without reviewing the rest of the series.

So unless you can't do otherwise, break this out as a precursor patch.

>  }
>  
>  /// Generic integer kernel error.
> diff --git a/rust/kernel/iio.rs b/rust/kernel/iio.rs
> new file mode 100644
> index 000000000000..f1638160fed1
> --- /dev/null
> +++ b/rust/kernel/iio.rs
...

> +
> +build_iio_enum! {
> +    /// Raw unprocessed value from the channel (`IIO_CHAN_INFO_RAW`).
> +    ///
> +    /// For sensors, this is typically the ADC reading or register value
> +    /// before any scaling or offset correction.
> +    Raw = iio_chan_info_enum_IIO_CHAN_INFO_RAW,

I guess there may be a rust convention for this but from a human trying to
read the code point of view this need a blank line here and in similar places
where you have docs / thing documented repeated back to back.

> +    /// Scale factor to convert raw values to SI units (`IIO_CHAN_INFO_SCALE`).
> +    ///
> +    /// The processed value is `raw * scale`. The unit depends on the channel
> +    /// type (e.g. V for voltage, m/s² for acceleration, rad for angle).
> +    Scale = iio_chan_info_enum_IIO_CHAN_INFO_SCALE,
> +}


> +
> +/// C-compatible trampoline for the `iio_info.read_raw` callback.
> +///
> +/// # Safety
> +///
> +/// This function is only called by the IIO core via the `read_raw` function
> +/// pointer in `iio_info`. The IIO core guarantees:
> +/// - `indio_dev` is a valid `iio_dev` allocated by `iio_device_alloc`.
> +/// - `chan` points to a valid channel spec from the device's channel array.
> +/// - `val` is a valid non-null pointer to a writable `int`.
> +/// - `val2` is a valid non-null pointer to a writable `int`. The IIO core
> +///   always passes stack-allocated storage for both, regardless of whether
> +///   the driver uses `val2` (e.g. `IIO_VAL_INT` only writes `val`; `val2`

That val2 is always a valid pointer smells a bit like the c interface leaking
into the rust.  I'm not necessarily against that being a constraint we take
on but I'm not sure how we document that. Probably add something to the C docs.
Any C driver relying on this today is probably buggy for other reasons.


> +///   is provided but left unread by the caller for that return type).
> +unsafe extern "C" fn read_raw_callback<T: IioDriver>(
> +    indio_dev: *mut iio_dev,
> +    chan: *const iio_chan_spec,
> +    val: *mut c_int,
> +    val2: *mut c_int,
> +    info: isize,
> +) -> c_int {
> +    // SAFETY: `indio_dev` is valid and was allocated with space for `T` in its
> +    // private data area. The `priv_` field was initialized in `Device::build_device()`.
> +    let priv_ptr = unsafe { (*indio_dev).priv_ as *mut T };
> +    // SAFETY: `priv_ptr` points to a valid, initialized instance of `T` that
> +    // lives as long as the `iio_dev` allocation.
> +    let driver = unsafe { &*priv_ptr };
> +
> +    let info_enum = match IioChanInfo::try_from(info as u32) {
> +        Ok(valid) => valid,
> +        Err(e) => return e.to_errno(),
> +    };
> +
> +    match driver.read_raw(chan, info_enum) {
> +        Ok(IioVal::Int(v)) => {
> +            // SAFETY: `val` is valid per the function's Safety contract above.
> +            // `val2` is not written; `IIO_VAL_INT` signals to the IIO core
> +            // that only `val` carries meaningful data.
> +            unsafe {
> +                *val = v;
> +            }
> +            IIO_VAL_INT
> +        }
> +        Ok(IioVal::Fractional(v, v2)) => {
> +            // SAFETY: both `val` and `val2` are valid per the Safety contract.
> +            unsafe {
> +                *val = v;
> +                *val2 = v2.get();

Why get in some places and not others?  May well be a gap in my really limited
rust knowledge.

> +            }
> +            IIO_VAL_FRACTIONAL
> +        }
> +        Ok(IioVal::IntPlusMicro(v, v2)) => {
> +            // SAFETY: both `val` and `val2` are valid per the Safety contract.
> +            unsafe {
> +                *val = v;
> +                *val2 = v2;
> +            }
> +            IIO_VAL_INT_PLUS_MICRO
> +        }
> +        Ok(IioVal::IntPlusNano(v, v2)) => {
> +            // SAFETY: both `val` and `val2` are valid per the Safety contract.
> +            unsafe {
> +                *val = v;
> +                *val2 = v2;
> +            }
> +            IIO_VAL_INT_PLUS_NANO
> +        }
> +        Err(e) => e.to_errno(),
> +    }
> +}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v5 3/3] iio: position: add Rust driver for ams AS5600
  2026-08-22  6:26 ` [RFC PATCH v5 3/3] iio: position: add Rust driver for ams AS5600 Muchamad Coirul Anwar
@ 2026-08-24  0:17   ` Jonathan Cameron
  0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2026-08-24  0:17 UTC (permalink / raw)
  To: Muchamad Coirul Anwar
  Cc: lars, linux-iio, linux-kernel, linux-i2c, rust-for-linux,
	andi.shyti, wsa+renesas, ojeda, dakr, igor.korotin, branstj,
	brucer42

On Sat, 22 Aug 2026 14:26:58 +0800
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> Add a Rust driver for the ams AS5600 12-bit magnetic rotary position
> sensor. The driver exposes in_angl_raw and in_angl_scale via the IIO
> sysfs interface.
> 
> Features:
> - ARef<I2cClient<Bound>> for safe refcounted I2C client access
> - Mutex-serialized status + angle read sequence
> - Static channel spec (module-level const)
> - No magnet validation at probe (deferred to read_raw per IIO convention)
> - Error propagation via ? operator (no recovery state machine)
> - Type-safe IioChanInfo enum dispatch in read_raw
> 
> The AS5600 stores the 12-bit raw angle big-endian across registers
> 0x0C-0x0D. smbus_read_word_swapped() handles the byte swap: SMBus
> always transmits the low byte first (little-endian wire), so an
> unconditional byte swap recovers the correct value regardless of CPU
> endianness. The long-term solution is regmap-rs where endianness is
> configured once at the transport level.
> 
> This driver uses INDIO_DIRECT_MODE without buffer or trigger support.

I'd only talk about what you do support.  There are always many things
that aren't in an initial driver so listing that bit doesn't provide much
value.

> All userspace access is through sysfs attributes, which ensures safe
> cleanup via kernfs_drain() synchronization in the IIO abstraction's
> PinnedDrop. See the module-level doc comment for details.


> 
> Tested on BeagleBone Black (AM335x) with AS5600 on i2c-2 (0x36).

Generally put things like testing info in the cover letter or below
the ---

> 
> Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>


The code itself looks fine to me - but as with earlier I'm looking
for rust expert review.

Thanks

Jonathan

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-08-24  0:17 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-22  6:26 [RFC PATCH v5 0/3] iio: position: add Rust driver for ams AS5600 Muchamad Coirul Anwar
2026-08-22  6:26 ` [RFC PATCH v5 1/3] i2c: rust: implement SMBus access via IoBackend and FallibleIoCapable Muchamad Coirul Anwar
2026-08-23 23:41   ` Jonathan Cameron
2026-08-22  6:26 ` [RFC PATCH v5 2/3] rust: add minimal IIO subsystem abstractions Muchamad Coirul Anwar
2026-08-24  0:07   ` Jonathan Cameron
2026-08-22  6:26 ` [RFC PATCH v5 3/3] iio: position: add Rust driver for ams AS5600 Muchamad Coirul Anwar
2026-08-24  0:17   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox