1. Patchew
  2. linux
  3. [PATCH 0/3] media: i2c: imx290: Add support for imx462
  4. View patch

[PATCH 3/3] media: i2c: imx290: Add configuration for IMX462

Dave Stevenson posted 3 patches 1 week, 1 day ago
There is a newer version of this series
[PATCH 3/3] media: i2c: imx290: Add configuration for IMX462
Posted by Dave Stevenson 1 week, 1 day ago 
IMX462 is the successor to IMX290, and wants very minor
changes to the register setup.

Add the relevant configuration to support it.

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

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index da654deb444a..f1780cc5d7cc 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -170,6 +170,8 @@ enum imx290_model {
 	IMX290_MODEL_IMX290LQR,
 	IMX290_MODEL_IMX290LLR,
 	IMX290_MODEL_IMX327LQR,
+	IMX290_MODEL_IMX462LQR,
+	IMX290_MODEL_IMX462LLR,
 };
 
 struct imx290_model_info {
@@ -316,6 +318,50 @@ static const struct cci_reg_sequence imx290_global_init_settings_290[] = {
 	{ CCI_REG8(0x33b3), 0x04 },
 };
 
+static const struct cci_reg_sequence imx290_global_init_settings_462[] = {
+	{ CCI_REG8(0x300f), 0x00 },
+	{ CCI_REG8(0x3010), 0x21 },
+	{ CCI_REG8(0x3011), 0x02 },
+	{ CCI_REG8(0x3016), 0x09 },
+	{ CCI_REG8(0x3070), 0x02 },
+	{ CCI_REG8(0x3071), 0x11 },
+	{ CCI_REG8(0x309b), 0x10 },
+	{ CCI_REG8(0x309c), 0x22 },
+	{ CCI_REG8(0x30a2), 0x02 },
+	{ CCI_REG8(0x30a6), 0x20 },
+	{ CCI_REG8(0x30a8), 0x20 },
+	{ CCI_REG8(0x30aa), 0x20 },
+	{ CCI_REG8(0x30ac), 0x20 },
+	{ CCI_REG8(0x30b0), 0x43 },
+	{ CCI_REG8(0x3119), 0x9e },
+	{ CCI_REG8(0x311c), 0x1e },
+	{ CCI_REG8(0x311e), 0x08 },
+	{ CCI_REG8(0x3128), 0x05 },
+	{ CCI_REG8(0x313d), 0x83 },
+	{ CCI_REG8(0x3150), 0x03 },
+	{ CCI_REG8(0x317e), 0x00 },
+	{ CCI_REG8(0x32b8), 0x50 },
+	{ CCI_REG8(0x32b9), 0x10 },
+	{ CCI_REG8(0x32ba), 0x00 },
+	{ CCI_REG8(0x32bb), 0x04 },
+	{ CCI_REG8(0x32c8), 0x50 },
+	{ CCI_REG8(0x32c9), 0x10 },
+	{ CCI_REG8(0x32ca), 0x00 },
+	{ CCI_REG8(0x32cb), 0x04 },
+	{ CCI_REG8(0x332c), 0xd3 },
+	{ CCI_REG8(0x332d), 0x10 },
+	{ CCI_REG8(0x332e), 0x0d },
+	{ CCI_REG8(0x3358), 0x06 },
+	{ CCI_REG8(0x3359), 0xe1 },
+	{ CCI_REG8(0x335a), 0x11 },
+	{ CCI_REG8(0x3360), 0x1e },
+	{ CCI_REG8(0x3361), 0x61 },
+	{ CCI_REG8(0x3362), 0x10 },
+	{ CCI_REG8(0x33b0), 0x50 },
+	{ CCI_REG8(0x33b2), 0x1a },
+	{ CCI_REG8(0x33b3), 0x04 },
+};
+
 #define IMX290_NUM_CLK_REGS	2
 static const struct cci_reg_sequence xclk_regs[][IMX290_NUM_CLK_REGS] = {
 	[IMX290_CLK_37_125] = {
@@ -1455,6 +1501,20 @@ static const struct imx290_model_info imx290_models[] = {
 		.max_analog_gain = 98,
 		.name = "imx327",
 	},
+	[IMX290_MODEL_IMX462LQR] = {
+		.colour_variant = IMX290_VARIANT_COLOUR,
+		.init_regs = imx290_global_init_settings_462,
+		.init_regs_num = ARRAY_SIZE(imx290_global_init_settings_462),
+		.max_analog_gain = 98,
+		.name = "imx462",
+	},
+	[IMX290_MODEL_IMX462LLR] = {
+		.colour_variant = IMX290_VARIANT_MONO,
+		.init_regs = imx290_global_init_settings_462,
+		.init_regs_num = ARRAY_SIZE(imx290_global_init_settings_462),
+		.max_analog_gain = 98,
+		.name = "imx462",
+	},
 };
 
 static int imx290_parse_dt(struct imx290 *imx290)
@@ -1653,6 +1713,12 @@ static const struct of_device_id imx290_of_match[] = {
 	}, {
 		.compatible = "sony,imx327lqr",
 		.data = &imx290_models[IMX290_MODEL_IMX327LQR],
+	}, {
+		.compatible = "sony,imx462lqr",
+		.data = &imx290_models[IMX290_MODEL_IMX462LQR],
+	}, {
+		.compatible = "sony,imx462llr",
+		.data = &imx290_models[IMX290_MODEL_IMX462LLR],
 	},
 	{ /* sentinel */ },
 };

-- 
2.34.1
Re: [PATCH 3/3] media: i2c: imx290: Add configuration for IMX462
Posted by Laurent Pinchart 1 week, 1 day ago 
Hi Dave,

Thank you for the patch.

On Thu, Nov 14, 2024 at 04:01:15PM +0000, Dave Stevenson wrote:
> IMX462 is the successor to IMX290, and wants very minor
> changes to the register setup.
> 
> Add the relevant configuration to support it.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx290.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index da654deb444a..f1780cc5d7cc 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -170,6 +170,8 @@ enum imx290_model {
>  	IMX290_MODEL_IMX290LQR,
>  	IMX290_MODEL_IMX290LLR,
>  	IMX290_MODEL_IMX327LQR,
> +	IMX290_MODEL_IMX462LQR,
> +	IMX290_MODEL_IMX462LLR,
>  };
>  
>  struct imx290_model_info {
> @@ -316,6 +318,50 @@ static const struct cci_reg_sequence imx290_global_init_settings_290[] = {
>  	{ CCI_REG8(0x33b3), 0x04 },
>  };
>  
> +static const struct cci_reg_sequence imx290_global_init_settings_462[] = {
> +	{ CCI_REG8(0x300f), 0x00 },
> +	{ CCI_REG8(0x3010), 0x21 },
> +	{ CCI_REG8(0x3011), 0x02 },

As far as I can tell, the only difference in the init sequence between
imx290_global_init_settings_290 and imx290_global_init_settings_462 is
0x3011 register which is not present in imx290_global_init_settings_290.
It is however included in imx290_global_init_settings, and set to 0x02.
Could we therefore use imx290_global_init_settings_290 for the imx462 ?

> +	{ CCI_REG8(0x3016), 0x09 },
> +	{ CCI_REG8(0x3070), 0x02 },
> +	{ CCI_REG8(0x3071), 0x11 },
> +	{ CCI_REG8(0x309b), 0x10 },
> +	{ CCI_REG8(0x309c), 0x22 },
> +	{ CCI_REG8(0x30a2), 0x02 },
> +	{ CCI_REG8(0x30a6), 0x20 },
> +	{ CCI_REG8(0x30a8), 0x20 },
> +	{ CCI_REG8(0x30aa), 0x20 },
> +	{ CCI_REG8(0x30ac), 0x20 },
> +	{ CCI_REG8(0x30b0), 0x43 },
> +	{ CCI_REG8(0x3119), 0x9e },
> +	{ CCI_REG8(0x311c), 0x1e },
> +	{ CCI_REG8(0x311e), 0x08 },
> +	{ CCI_REG8(0x3128), 0x05 },
> +	{ CCI_REG8(0x313d), 0x83 },
> +	{ CCI_REG8(0x3150), 0x03 },
> +	{ CCI_REG8(0x317e), 0x00 },
> +	{ CCI_REG8(0x32b8), 0x50 },
> +	{ CCI_REG8(0x32b9), 0x10 },
> +	{ CCI_REG8(0x32ba), 0x00 },
> +	{ CCI_REG8(0x32bb), 0x04 },
> +	{ CCI_REG8(0x32c8), 0x50 },
> +	{ CCI_REG8(0x32c9), 0x10 },
> +	{ CCI_REG8(0x32ca), 0x00 },
> +	{ CCI_REG8(0x32cb), 0x04 },
> +	{ CCI_REG8(0x332c), 0xd3 },
> +	{ CCI_REG8(0x332d), 0x10 },
> +	{ CCI_REG8(0x332e), 0x0d },
> +	{ CCI_REG8(0x3358), 0x06 },
> +	{ CCI_REG8(0x3359), 0xe1 },
> +	{ CCI_REG8(0x335a), 0x11 },
> +	{ CCI_REG8(0x3360), 0x1e },
> +	{ CCI_REG8(0x3361), 0x61 },
> +	{ CCI_REG8(0x3362), 0x10 },
> +	{ CCI_REG8(0x33b0), 0x50 },
> +	{ CCI_REG8(0x33b2), 0x1a },
> +	{ CCI_REG8(0x33b3), 0x04 },
> +};
> +
>  #define IMX290_NUM_CLK_REGS	2
>  static const struct cci_reg_sequence xclk_regs[][IMX290_NUM_CLK_REGS] = {
>  	[IMX290_CLK_37_125] = {
> @@ -1455,6 +1501,20 @@ static const struct imx290_model_info imx290_models[] = {
>  		.max_analog_gain = 98,
>  		.name = "imx327",
>  	},
> +	[IMX290_MODEL_IMX462LQR] = {
> +		.colour_variant = IMX290_VARIANT_COLOUR,
> +		.init_regs = imx290_global_init_settings_462,
> +		.init_regs_num = ARRAY_SIZE(imx290_global_init_settings_462),
> +		.max_analog_gain = 98,
> +		.name = "imx462",
> +	},
> +	[IMX290_MODEL_IMX462LLR] = {
> +		.colour_variant = IMX290_VARIANT_MONO,
> +		.init_regs = imx290_global_init_settings_462,
> +		.init_regs_num = ARRAY_SIZE(imx290_global_init_settings_462),
> +		.max_analog_gain = 98,
> +		.name = "imx462",
> +	},
>  };
>  
>  static int imx290_parse_dt(struct imx290 *imx290)
> @@ -1653,6 +1713,12 @@ static const struct of_device_id imx290_of_match[] = {
>  	}, {
>  		.compatible = "sony,imx327lqr",
>  		.data = &imx290_models[IMX290_MODEL_IMX327LQR],
> +	}, {
> +		.compatible = "sony,imx462lqr",
> +		.data = &imx290_models[IMX290_MODEL_IMX462LQR],
> +	}, {
> +		.compatible = "sony,imx462llr",
> +		.data = &imx290_models[IMX290_MODEL_IMX462LLR],
>  	},
>  	{ /* sentinel */ },
>  };

-- 
Regards,

Laurent Pinchart
Re: [PATCH 3/3] media: i2c: imx290: Add configuration for IMX462
Posted by Dave Stevenson 1 week ago 
Hi Laurent

On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> Thank you for the patch.
>
> On Thu, Nov 14, 2024 at 04:01:15PM +0000, Dave Stevenson wrote:
> > IMX462 is the successor to IMX290, and wants very minor
> > changes to the register setup.
> >
> > Add the relevant configuration to support it.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/imx290.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index da654deb444a..f1780cc5d7cc 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -170,6 +170,8 @@ enum imx290_model {
> >       IMX290_MODEL_IMX290LQR,
> >       IMX290_MODEL_IMX290LLR,
> >       IMX290_MODEL_IMX327LQR,
> > +     IMX290_MODEL_IMX462LQR,
> > +     IMX290_MODEL_IMX462LLR,
> >  };
> >
> >  struct imx290_model_info {
> > @@ -316,6 +318,50 @@ static const struct cci_reg_sequence imx290_global_init_settings_290[] = {
> >       { CCI_REG8(0x33b3), 0x04 },
> >  };
> >
> > +static const struct cci_reg_sequence imx290_global_init_settings_462[] = {
> > +     { CCI_REG8(0x300f), 0x00 },
> > +     { CCI_REG8(0x3010), 0x21 },
> > +     { CCI_REG8(0x3011), 0x02 },
>
> As far as I can tell, the only difference in the init sequence between
> imx290_global_init_settings_290 and imx290_global_init_settings_462 is
> 0x3011 register which is not present in imx290_global_init_settings_290.
> It is however included in imx290_global_init_settings, and set to 0x02.
> Could we therefore use imx290_global_init_settings_290 for the imx462 ?

I'd done a comparison of the datasheets, and register 0x3011 was the
only one that changed. I'd missed that it was in
imx290_global_init_settings.

My datasheets:
IMX327LQR-C rev E17Z06B93 2019/03/25. 3011h "Set to 02h" (value
changed in doc rev 0.3 from 0Ah)
IMX290LQR-C rev E15510G82 2018/02/09. 3011h "Fixed to 00h" (always
been that value).
IMX462LQR-C rev E19Y13C13 2021/03/19. 3011h "Set to 02h" (value
changed in doc rev 0.2 from 00h)
The default value stated in all of them is 00h. In true Sony fashion,
there's no description for that register functionality.

So actually it looks like it was the addition of IMX327 in [1] should
have changed that setting, unless someone else has a more recent
datasheet for IMX290 that updates that.
cc Alexander as the author of that patch. I'll find any discussion on it later.

  Dave

[1] https://github.com/torvalds/linux/commit/2d41947ec2c0140c65783982692c2e3d89853c47

> > +     { CCI_REG8(0x3016), 0x09 },
> > +     { CCI_REG8(0x3070), 0x02 },
> > +     { CCI_REG8(0x3071), 0x11 },
> > +     { CCI_REG8(0x309b), 0x10 },
> > +     { CCI_REG8(0x309c), 0x22 },
> > +     { CCI_REG8(0x30a2), 0x02 },
> > +     { CCI_REG8(0x30a6), 0x20 },
> > +     { CCI_REG8(0x30a8), 0x20 },
> > +     { CCI_REG8(0x30aa), 0x20 },
> > +     { CCI_REG8(0x30ac), 0x20 },
> > +     { CCI_REG8(0x30b0), 0x43 },
> > +     { CCI_REG8(0x3119), 0x9e },
> > +     { CCI_REG8(0x311c), 0x1e },
> > +     { CCI_REG8(0x311e), 0x08 },
> > +     { CCI_REG8(0x3128), 0x05 },
> > +     { CCI_REG8(0x313d), 0x83 },
> > +     { CCI_REG8(0x3150), 0x03 },
> > +     { CCI_REG8(0x317e), 0x00 },
> > +     { CCI_REG8(0x32b8), 0x50 },
> > +     { CCI_REG8(0x32b9), 0x10 },
> > +     { CCI_REG8(0x32ba), 0x00 },
> > +     { CCI_REG8(0x32bb), 0x04 },
> > +     { CCI_REG8(0x32c8), 0x50 },
> > +     { CCI_REG8(0x32c9), 0x10 },
> > +     { CCI_REG8(0x32ca), 0x00 },
> > +     { CCI_REG8(0x32cb), 0x04 },
> > +     { CCI_REG8(0x332c), 0xd3 },
> > +     { CCI_REG8(0x332d), 0x10 },
> > +     { CCI_REG8(0x332e), 0x0d },
> > +     { CCI_REG8(0x3358), 0x06 },
> > +     { CCI_REG8(0x3359), 0xe1 },
> > +     { CCI_REG8(0x335a), 0x11 },
> > +     { CCI_REG8(0x3360), 0x1e },
> > +     { CCI_REG8(0x3361), 0x61 },
> > +     { CCI_REG8(0x3362), 0x10 },
> > +     { CCI_REG8(0x33b0), 0x50 },
> > +     { CCI_REG8(0x33b2), 0x1a },
> > +     { CCI_REG8(0x33b3), 0x04 },
> > +};
> > +
> >  #define IMX290_NUM_CLK_REGS  2
> >  static const struct cci_reg_sequence xclk_regs[][IMX290_NUM_CLK_REGS] = {
> >       [IMX290_CLK_37_125] = {
> > @@ -1455,6 +1501,20 @@ static const struct imx290_model_info imx290_models[] = {
> >               .max_analog_gain = 98,
> >               .name = "imx327",
> >       },
> > +     [IMX290_MODEL_IMX462LQR] = {
> > +             .colour_variant = IMX290_VARIANT_COLOUR,
> > +             .init_regs = imx290_global_init_settings_462,
> > +             .init_regs_num = ARRAY_SIZE(imx290_global_init_settings_462),
> > +             .max_analog_gain = 98,
> > +             .name = "imx462",
> > +     },
> > +     [IMX290_MODEL_IMX462LLR] = {
> > +             .colour_variant = IMX290_VARIANT_MONO,
> > +             .init_regs = imx290_global_init_settings_462,
> > +             .init_regs_num = ARRAY_SIZE(imx290_global_init_settings_462),
> > +             .max_analog_gain = 98,
> > +             .name = "imx462",
> > +     },
> >  };
> >
> >  static int imx290_parse_dt(struct imx290 *imx290)
> > @@ -1653,6 +1713,12 @@ static const struct of_device_id imx290_of_match[] = {
> >       }, {
> >               .compatible = "sony,imx327lqr",
> >               .data = &imx290_models[IMX290_MODEL_IMX327LQR],
> > +     }, {
> > +             .compatible = "sony,imx462lqr",
> > +             .data = &imx290_models[IMX290_MODEL_IMX462LQR],
> > +     }, {
> > +             .compatible = "sony,imx462llr",
> > +             .data = &imx290_models[IMX290_MODEL_IMX462LLR],
> >       },
> >       { /* sentinel */ },
> >  };
>
> --
> Regards,
>
> Laurent Pinchart
Re: [PATCH 3/3] media: i2c: imx290: Add configuration for IMX462
Posted by Laurent Pinchart 4 days, 23 hours ago 
Hi Dave,

On Fri, Nov 15, 2024 at 08:51:55AM +0000, Dave Stevenson wrote:
> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart wrote:
> > On Thu, Nov 14, 2024 at 04:01:15PM +0000, Dave Stevenson wrote:
> > > IMX462 is the successor to IMX290, and wants very minor
> > > changes to the register setup.
> > >
> > > Add the relevant configuration to support it.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/imx290.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index da654deb444a..f1780cc5d7cc 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -170,6 +170,8 @@ enum imx290_model {
> > >       IMX290_MODEL_IMX290LQR,
> > >       IMX290_MODEL_IMX290LLR,
> > >       IMX290_MODEL_IMX327LQR,
> > > +     IMX290_MODEL_IMX462LQR,
> > > +     IMX290_MODEL_IMX462LLR,
> > >  };
> > >
> > >  struct imx290_model_info {
> > > @@ -316,6 +318,50 @@ static const struct cci_reg_sequence imx290_global_init_settings_290[] = {
> > >       { CCI_REG8(0x33b3), 0x04 },
> > >  };
> > >
> > > +static const struct cci_reg_sequence imx290_global_init_settings_462[] = {
> > > +     { CCI_REG8(0x300f), 0x00 },
> > > +     { CCI_REG8(0x3010), 0x21 },
> > > +     { CCI_REG8(0x3011), 0x02 },
> >
> > As far as I can tell, the only difference in the init sequence between
> > imx290_global_init_settings_290 and imx290_global_init_settings_462 is
> > 0x3011 register which is not present in imx290_global_init_settings_290.
> > It is however included in imx290_global_init_settings, and set to 0x02.
> > Could we therefore use imx290_global_init_settings_290 for the imx462 ?
> 
> I'd done a comparison of the datasheets, and register 0x3011 was the
> only one that changed. I'd missed that it was in
> imx290_global_init_settings.
> 
> My datasheets:
> IMX327LQR-C rev E17Z06B93 2019/03/25. 3011h "Set to 02h" (value
> changed in doc rev 0.3 from 0Ah)
> IMX290LQR-C rev E15510G82 2018/02/09. 3011h "Fixed to 00h" (always
> been that value).
> IMX462LQR-C rev E19Y13C13 2021/03/19. 3011h "Set to 02h" (value
> changed in doc rev 0.2 from 00h)
> The default value stated in all of them is 00h. In true Sony fashion,
> there's no description for that register functionality.
> 
> So actually it looks like it was the addition of IMX327 in [1] should
> have changed that setting, unless someone else has a more recent
> datasheet for IMX290 that updates that.

I agree with this analysis. It may be that setting the register to 0x02
would be fine, but it's hard to tell. Maybe it could be worth asking
Sony ?

> cc Alexander as the author of that patch. I'll find any discussion on it later.
> 
>   Dave
> 
> [1] https://github.com/torvalds/linux/commit/2d41947ec2c0140c65783982692c2e3d89853c47
> 
> > > +     { CCI_REG8(0x3016), 0x09 },
> > > +     { CCI_REG8(0x3070), 0x02 },
> > > +     { CCI_REG8(0x3071), 0x11 },
> > > +     { CCI_REG8(0x309b), 0x10 },
> > > +     { CCI_REG8(0x309c), 0x22 },
> > > +     { CCI_REG8(0x30a2), 0x02 },
> > > +     { CCI_REG8(0x30a6), 0x20 },
> > > +     { CCI_REG8(0x30a8), 0x20 },
> > > +     { CCI_REG8(0x30aa), 0x20 },
> > > +     { CCI_REG8(0x30ac), 0x20 },
> > > +     { CCI_REG8(0x30b0), 0x43 },
> > > +     { CCI_REG8(0x3119), 0x9e },
> > > +     { CCI_REG8(0x311c), 0x1e },
> > > +     { CCI_REG8(0x311e), 0x08 },
> > > +     { CCI_REG8(0x3128), 0x05 },
> > > +     { CCI_REG8(0x313d), 0x83 },
> > > +     { CCI_REG8(0x3150), 0x03 },
> > > +     { CCI_REG8(0x317e), 0x00 },
> > > +     { CCI_REG8(0x32b8), 0x50 },
> > > +     { CCI_REG8(0x32b9), 0x10 },
> > > +     { CCI_REG8(0x32ba), 0x00 },
> > > +     { CCI_REG8(0x32bb), 0x04 },
> > > +     { CCI_REG8(0x32c8), 0x50 },
> > > +     { CCI_REG8(0x32c9), 0x10 },
> > > +     { CCI_REG8(0x32ca), 0x00 },
> > > +     { CCI_REG8(0x32cb), 0x04 },
> > > +     { CCI_REG8(0x332c), 0xd3 },
> > > +     { CCI_REG8(0x332d), 0x10 },
> > > +     { CCI_REG8(0x332e), 0x0d },
> > > +     { CCI_REG8(0x3358), 0x06 },
> > > +     { CCI_REG8(0x3359), 0xe1 },
> > > +     { CCI_REG8(0x335a), 0x11 },
> > > +     { CCI_REG8(0x3360), 0x1e },
> > > +     { CCI_REG8(0x3361), 0x61 },
> > > +     { CCI_REG8(0x3362), 0x10 },
> > > +     { CCI_REG8(0x33b0), 0x50 },
> > > +     { CCI_REG8(0x33b2), 0x1a },
> > > +     { CCI_REG8(0x33b3), 0x04 },
> > > +};
> > > +
> > >  #define IMX290_NUM_CLK_REGS  2
> > >  static const struct cci_reg_sequence xclk_regs[][IMX290_NUM_CLK_REGS] = {
> > >       [IMX290_CLK_37_125] = {
> > > @@ -1455,6 +1501,20 @@ static const struct imx290_model_info imx290_models[] = {
> > >               .max_analog_gain = 98,
> > >               .name = "imx327",
> > >       },
> > > +     [IMX290_MODEL_IMX462LQR] = {
> > > +             .colour_variant = IMX290_VARIANT_COLOUR,
> > > +             .init_regs = imx290_global_init_settings_462,
> > > +             .init_regs_num = ARRAY_SIZE(imx290_global_init_settings_462),
> > > +             .max_analog_gain = 98,
> > > +             .name = "imx462",
> > > +     },
> > > +     [IMX290_MODEL_IMX462LLR] = {
> > > +             .colour_variant = IMX290_VARIANT_MONO,
> > > +             .init_regs = imx290_global_init_settings_462,
> > > +             .init_regs_num = ARRAY_SIZE(imx290_global_init_settings_462),
> > > +             .max_analog_gain = 98,
> > > +             .name = "imx462",
> > > +     },
> > >  };
> > >
> > >  static int imx290_parse_dt(struct imx290 *imx290)
> > > @@ -1653,6 +1713,12 @@ static const struct of_device_id imx290_of_match[] = {
> > >       }, {
> > >               .compatible = "sony,imx327lqr",
> > >               .data = &imx290_models[IMX290_MODEL_IMX327LQR],
> > > +     }, {
> > > +             .compatible = "sony,imx462lqr",
> > > +             .data = &imx290_models[IMX290_MODEL_IMX462LQR],
> > > +     }, {
> > > +             .compatible = "sony,imx462llr",
> > > +             .data = &imx290_models[IMX290_MODEL_IMX462LLR],
> > >       },
> > >       { /* sentinel */ },
> > >  };

-- 
Regards,

Laurent Pinchart
Re: [PATCH 3/3] media: i2c: imx290: Add configuration for IMX462
Posted by Dave Stevenson 2 days, 8 hours ago 
Hi Laurent

On Mon, 18 Nov 2024 at 02:07, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> On Fri, Nov 15, 2024 at 08:51:55AM +0000, Dave Stevenson wrote:
> > On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart wrote:
> > > On Thu, Nov 14, 2024 at 04:01:15PM +0000, Dave Stevenson wrote:
> > > > IMX462 is the successor to IMX290, and wants very minor
> > > > changes to the register setup.
> > > >
> > > > Add the relevant configuration to support it.
> > > >
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > ---
> > > >  drivers/media/i2c/imx290.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 66 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index da654deb444a..f1780cc5d7cc 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -170,6 +170,8 @@ enum imx290_model {
> > > >       IMX290_MODEL_IMX290LQR,
> > > >       IMX290_MODEL_IMX290LLR,
> > > >       IMX290_MODEL_IMX327LQR,
> > > > +     IMX290_MODEL_IMX462LQR,
> > > > +     IMX290_MODEL_IMX462LLR,
> > > >  };
> > > >
> > > >  struct imx290_model_info {
> > > > @@ -316,6 +318,50 @@ static const struct cci_reg_sequence imx290_global_init_settings_290[] = {
> > > >       { CCI_REG8(0x33b3), 0x04 },
> > > >  };
> > > >
> > > > +static const struct cci_reg_sequence imx290_global_init_settings_462[] = {
> > > > +     { CCI_REG8(0x300f), 0x00 },
> > > > +     { CCI_REG8(0x3010), 0x21 },
> > > > +     { CCI_REG8(0x3011), 0x02 },
> > >
> > > As far as I can tell, the only difference in the init sequence between
> > > imx290_global_init_settings_290 and imx290_global_init_settings_462 is
> > > 0x3011 register which is not present in imx290_global_init_settings_290.
> > > It is however included in imx290_global_init_settings, and set to 0x02.
> > > Could we therefore use imx290_global_init_settings_290 for the imx462 ?
> >
> > I'd done a comparison of the datasheets, and register 0x3011 was the
> > only one that changed. I'd missed that it was in
> > imx290_global_init_settings.
> >
> > My datasheets:
> > IMX327LQR-C rev E17Z06B93 2019/03/25. 3011h "Set to 02h" (value
> > changed in doc rev 0.3 from 0Ah)
> > IMX290LQR-C rev E15510G82 2018/02/09. 3011h "Fixed to 00h" (always
> > been that value).
> > IMX462LQR-C rev E19Y13C13 2021/03/19. 3011h "Set to 02h" (value
> > changed in doc rev 0.2 from 00h)
> > The default value stated in all of them is 00h. In true Sony fashion,
> > there's no description for that register functionality.
> >
> > So actually it looks like it was the addition of IMX327 in [1] should
> > have changed that setting, unless someone else has a more recent
> > datasheet for IMX290 that updates that.
>
> I agree with this analysis. It may be that setting the register to 0x02
> would be fine, but it's hard to tell. Maybe it could be worth asking
> Sony ?
>
> > cc Alexander as the author of that patch. I'll find any discussion on it later.

I've looked back to find the earlier discussion.

v3 at [1] was the version that was merged. This added register 0x3011
when previously it hadn't been set.

I had picked up at v2[2] that register 0x3011 should have been in
imx290_global_init_settings_327 rather than
imx290_global_init_settings, but that appeared not to have happened.

In my book that makes it a regression for imx290 due to that patch,
and we should correct it back to 0x00.
I could check with Sony over that, but it seems overkill seeing as we
have deviated from the originally submitted driver in a way that
contradicts the datasheet.

I'll send a v2 patchset on that basis.

  Dave

[1] https://lore.kernel.org/linux-media/20230217095221.499463-3-alexander.stein@ew.tq-group.com/
[2] https://lore.kernel.org/linux-media/CAPY8ntB_25yge6MB87N642-bMG-hd9qCVkom4A-c-pBzk3a4mQ@mail.gmail.com/

> >   Dave
> >
> > [1] https://github.com/torvalds/linux/commit/2d41947ec2c0140c65783982692c2e3d89853c47
> >
> > > > +     { CCI_REG8(0x3016), 0x09 },
> > > > +     { CCI_REG8(0x3070), 0x02 },
> > > > +     { CCI_REG8(0x3071), 0x11 },
> > > > +     { CCI_REG8(0x309b), 0x10 },
> > > > +     { CCI_REG8(0x309c), 0x22 },
> > > > +     { CCI_REG8(0x30a2), 0x02 },
> > > > +     { CCI_REG8(0x30a6), 0x20 },
> > > > +     { CCI_REG8(0x30a8), 0x20 },
> > > > +     { CCI_REG8(0x30aa), 0x20 },
> > > > +     { CCI_REG8(0x30ac), 0x20 },
> > > > +     { CCI_REG8(0x30b0), 0x43 },
> > > > +     { CCI_REG8(0x3119), 0x9e },
> > > > +     { CCI_REG8(0x311c), 0x1e },
> > > > +     { CCI_REG8(0x311e), 0x08 },
> > > > +     { CCI_REG8(0x3128), 0x05 },
> > > > +     { CCI_REG8(0x313d), 0x83 },
> > > > +     { CCI_REG8(0x3150), 0x03 },
> > > > +     { CCI_REG8(0x317e), 0x00 },
> > > > +     { CCI_REG8(0x32b8), 0x50 },
> > > > +     { CCI_REG8(0x32b9), 0x10 },
> > > > +     { CCI_REG8(0x32ba), 0x00 },
> > > > +     { CCI_REG8(0x32bb), 0x04 },
> > > > +     { CCI_REG8(0x32c8), 0x50 },
> > > > +     { CCI_REG8(0x32c9), 0x10 },
> > > > +     { CCI_REG8(0x32ca), 0x00 },
> > > > +     { CCI_REG8(0x32cb), 0x04 },
> > > > +     { CCI_REG8(0x332c), 0xd3 },
> > > > +     { CCI_REG8(0x332d), 0x10 },
> > > > +     { CCI_REG8(0x332e), 0x0d },
> > > > +     { CCI_REG8(0x3358), 0x06 },
> > > > +     { CCI_REG8(0x3359), 0xe1 },
> > > > +     { CCI_REG8(0x335a), 0x11 },
> > > > +     { CCI_REG8(0x3360), 0x1e },
> > > > +     { CCI_REG8(0x3361), 0x61 },
> > > > +     { CCI_REG8(0x3362), 0x10 },
> > > > +     { CCI_REG8(0x33b0), 0x50 },
> > > > +     { CCI_REG8(0x33b2), 0x1a },
> > > > +     { CCI_REG8(0x33b3), 0x04 },
> > > > +};
> > > > +
> > > >  #define IMX290_NUM_CLK_REGS  2
> > > >  static const struct cci_reg_sequence xclk_regs[][IMX290_NUM_CLK_REGS] = {
> > > >       [IMX290_CLK_37_125] = {
> > > > @@ -1455,6 +1501,20 @@ static const struct imx290_model_info imx290_models[] = {
> > > >               .max_analog_gain = 98,
> > > >               .name = "imx327",
> > > >       },
> > > > +     [IMX290_MODEL_IMX462LQR] = {
> > > > +             .colour_variant = IMX290_VARIANT_COLOUR,
> > > > +             .init_regs = imx290_global_init_settings_462,
> > > > +             .init_regs_num = ARRAY_SIZE(imx290_global_init_settings_462),
> > > > +             .max_analog_gain = 98,
> > > > +             .name = "imx462",
> > > > +     },
> > > > +     [IMX290_MODEL_IMX462LLR] = {
> > > > +             .colour_variant = IMX290_VARIANT_MONO,
> > > > +             .init_regs = imx290_global_init_settings_462,
> > > > +             .init_regs_num = ARRAY_SIZE(imx290_global_init_settings_462),
> > > > +             .max_analog_gain = 98,
> > > > +             .name = "imx462",
> > > > +     },
> > > >  };
> > > >
> > > >  static int imx290_parse_dt(struct imx290 *imx290)
> > > > @@ -1653,6 +1713,12 @@ static const struct of_device_id imx290_of_match[] = {
> > > >       }, {
> > > >               .compatible = "sony,imx327lqr",
> > > >               .data = &imx290_models[IMX290_MODEL_IMX327LQR],
> > > > +     }, {
> > > > +             .compatible = "sony,imx462lqr",
> > > > +             .data = &imx290_models[IMX290_MODEL_IMX462LQR],
> > > > +     }, {
> > > > +             .compatible = "sony,imx462llr",
> > > > +             .data = &imx290_models[IMX290_MODEL_IMX462LLR],
> > > >       },
> > > >       { /* sentinel */ },
> > > >  };
>
> --
> Regards,
>
> Laurent Pinchart

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.