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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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