[PATCH 0/2] Add support for mono version of Sony IMX290 sensor

[PATCH 0/2] Add support for mono version of Sony IMX290 sensor

[Dave Stevenson]

The IMX290 comes in both mono and colour variants, but has no runtime way
of determining which is connected.
These patches adds support for either option.

Dave Stevenson (2):
  media: dt-bindings: media: i2c: Add mono version to IMX290 bindings
  media: i2c: imx290: Add support for the mono sensor variant.

 .../bindings/media/i2c/sony,imx290.yaml       |  8 ++--
 drivers/media/i2c/imx290.c                    | 47 ++++++++++++-------
 2 files changed, 35 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/2] media: dt-bindings: media: i2c: Add mono version to IMX290 bindings

[Dave Stevenson]

The IMX290 module is available as either monochrome or colour and
the variant is not detectable at runtime.

Add a new compatible string for the monochrome version.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 .../devicetree/bindings/media/i2c/sony,imx290.yaml        | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
index 21377daae026..29ca4052591f 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
@@ -12,15 +12,17 @@ maintainers:
 
 description: |-
   The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with Square
-  Pixel for Color Cameras. It is programmable through I2C and 4-wire
-  interfaces. The sensor output is available via CMOS logic parallel SDR
-  output, Low voltage LVDS DDR output and CSI-2 serial data output. The CSI-2
+  Pixel, available in either mono or colour variants.
+  It is programmable through I2C and 4-wire interfaces.
+  The sensor output is available via CMOS logic parallel SDR output, Low
+  voltage LVDS DDR output and CSI-2 serial data output. The CSI-2
   bus is the default. No bindings have been defined for the other busses.
 
 properties:
   compatible:
     enum:
       - sony,imx290
+      - sony,imx290-mono
 
   reg:
     maxItems: 1
-- 
2.34.1

Re: [PATCH 1/2] media: dt-bindings: media: i2c: Add mono version to IMX290 bindings

[Rob Herring]

On Tue, 31 Jan 2023 19:06:59 +0000, Dave Stevenson wrote:
> The IMX290 module is available as either monochrome or colour and
> the variant is not detectable at runtime.
> 
> Add a new compatible string for the monochrome version.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  .../devicetree/bindings/media/i2c/sony,imx290.yaml        | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/2] media: dt-bindings: media: i2c: Add mono version to IMX290 bindings

[Laurent Pinchart]

Hi Dave,

Thank you for the patch.

On Tue, Jan 31, 2023 at 07:06:59PM +0000, Dave Stevenson wrote:
> The IMX290 module is available as either monochrome or colour and
> the variant is not detectable at runtime.
> 
> Add a new compatible string for the monochrome version.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  .../devicetree/bindings/media/i2c/sony,imx290.yaml        | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
> index 21377daae026..29ca4052591f 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
> @@ -12,15 +12,17 @@ maintainers:
>  
>  description: |-
>    The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with Square
> -  Pixel for Color Cameras. It is programmable through I2C and 4-wire
> -  interfaces. The sensor output is available via CMOS logic parallel SDR
> -  output, Low voltage LVDS DDR output and CSI-2 serial data output. The CSI-2
> +  Pixel, available in either mono or colour variants.
> +  It is programmable through I2C and 4-wire interfaces.
> +  The sensor output is available via CMOS logic parallel SDR output, Low
> +  voltage LVDS DDR output and CSI-2 serial data output. The CSI-2
>    bus is the default. No bindings have been defined for the other busses.

Nitpicking, could you reflow the text to the 80 columns limit, or add a
blank line between what you consider to be independent paragraphs ?

>  
>  properties:
>    compatible:
>      enum:
>        - sony,imx290
> +      - sony,imx290-mono

The monochrome version is called IMX290LLR, while the colour version is
the IMX290LQR. Could we use sony,imx290llr (or possibly sony,imx290ll)
as a compatible string instead of sony,imx290-mono ? We could also
deprecate sony,imx290 in favour of sony,imx290lqr (or sony,imx290lq) if
desired, and I can handle this on top.

>  
>    reg:

-- 
Regards,

Laurent Pinchart

[PATCH 2/2] media: i2c: imx290: Add support for the mono sensor variant.

[Dave Stevenson]

The IMX290 module is available as either mono or colour (Bayer).

Update the driver so that it can advertise the correct mono
formats instead of the colour ones.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx290.c | 47 ++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 49d6c8bdec41..a370f1102334 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -13,6 +13,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -177,6 +178,7 @@ struct imx290 {
 	struct clk *xclk;
 	struct regmap *regmap;
 	u8 nlanes;
+	u8 mono;
 
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -414,7 +416,8 @@ static inline int imx290_modes_num(const struct imx290 *imx290)
 }
 
 struct imx290_format_info {
-	u32 code;
+	/* Array of codes. [0] is for colour, [1] is for mono. */
+	u32 code[2];
 	u8 bpp;
 	const struct imx290_regval *regs;
 	unsigned int num_regs;
@@ -422,26 +425,27 @@ struct imx290_format_info {
 
 static const struct imx290_format_info imx290_formats[] = {
 	{
-		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
+		.code = { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_Y10_1X10 },
 		.bpp = 10,
 		.regs = imx290_10bit_settings,
 		.num_regs = ARRAY_SIZE(imx290_10bit_settings),
 	}, {
-		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
+		.code = { MEDIA_BUS_FMT_SRGGB12_1X12, MEDIA_BUS_FMT_Y12_1X12 },
 		.bpp = 12,
 		.regs = imx290_12bit_settings,
 		.num_regs = ARRAY_SIZE(imx290_12bit_settings),
 	}
 };
 
-static const struct imx290_format_info *imx290_format_info(u32 code)
+static const struct imx290_format_info *
+imx290_format_info(const struct imx290 *imx290, u32 code)
 {
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) {
 		const struct imx290_format_info *info = &imx290_formats[i];
 
-		if (info->code == code)
+		if (info->code[imx290->mono] == code)
 			return info;
 	}
 
@@ -536,7 +540,7 @@ static int imx290_set_black_level(struct imx290 *imx290,
 				  const struct v4l2_mbus_framefmt *format,
 				  unsigned int black_level, int *err)
 {
-	unsigned int bpp = imx290_format_info(format->code)->bpp;
+	unsigned int bpp = imx290_format_info(imx290, format->code)->bpp;
 
 	return imx290_write(imx290, IMX290_BLKLEVEL,
 			    black_level >> (16 - bpp), err);
@@ -548,7 +552,7 @@ static int imx290_setup_format(struct imx290 *imx290,
 	const struct imx290_format_info *info;
 	int ret;
 
-	info = imx290_format_info(format->code);
+	info = imx290_format_info(imx290, format->code);
 
 	ret = imx290_set_register_array(imx290, info->regs, info->num_regs);
 	if (ret < 0) {
@@ -844,10 +848,12 @@ static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
 {
+	const struct imx290 *imx290 = to_imx290(sd);
+
 	if (code->index >= ARRAY_SIZE(imx290_formats))
 		return -EINVAL;
 
-	code->code = imx290_formats[code->index].code;
+	code->code = imx290_formats[code->index].code[imx290->mono];
 
 	return 0;
 }
@@ -859,7 +865,7 @@ static int imx290_enum_frame_size(struct v4l2_subdev *sd,
 	const struct imx290 *imx290 = to_imx290(sd);
 	const struct imx290_mode *imx290_modes = imx290_modes_ptr(imx290);
 
-	if (!imx290_format_info(fse->code))
+	if (!imx290_format_info(imx290, fse->code))
 		return -EINVAL;
 
 	if (fse->index >= imx290_modes_num(imx290))
@@ -888,8 +894,8 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
 
-	if (!imx290_format_info(fmt->format.code))
-		fmt->format.code = imx290_formats[0].code;
+	if (!imx290_format_info(imx290, fmt->format.code))
+		fmt->format.code = imx290_formats[0].code[imx290->mono];
 
 	fmt->format.field = V4L2_FIELD_NONE;
 
@@ -1177,16 +1183,29 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290,
 	return 0;
 }
 
+static const struct of_device_id imx290_of_match[] = {
+	{ .compatible = "sony,imx290" },
+	{ .compatible = "sony,imx290-mono", .data = (void *)1 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx290_of_match);
+
 static int imx290_parse_dt(struct imx290 *imx290)
 {
+	struct i2c_client *client = to_i2c_client(imx290->dev);
 	/* Only CSI2 is supported for now: */
 	struct v4l2_fwnode_endpoint ep = {
 		.bus_type = V4L2_MBUS_CSI2_DPHY
 	};
+	const struct of_device_id *match;
 	struct fwnode_handle *endpoint;
 	int ret;
 	s64 fq;
 
+	match = i2c_of_match_device(imx290_of_match, client);
+	if (match)
+		imx290->mono = match->data ? 1 : 0;
+
 	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL);
 	if (!endpoint) {
 		dev_err(imx290->dev, "Endpoint node not found\n");
@@ -1351,12 +1370,6 @@ static void imx290_remove(struct i2c_client *client)
 	pm_runtime_set_suspended(imx290->dev);
 }
 
-static const struct of_device_id imx290_of_match[] = {
-	{ .compatible = "sony,imx290" },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, imx290_of_match);
-
 static struct i2c_driver imx290_i2c_driver = {
 	.probe_new  = imx290_probe,
 	.remove = imx290_remove,
-- 
2.34.1

Re: [PATCH 2/2] media: i2c: imx290: Add support for the mono sensor variant.

[Alexander Stein]

Hi Dave,

Am Dienstag, 31. Januar 2023, 20:07:00 CET schrieb Dave Stevenson:
> The IMX290 module is available as either mono or colour (Bayer).
> 
> Update the driver so that it can advertise the correct mono
> formats instead of the colour ones.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx290.c | 47 ++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 49d6c8bdec41..a370f1102334 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -13,6 +13,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> @@ -177,6 +178,7 @@ struct imx290 {
>  	struct clk *xclk;
>  	struct regmap *regmap;
>  	u8 nlanes;
> +	u8 mono;
> 
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
> @@ -414,7 +416,8 @@ static inline int imx290_modes_num(const struct imx290
> *imx290) }
> 
>  struct imx290_format_info {
> -	u32 code;
> +	/* Array of codes. [0] is for colour, [1] is for mono. */
> +	u32 code[2];

Please use a define for that.

>  	u8 bpp;
>  	const struct imx290_regval *regs;
>  	unsigned int num_regs;
> @@ -422,26 +425,27 @@ struct imx290_format_info {
> 
>  static const struct imx290_format_info imx290_formats[] = {
>  	{
> -		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
> +		.code = { MEDIA_BUS_FMT_SRGGB10_1X10, 
MEDIA_BUS_FMT_Y10_1X10 },
>  		.bpp = 10,
>  		.regs = imx290_10bit_settings,
>  		.num_regs = ARRAY_SIZE(imx290_10bit_settings),
>  	}, {
> -		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
> +		.code = { MEDIA_BUS_FMT_SRGGB12_1X12, 
MEDIA_BUS_FMT_Y12_1X12 },
>  		.bpp = 12,
>  		.regs = imx290_12bit_settings,
>  		.num_regs = ARRAY_SIZE(imx290_12bit_settings),
>  	}
>  };
> 
> -static const struct imx290_format_info *imx290_format_info(u32 code)
> +static const struct imx290_format_info *
> +imx290_format_info(const struct imx290 *imx290, u32 code)
>  {
>  	unsigned int i;
> 
>  	for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) {
>  		const struct imx290_format_info *info = 
&imx290_formats[i];
> 
> -		if (info->code == code)
> +		if (info->code[imx290->mono] == code)
>  			return info;
>  	}
> 
> @@ -536,7 +540,7 @@ static int imx290_set_black_level(struct imx290 *imx290,
> const struct v4l2_mbus_framefmt *format,
>  				  unsigned int black_level, int *err)
>  {
> -	unsigned int bpp = imx290_format_info(format->code)->bpp;
> +	unsigned int bpp = imx290_format_info(imx290, format->code)->bpp;
> 
>  	return imx290_write(imx290, IMX290_BLKLEVEL,
>  			    black_level >> (16 - bpp), err);
> @@ -548,7 +552,7 @@ static int imx290_setup_format(struct imx290 *imx290,
>  	const struct imx290_format_info *info;
>  	int ret;
> 
> -	info = imx290_format_info(format->code);
> +	info = imx290_format_info(imx290, format->code);
> 
>  	ret = imx290_set_register_array(imx290, info->regs, info->num_regs);
>  	if (ret < 0) {
> @@ -844,10 +848,12 @@ static int imx290_enum_mbus_code(struct v4l2_subdev
> *sd, struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum 
*code)
>  {
> +	const struct imx290 *imx290 = to_imx290(sd);
> +
>  	if (code->index >= ARRAY_SIZE(imx290_formats))
>  		return -EINVAL;
> 
> -	code->code = imx290_formats[code->index].code;
> +	code->code = imx290_formats[code->index].code[imx290->mono];
> 
>  	return 0;
>  }
> @@ -859,7 +865,7 @@ static int imx290_enum_frame_size(struct v4l2_subdev
> *sd, const struct imx290 *imx290 = to_imx290(sd);
>  	const struct imx290_mode *imx290_modes = imx290_modes_ptr(imx290);
> 
> -	if (!imx290_format_info(fse->code))
> +	if (!imx290_format_info(imx290, fse->code))
>  		return -EINVAL;
> 
>  	if (fse->index >= imx290_modes_num(imx290))
> @@ -888,8 +894,8 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  	fmt->format.width = mode->width;
>  	fmt->format.height = mode->height;
> 
> -	if (!imx290_format_info(fmt->format.code))
> -		fmt->format.code = imx290_formats[0].code;
> +	if (!imx290_format_info(imx290, fmt->format.code))
> +		fmt->format.code = imx290_formats[0].code[imx290->mono];
> 
>  	fmt->format.field = V4L2_FIELD_NONE;
> 
> @@ -1177,16 +1183,29 @@ static s64 imx290_check_link_freqs(const struct
> imx290 *imx290, return 0;
>  }
> 
> +static const struct of_device_id imx290_of_match[] = {
> +	{ .compatible = "sony,imx290" },
> +	{ .compatible = "sony,imx290-mono", .data = (void *)1 },

Would you mind using a model specific struct? I have a patch on my stack 
adding support for imx327. There are some imx327 specific writes to registers 
during initialization. I do not mind adding this struct later though.

Best regards
Alexander

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx290_of_match);
> +
>  static int imx290_parse_dt(struct imx290 *imx290)
>  {
> +	struct i2c_client *client = to_i2c_client(imx290->dev);
>  	/* Only CSI2 is supported for now: */
>  	struct v4l2_fwnode_endpoint ep = {
>  		.bus_type = V4L2_MBUS_CSI2_DPHY
>  	};
> +	const struct of_device_id *match;
>  	struct fwnode_handle *endpoint;
>  	int ret;
>  	s64 fq;
> 
> +	match = i2c_of_match_device(imx290_of_match, client);
> +	if (match)
> +		imx290->mono = match->data ? 1 : 0;
> +
>  	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), 
NULL);
>  	if (!endpoint) {
>  		dev_err(imx290->dev, "Endpoint node not found\n");
> @@ -1351,12 +1370,6 @@ static void imx290_remove(struct i2c_client *client)
>  	pm_runtime_set_suspended(imx290->dev);
>  }
> 
> -static const struct of_device_id imx290_of_match[] = {
> -	{ .compatible = "sony,imx290" },
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, imx290_of_match);
> -
>  static struct i2c_driver imx290_i2c_driver = {
>  	.probe_new  = imx290_probe,
>  	.remove = imx290_remove,

Re: [PATCH 2/2] media: i2c: imx290: Add support for the mono sensor variant.

[Laurent Pinchart]

On Wed, Feb 01, 2023 at 08:03:36AM +0100, Alexander Stein wrote:
> Am Dienstag, 31. Januar 2023, 20:07:00 CET schrieb Dave Stevenson:
> > The IMX290 module is available as either mono or colour (Bayer).
> > 
> > Update the driver so that it can advertise the correct mono
> > formats instead of the colour ones.
> > 
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/imx290.c | 47 ++++++++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 49d6c8bdec41..a370f1102334 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> > +#include <linux/of_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> > @@ -177,6 +178,7 @@ struct imx290 {
> >  	struct clk *xclk;
> >  	struct regmap *regmap;
> >  	u8 nlanes;
> > +	u8 mono;
> > 
> >  	struct v4l2_subdev sd;
> >  	struct media_pad pad;
> > @@ -414,7 +416,8 @@ static inline int imx290_modes_num(const struct imx290 *imx290) }
> > 
> >  struct imx290_format_info {
> > -	u32 code;
> > +	/* Array of codes. [0] is for colour, [1] is for mono. */
> > +	u32 code[2];
> 
> Please use a define for that.
> 
> >  	u8 bpp;
> >  	const struct imx290_regval *regs;
> >  	unsigned int num_regs;
> > @@ -422,26 +425,27 @@ struct imx290_format_info {
> > 
> >  static const struct imx290_format_info imx290_formats[] = {
> >  	{
> > -		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > +		.code = { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_Y10_1X10 },
> >  		.bpp = 10,
> >  		.regs = imx290_10bit_settings,
> >  		.num_regs = ARRAY_SIZE(imx290_10bit_settings),
> >  	}, {
> > -		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > +		.code = { MEDIA_BUS_FMT_SRGGB12_1X12, MEDIA_BUS_FMT_Y12_1X12 },
> >  		.bpp = 12,
> >  		.regs = imx290_12bit_settings,
> >  		.num_regs = ARRAY_SIZE(imx290_12bit_settings),
> >  	}
> >  };
> > 
> > -static const struct imx290_format_info *imx290_format_info(u32 code)
> > +static const struct imx290_format_info *
> > +imx290_format_info(const struct imx290 *imx290, u32 code)
> >  {
> >  	unsigned int i;
> > 
> >  	for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) {
> >  		const struct imx290_format_info *info = &imx290_formats[i];
> > 
> > -		if (info->code == code)
> > +		if (info->code[imx290->mono] == code)
> >  			return info;
> >  	}
> > 
> > @@ -536,7 +540,7 @@ static int imx290_set_black_level(struct imx290 *imx290,
> > const struct v4l2_mbus_framefmt *format,
> >  				  unsigned int black_level, int *err)
> >  {
> > -	unsigned int bpp = imx290_format_info(format->code)->bpp;
> > +	unsigned int bpp = imx290_format_info(imx290, format->code)->bpp;
> > 
> >  	return imx290_write(imx290, IMX290_BLKLEVEL,
> >  			    black_level >> (16 - bpp), err);
> > @@ -548,7 +552,7 @@ static int imx290_setup_format(struct imx290 *imx290,
> >  	const struct imx290_format_info *info;
> >  	int ret;
> > 
> > -	info = imx290_format_info(format->code);
> > +	info = imx290_format_info(imx290, format->code);
> > 
> >  	ret = imx290_set_register_array(imx290, info->regs, info->num_regs);
> >  	if (ret < 0) {
> > @@ -844,10 +848,12 @@ static int imx290_enum_mbus_code(struct v4l2_subdev
> > *sd, struct v4l2_subdev_state *sd_state,
> >  				 struct v4l2_subdev_mbus_code_enum *code)
> >  {
> > +	const struct imx290 *imx290 = to_imx290(sd);
> > +
> >  	if (code->index >= ARRAY_SIZE(imx290_formats))
> >  		return -EINVAL;
> > 
> > -	code->code = imx290_formats[code->index].code;
> > +	code->code = imx290_formats[code->index].code[imx290->mono];
> > 
> >  	return 0;
> >  }
> > @@ -859,7 +865,7 @@ static int imx290_enum_frame_size(struct v4l2_subdev
> > *sd, const struct imx290 *imx290 = to_imx290(sd);
> >  	const struct imx290_mode *imx290_modes = imx290_modes_ptr(imx290);
> > 
> > -	if (!imx290_format_info(fse->code))
> > +	if (!imx290_format_info(imx290, fse->code))
> >  		return -EINVAL;
> > 
> >  	if (fse->index >= imx290_modes_num(imx290))
> > @@ -888,8 +894,8 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> >  	fmt->format.width = mode->width;
> >  	fmt->format.height = mode->height;
> > 
> > -	if (!imx290_format_info(fmt->format.code))
> > -		fmt->format.code = imx290_formats[0].code;
> > +	if (!imx290_format_info(imx290, fmt->format.code))
> > +		fmt->format.code = imx290_formats[0].code[imx290->mono];
> > 
> >  	fmt->format.field = V4L2_FIELD_NONE;
> > 
> > @@ -1177,16 +1183,29 @@ static s64 imx290_check_link_freqs(const struct
> > imx290 *imx290, return 0;
> >  }
> > 
> > +static const struct of_device_id imx290_of_match[] = {
> > +	{ .compatible = "sony,imx290" },
> > +	{ .compatible = "sony,imx290-mono", .data = (void *)1 },
> 
> Would you mind using a model specific struct? I have a patch on my stack 
> adding support for imx327. There are some imx327 specific writes to registers 
> during initialization. I do not mind adding this struct later though.

If not a structure already, at least an enum

enum imx290_model {
	IMX290_MODEL_COLOUR,
	IMX290_MODEL_MONO,
};

> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx290_of_match);
> > +
> >  static int imx290_parse_dt(struct imx290 *imx290)
> >  {
> > +	struct i2c_client *client = to_i2c_client(imx290->dev);
> >  	/* Only CSI2 is supported for now: */
> >  	struct v4l2_fwnode_endpoint ep = {
> >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> >  	};
> > +	const struct of_device_id *match;
> >  	struct fwnode_handle *endpoint;
> >  	int ret;
> >  	s64 fq;
> > 
> > +	match = i2c_of_match_device(imx290_of_match, client);
> > +	if (match)
> > +		imx290->mono = match->data ? 1 : 0;
> > +

You can simplify this to

	imx290->mono = (enum imx290_model)of_device_get_match_data(imx290->dev);

which may then be best placed in the probe function.

I'd be tempted to rename the imx290 mono field to model as the above
looks weird, but if Alexander needs a structure anyway, we may also just
do it right away:

enum imx290_model {
	IMX290_MODEL_COLOUR,
	IMX290_MODEL_MONO,
};

struct imx290_model_info {
	bool mono;
};

static const struct imx290_model_info imx290_models[] = {
	[IMX290_MODEL_COLOUR] = {
		.mono = false,
	},
	[IMX290_MODEL_MONO] = {
		.mono = true,
	},
};

static const struct of_device_id imx290_of_match[] = {
	{
		.compatible = "sony,imx290",
		.data = &imx290_models[IMX290_MODEL_COLOUR],
	},
	{
		.compatible = "sony,imx290-mono",
		.data = &imx290_models[IMX290_MODEL_MONO],
	},
	{ /* sentinel */ },
};

...

	imx290->model = of_device_get_match_data(imx290->dev);

and use imx290->model->mono instead of imx290->mono through the code.

I'm OK if you don't want this additional complexity yet, but the code is
here already and will be needed soon :-)

> >  	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL);
> >  	if (!endpoint) {
> >  		dev_err(imx290->dev, "Endpoint node not found\n");
> > @@ -1351,12 +1370,6 @@ static void imx290_remove(struct i2c_client *client)
> >  	pm_runtime_set_suspended(imx290->dev);
> >  }
> > 
> > -static const struct of_device_id imx290_of_match[] = {
> > -	{ .compatible = "sony,imx290" },
> > -	{ /* sentinel */ }
> > -};
> > -MODULE_DEVICE_TABLE(of, imx290_of_match);
> > -
> >  static struct i2c_driver imx290_i2c_driver = {
> >  	.probe_new  = imx290_probe,
> >  	.remove = imx290_remove,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: i2c: imx290: Add support for the mono sensor variant.

[Dave Stevenson]

Hi Laurent

On Thu, 2 Feb 2023 at 00:15, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Feb 01, 2023 at 08:03:36AM +0100, Alexander Stein wrote:
> > Am Dienstag, 31. Januar 2023, 20:07:00 CET schrieb Dave Stevenson:
> > > The IMX290 module is available as either mono or colour (Bayer).
> > >
> > > Update the driver so that it can advertise the correct mono
> > > formats instead of the colour ones.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/imx290.c | 47 ++++++++++++++++++++++++--------------
> > >  1 file changed, 30 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 49d6c8bdec41..a370f1102334 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -13,6 +13,7 @@
> > >  #include <linux/gpio/consumer.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/module.h>
> > > +#include <linux/of_device.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/regulator/consumer.h>
> > > @@ -177,6 +178,7 @@ struct imx290 {
> > >     struct clk *xclk;
> > >     struct regmap *regmap;
> > >     u8 nlanes;
> > > +   u8 mono;
> > >
> > >     struct v4l2_subdev sd;
> > >     struct media_pad pad;
> > > @@ -414,7 +416,8 @@ static inline int imx290_modes_num(const struct imx290 *imx290) }
> > >
> > >  struct imx290_format_info {
> > > -   u32 code;
> > > +   /* Array of codes. [0] is for colour, [1] is for mono. */
> > > +   u32 code[2];
> >
> > Please use a define for that.
> >
> > >     u8 bpp;
> > >     const struct imx290_regval *regs;
> > >     unsigned int num_regs;
> > > @@ -422,26 +425,27 @@ struct imx290_format_info {
> > >
> > >  static const struct imx290_format_info imx290_formats[] = {
> > >     {
> > > -           .code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > +           .code = { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_Y10_1X10 },
> > >             .bpp = 10,
> > >             .regs = imx290_10bit_settings,
> > >             .num_regs = ARRAY_SIZE(imx290_10bit_settings),
> > >     }, {
> > > -           .code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > +           .code = { MEDIA_BUS_FMT_SRGGB12_1X12, MEDIA_BUS_FMT_Y12_1X12 },
> > >             .bpp = 12,
> > >             .regs = imx290_12bit_settings,
> > >             .num_regs = ARRAY_SIZE(imx290_12bit_settings),
> > >     }
> > >  };
> > >
> > > -static const struct imx290_format_info *imx290_format_info(u32 code)
> > > +static const struct imx290_format_info *
> > > +imx290_format_info(const struct imx290 *imx290, u32 code)
> > >  {
> > >     unsigned int i;
> > >
> > >     for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) {
> > >             const struct imx290_format_info *info = &imx290_formats[i];
> > >
> > > -           if (info->code == code)
> > > +           if (info->code[imx290->mono] == code)
> > >                     return info;
> > >     }
> > >
> > > @@ -536,7 +540,7 @@ static int imx290_set_black_level(struct imx290 *imx290,
> > > const struct v4l2_mbus_framefmt *format,
> > >                               unsigned int black_level, int *err)
> > >  {
> > > -   unsigned int bpp = imx290_format_info(format->code)->bpp;
> > > +   unsigned int bpp = imx290_format_info(imx290, format->code)->bpp;
> > >
> > >     return imx290_write(imx290, IMX290_BLKLEVEL,
> > >                         black_level >> (16 - bpp), err);
> > > @@ -548,7 +552,7 @@ static int imx290_setup_format(struct imx290 *imx290,
> > >     const struct imx290_format_info *info;
> > >     int ret;
> > >
> > > -   info = imx290_format_info(format->code);
> > > +   info = imx290_format_info(imx290, format->code);
> > >
> > >     ret = imx290_set_register_array(imx290, info->regs, info->num_regs);
> > >     if (ret < 0) {
> > > @@ -844,10 +848,12 @@ static int imx290_enum_mbus_code(struct v4l2_subdev
> > > *sd, struct v4l2_subdev_state *sd_state,
> > >                              struct v4l2_subdev_mbus_code_enum *code)
> > >  {
> > > +   const struct imx290 *imx290 = to_imx290(sd);
> > > +
> > >     if (code->index >= ARRAY_SIZE(imx290_formats))
> > >             return -EINVAL;
> > >
> > > -   code->code = imx290_formats[code->index].code;
> > > +   code->code = imx290_formats[code->index].code[imx290->mono];
> > >
> > >     return 0;
> > >  }
> > > @@ -859,7 +865,7 @@ static int imx290_enum_frame_size(struct v4l2_subdev
> > > *sd, const struct imx290 *imx290 = to_imx290(sd);
> > >     const struct imx290_mode *imx290_modes = imx290_modes_ptr(imx290);
> > >
> > > -   if (!imx290_format_info(fse->code))
> > > +   if (!imx290_format_info(imx290, fse->code))
> > >             return -EINVAL;
> > >
> > >     if (fse->index >= imx290_modes_num(imx290))
> > > @@ -888,8 +894,8 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> > >     fmt->format.width = mode->width;
> > >     fmt->format.height = mode->height;
> > >
> > > -   if (!imx290_format_info(fmt->format.code))
> > > -           fmt->format.code = imx290_formats[0].code;
> > > +   if (!imx290_format_info(imx290, fmt->format.code))
> > > +           fmt->format.code = imx290_formats[0].code[imx290->mono];
> > >
> > >     fmt->format.field = V4L2_FIELD_NONE;
> > >
> > > @@ -1177,16 +1183,29 @@ static s64 imx290_check_link_freqs(const struct
> > > imx290 *imx290, return 0;
> > >  }
> > >
> > > +static const struct of_device_id imx290_of_match[] = {
> > > +   { .compatible = "sony,imx290" },
> > > +   { .compatible = "sony,imx290-mono", .data = (void *)1 },
> >
> > Would you mind using a model specific struct? I have a patch on my stack
> > adding support for imx327. There are some imx327 specific writes to registers
> > during initialization. I do not mind adding this struct later though.
>
> If not a structure already, at least an enum
>
> enum imx290_model {
>         IMX290_MODEL_COLOUR,
>         IMX290_MODEL_MONO,
> };
>
> > > +   { /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, imx290_of_match);
> > > +
> > >  static int imx290_parse_dt(struct imx290 *imx290)
> > >  {
> > > +   struct i2c_client *client = to_i2c_client(imx290->dev);
> > >     /* Only CSI2 is supported for now: */
> > >     struct v4l2_fwnode_endpoint ep = {
> > >             .bus_type = V4L2_MBUS_CSI2_DPHY
> > >     };
> > > +   const struct of_device_id *match;
> > >     struct fwnode_handle *endpoint;
> > >     int ret;
> > >     s64 fq;
> > >
> > > +   match = i2c_of_match_device(imx290_of_match, client);
> > > +   if (match)
> > > +           imx290->mono = match->data ? 1 : 0;
> > > +
>
> You can simplify this to
>
>         imx290->mono = (enum imx290_model)of_device_get_match_data(imx290->dev);
>
> which may then be best placed in the probe function.
>
> I'd be tempted to rename the imx290 mono field to model as the above
> looks weird, but if Alexander needs a structure anyway, we may also just
> do it right away:
>
> enum imx290_model {
>         IMX290_MODEL_COLOUR,
>         IMX290_MODEL_MONO,
> };
>
> struct imx290_model_info {
>         bool mono;
> };

To my mind this gets confusing as to whether imx290_model is the
mono/colour switch reference for finding a code in imx290_formats, or
differentiating between imx290llr (mono), imx290lqr (colour),
imx327xyz, etc as different models of the sensor. (Having the IMX290
prefix because it is in the imx290 driver will get even more confusing
when you get IMX290_MODEL_IMX327_COLOUR).

Alexander was asking for a define to set the size of code in struct
imx290_format_info. As you also point out above, switching to using an
enum makes more sense, and then you can add a _MAX to get the array
size.

enum imx290_colour_variant {
        IMX290_VARIANT_COLOUR,
        IMX290_VARIANT_MONO,
        IMX290_VARIANT_MAX
};

enum imx290_model {
        IMX290_MODEL_IMX290LLR,
        IMX290_MODEL_IMX290LQR,
/*
        IMX290_MODEL_IMX327LQR,
        IMX290_MODEL_IMX462LQR,
        IMX290_MODEL_IMX462LLR,
        etc,
*/
};

static const struct imx290_model_info imx290_models[] = {
        [IMX290_MODEL_IMX290LQR]= {
                .colour_variant = IMX290_VARIANT_COLOUR,
        },
        [IMX290_MODEL_IMX290LLR] = {
                .colour_variant = IMX290_VARIANT_MONO,
        },
};

Thoughts?
Otherwise I will drop back to the simplest option - I'm afraid I
haven't got masses of time to be messing about revising patches at
present.

  Dave

>
> static const struct imx290_model_info imx290_models[] = {
>         [IMX290_MODEL_COLOUR] = {
>                 .mono = false,
>         },
>         [IMX290_MODEL_MONO] = {
>                 .mono = true,
>         },
> };
>
> static const struct of_device_id imx290_of_match[] = {
>         {
>                 .compatible = "sony,imx290",
>                 .data = &imx290_models[IMX290_MODEL_COLOUR],
>         },
>         {
>                 .compatible = "sony,imx290-mono",
>                 .data = &imx290_models[IMX290_MODEL_MONO],
>         },
>         { /* sentinel */ },
> };
>
> ...
>
>         imx290->model = of_device_get_match_data(imx290->dev);
>
> and use imx290->model->mono instead of imx290->mono through the code.
>
> I'm OK if you don't want this additional complexity yet, but the code is
> here already and will be needed soon :-)
>
> > >     endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL);
> > >     if (!endpoint) {
> > >             dev_err(imx290->dev, "Endpoint node not found\n");
> > > @@ -1351,12 +1370,6 @@ static void imx290_remove(struct i2c_client *client)
> > >     pm_runtime_set_suspended(imx290->dev);
> > >  }
> > >
> > > -static const struct of_device_id imx290_of_match[] = {
> > > -   { .compatible = "sony,imx290" },
> > > -   { /* sentinel */ }
> > > -};
> > > -MODULE_DEVICE_TABLE(of, imx290_of_match);
> > > -
> > >  static struct i2c_driver imx290_i2c_driver = {
> > >     .probe_new  = imx290_probe,
> > >     .remove = imx290_remove,
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 2/2] media: i2c: imx290: Add support for the mono sensor variant.

[Laurent Pinchart]

Hi Dave,

On Thu, Feb 02, 2023 at 11:20:14AM +0000, Dave Stevenson wrote:
> On Thu, 2 Feb 2023 at 00:15, Laurent Pinchart wrote:
> > On Wed, Feb 01, 2023 at 08:03:36AM +0100, Alexander Stein wrote:
> > > Am Dienstag, 31. Januar 2023, 20:07:00 CET schrieb Dave Stevenson:
> > > > The IMX290 module is available as either mono or colour (Bayer).
> > > >
> > > > Update the driver so that it can advertise the correct mono
> > > > formats instead of the colour ones.
> > > >
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > ---
> > > >  drivers/media/i2c/imx290.c | 47 ++++++++++++++++++++++++--------------
> > > >  1 file changed, 30 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index 49d6c8bdec41..a370f1102334 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -13,6 +13,7 @@
> > > >  #include <linux/gpio/consumer.h>
> > > >  #include <linux/i2c.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/of_device.h>
> > > >  #include <linux/pm_runtime.h>
> > > >  #include <linux/regmap.h>
> > > >  #include <linux/regulator/consumer.h>
> > > > @@ -177,6 +178,7 @@ struct imx290 {
> > > >     struct clk *xclk;
> > > >     struct regmap *regmap;
> > > >     u8 nlanes;
> > > > +   u8 mono;
> > > >
> > > >     struct v4l2_subdev sd;
> > > >     struct media_pad pad;
> > > > @@ -414,7 +416,8 @@ static inline int imx290_modes_num(const struct imx290 *imx290) }
> > > >
> > > >  struct imx290_format_info {
> > > > -   u32 code;
> > > > +   /* Array of codes. [0] is for colour, [1] is for mono. */
> > > > +   u32 code[2];
> > >
> > > Please use a define for that.
> > >
> > > >     u8 bpp;
> > > >     const struct imx290_regval *regs;
> > > >     unsigned int num_regs;
> > > > @@ -422,26 +425,27 @@ struct imx290_format_info {
> > > >
> > > >  static const struct imx290_format_info imx290_formats[] = {
> > > >     {
> > > > -           .code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > +           .code = { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_Y10_1X10 },
> > > >             .bpp = 10,
> > > >             .regs = imx290_10bit_settings,
> > > >             .num_regs = ARRAY_SIZE(imx290_10bit_settings),
> > > >     }, {
> > > > -           .code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > +           .code = { MEDIA_BUS_FMT_SRGGB12_1X12, MEDIA_BUS_FMT_Y12_1X12 },
> > > >             .bpp = 12,
> > > >             .regs = imx290_12bit_settings,
> > > >             .num_regs = ARRAY_SIZE(imx290_12bit_settings),
> > > >     }
> > > >  };
> > > >
> > > > -static const struct imx290_format_info *imx290_format_info(u32 code)
> > > > +static const struct imx290_format_info *
> > > > +imx290_format_info(const struct imx290 *imx290, u32 code)
> > > >  {
> > > >     unsigned int i;
> > > >
> > > >     for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) {
> > > >             const struct imx290_format_info *info = &imx290_formats[i];
> > > >
> > > > -           if (info->code == code)
> > > > +           if (info->code[imx290->mono] == code)
> > > >                     return info;
> > > >     }
> > > >
> > > > @@ -536,7 +540,7 @@ static int imx290_set_black_level(struct imx290 *imx290,
> > > > const struct v4l2_mbus_framefmt *format,
> > > >                               unsigned int black_level, int *err)
> > > >  {
> > > > -   unsigned int bpp = imx290_format_info(format->code)->bpp;
> > > > +   unsigned int bpp = imx290_format_info(imx290, format->code)->bpp;
> > > >
> > > >     return imx290_write(imx290, IMX290_BLKLEVEL,
> > > >                         black_level >> (16 - bpp), err);
> > > > @@ -548,7 +552,7 @@ static int imx290_setup_format(struct imx290 *imx290,
> > > >     const struct imx290_format_info *info;
> > > >     int ret;
> > > >
> > > > -   info = imx290_format_info(format->code);
> > > > +   info = imx290_format_info(imx290, format->code);
> > > >
> > > >     ret = imx290_set_register_array(imx290, info->regs, info->num_regs);
> > > >     if (ret < 0) {
> > > > @@ -844,10 +848,12 @@ static int imx290_enum_mbus_code(struct v4l2_subdev
> > > > *sd, struct v4l2_subdev_state *sd_state,
> > > >                              struct v4l2_subdev_mbus_code_enum *code)
> > > >  {
> > > > +   const struct imx290 *imx290 = to_imx290(sd);
> > > > +
> > > >     if (code->index >= ARRAY_SIZE(imx290_formats))
> > > >             return -EINVAL;
> > > >
> > > > -   code->code = imx290_formats[code->index].code;
> > > > +   code->code = imx290_formats[code->index].code[imx290->mono];
> > > >
> > > >     return 0;
> > > >  }
> > > > @@ -859,7 +865,7 @@ static int imx290_enum_frame_size(struct v4l2_subdev
> > > > *sd, const struct imx290 *imx290 = to_imx290(sd);
> > > >     const struct imx290_mode *imx290_modes = imx290_modes_ptr(imx290);
> > > >
> > > > -   if (!imx290_format_info(fse->code))
> > > > +   if (!imx290_format_info(imx290, fse->code))
> > > >             return -EINVAL;
> > > >
> > > >     if (fse->index >= imx290_modes_num(imx290))
> > > > @@ -888,8 +894,8 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> > > >     fmt->format.width = mode->width;
> > > >     fmt->format.height = mode->height;
> > > >
> > > > -   if (!imx290_format_info(fmt->format.code))
> > > > -           fmt->format.code = imx290_formats[0].code;
> > > > +   if (!imx290_format_info(imx290, fmt->format.code))
> > > > +           fmt->format.code = imx290_formats[0].code[imx290->mono];
> > > >
> > > >     fmt->format.field = V4L2_FIELD_NONE;
> > > >
> > > > @@ -1177,16 +1183,29 @@ static s64 imx290_check_link_freqs(const struct
> > > > imx290 *imx290, return 0;
> > > >  }
> > > >
> > > > +static const struct of_device_id imx290_of_match[] = {
> > > > +   { .compatible = "sony,imx290" },
> > > > +   { .compatible = "sony,imx290-mono", .data = (void *)1 },
> > >
> > > Would you mind using a model specific struct? I have a patch on my stack
> > > adding support for imx327. There are some imx327 specific writes to registers
> > > during initialization. I do not mind adding this struct later though.
> >
> > If not a structure already, at least an enum
> >
> > enum imx290_model {
> >         IMX290_MODEL_COLOUR,
> >         IMX290_MODEL_MONO,
> > };
> >
> > > > +   { /* sentinel */ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, imx290_of_match);
> > > > +
> > > >  static int imx290_parse_dt(struct imx290 *imx290)
> > > >  {
> > > > +   struct i2c_client *client = to_i2c_client(imx290->dev);
> > > >     /* Only CSI2 is supported for now: */
> > > >     struct v4l2_fwnode_endpoint ep = {
> > > >             .bus_type = V4L2_MBUS_CSI2_DPHY
> > > >     };
> > > > +   const struct of_device_id *match;
> > > >     struct fwnode_handle *endpoint;
> > > >     int ret;
> > > >     s64 fq;
> > > >
> > > > +   match = i2c_of_match_device(imx290_of_match, client);
> > > > +   if (match)
> > > > +           imx290->mono = match->data ? 1 : 0;
> > > > +
> >
> > You can simplify this to
> >
> >         imx290->mono = (enum imx290_model)of_device_get_match_data(imx290->dev);
> >
> > which may then be best placed in the probe function.
> >
> > I'd be tempted to rename the imx290 mono field to model as the above
> > looks weird, but if Alexander needs a structure anyway, we may also just
> > do it right away:
> >
> > enum imx290_model {
> >         IMX290_MODEL_COLOUR,
> >         IMX290_MODEL_MONO,
> > };
> >
> > struct imx290_model_info {
> >         bool mono;
> > };
> 
> To my mind this gets confusing as to whether imx290_model is the
> mono/colour switch reference for finding a code in imx290_formats, or
> differentiating between imx290llr (mono), imx290lqr (colour),
> imx327xyz, etc as different models of the sensor. (Having the IMX290
> prefix because it is in the imx290 driver will get even more confusing
> when you get IMX290_MODEL_IMX327_COLOUR).
> 
> Alexander was asking for a define to set the size of code in struct
> imx290_format_info. As you also point out above, switching to using an
> enum makes more sense, and then you can add a _MAX to get the array
> size.
> 
> enum imx290_colour_variant {
>         IMX290_VARIANT_COLOUR,
>         IMX290_VARIANT_MONO,
>         IMX290_VARIANT_MAX
> };
> 
> enum imx290_model {
>         IMX290_MODEL_IMX290LLR,
>         IMX290_MODEL_IMX290LQR,
> /*
>         IMX290_MODEL_IMX327LQR,
>         IMX290_MODEL_IMX462LQR,
>         IMX290_MODEL_IMX462LLR,
>         etc,
> */
> };
> 
> static const struct imx290_model_info imx290_models[] = {
>         [IMX290_MODEL_IMX290LQR]= {
>                 .colour_variant = IMX290_VARIANT_COLOUR,
>         },
>         [IMX290_MODEL_IMX290LLR] = {
>                 .colour_variant = IMX290_VARIANT_MONO,
>         },
> };
> 
> Thoughts?

I like the above :-)

> Otherwise I will drop back to the simplest option - I'm afraid I
> haven't got masses of time to be messing about revising patches at
> present.

No worries. I can also improve things on top if needed.

> > static const struct imx290_model_info imx290_models[] = {
> >         [IMX290_MODEL_COLOUR] = {
> >                 .mono = false,
> >         },
> >         [IMX290_MODEL_MONO] = {
> >                 .mono = true,
> >         },
> > };
> >
> > static const struct of_device_id imx290_of_match[] = {
> >         {
> >                 .compatible = "sony,imx290",
> >                 .data = &imx290_models[IMX290_MODEL_COLOUR],
> >         },
> >         {
> >                 .compatible = "sony,imx290-mono",
> >                 .data = &imx290_models[IMX290_MODEL_MONO],
> >         },
> >         { /* sentinel */ },
> > };
> >
> > ...
> >
> >         imx290->model = of_device_get_match_data(imx290->dev);
> >
> > and use imx290->model->mono instead of imx290->mono through the code.
> >
> > I'm OK if you don't want this additional complexity yet, but the code is
> > here already and will be needed soon :-)
> >
> > > >     endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL);
> > > >     if (!endpoint) {
> > > >             dev_err(imx290->dev, "Endpoint node not found\n");
> > > > @@ -1351,12 +1370,6 @@ static void imx290_remove(struct i2c_client *client)
> > > >     pm_runtime_set_suspended(imx290->dev);
> > > >  }
> > > >
> > > > -static const struct of_device_id imx290_of_match[] = {
> > > > -   { .compatible = "sony,imx290" },
> > > > -   { /* sentinel */ }
> > > > -};
> > > > -MODULE_DEVICE_TABLE(of, imx290_of_match);
> > > > -
> > > >  static struct i2c_driver imx290_i2c_driver = {
> > > >     .probe_new  = imx290_probe,
> > > >     .remove = imx290_remove,

-- 
Regards,

Laurent Pinchart