Re: [RFC PATCH v4 1/3] i2c: rust: implement SMBus read abstraction via kernel::io::Io for I2cClient

On Mon, 3 Aug 2026 at 07:53, Jonathan Cameron <[email protected]> wrote:

> > Implement the Io trait for I2cClient, providing SMBus byte and word
> > read/write operations with automatic offset validation via io_addr().
> >
> > I2cClient implements the generic Io trait rather than exposing
> > standalone SMBus methods, following the direction established in [1]
> > and [2]. The underlying calls are still i2c_smbus_read_byte_data and
> > i2c_smbus_read_word_data.
> >
> > I2cClient now implements IoCapable<u8> and IoCapable<u16> with
> > maxsize=256 (SMBus command byte range 0x00-0xFF, not the 7-bit
> > device address which is handled by the I2C core at adapter level).

> Just to make sure it doesn't get lost across versions - I'll repeat
> briefly what concerns me here.  I'm also interested to hear Wolfram's
> view on this as well as Wolfram has probably seen more weird
> i2c devices than anyone else.
>
> I2C isn't one nice set of here is how you read a byte and here is how
> you read a u16.  Even smbus effectively has 2 options given the large
> number of devices with the bytes flipped.  Those exist because
> they weren't designed against the smbus spec (very few datasheets
> even mention smbus) but rather the less constrained definitions of
> the i2c spec and under that there is no concept of a word so ordering
> is whatever the hardware fancies (wonderfully we even have devices that
> aren't consistent in ordering register pairs but let's ignore those).

Thanks for the feedback, Jonathan.

That's a fair point on naming. The underlying calls are
i2c_smbus_read_word_data and i2c_smbus_write_word_data, so the
interface really is SMBus, not raw I2C.

On smbus_swapped, AS5600 is actually big-endian and currently I handle
swap_bytes() manually in the driver as a workaround. The reason I
didn't add a swapped variant yet is that in v4, the I2C bindings still
followed the MMIO pattern (IoCapable), which is infallible. Adding
smbus_swapped on top of that felt premature when the foundation itself
wasn't right for a bus that can NACK or timeout.

> So maybe introduce this as smbus as long as we can chase it with smbus_swapped
> for the byte swapped version.  Or don't introduce smbus at all
> because if this limited subset of smbus applies, then maybe just jumping
> directly to regmap is the right way to go.
>
> Calling this i2c and provide these interfaces is to me giving the
> wrong impressions of what it is.

In v5 I switched to FallibleIoCapable, which was proposed by Danilo
specifically for fallible transports like I2C. With that
foundation in place, adding a smbus_swapped backend is straightforward
and I can include it in this version if that's the preferred direction.

Wolfram is on the CC list. He's no longer listed as I2C subsystem
maintainer in MAINTAINERS (that's now Andi Shyti) but I agree his
experience with I2C devices would be valuable here.

On regmap, I noted in the driver comment that regmap-rs with
val_format_endian=Big would handle the endianness more cleanly. That
would be the better path if the Rust bindings exist or are close. Since
they don't yet, I went with the SMBus approach as a practical stepping
stone. Happy to revisit once regmap-rs is available.

Coirul


> > ---
> >  rust/kernel/i2c.rs | 89 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 89 insertions(+)
> >
> > diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> > index 624b971ca8b0..31c7216d0299 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::{
> > @@ -601,3 +602,91 @@ unsafe impl Send for Registration {}
> >  // SAFETY: `Registration` offers no interior mutability (no mutation through &self
> >  // and no mutable access is exposed)
> >  unsafe impl Sync for Registration {}
> > +
> > +impl<Ctx: device::DeviceContext> IoCapable<u8> for I2cClient<Ctx> {
> > +    unsafe fn io_read(&self, address: usize) -> u8 {
> > +        // SAFETY: `self.as_raw()` returns a valid `struct i2c_client` pointer
> > +        // (type invariant). `address` was pre-validated by io_addr() before
> > +        // this function is called (trait contract).
> > +        let ret = unsafe { bindings::i2c_smbus_read_byte_data(self.as_raw(), address as u8) };
> > +
> > +        // NOTE: Error is lost here. This is only called via try_read() which
> > +        // first validates bounds via io_addr(). For I2C, the caller should
> > +        // always use try_read8() which provides proper error handling.
> > +        ret as u8
> > +    }
> > +
> > +    unsafe fn io_write(&self, value: u8, address: usize) {
> > +        // SAFETY: `self.as_raw()` returns a valid `struct i2c_client` pointer.
> > +        // `address` pre-validated by io_addr().
> > +        unsafe { bindings::i2c_smbus_write_byte_data(self.as_raw(), address as u8, value) };
> > +        // NOTE: Return value is ignored. `IoCapable` trait signature does not
> > +        // support error returns. Use with caution.
> > +    }
> > +}
> > +
> > +impl<Ctx: device::DeviceContext> IoCapable<u16> for I2cClient<Ctx> {
> > +    unsafe fn io_read(&self, address: usize) -> u16 {
> > +        // SAFETY: `self.as_raw()` returns a valid `struct i2c_client` pointer.
> > +        // `address` pre-validated by io_addr().
> > +        let ret = unsafe { bindings::i2c_smbus_read_word_data(self.as_raw(), address as u8) };
> > +
> > +        // NOTE: Error is lost here. See u8 implementation note.
> > +        ret as u16
> > +    }
> > +
> > +    unsafe fn io_write(&self, value: u16, address: usize) {
> > +        // SAFETY: `self.as_raw()` returns a valid `struct i2c_client` pointer.
> > +        // `address` pre-validated by io_addr().
> > +        unsafe { bindings::i2c_smbus_write_word_data(self.as_raw(), address as u8, value) };
> > +        // NOTE: Return value is ignored.
> > +    }
> > +}
> > +
> > +impl<Ctx: device::DeviceContext> Io for I2cClient<Ctx> {
> > +    #[inline]
> > +    fn addr(&self) -> usize {
> > +        0
> > +    }
> > +
> > +    /// SMBus command byte range: 0x00-0xFF (256 possible register addresses).
> > +    /// This is NOT the 7-bit device address; that is handled by the I2C core.
> > +    #[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)
> > +        }
> > +    }
> > +}
>