* [PATCH v2 0/3] media: Add support for the Sony IMX681
@ 2026-09-09 19:39 Sergey Lebedev
2026-09-09 19:40 ` [PATCH v2 1/3] dt-bindings: media: Add " Sergey Lebedev
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Sergey Lebedev @ 2026-09-09 19:39 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Andre Gilerson, Dan Scally
Cc: Hans de Goede, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
German, linux-media, devicetree, linux-kernel
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/20260909193627.85987-1-lsa.uz@pm.me
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/20260909174240.80023-1-lsa.uz@pm.me
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/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 | 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2 1/3] dt-bindings: media: Add Sony IMX681 2026-09-09 19:39 [PATCH v2 0/3] media: Add support for the Sony IMX681 Sergey Lebedev @ 2026-09-09 19:40 ` Sergey Lebedev 2026-09-09 19:40 ` [PATCH v2 2/3] media: i2c: Add Sony IMX681 sensor driver Sergey Lebedev 2026-09-09 19:40 ` [PATCH v2 3/3] media: ipu-bridge: Add Sony IMX681 Sergey Lebedev 2 siblings, 0 replies; 7+ messages in thread From: Sergey Lebedev @ 2026-09-09 19:40 UTC (permalink / raw) To: Sakari Ailus, Mauro Carvalho Chehab, Andre Gilerson, Dan Scally Cc: Hans de Goede, Rob Herring, Krzysztof Kozlowski, Conor Dooley, German, linux-media, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] media: i2c: Add Sony IMX681 sensor driver 2026-09-09 19:39 [PATCH v2 0/3] media: Add support for the Sony IMX681 Sergey Lebedev 2026-09-09 19:40 ` [PATCH v2 1/3] dt-bindings: media: Add " Sergey Lebedev @ 2026-09-09 19:40 ` Sergey Lebedev 2026-09-09 19:52 ` sashiko-bot 2026-09-09 20:35 ` Sergey Lebedev 2026-09-09 19:40 ` [PATCH v2 3/3] media: ipu-bridge: Add Sony IMX681 Sergey Lebedev 2 siblings, 2 replies; 7+ messages in thread From: Sergey Lebedev @ 2026-09-09 19:40 UTC (permalink / raw) To: Sakari Ailus, Mauro Carvalho Chehab, Andre Gilerson, Dan Scally Cc: Hans de Goede, Rob Herring, Krzysztof Kozlowski, Conor Dooley, German, linux-media, devicetree, linux-kernel 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 | 880 +++++++++++++++++++++++++++++++++++++ 4 files changed, 899 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..bdaf43554 --- /dev/null +++ b/drivers/media/i2c/imx681.c @@ -0,0 +1,880 @@ +// 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 ret = 0; + + /* Update exposure max when VBLANK changes (even when not streaming) */ + if (ctrl->id == V4L2_CID_VBLANK) { + exposure_max = IMX681_HEIGHT + ctrl->val - IMX681_EXPOSURE_OFFSET; + __v4l2_ctrl_modify_range(imx681->exposure, + IMX681_EXPOSURE_MIN, exposure_max, + 1, IMX681_EXPOSURE_DEFAULT); + } + + /* + * Only apply controls to hardware when streaming. The call returns 1 + * having taken a reference, 0 if the device is not active, or -EINVAL + * if runtime PM is disabled - and in that last case no reference is + * taken, so it must not reach the pm_runtime_put() below. + */ + if (pm_runtime_get_if_active(imx681->dev) <= 0) + 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; + } + + 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/3] media: i2c: Add Sony IMX681 sensor driver 2026-09-09 19:40 ` [PATCH v2 2/3] media: i2c: Add Sony IMX681 sensor driver Sergey Lebedev @ 2026-09-09 19:52 ` sashiko-bot 2026-09-09 20:35 ` Sergey Lebedev 1 sibling, 0 replies; 7+ messages in thread From: sashiko-bot @ 2026-09-09 19:52 UTC (permalink / raw) To: Sergey Lebedev; +Cc: media-ci, conor+dt, devicetree, robh Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] V4L2 controls are silently ignored when CONFIG_PM is disabled. -- commit 399d495a390ace2e779563ec50b4c97d653a6b76 Author: Andre Gilerson <andre.gilerson@gmail.com> media: i2c: Add Sony IMX681 sensor driver 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. > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 5c52007f9cbeb..5f7b0d22c8f07 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. > + [ ... ] > diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c > new file mode 100644 > index 0000000000000..bdaf435541a82 > --- /dev/null > +++ b/drivers/media/i2c/imx681.c > @@ -0,0 +1,880 @@ [ ... ] > +static int imx681_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct imx681 *imx681 = container_of(ctrl->handler, struct imx681, > + ctrl_handler); > + s64 exposure_max; > + int ret = 0; > + > + /* Update exposure max when VBLANK changes (even when not streaming) */ > + if (ctrl->id == V4L2_CID_VBLANK) { > + exposure_max = IMX681_HEIGHT + ctrl->val - IMX681_EXPOSURE_OFFSET; > + __v4l2_ctrl_modify_range(imx681->exposure, > + IMX681_EXPOSURE_MIN, exposure_max, > + 1, IMX681_EXPOSURE_DEFAULT); > + } > + > + /* > + * Only apply controls to hardware when streaming. The call returns 1 > + * having taken a reference, 0 if the device is not active, or -EINVAL > + * if runtime PM is disabled - and in that last case no reference is > + * taken, so it must not reach the pm_runtime_put() below. > + */ > + if (pm_runtime_get_if_active(imx681->dev) <= 0) > + return 0; [Severity: Medium] When the driver is built without CONFIG_PM, pm_runtime_get_if_active() returns -EINVAL. Does this code misinterpret that error code as the device being inactive? Since VIDEO_IMX681 doesn't select or depend on PM in Kconfig, returning 0 here when pm_runtime_get_if_active() returns -EINVAL will silently ignore all V4L2 control writes (exposure, gain, blanking, test pattern) when the kernel is compiled without CONFIG_PM. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260909193953.86192-1-lsa.uz@pm.me?part=2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/3] media: i2c: Add Sony IMX681 sensor driver 2026-09-09 19:40 ` [PATCH v2 2/3] media: i2c: Add Sony IMX681 sensor driver Sergey Lebedev 2026-09-09 19:52 ` sashiko-bot @ 2026-09-09 20:35 ` Sergey Lebedev 1 sibling, 0 replies; 7+ messages in thread From: Sergey Lebedev @ 2026-09-09 20:35 UTC (permalink / raw) To: Sakari Ailus, Mauro Carvalho Chehab, Andre Gilerson, Dan Scally Cc: Hans de Goede, Rob Herring, Krzysztof Kozlowski, Conor Dooley, German, sashiko-reviews, linux-media, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] media: ipu-bridge: Add Sony IMX681 2026-09-09 19:39 [PATCH v2 0/3] media: Add support for the Sony IMX681 Sergey Lebedev 2026-09-09 19:40 ` [PATCH v2 1/3] dt-bindings: media: Add " Sergey Lebedev 2026-09-09 19:40 ` [PATCH v2 2/3] media: i2c: Add Sony IMX681 sensor driver Sergey Lebedev @ 2026-09-09 19:40 ` Sergey Lebedev 2026-09-09 19:52 ` sashiko-bot 2 siblings, 1 reply; 7+ messages in thread From: Sergey Lebedev @ 2026-09-09 19:40 UTC (permalink / raw) To: Sakari Ailus, Mauro Carvalho Chehab, Andre Gilerson, Dan Scally Cc: Hans de Goede, Rob Herring, Krzysztof Kozlowski, Conor Dooley, German, linux-media, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] media: ipu-bridge: Add Sony IMX681 2026-09-09 19:40 ` [PATCH v2 3/3] media: ipu-bridge: Add Sony IMX681 Sergey Lebedev @ 2026-09-09 19:52 ` sashiko-bot 0 siblings, 0 replies; 7+ messages in thread From: sashiko-bot @ 2026-09-09 19:52 UTC (permalink / raw) To: Sergey Lebedev; +Cc: robh, media-ci, conor+dt, devicetree Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] Leaked ACPI device reference and dangling secondary fwnode pointers on error paths. -- commit 4b67afbc2a1ae41061ad9ba083bbebdbbe1112a4 Author: Sergey Lebedev <lsa.uz@pm.me> media: ipu-bridge: Add Sony IMX681 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. > 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 is a pre-existing issue, but while reviewing the addition of this new sensor, I noticed potential leaks and dangling pointers in the error handling paths of ipu_bridge_connect_sensor() and ipu_bridge_unregister_sensors(). If ipu_bridge_instantiate_ivsc() fails: drivers/media/pci/intel/ipu-bridge.c:ipu_bridge_connect_sensor() { ... sensor->adev = ACPI_PTR(acpi_dev_get(adev)); primary = acpi_fwnode_handle(adev); primary->secondary = fwnode; ret = ipu_bridge_instantiate_ivsc(sensor); if (ret) goto err_free_swnodes; ... err_free_swnodes: software_node_unregister_node_group(sensor->group); err_put_ivsc: put_device(sensor->csi_dev); acpi_dev_put(sensor->ivsc_adev); err_put_adev: acpi_dev_put(adev); return ret; } Since bridge->n_sensors is not incremented before the failure, the caller ipu_bridge_unregister_sensors() iterates up to the previously successful sensor and skips this failing sensor. Does this mean the acpi_dev_get(adev) reference held by sensor->adev is permanently leaked? Furthermore, primary->secondary is assigned but never cleared on this error path, leaving a dangling pointer. Additionally, in the cleanup function itself: drivers/media/pci/intel/ipu-bridge.c:ipu_bridge_unregister_sensors() { for (i = 0; i < bridge->n_sensors; i++) { sensor = &bridge->sensors[i]; software_node_unregister_node_group(sensor->group); acpi_dev_put(sensor->adev); put_device(sensor->csi_dev); acpi_dev_put(sensor->ivsc_adev); } } Does this code leave dangling secondary fwnode pointers? The function unregisters and frees the software nodes, but it doesn't appear to clear the secondary fwnode pointers on the ACPI or CSI devices first (e.g., setting acpi_fwnode_handle(sensor->adev)->secondary = NULL or calling set_secondary_fwnode(sensor->csi_dev, NULL)). If the ACPI or CSI devices are later queried for fwnode properties, could this trigger a use-after-free? > /* Sony IMX471 */ > IPU_SENSOR_CONFIG("SONY471A", 1, 200000000), > /* Sony IMX471 (found on Lenovo X1 Carbon G14) */ -- Sashiko AI review · https://sashiko.dev/#/patchset/20260909193953.86192-1-lsa.uz@pm.me?part=3 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-09-09 20:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-09 19:39 [PATCH v2 0/3] media: Add support for the Sony IMX681 Sergey Lebedev 2026-09-09 19:40 ` [PATCH v2 1/3] dt-bindings: media: Add " Sergey Lebedev 2026-09-09 19:40 ` [PATCH v2 2/3] media: i2c: Add Sony IMX681 sensor driver Sergey Lebedev 2026-09-09 19:52 ` sashiko-bot 2026-09-09 20:35 ` Sergey Lebedev 2026-09-09 19:40 ` [PATCH v2 3/3] media: ipu-bridge: Add Sony IMX681 Sergey Lebedev 2026-09-09 19:52 ` sashiko-bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox