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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+GgBR_5-9b+5oN9fcKRgiLc8gBdNZegJFGmkimtjJpy7LHa6Q@mail.gmail.com>
Date: Fri, 15 Nov 2024 19:11:29 +0200
From: Alexandru Ardelean <aardelean@...libre.com>
To: Kieran Bingham <kieran.bingham@...asonboard.com>
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, 
	laurent.pinchart@...asonboard.com, manivannan.sadhasivam@...aro.org, 
	sakari.ailus@...ux.intel.com, mchehab@...nel.org, 
	Alexandru Ardelean <alex@...uggie.ro>
Subject: Re: [PATCH] media: i2c: imx296: Implement simple retry for model identification

On Fri, Nov 15, 2024 at 4:57 PM Kieran Bingham
<kieran.bingham@...asonboard.com> wrote:
>
> Quoting Alexandru Ardelean (2024-11-15 14:20:21)
> > From: Alexandru Ardelean <alex@...uggie.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@...libre.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
> >