Hi Stanislaw, On 5/9/24 1:42 PM, Stanislaw Gruszka wrote: > On Mon, May 06, 2024 at 03:24:38PM +0200, Hans de Goede wrote: >> Before this commit on probe() the driver would do: >> >> reset=1 // from probe() calling gpiod_get(GPIOD_OUT_HIGH) >> reset=0 // from resume() >> msleep(20) // from resume() >> >> So if reset was 0 before getting the GPIO the reset line would only be >> driven high for a very short time and sometimes there would be errors >> reading the id register afterwards. >> >> Add a msleep(20) after getting the reset line to ensure the sensor is >> properly reset: >> >> reset=1 // from probe() calling gpiod_get(GPIOD_OUT_HIGH) >> msleep(20) // from probe() >> reset=0 // from resume() >> msleep(20) // from resume() >> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > Tested-by: Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx> > > This fixes this issue: > > [ 7.742633] ov2740 i2c-INT3474:01: chip id mismatch: 2740 != 0 > [ 7.742638] ov2740 i2c-INT3474:01: error -ENXIO: failed to find sensor > > for me as well. Thank you for testing. However there is still the issue of the sensor not always starting to stream reliably being discussed here: https://github.com/intel/ipu6-drivers/issues/187 I have been thinking about this and I think that something like this should fix this, can you give this a try (with the patch to disable runtime-pm reverted) : diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index c48dbcde9877..58818bcfe086 100644 --- a/drivers/media/i2c/ov2740.c +++ b/drivers/media/i2c/ov2740.c @@ -1294,7 +1294,14 @@ static int ov2740_suspend(struct device *dev) struct ov2740 *ov2740 = to_ov2740(sd); gpiod_set_value_cansleep(ov2740->reset_gpio, 1); + /* + * Ensure reset is asserted for at least 20 ms before disabling the clk. + * This also assures reset is properly asserted for 20 ms when a runtime + * suspend is immediately followed by a runtime resume. + */ + msleep(20); clk_disable_unprepare(ov2740->clk); + return 0; } @@ -1308,6 +1315,9 @@ static int ov2740_resume(struct device *dev) if (ret) return ret; + /* Give clk time to stabilize */ + msleep(20); + gpiod_set_value_cansleep(ov2740->reset_gpio, 0); msleep(20); Hopefully this will fix the start / stop stream issues when runtime pm is enabled. Regards, Hans >> --- >> drivers/media/i2c/ov2740.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c >> index 57906df7be4e..c48dbcde9877 100644 >> --- a/drivers/media/i2c/ov2740.c >> +++ b/drivers/media/i2c/ov2740.c >> @@ -1333,9 +1333,16 @@ static int ov2740_probe(struct i2c_client *client) >> return dev_err_probe(dev, ret, "failed to check HW configuration\n"); >> >> ov2740->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); >> - if (IS_ERR(ov2740->reset_gpio)) >> + if (IS_ERR(ov2740->reset_gpio)) { >> return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio), >> "failed to get reset GPIO\n"); >> + } else if (ov2740->reset_gpio) { >> + /* >> + * Ensure reset is asserted for at least 20 ms before >> + * ov2740_resume() deasserts it. >> + */ >> + msleep(20); >> + } >> >> ov2740->clk = devm_clk_get_optional(dev, "clk"); >> if (IS_ERR(ov2740->clk)) >> -- >> 2.44.0 >> >
