* Re: [RFC PATCH v3 1/4] i2c: rust: implement kernel::io::Io trait for I2cClient
[not found] ` <CAO26r3QZyVco2aZRqPSyWr3d-SBAVEknkaeNDM4g5DFCr3-2aw@mail.gmail.com>
@ 2026-06-01 9:05 ` Jonathan Cameron
2026-06-02 8:11 ` Muchamad Coirul Anwar
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2026-06-01 9:05 UTC (permalink / raw)
To: Muchamad Coirul Anwar
Cc: linux-iio, rust-for-linux, linux-kernel, Miguel Ojeda,
Igor Korotin, Brandon Saint-John, Wolfram Sang, linux-i2c
+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
^ permalink raw reply [flat|nested] 3+ messages in thread