Hi,
thanks for the feedback.
Am Donnerstag, 2. November 2023, 07:30:44 CET schrieb Sakari Ailus:
> Hi Laurent,
>
> On Thu, Nov 02, 2023 at 03:22:17AM +0200, Laurent Pinchart wrote:
> > Hi Alexander,
> >
> > Thank you for the patch.
> >
> > On Wed, Nov 01, 2023 at 01:23:53PM +0100, Alexander Stein wrote:
> > > Some sensors, e.g. Sony, are using little-endian registers. Add support
> > > for
> >
> > I would write Sony IMX290 here, as there are Sony sensors that use big
> > endian.
>
> Almost all of them. There are a few exceptions indeed. This seems to be a
> bug.
Let's name IMX290 here as an actual example. No need to worry about other
models here.
> > > those by encoding the endianess into Bit 20 of the register address.
> > >
> > > Fixes: af73323b97702 ("media: imx290: Convert to new CCI register access
> > > helpers") Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > > ---
> > >
> > > drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------
> > > include/media/v4l2-cci.h | 5 ++++
> > > 2 files changed, 41 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-cci.c
> > > b/drivers/media/v4l2-core/v4l2-cci.c index bc2dbec019b04..673637b67bf67
> > > 100644
> > > --- a/drivers/media/v4l2-core/v4l2-cci.c
> > > +++ b/drivers/media/v4l2-core/v4l2-cci.c
> > > @@ -18,6 +18,7 @@
> > >
> > > int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)
> > > {
> > >
> > > + bool little_endian;
> > >
> > > unsigned int len;
> > > u8 buf[8];
> > > int ret;
> > >
> > > @@ -25,6 +26,7 @@ int cci_read(struct regmap *map, u32 reg, u64 *val,
> > > int *err)> >
> > > if (err && *err)
> > >
> > > return *err;
> > >
> > > + little_endian = reg & CCI_REG_LE;
> >
> > You could initialize the variable when declaring it. Same below.
>
> I was thinking of the same, but then it'd be logical to move initialisation
> of all related variables there. reg is modified here though. I'd keep
> setting little_endian here. If someone wants to move it, that could be done
> in a separate patch.
>
> > > len = FIELD_GET(CCI_REG_WIDTH_MASK, reg);
> > > reg = FIELD_GET(CCI_REG_ADDR_MASK, reg);
> > >
> > > @@ -40,16 +42,28 @@ int cci_read(struct regmap *map, u32 reg, u64 *val,
> > > int *err)> >
> > > *val = buf[0];
> > > break;
> > >
> > > case 2:
> > > - *val = get_unaligned_be16(buf);
> > > + if (little_endian)
> > > + *val = get_unaligned_le16(buf);
> > > + else
> > > + *val = get_unaligned_be16(buf);
> >
> > Unrelated to this patch, isn't buf aligned to a 4 bytes boundary ?
>
> Very probably, as it's right after len that's an unsigned int. Adding
> __aligned(8) would ensure we don't need any of the unaligned variants, and
> most likely would keep the stack layout as-is.
You mean something like this?
u8 __aligned(8) buf[8];
[...]
if (little_endian)
*val = le64_to_cpup(buf);
else
*val = be64_to_cpup(buf);
But what about 24 Bits? There is no le24_to_cpup. I would rather use the same
API for all cases.
> Or... how about putting it in an union with a u64? That would mean it's
> accessible as u64 alignment-wise while the alignment itself is up to the
> ABI. A comment would be good to have probably.
An additional union seems a bit too much here. Unless something suitable
already exists for general usage.
Best regards,
Alexander
> > > break;
> > >
> > > case 3:
> > > - *val = get_unaligned_be24(buf);
> > > + if (little_endian)
> > > + *val = get_unaligned_le24(buf);
> > > + else
> > > + *val = get_unaligned_be24(buf);
> > >
> > > break;
> > >
> > > case 4:
> > > - *val = get_unaligned_be32(buf);
> > > + if (little_endian)
> > > + *val = get_unaligned_le32(buf);
> > > + else
> > > + *val = get_unaligned_be32(buf);
> > >
> > > break;
> > >
> > > case 8:
> > > - *val = get_unaligned_be64(buf);
> > > + if (little_endian)
> > > + *val = get_unaligned_le64(buf);
> > > + else
> > > + *val = get_unaligned_be64(buf);
> > >
> > > break;
> > >
> > > default:
> > > dev_err(regmap_get_device(map), "Error invalid reg-width
%u for reg
> > > 0x%04x\n",
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
