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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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