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

["Danilo Krummrich" <dakr@kernel.org>]

On Fri Aug 1, 2025 at 7:14 PM CEST, Daniel Almeida wrote:
>> +impl DeviceId {
>> +    const I2C_NAME_SIZE: usize = 20;
>> +
>> +    /// Create a new device id from an I2C 'id' string.
>> +    #[inline(always)]
>> +    pub const fn new(id: &'static CStr) -> Self {
>> +        build_assert!(
>> +            id.len_with_nul() <= Self::I2C_NAME_SIZE,
>> +            "ID exceeds 20 bytes"
>> +        );
>> +        let src = id.as_bytes_with_nul();
>> +        // Replace with `bindings::acpi_device_id::default()` once stabilized for `const`.
>> +        // SAFETY: FFI type is valid to be zero-initialized.
>> +        let mut i2c: bindings::i2c_device_id = unsafe { core::mem::zeroed() };
>> +        let mut i = 0;
>> +        while i < src.len() {
>> +            i2c.name[i] = src[i];
>> +            i += 1;
>> +        }
>
> Nit: 
>
> for i in 0..src.len() {
>
> }
>
> Saves you from manually keeping track of the loop variable.

That would be nice, but we can't do this in cost context. :(

>> +// SAFETY: Instances of `Device` are always reference-counted.
>> +unsafe impl crate::types::AlwaysRefCounted for Device {
>> +    fn inc_ref(&self) {
>> +        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
>> +        unsafe { bindings::get_device(self.as_ref().as_raw()) };
>> +    }
>> +
>> +    unsafe fn dec_ref(obj: NonNull<Self>) {
>> +        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
>> +        unsafe { bindings::put_device(addr_of_mut!((*obj.as_ref().as_raw()).dev)) }
>> +    }
>> +}
>> +
>> +impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
>> +    fn as_ref(&self) -> &device::Device<Ctx> {
>> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
>> +        // `struct i2c_client`.
>> +        let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
>
> &raw mut
>
> I also wonder whether this unsafe block can be made shorter, as only the
> dereference is unsafe.

No, this would create a temporary value.