diff mbox series

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

Message ID 20230627125109.52354-4-hdegoede@redhat.com
State Accepted
Commit af73323b97702e53b0a336972aaf23e7dd92c850
Headers show
Series media: Add MIPI CCI register access helper functions | expand

Commit Message

Hans de Goede June 27, 2023, 12:51 p.m. UTC 
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(-)

Comments

Laurent Pinchart Aug. 15, 2023, 1:15 p.m. UTC | #1 
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]
Sakari Ailus Aug. 15, 2023, 1:31 p.m. UTC | #2 
Hi Laurent,

On Tue, Aug 15, 2023 at 04:15:39PM +0300, Laurent Pinchart wrote:
> 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 :-(

Well spotted. This driver needs to address this.

It's a hardware issue though. It's the only sensor ever I've seen to have
little endian registers.

> 
> > -
> > -	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]
>
Laurent Pinchart Aug. 15, 2023, 1:43 p.m. UTC | #3 
On Tue, Aug 15, 2023 at 01:31:55PM +0000, Sakari Ailus wrote:
> On Tue, Aug 15, 2023 at 04:15:39PM +0300, 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 :-(
> 
> Well spotted. This driver needs to address this.
> 
> It's a hardware issue though. It's the only sensor ever I've seen to have
> little endian registers.

I'm not sure I would call that a hardware issue. It may be a peculiarity
of this sensor, but is it really the only one ?

How would you like to see this addressed ? We could add CCI_REG*_LE
macros, but I'm not sure to like that. Setting a flag at init time would
be nicer, but there's nowhere to store it.

> > > -
> > > -	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]
Alexander Stein Aug. 15, 2023, 1:48 p.m. UTC | #4 
Am Dienstag, 15. August 2023, 15:31:55 CEST schrieb Sakari Ailus:
> ********************
> 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 Laurent,
> 
> On Tue, Aug 15, 2023 at 04:15:39PM +0300, Laurent Pinchart wrote:
> > 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 :-(
> 
> Well spotted. This driver needs to address this.
> 
> It's a hardware issue though. It's the only sensor ever I've seen to have
> little endian registers.

It's getting even worse: There are cameras using this sensor when bulk read is 
broken (VC MIPI IMX327 C, Laurent knows about this peculiar hardware), so 
regmap_config.use_single_read has to be set to true. This is currently not 
possible anymore insode this driver.

Best regards,
Alexander

> > > -
> > > -	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]
Hans de Goede Aug. 15, 2023, 2:15 p.m. UTC | #5 
Hi,

On 8/15/23 15:15, Laurent Pinchart wrote:
> 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 :-(

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?

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.

Regards,

Hans



> 
>> -
>> -	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]
>
Laurent Pinchart Aug. 15, 2023, 2:21 p.m. UTC | #6 
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;
> >> -}
> >> -
diff mbox series

Patch

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 8467f4ce92eb..6365c15bc4d4 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -165,6 +165,7 @@  config VIDEO_IMX290
 	select VIDEO_V4L2_SUBDEV_API
 	select REGMAP_I2C
 	select V4L2_FWNODE
+	select V4L2_CCI_I2C
 	help
 	  This is a Video4Linux2 sensor driver for the Sony
 	  IMX290 camera sensor.
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 @@ 
 #include <asm/unaligned.h>
 
 #include <media/media-entity.h>
+#include <media/v4l2-cci.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
-#define IMX290_REG_SIZE_SHIFT				16
-#define IMX290_REG_ADDR_MASK				0xffff
-#define IMX290_REG_8BIT(n)				((1U << IMX290_REG_SIZE_SHIFT) | (n))
-#define IMX290_REG_16BIT(n)				((2U << IMX290_REG_SIZE_SHIFT) | (n))
-#define IMX290_REG_24BIT(n)				((3U << IMX290_REG_SIZE_SHIFT) | (n))
-
-#define IMX290_STANDBY					IMX290_REG_8BIT(0x3000)
-#define IMX290_REGHOLD					IMX290_REG_8BIT(0x3001)
-#define IMX290_XMSTA					IMX290_REG_8BIT(0x3002)
-#define IMX290_ADBIT					IMX290_REG_8BIT(0x3005)
+#define IMX290_STANDBY					CCI_REG8(0x3000)
+#define IMX290_REGHOLD					CCI_REG8(0x3001)
+#define IMX290_XMSTA					CCI_REG8(0x3002)
+#define IMX290_ADBIT					CCI_REG8(0x3005)
 #define IMX290_ADBIT_10BIT				(0 << 0)
 #define IMX290_ADBIT_12BIT				(1 << 0)
-#define IMX290_CTRL_07					IMX290_REG_8BIT(0x3007)
+#define IMX290_CTRL_07					CCI_REG8(0x3007)
 #define IMX290_VREVERSE					BIT(0)
 #define IMX290_HREVERSE					BIT(1)
 #define IMX290_WINMODE_1080P				(0 << 4)
 #define IMX290_WINMODE_720P				(1 << 4)
 #define IMX290_WINMODE_CROP				(4 << 4)
-#define IMX290_FR_FDG_SEL				IMX290_REG_8BIT(0x3009)
-#define IMX290_BLKLEVEL					IMX290_REG_16BIT(0x300a)
-#define IMX290_GAIN					IMX290_REG_8BIT(0x3014)
-#define IMX290_VMAX					IMX290_REG_24BIT(0x3018)
+#define IMX290_FR_FDG_SEL				CCI_REG8(0x3009)
+#define IMX290_BLKLEVEL					CCI_REG16(0x300a)
+#define IMX290_GAIN					CCI_REG8(0x3014)
+#define IMX290_VMAX					CCI_REG24(0x3018)
 #define IMX290_VMAX_MAX					0x3ffff
-#define IMX290_HMAX					IMX290_REG_16BIT(0x301c)
+#define IMX290_HMAX					CCI_REG16(0x301c)
 #define IMX290_HMAX_MAX					0xffff
-#define IMX290_SHS1					IMX290_REG_24BIT(0x3020)
-#define IMX290_WINWV_OB					IMX290_REG_8BIT(0x303a)
-#define IMX290_WINPV					IMX290_REG_16BIT(0x303c)
-#define IMX290_WINWV					IMX290_REG_16BIT(0x303e)
-#define IMX290_WINPH					IMX290_REG_16BIT(0x3040)
-#define IMX290_WINWH					IMX290_REG_16BIT(0x3042)
-#define IMX290_OUT_CTRL					IMX290_REG_8BIT(0x3046)
+#define IMX290_SHS1					CCI_REG24(0x3020)
+#define IMX290_WINWV_OB					CCI_REG8(0x303a)
+#define IMX290_WINPV					CCI_REG16(0x303c)
+#define IMX290_WINWV					CCI_REG16(0x303e)
+#define IMX290_WINPH					CCI_REG16(0x3040)
+#define IMX290_WINWH					CCI_REG16(0x3042)
+#define IMX290_OUT_CTRL					CCI_REG8(0x3046)
 #define IMX290_ODBIT_10BIT				(0 << 0)
 #define IMX290_ODBIT_12BIT				(1 << 0)
 #define IMX290_OPORTSEL_PARALLEL			(0x0 << 4)
 #define IMX290_OPORTSEL_LVDS_2CH			(0xd << 4)
 #define IMX290_OPORTSEL_LVDS_4CH			(0xe << 4)
 #define IMX290_OPORTSEL_LVDS_8CH			(0xf << 4)
-#define IMX290_XSOUTSEL					IMX290_REG_8BIT(0x304b)
+#define IMX290_XSOUTSEL					CCI_REG8(0x304b)
 #define IMX290_XSOUTSEL_XVSOUTSEL_HIGH			(0 << 0)
 #define IMX290_XSOUTSEL_XVSOUTSEL_VSYNC			(2 << 0)
 #define IMX290_XSOUTSEL_XHSOUTSEL_HIGH			(0 << 2)
 #define IMX290_XSOUTSEL_XHSOUTSEL_HSYNC			(2 << 2)
-#define IMX290_INCKSEL1					IMX290_REG_8BIT(0x305c)
-#define IMX290_INCKSEL2					IMX290_REG_8BIT(0x305d)
-#define IMX290_INCKSEL3					IMX290_REG_8BIT(0x305e)
-#define IMX290_INCKSEL4					IMX290_REG_8BIT(0x305f)
-#define IMX290_PGCTRL					IMX290_REG_8BIT(0x308c)
-#define IMX290_ADBIT1					IMX290_REG_8BIT(0x3129)
+#define IMX290_INCKSEL1					CCI_REG8(0x305c)
+#define IMX290_INCKSEL2					CCI_REG8(0x305d)
+#define IMX290_INCKSEL3					CCI_REG8(0x305e)
+#define IMX290_INCKSEL4					CCI_REG8(0x305f)
+#define IMX290_PGCTRL					CCI_REG8(0x308c)
+#define IMX290_ADBIT1					CCI_REG8(0x3129)
 #define IMX290_ADBIT1_10BIT				0x1d
 #define IMX290_ADBIT1_12BIT				0x00
-#define IMX290_INCKSEL5					IMX290_REG_8BIT(0x315e)
-#define IMX290_INCKSEL6					IMX290_REG_8BIT(0x3164)
-#define IMX290_ADBIT2					IMX290_REG_8BIT(0x317c)
+#define IMX290_INCKSEL5					CCI_REG8(0x315e)
+#define IMX290_INCKSEL6					CCI_REG8(0x3164)
+#define IMX290_ADBIT2					CCI_REG8(0x317c)
 #define IMX290_ADBIT2_10BIT				0x12
 #define IMX290_ADBIT2_12BIT				0x00
-#define IMX290_CHIP_ID					IMX290_REG_16BIT(0x319a)
-#define IMX290_ADBIT3					IMX290_REG_8BIT(0x31ec)
+#define IMX290_CHIP_ID					CCI_REG16(0x319a)
+#define IMX290_ADBIT3					CCI_REG8(0x31ec)
 #define IMX290_ADBIT3_10BIT				0x37
 #define IMX290_ADBIT3_12BIT				0x0e
-#define IMX290_REPETITION				IMX290_REG_8BIT(0x3405)
-#define IMX290_PHY_LANE_NUM				IMX290_REG_8BIT(0x3407)
-#define IMX290_OPB_SIZE_V				IMX290_REG_8BIT(0x3414)
-#define IMX290_Y_OUT_SIZE				IMX290_REG_16BIT(0x3418)
-#define IMX290_CSI_DT_FMT				IMX290_REG_16BIT(0x3441)
+#define IMX290_REPETITION				CCI_REG8(0x3405)
+#define IMX290_PHY_LANE_NUM				CCI_REG8(0x3407)
+#define IMX290_OPB_SIZE_V				CCI_REG8(0x3414)
+#define IMX290_Y_OUT_SIZE				CCI_REG16(0x3418)
+#define IMX290_CSI_DT_FMT				CCI_REG16(0x3441)
 #define IMX290_CSI_DT_FMT_RAW10				0x0a0a
 #define IMX290_CSI_DT_FMT_RAW12				0x0c0c
-#define IMX290_CSI_LANE_MODE				IMX290_REG_8BIT(0x3443)
-#define IMX290_EXTCK_FREQ				IMX290_REG_16BIT(0x3444)
-#define IMX290_TCLKPOST					IMX290_REG_16BIT(0x3446)
-#define IMX290_THSZERO					IMX290_REG_16BIT(0x3448)
-#define IMX290_THSPREPARE				IMX290_REG_16BIT(0x344a)
-#define IMX290_TCLKTRAIL				IMX290_REG_16BIT(0x344c)
-#define IMX290_THSTRAIL					IMX290_REG_16BIT(0x344e)
-#define IMX290_TCLKZERO					IMX290_REG_16BIT(0x3450)
-#define IMX290_TCLKPREPARE				IMX290_REG_16BIT(0x3452)
-#define IMX290_TLPX					IMX290_REG_16BIT(0x3454)
-#define IMX290_X_OUT_SIZE				IMX290_REG_16BIT(0x3472)
-#define IMX290_INCKSEL7					IMX290_REG_8BIT(0x3480)
+#define IMX290_CSI_LANE_MODE				CCI_REG8(0x3443)
+#define IMX290_EXTCK_FREQ				CCI_REG16(0x3444)
+#define IMX290_TCLKPOST					CCI_REG16(0x3446)
+#define IMX290_THSZERO					CCI_REG16(0x3448)
+#define IMX290_THSPREPARE				CCI_REG16(0x344a)
+#define IMX290_TCLKTRAIL				CCI_REG16(0x344c)
+#define IMX290_THSTRAIL					CCI_REG16(0x344e)
+#define IMX290_TCLKZERO					CCI_REG16(0x3450)
+#define IMX290_TCLKPREPARE				CCI_REG16(0x3452)
+#define IMX290_TLPX					CCI_REG16(0x3454)
+#define IMX290_X_OUT_SIZE				CCI_REG16(0x3472)
+#define IMX290_INCKSEL7					CCI_REG8(0x3480)
 
 #define IMX290_PGCTRL_REGEN				BIT(0)
 #define IMX290_PGCTRL_THRU				BIT(1)
@@ -181,7 +176,7 @@  enum imx290_model {
 
 struct imx290_model_info {
 	enum imx290_colour_variant colour_variant;
-	const struct imx290_regval *init_regs;
+	const struct cci_reg_sequence *init_regs;
 	size_t init_regs_num;
 	const char *name;
 };
@@ -192,11 +187,6 @@  enum imx290_clk_freq {
 	IMX290_NUM_CLK
 };
 
-struct imx290_regval {
-	u32 reg;
-	u32 val;
-};
-
 /*
  * Clock configuration for registers INCKSEL1 to INCKSEL6.
  */
@@ -217,7 +207,7 @@  struct imx290_mode {
 	u8 link_freq_index;
 	u8 ctrl_07;
 
-	const struct imx290_regval *data;
+	const struct cci_reg_sequence *data;
 	u32 data_size;
 
 	const struct imx290_clk_cfg *clk_cfg;
@@ -271,7 +261,7 @@  static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
  * Modes and formats
  */
 
-static const struct imx290_regval imx290_global_init_settings[] = {
+static const struct cci_reg_sequence imx290_global_init_settings[] = {
 	{ IMX290_WINWV_OB, 12 },
 	{ IMX290_WINPH, 0 },
 	{ IMX290_WINPV, 0 },
@@ -279,56 +269,56 @@  static const struct imx290_regval imx290_global_init_settings[] = {
 	{ IMX290_WINWV, 1097 },
 	{ IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC |
 			   IMX290_XSOUTSEL_XHSOUTSEL_HSYNC },
-	{ IMX290_REG_8BIT(0x3011), 0x02 },
-	{ IMX290_REG_8BIT(0x3012), 0x64 },
-	{ IMX290_REG_8BIT(0x3013), 0x00 },
+	{ CCI_REG8(0x3011), 0x02 },
+	{ CCI_REG8(0x3012), 0x64 },
+	{ CCI_REG8(0x3013), 0x00 },
 };
 
-static const struct imx290_regval imx290_global_init_settings_290[] = {
-	{ IMX290_REG_8BIT(0x300f), 0x00 },
-	{ IMX290_REG_8BIT(0x3010), 0x21 },
-	{ IMX290_REG_8BIT(0x3016), 0x09 },
-	{ IMX290_REG_8BIT(0x3070), 0x02 },
-	{ IMX290_REG_8BIT(0x3071), 0x11 },
-	{ IMX290_REG_8BIT(0x309b), 0x10 },
-	{ IMX290_REG_8BIT(0x309c), 0x22 },
-	{ IMX290_REG_8BIT(0x30a2), 0x02 },
-	{ IMX290_REG_8BIT(0x30a6), 0x20 },
-	{ IMX290_REG_8BIT(0x30a8), 0x20 },
-	{ IMX290_REG_8BIT(0x30aa), 0x20 },
-	{ IMX290_REG_8BIT(0x30ac), 0x20 },
-	{ IMX290_REG_8BIT(0x30b0), 0x43 },
-	{ IMX290_REG_8BIT(0x3119), 0x9e },
-	{ IMX290_REG_8BIT(0x311c), 0x1e },
-	{ IMX290_REG_8BIT(0x311e), 0x08 },
-	{ IMX290_REG_8BIT(0x3128), 0x05 },
-	{ IMX290_REG_8BIT(0x313d), 0x83 },
-	{ IMX290_REG_8BIT(0x3150), 0x03 },
-	{ IMX290_REG_8BIT(0x317e), 0x00 },
-	{ IMX290_REG_8BIT(0x32b8), 0x50 },
-	{ IMX290_REG_8BIT(0x32b9), 0x10 },
-	{ IMX290_REG_8BIT(0x32ba), 0x00 },
-	{ IMX290_REG_8BIT(0x32bb), 0x04 },
-	{ IMX290_REG_8BIT(0x32c8), 0x50 },
-	{ IMX290_REG_8BIT(0x32c9), 0x10 },
-	{ IMX290_REG_8BIT(0x32ca), 0x00 },
-	{ IMX290_REG_8BIT(0x32cb), 0x04 },
-	{ IMX290_REG_8BIT(0x332c), 0xd3 },
-	{ IMX290_REG_8BIT(0x332d), 0x10 },
-	{ IMX290_REG_8BIT(0x332e), 0x0d },
-	{ IMX290_REG_8BIT(0x3358), 0x06 },
-	{ IMX290_REG_8BIT(0x3359), 0xe1 },
-	{ IMX290_REG_8BIT(0x335a), 0x11 },
-	{ IMX290_REG_8BIT(0x3360), 0x1e },
-	{ IMX290_REG_8BIT(0x3361), 0x61 },
-	{ IMX290_REG_8BIT(0x3362), 0x10 },
-	{ IMX290_REG_8BIT(0x33b0), 0x50 },
-	{ IMX290_REG_8BIT(0x33b2), 0x1a },
-	{ IMX290_REG_8BIT(0x33b3), 0x04 },
+static const struct cci_reg_sequence imx290_global_init_settings_290[] = {
+	{ CCI_REG8(0x300f), 0x00 },
+	{ CCI_REG8(0x3010), 0x21 },
+	{ CCI_REG8(0x3016), 0x09 },
+	{ CCI_REG8(0x3070), 0x02 },
+	{ CCI_REG8(0x3071), 0x11 },
+	{ CCI_REG8(0x309b), 0x10 },
+	{ CCI_REG8(0x309c), 0x22 },
+	{ CCI_REG8(0x30a2), 0x02 },
+	{ CCI_REG8(0x30a6), 0x20 },
+	{ CCI_REG8(0x30a8), 0x20 },
+	{ CCI_REG8(0x30aa), 0x20 },
+	{ CCI_REG8(0x30ac), 0x20 },
+	{ CCI_REG8(0x30b0), 0x43 },
+	{ CCI_REG8(0x3119), 0x9e },
+	{ CCI_REG8(0x311c), 0x1e },
+	{ CCI_REG8(0x311e), 0x08 },
+	{ CCI_REG8(0x3128), 0x05 },
+	{ CCI_REG8(0x313d), 0x83 },
+	{ CCI_REG8(0x3150), 0x03 },
+	{ CCI_REG8(0x317e), 0x00 },
+	{ CCI_REG8(0x32b8), 0x50 },
+	{ CCI_REG8(0x32b9), 0x10 },
+	{ CCI_REG8(0x32ba), 0x00 },
+	{ CCI_REG8(0x32bb), 0x04 },
+	{ CCI_REG8(0x32c8), 0x50 },
+	{ CCI_REG8(0x32c9), 0x10 },
+	{ CCI_REG8(0x32ca), 0x00 },
+	{ CCI_REG8(0x32cb), 0x04 },
+	{ CCI_REG8(0x332c), 0xd3 },
+	{ CCI_REG8(0x332d), 0x10 },
+	{ CCI_REG8(0x332e), 0x0d },
+	{ CCI_REG8(0x3358), 0x06 },
+	{ CCI_REG8(0x3359), 0xe1 },
+	{ CCI_REG8(0x335a), 0x11 },
+	{ CCI_REG8(0x3360), 0x1e },
+	{ CCI_REG8(0x3361), 0x61 },
+	{ CCI_REG8(0x3362), 0x10 },
+	{ CCI_REG8(0x33b0), 0x50 },
+	{ CCI_REG8(0x33b2), 0x1a },
+	{ CCI_REG8(0x33b3), 0x04 },
 };
 
 #define IMX290_NUM_CLK_REGS	2
-static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
+static const struct cci_reg_sequence xclk_regs[][IMX290_NUM_CLK_REGS] = {
 	[IMX290_CLK_37_125] = {
 		{ IMX290_EXTCK_FREQ, (37125 * 256) / 1000 },
 		{ IMX290_INCKSEL7, 0x49 },
@@ -339,13 +329,13 @@  static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
 	},
 };
 
-static const struct imx290_regval imx290_global_init_settings_327[] = {
-	{ IMX290_REG_8BIT(0x309e), 0x4A },
-	{ IMX290_REG_8BIT(0x309f), 0x4A },
-	{ IMX290_REG_8BIT(0x313b), 0x61 },
+static const struct cci_reg_sequence imx290_global_init_settings_327[] = {
+	{ CCI_REG8(0x309e), 0x4A },
+	{ CCI_REG8(0x309f), 0x4A },
+	{ CCI_REG8(0x313b), 0x61 },
 };
 
-static const struct imx290_regval imx290_1080p_settings[] = {
+static const struct cci_reg_sequence imx290_1080p_settings[] = {
 	/* mode settings */
 	{ IMX290_WINWV_OB, 12 },
 	{ IMX290_OPB_SIZE_V, 10 },
@@ -353,7 +343,7 @@  static const struct imx290_regval imx290_1080p_settings[] = {
 	{ IMX290_Y_OUT_SIZE, 1080 },
 };
 
-static const struct imx290_regval imx290_720p_settings[] = {
+static const struct cci_reg_sequence imx290_720p_settings[] = {
 	/* mode settings */
 	{ IMX290_WINWV_OB, 6 },
 	{ IMX290_OPB_SIZE_V, 4 },
@@ -361,7 +351,7 @@  static const struct imx290_regval imx290_720p_settings[] = {
 	{ IMX290_Y_OUT_SIZE, 720 },
 };
 
-static const struct imx290_regval imx290_10bit_settings[] = {
+static const struct cci_reg_sequence imx290_10bit_settings[] = {
 	{ IMX290_ADBIT, IMX290_ADBIT_10BIT },
 	{ IMX290_OUT_CTRL, IMX290_ODBIT_10BIT },
 	{ IMX290_ADBIT1, IMX290_ADBIT1_10BIT },
@@ -370,7 +360,7 @@  static const struct imx290_regval imx290_10bit_settings[] = {
 	{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW10 },
 };
 
-static const struct imx290_regval imx290_12bit_settings[] = {
+static const struct cci_reg_sequence imx290_12bit_settings[] = {
 	{ IMX290_ADBIT, IMX290_ADBIT_12BIT },
 	{ IMX290_OUT_CTRL, IMX290_ODBIT_12BIT },
 	{ IMX290_ADBIT1, IMX290_ADBIT1_12BIT },
@@ -576,7 +566,7 @@  static inline int imx290_modes_num(const struct imx290 *imx290)
 struct imx290_format_info {
 	u32 code[IMX290_VARIANT_MAX];
 	u8 bpp;
-	const struct imx290_regval *regs;
+	const struct cci_reg_sequence *regs;
 	unsigned int num_regs;
 };
 
@@ -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);
-
-	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;
-}
-
 static int imx290_set_register_array(struct imx290 *imx290,
-				     const struct imx290_regval *settings,
+				     const struct cci_reg_sequence *settings,
 				     unsigned int num_settings)
 {
-	unsigned int i;
 	int ret;
 
-	for (i = 0; i < num_settings; ++i, ++settings) {
-		ret = imx290_write(imx290, settings->reg, settings->val, NULL);
-		if (ret < 0)
-			return ret;
-	}
+	ret = cci_multi_reg_write(imx290->regmap, settings, num_settings, NULL);
+	if (ret < 0)
+		return ret;
 
 	/* Provide 10ms settle time */
 	usleep_range(10000, 11000);
@@ -689,12 +631,12 @@  static int imx290_set_clock(struct imx290 *imx290)
 	ret = imx290_set_register_array(imx290, xclk_regs[clk_idx],
 					IMX290_NUM_CLK_REGS);
 
-	imx290_write(imx290, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
-	imx290_write(imx290, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
-	imx290_write(imx290, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
-	imx290_write(imx290, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
-	imx290_write(imx290, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
-	imx290_write(imx290, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
+	cci_write(imx290->regmap, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
+	cci_write(imx290->regmap, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
+	cci_write(imx290->regmap, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
+	cci_write(imx290->regmap, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
+	cci_write(imx290->regmap, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
+	cci_write(imx290->regmap, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
 
 	return ret;
 }
@@ -703,9 +645,11 @@  static int imx290_set_data_lanes(struct imx290 *imx290)
 {
 	int ret = 0;
 
-	imx290_write(imx290, IMX290_PHY_LANE_NUM, imx290->nlanes - 1, &ret);
-	imx290_write(imx290, IMX290_CSI_LANE_MODE, imx290->nlanes - 1, &ret);
-	imx290_write(imx290, IMX290_FR_FDG_SEL, 0x01, &ret);
+	cci_write(imx290->regmap, IMX290_PHY_LANE_NUM, imx290->nlanes - 1,
+		  &ret);
+	cci_write(imx290->regmap, IMX290_CSI_LANE_MODE, imx290->nlanes - 1,
+		  &ret);
+	cci_write(imx290->regmap, IMX290_FR_FDG_SEL, 0x01, &ret);
 
 	return ret;
 }
@@ -716,8 +660,8 @@  static int imx290_set_black_level(struct imx290 *imx290,
 {
 	unsigned int bpp = imx290_format_info(imx290, format->code)->bpp;
 
-	return imx290_write(imx290, IMX290_BLKLEVEL,
-			    black_level >> (16 - bpp), err);
+	return cci_write(imx290->regmap, IMX290_BLKLEVEL,
+			 black_level >> (16 - bpp), err);
 }
 
 static int imx290_set_csi_config(struct imx290 *imx290)
@@ -743,15 +687,16 @@  static int imx290_set_csi_config(struct imx290 *imx290)
 		return -EINVAL;
 	}
 
-	imx290_write(imx290, IMX290_REPETITION, csi_cfg->repetition, &ret);
-	imx290_write(imx290, IMX290_TCLKPOST, csi_cfg->tclkpost, &ret);
-	imx290_write(imx290, IMX290_THSZERO, csi_cfg->thszero, &ret);
-	imx290_write(imx290, IMX290_THSPREPARE, csi_cfg->thsprepare, &ret);
-	imx290_write(imx290, IMX290_TCLKTRAIL, csi_cfg->tclktrail, &ret);
-	imx290_write(imx290, IMX290_THSTRAIL, csi_cfg->thstrail, &ret);
-	imx290_write(imx290, IMX290_TCLKZERO, csi_cfg->tclkzero, &ret);
-	imx290_write(imx290, IMX290_TCLKPREPARE, csi_cfg->tclkprepare, &ret);
-	imx290_write(imx290, IMX290_TLPX, csi_cfg->tlpx, &ret);
+	cci_write(imx290->regmap, IMX290_REPETITION, csi_cfg->repetition, &ret);
+	cci_write(imx290->regmap, IMX290_TCLKPOST, csi_cfg->tclkpost, &ret);
+	cci_write(imx290->regmap, IMX290_THSZERO, csi_cfg->thszero, &ret);
+	cci_write(imx290->regmap, IMX290_THSPREPARE, csi_cfg->thsprepare, &ret);
+	cci_write(imx290->regmap, IMX290_TCLKTRAIL, csi_cfg->tclktrail, &ret);
+	cci_write(imx290->regmap, IMX290_THSTRAIL, csi_cfg->thstrail, &ret);
+	cci_write(imx290->regmap, IMX290_TCLKZERO, csi_cfg->tclkzero, &ret);
+	cci_write(imx290->regmap, IMX290_TCLKPREPARE, csi_cfg->tclkprepare,
+		  &ret);
+	cci_write(imx290->regmap, IMX290_TLPX, csi_cfg->tlpx, &ret);
 
 	return ret;
 }
@@ -817,13 +762,12 @@  static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_ANALOGUE_GAIN:
-		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
+		ret = cci_write(imx290->regmap, IMX290_GAIN, ctrl->val, NULL);
 		break;
 
 	case V4L2_CID_VBLANK:
-		ret = imx290_write(imx290, IMX290_VMAX,
-				   ctrl->val + imx290->current_mode->height,
-				   NULL);
+		ret = cci_write(imx290->regmap, IMX290_VMAX,
+				ctrl->val + imx290->current_mode->height, NULL);
 		/*
 		 * Due to the way that exposure is programmed in this sensor in
 		 * relation to VMAX, we have to reprogramme it whenever VMAX is
@@ -835,20 +779,20 @@  static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 		fallthrough;
 	case V4L2_CID_EXPOSURE:
 		vmax = imx290->vblank->val + imx290->current_mode->height;
-		ret = imx290_write(imx290, IMX290_SHS1,
-				   vmax - ctrl->val - 1, NULL);
+		ret = cci_write(imx290->regmap, IMX290_SHS1,
+				vmax - ctrl->val - 1, NULL);
 		break;
 
 	case V4L2_CID_TEST_PATTERN:
 		if (ctrl->val) {
 			imx290_set_black_level(imx290, format, 0, &ret);
 			usleep_range(10000, 11000);
-			imx290_write(imx290, IMX290_PGCTRL,
-				     (u8)(IMX290_PGCTRL_REGEN |
-				     IMX290_PGCTRL_THRU |
-				     IMX290_PGCTRL_MODE(ctrl->val)), &ret);
+			cci_write(imx290->regmap, IMX290_PGCTRL,
+				  (u8)(IMX290_PGCTRL_REGEN |
+				       IMX290_PGCTRL_THRU |
+				       IMX290_PGCTRL_MODE(ctrl->val)), &ret);
 		} else {
-			imx290_write(imx290, IMX290_PGCTRL, 0x00, &ret);
+			cci_write(imx290->regmap, IMX290_PGCTRL, 0x00, &ret);
 			usleep_range(10000, 11000);
 			imx290_set_black_level(imx290, format,
 					       IMX290_BLACK_LEVEL_DEFAULT, &ret);
@@ -856,9 +800,8 @@  static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 
 	case V4L2_CID_HBLANK:
-		ret = imx290_write(imx290, IMX290_HMAX,
-				   ctrl->val + imx290->current_mode->width,
-				   NULL);
+		ret = cci_write(imx290->regmap, IMX290_HMAX,
+				ctrl->val + imx290->current_mode->width, NULL);
 		break;
 
 	case V4L2_CID_HFLIP:
@@ -871,7 +814,7 @@  static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 			reg |= IMX290_HREVERSE;
 		if (imx290->vflip->val)
 			reg |= IMX290_VREVERSE;
-		ret = imx290_write(imx290, IMX290_CTRL_07, reg, NULL);
+		ret = cci_write(imx290->regmap, IMX290_CTRL_07, reg, NULL);
 		break;
 	}
 
@@ -1074,12 +1017,12 @@  static int imx290_start_streaming(struct imx290 *imx290,
 		return ret;
 	}
 
-	imx290_write(imx290, IMX290_STANDBY, 0x00, &ret);
+	cci_write(imx290->regmap, IMX290_STANDBY, 0x00, &ret);
 
 	msleep(30);
 
 	/* Start streaming */
-	return imx290_write(imx290, IMX290_XMSTA, 0x00, &ret);
+	return cci_write(imx290->regmap, IMX290_XMSTA, 0x00, &ret);
 }
 
 /* Stop streaming */
@@ -1087,11 +1030,11 @@  static int imx290_stop_streaming(struct imx290 *imx290)
 {
 	int ret = 0;
 
-	imx290_write(imx290, IMX290_STANDBY, 0x01, &ret);
+	cci_write(imx290->regmap, IMX290_STANDBY, 0x01, &ret);
 
 	msleep(30);
 
-	return imx290_write(imx290, IMX290_XMSTA, 0x01, &ret);
+	return cci_write(imx290->regmap, IMX290_XMSTA, 0x01, &ret);
 }
 
 static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
@@ -1417,11 +1360,6 @@  static const struct dev_pm_ops imx290_pm_ops = {
  * Probe & remove
  */
 
-static const struct regmap_config imx290_regmap_config = {
-	.reg_bits = 16,
-	.val_bits = 8,
-};
-
 static const char * const imx290_supply_name[IMX290_NUM_SUPPLIES] = {
 	"vdda",
 	"vddd",
@@ -1588,7 +1526,7 @@  static int imx290_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	imx290->dev = dev;
-	imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config);
+	imx290->regmap = devm_cci_regmap_init_i2c(client, 16);
 	if (IS_ERR(imx290->regmap)) {
 		dev_err(dev, "Unable to initialize I2C\n");
 		return -ENODEV;