Re: [PATCH v10 02/15] media: ov8858: support 19.2 MHz clock and manual white balance

[Sakari Ailus]

Hi Maurizio,

On Fri, Oct 02, 2026 at 10:57:40AM +0200, Maurizio Casciano via B4 Relay wrote:
> From: Maurizio Casciano <mauriziocasciano7@xxxxxxxxx>
> 
> Support both 19.2 MHz and 24 MHz input clocks. Move the PLL and
> global-timing registers out of the common mode tables and select register
> settings for the detected revision, lane count, input clock and 360 MHz
> link frequency when starting the stream. Use the actual input-clock rate
> when calculating the reset delay.
> 
> Add standard red and blue balance controls for the sensor's manual white
> balance registers. V4L2 defines red and blue balance relative to green but
> does not define a standard green-balance control, so keep green at unity.
> Accumulate errors while programming all three channels.
> 
> The manual white-balance register definitions and programming follow
> Intel's GPL-2.0 OV5670 driver, so retain its 2017 Intel copyright notice.

Any others notices that should be added?

> 
> Signed-off-by: Maurizio Casciano <mauriziocasciano7@xxxxxxxxx>
> Assisted-by: LLM sparse

The Assisted-by: tags are no longer used, please remove them.

> ---
>  drivers/media/i2c/ov8858.c | 183 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 147 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> index 2a0e2b08b9aa..64532237e8b5 100644
> --- a/drivers/media/i2c/ov8858.c
> +++ b/drivers/media/i2c/ov8858.c

...

> @@ -1541,6 +1606,23 @@ static int ov8858_set_long_digital_gain(struct ov8858 *ov8858, u32 gain)
>  	return ov8858_write(ov8858, OV8858_REG_LONG_DIGIGAIN, long_gain, NULL);
>  }
>  
> +static int ov8858_set_mwb_gains(struct ov8858 *ov8858)
> +{
> +	int ret = 0;
> +
> +	ov8858_write(ov8858, OV8858_REG_MWB_RED_GAIN,
> +		     ov8858->red_balance->val, &ret);
> +	/*
> +	 * Green is the unity reference for the red and blue balance controls.
> +	 */

UAPI-wise this doesn't look great. There's a digital gain control, too, so
you can still control the entire digital gain. But the hardware has
per-component gains and in this case the (likely analogue TV inspired?)
UAPI limits this. A few other sensor drivers appear to be doing the same.

Cc Laurent, too.

> +	ov8858_write(ov8858, OV8858_REG_MWB_GREEN_GAIN,
> +		     OV8858_MWB_GAIN_DEFAULT, &ret);
> +	ov8858_write(ov8858, OV8858_REG_MWB_BLUE_GAIN,
> +		     ov8858->blue_balance->val, &ret);
> +
> +	return ret;
> +}
> +
>  static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct ov8858 *ov8858 = container_of(ctrl->handler,
> @@ -1589,6 +1671,10 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
>  	case V4L2_CID_DIGITAL_GAIN:
>  		ret = ov8858_set_long_digital_gain(ov8858, ctrl->val);
>  		break;
> +	case V4L2_CID_RED_BALANCE:
> +	case V4L2_CID_BLUE_BALANCE:
> +		ret = ov8858_set_mwb_gains(ov8858);
> +		break;
>  	case V4L2_CID_VBLANK:
>  		ret = ov8858_write(ov8858, OV8858_REG_VTS,
>  				   ctrl->val + format->height, NULL);
> @@ -1623,9 +1709,6 @@ static int ov8858_power_on(struct ov8858 *ov8858)
>  	unsigned long delay_us;
>  	int ret;
>  
> -	if (clk_get_rate(ov8858->xvclk) != OV8858_XVCLK_FREQ)
> -		dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n");
> -
>  	ret = clk_prepare_enable(ov8858->xvclk);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to enable xvclk\n");
> @@ -1644,7 +1727,7 @@ static int ov8858_power_on(struct ov8858 *ov8858)
>  	 * transaction, but a double sleep between the release of gpios
>  	 * helps with sporadic failures observed at probe time.
>  	 */
> -	delay_us = DIV_ROUND_UP(8192, OV8858_XVCLK_FREQ / 1000 / 1000);
> +	delay_us = DIV_ROUND_UP(8192, ov8858->xvclk_rate / HZ_PER_MHZ);
>  
>  	gpiod_set_value_cansleep(ov8858->reset_gpio, 0);
>  	fsleep(delay_us);
> @@ -1710,7 +1793,7 @@ static int ov8858_init_ctrls(struct ov8858 *ov8858)
>  	u32 h_blank;
>  	int ret;
>  
> -	ret = v4l2_ctrl_handler_init(handler, 10);
> +	ret = v4l2_ctrl_handler_init(handler, 12);
>  	if (ret)
>  		return ret;
>  
> @@ -1752,6 +1835,19 @@ static int ov8858_init_ctrls(struct ov8858 *ov8858)
>  			  OV8858_LONG_DIGIGAIN_STEP,
>  			  OV8858_LONG_DIGIGAIN_DEFAULT);
>  
> +	ov8858->red_balance =
> +		v4l2_ctrl_new_std(handler, &ov8858_ctrl_ops,
> +				  V4L2_CID_RED_BALANCE,
> +				  OV8858_MWB_GAIN_MIN, OV8858_MWB_GAIN_MAX,
> +				  OV8858_MWB_GAIN_STEP,
> +				  OV8858_MWB_GAIN_DEFAULT);
> +	ov8858->blue_balance =
> +		v4l2_ctrl_new_std(handler, &ov8858_ctrl_ops,
> +				  V4L2_CID_BLUE_BALANCE,
> +				  OV8858_MWB_GAIN_MIN, OV8858_MWB_GAIN_MAX,
> +				  OV8858_MWB_GAIN_STEP,
> +				  OV8858_MWB_GAIN_DEFAULT);
> +
>  	v4l2_ctrl_new_std_menu_items(handler, &ov8858_ctrl_ops,
>  				     V4L2_CID_TEST_PATTERN,
>  				     ARRAY_SIZE(ov8858_test_pattern_menu) - 1,
> @@ -1805,15 +1901,20 @@ static int ov8858_check_sensor_id(struct ov8858 *ov8858)
>  
>  	if (id == OV8858_R2A) {
>  		/* R2A supports 2 and 4 lanes modes. */
> -		ov8858->global_regs = ov8858->num_lanes == 4
> -				    ? ov8858_global_regs_r2a_4lane
> -				    : ov8858_global_regs_r2a_2lane;
> +		if (ov8858->num_lanes == 4) {
> +			ov8858->global_regs = ov8858_global_regs_r2a_4lane;
> +			ov8858->xvclk_regs = ov8858_24mhz_360mhz_r2a_4lane;
> +		} else {
> +			ov8858->global_regs = ov8858_global_regs_r2a_2lane;
> +			ov8858->xvclk_regs = ov8858_24mhz_360mhz_r2a_2lane;
> +		}
>  	} else if (ov8858->num_lanes == 2) {
>  		/*
>  		 * R1A only supports 2 lanes mode and it's only partially
>  		 * supported.
>  		 */
>  		ov8858->global_regs = ov8858_global_regs_r1a;
> +		ov8858->xvclk_regs = ov8858_24mhz_360mhz_r1a_2lane;
>  		dev_warn(&client->dev, "R1A may not work well!\n");
>  	} else {
>  		dev_err(&client->dev,
> @@ -1821,6 +1922,9 @@ static int ov8858_check_sensor_id(struct ov8858 *ov8858)
>  		return -EINVAL;
>  	}
>  
> +	if (ov8858->xvclk_rate == OV8858_XVCLK_FREQ_19_2MHZ)
> +		ov8858->xvclk_regs = ov8858_19_2mhz_360mhz;

The 24 MHz configurations are lane number (and variant) dependent. Isn't
that applying to 19,2 MHz? If not, why?

> +
>  	return 0;
>  }
>  
> @@ -1888,6 +1992,13 @@ static int ov8858_probe(struct i2c_client *client)
>  		return dev_err_probe(dev, PTR_ERR(ov8858->xvclk),
>  				     "Failed to get xvclk\n");
>  
> +	ov8858->xvclk_rate = clk_get_rate(ov8858->xvclk);
> +	if (ov8858->xvclk_rate != OV8858_XVCLK_FREQ_19_2MHZ &&
> +	    ov8858->xvclk_rate != OV8858_XVCLK_FREQ_24MHZ)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Unsupported xvclk rate %lu Hz\n",
> +				     ov8858->xvclk_rate);
> +
>  	ov8858->reset_gpio = devm_gpiod_get_optional(dev, "reset",
>  						     GPIOD_OUT_HIGH);
>  	if (IS_ERR(ov8858->reset_gpio))
> 

-- 
Regards,

Sakari Ailus