Re: [PATCH] media: i2c: ov02c10: tolerate a sensor clock other than 19.2 MHz

[Sakari Ailus]

Hi Ryan,

Thanks for the patch.

On Thu, Jul 02, 2026 at 12:10:22PM -0400, Ryan Thomas Cragun wrote:
> The driver requires the sensor's external clock to be exactly 19.2 MHz
> (OV02C10_MCLK) and aborts probe with -EINVAL otherwise. This leaves the
> camera completely unusable on platforms that clock the sensor
> differently.
> 
> For example, the Microsoft Surface Laptop 7 (Intel) drives the OV02C10
> from a fixed 12 MHz clock (provided by an INT3472 "discrete" clock that
> cannot be reprogrammed). The sensor's register/PLL tables assume 19.2
> MHz, so at 12 MHz all timings scale by 12/19.2 (the nominal 30 fps mode
> runs at ~18.75 fps), but the sensor is otherwise fully functional and
> produces a correct image.
> 
> Rather than failing probe:
> 
>  - Attempt clk_set_rate(OV02C10_MCLK). On platforms whose sensor clock is
>    programmable (e.g. a TPS68470 PMIC) this yields the expected 19.2 MHz
>    and the native frame rate.
>  - If the rate still differs (fixed clock), warn and continue instead of
>    aborting, so the camera works.
> 
> Platforms that already provide 19.2 MHz are unaffected (no warning, no
> rate change). The reduced frame rate on a lower clock can be restored by
> shrinking the sensor's vertical blanking (VTS); that can be addressed
> separately.
> 
> Signed-off-by: Ryan Thomas Cragun <ryantcragun@xxxxxxxxx>
> ---
>  drivers/media/i2c/ov02c10.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
> index cf93d3603..01cbaedfc 100644
> --- a/drivers/media/i2c/ov02c10.c
> +++ b/drivers/media/i2c/ov02c10.c
> @@ -892,10 +892,19 @@ static int ov02c10_probe(struct i2c_client *client)
>  				     "failed to get imaging clock\n");
>  
>  	freq = clk_get_rate(ov02c10->img_clk);
> +	if (freq != OV02C10_MCLK) {
> +		/*
> +		 * Some platforms provide the sensor clock via a programmable
> +		 * PMIC. Ask for OV02C10_MCLK; if that is not possible (e.g. a
> +		 * fixed 12 MHz clock on the Surface Laptop 7) proceed anyway.
> +		 */
> +		if (clk_set_rate(ov02c10->img_clk, OV02C10_MCLK) == 0)
> +			freq = clk_get_rate(ov02c10->img_clk);

No need to try to set the rate -- in general, the frequency isn't supposed
to be changed by the driver here.

> +	}
>  	if (freq != OV02C10_MCLK)
> -		return dev_err_probe(ov02c10->dev, -EINVAL,
> -				     "external clock %lu is not supported",
> -				     freq);
> +		dev_warn(ov02c10->dev,
> +			 "external clock %lu differs from expected %u; proceeding anyway\n",
> +			 freq, OV02C10_MCLK);

I'd use dev_info().

The driver should have PLL configuration for 12 MHz external clock. The
driver should also calculate its pixel rate from the external clock. Both
are obviously out of scope of this patch.

>  
>  	v4l2_i2c_subdev_init(&ov02c10->sd, client, &ov02c10_subdev_ops);
>  

-- 
Regards,

Sakari Ailus