Hi Biju
On Sun, Sep 10, 2023 at 04:00:32PM +0100, Biju Das wrote:
> 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@xxxxxxxxxxxxxx>
> ---
> 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);
I was about to complain this function is not used, but then I noticed
you just introduced it, so this is like the first user!
> 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 */ }
The patch looks good to me, would be nice to know if you have been
able to run-time test at least one of the drivers you have moved to
use i2c_get_match_data().
Reviewed-by: Jacopo Mondi <jacopo.mondi+renesas@xxxxxxxxxxxxxxxx>
> };
> MODULE_DEVICE_TABLE(of, ov7670_of_match);
> #endif
> --
> 2.25.1
>
