Re: [PATCH 02/21] media: i2c: it6625: propagate control-update errors

[Sakari Ailus]

Hi Hermes,

On Fri, Sep 18, 2026 at 04:57:17PM +0800, Hermes Wu via B4 Relay wrote:
> From: Hermes Wu <Hermes.wu@xxxxxxxxxx>
> 
> it6625_v4l2_sd_ctrl_update() discarded the return value of all three
> v4l2_ctrl_s_ctrl() calls it makes. Make it return int, run the updates
> sequentially, and return the first error; both call sites now check
> and log it.
> 
> None of the three controls has a driver .ops, so the only failure path
> in v4l2_ctrl_s_ctrl() is range validation, and every value this driver
> passes is always in range -- this is an error-handling correctness fix
> responsive to review, not a fix for an observed runtime failure.
> 
> Signed-off-by: Hermes Wu <Hermes.wu@xxxxxxxxxx>
> ---
>  drivers/media/i2c/it6625.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
> index da17e5d5ce9e2dd1060abba1598b0e3f78857e9c..5d267676606e340ad0ee5ad2b78e5acb50a32980 100644
> --- a/drivers/media/i2c/it6625.c
> +++ b/drivers/media/i2c/it6625.c
> @@ -889,11 +889,19 @@ static int it6625_s_ctrl_audio_present(struct v4l2_subdev *sd)
>  				audio_present(it6625));
>  }
>  
> -static void it6625_v4l2_sd_ctrl_update(struct v4l2_subdev *sd)
> +static int it6625_v4l2_sd_ctrl_update(struct v4l2_subdev *sd)
>  {
> -	it6625_s_ctrl_detect_hdmi_5v(sd);
> -	it6625_s_ctrl_audio_sampling_rate(sd);
> -	it6625_s_ctrl_audio_present(sd);
> +	int ret;
> +
> +	ret = it6625_s_ctrl_detect_hdmi_5v(sd);
> +	if (ret)
> +		return ret;
> +
> +	ret = it6625_s_ctrl_audio_sampling_rate(sd);
> +	if (ret)
> +		return ret;
> +
> +	return it6625_s_ctrl_audio_present(sd);
>  }
>  
>  static void it6625_enable_stream_locked(struct it6625 *it6625, bool enable)
> @@ -1136,9 +1144,12 @@ static void it6625_clear_timings(struct it6625 *it6625)
>  static void it6625_irq_hdmi_5v_change(struct it6625 *it6625)
>  {
>  	struct v4l2_subdev *sd = &it6625->sd;
> +	int ret;
>  
>  	it6625_clear_timings(it6625);
> -	it6625_v4l2_sd_ctrl_update(sd);
> +	ret = it6625_v4l2_sd_ctrl_update(sd);
> +	if (ret)
> +		dev_err(it6625->dev, "%s: failed to update controls: %d", __func__, ret);
>  }
>  
>  static void it6625_irq_hdcp_change(struct it6625 *it6625)
> @@ -2297,7 +2308,9 @@ static int it6625_probe(struct i2c_client *client)
>  	it6625_debugfs_init(it6625, client);
>  
>  	it6625_initial_setup(it6625);

What if this fails?

> -	it6625_v4l2_sd_ctrl_update(sd);
> +	err = it6625_v4l2_sd_ctrl_update(sd);
> +	if (err)
> +		dev_err(it6625->dev, "%s: failed to update controls: %d", __func__, err);

Shouldn't you return an error in this case?

dev_err_probe() might be better here, too.

>  
>  	err = v4l2_async_register_subdev(sd);
>  	if (err < 0) {
> 

-- 
Regards,

Sakari Ailus