[RFC PATCH v3 0/4] iio: position: add Rust driver for ams AS5600

[RFC PATCH v3 0/4] iio: position: add Rust driver for ams AS5600

[Muchamad Coirul Anwar]

This is v3 of the Rust driver for the ams AS5600 12-bit magnetic rotary
position sensor. v2 introduced minimal IIO abstractions and exposed
in_angl_raw and in_angl_scale via sysfs. This revision addresses all
soundness and correctness issues identified during review of v2.

The primary focus of v3 is hardening the IIO abstraction layer against
undefined behaviour reachable from safe Rust, and restructuring the
driver to use proper kernel synchronisation primitives.

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

Changes since RFC v2:

  IIO abstraction (rust/kernel/iio.rs):
  - Eliminated division-by-zero via IioVal::Fractional(i32, NonZeroI32).
    The denominator is now core::num::NonZeroI32, making it impossible
    to construct a zero value that would trigger div_s64(x, 0) inside
    iio_format_value() in the C IIO core.
  - Added Send + Sync bounds on the IioDriver trait. Without these, a
    driver could use thread-unsafe interior mutability (e.g. Cell) in
    its private data while the IIO core invokes read_raw concurrently
    from multiple sysfs readers.
  - Device::build_device() now accepts `impl PinInit<T, E>` instead of
    taking T by value. This allows drivers to use kernel synchronisation
    primitives (Mutex, SpinLock) that require in-place initialisation
    via PinInit and cannot be moved after construction.
  - Introduced typestate pattern (Unregistered -> Registered) for
    Device<T, State>. register() consumes Device<T, Unregistered> and
    returns Device<T, Registered>, making double-registration a
    compile-time error rather than a runtime corruption of cdev/kobject
    state.
  - Added DirectModeGuard RAII type in the read_raw trampoline. Claims
    iio_device_claim_direct() on entry and releases on drop, preventing
    concurrent access conflicts between sysfs reads and buffer/trigger
    operations.
  - iio_info vtable is now a compile-time const with only read_raw set;
    remaining fields are zeroed (NULL-checked by IIO core before use).
  - Added #[inline] to Device::register() per Rust subsystem guidelines
    for small abstraction methods that forward to C bindings.
  - Cleanup uses iio_device_free() (= put_device, kref-based) rather
    than direct kfree, ensuring in-kernel consumers holding a reference
    do not trigger use-after-free on the iio_dev allocation itself.

  I2C abstraction (rust/kernel/i2c.rs):
  - Implemented the kernel::io::Io trait for I2cClient, per Igor
    Korotin's feedback. This aligns with the agreed-upon direction for
    I2C register access abstractions and provides runtime bounds checking
    via io_addr() for free.
  - I2cClient now implements IoCapable<u8> and IoCapable<u16>, exposing
    try_read8() and try_read16() with automatic offset validation
    (maxsize=256 for SMBus command byte range).
  - Added #[inline] to all Io trait method implementations per Rust
    subsystem guidelines for thin forwarding wrappers.

  Driver (drivers/iio/position/as5600.rs):
  - Added kernel::sync::Mutex<As5600HwState> to serialize the multi-byte
    angle read sequence. The AS5600 hardware freezes the internal angle
    value on reading the high byte until the low byte is read; without a
    driver-level lock, concurrent sysfs reads interleave and corrupt the
    hardware latch mechanism, producing mismatched high/low halves.
  - Mutex is initialized in-place via pin_init!/new_mutex! macros,
    leveraging the PinInit-based Device::build_device() API.
  - probe() now returns Result<Self> with #[allow(refining_impl_trait)]
    instead of attempting to return impl PinInit<Self, Error> directly.
    This compiles correctly because Result<T, E> implements PinInit<T, E>.
  - Implemented circuit breaker pattern (DeviceState::Normal/Poisoned)
    for I/O error resilience. After a bus failure, the driver marks the
    device as Poisoned and attempts a recovery read on the next call,
    preventing I/O storms on a dead bus while allowing automatic recovery
    when the bus comes back.
  - Fixed import formatting to follow vertical style (one item per line)
    per kernel Rust coding guidelines.
  - Channel spec allocated via KBox (heap) rather than stack, ensuring
    the pointer stored in indio_dev->channels remains valid for the
    lifetime of the IIO device.

  Known limitations (to be addressed before mainline):
  - channels[] is heap-allocated via KBox inside As5600Priv (in
    iio_dev->priv_). If an in-kernel consumer holds a reference via
    iio_channel_get() and the driver unbinds, drop_in_place(T) frees
    the KBox while indio_dev->channels still points to it. Fix: use a
    static const channel spec or tie the allocation lifetime to iio_dev
    itself. This is acceptable for RFC since the AS5600 has no known
    in-kernel consumers.
  - No power management (suspend/resume) hooks yet.
  - write_raw and buffer/trigger support deferred to future work.

Changes since RFC v1:
  - Moved magnet validation from probe() to read_raw() (Jonathan's
    feedback). probe() now only verifies I2C communication.
  - Added minimal Rust IIO abstractions (rust/kernel/iio.rs).
  - Added OF device table for devicetree matching (ams,as5600).
  - Replaced hex bit masks with kernel::bits::bit_u8() (Miguel's
    pointer).
  - Downgraded log messages to dev_dbg!(), removed unbind noise.
  - iio_info vtable is now a compile-time const.

Design notes:

  The IIO abstraction does NOT use devres (devm_iio_device_alloc). The
  Rust Drop implementation has full control over the cleanup sequence:
  iio_device_unregister -> drop_in_place(T) -> iio_device_free. This
  avoids lifetime conflicts between Rust ownership and the C devres
  teardown ordering.

  The mask parameter in read_raw uses `isize`, which is identical to
  `ffi::c_long` in the kernel (rust/ffi.rs defines c_long = isize on
  all supported architectures). No type mismatch exists.

  Module ownership is enforced via the second parameter of
  __iio_device_register(indio_dev, module), not via iio_info.owner.

The IIO abstraction is intentionally minimal: it supports read_raw with
IIO_VAL_INT, IIO_VAL_INT_PLUS_NANO, IIO_VAL_INT_PLUS_MICRO, and
IIO_VAL_FRACTIONAL. write_raw and buffer support are left for future
work.

The IIO abstraction design was informed by earlier unpublished work from
Brandon Saint-John.

Muchamad Coirul Anwar (4):
  i2c: rust: implement kernel::io::Io trait for I2cClient
  rust: add minimal IIO subsystem abstractions
  iio: position: add Rust driver for ams AS5600
  iio: position: as5600: add Kconfig and Makefile entries

 drivers/iio/position/Kconfig   |  14 ++
 drivers/iio/position/Makefile  |   1 +
 drivers/iio/position/as5600.rs | 289 ++++++++++++++++++++++++++++
 rust/helpers/helpers.c         |   1 +
 rust/helpers/iio.c             |  24 +++
 rust/kernel/i2c.rs             |  76 +++++---
 rust/kernel/iio.rs             | 341 +++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs             |   2 +
 8 files changed, 723 insertions(+), 25 deletions(-)
 create mode 100644 drivers/iio/position/as5600.rs
 create mode 100644 rust/helpers/iio.c
 create mode 100644 rust/kernel/iio.rs

Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
---
Testing performed on BeagleBone Black (AM335x), kernel v7.0.0-rc3,
AS5600 on i2c-2 (0x36) at 3.3V, 6mm diametric neodymium magnet.

Build: make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- M=drivers/iio/position
       (zero warnings, zero errors)

Functional tests:

  1. Probe & registration:
  $ echo "as5600 0x36" > /sys/bus/i2c/devices/i2c-2/new_device
  $ cat /sys/bus/iio/devices/iio:device0/name
  as5600

  2. Scale attribute:
  $ cat /sys/bus/iio/devices/iio:device0/in_angl_scale
  0.001533981

  3. Raw angle reads (magnet present, rotated by hand):
  $ cat /sys/bus/iio/devices/iio:device0/in_angl_raw
  576
  $ cat /sys/bus/iio/devices/iio:device0/in_angl_raw
  3758
  $ cat /sys/bus/iio/devices/iio:device0/in_angl_raw
  3115
  (multiple reads across 0-4095 range confirmed)

  4. Magnet removed (MD bit clear -> ENODATA):
  $ cat /sys/bus/iio/devices/iio:device0/in_angl_raw
  cat: '/sys/bus/iio/devices/iio:device0/in_angl_raw': No data available

  5. Transient bus error (glitch -> EIO, auto-recovery on next read):
  read_raw: STATUS read failed -> handle_io_error()
  handle_io_error: dummy read OK -> state stays Normal
  Next read: succeeds immediately

  6. Circuit breaker (bus disconnected -> Poisoned -> reconnected):
  read_raw: STATUS read failed -> handle_io_error()
  handle_io_error: dummy read failed -> state=Poisoned, return EIO
  Next read: state=Poisoned, recovery read failed -> stay Poisoned
  (reconnect bus)
  Next read: state=Poisoned, recovery read OK -> state=Normal
  Immediate angle read succeeds (no double-read, result used directly)
  Full cycle verified twice: Normal -> Poisoned -> Poisoned -> Normal

  7. Concurrent stress test (10 parallel readers, 10 seconds):
  $ for i in $(seq 1 10); do
      (while true; do cat .../in_angl_raw > /dev/null 2>&1; done) &
    done; sleep 10; kill $(jobs -p)
  $ dmesg | grep -i "oops\|panic\|bug\|rcu"
  (empty -- no kernel issues, no corrupted values)

  8. Unbind/rebind race (concurrent read + 20 lifecycle cycles):
  Terminal 1: while true; do cat .../in_angl_raw 2>/dev/null; done &
  Terminal 2: for i in {1..20}; do unbind; sleep 0.1; bind; sleep 0.1; done
  $ dmesg | grep -i "oops\|panic\|bug:"
  (empty -- iio_device_unregister drains callbacks before teardown)

  9. PinnedDrop cleanup verified via dmesg:
  iio_device_unregister -> drop_in_place(Mutex+KBox) -> iio_device_free

-- 
2.50.0

[RFC PATCH v3 1/4] i2c: rust: implement kernel::io::Io trait for I2cClient

[Muchamad Coirul Anwar]

Implement the Io trait for I2cClient per the agreed-upon direction
for I2C register access abstractions. This provides try_read8() and
try_read16() with automatic offset validation via io_addr().

I2cClient now implements IoCapable<u8> and IoCapable<u16> with
maxsize=256 (SMBus command byte range 0x00-0xFF).

Link: https://lore.kernel.org/rust-for-linux/20260131-i2c-adapter-v1-4-5a436e34cd1a@gmail.com/
Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
---
 rust/kernel/i2c.rs | 76 +++++++++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index 6eaea1158fda..cdbef6cfa344 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -14,6 +14,7 @@
     devres::Devres,
     driver,
     error::*,
+    io::{Io, IoCapable},
     of,
     prelude::*,
     sync::aref::{
@@ -477,30 +478,6 @@ impl<Ctx: device::DeviceContext> I2cClient<Ctx> {
     fn as_raw(&self) -> *mut bindings::i2c_client {
         self.0.get()
     }
-
-    /// Reads a single byte from a register via SMBus.
-    pub fn smbus_read_byte_data(&self, reg: u8) -> Result<u8> {
-        // SAFETY: `self.as_raw()` is a valid pointer to a `struct i2c_client`
-        // by the type invariant of `I2cClient`.
-        let ret = unsafe { bindings::i2c_smbus_read_byte_data(self.as_raw(), reg) };
-        if ret < 0 {
-            Err(Error::from_errno(ret))
-        } else {
-            Ok(ret as u8)
-        }
-    }
-
-    /// Reads a 16-bit word from a register via SMBus.
-    pub fn smbus_read_word_data(&self, reg: u8) -> Result<u16> {
-        // SAFETY: `self.as_raw()` is a valid pointer to a `struct i2c_client`
-        // 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)
-        }
-    }
 }
 
 // SAFETY: `I2cClient` is a transparent wrapper of `struct i2c_client`.
@@ -614,5 +591,54 @@ fn drop(&mut self) {
 unsafe impl Send for Registration {}
 
 // SAFETY: `Registration` offers no interior mutability (no mutation through &self
-// and no mutable access is exposed)
+// and no mutable access is exposed).
 unsafe impl Sync for Registration {}
+
+impl<Ctx: device::DeviceContext> IoCapable<u8> for I2cClient<Ctx> {}
+impl<Ctx: device::DeviceContext> IoCapable<u16> for I2cClient<Ctx> {}
+
+impl<Ctx: device::DeviceContext> Io for I2cClient<Ctx> {
+    #[inline]
+    fn addr(&self) -> usize {
+        0
+    }
+
+    #[inline]
+    fn maxsize(&self) -> usize {
+        256
+    }
+
+    #[inline]
+    fn try_read8(&self, offset: usize) -> Result<u8>
+    where
+        Self: IoCapable<u8>,
+    {
+        let reg = self.io_addr::<u8>(offset)? as u8;
+        // SAFETY: `self.as_raw()` returns a valid pointer to a `struct i2c_client`
+        // as guaranteed by the type invariant of `I2cClient`. `reg` is bounds-checked
+        // by `io_addr()` above (offset + 1 <= 256).
+        let ret = unsafe { bindings::i2c_smbus_read_byte_data(self.as_raw(), reg) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u8)
+        }
+    }
+
+    #[inline]
+    fn try_read16(&self, offset: usize) -> Result<u16>
+    where
+        Self: IoCapable<u16>,
+    {
+        let reg = self.io_addr::<u16>(offset)? as u8;
+        // SAFETY: `self.as_raw()` returns a valid pointer to a `struct i2c_client`
+        // as guaranteed by the type invariant of `I2cClient`. `reg` is bounds-checked
+        // by `io_addr()` above (offset + 2 <= 256).
+        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)
+        }
+    }
+}
-- 
2.50.0

[RFC PATCH v3 2/4] rust: add minimal IIO subsystem abstractions

[Muchamad Coirul Anwar]

Add safe Rust wrappers for the Linux IIO subsystem. Provides:
- Device<T, State> with typestate pattern (Unregistered/Registered)
- IioDriver trait with read_raw callback
- DirectModeGuard RAII for iio_device_claim_direct
- IioVal enum with NonZeroI32 for division-by-zero prevention
- PinnedDrop cleanup: unregister -> drop_in_place -> iio_device_free

Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
---
 rust/helpers/helpers.c |   1 +
 rust/helpers/iio.c     |  24 +++
 rust/kernel/iio.rs     | 341 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   2 +
 4 files changed, 368 insertions(+)
 create mode 100644 rust/helpers/iio.c
 create mode 100644 rust/kernel/iio.rs

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index a3c42e51f00a..c69a9a93367d 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -33,6 +33,7 @@
 #include "irq.c"
 #include "fs.c"
 #include "io.c"
+#include "iio.c"
 #include "jump_label.c"
 #include "kunit.c"
 #include "maple_tree.c"
diff --git a/rust/helpers/iio.c b/rust/helpers/iio.c
new file mode 100644
index 000000000000..a5402440583c
--- /dev/null
+++ b/rust/helpers/iio.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/iio/iio.h>
+
+/*
+ * iio_device_claim_direct() is a static inline in iio.h.
+ * This helper exports it as a callable symbol for Rust.
+ */
+__rust_helper bool
+rust_helper_iio_device_claim_direct(struct iio_dev *indio_dev)
+{
+	return iio_device_claim_direct(indio_dev);
+}
+
+/*
+ * iio_device_release_direct() is a macro expanding to __iio_dev_mode_unlock().
+ * This helper exports it as a callable symbol for Rust.
+ */
+__rust_helper void
+rust_helper_iio_device_release_direct(struct iio_dev *indio_dev)
+{
+	iio_device_release_direct(indio_dev);
+}
diff --git a/rust/kernel/iio.rs b/rust/kernel/iio.rs
new file mode 100644
index 000000000000..bbd34f1c819a
--- /dev/null
+++ b/rust/kernel/iio.rs
@@ -0,0 +1,341 @@
+// 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,
+        INDIO_DIRECT_MODE, //
+    },
+    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;
+
+/// 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 (10⁻⁶) fractional part: `val.val2`.
+    IntPlusMicro(i32, i32),
+    /// An integer plus a nano (10⁻⁹) 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; `mask` selects the attribute
+    /// (e.g. `IIO_CHAN_INFO_RAW`, `IIO_CHAN_INFO_SCALE`).
+    fn read_raw(&self, chan: *const iio_chan_spec, mask: isize) -> Result<IioVal>;
+
+    /// Returns the channel specifications for this driver.
+    ///
+    /// The default implementation returns an empty slice.
+    fn channels(&self) -> &[iio_chan_spec] {
+        &[]
+    }
+}
+
+// ---------------------------------------------------------------------------
+// DirectModeGuard — RAII claim on IIO direct mode
+// ---------------------------------------------------------------------------
+
+/// RAII guard that claims IIO direct mode on construction and releases it on drop.
+///
+/// This prevents concurrent access conflicts between sysfs reads and
+/// buffer/trigger operations.
+struct DirectModeGuard(*mut iio_dev);
+
+impl DirectModeGuard {
+    fn new(indio_dev: *mut iio_dev) -> Result<Self> {
+        // SAFETY: `indio_dev` is a valid pointer to a fully initialized `iio_dev`
+        // allocated by `iio_device_alloc`. `iio_device_claim_direct` returns `true`
+        // if the device is in direct mode (success), `false` if buffer mode is active.
+        let claimed = unsafe { crate::bindings::iio_device_claim_direct(indio_dev) };
+        if claimed {
+            Ok(Self(indio_dev))
+        } else {
+            Err(EBUSY)
+        }
+    }
+}
+
+impl Drop for DirectModeGuard {
+    fn drop(&mut self) {
+        // SAFETY: `self.0` was successfully claimed in `new()`. Releasing it
+        // unlocks the IIO mode lock acquired during claim.
+        unsafe {
+            crate::bindings::iio_device_release_direct(self.0);
+        }
+    }
+}
+
+// ---------------------------------------------------------------------------
+// read_raw_callback — C-to-Rust FFI trampoline
+// ---------------------------------------------------------------------------
+
+/// C-compatible trampoline for the `iio_info.read_raw` callback.
+///
+/// # Safety
+///
+/// This function is only called by the IIO core with valid pointers:
+/// - `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` and `val2` are valid pointers for writing the result.
+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,
+    mask: 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 };
+
+    // Claim direct mode via RAII guard. If the device is in buffer mode,
+    // return -EBUSY to userspace immediately.
+    let _guard = match DirectModeGuard::new(indio_dev) {
+        Ok(g) => g,
+        Err(e) => return e.to_errno(),
+    };
+
+    match driver.read_raw(chan, mask) {
+        Ok(IioVal::Int(v)) => {
+            // SAFETY: `val` is a valid pointer provided by the IIO core.
+            unsafe {
+                *val = v;
+            }
+            IIO_VAL_INT
+        }
+        Ok(IioVal::Fractional(v, v2)) => {
+            // SAFETY: `val` and `val2` are valid pointers provided by the IIO core.
+            unsafe {
+                *val = v;
+                *val2 = v2.get();
+            }
+            IIO_VAL_FRACTIONAL
+        }
+        Ok(IioVal::IntPlusMicro(v, v2)) => {
+            // SAFETY: `val` and `val2` are valid pointers provided by the IIO core.
+            unsafe {
+                *val = v;
+                *val2 = v2;
+            }
+            IIO_VAL_INT_PLUS_MICRO
+        }
+        Ok(IioVal::IntPlusNano(v, v2)) => {
+            // SAFETY: `val` and `val2` are valid pointers provided by the IIO core.
+            unsafe {
+                *val = v;
+                *val2 = v2;
+            }
+            IIO_VAL_INT_PLUS_NANO
+        }
+        Err(e) => e.to_errno(),
+    }
+}
+
+// ---------------------------------------------------------------------------
+// Device — 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 (no devres involvement) and
+/// manual cleanup via `Drop`: `iio_device_unregister` → `drop_in_place`
+/// for `T` → `iio_device_free`.
+#[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: The device was successfully registered via
+            // `__iio_device_register`. Unregistering drains all pending
+            // callbacks, ensuring no `read_raw` is in flight after this.
+            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: The remaining fields of `iio_info` are pointers and function
+    // pointers. Zeroed values are NULL, and the IIO core checks for NULL
+    // before invoking callbacks or dereferencing attribute group pointers.
+    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,
+        init: impl PinInit<T, E>,
+    ) -> Result<Self>
+    where
+        Error: From<E>,
+    {
+        let priv_size = size_of::<T>();
+
+        // 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 as i32) };
+        if indio_dev.is_null() {
+            return Err(ENOMEM);
+        }
+
+        // SAFETY: `indio_dev` is valid and freshly allocated. `priv_` points
+        // to uninitialized memory of `sizeof(T)` bytes. `PinInit::__pinned_init`
+        // initializes `priv_` in place without reading the previous
+        // (uninitialized) 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()` returns a reference to data owned by `T` in `priv_`,
+        //   which remains at a fixed address because `priv_` is heap-allocated
+        //   inside `iio_dev`.
+        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 = INDIO_DIRECT_MODE 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 138d846f798d..ec6eb4dbdb6a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -99,6 +99,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

[RFC PATCH v3 3/4] iio: position: add Rust driver for ams AS5600

[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:
- Circuit breaker pattern for I/O error resilience
- Mutex-serialized multi-byte angle read sequence
- Automatic recovery from bus failures (Poisoned -> Normal)
- No magnet validation at probe time (deferred to read_raw)

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

Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
---
 drivers/iio/position/as5600.rs | 289 +++++++++++++++++++++++++++++++++
 1 file changed, 289 insertions(+)
 create mode 100644 drivers/iio/position/as5600.rs

diff --git a/drivers/iio/position/as5600.rs b/drivers/iio/position/as5600.rs
new file mode 100644
index 000000000000..f87df650a91d
--- /dev/null
+++ b/drivers/iio/position/as5600.rs
@@ -0,0 +1,289 @@
+// 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.
+//!
+//! Datasheet: https://ams.com/documents/20143/36005/AS5600_DS000365_5-00.pdf
+
+use kernel::{
+    alloc::KBox,
+    bindings::{
+        i2c_client,
+        iio_chan_info_enum_IIO_CHAN_INFO_RAW,
+        iio_chan_info_enum_IIO_CHAN_INFO_SCALE,
+        iio_chan_spec,
+        iio_chan_type_IIO_ANGL,
+        ENODATA, //
+    },
+    bits::bit_u8,
+    device::Core,
+    error::{code::EIO, Error},
+    i2c::{
+        DeviceId,
+        Driver,
+        I2cClient,
+        IdTable, //
+    },
+    i2c_device_table,
+    iio::{
+        Device,
+        IioDriver,
+        IioVal,
+        Registered, //
+    },
+    io::{Io, IoCapable},
+    module_i2c_driver, of, of_device_table,
+    prelude::*,
+    sync::{
+        new_mutex,
+        Mutex, //
+    },
+};
+
+const AS5600_REG_STATUS: u8 = 0x0B;
+const AS5600_REG_RAW_ANGLE_H: u8 = 0x0C;
+const AS5600_REG_RAW_ANGLE_L: u8 = 0x0D;
+
+const AS5600_STATUS_MD: u8 = bit_u8(5);
+
+/// Returns kernel error `ENODATA`.
+///
+/// Helper needed because `Error::from_errno` is not `const fn` and `ENODATA`
+/// is only available as a raw `u32` binding, not as a wrapped `kernel::error::code`.
+#[inline(always)]
+fn err_enodata() -> Error {
+    Error::from_errno(-(ENODATA as i32))
+}
+
+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"), ())]
+);
+
+#[derive(Clone, Copy)]
+struct As5600Io(*mut i2c_client);
+
+/// Tracks the health state of the hardware bus to prevent I/O storms.
+#[derive(Clone, Copy, PartialEq, Eq)]
+enum DeviceState {
+    Normal,
+    Poisoned,
+}
+
+// SAFETY: `As5600Io` wraps a raw pointer to `i2c_client`. This is `Send`
+// and `Sync` because:
+// - The I2C subsystem guarantees the `i2c_client` (parent) outlives the
+//   IIO device (child) via the Linux Device Model unbind ordering.
+// - All hardware access is serialized through `Mutex<As5600HwState<As5600Io>>`
+//   (`io_lock`), and individual SMBus transactions are serialized by the I2C
+//   adapter lock.
+unsafe impl Send for As5600Io {}
+unsafe impl Sync for As5600Io {}
+
+impl IoCapable<u8> for As5600Io {}
+impl IoCapable<u16> for As5600Io {}
+
+impl Io for As5600Io {
+    #[inline]
+    fn addr(&self) -> usize {
+        0
+    }
+
+    #[inline]
+    fn maxsize(&self) -> usize {
+        256
+    }
+
+    #[inline]
+    fn try_read8(&self, offset: usize) -> Result<u8>
+    where
+        Self: IoCapable<u8>,
+    {
+        // SAFETY: `self.0` points to a valid `i2c_client` guaranteed by the
+        // Device Model lifetime hierarchy (parent outlives child). The cast is
+        // valid because `I2cClient` is `#[repr(transparent)]` over `i2c_client`.
+        let client = unsafe { &*(self.0 as *const I2cClient<Core>) };
+        client.try_read8(offset)
+    }
+
+    #[inline]
+    fn try_read16(&self, offset: usize) -> Result<u16>
+    where
+        Self: IoCapable<u16>,
+    {
+        // SAFETY: `self.0` points to a valid `i2c_client` guaranteed by the
+        // Device Model lifetime hierarchy (parent outlives child). The cast is
+        // valid because `I2cClient` is `#[repr(transparent)]` over `i2c_client`.
+        let client = unsafe { &*(self.0 as *const I2cClient<Core>) };
+        client.try_read16(offset)
+    }
+}
+
+#[pin_data]
+struct As5600Priv<T> {
+    #[pin]
+    io_lock: Mutex<As5600HwState<T>>,
+    channels: KBox<[iio_chan_spec; 1]>,
+}
+
+/// Encapsulates the I/O interface and its runtime health state.
+///
+/// This prevents operations on a known-dead bus (Circuit Breaker pattern).
+struct As5600HwState<T> {
+    io: T,
+    state: DeviceState,
+}
+
+impl<T: Io + IoCapable<u8>> As5600HwState<T> {
+    /// Performs a dummy read to probe bus health after an I/O failure.
+    ///
+    /// Returns `EIO` in all cases — the caller should always propagate the error.
+    /// The side effect determines recovery behavior:
+    /// - If the dummy read **succeeds**: state is reset to `Normal`, meaning the
+    ///   next `read_raw` call will attempt normal operation directly.
+    /// - If the dummy read **fails**: state is set to `Poisoned`, meaning the
+    ///   next `read_raw` call will attempt recovery before normal operation.
+    fn handle_io_error(&mut self) -> Error {
+        match self.io.try_read8(AS5600_REG_STATUS as usize) {
+            Ok(_) => {
+                self.state = DeviceState::Normal;
+                EIO
+            }
+            Err(_) => {
+                self.state = DeviceState::Poisoned;
+                EIO
+            }
+        }
+    }
+}
+
+// SAFETY: `As5600Priv<T>` is `Send` and `Sync` because:
+// - `T: IoCapable<u8>` is a marker trait with no interior mutability.
+//   The underlying `As5600Io` wrapper's Send/Sync is guaranteed by its
+//   manual impls (serialized via Mutex + I2C adapter lock).
+// - `channels` is a heap-allocated array (`KBox`) with no interior mutability.
+// - `io_lock: Mutex<As5600HwState<T>>` provides synchronized interior mutability.
+// - `DeviceState` is a plain enum without interior mutability (Send + Sync
+//   implicitly).
+// All concurrent access to hardware goes through the `Mutex` guard.
+// The `Unpin` bound is strictly required because `kernel::sync::lock::Guard`
+// only implements `DerefMut` for `T: Unpin`. Without it, state mutation fails.
+unsafe impl<T: IoCapable<u8> + Unpin> Send for As5600Priv<T> {}
+unsafe impl<T: IoCapable<u8> + Unpin> Sync for As5600Priv<T> {}
+
+impl<T: Io + IoCapable<u8> + Unpin> IioDriver for As5600Priv<T> {
+    fn read_raw(&self, _chan: *const iio_chan_spec, mask: isize) -> Result<IioVal> {
+        match mask {
+            // IIO_CHAN_INFO_RAW — read the 12-bit raw angle value.
+            m if m == iio_chan_info_enum_IIO_CHAN_INFO_RAW as isize => {
+                let mut hw_guard = self.io_lock.lock();
+
+                // If the bus was previously poisoned, attempt a single recovery
+                // read before proceeding with normal operation.
+                let status = if hw_guard.state == DeviceState::Poisoned {
+                    match hw_guard.io.try_read8(AS5600_REG_STATUS as usize) {
+                        Ok(s) => {
+                            hw_guard.state = DeviceState::Normal;
+                            s
+                        }
+                        Err(_) => return Err(EIO),
+                    }
+                } else {
+                    match hw_guard.io.try_read8(AS5600_REG_STATUS as usize) {
+                        Ok(s) => s,
+                        Err(_) => return Err(hw_guard.handle_io_error()),
+                    }
+                };
+
+                // Check magnet presence (MD bit). Without a magnet the angle
+                // register contains stale/invalid data.
+                if (status & AS5600_STATUS_MD) == 0 {
+                    return Err(err_enodata());
+                }
+
+                // Read the 12-bit angle as two bytes. The AS5600 hardware
+                // freezes the internal angle value on reading the high byte
+                // until the low byte is read — the Mutex ensures this
+                // sequence is not interleaved by concurrent readers.
+                let angle_h = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_H as usize) {
+                    Ok(v) => v as u16,
+                    Err(_) => return Err(hw_guard.handle_io_error()),
+                };
+                let angle_l = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_L as usize) {
+                    Ok(v) => v as u16,
+                    Err(_) => return Err(hw_guard.handle_io_error()),
+                };
+
+                let angle = (angle_h << 8 | angle_l) & 0x0FFF;
+                Ok(IioVal::Int(angle as i32))
+            }
+            // IIO_CHAN_INFO_SCALE — radians per LSB: 2π / 4096 ≈ 0.001533981.
+            m if m == iio_chan_info_enum_IIO_CHAN_INFO_SCALE as isize => {
+                Ok(IioVal::IntPlusNano(0, 1533981))
+            }
+            _ => Err(kernel::error::code::EINVAL),
+        }
+    }
+
+    fn channels(&self) -> &[iio_chan_spec] {
+        &self.channels[..]
+    }
+}
+
+struct As5600 {
+    _iio_dev: Device<As5600Priv<As5600Io>, Registered>,
+}
+
+impl Driver for As5600 {
+    type IdInfo = ();
+    const I2C_ID_TABLE: Option<IdTable<Self::IdInfo>> = Some(&I2C_TABLE);
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+    #[allow(refining_impl_trait)]
+    fn probe(dev: &I2cClient<Core>, _id_info: Option<&Self::IdInfo>) -> Result<Self> {
+        // SAFETY: `iio_chan_spec` is a C struct whose fields are all integers
+        // and pointers. Zero is a valid initialization for all of them.
+        let mut channels_alloc = kernel::alloc::KBox::new(
+            [unsafe { core::mem::zeroed::<iio_chan_spec>() }],
+            kernel::alloc::flags::GFP_KERNEL,
+        )?;
+
+        channels_alloc[0].info_mask_separate = (1 << iio_chan_info_enum_IIO_CHAN_INFO_RAW)
+            | (1 << iio_chan_info_enum_IIO_CHAN_INFO_SCALE);
+        channels_alloc[0].type_ = iio_chan_type_IIO_ANGL;
+
+        let client_ptr = dev as *const _ as *mut i2c_client;
+
+        let priv_init = pin_init!(As5600Priv {
+            io_lock <- new_mutex!(As5600HwState {
+                io: As5600Io(client_ptr),
+                state: DeviceState::Normal,
+            }),
+            channels: channels_alloc,
+        });
+
+        let iio_dev = Device::build_device(dev.as_ref(), c"as5600", priv_init)?;
+        let iio_dev_registered = iio_dev.register(&crate::THIS_MODULE)?;
+
+        dev_info!(dev.as_ref(), "AS5600 magnetic position sensor ready\n");
+        Ok(As5600 {
+            _iio_dev: iio_dev_registered,
+        })
+    }
+}
-- 
2.50.0

[RFC PATCH v3 4/4] iio: position: as5600: add Kconfig and Makefile entries

[Muchamad Coirul Anwar]

Add build system integration for the AS5600 Rust IIO driver.
CONFIG_AS5600 depends on I2C, IIO, and RUST.

Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
---
 drivers/iio/position/Kconfig  | 14 ++++++++++++++
 drivers/iio/position/Makefile |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig
index 1576a6380b53..dab9310e8079 100644
--- a/drivers/iio/position/Kconfig
+++ b/drivers/iio/position/Kconfig
@@ -6,6 +6,20 @@
 
 menu "Linear and angular position sensors"
 
+config AS5600
+	tristate "ams AS5600 magnetic rotary position sensor"
+	depends on I2C && RUST
+	help
+	  Say Y here to build support for the ams AS5600 12-bit
+	  magnetic rotary position sensor with IIO channel support
+	  (in_angl_raw and in_angl_scale).
+
+	  This is a Rust driver that exposes the 12-bit raw angle
+	  and radian scale via the IIO subsystem.
+
+	  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
-- 
2.50.0

Re: [RFC PATCH v3 1/4] i2c: rust: implement kernel::io::Io trait for I2cClient

[Jonathan Cameron]

On Sun, 24 May 2026 20:28:20 +0700
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> Implement the Io trait for I2cClient per the agreed-upon direction
> for I2C register access abstractions. This provides try_read8() and
> try_read16() with automatic offset validation via io_addr().
> 
> I2cClient now implements IoCapable<u8> and IoCapable<u16> with
> maxsize=256 (SMBus command byte range 0x00-0xFF).
> Link: https://lore.kernel.org/rust-for-linux/20260131-i2c-adapter-v1-4-5a436e34cd1a@gmail.com/
> Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>

Usual thing that I have near zero rust experience :(
So this is vary superficial..

My main concern is this currently takes away the clarity off smbus
naming and replaces it with the impression this is how i2c reads and writes
are done in general.  How will this support other forms of access?

How do we have lots of different types of i2c supported?  Simplest being
the ones regmap supports today. There are 7ish in drivers/base/regmap-i2c.c

Or if the plan is to only support register style interfaces why not only
allow for use of regmap?

One other comment inline. I'm seeing what looks to be a check for an 8 bit
address whereas smbus is 7 bit addressing.
	
Jonathan



> ---
>  rust/kernel/i2c.rs | 76 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 51 insertions(+), 25 deletions(-)
> 
> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> index 6eaea1158fda..cdbef6cfa344 100644
> --- a/rust/kernel/i2c.rs
> +++ b/rust/kernel/i2c.rs
> @@ -14,6 +14,7 @@
>      devres::Devres,
>      driver,
>      error::*,
> +    io::{Io, IoCapable},
>      of,
>      prelude::*,
>      sync::aref::{
> @@ -477,30 +478,6 @@ impl<Ctx: device::DeviceContext> I2cClient<Ctx> {
>      fn as_raw(&self) -> *mut bindings::i2c_client {
>          self.0.get()
>      }
> -
> -    /// Reads a single byte from a register via SMBus.
> -    pub fn smbus_read_byte_data(&self, reg: u8) -> Result<u8> {
> -        // SAFETY: `self.as_raw()` is a valid pointer to a `struct i2c_client`
> -        // by the type invariant of `I2cClient`.
> -        let ret = unsafe { bindings::i2c_smbus_read_byte_data(self.as_raw(), reg) };
> -        if ret < 0 {
> -            Err(Error::from_errno(ret))
> -        } else {
> -            Ok(ret as u8)
> -        }
> -    }
> -
> -    /// Reads a 16-bit word from a register via SMBus.
> -    pub fn smbus_read_word_data(&self, reg: u8) -> Result<u16> {
> -        // SAFETY: `self.as_raw()` is a valid pointer to a `struct i2c_client`
> -        // 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)
> -        }
> -    }
>  }
>  
>  // SAFETY: `I2cClient` is a transparent wrapper of `struct i2c_client`.
> @@ -614,5 +591,54 @@ fn drop(&mut self) {
>  unsafe impl Send for Registration {}
>  
>  // SAFETY: `Registration` offers no interior mutability (no mutation through &self
> -// and no mutable access is exposed)
> +// and no mutable access is exposed).

Unrelated change.

>  unsafe impl Sync for Registration {}
> +
> +impl<Ctx: device::DeviceContext> IoCapable<u8> for I2cClient<Ctx> {}
> +impl<Ctx: device::DeviceContext> IoCapable<u16> for I2cClient<Ctx> {}
> +
> +impl<Ctx: device::DeviceContext> Io for I2cClient<Ctx> {
> +    #[inline]
> +    fn addr(&self) -> usize {
> +        0
> +    }
> +
> +    #[inline]
> +    fn maxsize(&self) -> usize {
> +        256
> +    }
> +
> +    #[inline]
> +    fn try_read8(&self, offset: usize) -> Result<u8>
> +    where
> +        Self: IoCapable<u8>,
> +    {
> +        let reg = self.io_addr::<u8>(offset)? as u8;
> +        // SAFETY: `self.as_raw()` returns a valid pointer to a `struct i2c_client`
> +        // as guaranteed by the type invariant of `I2cClient`. `reg` is bounds-checked
> +        // by `io_addr()` above (offset + 1 <= 256).

Except smbus standard addressing is 7 bit.
Need space for the read / write bit. https://docs.kernel.org/i2c/smbus-protocol.html

> +        let ret = unsafe { bindings::i2c_smbus_read_byte_data(self.as_raw(), reg) };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u8)
> +        }
> +    }
> +
> +    #[inline]
> +    fn try_read16(&self, offset: usize) -> Result<u16>
> +    where
> +        Self: IoCapable<u16>,
> +    {
> +        let reg = self.io_addr::<u16>(offset)? as u8;
> +        // SAFETY: `self.as_raw()` returns a valid pointer to a `struct i2c_client`
> +        // as guaranteed by the type invariant of `I2cClient`. `reg` is bounds-checked
> +        // by `io_addr()` above (offset + 2 <= 256).
> +        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)
> +        }
> +    }
> +}

Re: [RFC PATCH v3 3/4] iio: position: add Rust driver for ams AS5600

[Jonathan Cameron]

On Sun, 24 May 2026 20:28:22 +0700
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:
> - Circuit breaker pattern for I/O error resilience

I'm not sure why this is particularly useful, but maybe I'm missing something.

> - Mutex-serialized multi-byte angle read sequence
> - Automatic recovery from bus failures (Poisoned -> Normal)

I mention this below - it works (sort of) because right now you just
do reads with no state changes.  In general device recovery is way
more complex than what you do.

Various other comments inline. Some are wish list things that may
or may not have rust equivalents of what we'd do in C.

> - No magnet validation at probe time (deferred to read_raw)
> 
> Tested on BeagleBone Black (AM335x) with AS5600 on i2c-2 (0x36).
> 
> Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
> ---
>  drivers/iio/position/as5600.rs | 289 +++++++++++++++++++++++++++++++++
>  1 file changed, 289 insertions(+)
>  create mode 100644 drivers/iio/position/as5600.rs

> +
> +#[derive(Clone, Copy)]
> +struct As5600Io(*mut i2c_client);
> +
> +/// Tracks the health state of the hardware bus to prevent I/O storms.
I think we need a separate discussion on how to do device recovery.
It is at best a black art and very device specific if bus corruption occurred.
On a read you can maybe get away with this but on a write we have no idea
if it wrote or not and that write may or may not have had side effects.

If you lose a write, the only always correct thing to do is a device
reset.  Other stuff is very device specific.

No writes in here yet which helps ;)

> +#[derive(Clone, Copy, PartialEq, Eq)]
> +enum DeviceState {
> +    Normal,
> +    Poisoned,
> +}


> +
> +impl<T: Io + IoCapable<u8>> As5600HwState<T> {
> +    /// Performs a dummy read to probe bus health after an I/O failure.

If you got a write failure, this recovers the bus but leaves you not knowing
if the write succeeded or not. I'm not sure how helpful it is.

> +    ///
> +    /// Returns `EIO` in all cases — the caller should always propagate the error.
> +    /// The side effect determines recovery behavior:
> +    /// - If the dummy read **succeeds**: state is reset to `Normal`, meaning the
> +    ///   next `read_raw` call will attempt normal operation directly.
> +    /// - If the dummy read **fails**: state is set to `Poisoned`, meaning the
> +    ///   next `read_raw` call will attempt recovery before normal operation.
> +    fn handle_io_error(&mut self) -> Error {
> +        match self.io.try_read8(AS5600_REG_STATUS as usize) {
> +            Ok(_) => {
> +                self.state = DeviceState::Normal;
> +                EIO
> +            }
> +            Err(_) => {
> +                self.state = DeviceState::Poisoned;
> +                EIO
> +            }
> +        }
> +    }
> +}
> +
> +// SAFETY: `As5600Priv<T>` is `Send` and `Sync` because:
> +// - `T: IoCapable<u8>` is a marker trait with no interior mutability.
> +//   The underlying `As5600Io` wrapper's Send/Sync is guaranteed by its
> +//   manual impls (serialized via Mutex + I2C adapter lock).
> +// - `channels` is a heap-allocated array (`KBox`) with no interior mutability.
> +// - `io_lock: Mutex<As5600HwState<T>>` provides synchronized interior mutability.
> +// - `DeviceState` is a plain enum without interior mutability (Send + Sync
> +//   implicitly).
> +// All concurrent access to hardware goes through the `Mutex` guard.
> +// The `Unpin` bound is strictly required because `kernel::sync::lock::Guard`
> +// only implements `DerefMut` for `T: Unpin`. Without it, state mutation fails.
> +unsafe impl<T: IoCapable<u8> + Unpin> Send for As5600Priv<T> {}
> +unsafe impl<T: IoCapable<u8> + Unpin> Sync for As5600Priv<T> {}
> +
> +impl<T: Io + IoCapable<u8> + Unpin> IioDriver for As5600Priv<T> {
> +    fn read_raw(&self, _chan: *const iio_chan_spec, mask: isize) -> Result<IioVal> {
> +        match mask {
> +            // IIO_CHAN_INFO_RAW — read the 12-bit raw angle value.
> +            m if m == iio_chan_info_enum_IIO_CHAN_INFO_RAW as isize => {
> +                let mut hw_guard = self.io_lock.lock();
> +
> +                // If the bus was previously poisoned, attempt a single recovery
> +                // read before proceeding with normal operation.
> +                let status = if hw_guard.state == DeviceState::Poisoned {
> +                    match hw_guard.io.try_read8(AS5600_REG_STATUS as usize) {
> +                        Ok(s) => {
> +                            hw_guard.state = DeviceState::Normal;
> +                            s
> +                        }
> +                        Err(_) => return Err(EIO),
> +                    }
> +                } else {
> +                    match hw_guard.io.try_read8(AS5600_REG_STATUS as usize) {
> +                        Ok(s) => s,
> +                        Err(_) => return Err(hw_guard.handle_io_error()),
> +                    }
> +                };
> +
> +                // Check magnet presence (MD bit). Without a magnet the angle
> +                // register contains stale/invalid data.
> +                if (status & AS5600_STATUS_MD) == 0 {
> +                    return Err(err_enodata());
> +                }
> +
> +                // Read the 12-bit angle as two bytes. The AS5600 hardware
> +                // freezes the internal angle value on reading the high byte
> +                // until the low byte is read — the Mutex ensures this
> +                // sequence is not interleaved by concurrent readers.

Would be very unusual if the device does this but doesn't support some form of larger
read. Would be nice to be able to do that in a rust driver.

> +                let angle_h = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_H as usize) {
> +                    Ok(v) => v as u16,
> +                    Err(_) => return Err(hw_guard.handle_io_error()),
> +                };
> +                let angle_l = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_L as usize) {
> +                    Ok(v) => v as u16,
> +                    Err(_) => return Err(hw_guard.handle_io_error()),
> +                };
> +
> +                let angle = (angle_h << 8 | angle_l) & 0x0FFF;

This is the sort of thing we'd never do in a C driver because we have well defined meaningful
functions / macros for doing this.  I'd like to see something equivalent in the rust code of.

   Bulk read int a two byte array. i2c_smbus_read_word_data() or swapped variant.
   Unaligned endian read  get_unaligned_be16()
   Masking to extract the 12 bits that are valid. FIELD_GET() whatever.

Might not matter in this driver but once you get 24bit reads or bigger doing open coded
version is not particularly readable.

> +                Ok(IioVal::Int(angle as i32))
> +            }
> +            // IIO_CHAN_INFO_SCALE — radians per LSB: 2π / 4096 ≈ 0.001533981.
> +            m if m == iio_chan_info_enum_IIO_CHAN_INFO_SCALE as isize => {
> +                Ok(IioVal::IntPlusNano(0, 1533981))
> +            }
> +            _ => Err(kernel::error::code::EINVAL),
> +        }
> +    }
> +
> +    fn channels(&self) -> &[iio_chan_spec] {
> +        &self.channels[..]
> +    }
> +}
> +
> +struct As5600 {
> +    _iio_dev: Device<As5600Priv<As5600Io>, Registered>,
> +}
> +
> +impl Driver for As5600 {
> +    type IdInfo = ();
> +    const I2C_ID_TABLE: Option<IdTable<Self::IdInfo>> = Some(&I2C_TABLE);
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    #[allow(refining_impl_trait)]
> +    fn probe(dev: &I2cClient<Core>, _id_info: Option<&Self::IdInfo>) -> Result<Self> {
> +        // SAFETY: `iio_chan_spec` is a C struct whose fields are all integers
> +        // and pointers. Zero is a valid initialization for all of them.
> +        let mut channels_alloc = kernel::alloc::KBox::new(
> +            [unsafe { core::mem::zeroed::<iio_chan_spec>() }],
> +            kernel::alloc::flags::GFP_KERNEL,
> +        )?;
> +
> +        channels_alloc[0].info_mask_separate = (1 << iio_chan_info_enum_IIO_CHAN_INFO_RAW)
> +            | (1 << iio_chan_info_enum_IIO_CHAN_INFO_SCALE);

I believe there is a kernel rust equivalent of BIT(). Can you use that here.

> +        channels_alloc[0].type_ = iio_chan_type_IIO_ANGL;
> +
> +        let client_ptr = dev as *const _ as *mut i2c_client;
> +
> +        let priv_init = pin_init!(As5600Priv {
> +            io_lock <- new_mutex!(As5600HwState {
> +                io: As5600Io(client_ptr),
> +                state: DeviceState::Normal,
> +            }),
> +            channels: channels_alloc,
> +        });
> +
> +        let iio_dev = Device::build_device(dev.as_ref(), c"as5600", priv_init)?;
> +        let iio_dev_registered = iio_dev.register(&crate::THIS_MODULE)?;
> +
> +        dev_info!(dev.as_ref(), "AS5600 magnetic position sensor ready\n");

Too noisy. dev_dbg() at most.

> +        Ok(As5600 {
> +            _iio_dev: iio_dev_registered,
> +        })
> +    }
> +}

Re: [RFC PATCH v3 4/4] iio: position: as5600: add Kconfig and Makefile entries

[Jonathan Cameron]

On Sun, 24 May 2026 20:28:23 +0700
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> Add build system integration for the AS5600 Rust IIO driver.
> CONFIG_AS5600 depends on I2C, IIO, and RUST.
> 
> Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
Combine with the driver patch.  Separate patches for build files
provide no value.

> ---
>  drivers/iio/position/Kconfig  | 14 ++++++++++++++
>  drivers/iio/position/Makefile |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig
> index 1576a6380b53..dab9310e8079 100644
> --- a/drivers/iio/position/Kconfig
> +++ b/drivers/iio/position/Kconfig
> @@ -6,6 +6,20 @@
>  
>  menu "Linear and angular position sensors"
>  
> +config AS5600
> +	tristate "ams AS5600 magnetic rotary position sensor"
> +	depends on I2C && RUST
> +	help
> +	  Say Y here to build support for the ams AS5600 12-bit
> +	  magnetic rotary position sensor with IIO channel support
> +	  (in_angl_raw and in_angl_scale).
> +
> +	  This is a Rust driver that exposes the 12-bit raw angle
> +	  and radian scale via the IIO subsystem.
> +
> +	  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

Re: [RFC PATCH v3 2/4] rust: add minimal IIO subsystem abstractions

[Jonathan Cameron]

On Sun, 24 May 2026 20:28:21 +0700
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> Add safe Rust wrappers for the Linux IIO subsystem. Provides:
> - Device<T, State> with typestate pattern (Unregistered/Registered)
> - IioDriver trait with read_raw callback
> - DirectModeGuard RAII for iio_device_claim_direct

You'll need a lot more stuff before it makes any sense to be messing
with claims on the state.  They only come in once drivers are dealing
with buffers etc. I'd leave them until then.

> - IioVal enum with NonZeroI32 for division-by-zero prevention
> - PinnedDrop cleanup: unregister -> drop_in_place -> iio_device_free
> 
> Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
> ---
>  rust/helpers/helpers.c |   1 +
>  rust/helpers/iio.c     |  24 +++
>  rust/kernel/iio.rs     | 341 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |   2 +
>  4 files changed, 368 insertions(+)
>  create mode 100644 rust/helpers/iio.c
>  create mode 100644 rust/kernel/iio.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index a3c42e51f00a..c69a9a93367d 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -33,6 +33,7 @@
>  #include "irq.c"
>  #include "fs.c"
>  #include "io.c"
> +#include "iio.c"
>  #include "jump_label.c"
>  #include "kunit.c"
>  #include "maple_tree.c"
> diff --git a/rust/helpers/iio.c b/rust/helpers/iio.c
> new file mode 100644
> index 000000000000..a5402440583c
> --- /dev/null
> +++ b/rust/helpers/iio.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/iio/iio.h>
> +
> +/*
> + * iio_device_claim_direct() is a static inline in iio.h.
> + * This helper exports it as a callable symbol for Rust.
> + */
> +__rust_helper bool
> +rust_helper_iio_device_claim_direct(struct iio_dev *indio_dev)
> +{
> +	return iio_device_claim_direct(indio_dev);
> +}
> +
> +/*
> + * iio_device_release_direct() is a macro expanding to __iio_dev_mode_unlock().
> + * This helper exports it as a callable symbol for Rust.
> + */
> +__rust_helper void
> +rust_helper_iio_device_release_direct(struct iio_dev *indio_dev)
> +{
> +	iio_device_release_direct(indio_dev);
> +}

As above with the level of bindings you are doing so far, neither of these
should ever be called.  They are not for use by drivers simply wanting
to serialize things but about controlling the ability of the IIO core to
change the fundamental data flow from simple polling to streaming data to
userspace accessible kfifos.


> diff --git a/rust/kernel/iio.rs b/rust/kernel/iio.rs
> new file mode 100644
> index 000000000000..bbd34f1c819a
> --- /dev/null
> +++ b/rust/kernel/iio.rs


> +// ---------------------------------------------------------------------------
> +// DirectModeGuard — RAII claim on IIO direct mode
> +// ---------------------------------------------------------------------------
> +
> +/// RAII guard that claims IIO direct mode on construction and releases it on drop.
> +///
> +/// This prevents concurrent access conflicts between sysfs reads and
> +/// buffer/trigger operations.

Sort of. It ensures the state doesn't change.  Given buffer/trigger operations only
occur when in a buffer state it sort of of prevents their operations. That's not
including stuff like setting up buffers though - just the data streaming.
Anyhow, kick it in the long grass (drop for now) given you don't support buffer state.

Good to see what this probably looks like though.


> +struct DirectModeGuard(*mut iio_dev);
> +
> +impl DirectModeGuard {
> +    fn new(indio_dev: *mut iio_dev) -> Result<Self> {
> +        // SAFETY: `indio_dev` is a valid pointer to a fully initialized `iio_dev`
> +        // allocated by `iio_device_alloc`. `iio_device_claim_direct` returns `true`
> +        // if the device is in direct mode (success), `false` if buffer mode is active.
> +        let claimed = unsafe { crate::bindings::iio_device_claim_direct(indio_dev) };
> +        if claimed {
> +            Ok(Self(indio_dev))
> +        } else {
> +            Err(EBUSY)
> +        }
> +    }
> +}
> +
> +impl Drop for DirectModeGuard {
> +    fn drop(&mut self) {
> +        // SAFETY: `self.0` was successfully claimed in `new()`. Releasing it
> +        // unlocks the IIO mode lock acquired during claim.
> +        unsafe {
> +            crate::bindings::iio_device_release_direct(self.0);
> +        }
> +    }
> +}
> +
> +// ---------------------------------------------------------------------------
> +// read_raw_callback — C-to-Rust FFI trampoline
> +// ---------------------------------------------------------------------------
> +
> +/// C-compatible trampoline for the `iio_info.read_raw` callback.
> +///
> +/// # Safety
> +///
> +/// This function is only called by the IIO core with valid pointers:
> +/// - `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` and `val2` are valid pointers for writing the result.
> +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,
> +    mask: 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 };
> +
> +    // Claim direct mode via RAII guard. If the device is in buffer mode,
> +    // return -EBUSY to userspace immediately.

We only need to do this if the particular device the driver is supporting
needs for the specific operation to ensure that that no accesses occur to the
device.   We should not do this for reading stuff back that is cached in the
driver for instance.  In many cases devices have interfaces where absolutely
anything is safe in buffer modes.

This is unfortunately going to be hard to do other than in specific drivers
that know those rules. + doesn't belong here at all yet as a rust driver
can't get into a state where this fails and should never be relying on this
for it's own sychronization (e.g. between concurrent calls of read_raw()).

> +    let _guard = match DirectModeGuard::new(indio_dev) {
> +        Ok(g) => g,
> +        Err(e) => return e.to_errno(),
> +    };
> +
> +    match driver.read_raw(chan, mask) {
> +        Ok(IioVal::Int(v)) => {
> +            // SAFETY: `val` is a valid pointer provided by the IIO core.
> +            unsafe {
> +                *val = v;
> +            }
> +            IIO_VAL_INT
> +        }
> +        Ok(IioVal::Fractional(v, v2)) => {
> +            // SAFETY: `val` and `val2` are valid pointers provided by the IIO core.
> +            unsafe {
> +                *val = v;
> +                *val2 = v2.get();

why .get() for this one.

> +            }
> +            IIO_VAL_FRACTIONAL
> +        }
> +        Ok(IioVal::IntPlusMicro(v, v2)) => {
> +            // SAFETY: `val` and `val2` are valid pointers provided by the IIO core.
> +            unsafe {
> +                *val = v;
> +                *val2 = v2;
> +            }
> +            IIO_VAL_INT_PLUS_MICRO
> +        }
> +        Ok(IioVal::IntPlusNano(v, v2)) => {
> +            // SAFETY: `val` and `val2` are valid pointers provided by the IIO core.
> +            unsafe {
> +                *val = v;
> +                *val2 = v2;
> +            }
> +            IIO_VAL_INT_PLUS_NANO
> +        }
> +        Err(e) => e.to_errno(),
> +    }
> +}

> +
> +impl<T: IioDriver> Device<T> {

> +    /// 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,
> +        init: impl PinInit<T, E>,
> +    ) -> Result<Self>
> +    where
> +        Error: From<E>,
> +    {
> +        let priv_size = size_of::<T>();
> +
> +        // 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 as i32) };
> +        if indio_dev.is_null() {
> +            return Err(ENOMEM);
> +        }
> +
> +        // SAFETY: `indio_dev` is valid and freshly allocated. `priv_` points
> +        // to uninitialized memory of `sizeof(T)` bytes. `PinInit::__pinned_init`

The data accessed in c by iio_priv() is zeroed, not uninitialized but
I'm not sure that's what you are referring to.

> +        // initializes `priv_` in place without reading the previous
> +        // (uninitialized) 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()` returns a reference to data owned by `T` in `priv_`,
> +        //   which remains at a fixed address because `priv_` is heap-allocated
> +        //   inside `iio_dev`.

Can channels be static const?  It is in most IIO drivers written in C.

> +        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 = INDIO_DIRECT_MODE as i32;
> +        }
> +
> +        Ok(Self {
> +            indio_dev,
> +            registered: false,
> +            _p: PhantomData,
> +        })
> +    }
> +

> +}

Re: [RFC PATCH v3 3/4] iio: position: add Rust driver for ams AS5600

[Brandon Saint-John]

On Sun, 24 May 2026 20:28:22 +0700 Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> +//! Driver for ams AS5600 12-bit magnetic rotary position sensor.
> +//!
> +//! Datasheet: https://ams.com/documents/20143/36005/AS5600_DS000365_5-00.pdf

Small nitpick, but going to the above link doesn't resolve to a pdf.
The link that works as of today for me is:

https://look.ams-osram.com/m/7059eac7531a86fd/original/AS5600-DS000365.pdf

> +fn err_enodata() -> Error {
> +    Error::from_errno(-(ENODATA as i32))
> +}

Later on it's probably better to add ENODATA to kernel::error::code instead of the helper.

> +#[derive(Clone, Copy)]
> +struct As5600Io(*mut i2c_client);
> +

You can replace the *mut i2c_client with an ARef<I2cClient>. The
kernel::impl_device_context_into_aref! macro is run on &I2cClient<Core>
so you can call ARef::from on dev instead of casting it to the raw pointer
to hold it.

Then it saves you from repeating a few different parts, like redoing unsafe impls,
recasting back to I2cClient<Core> in try_readN, etc.

> +impl IoCapable<u8> for As5600Io {}
> +impl IoCapable<u16> for As5600Io {}

None of the read_u16 or IoCapable<u16> are used at this point, so those traits/methods
could be dropped.

As a side note, in the most recent rust-next branch, there are a few changes with IoCapable
so maybe worth rebasing at some point to get those changes. IoCapable isn't a marker trait
anymore so I get compile errors trying to rebase there.

> +#[pin_data]
> +struct As5600Priv<T> {
> +    #[pin]
> +    io_lock: Mutex<As5600HwState<T>>,
> +    channels: KBox<[iio_chan_spec; 1]>,
> +}
> +
> +/// Encapsulates the I/O interface and its runtime health state.
> +///
> +/// This prevents operations on a known-dead bus (Circuit Breaker pattern).
> +struct As5600HwState<T> {
> +    io: T,
> +    state: DeviceState,
> +}

Could As5600Priv/HwState hold the As5600Io directly instead of a generic T?
As5600Priv was not generic over T in v2, and since As5600Io implements Io/IoCapable
and the sensor only uses the I2C bus it doesn't seem like it needs to be generic,
at least at the moment.

> +impl<T: Io + IoCapable<u8> + Unpin> IioDriver for As5600Priv<T> {
> +    fn read_raw(&self, _chan: *const iio_chan_spec, mask: isize) -> Result<IioVal> {
> +        match mask {
> +            // IIO_CHAN_INFO_RAW — read the 12-bit raw angle value.
> +            m if m == iio_chan_info_enum_IIO_CHAN_INFO_RAW as isize => {

Ideally in the future, the iio_chan_info_enum_* variants can be wrapped in an Rust
enum with an #[repr] attribute. At this stage, match isn't as useful as it could be
since you still need to call "if m == ...".

Sent using hkml (https://github.com/sjp38/hackermail)

Re: [RFC PATCH v3 1/4] i2c: rust: implement kernel::io::Io trait for I2cClient

[Muchamad Coirul Anwar]

On Thu, 28 May 2026 16:25:57 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> My main concern is this currently takes away the clarity off smbus
> naming and replaces it with the impression this is how i2c reads and writes
> are done in general.  How will this support other forms of access?
>
> How do we have lots of different types of i2c supported?  Simplest being
> the ones regmap supports today. There are 7ish in drivers/base/regmap-i2c.c
>
> Or if the plan is to only support register style interfaces why not only
> allow for use of regmap?

Some context on how we got here: in v2 I added standalone SMBus methods
on I2cClient (smbus_read_byte_data, etc). Igor reviewed it [1] and
pointed out that the agreed direction is for I2cClient to implement
the generic Io trait [2] from Zhi Wang's driver-core-testing work.
That discussion happened during Igor's own i2c-adapter series. I
offered to take it on, and he confirmed bundling it in my series was
fine.

I take your point about losing the SMBus naming clarity. The underlying
calls are still `i2c_smbus_read_byte_data` and friends though, the Io
trait just provides a uniform interface on top with bounds checking via
`io_addr()`. The call sites in the driver use `try_read8`/`try_read16`
which map 1:1 to the smbus byte/word operations.

For other I2C access types (block, raw msg, etc), those would need
additional trait methods or a separate abstraction. Rust regmap bindings
don't exist yet, so this is the available path for now. Once regmap
lands in Rust, drivers that fit the regmap model can use that instead.

[1] https://lore.kernel.org/rust-for-linux/20260131-i2c-adapter-v1-4-5a436e34cd1a@gmail.com/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/commit/?h=driver-core-testing&id=121d87b28e1d9061d3aaa156c43a627d3cb5e620

> One other comment inline. I'm seeing what looks to be a check for an
> 8 bit address whereas smbus is 7 bit addressing.

> Except smbus standard addressing is 7 bit.
> Need space for the read / write bit.

To clarify, `maxsize=256` here refers to the SMBus command byte
(register address)
range, not the device address. The command byte is a full 8 bits
(0x00..0xFF = 256 values). The 7-bit device address plus R/W bit is
handled at the adapter level by the I2C core when the client is
instantiated; it's not part of the register access path that Io wraps.

The `io_addr()` bounds check ensures `offset + sizeof(access) <= 256`,
i.e. the register address stays within the valid command byte range.
I'll add a comment in the code clarifying this distinction since the
naming is confusing without it.

> Unrelated change.

Dropping the trailing-period fix from this patch in v4.

Thanks,
Coirul

Re: [RFC PATCH v3 4/4] iio: position: as5600: add Kconfig and Makefile entries

[Muchamad Coirul Anwar]

On Thu, 28 May 2026 17:09:00 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> Combine with the driver patch.  Separate patches for build files
> provide no value.

Done. v4 will be a 3-patch series with Kconfig and Makefile included in
the driver patch.

Thanks,
Coirul

Re: [RFC PATCH v3 2/4] rust: add minimal IIO subsystem abstractions

[Muchamad Coirul Anwar]

On Thu, 28 May 2026 17:09:00 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> You'll need a lot more stuff before it makes any sense to be messing
> with claims on the state.  They only come in once drivers are dealing
> with buffers etc. I'd leave them until then.

> As above with the level of bindings you are doing so far, neither of these
> should ever be called.  They are not for use by drivers simply wanting
> to serialize things but about controlling the ability of the IIO core to
> change the fundamental data flow from simple polling to streaming data to
> user space accessible kfifos.

Understood. I was conflating "serialize concurrent sysfs reads" with
"prevent mode transitions", which are two different things. The driver's
Mutex handles the former; DirectModeGuard is for the latter and has no
business being here until buffer support exists.

Dropping DirectModeGuard entirely from v4: struct, C helpers, and the
trampoline call. Will bring it back when adding buffer/trigger support,
where it belongs in the individual driver rather than the generic
trampoline.

> Sort of. It ensures the state doesn't change.  Given buffer/trigger
> operations only occur when in a buffer state it sort of prevents their
> operations. That's not including stuff like setting up buffers though -
> just the data streaming.
>
> Anyhow, kick it in the long grass (drop for now) given you don't support
> buffer state. Good to see what this probably looks like though.

Noted. Good to have the shape sketched out at least.

> We only need to do this if the particular device the driver is supporting
> needs for the specific operation to ensure that no accesses occur to
> the device. We should not do this for reading stuff back that is cached in the
> driver for instance.  In many cases devices have interfaces where absolutely
> anything is safe in buffer modes.
>
> This is unfortunately going to be hard to do other than in specific drivers
> that knows those rules. + doesn't belong here at all yet as a rust driver
> can't get into a state where this fails and should never be relying on this
> for its own synchronization (e.g. between concurrent calls of read_raw()).

Right. The trampoline shouldn't be making policy decisions about when to
claim direct mode, that's driver-specific knowledge. Removing from the
generic abstraction.

> why .get() for this one.

I used .get() because NonZeroI32 is a wrapper type, meaning we have to
explicitly extract the
inner i32. Since the other variants use a plain i32, they don't need
it. I'll add a quick comment in
v4 to clarify this.

> The data accessed in c by iio_priv() is zeroed, not uninitialized but
> I'm not sure that's what you are referring to.

You're right, `iio_device_alloc` uses `kzalloc`, so `priv_` is zeroed.
The SAFETY comment was wrong. Fixing in v4 to say "zeroed memory" and
note that PinInit overwrites it with the initialized state.

> Can channels be static const?  It is in most IIO drivers written in C.

Yes. Moving it to a module-level static in v4. That removes the KBox
allocation and the lifetime question around `indio_dev->channels`
outliving `priv_`.

Thanks,
Coirul

Re: [RFC PATCH v3 1/4] i2c: rust: implement kernel::io::Io trait for I2cClient

[Jonathan Cameron]

+CC linux-i2c and Wolfram - make sure to keep them on future versions
of this patch.

On Mon, 1 Jun 2026 14:58:45 +0700
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> On Thu, 28 May 2026 16:25:57 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > My main concern is this currently takes away the clarity off smbus
> > naming and replaces it with the impression this is how i2c reads and writes
> > are done in general.  How will this support other forms of access?
> >
> > How do we have lots of different types of i2c supported?  Simplest being
> > the ones regmap supports today. There are 7ish in drivers/base/regmap-i2c.c
> >
> > Or if the plan is to only support register style interfaces why not only
> > allow for use of regmap?  
> 
> Some context on how we got here: in v2 I added standalone SMBus methods
> on I2cClient (smbus_read_byte_data, etc). Igor reviewed it [1] and
> pointed out that the agreed direction is for I2cClient to implement
> the generic Io trait [2] from Zhi Wang's driver-core-testing work.
> That discussion happened during Igor's own i2c-adapter series. I
> offered to take it on, and he confirmed bundling it in my series was
> fine.
> 
> I take your point about losing the SMBus naming clarity. The underlying
> calls are still `i2c_smbus_read_byte_data` and friends though, the Io
> trait just provides a uniform interface on top with bounds checking via
> `io_addr()`. The call sites in the driver use `try_read8`/`try_read16`
> which map 1:1 to the smbus byte/word operations.

The reason we need Wolfram and I2C folk in the discussion here is
the 'richness' of how the I2C spec is used.

I'll go a little further.  If this was renamed to make it the rust smbus
binding them I wouldn't be as bothered by this. For something claiming to
be I2C this is a misleading interface and I am very much against it.
Doing this is a path to a lot of confusion and problems for extensibility.

Maybe less than 50% of I2C devices use smbus calls (or at least in IIO,
it might be more standard elsewhere). That is partly because almost no
one uses that naming on datasheets so not everyone notices that they
can use them - this is also the reason the byte swapped variant is very
common - if you look at the I2C spec and implement auto increment on
addressing, then the byte order is a random choice.  A substantial
set of devices use a register style interface but due to subtle
protocol differences cannot use smbus.

Also perhaps relevant to this: If you'd submitted the driver in this
series as a c driver, you would have been asked the question: "why aren't
you using regmap".  It brings a rich set of helpers, standarized caching
etc.  The answer that applies here of the regmap binding isn't ready
isn't a great way to answer that question!

> 
> For other I2C access types (block, raw msg, etc), those would need
> additional trait methods or a separate abstraction.

Just sticking to byte and word reads, see the c. 7 different options in
regmap.  Given this is the I2C binding (not Smbus) one you've picked one
random choice from that set.  Also note there are other custom i2c regmap
implementations in drivers to cover the long tail of 'other' ways of doing
register access.

> Rust regmap bindings
> don't exist yet, so this is the available path for now. Once regmap
> lands in Rust, drivers that fit the regmap model can use that instead.

Understood that there is more to do, but given the regmap already encapsulates
the smbus support you have here, I'd be much more in favour of the focus
going on getting that done.  We will need i2c bindings as well but that
will be for the many devices that aren't register based etc for which this
trait approach is wrong.  I would almost suggest not merging a non regmap
interface for what you cover here, except we do get annoying corner cases
where the device uses a mixture of smbus like commands and non smbus so there
probably will need to be support at the i2c / smbus level.

> 
> [1] https://lore.kernel.org/rust-for-linux/20260131-i2c-adapter-v1-4-5a436e34cd1a@gmail.com/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/commit/?h=driver-core-testing&id=121d87b28e1d9061d3aaa156c43a627d3cb5e620
> 
> > One other comment inline. I'm seeing what looks to be a check for an
> > 8 bit address whereas smbus is 7 bit addressing.  
> 
> > Except smbus standard addressing is 7 bit.
> > Need space for the read / write bit.  
> 
> To clarify, `maxsize=256` here refers to the SMBus command byte
> (register address)
> range, not the device address. The command byte is a full 8 bits
> (0x00..0xFF = 256 values). 

I understood that - but to me it seems to be irrelevant.

> The 7-bit device address plus R/W bit is
> handled at the adapter level by the I2C core when the client is
> instantiated; it's not part of the register access path that Io wraps.

Agreed.

> 
> The `io_addr()` bounds check ensures `offset + sizeof(access) <= 256`,
> i.e. the register address stays within the valid command byte range.
> I'll add a comment in the code clarifying this distinction since the
> naming is confusing without it.

I'm still lost. The address must <= 128 to fit in the available 7 bits.
Why would you let it be bigger than that? It's going in a 7 bit field
not the byte that contains that field.

Is this separating a safety argument from a bug check?  If so why
not just use the tighter one?


> 
> > Unrelated change.  
> 
> Dropping the trailing-period fix from this patch in v4.
> 
> Thanks,
> Coirul

Re: [RFC PATCH v3 2/4] rust: add minimal IIO subsystem abstractions

[Jonathan Cameron]

On Mon, 1 Jun 2026 15:30:40 +0700
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> On Thu, 28 May 2026 17:09:00 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
Hi Coirul,

A small kernel review process thing.  You need to keep more context.
A reader shouldn't need to go open previous email just to find
out what code we are talking about. Take a look at other review
discussions on the mailing lists you are sending this to.
(though oddly this pattern has become a common thing in last week or
 so - hence I'm sending this comment a lot!)



> 
> > why .get() for this one.  
> 
> I used .get() because NonZeroI32 is a wrapper type, meaning we have to
> explicitly extract the
> inner i32. Since the other variants use a plain i32, they don't need
> it. I'll add a quick comment in
> v4 to clarify this.

Ah no need. That was my lack of rust knowledge.  You'll be helping
reviewers understand this stuff for a while!

Re: [RFC PATCH v3 3/4] iio: position: add Rust driver for ams AS5600

[Muchamad Coirul Anwar]

On Thu, 28 May 2026 17:08:00 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> > Features:
> > - Circuit breaker pattern for I/O error resilience
> I'm not sure why this is particularly useful, but maybe I'm missing something.

> I mention this below - it works (sort of) because right now you just
> do reads with no state changes.  In general device recovery is way
> more complex than what you do.

Fair enough. It's over-engineered for what this driver does. Simple -EIO
propagation is enough since the sensor is read-only and the Mutex
already limits bus access to one reader at a time. I'll remove the circuit
breaker in v4. The read_raw path will just propagate errors from
try_read8/try_read16 directly.

> I think we need a separate discussion on how to do device recovery.
> It is at best a black art and very device specific if bus corruption occurred.

> On a read you can maybe get away with this but on a write we have no
> idea if it wrote or not and that write may or may not have had side
> effects. If you lose a write, the only always correct thing to do is
> a device reset. Other stuff is very device specific.

Agreed. Since this driver is read-only today, the whole mechanism is
solving a problem that doesn't really exist here. If device recovery
patterns come up as a broader R4L discussion topic, happy to participate.

> > +                let angle_h = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_H as usize) {
> > +                    Ok(v) => v as u16,
> > +                    Err(_) => return Err(hw_guard.handle_io_error()),
> > +                };
> > +                let angle_l = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_L as usize) {
> > +                    Ok(v) => v as u16,
> > +                    Err(_) => return Err(hw_guard.handle_io_error()),
> > +                };
> > +
> > +                let angle = (angle_h << 8 | angle_l) & 0x0FFF;
>
> This is the sort of thing we'd never do in a C driver because we have well
> defined meaningful functions / macros for doing this.  I'd like to see
> something equivalent in the rust code of.
>    Bulk read int a two byte array. i2c_smbus_read_word_data() or swapped variant.
>    Unaligned endian read  get_unaligned_be16()
>    Masking to extract the 12 bits that are valid. FIELD_GET() whatever.

Switching to `try_read16()` (calls `i2c_smbus_read_word_data`) plus
`swap_bytes()` for the byte order, then mask:

    const AS5600_RAW_ANGLE_MASK: u16 = 0x0FFF;

    let raw = client.try_read16(AS5600_REG_RAW_ANGLE_H as usize)?;
    let angle = raw.swap_bytes() & AS5600_RAW_ANGLE_MASK;

Rust doesn't have FIELD_GET yet, but a named constant serves the same
documentation purpose. Single call, no manual byte assembly.

> > +                // Read the 12-bit angle as two bytes. The AS5600 hardware
> > +                // freezes the internal angle value on reading the high byte
> > +                // until the low byte is read
>
> Would be very unusual if the device does this but doesn't support some form
> of larger read. Would be nice to be able to do that in a rust driver.

Yes, the AS5600 supports word reads over SMBus. I was being overly
cautious with the byte-at-a-time approach based on the datasheet's
latch mechanism description, but a word read achieves the same atomicity
from the hardware side. Using it in v4.

> > +        channels_alloc[0].info_mask_separate = (1 << iio_chan_info_enum_IIO_CHAN_INFO_RAW)
> > +            | (1 << iio_chan_info_enum_IIO_CHAN_INFO_SCALE);
>
> I believe there is a kernel rust equivalent of BIT(). Can you use that here.

This should use `kernel::bits::bit()` for consistency.
The status register check already uses `bit_u8(5)` but the channel
setup didn't follow the same pattern. I'll fix this in v4.

> > +        dev_info!(dev.as_ref(), "AS5600 magnetic position sensor ready\n");
>
> Too noisy. dev_dbg() at most.

I will change this to dev_dbg!().

Thanks,
Coirul

Re: [RFC PATCH v3 3/4] iio: position: add Rust driver for ams AS5600

[Muchamad Coirul Anwar]

On Fri, 29 May 2026 05:37:00 +0000
Brandon Saint-John <branstj@gmail.com> wrote:

> > +//! Datasheet: https://ams.com/documents/20143/36005/AS5600_DS000365_5-00.pdf
>
> Small nitpick, but going to the above link doesn't resolve to a pdf.
> The link that works as of today for me is:
> https://look.ams-osram.com/m/7059eac7531a86fd/original/AS5600-DS000365.pdf

I'll update in v4. Looks like ams reorganized under the ams-osram.com domain.

> > +fn err_enodata() -> Error {
> > +    Error::from_errno(-(ENODATA as i32))
> > +}
>
> Later on it's probably better to add ENODATA to kernel::error::code
> instead of the helper.

Agreed, the `err_enodata()` helper is a workaround. Will check if
ENODATA can be added to kernel::error::code directly for v4, or at
minimum leave a TODO comment.

> > +#[derive(Clone, Copy)]
> > +struct As5600Io(*mut i2c_client);
>
> You can replace the *mut i2c_client with an ARef<I2cClient>. The
> kernel::impl_device_context_into_aref! macro is run on &I2cClient<Core>
> so you can call ARef::from on dev instead of casting it to the raw pointer
> to hold it.
>
> Then it saves you from repeating a few different parts, like redoing unsafe
> impls, recasting back to I2cClient<Core> in try_readN, etc.

Good suggestion. I'll restructure around `ARef<I2cClient>` in v4, it
eliminates the As5600Io wrapper, the manual Send/Sync impls, and the
unsafe casts entirely.

> > +impl IoCapable<u16> for As5600Io {}
>
> None of the read_u16 or IoCapable<u16> are used at this point, so those
> traits/methods could be dropped.

Per Jonathan's feedback, I'm switching to a word read for the angle
register, so `try_read16` will actually be used in v4. But the As5600Io
wrapper (and its IoCapable impls) goes away regardless since I'm moving
to `ARef<I2cClient>` which already has the Io impl from patch 1.

> As a side note, in the most recent rust-next branch, there are a few changes
> with IoCapable so maybe worth rebasing at some point to get those changes.
> IoCapable isn't a marker trait anymore so I get compile errors trying to
> rebase there.

I checked and you're right. `IoCapable` changed upstream, the empty
marker impls in patch 1 won't compile against rust-next anymore.
Will provide proper implementations for I2cClient in v4.

> > +#[pin_data]
> > +struct As5600Priv<T> {
> > +    #[pin]
> > +    io_lock: Mutex<As5600HwState<T>>,
> > +    channels: KBox<[iio_chan_spec; 1]>,
> > +}
>
> Could As5600Priv/HwState hold the As5600Io directly instead of a generic T?
> As5600Priv was not generic over T in v2, and since As5600Io implements
> Io/IoCapable and the sensor only uses the I2C bus it doesn't seem like it
> needs to be generic, at least at the moment.

I will make this change in v4. I agree that simplifying the struct to
use a concrete
type is the better approach here.With ARef<I2cClient> replacing
As5600Io, the struct
becomes As5600Priv { io_lock: Mutex<As5600HwState>, ... } with no type
parameter."

> > +            m if m == iio_chan_info_enum_IIO_CHAN_INFO_RAW as isize => {
>
> Ideally in the future, the iio_chan_info_enum_* variants can be wrapped in
> an Rust enum with an #[repr] attribute. At this stage, match isn't as
> useful as it could be since you still need to call "if m == ...".

Makes sense as future work for the IIO abstraction. Would make the match
exhaustive and eliminate the catch-all arm.

Thanks,
Coirul

Re: [RFC PATCH v3 1/4] i2c: rust: implement kernel::io::Io trait for I2cClient

[Muchamad Coirul Anwar]

On Mon, 1 Jun 2026 10:05:00 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> +CC linux-i2c and Wolfram - make sure to keep them on future versions
> of this patch.

Noted.

> If this was renamed to make it the rust smbus binding then I wouldn't
> be as bothered by this. For something claiming to be I2C this is a
> misleading interface and I am very much against it.

Agreed. I'll rename it to make it clearly SMBus-scoped in v4.

> Understood that there is more to do, but given the regmap already
> encapsulates the smbus support you have here, I'd be much more in
> favour of the focus going on getting that done.
>
> I would almost suggest not merging a non regmap interface for what you
> cover here, except we do get annoying corner cases where the device
> uses a mixture of smbus like commands and non smbus so there probably
> will need to be support at the i2c / smbus level.

Makes sense. Will defer to Wolfram and the i2c folks on whether this
should wait for regmap or land as a clearly-scoped SMBus patch.


> Is this separating a safety argument from a bug check? If so why
> not just use the tighter one?

> +    fn maxsize(&self) -> usize {
> +        256
> +    }

The try_read8 here is a general SMBus wrapper, not AS5600-specific.
The u8 command (register address) applies to any SMBus device, so
8-bit is the protocol max. If we tighten below that, it breaks devices
with registers at 0x80 and above. The 7-bit limit applies to the
device address, not this command byte.

Thanks,
Coirul

Re: [RFC PATCH v3 1/4] i2c: rust: implement kernel::io::Io trait for I2cClient

[Jonathan Cameron]

On Tue, 2 Jun 2026 15:11:12 +0700
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> On Mon, 1 Jun 2026 10:05:00 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > +CC linux-i2c and Wolfram - make sure to keep them on future versions
> > of this patch.  
> 
> Noted.
> 
> > If this was renamed to make it the rust smbus binding then I wouldn't
> > be as bothered by this. For something claiming to be I2C this is a
> > misleading interface and I am very much against it.  
> 
> Agreed. I'll rename it to make it clearly SMBus-scoped in v4.
> 
> > Understood that there is more to do, but given the regmap already
> > encapsulates the smbus support you have here, I'd be much more in
> > favour of the focus going on getting that done.
> >
> > I would almost suggest not merging a non regmap interface for what you
> > cover here, except we do get annoying corner cases where the device
> > uses a mixture of smbus like commands and non smbus so there probably
> > will need to be support at the i2c / smbus level.  
> 
> Makes sense. Will defer to Wolfram and the i2c folks on whether this
> should wait for regmap or land as a clearly-scoped SMBus patch.
> 
> 
> > Is this separating a safety argument from a bug check? If so why
> > not just use the tighter one?  
> 
> > +    fn maxsize(&self) -> usize {
> > +        256
> > +    }  
> 
> The try_read8 here is a general SMBus wrapper, not AS5600-specific.
> The u8 command (register address) applies to any SMBus device, so
> 8-bit is the protocol max. If we tighten below that, it breaks devices
> with registers at 0x80 and above. The 7-bit limit applies to the
> device address, not this command byte.

Ah.. Sorry, I was being stupid and had forgotten the meaning
of the address byte (which device + that magic r/w bit which
is oddly in that byte).  I even read wrong section of the smbus
protocol description as it has both read byte and read byte data
only the second of which takes a command byte.

I think I got thrown by the generic naming of try_read8 that
doesn't capture that distinction but given it comes from the trait
I guess we can't do much about that.

Jonathan

> 
> Thanks,
> Coirul

Re: [RFC PATCH v3 3/4] iio: position: add Rust driver for ams AS5600

[Jonathan Cameron]

> > > +                let angle_h = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_H as usize) {
> > > +                    Ok(v) => v as u16,
> > > +                    Err(_) => return Err(hw_guard.handle_io_error()),
> > > +                };
> > > +                let angle_l = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_L as usize) {
> > > +                    Ok(v) => v as u16,
> > > +                    Err(_) => return Err(hw_guard.handle_io_error()),
> > > +                };
> > > +
> > > +                let angle = (angle_h << 8 | angle_l) & 0x0FFF;  
> >
> > This is the sort of thing we'd never do in a C driver because we have well
> > defined meaningful functions / macros for doing this.  I'd like to see
> > something equivalent in the rust code of.
> >    Bulk read int a two byte array. i2c_smbus_read_word_data() or swapped variant.
> >    Unaligned endian read  get_unaligned_be16()
> >    Masking to extract the 12 bits that are valid. FIELD_GET() whatever.  
> 
> Switching to `try_read16()` (calls `i2c_smbus_read_word_data`) plus
> `swap_bytes()` for the byte order, then mask:
> 
>     const AS5600_RAW_ANGLE_MASK: u16 = 0x0FFF;
> 
>     let raw = client.try_read16(AS5600_REG_RAW_ANGLE_H as usize)?;
>     let angle = raw.swap_bytes() & AS5600_RAW_ANGLE_MASK;

That swap goes back to a pattern we ripped out of the C code years ago
and why we have the smbus swapped functions and regmap support fort htat.

If a given part always does the bytes in opposite byte order of smbus
then it should be handled as part of the read function rather than every
word read having to be followed by a swap.  Here you only have one so
it doesn't look that bad, but for some other devices this is the common
call sequence to ready almost anything.

> 
> Rust doesn't have FIELD_GET yet, but a named constant serves the same
> documentation purpose. Single call, no manual byte assembly.

A named constant serves only part of the purpose. The main gain from FIELD_GET()
is we don't have to go check if a shift is also needed.  I'd strongly
support work on getting something similar for rust as it makes for a lot
more consistent and readable driver code.

Basically I want all the useful helper stuff we've built up in C to be
available in Rust.  In cases like this one I would prefer there was never
a legacy of doing it any other way!

Jonathan

Re: [RFC PATCH v3 3/4] iio: position: add Rust driver for ams AS5600

[Muchamad Coirul Anwar]

On Mon, 2 Jun 2026 13:06:00 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> > > +                let angle = (angle_h << 8 | angle_l) & 0x0FFF;
> >
> > Switching to `try_read16()` (calls `i2c_smbus_read_word_data`) plus
> > `swap_bytes()` for the byte order, then mask:
> >
> >     const AS5600_RAW_ANGLE_MASK: u16 = 0x0FFF;
> >
> >     let raw = client.try_read16(AS5600_REG_RAW_ANGLE_H as usize)?;
> >     let angle = raw.swap_bytes() & AS5600_RAW_ANGLE_MASK;
>
> That swap goes back to a pattern we ripped out of the C code years ago
> and why we have the smbus swapped functions and regmap support for that.
> If a given part always does the bytes in opposite byte order of smbus
> then it should be handled as part of the read function rather than every
> word read having to be followed by a swap.

Understood. I'll use i2c_smbus_read_word_swapped or a Rust wrapper
around it, so the byte order is handled at the transport level.

> > Rust doesn't have FIELD_GET yet, but a named constant serves the same
> > documentation purpose. Single call, no manual byte assembly.
>
> A named constant serves only part of the purpose. The main gain from
> FIELD_GET() is we don't have to go check if a shift is also needed.
> I'd strongly support work on getting something similar for rust as it
> makes for a lot more consistent and readable driver code.
>
> Basically I want all the useful helper stuff we've built up in C to be
> available in Rust. In cases like this one I would prefer there was never
> a legacy of doing it any other way!

Agreed, I'll look into what exists for a Rust FIELD_GET equivalent.

Thanks,
Coirul