Message-ID: <169442220081.2427060.12168685477104863217@ping.linuxembedded.co.uk>
Date:   Mon, 11 Sep 2023 09:50:00 +0100
From:   Kieran Bingham <kieran.bingham@...asonboard.com>
To:     Biju Das <biju.das.jz@...renesas.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Biju Das <biju.das.jz@...renesas.com>, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, Biju Das <biju.das.au@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH 1/2] media: i2c: ov7670: Extend match support for OF tables

Quoting Biju Das (2023-09-10 16:00:32)
> The driver has an OF match table, still, it uses an ID lookup table for
> retrieving match data. Currently, the driver is working on the
> assumption that an I2C device registered via OF will always match a
> legacy I2C device ID. The correct approach is to have an OF device ID
> table using i2c_get_match_data() if the devices are registered via OF/ID.
> 
> Unify the OF/ID table by using struct ov7670_devtype as match data for
> both these tables and replace the ID lookup table for the match data by
> i2c_get_match_data().
> 
> Split the array ov7670_devdata[] as individual variables, and
> make lines shorter by referring to e.g. &ov7670_devdata instead of
> &ov7670_devdata[MODEL_OV7670].
> 
> Drop enum ov7670_model as there is no user.
> 
> While at it, remove the trailing comma in the terminator entry for the OF
> table making code robust against (theoretical) misrebases or other similar
> things where the new entry goes _after_ the termination without the
> compiler noticing.
> 
> Signed-off-by: Biju Das <biju.das.jz@...renesas.com>
> ---
>  drivers/media/i2c/ov7670.c | 47 ++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 2f55491ef571..644457e6993d 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -186,11 +186,6 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
>  #define REG_HAECC7     0xaa    /* Hist AEC/AGC control 7 */
>  #define REG_BD60MAX    0xab    /* 60hz banding step limit */
>  
> -enum ov7670_model {
> -       MODEL_OV7670 = 0,
> -       MODEL_OV7675,
> -};
> -
>  struct ov7670_win_size {
>         int     width;
>         int     height;
> @@ -1774,21 +1769,6 @@ static const struct v4l2_subdev_internal_ops ov7670_subdev_internal_ops = {
>  
>  /* ----------------------------------------------------------------------- */
>  
> -static const struct ov7670_devtype ov7670_devdata[] = {
> -       [MODEL_OV7670] = {
> -               .win_sizes = ov7670_win_sizes,
> -               .n_win_sizes = ARRAY_SIZE(ov7670_win_sizes),
> -               .set_framerate = ov7670_set_framerate_legacy,
> -               .get_framerate = ov7670_get_framerate_legacy,
> -       },
> -       [MODEL_OV7675] = {
> -               .win_sizes = ov7675_win_sizes,
> -               .n_win_sizes = ARRAY_SIZE(ov7675_win_sizes),
> -               .set_framerate = ov7675_set_framerate,
> -               .get_framerate = ov7675_get_framerate,
> -       },
> -};
> -
>  static int ov7670_init_gpio(struct i2c_client *client, struct ov7670_info *info)
>  {
>         info->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> @@ -1849,7 +1829,6 @@ static int ov7670_parse_dt(struct device *dev,
>  
>  static int ov7670_probe(struct i2c_client *client)
>  {
> -       const struct i2c_device_id *id = i2c_client_get_device_id(client);
>         struct v4l2_fract tpf;
>         struct v4l2_subdev *sd;
>         struct ov7670_info *info;
> @@ -1923,7 +1902,7 @@ static int ov7670_probe(struct i2c_client *client)
>         v4l_info(client, "chip found @ 0x%02x (%s)\n",
>                         client->addr << 1, client->adapter->name);
>  
> -       info->devtype = &ov7670_devdata[id->driver_data];
> +       info->devtype = i2c_get_match_data(client);
>         info->fmt = &ov7670_formats[0];
>         info->wsize = &info->devtype->win_sizes[0];
>  
> @@ -2013,17 +1992,31 @@ static void ov7670_remove(struct i2c_client *client)
>         media_entity_cleanup(&info->sd.entity);
>  }
>  
> +static const struct ov7670_devtype ov7670_devdata = {
> +       .win_sizes = ov7670_win_sizes,
> +       .n_win_sizes = ARRAY_SIZE(ov7670_win_sizes),
> +       .set_framerate = ov7670_set_framerate_legacy,
> +       .get_framerate = ov7670_get_framerate_legacy,
> +};
> +
> +static const struct ov7670_devtype ov7675_devdata = {
> +       .win_sizes = ov7675_win_sizes,
> +       .n_win_sizes = ARRAY_SIZE(ov7675_win_sizes),
> +       .set_framerate = ov7675_set_framerate,
> +       .get_framerate = ov7675_get_framerate,
> +};
> +
>  static const struct i2c_device_id ov7670_id[] = {
> -       { "ov7670", MODEL_OV7670 },
> -       { "ov7675", MODEL_OV7675 },
> -       { }
> +       { "ov7670", (kernel_ulong_t)&ov7670_devdata },
> +       { "ov7675", (kernel_ulong_t)&ov7675_devdata },
> +       { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(i2c, ov7670_id);
>  
>  #if IS_ENABLED(CONFIG_OF)
>  static const struct of_device_id ov7670_of_match[] = {
> -       { .compatible = "ovti,ov7670", },
> -       { /* sentinel */ },
> +       { .compatible = "ovti,ov7670", &ov7670_devdata },
> +       { /* sentinel */ }

This all looks good to me - by why is there no ov7675 compatible string?
Should that be added? (as a separate patch?)

Can the model be detected at runtime (Is there a version register on
this chip?)

But otherwise looks fine.


Reviewed-by: Kieran Bingham <kieran.bingham+renesas@...asonboard.com>

>  };
>  MODULE_DEVICE_TABLE(of, ov7670_of_match);
>  #endif
> -- 
> 2.25.1
>

Powered by blists - more mailing lists