[PATCH v2 0/2] v4l2-cci: little-endian registers

[PATCH v2 0/2] v4l2-cci: little-endian registers

[Alexander Stein]

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

[PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Alexander Stein]

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

[PATCH v2 2/2] media: i2c: imx290: Properly encode registers as little-endian

[Alexander Stein]

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

Re: [PATCH v2 0/2] v4l2-cci: little-endian registers

[Hans de Goede]

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

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Laurent Pinchart]

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

Re: [PATCH v2 2/2] media: i2c: imx290: Properly encode registers as little-endian

[Laurent Pinchart]

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

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[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.

> 
> > 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

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Alexander Stein]

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/

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Alexander Stein]

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/

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Laurent Pinchart]

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

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Sakari Ailus]

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

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Sakari Ailus]

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

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Sakari Ailus]

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

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Hans de Goede]

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

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Sakari Ailus]

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

Re: [PATCH v2 1/2] media: v4l2-cci: Add support for little-endian encoded registers

[Sakari Ailus]

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