Hi Marek, Jacopo,
On Jul 25, 2023 at 11:09:20 +0200, Jacopo Mondi wrote:
> Cc Jai which has recently changed this part to accomodate a design
> where RESETB and PWDN are not exposed as separate lines
>
> On Tue, Jul 25, 2023 at 12:21:16AM +0200, Marek Vasut wrote:
> > The initial state of RESETB input signal of OV5640 should be
> > asserted, i.e. the sensor should be in reset. This is not the
> > case, make it so.
> >
> > Since the subsequent assertion of RESETB signal is no longer
> > necessary and the timing of the power sequencing could be
> > slightly adjusted, add annotations to the delays which match
> > OV5640 datasheet rev. 2.03, both:
> > figure 2-3 power up timing with internal DVDD
> > figure 2-4 power up timing with external DVDD source
> >
> > The 5..10ms delay between PWDN assertion and RESETB assertion
> > is not even documented in the power sequencing diagram, and
> > with this reset fix, it is no longer even necessary.
> >
> > Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
> > Reported-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Marek Vasut <marex@xxxxxxx>
> > ---
> > Cc: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> > Cc: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> > Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Cc: Steve Longerbeam <slongerbeam@xxxxxxxxx>
> > Cc: linux-media@xxxxxxxxxxxxxxx
> > ---
> > drivers/media/i2c/ov5640.c | 11 ++++-------
> > 1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 7c065c39082dd..74b58380b5e69 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -2452,16 +2452,13 @@ static void ov5640_power(struct ov5640_dev *sensor, bool enable)
> > static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
> > {
> > if (sensor->pwdn_gpio) {
> > - gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> > + gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> >
> > /* camera power cycle */
> > ov5640_power(sensor, false);
>
> This is also probably a NOP for most designs but the one Jai had, as
> in his case a single "powerdown" line controls both PWDN and RESETB
> with some internal circuitry that handles signals inversions and
> delays. So I guess it is fine to have it here.
>
Yes the modules in question are -
- Digilent PCam 5C
- ALINX AN5641
- TEVI-OV5640-*-RPI
All of them have a 15-pin FFC (RPi) compatible connector.
> > - usleep_range(5000, 10000);
> > + usleep_range(5000, 10000); /* t2 */
> > ov5640_power(sensor, true);
> > - usleep_range(5000, 10000);
I remember noticing this before, but chose to keep it around as the
above mentioned modules have a 5.5ms + 1ms delay mentioned in the
schematics [1] between toggling the weird "PWUP" gpio to the RESET
actually getting toggled.
But in my tests your patch is not breaking any of those modules, and
this delay does look very redundant from driver p.o.v. so I am happy
with removing it here.
In future if I face any issues I will try to come up with a better
approach that does not add redundant delays for modules that don't
perform magical delays on-board like those three special ones :)
[1]
https://digilent.com/reference/_media/reference/add-ons/pcam-5c/pcam_5c_sch.pdf
> > -
> > - gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> > - usleep_range(1000, 2000);
> > + usleep_range(1000, 2000); /* t3 */
> >
> > gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> > } else {
> > @@ -2469,7 +2466,7 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
> > ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
> > OV5640_REG_SYS_CTRL0_SW_RST);
> > }
> > - usleep_range(20000, 25000);
> > + usleep_range(20000, 25000); /* t4 */
>
>
> This all looks good to me
> Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
>
> Jai could you give this a run to make sure this works on your setup as
> well ?
>
Works on BeaglePlay and SK-AM62 (with my CSI v8 series applied on next)
Tested-by: Jai Luthra <j-luthra@xxxxxx>
> Thanks
> j
>
> >
> > /*
> > * software standby: allows registers programming;
> > --
> > 2.40.1
> >
>
--
Thanks,
Jai
GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145
Attachment:
signature.asc
Description: PGP signature
