Hi Sakari,
Thanks for the feedback.
> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Sent: Wednesday, February 14, 2024 8:49 PM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from
> ov5645_global_init_setting
>
> Hi Biju,
>
> On Wed, Feb 14, 2024 at 08:25:16PM +0000, Biju Das wrote:
> > Hi Wolfram,
> >
> > Thanks for the feedback.
> >
> > > -----Original Message-----
> > > From: Wolfram Sang <wsa@xxxxxxxxxx>
> > > Sent: Wednesday, February 14, 2024 8:31 AM
> > > Subject: Re: [PATCH 1/2] media: i2c: ov5645: Move the register
> > > 0x3008 from ov5645_global_init_setting
> > >
> > > Hi Biju,
> > >
> > > > I think it is different here. That 1 msec is delay associated with
> > > > applying hardware power see [1]
> > >
> > > Okay, ack.
> > >
> > > > I will restore it.
> > >
> > > Thanks!
> > >
> > > I had meanwhile another thought. What if we kind of merge the two
> > > patches, so the outcome is basically this:
> > >
> > > In ov5645_set_register_array:
> > >
> > > If (settings->reg == 0x3008 && settings->val == 0x82)
> > > usleep_range(1000, 2000)
> > >
> > > ?
> > >
> > > Then, we don't need to split the array and we are also future proof
> > > if we ever need to set the reset bit again somewhere else.
> > >
> > > Bonus points for replacing 0x82 with a define :)
> > >
> > > What do you think?
> >
> >
> > OK, this will do check for all other registers.
> >
> > But from your power down clue and checking ov5640.c Looks like there
> > are 2 registers changes values after writing.
> >
> > [1] 0x3008, 0x82-->0x80
> > [2] 0x0601, 0x02-->0x00
> >
> > I think [1] is soft reset based on ov5640. Since there is a gpio based
> > hardware reset available, we can safely remove soft reset[1] and like
> > ov5640.c, if there is no gpio for reset, then do the soft reset[1].
>
> I guess that would work. My understanding is that hard reset control is
> mandatory for the device, so there really should be no need for soft reset
> in the driver.
OK.
>
> >
> >
> > Then add 1msec delay for power down/up(0x3008: 0x42,0x02) and 0x0601
> > registers.
> >
> > With this looks like the Camera works ok @400kHz.
> >
> > The plans is to add a u8 variable for delay and enable delays for the
> > above registers and add a check like below
> >
> > static int ov5645_set_register_array(struct ov5645 *ov5645,
> > const struct reg_value *settings,
> > unsigned int num_settings)
> > {
> > unsigned int i;
> > int ret;
> >
> > for (i = 0; i < num_settings; ++i, ++settings) {
> > ret = ov5645_write_reg(ov5645, settings->reg, settings->val);
> > if (ret < 0)
> > return ret;
> >
> > if (settings->delay_ms)
> > usleep_range(1000 * settings->delay_ms, 2 * 1000 *
> > settings->delay_ms);
>
> I'd prefer checking the register address in the write function instead of
> this if you really need it. But it seems you don't.
With delays in powerup/down registers (0x3008 : 0x42,0x02) it is not stable.
So, as you and wolfram mentioned will check the register address instead.
I will post the patch, after testing on various platforms(RZ/{G2L,G2LC,V2L,G2UL}
with 400kHz and 100 kHz
Cheers,
Biju
>
> > }
> >
> > return 0;
> > }
> >
> > Please share your thoughts on this approach.
> >
> > Cheers,
> > Biju
>
> --
> Regards,
>
> Sakari Ailus
