[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240607155704.GB1242@pendragon.ideasonboard.com>
Date: Fri, 7 Jun 2024 18:57:04 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Conor Dooley <conor@...nel.org>
Cc: linux-media@...r.kernel.org, Conor Dooley <conor.dooley@...rochip.com>,
Dave Stevenson <dave.stevenson@...pberrypi.com>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Adam Ford <aford173@...il.com>,
Andrey Konovalov <andrey.konovalov@...aro.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] media: i2c: imx219: fix msr access command sequence
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@...rochip.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@...rochip.com>
This looks reasonable, based on the above link.
Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.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@...pberrypi.com>
> CC: Sakari Ailus <sakari.ailus@...ux.intel.com>
> CC: Mauro Carvalho Chehab <mchehab@...nel.org>
> CC: Adam Ford <aford173@...il.com>
> CC: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> CC: Andrey Konovalov <andrey.konovalov@...aro.org>
> CC: linux-media@...r.kernel.org
> CC: linux-kernel@...r.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
Powered by blists - more mailing lists