[PATCH] media: ov2740: Ensure proper reset sequence on probe()

[PATCH] media: ov2740: Ensure proper reset sequence on probe()

[Hans de Goede]

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(-)

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

Re: [PATCH] media: ov2740: Ensure proper reset sequence on probe()

[Stanislaw Gruszka]

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
> 

Re: [PATCH] media: ov2740: Ensure proper reset sequence on probe()

[Hans de Goede]

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
>>
> 

Re: [PATCH] media: ov2740: Ensure proper reset sequence on probe()

[Stanislaw Gruszka]

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