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

[Danilo Krummrich]

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 })
	    }
	}

Note that you can also combine you two sample drivers into one by doing the
above *and* register a and I2C driver that probes against your device
registration. :)