Hi Jai, bear with me a little longer.. On Tue, Jan 03, 2023 at 05:57:35PM +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") > Reported-by: Nishanth Menon <nm@xxxxxx> > Suggested-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> > Signed-off-by: Jai Luthra <j-luthra@xxxxxx> > --- > drivers/media/i2c/ov5640.c | 33 +++++++++++++++++++++++---------- > 1 file changed, 23 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index e0f908af581b..9f3913aad93c 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}, > {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}, > @@ -2429,19 +2430,31 @@ static void ov5640_reset(struct ov5640_dev *sensor) > if (!sensor->reset_gpio) > return; > > - 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(20000, 25000); > + > + /* software standby: allows registers programming; > + * exit at restore_mode() for CSI, s_stream(1) for DVP > + */ Multiline comments are usually written as /* * Software standby: allows registers programming; * exit at restore_mode() for CSI, s_stream(1) for DVP */ It's a trivial change, I'm not collecting patches so I can't offer to change it when doing so, but maybe Sakari could help with that so that you don't have to send a new version ? (if that's the only comment you receive ofc) The patch looks good to me Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxxx> > + ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > + OV5640_REG_SYS_CTRL0_SW_PWDN); > } > > static int ov5640_set_power_on(struct ov5640_dev *sensor) > -- > 2.17.1 >
