linux-kernel - Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <33e3e806-935f-4867-9a94-fa914d17a3ab@oss.qualcomm.com>
Date: Mon, 29 Dec 2025 14:26:29 +0100
From: johannes.goede@....qualcomm.com
To: Xiaolei Wang <xiaolei.wang@...driver.com>,
        laurent.pinchart@...asonboard.com, sakari.ailus@...ux.intel.com,
        dave.stevenson@...pberrypi.com, jacopo@...ndi.org, mchehab@...nel.org,
        prabhakar.mahadev-lad.rj@...renesas.com, hverkuil+cisco@...nel.org,
        hverkuil-cisco@...all.nl, jai.luthra@...asonboard.com
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access
 helpers

Hi,

On 29-Dec-25 03:30, Xiaolei Wang wrote:
> Use the new common CCI register access helpers to replace the private
> register access helpers in the ov5647 driver. This simplifies the driver
> by reducing the amount of code.

Thank you for your patch, it is great to see more drivers
being converted to the CCI register access helpers.

> Signed-off-by: Xiaolei Wang <xiaolei.wang@...driver.com>
> ---

...

> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index e193fef4fced..fd69f1616794 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c

...

> @@ -130,377 +122,377 @@ static const u8 ov5647_test_pattern_val[] = {
>  	0x81,	/* Random Data */
>  };
>  
> -static const struct regval_list sensor_oe_disable_regs[] = {
> -	{0x3000, 0x00},
> -	{0x3001, 0x00},
> -	{0x3002, 0x00},
> +static const struct cci_reg_sequence sensor_oe_disable_regs[] = {
> +	{ CCI_REG8(0x3000), 0x00 },
> +	{ CCI_REG8(0x3001), 0x00 },
> +	{ CCI_REG8(0x3002), 0x00 },
>  };
>  
> -static const struct regval_list sensor_oe_enable_regs[] = {
> -	{0x3000, 0x0f},
> -	{0x3001, 0xff},
> -	{0x3002, 0xe4},
> +static const struct cci_reg_sequence sensor_oe_enable_regs[] = {
> +	{ CCI_REG8(0x3000), 0x0f },
> +	{ CCI_REG8(0x3001), 0xff },
> +	{ CCI_REG8(0x3002), 0xe4 },
>  };

For these 2, but also for the 2 much longer arrays with
address, value pairs which you are replacing, you are
replacing all the register addresses with CCI_REG8(0x3000).

Even though some of these are like 16 bit or even 24bit
registers, this in itself is not a problem.

But if you are replacing them 1:1 like this anyway then
IMHO it is better to just:

- Directly use struct reg_sequence instead of struct cci_reg_sequence
- Call regmap_multi_reg_write() instead of cci_multi_reg_write()

then you can keep all the initializer values for
the arrays the same (no need to add CCI_REG8() around
the addresses). This is also how this was done in other
drivers when they were converted to the CCI helpers.

This greatly reduces the size of the diff and makes it
much easier to review the patch.

Regards,

Hans