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

[Hermes.Wu]

Hi Sakari

Thanks for review.

>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..5d267676606e340ad0ee5ad2b78e
>> 5acb50a32980 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?

will fix in v2
>> -	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?
>
will fix in v2

>dev_err_probe() might be better here, too.
>
>>  
>>  	err = v4l2_async_register_subdev(sd);
>>  	if (err < 0) {
>> 
>
>--
>Regards,
>
>Sakari Ailus
>