Re: [PATCH 19/21] media: i2c: it6625: fold subdev initialization into probe

[Sakari Ailus]

Hi Hermes,

Overall the patches are nice, thank you. One comment below...

On Fri, Sep 18, 2026 at 04:57:34PM +0800, Hermes Wu via B4 Relay wrote:
> From: Hermes Wu <Hermes.wu@xxxxxxxxxx>
> 
> it6625_init_v4l2_subdev() split subdev/control-handler/media-entity
> initialization out of probe(), which made error handling harder to
> follow across the two functions and forced probe() to normalize every
> init_v4l2_subdev() failure to -ENOMEM regardless of the real error
> (e.g. a media_entity_pads_init() failure was reported to the caller as
> -ENOMEM instead of its actual code).
> 
> Inline it into it6625_probe() so initialization and its unwind path
> are visible together, and propagate the real error from
> it6625_v4l2_init_controls() instead of replacing it. Cleanup behavior
> on each failure path is unchanged: it6625_v4l2_init_controls() already
> frees the control handler internally before returning an error, and a
> media_entity_pads_init() failure still frees it explicitly before
> unwinding, so neither path double-frees it through the later
> err_clean_hdl label.
> 
> State finalization (v4l2_subdev_init_finalize()/cleanup()) is
> deliberately left for a follow-up change, to keep this a pure
> restructuring and keep the locking-model transition atomic on its own.
> 
> Signed-off-by: Hermes Wu <Hermes.wu@xxxxxxxxxx>
> ---
>  drivers/media/i2c/it6625.c | 46 +++++++++++++++++-----------------------------
>  1 file changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
> index 40cd413e0ed49c118421ce3dec77bb2df08cf042..ec6aaa878471ff210264a35e4aa66dfb1e63ed3b 100644
> --- a/drivers/media/i2c/it6625.c
> +++ b/drivers/media/i2c/it6625.c
> @@ -2142,33 +2142,6 @@ static int it6625_parse_dt(struct it6625 *it6625)
>  	return it6625_parse_endpoint(it6625);
>  }
>  
> -static int it6625_init_v4l2_subdev(struct it6625 *it6625)
> -{
> -	struct v4l2_subdev *sd = &it6625->sd;
> -	int err;
> -
> -	sd->dev = it6625->dev;
> -
> -	v4l2_i2c_subdev_init(sd, it6625->i2c_client, &it6625_ops);
> -	sd->internal_ops = &it6625_internal_ops;
> -	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> -	if (it6625_v4l2_init_controls(sd)) {
> -		dev_err(it6625->dev, "Failed to initialize v4l2 controls");
> -		return -ENOMEM;
> -	}
> -
> -	it6625->pad.flags = MEDIA_PAD_FL_SOURCE;
> -	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> -	err = media_entity_pads_init(&sd->entity, 1, &it6625->pad);
> -	if (err < 0) {
> -		dev_err(it6625->dev, "%s %d err=%d", __func__, __LINE__, err);
> -		v4l2_ctrl_handler_free(sd->ctrl_handler);
> -		return err;
> -	}
> -
> -	return 0;
> -}
> -
>  static int it6625_check_device(struct it6625 *it6625)
>  {
>  	static const u8 chip_ids[][2] = {
> @@ -2254,9 +2227,24 @@ static int it6625_probe(struct i2c_client *client)
>  	}
>  
>  	sd = &it6625->sd;
> -	err = it6625_init_v4l2_subdev(it6625);
> -	if (err)
> +	v4l2_i2c_subdev_init(sd, it6625->i2c_client, &it6625_ops);
> +	sd->internal_ops = &it6625_internal_ops;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> +
> +	err = it6625_v4l2_init_controls(sd);
> +	if (err) {
> +		dev_err(it6625->dev, "failed to initialize v4l2 controls: %d", err);
>  		goto err_clean_work_queues;
> +	}
> +
> +	it6625->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	err = media_entity_pads_init(&sd->entity, 1, &it6625->pad);
> +	if (err < 0) {
> +		dev_err(it6625->dev, "%s %d err=%d", __func__, __LINE__, err);
> +		v4l2_ctrl_handler_free(sd->ctrl_handler);

Please add a new label instead of freeing the control handler here.

> +		goto err_clean_work_queues;
> +	}
>  
>  	err = v4l2_ctrl_handler_setup(sd->ctrl_handler);
>  	if (err)
> 

-- 
Kind regards,

Sakari Ailus