[PATCH 0/2] Match data improvements for mt9p031 driver

[PATCH 0/2] Match data improvements for mt9p031 driver

[Biju Das]

This patch series aims to add match data improvements for mt9p031 driver.

This patch series is only compile tested.

Biju Das (2):
  media: mt9p031: Extend match support for OF tables
  media: mt9p031: Drop CONFIG_OF ifdeffery

 drivers/media/i2c/mt9p031.c | 38 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH 1/2] media: mt9p031: Extend match support for OF tables

[Biju Das]

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 MEDIA_BUS_FMT as match data for both these
tables and replace the ID lookup table for the match data by
i2c_get_match_data() and simplifly probe() and mt9p031_init_cfg()

Drop mt9p031_init_cfg 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@bp.renesas.com>
---
 drivers/media/i2c/mt9p031.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 348f1e1098fb..540cb519915c 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -112,11 +112,6 @@
 #define MT9P031_TEST_PATTERN_RED			0xa2
 #define MT9P031_TEST_PATTERN_BLUE			0xa3
 
-enum mt9p031_model {
-	MT9P031_MODEL_COLOR,
-	MT9P031_MODEL_MONOCHROME,
-};
-
 struct mt9p031 {
 	struct v4l2_subdev subdev;
 	struct media_pad pad;
@@ -129,7 +124,7 @@ struct mt9p031 {
 	struct clk *clk;
 	struct regulator_bulk_data regulators[3];
 
-	enum mt9p031_model model;
+	u32 code;
 	struct aptina_pll pll;
 	unsigned int clk_div;
 	bool use_pll;
@@ -714,12 +709,7 @@ static int mt9p031_init_cfg(struct v4l2_subdev *subdev,
 	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
 
 	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0, which);
-
-	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
-		format->code = MEDIA_BUS_FMT_Y12_1X12;
-	else
-		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
-
+	format->code = mt9p031->code;
 	format->width = MT9P031_WINDOW_WIDTH_DEF;
 	format->height = MT9P031_WINDOW_HEIGHT_DEF;
 	format->field = V4L2_FIELD_NONE;
@@ -1104,7 +1094,6 @@ mt9p031_get_pdata(struct i2c_client *client)
 
 static int mt9p031_probe(struct i2c_client *client)
 {
-	const struct i2c_device_id *did = i2c_client_get_device_id(client);
 	struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
 	struct i2c_adapter *adapter = client->adapter;
 	struct mt9p031 *mt9p031;
@@ -1129,7 +1118,7 @@ static int mt9p031_probe(struct i2c_client *client)
 	mt9p031->pdata = pdata;
 	mt9p031->output_control	= MT9P031_OUTPUT_CONTROL_DEF;
 	mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
-	mt9p031->model = did->driver_data;
+	mt9p031->code = (uintptr_t)i2c_get_match_data(client);
 
 	mt9p031->regulators[0].supply = "vdd";
 	mt9p031->regulators[1].supply = "vdd_io";
@@ -1226,19 +1215,19 @@ static void mt9p031_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id mt9p031_id[] = {
-	{ "mt9p006", MT9P031_MODEL_COLOR },
-	{ "mt9p031", MT9P031_MODEL_COLOR },
-	{ "mt9p031m", MT9P031_MODEL_MONOCHROME },
-	{ }
+	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
+	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
+	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, mt9p031_id);
 
 #if IS_ENABLED(CONFIG_OF)
 static const struct of_device_id mt9p031_of_match[] = {
-	{ .compatible = "aptina,mt9p006", },
-	{ .compatible = "aptina,mt9p031", },
-	{ .compatible = "aptina,mt9p031m", },
-	{ /* sentinel */ },
+	{ .compatible = "aptina,mt9p006", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
+	{ .compatible = "aptina,mt9p031", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
+	{ .compatible = "aptina,mt9p031m", .data = (void *)MEDIA_BUS_FMT_Y12_1X12 },
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, mt9p031_of_match);
 #endif
-- 
2.25.1

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

[Jacopo Mondi]

Hi Biju

On Sun, Sep 10, 2023 at 05:01:25PM +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 MEDIA_BUS_FMT as match data for both these
> tables and replace the ID lookup table for the match data by
> i2c_get_match_data() and simplifly probe() and mt9p031_init_cfg()
>
> Drop mt9p031_init_cfg 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@bp.renesas.com>
> ---
>  drivers/media/i2c/mt9p031.c | 33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 348f1e1098fb..540cb519915c 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -112,11 +112,6 @@
>  #define MT9P031_TEST_PATTERN_RED			0xa2
>  #define MT9P031_TEST_PATTERN_BLUE			0xa3
>
> -enum mt9p031_model {
> -	MT9P031_MODEL_COLOR,
> -	MT9P031_MODEL_MONOCHROME,
> -};
> -
>  struct mt9p031 {
>  	struct v4l2_subdev subdev;
>  	struct media_pad pad;
> @@ -129,7 +124,7 @@ struct mt9p031 {
>  	struct clk *clk;
>  	struct regulator_bulk_data regulators[3];
>
> -	enum mt9p031_model model;
> +	u32 code;
>  	struct aptina_pll pll;
>  	unsigned int clk_div;
>  	bool use_pll;
> @@ -714,12 +709,7 @@ static int mt9p031_init_cfg(struct v4l2_subdev *subdev,
>  	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
>
>  	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0, which);
> -
> -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> -		format->code = MEDIA_BUS_FMT_Y12_1X12;
> -	else
> -		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> -
> +	format->code = mt9p031->code;
>  	format->width = MT9P031_WINDOW_WIDTH_DEF;
>  	format->height = MT9P031_WINDOW_HEIGHT_DEF;
>  	format->field = V4L2_FIELD_NONE;
> @@ -1104,7 +1094,6 @@ mt9p031_get_pdata(struct i2c_client *client)
>
>  static int mt9p031_probe(struct i2c_client *client)
>  {
> -	const struct i2c_device_id *did = i2c_client_get_device_id(client);
>  	struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
>  	struct i2c_adapter *adapter = client->adapter;
>  	struct mt9p031 *mt9p031;
> @@ -1129,7 +1118,7 @@ static int mt9p031_probe(struct i2c_client *client)
>  	mt9p031->pdata = pdata;
>  	mt9p031->output_control	= MT9P031_OUTPUT_CONTROL_DEF;
>  	mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
> -	mt9p031->model = did->driver_data;
> +	mt9p031->code = (uintptr_t)i2c_get_match_data(client);
>
>  	mt9p031->regulators[0].supply = "vdd";
>  	mt9p031->regulators[1].supply = "vdd_io";
> @@ -1226,19 +1215,19 @@ static void mt9p031_remove(struct i2c_client *client)
>  }
>
>  static const struct i2c_device_id mt9p031_id[] = {
> -	{ "mt9p006", MT9P031_MODEL_COLOR },
> -	{ "mt9p031", MT9P031_MODEL_COLOR },
> -	{ "mt9p031m", MT9P031_MODEL_MONOCHROME },
> -	{ }
> +	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
> +	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
> +	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
> +	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(i2c, mt9p031_id);
>
>  #if IS_ENABLED(CONFIG_OF)
>  static const struct of_device_id mt9p031_of_match[] = {
> -	{ .compatible = "aptina,mt9p006", },
> -	{ .compatible = "aptina,mt9p031", },
> -	{ .compatible = "aptina,mt9p031m", },
> -	{ /* sentinel */ },
> +	{ .compatible = "aptina,mt9p006", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> +	{ .compatible = "aptina,mt9p031", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> +	{ .compatible = "aptina,mt9p031m", .data = (void *)MEDIA_BUS_FMT_Y12_1X12 },
> +	{ /* sentinel */ }

I know it might sound not necessary, but isn't it better to wrap the
format in some sort of per-model structure. It would avoid a few type
casts too. Up to you thouhg

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

RE: [PATCH 1/2] media: mt9p031: Extend match support for OF tables

[Biju Das]

Hi Jacopo Mondi,

Thanks for the feedback.

> Subject: Re: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
> 
> Hi Biju
> 
> On Sun, Sep 10, 2023 at 05:01:25PM +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 MEDIA_BUS_FMT as match data for both
> > these tables and replace the ID lookup table for the match data by
> > i2c_get_match_data() and simplifly probe() and mt9p031_init_cfg()
> >
> > Drop mt9p031_init_cfg 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@bp.renesas.com>
> > ---
> >  drivers/media/i2c/mt9p031.c | 33 +++++++++++----------------------
> >  1 file changed, 11 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > index 348f1e1098fb..540cb519915c 100644
> > --- a/drivers/media/i2c/mt9p031.c
> > +++ b/drivers/media/i2c/mt9p031.c
> > @@ -112,11 +112,6 @@
> >  #define MT9P031_TEST_PATTERN_RED			0xa2
> >  #define MT9P031_TEST_PATTERN_BLUE			0xa3
> >
> > -enum mt9p031_model {
> > -	MT9P031_MODEL_COLOR,
> > -	MT9P031_MODEL_MONOCHROME,
> > -};
> > -
> >  struct mt9p031 {
> >  	struct v4l2_subdev subdev;
> >  	struct media_pad pad;
> > @@ -129,7 +124,7 @@ struct mt9p031 {
> >  	struct clk *clk;
> >  	struct regulator_bulk_data regulators[3];
> >
> > -	enum mt9p031_model model;
> > +	u32 code;
> >  	struct aptina_pll pll;
> >  	unsigned int clk_div;
> >  	bool use_pll;
> > @@ -714,12 +709,7 @@ static int mt9p031_init_cfg(struct v4l2_subdev
> *subdev,
> >  	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
> >
> >  	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0, which);
> > -
> > -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> > -		format->code = MEDIA_BUS_FMT_Y12_1X12;
> > -	else
> > -		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> > -
> > +	format->code = mt9p031->code;
> >  	format->width = MT9P031_WINDOW_WIDTH_DEF;
> >  	format->height = MT9P031_WINDOW_HEIGHT_DEF;
> >  	format->field = V4L2_FIELD_NONE;
> > @@ -1104,7 +1094,6 @@ mt9p031_get_pdata(struct i2c_client *client)
> >
> >  static int mt9p031_probe(struct i2c_client *client)  {
> > -	const struct i2c_device_id *did = i2c_client_get_device_id(client);
> >  	struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
> >  	struct i2c_adapter *adapter = client->adapter;
> >  	struct mt9p031 *mt9p031;
> > @@ -1129,7 +1118,7 @@ static int mt9p031_probe(struct i2c_client *client)
> >  	mt9p031->pdata = pdata;
> >  	mt9p031->output_control	= MT9P031_OUTPUT_CONTROL_DEF;
> >  	mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
> > -	mt9p031->model = did->driver_data;
> > +	mt9p031->code = (uintptr_t)i2c_get_match_data(client);
> >
> >  	mt9p031->regulators[0].supply = "vdd";
> >  	mt9p031->regulators[1].supply = "vdd_io";
> > @@ -1226,19 +1215,19 @@ static void mt9p031_remove(struct i2c_client
> *client)
> >  }
> >
> >  static const struct i2c_device_id mt9p031_id[] = {
> > -	{ "mt9p006", MT9P031_MODEL_COLOR },
> > -	{ "mt9p031", MT9P031_MODEL_COLOR },
> > -	{ "mt9p031m", MT9P031_MODEL_MONOCHROME },
> > -	{ }
> > +	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > +	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > +	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
> > +	{ /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, mt9p031_id);
> >
> >  #if IS_ENABLED(CONFIG_OF)
> >  static const struct of_device_id mt9p031_of_match[] = {
> > -	{ .compatible = "aptina,mt9p006", },
> > -	{ .compatible = "aptina,mt9p031", },
> > -	{ .compatible = "aptina,mt9p031m", },
> > -	{ /* sentinel */ },
> > +	{ .compatible = "aptina,mt9p006", .data = (void
> *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > +	{ .compatible = "aptina,mt9p031", .data = (void
> *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > +	{ .compatible = "aptina,mt9p031m", .data = (void
> *)MEDIA_BUS_FMT_Y12_1X12 },
> > +	{ /* sentinel */ }
> 
> I know it might sound not necessary, but isn't it better to wrap the
> format in some sort of per-model structure. It would avoid a few type
> casts too. Up to you though

The problem with structure is, it will have one
variable entry. I got some feedback related to similar
patches not to add a single variable to structure
and use the value directly instead.

Cheers,
Biju

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

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

[Jacopo Mondi]

Hi Biju

On Mon, Sep 11, 2023 at 09:14:35AM +0000, Biju Das wrote:
> Hi Jacopo Mondi,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
> >
> > Hi Biju
> >
> > On Sun, Sep 10, 2023 at 05:01:25PM +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 MEDIA_BUS_FMT as match data for both
> > > these tables and replace the ID lookup table for the match data by
> > > i2c_get_match_data() and simplifly probe() and mt9p031_init_cfg()
> > >
> > > Drop mt9p031_init_cfg 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@bp.renesas.com>
> > > ---
> > >  drivers/media/i2c/mt9p031.c | 33 +++++++++++----------------------
> > >  1 file changed, 11 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > > index 348f1e1098fb..540cb519915c 100644
> > > --- a/drivers/media/i2c/mt9p031.c
> > > +++ b/drivers/media/i2c/mt9p031.c
> > > @@ -112,11 +112,6 @@
> > >  #define MT9P031_TEST_PATTERN_RED			0xa2
> > >  #define MT9P031_TEST_PATTERN_BLUE			0xa3
> > >
> > > -enum mt9p031_model {
> > > -	MT9P031_MODEL_COLOR,
> > > -	MT9P031_MODEL_MONOCHROME,
> > > -};
> > > -
> > >  struct mt9p031 {
> > >  	struct v4l2_subdev subdev;
> > >  	struct media_pad pad;
> > > @@ -129,7 +124,7 @@ struct mt9p031 {
> > >  	struct clk *clk;
> > >  	struct regulator_bulk_data regulators[3];
> > >
> > > -	enum mt9p031_model model;
> > > +	u32 code;
> > >  	struct aptina_pll pll;
> > >  	unsigned int clk_div;
> > >  	bool use_pll;
> > > @@ -714,12 +709,7 @@ static int mt9p031_init_cfg(struct v4l2_subdev
> > *subdev,
> > >  	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
> > >
> > >  	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0, which);
> > > -
> > > -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> > > -		format->code = MEDIA_BUS_FMT_Y12_1X12;
> > > -	else
> > > -		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> > > -
> > > +	format->code = mt9p031->code;
> > >  	format->width = MT9P031_WINDOW_WIDTH_DEF;
> > >  	format->height = MT9P031_WINDOW_HEIGHT_DEF;
> > >  	format->field = V4L2_FIELD_NONE;
> > > @@ -1104,7 +1094,6 @@ mt9p031_get_pdata(struct i2c_client *client)
> > >
> > >  static int mt9p031_probe(struct i2c_client *client)  {
> > > -	const struct i2c_device_id *did = i2c_client_get_device_id(client);
> > >  	struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
> > >  	struct i2c_adapter *adapter = client->adapter;
> > >  	struct mt9p031 *mt9p031;
> > > @@ -1129,7 +1118,7 @@ static int mt9p031_probe(struct i2c_client *client)
> > >  	mt9p031->pdata = pdata;
> > >  	mt9p031->output_control	= MT9P031_OUTPUT_CONTROL_DEF;
> > >  	mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
> > > -	mt9p031->model = did->driver_data;
> > > +	mt9p031->code = (uintptr_t)i2c_get_match_data(client);
> > >
> > >  	mt9p031->regulators[0].supply = "vdd";
> > >  	mt9p031->regulators[1].supply = "vdd_io";
> > > @@ -1226,19 +1215,19 @@ static void mt9p031_remove(struct i2c_client
> > *client)
> > >  }
> > >
> > >  static const struct i2c_device_id mt9p031_id[] = {
> > > -	{ "mt9p006", MT9P031_MODEL_COLOR },
> > > -	{ "mt9p031", MT9P031_MODEL_COLOR },
> > > -	{ "mt9p031m", MT9P031_MODEL_MONOCHROME },
> > > -	{ }
> > > +	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > +	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > +	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
> > > +	{ /* sentinel */ }
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, mt9p031_id);
> > >
> > >  #if IS_ENABLED(CONFIG_OF)
> > >  static const struct of_device_id mt9p031_of_match[] = {
> > > -	{ .compatible = "aptina,mt9p006", },
> > > -	{ .compatible = "aptina,mt9p031", },
> > > -	{ .compatible = "aptina,mt9p031m", },
> > > -	{ /* sentinel */ },
> > > +	{ .compatible = "aptina,mt9p006", .data = (void
> > *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > +	{ .compatible = "aptina,mt9p031", .data = (void
> > *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > +	{ .compatible = "aptina,mt9p031m", .data = (void
> > *)MEDIA_BUS_FMT_Y12_1X12 },
> > > +	{ /* sentinel */ }
> >
> > I know it might sound not necessary, but isn't it better to wrap the
> > format in some sort of per-model structure. It would avoid a few type
> > casts too. Up to you though
>
> The problem with structure is, it will have one
> variable entry. I got some feedback related to similar
> patches not to add a single variable to structure
> and use the value directly instead.
>

Ok then, a matter of preferences I think. Up to you, really.

Cheers
  j

> Cheers,
> Biju
>
> >
> > >  };
> > >  MODULE_DEVICE_TABLE(of, mt9p031_of_match);
> > >  #endif
> > > --
> > > 2.25.1
> > >

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

[Jacopo Mondi]

Hello again

On Mon, Sep 11, 2023 at 11:25:03AM +0200, Jacopo Mondi wrote:
> Hi Biju
>
> On Mon, Sep 11, 2023 at 09:14:35AM +0000, Biju Das wrote:
> > Hi Jacopo Mondi,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
> > >
> > > Hi Biju
> > >
> > > On Sun, Sep 10, 2023 at 05:01:25PM +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 MEDIA_BUS_FMT as match data for both
> > > > these tables and replace the ID lookup table for the match data by
> > > > i2c_get_match_data() and simplifly probe() and mt9p031_init_cfg()
> > > >
> > > > Drop mt9p031_init_cfg 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@bp.renesas.com>
> > > > ---
> > > >  drivers/media/i2c/mt9p031.c | 33 +++++++++++----------------------
> > > >  1 file changed, 11 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > > > index 348f1e1098fb..540cb519915c 100644
> > > > --- a/drivers/media/i2c/mt9p031.c
> > > > +++ b/drivers/media/i2c/mt9p031.c
> > > > @@ -112,11 +112,6 @@
> > > >  #define MT9P031_TEST_PATTERN_RED			0xa2
> > > >  #define MT9P031_TEST_PATTERN_BLUE			0xa3
> > > >
> > > > -enum mt9p031_model {
> > > > -	MT9P031_MODEL_COLOR,
> > > > -	MT9P031_MODEL_MONOCHROME,
> > > > -};
> > > > -
> > > >  struct mt9p031 {
> > > >  	struct v4l2_subdev subdev;
> > > >  	struct media_pad pad;
> > > > @@ -129,7 +124,7 @@ struct mt9p031 {
> > > >  	struct clk *clk;
> > > >  	struct regulator_bulk_data regulators[3];
> > > >
> > > > -	enum mt9p031_model model;
> > > > +	u32 code;
> > > >  	struct aptina_pll pll;
> > > >  	unsigned int clk_div;
> > > >  	bool use_pll;
> > > > @@ -714,12 +709,7 @@ static int mt9p031_init_cfg(struct v4l2_subdev
> > > *subdev,
> > > >  	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
> > > >
> > > >  	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0, which);
> > > > -
> > > > -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> > > > -		format->code = MEDIA_BUS_FMT_Y12_1X12;
> > > > -	else
> > > > -		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> > > > -
> > > > +	format->code = mt9p031->code;
> > > >  	format->width = MT9P031_WINDOW_WIDTH_DEF;
> > > >  	format->height = MT9P031_WINDOW_HEIGHT_DEF;
> > > >  	format->field = V4L2_FIELD_NONE;
> > > > @@ -1104,7 +1094,6 @@ mt9p031_get_pdata(struct i2c_client *client)
> > > >
> > > >  static int mt9p031_probe(struct i2c_client *client)  {
> > > > -	const struct i2c_device_id *did = i2c_client_get_device_id(client);
> > > >  	struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
> > > >  	struct i2c_adapter *adapter = client->adapter;
> > > >  	struct mt9p031 *mt9p031;
> > > > @@ -1129,7 +1118,7 @@ static int mt9p031_probe(struct i2c_client *client)
> > > >  	mt9p031->pdata = pdata;
> > > >  	mt9p031->output_control	= MT9P031_OUTPUT_CONTROL_DEF;
> > > >  	mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
> > > > -	mt9p031->model = did->driver_data;
> > > > +	mt9p031->code = (uintptr_t)i2c_get_match_data(client);
> > > >
> > > >  	mt9p031->regulators[0].supply = "vdd";
> > > >  	mt9p031->regulators[1].supply = "vdd_io";
> > > > @@ -1226,19 +1215,19 @@ static void mt9p031_remove(struct i2c_client
> > > *client)
> > > >  }
> > > >
> > > >  static const struct i2c_device_id mt9p031_id[] = {
> > > > -	{ "mt9p006", MT9P031_MODEL_COLOR },
> > > > -	{ "mt9p031", MT9P031_MODEL_COLOR },
> > > > -	{ "mt9p031m", MT9P031_MODEL_MONOCHROME },
> > > > -	{ }
> > > > +	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > +	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > +	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
> > > > +	{ /* sentinel */ }
> > > >  };
> > > >  MODULE_DEVICE_TABLE(i2c, mt9p031_id);
> > > >
> > > >  #if IS_ENABLED(CONFIG_OF)
> > > >  static const struct of_device_id mt9p031_of_match[] = {
> > > > -	{ .compatible = "aptina,mt9p006", },
> > > > -	{ .compatible = "aptina,mt9p031", },
> > > > -	{ .compatible = "aptina,mt9p031m", },
> > > > -	{ /* sentinel */ },
> > > > +	{ .compatible = "aptina,mt9p006", .data = (void
> > > *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > +	{ .compatible = "aptina,mt9p031", .data = (void
> > > *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > +	{ .compatible = "aptina,mt9p031m", .data = (void
> > > *)MEDIA_BUS_FMT_Y12_1X12 },
> > > > +	{ /* sentinel */ }
> > >
> > > I know it might sound not necessary, but isn't it better to wrap the
> > > format in some sort of per-model structure. It would avoid a few type
> > > casts too. Up to you though
> >
> > The problem with structure is, it will have one
> > variable entry. I got some feedback related to similar
> > patches not to add a single variable to structure
> > and use the value directly instead.
> >
>
> Ok then, a matter of preferences I think. Up to you, really.

Seems like I forgot to send a tag after your reply!

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>
> Cheers
>   j
>
> > Cheers,
> > Biju
> >
> > >
> > > >  };
> > > >  MODULE_DEVICE_TABLE(of, mt9p031_of_match);
> > > >  #endif
> > > > --
> > > > 2.25.1
> > > >

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

[Laurent Pinchart]

Hello,

On Mon, Sep 11, 2023 at 11:25:03AM +0200, Jacopo Mondi wrote:
> On Mon, Sep 11, 2023 at 09:14:35AM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
> > >
> > > Hi Biju
> > >
> > > On Sun, Sep 10, 2023 at 05:01:25PM +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 MEDIA_BUS_FMT as match data for both
> > > > these tables and replace the ID lookup table for the match data by
> > > > i2c_get_match_data() and simplifly probe() and mt9p031_init_cfg()
> > > >
> > > > Drop mt9p031_init_cfg 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@bp.renesas.com>
> > > > ---
> > > >  drivers/media/i2c/mt9p031.c | 33 +++++++++++----------------------
> > > >  1 file changed, 11 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > > > index 348f1e1098fb..540cb519915c 100644
> > > > --- a/drivers/media/i2c/mt9p031.c
> > > > +++ b/drivers/media/i2c/mt9p031.c
> > > > @@ -112,11 +112,6 @@
> > > >  #define MT9P031_TEST_PATTERN_RED			0xa2
> > > >  #define MT9P031_TEST_PATTERN_BLUE			0xa3
> > > >
> > > > -enum mt9p031_model {
> > > > -	MT9P031_MODEL_COLOR,
> > > > -	MT9P031_MODEL_MONOCHROME,
> > > > -};
> > > > -
> > > >  struct mt9p031 {
> > > >  	struct v4l2_subdev subdev;
> > > >  	struct media_pad pad;
> > > > @@ -129,7 +124,7 @@ struct mt9p031 {
> > > >  	struct clk *clk;
> > > >  	struct regulator_bulk_data regulators[3];
> > > >
> > > > -	enum mt9p031_model model;
> > > > +	u32 code;
> > > >  	struct aptina_pll pll;
> > > >  	unsigned int clk_div;
> > > >  	bool use_pll;
> > > > @@ -714,12 +709,7 @@ static int mt9p031_init_cfg(struct v4l2_subdev *subdev,
> > > >  	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
> > > >
> > > >  	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0, which);
> > > > -
> > > > -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> > > > -		format->code = MEDIA_BUS_FMT_Y12_1X12;
> > > > -	else
> > > > -		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> > > > -
> > > > +	format->code = mt9p031->code;
> > > >  	format->width = MT9P031_WINDOW_WIDTH_DEF;
> > > >  	format->height = MT9P031_WINDOW_HEIGHT_DEF;
> > > >  	format->field = V4L2_FIELD_NONE;
> > > > @@ -1104,7 +1094,6 @@ mt9p031_get_pdata(struct i2c_client *client)
> > > >
> > > >  static int mt9p031_probe(struct i2c_client *client)  {
> > > > -	const struct i2c_device_id *did = i2c_client_get_device_id(client);
> > > >  	struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
> > > >  	struct i2c_adapter *adapter = client->adapter;
> > > >  	struct mt9p031 *mt9p031;
> > > > @@ -1129,7 +1118,7 @@ static int mt9p031_probe(struct i2c_client *client)
> > > >  	mt9p031->pdata = pdata;
> > > >  	mt9p031->output_control	= MT9P031_OUTPUT_CONTROL_DEF;
> > > >  	mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
> > > > -	mt9p031->model = did->driver_data;
> > > > +	mt9p031->code = (uintptr_t)i2c_get_match_data(client);
> > > >
> > > >  	mt9p031->regulators[0].supply = "vdd";
> > > >  	mt9p031->regulators[1].supply = "vdd_io";
> > > > @@ -1226,19 +1215,19 @@ static void mt9p031_remove(struct i2c_client *client)
> > > >  }
> > > >
> > > >  static const struct i2c_device_id mt9p031_id[] = {
> > > > -	{ "mt9p006", MT9P031_MODEL_COLOR },
> > > > -	{ "mt9p031", MT9P031_MODEL_COLOR },
> > > > -	{ "mt9p031m", MT9P031_MODEL_MONOCHROME },
> > > > -	{ }
> > > > +	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > +	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > +	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
> > > > +	{ /* sentinel */ }
> > > >  };
> > > >  MODULE_DEVICE_TABLE(i2c, mt9p031_id);
> > > >
> > > >  #if IS_ENABLED(CONFIG_OF)
> > > >  static const struct of_device_id mt9p031_of_match[] = {
> > > > -	{ .compatible = "aptina,mt9p006", },
> > > > -	{ .compatible = "aptina,mt9p031", },
> > > > -	{ .compatible = "aptina,mt9p031m", },
> > > > -	{ /* sentinel */ },
> > > > +	{ .compatible = "aptina,mt9p006", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > +	{ .compatible = "aptina,mt9p031", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > +	{ .compatible = "aptina,mt9p031m", .data = (void *)MEDIA_BUS_FMT_Y12_1X12 },
> > > > +	{ /* sentinel */ }
> > >
> > > I know it might sound not necessary, but isn't it better to wrap the
> > > format in some sort of per-model structure. It would avoid a few type
> > > casts too. Up to you though
> >
> > The problem with structure is, it will have one
> > variable entry. I got some feedback related to similar
> > patches not to add a single variable to structure
> > and use the value directly instead.
> 
> Ok then, a matter of preferences I think. Up to you, really.

My preference actually goes for a structure too.

Given how long it took me to review this, I'll send a v2 of the series
myself.

> > > >  };
> > > >  MODULE_DEVICE_TABLE(of, mt9p031_of_match);
> > > >  #endif

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Fri, Aug 23, 2024 at 06:55:04PM +0300, Laurent Pinchart wrote:
> Hello,
> 
> On Mon, Sep 11, 2023 at 11:25:03AM +0200, Jacopo Mondi wrote:
> > On Mon, Sep 11, 2023 at 09:14:35AM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
> > > >
> > > > Hi Biju
> > > >
> > > > On Sun, Sep 10, 2023 at 05:01:25PM +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 MEDIA_BUS_FMT as match data for both
> > > > > these tables and replace the ID lookup table for the match data by
> > > > > i2c_get_match_data() and simplifly probe() and mt9p031_init_cfg()
> > > > >
> > > > > Drop mt9p031_init_cfg 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@bp.renesas.com>
> > > > > ---
> > > > >  drivers/media/i2c/mt9p031.c | 33 +++++++++++----------------------
> > > > >  1 file changed, 11 insertions(+), 22 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > > > > index 348f1e1098fb..540cb519915c 100644
> > > > > --- a/drivers/media/i2c/mt9p031.c
> > > > > +++ b/drivers/media/i2c/mt9p031.c
> > > > > @@ -112,11 +112,6 @@
> > > > >  #define MT9P031_TEST_PATTERN_RED			0xa2
> > > > >  #define MT9P031_TEST_PATTERN_BLUE			0xa3
> > > > >
> > > > > -enum mt9p031_model {
> > > > > -	MT9P031_MODEL_COLOR,
> > > > > -	MT9P031_MODEL_MONOCHROME,
> > > > > -};
> > > > > -
> > > > >  struct mt9p031 {
> > > > >  	struct v4l2_subdev subdev;
> > > > >  	struct media_pad pad;
> > > > > @@ -129,7 +124,7 @@ struct mt9p031 {
> > > > >  	struct clk *clk;
> > > > >  	struct regulator_bulk_data regulators[3];
> > > > >
> > > > > -	enum mt9p031_model model;
> > > > > +	u32 code;
> > > > >  	struct aptina_pll pll;
> > > > >  	unsigned int clk_div;
> > > > >  	bool use_pll;
> > > > > @@ -714,12 +709,7 @@ static int mt9p031_init_cfg(struct v4l2_subdev *subdev,
> > > > >  	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
> > > > >
> > > > >  	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0, which);
> > > > > -
> > > > > -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> > > > > -		format->code = MEDIA_BUS_FMT_Y12_1X12;
> > > > > -	else
> > > > > -		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> > > > > -
> > > > > +	format->code = mt9p031->code;
> > > > >  	format->width = MT9P031_WINDOW_WIDTH_DEF;
> > > > >  	format->height = MT9P031_WINDOW_HEIGHT_DEF;
> > > > >  	format->field = V4L2_FIELD_NONE;
> > > > > @@ -1104,7 +1094,6 @@ mt9p031_get_pdata(struct i2c_client *client)
> > > > >
> > > > >  static int mt9p031_probe(struct i2c_client *client)  {
> > > > > -	const struct i2c_device_id *did = i2c_client_get_device_id(client);
> > > > >  	struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
> > > > >  	struct i2c_adapter *adapter = client->adapter;
> > > > >  	struct mt9p031 *mt9p031;
> > > > > @@ -1129,7 +1118,7 @@ static int mt9p031_probe(struct i2c_client *client)
> > > > >  	mt9p031->pdata = pdata;
> > > > >  	mt9p031->output_control	= MT9P031_OUTPUT_CONTROL_DEF;
> > > > >  	mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
> > > > > -	mt9p031->model = did->driver_data;
> > > > > +	mt9p031->code = (uintptr_t)i2c_get_match_data(client);
> > > > >
> > > > >  	mt9p031->regulators[0].supply = "vdd";
> > > > >  	mt9p031->regulators[1].supply = "vdd_io";
> > > > > @@ -1226,19 +1215,19 @@ static void mt9p031_remove(struct i2c_client *client)
> > > > >  }
> > > > >
> > > > >  static const struct i2c_device_id mt9p031_id[] = {
> > > > > -	{ "mt9p006", MT9P031_MODEL_COLOR },
> > > > > -	{ "mt9p031", MT9P031_MODEL_COLOR },
> > > > > -	{ "mt9p031m", MT9P031_MODEL_MONOCHROME },
> > > > > -	{ }
> > > > > +	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > +	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > +	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
> > > > > +	{ /* sentinel */ }
> > > > >  };
> > > > >  MODULE_DEVICE_TABLE(i2c, mt9p031_id);
> > > > >
> > > > >  #if IS_ENABLED(CONFIG_OF)
> > > > >  static const struct of_device_id mt9p031_of_match[] = {
> > > > > -	{ .compatible = "aptina,mt9p006", },
> > > > > -	{ .compatible = "aptina,mt9p031", },
> > > > > -	{ .compatible = "aptina,mt9p031m", },
> > > > > -	{ /* sentinel */ },
> > > > > +	{ .compatible = "aptina,mt9p006", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > +	{ .compatible = "aptina,mt9p031", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > +	{ .compatible = "aptina,mt9p031m", .data = (void *)MEDIA_BUS_FMT_Y12_1X12 },
> > > > > +	{ /* sentinel */ }
> > > >
> > > > I know it might sound not necessary, but isn't it better to wrap the
> > > > format in some sort of per-model structure. It would avoid a few type
> > > > casts too. Up to you though
> > >
> > > The problem with structure is, it will have one
> > > variable entry. I got some feedback related to similar
> > > patches not to add a single variable to structure
> > > and use the value directly instead.
> > 
> > Ok then, a matter of preferences I think. Up to you, really.
> 
> My preference actually goes for a structure too.
> 
> Given how long it took me to review this, I'll send a v2 of the series
> myself.

Actually it will be simpler to apply this series first and add changes
on top.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> > > > >  };
> > > > >  MODULE_DEVICE_TABLE(of, mt9p031_of_match);
> > > > >  #endif

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Sat, Aug 24, 2024 at 06:55:19PM +0300, Laurent Pinchart wrote:
> On Fri, Aug 23, 2024 at 06:55:04PM +0300, Laurent Pinchart wrote:
> > Hello,
> > 
> > On Mon, Sep 11, 2023 at 11:25:03AM +0200, Jacopo Mondi wrote:
> > > On Mon, Sep 11, 2023 at 09:14:35AM +0000, Biju Das wrote:
> > > > > Subject: Re: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
> > > > >
> > > > > Hi Biju
> > > > >
> > > > > On Sun, Sep 10, 2023 at 05:01:25PM +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 MEDIA_BUS_FMT as match data for both
> > > > > > these tables and replace the ID lookup table for the match data by
> > > > > > i2c_get_match_data() and simplifly probe() and mt9p031_init_cfg()
> > > > > >
> > > > > > Drop mt9p031_init_cfg as there is no user.
> > > > > >

I'll drop this sentence when applying.

> > > > > > 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@bp.renesas.com>
> > > > > > ---
> > > > > >  drivers/media/i2c/mt9p031.c | 33 +++++++++++----------------------
> > > > > >  1 file changed, 11 insertions(+), 22 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > > > > > index 348f1e1098fb..540cb519915c 100644
> > > > > > --- a/drivers/media/i2c/mt9p031.c
> > > > > > +++ b/drivers/media/i2c/mt9p031.c
> > > > > > @@ -112,11 +112,6 @@
> > > > > >  #define MT9P031_TEST_PATTERN_RED			0xa2
> > > > > >  #define MT9P031_TEST_PATTERN_BLUE			0xa3
> > > > > >
> > > > > > -enum mt9p031_model {
> > > > > > -	MT9P031_MODEL_COLOR,
> > > > > > -	MT9P031_MODEL_MONOCHROME,
> > > > > > -};
> > > > > > -
> > > > > >  struct mt9p031 {
> > > > > >  	struct v4l2_subdev subdev;
> > > > > >  	struct media_pad pad;
> > > > > > @@ -129,7 +124,7 @@ struct mt9p031 {
> > > > > >  	struct clk *clk;
> > > > > >  	struct regulator_bulk_data regulators[3];
> > > > > >
> > > > > > -	enum mt9p031_model model;
> > > > > > +	u32 code;
> > > > > >  	struct aptina_pll pll;
> > > > > >  	unsigned int clk_div;
> > > > > >  	bool use_pll;
> > > > > > @@ -714,12 +709,7 @@ static int mt9p031_init_cfg(struct v4l2_subdev *subdev,
> > > > > >  	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
> > > > > >
> > > > > >  	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0, which);
> > > > > > -
> > > > > > -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> > > > > > -		format->code = MEDIA_BUS_FMT_Y12_1X12;
> > > > > > -	else
> > > > > > -		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> > > > > > -
> > > > > > +	format->code = mt9p031->code;
> > > > > >  	format->width = MT9P031_WINDOW_WIDTH_DEF;
> > > > > >  	format->height = MT9P031_WINDOW_HEIGHT_DEF;
> > > > > >  	format->field = V4L2_FIELD_NONE;
> > > > > > @@ -1104,7 +1094,6 @@ mt9p031_get_pdata(struct i2c_client *client)
> > > > > >
> > > > > >  static int mt9p031_probe(struct i2c_client *client)  {
> > > > > > -	const struct i2c_device_id *did = i2c_client_get_device_id(client);
> > > > > >  	struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
> > > > > >  	struct i2c_adapter *adapter = client->adapter;
> > > > > >  	struct mt9p031 *mt9p031;
> > > > > > @@ -1129,7 +1118,7 @@ static int mt9p031_probe(struct i2c_client *client)
> > > > > >  	mt9p031->pdata = pdata;
> > > > > >  	mt9p031->output_control	= MT9P031_OUTPUT_CONTROL_DEF;
> > > > > >  	mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
> > > > > > -	mt9p031->model = did->driver_data;
> > > > > > +	mt9p031->code = (uintptr_t)i2c_get_match_data(client);
> > > > > >
> > > > > >  	mt9p031->regulators[0].supply = "vdd";
> > > > > >  	mt9p031->regulators[1].supply = "vdd_io";
> > > > > > @@ -1226,19 +1215,19 @@ static void mt9p031_remove(struct i2c_client *client)
> > > > > >  }
> > > > > >
> > > > > >  static const struct i2c_device_id mt9p031_id[] = {
> > > > > > -	{ "mt9p006", MT9P031_MODEL_COLOR },
> > > > > > -	{ "mt9p031", MT9P031_MODEL_COLOR },
> > > > > > -	{ "mt9p031m", MT9P031_MODEL_MONOCHROME },
> > > > > > -	{ }
> > > > > > +	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > > +	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > > +	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
> > > > > > +	{ /* sentinel */ }
> > > > > >  };
> > > > > >  MODULE_DEVICE_TABLE(i2c, mt9p031_id);
> > > > > >
> > > > > >  #if IS_ENABLED(CONFIG_OF)
> > > > > >  static const struct of_device_id mt9p031_of_match[] = {
> > > > > > -	{ .compatible = "aptina,mt9p006", },
> > > > > > -	{ .compatible = "aptina,mt9p031", },
> > > > > > -	{ .compatible = "aptina,mt9p031m", },
> > > > > > -	{ /* sentinel */ },
> > > > > > +	{ .compatible = "aptina,mt9p006", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > > +	{ .compatible = "aptina,mt9p031", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > > +	{ .compatible = "aptina,mt9p031m", .data = (void *)MEDIA_BUS_FMT_Y12_1X12 },
> > > > > > +	{ /* sentinel */ }
> > > > >
> > > > > I know it might sound not necessary, but isn't it better to wrap the
> > > > > format in some sort of per-model structure. It would avoid a few type
> > > > > casts too. Up to you though
> > > >
> > > > The problem with structure is, it will have one
> > > > variable entry. I got some feedback related to similar
> > > > patches not to add a single variable to structure
> > > > and use the value directly instead.
> > > 
> > > Ok then, a matter of preferences I think. Up to you, really.
> > 
> > My preference actually goes for a structure too.
> > 
> > Given how long it took me to review this, I'll send a v2 of the series
> > myself.
> 
> Actually it will be simpler to apply this series first and add changes
> on top.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> > > > > >  };
> > > > > >  MODULE_DEVICE_TABLE(of, mt9p031_of_match);
> > > > > >  #endif

-- 
Regards,

Laurent Pinchart

RE: [PATCH 1/2] media: mt9p031: Extend match support for OF tables

[Biju Das]

Hi Laurent,

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Saturday, August 24, 2024 4:57 PM
> To: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Cc: Biju Das <biju.das.jz@bp.renesas.com>; Sakari Ailus <sakari.ailus@linux.intel.com>; Mauro Carvalho
> Chehab <mchehab@kernel.org>; linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; biju.das.au
> <biju.das.au@gmail.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Subject: Re: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
> 
> On Sat, Aug 24, 2024 at 06:55:19PM +0300, Laurent Pinchart wrote:
> > On Fri, Aug 23, 2024 at 06:55:04PM +0300, Laurent Pinchart wrote:
> > > Hello,
> > >
> > > On Mon, Sep 11, 2023 at 11:25:03AM +0200, Jacopo Mondi wrote:
> > > > On Mon, Sep 11, 2023 at 09:14:35AM +0000, Biju Das wrote:
> > > > > > Subject: Re: [PATCH 1/2] media: mt9p031: Extend match support
> > > > > > for OF tables
> > > > > >
> > > > > > Hi Biju
> > > > > >
> > > > > > On Sun, Sep 10, 2023 at 05:01:25PM +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 MEDIA_BUS_FMT as match data
> > > > > > > for both these tables and replace the ID lookup table for
> > > > > > > the match data by
> > > > > > > i2c_get_match_data() and simplifly probe() and
> > > > > > > mt9p031_init_cfg()
> > > > > > >
> > > > > > > Drop mt9p031_init_cfg as there is no user.
> > > > > > >
> 
> I'll drop this sentence when applying.

OK for me.

Thanks,
Biju

> 
> > > > > > > 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@bp.renesas.com>
> > > > > > > ---
> > > > > > >  drivers/media/i2c/mt9p031.c | 33
> > > > > > > +++++++++++----------------------
> > > > > > >  1 file changed, 11 insertions(+), 22 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/mt9p031.c
> > > > > > > b/drivers/media/i2c/mt9p031.c index
> > > > > > > 348f1e1098fb..540cb519915c 100644
> > > > > > > --- a/drivers/media/i2c/mt9p031.c
> > > > > > > +++ b/drivers/media/i2c/mt9p031.c
> > > > > > > @@ -112,11 +112,6 @@
> > > > > > >  #define MT9P031_TEST_PATTERN_RED			0xa2
> > > > > > >  #define MT9P031_TEST_PATTERN_BLUE			0xa3
> > > > > > >
> > > > > > > -enum mt9p031_model {
> > > > > > > -	MT9P031_MODEL_COLOR,
> > > > > > > -	MT9P031_MODEL_MONOCHROME,
> > > > > > > -};
> > > > > > > -
> > > > > > >  struct mt9p031 {
> > > > > > >  	struct v4l2_subdev subdev;
> > > > > > >  	struct media_pad pad;
> > > > > > > @@ -129,7 +124,7 @@ struct mt9p031 {
> > > > > > >  	struct clk *clk;
> > > > > > >  	struct regulator_bulk_data regulators[3];
> > > > > > >
> > > > > > > -	enum mt9p031_model model;
> > > > > > > +	u32 code;
> > > > > > >  	struct aptina_pll pll;
> > > > > > >  	unsigned int clk_div;
> > > > > > >  	bool use_pll;
> > > > > > > @@ -714,12 +709,7 @@ static int mt9p031_init_cfg(struct v4l2_subdev *subdev,
> > > > > > >  	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
> > > > > > >
> > > > > > >  	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0,
> > > > > > > which);
> > > > > > > -
> > > > > > > -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> > > > > > > -		format->code = MEDIA_BUS_FMT_Y12_1X12;
> > > > > > > -	else
> > > > > > > -		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> > > > > > > -
> > > > > > > +	format->code = mt9p031->code;
> > > > > > >  	format->width = MT9P031_WINDOW_WIDTH_DEF;
> > > > > > >  	format->height = MT9P031_WINDOW_HEIGHT_DEF;
> > > > > > >  	format->field = V4L2_FIELD_NONE; @@ -1104,7 +1094,6 @@
> > > > > > > mt9p031_get_pdata(struct i2c_client *client)
> > > > > > >
> > > > > > >  static int mt9p031_probe(struct i2c_client *client)  {
> > > > > > > -	const struct i2c_device_id *did = i2c_client_get_device_id(client);
> > > > > > >  	struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
> > > > > > >  	struct i2c_adapter *adapter = client->adapter;
> > > > > > >  	struct mt9p031 *mt9p031;
> > > > > > > @@ -1129,7 +1118,7 @@ static int mt9p031_probe(struct i2c_client *client)
> > > > > > >  	mt9p031->pdata = pdata;
> > > > > > >  	mt9p031->output_control	= MT9P031_OUTPUT_CONTROL_DEF;
> > > > > > >  	mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
> > > > > > > -	mt9p031->model = did->driver_data;
> > > > > > > +	mt9p031->code = (uintptr_t)i2c_get_match_data(client);
> > > > > > >
> > > > > > >  	mt9p031->regulators[0].supply = "vdd";
> > > > > > >  	mt9p031->regulators[1].supply = "vdd_io"; @@ -1226,19
> > > > > > > +1215,19 @@ static void mt9p031_remove(struct i2c_client
> > > > > > > *client)  }
> > > > > > >
> > > > > > >  static const struct i2c_device_id mt9p031_id[] = {
> > > > > > > -	{ "mt9p006", MT9P031_MODEL_COLOR },
> > > > > > > -	{ "mt9p031", MT9P031_MODEL_COLOR },
> > > > > > > -	{ "mt9p031m", MT9P031_MODEL_MONOCHROME },
> > > > > > > -	{ }
> > > > > > > +	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > > > +	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > > > +	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
> > > > > > > +	{ /* sentinel */ }
> > > > > > >  };
> > > > > > >  MODULE_DEVICE_TABLE(i2c, mt9p031_id);
> > > > > > >
> > > > > > >  #if IS_ENABLED(CONFIG_OF)
> > > > > > >  static const struct of_device_id mt9p031_of_match[] = {
> > > > > > > -	{ .compatible = "aptina,mt9p006", },
> > > > > > > -	{ .compatible = "aptina,mt9p031", },
> > > > > > > -	{ .compatible = "aptina,mt9p031m", },
> > > > > > > -	{ /* sentinel */ },
> > > > > > > +	{ .compatible = "aptina,mt9p006", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > > > +	{ .compatible = "aptina,mt9p031", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> > > > > > > +	{ .compatible = "aptina,mt9p031m", .data = (void *)MEDIA_BUS_FMT_Y12_1X12 },
> > > > > > > +	{ /* sentinel */ }
> > > > > >
> > > > > > I know it might sound not necessary, but isn't it better to
> > > > > > wrap the format in some sort of per-model structure. It would
> > > > > > avoid a few type casts too. Up to you though
> > > > >
> > > > > The problem with structure is, it will have one variable entry.
> > > > > I got some feedback related to similar patches not to add a
> > > > > single variable to structure and use the value directly instead.
> > > >
> > > > Ok then, a matter of preferences I think. Up to you, really.
> > >
> > > My preference actually goes for a structure too.
> > >
> > > Given how long it took me to review this, I'll send a v2 of the
> > > series myself.
> >
> > Actually it will be simpler to apply this series first and add changes
> > on top.
> >
> > Reviewed-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> >
> > > > > > >  };
> > > > > > >  MODULE_DEVICE_TABLE(of, mt9p031_of_match);  #endif
> 
> --
> Regards,
> 
> Laurent Pinchart

[PATCH 2/2] media: mt9p031: Drop CONFIG_OF ifdeffery

[Biju Das]

Drop of_match_ptr() from mt9p031_i2c_driver and get rid of ugly CONFIG_OF
if check. This slightly increases the size of mt9p031_i2c_driver on non-OF
system and shouldn't be an issue.

Add mod_devicetable.h include.

It also allows, in case if needed, to enumerate this device via ACPI with
PRP0001 magic.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/i2c/mt9p031.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 540cb519915c..91d5de5b95f0 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -15,6 +15,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/log2.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_graph.h>
@@ -1222,7 +1223,6 @@ static const struct i2c_device_id mt9p031_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, mt9p031_id);
 
-#if IS_ENABLED(CONFIG_OF)
 static const struct of_device_id mt9p031_of_match[] = {
 	{ .compatible = "aptina,mt9p006", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
 	{ .compatible = "aptina,mt9p031", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
@@ -1230,11 +1230,10 @@ static const struct of_device_id mt9p031_of_match[] = {
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, mt9p031_of_match);
-#endif
 
 static struct i2c_driver mt9p031_i2c_driver = {
 	.driver = {
-		.of_match_table = of_match_ptr(mt9p031_of_match),
+		.of_match_table = mt9p031_of_match,
 		.name = "mt9p031",
 	},
 	.probe          = mt9p031_probe,
-- 
2.25.1

Re: [PATCH 2/2] media: mt9p031: Drop CONFIG_OF ifdeffery

[Jacopo Mondi]

Hi Biju

On Sun, Sep 10, 2023 at 05:01:26PM +0100, Biju Das wrote:
> Drop of_match_ptr() from mt9p031_i2c_driver and get rid of ugly CONFIG_OF
> if check. This slightly increases the size of mt9p031_i2c_driver on non-OF
> system and shouldn't be an issue.
>
> Add mod_devicetable.h include.
>
> It also allows, in case if needed, to enumerate this device via ACPI with
> PRP0001 magic.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  drivers/media/i2c/mt9p031.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 540cb519915c..91d5de5b95f0 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -15,6 +15,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/log2.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
> @@ -1222,7 +1223,6 @@ static const struct i2c_device_id mt9p031_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, mt9p031_id);
>
> -#if IS_ENABLED(CONFIG_OF)
>  static const struct of_device_id mt9p031_of_match[] = {
>  	{ .compatible = "aptina,mt9p006", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
>  	{ .compatible = "aptina,mt9p031", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> @@ -1230,11 +1230,10 @@ static const struct of_device_id mt9p031_of_match[] = {
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, mt9p031_of_match);
> -#endif
>
>  static struct i2c_driver mt9p031_i2c_driver = {
>  	.driver = {
> -		.of_match_table = of_match_ptr(mt9p031_of_match),
> +		.of_match_table = mt9p031_of_match,
>  		.name = "mt9p031",
>  	},
>  	.probe          = mt9p031_probe,
> --
> 2.25.1
>

Re: [PATCH 2/2] media: mt9p031: Drop CONFIG_OF ifdeffery

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Sun, Sep 10, 2023 at 05:01:26PM +0100, Biju Das wrote:
> Drop of_match_ptr() from mt9p031_i2c_driver and get rid of ugly CONFIG_OF
> if check. This slightly increases the size of mt9p031_i2c_driver on non-OF
> system and shouldn't be an issue.
> 
> Add mod_devicetable.h include.
> 
> It also allows, in case if needed, to enumerate this device via ACPI with
> PRP0001 magic.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/mt9p031.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 540cb519915c..91d5de5b95f0 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -15,6 +15,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/log2.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
> @@ -1222,7 +1223,6 @@ static const struct i2c_device_id mt9p031_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, mt9p031_id);
>  
> -#if IS_ENABLED(CONFIG_OF)
>  static const struct of_device_id mt9p031_of_match[] = {
>  	{ .compatible = "aptina,mt9p006", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
>  	{ .compatible = "aptina,mt9p031", .data = (void *)MEDIA_BUS_FMT_SGRBG12_1X12 },
> @@ -1230,11 +1230,10 @@ static const struct of_device_id mt9p031_of_match[] = {
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, mt9p031_of_match);
> -#endif
>  
>  static struct i2c_driver mt9p031_i2c_driver = {
>  	.driver = {
> -		.of_match_table = of_match_ptr(mt9p031_of_match),
> +		.of_match_table = mt9p031_of_match,
>  		.name = "mt9p031",
>  	},
>  	.probe          = mt9p031_probe,

-- 
Regards,

Laurent Pinchart