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... > --- 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); } > + > + 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); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx 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