[PATCH v1] media: i2c: imx219: fix msr access command sequence

[PATCH v1] media: i2c: imx219: fix msr access command sequence

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

It was reported to me that the imx219 didn't work on one of our
development kits partly because the access sequence is incorrect.
The datasheet I could find [1] for this camera has the access sequence:
Seq. No. Address (Hex) data
1        30EB          05
2        30EB          0C
3        300A          FF
4        300B          FF
5        30EB          05
6        30EB          09

but the driver swaps the first two elements. Laurent pointed out on IRC
that the original code used the correct sequence for 1920x1080 but the
current sequence for 3280x2464 and 1640x1232. During refactoring of the
init sequence the current order was used for all formats.

Switch to using the documented sequence.

Link: https://www.opensourceinstruments.com/Electronics/Data/IMX219PQ.pdf [1]
Fixes: 8508455961d5 ("media: i2c: imx219: Split common registers from mode tables")
Fixes: 1283b3b8f82b ("media: i2c: Add driver for Sony IMX219 sensor")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
I got the report of this third hand, I don't have a device and can't
test this. I do wonder why the RPis get away with the sequence that
seemingly doesn't work for the guy that reported this to me. My theory
is either that they noticed the sequence was wrong while adding some
other MSR access that is needed on this board while either cross
checking the values written or because the other MSR accesses didn't
take effect.

CC: Dave Stevenson <dave.stevenson@raspberrypi.com>
CC: Sakari Ailus <sakari.ailus@linux.intel.com>
CC: Mauro Carvalho Chehab <mchehab@kernel.org>
CC: Adam Ford <aford173@gmail.com>
CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
CC: Andrey Konovalov <andrey.konovalov@linaro.org>
CC: linux-media@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/media/i2c/imx219.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 51ebf5453fce..e78a80b2bb2e 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -162,8 +162,8 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
 	{ IMX219_REG_MODE_SELECT, 0x00 },	/* Mode Select */
 
 	/* To Access Addresses 3000-5fff, send the following commands */
-	{ CCI_REG8(0x30eb), 0x0c },
 	{ CCI_REG8(0x30eb), 0x05 },
+	{ CCI_REG8(0x30eb), 0x0c },
 	{ CCI_REG8(0x300a), 0xff },
 	{ CCI_REG8(0x300b), 0xff },
 	{ CCI_REG8(0x30eb), 0x05 },
-- 
2.43.0

Re: [PATCH v1] media: i2c: imx219: fix msr access command sequence

[Laurent Pinchart]

Hi Conor,

Thank you for the patch.

On Fri, Jun 07, 2024 at 04:50:23PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> It was reported to me that the imx219 didn't work on one of our
> development kits partly because the access sequence is incorrect.
> The datasheet I could find [1] for this camera has the access sequence:
> Seq. No. Address (Hex) data
> 1        30EB          05
> 2        30EB          0C
> 3        300A          FF
> 4        300B          FF
> 5        30EB          05
> 6        30EB          09
> 
> but the driver swaps the first two elements. Laurent pointed out on IRC
> that the original code used the correct sequence for 1920x1080 but the
> current sequence for 3280x2464 and 1640x1232. During refactoring of the
> init sequence the current order was used for all formats.
> 
> Switch to using the documented sequence.
> 
> Link: https://www.opensourceinstruments.com/Electronics/Data/IMX219PQ.pdf [1]
> Fixes: 8508455961d5 ("media: i2c: imx219: Split common registers from mode tables")
> Fixes: 1283b3b8f82b ("media: i2c: Add driver for Sony IMX219 sensor")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

This looks reasonable, based on the above link.

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

Dave, could you check the impact on the Raspberry Pi kernel ? It seems
to be shipping the incorrect sequence unconditionally.

Any information about what the 12 undocumented MSRs that are programmed
by the driver do would be appreciated too ;-)

> ---
> I got the report of this third hand, I don't have a device and can't
> test this. I do wonder why the RPis get away with the sequence that
> seemingly doesn't work for the guy that reported this to me. My theory
> is either that they noticed the sequence was wrong while adding some
> other MSR access that is needed on this board while either cross
> checking the values written or because the other MSR accesses didn't
> take effect.
> 
> CC: Dave Stevenson <dave.stevenson@raspberrypi.com>
> CC: Sakari Ailus <sakari.ailus@linux.intel.com>
> CC: Mauro Carvalho Chehab <mchehab@kernel.org>
> CC: Adam Ford <aford173@gmail.com>
> CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> CC: Andrey Konovalov <andrey.konovalov@linaro.org>
> CC: linux-media@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  drivers/media/i2c/imx219.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 51ebf5453fce..e78a80b2bb2e 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -162,8 +162,8 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
>  	{ IMX219_REG_MODE_SELECT, 0x00 },	/* Mode Select */
>  
>  	/* To Access Addresses 3000-5fff, send the following commands */
> -	{ CCI_REG8(0x30eb), 0x0c },
>  	{ CCI_REG8(0x30eb), 0x05 },
> +	{ CCI_REG8(0x30eb), 0x0c },
>  	{ CCI_REG8(0x300a), 0xff },
>  	{ CCI_REG8(0x300b), 0xff },
>  	{ CCI_REG8(0x30eb), 0x05 },

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1] media: i2c: imx219: fix msr access command sequence

[Dave Stevenson]

Hi Conor and Laurent

On Fri, 7 Jun 2024 at 16:57, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Conor,
>
> Thank you for the patch.
>
> On Fri, Jun 07, 2024 at 04:50:23PM +0100, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> >
> > It was reported to me that the imx219 didn't work on one of our
> > development kits partly because the access sequence is incorrect.
> > The datasheet I could find [1] for this camera has the access sequence:
> > Seq. No. Address (Hex) data
> > 1        30EB          05
> > 2        30EB          0C
> > 3        300A          FF
> > 4        300B          FF
> > 5        30EB          05
> > 6        30EB          09
> >
> > but the driver swaps the first two elements. Laurent pointed out on IRC
> > that the original code used the correct sequence for 1920x1080 but the
> > current sequence for 3280x2464 and 1640x1232. During refactoring of the
> > init sequence the current order was used for all formats.
> >
> > Switch to using the documented sequence.
> >
> > Link: https://www.opensourceinstruments.com/Electronics/Data/IMX219PQ.pdf [1]
> > Fixes: 8508455961d5 ("media: i2c: imx219: Split common registers from mode tables")
> > Fixes: 1283b3b8f82b ("media: i2c: Add driver for Sony IMX219 sensor")
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>
> This looks reasonable, based on the above link.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Dave, could you check the impact on the Raspberry Pi kernel ? It seems
> to be shipping the incorrect sequence unconditionally.

I've compared the values against the old firmware driver. There we
have the cropped 1080p and VGA high framerate modes had 0x05 0x0c, and
the full res 3280x2464 and 2x2 binned 1640x1232 modes had 0x0c 0x05,
so the same as the original kernel driver. Not totally unsurprising as
the kernel driver register sets were copied from the firmware.
So the Pi has used imx219 in this manner since launch of the sensor in
2014! Whether that was a transcription typo or an error in the
register sets from Sony I couldn't say (Naush may still have the
original register set information).

I don't have an imx219 to hand right now to test with, but will check
it out on Monday. I'll agree that the patch looks valid based on the
datasheet.

> Any information about what the 12 undocumented MSRs that are programmed
> by the driver do would be appreciated too ;-)

Sadly I have no extra information on those.

> > ---
> > I got the report of this third hand, I don't have a device and can't
> > test this. I do wonder why the RPis get away with the sequence that
> > seemingly doesn't work for the guy that reported this to me. My theory
> > is either that they noticed the sequence was wrong while adding some
> > other MSR access that is needed on this board while either cross
> > checking the values written or because the other MSR accesses didn't
> > take effect.

Did the change fix it for the reporter? We're using the driver with no
changes to the register settings cf mainline.
Why it works on the Pi but not on a Microchip board is likely to be
something quite subtle.

  Dave

> > CC: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > CC: Sakari Ailus <sakari.ailus@linux.intel.com>
> > CC: Mauro Carvalho Chehab <mchehab@kernel.org>
> > CC: Adam Ford <aford173@gmail.com>
> > CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > CC: Andrey Konovalov <andrey.konovalov@linaro.org>
> > CC: linux-media@vger.kernel.org
> > CC: linux-kernel@vger.kernel.org
> > ---
> >  drivers/media/i2c/imx219.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 51ebf5453fce..e78a80b2bb2e 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -162,8 +162,8 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
> >       { IMX219_REG_MODE_SELECT, 0x00 },       /* Mode Select */
> >
> >       /* To Access Addresses 3000-5fff, send the following commands */
> > -     { CCI_REG8(0x30eb), 0x0c },
> >       { CCI_REG8(0x30eb), 0x05 },
> > +     { CCI_REG8(0x30eb), 0x0c },
> >       { CCI_REG8(0x300a), 0xff },
> >       { CCI_REG8(0x300b), 0xff },
> >       { CCI_REG8(0x30eb), 0x05 },
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v1] media: i2c: imx219: fix msr access command sequence

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

On Fri, Jun 07, 2024 at 06:29:49PM +0100, Dave Stevenson wrote:

> > > ---
> > > I got the report of this third hand, I don't have a device and can't
> > > test this. I do wonder why the RPis get away with the sequence that
> > > seemingly doesn't work for the guy that reported this to me. My theory
> > > is either that they noticed the sequence was wrong while adding some
> > > other MSR access that is needed on this board while either cross
> > > checking the values written or because the other MSR accesses didn't
> > > take effect.
> 
> Did the change fix it for the reporter? We're using the driver with no
> changes to the register settings cf mainline.
> Why it works on the Pi but not on a Microchip board is likely to be
> something quite subtle.

I've asked, maybe it turns out to just be the first of my suggestions,
and they noticed it was not matching in passing. They did introduce two
additional MSR accesses, both outside of the range documented in the
datasheets I could find online. They did have explanations for what those
undocumented MSRs did (0x5040 and 0x5041) in the mail I got, but given
it's third hand info to me, I dunno if we have the datasheet etc. I'll
try to find out some more next week.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1] media: i2c: imx219: fix msr access command sequence

[Adam Ford]

On Fri, Jun 7, 2024 at 12:55 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Jun 07, 2024 at 06:29:49PM +0100, Dave Stevenson wrote:
>
> > > > ---
> > > > I got the report of this third hand, I don't have a device and can't
> > > > test this. I do wonder why the RPis get away with the sequence that
> > > > seemingly doesn't work for the guy that reported this to me. My theory
> > > > is either that they noticed the sequence was wrong while adding some
> > > > other MSR access that is needed on this board while either cross
> > > > checking the values written or because the other MSR accesses didn't
> > > > take effect.
> >
> > Did the change fix it for the reporter? We're using the driver with no
> > changes to the register settings cf mainline.
> > Why it works on the Pi but not on a Microchip board is likely to be
> > something quite subtle.

I have two different platforms which can use this sensor.  I can try
to test it this weekend and I'll report back my findings.

adam
>
> I've asked, maybe it turns out to just be the first of my suggestions,
> and they noticed it was not matching in passing. They did introduce two
> additional MSR accesses, both outside of the range documented in the
> datasheets I could find online. They did have explanations for what those
> undocumented MSRs did (0x5040 and 0x5041) in the mail I got, but given
> it's third hand info to me, I dunno if we have the datasheet etc. I'll
> try to find out some more next week.
>
> Thanks,
> Conor.

Re: [PATCH v1] media: i2c: imx219: fix msr access command sequence

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]

On Fri, Jun 07, 2024 at 06:55:01PM +0100, Conor Dooley wrote:
> On Fri, Jun 07, 2024 at 06:29:49PM +0100, Dave Stevenson wrote:
> 
> > > > ---
> > > > I got the report of this third hand, I don't have a device and can't
> > > > test this. I do wonder why the RPis get away with the sequence that
> > > > seemingly doesn't work for the guy that reported this to me. My theory
> > > > is either that they noticed the sequence was wrong while adding some
> > > > other MSR access that is needed on this board while either cross
> > > > checking the values written or because the other MSR accesses didn't
> > > > take effect.
> > 
> > Did the change fix it for the reporter? We're using the driver with no
> > changes to the register settings cf mainline.
> > Why it works on the Pi but not on a Microchip board is likely to be
> > something quite subtle.
> 
> I've asked, maybe it turns out to just be the first of my suggestions,
> and they noticed it was not matching in passing.

Apparently it was the latter & they did need to fix the sequence to be
able to write the MSRs.

> They did introduce two
> additional MSR accesses, both outside of the range documented in the
> datasheets I could find online. They did have explanations for what those
> undocumented MSRs did (0x5040 and 0x5041) in the mail I got, but given
> it's third hand info to me, I dunno if we have the datasheet etc. I'll
> try to find out some more next week.

Seemingly what those two additional MSRs do is under NDA so I would have
no way of justifying a patch to add them or the devicetree property
required to know whether or not the additional MSR writes is needed. :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1] media: i2c: imx219: fix msr access command sequence

[Dave Stevenson]

Hi Conor

On Mon, 10 Jun 2024 at 08:32, Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Fri, Jun 07, 2024 at 06:55:01PM +0100, Conor Dooley wrote:
> > On Fri, Jun 07, 2024 at 06:29:49PM +0100, Dave Stevenson wrote:
> >
> > > > > ---
> > > > > I got the report of this third hand, I don't have a device and can't
> > > > > test this. I do wonder why the RPis get away with the sequence that
> > > > > seemingly doesn't work for the guy that reported this to me. My theory
> > > > > is either that they noticed the sequence was wrong while adding some
> > > > > other MSR access that is needed on this board while either cross
> > > > > checking the values written or because the other MSR accesses didn't
> > > > > take effect.
> > >
> > > Did the change fix it for the reporter? We're using the driver with no
> > > changes to the register settings cf mainline.
> > > Why it works on the Pi but not on a Microchip board is likely to be
> > > something quite subtle.
> >
> > I've asked, maybe it turns out to just be the first of my suggestions,
> > and they noticed it was not matching in passing.
>
> Apparently it was the latter & they did need to fix the sequence to be
> able to write the MSRs.

Fair enough.

> > They did introduce two
> > additional MSR accesses, both outside of the range documented in the
> > datasheets I could find online. They did have explanations for what those
> > undocumented MSRs did (0x5040 and 0x5041) in the mail I got, but given
> > it's third hand info to me, I dunno if we have the datasheet etc. I'll
> > try to find out some more next week.
>
> Seemingly what those two additional MSRs do is under NDA so I would have
> no way of justifying a patch to add them or the devicetree property
> required to know whether or not the additional MSR writes is needed. :)

NDAs can be a real pain sometimes.
I'm happy with the patch though, and it doesn't stop anything else
working on the Pi.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Re: [PATCH v1] media: i2c: imx219: fix msr access command sequence

[Adam Ford]

On Fri, Jun 7, 2024 at 10:50 AM Conor Dooley <conor@kernel.org> wrote:
>
> From: Conor Dooley <conor.dooley@microchip.com>
>
> It was reported to me that the imx219 didn't work on one of our
> development kits partly because the access sequence is incorrect.
> The datasheet I could find [1] for this camera has the access sequence:
> Seq. No. Address (Hex) data
> 1        30EB          05
> 2        30EB          0C
> 3        300A          FF
> 4        300B          FF
> 5        30EB          05
> 6        30EB          09
>
> but the driver swaps the first two elements. Laurent pointed out on IRC
> that the original code used the correct sequence for 1920x1080 but the
> current sequence for 3280x2464 and 1640x1232. During refactoring of the
> init sequence the current order was used for all formats.
>
> Switch to using the documented sequence.
>


Tested-by:  Adam Ford <aford173@gmail.com>  #imx8mp-beacon-kit

adam

> Link: https://www.opensourceinstruments.com/Electronics/Data/IMX219PQ.pdf [1]
> Fixes: 8508455961d5 ("media: i2c: imx219: Split common registers from mode tables")
> Fixes: 1283b3b8f82b ("media: i2c: Add driver for Sony IMX219 sensor")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> I got the report of this third hand, I don't have a device and can't
> test this. I do wonder why the RPis get away with the sequence that
> seemingly doesn't work for the guy that reported this to me. My theory
> is either that they noticed the sequence was wrong while adding some
> other MSR access that is needed on this board while either cross
> checking the values written or because the other MSR accesses didn't
> take effect.
>
> CC: Dave Stevenson <dave.stevenson@raspberrypi.com>
> CC: Sakari Ailus <sakari.ailus@linux.intel.com>
> CC: Mauro Carvalho Chehab <mchehab@kernel.org>
> CC: Adam Ford <aford173@gmail.com>
> CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> CC: Andrey Konovalov <andrey.konovalov@linaro.org>
> CC: linux-media@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  drivers/media/i2c/imx219.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 51ebf5453fce..e78a80b2bb2e 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -162,8 +162,8 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
>         { IMX219_REG_MODE_SELECT, 0x00 },       /* Mode Select */
>
>         /* To Access Addresses 3000-5fff, send the following commands */
> -       { CCI_REG8(0x30eb), 0x0c },
>         { CCI_REG8(0x30eb), 0x05 },
> +       { CCI_REG8(0x30eb), 0x0c },
>         { CCI_REG8(0x300a), 0xff },
>         { CCI_REG8(0x300b), 0xff },
>         { CCI_REG8(0x30eb), 0x05 },
> --
> 2.43.0
>