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

[PATCH v3 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 - not as a degraded device, not 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, the instrumentation is scripted, and I would rather measure what
you ask for than argue about it.

Changes in v3
=============

One fix, and a retraction that matters more than the fix.

The automated review of v2 caught a regression I introduced between v1 and v2,
and it is right. v1 tested pm_runtime_get_if_active() with "!", which I called
a bug - it reads -EINVAL as success and then puts a reference it never took.
v2 changed it to "<= 0", which fixes that and breaks something v1 had right:
without CONFIG_PM the helper returns -EINVAL unconditionally,

  static inline int pm_runtime_get_if_active(struct device *dev)
  {
          return -EINVAL;
  }

so "<= 0" drops every control write - exposure, gain, blanking, test pattern -
silently. With no runtime PM the sensor is powered from probe and never
suspended, so those writes still have to go out.

v3 uses the shape ov64a40 already has, which is the only fully correct caller
of this helper in drivers/media/i2c:

        pm_status = pm_runtime_get_if_active(imx681->dev);
        if (!pm_status)
                return 0;
        ...
        if (pm_status > 0)
                pm_runtime_put(imx681->dev);

0 skips, 1 applies and puts, -EINVAL applies and does not put. Ten lines
changed, six of them comment. 1/3 and 3/3 are byte-identical to v2.

Withdrawing an offer made in the v2 cover letter. It said the "!" form is in
gc05a2, gc08a3, imx283, lm3560, og0ve1b and ov6211 and offered a cleanup
converting them. Please do not take that up: converting them to "<= 0" would
break all six exactly as it broke this driver. If a cleanup is still wanted it
should take ov64a40's shape, and I will send one only if a maintainer asks.

The reasoning behind that offer was wrong in a way worth naming, because it is
easy to repeat. I compared against i915, xe, ipa, ivpu and arm-smmu, which do
test "> 0" or "<= 0" - but they sit where CONFIG_PM is effectively mandatory,
so the third return never reaches them. drivers/media/i2c has no such
guarantee. I also read ov64a40 at the time, wrote it down as a variant of the
same thing, and did not look at its second half.

Blast radius of the v2 defect, since it is fair to ask: nil in practice, and
not to my credit. v2 was on the list for four hours and applied nowhere, and on
x86 with suspend PM_SLEEP is def_bool y and selects PM, so every machine this
sensor exists on forces CONFIG_PM on and could not have hit it. The real
exposure was the cleanup offer, which pointed at six drivers that ship. That is
why it is retracted at the top of this list rather than the bottom.

My full answer to the v2 review, which also carries a report against
ipu-bridge that the same run raised and that is not this series' to fix:

  https://patch.msgid.link/20260909203527.90372-1-lsa.uz@pm.me

Link to v2: https://patch.msgid.link/20260909193953.86192-1-lsa.uz@pm.me
Link to v1: https://patch.msgid.link/20260909174240.80023-1-lsa.uz@pm.me

The four fixes v2 introduced, carried into v3 unchanged
=======================================================

1. devm_regulator_bulk_get() failure is now fatal, via dev_err_probe().

   v1 logged a dev_dbg and continued, and imx681_power_on() then called
   regulator_bulk_enable() on an array nothing had filled in. That is worse
   than it sounds: _regulator_bulk_get() unwinds with regulator_put() and
   leaves the pointers where they are, so a get that fails partway puts freed
   regulators into supplies[] rather than NULLs.

   This reverses a deliberate choice - the code carried the author's comment
   "Continue without regulators - INT3472 may handle power" - so the reasoning
   is worth stating. The get does not fail for supplies the firmware leaves
   undescribed; it hands out dummies, which is what this machine shows for
   dvdd and dovdd. A real failure is therefore exceptional, and the likely one
   is -EPROBE_DEFER, which is what dev_err_probe() exists to handle. Andre can
   overrule me when he is back on the 28th.

2. pm_runtime_get_if_active() is tested against all three of its return values.

   v1 wrote "if (!pm_runtime_get_if_active(dev))", which reads -EINVAL as
   success, touches the sensor over I2C and then pm_runtime_put()s a reference
   it never took. The kerneldoc is explicit that on -EINVAL "the usage_count
   will remain unmodified".

   Beyond this driver, and offered rather than assumed: the same "!" form is in
   gc05a2, gc08a3, imx283, lm3560, og0ve1b and ov6211. Everywhere else in the
   tree that I looked - i915, xe, ipa, ivpu, arm-smmu - it is tested as "> 0"
   or "<= 0". If a cleanup for drivers/media/i2c is wanted I will send one
   separately; it is not in this series.

3. pm_runtime_put_noidle() added on the probe error path, which otherwise
   called pm_runtime_set_suspended() while still holding the reference taken by
   pm_runtime_get_noresume().

4. V4L2_CID_ANALOGUE_GAIN no longer advertises or applies digital gain.

   v1 advertised 0..1020 and made up everything above code 960 by writing
   IMX681_REG_DIGITAL_GAIN - the register V4L2_CID_DIGITAL_GAIN also writes.
   The clash is not limited to that top end, though, and that is what made it
   easy to miss: the analogue handler wrote the digital register on *every*
   call, putting back the 1.0x it had initialised the local to whenever the
   requested code was at or below 960. So setting digital gain to 4x and then
   moving analogue gain anywhere at all silently reset it, with no error and
   with the DIGITAL_GAIN control still reporting 4x.

   Clustering the two controls would have stopped them overwriting each other.
   I did not do that, because it would then need an invented rule for what a
   user means by analogue 1020 together with digital 2x, and there is no honest
   answer: they are two names for one register. Removing the overlap answers the
   clash and the range question together, which is why they turned out to be one
   question rather than two.

   So ANALOGUE_GAIN is now 0..960 - 1x to 16x, what the analogue stage actually
   does - and DIGITAL_GAIN owns 0x020E alone. The driver's own define admitted
   the problem: IMX681_ANA_GAIN_MAX carried the comment "combined
   analog+digital", and V4L2 has no control for combined gain. 256x is still
   reachable, as 16x analogue times 16x digital, which is what the hardware was
   doing all along; the difference is that userspace can now see which half is
   which, and an AE loop that prefers analogue gain in order not to amplify
   noise gets what it asks for. Nothing is broken by narrowing the range,
   because the driver has never been upstream.

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.
Twenty-one, not a thousand-line table, which is the one mercy of this sensor.

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.

Still open, and still Andre's to decide
=======================================

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. Unlike the four above, this is not a bug anyone has
reported against the code; it is an improvement he has already deferred.

Worth saying because it may not be this sensor's fault: the ov13858 on the same
machine has now returned a wrong chip id once as well, 0x1000 against 0xd855,
and it reads its id the same unretried way. Two sensors, one board. I would not
draw a conclusion from two events, but a retry looks cheap either way.

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.

And one that is open in a different sense: 2/3 adds a MAINTAINERS entry naming
Andre as M:, which is the convention for a driver's author but is also an
obligation he has not been asked about, since he is away. It is in the series
because a new driver with no maintainer entry is worse. If he would rather it
named me, or both of us, that is a v3 change and I have put it to him.

Testing, and what it does not reach
===================================

On a Surface Pro 11, front camera, all three patches applied, on a kernel built
from this base rather than 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: supply dvdd not found, using dummy regulator
  imx681 i2c-SONY0681:00: supply dovdd not found, using dummy regulator
  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 two dummy-regulator lines are there deliberately. They are the evidence for
change 1: the get succeeds and hands out dummies for the supplies this firmware
does not describe, and probe still completes with the failure now fatal. Had
that reasoning been wrong, the camera would not have come up at all.

  streams                  3844x2640 SGRBG10, 30.01 fps
  analogue gain sweep      65.0 / 65.5 / 67.7 / 84.2 mean, codes 0/300/700/960
                           (a dark room at midnight; the pedestal is near 64)

Change 4 was measured rather than argued, by reading register 0x020E over i2c
while the sensor streams. The stream has to be running: at stream start
__v4l2_ctrl_handler_setup() applies every control in creation order, analogue
before digital, so digital always wins and the clash is invisible. It bites a
live AE loop, which is the case that matters.

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

The last row is the point: the control reports 4x in both columns, and before
the fix the register disagrees with it and nothing says so. Both modules were
built from the same tree and swapped with rmmod/insmod, so the driver is the
only difference between them. v3 carries this code unchanged from v2 and was
re-measured on the hardware after the PM change above, with the same result.

  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 - CHKDT, LINT, STYLE, and the example
                           extracted and compiled
  build, W=1               imx681.o and ipu-bridge.o, no warnings

And what it does not reach, set out because v2 was caught by exactly the gap
rather than by anything in the list above:

  build configurations     CONFIG_PM is y here and cannot be turned off - on
                           x86 with suspend, PM_SLEEP is def_bool y and selects
                           it. So the !CONFIG_PM path is read rather than
                           compiled, and that is precisely what let v2 through.
                           If anyone has a config where this driver builds with
                           PM off, that is the build I would most like a report
                           from.

  error paths              The regulator get succeeds here because INT3472
                           hands out dummies for dvdd and dovdd, so the fatal
                           path under 1 is reasoned rather than exercised. The
                           same holds for the probe unwind under 3.

  IVSC                     None of INTC1059, INTC1095, INTC100A, INTC10CF,
                           INTC10DE, INTC10E0 or INTC10E1 is present here, so
                           sensor->csi_dev is NULL and ipu-bridge never enters
                           its IVSC paths. That is why the review's finding
                           against ipu-bridge is reported in the answer
                           linked above rather than patched in this series.

  one machine, one part    One Surface Pro 11 and one sensor sample. German
                           Papulindez has the same HID on another machine, but
                           reaches IPU7 through the staging driver, so that is
                           a second path rather than a second test of this one.

  one mode                 3844x2640 SGRBG10 at 969.6 MHz per lane is the only
                           configuration the driver has, and the only one
                           measured.

  no sanitiser run         Not built with KASAN or lockdep. That is a kernel
                           rebuild here rather than a problem, so ask if it
                           would help.

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                    | 884 ++++++++++++++++++
 drivers/media/pci/intel/ipu-bridge.c          |   2 +
 6 files changed, 1012 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml
 create mode 100644 drivers/media/i2c/imx681.c

-- 
2.53.0

[PATCH v3 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

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

[Krzysztof Kozlowski]

On Wed, Sep 09, 2026 at 08:37:30PM +0000, Sergey Lebedev wrote:
> 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>

Just remember that "Tested-by" does not apply here.

> ---
>  .../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>

Andre should ack this.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

[PATCH v3 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 | 884 +++++++++++++++++++++++++++++++++++++
 4 files changed, 903 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..eba792ca1
--- /dev/null
+++ b/drivers/media/i2c/imx681.c
@@ -0,0 +1,884 @@
+// 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,
+				 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 */
+	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) {
+		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_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.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Hardware exposure register becomes out of sync with V4L2 state when VBLANK is changed.
- [Medium] Independent toggling of group hold register defeats atomic parameter updates.
--

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

media: i2c: Add Sony IMX681 sensor driver

This commit adds a new V4L2 I2C sensor driver for the Sony IMX681 camera,
which is used on the Surface Pro 11 for Business. It implements standard
functionality including register initialization, the streams API, and
runtime PM.

> diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c
> new file mode 100644
> index 0000000000000..eba792ca18ef4
> --- /dev/null
> +++ b/drivers/media/i2c/imx681.c
[ ... ]
> +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);

[Severity: Medium]
Does this leave the hardware exposure register out of sync with the V4L2
state?

When VBLANK is modified, the exposure control range is clamped earlier in
this function. However, if the old exposure value is clamped to a new, smaller
maximum, the newly clamped exposure value is never written to the hardware
because VBLANK and EXPOSURE are not clustered. This could leave the sensor's
hardware exposure register with an out-of-bounds value.

> +		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 independent toggling of the group hold register defeat its purpose?

Group hold is intended to defer register updates until a frame boundary to
ensure multiple settings apply atomically. By toggling it independently for
each V4L2 control, simultaneous user updates to VBLANK, EXPOSURE, and GAIN
will take effect individually and may split across different frames,
potentially causing temporary visual tearing or artifacts.

Would it be better to cluster these controls and use a single group hold block
when they are updated together?

> +		dev_dbg(imx681->dev, "set exposure: %d, ret=%d\n",
> +			ctrl->val, ret);
> +		break;
[ ... ]

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

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

[Sergey Lebedev]

What this is answering, since most of you cannot see it
=======================================================

These replies answer sashiko-bot, an automated reviewer at sashiko.dev. Where
its mail goes is worth stating, because it is not here: devicetree, robh,
conor+dt and media-ci, and not linux-media at all. So the findings I am
quoting have not reached most people reading this, and the review of the
btintel series that Luiz pointed me at earlier tonight reached no list at all
and exists only on the site. I am quoting rather than linking for that reason.

Credit before I start disagreeing with it, because it has earned some. This
series is at v3 because of that reviewer. Its pass on v1 raised five things
and four were real: devm_regulator_bulk_get() ignored, which on a partial
failure leaves freed regulators in supplies[] rather than NULLs; a
pm_runtime_get_if_active() tested so that -EINVAL read as success and a
reference never taken was put; ANALOGUE_GAIN and DIGITAL_GAIN fighting over
one register, which I then measured at 0x020E and which is real; and a missing
pm_runtime_put_noidle(). All four are fixed. Its pass on v2 caught a
regression I introduced while fixing the second of those, and that is the
whole reason a v3 exists. None of those would have been found by the testing I
had listed in the v1 cover letter.

The three things below are where I cannot follow it, or where I can and it is
not this series' to change. Set out at length so the reasoning can be checked
rather than taken on trust. If I have any of it wrong I would rather hear it.

Correcting myself on ipu-bridge
===============================

In my answer to the v2 review I wrote that the dangling secondary fwnodes are
reached on "the ordinary unbind path rather than an error path". That is
wrong. ipu_bridge_unregister_sensors() has exactly two callers and both are
inside ipu_bridge_init()'s error unwind; there is no module_exit, no remove
callback and no devm_add_action in the file, and on success struct ipu_bridge
is deliberately left alive, as its own comment says. I described the reach
without grepping for the callers.

So both halves of that finding are error-path only, and the one I called
narrow and the one I called wider are the same width. The pointers are still
real - nothing clears primary->secondary or the csi_dev's secondary anywhere,
and software_node_unregister_node_group() frees what they point at - but
reaching them needs ipu_bridge_init() to fail after sensors are connected.
Smaller than what I claimed, and the correct claim.

The VBLANK/exposure finding: I do not think it holds
====================================================

The review asks whether clamping the exposure range on a VBLANK change leaves
the hardware register out of sync, because the two controls are not clustered.

__v4l2_ctrl_modify_range() writes it itself. From v4l2-ctrls-api.c:

    cur_to_new(ctrl);
    if (validate_new(ctrl, ctrl->p_new)) { ... = def; }
    ...
    value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32;
    if (value_changed)
            ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);

validate_new() reaches std_validate, which clamps an integer control with
ROUND_TO_RANGE - clamp_t(val, minimum, maximum) - rather than rejecting it. So
a clamped exposure is a changed value, set_ctrl() runs the driver's own
s_ctrl for V4L2_CID_EXPOSURE, and the register is written. It also happens in
the right order: exposure shrinks before the frame length that forced it to.

If I have misread the core I would rather be told than leave it, but as far as
I can follow it there is nothing to fix here.

The group hold finding: fair, and not a bug
===========================================

Correct as an observation. Each control's handler opens and closes group hold
around its own write, so setting exposure, gain and VBLANK together through
VIDIOC_S_EXT_CTRLS gives three separate groups that the sensor may apply on
different frames.

Two reasons it is not in this version. V4L2 calls s_ctrl once per control
unless they are clustered, so nothing is being lost that the current shape
could have kept - clustering is what would add the guarantee, not what would
restore it. And ov5675, the nearest driver in-tree that uses a group-hold
register at all, uses it the other way: to make one logical value spread over
several registers atomic, not to group controls.

It is a real improvement and it is the author's design to change. Andre is
back on 28 September; I would rather put it to him than reshape his control
handling on his behalf for a second time in one night. If a maintainer wants
it sooner, say so and it goes in the next version.

One last thing about the reviewer, since it will reach your patches too
=======================================================================

It has now reviewed this series' 3/3 twice, on v2 and on v3, and that patch is
byte-identical between the two - I diffed the files. The v2 run raised an ACPI
reference leak and the fwnode pointers; the v3 run raised only the fwnode
pointers. Same input, so that is run-to-run variance rather than anything
having changed. Read the other way round, two independent runs agreeing on the
fwnode half is the stronger signal in it.

Sergey

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

[Krzysztof Kozlowski]

On Wed, Sep 09, 2026 at 08:37:40PM +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.
> 
> 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 | 884 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 903 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

Drop, I don't think you/Andre have commit rights there.


> +F:	Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml
> +F:	drivers/media/i2c/imx681.c
> +

...

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

return dev_err_probe

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

Drop. clk_summary tells that. No need for obvious debug messages.

Best regards,
Krzysztof

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

[Sergey Lebedev]

All three taken, and this answers your note on 1/3 as well. One of them is
wider than you flagged.

- MAINTAINERS `T:` dropped. You are right that we have no commit rights there,
  and the convention agrees: 14 of 96 drivers/media/i2c entries carry a `T:`
  line and 82 do not.

- `dev_err` then `return ret` after devm_cci_regmap_init_i2c() becomes
  dev_err_probe(). The same shape is in twelve other probe-path sites, so v4
  converts all thirteen rather than only the one quoted. The seven in
  imx681_start_streaming(), imx681_stop_streaming() and imx681_power_on() stay
  as they are - power_on() is reached from runtime-PM resume as well as from
  probe, so it is not a probe path.

- The clk_get_rate() dev_dbg goes.

Recorded for v4: your Reviewed-by, on 1/3 where you gave it. German supplied a
full name after your note - German Pablo Lindo - and his Tested-by goes on 2/3
and nowhere else, since as you say it does not apply to a binding.

v4 is not sent today, and the reason is the other thing you asked for: André
should ack the binding, and he is away until 28 September. Rather than respin
twice I will hold briefly for that and for any comment on 2/3 and 3/3 on their
merits, then send once.

Sergey

[PATCH v3 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 v3 3/3] media: ipu-bridge: Add Sony IMX681

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Dangling secondary fwnode pointers upon unregistration leading to UAF
--

commit 14048fa470e12c0fb57d0c87c84bc92c1497b100
Author: Sergey Lebedev <lsa.uz@pm.me>

media: ipu-bridge: Add Sony IMX681

This patch adds support for the Sony IMX681 user-facing sensor to the IPU
bridge. It ensures the sensor can be properly connected in the graph by
mapping its ACPI identifier.

> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 7e65491faf6dc..68c2eb326f1dc 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),

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing issue
in the IPU bridge setup and teardown path.

In ipu_bridge_connect_sensor(), the ACPI device's secondary fwnode pointer is
manually assigned to point to the newly created software node:

    primary->secondary = fwnode;

Additionally, ipu_bridge_instantiate_ivsc() also sets the secondary fwnode
for the CSI device:

    set_secondary_fwnode(sensor->csi_dev, fwnode);

When an error occurs and err_free_swnodes is reached during module
initialization, or when ipu_bridge_unregister_sensors() is called during
teardown, the underlying software nodes are freed:

    software_node_unregister_node_group(sensor->group);

However, primary->secondary and the csi_dev secondary fwnode are never cleared
back to NULL. Does this leave dangling pointers that could lead to a
use-after-free if another subsystem iterates over or accesses the secondary
fwnodes after the group is unregistered?

>  	/* Sony IMX471 */
>  	IPU_SENSOR_CONFIG("SONY471A", 1, 200000000),

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

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

[German]

I have tested the three patches mentioned in this email in the linux version 7.3.0-rc2 in order to test if they solve the IMX681 problem

##DEVICE USED
Microsoft surface pro 11
Cachy OS

##WORKING:
The camera is totally functional in apps like qcam and firefox after aplying the apropiate configuration and installing the IPU7 Hal and bin of intel

##NOT WORKING:
For some reason, when i tested the camera on gnome snapshot (version 50, istalled with pacman) the colors where completelly wrong (the whole image had a magenta tint to it in addition with some horizontal magenta and yellow lines and a diagonal line that went from the bottom left cornet to the top rigth corner) and the image was inverted

##MINOR ISSUES
all things reported here are little details barely noticable and dont influence on the usability of the device
- When exposed to a really bright ligth the image starts to gain a green color and blue if the room is almost devoided of ligth
- The cammera has some kind of "Snow" on the image captured
- The image captured tends to be brighter than a normal camera, this error increases the more ligth there is in the room

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

[Sergey Lebedev]

German,

Thank you - a second machine, which the v3 cover letter named as the
limitation it most wanted lifted. Your report also found something real.

Your inverted image is the driver
=================================

Prompted by it I held a sheet of printed text in front of the camera and
captured one raw frame: /dev/video16, SGRBG10, a crude 2x2 debayer, nothing
clever. The text came out reversed, and flipping the frame horizontally makes
it read correctly. So the sensor output is horizontally mirrored, always -
not rotated, not flipped vertically.

The other half of that result is worth saying: apart from the mirror the frame
is an ordinary photograph. No tearing, no skew, no diagonals, no wrong
geometry.

The cause is in the init sequence, with the author's comment:

    /* Image orientation: H-flip to match Windows AIQB (RGGB native -> GRBG) */
    { CCI_REG8(0x0101),  0x01 },

The flip makes the Bayer order GRBG, which is what the driver advertises and
what Intel's tuning expects, so the format declaration is honest. The geometry
is not declared at all: there is no V4L2_CID_HFLIP, and camera_orientation
says only that the camera faces front, not that frames arrive mirrored.

Which is why one of your applications is right and the other is not. A front
camera is conventionally mirrored by the application. An app that does that
applies a second flip and gets the true scene - qcam, Firefox. An app that
does not shows our output backwards - Snapshot. Both are behaving sensibly;
the driver put them in that position by transforming the image invisibly.

It is fixable above the driver, so nobody is stuck - but only if userspace
knows, and today it cannot find out.

Bug, or acceptable? I would like the list's view
================================================

There is a case for acceptable: the flip is part of the mode, the advertised
Bayer code matches what is really on the bus, and a front camera ends up
mirrored anyway. I lean the other way: your two applications are what it looks
like when a consumer cannot learn about the transform. But that is one machine
and one opinion, and the code is Andre's, who is back on 28 September.

If it is a bug, two shapes:

  1. RECOMMENDED. Keep the flip, expose V4L2_CID_HFLIP defaulting to 1, and
     swap the advertised code with it - SGRBG when set, SRGGB when clear.
     Nothing changes for anyone who does nothing, and the transform becomes
     visible and controllable. imx415 exposes both flips but does not vary its
     code, so the tree is not a clean precedent.

  2. Drop the flip and advertise SRGGB10, leaving mirroring to userspace.
     Cleaner as a driver, but it changes the advertised format - the very
     thing the flip exists to control - and would likely break the setup you
     have working.

1 costs no existing user anything, which is why I recommend it; 2 is the
better driver if the AIQB turns out not to require GRBG, and that is Andre's
to say. Sakari, Dan, Hans - a view either way settles it and I will do the
work.

Two notes alongside that. Nothing here blocks anyone - the camera is usable as
it stands, as your own report shows. This is about making the driver honest,
not about making it work.

And I have an open thread on libcamera-devel about this sensor's delays, still
waiting on a reply:

  https://lists.libcamera.org/pipermail/libcamera-devel/2026-September/061928.html

libcamera consumes precisely what is in question here - sensor orientation and
the flip controls - so a view from that side bears directly on which option is
right. I have not cross-posted this, because it is a kernel-side decision
first, but I will carry the answer across once there is one.

Your other notes
================

The colour cast with light level, the brightness drift and the snow all look
like AE, AWB and tuning, none of which this driver has: it exposes exposure,
blanking, two gains, link frequency, pixel rate and a test pattern, and that
is all.

Two guesses of mine were wrong, so skip them: the capture node's padded line
(7744 bytes for a 3844-pixel row) would draw about nineteen diagonals, not
your one; and imx681_MSHW0520, MSHW0580 and MSHW0580Second are byte-identical
.aiqb files here, so the variant cannot change colour.

One question that is ours
=========================

v3 changed the gain ABI and you tested v3. Up to v2, V4L2_CID_ANALOGUE_GAIN
advertised 0..1020 and quietly drove the digital gain register above code 960,
so one control could reach 256x; in v3 it stops at 960, the 16x the analogue
stage actually does. If your HAL drives only ANALOGUE_GAIN it now has sixteen
times less range - so did brightness behave differently on v1 or v2? If you
only ran v3, saying so is just as useful.

If you are comfortable with it, a

  Tested-by: German <germanpapulindez@gmail.com>

would carry weight: a second machine and a second userspace stack. It would
not be claiming Snapshot works or that the tuning is right.

Thanks again.

Sergey

Test on another machine and userspace for [PATCH v3 0/3] media: Add support for the Sony IMX681 Thanks for clarifying the origin of the errors I am more than happy to provide the tag Tested-by: German <germanpapulindez@gmail.com>

[German]

Test on another machine and userspace for [PATCH v3 0/3] media: Add support for the Sony IMX681

[German]

 Thanks for clarifying the origin of the errors
 I am more than happy to provide the tag
 Tested-by: German <germanpapulindez@gmail.com>

Re: Test on another machine and userspace for [PATCH v3 0/3] media: Add support for the Sony IMX681

[Krzysztof Kozlowski]

On Thu, Sep 10, 2026 at 01:53:32PM +0200, German wrote:
>  Thanks for clarifying the origin of the errors
>  I am more than happy to provide the tag
>  Tested-by: German <germanpapulindez@gmail.com>

Please use full name for known identities. In general we do not deal
with anonynymous folks.

Best regards,
Krzysztof

cause of the image error in Gnome Snapshot [PATCH v3 0/3] media: Add support for the Sony IMX681

[German]

From: German Pablo Lindo <germanpapulindez@gmail.com>

I think i found (at least in my system) why Gnome Snapshot dosent work, i dont know if this error is also present on your machine with the patches, so i have organiced this message with commands for you to be able to directly copy-paste them and make tests

There are resolution in wich the cammera works fine like:
qcam --stream "width=1280,height=720" the resolution that uses google meet
qcam --stream "width=640,height=480" resolution on a web camera test online
qcam whithout the stream parameter, which has a cuality of 3836 × 2640

But there are some that experience the violet and lines error i reported o my previous test
qcam --stream "width=1920,height=1080" this one is the resolution gnome snapshot uses
qcam --stream "width=1440,height=810"
qcam --stream "width=960,height=540"

I hope this helps in the development of the driver