Re: [PATCH v2 02/11] media: ov8858: support 19.2 MHz clock and CHT gain setup

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

On Thu, Aug 27, 2026 at 08:17:47PM +0200, Maurizio Casciano wrote:
> The Yoga Book drives its OV8858 from a 19.2 MHz platform clock, while
> the existing mode tables program the sensor PLL for 24 MHz. Reusing
> those settings produces incorrect internal and CSI-2 clocks.
> 
> Accept both input rates and use the actual rate for the reset delay. For
> 19.2 MHz, apply the Cherry Trail MRD PLL and black-level settings after
> the generic mode table.
> 
> The 19.2 MHz platform uses the per-channel manual white-balance
> registers for digital gain. Program registers 0x5032, 0x5034 and 0x5036
> and expose their 1x-to-4x range, while retaining the existing
> long-exposure gain block for 24 MHz systems.
> 
> The manual white-balance register definitions and programming follow
> the GPL-2.0 Intel OV5670 driver, so retain its 2017 Intel copyright
> notice in this file. No proprietary source or tuning binary is included.
> 
> Tested on the Lenovo Yoga Book YB1-X91L OV8858 with full-range test bars
> and real 10-bit Bayer frames.

...

> -#define OV8858_LINK_FREQ		360000000U
> -#define OV8858_XVCLK_FREQ		24000000
> +#define OV8858_LINK_FREQ		(360 * HZ_PER_MHZ)
> +#define OV8858_XVCLK_FREQ_19_2MHZ	(192 * HZ_PER_MHZ / 10)
> +#define OV8858_XVCLK_FREQ_24MHZ		(24 * HZ_PER_MHZ)

Ah, I completely missed that 19.2 has a fractional part in MHz units.
Then this conversion makes a little sense. Sorry for misleading review
comment.

...

>  struct ov8858 {
>  	struct clk		*xvclk;
> +	unsigned long		xvclk_rate;
>  	struct gpio_desc	*reset_gpio;
>  	struct gpio_desc	*pwdn_gpio;
>  	struct regulator_bulk_data supplies[ARRAY_SIZE(ov8858_supply_names)];

>  	unsigned int		num_lanes;
>  };

Have you checked the layouts with `pahole`? Can you provide the summary?

...

> -	/*
> -	 * Digital gain is assembled as:
> -	 * 0x350a[7:0] = dgain[13:6]
> -	 * 0x350b[5:0] = dgain[5:0]
> -	 * Reassemble the control value to write it in one go.
> -	 */

Why is this comment got removed?

> -	long_gain = (gain & OV8858_LONG_DIGIGAIN_L_MASK) |
> -		    ((gain & OV8858_LONG_DIGIGAIN_H_MASK) <<
> -		     OV8858_LONG_DIGIGAIN_H_SHIFT);
> +	if (ov8858->xvclk_rate != OV8858_XVCLK_FREQ_19_2MHZ) {
> +		long_gain = (gain & OV8858_LONG_DIGIGAIN_L_MASK) |
> +			    ((gain & OV8858_LONG_DIGIGAIN_H_MASK) <<
> +			     OV8858_LONG_DIGIGAIN_H_SHIFT);
>  

> +		return ov8858_write(ov8858, OV8858_REG_LONG_DIGIGAIN,
> +				    long_gain, NULL);
> +	}

Instead it's better to keep the current function as a helper. You might need to
rename it first.

> +	ret = ov8858_write(ov8858, OV8858_REG_MWB_RED_GAIN, gain, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov8858_write(ov8858, OV8858_REG_MWB_GREEN_GAIN, gain, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return ov8858_write(ov8858, OV8858_REG_MWB_BLUE_GAIN, gain, NULL);
>  }

-- 
With Best Regards,
Andy Shevchenko