[PATCH v3 1/5] i2c: Enhance i2c_new_ancillary_device API

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 1/5] i2c: Enhance i2c_new_ancillary_device API
> 
> Hi Biju,
> 
> On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz at bp.renesas.com>
> wrote:
> > Renesas PMIC RAA215300 exposes two separate i2c devices, one for the
> > main device and another for rtc device.
> >
> > Enhance i2c_new_ancillary_device() to instantiate a real device.
> > (eg: Instantiate rtc device from PMIC driver)
> >
> > Signed-off-by: Biju Das <biju.das.jz at bp.renesas.com>
> > ---
> > v3:
> >  * New patch
> 
> Thanks for your patch!
> 
> Looks correct to me, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas at glider.be>
> 
> Some suggestions for improvement below...

OK.

> 
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -1153,7 +1157,27 @@ struct i2c_client
> *i2c_new_ancillary_device(struct i2c_client *client,
> >         }
> >
> >         dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n",
> name, addr);
> > -       return i2c_new_dummy_device(client->adapter, addr);
> > +
> > +       if (aux_device_name) {
> > +               struct i2c_board_info info;
> > +               size_t aux_device_name_len = strlen(aux_device_name);
> > +
> > +               if (aux_device_name_len > I2C_NAME_SIZE - 1) {
> > +                       dev_err(&client->adapter->dev, "Invalid device
> name\n");
> > +                       return ERR_PTR(-EINVAL);
> > +               }
> 
> strscpy() return value?
> 
> > +
> > +               memset(&info, 0, sizeof(struct i2c_board_info));
> 
> The call to memset() would not be needed if info would be initialized at
> declaration time, i.e.
> 
>     struct i2c_board_info info = { .addr = addr };
> 
> Or, use I2C_BOARD_INFO(), to guarantee initialization is aligned with
> whatever future changes made to i2c_board_info? But that relies on
> providing the name at declaration time, which we already have in
> i2c_new_dummy_device().
> 
> So I suggest to add a name parameter to i2c_new_dummy_device(), rename
> it to __i2c_new_dummy_device(), and create a wrapper for compatibility
> with existing users:
> 
>     struct i2c_client *__i2c_new_dummy_device(struct i2c_adapter
> *adapter, u16 address,
>                                              const char *name)
>     {
>             struct i2c_board_info info = {
>                     I2C_BOARD_INFO("dummy", address),
>             };
> 
>             if (name) {
>                     ssize_ret = strscpy(info.type, name,
> sizeof(info.type));
> 
>                     if (ret < 0)
>                             return ERR_PTR(dev_err_probe(&client-
> >adapter->dev,
>                                            ret, "Invalid device
> name\n");
>             }
> 
>             return i2c_new_client_device(adapter, &info);
>     }

OK will introduce, static function

static struct i2c_client *__i2c_new_dummy_device(struct i2c_adapter *adapter,
                                                u16 address, const char *name)

and is called by both "i2c_new_dummy_device" and "i2c_new_ancillary_device"

Cheers,
Biju


> 
> > +
> > +               memcpy(info.type, aux_device_name,
> aux_device_name_len);
> > +               info.addr = addr;
> > +
> > +               i2c_aux_client = i2c_new_client_device(client-
> >adapter, &info);
> > +       } else {
> > +               i2c_aux_client = i2c_new_dummy_device(client->adapter,
> addr);
> > +       }
> > +
> > +       return i2c_aux_client;
> >  }
> >  EXPORT_SYMBOL_GPL(i2c_new_ancillary_device);