Re: [PATCH v5 2/3] rust: i2c: add manual I2C device creation abstractions

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

On Thu Sep 11, 2025 at 5:50 PM CEST, Igor Korotin wrote:
> +impl I2cAdapter {
> +    /// Gets pointer to an `i2c_adapter` by index.
> +    pub fn get(index: i32) -> Result<ARef<Self>> {

Where do we get this index usually from? OF, ACPI, etc. I assume? I feel like it
could make sense to wrap it into a new type. Even though it is not safety
relevant it eliminates a source for mistakes.

> +        // SAFETY: `index` must refer to a valid I2C adapter; the kernel
> +        // guarantees that `i2c_get_adapter(index)` returns either a valid
> +        // pointer or NULL. `NonNull::new` guarantees the correct check.
> +        let adapter = NonNull::new(unsafe { bindings::i2c_get_adapter(index) }).ok_or(ENODEV)?;
> +
> +        // SAFETY: `adapter` is non-null and points to a live `i2c_adapter`.
> +        // `I2cAdapter` is #[repr(transparent)], so this cast is valid.
> +        Ok(unsafe { (&*adapter.as_ptr().cast::<I2cAdapter<device::Normal>>()).into() })
> +    }
> +}
> +
> +impl<Ctx: device::DeviceContext> Drop for I2cAdapter<Ctx> {
> +    fn drop(&mut self) {
> +        // SAFETY: This `I2cAdapter` was obtained from `i2c_get_adapter`,
> +        // and calling `i2c_put_adapter` exactly once will correctly release
> +        // the reference count in the I2C core. It is safe to call from any context
> +        unsafe { bindings::i2c_put_adapter(self.as_raw()) }
> +    }
> +}

The Drop implementation is not needed, you only ever give out an
ARef<I2cAdapter>, but never a "raw" I2cAdapter, which is the correct thing to
do.

> +
> +// SAFETY: `I2cAdapter` is a transparent wrapper of a type that doesn't depend on `I2cAdapter`'s generic
> +// argument.
> +kernel::impl_device_context_deref!(unsafe { I2cAdapter });
> +kernel::impl_device_context_into_aref!(I2cAdapter);
> +
> +// SAFETY: Instances of `I2cAdapter` are always reference-counted.
> +unsafe impl crate::types::AlwaysRefCounted for I2cAdapter {
> +    fn inc_ref(&self) {
> +        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> +        unsafe { bindings::i2c_get_adapter((*self.as_raw()).nr) };

Please make accessing the nr field a separate inline function, or at least put
it in a separate unsafe block.

> +    }
> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> +        unsafe { bindings::i2c_put_adapter(&raw mut (*obj.as_ref().as_raw())) }

Same here, separate unsafe blocks please.

> +    }
> +}
> +
> +impl<Ctx: device::DeviceContext> AsRef<I2cAdapter<Ctx>> for I2cAdapter<Ctx> {
> +    fn as_ref(&self) -> &I2cAdapter<Ctx> {
> +        &self
> +    }
> +}

This AsRef implementation doesn't seem to do anything?