Re: [PATCH v3 4/4] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

Hi Fernando,

On Tue, Jul 21, 2026 at 01:50:17AM +0200, Fernando Rimoli wrote:
> Hi Dan,
> 
> Thanks for the reviews on 1-3.
> 
> You're right that keying on both the PCI ID and the sensor is a bit awkward. My
> reasoning for scoping it that tightly was caution rather than a known IPU3
> failure: I only have IPU6 hardware (Surface Pro 9), so I couldn't confirm that
> gating the ov5693's clock lane is safe on the IPU3 CSI-2 receiver, and I didn't
> want to risk regressing the existing cio2 + ov5693 users (the INT33BE Surface
> Pro/Book devices) that work today with the free-running default.

Please limit the line length to around 75.

> 
> For what it's worth, from the receiver side IPU3 looks agnostic to the flag:
> ipu3-cio2 only consumes bus.mipi_csi2.num_data_lanes from the parsed endpoint
> and programs its D-PHY Rx timing (clk_termen/clk_settle) the same way regardless
> of V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK, it never looks at that flag. So the open
> question is purely sensor-side: whether the ov5693 idling its clock lane in LP11
> (bit 5) upsets the cio2 D-PHY's lock. I can't answer that without IPU3 hardware.

The support for non-contiguous clock isn't mandatory on either side
(whereas free-running clock is and should always "just work") so as a whole
this is weird. But as we know the sensor works with IPU6 with non-continous
clock, that's what I guess we'll just have to do then.

> 
> If your test tomorrow shows cio2 + ov5693 still streams fine with
> clock-noncontinuous set, I'm happy to drop the ipu6_pci_tbl check entirely and
> just request the property for the ov5693 unconditionally in v4 which removes
> the PCI quirk and is much cleaner. (The sensor-driver side already no-ops when
> the flag is absent, so nothing else needs to change.)
> 
> If it turns out IPU3 doesn't like it, then the PCI gate is doing real work and
> I'd keep it, but I can add a comment making that rationale explicit.
> 
> Either way I'll respin once we know. Thanks a lot for offering to test on IPU3,
> that's the one platform I can't cover.

How about adding PCI IDs (for matching the particualr IPU) and flags to
struct ipu_sensor_config? I have a feeling we'll need this elsewhere, too.

Then e.g.

#define IPU_SENSOR_CONFIG_MATCH_FL(_HID, _ID, _FLAGS, _NR, ...)	\
	(const struct ipu_sensor_config) {			\
		.hid = _HID,					\
		.pci_id = _ID,					\
		.flags = IPU_BR_FL_##_FLAGS,			\
		.nr_link_freqs = _NR,				\
		.link_freqs = { __VA_ARGS__ }			\
	}

#define IPU_SENSOR_CONFIG(_HID, _ID, ...) \
	IPU_SENSOR_CONFIG_MATCH_FL(_HID, 0, 0, _NR, ...)

Where _ID is the IPU PCI product ID and flags is e.g.

#define IPU_BR_FL_CSI2_CLK_NONCONTINUOUS		BIT(0)

You could also switch to dynamically assigning the property index so
there's no need to rely on a particular device having a list of link
frequencies. See NEXT_PROPERTY() macro in drivers/acpi/mipi-disco-img.c .
That should go to a separate patch, like adding the above mechanism.

-- 
Kind regards,

Sakari Ailus