media: Add support for the Sony IMX681

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 v2
=============

An automated review of 2/3 was posted into the v1 thread. It raised five things; four are real and are fixed below. The fifth, a claimed NULL-state dereference in imx681_set_pad_format(), I do not accept: the core reaches .set_fmt through v4l2_subdev_call_state_active(), which always supplies a state, and imx415, imx355, ov02c10 and ov08x40 all dereference it unchecked too. My full answer to the review is here:

https://patch.msgid.link/[email protected]

Two of the four are mechanical - 2 and 3 below. The other two are not: 1 reverses something the author wrote deliberately, and 4 is the question this cover put to you in v1. I have decided both rather than holding the series until he is back, and said why, so that review has something to argue with instead of an open question.

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.

Link to v1: https://patch.msgid.link/[email protected]

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
=======

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.

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

The last row is the point: the control reports 4x in both columns, and in v1 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.

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

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/[email protected]/

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

2.53.0

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.

---
 .../bindings/media/i2c/sony,imx681.yaml       | 107 ++++++++++++++++++
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml

new file mode 100644
index 000000000..6d2e4ddce
@@ -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 <[email protected]>
+
+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

From: Andre Gilerson <[email protected]>

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.

---
 MAINTAINERS                |   8 +
 drivers/media/i2c/Kconfig  |  10 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/imx681.c | 880 +++++++++++++++++++++++++++++++++++++
 4 files changed, 899 insertions(+)
 create mode 100644 drivers/media/i2c/imx681.c

index e12dc3ca5..198eaeefa 100644
@@ -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 <[email protected]>
+L:	[email protected]
+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 <[email protected]>
 M:	Alex Dubov <[email protected]>

index 5c52007f9..5f7b0d22c 100644
@@ -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
 

index d04bd5724..8d6bd7466 100644
@@ -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

2.53.0

Correct, and it is a regression I introduced between v1 and v2. The v2 cover letter also offered to spread the same mistake across six more drivers, so that offer needs withdrawing before anything else.

Withdrawing the cleanup offer
=============================

The cover said the "!" form is in gc05a2, gc08a3, imx283, lm3560, og0ve1b and ov6211 and offered a cleanup converting them. Please do not take that up, and if anyone has begun, stop: converting them to "<= 0" would break each of them exactly as it broke this driver.

The reasoning behind the offer was wrong in a specific way worth naming. 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 builds without PM, and that is the whole of the difference.

The finding
===========

Without CONFIG_PM the helper is, unconditionally:

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

v1's "if (!...)" fell through on that and applied the control, which is the right thing to do: with no runtime PM the sensor is powered from probe and never suspended. v2's "<= 0" returns 0 instead and drops it - exposure, gain, blanking, test pattern, all of them, with nothing reported to the caller.

v3 will use the shape ov64a40 already has:

          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. That answers the first review's finding - the put of a reference never taken - without losing the case v1 had right by accident.

ov64a40 is the only fully correct caller of this helper in drivers/media/i2c. I looked at it when the first review landed, wrote it down as a variant of the same thing, and did not read its second half. Had I done so, v2 would have carried this.

If a cleanup for the other six is still wanted, that is the shape it should take, and I will send one only if a maintainer asks.

v3 follows this message. Respinning within hours is not something I would normally do, but nobody has reviewed v2 yet and the alternative is asking people to read code I already know is wrong. The change is ten lines, six of them comment, and 1/3 and 3/3 are byte-identical to v2.

The other finding, on 3/3, which is not ours
============================================

The same run flagged a High issue in ipu-bridge and marked it pre-existing. It is - our 3/3 is two lines of table entry - but I checked it rather than waving it past, and it looks real. Sakari, Dan: this is yours rather than mine.

The reference. sensor->adev = ACPI_PTR(acpi_dev_get(adev)) takes a second reference to the ACPI device. On "goto err_free_swnodes" only the iterator's is dropped, by acpi_dev_put(adev), and bridge->n_sensors has not been incremented yet, so ipu_bridge_unregister_sensors() does not walk this sensor either. Narrow in practice: ipu_bridge_instantiate_ivsc() returns 0 immediately when there is no csi_dev, so reaching it needs a machine with an IVSC.

The fwnode pointers look wider to me. primary->secondary is assigned in ipu_bridge_connect_sensor(), and set_secondary_fwnode(sensor->csi_dev, fwnode) in ipu_bridge_instantiate_ivsc(). Neither is cleared anywhere in the file, while ipu_bridge_unregister_sensors() calls software_node_unregister_node_group() on the nodes they point at. The file walks that chain itself, in ipu_bridge_check_fwnode_graph(fwnode->secondary). That is the ordinary unbind path rather than an error path.

No patch from me for either: I have no IVSC machine and no way to force the failure, and an untested lifetime change in someone else's driver is worth less than a clear report.

Sergey

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.

---
 drivers/media/pci/intel/ipu-bridge.c | 2 ++
 1 file changed, 2 insertions(+)

index 7e65491fa..68c2eb326 100644
@@ -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