[PATCH v5 0/3] Add support for the Sony IMX681 camera sensor

[PATCH v5 0/3] Add support for the Sony IMX681 camera sensor

[Sergey Lebedev]

The Sony IMX681 is the user-facing camera on the Microsoft Surface Pro 11 for
Business (Intel Lunar Lake, IPU7), 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.

Changes in v5
=============

v4 does not compile on aedd77ea8168, the base it declares. .set_fmt and
.get_selection gained a const v4l2_subdev_client_info *ci argument between
b38d06ad1e13 and that commit; v4 was rebased across the window and 2/3 was
carried unchanged. v5 adds the argument, two lines, and differs from v4 in
nothing else. Krzysztof's Reviewed-by on 1/3 and German's Tested-by on 2/3
stand.

It took building the series to see it: git apply reports no offset on v4
even now, and checkpatch passes it.

Checked
=======

Built into a kernel from this series and booted on the machine. 2/3's
imx681.c and the file that built the running module hash to the same bytes.

  intel-ipu7: Found supported sensor SONY0681:00 (\_SB.PC00.I2C5.CAMF)

  v4l2-compliance 1.32.0 on the sensor subdev:
  Total for device /dev/v4l-subdev5: 46, Succeeded: 46, Failed: 0, Warnings: 0

checkpatch --strict, sparse and W=1 are clean on 2/3 and 3/3; 1/3's only
warning asks whether MAINTAINERS needs updating, which 2/3 does.

v4's limitations are unchanged and still hold: one machine, one sensor
sample, one mode (3844x2640 SGRBG10 at 969.6 MHz per lane), dummy dvdd and
dovdd from INT3472 so the fatal regulator path is reasoned rather than
exercised, no IVSC HID present, and no KASAN or lockdep build.

Still open, as in v4: Andre's ack on the binding, which Krzysztof asked for
and which is properly Andre's. And the mirroring question raised against v3.

Based on media/next at aedd77ea8168 ("media: qcom: camss: use
fwnode_graph_for_each_endpoint_scoped() to simplify code"). That branch
rewinds, so if this does not apply where you are, say so and I will rebase -
and build it afterwards this time.

German reported this HID as a bug on this list on 3 September and has had no
reply since; he is on Cc here.

  https://lore.kernel.org/linux-media/20260903080854.16266-1-germanpapulindez@gmail.com/
-- 
2.54.0 (Apple Git-157)

[PATCH v5 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 .../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 0000000000..6d2e4ddcee
--- /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.54.0 (Apple Git-157)

[PATCH v5 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>
Tested-by: German Pablo Lindo <germanpapulindez@gmail.com>
---
 MAINTAINERS                |   7 +
 drivers/media/i2c/Kconfig  |  10 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/imx681.c | 876 +++++++++++++++++++++++++++++++++++++
 4 files changed, 894 insertions(+)
 create mode 100644 drivers/media/i2c/imx681.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4cc4a2dc6d..4479f96d0d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25609,6 +25609,13 @@ 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
+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 4d99464791..c759a2d239 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -321,6 +321,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 fd1cb25718..98bcecf0c4 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -64,6 +64,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_IT6625) += it6625.o
diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c
new file mode 100644
index 0000000000..825d8c80a2
--- /dev/null
+++ b/drivers/media/i2c/imx681.c
@@ -0,0 +1,876 @@
+// 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		960	/* 16x, where the analogue stage ends */
+#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 pm_status;
+	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);
+	}
+
+	/*
+	 * 1 with a reference taken, 0 if the device is not active, or -EINVAL
+	 * if runtime PM is unavailable. Only the 0 means there is nothing to
+	 * do: without runtime PM the sensor is powered from probe and never
+	 * suspended, so the write still has to go out - but no reference was
+	 * taken then, and none may be dropped.
+	 */
+	pm_status = pm_runtime_get_if_active(imx681->dev);
+	if (!pm_status)
+		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); code 960 is 16x. */
+		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
+		cci_write(imx681->cci, IMX681_REG_ANALOG_GAIN, ctrl->val,
+			  &ret);
+		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);
+		dev_dbg(imx681->dev, "set analogue gain code %d, ret=%d\n",
+			ctrl->val, 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;
+	}
+
+	if (pm_status > 0)
+		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,
+				 const struct v4l2_subdev_client_info *ci,
+				 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,
+				const struct v4l2_subdev_client_info *ci,
+				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)
+		return dev_err_probe(imx681->dev, ret,
+				     "failed to read chip ID register 0x0016\n");
+
+	if (val != IMX681_CHIP_ID) {
+		return dev_err_probe(imx681->dev, -EIO,
+				     "chip ID mismatch: 0x%04llx != 0x%04x\n",
+				     val, IMX681_CHIP_ID);
+	}
+
+	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;
+		ret = dev_err_probe(imx681->dev, ret, "control init failed\n");
+		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) {
+		return dev_err_probe(imx681->dev, -ENXIO,
+				     "no endpoint found in firmware node\n");
+	}
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+	fwnode_handle_put(ep);
+	if (ret)
+		return dev_err_probe(imx681->dev, ret,
+				     "failed to parse endpoint\n");
+
+	if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX681_NUM_LANES) {
+		ret = dev_err_probe(imx681->dev, -EINVAL,
+				    "expected %d data lanes, got %d\n",
+				    IMX681_NUM_LANES,
+				    bus_cfg.bus.mipi_csi2.num_data_lanes);
+		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)
+		ret = dev_err_probe(imx681->dev, ret,
+				    "link frequency mismatch\n");
+
+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))
+		return dev_err_probe(imx681->dev, PTR_ERR(imx681->cci),
+				     "failed to init CCI\n");
+
+	/* 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");
+
+	/* Get regulators */
+	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)
+		return dev_err_probe(imx681->dev, ret,
+				     "failed to get regulators\n");
+
+	/* 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)
+		return dev_err_probe(imx681->dev, ret,
+				     "endpoint parse failed\n");
+
+	/* Power on and verify chip ID */
+	ret = imx681_power_on(imx681->dev);
+	if (ret)
+		return dev_err_probe(imx681->dev, ret, "power on failed\n");
+
+	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) {
+		ret = dev_err_probe(imx681->dev, ret,
+				    "media entity init failed\n");
+		goto error_handler_free;
+	}
+
+	imx681->sd.state_lock = imx681->ctrl_handler.lock;
+	ret = v4l2_subdev_init_finalize(&imx681->sd);
+	if (ret < 0) {
+		ret = dev_err_probe(imx681->dev, ret,
+				    "subdev init finalize failed\n");
+		goto error_media_entity;
+	}
+
+	ret = v4l2_async_register_subdev_sensor(&imx681->sd);
+	if (ret < 0) {
+		ret = dev_err_probe(imx681->dev, ret,
+				    "async register subdev failed\n");
+		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_put_noidle(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.54.0 (Apple Git-157)

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] media: i2c: imx681: non-atomic control updates due to missing cluster
--

commit acaa5eee3740a4ac3a0c239fabfd5d066ae26848
Author: Andre Gilerson <andre.gilerson@gmail.com>

media: i2c: Add Sony IMX681 sensor driver

This commit adds support for the Sony IMX681 sensor found on Intel Lunar Lake 
devices like the Surface Pro 11. It configures the sensor using 
reverse-engineered I2C sequences and exposes controls via the V4L2 API.

> diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c
> new file mode 100644
> index 0000000000000..825d8c80a29b8
> --- /dev/null
> +++ b/drivers/media/i2c/imx681.c
[ ... ]
> +static int imx681_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	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);

[Severity: Medium]
Does this break hardware atomicity during control updates?

The group hold is toggled independently for every control modification. If a
frame readout boundary occurs between these separate I2C transactions, could
the sensor latch a partial state, causing image tearing or brightness glitches?

[ ... ]
> +static int imx681_init_controls(struct imx681 *imx681)
> +{
[ ... ]
> +	/* 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);

[Severity: Medium]
Are these related controls missing a call to v4l2_ctrl_cluster()?

Because vblank, exposure, and the gains are instantiated but never clustered,
the V4L2 core will invoke imx681_set_ctrl() sequentially for each individual
control rather than passing them as a single atomic update.

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

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

[Sakari Ailus]

Hi Sergey, Andre,

Thanks for the patch.

On Mon, Sep 21, 2026 at 07:25:07PM +0000, Sergey Lebedev wrote:
> 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.

That's no surprise. A commit message should describe a patch; the rest
should go to the cover letter.

> 
> 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>
> Tested-by: German Pablo Lindo <germanpapulindez@gmail.com>
> ---
>  MAINTAINERS                |   7 +
>  drivers/media/i2c/Kconfig  |  10 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/imx681.c | 876 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 894 insertions(+)
>  create mode 100644 drivers/media/i2c/imx681.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4cc4a2dc6d..4479f96d0d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25609,6 +25609,13 @@ 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
> +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 4d99464791..c759a2d239 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -321,6 +321,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 fd1cb25718..98bcecf0c4 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -64,6 +64,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_IT6625) += it6625.o
> diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c
> new file mode 100644
> index 0000000000..825d8c80a2
> --- /dev/null
> +++ b/drivers/media/i2c/imx681.c
> @@ -0,0 +1,876 @@
> +// 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 */

You should define a margin instead, which is subtracted from frame length
to come up with the maximum exposure time. Frame length in lines could
later be made dynamic if it isn't now.

> +#define IMX681_EXPOSURE_OFFSET		4	/* frame_length - exposure_max */
> +#define IMX681_EXPOSURE_DEFAULT		1907	/* from Windows trace */

You could just set it to 0 (or other minimum); the userspace will set it in
any case.

> +
> +/* Analog gain */
> +#define IMX681_REG_ANALOG_GAIN		CCI_REG16(0x0204)
> +#define IMX681_ANA_GAIN_MIN		0
> +#define IMX681_ANA_GAIN_MAX		960	/* 16x, where the analogue stage ends */
> +#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 },

The sensor is already in software stand-by mode here.

> +	/* External clock frequency = 19.2 MHz (encoded as MHz * 256) */
> +	{ CCI_REG16(0x0136), 0x1333 },

Please write the actual clock frequency here.

> +	/* 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 },

This should be set using V4L2 HFLIP controls.

> +	/* 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 },

This should come from the HBLANK control.

> +	/* Frame length = 3177 */
> +	{ CCI_REG16(0x033E), 0x0C69 },

And this from the VBLANK control.

> +	/* 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 },

These registers are the same as in CCS. See discussion here
<URL:https://lore.kernel.org/linux-media/20260827-imx355-ccsify-v2-1-3a97bc708315@ixit.cz/>.

> +	/* PLL multiplier = 225 */
> +	{ CCI_REG8(0x0307),  0xE1 },
> +	/* PLL2: pre_div=3, multiplier=303 (0x012F) */

Looks like the sensor has dual PLL configuration. The PIXEL_RATE then is
unlikely to be what can be calculated from the CSI-2 configuration.

> +	{ 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 */

These should all be written programmatically, not through a register list.

Group hold should make no difference if the sensor isn't streaming.

> +	{ 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)

Just use ARRAY_SIZE(imx681_supply_names) where you need it.

> +
> +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 pm_status;
> +	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);

Presumably this can fail, too.

> +	}
> +
> +	/*
> +	 * 1 with a reference taken, 0 if the device is not active, or -EINVAL
> +	 * if runtime PM is unavailable. Only the 0 means there is nothing to
> +	 * do: without runtime PM the sensor is powered from probe and never
> +	 * suspended, so the write still has to go out - but no reference was
> +	 * taken then, and none may be dropped.
> +	 */

You can omit this comment; there's nothing special about this driver or
device here.

> +	pm_status = pm_runtime_get_if_active(imx681->dev);
> +	if (!pm_status)
> +		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);

I don't think you don't need group hold for these.

> +		dev_dbg(imx681->dev, "set frame_length: %d, ret=%d\n",
> +			IMX681_HEIGHT + ctrl->val, ret);

Similarly, the dev_dbg() call here can be dropped, same below.

> +		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); code 960 is 16x. */
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
> +		cci_write(imx681->cci, IMX681_REG_ANALOG_GAIN, ctrl->val,
> +			  &ret);
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);
> +		dev_dbg(imx681->dev, "set analogue gain code %d, ret=%d\n",
> +			ctrl->val, 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;
> +	}
> +
> +	if (pm_status > 0)
> +		pm_runtime_put(imx681->dev);

A newline here (before return, unless it was conditional)? Same elsewhere,
too.

> +	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++)

You can declare i here.

> +		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,
> +				 const struct v4l2_subdev_client_info *ci,
> +				 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;

The init_state() callback should do this as there's nothing to configure on
hardware. You can drop the set_fmt() callback, too.

> +
> +	return 0;
> +}
> +
> +static int imx681_get_selection(struct v4l2_subdev *sd,
> +				const struct v4l2_subdev_client_info *ci,
> +				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);

Please drop.

> +
> +	/* 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");

Ditto.

> +
> +	/* 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");

Ditto.

> +	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)

The sensor currently supports no other pads so you can drop this check.
Elsewhere, too -- the framework already does this.

> +		return -EINVAL;
> +
> +	ret = pm_runtime_get_sync(imx681->dev);

You could use pm_runtime_resume_and_get() instead...

> +	if (ret < 0) {
> +		pm_runtime_put_noidle(imx681->dev);

And drop this call.

> +		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");

Please drop this.

> +
> +	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");

Ditto.

> +
> +	/* 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)
> +		return dev_err_probe(imx681->dev, ret,
> +				     "failed to read chip ID register 0x0016\n");
> +
> +	if (val != IMX681_CHIP_ID) {
> +		return dev_err_probe(imx681->dev, -EIO,
> +				     "chip ID mismatch: 0x%04llx != 0x%04x\n",
> +				     val, IMX681_CHIP_ID);
> +	}
> +
> +	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;
> +		ret = dev_err_probe(imx681->dev, ret, "control init failed\n");

Uh-oh.

> +		goto error;
> +	}
> +
> +	ret = v4l2_fwnode_device_parse(imx681->dev, &props);

Call this before initialising the handler and error handling is simplified.

> +	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);

	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);

> +	if (!ep) {
> +		return dev_err_probe(imx681->dev, -ENXIO,
> +				     "no endpoint found in firmware node\n");

You can omit this check.

> +	}
> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +	fwnode_handle_put(ep);
> +	if (ret)
> +		return dev_err_probe(imx681->dev, ret,
> +				     "failed to parse endpoint\n");
> +
> +	if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX681_NUM_LANES) {
> +		ret = dev_err_probe(imx681->dev, -EINVAL,
> +				    "expected %d data lanes, got %d\n",
> +				    IMX681_NUM_LANES,
> +				    bus_cfg.bus.mipi_csi2.num_data_lanes);
> +		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)
> +		ret = dev_err_probe(imx681->dev, ret,
> +				    "link frequency mismatch\n");
> +
> +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))
> +		return dev_err_probe(imx681->dev, PTR_ERR(imx681->cci),
> +				     "failed to init CCI\n");
> +
> +	/* 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");
> +
> +	/* Get regulators */
> +	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)
> +		return dev_err_probe(imx681->dev, ret,
> +				     "failed to get regulators\n");
> +
> +	/* 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)
> +		return dev_err_probe(imx681->dev, ret,
> +				     "endpoint parse failed\n");
> +
> +	/* Power on and verify chip ID */
> +	ret = imx681_power_on(imx681->dev);
> +	if (ret)
> +		return dev_err_probe(imx681->dev, ret, "power on failed\n");
> +
> +	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);

Please drop this call and...

> +	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) {
> +		ret = dev_err_probe(imx681->dev, ret,
> +				    "media entity init failed\n");
> +		goto error_handler_free;
> +	}
> +
> +	imx681->sd.state_lock = imx681->ctrl_handler.lock;
> +	ret = v4l2_subdev_init_finalize(&imx681->sd);
> +	if (ret < 0) {
> +		ret = dev_err_probe(imx681->dev, ret,
> +				    "subdev init finalize failed\n");
> +		goto error_media_entity;
> +	}
> +
> +	ret = v4l2_async_register_subdev_sensor(&imx681->sd);
> +	if (ret < 0) {
> +		ret = dev_err_probe(imx681->dev, ret,
> +				    "async register subdev failed\n");
> +		goto error_subdev_cleanup;
> +	}
> +
> +	pm_runtime_put_autosuspend(imx681->dev);

... call pm_runtime_idle() here instead. Error handling changes, too.

> +
> +	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_put_noidle(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);

Call pm_runtime_set_suspended() conditionally, with imx681_power_off().

> +}
> +
> +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");

-- 
Kind regards,

Sakari Ailus

[PATCH v5 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 77257311d1..a1a4023f81 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -116,6 +116,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
 	/* 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.54.0 (Apple Git-157)