Re: [PATCH v2 2/4] rust: i2c: add manual I2C device creation abstractions

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

On 7/7/25 13:02, Danilo Krummrich wrote:
> On Mon, Jul 07, 2025 at 12:20:15PM +0100, Igor Korotin wrote:
>>
>>
>> On 7/4/25 20:54, Danilo Krummrich wrote:
>>> On Fri, Jul 04, 2025 at 04:39:12PM +0100, Igor Korotin wrote:
>>>> -pub struct Device<Ctx: device::DeviceContext = device::Normal>(
>>>> +pub struct Device<Ctx: device::DeviceContext = device::Normal, State: DeviceState = state::Borrowed>(
>>>>      Opaque<bindings::i2c_client>,
>>>>      PhantomData<Ctx>,
>>>> +    PhantomData<State>,
>>>>  );
>>>
>>> I see what you're doing here, but I think you're thinking this way too
>>> complicated.
>>>
>>> I recommend not to reuse the Device type to register a new I2C client device,
>>> it's adding too much complexity without any real value.
>>>
>>> You also don't want the DeviceContext types for a device registration, since the
>>> registration will never have any other DeviceContext than device::Normal (see
>>> also my comment on the sample module).
>>>
>>> DeviceContext types are only useful for &Device (i.e. references) given out for
>>> a specific scope, such as probe(), remove(), etc.
>>>
>>> The only thing you really want to do is to register a new I2C client device, get
>>> a i2c::Registration instance and call i2c_unregister_device() when the
>>> i2c::Registration is dropped.
>>>
>>> This is exactly the same use-case as we have in the auxiliary bus. I highly
>>> recommend looking at what auxiliary::Registration does [1].
>>>
>>> Also note that if you want a reference to the device in the i2c::Registration,
>>> you can also add a i2c::Registration::device() method that returns an
>>> &i2c::Device, which through into() you can obtain an ARef<i2c::Device> from.
>>>
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/auxiliary.rs?h=v6.16-rc4#n299
>>
>> I took a quick look at the auxiliary Registration abstraction and I see
>> that it is not applicable for I2C subsystem. The issue here is that I2C
>> C code doesn't provide with an API that can registers an I2C client from
>> already existing `struct i2c_client`.
> 
> I don't see why the following wouldn't work:
> 
> 	struct Registration(NonNull<bindings::i2c_client>);
> 
> 	impl Registration {
> 	   pub fn new(adp: &I2cAdapterRef, info: &I2cBoardInfo) -> Result<Self> {
> 	      // SAFETY: [...]
> 	      let cli = unsafe { bindings::i2c_new_client_device(adp.as_raw(), info.as_raw()) };
> 	
> 	      // Handle ERR_PTR()
> 	
> 	      Self(NonNull::new(cli))
> 	   }
> 	}
> 	
> 	impl Drop for Registration {
> 	   fn drop(&mut self) {
> 	      // SAFETY: [...]
> 	      unsafe { bindings::i2c_unregister_device(self.as_ptr()) };
> 	   }
> 	}
> 
> And in you sample driver you can still the exactly the same as you did before:
> 
> 	struct SampleDriver {
> 	   _reg: i2c::Registration,
> 	}
> 	
> 	impl kernel::Module for SampleDriver {
> 	    fn init(_module: &'static ThisModule) -> Result<Self> {
> 	        let adapter = i2c::I2cAdapterRef::get(0).ok_or(EINVAL)?;
> 	
> 	        let reg = i2c::Registration::new(&adapter, &BOARD_INFO)?;
> 	
> 	        Ok(Self { _reg: reg })
> 	    }
> 	}
> 

Ok, I think I've caught the idea. In general I worried that if one has
link to an i2c::Device which is a transparent representation of `struct
i2c_client` he could somehow cast that `i2c::Device` to
`Registration(NonNull<bindings::i2c_client>)`. After some experiments, I
found out that this is not possible due to scope of the
`i2c::Device::as_raw()`. So the simple NonNull implementation is as safe
as my "super-puper secured" DeviceOwned implementation.
Thanks
Igor