Re: [PATCH v4 3/6] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

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

Hi Fernando,

On Mon, Aug 31, 2026 at 08:18:55PM +0200, Fernando Rimoli wrote:
> The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
> power-on default, which lets the MIPI clock run freely. The IPU3 CSI-2
> receiver tolerates this, but the IPU6 receiver (e.g. on Microsoft
> Surface Pro 8/9 and Surface Go 4) fails to lock onto the link, so the
> sensor streams but capture times out with "stream stop time out" and no
> frames arrive.
> 
> Parse the "clock-noncontinuous" endpoint property (which sets the
> V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK flag) and, when it is present, gate
> the clock lane while idle at stream on. Bit 5 of MIPI_CTRL00 has the

I'd clip what's after the period above.

> same meaning here as in ov5647, which sets it for the same purpose.
> 
> Unlike ov5647, which owns the register across its own stream on and off,
> this is a read-modify-write of the single gate bit: the ov5693 otherwise
> never touches MIPI_CTRL00, so preserving the rest of it keeps every
> platform that does not ask for a non-continuous clock bit-for-bit as it
> was. No counterpart is needed at stream off, as the link is down by then
> and the register returns to its default when the sensor is powered off.
> 
> The "clock-noncontinuous" property is supplied by the ipu-bridge for the
> affected IPU6 variants in a subsequent patch.
> 
> Link: https://github.com/linux-surface/linux-surface/pull/2171
> Co-developed-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> ---
>  drivers/media/i2c/ov5693.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 02236f3db..cedc6ea03 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -35,6 +35,11 @@
>  #define OV5693_STOP_STREAMING			0x00
>  #define OV5693_SW_RESET				0x01
>  
> +/* MIPI transmitter control */
> +#define OV5693_MIPI_CTRL00_REG			CCI_REG8(0x4800)
> +/* Gate the clock lane when there is no packet to transmit */
> +#define OV5693_MIPI_CTRL00_CLOCK_LANE_GATE	BIT(5)
> +
>  #define OV5693_REG_CHIP_ID			CCI_REG16(0x300a)
>  /* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */
>  #define OV5693_CHIP_ID				0x5690
> @@ -144,6 +149,9 @@ struct ov5693_device {
>  	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
>  	struct clk *xvclk;
>  
> +	/* Gate the MIPI clock lane when idle (CSI-2 non-continuous clock) */
> +	bool clock_ncont;
> +
>  	struct ov5693_mode {
>  		struct v4l2_rect crop;
>  		struct v4l2_mbus_framefmt format;
> @@ -611,6 +619,19 @@ static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
>  {
>  	int ret = 0;
>  
> +	/*
> +	 * Gate the MIPI clock lane while idle if the CSI-2 link is configured
> +	 * for a non-continuous clock. Only that bit is touched, and only in
> +	 * that case, so the register keeps whatever the platform left in it
> +	 * and the clock stays free-running as before everywhere else. It
> +	 * needs no counterpart at stream off: the link is down by then, and
> +	 * the register returns to its default when the sensor is powered off.
> +	 */
> +	if (enable && ov5693->clock_ncont)
> +		cci_update_bits(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
> +				OV5693_MIPI_CTRL00_CLOCK_LANE_GATE,
> +				OV5693_MIPI_CTRL00_CLOCK_LANE_GATE, &ret);
> +
>  	cci_write(ov5693->regmap, OV5693_SW_STREAM_REG,
>  		  enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
>  		  &ret);
> @@ -1259,6 +1280,9 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>  		goto out_free_bus_cfg;
>  	}
>  
> +	ov5693->clock_ncont = bus_cfg.bus.mipi_csi2.flags &
> +			      V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;

Could you also make the change to the DT bindings, adding "clock-noncontinuous:
true" there?

> +
>  out_free_bus_cfg:
>  	v4l2_fwnode_endpoint_free(&bus_cfg);
>  

-- 
Regards,

Sakari Ailus