[PATCH v3 0/9] media: i2c: ov7251: fixes and infrared strobe for Surface Book 2

[PATCH v3 0/9] media: i2c: ov7251: fixes and infrared strobe for Surface Book 2

[Antoine Pichot]

The Microsoft Surface Book 2 has an OV7251 infrared camera (used by
Windows Hello) next to its front camera, both on the Intel IPU3 camera
interface. Its 850 nm emitter is wired to the sensor's STROBE output
pin: the ACPI tables describe no LED, GPIO line or regulator for it.
With the mainline driver the camera streams, but always unlit.

Patches 1 to 4 fix bugs found while testing on this machine and on a
Surface Pro 7+, 5 and 6 rework the driver's state handling and add the
placement controls:

- 1: the 19.2 MHz PLL table for the 319.2 MHz link multiplies the
  clock before dividing it, which runs the PLL's voltage-controlled
  oscillator (VCO) at 2553.6 MHz against the 300 to 800 MHz the
  datasheet allows. The Surface Pro 7+ gets CSI-2 errors or stalls in
  most runs at that setting; the Book 2 copes. Dividing first keeps
  the link rate and brings the VCO to 638.4 MHz (Fixes, Cc stable). It
  precedes patch 2, which is what makes the driver select this table.
- 2: the link frequency index is taken from the firmware list instead
  of the driver's table, so the sensor runs at 240 MHz while
  ipu-bridge asks for 319.2 MHz (Fixes, Cc stable).
- 3: V4L2_CID_PIXEL_RATE reports the CSI-2 bus rate instead of the
  pixel array clock, which is 48 MHz at both link frequencies (Fixes,
  Cc stable).
- 4: the 60 and 90 fps frame intervals are accepted but the sensor
  keeps streaming at 30 fps, the vertical blanking control not being
  updated (Fixes, Cc stable).
- 5: conversion to the sub-device active state, as Sakari asked on
  v1 [7]. The core then handles control events and v4l2-compliance
  passes on the sensor.
- 6: orientation and rotation controls from firmware.

Patch 4 only fixes the current behaviour of set_frame_interval, which
Dave Stevenson noted on v1 does not belong in a raw sensor driver that
also exposes the vertical blanking control. Patches 5 to 9 apply on
top of 1 to 4.

Patches 7 and 8 add the emitter.

Patch 7 is Dave Stevenson's, taken from his ov7251 branch with his
agreement. The three mode tables wrote the same 117 registers; the patch
moves them into the global init table, written once after the power-on
reset, and leaves each mode table with the six clock control registers
(0x3016 to 0x301c). The frame length, the only other value that differed
between the tables, is dropped from them since the vertical blanking
control writes it. Two side effects show on the bus: a stream restarted
before the runtime autosuspend powers the sensor down no longer rewrites
the common registers, which the sensor keeps in software standby, and
register 0x3820 holds 0x40 while streaming, the value the mode tables
had always asked for, instead of 0x00; the picture is unchanged. The
datasheet names the bits of the clock control registers as per-block
clock gates and resets and does not say what a cleared bit does. The
30 fps table holds the reset values but for one reset bit, the 60 and
90 fps tables what the machine's Windows driver writes in all four of
its modes. On the Book 2 the sensor streams the same with either set at
all three rates (rate, black level, test pattern, no CSI-2 errors).
Clearing the gates, in [4] and then one at a time, changed only two
blocks the driver does not use (OTP load, group hold) and the timing
control gate, which stops the frames when cleared. Power was not
measured. Whether one set moves to the init table, as [4] proposed, is
left to a follow-up; the rest of Dave's branch (CCI register helpers
among them) waits for this series to land, as agreed on the v2 thread.

Patch 8 adds a V4L2_CID_FLASH_STROBE_OE control that enables the
STROBE pin, pulses it on alternate frames and makes the pulse follow
the exposure, from 52 lines up to 45% of the frame, capped at 776
lines. This matches the Windows driver of this machine (Intel
ov7251.sys 30.18305.6.5127, read with a decompiler): all four of its
streaming modes enable the STROBE pin and clamp the pulse to the
exposure within 52 to 776 lines; the two Windows Hello uses light
alternate frames (0xaa), the other two every frame (0xff). A register
dump from a Surface Book 2 during a Windows Hello sign-in [1] matches
the 30 fps alternate mode. With every frame lit and filmed by a phone
at locked exposure, each pulse is 1.1x (default exposure) to 1.2x
(776-line pulse) as bright as during a Windows Hello sign-in on the
same unit; with alternate frames the average emission is within the
same ratio of Windows'.

Two design choices differ from ov9282, the other sensor with this
control. ov9282 exposes the pulse length as V4L2_CID_FLASH_DURATION;
here the pulse follows the exposure within the envelope the Windows
driver applies at every frame rate, so user space cannot drive the
emitter beyond it, and a duration control can be added later. ov9282
pulses every frame; here the pattern is fixed to alternate frames
because the dark frames are the ambient-light reference that face
recognition subtracts from the lit ones (V4L2 has no per-frame
illumination flag, so user space tells them apart by brightness). A
control to light every frame, asked about on v2 [5] without an answer,
can be added later without changing this one.

A linux-surface pull request for the Surface Pro 7+ [2] turns the
emitter on whenever the sensor streams, with a module parameter to
disable it. This series leaves the choice to user space: the control
is off by default, and the emitter is off whenever the sensor is not
streaming.

Patch 9 makes ipu-bridge report a rotation of 90 degrees for the
infrared sensor, whose frames show an upright scene turned 90 degrees
clockwise while the firmware says 0. It adds a DMI quirk table next to
the existing one for upside-down sensors, with entries for the Surface
Book 2 and the Surface Pro 7+, the two machines checked. The entries
match on the SKU because Microsoft reuses the product name across
sizes and the 15 inch Book 2 was not checked. A user-space project
rotates the Surface Book 3's infrared frames by 90 degrees too [3].

Patch 8 needs V4L2_CID_FLASH_STROBE_OE, added in v7.0 by commit
5be4154f6255 ("media: v4l: ctrls: add a control for enabling strobe
output"), and is not meant for stable. Only patches 1 to 4 apply to
older trees on their own.

[1] https://github.com/linux-surface/linux-surface/issues/739
[2] https://github.com/linux-surface/linux-surface/pull/2252
[3] https://github.com/linux-surface/linux-surface/pull/2289
[4] https://lore.kernel.org/linux-media/20261030142628.2235190-1-antoine.pichot@gmail.com/
[5] https://lore.kernel.org/linux-media/20261030121235.2163478-1-antoine.pichot@gmail.com/

Changes since v2 [6]:
- Patch 1: Reported-by and Closes tags for D. Manresa's report, and
  Dave Stevenson's measurement cited as [1] in the text (Andy
  Shevchenko). The sentence on the 24 MHz tables was wrong, Dave has
  tested them: they are left to a separate change (Dave Stevenson).
- Patch 3: the pixel clock is defined as 48 * HZ_PER_MHZ, and Dave's
  report carries Reported-by and Closes tags (Andy Shevchenko).
- Patch 5: code unchanged; Suggested-by and Link added for Sakari's v1
  request. V4L2 has no guard class for the state lock, so the explicit
  lock and unlock pair stays, as imx290, imx415, ov2740 and ov8858 do in
  s_stream (Andy Shevchenko, answered on the thread). The move to
  enable_streams and disable_streams, which removes the pair, is left to
  a follow-up to keep this series to the fixes and the camera.
- Patch 7: replaced by Dave Stevenson's patch described above, which
  answers his question on v1 about where the strobe registers belong;
  v2's patch moved only the strobe registers out of the mode tables.
  D. Manresa's Tested-by from v2's patch 7 is not carried over, the
  patch being a different one.
- Patch 8: follows the init table of patch 7, which now holds the
  strobe defaults it restores; the commit message now also says that
  the dark frames are the ambient-light reference face recognition
  subtracts.
- Patch 9: D. Manresa's report cited as [1] in the text (Andy
  Shevchenko).
- Rebased on the current media-committers next.

[6] https://lore.kernel.org/linux-media/20261028112154.855185-1-antoine.pichot@gmail.com/
[7] https://lore.kernel.org/linux-media/20261026111032.88355-1-antoine.pichot@gmail.com/

Tested on a Surface Book 2 (i7-8650U) with the series built out of
tree against linux-surface 7.2.12 and 7.2.15 (test suite and logs:
https://github.com/antoine79/surface-ir-camera). The rebase changed
no code: ov7251.c on the base commit is byte-identical to the driver
tested, and both files build on the base with W=1 without warnings.

- link frequency 319200000, pixel rate 48000000, 30.0 fps measured;
  PLL registers read back as programmed while streaming
- 3 x 155 frames at 30 fps and 3 x 155 at 90 fps on the 319.2 MHz
  link: 0 dropped buffers, 0 CSI-2 error lines
- v4l2-compliance 1.32.0 -u on the sensor: 0 failures
- --set-subdev-fps 60 / 90: 60.16 / 90.44 fps measured; mode switches
  from 30 to 90, to 60 and back to 30 fps on a powered sensor give a
  correct first frame every time
- the clock, timing, PLL, flip and strobe registers read back while
  streaming at 30, 60 and 90 fps are those of v2, apart from 0x3820
- orientation front, rotation 90 with patch 9 (on 7.2.12; the 7.2.15
  runs kept the stock ipu-bridge); the colour sensors still report 180
- dark frames and the sensor test pattern unchanged
- strobe span read back while streaming: 504 lines at exposure 504 and
  776 at exposure 1704 (30 fps), 258 at exposure 552 (90 fps); it
  follows exposure changes mid-stream
- mean pixel value of an indoor infrared frame at analogue gain 128
  (12 Nov run; patch 8's message quotes the 25 Oct run): 18 to 19 unlit;
  with the strobe enabled the lit frames read 517 at exposure 504 and
  681 at 1704, the dark frames in between 18 to 21
- Howdy face login recognises the enrolled face through a v4l2loopback
  bridge that passes the lit frames
- strobe off and sensor suspended within 2 s of the stream stopping

D. Manresa's Tested-by tags on patches 4 and 6 date from v1; the changes
those patches make are the same since. The Surface Pro 7+ ran v1 and the
divider fix of patch 1 as a test patch (13 clean 150-frame runs of 13),
not v2 or v3. The remaining v4l2-compliance failures are in the
ipu3-cio2 capture driver (buffer creation and mapping checks) and
unchanged by this series.

I developed this series with an AI coding assistant (Claude) over
interactive sessions: it drafted the code, the commit messages, the
test scripts and this letter from my notes, measurements and register
dumps. The design choices (alternate frames, the pulse limits, the
exact SKU match), the reading of the Windows driver and the
measurements above are mine, and I reviewed and tested every patch on
the hardware before signing it off.

Antoine Pichot (8):
  media: i2c: ov7251: Use in-range PLL1 dividers for the 319.2 MHz link
  media: i2c: ov7251: Fix link frequency index
  media: i2c: ov7251: Report the pixel array rate for both link
    frequencies
  media: i2c: ov7251: Reset vblank when the frame interval changes
  media: i2c: ov7251: Use sub-device active state
  media: i2c: ov7251: Add orientation and rotation controls
  media: i2c: ov7251: Add strobe output enable control
  media: ipu-bridge: Add a rotation quirk for the Surface Book 2 and Pro
    7+

Dave Stevenson (1):
  media: i2c: ov7251: Move the common registers into the global init
    table

 drivers/media/i2c/ov7251.c           | 531 ++++++++-------------------
 drivers/media/pci/intel/ipu-bridge.c |  31 ++
 2 files changed, 191 insertions(+), 371 deletions(-)


base-commit: 3fc9303dba01676c2090768126e5327e341b7761
-- 
2.56.0

[PATCH v3 1/9] media: i2c: ov7251: Use in-range PLL1 dividers for the 319.2 MHz link

[Antoine Pichot]

The 19.2 MHz PLL1 table for the 319.2 MHz link frequency puts a
pre-divider of 1 before the multiplier of 133 and a divider of 4 after
it, so the PLL1 multiplier output (the VCO) runs at 19.2 x 133 =
2553.6 MHz before being divided down to 638.4 MHz. The OV7750/OV7251
datasheet (rev 2.12, table 2-11) limits it to 300 to 800 MHz.

Use pre-divider 4 (register 0x30B4 = 0x04) and divider 1 (0x30B1 =
0x01). The multiplier output is then 4.8 x 133 = 638.4 MHz, inside the
range, and the MIPI clock stays 638.4 Mbps, so the link frequency does
not change. The Windows driver programs these values for the Surface
Book 2's Windows Hello mode.

On ACPI systems this table has been unreachable since it was added,
because ov7251_check_hwcfg() selects the wrong index. The next patch
fixes the index, so the two must go in together. With the index fix
alone, D. Manresa's Surface Pro 7+ (IPU6) had CSI-2 errors in 7 of 10
150-frame runs, 3 of them stalls; with this change 0 of 13. A Surface
Book 2 (IPU3) streams clean with either table. Dave Stevenson measured
on a 24 MHz Raspberry Pi board that the out-of-range tables miss their
nominal frequency and proposed the same values [1]. The 24 MHz tables
have problems of their own and are left to a separate change.

Fixes: ed9566ce1946 ("media: i2c: Add support for new frequencies to ov7251")
Cc: stable@vger.kernel.org
Reported-by: D. Manresa <dmanresa@gmail.com>
Closes: https://lore.kernel.org/linux-media/6ae0e944.b6db0abd.16b13d.d261@mx.google.com/
Link: https://lore.kernel.org/linux-media/CAPY8ntBFJJPg3y46gC=bNq_55QifC7mKmeq8+pAEDVN42fGUXw@mail.gmail.com/ [1]
Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index fce6344b5120..f429de5f1f7c 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -174,9 +174,9 @@ static const struct ov7251_pll1_cfg ov7251_pll1_cfg_19_2_mhz_240_mhz = {
 };
 
 static const struct ov7251_pll1_cfg ov7251_pll1_cfg_19_2_mhz_319_2_mhz = {
-	.pre_div = 0x01,
+	.pre_div = 0x04,
 	.mult = 0x85,
-	.div = 0x04,
+	.div = 0x01,
 	.pix_div = 0x0a,
 	.mipi_div = 0x05,
 };
-- 
2.56.0

[PATCH v3 2/9] media: i2c: ov7251: Fix link frequency index

[Antoine Pichot]

ov7251_check_hwcfg() stores i, the index into the firmware's
link_frequencies array, in link_freq_idx, which the driver then uses to
index its own link_freq[], pixel_rates[] and pll1[] tables. The matching
driver entry is j. Store j.

Observed on a Surface Book 2, where ipu-bridge lists one link frequency,
319.2 MHz, which is link_freq[1]: V4L2_CID_LINK_FREQ reads 240000000,
and the pixel rate and PLL registers are off the same way.

The 319.2 MHz table this selects ran its PLL outside the datasheet
range until the previous patch, so do not apply this fix without it.

Fixes: cc125aaa5a78 ("media: i2c: Provide ov7251_check_hwcfg()")
Cc: stable@vger.kernel.org
Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/media/i2c/ov7251.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index f429de5f1f7c..1f30a01c85b3 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1523,7 +1523,7 @@ static int ov7251_check_hwcfg(struct ov7251 *ov7251)
 		goto out_free_bus_cfg;
 	}
 
-	ov7251->link_freq_idx = i;
+	ov7251->link_freq_idx = j;
 
 out_free_bus_cfg:
 	v4l2_fwnode_endpoint_free(&bus_cfg);
-- 
2.56.0

[PATCH v3 3/9] media: i2c: ov7251: Report the pixel array rate for both link frequencies

[Antoine Pichot]

pixel_rates[] gives V4L2_CID_PIXEL_RATE 48 MHz for the 240 MHz link and
63.84 MHz for the 319.2 MHz link, which is the CSI-2 bus rate of each
link (one lane, 10 bits per pixel). The control is the pixel array rate,
and Documentation/driver-api/media/tx-rx.rst says the bus rate "is not
the same thing as the pixel rate on the camera sensor's pixel array
which is indicated by the V4L2_CID_PIXEL_RATE control".

The pixel array runs from PLL2's 48 MHz system clock at both link
frequencies: the PLL2 registers are the same in both tables, and the
measured frame rate is 48000000 / (928 x VTS) with either link. On a
Surface Book 2 at 319.2 MHz, VTS 1724 gives 30.0 fps; 63.84 MHz would
give 39.9 fps. A Surface Pro 7+ measures 30.01, 60.16 and 90.45 fps at
VTS 1724, 860 and 572, the same 48 MHz. Dave Stevenson reported the
same on a Raspberry Pi with a 24 MHz clock, where libcamera asked for
100 fps and got 75, the ratio of the two rates.

Report 48 MHz for both link frequencies and drop the table.

Fixes: ed9566ce1946 ("media: i2c: Add support for new frequencies to ov7251")
Cc: stable@vger.kernel.org
Reported-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Closes: https://lore.kernel.org/linux-media/CAPY8ntBFJJPg3y46gC=bNq_55QifC7mKmeq8+pAEDVN42fGUXw@mail.gmail.com/
Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 1f30a01c85b3..65f3fc2d5889 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -18,10 +18,14 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/units.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
+/* PLL2 sys_clk: the pixel array clock at both link frequencies */
+#define OV7251_PIXEL_CLOCK_HZ	(48 * HZ_PER_MHZ)
+
 #define OV7251_SC_MODE_SELECT		0x0100
 #define OV7251_SC_MODE_SELECT_SW_STANDBY	0x0
 #define OV7251_SC_MODE_SELECT_STREAMING		0x1
@@ -633,11 +637,6 @@ static const s64 link_freq[] = {
 	[OV7251_LINK_FREQ_319_2_MHZ] = 319200000,
 };
 
-static const s64 pixel_rates[] = {
-	[OV7251_LINK_FREQ_240_MHZ] = 48000000,
-	[OV7251_LINK_FREQ_319_2_MHZ] = 63840000,
-};
-
 static const struct ov7251_mode_info ov7251_mode_info_data[] = {
 	{
 		.width = 640,
@@ -1567,7 +1566,6 @@ static int ov7251_detect_chip(struct ov7251 *ov7251)
 static int ov7251_init_ctrls(struct ov7251 *ov7251)
 {
 	int vblank_max, vblank_def;
-	s64 pixel_rate;
 	int hblank;
 
 	v4l2_ctrl_handler_init(&ov7251->ctrls, 7);
@@ -1586,12 +1584,12 @@ static int ov7251_init_ctrls(struct ov7251 *ov7251)
 				     ARRAY_SIZE(ov7251_test_pattern_menu) - 1,
 				     0, 0, ov7251_test_pattern_menu);
 
-	pixel_rate = pixel_rates[ov7251->link_freq_idx];
 	ov7251->pixel_clock = v4l2_ctrl_new_std(&ov7251->ctrls,
 						&ov7251_ctrl_ops,
 						V4L2_CID_PIXEL_RATE,
-						pixel_rate, INT_MAX,
-						pixel_rate, pixel_rate);
+						OV7251_PIXEL_CLOCK_HZ, INT_MAX,
+						OV7251_PIXEL_CLOCK_HZ,
+						OV7251_PIXEL_CLOCK_HZ);
 	ov7251->link_freq = v4l2_ctrl_new_int_menu(&ov7251->ctrls,
 						   &ov7251_ctrl_ops,
 						   V4L2_CID_LINK_FREQ,
-- 
2.56.0

[PATCH v3 4/9] media: i2c: ov7251: Reset vblank when the frame interval changes

[Antoine Pichot]

Setting a 60 or 90 fps frame interval is accepted and read back, but
the sensor keeps streaming at 30 fps.

The three modes differ only in their frame length (VTS: 1724, 860 and
572 lines), which each mode's register table writes to 0x380e/0x380f.
Since the V4L2_CID_VBLANK control was added, s_stream() applies the
controls with __v4l2_ctrl_handler_setup() after the mode table, so the
VBLANK control rewrites VTS. ov7251_set_frame_interval() switches
current_mode but never updates the VBLANK control, which keeps the 30
fps value (1244, VTS 1724) and overrides the 60 or 90 fps VTS at every
stream start.

ov7251_set_format() has the same issue in the other direction: it
updates the VBLANK range and default for the new mode, but
__v4l2_ctrl_modify_range() keeps the current value when it is still
in range, so after a 60 fps frame interval a format change would go
back to the 30 fps mode with a 60 fps VTS.

Update the VBLANK range and default in ov7251_set_frame_interval() too,
and set VBLANK to the mode's default in both places, as is already
done for exposure and gain. The VBLANK handler then also restores the
exposure range for the new frame length.

Tested on a Microsoft Surface Book 2 (IPU3 CIO2): after
v4l2-ctl --set-subdev-fps pad=0,fps=60 (90), v4l2-ctl measures 60.15
(90.45) fps instead of 30.01, with vblank 380 (92).

Fixes: 1b598f413c7a ("media: i2c: Add vblank control to ov7251 driver")
Cc: stable@vger.kernel.org
Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
Tested-by: D. Manresa <dmanresa@gmail.com> # Surface Pro 7+, IPU6 (8086:9a19)
---
 drivers/media/i2c/ov7251.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 65f3fc2d5889..81520833120a 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1260,6 +1260,10 @@ static int ov7251_set_format(struct v4l2_subdev *sd,
 		if (ret < 0)
 			goto exit;
 
+		ret = __v4l2_ctrl_s_ctrl(ov7251->vblank, vblank_def);
+		if (ret < 0)
+			goto exit;
+
 		ov7251->current_mode = new_mode;
 	}
 
@@ -1413,6 +1417,7 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 {
 	struct ov7251 *ov7251 = to_ov7251(subdev);
 	const struct ov7251_mode_info *new_mode;
+	int vblank_def;
 	int ret = 0;
 
 	/*
@@ -1441,6 +1446,19 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 		if (ret < 0)
 			goto exit;
 
+		vblank_def = new_mode->vts - new_mode->height;
+		ret = __v4l2_ctrl_modify_range(ov7251->vblank,
+					       OV7251_TIMING_MIN_VTS,
+					       OV7251_TIMING_MAX_VTS -
+					       new_mode->height,
+					       1, vblank_def);
+		if (ret < 0)
+			goto exit;
+
+		ret = __v4l2_ctrl_s_ctrl(ov7251->vblank, vblank_def);
+		if (ret < 0)
+			goto exit;
+
 		ov7251->current_mode = new_mode;
 	}
 
-- 
2.56.0

[PATCH v3 5/9] media: i2c: ov7251: Use sub-device active state

[Antoine Pichot]

Port the ov7251 driver to the sub-device active state.

The driver kept its own copies of the active format and crop rectangle
and selected between them and the try state with a pair of helpers
switching on the format's "which" field. Move the format and the crop
rectangle to the sub-device state for both the try and the active case,
drop the helpers and the private copies, and use the v4l2_subdev_get_fmt
helper for get_fmt. The init_state operation now fills whichever state
it is given through set_fmt, so the explicit ov7251_init_state() call at
the end of probe goes away; the exposure control gets the default mode's
range at creation instead of being adjusted by that call.

The driver mutex went away with it. The control handler's own lock now
serves as the state lock as well, so the control framework and the
sub-device core serialise the pad operations, the control operations
and the stream start between them; s_stream takes the active state lock
itself. The mode pointer, which the frame interval operations still
select by frame rate, stays and is protected by the same lock.

Behaviour is unchanged for applications. As a side effect the core now
reports V4L2_SUBDEV_FL_HAS_EVENTS for the sub-device and handles control
event subscription, so v4l2-compliance's VIDIOC_(UN)SUBSCRIBE_EVENT test
passes without the driver having to declare the event operations.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Link: https://lore.kernel.org/linux-media/at863XdUDea7JDA8@kekkonen.localdomain/
Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 154 +++++++++++--------------------------
 1 file changed, 43 insertions(+), 111 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 81520833120a..c7489c6305aa 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -131,8 +131,6 @@ struct ov7251 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct v4l2_fwnode_endpoint ep;
-	struct v4l2_mbus_framefmt fmt;
-	struct v4l2_rect crop;
 	struct clk *xclk;
 	u32 xclk_freq;
 
@@ -158,7 +156,6 @@ struct ov7251 {
 	u8 timing_format1;
 	u8 timing_format2;
 
-	struct mutex lock; /* lock to protect power state, ctrls and mode */
 	bool power_on;
 
 	struct gpio_desc *enable_gpio;
@@ -1042,8 +1039,6 @@ static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
 					     exposure_max));
 	}
 
-	/* v4l2_ctrl_lock() locks our mutex */
-
 	if (!pm_runtime_get_if_in_use(ov7251->dev))
 		return 0;
 
@@ -1131,52 +1126,6 @@ static int ov7251_enum_frame_ival(struct v4l2_subdev *subdev,
 	return -EINVAL;
 }
 
-static struct v4l2_mbus_framefmt *
-__ov7251_get_pad_format(struct ov7251 *ov7251,
-			struct v4l2_subdev_state *sd_state,
-			unsigned int pad,
-			enum v4l2_subdev_format_whence which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_state_get_format(sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &ov7251->fmt;
-	default:
-		return NULL;
-	}
-}
-
-static int ov7251_get_format(struct v4l2_subdev *sd,
-			     struct v4l2_subdev_state *sd_state,
-			     struct v4l2_subdev_format *format)
-{
-	struct ov7251 *ov7251 = to_ov7251(sd);
-
-	mutex_lock(&ov7251->lock);
-	format->format = *__ov7251_get_pad_format(ov7251, sd_state,
-						  format->pad,
-						  format->which);
-	mutex_unlock(&ov7251->lock);
-
-	return 0;
-}
-
-static struct v4l2_rect *
-__ov7251_get_pad_crop(struct ov7251 *ov7251,
-		      struct v4l2_subdev_state *sd_state,
-		      unsigned int pad, enum v4l2_subdev_format_whence which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_state_get_crop(sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &ov7251->crop;
-	default:
-		return NULL;
-	}
-}
-
 static inline u32 avg_fps(const struct v4l2_fract *t)
 {
 	return (t->denominator + (t->numerator >> 1)) / t->numerator;
@@ -1221,12 +1170,9 @@ static int ov7251_set_format(struct v4l2_subdev *sd,
 	int vblank_max, vblank_def;
 	struct v4l2_rect *__crop;
 	const struct ov7251_mode_info *new_mode;
-	int ret = 0;
-
-	mutex_lock(&ov7251->lock);
+	int ret;
 
-	__crop = __ov7251_get_pad_crop(ov7251, sd_state, format->pad,
-				       format->which);
+	__crop = v4l2_subdev_state_get_crop(sd_state, format->pad);
 
 	new_mode = v4l2_find_nearest_size(ov7251_mode_info_data,
 				ARRAY_SIZE(ov7251_mode_info_data),
@@ -1241,16 +1187,16 @@ static int ov7251_set_format(struct v4l2_subdev *sd,
 					       1, new_mode->exposure_max,
 					       1, new_mode->exposure_def);
 		if (ret < 0)
-			goto exit;
+			return ret;
 
 		ret = __v4l2_ctrl_s_ctrl(ov7251->exposure,
 					 new_mode->exposure_def);
 		if (ret < 0)
-			goto exit;
+			return ret;
 
 		ret = __v4l2_ctrl_s_ctrl(ov7251->gain, 16);
 		if (ret < 0)
-			goto exit;
+			return ret;
 
 		vblank_max = OV7251_TIMING_MAX_VTS - new_mode->height;
 		vblank_def = new_mode->vts - new_mode->height;
@@ -1258,17 +1204,16 @@ static int ov7251_set_format(struct v4l2_subdev *sd,
 					       OV7251_TIMING_MIN_VTS,
 					       vblank_max, 1, vblank_def);
 		if (ret < 0)
-			goto exit;
+			return ret;
 
 		ret = __v4l2_ctrl_s_ctrl(ov7251->vblank, vblank_def);
 		if (ret < 0)
-			goto exit;
+			return ret;
 
 		ov7251->current_mode = new_mode;
 	}
 
-	__format = __ov7251_get_pad_format(ov7251, sd_state, format->pad,
-					   format->which);
+	__format = v4l2_subdev_state_get_format(sd_state, format->pad);
 	__format->width = __crop->width;
 	__format->height = __crop->height;
 	__format->code = MEDIA_BUS_FMT_Y10_1X10;
@@ -1281,27 +1226,21 @@ static int ov7251_set_format(struct v4l2_subdev *sd,
 
 	format->format = *__format;
 
-exit:
-	mutex_unlock(&ov7251->lock);
-
-	return ret;
+	return 0;
 }
 
 static int ov7251_init_state(struct v4l2_subdev *subdev,
 			     struct v4l2_subdev_state *sd_state)
 {
 	struct v4l2_subdev_format fmt = {
-		.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
-		: V4L2_SUBDEV_FORMAT_ACTIVE,
+		.which = V4L2_SUBDEV_FORMAT_TRY,
 		.format = {
 			.width = 640,
 			.height = 480
 		}
 	};
 
-	ov7251_set_format(subdev, NULL, sd_state, &fmt);
-
-	return 0;
+	return ov7251_set_format(subdev, NULL, sd_state, &fmt);
 }
 
 static int ov7251_get_selection(struct v4l2_subdev *sd,
@@ -1309,15 +1248,10 @@ static int ov7251_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *sd_state,
 				struct v4l2_subdev_selection *sel)
 {
-	struct ov7251 *ov7251 = to_ov7251(sd);
-
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP_DEFAULT:
 	case V4L2_SEL_TGT_CROP:
-	mutex_lock(&ov7251->lock);
-		sel->r = *__ov7251_get_pad_crop(ov7251, sd_state, sel->pad,
-						sel->which);
-		mutex_unlock(&ov7251->lock);
+		sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
 		break;
 	case V4L2_SEL_TGT_NATIVE_SIZE:
 		sel->r.top = 0;
@@ -1341,14 +1275,15 @@ static int ov7251_get_selection(struct v4l2_subdev *sd,
 static int ov7251_s_stream(struct v4l2_subdev *subdev, int enable)
 {
 	struct ov7251 *ov7251 = to_ov7251(subdev);
+	struct v4l2_subdev_state *state;
 	int ret;
 
-	mutex_lock(&ov7251->lock);
+	state = v4l2_subdev_lock_and_get_active_state(subdev);
 
 	if (enable) {
 		ret = pm_runtime_resume_and_get(ov7251->dev);
 		if (ret) {
-			mutex_unlock(&ov7251->lock);
+			v4l2_subdev_unlock_state(state);
 			return ret;
 		}
 
@@ -1382,12 +1317,12 @@ static int ov7251_s_stream(struct v4l2_subdev *subdev, int enable)
 		pm_runtime_put(ov7251->dev);
 	}
 
-	mutex_unlock(&ov7251->lock);
+	v4l2_subdev_unlock_state(state);
 	return ret;
 
 err_power_down:
 	pm_runtime_put(ov7251->dev);
-	mutex_unlock(&ov7251->lock);
+	v4l2_subdev_unlock_state(state);
 	return ret;
 }
 
@@ -1404,9 +1339,7 @@ static int ov7251_get_frame_interval(struct v4l2_subdev *subdev,
 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
 		return -EINVAL;
 
-	mutex_lock(&ov7251->lock);
 	fi->interval = ov7251->current_mode->timeperframe;
-	mutex_unlock(&ov7251->lock);
 
 	return 0;
 }
@@ -1418,7 +1351,7 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 	struct ov7251 *ov7251 = to_ov7251(subdev);
 	const struct ov7251_mode_info *new_mode;
 	int vblank_def;
-	int ret = 0;
+	int ret;
 
 	/*
 	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
@@ -1427,7 +1360,6 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
 		return -EINVAL;
 
-	mutex_lock(&ov7251->lock);
 	new_mode = ov7251_find_mode_by_ival(ov7251, &fi->interval);
 
 	if (new_mode != ov7251->current_mode) {
@@ -1435,16 +1367,16 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 					       1, new_mode->exposure_max,
 					       1, new_mode->exposure_def);
 		if (ret < 0)
-			goto exit;
+			return ret;
 
 		ret = __v4l2_ctrl_s_ctrl(ov7251->exposure,
 					 new_mode->exposure_def);
 		if (ret < 0)
-			goto exit;
+			return ret;
 
 		ret = __v4l2_ctrl_s_ctrl(ov7251->gain, 16);
 		if (ret < 0)
-			goto exit;
+			return ret;
 
 		vblank_def = new_mode->vts - new_mode->height;
 		ret = __v4l2_ctrl_modify_range(ov7251->vblank,
@@ -1453,21 +1385,18 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 					       new_mode->height,
 					       1, vblank_def);
 		if (ret < 0)
-			goto exit;
+			return ret;
 
 		ret = __v4l2_ctrl_s_ctrl(ov7251->vblank, vblank_def);
 		if (ret < 0)
-			goto exit;
+			return ret;
 
 		ov7251->current_mode = new_mode;
 	}
 
 	fi->interval = ov7251->current_mode->timeperframe;
 
-exit:
-	mutex_unlock(&ov7251->lock);
-
-	return ret;
+	return 0;
 }
 
 static const struct v4l2_subdev_video_ops ov7251_video_ops = {
@@ -1478,7 +1407,7 @@ static const struct v4l2_subdev_pad_ops ov7251_subdev_pad_ops = {
 	.enum_mbus_code = ov7251_enum_mbus_code,
 	.enum_frame_size = ov7251_enum_frame_size,
 	.enum_frame_interval = ov7251_enum_frame_ival,
-	.get_fmt = ov7251_get_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = ov7251_set_format,
 	.get_selection = ov7251_get_selection,
 	.get_frame_interval = ov7251_get_frame_interval,
@@ -1583,18 +1512,20 @@ static int ov7251_detect_chip(struct ov7251 *ov7251)
 
 static int ov7251_init_ctrls(struct ov7251 *ov7251)
 {
+	const struct ov7251_mode_info *mode = ov7251->current_mode;
 	int vblank_max, vblank_def;
 	int hblank;
 
 	v4l2_ctrl_handler_init(&ov7251->ctrls, 7);
-	ov7251->ctrls.lock = &ov7251->lock;
 
 	v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
 			  V4L2_CID_HFLIP, 0, 1, 1, 0);
 	v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
 			  V4L2_CID_VFLIP, 0, 1, 1, 0);
 	ov7251->exposure = v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
-					     V4L2_CID_EXPOSURE, 1, 32, 1, 32);
+					     V4L2_CID_EXPOSURE, 1,
+					     mode->exposure_max, 1,
+					     mode->exposure_def);
 	ov7251->gain = v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
 					 V4L2_CID_GAIN, 16, 1023, 1, 16);
 	v4l2_ctrl_new_std_menu_items(&ov7251->ctrls, &ov7251_ctrl_ops,
@@ -1704,14 +1635,10 @@ static int ov7251_probe(struct i2c_client *client)
 		return PTR_ERR(ov7251->enable_gpio);
 	}
 
-	mutex_init(&ov7251->lock);
-
 	ov7251->current_mode = &ov7251_mode_info_data[0];
 	ret = ov7251_init_ctrls(ov7251);
-	if (ret) {
-		dev_err_probe(dev, ret, "error during v4l2 ctrl init\n");
-		goto destroy_mutex;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "error during v4l2 ctrl init\n");
 
 	v4l2_i2c_subdev_init(&ov7251->sd, client, &ov7251_subdev_ops);
 	ov7251->sd.internal_ops = &ov7251_internal_ops;
@@ -1726,9 +1653,16 @@ static int ov7251_probe(struct i2c_client *client)
 		goto free_ctrl;
 	}
 
+	ov7251->sd.state_lock = ov7251->ctrls.lock;
+	ret = v4l2_subdev_init_finalize(&ov7251->sd);
+	if (ret < 0) {
+		dev_err(dev, "could not initialize subdev state\n");
+		goto free_entity;
+	}
+
 	ret = ov7251_set_power_on(ov7251->dev);
 	if (ret)
-		goto free_entity;
+		goto subdev_cleanup;
 
 	ret = ov7251_detect_chip(ov7251);
 	if (ret)
@@ -1769,11 +1703,9 @@ static int ov7251_probe(struct i2c_client *client)
 	ret = v4l2_async_register_subdev(&ov7251->sd);
 	if (ret < 0) {
 		dev_err(dev, "could not register v4l2 device\n");
-		goto free_entity;
+		goto subdev_cleanup;
 	}
 
-	ov7251_init_state(&ov7251->sd, NULL);
-
 	return 0;
 
 err_pm_runtime:
@@ -1781,12 +1713,12 @@ static int ov7251_probe(struct i2c_client *client)
 	pm_runtime_put_noidle(ov7251->dev);
 power_down:
 	ov7251_set_power_off(ov7251->dev);
+subdev_cleanup:
+	v4l2_subdev_cleanup(&ov7251->sd);
 free_entity:
 	media_entity_cleanup(&ov7251->sd.entity);
 free_ctrl:
 	v4l2_ctrl_handler_free(&ov7251->ctrls);
-destroy_mutex:
-	mutex_destroy(&ov7251->lock);
 
 	return ret;
 }
@@ -1797,9 +1729,9 @@ static void ov7251_remove(struct i2c_client *client)
 	struct ov7251 *ov7251 = to_ov7251(sd);
 
 	v4l2_async_unregister_subdev(&ov7251->sd);
+	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&ov7251->sd.entity);
 	v4l2_ctrl_handler_free(&ov7251->ctrls);
-	mutex_destroy(&ov7251->lock);
 
 	pm_runtime_disable(ov7251->dev);
 	if (!pm_runtime_status_suspended(ov7251->dev))
-- 
2.56.0

[PATCH v3 6/9] media: i2c: ov7251: Add orientation and rotation controls

[Antoine Pichot]

Parse the firmware device properties and register the
V4L2_CID_CAMERA_ORIENTATION and V4L2_CID_CAMERA_SENSOR_ROTATION
controls, as documented in
Documentation/driver-api/media/camera-sensor.rst. libcamera uses them to
place the camera and to correct the image orientation.

While at it, bump the control handler size hint to the number of
controls the driver now creates.

On the Microsoft Surface Book 2 the ACPI _PLD (via ipu-bridge) reports
the IR sensor as front facing with no rotation.

Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
Tested-by: D. Manresa <dmanresa@gmail.com> # Surface Pro 7+, IPU6 (8086:9a19)
---
 drivers/media/i2c/ov7251.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index c7489c6305aa..834420f2f843 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1513,10 +1513,16 @@ static int ov7251_detect_chip(struct ov7251 *ov7251)
 static int ov7251_init_ctrls(struct ov7251 *ov7251)
 {
 	const struct ov7251_mode_info *mode = ov7251->current_mode;
+	struct v4l2_fwnode_device_properties props;
 	int vblank_max, vblank_def;
 	int hblank;
+	int ret;
 
-	v4l2_ctrl_handler_init(&ov7251->ctrls, 7);
+	ret = v4l2_fwnode_device_parse(ov7251->dev, &props);
+	if (ret)
+		return ret;
+
+	v4l2_ctrl_handler_init(&ov7251->ctrls, 11);
 
 	v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
 			  V4L2_CID_HFLIP, 0, 1, 1, 0);
@@ -1564,6 +1570,9 @@ static int ov7251_init_ctrls(struct ov7251 *ov7251)
 					   OV7251_TIMING_MIN_VTS, vblank_max, 1,
 					   vblank_def);
 
+	v4l2_ctrl_new_fwnode_properties(&ov7251->ctrls, &ov7251_ctrl_ops,
+					&props);
+
 	ov7251->sd.ctrl_handler = &ov7251->ctrls;
 
 	if (ov7251->ctrls.error) {
-- 
2.56.0

[PATCH v3 7/9] media: i2c: ov7251: Move the common registers into the global init table

[Antoine Pichot]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

The three mode tables write the same 117 registers. Only the six clock
control registers 0x3016 to 0x301c differ between the 30 fps table and
the 60 and 90 fps ones, and the frame length (0x380e and 0x380f), which
the V4L2_CID_VBLANK handler writes anyway.

Move the common registers into ov7251_global_init_setting, written after
the software reset by ov7251_set_power_on(), and drop the frame length
from the mode tables, which keep the clock control registers only.

The common registers are thus written once per power-on instead of at
every stream start. The sensor keeps its register values in software
standby (datasheet table 2-8), so a stream restarted, or switched to
another mode, before the runtime PM autosuspend powers the sensor down
gets the same settings as a stream started from power-on.

One value on the bus changes: 0x3820 (TIMING_FORMAT1). The mode tables
wrote 0x40 (bit 6, vflip_blc) at stream start, but the vertical flip
control, applied after the table, rewrote the register from the value
probe had read right after the reset, 0x00, so the sensor streamed with
bit 6 clear. Probe now reads 0x40 from the init table and the control
keeps bit 6 set, as the mode tables have always asked for.

Tested on a Surface Book 2 (19.2 MHz clock, 319.2 MHz link): the clock,
timing, PLL, flip and strobe registers read back while streaming at
30, 60 and 90 fps are identical to those of the driver before this
change, except 0x3820, which reads 0x40 instead of 0x00 as described
above; stream restarts and mode switches on a sensor kept in software
standby give valid frames from the first one, at the expected rates.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
[antoine.pichot@gmail.com: subject and commit message, 0x3820 note]
Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 258 ++-----------------------------------
 1 file changed, 9 insertions(+), 249 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 834420f2f843..2abf1b112351 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -238,19 +238,10 @@ static const struct ov7251_pll_cfgs *ov7251_pll_cfgs[] = {
 static const struct reg_value ov7251_global_init_setting[] = {
 	{ 0x0103, 0x01 },
 	{ 0x303b, 0x02 },
-};
-
-static const struct reg_value ov7251_setting_vga_30fps[] = {
 	{ 0x3005, 0x00 },
 	{ 0x3012, 0xc0 },
 	{ 0x3013, 0xd2 },
 	{ 0x3014, 0x04 },
-	{ 0x3016, 0xf0 },
-	{ 0x3017, 0xf0 },
-	{ 0x3018, 0xf0 },
-	{ 0x301a, 0xf0 },
-	{ 0x301b, 0xf0 },
-	{ 0x301c, 0xf0 },
 	{ 0x3023, 0x05 },
 	{ 0x3037, 0xf0 },
 	{ 0x3106, 0xda },
@@ -298,8 +289,6 @@ static const struct reg_value ov7251_setting_vga_30fps[] = {
 	{ 0x380b, 0xe0 }, /* height low */
 	{ 0x380c, 0x03 }, /* total horiz timing high */
 	{ 0x380d, 0xa0 }, /* total horiz timing low */
-	{ 0x380e, 0x06 }, /* total vertical timing high */
-	{ 0x380f, 0xbc }, /* total vertical timing low */
 	{ 0x3810, 0x00 },
 	{ 0x3811, 0x04 },
 	{ 0x3812, 0x00 },
@@ -368,260 +357,31 @@ static const struct reg_value ov7251_setting_vga_30fps[] = {
 	{ 0x5001, 0x80 },
 };
 
+static const struct reg_value ov7251_setting_vga_30fps[] = {
+	{ 0x3016, 0xf0 },
+	{ 0x3017, 0xf0 },
+	{ 0x3018, 0xf0 },
+	{ 0x301a, 0xf0 },
+	{ 0x301b, 0xf0 },
+	{ 0x301c, 0xf0 },
+};
+
 static const struct reg_value ov7251_setting_vga_60fps[] = {
-	{ 0x3005, 0x00 },
-	{ 0x3012, 0xc0 },
-	{ 0x3013, 0xd2 },
-	{ 0x3014, 0x04 },
 	{ 0x3016, 0x10 },
 	{ 0x3017, 0x00 },
 	{ 0x3018, 0x00 },
 	{ 0x301a, 0x00 },
 	{ 0x301b, 0x00 },
 	{ 0x301c, 0x00 },
-	{ 0x3023, 0x05 },
-	{ 0x3037, 0xf0 },
-	{ 0x3106, 0xda },
-	{ 0x3503, 0x07 },
-	{ 0x3509, 0x10 },
-	{ 0x3600, 0x1c },
-	{ 0x3602, 0x62 },
-	{ 0x3620, 0xb7 },
-	{ 0x3622, 0x04 },
-	{ 0x3626, 0x21 },
-	{ 0x3627, 0x30 },
-	{ 0x3630, 0x44 },
-	{ 0x3631, 0x35 },
-	{ 0x3634, 0x60 },
-	{ 0x3636, 0x00 },
-	{ 0x3662, 0x01 },
-	{ 0x3663, 0x70 },
-	{ 0x3664, 0x50 },
-	{ 0x3666, 0x0a },
-	{ 0x3669, 0x1a },
-	{ 0x366a, 0x00 },
-	{ 0x366b, 0x50 },
-	{ 0x3673, 0x01 },
-	{ 0x3674, 0xff },
-	{ 0x3675, 0x03 },
-	{ 0x3705, 0xc1 },
-	{ 0x3709, 0x40 },
-	{ 0x373c, 0x08 },
-	{ 0x3742, 0x00 },
-	{ 0x3757, 0xb3 },
-	{ 0x3788, 0x00 },
-	{ 0x37a8, 0x01 },
-	{ 0x37a9, 0xc0 },
-	{ 0x3800, 0x00 },
-	{ 0x3801, 0x04 },
-	{ 0x3802, 0x00 },
-	{ 0x3803, 0x04 },
-	{ 0x3804, 0x02 },
-	{ 0x3805, 0x8b },
-	{ 0x3806, 0x01 },
-	{ 0x3807, 0xeb },
-	{ 0x3808, 0x02 }, /* width high */
-	{ 0x3809, 0x80 }, /* width low */
-	{ 0x380a, 0x01 }, /* height high */
-	{ 0x380b, 0xe0 }, /* height low */
-	{ 0x380c, 0x03 }, /* total horiz timing high */
-	{ 0x380d, 0xa0 }, /* total horiz timing low */
-	{ 0x380e, 0x03 }, /* total vertical timing high */
-	{ 0x380f, 0x5c }, /* total vertical timing low */
-	{ 0x3810, 0x00 },
-	{ 0x3811, 0x04 },
-	{ 0x3812, 0x00 },
-	{ 0x3813, 0x05 },
-	{ 0x3814, 0x11 },
-	{ 0x3815, 0x11 },
-	{ 0x3820, 0x40 },
-	{ 0x3821, 0x00 },
-	{ 0x382f, 0x0e },
-	{ 0x3832, 0x00 },
-	{ 0x3833, 0x05 },
-	{ 0x3834, 0x00 },
-	{ 0x3835, 0x0c },
-	{ 0x3837, 0x00 },
-	{ 0x3b80, 0x00 },
-	{ 0x3b81, 0xa5 },
-	{ 0x3b82, 0x10 },
-	{ 0x3b83, 0x00 },
-	{ 0x3b84, 0x08 },
-	{ 0x3b85, 0x00 },
-	{ 0x3b86, 0x01 },
-	{ 0x3b87, 0x00 },
-	{ 0x3b88, 0x00 },
-	{ 0x3b89, 0x00 },
-	{ 0x3b8a, 0x00 },
-	{ 0x3b8b, 0x05 },
-	{ 0x3b8c, 0x00 },
-	{ 0x3b8d, 0x00 },
-	{ 0x3b8e, 0x00 },
-	{ 0x3b8f, 0x1a },
-	{ 0x3b94, 0x05 },
-	{ 0x3b95, 0xf2 },
-	{ 0x3b96, 0x40 },
-	{ 0x3c00, 0x89 },
-	{ 0x3c01, 0x63 },
-	{ 0x3c02, 0x01 },
-	{ 0x3c03, 0x00 },
-	{ 0x3c04, 0x00 },
-	{ 0x3c05, 0x03 },
-	{ 0x3c06, 0x00 },
-	{ 0x3c07, 0x06 },
-	{ 0x3c0c, 0x01 },
-	{ 0x3c0d, 0xd0 },
-	{ 0x3c0e, 0x02 },
-	{ 0x3c0f, 0x0a },
-	{ 0x4001, 0x42 },
-	{ 0x4004, 0x04 },
-	{ 0x4005, 0x00 },
-	{ 0x404e, 0x01 },
-	{ 0x4300, 0xff },
-	{ 0x4301, 0x00 },
-	{ 0x4315, 0x00 },
-	{ 0x4501, 0x48 },
-	{ 0x4600, 0x00 },
-	{ 0x4601, 0x4e },
-	{ 0x4801, 0x0f },
-	{ 0x4806, 0x0f },
-	{ 0x4819, 0xaa },
-	{ 0x4823, 0x3e },
-	{ 0x4837, 0x19 },
-	{ 0x4a0d, 0x00 },
-	{ 0x4a47, 0x7f },
-	{ 0x4a49, 0xf0 },
-	{ 0x4a4b, 0x30 },
-	{ 0x5000, 0x85 },
-	{ 0x5001, 0x80 },
 };
 
 static const struct reg_value ov7251_setting_vga_90fps[] = {
-	{ 0x3005, 0x00 },
-	{ 0x3012, 0xc0 },
-	{ 0x3013, 0xd2 },
-	{ 0x3014, 0x04 },
 	{ 0x3016, 0x10 },
 	{ 0x3017, 0x00 },
 	{ 0x3018, 0x00 },
 	{ 0x301a, 0x00 },
 	{ 0x301b, 0x00 },
 	{ 0x301c, 0x00 },
-	{ 0x3023, 0x05 },
-	{ 0x3037, 0xf0 },
-	{ 0x3106, 0xda },
-	{ 0x3503, 0x07 },
-	{ 0x3509, 0x10 },
-	{ 0x3600, 0x1c },
-	{ 0x3602, 0x62 },
-	{ 0x3620, 0xb7 },
-	{ 0x3622, 0x04 },
-	{ 0x3626, 0x21 },
-	{ 0x3627, 0x30 },
-	{ 0x3630, 0x44 },
-	{ 0x3631, 0x35 },
-	{ 0x3634, 0x60 },
-	{ 0x3636, 0x00 },
-	{ 0x3662, 0x01 },
-	{ 0x3663, 0x70 },
-	{ 0x3664, 0x50 },
-	{ 0x3666, 0x0a },
-	{ 0x3669, 0x1a },
-	{ 0x366a, 0x00 },
-	{ 0x366b, 0x50 },
-	{ 0x3673, 0x01 },
-	{ 0x3674, 0xff },
-	{ 0x3675, 0x03 },
-	{ 0x3705, 0xc1 },
-	{ 0x3709, 0x40 },
-	{ 0x373c, 0x08 },
-	{ 0x3742, 0x00 },
-	{ 0x3757, 0xb3 },
-	{ 0x3788, 0x00 },
-	{ 0x37a8, 0x01 },
-	{ 0x37a9, 0xc0 },
-	{ 0x3800, 0x00 },
-	{ 0x3801, 0x04 },
-	{ 0x3802, 0x00 },
-	{ 0x3803, 0x04 },
-	{ 0x3804, 0x02 },
-	{ 0x3805, 0x8b },
-	{ 0x3806, 0x01 },
-	{ 0x3807, 0xeb },
-	{ 0x3808, 0x02 }, /* width high */
-	{ 0x3809, 0x80 }, /* width low */
-	{ 0x380a, 0x01 }, /* height high */
-	{ 0x380b, 0xe0 }, /* height low */
-	{ 0x380c, 0x03 }, /* total horiz timing high */
-	{ 0x380d, 0xa0 }, /* total horiz timing low */
-	{ 0x380e, 0x02 }, /* total vertical timing high */
-	{ 0x380f, 0x3c }, /* total vertical timing low */
-	{ 0x3810, 0x00 },
-	{ 0x3811, 0x04 },
-	{ 0x3812, 0x00 },
-	{ 0x3813, 0x05 },
-	{ 0x3814, 0x11 },
-	{ 0x3815, 0x11 },
-	{ 0x3820, 0x40 },
-	{ 0x3821, 0x00 },
-	{ 0x382f, 0x0e },
-	{ 0x3832, 0x00 },
-	{ 0x3833, 0x05 },
-	{ 0x3834, 0x00 },
-	{ 0x3835, 0x0c },
-	{ 0x3837, 0x00 },
-	{ 0x3b80, 0x00 },
-	{ 0x3b81, 0xa5 },
-	{ 0x3b82, 0x10 },
-	{ 0x3b83, 0x00 },
-	{ 0x3b84, 0x08 },
-	{ 0x3b85, 0x00 },
-	{ 0x3b86, 0x01 },
-	{ 0x3b87, 0x00 },
-	{ 0x3b88, 0x00 },
-	{ 0x3b89, 0x00 },
-	{ 0x3b8a, 0x00 },
-	{ 0x3b8b, 0x05 },
-	{ 0x3b8c, 0x00 },
-	{ 0x3b8d, 0x00 },
-	{ 0x3b8e, 0x00 },
-	{ 0x3b8f, 0x1a },
-	{ 0x3b94, 0x05 },
-	{ 0x3b95, 0xf2 },
-	{ 0x3b96, 0x40 },
-	{ 0x3c00, 0x89 },
-	{ 0x3c01, 0x63 },
-	{ 0x3c02, 0x01 },
-	{ 0x3c03, 0x00 },
-	{ 0x3c04, 0x00 },
-	{ 0x3c05, 0x03 },
-	{ 0x3c06, 0x00 },
-	{ 0x3c07, 0x06 },
-	{ 0x3c0c, 0x01 },
-	{ 0x3c0d, 0xd0 },
-	{ 0x3c0e, 0x02 },
-	{ 0x3c0f, 0x0a },
-	{ 0x4001, 0x42 },
-	{ 0x4004, 0x04 },
-	{ 0x4005, 0x00 },
-	{ 0x404e, 0x01 },
-	{ 0x4300, 0xff },
-	{ 0x4301, 0x00 },
-	{ 0x4315, 0x00 },
-	{ 0x4501, 0x48 },
-	{ 0x4600, 0x00 },
-	{ 0x4601, 0x4e },
-	{ 0x4801, 0x0f },
-	{ 0x4806, 0x0f },
-	{ 0x4819, 0xaa },
-	{ 0x4823, 0x3e },
-	{ 0x4837, 0x19 },
-	{ 0x4a0d, 0x00 },
-	{ 0x4a47, 0x7f },
-	{ 0x4a49, 0xf0 },
-	{ 0x4a4b, 0x30 },
-	{ 0x5000, 0x85 },
-	{ 0x5001, 0x80 },
 };
 
 static const unsigned long supported_xclk_rates[] = {
-- 
2.56.0

[PATCH v3 8/9] media: i2c: ov7251: Add strobe output enable control

[Antoine Pichot]

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 global init table leaves 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 of the global init table. 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. Face recognition uses the dark frames too: subtracting a dark
frame from the lit frame next to it removes the ambient light. Lighting
every frame (0xff, which two of the Windows driver's other modes use)
would lose that reference and 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 table.
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.

Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 76 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 2abf1b112351..7c1213d13ef4 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -18,6 +18,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/unaligned.h>
 #include <linux/units.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fwnode.h>
@@ -56,6 +57,16 @@
 #define OV7251_PLL2_DIVIDER_REG		0x309d
 #define OV7251_PLL2_SYS_DIV_REG		0x309a
 #define OV7251_PLL2_ADC_DIV_REG		0x309b
+#define OV7251_SC_REG5			0x3005
+#define OV7251_SC_REG5_STROBE_OE	BIT(3)
+#define OV7251_STROBE_FRAME_PATTERN	0x3b81
+#define OV7251_STROBE_FRAME_PATTERN_ALT	0xaa	/* alternate frames */
+#define OV7251_STROBE_FRAME_PATTERN_DEF	0xa5
+#define OV7251_STROBE_FRAME_SPAN	0x3b8c
+#define OV7251_STROBE_FRAME_SPAN_DEF	0x1a
+#define OV7251_STROBE_SPAN_MIN		52
+#define OV7251_STROBE_SPAN_MAX		776
+#define OV7251_STROBE_MAX_DUTY_PERCENT	45
 
 #define OV7251_NATIVE_WIDTH		656
 #define OV7251_NATIVE_HEIGHT		496
@@ -149,6 +160,7 @@ struct ov7251 {
 	struct v4l2_ctrl *gain;
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *strobe;
 
 	/* Cached register values */
 	u8 aec_pk_manual;
@@ -512,7 +524,7 @@ static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val)
 static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val,
 				 u8 num)
 {
-	u8 regbuf[5];
+	u8 regbuf[6];
 	u8 nregbuf = sizeof(reg) + num * sizeof(*val);
 	int ret = 0;
 
@@ -764,6 +776,55 @@ static int ov7251_set_test_pattern(struct ov7251 *ov7251, s32 value)
 	return ret;
 }
 
+static int ov7251_set_strobe_span(struct ov7251 *ov7251)
+{
+	int span = OV7251_STROBE_FRAME_SPAN_DEF;
+	int vts, max_span;
+	u8 val[4];
+
+	/*
+	 * The pulse starts with the integration: make it cover the exposure,
+	 * from 52 lines up to 776 lines and never more than 45% of the frame.
+	 * 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,
+					PERCENT);
+		span = min(clamp(ov7251->exposure->val, OV7251_STROBE_SPAN_MIN,
+				 OV7251_STROBE_SPAN_MAX), max_span);
+	}
+
+	put_unaligned_be32(span, val);
+
+	return ov7251_write_seq_regs(ov7251, OV7251_STROBE_FRAME_SPAN, val,
+				     sizeof(val));
+}
+
+static int ov7251_set_strobe(struct ov7251 *ov7251, s32 value)
+{
+	int ret;
+
+	/*
+	 * The other SC_REG5 bits enable other pads, which the mode tables
+	 * have always left disabled (0x00).
+	 */
+	ret = ov7251_write_reg(ov7251, OV7251_SC_REG5,
+			       value ? OV7251_SC_REG5_STROBE_OE : 0);
+	if (ret)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_STROBE_FRAME_PATTERN,
+			       value ? OV7251_STROBE_FRAME_PATTERN_ALT :
+				       OV7251_STROBE_FRAME_PATTERN_DEF);
+	if (ret)
+		return ret;
+
+	return ov7251_set_strobe_span(ov7251);
+}
+
 static const char * const ov7251_test_pattern_menu[] = {
 	"Disabled",
 	"Vertical Pattern Bars",
@@ -805,6 +866,8 @@ static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
 	switch (ctrl->id) {
 	case V4L2_CID_EXPOSURE:
 		ret = ov7251_set_exposure(ov7251, ctrl->val);
+		if (!ret && ov7251->strobe->val)
+			ret = ov7251_set_strobe_span(ov7251);
 		break;
 	case V4L2_CID_GAIN:
 		ret = ov7251_set_gain(ov7251, ctrl->val);
@@ -820,6 +883,11 @@ static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_VBLANK:
 		ret = ov7251_vts_configure(ov7251, ctrl->val);
+		if (!ret && ov7251->strobe->val)
+			ret = ov7251_set_strobe_span(ov7251);
+		break;
+	case V4L2_CID_FLASH_STROBE_OE:
+		ret = ov7251_set_strobe(ov7251, ctrl->val);
 		break;
 	default:
 		ret = -EINVAL;
@@ -1282,7 +1350,7 @@ static int ov7251_init_ctrls(struct ov7251 *ov7251)
 	if (ret)
 		return ret;
 
-	v4l2_ctrl_handler_init(&ov7251->ctrls, 11);
+	v4l2_ctrl_handler_init(&ov7251->ctrls, 12);
 
 	v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
 			  V4L2_CID_HFLIP, 0, 1, 1, 0);
@@ -1330,6 +1398,10 @@ static int ov7251_init_ctrls(struct ov7251 *ov7251)
 					   OV7251_TIMING_MIN_VTS, vblank_max, 1,
 					   vblank_def);
 
+	ov7251->strobe = v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
+					   V4L2_CID_FLASH_STROBE_OE,
+					   0, 1, 1, 0);
+
 	v4l2_ctrl_new_fwnode_properties(&ov7251->ctrls, &ov7251_ctrl_ops,
 					&props);
 
-- 
2.56.0

[PATCH v3 9/9] media: ipu-bridge: Add a rotation quirk for the Surface Book 2 and Pro 7+

[Antoine Pichot]

The infrared camera of the Microsoft Surface Book 2, an OmniVision
OV7251 with ACPI ID INT347E, is mounted rotated by 90 degrees. An
upright scene appears rotated 90 degrees clockwise in its frames, with
the top of the scene at the right edge of the image, while a frame
taken at the same moment by the colour camera next to it shows the
scene upright. The degree field of the sensor's SSDB buffer in the ACPI
tables is 0, and so is the rotation field of its _PLD, so
ipu_bridge_parse_rotation() sets the rotation property to 0 and
userspace has no way to know that the image needs correcting.

The firmware is wrong here, and ipu-bridge is where the firmware data
is turned into the rotation property: it already carries DMI (Desktop
Management Interface) quirks for sensors mounted upside down without
the firmware saying so. Add a second table for sensors rotated by 90
degrees, checked the same way, with entries for the Surface Book 2 and
Surface Pro 7+ infrared sensors. Following
V4L2_CID_CAMERA_SENSOR_ROTATION, 90 is the counter-clockwise rotation
to apply to correct the image. The Book 2 entry matches on the SKU of
the 13 inch model, the one checked; the 15 inch model has its own SKU
and has not been checked. The colour sensors of this machine (INT33BE
and INT347A) already report 180 degrees and are unchanged.

D. Manresa measured the same rotation on a Surface Pro 7+ (IPU6), whose
infrared sensor is also an INT347E: an upright scene comes out rotated
90 degrees clockwise, with both flip controls at 0 and the firmware
reporting a rotation of 0 [1], so the Pro 7+ gets an entry too.

Tested on a 13 inch Surface Book 2 (SKU Surface_Book_1832) with a
7.2.12 kernel: V4L2_CID_CAMERA_SENSOR_ROTATION on the OV7251 reads 90
instead of 0, the colour sensors still read 180 and the orientation
controls are unchanged.

Link: https://lore.kernel.org/linux-media/6ae0f151.0f9a9df6.1cace3.5239@mx.google.com/ [1]
Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 31 ++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 5e419c7b9287..cdc6c8fe30af 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -303,6 +303,32 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
 	{} /* Terminating entry */
 };
 
+/*
+ * DMI matches for laptops which have their sensor mounted rotated by 90 degrees
+ * without reporting it in the SSDB.
+ */
+static const struct dmi_system_id rotated_90_sensor_dmi_ids[] = {
+	{
+		/* Microsoft Surface Book 2 13 inch, infrared sensor */
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR,
+					"Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Book_1832"),
+		},
+		.driver_data = "INT347E",
+	},
+	{
+		/* Microsoft Surface Pro 7+, infrared sensor */
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR,
+					"Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_7+_1960"),
+		},
+		.driver_data = "INT347E",
+	},
+	{} /* Terminating entry */
+};
+
 static const struct ipu_property_names prop_names = {
 	.clock_frequency = "clock-frequency",
 	.rotation = "rotation",
@@ -496,6 +522,11 @@ static u32 ipu_bridge_parse_rotation(struct acpi_device *adev,
 		if (acpi_dev_hid_match(adev, dmi_id->driver_data))
 			return 180;
 
+	for (dmi_id = dmi_first_match(rotated_90_sensor_dmi_ids); dmi_id;
+	     dmi_id = dmi_first_match(dmi_id + 1))
+		if (acpi_dev_hid_match(adev, dmi_id->driver_data))
+			return 90;
+
 	switch (ssdb->degree) {
 	case IPU_SENSOR_ROTATION_NORMAL:
 		return 0;
-- 
2.56.0