Re: [PATCH v2 1/4] rust: i2c: add basic I2C device and driver abstractions

[Igor Korotin]

On 7/4/25 21:16, Danilo Krummrich wrote:
> On Fri, Jul 04, 2025 at 04:36:57PM +0100, Igor Korotin wrote:
>> Implement the core abstractions needed for I2C drivers, including:
>>
>> * `i2c::Driver` — the trait drivers must implement, including `probe`
>>
>> * `i2c::Device` — a safe wrapper around `struct i2c_client`
>>
>> * `i2c::Adapter` — implements `driver::RegistrationOps` to hook into the
>>   generic `driver::Registration` machinery
>>
>> * `i2c::DeviceId` — a `RawDeviceId` implementation for I2C device IDs
>>
>> Signed-off-by: Igor Korotin <igor.korotin.linux@xxxxxxxxx>
>> ---
>>  MAINTAINERS                     |   2 +
>>  rust/bindings/bindings_helper.h |   1 +
>>  rust/helpers/helpers.c          |   1 +
>>  rust/helpers/i2c.c              |  15 ++
>>  rust/kernel/i2c.rs              | 391 ++++++++++++++++++++++++++++++++
>>  rust/kernel/lib.rs              |   2 +
>>  6 files changed, 412 insertions(+)
>>  create mode 100644 rust/helpers/i2c.c
>>  create mode 100644 rust/kernel/i2c.rs
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7f8ddeec3b17..688a0ff23e69 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11363,6 +11363,8 @@ F:	include/linux/i2c-smbus.h
>>  F:	include/linux/i2c.h
>>  F:	include/uapi/linux/i2c-*.h
>>  F:	include/uapi/linux/i2c.h
>> +F:	rust/helpers/i2c.c
>> +F:	rust/kernel/i2c.rs
> 
> Is this agreed with the maintainer?
> 
> There are multiple possible options, for instance:
> 
>   1) Maintain the Rust abstractions as part of the existing MAINTAINERS entry.
>      Optionally, the author can be added as another maintainer or reviewer.
> 
>   2) Add a separate MAINTAINERS entry; patches still go through the same
>      subsystem tree.
> 
>   3) Add a separate MAINTAINERS entry; patches don't go through the subsystem
>      tree (e.g. because the subsystem maintainers don't want to deal with it).
> 
> I usually recommend (1), which is what is proposes here, but that's of course up
> to you and the I2C maintainer.
> 
> @Wolfram: In case there's no agreement yet, what's your preference of
> maintainership for this?
> 

Oh, that makes sense - I didn’t realize I needed the current
maintainer’s sign-off to add new files under their MAINTAINERS entry.

As for being added as a reviewer or co-maintainer, I’m not yet confident
in my Rust skills. I’m learning Rust from scratch and, given my
extensive C-kernel background, I thought I’d start by contributing
something useful to the Rust side.

>> +    }
>> +
>> +    extern "C" fn shutdown_callback(pdev: *mut bindings::i2c_client) {
>> +        let pdev = unsafe { &*pdev.cast::<Device<device::Core>>() };
>> +
>> +        T::shutdown(pdev);
>> +    }
>> +
>> +    /// The [`i2c::IdTable`] of the corresponding driver.
>> +    fn i2c_id_table() -> Option<IdTable<<Self as driver::Adapter>::IdInfo>> {
>> +        T::I2C_ID_TABLE
>> +    }
>> +
>> +    /// Returns the driver's private data from the matching entry in the [`i2c::IdTable`], if any.
>> +    ///
>> +    /// If this returns `None`, it means there is no match with an entry in the [`i2c::IdTable`].
>> +    fn i2c_id_info(dev: &Device) -> Option<&'static <Self as driver::Adapter>::IdInfo> {
>> +        let table = Self::i2c_id_table()?;
>> +
>> +        // SAFETY:
>> +        // - `table` has static lifetime, hence it's valid for read,
>> +        // - `dev` is guaranteed to be valid while it's alive, and so is `pdev.as_ref().as_raw()`.
>> +        let raw_id = unsafe { bindings::i2c_match_id(table.as_ptr(), dev.as_raw()) };
>> +
>> +        if raw_id.is_null() {
>> +            None
>> +        } else {
>> +            // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct i2c_device_id` and
> 
> Nit: Missing ']', probably a copy-paste mistake from other busses.
> 
> 
> Yes? :)
> 

Yes, a lot of the code in this patch series is copy-paste of other
chunks of the existing Rust code because these parts are not part of
generic device/driver code.

I've made the same in ACPI patch series. I have already asked in that
patch series, if I need explicitly
mention that in the code or commit messages, I'm happy to do that.

>
> Just a note, this won't be needed in the future, I have patches in the
queue
> that let you use generic accessors from the generic Device type. [1]
>
> [1] https://lore.kernel.org/lkml/20250621195118.124245-3-dakr@xxxxxxxxxx/
>

>
> Just a heads-up, this trait will be split up. [2]
>
> [2]
https://lore.kernel.org/lkml/20250704041003.734033-2-fujita.tomonori@xxxxxxxxx/
>

>
> Better use from_result() here.
>

>
> I like to do that as well, but I usually get asked to use drop()
instead, let's
> do that here as well. :)
>

> 
> For now I think you have to ensure that I2C is built-in.
> 

Noted. Thanks for the review.

Best Regards
Igor