Re: [PATCH v4 06/16] media: sony-starvis2: Allow runtime control of link frequency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] media: starvis2: silent failure and incorrect error logging in control init
--

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

media: sony-starvis2: Allow runtime control of link frequency

This commit links V4L2_CID_LINK_FREQ to runtime controls, allowing the
link frequency to be configured while the sensor isn't streaming. It updates
starvis2_init_controls to handle the new link_freq allocations and refactors
the error handling path.

> diff --git a/drivers/media/i2c/sony-starvis2.c b/drivers/media/i2c/sony-starvis2.c
> --- a/drivers/media/i2c/sony-starvis2.c
> +++ b/drivers/media/i2c/sony-starvis2.c
> @@ -1224,25 +1250,26 @@ static int starvis2_init_controls(struct starvis2 *starvis2)
[ ... ]
> -	/* LINK_FREQ is also read only */
> -	link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &starvis2_ctrl_ops,
> -					   V4L2_CID_LINK_FREQ,
> -					   ARRAY_SIZE(link_freqs) - 1,
> -					   __ffs(starvis2->link_freq_bitmap),
> -					   link_freqs);
> -
> -	if (link_freq)
> -		link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	starvis2->link_freq =
> +		v4l2_ctrl_new_int_menu(ctrl_hdlr, &starvis2_ctrl_ops,
> +				       V4L2_CID_LINK_FREQ,
> +				       __fls(starvis2->link_freq_bitmap),
> +				       __ffs(starvis2->link_freq_bitmap),
> +				       link_freqs);
> +	if (!starvis2->link_freq)
> +		goto err_free_handler;

[Severity: Low]
Does this bypass the error logging block? If v4l2_ctrl_new_int_menu() fails
to allocate the link_freq control, jumping directly to err_free_handler
results in a silent failure instead of printing a control init failure message.

> +
> +	v4l2_ctrl_modify_range(starvis2->link_freq,
> +			       __ffs(starvis2->link_freq_bitmap),
> +			       __fls(starvis2->link_freq_bitmap),
> +			       ~(starvis2->link_freq_bitmap),
> +			       __ffs(starvis2->link_freq_bitmap));
[ ... ]
> @@ -1285,16 +1316,20 @@ static int starvis2_init_controls(struct starvis2 *starvis2)
>  	v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &starvis2_ctrl_ops, &props);
>  
>  	if (ctrl_hdlr->error) {
> -		ret = ctrl_hdlr->error;
>  		dev_err(&client->dev, "%s control init failed (%d)\n",
>  			__func__, ret);

[Severity: Low]
Does this print the unupdated ret variable (which is 0) rather than the
actual error code? The assignment ret = ctrl_hdlr->error was removed from
before the dev_err call and moved to the err_free_handler label at the end
of the function, leaving ret as 0 during this print.

> -		v4l2_ctrl_handler_free(ctrl_hdlr);
> -		return ret;
> +		goto err_free_handler;
>  	}
>  
>  	starvis2->sd.ctrl_handler = ctrl_hdlr;
>  
>  	return 0;
> +
> +err_free_handler:
> +	ret = ctrl_hdlr->error;
> +	v4l2_ctrl_handler_free(ctrl_hdlr);
> +
> +	return ret;
>  }

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