Message ID | 20240506132438.278920-1-hdegoede@redhat.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | media: ov2740: Ensure proper reset sequence on probe() | expand |
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@redhat.com> Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> 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. Thanks! Stanislaw > --- > 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 >
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@redhat.com> > > Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > > 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 >> >
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@redhat.com> > > > > Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > > > > 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
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))
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@redhat.com> --- drivers/media/i2c/ov2740.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)