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.
> - usleep_range(5000, 10000);
> + usleep_range(5000, 10000); /* t2 */
> ov5640_power(sensor, true);
> - usleep_range(5000, 10000);
> -
> - 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 ?
Thanks
j
>
> /*
> * software standby: allows registers programming;
> --
> 2.40.1
>
