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 ?
> > 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 ?
> > 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 ?
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
>
