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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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/