Re: [PATCH v2 3/3] media: ov02c10: Accept a 26 MHz external clock

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Felipe,

Thanks for the update.

I think this patch still needs a bit more work, see below.

On Wed, Sep 23, 2026 at 11:42:57AM -0300, Felipe Calliari wrote:
> Several Meteor Lake / Lunar Lake designs (e.g. the Samsung Galaxy Book3/4
> series) wire the OV02C10 to a 26 MHz external clock instead of the
> 19.2 MHz assumed so far. The IPU6 ipu-bridge forwards the rate from the
> ACPI SSDB verbatim as the "clock-frequency" property, so probe() just
> rejects it today:
> 
>   ov02c10 i2c-OVTI02C1:00: external clock 26000000 is not supported
> 
> Rename OV02C10_MCLK to OV02C10_MCLK_19_2MHZ, add OV02C10_MCLK_26MHZ and
> accept both.
> 
> The register tables program the OP PLL multiplier (0x0304/0x0305, 16-bit)
> to 0x0190 = 400 for a 19.2 MHz clock: the common table writes
> 0x0304 = 0x01 and the per-lane tables override 0x0305 = 0x90. Left alone
> on a 26 MHz clock the same dividers run every internal clock, and
> therefore the MIPI link, 26/19.2 = 1.3542x faster: a ~541.7 MHz link at
> ~40 fps instead of the nominal 400 MHz at ~30 fps.
> 
> Re-program both PLL multipliers by the inverse factor instead, so that
> the link stays where the driver and the ipu-bridge fwnode already
> describe it: 400 * 19.2 / 26 = 295 = 0x0127, written to 0x0304/0x0305
> and 0x0315/0x0316 after the per-lane table when the external clock is
> 26 MHz. That puts the link at 295 * 26 / 19.2 = 399.5 MHz, within 0.13%
> of the nominal 400 MHz, so link-frequency and pixel-rate stay accurate
> with the single existing menu entry and the ipu-bridge needs no change.

Please do add that frequency; it's still different from what's supported
now. If someone later on improves the driver and implements a PLL
calculator for it, this will stop working.

What about pixel rate? It's also affected, isn't it?

> 
> Tested on a Samsung Galaxy Book3 (two CSI-2 data lanes, 26 MHz clock
> confirmed via clk_summary): the multiplier registers read back 0x0127
> while streaming and capture runs at a steady 30.01 fps by buffer
> timestamp, against ~40 fps with the unmodified tables, matching the
> 30.14 fps that the exported hblank, vblank and pixel-rate describe.

If the patch is adding support for a new link frequency, just say that. The
rest goes to the cover letter.

> 
> While touching the clock check, terminate its error string with a
> newline.
> 
> Link: https://lore.kernel.org/linux-media/ap_CGQTdNFysTLot@kekkonen.localdomain/
> Signed-off-by: Felipe Calliari <calliarifelipe@gmail.com>
> ---
> 
> Changes in v2:
> - Re-program the OP and VT PLL multipliers instead of advertising the
>   scaled 541.667 MHz link frequency (Sakari). The suggested 0x28a / 0x1b1
>   produce no output on this hardware; 0x0127 -- the 0x0190 the tables
>   already program, scaled by 19.2/26 -- does, and brings the frame rate
>   back to the nominal ~30 fps.
> - Drop the second V4L2_CID_LINK_FREQ entry that v1 added: with the link
>   back at ~400 MHz the existing single entry matches the ipu-bridge
>   fwnode directly, so no ipu-bridge change is needed either.
> - Bryan's Reviewed-by and the Tested-by on v1 are not carried over, as
>   the patch was rewritten.
> 
>  drivers/media/i2c/ov02c10.c | 43 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
> index cdccbdef3..17208f2e6 100644
> --- a/drivers/media/i2c/ov02c10.c
> +++ b/drivers/media/i2c/ov02c10.c
> @@ -16,7 +16,8 @@
>  #include <media/v4l2-fwnode.h>
>  
>  #define OV02C10_LINK_FREQ_400MHZ	400000000ULL
> -#define OV02C10_MCLK			19200000
> +#define OV02C10_MCLK_19_2MHZ		19200000
> +#define OV02C10_MCLK_26MHZ		26000000
>  #define OV02C10_RGB_DEPTH		10
>  
>  #define OV02C10_NATIVE_WIDTH		1928
> @@ -337,6 +338,23 @@ static const struct reg_sequence sensor_1928x1092_30fps_2lane_setting[] = {
>  	{0x3016, 0x32},
>  };
>  
> +/*
> + * The mode tables target a 19.2 MHz input clock, programming the OP PLL
> + * multiplier (0x0304/0x0305, 16-bit) to 0x0190 = 400 for a 400 MHz link at
> + * ~30 fps.  A 26 MHz input clock instead runs every internal clock, and
> + * therefore the MIPI link, 26/19.2 = 1.3542x faster (~541.7 MHz, ~40 fps).
> + * Scaling both PLL multipliers by 19.2/26 -- 400 * 19.2 / 26 = 295 = 0x0127
> + * -- puts the link back at 295 * 26 / 19.2 = 399.5 MHz, within 0.13% of the
> + * nominal 400 MHz, so the frame rate and the advertised link frequency both
> + * stay correct without a second link-frequency entry.
> + */
> +static const struct reg_sequence sensor_pll_26mhz_setting[] = {
> +	{0x0304, 0x01},
> +	{0x0305, 0x27},
> +	{0x0315, 0x01},
> +	{0x0316, 0x27},
> +};

This needs to be preceded by a patch splitting off these registers from the
main register list. The result, in this patch, should be two register lists
to choose from (or even better, a PLL calculator).

> +
>  static const char * const ov02c10_test_pattern_menu[] = {
>  	"Disabled",
>  	"Color Bar",
> @@ -396,6 +414,9 @@ struct ov02c10 {
>  	/* MIPI lane info */
>  	u32 link_freq_index;
>  	u8 mipi_lanes;
> +
> +	/* External (sensor) clock rate, Hz */
> +	u32 xvclk_freq;
>  };
>  
>  static inline struct ov02c10 *to_ov02c10(struct v4l2_subdev *subdev)
> @@ -619,6 +640,17 @@ static int ov02c10_enable_streams(struct v4l2_subdev *sd,
>  		goto out;
>  	}
>  
> +	if (ov02c10->xvclk_freq == OV02C10_MCLK_26MHZ) {
> +		reg_sequence = sensor_pll_26mhz_setting;
> +		sequence_length = ARRAY_SIZE(sensor_pll_26mhz_setting);
> +		ret = regmap_multi_reg_write(ov02c10->regmap,
> +					     reg_sequence, sequence_length);
> +		if (ret) {
> +			dev_err(ov02c10->dev, "failed to write PLL settings\n");
> +			goto out;
> +		}
> +	}
> +
>  	ret = __v4l2_ctrl_handler_setup(ov02c10->sd.ctrl_handler);
>  	if (ret)
>  		goto out;
> @@ -877,6 +909,10 @@ static int ov02c10_check_hwcfg(struct ov02c10 *ov02c10)
>  	/* v4l2_link_freq_to_bitmap() guarantees at least 1 bit is set */
>  	ov02c10->link_freq_index = ffs(link_freq_bitmap) - 1;
>  
> +	dev_dbg(dev, "%u Hz external clock, link freq %lld Hz\n",
> +		ov02c10->xvclk_freq,
> +		link_freq_menu_items[ov02c10->link_freq_index]);

Maybe useful at development time, but hardly anymore.

> +
>  	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 1 &&
>  	    bus_cfg.bus.mipi_csi2.num_data_lanes != 2) {
>  		ret = dev_err_probe(dev, -EINVAL,
> @@ -926,10 +962,11 @@ 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)
> +	if (freq != OV02C10_MCLK_19_2MHZ && freq != OV02C10_MCLK_26MHZ)
>  		return dev_err_probe(ov02c10->dev, -EINVAL,
> -				     "external clock %lu is not supported",
> +				     "external clock %lu is not supported\n",
>  				     freq);
> +	ov02c10->xvclk_freq = freq;
>  
>  	v4l2_i2c_subdev_init(&ov02c10->sd, client, &ov02c10_subdev_ops);
>  

-- 
Regards,

Sakari Ailus