Re: [PATCH v5 07/15] media: i2c: os05b10: remove unused control fields, simplify error handling

[Sakari Ailus]

Hi Tarang,

On Fri, Oct 30, 2026 at 04:17:56PM +0530, Tarang Raval wrote:
> link_freq and gain don't need to be stored in struct os05b10; make
> them local.
> 
> Parse the fwnode properties up front and register them before the
> single ctrl_hdlr->error check, so all controls are covered by one
> check and the per-control NULL guards before setting flags can be
> dropped.
> 
> Signed-off-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@xxxxxxxxxx>
> ---
>  drivers/media/i2c/os05b10.c | 46 +++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 46d514d2a479..abcb7c766754 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -457,10 +457,8 @@ struct os05b10 {
>  
>  	/* V4L2 Controls */
>  	struct v4l2_ctrl_handler handler;
> -	struct v4l2_ctrl *link_freq;
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *vblank;
> -	struct v4l2_ctrl *gain;
>  	struct v4l2_ctrl *exposure;
>  
>  	u32 link_freq_index;
> @@ -920,8 +918,13 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>  	u64 hblank_def, vblank_def, exp_max, pixel_rate;
>  	struct v4l2_fwnode_device_properties props;
>  	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	struct v4l2_ctrl *link_freq;
>  	int ret;
>  
> +	ret = v4l2_fwnode_device_parse(os05b10->dev, &props);
> +	if (ret)
> +	        return ret;
> +
>  	ctrl_hdlr = &os05b10->handler;
>  	v4l2_ctrl_handler_init(ctrl_hdlr, 8);
>  
> @@ -929,21 +932,16 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>  	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_PIXEL_RATE,
>  			  pixel_rate, pixel_rate, 1, pixel_rate);
>  
> -	os05b10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os05b10_ctrl_ops,
> -						    V4L2_CID_LINK_FREQ,
> -						    ARRAY_SIZE(link_frequencies) - 1,
> -						    os05b10->link_freq_index,
> -						    link_frequencies);
> -
> -	if (os05b10->link_freq)
> -		os05b10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os05b10_ctrl_ops,
> +					   V4L2_CID_LINK_FREQ,
> +					   ARRAY_SIZE(link_frequencies) - 1,
> +					   os05b10->link_freq_index,
> +					   link_frequencies);
>  
>  	hblank_def = mode->hts - mode->width;
>  	os05b10->hblank = v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_HBLANK,
>  					    hblank_def, hblank_def,
>  					    1, hblank_def);
> -	if (os05b10->hblank)
> -		os05b10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
>  	vblank_def = mode->vts - mode->height;
>  	os05b10->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> @@ -958,27 +956,19 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>  					      exp_max, OS05B10_EXPOSURE_STEP,
>  					      mode->exp);
>  
> -	os05b10->gain = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> -					  V4L2_CID_ANALOGUE_GAIN,
> -					  OS05B10_ANALOG_GAIN_MIN,
> -					  OS05B10_ANALOG_GAIN_MAX,
> -					  OS05B10_ANALOG_GAIN_STEP,
> -					  OS05B10_ANALOG_GAIN_DEFAULT);
> +	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> +			  OS05B10_ANALOG_GAIN_MIN, OS05B10_ANALOG_GAIN_MAX,
> +			  OS05B10_ANALOG_GAIN_STEP, OS05B10_ANALOG_GAIN_DEFAULT);
>  
> -	if (ctrl_hdlr->error) {
> -		ret = ctrl_hdlr->error;
> +	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &os05b10_ctrl_ops,
> +					      &props);
> +	if (ret) {
>  		dev_err(os05b10->dev, "control init failed (%d)\n", ret);
>  		goto error;

If you're left with a single goto (hopefully that's the case), you can just
handle the error here (and avoid the goto).

>  	}
>  
> -	ret = v4l2_fwnode_device_parse(os05b10->dev, &props);
> -	if (ret)
> -		goto error;
> -
> -	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &os05b10_ctrl_ops,
> -					      &props);
> -	if (ret)
> -		goto error;
> +	link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	os05b10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
>  	os05b10->sd.ctrl_handler = ctrl_hdlr;
>  

-- 
Regards,

Sakari Ailus