Re: [PATCH 6/7] media: i2c: ov7251: Add strobe output enable control

[Andy Shevchenko]

On Mon, Oct 26, 2026 at 08:10:31PM +0900, Antoine Pichot wrote:
> The OV7251 has a STROBE output that pulses during the exposure of
> selected frames, meant to drive an illuminator. On the Microsoft
> Surface Book 2 the front OV7251 is an infrared camera (Windows Hello)
> and its 850 nm emitter is wired to that pin: ACPI describes no separate
> LED, GPIO line or regulator for it. The mode tables leave the pad
> disabled, so the camera only sees an unlit scene.
> 
> Add a V4L2_CID_FLASH_STROBE_OE control, off by default. When enabled
> it sets the STROBE pad output enable (SC_REG5 bit 3), strobes
> alternate frames (frame pattern 0xaa instead of the default 0xa5, which
> lights four frames in eight) and sets the
> pulse length (strobe frame span, in lines) to the exposure time,
> clamped between 52 lines and 45% of the frame length, never more than
> 776 lines. When disabled it
> restores the defaults the mode tables used to write. The span follows
> exposure and vertical blanking changes while the strobe is enabled.
> 
> The limits come from the Windows driver, which clamps the span to 52
> to 776 lines: 776 lines is 45% of the 1724-line frame of the 30 fps
> mode. Expressing the upper limit as a duty cycle keeps the emitter
> within that envelope (eye safety) at 60 and 90 fps too, where a fixed
> 776 lines would let the pulse cover up to 96% of the frame (552 of 572
> lines). Keeping the 776-line limit as well bounds the pulse when
> vertical blanking stretches the frame, up to 65535 lines.
> 
> Alternate frames is what the Windows driver uses for face recognition
> on this hardware: its 30 and 60 fps modes write 0xaa, and a register
> dump taken during a Windows Hello sign-in on a Surface Book 2 shows the
> same. Lighting every frame (0xff, which two of its other modes use)
> would double the average emitter power. User space tells lit frames
> from dark ones by their brightness; V4L2 has no per-frame illumination
> flag yet.
> 
> The control is written only while the sensor is powered, and from
> __v4l2_ctrl_handler_setup() at stream start, after the mode and strobe
> register tables. Setting it while the sensor is idle does not power it
> up, and the emitter goes off with the sensor when streaming stops.
> 
> On a Surface Book 2 at analogue gain 128 the mean pixel value of the
> infrared image of a room is 19 unlit; with the strobe enabled the lit
> frames read 496 at exposure 504 (span 504) and 587 at exposure 1704
> (span 776), the dark frames in between 18 to 20.
> The span read back while streaming is 258 at 90 fps, 45% of the
> 572-line frame.

...

> +static int ov7251_set_strobe_span(struct ov7251 *ov7251)
> +{
> +	u32 span = OV7251_STROBE_FRAME_SPAN_DEF;
> +	u32 vts, max_span;
> +	u8 val[4];

> +	/*
> +	 * The pulse starts with the integration: make it cover the exposure,
> +	 * from 52 lines up to 45% of the frame and never more than 776 lines.
> +	 * The vendor driver caps it at 776 lines, 45% of the 1724 lines 30 fps
> +	 * frame; the duty cycle limit keeps the emitter within that envelope
> +	 * at higher frame rates, the absolute one with long vertical blanking.
> +	 */
> +	if (ov7251->strobe->val) {
> +		vts = ov7251->current_mode->height + ov7251->vblank->val;
> +		max_span = DIV_ROUND_UP(vts * OV7251_STROBE_MAX_DUTY_PERCENT,
> +					100);

units.h provides PERCENT constant.

		max_span = DIV_ROUND_UP(vts * OV7251_STROBE_MAX_DUTY_PERCENT,
					PERCENT);

> +		max_span = min(max_span, OV7251_STROBE_SPAN_MAX);
> +		span = clamp_t(u32, ov7251->exposure->val,
> +			       OV7251_STROBE_SPAN_MIN, max_span);

Can we avoid using _t version of clamp()?

And naturally I would first clamp over the _MAX/_MIN range (this addresses the
previous Q) and then run min() on top.

> +	}
> +
> +	put_unaligned_be32(span, val);
> +
> +	return ov7251_write_seq_regs(ov7251, OV7251_STROBE_FRAME_SPAN, val,
> +				     sizeof(val));

Can we move at some point to CCI accessors (v4l2-cci.h)?

> +}

-- 
With Best Regards,
Andy Shevchenko