Re: [PATCH v2 13/20] media: imx355: Convert to new CCI register access helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Unintended permutation of initialization values for registers 0x0305, 0x0306, and 0x0307 in `imx355_global_regs`.
- [Medium] `IMX355_REG_TEST_PATTERN` is incorrectly defined as an 8-bit register, causing truncated writes and breaking test pattern configuration.
- [Low] The error code from `devm_cci_regmap_init_i2c()` is incorrectly discarded and replaced with a hardcoded `-ENODEV`.

Pre-existing issues:
- [High] Incorrect check of `pm_runtime_get_if_in_use()` in `imx355_set_ctrl` allows unpowered hardware access and PM counter underflow when runtime PM is disabled.
--

--- Patch [13]: [PATCH v2 13/20] media: imx355: Convert to new CCI register access helpers ---
commit 85e1b3f8ecdbe34d874840dcfedd44c3422e9cf3
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: imx355: Convert to new CCI register access helpers

This patch updates the imx355 driver to use the common v4l2 CCI register
access helpers. It replaces the private i2c transfer logic with cci_read
and cci_write calls, and updates the register definition tables.

> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index c7a24c69c0df8..c710dda5795b6 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
[ ... ]
>  /* Digital gain control */
> -#define IMX355_REG_DPGA_USE_GLOBAL_GAIN	0x3070
> -#define IMX355_REG_DIG_GAIN_GLOBAL	0x020e
> +#define IMX355_REG_DPGA_USE_GLOBAL_GAIN	CCI_REG8(0x3070)
> +#define IMX355_REG_DIG_GAIN_GLOBAL	CCI_REG16(0x020e)
>  #define IMX355_DGTL_GAIN_MIN		256
>  #define IMX355_DGTL_GAIN_MAX		4095
>  #define IMX355_DGTL_GAIN_STEP		1
>  #define IMX355_DGTL_GAIN_DEFAULT	256
>  
>  /* Test Pattern Control */
> -#define IMX355_REG_TEST_PATTERN		0x0600
> +#define IMX355_REG_TEST_PATTERN		CCI_REG8(0x0600)

[Severity: Medium]
Does this change the width of the test pattern write?

The original code used imx355_write_reg() with a length of 2 for
IMX355_REG_TEST_PATTERN, performing a 16-bit write. With this macro
defined as an 8-bit register, cci_write() will truncate the payload into
a 1-byte write, dropping the lower byte that actually contains the
control value. Should this be defined as CCI_REG16(0x0600)?

[ ... ]
> -static const struct imx355_reg imx355_global_regs[] = {
[ ... ]
> -	{ 0x0301, 0x05 },
> -	{ 0x0303, 0x01 },
> -	{ 0x0305, 0x02 },
> -	{ 0x0306, 0x00 },
> -	{ 0x0307, 0x78 },
> -	{ 0x030d, 0x02 },
> -	{ 0x0310, 0x00 },
[ ... ]
> +static const struct cci_reg_sequence imx355_global_regs[] = {
[ ... ]
> +	{ CCI_REG8(0x0301), 0x05 },
> +	{ CCI_REG8(0x0303), 0x01 },
> +	{ CCI_REG8(0x0305), 0x00 },
> +	{ CCI_REG8(0x0306), 0x78 },
> +	{ CCI_REG8(0x0307), 0x02 },
> +	{ CCI_REG8(0x030d), 0x02 },
> +	{ CCI_REG8(0x0310), 0x00 },

[Severity: High]
Were the values for these three registers accidentally swapped during the
conversion?

The original sequence was:
{ 0x0305, 0x02 }
{ 0x0306, 0x00 }
{ 0x0307, 0x78 }

The new sequence assigns 0x00 to 0x0305, 0x78 to 0x0306, and 0x02 to 0x0307.
Could this incorrect initialization sequence break the sensor's hardware
timing configuration?

[ ... ]
> @@ -726,31 +647,31 @@ static int imx355_set_ctrl(struct v4l2_ctrl *ctrl)

[Severity: High]
This isn't a bug introduced by this patch, but does the runtime PM check in
this function handle negative error codes correctly?

In imx355_set_ctrl(), the code checks:

        if (!pm_runtime_get_if_in_use(imx355->dev))
                return 0;

pm_runtime_get_if_in_use() returns -EINVAL when runtime PM is disabled.
Since -EINVAL is non-zero, the check evaluates to false, and the code
falls through to hardware accesses while the device might be unpowered.
Could this also lead to an underflow when pm_runtime_put() is called at
the end of the function?

[ ... ]
> @@ -1344,6 +1228,12 @@ static int imx355_probe(struct i2c_client *client)
>  
>  	mutex_init(&imx355->mutex);
>  
> +	imx355->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(imx355->regmap)) {
> +		dev_err(imx355->dev, "Unable to initialize I2C\n");
> +		return -ENODEV;
> +	}
> +
>  	imx355->clk = devm_v4l2_sensor_clk_get(imx355->dev, NULL);
>  	if (IS_ERR(imx355->clk))

[Severity: Low]
Should this propagate the actual error code instead of returning -ENODEV?

When devm_cci_regmap_init_i2c() fails (e.g., due to memory allocation
failure returning -ENOMEM), returning a hardcoded -ENODEV masks the true
cause of the failure. Would it be better to return PTR_ERR(imx355->regmap)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-media-imx355-v2-0-1683ec07b897@raspberrypi.com?part=13