Hi Laurent
On Mon, 18 Nov 2024 at 02:07, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxx/
[2] https://lore.kernel.org/linux-media/CAPY8ntB_25yge6MB87N642-bMG-hd9qCVkom4A-c-pBzk3a4mQ@xxxxxxxxxxxxxx/
> > 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
