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

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

Hi Hans,

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

> -
> -	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;
> -}
> -

[snip]

-- 
Regards,

Laurent Pinchart