Re: [PATCH 6/8] media: ov2740: add manual white balance controls

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Aug 26, 2026 at 03:22:54PM +0200, Maurizio Casciano wrote:
> The sensor has separate red, green and blue manual white-balance gain
> registers, but the driver currently writes the same digital-gain value
> to all three channels. This prevents userspace from correcting the
> strong color cast of raw Bayer capture.
> 
> Expose red- and blue-balance controls relative to the digital gain,
> update all three channels under group hold, and always release and
> launch the group even when a channel write fails.
> 
> Tested on the Yoga Book OV2740 with live gain changes and continuous raw
> capture.

...

> -static int ov2740_update_digital_gain(struct ov2740 *ov2740, u32 d_gain)
> +static int ov2740_update_mwb_gains(struct ov2740 *ov2740)
>  {
> -	int ret;
> +	u32 green_gain = ov2740->digital_gain->val;
> +	u32 red_gain, blue_gain;

Split assignment and combine all three on a single line.
Also, why not u64 from the start?

> +	int end_ret, launch_ret, ret;
> +
> +	/* Balance controls use 1024 as unity relative to the digital gain. */
> +	red_gain = min_t(u64,

No min_t() in the new code. This macro is very exceptional.

> +			 DIV_ROUND_CLOSEST_ULL((u64)green_gain *
> +					       ov2740->red_balance->val,
> +					       OV2740_DGTL_GAIN_DEFAULT),

Is _ULL variant really required? What are the ranges of the _gain and ->val?

> +			 OV2740_DGTL_GAIN_MAX);
> +	blue_gain = min_t(u64,
> +			  DIV_ROUND_CLOSEST_ULL((u64)green_gain *
> +						ov2740->blue_balance->val,
> +						OV2740_DGTL_GAIN_DEFAULT),
> +			  OV2740_DGTL_GAIN_MAX);
>  
>  	ret = ov2740_write_reg(ov2740, OV2740_REG_GROUP_ACCESS, 1,
>  			       OV2740_GROUP_HOLD_START);
>  	if (ret)
>  		return ret;
>  
> -	ret = ov2740_write_reg(ov2740, OV2740_REG_MWB_R_GAIN, 2, d_gain);
> +	ret = ov2740_write_reg(ov2740, OV2740_REG_MWB_R_GAIN, 2, red_gain);
>  	if (ret)
> -		return ret;
> +		goto release_group;
>  
> -	ret = ov2740_write_reg(ov2740, OV2740_REG_MWB_G_GAIN, 2, d_gain);
> +	ret = ov2740_write_reg(ov2740, OV2740_REG_MWB_G_GAIN, 2, green_gain);
>  	if (ret)
> -		return ret;
> +		goto release_group;
>  
> -	ret = ov2740_write_reg(ov2740, OV2740_REG_MWB_B_GAIN, 2, d_gain);
> -	if (ret)
> -		return ret;
> +	ret = ov2740_write_reg(ov2740, OV2740_REG_MWB_B_GAIN, 2, blue_gain);
>  
> -	ret = ov2740_write_reg(ov2740, OV2740_REG_GROUP_ACCESS, 1,
> -			       OV2740_GROUP_HOLD_END);
> -	if (ret)
> -		return ret;
> +release_group:
> +	end_ret = ov2740_write_reg(ov2740, OV2740_REG_GROUP_ACCESS, 1,
> +				   OV2740_GROUP_HOLD_END);
> +	launch_ret = ov2740_write_reg(ov2740, OV2740_REG_GROUP_ACCESS, 1,
> +				      OV2740_GROUP_HOLD_LAUNCH);
>  
> -	ret = ov2740_write_reg(ov2740, OV2740_REG_GROUP_ACCESS, 1,
> -			       OV2740_GROUP_HOLD_LAUNCH);
> -	return ret;
> +	return ret ?: end_ret ?: launch_ret;

To make this looking better it might be good to convert the driver to use CCI
accessors [1]. Maybe Hans knows more about this as he worked a lot on this drivers
and sensors in the past.

>  }

[1]: include/media/v4l2-cci.h

-- 
With Best Regards,
Andy Shevchenko