On Thu, Nov 02, 2023 at 09:56:24AM +0000, Sakari Ailus wrote:
> On Thu, Nov 02, 2023 at 10:27:45AM +0100, Hans de Goede wrote:
> > Hi,
> >
> > On 11/2/23 09:25, Sakari Ailus wrote:
> > > Hi Alexander,
> > >
> > > On Thu, Nov 02, 2023 at 08:51:12AM +0100, Alexander Stein wrote:
> > >> 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.
> > >
> > > The aligned APIs are much better choice when you can use them. The 24 bit
> > > case can remain special IMO.
> > >
> > >>
> > >>> 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.
> > >
> > > I think it's nicer than using __aligned() as you get ABI alignment that
> > > way, not something you force manually --- that's a bit crude.
> > >
> > > I wonder that others think.
> >
> > I'm fine with adding the __aligned(8) and switching the non 24 bit
> > cases to helpers which assume alignment. The most important note
> > I have is that that is a separate improvement from this series though.
> >
> > So this should be done in a follow-up patch which is not Cc: stable .
>
> I'm fine with that.
>
> So I think these are good as-is then.
Or rather with the non-functional changes made in v3.
--
Sakari Ailus
