All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Match data improvements for mt9p031 driver
@ 2023-09-10 16:01 Biju Das
  2023-09-10 16:01 ` [PATCH 1/2] media: mt9p031: Extend match support for OF tables Biju Das
  2023-09-10 16:01 ` [PATCH 2/2] media: mt9p031: Drop CONFIG_OF ifdeffery Biju Das
  0 siblings, 2 replies; 8+ messages in thread
From: Biju Das @ 2023-09-10 16:01 UTC (permalink / raw)
  To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab
  Cc: Biju Das, linux-media, linux-kernel, Biju Das, Andy Shevchenko

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


^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH 1/2] media: mt9p031: Extend match support for OF tables
  2023-09-10 16:01 [PATCH 0/2] Match data improvements for mt9p031 driver Biju Das
@ 2023-09-10 16:01 ` Biju Das
  2023-09-11  9:06   ` Jacopo Mondi
  2023-09-10 16:01 ` [PATCH 2/2] media: mt9p031: Drop CONFIG_OF ifdeffery Biju Das
  1 sibling, 1 reply; 8+ messages in thread
From: Biju Das @ 2023-09-10 16:01 UTC (permalink / raw)
  To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab
  Cc: Biju Das, linux-media, linux-kernel, Biju Das, Andy Shevchenko

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH 2/2] media: mt9p031: Drop CONFIG_OF ifdeffery
  2023-09-10 16:01 [PATCH 0/2] Match data improvements for mt9p031 driver Biju Das
  2023-09-10 16:01 ` [PATCH 1/2] media: mt9p031: Extend match support for OF tables Biju Das
@ 2023-09-10 16:01 ` Biju Das
  2023-09-11  9:07   ` Jacopo Mondi
  1 sibling, 1 reply; 8+ messages in thread
From: Biju Das @ 2023-09-10 16:01 UTC (permalink / raw)
  To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab
  Cc: Biju Das, linux-media, linux-kernel, Biju Das, Andy Shevchenko

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
  2023-09-10 16:01 ` [PATCH 1/2] media: mt9p031: Extend match support for OF tables Biju Das
@ 2023-09-11  9:06   ` Jacopo Mondi
  2023-09-11  9:14     ` Biju Das
  0 siblings, 1 reply; 8+ messages in thread
From: Jacopo Mondi @ 2023-09-11  9:06 UTC (permalink / raw)
  To: Biju Das
  Cc: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab,
	linux-media, linux-kernel, Biju Das, Andy Shevchenko

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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] media: mt9p031: Drop CONFIG_OF ifdeffery
  2023-09-10 16:01 ` [PATCH 2/2] media: mt9p031: Drop CONFIG_OF ifdeffery Biju Das
@ 2023-09-11  9:07   ` Jacopo Mondi
  0 siblings, 0 replies; 8+ messages in thread
From: Jacopo Mondi @ 2023-09-11  9:07 UTC (permalink / raw)
  To: Biju Das
  Cc: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab,
	linux-media, linux-kernel, Biju Das, Andy Shevchenko

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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread
* RE: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
  2023-09-11  9:06   ` Jacopo Mondi
@ 2023-09-11  9:14     ` Biju Das
  2023-09-11  9:25       ` Jacopo Mondi
  0 siblings, 1 reply; 8+ messages in thread
From: Biju Das @ 2023-09-11  9:14 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab,
	linux-media, linux-kernel, Biju Das, Andy Shevchenko

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
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
  2023-09-11  9:14     ` Biju Das
@ 2023-09-11  9:25       ` Jacopo Mondi
  2023-09-13 13:45         ` Jacopo Mondi
  0 siblings, 1 reply; 8+ messages in thread
From: Jacopo Mondi @ 2023-09-11  9:25 UTC (permalink / raw)
  To: Biju Das
  Cc: Jacopo Mondi, Laurent Pinchart, Sakari Ailus,
	Mauro Carvalho Chehab, linux-media, linux-kernel, Biju Das,
	Andy Shevchenko

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
> > >

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: mt9p031: Extend match support for OF tables
  2023-09-11  9:25       ` Jacopo Mondi
@ 2023-09-13 13:45         ` Jacopo Mondi
  0 siblings, 0 replies; 8+ messages in thread
From: Jacopo Mondi @ 2023-09-13 13:45 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Biju Das, Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab,
	linux-media, linux-kernel, Biju Das, Andy Shevchenko

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
> > > >

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-09-13 13:45 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-10 16:01 [PATCH 0/2] Match data improvements for mt9p031 driver Biju Das
2023-09-10 16:01 ` [PATCH 1/2] media: mt9p031: Extend match support for OF tables Biju Das
2023-09-11  9:06   ` Jacopo Mondi
2023-09-11  9:14     ` Biju Das
2023-09-11  9:25       ` Jacopo Mondi
2023-09-13 13:45         ` Jacopo Mondi
2023-09-10 16:01 ` [PATCH 2/2] media: mt9p031: Drop CONFIG_OF ifdeffery Biju Das
2023-09-11  9:07   ` Jacopo Mondi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.