Re: [RFC PATCH v5 1/3] i2c: rust: implement SMBus access via IoBackend and FallibleIoCapable

Hi Jonathan,

Thanks for the detailed review and for CC-ing Mark.

On Mon, 24 Aug 2026 at 06:41, Jonathan Cameron <[email protected]> wrote:

> > I2cClient<Bound>::smbus_io() returns an I2cView handle for use with the
> > generic try_read8/try_read16 methods. Two standalone methods are also
> > provided for odd-offset word access that bypasses the alignment check
> > in the Io trait:
>
> Given some devices implement auto address increment and others decrement
> even in aligned byte pairs it seems you will see things that 'smell' like
> they are unaligned.
>
> I'd forgotten this fun corner of smbus like i2c devices!

The standalone smbus_read_word() and smbus_read_word_swapped()
methods exist for odd-offset access, but I didn't document that
try_read16 and try_write16 reject odd offsets and that auto-increment
devices may need to use them. I'll add that to smbus_io()'s doc comment
in v6.


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

Agreed. regmap-rs is the correct long-term path. This series uses the
IoBackend/SMBus route because regmap-rs does not yet exist, and the
scope is intentionally minimal: byte and word reads only. I should have
stated this in the cover letter from the start. I'll fix that in v6.

Mark, if you have a view on whether this stepping-stone approach is
acceptable or the series should wait, I'd appreciate your view.


> There are mixed devices where register stuff is used alongside other accesses,
> however for those I'm not sure it is worth doing anything other than
> wrapping the raw bus access functions.
>
> A few more references to the real variations we have to cope with inline.

Agreed. smbus_read_word() and smbus_read_word_swapped() on
I2cClient<Bound> are exactly that: thin wrappers over the raw bus
functions. For mixed devices that don't fit the register model, those
standalone methods are the intended path, not the IoBackend machinery.


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

> > +impl FallibleIoCapable<u16> for I2cBackend {
> > +    #[inline]
> > +    fn io_try_read<'a>(view: I2cView<'a, u16>) -> Result<u16> {
> > +        // `io_view()` ensures `offset + 2 <= 256`, so `addr()` is at most 254;
> > +        // the `as u8` cast below is therefore lossless.
> > +        let reg = Self::as_ptr(view).addr() as u8;
> > +        // SAFETY: `view.client.as_raw()` returns a valid `*mut struct i2c_client`
> > +        // pointer as guaranteed by the type invariant of `I2cClient`.
> > +        // `i2c_smbus_read_word_data` is safe to call with any valid client pointer
> > +        // and any u8 command byte.
> > +        let ret = unsafe { bindings::i2c_smbus_read_word_data(view.client.as_raw(), reg) };
> > +        if ret < 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            Ok(ret as u16)
> > +        }
> > +    }
> > +
> > +    #[inline]
> > +    fn io_try_write<'a>(view: I2cView<'a, u16>, value: u16) -> Result {
> > +        // `io_view()` ensures `offset + 2 <= 256`, so `addr()` is at most 254;
>
> There are smbus devices that have fully 2 byte registers.  For those you'd
> need to divide this by 2 and the range would go up to 510
> E.g. drivers/light/cm32181.c (though that doesn't have that many registers).

Understood. The current maxsize=256 works for AS5600 but doesn't cover
devices where all 256 command bytes address 16-bit registers; for those
the range would need to go up to 510. I'll note this as a limitation
in v6.


> > +impl I2cClient<device::Bound> {
> > +    /// Returns an I/O handle for SMBus register access on this I2C client.
> > +    ///
> > +    /// The returned handle provides fallible read/write methods for the
> > +    /// 256-byte SMBus command address space (0x00–0xFF). This is the SMBus
> > +    /// command byte range, NOT the 7-bit device address, which is handled
> > +    /// by the I2C core at the adapter level.
>
> That rather feels like you are correcting my confusion in the earlier version!
> I'd assume people are more awake than me and skip the NOT part ;)

Sorry for the tone. I didn't mean to correct you. I'll drop that line.


> > +    /// Reads a 16-bit word from an SMBus register in CPU-native byte order.
> > +    ///
> > +    /// Wraps `i2c_smbus_read_word_data`. The `reg` parameter is the SMBus
> > +    /// command byte (0x00–0xFF) — an instruction sent to the device over the
> > +    /// serial bus, not a memory address. There is no alignment requirement:
> > +    /// any command byte value is valid regardless of whether it is odd or even.
>
> It might be a memory address, could be almost anything.  Maybe 'not necessarily'
> a memory address.
> The kernel docs have it as:
> "Command byte, a data byte which often selects a register on the device"

I'll use the kernel-standard phrasing


> > +    /// Reads a 16-bit word from an SMBus register with bytes unconditionally
> > +    /// swapped.
> > +    ///
> > +    /// Wraps `i2c_smbus_read_word_data` and applies [`u16::swap_bytes`] to the
> > +    /// result. Use this for devices that store multi-byte registers in
> > +    /// big-endian (MSB-first) format, which is common among I2C sensors whose
> > +    /// datasheets do not reference the SMBus specification.
>
> This last bit feels like cover letter, patch description material.
> I wouldn't normally expect function documentation to justify how useful
> a function is!

Fair point. I'll remove it.

> > +    ///
> > +    /// The swap is **unconditional** — it is not equivalent to `be16_to_cpu`.
> > +    /// On a big-endian CPU, `be16_to_cpu` would be a no-op, but this method
> > +    /// still swaps. The reason: SMBus always transmits the low byte first, so
> > +    /// the driver always receives data in little-endian wire order regardless
> > +    /// of CPU endianness. The swap corrects for the device's wire-level byte
> > +    /// order, not the CPU's native order.
>
> This feels like we are justifying why it isn't a different implementation.
> Can we rewrite to not need that reference to what else it isn't.

Agreed. I was over-explaining in the doc. I'll rewrite to describe
only what the function does.

> > +    ///
> > +    /// The `reg` parameter is the SMBus command byte (0x00–0xFF). There is no
> > +    /// alignment requirement; any command byte value is valid.
>
> What would an alignment requirement mean here?

Right, it doesn't apply here. I'll drop that sentence.

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

I'll move FallibleIoCapable to a separate prerequisite patch in v6.

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