[PATCH RFC 0/5] media: Fault-Tolerant V4L2

[PATCH RFC 0/5] media: Fault-Tolerant V4L2

[Mattijs Korpershoek]

On media-centric camera pipelines, every entity is known at boot time
and described statically (for example via device-tree).

The V4L2 framework assumes that every entity of the camera
pipeline is always present. This is a reasonable assumption for
integrated cameras in laptops or phones. However, in automotive systems,
cameras (e.g. rear-view or surround-view) can be damaged or disconnected.
When the framework assumes all sensors are present, a single missing
camera can prevent the entire pipeline from operating.

This is a known problem and has been discussed at media summit a couple of times:
https://www.linuxtv.org/downloads/presentations/media_summit_2026/Michael%20-%20The%20Butterfly%20Effect.pdf
https://www.linuxtv.org/news.php?entry=2022-11-14-0.hverkuil

For example, in the following pipeline:
"""
  imx219  9-0010 -> ds90ub953 7-0044 -> ds90ub960 pad0 -+
                                                        |-- pad4 -> cdns_csi2rx -> ticsi2rx
  imx219 10-0010 -> ds90ub953 7-0045 -> ds90ub960 pad1 -+
"""

If imx219 9-0010 fails to probe, media-ctl -p will show:
"""
                    ds90ub953 7-0044 -> ds90ub960 pad0 -+
                                                        |-- pad4 -> cdns_csi2rx -> ticsi2rx
  imx219 10-0010 -> ds90ub953 7-0045 -> ds90ub960 pad1 -+
"""

Now, if ds90ub953 7-0044 fails to probe(), media-ctl -p will only show the
following topology:
"""
                                                                    cdns_csi2rx -> ticsi2rx
"""

Which is unexpected - we should be able to use the imx219 10-0010 sensor
if it's connected.
Also, the media graph should reflect the hardware topology, not just
what's currently working. This way, userspace can distinguish
"camera X is disconnected" from "camera X does not exist".

Applications thus need to know if a sensor or an entity is missing, and
to react when it becomes available, or when an available one becomes
missing.

To solve this, we chose to go for a simple solution inspired by
DRM connectors: we created an ioctl for subdevs to report their
current connection status.
This allows applications to discover a sensor state when discovering
the graph.
The current status is then polled or updated on a regular basis and
reported as a uevent every time it changes, allowing applications to
react.
This mandates that the sensor driver can probe even when the device is
not connected.

This has the benefits of being fairly simple to implement, understand
and support in drivers.

The major downside however is that it will require to modify every
driver since we're breaking away from the current pattern of not
probing if the device isn't available.

The last three patches implement this pattern switch for the
imx219 driver.

With this, we can hot-plug the sensor and get notified via a udev
script to start a capture:

Oct 01 12:02:38 am69-sk run_capture[1463]: /dev/v4l-subdev6 is disconnected
[   69.258616] imx219 10-0010: Error reading reg 0x0000: -121
[   69.264182] imx219 10-0010: Error reading reg 0x0000: -121
[   71.306593] imx219 10-0010: Error reading reg 0x0000: -121
[   71.312158] imx219 10-0010: Error reading reg 0x0000: -121
Oct 01 12:02:44 am69-sk run_capture[1480]: /dev/v4l-subdev6 is connected
Oct 01 12:02:44 am69-sk run_capture[1481]: /dev/v4l-subdev6: Setting up routes
Oct 01 12:02:44 am69-sk run_capture[1487]: /dev/v4l-subdev6: Setting up format for imx219 10-0010
Oct 01 12:02:44 am69-sk run_capture[1489]: /dev/v4l-subdev6: Setting up format for remaining entities
Oct 01 12:02:44 am69-sk run_capture[1506]: /dev/v4l-subdev6: Starting capture on /dev/video5

The code for this demo is available at:
https://gitlab.com/-/snippets/6064180

Current known limitations:
- Only leaf entities (typically sensor nodes) are covered. We should
  also address entities that are in the middle of the media graph

- We don't handle the case where we attempt to stream on a disconnected
  sensor.

- The media graph is not updated to reflect the sensor's presence/absence.
  I have assumed that the media graph must reflect the hardware
  description (i.e. the device-tree) of the pipeline, not the actual
  real state (when a sensor is missing for example). Is that a wrong
  assumption?

I don't expect this approach to be useable or mergeable as is but rather
to use this as a starting point for discussions at Plumbers and on the
mailing list.

This has been tested on a SK-AM69 board on top of the following series:
https://lore.kernel.org/all/20260824121451.3348583-1-sakari.ailus@linux.intel.com/

Thanks,
Mattijs

---
Mattijs Korpershoek (5):
      media: imx219: Move LP-11 state switch to power_on()
      media: v4l2-subdev: Add new ioctl for connection status
      media: imx219: Allow driver probe with missing sensor
      media: imx219: Implement .detect() sensor operation
      media: imx219: Add status polling using .detect()

 .../userspace-api/media/v4l/user-func.rst          |   1 +
 .../v4l/vidioc-subdev-g-connection-status.rst      |  89 ++++++++++
 drivers/media/i2c/imx219.c                         | 191 ++++++++++++++-------
 drivers/media/v4l2-core/v4l2-subdev.c              |   8 +
 include/media/v4l2-subdev.h                        |   3 +
 include/uapi/linux/v4l2-subdev.h                   |  12 ++
 6 files changed, 245 insertions(+), 59 deletions(-)
---
base-commit: fe2ec83746e501645709761605c2464a44fd2929
change-id: 20260923-v4l2-sensor-detect-32a0b2936cc1

Best regards,
-- 
Mattijs Korpershoek <mkorpershoek@kernel.org>

[PATCH RFC 1/5] media: imx219: Move LP-11 state switch to power_on()

[Mattijs Korpershoek]

During probe(), we write the IMX219_MODE_STREAMING register to
transition from streaming -> standby to force LP-11 state.

This should be done at each power-up of the sensor, but is only
done *once* for the driver lifecycle (at probe).

Move the LP-11 sequence to power_on() to ensure that it's always put
into standby mode whenever the pm framework detects it's a power up.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
---
 drivers/media/i2c/imx219.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 9571f3622d2d..7978fee5f4a2 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -1056,8 +1056,32 @@ static int imx219_power_on(struct device *dev)
 	usleep_range(IMX219_XCLR_MIN_DELAY_US,
 		     IMX219_XCLR_MIN_DELAY_US + IMX219_XCLR_DELAY_RANGE_US);
 
+	/*
+	 * Sensor doesn't enter LP-11 state upon power up until and unless
+	 * streaming is started, so upon power up switch the modes to:
+	 * streaming -> standby
+	 */
+	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
+			IMX219_MODE_STREAMING, NULL);
+	if (ret < 0)
+		goto gpio_off;
+
+	usleep_range(100, 110);
+
+	/* put sensor back to standby mode */
+	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
+			IMX219_MODE_STANDBY, NULL);
+	if (ret < 0)
+		goto gpio_off;
+
+	usleep_range(100, 110);
+
 	return 0;
 
+gpio_off:
+	gpiod_set_value_cansleep(imx219->reset_gpio, 0);
+	clk_disable_unprepare(imx219->xclk);
+
 reg_off:
 	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
 
@@ -1240,26 +1264,6 @@ static int imx219_probe(struct i2c_client *client)
 	if (ret)
 		goto error_power_off;
 
-	/*
-	 * Sensor doesn't enter LP-11 state upon power up until and unless
-	 * streaming is started, so upon power up switch the modes to:
-	 * streaming -> standby
-	 */
-	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
-			IMX219_MODE_STREAMING, NULL);
-	if (ret < 0)
-		goto error_power_off;
-
-	usleep_range(100, 110);
-
-	/* put sensor back to standby mode */
-	ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
-			IMX219_MODE_STANDBY, NULL);
-	if (ret < 0)
-		goto error_power_off;
-
-	usleep_range(100, 110);
-
 	ret = imx219_init_controls(imx219);
 	if (ret)
 		goto error_power_off;

-- 
2.55.0

[PATCH RFC 2/5] media: v4l2-subdev: Add new ioctl for connection status

[Mattijs Korpershoek]

The V4L2 framework has historically assumed that every component of the
camera pipeline is always present. This is a reasonable assumption for
integrated cameras in laptops or phones. However, in automotive systems,
cameras (e.g. rear-view or surround-view) can be damaged or
disconnected.

When a camera gets disconnected, userspace cannot detect this and thus
cannot react (for example, by disabling streaming on a faulty camera).

Add a new v4l2-subdev ioctl (VIDIOC_SUBDEV_G_CONNECTION_STATUS) that
drivers can implement to report their connection status.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
---
 .../userspace-api/media/v4l/user-func.rst          |  1 +
 .../v4l/vidioc-subdev-g-connection-status.rst      | 89 ++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-subdev.c              |  8 ++
 include/media/v4l2-subdev.h                        |  3 +
 include/uapi/linux/v4l2-subdev.h                   | 12 +++
 5 files changed, 113 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
index 5fc95c792408..3d7fcfbfabde 100644
--- a/Documentation/userspace-api/media/v4l/user-func.rst
+++ b/Documentation/userspace-api/media/v4l/user-func.rst
@@ -75,6 +75,7 @@ Function Reference
     vidioc-subdev-g-routing
     vidioc-subdev-g-selection
     vidioc-subdev-g-client-cap
+    vidioc-subdev-g-connection-status
     vidioc-subdev-querycap
     vidioc-subscribe-event
     func-mmap
diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-connection-status.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-connection-status.rst
new file mode 100644
index 000000000000..1ea83e5bbbcd
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-connection-status.rst
@@ -0,0 +1,89 @@
+.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
+.. c:namespace:: V4L
+
+.. _VIDIOC_SUBDEV_G_CONNECTION_STATUS:
+
+****************************************
+ioctl VIDIOC_SUBDEV_G_CONNECTION_STATUS
+****************************************
+
+Name
+====
+
+VIDIOC_SUBDEV_G_CONNECTION_STATUS - Query whether the sub-device hardware is
+physically connected.
+
+Synopsis
+========
+
+.. c:macro:: VIDIOC_SUBDEV_G_CONNECTION_STATUS
+
+``int ioctl(int fd, VIDIOC_SUBDEV_G_CONNECTION_STATUS, struct v4l2_subdev_connected_status *argp)``
+
+Arguments
+=========
+
+``fd``
+    File descriptor returned by :c:func:`open()`.
+
+``argp``
+    Pointer to struct :c:type:`v4l2_subdev_connected_status`.
+
+Description
+===========
+
+The ``VIDIOC_SUBDEV_G_CONNECTION_STATUS`` ioctl allows userspace to query
+whether the hardware behind a V4L2 sub-device is physically connected.
+
+The ioctl takes a pointer to a struct :c:type:`v4l2_subdev_connected_status`
+which is filled by the driver. The driver probes the hardware and reports the
+connection status.
+
+.. tabularcolumns:: |p{1.5cm}|p{2.9cm}|p{12.9cm}|
+
+.. c:type:: v4l2_subdev_connected_status
+
+.. flat-table:: struct v4l2_subdev_connected_status
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       3 4 20
+
+    * - __u32
+      - ``status``
+      - Connection status of the sub-device, see
+	:ref:`subdev-connection-status`.
+    * - __u32
+      - ``reserved``\ [7]
+      - Reserved for future extensions. Set to 0 by the V4L2 core.
+
+.. tabularcolumns:: |p{6.8cm}|p{2.4cm}|p{8.1cm}|
+
+.. _subdev-connection-status:
+
+.. flat-table:: Connection Status Values
+    :header-rows:  1
+    :stub-columns: 0
+    :widths:       3 1 4
+
+    * - Status
+      - Value
+      - Description
+    * - ``V4L2_SUBDEV_STATUS_CONNECTED``
+      - 1
+      - The sub-device hardware is connected and reachable.
+    * - ``V4L2_SUBDEV_STATUS_DISCONNECTED``
+      - 2
+      - The sub-device hardware is not connected or not reachable.
+    * - ``V4L2_SUBDEV_STATUS_UNKNOWN``
+      - 3
+      - The connection status could not be determined.
+
+Return Value
+============
+
+On success 0 is returned, on error -1 and the ``errno`` variable is set
+appropriately. The generic error codes are described at the
+:ref:`Generic Error Codes <gen-errors>` chapter.
+
+ENOIOCTLCMD
+   The kernel does not support this ioctl.
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index e9f81b9be9e2..bfd5c5a6a060 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1142,6 +1142,14 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
 		return 0;
 	}
 
+	case VIDIOC_SUBDEV_G_CONNECTION_STATUS: {
+		struct v4l2_subdev_connected_status *status = arg;
+
+		memset(status->reserved, 0, sizeof(status->reserved));
+
+		return v4l2_subdev_call(sd, sensor, detect, status);
+	}
+
 	default:
 		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
 	}
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d256b7ec8f84..d47d6eae6177 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -550,10 +550,13 @@ struct v4l2_subdev_vbi_ops {
  * @g_skip_frames: number of frames to skip at stream start. This is needed for
  *		   buggy sensors that generate faulty frames when they are
  *		   turned on.
+ * @detect: query whether the sensor is physically connected and reachable
  */
 struct v4l2_subdev_sensor_ops {
 	int (*g_skip_top_lines)(struct v4l2_subdev *sd, u32 *lines);
 	int (*g_skip_frames)(struct v4l2_subdev *sd, u32 *frames);
+	int (*detect)(struct v4l2_subdev *sd,
+		      struct v4l2_subdev_connected_status *status);
 };
 
 /**
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
index 2347e266cf75..8d50c73e8ec5 100644
--- a/include/uapi/linux/v4l2-subdev.h
+++ b/include/uapi/linux/v4l2-subdev.h
@@ -268,6 +268,17 @@ struct v4l2_subdev_client_capability {
 	__u64 capabilities;
 };
 
+enum v4l2_subdev_connected_status_whence {
+	V4L2_SUBDEV_STATUS_CONNECTED = 1,
+	V4L2_SUBDEV_STATUS_DISCONNECTED = 2,
+	V4L2_SUBDEV_STATUS_UNKNOWN = 3,
+};
+
+struct v4l2_subdev_connected_status {
+	__u32 status;
+	__u32 reserved[7];
+};
+
 /* Backwards compatibility define --- to be removed */
 #define v4l2_subdev_edid v4l2_edid
 
@@ -287,6 +298,7 @@ struct v4l2_subdev_client_capability {
 #define VIDIOC_SUBDEV_S_ROUTING			_IOWR('V', 39, struct v4l2_subdev_routing)
 #define VIDIOC_SUBDEV_G_CLIENT_CAP		_IOR('V',  101, struct v4l2_subdev_client_capability)
 #define VIDIOC_SUBDEV_S_CLIENT_CAP		_IOWR('V',  102, struct v4l2_subdev_client_capability)
+#define VIDIOC_SUBDEV_G_CONNECTION_STATUS	_IOR('V',  103, struct v4l2_subdev_connected_status)
 
 /* The following ioctls are identical to the ioctls in videodev2.h */
 #define VIDIOC_SUBDEV_G_STD			_IOR('V', 23, v4l2_std_id)

-- 
2.55.0

[PATCH RFC 3/5] media: imx219: Allow driver probe with missing sensor

[Mattijs Korpershoek]

Probe() should complete even when a sensor is disconnected. This would
allow the v4l-subdev to be created and improve fault tolerance.

Currently, the driver reads the CHIP_ID over i2c in the probe().
When we can't read CHIP_ID, the probe errors out - which result in the
v4l2-subdev not being created.

Remove all i2c communications to allow the driver to probe with a
missing sensor.

Note: Since we no longer power on the sensor during probe, the driver
now starts in suspended mode by default.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
---
 drivers/media/i2c/imx219.c | 72 +++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 39 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 7978fee5f4a2..aeac70123b9b 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -1000,6 +1000,29 @@ static int imx219_init_state(struct v4l2_subdev *sd,
 	return imx219_set_pad_format(sd, state, &fmt);
 }
 
+/* Verify chip ID */
+static int imx219_identify_module(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int ret;
+	u64 val;
+
+	ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
+	if (ret) {
+		dev_dbg(&client->dev, "failed to read chip id %x\n",
+			IMX219_CHIP_ID);
+		return ret;
+	}
+
+	if (val != IMX219_CHIP_ID) {
+		dev_dbg(&client->dev, "chip id mismatch: %x!=%llx\n",
+			IMX219_CHIP_ID, val);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static const struct v4l2_subdev_video_ops imx219_video_ops = {
 	.s_stream = v4l2_subdev_s_stream_helper,
 };
@@ -1056,6 +1079,14 @@ static int imx219_power_on(struct device *dev)
 	usleep_range(IMX219_XCLR_MIN_DELAY_US,
 		     IMX219_XCLR_MIN_DELAY_US + IMX219_XCLR_DELAY_RANGE_US);
 
+	/*
+	 * If we can't identify the module here, it might be disconnected.
+	 * Consider power_on() complete and exit early in that case.
+	 */
+	ret = imx219_identify_module(imx219);
+	if (ret)
+		return 0;
+
 	/*
 	 * Sensor doesn't enter LP-11 state upon power up until and unless
 	 * streaming is started, so upon power up switch the modes to:
@@ -1117,27 +1148,6 @@ static int imx219_get_regulators(struct imx219 *imx219)
 				       imx219->supplies);
 }
 
-/* Verify chip ID */
-static int imx219_identify_module(struct imx219 *imx219)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
-	int ret;
-	u64 val;
-
-	ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
-	if (ret)
-		return dev_err_probe(&client->dev, ret,
-				     "failed to read chip id %x\n",
-				     IMX219_CHIP_ID);
-
-	if (val != IMX219_CHIP_ID)
-		return dev_err_probe(&client->dev, -EIO,
-				     "chip id mismatch: %x!=%llx\n",
-				     IMX219_CHIP_ID, val);
-
-	return 0;
-}
-
 static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
 {
 	struct fwnode_handle *endpoint;
@@ -1252,21 +1262,9 @@ static int imx219_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(imx219->reset_gpio),
 				     "failed to get reset gpio\n");
 
-	/*
-	 * The sensor must be powered for imx219_identify_module()
-	 * to be able to read the CHIP_ID register
-	 */
-	ret = imx219_power_on(dev);
-	if (ret)
-		return ret;
-
-	ret = imx219_identify_module(imx219);
-	if (ret)
-		goto error_power_off;
-
 	ret = imx219_init_controls(imx219);
 	if (ret)
-		goto error_power_off;
+		return ret;
 
 	/* Initialize subdev */
 	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -1288,7 +1286,7 @@ static int imx219_probe(struct i2c_client *client)
 		goto error_media_entity;
 	}
 
-	pm_runtime_set_active(dev);
+	pm_runtime_set_suspended(dev);
 	pm_runtime_enable(dev);
 
 	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
@@ -1298,7 +1296,6 @@ static int imx219_probe(struct i2c_client *client)
 		goto error_subdev_cleanup;
 	}
 
-	pm_runtime_idle(dev);
 	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 
@@ -1315,9 +1312,6 @@ static int imx219_probe(struct i2c_client *client)
 error_handler_free:
 	imx219_free_controls(imx219);
 
-error_power_off:
-	imx219_power_off(dev);
-
 	return ret;
 }
 

-- 
2.55.0

[PATCH RFC 4/5] media: imx219: Implement .detect() sensor operation

[Mattijs Korpershoek]

Now that the driver can probe() with a missing sensor, userspace should
be able to query the sensor connection state from the v4l2-subdev.

Implement the .detect() sensor operation so that userspace can do that
using the VIDIOC_SUBDEV_G_CONNECTION_STATUS ioctl.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
---
 drivers/media/i2c/imx219.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index aeac70123b9b..e198d3fe99c6 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -28,6 +28,7 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-mediabus.h>
+#include <media/v4l2-subdev.h>
 
 /* Chip ID */
 #define IMX219_REG_CHIP_ID		CCI_REG16(0x0000)
@@ -1023,6 +1024,35 @@ static int imx219_identify_module(struct imx219 *imx219)
 	return 0;
 }
 
+static int imx219_detect(struct v4l2_subdev *sd,
+			 struct v4l2_subdev_connected_status *status)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	struct device *dev = &client->dev;
+	int ret;
+
+	/*
+	 * The sensor must be powered for imx219_identify_module()
+	 * to be able to read the CHIP_ID register
+	 */
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		status->status = V4L2_SUBDEV_STATUS_UNKNOWN;
+		return ret;
+	}
+
+	ret = imx219_identify_module(imx219);
+	if (ret)
+		status->status = V4L2_SUBDEV_STATUS_DISCONNECTED;
+	else
+		status->status = V4L2_SUBDEV_STATUS_CONNECTED;
+
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+
 static const struct v4l2_subdev_video_ops imx219_video_ops = {
 	.s_stream = v4l2_subdev_s_stream_helper,
 };
@@ -1037,9 +1067,14 @@ static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
 	.disable_streams = imx219_disable_streams,
 };
 
+static const struct v4l2_subdev_sensor_ops imx219_sensor_ops = {
+	.detect = imx219_detect,
+};
+
 static const struct v4l2_subdev_ops imx219_subdev_ops = {
 	.video = &imx219_video_ops,
 	.pad = &imx219_pad_ops,
+	.sensor = &imx219_sensor_ops,
 };
 
 static const struct v4l2_subdev_internal_ops imx219_internal_ops = {

-- 
2.55.0

[PATCH RFC 5/5] media: imx219: Add status polling using .detect()

[Mattijs Korpershoek]

Userspace needs to be notified when a sensor connection status
changes (e.g. disconnected at boot, then later reconnected) so it can
react accordingly.

Add periodic polling using a delayed work that calls .detect() every
2s and sends a KOBJ_CHANGE uevent with HOTPLUG=1 on status changes.
This mirrors the approach used by DRM connectors in output_poll_execute().

Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
---
 drivers/media/i2c/imx219.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index e198d3fe99c6..76e578a8eab5 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -20,6 +20,7 @@
 #include <linux/i2c.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
+#include <linux/workqueue.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
@@ -374,6 +375,9 @@ struct imx219 {
 
 	/* Two or Four lanes */
 	u8 lanes;
+
+	struct delayed_work detect_work;
+	enum v4l2_subdev_connected_status_whence detect_status;
 };
 
 static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
@@ -1252,6 +1256,35 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
 	return ret;
 }
 
+#define IMX219_DETECT_INTERVAL_MS 2000
+static void imx219_detect_work(struct work_struct *work)
+{
+	struct imx219 *imx219 = container_of(work, struct imx219,
+					     detect_work.work);
+	struct v4l2_subdev_connected_status status = {};
+
+	/*
+	 * All async notifiers should have been run before
+	 * we can use sd.devnode
+	 */
+	if (!imx219->sd.devnode)
+		goto reschedule_detect_work;
+
+	imx219_detect(&imx219->sd, &status);
+
+	if (status.status != imx219->detect_status) {
+		struct device *dev = &imx219->sd.devnode->dev;
+		char *envp[] = { "HOTPLUG=1", NULL };
+
+		imx219->detect_status = status.status;
+		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
+	}
+
+reschedule_detect_work:
+	schedule_delayed_work(&imx219->detect_work,
+			      msecs_to_jiffies(IMX219_DETECT_INTERVAL_MS));
+}
+
 static int imx219_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -1334,6 +1367,11 @@ static int imx219_probe(struct i2c_client *client)
 	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 
+	imx219->detect_status = V4L2_SUBDEV_STATUS_UNKNOWN;
+	INIT_DELAYED_WORK(&imx219->detect_work, imx219_detect_work);
+	schedule_delayed_work(&imx219->detect_work,
+			      msecs_to_jiffies(IMX219_DETECT_INTERVAL_MS));
+
 	return 0;
 
 error_subdev_cleanup:
@@ -1355,6 +1393,8 @@ static void imx219_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct imx219 *imx219 = to_imx219(sd);
 
+	cancel_delayed_work_sync(&imx219->detect_work);
+
 	v4l2_async_unregister_subdev(sd);
 	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&sd->entity);

-- 
2.55.0

Re: [PATCH RFC 5/5] media: imx219: Add status polling using .detect()

[Dave Stevenson]

Hi Mattij

On Thu, 1 Oct 2026 at 13:55, Mattijs Korpershoek
<mkorpershoek@kernel.org> wrote:
>
> Userspace needs to be notified when a sensor connection status
> changes (e.g. disconnected at boot, then later reconnected) so it can
> react accordingly.
>
> Add periodic polling using a delayed work that calls .detect() every
> 2s and sends a KOBJ_CHANGE uevent with HOTPLUG=1 on status changes.
> This mirrors the approach used by DRM connectors in output_poll_execute().

AIUI DRM polls from within the framework (drm_probe_helper.c), not by
a workqueue in the individual drivers.

Admittedly V4L2 doesn't currently have a totally obvious place to
setup this, but it would be far less effort to have the polling
framework within the core code rather than driver.
Possibly initialised in __v4l2_async_register_subdev_sensor() based on
whether .detect is set, and cleaned up in
v4l2_async_unregister_subdev, with the workqueue calling .detect and
generating the udev event based on the return value? I think that's
feasible.

  Dave

> Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
> ---
>  drivers/media/i2c/imx219.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index e198d3fe99c6..76e578a8eab5 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -20,6 +20,7 @@
>  #include <linux/i2c.h>
>  #include <linux/minmax.h>
>  #include <linux/module.h>
> +#include <linux/workqueue.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>
> @@ -374,6 +375,9 @@ struct imx219 {
>
>         /* Two or Four lanes */
>         u8 lanes;
> +
> +       struct delayed_work detect_work;
> +       enum v4l2_subdev_connected_status_whence detect_status;
>  };
>
>  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> @@ -1252,6 +1256,35 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
>         return ret;
>  }
>
> +#define IMX219_DETECT_INTERVAL_MS 2000
> +static void imx219_detect_work(struct work_struct *work)
> +{
> +       struct imx219 *imx219 = container_of(work, struct imx219,
> +                                            detect_work.work);
> +       struct v4l2_subdev_connected_status status = {};
> +
> +       /*
> +        * All async notifiers should have been run before
> +        * we can use sd.devnode
> +        */
> +       if (!imx219->sd.devnode)
> +               goto reschedule_detect_work;
> +
> +       imx219_detect(&imx219->sd, &status);
> +
> +       if (status.status != imx219->detect_status) {
> +               struct device *dev = &imx219->sd.devnode->dev;
> +               char *envp[] = { "HOTPLUG=1", NULL };
> +
> +               imx219->detect_status = status.status;
> +               kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> +       }
> +
> +reschedule_detect_work:
> +       schedule_delayed_work(&imx219->detect_work,
> +                             msecs_to_jiffies(IMX219_DETECT_INTERVAL_MS));
> +}
> +
>  static int imx219_probe(struct i2c_client *client)
>  {
>         struct device *dev = &client->dev;
> @@ -1334,6 +1367,11 @@ static int imx219_probe(struct i2c_client *client)
>         pm_runtime_set_autosuspend_delay(dev, 1000);
>         pm_runtime_use_autosuspend(dev);
>
> +       imx219->detect_status = V4L2_SUBDEV_STATUS_UNKNOWN;
> +       INIT_DELAYED_WORK(&imx219->detect_work, imx219_detect_work);
> +       schedule_delayed_work(&imx219->detect_work,
> +                             msecs_to_jiffies(IMX219_DETECT_INTERVAL_MS));
> +
>         return 0;
>
>  error_subdev_cleanup:
> @@ -1355,6 +1393,8 @@ static void imx219_remove(struct i2c_client *client)
>         struct v4l2_subdev *sd = i2c_get_clientdata(client);
>         struct imx219 *imx219 = to_imx219(sd);
>
> +       cancel_delayed_work_sync(&imx219->detect_work);
> +
>         v4l2_async_unregister_subdev(sd);
>         v4l2_subdev_cleanup(sd);
>         media_entity_cleanup(&sd->entity);
>
> --
> 2.55.0
>

Re: [PATCH RFC 3/5] media: imx219: Allow driver probe with missing sensor

[Dave Stevenson]

Hi Mattijs

On Thu, 1 Oct 2026 at 13:55, Mattijs Korpershoek
<mkorpershoek@kernel.org> wrote:
>
> Probe() should complete even when a sensor is disconnected. This would
> allow the v4l-subdev to be created and improve fault tolerance.
>
> Currently, the driver reads the CHIP_ID over i2c in the probe().
> When we can't read CHIP_ID, the probe errors out - which result in the
> v4l2-subdev not being created.
>
> Remove all i2c communications to allow the driver to probe with a
> missing sensor.
>
> Note: Since we no longer power on the sensor during probe, the driver
> now starts in suspended mode by default.
>
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
> ---
>  drivers/media/i2c/imx219.c | 72 +++++++++++++++++++++-------------------------
>  1 file changed, 33 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 7978fee5f4a2..aeac70123b9b 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -1000,6 +1000,29 @@ static int imx219_init_state(struct v4l2_subdev *sd,
>         return imx219_set_pad_format(sd, state, &fmt);
>  }
>
> +/* Verify chip ID */
> +static int imx219_identify_module(struct imx219 *imx219)
> +{
> +       struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +       int ret;
> +       u64 val;
> +
> +       ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
> +       if (ret) {
> +               dev_dbg(&client->dev, "failed to read chip id %x\n",
> +                       IMX219_CHIP_ID);
> +               return ret;
> +       }
> +
> +       if (val != IMX219_CHIP_ID) {
> +               dev_dbg(&client->dev, "chip id mismatch: %x!=%llx\n",
> +                       IMX219_CHIP_ID, val);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
>  static const struct v4l2_subdev_video_ops imx219_video_ops = {
>         .s_stream = v4l2_subdev_s_stream_helper,
>  };
> @@ -1056,6 +1079,14 @@ static int imx219_power_on(struct device *dev)
>         usleep_range(IMX219_XCLR_MIN_DELAY_US,
>                      IMX219_XCLR_MIN_DELAY_US + IMX219_XCLR_DELAY_RANGE_US);
>
> +       /*
> +        * If we can't identify the module here, it might be disconnected.
> +        * Consider power_on() complete and exit early in that case.
> +        */
> +       ret = imx219_identify_module(imx219);

Do we need to identify the module on every power on? Admittedly it's a
lightweight operation here, but for imx678 and the other Starvis2
sensors I'm currently working with you're needing to come out of
standby and wait 80ms before reading the ID registers.

Looking at the rest of the series, polling of detect would notice if
the sensor goes away again within a system that cares about it, so
caching the first successful identify would largely restore the
behaviour for systems that don't care about fault tolerance.
Actually I'd be tempted to keep a call to detect/identify from within
probe so that if the sensor is connected at boot we don't have any
change in behaviour, nor the reporting of the unknown status.

  Dave

> +       if (ret)
> +               return 0;
> +
>         /*
>          * Sensor doesn't enter LP-11 state upon power up until and unless
>          * streaming is started, so upon power up switch the modes to:
> @@ -1117,27 +1148,6 @@ static int imx219_get_regulators(struct imx219 *imx219)
>                                        imx219->supplies);
>  }
>
> -/* Verify chip ID */
> -static int imx219_identify_module(struct imx219 *imx219)
> -{
> -       struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> -       int ret;
> -       u64 val;
> -
> -       ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
> -       if (ret)
> -               return dev_err_probe(&client->dev, ret,
> -                                    "failed to read chip id %x\n",
> -                                    IMX219_CHIP_ID);
> -
> -       if (val != IMX219_CHIP_ID)
> -               return dev_err_probe(&client->dev, -EIO,
> -                                    "chip id mismatch: %x!=%llx\n",
> -                                    IMX219_CHIP_ID, val);
> -
> -       return 0;
> -}
> -
>  static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
>  {
>         struct fwnode_handle *endpoint;
> @@ -1252,21 +1262,9 @@ static int imx219_probe(struct i2c_client *client)
>                 return dev_err_probe(dev, PTR_ERR(imx219->reset_gpio),
>                                      "failed to get reset gpio\n");
>
> -       /*
> -        * The sensor must be powered for imx219_identify_module()
> -        * to be able to read the CHIP_ID register
> -        */
> -       ret = imx219_power_on(dev);
> -       if (ret)
> -               return ret;
> -
> -       ret = imx219_identify_module(imx219);
> -       if (ret)
> -               goto error_power_off;
> -
>         ret = imx219_init_controls(imx219);
>         if (ret)
> -               goto error_power_off;
> +               return ret;
>
>         /* Initialize subdev */
>         imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -1288,7 +1286,7 @@ static int imx219_probe(struct i2c_client *client)
>                 goto error_media_entity;
>         }
>
> -       pm_runtime_set_active(dev);
> +       pm_runtime_set_suspended(dev);
>         pm_runtime_enable(dev);
>
>         ret = v4l2_async_register_subdev_sensor(&imx219->sd);
> @@ -1298,7 +1296,6 @@ static int imx219_probe(struct i2c_client *client)
>                 goto error_subdev_cleanup;
>         }
>
> -       pm_runtime_idle(dev);
>         pm_runtime_set_autosuspend_delay(dev, 1000);
>         pm_runtime_use_autosuspend(dev);
>
> @@ -1315,9 +1312,6 @@ static int imx219_probe(struct i2c_client *client)
>  error_handler_free:
>         imx219_free_controls(imx219);
>
> -error_power_off:
> -       imx219_power_off(dev);
> -
>         return ret;
>  }
>
>
> --
> 2.55.0
>

Re: [PATCH RFC 1/5] media: imx219: Move LP-11 state switch to power_on()

[Dave Stevenson]

Hi Mattijs

On Thu, 1 Oct 2026 at 13:55, Mattijs Korpershoek
<mkorpershoek@kernel.org> wrote:
>
> During probe(), we write the IMX219_MODE_STREAMING register to
> transition from streaming -> standby to force LP-11 state.
>
> This should be done at each power-up of the sensor, but is only
> done *once* for the driver lifecycle (at probe).
>
> Move the LP-11 sequence to power_on() to ensure that it's always put
> into standby mode whenever the pm framework detects it's a power up.

Actually this is an interesting one for the Renesas folks to answer as
they added this.

Yes the sensor powers up with the MIPI lanes in LP00, transitioning to
LP10, then LP11, and finally to HS when starting streaming.
On stopping streaming, the MIPI lanes remain in LP11 until the XCLR
reset line is dropped.

If their hardware wants to see LP11, then how does it keep working if
the sensor has the reset line wired up which would take it back to
LP00? What phase exactly is it that needs this, and how long does LP11
need to be held for?
Do they have power gating such that the CSI receiver block ever gets
powered off and needs to go through the loop again?

I'm not against the patch, but it'd be nice to understand what the
requirements actually are. The sensor goes through LP11 even without
this, so it's something sensitive in a state machine somewhere.

  Dave

> Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
> ---
>  drivers/media/i2c/imx219.c | 44 ++++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 9571f3622d2d..7978fee5f4a2 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -1056,8 +1056,32 @@ static int imx219_power_on(struct device *dev)
>         usleep_range(IMX219_XCLR_MIN_DELAY_US,
>                      IMX219_XCLR_MIN_DELAY_US + IMX219_XCLR_DELAY_RANGE_US);
>
> +       /*
> +        * Sensor doesn't enter LP-11 state upon power up until and unless
> +        * streaming is started, so upon power up switch the modes to:
> +        * streaming -> standby
> +        */
> +       ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
> +                       IMX219_MODE_STREAMING, NULL);
> +       if (ret < 0)
> +               goto gpio_off;
> +
> +       usleep_range(100, 110);
> +
> +       /* put sensor back to standby mode */
> +       ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
> +                       IMX219_MODE_STANDBY, NULL);
> +       if (ret < 0)
> +               goto gpio_off;
> +
> +       usleep_range(100, 110);
> +
>         return 0;
>
> +gpio_off:
> +       gpiod_set_value_cansleep(imx219->reset_gpio, 0);
> +       clk_disable_unprepare(imx219->xclk);
> +
>  reg_off:
>         regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
>
> @@ -1240,26 +1264,6 @@ static int imx219_probe(struct i2c_client *client)
>         if (ret)
>                 goto error_power_off;
>
> -       /*
> -        * Sensor doesn't enter LP-11 state upon power up until and unless
> -        * streaming is started, so upon power up switch the modes to:
> -        * streaming -> standby
> -        */
> -       ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
> -                       IMX219_MODE_STREAMING, NULL);
> -       if (ret < 0)
> -               goto error_power_off;
> -
> -       usleep_range(100, 110);
> -
> -       /* put sensor back to standby mode */
> -       ret = cci_write(imx219->regmap, IMX219_REG_MODE_SELECT,
> -                       IMX219_MODE_STANDBY, NULL);
> -       if (ret < 0)
> -               goto error_power_off;
> -
> -       usleep_range(100, 110);
> -
>         ret = imx219_init_controls(imx219);
>         if (ret)
>                 goto error_power_off;
>
> --
> 2.55.0
>

Re: [PATCH RFC 5/5] media: imx219: Add status polling using .detect()

[Dave Stevenson]

On Thu, 1 Oct 2026 at 16:58, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Mattij
>
> On Thu, 1 Oct 2026 at 13:55, Mattijs Korpershoek
> <mkorpershoek@kernel.org> wrote:
> >
> > Userspace needs to be notified when a sensor connection status
> > changes (e.g. disconnected at boot, then later reconnected) so it can
> > react accordingly.
> >
> > Add periodic polling using a delayed work that calls .detect() every
> > 2s and sends a KOBJ_CHANGE uevent with HOTPLUG=1 on status changes.
> > This mirrors the approach used by DRM connectors in output_poll_execute().
>
> AIUI DRM polls from within the framework (drm_probe_helper.c), not by
> a workqueue in the individual drivers.
>
> Admittedly V4L2 doesn't currently have a totally obvious place to
> setup this, but it would be far less effort to have the polling
> framework within the core code rather than driver.
> Possibly initialised in __v4l2_async_register_subdev_sensor() based on
> whether .detect is set, and cleaned up in
> v4l2_async_unregister_subdev, with the workqueue calling .detect and
> generating the udev event based on the return value? I think that's
> feasible.

2 followup thoughts:

1 - This rather defeats pm_runtime_autosuspend.
The sensor will be powering up and down for every detect call, which
may or may not be within the autosuspend time. A grep for
pm_runtime_set_autosuspend_delay in the current tree gives mainly 1
second, but video-i2c uses 2 seconds, and vd55g1 uses 4 seconds.
If the regulator has a startup delay defined, it'll be slowing down
your polling.

DRM hotplug polling is at 10 second intervals.
Assuming that enable_streaming triggering power_on reports the error,
then your application always has to handle that failure mode, so a
larger poll interval isn't a big issue.

2 - if the sensor has a privacy LED connected to the power rail, it'll
be blinking away with every poll. We've already got folks worrying
about that blink during probe, but it's now become 100 times worse.
This polling process likely needs to be opt-in based on use-case,
either through some configuration parameter, or possibly by the first
call to VIDIOC_SUBDEV_G_CONNECTION_STATUS starting the process.

  Dave

>   Dave
>
> > Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
> > ---
> >  drivers/media/i2c/imx219.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index e198d3fe99c6..76e578a8eab5 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/minmax.h>
> >  #include <linux/module.h>
> > +#include <linux/workqueue.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> >
> > @@ -374,6 +375,9 @@ struct imx219 {
> >
> >         /* Two or Four lanes */
> >         u8 lanes;
> > +
> > +       struct delayed_work detect_work;
> > +       enum v4l2_subdev_connected_status_whence detect_status;
> >  };
> >
> >  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > @@ -1252,6 +1256,35 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> >         return ret;
> >  }
> >
> > +#define IMX219_DETECT_INTERVAL_MS 2000
> > +static void imx219_detect_work(struct work_struct *work)
> > +{
> > +       struct imx219 *imx219 = container_of(work, struct imx219,
> > +                                            detect_work.work);
> > +       struct v4l2_subdev_connected_status status = {};
> > +
> > +       /*
> > +        * All async notifiers should have been run before
> > +        * we can use sd.devnode
> > +        */
> > +       if (!imx219->sd.devnode)
> > +               goto reschedule_detect_work;
> > +
> > +       imx219_detect(&imx219->sd, &status);
> > +
> > +       if (status.status != imx219->detect_status) {
> > +               struct device *dev = &imx219->sd.devnode->dev;
> > +               char *envp[] = { "HOTPLUG=1", NULL };
> > +
> > +               imx219->detect_status = status.status;
> > +               kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> > +       }
> > +
> > +reschedule_detect_work:
> > +       schedule_delayed_work(&imx219->detect_work,
> > +                             msecs_to_jiffies(IMX219_DETECT_INTERVAL_MS));
> > +}
> > +
> >  static int imx219_probe(struct i2c_client *client)
> >  {
> >         struct device *dev = &client->dev;
> > @@ -1334,6 +1367,11 @@ static int imx219_probe(struct i2c_client *client)
> >         pm_runtime_set_autosuspend_delay(dev, 1000);
> >         pm_runtime_use_autosuspend(dev);
> >
> > +       imx219->detect_status = V4L2_SUBDEV_STATUS_UNKNOWN;
> > +       INIT_DELAYED_WORK(&imx219->detect_work, imx219_detect_work);
> > +       schedule_delayed_work(&imx219->detect_work,
> > +                             msecs_to_jiffies(IMX219_DETECT_INTERVAL_MS));
> > +
> >         return 0;
> >
> >  error_subdev_cleanup:
> > @@ -1355,6 +1393,8 @@ static void imx219_remove(struct i2c_client *client)
> >         struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >         struct imx219 *imx219 = to_imx219(sd);
> >
> > +       cancel_delayed_work_sync(&imx219->detect_work);
> > +
> >         v4l2_async_unregister_subdev(sd);
> >         v4l2_subdev_cleanup(sd);
> >         media_entity_cleanup(&sd->entity);
> >
> > --
> > 2.55.0
> >

Re: [PATCH RFC 3/5] media: imx219: Allow driver probe with missing sensor

[Dave Stevenson]

On Thu, 1 Oct 2026 at 17:50, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Mattijs
>
> On Thu, 1 Oct 2026 at 13:55, Mattijs Korpershoek
> <mkorpershoek@kernel.org> wrote:
> >
> > Probe() should complete even when a sensor is disconnected. This would
> > allow the v4l-subdev to be created and improve fault tolerance.
> >
> > Currently, the driver reads the CHIP_ID over i2c in the probe().
> > When we can't read CHIP_ID, the probe errors out - which result in the
> > v4l2-subdev not being created.
> >
> > Remove all i2c communications to allow the driver to probe with a
> > missing sensor.
> >
> > Note: Since we no longer power on the sensor during probe, the driver
> > now starts in suspended mode by default.
> >
> > Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
> > ---
> >  drivers/media/i2c/imx219.c | 72 +++++++++++++++++++++-------------------------
> >  1 file changed, 33 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 7978fee5f4a2..aeac70123b9b 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -1000,6 +1000,29 @@ static int imx219_init_state(struct v4l2_subdev *sd,
> >         return imx219_set_pad_format(sd, state, &fmt);
> >  }
> >
> > +/* Verify chip ID */
> > +static int imx219_identify_module(struct imx219 *imx219)
> > +{
> > +       struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > +       int ret;
> > +       u64 val;
> > +
> > +       ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
> > +       if (ret) {
> > +               dev_dbg(&client->dev, "failed to read chip id %x\n",
> > +                       IMX219_CHIP_ID);
> > +               return ret;
> > +       }
> > +
> > +       if (val != IMX219_CHIP_ID) {
> > +               dev_dbg(&client->dev, "chip id mismatch: %x!=%llx\n",
> > +                       IMX219_CHIP_ID, val);
> > +               return -EIO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static const struct v4l2_subdev_video_ops imx219_video_ops = {
> >         .s_stream = v4l2_subdev_s_stream_helper,
> >  };
> > @@ -1056,6 +1079,14 @@ static int imx219_power_on(struct device *dev)
> >         usleep_range(IMX219_XCLR_MIN_DELAY_US,
> >                      IMX219_XCLR_MIN_DELAY_US + IMX219_XCLR_DELAY_RANGE_US);
> >
> > +       /*
> > +        * If we can't identify the module here, it might be disconnected.
> > +        * Consider power_on() complete and exit early in that case.
> > +        */
> > +       ret = imx219_identify_module(imx219);
>
> Do we need to identify the module on every power on? Admittedly it's a
> lightweight operation here, but for imx678 and the other Starvis2
> sensors I'm currently working with you're needing to come out of
> standby and wait 80ms before reading the ID registers.
>
> Looking at the rest of the series, polling of detect would notice if
> the sensor goes away again within a system that cares about it, so
> caching the first successful identify would largely restore the
> behaviour for systems that don't care about fault tolerance.
> Actually I'd be tempted to keep a call to detect/identify from within
> probe so that if the sensor is connected at boot we don't have any
> change in behaviour, nor the reporting of the unknown status.

And a follow up thought on this one too.

We already return any errors from the I2C writes in
imx219_enable_streams, so reading the ID value here is fairly
redundant. The likelihood of someone having connected a totally
different I2C device on the same I2C bus and address is very low, so
if the writes succeed then you can reasonably assume that the relevant
device is connected.

Perhaps a more useful solution is to still try reading the device ID
during probe. An I2C failure at that point shouldn't abort probe, but
a mismatch on the ID register after a successful read should. Again
that keeps existing users experiencing largely the current behaviour,
but your use case of fault tolerance if not present will also work.

  Dave

>   Dave
>
> > +       if (ret)
> > +               return 0;
> > +
> >         /*
> >          * Sensor doesn't enter LP-11 state upon power up until and unless
> >          * streaming is started, so upon power up switch the modes to:
> > @@ -1117,27 +1148,6 @@ static int imx219_get_regulators(struct imx219 *imx219)
> >                                        imx219->supplies);
> >  }
> >
> > -/* Verify chip ID */
> > -static int imx219_identify_module(struct imx219 *imx219)
> > -{
> > -       struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > -       int ret;
> > -       u64 val;
> > -
> > -       ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
> > -       if (ret)
> > -               return dev_err_probe(&client->dev, ret,
> > -                                    "failed to read chip id %x\n",
> > -                                    IMX219_CHIP_ID);
> > -
> > -       if (val != IMX219_CHIP_ID)
> > -               return dev_err_probe(&client->dev, -EIO,
> > -                                    "chip id mismatch: %x!=%llx\n",
> > -                                    IMX219_CHIP_ID, val);
> > -
> > -       return 0;
> > -}
> > -
> >  static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> >  {
> >         struct fwnode_handle *endpoint;
> > @@ -1252,21 +1262,9 @@ static int imx219_probe(struct i2c_client *client)
> >                 return dev_err_probe(dev, PTR_ERR(imx219->reset_gpio),
> >                                      "failed to get reset gpio\n");
> >
> > -       /*
> > -        * The sensor must be powered for imx219_identify_module()
> > -        * to be able to read the CHIP_ID register
> > -        */
> > -       ret = imx219_power_on(dev);
> > -       if (ret)
> > -               return ret;
> > -
> > -       ret = imx219_identify_module(imx219);
> > -       if (ret)
> > -               goto error_power_off;
> > -
> >         ret = imx219_init_controls(imx219);
> >         if (ret)
> > -               goto error_power_off;
> > +               return ret;
> >
> >         /* Initialize subdev */
> >         imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > @@ -1288,7 +1286,7 @@ static int imx219_probe(struct i2c_client *client)
> >                 goto error_media_entity;
> >         }
> >
> > -       pm_runtime_set_active(dev);
> > +       pm_runtime_set_suspended(dev);
> >         pm_runtime_enable(dev);
> >
> >         ret = v4l2_async_register_subdev_sensor(&imx219->sd);
> > @@ -1298,7 +1296,6 @@ static int imx219_probe(struct i2c_client *client)
> >                 goto error_subdev_cleanup;
> >         }
> >
> > -       pm_runtime_idle(dev);
> >         pm_runtime_set_autosuspend_delay(dev, 1000);
> >         pm_runtime_use_autosuspend(dev);
> >
> > @@ -1315,9 +1312,6 @@ static int imx219_probe(struct i2c_client *client)
> >  error_handler_free:
> >         imx219_free_controls(imx219);
> >
> > -error_power_off:
> > -       imx219_power_off(dev);
> > -
> >         return ret;
> >  }
> >
> >
> > --
> > 2.55.0
> >