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

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@bp.renesas.com>
---
v3:
 * New patch

Ref:
 https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230505172530.357455-5-biju.das.jz@bp.renesas.com/
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  6 ++--
 drivers/i2c/i2c-core-base.c                  | 38 ++++++++++++++++----
 drivers/media/i2c/adv748x/adv748x-core.c     |  2 +-
 drivers/media/i2c/adv7604.c                  |  3 +-
 include/linux/i2c.h                          |  3 +-
 5 files changed, 39 insertions(+), 13 deletions(-)

Hi Biju,

On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@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@bp.renesas.com>
> ---
> v3:
>  * New patch

Thanks for your patch!

Looks correct to me, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@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

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