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

[Geert Uytterhoeven]

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

Sorry for missing that before.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds