| Message ID | 20231101122354.270453-2-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v4l2-cci: little-endian registers | expand |
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. > 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@vger.kernel.org > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > --- > 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. > 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 ? > 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", > @@ -68,6 +82,7 @@ EXPORT_SYMBOL_GPL(cci_read); > > int cci_write(struct regmap *map, u32 reg, u64 val, int *err) > { > + bool little_endian; > unsigned int len; > u8 buf[8]; > int ret; > @@ -75,6 +90,7 @@ int cci_write(struct regmap *map, u32 reg, u64 val, int *err) > if (err && *err) > return *err; > > + little_endian = reg & CCI_REG_LE; > len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); > reg = FIELD_GET(CCI_REG_ADDR_MASK, reg); > > @@ -83,16 +99,28 @@ int cci_write(struct regmap *map, u32 reg, u64 val, int *err) > buf[0] = val; > break; > case 2: > - put_unaligned_be16(val, buf); > + if (little_endian) > + put_unaligned_le16(val, buf); > + else > + put_unaligned_be16(val, buf); > break; > case 3: > - put_unaligned_be24(val, buf); > + if (little_endian) > + put_unaligned_le24(val, buf); > + else > + put_unaligned_be24(val, buf); > break; > case 4: > - put_unaligned_be32(val, buf); > + if (little_endian) > + put_unaligned_le32(val, buf); > + else > + put_unaligned_be32(val, buf); > break; > case 8: > - put_unaligned_be64(val, buf); > + if (little_endian) > + put_unaligned_le64(val, buf); > + else > + put_unaligned_be64(val, buf); > break; > default: > dev_err(regmap_get_device(map), "Error invalid reg-width %u for reg 0x%04x\n", > diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h > index 0f6803e4b17e9..ef3faf0c9d44d 100644 > --- a/include/media/v4l2-cci.h > +++ b/include/media/v4l2-cci.h > @@ -32,12 +32,17 @@ struct cci_reg_sequence { > #define CCI_REG_ADDR_MASK GENMASK(15, 0) > #define CCI_REG_WIDTH_SHIFT 16 > #define CCI_REG_WIDTH_MASK GENMASK(19, 16) > +#define CCI_REG_LE BIT(20) > > #define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) > #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) > #define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x)) > #define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x)) > #define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x)) > +#define CCI_REG16_LE(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > +#define CCI_REG24_LE(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > +#define CCI_REG32_LE(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > +#define CCI_REG64_LE(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) I would put CCI_REG_LE first, to match the bits order. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > /** > * cci_read() - Read a value from a single CCI register
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. > > > 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@vger.kernel.org > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > --- > > 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. 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. > > > 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",
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@vger.kernel.org > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > > --- > > > > > > 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",
Am Donnerstag, 2. November 2023, 02:22:17 CET schrieb Laurent Pinchart: > ******************** > Achtung externe E-Mail: Öffnen Sie Anhänge und Links nur, wenn Sie wissen, > dass diese aus einer sicheren Quelle stammen und sicher sind. Leiten Sie > die E-Mail im Zweifelsfall zur Prüfung an den IT-Helpdesk weiter. > Attention external email: Open attachments and links only if you know that > they are from a secure source and are safe. In doubt forward the email to > the IT-Helpdesk to check it. ******************** > > 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. > > > 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@vger.kernel.org > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > --- > > > > 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. > > > 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 ? > > > 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",> > > @@ -68,6 +82,7 @@ EXPORT_SYMBOL_GPL(cci_read); > > > > int cci_write(struct regmap *map, u32 reg, u64 val, int *err) > > { > > > > + bool little_endian; > > > > unsigned int len; > > u8 buf[8]; > > int ret; > > > > @@ -75,6 +90,7 @@ int cci_write(struct regmap *map, u32 reg, u64 val, int > > *err)> > > if (err && *err) > > > > return *err; > > > > + little_endian = reg & CCI_REG_LE; > > > > len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); > > reg = FIELD_GET(CCI_REG_ADDR_MASK, reg); > > > > @@ -83,16 +99,28 @@ int cci_write(struct regmap *map, u32 reg, u64 val, > > int *err)> > > buf[0] = val; > > break; > > > > case 2: > > - put_unaligned_be16(val, buf); > > + if (little_endian) > > + put_unaligned_le16(val, buf); > > + else > > + put_unaligned_be16(val, buf); > > > > break; > > > > case 3: > > - put_unaligned_be24(val, buf); > > + if (little_endian) > > + put_unaligned_le24(val, buf); > > + else > > + put_unaligned_be24(val, buf); > > > > break; > > > > case 4: > > - put_unaligned_be32(val, buf); > > + if (little_endian) > > + put_unaligned_le32(val, buf); > > + else > > + put_unaligned_be32(val, buf); > > > > break; > > > > case 8: > > - put_unaligned_be64(val, buf); > > + if (little_endian) > > + put_unaligned_le64(val, buf); > > + else > > + put_unaligned_be64(val, buf); > > > > break; > > > > default: > > dev_err(regmap_get_device(map), "Error invalid reg-width %u for reg > > 0x%04x\n",> > > diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h > > index 0f6803e4b17e9..ef3faf0c9d44d 100644 > > --- a/include/media/v4l2-cci.h > > +++ b/include/media/v4l2-cci.h > > @@ -32,12 +32,17 @@ struct cci_reg_sequence { > > > > #define CCI_REG_ADDR_MASK GENMASK(15, 0) > > #define CCI_REG_WIDTH_SHIFT 16 > > #define CCI_REG_WIDTH_MASK GENMASK(19, 16) > > > > +#define CCI_REG_LE BIT(20) > > > > #define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) > > #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) > > #define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x)) > > #define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x)) > > #define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x)) > > > > +#define CCI_REG16_LE(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > +#define CCI_REG24_LE(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > +#define CCI_REG32_LE(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > +#define CCI_REG64_LE(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > I would put CCI_REG_LE first, to match the bits order. You mean this order? CCI_REG8(x) CCI_REG16_LE(x) CCI_REG16(x) CCI_REG24_LE(x) CCI_REG24(x) CCI_REG32_LE(x) CCI_REG32(x) CCI_REG64_LE(x) CCI_REG64(x) I would either keep the _LE variants at the bottom or below to their big- endian counterpart. I prefer readability thus I would put the _LE at the bottom, also it aligns nicely with the additional bit set. Best regards, Alexander > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > /** > > > > * cci_read() - Read a value from a single CCI register
Hi Alexander, On Thu, Nov 02, 2023 at 08:55:01AM +0100, Alexander Stein wrote: > Am Donnerstag, 2. November 2023, 02:22:17 CET schrieb Laurent Pinchart: > > 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. > > > > > 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@vger.kernel.org > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > > --- > > > > > > drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------ > > > include/media/v4l2-cci.h | 5 ++++ > > > 2 files changed, 41 insertions(+), 8 deletions(-) [snip] > > > diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h > > > index 0f6803e4b17e9..ef3faf0c9d44d 100644 > > > --- a/include/media/v4l2-cci.h > > > +++ b/include/media/v4l2-cci.h > > > @@ -32,12 +32,17 @@ struct cci_reg_sequence { > > > #define CCI_REG_ADDR_MASK GENMASK(15, 0) > > > #define CCI_REG_WIDTH_SHIFT 16 > > > #define CCI_REG_WIDTH_MASK GENMASK(19, 16) > > > +#define CCI_REG_LE BIT(20) > > > > > > #define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) > > > #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) > > > #define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x)) > > > #define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x)) > > > #define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x)) > > > +#define CCI_REG16_LE(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > > +#define CCI_REG24_LE(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > > +#define CCI_REG32_LE(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > > +#define CCI_REG64_LE(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > > > I would put CCI_REG_LE first, to match the bits order. > > You mean this order? > > CCI_REG8(x) > CCI_REG16_LE(x) > CCI_REG16(x) > CCI_REG24_LE(x) > CCI_REG24(x) > CCI_REG32_LE(x) > CCI_REG32(x) > CCI_REG64_LE(x) > CCI_REG64(x) > > I would either keep the _LE variants at the bottom or below to their big- > endian counterpart. I prefer readability thus I would put the _LE at the > bottom, also it aligns nicely with the additional bit set. No, I meant #define CCI_REG16_LE(x) (CCI_REG_LE | (2 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG24_LE(x) (CCI_REG_LE | (3 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG32_LE(x) (CCI_REG_LE | (4 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG64_LE(x) (CCI_REG_LE | (8 << CCI_REG_WIDTH_SHIFT) | (x)) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > /** > > > > > > * cci_read() - Read a value from a single CCI register
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@vger.kernel.org > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > > > --- > > > > > > > > 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.
Hi Laurent, Alexander, On Thu, Nov 02, 2023 at 10:24:30AM +0200, Laurent Pinchart wrote: > Hi Alexander, > > On Thu, Nov 02, 2023 at 08:55:01AM +0100, Alexander Stein wrote: > > Am Donnerstag, 2. November 2023, 02:22:17 CET schrieb Laurent Pinchart: > > > 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. > > > > > > > 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@vger.kernel.org > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > > > --- > > > > > > > > drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------ > > > > include/media/v4l2-cci.h | 5 ++++ > > > > 2 files changed, 41 insertions(+), 8 deletions(-) > > [snip] > > > > > diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h > > > > index 0f6803e4b17e9..ef3faf0c9d44d 100644 > > > > --- a/include/media/v4l2-cci.h > > > > +++ b/include/media/v4l2-cci.h > > > > @@ -32,12 +32,17 @@ struct cci_reg_sequence { > > > > #define CCI_REG_ADDR_MASK GENMASK(15, 0) > > > > #define CCI_REG_WIDTH_SHIFT 16 > > > > #define CCI_REG_WIDTH_MASK GENMASK(19, 16) > > > > +#define CCI_REG_LE BIT(20) > > > > > > > > #define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) > > > > #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) > > > > #define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x)) > > > > #define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x)) > > > > #define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x)) > > > > +#define CCI_REG16_LE(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > > > +#define CCI_REG24_LE(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > > > +#define CCI_REG32_LE(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > > > +#define CCI_REG64_LE(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) > > > > > > I would put CCI_REG_LE first, to match the bits order. > > > > You mean this order? > > > > CCI_REG8(x) > > CCI_REG16_LE(x) > > CCI_REG16(x) > > CCI_REG24_LE(x) > > CCI_REG24(x) > > CCI_REG32_LE(x) > > CCI_REG32(x) > > CCI_REG64_LE(x) > > CCI_REG64(x) > > > > I would either keep the _LE variants at the bottom or below to their big- > > endian counterpart. I prefer readability thus I would put the _LE at the > > bottom, also it aligns nicely with the additional bit set. > > No, I meant > > #define CCI_REG16_LE(x) (CCI_REG_LE | (2 << CCI_REG_WIDTH_SHIFT) | (x)) > #define CCI_REG24_LE(x) (CCI_REG_LE | (3 << CCI_REG_WIDTH_SHIFT) | (x)) > #define CCI_REG32_LE(x) (CCI_REG_LE | (4 << CCI_REG_WIDTH_SHIFT) | (x)) > #define CCI_REG64_LE(x) (CCI_REG_LE | (8 << CCI_REG_WIDTH_SHIFT) | (x)) Ideally these numbers would be unsigned, i.e. #define CCI_REG16_LE(x) (CCI_REG_LE | (2U << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG24_LE(x) (CCI_REG_LE | (3U << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG32_LE(x) (CCI_REG_LE | (4U << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG64_LE(x) (CCI_REG_LE | (8U << CCI_REG_WIDTH_SHIFT) | (x)) This is a pre-existing problem. Feel free to add a patch to address it. :-)
On Thu, Nov 02, 2023 at 08:31:44AM +0000, Sakari Ailus wrote:
> This is a pre-existing problem. Feel free to add a patch to address it. :-)
I forgot to add that addressing may be part of the same set but come as
last, to avoid unnecessarily backporting patches. There's no bug in the
code related to this --- just a bug-prone pattern.
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@vger.kernel.org >>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> >>>>> --- >>>>> >>>>> 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 . Regards, Hans
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@vger.kernel.org > >>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > >>>>> --- > >>>>> > >>>>> 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.
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@vger.kernel.org > > >>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > >>>>> --- > > >>>>> > > >>>>> 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.
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; 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); 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", @@ -68,6 +82,7 @@ EXPORT_SYMBOL_GPL(cci_read); int cci_write(struct regmap *map, u32 reg, u64 val, int *err) { + bool little_endian; unsigned int len; u8 buf[8]; int ret; @@ -75,6 +90,7 @@ int cci_write(struct regmap *map, u32 reg, u64 val, int *err) if (err && *err) return *err; + little_endian = reg & CCI_REG_LE; len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); reg = FIELD_GET(CCI_REG_ADDR_MASK, reg); @@ -83,16 +99,28 @@ int cci_write(struct regmap *map, u32 reg, u64 val, int *err) buf[0] = val; break; case 2: - put_unaligned_be16(val, buf); + if (little_endian) + put_unaligned_le16(val, buf); + else + put_unaligned_be16(val, buf); break; case 3: - put_unaligned_be24(val, buf); + if (little_endian) + put_unaligned_le24(val, buf); + else + put_unaligned_be24(val, buf); break; case 4: - put_unaligned_be32(val, buf); + if (little_endian) + put_unaligned_le32(val, buf); + else + put_unaligned_be32(val, buf); break; case 8: - put_unaligned_be64(val, buf); + if (little_endian) + put_unaligned_le64(val, buf); + else + put_unaligned_be64(val, buf); break; default: dev_err(regmap_get_device(map), "Error invalid reg-width %u for reg 0x%04x\n", diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h index 0f6803e4b17e9..ef3faf0c9d44d 100644 --- a/include/media/v4l2-cci.h +++ b/include/media/v4l2-cci.h @@ -32,12 +32,17 @@ struct cci_reg_sequence { #define CCI_REG_ADDR_MASK GENMASK(15, 0) #define CCI_REG_WIDTH_SHIFT 16 #define CCI_REG_WIDTH_MASK GENMASK(19, 16) +#define CCI_REG_LE BIT(20) #define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x)) +#define CCI_REG16_LE(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG24_LE(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG32_LE(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG64_LE(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) /** * cci_read() - Read a value from a single CCI register
Some sensors, e.g. Sony, are using little-endian registers. Add support for 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@vger.kernel.org Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------ include/media/v4l2-cci.h | 5 ++++ 2 files changed, 41 insertions(+), 8 deletions(-)