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

  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(-)
  

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.

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);
> >  		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,

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

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
  

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 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 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.
  

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.