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
