Hi Jai On Tue, Dec 27, 2022 at 11:06:33PM +0530, Jai Luthra wrote: > Move the register-based reset out of the init_setting[] and into the > powerup_sequence function. The sensor is power cycled and reset using > the gpio pins so the soft reset is not always necessary. > > This also ensures that soft reset honors the timing sequence > from the datasheet [1]. > > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf > > Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") > Suggested-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> > Signed-off-by: Jai Luthra <j-luthra@xxxxxx> > --- > drivers/media/i2c/ov5640.c | 30 ++++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index e6525caa9b8c..5df16fb6f0a0 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -50,6 +50,7 @@ > #define OV5640_REG_SYS_CTRL0 0x3008 > #define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 > #define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 > +#define OV5640_REG_SYS_CTRL0_SW_RST 0x82 > #define OV5640_REG_CHIP_ID 0x300a > #define OV5640_REG_IO_MIPI_CTRL00 0x300e > #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 > @@ -532,7 +533,7 @@ static const struct v4l2_mbus_framefmt ov5640_default_fmt = { > }; > > static const struct reg_value ov5640_init_setting[] = { > - {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0}, > + {0x3103, 0x11, 0, 0}, {0x3008, 0x42, 0, 0}, have you tried removing the second entry {0x3008, 0x42, 0, 0}, as well ? The SW_PWDN state allows programming registers (how can you exit with SW_PWUP otherwise?), so it's probably right to have it there. > {0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0}, > {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0}, > {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0}, > @@ -2440,18 +2441,27 @@ static void ov5640_power(struct ov5640_dev *sensor, bool enable) > */ > static void ov5640_powerup_sequence(struct ov5640_dev *sensor) > { > - gpiod_set_value_cansleep(sensor->reset_gpio, 0); > + if (sensor->pwdn_gpio) { > + gpiod_set_value_cansleep(sensor->reset_gpio, 0); > > - /* camera power cycle */ > - ov5640_power(sensor, false); > - usleep_range(5000, 10000); > - ov5640_power(sensor, true); > - usleep_range(5000, 10000); > + /* camera power cycle */ > + ov5640_power(sensor, false); > + usleep_range(5000, 10000); > + ov5640_power(sensor, true); > + usleep_range(5000, 10000); > > - gpiod_set_value_cansleep(sensor->reset_gpio, 1); > - usleep_range(1000, 2000); > + gpiod_set_value_cansleep(sensor->reset_gpio, 1); > + usleep_range(1000, 2000); > > - gpiod_set_value_cansleep(sensor->reset_gpio, 0); > + gpiod_set_value_cansleep(sensor->reset_gpio, 0); > + } else { > + /* software reset */ > + ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > + OV5640_REG_SYS_CTRL0_SW_RST); > + usleep_range(5000, 10000); > + ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > + OV5640_REG_SYS_CTRL0_SW_PWUP); But now I wonder if this last PWUP is necessary, since init_settings[] (which programs PWDN) is programmed immediately after. > + } > usleep_range(20000, 25000); > } > > -- > 2.17.1 >
