Re: [PATCH v4 01/16] media: imx678: Rework into generic Starvis 2 sensor driver.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] media: dead code in starvis2_enable_streams() due to unconditional power-on writes
- [Low] media: starvis2_write_common() unconditionally sets success flag on I2C failure
--

commit 6500c5ca0127e3e5085414806a5f7d7e712bdaa6
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: imx678: Rework into generic Starvis 2 sensor driver.

The register layout and programming model for multiple of the
Sony Starvis2 sensors is identical, so prepare for supporting
those by renaming all common parts of the driver to a starvis2_
prefix, and renaming the driver.

> diff --git a/drivers/media/i2c/imx678.c b/drivers/media/i2c/sony-starvis2.c
> similarity index 56%
> rename from drivers/media/i2c/imx678.c
> rename to drivers/media/i2c/sony-starvis2.c
> --- a/drivers/media/i2c/imx678.c
> +++ b/drivers/media/i2c/sony-starvis2.c

[ ... ]

> @@ -894,50 +903,55 @@ static int imx678_init_state(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -static int imx678_write_common(struct imx678 *imx678)
> +static int starvis2_write_common(struct starvis2 *starvis2)
>  {
>  	int ret = 0;
>  
> -	cci_multi_reg_write(imx678->cci, common_regs, ARRAY_SIZE(common_regs),
> +	cci_multi_reg_write(starvis2->cci, common_regs, ARRAY_SIZE(common_regs),
>  			    &ret);
>  
[ ... ]
> +	cci_write(starvis2->cci, STARVIS2_REG_INTERFACE_SEL,
> +		  STARVIS2_INTERFACE_2L_4L, &ret);
> +
> +	starvis2->common_regs_written = true;
> +

[Severity: Low]
Does starvis2_write_common() set the success flag even if the I2C writes fail?

cci_multi_reg_write() and cci_write() accumulate errors into ret. However,
this flag is unconditionally set to true before returning, which would falsely
indicate that the registers were successfully written even on an error.

>  	return ret;
>  }
>  

[ ... ]

> @@ -945,25 +959,34 @@ static int imx678_enable_streams(struct v4l2_subdev *sd,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (!starvis2->common_regs_written) {

[Severity: Low]
Is this block unreachable dead code?

starvis2_enable_streams() begins by calling pm_runtime_resume_and_get(),
which triggers the runtime PM resume callback starvis2_power_on(). 
starvis2_power_on() unconditionally calls starvis2_write_common(),
which sets the starvis2->common_regs_written flag to true.

Therefore, when execution reaches this check, the flag will always be true,
making the body of this statement impossible to execute. Was the intention
to remove the register writes from starvis2_power_on() and defer them to
stream start?

> +		ret = starvis2_write_common(starvis2);
> +		if (ret) {
> +			dev_err(&client->dev, "%s failed to write common registers\n",
> +				__func__);
> +			goto err_rpm_put;
> +		}
> +	}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-media-starvis2-v4-0-75fcdb4163f3@raspberrypi.com?part=1