Re: [PATCH 1/1] Revert "media: imx290: Convert to new CCI register access helpers"

[Hans de Goede]

Hi,

On 11/1/23 11:31, Sakari Ailus wrote:
> Hi Alexander,
> 
> On Wed, Nov 01, 2023 at 11:09:00AM +0100, Alexander Stein wrote:
>> cci_write() unconditionally writes multi-byte data in big-endian format.
>> But IMX290 and IMX327 use little-endian format for multi-byte registers.
>> Revert the CCI usage until little-endian is supported by CCI register
>> access helpers.
>> This reverts commit af73323b97702e53b0a336972aaf23e7dd92c850.
>>
>> Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
>> ---
>> The difference is subtile, but imx290_write() uses put_unaligned_le24(),
>> while cci_write() uses put_unaligned_be24().
>>
>> I assume this should go into stable as well. How to deal with that?
>> Shall a revert get the fixes tag as well?
> 
> I'd very much prefer addressing this in v4l2-cci instead. It should also be
> a much smaller patch (or patches).

I was under the impression the v4l2-cci conversion was already
reverted because this breakage has been known for a while now.

Anyways, if someone wants to fix this directly this has been
discussed in this thread:

https://lore.kernel.org/linux-media/20231030173637.GA2977515@xxxxxxxxxxxxxxxxxxxxx/

and the consensus is that the best way to fix this is to
add CCI_REG16_LE(x) CCI_REG24_LE(x), etc. macros to
mirror the existing CCI_REG16(x), etc. macros.

The _LE macros would then look something like this:

#define CCI_REG_LE		BIT(20)

#define CCI_REG16_LE(x)                (CCI_REG_LE | (2 << CCI_REG_WIDTH_SHIFT) | (x))

etc.

And then the get_unaligned_beXX() and put_unaligned_beXX()
calls in drivers/media/v4l2-core/v4l2-cci.c would need
to be made conditional on a check for the CCI_REG_LE flag
and if that flag is set use the _le_ versions of those
functions instead.

The reason to go this way instead of a global LE flag
somewhere is that some sensors have mixed endianness
for different registers, so encoding this in the
register-address high bits is the best solution.

Alexander, perhaps you can prepare 2 patches:

1. Adding the discussed CCI_REGXX_LE(x) macros to v4l2-cci
described above.

2. Patch the imx290 code to use the _LE versions for the
registers which are 2 or more bytes wide.

?

Note I know that Alain (added to the Cc) from the thread
linked above is also looking into implementing 1. 

Regards,

Hans