Hi Hans, On Wed, Nov 01, 2023 at 11:41:36AM +0100, Hans de Goede wrote: > 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. Seems good to me. > > 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, Sakari Ailus
