[PATCH] media: i2c: imx296: Implement simple retry for model identification

[PATCH] media: i2c: imx296: Implement simple retry for model identification

[Alexandru Ardelean]

From: Alexandru Ardelean <alex@shruggie.ro>

On a cold boot of the device (and sensor), and when using the 'sony,imx296'
compatible string, it often seems that I get 'invalid device model 0x0000'.
After doing a soft reboot, it seems to work fine.

After applying this change (to do several retries), the sensor is
identified on the first cold boot. The assumption here would be that the
wake-up from standby takes too long. But even trying a 'udelay(100)' after
writing register IMX296_CTRL00 doesn't seem to help (100 microseconds
should be a reasonable fixed time).

However, after implementing the retry loop (as this patch does it), seems
to resolve the issue on the cold boot, and the device is identified.

When using the 'sony,imx296ll' and 'sony,imx296lq' compatible strings, the
device identification process isn't happening, and the sensor works fine.

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/media/i2c/imx296.c | 44 ++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/imx296.c b/drivers/media/i2c/imx296.c
index 83149fa729c4..9c3641c005a4 100644
--- a/drivers/media/i2c/imx296.c
+++ b/drivers/media/i2c/imx296.c
@@ -931,7 +931,7 @@ static int imx296_read_temperature(struct imx296 *sensor, int *temp)
 static int imx296_identify_model(struct imx296 *sensor)
 {
 	unsigned int model;
-	int temp = 0;
+	int temp = 0, retries;
 	int ret;
 
 	model = (uintptr_t)of_device_get_match_data(sensor->dev);
@@ -943,25 +943,33 @@ static int imx296_identify_model(struct imx296 *sensor)
 		return 0;
 	}
 
-	/*
-	 * While most registers can be read when the sensor is in standby, this
-	 * is not the case of the sensor info register :-(
-	 */
-	ret = imx296_write(sensor, IMX296_CTRL00, 0, NULL);
-	if (ret < 0) {
-		dev_err(sensor->dev,
-			"failed to get sensor out of standby (%d)\n", ret);
-		return ret;
-	}
+	retries = 0;
+	do {
+		/*
+		 * While most registers can be read when the sensor is in
+		 * standby, this is not the case of the sensor info register :-(
+		 */
+		ret = imx296_write(sensor, IMX296_CTRL00, 0, NULL);
+		if (ret < 0) {
+			dev_err(sensor->dev,
+				"failed to get sensor out of standby (%d)\n",
+				ret);
+			return ret;
+		}
 
-	ret = imx296_read(sensor, IMX296_SENSOR_INFO);
-	if (ret < 0) {
-		dev_err(sensor->dev, "failed to read sensor information (%d)\n",
-			ret);
-		goto done;
-	}
+		udelay(10);
+
+		ret = imx296_read(sensor, IMX296_SENSOR_INFO);
+		if (ret < 0) {
+			dev_err(sensor->dev,
+				"failed to read sensor information (%d)\n",
+				ret);
+			goto done;
+		}
+
+		model = (ret >> 6) & 0x1ff;
+	} while (model == 0 && retries++ < 3);
 
-	model = (ret >> 6) & 0x1ff;
 
 	switch (model) {
 	case 296:
-- 
2.46.1

Re: [PATCH] media: i2c: imx296: Implement simple retry for model identification

[Kieran Bingham]

Quoting Alexandru Ardelean (2024-11-15 14:20:21)
> From: Alexandru Ardelean <alex@shruggie.ro>
> 
> On a cold boot of the device (and sensor), and when using the 'sony,imx296'
> compatible string, it often seems that I get 'invalid device model 0x0000'.
> After doing a soft reboot, it seems to work fine.
> 
> After applying this change (to do several retries), the sensor is
> identified on the first cold boot. The assumption here would be that the
> wake-up from standby takes too long. But even trying a 'udelay(100)' after
> writing register IMX296_CTRL00 doesn't seem to help (100 microseconds
> should be a reasonable fixed time).

I believe Raspberry Pi have an IMX296 and have some out of tree patches.

 https://github.com/raspberrypi/linux/commits/rpi-6.6.y/drivers/media/i2c/imx296.c

It looks like they do similar fixes for bootup, for instance:
 
 https://github.com/raspberrypi/linux/commit/7713ce38e6a26425ace3a57b3d03ba0125c16f89

which introduces a 2-5ms delay before reading the IMX296_SENSOR_INFO
register.

As this delay is significantly longer tahn the 100microseconds you've
tried it might be worth testing Naushir's patch, which states:

"""
Add a 2-5ms delay when coming out of standby and before reading the
sensor info register durning probe, as instructed by the datasheet. This
standby delay is already present when the sensor starts streaming.
"""

Regards
--
Kieran

> 
> However, after implementing the retry loop (as this patch does it), seems
> to resolve the issue on the cold boot, and the device is identified.
> 
> When using the 'sony,imx296ll' and 'sony,imx296lq' compatible strings, the
> device identification process isn't happening, and the sensor works fine.
> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---
>  drivers/media/i2c/imx296.c | 44 ++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx296.c b/drivers/media/i2c/imx296.c
> index 83149fa729c4..9c3641c005a4 100644
> --- a/drivers/media/i2c/imx296.c
> +++ b/drivers/media/i2c/imx296.c
> @@ -931,7 +931,7 @@ static int imx296_read_temperature(struct imx296 *sensor, int *temp)
>  static int imx296_identify_model(struct imx296 *sensor)
>  {
>         unsigned int model;
> -       int temp = 0;
> +       int temp = 0, retries;
>         int ret;
>  
>         model = (uintptr_t)of_device_get_match_data(sensor->dev);
> @@ -943,25 +943,33 @@ static int imx296_identify_model(struct imx296 *sensor)
>                 return 0;
>         }
>  
> -       /*
> -        * While most registers can be read when the sensor is in standby, this
> -        * is not the case of the sensor info register :-(
> -        */
> -       ret = imx296_write(sensor, IMX296_CTRL00, 0, NULL);
> -       if (ret < 0) {
> -               dev_err(sensor->dev,
> -                       "failed to get sensor out of standby (%d)\n", ret);
> -               return ret;
> -       }
> +       retries = 0;
> +       do {
> +               /*
> +                * While most registers can be read when the sensor is in
> +                * standby, this is not the case of the sensor info register :-(
> +                */
> +               ret = imx296_write(sensor, IMX296_CTRL00, 0, NULL);
> +               if (ret < 0) {
> +                       dev_err(sensor->dev,
> +                               "failed to get sensor out of standby (%d)\n",
> +                               ret);
> +                       return ret;
> +               }
>  
> -       ret = imx296_read(sensor, IMX296_SENSOR_INFO);
> -       if (ret < 0) {
> -               dev_err(sensor->dev, "failed to read sensor information (%d)\n",
> -                       ret);
> -               goto done;
> -       }
> +               udelay(10);
> +
> +               ret = imx296_read(sensor, IMX296_SENSOR_INFO);
> +               if (ret < 0) {
> +                       dev_err(sensor->dev,
> +                               "failed to read sensor information (%d)\n",
> +                               ret);
> +                       goto done;
> +               }
> +
> +               model = (ret >> 6) & 0x1ff;
> +       } while (model == 0 && retries++ < 3);
>  
> -       model = (ret >> 6) & 0x1ff;
>  
>         switch (model) {
>         case 296:
> -- 
> 2.46.1
>

Re: [PATCH] media: i2c: imx296: Implement simple retry for model identification

[Alexandru Ardelean]

On Fri, Nov 15, 2024 at 4:57 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Alexandru Ardelean (2024-11-15 14:20:21)
> > From: Alexandru Ardelean <alex@shruggie.ro>
> >
> > On a cold boot of the device (and sensor), and when using the 'sony,imx296'
> > compatible string, it often seems that I get 'invalid device model 0x0000'.
> > After doing a soft reboot, it seems to work fine.
> >
> > After applying this change (to do several retries), the sensor is
> > identified on the first cold boot. The assumption here would be that the
> > wake-up from standby takes too long. But even trying a 'udelay(100)' after
> > writing register IMX296_CTRL00 doesn't seem to help (100 microseconds
> > should be a reasonable fixed time).
>
> I believe Raspberry Pi have an IMX296 and have some out of tree patches.
>

Oh.
It didn't occur to me to look into RPi's tree.
But I will try out their patch.
I don't have access to the datasheet for this sensor.
I was just playing around with it and found this annoying issue on
cold-boot, which made me wonder if it's a reset delay issue or
something else.

Thanks
Alex

>  https://github.com/raspberrypi/linux/commits/rpi-6.6.y/drivers/media/i2c/imx296.c
>
> It looks like they do similar fixes for bootup, for instance:
>
>  https://github.com/raspberrypi/linux/commit/7713ce38e6a26425ace3a57b3d03ba0125c16f89
>
> which introduces a 2-5ms delay before reading the IMX296_SENSOR_INFO
> register.
>
> As this delay is significantly longer tahn the 100microseconds you've
> tried it might be worth testing Naushir's patch, which states:
>
> """
> Add a 2-5ms delay when coming out of standby and before reading the
> sensor info register durning probe, as instructed by the datasheet. This
> standby delay is already present when the sensor starts streaming.
> """
>
> Regards
> --
> Kieran
>
> >
> > However, after implementing the retry loop (as this patch does it), seems
> > to resolve the issue on the cold boot, and the device is identified.
> >
> > When using the 'sony,imx296ll' and 'sony,imx296lq' compatible strings, the
> > device identification process isn't happening, and the sensor works fine.
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > ---
> >  drivers/media/i2c/imx296.c | 44 ++++++++++++++++++++++----------------
> >  1 file changed, 26 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx296.c b/drivers/media/i2c/imx296.c
> > index 83149fa729c4..9c3641c005a4 100644
> > --- a/drivers/media/i2c/imx296.c
> > +++ b/drivers/media/i2c/imx296.c
> > @@ -931,7 +931,7 @@ static int imx296_read_temperature(struct imx296 *sensor, int *temp)
> >  static int imx296_identify_model(struct imx296 *sensor)
> >  {
> >         unsigned int model;
> > -       int temp = 0;
> > +       int temp = 0, retries;
> >         int ret;
> >
> >         model = (uintptr_t)of_device_get_match_data(sensor->dev);
> > @@ -943,25 +943,33 @@ static int imx296_identify_model(struct imx296 *sensor)
> >                 return 0;
> >         }
> >
> > -       /*
> > -        * While most registers can be read when the sensor is in standby, this
> > -        * is not the case of the sensor info register :-(
> > -        */
> > -       ret = imx296_write(sensor, IMX296_CTRL00, 0, NULL);
> > -       if (ret < 0) {
> > -               dev_err(sensor->dev,
> > -                       "failed to get sensor out of standby (%d)\n", ret);
> > -               return ret;
> > -       }
> > +       retries = 0;
> > +       do {
> > +               /*
> > +                * While most registers can be read when the sensor is in
> > +                * standby, this is not the case of the sensor info register :-(
> > +                */
> > +               ret = imx296_write(sensor, IMX296_CTRL00, 0, NULL);
> > +               if (ret < 0) {
> > +                       dev_err(sensor->dev,
> > +                               "failed to get sensor out of standby (%d)\n",
> > +                               ret);
> > +                       return ret;
> > +               }
> >
> > -       ret = imx296_read(sensor, IMX296_SENSOR_INFO);
> > -       if (ret < 0) {
> > -               dev_err(sensor->dev, "failed to read sensor information (%d)\n",
> > -                       ret);
> > -               goto done;
> > -       }
> > +               udelay(10);
> > +
> > +               ret = imx296_read(sensor, IMX296_SENSOR_INFO);
> > +               if (ret < 0) {
> > +                       dev_err(sensor->dev,
> > +                               "failed to read sensor information (%d)\n",
> > +                               ret);
> > +                       goto done;
> > +               }
> > +
> > +               model = (ret >> 6) & 0x1ff;
> > +       } while (model == 0 && retries++ < 3);
> >
> > -       model = (ret >> 6) & 0x1ff;
> >
> >         switch (model) {
> >         case 296:
> > --
> > 2.46.1
> >

[PATCH v2] drivers: media: imx296: Add standby delay during probe

[Alexandru Ardelean]

From: Naushir Patuck <naush@raspberrypi.com>

Add a 2-5ms delay when coming out of standby and before reading the
sensor info register durning probe, as instructed by the datasheet. This
standby delay is already present when the sensor starts streaming.

During a cold-boot, reading the IMX296_SENSOR_INFO register would often
return a value of 0x0000, if this delay is not present before.

Fixes: cb33db2b6ccfe ("media: i2c: IMX296 camera sensor driver")
Tested-by: Alexandru Ardelean <aardelean@baylibre.com>
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---

Changelog v1 -> v2:
* https://lore.kernel.org/linux-media/20241115142021.574402-1-aardelean@baylibre.com/
* Technically, this is not a true V2, but rather a new patch
  - But in V1, the attempt was to fix an issue found with the upstream
    IMX296 driver, which was pointed out by Kieran that it was already
    fixed (more elegantly) in the RPi tree.
  - The standby delay helps during a cold-boot so that the driver can read
    the IMX296_SENSOR_INFO register. If the delay isn't present the value
    read is 0xx0000.
  - Original patch can be found:
    https://github.com/raspberrypi/linux/commit/7713ce38e6a26425ace3a57b3d03ba0125c16f89
  - From the original patch of Naushir Patuck,
    - Added comment 
      -------
      During a cold-boot, reading the IMX296_SENSOR_INFO register would often 
      return a value of 0x0000, if this delay is not present before.
      -------
    - Added 'Tested-by: Alexandru Ardelean <aardelean@baylibre.com>
    - Added 'Fixes: cb33db2b6ccfe ("media: i2c: IMX296 camera sensor driver")'

 drivers/media/i2c/imx296.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/imx296.c b/drivers/media/i2c/imx296.c
index f942f66fa664..395bfe4fb23d 100644
--- a/drivers/media/i2c/imx296.c
+++ b/drivers/media/i2c/imx296.c
@@ -940,6 +940,8 @@ static int imx296_identify_model(struct imx296 *sensor)
 		return ret;
 	}
 
+	usleep_range(2000, 5000);
+
 	ret = imx296_read(sensor, IMX296_SENSOR_INFO);
 	if (ret < 0) {
 		dev_err(sensor->dev, "failed to read sensor information (%d)\n",
-- 
2.46.1

Re: [PATCH v2] drivers: media: imx296: Add standby delay during probe

[Laurent Pinchart]

Hi Alexandru, Naush,

Thank you for the patch.

On Fri, Nov 15, 2024 at 08:07:17PM +0200, Alexandru Ardelean wrote:
> From: Naushir Patuck <naush@raspberrypi.com>
> 
> Add a 2-5ms delay when coming out of standby and before reading the
> sensor info register durning probe, as instructed by the datasheet. This
> standby delay is already present when the sensor starts streaming.
> 
> During a cold-boot, reading the IMX296_SENSOR_INFO register would often
> return a value of 0x0000, if this delay is not present before.
> 
> Fixes: cb33db2b6ccfe ("media: i2c: IMX296 camera sensor driver")
> Tested-by: Alexandru Ardelean <aardelean@baylibre.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

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

> ---
> 
> Changelog v1 -> v2:
> * https://lore.kernel.org/linux-media/20241115142021.574402-1-aardelean@baylibre.com/
> * Technically, this is not a true V2, but rather a new patch
>   - But in V1, the attempt was to fix an issue found with the upstream
>     IMX296 driver, which was pointed out by Kieran that it was already
>     fixed (more elegantly) in the RPi tree.
>   - The standby delay helps during a cold-boot so that the driver can read
>     the IMX296_SENSOR_INFO register. If the delay isn't present the value
>     read is 0xx0000.
>   - Original patch can be found:
>     https://github.com/raspberrypi/linux/commit/7713ce38e6a26425ace3a57b3d03ba0125c16f89
>   - From the original patch of Naushir Patuck,
>     - Added comment 
>       -------
>       During a cold-boot, reading the IMX296_SENSOR_INFO register would often 
>       return a value of 0x0000, if this delay is not present before.
>       -------
>     - Added 'Tested-by: Alexandru Ardelean <aardelean@baylibre.com>
>     - Added 'Fixes: cb33db2b6ccfe ("media: i2c: IMX296 camera sensor driver")'
> 
>  drivers/media/i2c/imx296.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx296.c b/drivers/media/i2c/imx296.c
> index f942f66fa664..395bfe4fb23d 100644
> --- a/drivers/media/i2c/imx296.c
> +++ b/drivers/media/i2c/imx296.c
> @@ -940,6 +940,8 @@ static int imx296_identify_model(struct imx296 *sensor)
>  		return ret;
>  	}
>  
> +	usleep_range(2000, 5000);
> +
>  	ret = imx296_read(sensor, IMX296_SENSOR_INFO);
>  	if (ret < 0) {
>  		dev_err(sensor->dev, "failed to read sensor information (%d)\n",

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] drivers: media: imx296: Add standby delay during probe

[Kieran Bingham]

Quoting Alexandru Ardelean (2024-11-15 18:07:17)
> From: Naushir Patuck <naush@raspberrypi.com>
> 
> Add a 2-5ms delay when coming out of standby and before reading the
> sensor info register durning probe, as instructed by the datasheet. This
> standby delay is already present when the sensor starts streaming.
> 
> During a cold-boot, reading the IMX296_SENSOR_INFO register would often
> return a value of 0x0000, if this delay is not present before.
> 
> Fixes: cb33db2b6ccfe ("media: i2c: IMX296 camera sensor driver")
> Tested-by: Alexandru Ardelean <aardelean@baylibre.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
> 
> Changelog v1 -> v2:
> * https://lore.kernel.org/linux-media/20241115142021.574402-1-aardelean@baylibre.com/
> * Technically, this is not a true V2, but rather a new patch
>   - But in V1, the attempt was to fix an issue found with the upstream
>     IMX296 driver, which was pointed out by Kieran that it was already
>     fixed (more elegantly) in the RPi tree.

Thanks,


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

>   - The standby delay helps during a cold-boot so that the driver can read
>     the IMX296_SENSOR_INFO register. If the delay isn't present the value
>     read is 0xx0000.
>   - Original patch can be found:
>     https://github.com/raspberrypi/linux/commit/7713ce38e6a26425ace3a57b3d03ba0125c16f89
>   - From the original patch of Naushir Patuck,
>     - Added comment 
>       -------
>       During a cold-boot, reading the IMX296_SENSOR_INFO register would often 
>       return a value of 0x0000, if this delay is not present before.
>       -------
>     - Added 'Tested-by: Alexandru Ardelean <aardelean@baylibre.com>
>     - Added 'Fixes: cb33db2b6ccfe ("media: i2c: IMX296 camera sensor driver")'
> 
>  drivers/media/i2c/imx296.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx296.c b/drivers/media/i2c/imx296.c
> index f942f66fa664..395bfe4fb23d 100644
> --- a/drivers/media/i2c/imx296.c
> +++ b/drivers/media/i2c/imx296.c
> @@ -940,6 +940,8 @@ static int imx296_identify_model(struct imx296 *sensor)
>                 return ret;
>         }
>  
> +       usleep_range(2000, 5000);
> +
>         ret = imx296_read(sensor, IMX296_SENSOR_INFO);
>         if (ret < 0) {
>                 dev_err(sensor->dev, "failed to read sensor information (%d)\n",
> -- 
> 2.46.1
>

Re: [PATCH v2] drivers: media: imx296: Add standby delay during probe

[Sakari Ailus]

Hi Alexandru,

On Fri, Nov 15, 2024 at 08:07:17PM +0200, Alexandru Ardelean wrote:
> From: Naushir Patuck <naush@raspberrypi.com>
> 
> Add a 2-5ms delay when coming out of standby and before reading the
> sensor info register durning probe, as instructed by the datasheet. This
> standby delay is already present when the sensor starts streaming.
> 
> During a cold-boot, reading the IMX296_SENSOR_INFO register would often
> return a value of 0x0000, if this delay is not present before.
> 
> Fixes: cb33db2b6ccfe ("media: i2c: IMX296 camera sensor driver")
> Tested-by: Alexandru Ardelean <aardelean@baylibre.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Thanks for the patch.

You always need to Cc: stable if you have a Fixes: tag nowadays. I'll add
it this time.

-- 
Sakari Ailus