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
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);
*val = get_unaligned_be32(buf);
if (little_endian)*val = get_unaligned_le32(buf);else*val = get_unaligned_be32(buf);
*val = get_unaligned_be64(buf);
if (little_endian)*val = get_unaligned_le64(buf);else*val = get_unaligned_be64(buf);@@ -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);elseput_unaligned_be16(val, buf);
put_unaligned_be24(val, buf);
if (little_endian)put_unaligned_le24(val, buf);elseput_unaligned_be24(val, buf);
put_unaligned_be32(val, buf);
if (little_endian)put_unaligned_le32(val, buf);elseput_unaligned_be32(val, buf);
put_unaligned_be64(val, buf);
if (little_endian)put_unaligned_le64(val, buf);elseput_unaligned_be64(val, buf);diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h index 0f6803e4b17e9..ef3faf0c9d44d 100644 --- a/include/media/v4l2-cci.h +++ b/include/media/v4l2-cci.h @@ -32,12 +32,17 @@ struct cci_reg_sequence { #define CCI_REG_ADDR_MASK GENMASK(15, 0) #define CCI_REG_WIDTH_SHIFT 16 #define CCI_REG_WIDTH_MASK GENMASK(19, 16) +#define CCI_REG_LE BIT(20) #define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x)) +#define CCI_REG16_LE(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG24_LE(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG32_LE(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG64_LE(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE)
I would put CCI_REG_LE first, to match the bits order.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
/**
- cci_read() - Read a value from a single CCI register
Hi Laurent,
On Thu, Nov 02, 2023 at 03:22:17AM +0200, Laurent Pinchart wrote:
Hi Alexander,
Thank you for the patch.
On Wed, Nov 01, 2023 at 01:23:53PM +0100, Alexander Stein wrote:
Some sensors, e.g. Sony, are using little-endian registers. Add support for
I would write Sony IMX290 here, as there are Sony sensors that use big endian.
Almost all of them. There are a few exceptions indeed. This seems to be a bug.
those by encoding the endianess into Bit 20 of the register address.
Fixes: af73323b97702 ("media: imx290: Convert to new CCI register access helpers") Cc: stable@vger.kernel.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------ include/media/v4l2-cci.h | 5 ++++ 2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-cci.c b/drivers/media/v4l2-core/v4l2-cci.c index bc2dbec019b04..673637b67bf67 100644 --- a/drivers/media/v4l2-core/v4l2-cci.c +++ b/drivers/media/v4l2-core/v4l2-cci.c @@ -18,6 +18,7 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) {
- bool little_endian; unsigned int len; u8 buf[8]; int ret;
@@ -25,6 +26,7 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) if (err && *err) return *err;
- little_endian = reg & CCI_REG_LE;
You could initialize the variable when declaring it. Same below.
I was thinking of the same, but then it'd be logical to move initialisation of all related variables there. reg is modified here though. I'd keep setting little_endian here. If someone wants to move it, that could be done in a separate patch.
len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); reg = FIELD_GET(CCI_REG_ADDR_MASK, reg); @@ -40,16 +42,28 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) *val = buf[0]; break; case 2:
*val = get_unaligned_be16(buf);
if (little_endian)*val = get_unaligned_le16(buf);else*val = get_unaligned_be16(buf);Unrelated to this patch, isn't buf aligned to a 4 bytes boundary ?
Very probably, as it's right after len that's an unsigned int. Adding __aligned(8) would ensure we don't need any of the unaligned variants, and most likely would keep the stack layout as-is.
Or... how about putting it in an union with a u64? That would mean it's accessible as u64 alignment-wise while the alignment itself is up to the ABI. A comment would be good to have probably.
break;case 3:
*val = get_unaligned_be24(buf);
if (little_endian)*val = get_unaligned_le24(buf);else*val = get_unaligned_be24(buf);
*val = get_unaligned_be32(buf);
if (little_endian)*val = get_unaligned_le32(buf);else*val = get_unaligned_be32(buf);
*val = get_unaligned_be64(buf);
if (little_endian)*val = get_unaligned_le64(buf);else*val = get_unaligned_be64(buf);
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",
Hi Alexander,
On Thu, Nov 02, 2023 at 08:51:12AM +0100, Alexander Stein wrote:
Hi,
thanks for the feedback.
Am Donnerstag, 2. November 2023, 07:30:44 CET schrieb Sakari Ailus:
Hi Laurent,
On Thu, Nov 02, 2023 at 03:22:17AM +0200, Laurent Pinchart wrote:
Hi Alexander,
Thank you for the patch.
On Wed, Nov 01, 2023 at 01:23:53PM +0100, Alexander Stein wrote:
Some sensors, e.g. Sony, are using little-endian registers. Add support for
I would write Sony IMX290 here, as there are Sony sensors that use big endian.
Almost all of them. There are a few exceptions indeed. This seems to be a bug.
Let's name IMX290 here as an actual example. No need to worry about other models here.
those by encoding the endianess into Bit 20 of the register address.
Fixes: af73323b97702 ("media: imx290: Convert to new CCI register access helpers") Cc: stable@vger.kernel.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------ include/media/v4l2-cci.h | 5 ++++ 2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-cci.c b/drivers/media/v4l2-core/v4l2-cci.c index bc2dbec019b04..673637b67bf67 100644 --- a/drivers/media/v4l2-core/v4l2-cci.c +++ b/drivers/media/v4l2-core/v4l2-cci.c @@ -18,6 +18,7 @@
int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) {
bool little_endian;
unsigned int len; u8 buf[8]; int ret;
@@ -25,6 +26,7 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)> > if (err && *err) return *err;
- little_endian = reg & CCI_REG_LE;
You could initialize the variable when declaring it. Same below.
I was thinking of the same, but then it'd be logical to move initialisation of all related variables there. reg is modified here though. I'd keep setting little_endian here. If someone wants to move it, that could be done in a separate patch.
len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); reg = FIELD_GET(CCI_REG_ADDR_MASK, reg);
@@ -40,16 +42,28 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)> > *val = buf[0]; break; case 2:
*val = get_unaligned_be16(buf);
if (little_endian)*val = get_unaligned_le16(buf);else*val = get_unaligned_be16(buf);Unrelated to this patch, isn't buf aligned to a 4 bytes boundary ?
Very probably, as it's right after len that's an unsigned int. Adding __aligned(8) would ensure we don't need any of the unaligned variants, and most likely would keep the stack layout as-is.
You mean something like this?
u8 __aligned(8) buf[8]; [...] if (little_endian) *val = le64_to_cpup(buf); else *val = be64_to_cpup(buf);
But what about 24 Bits? There is no le24_to_cpup. I would rather use the same API for all cases.
The aligned APIs are much better choice when you can use them. The 24 bit case can remain special IMO.
Or... how about putting it in an union with a u64? That would mean it's accessible as u64 alignment-wise while the alignment itself is up to the ABI. A comment would be good to have probably.
An additional union seems a bit too much here. Unless something suitable already exists for general usage.
I think it's nicer than using __aligned() as you get ABI alignment that way, not something you force manually --- that's a bit crude.
I wonder that others think.
Hi,
On 11/2/23 09:25, Sakari Ailus wrote:
Hi Alexander,
On Thu, Nov 02, 2023 at 08:51:12AM +0100, Alexander Stein wrote:
Hi,
thanks for the feedback.
Am Donnerstag, 2. November 2023, 07:30:44 CET schrieb Sakari Ailus:
Hi Laurent,
On Thu, Nov 02, 2023 at 03:22:17AM +0200, Laurent Pinchart wrote:
Hi Alexander,
Thank you for the patch.
On Wed, Nov 01, 2023 at 01:23:53PM +0100, Alexander Stein wrote:
Some sensors, e.g. Sony, are using little-endian registers. Add support for
I would write Sony IMX290 here, as there are Sony sensors that use big endian.
Almost all of them. There are a few exceptions indeed. This seems to be a bug.
Let's name IMX290 here as an actual example. No need to worry about other models here.
those by encoding the endianess into Bit 20 of the register address.
Fixes: af73323b97702 ("media: imx290: Convert to new CCI register access helpers") Cc: stable@vger.kernel.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------ include/media/v4l2-cci.h | 5 ++++ 2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-cci.c b/drivers/media/v4l2-core/v4l2-cci.c index bc2dbec019b04..673637b67bf67 100644 --- a/drivers/media/v4l2-core/v4l2-cci.c +++ b/drivers/media/v4l2-core/v4l2-cci.c @@ -18,6 +18,7 @@
int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) {
bool little_endian;
unsigned int len; u8 buf[8]; int ret;
@@ -25,6 +26,7 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)> > if (err && *err) return *err;
- little_endian = reg & CCI_REG_LE;
You could initialize the variable when declaring it. Same below.
I was thinking of the same, but then it'd be logical to move initialisation of all related variables there. reg is modified here though. I'd keep setting little_endian here. If someone wants to move it, that could be done in a separate patch.
len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); reg = FIELD_GET(CCI_REG_ADDR_MASK, reg);
@@ -40,16 +42,28 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)> > *val = buf[0]; break; case 2:
*val = get_unaligned_be16(buf);
if (little_endian)*val = get_unaligned_le16(buf);else*val = get_unaligned_be16(buf);Unrelated to this patch, isn't buf aligned to a 4 bytes boundary ?
Very probably, as it's right after len that's an unsigned int. Adding __aligned(8) would ensure we don't need any of the unaligned variants, and most likely would keep the stack layout as-is.
You mean something like this?
u8 __aligned(8) buf[8]; [...] if (little_endian) *val = le64_to_cpup(buf); else *val = be64_to_cpup(buf);
But what about 24 Bits? There is no le24_to_cpup. I would rather use the same API for all cases.
The aligned APIs are much better choice when you can use them. The 24 bit case can remain special IMO.
Or... how about putting it in an union with a u64? That would mean it's accessible as u64 alignment-wise while the alignment itself is up to the ABI. A comment would be good to have probably.
An additional union seems a bit too much here. Unless something suitable already exists for general usage.
I think it's nicer than using __aligned() as you get ABI alignment that way, not something you force manually --- that's a bit crude.
I wonder that others think.
I'm fine with adding the __aligned(8) and switching the non 24 bit cases to helpers which assume alignment. The most important note I have is that that is a separate improvement from this series though.
So this should be done in a follow-up patch which is not Cc: stable .
Regards,
Hans
On Thu, Nov 02, 2023 at 10:27:45AM +0100, Hans de Goede wrote:
Hi,
On 11/2/23 09:25, Sakari Ailus wrote:
Hi Alexander,
On Thu, Nov 02, 2023 at 08:51:12AM +0100, Alexander Stein wrote:
Hi,
thanks for the feedback.
Am Donnerstag, 2. November 2023, 07:30:44 CET schrieb Sakari Ailus:
Hi Laurent,
On Thu, Nov 02, 2023 at 03:22:17AM +0200, Laurent Pinchart wrote:
Hi Alexander,
Thank you for the patch.
On Wed, Nov 01, 2023 at 01:23:53PM +0100, Alexander Stein wrote:
Some sensors, e.g. Sony, are using little-endian registers. Add support for
I would write Sony IMX290 here, as there are Sony sensors that use big endian.
Almost all of them. There are a few exceptions indeed. This seems to be a bug.
Let's name IMX290 here as an actual example. No need to worry about other models here.
those by encoding the endianess into Bit 20 of the register address.
Fixes: af73323b97702 ("media: imx290: Convert to new CCI register access helpers") Cc: stable@vger.kernel.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------ include/media/v4l2-cci.h | 5 ++++ 2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-cci.c b/drivers/media/v4l2-core/v4l2-cci.c index bc2dbec019b04..673637b67bf67 100644 --- a/drivers/media/v4l2-core/v4l2-cci.c +++ b/drivers/media/v4l2-core/v4l2-cci.c @@ -18,6 +18,7 @@
int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) {
bool little_endian;
unsigned int len; u8 buf[8]; int ret;
@@ -25,6 +26,7 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)> > if (err && *err) return *err;
- little_endian = reg & CCI_REG_LE;
You could initialize the variable when declaring it. Same below.
I was thinking of the same, but then it'd be logical to move initialisation of all related variables there. reg is modified here though. I'd keep setting little_endian here. If someone wants to move it, that could be done in a separate patch.
len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); reg = FIELD_GET(CCI_REG_ADDR_MASK, reg);
@@ -40,16 +42,28 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)> > *val = buf[0]; break; case 2:
*val = get_unaligned_be16(buf);
if (little_endian)*val = get_unaligned_le16(buf);else*val = get_unaligned_be16(buf);Unrelated to this patch, isn't buf aligned to a 4 bytes boundary ?
Very probably, as it's right after len that's an unsigned int. Adding __aligned(8) would ensure we don't need any of the unaligned variants, and most likely would keep the stack layout as-is.
You mean something like this?
u8 __aligned(8) buf[8]; [...] if (little_endian) *val = le64_to_cpup(buf); else *val = be64_to_cpup(buf);
But what about 24 Bits? There is no le24_to_cpup. I would rather use the same API for all cases.
The aligned APIs are much better choice when you can use them. The 24 bit case can remain special IMO.
Or... how about putting it in an union with a u64? That would mean it's accessible as u64 alignment-wise while the alignment itself is up to the ABI. A comment would be good to have probably.
An additional union seems a bit too much here. Unless something suitable already exists for general usage.
I think it's nicer than using __aligned() as you get ABI alignment that way, not something you force manually --- that's a bit crude.
I wonder that others think.
I'm fine with adding the __aligned(8) and switching the non 24 bit cases to helpers which assume alignment. The most important note I have is that that is a separate improvement from this series though.
So this should be done in a follow-up patch which is not Cc: stable .
I'm fine with that.
So I think these are good as-is then.
On Thu, Nov 02, 2023 at 09:56:24AM +0000, Sakari Ailus wrote:
On Thu, Nov 02, 2023 at 10:27:45AM +0100, Hans de Goede wrote:
Hi,
On 11/2/23 09:25, Sakari Ailus wrote:
Hi Alexander,
On Thu, Nov 02, 2023 at 08:51:12AM +0100, Alexander Stein wrote:
Hi,
thanks for the feedback.
Am Donnerstag, 2. November 2023, 07:30:44 CET schrieb Sakari Ailus:
Hi Laurent,
On Thu, Nov 02, 2023 at 03:22:17AM +0200, Laurent Pinchart wrote:
Hi Alexander,
Thank you for the patch.
On Wed, Nov 01, 2023 at 01:23:53PM +0100, Alexander Stein wrote: > Some sensors, e.g. Sony, are using little-endian registers. Add support > for
I would write Sony IMX290 here, as there are Sony sensors that use big endian.
Almost all of them. There are a few exceptions indeed. This seems to be a bug.
Let's name IMX290 here as an actual example. No need to worry about other models here.
> those by encoding the endianess into Bit 20 of the register address. > > Fixes: af73323b97702 ("media: imx290: Convert to new CCI register access > helpers") Cc: stable@vger.kernel.org > Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com > --- > > drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------ > include/media/v4l2-cci.h | 5 ++++ > 2 files changed, 41 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-cci.c > b/drivers/media/v4l2-core/v4l2-cci.c index bc2dbec019b04..673637b67bf67 > 100644 > --- a/drivers/media/v4l2-core/v4l2-cci.c > +++ b/drivers/media/v4l2-core/v4l2-cci.c > @@ -18,6 +18,7 @@ > > int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) > { > > + bool little_endian; > > unsigned int len; > u8 buf[8]; > int ret; > > @@ -25,6 +26,7 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, > int *err)> > > if (err && *err) > > return *err; > > + little_endian = reg & CCI_REG_LE;
You could initialize the variable when declaring it. Same below.
I was thinking of the same, but then it'd be logical to move initialisation of all related variables there. reg is modified here though. I'd keep setting little_endian here. If someone wants to move it, that could be done in a separate patch.
> len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); > reg = FIELD_GET(CCI_REG_ADDR_MASK, reg); > > @@ -40,16 +42,28 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, > int *err)> > > *val = buf[0]; > break; > > case 2: > - *val = get_unaligned_be16(buf); > + if (little_endian) > + *val = get_unaligned_le16(buf); > + else > + *val = get_unaligned_be16(buf);
Unrelated to this patch, isn't buf aligned to a 4 bytes boundary ?
Very probably, as it's right after len that's an unsigned int. Adding __aligned(8) would ensure we don't need any of the unaligned variants, and most likely would keep the stack layout as-is.
You mean something like this?
u8 __aligned(8) buf[8]; [...] if (little_endian) *val = le64_to_cpup(buf); else *val = be64_to_cpup(buf);
But what about 24 Bits? There is no le24_to_cpup. I would rather use the same API for all cases.
The aligned APIs are much better choice when you can use them. The 24 bit case can remain special IMO.
Or... how about putting it in an union with a u64? That would mean it's accessible as u64 alignment-wise while the alignment itself is up to the ABI. A comment would be good to have probably.
An additional union seems a bit too much here. Unless something suitable already exists for general usage.
I think it's nicer than using __aligned() as you get ABI alignment that way, not something you force manually --- that's a bit crude.
I wonder that others think.
I'm fine with adding the __aligned(8) and switching the non 24 bit cases to helpers which assume alignment. The most important note I have is that that is a separate improvement from this series though.
So this should be done in a follow-up patch which is not Cc: stable .
I'm fine with that.
So I think these are good as-is then.
Or rather with the non-functional changes made in v3.
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);elseput_unaligned_be16(val, buf);break;
case 3:
put_unaligned_be24(val, buf);
if (little_endian)put_unaligned_le24(val, buf);elseput_unaligned_be24(val, buf);break;
case 4:
put_unaligned_be32(val, buf);
if (little_endian)put_unaligned_le32(val, buf);elseput_unaligned_be32(val, buf);break;
case 8:
put_unaligned_be64(val, buf);
if (little_endian)put_unaligned_le64(val, buf);elseput_unaligned_be64(val, buf);break;
default: dev_err(regmap_get_device(map), "Error invalid reg-width
%u for reg
0x%04x\n",>diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h index 0f6803e4b17e9..ef3faf0c9d44d 100644 --- a/include/media/v4l2-cci.h +++ b/include/media/v4l2-cci.h @@ -32,12 +32,17 @@ struct cci_reg_sequence {
#define CCI_REG_ADDR_MASK GENMASK(15, 0) #define CCI_REG_WIDTH_SHIFT 16 #define CCI_REG_WIDTH_MASK GENMASK(19, 16)
+#define CCI_REG_LE BIT(20)
#define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT)
| (x))
#define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT)
| (x))
#define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT)
| (x))
#define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT)
| (x))
#define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT)
| (x))
+#define CCI_REG16_LE(x) ((2 << CCI_REG_WIDTH_SHIFT)
| (x) | CCI_REG_LE)
+#define CCI_REG24_LE(x) ((3 << CCI_REG_WIDTH_SHIFT)
| (x) | CCI_REG_LE)
+#define CCI_REG32_LE(x) ((4 << CCI_REG_WIDTH_SHIFT)
| (x) | CCI_REG_LE)
+#define CCI_REG64_LE(x) ((8 << CCI_REG_WIDTH_SHIFT)
| (x) | CCI_REG_LE)
I would put CCI_REG_LE first, to match the bits order.
You mean this order?
CCI_REG8(x) CCI_REG16_LE(x) CCI_REG16(x) CCI_REG24_LE(x) CCI_REG24(x) CCI_REG32_LE(x) CCI_REG32(x) CCI_REG64_LE(x) CCI_REG64(x)
I would either keep the _LE variants at the bottom or below to their big- endian counterpart. I prefer readability thus I would put the _LE at the bottom, also it aligns nicely with the additional bit set.
Best regards, Alexander
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
/**
- cci_read() - Read a value from a single CCI register
Hi Alexander,
On Thu, Nov 02, 2023 at 08:55:01AM +0100, Alexander Stein wrote:
Am Donnerstag, 2. November 2023, 02:22:17 CET schrieb Laurent Pinchart:
On Wed, Nov 01, 2023 at 01:23:53PM +0100, Alexander Stein wrote:
Some sensors, e.g. Sony, are using little-endian registers. Add support for
I would write Sony IMX290 here, as there are Sony sensors that use big endian.
those by encoding the endianess into Bit 20 of the register address.
Fixes: af73323b97702 ("media: imx290: Convert to new CCI register access helpers") Cc: stable@vger.kernel.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------ include/media/v4l2-cci.h | 5 ++++ 2 files changed, 41 insertions(+), 8 deletions(-)
[snip]
diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h index 0f6803e4b17e9..ef3faf0c9d44d 100644 --- a/include/media/v4l2-cci.h +++ b/include/media/v4l2-cci.h @@ -32,12 +32,17 @@ struct cci_reg_sequence { #define CCI_REG_ADDR_MASK GENMASK(15, 0) #define CCI_REG_WIDTH_SHIFT 16 #define CCI_REG_WIDTH_MASK GENMASK(19, 16) +#define CCI_REG_LE BIT(20)
#define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x)) +#define CCI_REG16_LE(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG24_LE(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG32_LE(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG64_LE(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE)
I would put CCI_REG_LE first, to match the bits order.
You mean this order?
CCI_REG8(x) CCI_REG16_LE(x) CCI_REG16(x) CCI_REG24_LE(x) CCI_REG24(x) CCI_REG32_LE(x) CCI_REG32(x) CCI_REG64_LE(x) CCI_REG64(x)
I would either keep the _LE variants at the bottom or below to their big- endian counterpart. I prefer readability thus I would put the _LE at the bottom, also it aligns nicely with the additional bit set.
No, I meant
#define CCI_REG16_LE(x) (CCI_REG_LE | (2 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG24_LE(x) (CCI_REG_LE | (3 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG32_LE(x) (CCI_REG_LE | (4 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG64_LE(x) (CCI_REG_LE | (8 << CCI_REG_WIDTH_SHIFT) | (x))
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
/**
- cci_read() - Read a value from a single CCI register
Hi Laurent, Alexander,
On Thu, Nov 02, 2023 at 10:24:30AM +0200, Laurent Pinchart wrote:
Hi Alexander,
On Thu, Nov 02, 2023 at 08:55:01AM +0100, Alexander Stein wrote:
Am Donnerstag, 2. November 2023, 02:22:17 CET schrieb Laurent Pinchart:
On Wed, Nov 01, 2023 at 01:23:53PM +0100, Alexander Stein wrote:
Some sensors, e.g. Sony, are using little-endian registers. Add support for
I would write Sony IMX290 here, as there are Sony sensors that use big endian.
those by encoding the endianess into Bit 20 of the register address.
Fixes: af73323b97702 ("media: imx290: Convert to new CCI register access helpers") Cc: stable@vger.kernel.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/media/v4l2-core/v4l2-cci.c | 44 ++++++++++++++++++++++++------ include/media/v4l2-cci.h | 5 ++++ 2 files changed, 41 insertions(+), 8 deletions(-)
[snip]
diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h index 0f6803e4b17e9..ef3faf0c9d44d 100644 --- a/include/media/v4l2-cci.h +++ b/include/media/v4l2-cci.h @@ -32,12 +32,17 @@ struct cci_reg_sequence { #define CCI_REG_ADDR_MASK GENMASK(15, 0) #define CCI_REG_WIDTH_SHIFT 16 #define CCI_REG_WIDTH_MASK GENMASK(19, 16) +#define CCI_REG_LE BIT(20)
#define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x)) +#define CCI_REG16_LE(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG24_LE(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG32_LE(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE) +#define CCI_REG64_LE(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x) | CCI_REG_LE)
I would put CCI_REG_LE first, to match the bits order.
You mean this order?
CCI_REG8(x) CCI_REG16_LE(x) CCI_REG16(x) CCI_REG24_LE(x) CCI_REG24(x) CCI_REG32_LE(x) CCI_REG32(x) CCI_REG64_LE(x) CCI_REG64(x)
I would either keep the _LE variants at the bottom or below to their big- endian counterpart. I prefer readability thus I would put the _LE at the bottom, also it aligns nicely with the additional bit set.
No, I meant
#define CCI_REG16_LE(x) (CCI_REG_LE | (2 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG24_LE(x) (CCI_REG_LE | (3 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG32_LE(x) (CCI_REG_LE | (4 << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG64_LE(x) (CCI_REG_LE | (8 << CCI_REG_WIDTH_SHIFT) | (x))
Ideally these numbers would be unsigned, i.e.
#define CCI_REG16_LE(x) (CCI_REG_LE | (2U << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG24_LE(x) (CCI_REG_LE | (3U << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG32_LE(x) (CCI_REG_LE | (4U << CCI_REG_WIDTH_SHIFT) | (x)) #define CCI_REG64_LE(x) (CCI_REG_LE | (8U << CCI_REG_WIDTH_SHIFT) | (x))
This is a pre-existing problem. Feel free to add a patch to address it. :-)
On Thu, Nov 02, 2023 at 08:31:44AM +0000, Sakari Ailus wrote:
This is a pre-existing problem. Feel free to add a patch to address it. :-)
I forgot to add that addressing may be part of the same set but come as last, to avoid unnecessarily backporting patches. There's no bug in the code related to this --- just a bug-prone pattern.
linux-stable-mirror@lists.linaro.org
-
Alexander Stein -
Hans de Goede -
Laurent Pinchart -
Sakari Ailus