* [PATCH v2 0/2] v4l2-cci: little-endian registers
@ 2023-11-01 12:23 Alexander Stein
2023-11-01 12:23 ` [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers Alexander Stein
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Alexander Stein @ 2023-11-01 12:23 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Manivannan Sadhasivam,
Laurent Pinchart, Hans de Goede
Cc: Alexander Stein, linux-media, Alain Volmat
Hi,
after the discussions at [1] and [2], this series adds proper support for
little-endian encoded registers.
Patch 1 adds the infrastructure and patch 2 fixes imx290 driver.
As v6.6 was released with imx290 broken, both should be added to stable.
Changes in v2:
* Add proper Fixes/CC tags to both patches
* Add little-endian support for cci_read()
Best regards,
Alexander
[1] https://lore.kernel.org/linux-media/20231030173637.GA2977515@gnbcxd0016.gnb.st.com/
[2] https://lore.kernel.org/linux-media/ZUIuNDTJAN_fz3q6@kekkonen.localdomain/
Alexander Stein (2):
media: v4l2-cci: Add support for little-endian encoded registers
media: i2c: imx290: Properly encode registers as little-endian
drivers/media/i2c/imx290.c | 42 ++++++++++++++--------------
drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------
include/media/v4l2-cci.h | 5 ++++
3 files changed, 62 insertions(+), 29 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-01 12:23 [PATCH v2 0/2] v4l2-cci: little-endian registers Alexander Stein
@ 2023-11-01 12:23 ` Alexander Stein
2023-11-02 1:22 ` Laurent Pinchart
2023-11-01 12:23 ` [PATCH v2 2/2] media: i2c: imx290: Properly encode registers as little-endian Alexander Stein
2023-11-01 15:26 ` [PATCH v2 0/2] v4l2-cci: little-endian registers Hans de Goede
2 siblings, 1 reply; 16+ messages in thread
From: Alexander Stein @ 2023-11-01 12:23 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Manivannan Sadhasivam,
Laurent Pinchart, Hans de Goede
Cc: Alexander Stein, linux-media, Alain Volmat, stable
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(-)
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
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] media: i2c: imx290: Properly encode registers as little-endian
2023-11-01 12:23 [PATCH v2 0/2] v4l2-cci: little-endian registers Alexander Stein
2023-11-01 12:23 ` [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers Alexander Stein
@ 2023-11-01 12:23 ` Alexander Stein
2023-11-02 1:23 ` Laurent Pinchart
2023-11-01 15:26 ` [PATCH v2 0/2] v4l2-cci: little-endian registers Hans de Goede
2 siblings, 1 reply; 16+ messages in thread
From: Alexander Stein @ 2023-11-01 12:23 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Manivannan Sadhasivam,
Laurent Pinchart, Hans de Goede
Cc: Alexander Stein, linux-media, Alain Volmat, stable
The conversion to CCI also converted the multi-byte register access to
big-endian. Correct the register definition by using the correct
little-endian ones.
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/i2c/imx290.c | 42 +++++++++++++++++++-------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 29098612813cb..c6fea5837a19f 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -41,18 +41,18 @@
#define IMX290_WINMODE_720P (1 << 4)
#define IMX290_WINMODE_CROP (4 << 4)
#define IMX290_FR_FDG_SEL CCI_REG8(0x3009)
-#define IMX290_BLKLEVEL CCI_REG16(0x300a)
+#define IMX290_BLKLEVEL CCI_REG16_LE(0x300a)
#define IMX290_GAIN CCI_REG8(0x3014)
-#define IMX290_VMAX CCI_REG24(0x3018)
+#define IMX290_VMAX CCI_REG24_LE(0x3018)
#define IMX290_VMAX_MAX 0x3ffff
-#define IMX290_HMAX CCI_REG16(0x301c)
+#define IMX290_HMAX CCI_REG16_LE(0x301c)
#define IMX290_HMAX_MAX 0xffff
-#define IMX290_SHS1 CCI_REG24(0x3020)
+#define IMX290_SHS1 CCI_REG24_LE(0x3020)
#define IMX290_WINWV_OB CCI_REG8(0x303a)
-#define IMX290_WINPV CCI_REG16(0x303c)
-#define IMX290_WINWV CCI_REG16(0x303e)
-#define IMX290_WINPH CCI_REG16(0x3040)
-#define IMX290_WINWH CCI_REG16(0x3042)
+#define IMX290_WINPV CCI_REG16_LE(0x303c)
+#define IMX290_WINWV CCI_REG16_LE(0x303e)
+#define IMX290_WINPH CCI_REG16_LE(0x3040)
+#define IMX290_WINWH CCI_REG16_LE(0x3042)
#define IMX290_OUT_CTRL CCI_REG8(0x3046)
#define IMX290_ODBIT_10BIT (0 << 0)
#define IMX290_ODBIT_12BIT (1 << 0)
@@ -78,28 +78,28 @@
#define IMX290_ADBIT2 CCI_REG8(0x317c)
#define IMX290_ADBIT2_10BIT 0x12
#define IMX290_ADBIT2_12BIT 0x00
-#define IMX290_CHIP_ID CCI_REG16(0x319a)
+#define IMX290_CHIP_ID CCI_REG16_LE(0x319a)
#define IMX290_ADBIT3 CCI_REG8(0x31ec)
#define IMX290_ADBIT3_10BIT 0x37
#define IMX290_ADBIT3_12BIT 0x0e
#define IMX290_REPETITION CCI_REG8(0x3405)
#define IMX290_PHY_LANE_NUM CCI_REG8(0x3407)
#define IMX290_OPB_SIZE_V CCI_REG8(0x3414)
-#define IMX290_Y_OUT_SIZE CCI_REG16(0x3418)
-#define IMX290_CSI_DT_FMT CCI_REG16(0x3441)
+#define IMX290_Y_OUT_SIZE CCI_REG16_LE(0x3418)
+#define IMX290_CSI_DT_FMT CCI_REG16_LE(0x3441)
#define IMX290_CSI_DT_FMT_RAW10 0x0a0a
#define IMX290_CSI_DT_FMT_RAW12 0x0c0c
#define IMX290_CSI_LANE_MODE CCI_REG8(0x3443)
-#define IMX290_EXTCK_FREQ CCI_REG16(0x3444)
-#define IMX290_TCLKPOST CCI_REG16(0x3446)
-#define IMX290_THSZERO CCI_REG16(0x3448)
-#define IMX290_THSPREPARE CCI_REG16(0x344a)
-#define IMX290_TCLKTRAIL CCI_REG16(0x344c)
-#define IMX290_THSTRAIL CCI_REG16(0x344e)
-#define IMX290_TCLKZERO CCI_REG16(0x3450)
-#define IMX290_TCLKPREPARE CCI_REG16(0x3452)
-#define IMX290_TLPX CCI_REG16(0x3454)
-#define IMX290_X_OUT_SIZE CCI_REG16(0x3472)
+#define IMX290_EXTCK_FREQ CCI_REG16_LE(0x3444)
+#define IMX290_TCLKPOST CCI_REG16_LE(0x3446)
+#define IMX290_THSZERO CCI_REG16_LE(0x3448)
+#define IMX290_THSPREPARE CCI_REG16_LE(0x344a)
+#define IMX290_TCLKTRAIL CCI_REG16_LE(0x344c)
+#define IMX290_THSTRAIL CCI_REG16_LE(0x344e)
+#define IMX290_TCLKZERO CCI_REG16_LE(0x3450)
+#define IMX290_TCLKPREPARE CCI_REG16_LE(0x3452)
+#define IMX290_TLPX CCI_REG16_LE(0x3454)
+#define IMX290_X_OUT_SIZE CCI_REG16_LE(0x3472)
#define IMX290_INCKSEL7 CCI_REG8(0x3480)
#define IMX290_PGCTRL_REGEN BIT(0)
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/2] v4l2-cci: little-endian registers
2023-11-01 12:23 [PATCH v2 0/2] v4l2-cci: little-endian registers Alexander Stein
2023-11-01 12:23 ` [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers Alexander Stein
2023-11-01 12:23 ` [PATCH v2 2/2] media: i2c: imx290: Properly encode registers as little-endian Alexander Stein
@ 2023-11-01 15:26 ` Hans de Goede
2 siblings, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-11-01 15:26 UTC (permalink / raw)
To: Alexander Stein, Mauro Carvalho Chehab, Sakari Ailus,
Manivannan Sadhasivam, Laurent Pinchart
Cc: linux-media, Alain Volmat
Hoi,
On 11/1/23 13:23, Alexander Stein wrote:
> Hi,
>
> after the discussions at [1] and [2], this series adds proper support for
> little-endian encoded registers.
> Patch 1 adds the infrastructure and patch 2 fixes imx290 driver.
> As v6.6 was released with imx290 broken, both should be added to stable.
>
> Changes in v2:
> * Add proper Fixes/CC tags to both patches
> * Add little-endian support for cci_read()
Thanks, this version looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
for the series.
Regards,
Hans
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-01 12:23 ` [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers Alexander Stein
@ 2023-11-02 1:22 ` Laurent Pinchart
2023-11-02 6:30 ` Sakari Ailus
2023-11-02 7:55 ` Alexander Stein
0 siblings, 2 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-11-02 1:22 UTC (permalink / raw)
To: Alexander Stein
Cc: Mauro Carvalho Chehab, Sakari Ailus, Manivannan Sadhasivam,
Hans de Goede, linux-media, Alain Volmat, stable
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
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] media: i2c: imx290: Properly encode registers as little-endian
2023-11-01 12:23 ` [PATCH v2 2/2] media: i2c: imx290: Properly encode registers as little-endian Alexander Stein
@ 2023-11-02 1:23 ` Laurent Pinchart
0 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-11-02 1:23 UTC (permalink / raw)
To: Alexander Stein
Cc: Mauro Carvalho Chehab, Sakari Ailus, Manivannan Sadhasivam,
Hans de Goede, linux-media, Alain Volmat, stable
Hi Alexander,
Thank you for the patch.
On Wed, Nov 01, 2023 at 01:23:54PM +0100, Alexander Stein wrote:
> The conversion to CCI also converted the multi-byte register access to
> big-endian. Correct the register definition by using the correct
> little-endian ones.
>
> 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/i2c/imx290.c | 42 +++++++++++++++++++-------------------
> 1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 29098612813cb..c6fea5837a19f 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -41,18 +41,18 @@
> #define IMX290_WINMODE_720P (1 << 4)
> #define IMX290_WINMODE_CROP (4 << 4)
> #define IMX290_FR_FDG_SEL CCI_REG8(0x3009)
> -#define IMX290_BLKLEVEL CCI_REG16(0x300a)
> +#define IMX290_BLKLEVEL CCI_REG16_LE(0x300a)
> #define IMX290_GAIN CCI_REG8(0x3014)
> -#define IMX290_VMAX CCI_REG24(0x3018)
> +#define IMX290_VMAX CCI_REG24_LE(0x3018)
> #define IMX290_VMAX_MAX 0x3ffff
> -#define IMX290_HMAX CCI_REG16(0x301c)
> +#define IMX290_HMAX CCI_REG16_LE(0x301c)
> #define IMX290_HMAX_MAX 0xffff
> -#define IMX290_SHS1 CCI_REG24(0x3020)
> +#define IMX290_SHS1 CCI_REG24_LE(0x3020)
> #define IMX290_WINWV_OB CCI_REG8(0x303a)
> -#define IMX290_WINPV CCI_REG16(0x303c)
> -#define IMX290_WINWV CCI_REG16(0x303e)
> -#define IMX290_WINPH CCI_REG16(0x3040)
> -#define IMX290_WINWH CCI_REG16(0x3042)
> +#define IMX290_WINPV CCI_REG16_LE(0x303c)
> +#define IMX290_WINWV CCI_REG16_LE(0x303e)
> +#define IMX290_WINPH CCI_REG16_LE(0x3040)
> +#define IMX290_WINWH CCI_REG16_LE(0x3042)
> #define IMX290_OUT_CTRL CCI_REG8(0x3046)
> #define IMX290_ODBIT_10BIT (0 << 0)
> #define IMX290_ODBIT_12BIT (1 << 0)
> @@ -78,28 +78,28 @@
> #define IMX290_ADBIT2 CCI_REG8(0x317c)
> #define IMX290_ADBIT2_10BIT 0x12
> #define IMX290_ADBIT2_12BIT 0x00
> -#define IMX290_CHIP_ID CCI_REG16(0x319a)
> +#define IMX290_CHIP_ID CCI_REG16_LE(0x319a)
> #define IMX290_ADBIT3 CCI_REG8(0x31ec)
> #define IMX290_ADBIT3_10BIT 0x37
> #define IMX290_ADBIT3_12BIT 0x0e
> #define IMX290_REPETITION CCI_REG8(0x3405)
> #define IMX290_PHY_LANE_NUM CCI_REG8(0x3407)
> #define IMX290_OPB_SIZE_V CCI_REG8(0x3414)
> -#define IMX290_Y_OUT_SIZE CCI_REG16(0x3418)
> -#define IMX290_CSI_DT_FMT CCI_REG16(0x3441)
> +#define IMX290_Y_OUT_SIZE CCI_REG16_LE(0x3418)
> +#define IMX290_CSI_DT_FMT CCI_REG16_LE(0x3441)
> #define IMX290_CSI_DT_FMT_RAW10 0x0a0a
> #define IMX290_CSI_DT_FMT_RAW12 0x0c0c
> #define IMX290_CSI_LANE_MODE CCI_REG8(0x3443)
> -#define IMX290_EXTCK_FREQ CCI_REG16(0x3444)
> -#define IMX290_TCLKPOST CCI_REG16(0x3446)
> -#define IMX290_THSZERO CCI_REG16(0x3448)
> -#define IMX290_THSPREPARE CCI_REG16(0x344a)
> -#define IMX290_TCLKTRAIL CCI_REG16(0x344c)
> -#define IMX290_THSTRAIL CCI_REG16(0x344e)
> -#define IMX290_TCLKZERO CCI_REG16(0x3450)
> -#define IMX290_TCLKPREPARE CCI_REG16(0x3452)
> -#define IMX290_TLPX CCI_REG16(0x3454)
> -#define IMX290_X_OUT_SIZE CCI_REG16(0x3472)
> +#define IMX290_EXTCK_FREQ CCI_REG16_LE(0x3444)
> +#define IMX290_TCLKPOST CCI_REG16_LE(0x3446)
> +#define IMX290_THSZERO CCI_REG16_LE(0x3448)
> +#define IMX290_THSPREPARE CCI_REG16_LE(0x344a)
> +#define IMX290_TCLKTRAIL CCI_REG16_LE(0x344c)
> +#define IMX290_THSTRAIL CCI_REG16_LE(0x344e)
> +#define IMX290_TCLKZERO CCI_REG16_LE(0x3450)
> +#define IMX290_TCLKPREPARE CCI_REG16_LE(0x3452)
> +#define IMX290_TLPX CCI_REG16_LE(0x3454)
> +#define IMX290_X_OUT_SIZE CCI_REG16_LE(0x3472)
> #define IMX290_INCKSEL7 CCI_REG8(0x3480)
>
> #define IMX290_PGCTRL_REGEN BIT(0)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-02 1:22 ` Laurent Pinchart
@ 2023-11-02 6:30 ` Sakari Ailus
2023-11-02 7:51 ` Alexander Stein
2023-11-02 7:55 ` Alexander Stein
1 sibling, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2023-11-02 6:30 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Alexander Stein, Mauro Carvalho Chehab, Manivannan Sadhasivam,
Hans de Goede, linux-media, Alain Volmat, stable
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",
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-02 6:30 ` Sakari Ailus
@ 2023-11-02 7:51 ` Alexander Stein
2023-11-02 8:25 ` Sakari Ailus
0 siblings, 1 reply; 16+ messages in thread
From: Alexander Stein @ 2023-11-02 7:51 UTC (permalink / raw)
To: Laurent Pinchart, Sakari Ailus
Cc: Mauro Carvalho Chehab, Manivannan Sadhasivam, Hans de Goede,
linux-media, Alain Volmat, stable
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",
--
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/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-02 1:22 ` Laurent Pinchart
2023-11-02 6:30 ` Sakari Ailus
@ 2023-11-02 7:55 ` Alexander Stein
2023-11-02 8:24 ` Laurent Pinchart
1 sibling, 1 reply; 16+ messages in thread
From: Alexander Stein @ 2023-11-02 7:55 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, Manivannan Sadhasivam,
Hans de Goede, linux-media, Alain Volmat, stable
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
--
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/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-02 7:55 ` Alexander Stein
@ 2023-11-02 8:24 ` Laurent Pinchart
2023-11-02 8:31 ` Sakari Ailus
0 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2023-11-02 8:24 UTC (permalink / raw)
To: Alexander Stein
Cc: Mauro Carvalho Chehab, Sakari Ailus, Manivannan Sadhasivam,
Hans de Goede, linux-media, Alain Volmat, stable
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
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-02 7:51 ` Alexander Stein
@ 2023-11-02 8:25 ` Sakari Ailus
2023-11-02 9:27 ` Hans de Goede
0 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2023-11-02 8:25 UTC (permalink / raw)
To: Alexander Stein
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Manivannan Sadhasivam,
Hans de Goede, linux-media, Alain Volmat, stable
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.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-02 8:24 ` Laurent Pinchart
@ 2023-11-02 8:31 ` Sakari Ailus
2023-11-02 8:33 ` Sakari Ailus
0 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2023-11-02 8:31 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Alexander Stein, Mauro Carvalho Chehab, Manivannan Sadhasivam,
Hans de Goede, linux-media, Alain Volmat, stable
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. :-)
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-02 8:31 ` Sakari Ailus
@ 2023-11-02 8:33 ` Sakari Ailus
0 siblings, 0 replies; 16+ messages in thread
From: Sakari Ailus @ 2023-11-02 8:33 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Alexander Stein, Mauro Carvalho Chehab, Manivannan Sadhasivam,
Hans de Goede, linux-media, Alain Volmat, stable
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.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-02 8:25 ` Sakari Ailus
@ 2023-11-02 9:27 ` Hans de Goede
2023-11-02 9:56 ` Sakari Ailus
0 siblings, 1 reply; 16+ messages in thread
From: Hans de Goede @ 2023-11-02 9:27 UTC (permalink / raw)
To: Sakari Ailus, Alexander Stein
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Manivannan Sadhasivam,
linux-media, Alain Volmat, stable
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-02 9:27 ` Hans de Goede
@ 2023-11-02 9:56 ` Sakari Ailus
2023-11-02 9:58 ` Sakari Ailus
0 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2023-11-02 9:56 UTC (permalink / raw)
To: Hans de Goede
Cc: Alexander Stein, Laurent Pinchart, Mauro Carvalho Chehab,
Manivannan Sadhasivam, linux-media, Alain Volmat, stable
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.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers
2023-11-02 9:56 ` Sakari Ailus
@ 2023-11-02 9:58 ` Sakari Ailus
0 siblings, 0 replies; 16+ messages in thread
From: Sakari Ailus @ 2023-11-02 9:58 UTC (permalink / raw)
To: Hans de Goede
Cc: Alexander Stein, Laurent Pinchart, Mauro Carvalho Chehab,
Manivannan Sadhasivam, linux-media, Alain Volmat, stable
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.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-11-02 9:58 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-01 12:23 [PATCH v2 0/2] v4l2-cci: little-endian registers Alexander Stein
2023-11-01 12:23 ` [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers Alexander Stein
2023-11-02 1:22 ` Laurent Pinchart
2023-11-02 6:30 ` Sakari Ailus
2023-11-02 7:51 ` Alexander Stein
2023-11-02 8:25 ` Sakari Ailus
2023-11-02 9:27 ` Hans de Goede
2023-11-02 9:56 ` Sakari Ailus
2023-11-02 9:58 ` Sakari Ailus
2023-11-02 7:55 ` Alexander Stein
2023-11-02 8:24 ` Laurent Pinchart
2023-11-02 8:31 ` Sakari Ailus
2023-11-02 8:33 ` Sakari Ailus
2023-11-01 12:23 ` [PATCH v2 2/2] media: i2c: imx290: Properly encode registers as little-endian Alexander Stein
2023-11-02 1:23 ` Laurent Pinchart
2023-11-01 15:26 ` [PATCH v2 0/2] v4l2-cci: little-endian registers Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).