diff mbox series

[06/16] media: i2c: rdacm20: Re-work ov10635 reset

Message ID 20210216174146.106639-7-jacopo+renesas@jmondi.org (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series media: i2c: GMSL reliability improvements | expand

Commit Message

Jacopo Mondi Feb. 16, 2021, 5:41 p.m. UTC
The OV10635 image sensor embedded in the camera module is currently
reset after the MAX9271 initialization with two long delays that were
most probably not correctly characterized.

Re-work the image sensor reset procedure by holding the chip in reset
during the MAX9271 configuration, removing the long sleep delays and
only wait after the chip exits from reset for 350-500 microseconds
interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
characterized in the chip manual.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Comments

Kieran Bingham Feb. 17, 2021, 1:22 p.m. UTC | #1
On 16/02/2021 17:41, Jacopo Mondi wrote:
> The OV10635 image sensor embedded in the camera module is currently
> reset after the MAX9271 initialization with two long delays that were
> most probably not correctly characterized.
> 
> Re-work the image sensor reset procedure by holding the chip in reset
> during the MAX9271 configuration, removing the long sleep delays and
> only wait after the chip exits from reset for 350-500 microseconds
> interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
> characterized in the chip manual.

Holding the OV10635 in reset earlier sounds good to me, but I don't know
beyond that what implications there would be. If it's working better
that's good though.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index e982373908f2..ea30cc936531 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -477,6 +477,15 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* Hold OV10635 in reset during max9271 configuration. */
> +	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +
> +	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +
>  	ret = max9271_configure_gmsl_link(&dev->serializer);
>  	if (ret)
>  		return ret;
> @@ -490,23 +499,11 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  		return ret;
>  	dev->serializer.client->addr = dev->addrs[0];
>  
> -	/*
> -	 * Reset the sensor by cycling the OV10635 reset signal connected to the
> -	 * MAX9271 GPIO1 and verify communication with the OV10635.
> -	 */
> -	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> -	if (ret)
> -		return ret;
> -
> -	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> -	if (ret)
> -		return ret;
> -	usleep_range(10000, 15000);
> -
> +	/* Release ov10635 from reset and initialize it. */
>  	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
> -	usleep_range(10000, 15000);
> +	usleep_range(350, 500);
>  
>  	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
>  		ret = ov10635_read16(dev, OV10635_PID);
>
Laurent Pinchart Feb. 22, 2021, 1:15 a.m. UTC | #2
Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:36PM +0100, Jacopo Mondi wrote:
> The OV10635 image sensor embedded in the camera module is currently
> reset after the MAX9271 initialization with two long delays that were
> most probably not correctly characterized.
> 
> Re-work the image sensor reset procedure by holding the chip in reset
> during the MAX9271 configuration, removing the long sleep delays and
> only wait after the chip exits from reset for 350-500 microseconds
> interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
> characterized in the chip manual.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index e982373908f2..ea30cc936531 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -477,6 +477,15 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* Hold OV10635 in reset during max9271 configuration. */
> +	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +
> +	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> +	if (ret)
> +		return ret;
> +

Unrelated to this patch, it could be nice to rename the GPIO-related
functions to use a similar naming scheme as the gpiod API.

>  	ret = max9271_configure_gmsl_link(&dev->serializer);
>  	if (ret)
>  		return ret;
> @@ -490,23 +499,11 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  		return ret;
>  	dev->serializer.client->addr = dev->addrs[0];
>  
> -	/*
> -	 * Reset the sensor by cycling the OV10635 reset signal connected to the
> -	 * MAX9271 GPIO1 and verify communication with the OV10635.
> -	 */
> -	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> -	if (ret)
> -		return ret;
> -
> -	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> -	if (ret)
> -		return ret;
> -	usleep_range(10000, 15000);

The OV10635 requires the reset signal to be asserted for at least 200┬Ás.
Is this guaranteed by the different calls we have here after asserting
reset ? Maybe a comment to explain this could be useful ?

> -
> +	/* Release ov10635 from reset and initialize it. */
>  	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
> -	usleep_range(10000, 15000);

Maybe a comment here to state that the delay has to be at least 2048
XVCLK cycles would be useful ?

With these taken into account,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	usleep_range(350, 500);
>  
>  	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
>  		ret = ov10635_read16(dev, OV10635_PID);
diff mbox series

Patch

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index e982373908f2..ea30cc936531 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -477,6 +477,15 @@  static int rdacm20_initialize(struct rdacm20_device *dev)
 	if (ret)
 		return ret;
 
+	/* Hold OV10635 in reset during max9271 configuration. */
+	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
+	if (ret)
+		return ret;
+
+	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
+	if (ret)
+		return ret;
+
 	ret = max9271_configure_gmsl_link(&dev->serializer);
 	if (ret)
 		return ret;
@@ -490,23 +499,11 @@  static int rdacm20_initialize(struct rdacm20_device *dev)
 		return ret;
 	dev->serializer.client->addr = dev->addrs[0];
 
-	/*
-	 * Reset the sensor by cycling the OV10635 reset signal connected to the
-	 * MAX9271 GPIO1 and verify communication with the OV10635.
-	 */
-	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
-	if (ret)
-		return ret;
-
-	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
-	if (ret)
-		return ret;
-	usleep_range(10000, 15000);
-
+	/* Release ov10635 from reset and initialize it. */
 	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
-	usleep_range(10000, 15000);
+	usleep_range(350, 500);
 
 	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
 		ret = ov10635_read16(dev, OV10635_PID);