Re: [PATCH] media: i2c: imx296: Implement simple retry for model identification
From: Alexandru Ardelean
Date: Fri Nov 15 2024 - 12:12:13 EST
On Fri, Nov 15, 2024 at 4:57 PM Kieran Bingham
<kieran.bingham@xxxxxxxxxxxxxxxx> wrote:
>
> Quoting Alexandru Ardelean (2024-11-15 14:20:21)
> > From: Alexandru Ardelean <alex@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> > 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
> >