Re: [PATCH v6 2/3] rust: i2c: Add basic I2C driver abstractions

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

Hello Danilo

On 10/26/2025 7:20 PM, Danilo Krummrich wrote:
> On 10/26/25 7:41 PM, Igor Korotin wrote:
>> Hello Danilo
>>
>>> On 10/5/25 12:23 PM, Igor Korotin wrote:
>>>> +impl Registration {
>>>> +    /// The C `i2c_new_client_device` function wrapper for manual I2C client creation.
>>>> +    pub fn new(i2c_adapter: &I2cAdapter, i2c_board_info: &I2cBoardInfo) -> Result<Self> {
>>>> +        // SAFETY: the kernel guarantees that `i2c_new_client_device()` returns either a valid
>>>> +        // pointer or NULL. `from_err_ptr` separates errors. Following `NonNull::new` checks for NULL.
>>>> +        let raw_dev = from_err_ptr(unsafe {
>>>> +            bindings::i2c_new_client_device(i2c_adapter.as_raw(), i2c_board_info.as_raw())
>>>> +        })?;
>>>> +
>>>> +        let dev_ptr = NonNull::new(raw_dev).ok_or(ENODEV)?;
>>>> +
>>>> +        Ok(Self(dev_ptr))
>>>> +    }
>>>> +}
>>>
>>> I wonder if we want to ensure that a Registration can't out-live the driver that
>>> registers the I2C client device.
>>>
>>> This should only ever be called by drivers bound to more complex devices, so if
>>> the parent driver is unbound I don't think I2C client device registered by this
>>> driver should be able to survive.
>>>
>>> Hence, I think Registration::new() should return
>>> impl PinInit<Devres<Self>, Error> instead.
>>
>> Maybe I'm missing something here, but as far as I understand, Devres is bound to
>> an existing device. However `Registration::new` creates new device and registers
>> new i2c_client using function `i2c_new_client_device`. Created i2c_client uses
>> i2c_adapter as its parent.
> 
> Correct, but the question is what's the correct lifetime boundary for this
> i2c:Registration.
>> The driver that declares Registration doesn't own that i2c_adapter. `Registration`
>> itself is not part of the new client’s managed resources, so returning
>> `impl PinInit<Devres<Self>, Error>` wouldn’t make sense here.
> 
> It does make sense, it's just not required for safety reasons.
> 
> This is an API that should be used by drivers operating complicated devices
> (DRM, NET, etc.) where there is no point in keeping an i2c::Registration alive
> after the driver that registered the I2C client has been unbound itself.
> 
> For instance, a GPU driver may call this in probe() to register an I2C device
> for some redriver, repeater, multiplexer, etc. So, it makes no sense to allow a
> corresponding i2c::Registration to still exist beyond the GPU driver being unbound.
> 
> Hence, besides not really being necessary for safety reasons, it still seems
> reasonable to enforce this for semantic reasons.

I might be misunderstanding your point, but as I see it, Devres cannot 
apply here
because we can't bind it to i2c_adapter. There's no guarantee that driver
owns it. Registration can't be bound to i2c_client, cause it's kind of 
chicken and egg situation.

Even if we returned impl PinInit<Self, Error>, it wouldn’t prevent other 
code from holding an additional reference to the client.
>> Drop for Registration calls `i2c_unregister_client()`, which gracefully unregisters
>> and deallocates the i2c_client.
> 
> Not quite, it unregisters the I2C client (which is why we call the object
> Registration), but it does not necessarily free the I2C client. Someone else can
> still hold a reference count of the I2C client.

You’re right, it doesn’t necessarily free the client immediately.
However, i2c_unregister_client() calls device_unregister(), which triggers
device_del() and put_device().

So while the memory may remain allocated until the last reference is 
dropped,
the device itself is already unregistered and deactivated at that point.
This aligns with the standard lifetime guarantees of the C driver model:
the structure remains valid only until its refcount reaches zero, after
which it’s released automatically.
That said, could you elaborate a bit more on what exactly you’d like to 
achieve by returning impl PinInit<Devres<Self>, Error> here?
Is the idea to explicitly tie the Registration lifetime to the parent 
device (for example, the i2c_adapter), or is this more about defining a 
general pattern for device-managed helper types in the Rust driver API?
I just want to be sure I’m following the intent of your suggestion 
correctly.

Cheers,
Igor