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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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