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

[Geert Uytterhoeven]

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...

> --- 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 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