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