[PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> 
> Hi Biju,
> 
> On Wed, May 31, 2023 at 2:53 PM Biju Das <biju.das.jz at bp.renesas.com>
> wrote:
> > > > * This function creates and returns an I2C ancillary client whose
> > > > I2C address
> > > > * is retrieved from the platform firmware based on the given slave
> > > > name. If
> > > > * aux_device_name is not NULL, the ancillary's device parent
> > > > * will be set to the primary device otherwise it will be set to
> > > > I2C
> > > adapter.
> > >
> > > The wording is better, but this is not what you have implemented in
> > > the code. The name doesn't select which parent is used.
> >
> > It is the same implemented in the code.
> >
> > For the existing users, aux_device_name is NULL --> The parent is set
> as "I2C adapter".
> >
> > For instantiating a "i2c client device", aux_device_name is not NULL -
> -> The parent is set as primary device.
> >
> > The primary device is the one instantiated the "i2c client device"
> > using i2c_new_ancillary_device().
> >
> > Please correct me if anything wrong here.
> >
> > >
> > > > * If no address is specified by the firmware default_addr is used.
> > > >
> > > > > > > > the ancillary's device parent
> > > > > > > > + * will be set to the primary device.
> > > > > > >
> > > > > > > This doesn't seem to match the implementation. With this
> > > > > > > patch the ancillary device's parent is always the primary
> > > > > > > device. Are you sure this won't cause any regression ?
> > > > > >
> > > > > > There is no regression as existing users only instantiate
> > > > > > dummy
> > > > > device.
> > > > >
> > > > > Sorry, I don't follow you here. Existing callers of
> > > > > i2c_new_ancillary_device() today get an i2c_client device whose
> > > > > parent is the I2C adapter. With this patch they will get an
> > > > > i2c_client device whose parent is the main i2c_client. That's a
> > > > > change in behaviour, which could cause all sorts of issues.
> > > >
> > > > Please see the patch snippet below, there is no regression.
> > > >
> > > > client->dev.parent = parent ? parent : &client->adapter->dev;
> > >
> > > When called from i2c_new_ancillary_device(),
> > > __i2c_new_dummy_device() as a non-NULL parent argument. There is no
> > > change of behaviour *for i2c_new_dummy_device()*, but thre is a
> > > change of behaviour *for i2c_new_ancillary_device()*.
> >
> >
> > I don't think I understand what you mean.
> >
> > For existing users, i2c_new_ancillary_device(...,
> aux_device_name=NULL) the behaviour is not changed.
> >
> > Could you please elaborate further?
> 
> Laurent is right, there is a small issue:
> 
>     struct i2c_client *i2c_new_ancillary_device(struct i2c_client
> *client,
>                                                const char *name,
>                                                u16 default_addr,
>                                                const char
> *aux_device_name)
>     {
>             ...
>            return __i2c_new_dummy_device(client->adapter, addr,
> aux_device_name,
>                                          &client->dev);
>     }
> 
> To preserve backwards compatibility, the last parameter should be
> 
>      aux_device_name ? &client->dev : NULL

OK, will fix this in next version.

Cheers,
Biju