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

[Igor Korotin <igor.korotin.linux@gmail.com>]

Hello Daniel

Thank you for the review.

On 8/1/25 18:14, Daniel Almeida wrote:
>> +
>> +        Self(i2c)
>> +    }
>> +}
>> +
>> +// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `i2c_device_id` and does not add
>> +// additional invariants, so it's safe to transmute to `RawType`.
>> +unsafe impl RawDeviceId for DeviceId {
>> +    type RawType = bindings::i2c_device_id;
>> +}
>> +
>> +// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
>> +unsafe impl RawDeviceIdIndex for DeviceId {
>> +    const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::i2c_device_id, driver_data);
>> +
>> +    fn index(&self) -> usize {
>> +        self.0.driver_data as _
>> +    }
>> +}
>> +
>> +/// IdTable type for I2C
>> +pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
>> +
>> +/// Create a I2C `IdTable` with its alias for modpost.
>> +#[macro_export]
>> +macro_rules! i2c_device_table {
>> +    ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => {
>> +        const $table_name: $crate::device_id::IdArray<
>> +            $crate::i2c::DeviceId,
>> +            $id_info_type,
>> +            { $table_data.len() },
>> +        > = $crate::device_id::IdArray::new($table_data);
>> +
>> +        $crate::module_device_table!("i2c", $module_table_name, $table_name);
>> +    };
>> +}
>> +
>> +/// An adapter for the registration of I2C drivers.
>> +pub struct Adapter<T: Driver>(T);
>> +
>> +// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
>> +// a preceding call to `register` has been successful.
>> +unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
>> +    type RegType = bindings::i2c_driver;
>> +
>> +    unsafe fn register(
>> +        pdrv: &Opaque<Self::RegType>,
>> +        name: &'static CStr,
>> +        module: &'static ThisModule,
>> +    ) -> Result {
>> +        let i2c_table = match T::I2C_ID_TABLE {
>> +            Some(table) => table.as_ptr(),
>> +            None => core::ptr::null(),
>> +        };
>> +
>> +        let of_table = match T::OF_ID_TABLE {
>> +            Some(table) => table.as_ptr(),
>> +            None => core::ptr::null(),
>> +        };
>> +
>> +        let acpi_table = match T::ACPI_ID_TABLE {
>> +            Some(table) => table.as_ptr(),
>> +            None => core::ptr::null(),
>> +        };
> 
> I wonder what happens if these are all None?
> 

C code in the i2c_register_driver function doesn't require id_table, OF
table, or ACPI table to be non-NULL. Technically, there's no issue
registering such an I2C driver. Though there's the following code in
i2c_device_probe:

    /*
     * An I2C ID table is not mandatory, if and only if, a suitable OF
     * or ACPI ID table is supplied for the probing device.
     */
    if (!driver->id_table &&
        !acpi_driver_match_device(dev, dev->driver) &&
        !i2c_of_match_device(dev->driver->of_match_table, client)) {
        status = -ENODEV;
        goto put_sync_adapter;
    }

Based on that, maybe it makes sense to add some kind of
compile-time/build-time assertion to make sure the developer defines at
least one of those tables. If so, I'm happy to think about how to
implement something like that.

>> +
>> +        // SAFETY: It's safe to set the fields of `struct i2c_client` on initialization.
>> +        unsafe {
>> +            (*pdrv.get()).driver.name = name.as_char_ptr();
>> +            (*pdrv.get()).probe = Some(Self::probe_callback);
>> +            (*pdrv.get()).remove = Some(Self::remove_callback);
>> +            (*pdrv.get()).shutdown = Some(Self::shutdown_callback);
>> +            (*pdrv.get()).id_table = i2c_table;
>> +            (*pdrv.get()).driver.of_match_table = of_table;
>> +            (*pdrv.get()).driver.acpi_match_table = acpi_table;
>> +        }
>> +
>> +        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
>> +        to_result(unsafe { bindings::i2c_register_driver(module.0, pdrv.get()) })
>> +    }
>> +
>> +    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
>> +        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
>> +        unsafe { bindings::i2c_del_driver(pdrv.get()) }
>> +    }
>> +}
>> +
>> +impl<T: Driver + 'static> Adapter<T> {
>> +    extern "C" fn probe_callback(pdev: *mut bindings::i2c_client) -> kernel::ffi::c_int {
> 
> Nit: throughout the file: I think that pdev comes either from pci or platform
> devices? Maybe a different variable name should be used?
> 

To be honest, I always thought pdev/pdrv/etc just meant "pointer to
device"/"pointer to driver" and not necessarily PCI/platform. If it
actually implies platform/PCI, then yeah, I agree — better to use
something like idev/idrv, as Danilo also pointed out.

>> +        // - `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()) };
> 
> Wait, where’s `pdev.as_ref().as_raw()` here?
> 
Yeah, good catch. There was a significant rework between v2 and v3, and
I didn’t clean up all the comments properly. My bad.

Noted all the other comments too — I’ll fix those in the next drop.

Best Regards
Igor