Hi Jacopo,
Thanks for you replying.
> -----Original Message-----
> From: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Sent: 2023年6月7日 15:58
> To: G.N. Zhou <guoniu.zhou@xxxxxxx>
> Cc: G.N. Zhou (OSS) <guoniu.zhou@xxxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> mchehab@xxxxxxxxxx; slongerbeam@xxxxxxxxx; jacopo@xxxxxxxxxx;
> sakari.ailus@xxxxxxxxxxxxxxx; laurent.pinchart@xxxxxxxxxxxxxxxx
> Subject: [EXT] Re: [PATCH] media: ov5640: fix low resolution image abnormal
> issue
>
> Caution: This is an external email. Please take care when clicking links or opening
> attachments. When in doubt, report the message using the 'Report this email'
> button
>
>
> Hello Guoniu Zhou
>
> On Wed, Jun 07, 2023 at 06:26:22AM +0000, G.N. Zhou wrote:
> > Hi ALL,
> >
> > Is there any comments or update? I will appreciate that if there are any.
>
> You're very right, sorry about the delay
>
> >
> > Best Regards
> > G.N Zhou
> >
> >
> > > -----Original Message-----
> > > From: G.N. Zhou (OSS)
> > > Sent: 2023年5月18日 18:01
> > > To: linux-media@xxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx;
> > > slongerbeam@xxxxxxxxx; jacopo@xxxxxxxxxx;
> > > sakari.ailus@xxxxxxxxxxxxxxx
> > > Cc: laurent.pinchart@xxxxxxxxxxxxxxxx
> > > Subject: [PATCH] media: ov5640: fix low resolution image abnormal
> > > issue
> > >
> > > From: "Guoniu.zhou" <guoniu.zhou@xxxxxxx>
> > >
> > > OV5640 will output abnormal image data when work at low resolution
> > > (320x240,
> > > 176x144 and 160x120) after switching from high resolution, such as
> > > 1080P, the time interval between high and low switching must be less
> > > than 1000ms in order to OV5640 don't enter suspend state during the time.
> > >
>
> Thanks for finding this out, I presume it took quite some effort to dig that register
> out.
>
> However I don't have the register documented anywhere. Do you ?
I only find a mention about 0x3824 in table 7-17 (" DVP PCLK divider control by 0x3824[4:0] ") in datasheet as bellow:
https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
>
> > > The reason is by 0x3824 value don't restore to initialize value when
> > > do resolution switching. In high resolution setting array, 0x3824 is
> > > set to 0x04, but low
>
> Why I do see:
> ov5640_setting_QSXGA_2592_1944[] = { ... {0x3824, 0x02, 0, 0},.. };
>
> Have you tested switching to full-resolution mode to a lower resolution ?
Sorry I tested all resolutions except 2592x1944, but I can do that later.
>
> > > resolution setting array remove 0x3824 in commit db15c1957a2d ("media:
> > > ov5640: Remove duplicated mode settings"). So when do resolution
> > > switching from high to low, such as 1080P to 320x240, and the time
> > > interval is less than auto suspend delay time which means global
> > > initialize setting array will not be loaded, the output image data are
> abnormal.
> > >
>
> Ok, this was possibily either a micro-optimization or a plain mistake as I do see in
> commit db15c1957a2d the ov5640_setting_low_res[] array being introduced,
> but compared to the register tables it replaces it is missing:
>
> {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> {0x3824, 0x02, 0, 0}
>
> These registers are already programmed with these values by
> ov5640_init_setting[], that might be the reason I left them out from
> ov5640_setting_low_res[].
>
> But as you have correctly noticed, switching between modes doesn't go through
> ov5640_init_setting[] (of course) so I can only conclude the above register
> should all be re-introduced in ov5640_setting_low_res[]. What do you think ?
Sure, if so, it can save some time. Let me have a check and if it work fine, I will send v2 patch to update them. Thanks for your suggestion.
>
> Thanks again!
>
>
> > > Signed-off-by: Guoniu.zhou <guoniu.zhou@xxxxxxx>
> > > ---
> > > drivers/media/i2c/ov5640.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > index 1536649b9e90..b1a4565fdc0f 100644
> > > --- a/drivers/media/i2c/ov5640.c
> > > +++ b/drivers/media/i2c/ov5640.c
> > > @@ -634,7 +634,7 @@ static const struct reg_value
> > > ov5640_setting_low_res[] = {
> > > {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
> > > {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> > > {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
> > > - {0x4407, 0x04, 0, 0}, {0x5001, 0xa3, 0, 0},
> > > + {0x4407, 0x04, 0, 0}, {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0,
> > > + 0},
> > > };
> > >
> > > static const struct reg_value ov5640_setting_720P_1280_720[] = {
> > > --
> > > 2.37.1
> >
