Hi Jay On Fri, Dec 30, 2022 at 02:33:41PM +0530, Luthra, Jai wrote: > Hi Jacopo, > > Thank you for the comments. > > On 29/12/22 23:37, Jacopo Mondi wrote: > > 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. > > Yes, I initially removed it from here and kept it in the powerup_sequence() > but logic to set SW_PWDN became in that function became a bit complicated: > > When we are using HW pins, we want to wait 20ms after de-asserting RESET > before doing any SCCB register writes. But when we are using SW reset, we > were using this register write to de-assert RESET, and then wait 20ms. > > I found it cleaner to just keep it here where the registers are being > programmed for both the cases. But I don't have a strong opinion either way. > > > > > > > > > > {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. > > Using the PWUP (0x02) as a way to de-assert RST (0x42) here, and not as an RST is 0x82 if I read it right > actual PWUP per se. > It is not necessary. The RST cleans up itself when the chip has done resetting. I tested it with: @@ -2463,12 +2478,24 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor) gpiod_set_value_cansleep(sensor->reset_gpio, 0); } else { + int ret; + u16 sysctrl = 0; + u16 counter = 0; + /* 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); + + do { + usleep_range(2000, 2500); + ret = ov5640_read_reg16(sensor, OV5640_REG_SYS_CTRL0, + &sysctrl); + if (ret) + pr_err("Failed to read sysctrl: %d\n", ret); + } while (sysctrl & BIT(7) || ++counter > 100); + + pr_err("Exit from reset in %u loops\n", counter); } usleep_range(20000, 25000); The loop exists after 1 iteration So I guess you can remove the last SW_PWUP write > > > > > > > + } > > > usleep_range(20000, 25000); > > Now that I think of it, we can move the PWDN from the register table to > here, after this sleep - and keep the (0x02) above to de-assert reset. Let > me know your thoughts. > Yes, I think it might be a good idea. The diff on top of your series should be like ------------------------------------------------------------------------------ -- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -533,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, 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}, @@ -2466,11 +2466,12 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor) /* 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); } usleep_range(20000, 25000); + + /* Software standby: allows registers programming; exit at s_stream(1) */ + ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, + OV5640_REG_SYS_CTRL0_SW_PWDN); } static int ov5640_set_power_on(struct ov5640_dev *sensor) ------------------------------------------------------------------------------ Do you agree with this ? Also, since you have to resend, could you please swap the order of patches 1 and 2 ? It's more logically correct to plumb the SW reset handling in, then allow it to happen as you do in patch 1/3. > > > } > > > > > > -- > > > 2.17.1 > > > > > Thanks, > Jai
