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

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

Hi Daniel

On 8/27/2025 7:37 PM, Daniel Almeida wrote:
>> +
>> +        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(),
>> +        };
>> +
>> +        // SAFETY: It's safe to set the fields of `struct i2c_client` on initialization.
>> +        unsafe {
>> +            (*idrv.get()).driver.name = name.as_char_ptr();
>> +            (*idrv.get()).probe = Some(Self::probe_callback);
>> +            (*idrv.get()).remove = Some(Self::remove_callback);
>> +            (*idrv.get()).shutdown = Some(Self::shutdown_callback);
>> +            (*idrv.get()).id_table = i2c_table;
>> +            (*idrv.get()).driver.of_match_table = of_table;
>> +            (*idrv.get()).driver.acpi_match_table = acpi_table;
>> +        }
>> +
>> +        // SAFETY: `idrv` is guaranteed to be a valid `RegType`.
>> +        to_result(unsafe { bindings::i2c_register_driver(module.0, idrv.get()) })
>> +    }
>> +
>> +    unsafe fn unregister(idrv: &Opaque<Self::RegType>) {
>> +        // SAFETY: `idrv` is guaranteed to be a valid `RegType`.
>> +        unsafe { bindings::i2c_del_driver(idrv.get()) }
>> +    }
>> +}
>> +
>> +impl<T: Driver + 'static> Adapter<T> {
>> +    extern "C" fn probe_callback(idev: *mut bindings::i2c_client) -> kernel::ffi::c_int {
>> +        // SAFETY: The I2C bus only ever calls the probe callback with a valid pointer to a
>> +        // `struct i2c_client`.
>> +        //
>> +        // INVARIANT: `idev` is valid for the duration of `probe_callback()`.
>> +        let idev = unsafe { &*idev.cast::<I2cClient<device::CoreInternal>>() };
>> +
>> +        let info =
>> +            Self::i2c_id_info(idev).or_else(|| <Self as driver::Adapter>::id_info(idev.as_ref()));
> 
> I wonder if these should be private member functions?
> 
>> +
>> +        from_result(|| {
>> +            let data = T::probe(idev, info)?;
>> +
>> +            idev.as_ref().set_drvdata(data);
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    extern "C" fn remove_callback(idev: *mut bindings::i2c_client) {
>> +        // SAFETY: `idev` is a valid pointer to a `struct i2c_client`.
>> +        let idev = unsafe { &*idev.cast::<I2cClient<device::CoreInternal>>() };
> 
>> +
>> +        // SAFETY: `remove_callback` is only ever called after a successful call to
>> +        // `probe_callback`, hence it's guaranteed that `I2cClient::set_drvdata()` has been called
>> +        // and stored a `Pin<KBox<T>>`.
>> +        drop(unsafe { idev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() });
>> +    }
>> +
>> +    extern "C" fn shutdown_callback(idev: *mut bindings::i2c_client) {
>> +        // SAFETY: `shutdown_callback` is only ever called for a valid `idev`
>> +        let idev = unsafe { &*idev.cast::<I2cClient<device::Core>>() };
>> +
>> +        T::shutdown(idev);
>> +    }
>> +
>> +    /// 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: &I2cClient) -> Option<&'static <Self as driver::Adapter>::IdInfo> {
> 
> Again, perhaps a private member function? I’m trying to simplify the syntax here.

Can you, please, kindly clarify what do you mean? If a function is not 
pub/pub(crate),
it is a private function.

Thanks,
Igor