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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> > ---
> > v3:
> > * New patch
>
> Thanks for your patch!
>
> Looks correct to me, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> 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);
