Re: [PATCH v4 3/5] media: imx290: Convert to new CCI register access helpers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Tue, Aug 15, 2023 at 04:15:39PM +0200, Hans de Goede wrote:
> On 8/15/23 15:15, Laurent Pinchart wrote:
> > On Tue, Jun 27, 2023 at 02:51:06PM +0200, Hans de Goede wrote:
> >> Use the new comon CCI register access helpers to replace the private
> >> register access helpers in the imx290 driver.
> >>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> Note:
> >> 1. This is untested
> >> 2. For reviewers: all the IMX290_REG_?BIT defines in both the register
> >> address defines as well as in various reg-sequences were automatically
> >> changed using search replace.
> >> ---
> >> Changes in v3:
> >> - Fixed a couple of lines > 80 chars
> >>
> >> Changes in v2:
> >> - New patch in v2 of this series
> >> ---
> >>  drivers/media/i2c/Kconfig  |   1 +
> >>  drivers/media/i2c/imx290.c | 360 +++++++++++++++----------------------
> >>  2 files changed, 150 insertions(+), 211 deletions(-)
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> >> index b3f832e9d7e1..e78c7b91ae72 100644
> >> --- a/drivers/media/i2c/imx290.c
> >> +++ b/drivers/media/i2c/imx290.c
> >> @@ -21,91 +21,86 @@
> > 
> > [snip]
> > 
> >> @@ -615,63 +605,15 @@ imx290_format_info(const struct imx290 *imx290, u32 code)
> >>  	return NULL;
> >>  }
> >>  
> >> -/* -----------------------------------------------------------------------------
> >> - * Register access
> >> - */
> >> -
> >> -static int __always_unused imx290_read(struct imx290 *imx290, u32 addr, u32 *value)
> >> -{
> >> -	u8 data[3] = { 0, 0, 0 };
> >> -	int ret;
> >> -
> >> -	ret = regmap_raw_read(imx290->regmap, addr & IMX290_REG_ADDR_MASK,
> >> -			      data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
> >> -	if (ret < 0) {
> >> -		dev_err(imx290->dev, "%u-bit read from 0x%04x failed: %d\n",
> >> -			((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
> >> -			 addr & IMX290_REG_ADDR_MASK, ret);
> >> -		return ret;
> >> -	}
> >> -
> >> -	*value = get_unaligned_le24(data);
> >> -	return 0;
> >> -}
> >> -
> >> -static int imx290_write(struct imx290 *imx290, u32 addr, u32 value, int *err)
> >> -{
> >> -	u8 data[3];
> >> -	int ret;
> >> -
> >> -	if (err && *err)
> >> -		return *err;
> >> -
> >> -	put_unaligned_le24(value, data);
> > 
> > We seem to be having a problem here, as the CCI helpers unconditionally
> > use big endian for the data :-(
> 
> That is because that is what the specification says, from the MIPI CSI spec:
> 
> """
> 6.3.2 The Transmission Byte Order for Multi-byte Register Values
> 
> This is a normative section.
> 
> The first byte of a CCI message is always the MS byte of a multi-byte
> register and the last byte is always the LS byte.
> """
> 
> So it seems that the IMX sensors are special here and it might be best
> to just revert the conversion to the CCI helpers?

I'm fine with reverting for v6.6, as we're close to the release of v6.5.

> Alternative would be to make devm_cci_regmap_init_i2c() return a newly
> allocated struct which contains both a struct regmap * and a long
> flags and make the helpers take a pointer to that struct, combined
> with adding an endianess flag to the flags member.

I think I like the idea, it will probably help adding support for
various device-specific quirks in the future.

> >> -
> >> -	ret = regmap_raw_write(imx290->regmap, addr & IMX290_REG_ADDR_MASK,
> >> -			       data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
> >> -	if (ret < 0) {
> >> -		dev_err(imx290->dev, "%u-bit write to 0x%04x failed: %d\n",
> >> -			((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
> >> -			 addr & IMX290_REG_ADDR_MASK, ret);
> >> -		if (err)
> >> -			*err = ret;
> >> -	}
> >> -
> >> -	return ret;
> >> -}
> >> -

-- 
Regards,

Laurent Pinchart