Re: [PATCH 1/2] media: i2c: adv7180: Extend match support for OF tables

[Jacopo Mondi]

Hi Biju

On Sun, Sep 10, 2023 at 04:21:59PM +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 adv7180_chip_info as match data for
> both these tables and replace the ID lookup table for the match data by
> i2c_get_match_data().
>
> 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>

Reviewed-by: Jacopo Mondi <jacopo.mondi+renesas@xxxxxxxxxxxxxxxx>

Thanks
  j

> ---
>  drivers/media/i2c/adv7180.c | 60 ++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 99ba925e8ec8..fc4f29e74e05 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -1395,7 +1395,6 @@ static int init_device(struct adv7180_state *state)
>
>  static int adv7180_probe(struct i2c_client *client)
>  {
> -	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>  	struct device_node *np = client->dev.of_node;
>  	struct adv7180_state *state;
>  	struct v4l2_subdev *sd;
> @@ -1411,7 +1410,7 @@ static int adv7180_probe(struct i2c_client *client)
>
>  	state->client = client;
>  	state->field = V4L2_FIELD_ALTERNATE;
> -	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
> +	state->chip_info = i2c_get_match_data(client);
>
>  	state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
>  						   GPIOD_OUT_HIGH);
> @@ -1536,22 +1535,6 @@ static void adv7180_remove(struct i2c_client *client)
>  	mutex_destroy(&state->mutex);
>  }
>
> -static const struct i2c_device_id adv7180_id[] = {
> -	{ "adv7180", (kernel_ulong_t)&adv7180_info },
> -	{ "adv7180cp", (kernel_ulong_t)&adv7180_info },
> -	{ "adv7180st", (kernel_ulong_t)&adv7180_info },
> -	{ "adv7182", (kernel_ulong_t)&adv7182_info },
> -	{ "adv7280", (kernel_ulong_t)&adv7280_info },
> -	{ "adv7280-m", (kernel_ulong_t)&adv7280_m_info },
> -	{ "adv7281", (kernel_ulong_t)&adv7281_info },
> -	{ "adv7281-m", (kernel_ulong_t)&adv7281_m_info },
> -	{ "adv7281-ma", (kernel_ulong_t)&adv7281_ma_info },
> -	{ "adv7282", (kernel_ulong_t)&adv7282_info },
> -	{ "adv7282-m", (kernel_ulong_t)&adv7282_m_info },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(i2c, adv7180_id);
> -
>  #ifdef CONFIG_PM_SLEEP
>  static int adv7180_suspend(struct device *dev)
>  {
> @@ -1585,22 +1568,37 @@ static SIMPLE_DEV_PM_OPS(adv7180_pm_ops, adv7180_suspend, adv7180_resume);
>  #define ADV7180_PM_OPS NULL
>  #endif
>
> +static const struct i2c_device_id adv7180_id[] = {
> +	{ "adv7180", (kernel_ulong_t)&adv7180_info },
> +	{ "adv7180cp", (kernel_ulong_t)&adv7180_info },
> +	{ "adv7180st", (kernel_ulong_t)&adv7180_info },
> +	{ "adv7182", (kernel_ulong_t)&adv7182_info },
> +	{ "adv7280", (kernel_ulong_t)&adv7280_info },
> +	{ "adv7280-m", (kernel_ulong_t)&adv7280_m_info },
> +	{ "adv7281", (kernel_ulong_t)&adv7281_info },
> +	{ "adv7281-m", (kernel_ulong_t)&adv7281_m_info },
> +	{ "adv7281-ma", (kernel_ulong_t)&adv7281_ma_info },
> +	{ "adv7282", (kernel_ulong_t)&adv7282_info },
> +	{ "adv7282-m", (kernel_ulong_t)&adv7282_m_info },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, adv7180_id);
> +
>  #ifdef CONFIG_OF
>  static const struct of_device_id adv7180_of_id[] = {
> -	{ .compatible = "adi,adv7180", },
> -	{ .compatible = "adi,adv7180cp", },
> -	{ .compatible = "adi,adv7180st", },
> -	{ .compatible = "adi,adv7182", },
> -	{ .compatible = "adi,adv7280", },
> -	{ .compatible = "adi,adv7280-m", },
> -	{ .compatible = "adi,adv7281", },
> -	{ .compatible = "adi,adv7281-m", },
> -	{ .compatible = "adi,adv7281-ma", },
> -	{ .compatible = "adi,adv7282", },
> -	{ .compatible = "adi,adv7282-m", },
> -	{ },
> +	{ .compatible = "adi,adv7180", &adv7180_info },
> +	{ .compatible = "adi,adv7180cp", &adv7180_info },
> +	{ .compatible = "adi,adv7180st", &adv7180_info },
> +	{ .compatible = "adi,adv7182", &adv7182_info },
> +	{ .compatible = "adi,adv7280", &adv7280_info },
> +	{ .compatible = "adi,adv7280-m", &adv7280_m_info },
> +	{ .compatible = "adi,adv7281", &adv7281_info },
> +	{ .compatible = "adi,adv7281-m", &adv7281_m_info },
> +	{ .compatible = "adi,adv7281-ma", &adv7281_ma_info },
> +	{ .compatible = "adi,adv7282", &adv7282_info },
> +	{ .compatible = "adi,adv7282-m", &adv7282_m_info },
> +	{}
>  };
> -
>  MODULE_DEVICE_TABLE(of, adv7180_of_id);
>  #endif
>
> --
> 2.25.1
>