RE: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting Biju Das (2024-02-14 20:57:36)
> Hi Sakari,
> 
> > -----Original Message-----
> > From: Biju Das
> > Sent: Wednesday, February 14, 2024 8:55 PM
> > Subject: RE: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from
> > ov5645_global_init_setting
> > 
> > Hi Sakari,
> > 
> > Thanks for the feedback.
> > 
> > > -----Original Message-----
> > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Sent: Wednesday, February 14, 2024 8:49 PM
> > > To: Biju Das <biju.das.jz@bp.renesas.com>
> > > 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@kernel.org>
> > > > > 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 :)

Pulling up the datasheet OV5645-A66A v1.1 from the web:

(a little google-fu++ got there, let me know if you need hints if you
want to do a full clean up)

2.8 reset

The OV5645 sensor includes a RESETB pin that forces a complete hardware
reset when it is pulled low (GND). The OV5645 clears all registers and
resets them to their default values when a hardware reset occurs. A
reset can also be initiated through the SCCB interface by setting
register 0x3008[7] to high.  Manually applying a hard reset upon power
up is required even though on-chip reset is included. The hard reset is
active low with an asynchronized design. The reset pulse width should be
greater than or equal to 1 ms.

2.9 hardware and software standby
Two suspend modes are available for the OV5645:

• hardware standby
• SCCB software standby

To initiate hardware standby mode, the PWDNB pin must be tied to low
(while in MIPI mode, set register 0x300E[4:3] to 2’b11 before the PWDNB
pin is set to low). When this occurs, the OV5645 internal device clock
is halted and all internal counters are reset and registers are
maintained.  Executing a software standby through the SCCB interface
suspends internal circuit activity but does not halt the device clock.
All register content is maintained in standby mode.


 Address: 0x3008 
 RegName: SYSTEM_CTROL0
 Default: 0x02
 R/W:	RW
Description: System Control
  Bit[7]: Software reset
  Bit[6]: Software power down

Then later in the datasheet it also describes:
System Control
Bit[7]: Software reset
Bit[6]: Software power down
Bit[5]: Debug mode
Bit[4]: SRB clock sync enable
Bit[3]: Isolation suspend select
Bit[2]: MIPI reset mask
Bit[1]: MIPI suspend mask
Bit[0]: MIPI reset select


> > > > >
> > > > > 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.
> 
> Typo: without delays in powerup/down registers (0x3008 : 0x42,0x02) it is not
> stable.
> 
> You can see 0x42 followed by 0x02 which is an indication of power down/up procedure.
> 
> Cheers,
> Biju