[PATCH 0/3] media: Add support for the Sony IMX681

[PATCH 0/3] media: Add support for the Sony IMX681

[Sergey Lebedev]

The Sony IMX681 is the user-facing camera on the Microsoft Surface Pro 11 for
Business (Intel Lunar Lake, IPU7), enumerated as ACPI device SONY0681. Without
a driver the camera does not appear at all.

  1/3  dt-bindings: media: Add Sony IMX681            (mine)
  2/3  media: i2c: Add Sony IMX681 sensor driver      (Andre Gilerson's)
  3/3  media: ipu-bridge: Add Sony IMX681             (mine)

The driver is Andre's work, reverse-engineered from I2C traces taken under
Windows. I am carrying the submission, not the code: his Signed-off-by is on
2/3 with mine beneath it as the person passing it on. He is away until 28
September, so replies to review in the next few weeks will come from me. I have
the hardware and the instrumentation is scripted, so anything you want measured
I can measure.

Where the numbers come from
===========================

The parts taken from traces are labelled as such and not dressed up.
imx681_init_regs[] is 21 register writes whose individual meaning is not known.

What is derived is written down. The link frequency comes from the PLL
configuration visible in the same traces - 19.2 MHz EXCK, PLL2_MUL 303,
PLL2_PRE_DIV 3, giving 1939.2 MHz on the bus and therefore 969.6 MHz per lane -
and the pixel rate follows from that, the lane count and the bit depth. The
gain law and the black level were measured against the sensor rather than read
off a datasheet, because there is no public datasheet for this part.

Three things we know are arguable, raised here rather than left for review
=========================================================================

V4L2_CID_ANALOGUE_GAIN advertises 0..1020, which under the 1024/(1024-code)
law reads as up to 256x, while the analogue stage stops at code 960. Above that
the driver makes up the difference digitally, so the total gain still follows
the formula and an AGC's arithmetic is correct - it is buying noise instead of
exposure at the top of the range. Splitting it between ANALOGUE_GAIN and
DIGITAL_GAIN is the obvious alternative. Andre has not given a view on that one,
so if you want it, I will prepare it and put it to him rather than answer for
him - which may mean it waits for his return at the end of September.

The chip-ID read is a single cci_read with no retry. This machine has failed it
twice, once as 0x0081 and once as 0x0000 against 0x0681 - a sensor answering
before it is ready. A retry absorbs that without inventing a longer reset delay.
Andre would rather land the driver as written and add this afterwards, and I
have kept it that way rather than editing his patch.

It may not be this sensor's fault: the ov13858 on the same board has now
returned a wrong chip id once too, 0x1000 against 0xd855, reading it the same
unretried way. Two events are not a conclusion, but a retry is cheap.

The input clock rate is read and logged but never checked against the 19.2 MHz
the register sequence assumes. Same disposition: a follow-up, not a silent edit.

Testing
=======

On a Surface Pro 11, front camera, with all three patches applied - and on a
kernel built from this base, not backported into a distro one:

  # uname -r
  7.3.0-rc1-imx681-medianext+
  # dmesg
  intel-ipu7 0000:00:05.0: Found supported sensor SONY0681:00 (\_SB.PC00.I2C5.CAMF)
  imx681 i2c-SONY0681:00: IMX681 probed successfully: 3844x2640 @ 969600000 Hz link freq
  intel_ipu7_isys.isys intel_ipu7.isys.40: bind imx681 3-0010 nlanes is 2 port is 2

The first of those lines is 3/3 doing its job: without the ipu-bridge entry the
IPU does not recognise SONY0681 and nothing binds. No errors or warnings from
the driver at all.

  streams                  3844x2640 SGRBG10, 30.01 fps
  brightness vs gain       10.4 / 13.1 / 20.0 / 47.0 for codes 0 / 300 / 700 / 960

The second line is there because a mean brightness on its own cannot tell a
dark room from a dead pipeline. Driving the gain and watching the frames follow
it settles that; the captured frames are a recognisable room, right way up. The
same sweep on a 7.0.0 kernel with the same driver source gives 11.8 / 14.7 /
22.7 / 52.8 - the difference is the room, an hour later.

  checkpatch --strict      clean on 2/3 and 3/3; on 1/3 only "does MAINTAINERS
                           need updating?", which 2/3 answers
  make dt_binding_check    passes, example extracted and compiled
  build, W=1               imx681.o and ipu-bridge.o, no warnings

Based on media/next at f9536a8065 ("media: ipu-bridge: Add support additional
link frequency").

German reported this HID as a bug on this list on 3 September and has had no
reply since; he is on Cc here. He reaches IPU7 through the staging driver rather
than ipu-bridge, so if this goes anywhere there is a second machine on a second
path ready to try it.

  https://lore.kernel.org/linux-media/20260903080854.16266-1-germanpapulindez@gmail.com/

Andre Gilerson (1):
  media: i2c: Add Sony IMX681 sensor driver

Sergey Lebedev (2):
  dt-bindings: media: Add Sony IMX681
  media: ipu-bridge: Add Sony IMX681

 .../bindings/media/i2c/sony,imx681.yaml       | 107 +++
 MAINTAINERS                                   |   8 +
 drivers/media/i2c/Kconfig                     |  10 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/imx681.c                    | 900 ++++++++++++++++++
 drivers/media/pci/intel/ipu-bridge.c          |   2 +
 6 files changed, 1028 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml
 create mode 100644 drivers/media/i2c/imx681.c

-- 
2.53.0

[PATCH 1/3] dt-bindings: media: Add Sony IMX681

[Sergey Lebedev]

The Sony IMX681 is a CMOS image sensor used as the user-facing camera on
several Microsoft Surface devices. It operates from analog 2.8 V, digital
1.05 V and interface 1.8 V supplies, is programmable over I2C, and outputs
10-bit Bayer data over a two-lane MIPI CSI-2 D-PHY interface.

Signed-off-by: Sergey Lebedev <lsa.uz@pm.me>
---
 .../bindings/media/i2c/sony,imx681.yaml       | 107 ++++++++++++++++++
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml
new file mode 100644
index 000000000..6d2e4ddce
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml
@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/sony,imx681.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony IMX681 CMOS Image Sensor
+
+maintainers:
+  - Andre Gilerson <andre.gilerson@gmail.com>
+
+description:
+  The Sony IMX681 is a CMOS active pixel image sensor with a square pixel
+  array, used as the user-facing camera on several Microsoft Surface devices.
+  It operates from analog 2.8 V, digital 1.05 V and interface 1.8 V supplies,
+  is programmable over I2C, and outputs 10-bit Bayer data over a two-lane
+  MIPI CSI-2 D-PHY interface.
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: sony,imx681
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: Input clock (19.2 MHz)
+    maxItems: 1
+
+  avdd-supply:
+    description: Analog power supply (2.8 V)
+
+  dvdd-supply:
+    description: Digital power supply (1.05 V)
+
+  dovdd-supply:
+    description: Interface power supply (1.8 V)
+
+  reset-gpios:
+    description: Sensor reset (XCLR) GPIO, active low
+    maxItems: 1
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    unevaluatedProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            items:
+              - const: 1
+              - const: 2
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - avdd-supply
+  - dvdd-supply
+  - dovdd-supply
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera-sensor@10 {
+            compatible = "sony,imx681";
+            reg = <0x10>;
+            clocks = <&clock_cam>;
+            avdd-supply = <&vcc2v8_cam>;
+            dvdd-supply = <&vcc1v05_cam>;
+            dovdd-supply = <&vcc1v8_cam>;
+            reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
+            orientation = <0>;
+            rotation = <0>;
+
+            port {
+                imx681_ep: endpoint {
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <969600000>;
+                    remote-endpoint = <&csi_in>;
+                };
+            };
+        };
+    };
+...
-- 
2.53.0

[PATCH 2/3] media: i2c: Add Sony IMX681 sensor driver

[Sergey Lebedev]

From: Andre Gilerson <andre.gilerson@gmail.com>

The Sony IMX681 is the user-facing camera on the Microsoft Surface Pro 11
for Business (Intel Lunar Lake, IPU7), where it is enumerated as ACPI
device SONY0681. Without a driver the camera does not appear at all.

There is no public documentation for this sensor. The initialisation
sequence and the mode registers were recovered from I2C traces taken under
Windows, and the driver does not pretend otherwise: imx681_init_regs[] is
21 register writes whose individual meaning is not known.

What is not from the traces is derived and written down. The link frequency
comes from the PLL configuration visible in the same traces - 19.2 MHz
EXCK, PLL2_MUL 303, PLL2_PRE_DIV 3, giving 1939.2 MHz on the bus and
therefore 969.6 MHz per lane - and the pixel rate follows from that, the
lane count and the bit depth. The gain law and the black level were
measured against the sensor rather than taken from the traces; the numbers
are in the cover letter.

The driver uses the streams API, v4l2-cci for register access, the subdev
state API and runtime PM, and validates the endpoint's lane count and link
frequency against what the firmware describes.

Signed-off-by: Andre Gilerson <andre.gilerson@gmail.com>
Signed-off-by: Sergey Lebedev <lsa.uz@pm.me>
---
 MAINTAINERS                |   8 +
 drivers/media/i2c/Kconfig  |  10 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/imx681.c | 900 +++++++++++++++++++++++++++++++++++++
 4 files changed, 919 insertions(+)
 create mode 100644 drivers/media/i2c/imx681.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e12dc3ca5..198eaeefa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25586,6 +25586,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/media/i2c/sony,imx678.yaml
 F:	drivers/media/i2c/imx678.c
 
+SONY IMX681 SENSOR DRIVER
+M:	Andre Gilerson <andre.gilerson@gmail.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+T:	git git://linuxtv.org/media.git
+F:	Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml
+F:	drivers/media/i2c/imx681.c
+
 SONY MEMORYSTICK SUBSYSTEM
 M:	Maxim Levitsky <maximlevitsky@gmail.com>
 M:	Alex Dubov <oakad@yahoo.com>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 5c52007f9..5f7b0d22c 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -310,6 +310,16 @@ config VIDEO_IMX678
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx678.
 
+config VIDEO_IMX681
+	tristate "Sony IMX681 sensor support"
+	select V4L2_CCI_I2C
+	help
+	  This is a Video4Linux2 sensor driver for the Sony
+	  IMX681 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called imx681.
+
 config VIDEO_MAX9271_LIB
 	tristate
 
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index d04bd5724..8d6bd7466 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_IMX412) += imx412.o
 obj-$(CONFIG_VIDEO_IMX415) += imx415.o
 obj-$(CONFIG_VIDEO_IMX678) += imx678.o
 obj-$(CONFIG_VIDEO_IMX471) += imx471.o
+obj-$(CONFIG_VIDEO_IMX681) += imx681.o
 obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
 obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o
 obj-$(CONFIG_VIDEO_KS0127) += ks0127.o
diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c
new file mode 100644
index 000000000..78c8da89c
--- /dev/null
+++ b/drivers/media/i2c/imx681.c
@@ -0,0 +1,900 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Sony IMX681 CMOS Image Sensor Driver
+ *
+ * Front camera on Surface Pro 11 Business (Intel/Lunar Lake).
+ * Register sequences reverse-engineered from Windows I2C traces.
+ *
+ * Copyright (C) 2025
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+
+/* Chip ID register and expected value */
+#define IMX681_REG_CHIP_ID		CCI_REG16(0x0016)
+#define IMX681_CHIP_ID			0x0681
+
+/* Mode select */
+#define IMX681_REG_MODE_SELECT		CCI_REG8(0x0100)
+#define IMX681_MODE_STANDBY		0x00
+#define IMX681_MODE_STREAMING		0x01
+
+/* Group parameter hold */
+#define IMX681_REG_GROUP_HOLD		CCI_REG8(0x0104)
+
+/* Exposure (coarse integration time, 24-bit) */
+#define IMX681_REG_EXPOSURE		CCI_REG24(0x0229)
+#define IMX681_EXPOSURE_MIN		4
+#define IMX681_EXPOSURE_MAX		3173	/* frame_length - 4 */
+#define IMX681_EXPOSURE_OFFSET		4	/* frame_length - exposure_max */
+#define IMX681_EXPOSURE_DEFAULT		1907	/* from Windows trace */
+
+/* Analog gain */
+#define IMX681_REG_ANALOG_GAIN		CCI_REG16(0x0204)
+#define IMX681_ANA_GAIN_MIN		0
+#define IMX681_ANA_GAIN_MAX		1020	/* combined analog+digital */
+#define IMX681_ANA_GAIN_DEFAULT		0
+
+/* Digital gain */
+#define IMX681_REG_DIGITAL_GAIN		CCI_REG16(0x020E)
+#define IMX681_DIG_GAIN_MIN		0x0100	/* 1.0x */
+#define IMX681_DIG_GAIN_MAX		0x0FFF
+#define IMX681_DIG_GAIN_DEFAULT		0x0100
+
+/* Test pattern */
+#define IMX681_REG_TEST_PATTERN		CCI_REG16(0x0600)
+
+/* Frame length (24-bit, for streaming updates) */
+#define IMX681_REG_FRAME_LENGTH		CCI_REG24(0x033D)
+
+/* Image dimensions — native sensor output */
+#define IMX681_WIDTH			3844
+#define IMX681_HEIGHT			2640
+#define IMX681_LINE_LENGTH_PCK		7552	/* 0x1D80 */
+#define IMX681_FRAME_LENGTH_LINES	3177	/* 0x0C69 */
+#define IMX681_FRAME_LENGTH_MAX		0xFFFF	/* 24-bit reg, limit to 16-bit */
+
+/* MIPI lanes */
+#define IMX681_NUM_LANES		2
+
+/*
+ * Link frequency derived from PLL settings in Windows trace:
+ * EXCK=19.2MHz, PLL2_MUL=303, PLL2_PRE_DIV=3
+ * OP output = 19.2 * 303 / 3 = 1939.2 MHz (MIPI bit rate)
+ * Link freq = 1939.2 / 2 (DDR) = 969.6 MHz
+ */
+#define IMX681_LINK_FREQ		969600000LL
+
+/* Pixel rate = link_freq * 2 (DDR) * num_lanes / bpp */
+#define IMX681_PIXEL_RATE		(IMX681_LINK_FREQ * 2 * IMX681_NUM_LANES / 10)
+
+/* Power-on delay after reset deassert */
+#define IMX681_RESET_DELAY_US		1000
+#define IMX681_RESET_DELAY_RANGE_US	1000
+
+/* Post-standby-cancel stabilisation delays */
+#define IMX681_INIT_DELAY_US		10000
+
+#define IMAGE_PAD			0
+
+static const s64 imx681_link_frequencies[] = {
+	IMX681_LINK_FREQ,
+};
+
+/*
+ * Sensor init register sequence, captured from Windows I2C traces.
+ * This configures the sensor for 3844x2640 RAW10 output at ~30fps
+ * with 2-lane MIPI CSI-2, 19.2MHz input clock.
+ */
+static const struct cci_reg_sequence imx681_init_regs[] = {
+	/* Software standby */
+	{ CCI_REG8(0x0100),  0x00 },
+	/* External clock frequency = 19.2 MHz (encoded as MHz * 256) */
+	{ CCI_REG16(0x0136), 0x1333 },
+	/* Vendor specific configuration */
+	{ CCI_REG16(0x002C), 0x0505 },
+	/* CSI-2 signaling mode */
+	{ CCI_REG8(0x0111),  0x02 },
+	/* Image orientation: H-flip to match Windows AIQB (RGGB native → GRBG) */
+	{ CCI_REG8(0x0101),  0x01 },
+	/* Vendor access unlock sequence */
+	{ CCI_REG8(0x30EB),  0x05 },
+	{ CCI_REG8(0x30EB),  0x0C },
+	/* Vendor specific */
+	{ CCI_REG16(0x300A), 0xFFFF },
+	{ CCI_REG16(0x3532), 0xFFFF },
+	/* LINE_LENGTH_PCK = 7552 */
+	{ CCI_REG16(0x0342), 0x1D80 },
+	/* Frame length = 3177 */
+	{ CCI_REG16(0x033E), 0x0C69 },
+	/* Crop window start: X_ADD_STA[7:0]=100, Y_ADD_STA=256 */
+	{ CCI_REG24(0x0345), 0x640100 },
+	/* Crop window end: X_ADD_END[7:0]=103, Y_ADD_END=2895 */
+	{ CCI_REG24(0x0349), 0x670B4F },
+	/* Digital crop / vendor config */
+	{ CCI_REG24(0x040D), 0x040A50 },
+	/* X_OUTPUT_SIZE=3844, Y_OUTPUT_SIZE=2640 */
+	{ CCI_REG32(0x034C), 0x0F040A50 },
+	/* PLL multiplier = 225 */
+	{ CCI_REG8(0x0307),  0xE1 },
+	/* PLL2: pre_div=3, multiplier=303 (0x012F) */
+	{ CCI_REG24(0x030D), 0x03012F },
+	/* Frame duration initial */
+	{ CCI_REG16(0x022A), 0x0C61 },
+	/* Vendor specific registers */
+	{ CCI_REG8(0x7E9B),  0x02 },
+	{ CCI_REG8(0x0368),  0x00 },
+	{ CCI_REG8(0xD383),  0x01 },
+};
+
+/*
+ * First exposure settings applied before stream-on.
+ * Uses group parameter hold to ensure atomic update.
+ */
+static const struct cci_reg_sequence imx681_first_exposure[] = {
+	{ CCI_REG8(0x0104),  0x01 },		/* Group hold ON */
+	{ CCI_REG24(0x033D), 0x000C69 },	/* Frame length = 3177 */
+	{ CCI_REG24(0x0229), 0x000773 },	/* Exposure = 1907 lines */
+	{ CCI_REG16(0x0204), 0x0000 },		/* Analog gain = 0 (1x) */
+	{ CCI_REG16(0x020E), 0x0100 },		/* Digital gain = 1.0x */
+	{ CCI_REG8(0x0104),  0x00 },		/* Group hold OFF */
+};
+
+static const char * const imx681_test_pattern_menu[] = {
+	"Disabled",
+	"Solid Colour",
+	"Eight Vertical Colour Bars",
+	"Colour Bars With Fade to Grey",
+	"Pseudorandom Sequence (PN9)",
+};
+
+static const u32 imx681_mbus_codes[] = {
+	MEDIA_BUS_FMT_SGRBG10_1X10,
+};
+
+/* Regulator supplies */
+static const char * const imx681_supply_names[] = {
+	"avdd",		/* Analog 2.8V */
+	"dvdd",		/* Digital 1.05V */
+	"dovdd",	/* I/O 1.8V */
+};
+
+#define IMX681_NUM_SUPPLIES ARRAY_SIZE(imx681_supply_names)
+
+struct imx681 {
+	struct device *dev;
+	struct regmap *cci;
+
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+
+	struct clk *xclk;
+	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data supplies[IMX681_NUM_SUPPLIES];
+
+	/* V4L2 Controls */
+	struct v4l2_ctrl_handler ctrl_handler;
+	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *hblank;
+
+	unsigned long link_freq_bitmap;
+};
+
+static inline struct imx681 *to_imx681(struct v4l2_subdev *sd)
+{
+	return container_of_const(sd, struct imx681, sd);
+}
+
+static int imx681_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct imx681 *imx681 = container_of(ctrl->handler, struct imx681,
+					     ctrl_handler);
+	s64 exposure_max;
+	int ret = 0;
+
+	/* Update exposure max when VBLANK changes (even when not streaming) */
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		exposure_max = IMX681_HEIGHT + ctrl->val - IMX681_EXPOSURE_OFFSET;
+		__v4l2_ctrl_modify_range(imx681->exposure,
+					 IMX681_EXPOSURE_MIN, exposure_max,
+					 1, IMX681_EXPOSURE_DEFAULT);
+	}
+
+	/* Only apply controls to hardware when streaming */
+	if (!pm_runtime_get_if_active(imx681->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_VBLANK:
+		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
+		cci_write(imx681->cci, IMX681_REG_FRAME_LENGTH,
+			  IMX681_HEIGHT + ctrl->val, &ret);
+		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);
+		dev_dbg(imx681->dev, "set frame_length: %d, ret=%d\n",
+			IMX681_HEIGHT + ctrl->val, ret);
+		break;
+
+	case V4L2_CID_EXPOSURE:
+		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
+		cci_write(imx681->cci, IMX681_REG_EXPOSURE, ctrl->val, &ret);
+		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);
+		dev_dbg(imx681->dev, "set exposure: %d, ret=%d\n",
+			ctrl->val, ret);
+		break;
+
+	case V4L2_CID_ANALOGUE_GAIN: {
+		/*
+		 * Gain formula: gain = 1024/(1024-code).
+		 * Analog max is code 960 (16x). For codes above 960,
+		 * set analog to max and use digital gain for the rest.
+		 * Digital gain register: 0x0100 = 1.0x, value = gain * 256.
+		 */
+		int ana_code = min(ctrl->val, 960);
+		int dig_reg = IMX681_DIG_GAIN_MIN; /* 0x0100 = 1.0x */
+
+		if (ctrl->val > 960) {
+			/*
+			 * digital = total_gain / analog_gain
+			 *         = (1024/(1024-code)) / (1024/64)
+			 *         = 64 / (1024-code)
+			 * dig_reg = digital * 256 = 16384 / (1024-code)
+			 */
+			dig_reg = 16384 / (1024 - ctrl->val);
+			dig_reg = clamp(dig_reg, (int)IMX681_DIG_GAIN_MIN,
+					(int)IMX681_DIG_GAIN_MAX);
+		}
+
+		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
+		cci_write(imx681->cci, IMX681_REG_ANALOG_GAIN, ana_code,
+			  &ret);
+		cci_write(imx681->cci, IMX681_REG_DIGITAL_GAIN, dig_reg,
+			  &ret);
+		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);
+		dev_dbg(imx681->dev, "set gain code %d: analog=%d digital=0x%x, ret=%d\n",
+			ctrl->val, ana_code, dig_reg, ret);
+		break;
+	}
+
+	case V4L2_CID_DIGITAL_GAIN:
+		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
+		cci_write(imx681->cci, IMX681_REG_DIGITAL_GAIN, ctrl->val,
+			  &ret);
+		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);
+		dev_dbg(imx681->dev, "set digital gain: %d, ret=%d\n",
+			ctrl->val, ret);
+		break;
+
+	case V4L2_CID_TEST_PATTERN:
+		ret = cci_write(imx681->cci, IMX681_REG_TEST_PATTERN,
+				ctrl->val, NULL);
+		break;
+
+	default:
+		dev_dbg(imx681->dev, "unhandled ctrl id: 0x%x val: 0x%x\n",
+			ctrl->id, ctrl->val);
+		break;
+	}
+
+	pm_runtime_put(imx681->dev);
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops imx681_ctrl_ops = {
+	.s_ctrl = imx681_set_ctrl,
+};
+
+static int imx681_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index >= ARRAY_SIZE(imx681_mbus_codes))
+		return -EINVAL;
+
+	code->code = imx681_mbus_codes[code->index];
+	return 0;
+}
+
+static bool imx681_is_valid_mbus_code(u32 code)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(imx681_mbus_codes); i++)
+		if (imx681_mbus_codes[i] == code)
+			return true;
+	return false;
+}
+
+static int imx681_enum_frame_size(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	if (fse->index > 0)
+		return -EINVAL;
+
+	if (!imx681_is_valid_mbus_code(fse->code))
+		return -EINVAL;
+
+	fse->min_width = IMX681_WIDTH;
+	fse->max_width = IMX681_WIDTH;
+	fse->min_height = IMX681_HEIGHT;
+	fse->max_height = IMX681_HEIGHT;
+
+	return 0;
+}
+
+static int imx681_init_state(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *state)
+{
+	struct v4l2_mbus_framefmt *format;
+
+	format = v4l2_subdev_state_get_format(state, IMAGE_PAD);
+	format->width = IMX681_WIDTH;
+	format->height = IMX681_HEIGHT;
+	format->code = MEDIA_BUS_FMT_SGRBG10_1X10;
+	format->field = V4L2_FIELD_NONE;
+	format->colorspace = V4L2_COLORSPACE_RAW;
+	format->ycbcr_enc = V4L2_YCBCR_ENC_601;
+	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	format->xfer_func = V4L2_XFER_FUNC_NONE;
+
+	return 0;
+}
+
+static int imx681_set_pad_format(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state,
+				 struct v4l2_subdev_format *fmt)
+{
+	struct v4l2_mbus_framefmt *format;
+
+	/* Fixed resolution, fixed Bayer order */
+	fmt->format.width = IMX681_WIDTH;
+	fmt->format.height = IMX681_HEIGHT;
+	if (!imx681_is_valid_mbus_code(fmt->format.code))
+		fmt->format.code = imx681_mbus_codes[0];
+	fmt->format.field = V4L2_FIELD_NONE;
+	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
+	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_601;
+	fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
+
+	format = v4l2_subdev_state_get_format(state, fmt->pad);
+	*format = fmt->format;
+
+	return 0;
+}
+
+static int imx681_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = IMX681_WIDTH;
+		sel->r.height = IMX681_HEIGHT;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int imx681_start_streaming(struct imx681 *imx681)
+{
+	int ret;
+
+	dev_dbg(imx681->dev, "starting stream: %dx%d RAW10 2-lane\n",
+		IMX681_WIDTH, IMX681_HEIGHT);
+
+	/* Write init register sequence */
+	ret = cci_multi_reg_write(imx681->cci, imx681_init_regs,
+				  ARRAY_SIZE(imx681_init_regs), NULL);
+	if (ret) {
+		dev_err(imx681->dev, "failed to write init regs: %d\n", ret);
+		return ret;
+	}
+
+	dev_dbg(imx681->dev, "init registers written successfully\n");
+
+	/* Wait for sensor to stabilise after configuration */
+	usleep_range(IMX681_INIT_DELAY_US, IMX681_INIT_DELAY_US + 1000);
+
+	/* Apply first exposure settings with group hold */
+	ret = cci_multi_reg_write(imx681->cci, imx681_first_exposure,
+				  ARRAY_SIZE(imx681_first_exposure), NULL);
+	if (ret) {
+		dev_err(imx681->dev, "failed to write exposure: %d\n", ret);
+		return ret;
+	}
+
+	/* Apply any pending V4L2 control values */
+	ret = __v4l2_ctrl_handler_setup(imx681->sd.ctrl_handler);
+	if (ret) {
+		dev_err(imx681->dev, "failed to apply controls: %d\n", ret);
+		return ret;
+	}
+
+	/* Start streaming */
+	ret = cci_write(imx681->cci, IMX681_REG_MODE_SELECT,
+			IMX681_MODE_STREAMING, NULL);
+	if (ret) {
+		dev_err(imx681->dev, "failed to start streaming: %d\n", ret);
+		return ret;
+	}
+
+	dev_dbg(imx681->dev, "streaming started\n");
+	return 0;
+}
+
+static int imx681_stop_streaming(struct imx681 *imx681)
+{
+	int ret;
+
+	ret = cci_write(imx681->cci, IMX681_REG_MODE_SELECT,
+			IMX681_MODE_STANDBY, NULL);
+	if (ret)
+		dev_err(imx681->dev, "failed to stop streaming: %d\n", ret);
+
+	return ret;
+}
+
+static int imx681_enable_streams(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state,
+				 u32 pad, u64 streams_mask)
+{
+	struct imx681 *imx681 = to_imx681(sd);
+	int ret;
+
+	if (pad != IMAGE_PAD)
+		return -EINVAL;
+
+	ret = pm_runtime_get_sync(imx681->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(imx681->dev);
+		return ret;
+	}
+
+	ret = imx681_start_streaming(imx681);
+	if (ret)
+		pm_runtime_put_autosuspend(imx681->dev);
+
+	return ret;
+}
+
+static int imx681_disable_streams(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state,
+				  u32 pad, u64 streams_mask)
+{
+	struct imx681 *imx681 = to_imx681(sd);
+
+	if (pad != IMAGE_PAD)
+		return -EINVAL;
+
+	imx681_stop_streaming(imx681);
+	pm_runtime_put_autosuspend(imx681->dev);
+
+	return 0;
+}
+
+static const struct v4l2_subdev_video_ops imx681_video_ops = {
+	.s_stream = v4l2_subdev_s_stream_helper,
+};
+
+static const struct v4l2_subdev_pad_ops imx681_pad_ops = {
+	.enum_mbus_code = imx681_enum_mbus_code,
+	.get_fmt = v4l2_subdev_get_fmt,
+	.set_fmt = imx681_set_pad_format,
+	.get_selection = imx681_get_selection,
+	.enum_frame_size = imx681_enum_frame_size,
+	.enable_streams = imx681_enable_streams,
+	.disable_streams = imx681_disable_streams,
+};
+
+static const struct v4l2_subdev_ops imx681_subdev_ops = {
+	.video = &imx681_video_ops,
+	.pad = &imx681_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops imx681_internal_ops = {
+	.init_state = imx681_init_state,
+};
+
+/* Power management */
+static int imx681_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx681 *imx681 = to_imx681(sd);
+	int ret;
+
+	dev_dbg(imx681->dev, "power on\n");
+
+	ret = regulator_bulk_enable(IMX681_NUM_SUPPLIES, imx681->supplies);
+	if (ret) {
+		dev_err(imx681->dev, "failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(imx681->xclk);
+	if (ret) {
+		dev_err(imx681->dev, "failed to enable clock: %d\n", ret);
+		goto err_reg_disable;
+	}
+
+	/* Deassert reset (active low) */
+	gpiod_set_value_cansleep(imx681->reset_gpio, 0);
+
+	usleep_range(IMX681_RESET_DELAY_US,
+		     IMX681_RESET_DELAY_US + IMX681_RESET_DELAY_RANGE_US);
+
+	return 0;
+
+err_reg_disable:
+	regulator_bulk_disable(IMX681_NUM_SUPPLIES, imx681->supplies);
+	return ret;
+}
+
+static int imx681_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx681 *imx681 = to_imx681(sd);
+
+	dev_dbg(imx681->dev, "power off\n");
+
+	/* Assert reset */
+	gpiod_set_value_cansleep(imx681->reset_gpio, 1);
+	clk_disable_unprepare(imx681->xclk);
+	regulator_bulk_disable(IMX681_NUM_SUPPLIES, imx681->supplies);
+
+	return 0;
+}
+
+static int imx681_identify_module(struct imx681 *imx681)
+{
+	u64 val;
+	int ret;
+
+	ret = cci_read(imx681->cci, IMX681_REG_CHIP_ID, &val, NULL);
+	if (ret) {
+		dev_err(imx681->dev,
+			"failed to read chip ID register 0x0016: %d\n", ret);
+		return ret;
+	}
+
+	if (val != IMX681_CHIP_ID) {
+		dev_err(imx681->dev, "chip ID mismatch: 0x%04llx != 0x%04x\n",
+			val, IMX681_CHIP_ID);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int imx681_init_controls(struct imx681 *imx681)
+{
+	struct v4l2_ctrl_handler *ctrl_hdlr = &imx681->ctrl_handler;
+	struct v4l2_fwnode_device_properties props;
+	struct v4l2_ctrl *link_freq;
+	s64 hblank, vblank;
+	int ret;
+
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
+	if (ret)
+		return ret;
+
+	/* Pixel rate (read-only) */
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx681_ctrl_ops,
+			  V4L2_CID_PIXEL_RATE, IMX681_PIXEL_RATE,
+			  IMX681_PIXEL_RATE, 1, IMX681_PIXEL_RATE);
+
+	/* Link frequency (read-only) */
+	link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx681_ctrl_ops,
+					   V4L2_CID_LINK_FREQ,
+					   __fls(imx681->link_freq_bitmap),
+					   __ffs(imx681->link_freq_bitmap),
+					   imx681_link_frequencies);
+	if (link_freq)
+		link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	/* Horizontal blanking (read-only, fixed) */
+	hblank = IMX681_LINE_LENGTH_PCK - IMX681_WIDTH;
+	imx681->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx681_ctrl_ops,
+					   V4L2_CID_HBLANK, hblank, hblank,
+					   1, hblank);
+	if (imx681->hblank)
+		imx681->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	/* Vertical blanking (writable to allow longer exposures) */
+	vblank = IMX681_FRAME_LENGTH_LINES - IMX681_HEIGHT;
+	imx681->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx681_ctrl_ops,
+					   V4L2_CID_VBLANK, vblank,
+					   IMX681_FRAME_LENGTH_MAX - IMX681_HEIGHT,
+					   1, vblank);
+
+	/* Exposure */
+	imx681->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx681_ctrl_ops,
+					     V4L2_CID_EXPOSURE,
+					     IMX681_EXPOSURE_MIN,
+					     IMX681_EXPOSURE_MAX, 1,
+					     IMX681_EXPOSURE_DEFAULT);
+
+	/* Analog gain */
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx681_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
+			  IMX681_ANA_GAIN_MIN, IMX681_ANA_GAIN_MAX, 1,
+			  IMX681_ANA_GAIN_DEFAULT);
+
+	/* Digital gain */
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx681_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
+			  IMX681_DIG_GAIN_MIN, IMX681_DIG_GAIN_MAX, 1,
+			  IMX681_DIG_GAIN_DEFAULT);
+
+	/* Test pattern */
+	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx681_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(imx681_test_pattern_menu) - 1,
+				     0, 0, imx681_test_pattern_menu);
+
+	if (ctrl_hdlr->error) {
+		ret = ctrl_hdlr->error;
+		dev_err(imx681->dev, "control init failed: %d\n", ret);
+		goto error;
+	}
+
+	ret = v4l2_fwnode_device_parse(imx681->dev, &props);
+	if (ret)
+		goto error;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx681_ctrl_ops,
+					      &props);
+	if (ret)
+		goto error;
+
+	imx681->sd.ctrl_handler = ctrl_hdlr;
+	return 0;
+
+error:
+	v4l2_ctrl_handler_free(ctrl_hdlr);
+	return ret;
+}
+
+static int imx681_parse_endpoint(struct imx681 *imx681)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(imx681->dev);
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
+	struct fwnode_handle *ep;
+	int ret;
+
+	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
+	if (!ep) {
+		dev_err(imx681->dev, "no endpoint found in firmware node\n");
+		return -ENXIO;
+	}
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+	fwnode_handle_put(ep);
+	if (ret) {
+		dev_err(imx681->dev, "failed to parse endpoint: %d\n", ret);
+		return ret;
+	}
+
+	if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX681_NUM_LANES) {
+		dev_err(imx681->dev,
+			"expected %d data lanes, got %d\n",
+			IMX681_NUM_LANES,
+			bus_cfg.bus.mipi_csi2.num_data_lanes);
+		ret = -EINVAL;
+		goto done;
+	}
+
+	ret = v4l2_link_freq_to_bitmap(imx681->dev,
+				       bus_cfg.link_frequencies,
+				       bus_cfg.nr_of_link_frequencies,
+				       imx681_link_frequencies,
+				       ARRAY_SIZE(imx681_link_frequencies),
+				       &imx681->link_freq_bitmap);
+	if (ret)
+		dev_err(imx681->dev, "link frequency mismatch: %d\n", ret);
+
+done:
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+	return ret;
+}
+
+static int imx681_probe(struct i2c_client *client)
+{
+	struct imx681 *imx681;
+	unsigned int i;
+	int ret;
+
+	imx681 = devm_kzalloc(&client->dev, sizeof(*imx681), GFP_KERNEL);
+	if (!imx681)
+		return -ENOMEM;
+
+	imx681->dev = &client->dev;
+
+	/* Initialise V4L2 subdev */
+	v4l2_i2c_subdev_init(&imx681->sd, client, &imx681_subdev_ops);
+
+	/* Initialise CCI regmap for 16-bit register addresses */
+	imx681->cci = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(imx681->cci)) {
+		ret = PTR_ERR(imx681->cci);
+		dev_err(imx681->dev, "failed to init CCI: %d\n", ret);
+		return ret;
+	}
+
+	/* Get clock (optional - INT3472 provides it on Surface devices) */
+	imx681->xclk = devm_clk_get_optional(imx681->dev, NULL);
+	if (IS_ERR(imx681->xclk))
+		return dev_err_probe(imx681->dev, PTR_ERR(imx681->xclk),
+				     "failed to get clock\n");
+
+	if (imx681->xclk)
+		dev_dbg(imx681->dev, "clock rate: %lu Hz\n",
+			clk_get_rate(imx681->xclk));
+
+	/* Get regulators (optional) */
+	for (i = 0; i < IMX681_NUM_SUPPLIES; i++)
+		imx681->supplies[i].supply = imx681_supply_names[i];
+
+	ret = devm_regulator_bulk_get(imx681->dev, IMX681_NUM_SUPPLIES,
+				      imx681->supplies);
+	if (ret) {
+		dev_dbg(imx681->dev,
+			"regulators not available (expected on ACPI): %d\n",
+			ret);
+		/* Continue without regulators - INT3472 may handle power */
+	}
+
+	/* Get reset GPIO (optional) */
+	imx681->reset_gpio = devm_gpiod_get_optional(imx681->dev, "reset",
+						     GPIOD_OUT_HIGH);
+	if (IS_ERR(imx681->reset_gpio))
+		return dev_err_probe(imx681->dev,
+				     PTR_ERR(imx681->reset_gpio),
+				     "failed to get reset GPIO\n");
+
+	/* Parse CSI-2 endpoint */
+	ret = imx681_parse_endpoint(imx681);
+	if (ret) {
+		dev_err(imx681->dev, "endpoint parse failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Power on and verify chip ID */
+	ret = imx681_power_on(imx681->dev);
+	if (ret) {
+		dev_err(imx681->dev, "power on failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = imx681_identify_module(imx681);
+	if (ret)
+		goto error_power_off;
+
+	/* Enable runtime PM */
+	pm_runtime_set_active(imx681->dev);
+	pm_runtime_get_noresume(imx681->dev);
+	pm_runtime_enable(imx681->dev);
+	pm_runtime_set_autosuspend_delay(imx681->dev, 1000);
+	pm_runtime_use_autosuspend(imx681->dev);
+
+	/* Init V4L2 controls */
+	ret = imx681_init_controls(imx681);
+	if (ret)
+		goto error_pm;
+
+	/* Setup subdev */
+	imx681->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	imx681->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	imx681->sd.internal_ops = &imx681_internal_ops;
+
+	/* Init media entity */
+	imx681->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_pads_init(&imx681->sd.entity, 1, &imx681->pad);
+	if (ret) {
+		dev_err(imx681->dev, "media entity init failed: %d\n", ret);
+		goto error_handler_free;
+	}
+
+	imx681->sd.state_lock = imx681->ctrl_handler.lock;
+	ret = v4l2_subdev_init_finalize(&imx681->sd);
+	if (ret < 0) {
+		dev_err(imx681->dev, "subdev init finalize failed: %d\n", ret);
+		goto error_media_entity;
+	}
+
+	ret = v4l2_async_register_subdev_sensor(&imx681->sd);
+	if (ret < 0) {
+		dev_err(imx681->dev,
+			"async register subdev failed: %d\n", ret);
+		goto error_subdev_cleanup;
+	}
+
+	pm_runtime_put_autosuspend(imx681->dev);
+
+	dev_info(imx681->dev,
+		 "IMX681 probed successfully: %dx%d @ %lld Hz link freq\n",
+		 IMX681_WIDTH, IMX681_HEIGHT, IMX681_LINK_FREQ);
+
+	return 0;
+
+error_subdev_cleanup:
+	v4l2_subdev_cleanup(&imx681->sd);
+error_media_entity:
+	media_entity_cleanup(&imx681->sd.entity);
+error_handler_free:
+	v4l2_ctrl_handler_free(imx681->sd.ctrl_handler);
+error_pm:
+	pm_runtime_disable(imx681->dev);
+	pm_runtime_set_suspended(imx681->dev);
+error_power_off:
+	imx681_power_off(imx681->dev);
+	return ret;
+}
+
+static void imx681_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx681 *imx681 = to_imx681(sd);
+
+	v4l2_async_unregister_subdev(sd);
+	v4l2_subdev_cleanup(&imx681->sd);
+	media_entity_cleanup(&sd->entity);
+	v4l2_ctrl_handler_free(imx681->sd.ctrl_handler);
+
+	pm_runtime_disable(imx681->dev);
+	if (!pm_runtime_status_suspended(imx681->dev))
+		imx681_power_off(imx681->dev);
+	pm_runtime_set_suspended(imx681->dev);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(imx681_pm_ops, imx681_power_off,
+				 imx681_power_on, NULL);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id imx681_acpi_ids[] = {
+	{ "SONY0681" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(acpi, imx681_acpi_ids);
+#endif
+
+static const struct of_device_id imx681_dt_ids[] = {
+	{ .compatible = "sony,imx681" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx681_dt_ids);
+
+static struct i2c_driver imx681_i2c_driver = {
+	.driver = {
+		.name = "imx681",
+		.pm = pm_ptr(&imx681_pm_ops),
+		.acpi_match_table = ACPI_PTR(imx681_acpi_ids),
+		.of_match_table = imx681_dt_ids,
+	},
+	.probe = imx681_probe,
+	.remove = imx681_remove,
+};
+module_i2c_driver(imx681_i2c_driver);
+
+MODULE_DESCRIPTION("Sony IMX681 CMOS Image Sensor Driver");
+MODULE_AUTHOR("Andre Gilerson <andre.gilerson@gmail.com>");
+MODULE_LICENSE("GPL");
-- 
2.53.0

[PATCH 3/3] media: ipu-bridge: Add Sony IMX681

[Sergey Lebedev]

The Microsoft Surface Pro 11 for Business (Intel Lunar Lake, IPU7) carries
a Sony IMX681 as its user-facing sensor, enumerated as ACPI device
SONY0681. Add it so ipu_bridge_connect_sensors() builds its half of the
graph; without the entry the sensor is never connected, whatever driver is
present.

One link frequency, 969.6 MHz, on two CSI-2 lanes.

Signed-off-by: Sergey Lebedev <lsa.uz@pm.me>
---
 drivers/media/pci/intel/ipu-bridge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 7e65491fa..68c2eb326 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -97,6 +97,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
 	/* Omnivision OV8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
+	/* Sony IMX681 */
+	IPU_SENSOR_CONFIG("SONY0681", 1, 969600000),
 	/* Sony IMX471 */
 	IPU_SENSOR_CONFIG("SONY471A", 1, 200000000),
 	/* Sony IMX471 (found on Lenovo X1 Carbon G14) */
-- 
2.53.0

Re: [PATCH 2/3] media: i2c: Add Sony IMX681 sensor driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Ignoring devm_regulator_bulk_get() return value causes NULL pointer dereference and Use-After-Free during regulator_bulk_enable().
- [High] Potential NULL pointer dereference in imx681_set_pad_format() when state is NULL.
- [High] Incorrect check of pm_runtime_get_if_active() return value leads to hardware access while suspended and reference count underflow.
- [High] Unclustered controls V4L2_CID_ANALOGUE_GAIN and V4L2_CID_DIGITAL_GAIN clash over the same hardware register.
- [Medium] Missing pm_runtime_put_noidle() on the probe error path leads to a PM usage counter leak.
--

commit def0570ed5235b7103f00a2f2f7f233dec35fdd6
Author: Andre Gilerson <andre.gilerson@gmail.com>
Subject: media: i2c: Add Sony IMX681 sensor driver

This commit adds support for the Sony IMX681 sensor, used in the
Microsoft Surface Pro 11 for Business. It implements standard V4L2
subdev APIs, CCI register access, runtime PM, and streams support based
on register sequences recovered from I2C traces.

> diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c
> new file mode 100644
> index 0000000000000..78c8da89c6323
> --- /dev/null
> +++ b/drivers/media/i2c/imx681.c
> @@ -0,0 +1,900 @@
[ ... ]
> +static int imx681_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	/* Only apply controls to hardware when streaming */
> +	if (!pm_runtime_get_if_active(imx681->dev))
> +		return 0;

[Severity: High]
Does this incorrectly treat negative error codes from
pm_runtime_get_if_active() as a success condition?

pm_runtime_get_if_active() returns a negative error code (like -EINVAL) when
Runtime PM is disabled. Because negative values are non-zero, this check
evaluates to false and execution proceeds to access the hardware, which could
be powered off.

[ ... ]
> +	case V4L2_CID_ANALOGUE_GAIN: {
[ ... ]
> +		cci_write(imx681->cci, IMX681_REG_ANALOG_GAIN, ana_code,
> +			  &ret);
> +		cci_write(imx681->cci, IMX681_REG_DIGITAL_GAIN, dig_reg,
> +			  &ret);
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);
> +		dev_dbg(imx681->dev, "set gain code %d: analog=%d digital=0x%x, ret=%d\n",
> +			ctrl->val, ana_code, dig_reg, ret);
> +		break;
> +	}
> +
> +	case V4L2_CID_DIGITAL_GAIN:
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
> +		cci_write(imx681->cci, IMX681_REG_DIGITAL_GAIN, ctrl->val,
> +			  &ret);

[Severity: High]
Will this independent digital gain control blindly overwrite the compensatory
value computed in the V4L2_CID_ANALOGUE_GAIN handler?

Since these controls are unclustered, the framework applies them individually
in ID order when streaming starts. The digital component computed and written
by ANALOGUE_GAIN to IMX681_REG_DIGITAL_GAIN could be overwritten here, losing
the intended high-gain configuration.

> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);
> +		dev_dbg(imx681->dev, "set digital gain: %d, ret=%d\n",
> +			ctrl->val, ret);
> +		break;
[ ... ]
> +	}
> +
> +	pm_runtime_put(imx681->dev);
> +	return ret;
> +}

[Severity: High]
If pm_runtime_get_if_active() failed and returned a negative error code, the
usage counter was never incremented. Does this unconditional pm_runtime_put()
call corrupt the usage counter by causing an underflow?

[ ... ]
> +static int imx681_set_pad_format(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *state,
> +				 struct v4l2_subdev_format *fmt)
> +{
> +	struct v4l2_mbus_framefmt *format;
[ ... ]
> +	fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
> +
> +	format = v4l2_subdev_state_get_format(state, fmt->pad);
> +	*format = fmt->format;

[Severity: High]
Is it possible for state to be NULL here, leading to a NULL pointer dereference
when v4l2_subdev_state_get_format() returns NULL?

For subdevices without the V4L2_SUBDEV_FL_STREAMS flag, the media framework's
check_state() wrapper does not enforce that state is non-NULL for active
formats. If a bridge driver directly calls v4l2_subdev_call() and passes a NULL
active state, this will oops.

[ ... ]
> +static int imx681_power_on(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct imx681 *imx681 = to_imx681(sd);
> +	int ret;
> +
> +	dev_dbg(imx681->dev, "power on\n");
> +
> +	ret = regulator_bulk_enable(IMX681_NUM_SUPPLIES, imx681->supplies);

[Severity: High]
Can this cause a crash in the regulator core if regulators were unavailable
during probe?

In imx681_probe(), the driver explicitly ignores failures from
devm_regulator_bulk_get() to support ACPI setups. However, when that fails, the
regulator core's error path leaves the imx681->supplies array populated with
NULL pointers or dangling pointers for partially acquired regulators. Calling
regulator_bulk_enable() on that invalid array dereferences them.

[ ... ]
> +static int imx681_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ret = devm_regulator_bulk_get(imx681->dev, IMX681_NUM_SUPPLIES,
> +				      imx681->supplies);
> +	if (ret) {
> +		dev_dbg(imx681->dev,
> +			"regulators not available (expected on ACPI): %d\n",
> +			ret);
> +		/* Continue without regulators - INT3472 may handle power */
> +	}

[Severity: High]
Since failures here are expected and ignored, does the imx681->supplies array
need to be cleared to avoid the crash in imx681_power_on() mentioned above?

[ ... ]
> +	/* Enable runtime PM */
> +	pm_runtime_set_active(imx681->dev);
> +	pm_runtime_get_noresume(imx681->dev);
> +	pm_runtime_enable(imx681->dev);
> +	pm_runtime_set_autosuspend_delay(imx681->dev, 1000);
> +	pm_runtime_use_autosuspend(imx681->dev);
> +
> +	/* Init V4L2 controls */
> +	ret = imx681_init_controls(imx681);
> +	if (ret)
> +		goto error_pm;
[ ... ]
> +error_pm:
> +	pm_runtime_disable(imx681->dev);
> +	pm_runtime_set_suspended(imx681->dev);

[Severity: Medium]
Will this error path leak the runtime PM usage counter?

The probe function increments the usage counter via pm_runtime_get_noresume(),
but this cleanup path does not decrement it with pm_runtime_put_noidle().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909174240.80023-1-lsa.uz@pm.me?part=2

Re: [PATCH 2/3] media: i2c: Add Sony IMX681 sensor driver

[Sergey Lebedev]

Thank you - four of the five are real, and I have checked each against the
patch rather than agreeing on sight. All four are fixed in v2, which follows
this message. The author is away until 28 September, so v2 comes from me; where
a fix reverses something he wrote deliberately I have said so and given the
reasoning rather than leaving the question hanging for three weeks.

Confirmed
=========

1. devm_regulator_bulk_get() ignored. probe() logs a dev_dbg and continues, and
   imx681_power_on() then calls regulator_bulk_enable() on an array nothing
   filled in. Your use-after-free reading is the sharper one and it holds:
   _regulator_bulk_get() unwinds with "while (--i >= 0)
   regulator_put(consumers[i].consumer)" and leaves the pointers where they
   are, so a get that fails partway puts freed regulators into supplies[]
   rather than NULLs. The likely failure is -EPROBE_DEFER, which is ordinary.
   On this machine INT3472 hands out dummies so the get succeeds, which is
   exactly why testing did not find it. v2 makes it fatal, via dev_err_probe(),
   which is also the right handling for the -EPROBE_DEFER case.

2. pm_runtime_get_if_active(). It returns 1, 0, or a negative errno, and
   "if (!...)" only catches the 0. An error therefore falls through to issue
   I2C to a possibly-suspended sensor and then pm_runtime_put()s a reference it
   never took. v2 tests it as "if (pm_runtime_get_if_active(dev) <= 0)". The
   kerneldoc is explicit that on -EINVAL "the usage_count will remain
   unmodified", so the put is unbalanced and not merely redundant.

3. Missing pm_runtime_put_noidle() on the probe error path. The reference
   taken by pm_runtime_get_noresume() is never released, and
   pm_runtime_set_suspended() is called while it is still held. v2 adds it at
   error_pm.

Not a bug, and I think demonstrably so
======================================

The NULL-state dereference in imx681_set_pad_format(). The subdev core reaches
.set_fmt through v4l2_subdev_call_state_active(), which always supplies a
state, and the drivers around it agree: imx415, imx355, ov02c10 and ov08x40
all call v4l2_subdev_state_get_format(state, ...) with no NULL check between
them. If the core can hand a NULL state to .set_fmt then this is a
subsystem-wide problem rather than one driver's, and I would rather hear that
from a maintainer than paper over it here.

The gain clash, which is the interesting one
============================================

Correct, and worse than it looks. The analogue handler writes
IMX681_REG_DIGITAL_GAIN on *every* call, not only above code 960: above 960 it
writes the spillover it computed, and at or below 960 it writes the 1.0x it
initialised the local to. V4L2_CID_DIGITAL_GAIN writes the same register
directly. So setting digital gain to 4x and then moving analogue gain anywhere
at all puts 1x back into 0x020E - no error returned, and the DIGITAL_GAIN
control still reporting 4x.

The v1 cover asked whether the range should be split honestly between the two
controls. I have stopped asking and done it, because the two turn out to be one
question: clustering would stop the controls overwriting each other, but it
would then need an invented rule for what analogue 1020 together with digital
2x means, and there is no honest answer - they are two names for one register.
So v2 advertises ANALOGUE_GAIN as 0..960, the 1x..16x the analogue stage
actually does, and DIGITAL_GAIN owns 0x020E alone.

Measured rather than argued, by reading 0x020E over i2c while the sensor
streams. It has to be while streaming: at stream start
__v4l2_ctrl_handler_setup() applies the controls in creation order, analogue
before digital, so digital wins and the clash is invisible. It bites a live AE
loop, which is the case that matters.

                                     v1 module     v2 module
  start                              0x0100        0x0100
  after digital_gain = 4x            0x0400        0x0400
  after analogue_gain 500 -> 700     0x0100  <--   0x0400
  DIGITAL_GAIN control reads         1024          1024

Both modules were built from the same tree and swapped with rmmod/insmod, so
nothing but the driver differs between the columns. The last row is the point:
the control reports 4x in both, and in v1 the register disagrees with it
silently.

For Andre, on your return
=========================

Two of these touch choices that look deliberate in your code, so they are yours
to overrule.

The regulator handling. Your comment says "Continue without regulators -
INT3472 may handle power", and v2 makes that failure fatal instead. My reading
is that the get does not fail for supplies the firmware leaves undescribed - it
hands out dummies, which is where this machine's "supply dvdd not found, using
dummy regulator" comes from, and the camera still probes with the change in
place. If you wrote the comment because you had actually seen the get fail on
some machine, then I have removed a workaround you needed: say so and it goes
back in v3.

The gain range. I have split it rather than holding v2 for three weeks:
ANALOGUE_GAIN stops at 960 and no longer touches the digital register. Your own
define carried the comment "combined analog+digital", and V4L2 has no control
meaning combined gain, so I believe this is what the ABI asks for - but it is
your design and you can have it back in v3.

And one you did not ask for: the series lists you as M: for SONY IMX681 in
MAINTAINERS. That is the convention for a driver's author, but it is also a
standing obligation to everyone who reports a bug against it later, so it
should be your choice and not mine. It is already in v2 because leaving a new
driver with no maintainer entry is worse; say the word and it becomes me, or
both of us.

I ran an independent review of the same file before writing this, and it found
the same four and nothing else.

One thing worth saying plainly, since v1's cover letter listed its testing at
some length. That list - applies to media/next, correct authorship under git
am, checkpatch, dt_binding_check, a W=1 build, booted on the hardware with the
camera streaming - would not have found a single one of these four. They live
on error paths this board never takes and in a control interaction that needs
both controls driven while streaming. Testing that the happy path works is not
review, and I should not have been as satisfied with that list as I was.

Sergey