1. Patchew
  2. linux
  3. View series

[PATCH] media: mt9p031: Refactor format handling for different sensor models

Tarang Raval posted 1 patch 1 month ago
There is a newer version of this series
drivers/media/i2c/mt9p031.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
[PATCH] media: mt9p031: Refactor format handling for different sensor models
Posted by Tarang Raval 1 month ago 
Add new structure 'mt9p031_model_info' to encapsulate format codes for
the mt9p031 camera sensor family. This approach enhances code clarity
and maintainability.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/mt9p031.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index d8735c246e52..d4fcc692311c 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -113,6 +113,16 @@
 #define MT9P031_TEST_PATTERN_RED			0xa2
 #define MT9P031_TEST_PATTERN_BLUE			0xa3
 
+struct mt9p031_model_info {
+       u32 code;
+};
+
+static const struct mt9p031_model_info mt9p031_models[] = {
+	{.code = MEDIA_BUS_FMT_SGRBG12_1X12}, /* mt9p006  */
+	{.code = MEDIA_BUS_FMT_SGRBG12_1X12}, /* mt9p031  */
+	{.code = MEDIA_BUS_FMT_Y12_1X12},     /* mt9p031m */
+};
+
 struct mt9p031 {
 	struct v4l2_subdev subdev;
 	struct media_pad pad;
@@ -125,7 +135,7 @@ struct mt9p031 {
 	struct clk *clk;
 	struct regulator_bulk_data regulators[3];
 
-	u32 code;
+	const struct mt9p031_model_info *model;
 	struct aptina_pll pll;
 	unsigned int clk_div;
 	bool use_pll;
@@ -708,7 +718,7 @@ static int mt9p031_init_state(struct v4l2_subdev *subdev,
 	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
 
 	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0, which);
-	format->code = mt9p031->code;
+	format->code = mt9p031->model->code;
 	format->width = MT9P031_WINDOW_WIDTH_DEF;
 	format->height = MT9P031_WINDOW_HEIGHT_DEF;
 	format->field = V4L2_FIELD_NONE;
@@ -1117,7 +1127,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->code = (uintptr_t)i2c_get_match_data(client);
+	mt9p031->model = &mt9p031_models[(uintptr_t)i2c_get_match_data(client)];
 
 	mt9p031->regulators[0].supply = "vdd";
 	mt9p031->regulators[1].supply = "vdd_io";
@@ -1214,17 +1224,17 @@ static void mt9p031_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id mt9p031_id[] = {
-	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
-	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
-	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
+	{ "mt9p006", 0 },
+	{ "mt9p031", 1 },
+	{ "mt9p031m", 2 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, mt9p031_id);
 
 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 },
-	{ .compatible = "aptina,mt9p031m", .data = (void *)MEDIA_BUS_FMT_Y12_1X12 },
+	{ .compatible = "aptina,mt9p006", .data = (void *)0 },
+	{ .compatible = "aptina,mt9p031", .data = (void *)1 },
+	{ .compatible = "aptina,mt9p031m", .data = (void *)2 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, mt9p031_of_match);
-- 
2.43.0
Re: [PATCH] media: mt9p031: Refactor format handling for different sensor models
Posted by Laurent Pinchart 1 month ago 
Hi Tarang,

Thank you for the patch.

On Fri, Oct 25, 2024 at 06:32:17PM +0530, Tarang Raval wrote:
> Add new structure 'mt9p031_model_info' to encapsulate format codes for
> the mt9p031 camera sensor family. This approach enhances code clarity
> and maintainability.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>  drivers/media/i2c/mt9p031.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index d8735c246e52..d4fcc692311c 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -113,6 +113,16 @@
>  #define MT9P031_TEST_PATTERN_RED			0xa2
>  #define MT9P031_TEST_PATTERN_BLUE			0xa3
>  
> +struct mt9p031_model_info {
> +       u32 code;
> +};
> +
> +static const struct mt9p031_model_info mt9p031_models[] = {
> +	{.code = MEDIA_BUS_FMT_SGRBG12_1X12}, /* mt9p006  */

There should be spaces after { and before }

> +	{.code = MEDIA_BUS_FMT_SGRBG12_1X12}, /* mt9p031  */

You can use the same entry for both the MT9P006 and MT9P031 as they
don't need to be deferentiated.

> +	{.code = MEDIA_BUS_FMT_Y12_1X12},     /* mt9p031m */
> +};
> +
>  struct mt9p031 {
>  	struct v4l2_subdev subdev;
>  	struct media_pad pad;
> @@ -125,7 +135,7 @@ struct mt9p031 {
>  	struct clk *clk;
>  	struct regulator_bulk_data regulators[3];
>  
> -	u32 code;
> +	const struct mt9p031_model_info *model;
>  	struct aptina_pll pll;
>  	unsigned int clk_div;
>  	bool use_pll;
> @@ -708,7 +718,7 @@ static int mt9p031_init_state(struct v4l2_subdev *subdev,
>  	crop->height = MT9P031_WINDOW_HEIGHT_DEF;
>  
>  	format = __mt9p031_get_pad_format(mt9p031, sd_state, 0, which);
> -	format->code = mt9p031->code;
> +	format->code = mt9p031->model->code;
>  	format->width = MT9P031_WINDOW_WIDTH_DEF;
>  	format->height = MT9P031_WINDOW_HEIGHT_DEF;
>  	format->field = V4L2_FIELD_NONE;
> @@ -1117,7 +1127,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->code = (uintptr_t)i2c_get_match_data(client);
> +	mt9p031->model = &mt9p031_models[(uintptr_t)i2c_get_match_data(client)];
>  
>  	mt9p031->regulators[0].supply = "vdd";
>  	mt9p031->regulators[1].supply = "vdd_io";
> @@ -1214,17 +1224,17 @@ static void mt9p031_remove(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id mt9p031_id[] = {
> -	{ "mt9p006", MEDIA_BUS_FMT_SGRBG12_1X12 },
> -	{ "mt9p031", MEDIA_BUS_FMT_SGRBG12_1X12 },
> -	{ "mt9p031m", MEDIA_BUS_FMT_Y12_1X12 },
> +	{ "mt9p006", 0 },
> +	{ "mt9p031", 1 },
> +	{ "mt9p031m", 2 },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(i2c, mt9p031_id);

I think we can drop mt9p031_id. I'll send a patch series to do so.

>  
>  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 },
> -	{ .compatible = "aptina,mt9p031m", .data = (void *)MEDIA_BUS_FMT_Y12_1X12 },
> +	{ .compatible = "aptina,mt9p006", .data = (void *)0 },
> +	{ .compatible = "aptina,mt9p031", .data = (void *)1 },
> +	{ .compatible = "aptina,mt9p031m", .data = (void *)2 },

Let's avoid magic values. You can write

	{ .compatible = "aptina,mt9p006", .data = &mt9p031_models[0] },
	{ .compatible = "aptina,mt9p031", .data = &mt9p031_models[0] },
	{ .compatible = "aptina,mt9p031m", .data = &mt9p031_models[1] },

but it may be even more readable to introduce a

enum mt9p031_model {
	MT9P031_MODEL_BAYER,
	MT9P031_MODEL_MONO,
};

static const struct mt9p031_model_info mt9p031_models[] = {
	[MT9P031_MODEL_BAYER] = {
		.code = MEDIA_BUS_FMT_SGRBG12_1X12,
	},
	[MT9P031_MODEL_MONO] = {
		.code = MEDIA_BUS_FMT_Y12_1X12,
	},
};

static const struct of_device_id mt9p031_of_match[] = {
	{
		.compatible = "aptina,mt9p006",
		.data = &mt9p031_models[MT9P031_MODEL_BAYER],
	}, {
		.compatible = "aptina,mt9p031",
		.data = &mt9p031_models[MT9P031_MODEL_BAYER],
	}, {
		.compatible = "aptina,mt9p031m",
		.data = &mt9p031_models[MEDIA_BUS_FMT_Y12_1X12],
	},
	{ /* sentinel */ }
};

>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, mt9p031_of_match);

-- 
Regards,

Laurent Pinchart

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.