Hi Hans On Thu, May 09, 2024 at 03:42:25PM +0200, Hans de Goede wrote: > 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. With the both patches I have quite good reproducible "chip id mismatch" problem. I test patch 1 and patch 2 separately and both of them together. Unfortunately non combination fixes the issues: mismatch and steam start and some other problems with ov2740 on my lenovo x1 laptop Looks previously I get good result with original patch just by sheer luck. Those are random problems and reuire more testing before confirmation of fix. So sadly I have to scratch result that original patch fixes chip mishmash id issue :-( Problems need to be properly debugged and require deeper investigation. Regards Stanislaw
