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.