* [PATCH RFC 1/5] media: imx219: Move LP-11 state switch to power_on()
2026-10-01 12:55 [PATCH RFC 0/5] media: Fault-Tolerant V4L2 Mattijs Korpershoek
@ 2026-10-01 12:55 ` Mattijs Korpershoek
2026-10-01 17:03 ` Dave Stevenson
2026-10-01 12:55 ` [PATCH RFC 2/5] media: v4l2-subdev: Add new ioctl for connection status Mattijs Korpershoek
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Mattijs Korpershoek @ 2026-10-01 12:55 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Sakari Ailus, Mauro Carvalho Chehab
Cc: Michael Riesch, Dave Stevenson, Maxime Ripard, linux-media,
linux-kernel, 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH RFC 1/5] media: imx219: Move LP-11 state switch to power_on()
2026-10-01 12:55 ` [PATCH RFC 1/5] media: imx219: Move LP-11 state switch to power_on() Mattijs Korpershoek
@ 2026-10-01 17:03 ` Dave Stevenson
0 siblings, 0 replies; 11+ messages in thread
From: Dave Stevenson @ 2026-10-01 17:03 UTC (permalink / raw)
To: Mattijs Korpershoek
Cc: Laurent Pinchart, Kieran Bingham, Sakari Ailus,
Mauro Carvalho Chehab, Michael Riesch, Maxime Ripard,
linux-media, linux-kernel, Lad Prabhakar
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC 2/5] media: v4l2-subdev: Add new ioctl for connection status
2026-10-01 12:55 [PATCH RFC 0/5] media: Fault-Tolerant V4L2 Mattijs Korpershoek
2026-10-01 12:55 ` [PATCH RFC 1/5] media: imx219: Move LP-11 state switch to power_on() Mattijs Korpershoek
@ 2026-10-01 12:55 ` Mattijs Korpershoek
2026-10-01 12:55 ` [PATCH RFC 3/5] media: imx219: Allow driver probe with missing sensor Mattijs Korpershoek
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Mattijs Korpershoek @ 2026-10-01 12:55 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Sakari Ailus, Mauro Carvalho Chehab
Cc: Michael Riesch, Dave Stevenson, Maxime Ripard, linux-media,
linux-kernel, 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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH RFC 3/5] media: imx219: Allow driver probe with missing sensor
2026-10-01 12:55 [PATCH RFC 0/5] media: Fault-Tolerant V4L2 Mattijs Korpershoek
2026-10-01 12:55 ` [PATCH RFC 1/5] media: imx219: Move LP-11 state switch to power_on() Mattijs Korpershoek
2026-10-01 12:55 ` [PATCH RFC 2/5] media: v4l2-subdev: Add new ioctl for connection status Mattijs Korpershoek
@ 2026-10-01 12:55 ` Mattijs Korpershoek
2026-10-01 16:50 ` Dave Stevenson
2026-10-01 12:55 ` [PATCH RFC 4/5] media: imx219: Implement .detect() sensor operation Mattijs Korpershoek
2026-10-01 12:55 ` [PATCH RFC 5/5] media: imx219: Add status polling using .detect() Mattijs Korpershoek
4 siblings, 1 reply; 11+ messages in thread
From: Mattijs Korpershoek @ 2026-10-01 12:55 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Sakari Ailus, Mauro Carvalho Chehab
Cc: Michael Riesch, Dave Stevenson, Maxime Ripard, linux-media,
linux-kernel, 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH RFC 3/5] media: imx219: Allow driver probe with missing sensor
2026-10-01 12:55 ` [PATCH RFC 3/5] media: imx219: Allow driver probe with missing sensor Mattijs Korpershoek
@ 2026-10-01 16:50 ` Dave Stevenson
2026-10-01 17:35 ` Dave Stevenson
0 siblings, 1 reply; 11+ messages in thread
From: Dave Stevenson @ 2026-10-01 16:50 UTC (permalink / raw)
To: Mattijs Korpershoek
Cc: Laurent Pinchart, Kieran Bingham, Sakari Ailus,
Mauro Carvalho Chehab, Michael Riesch, Maxime Ripard,
linux-media, linux-kernel
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH RFC 3/5] media: imx219: Allow driver probe with missing sensor
2026-10-01 16:50 ` Dave Stevenson
@ 2026-10-01 17:35 ` Dave Stevenson
0 siblings, 0 replies; 11+ messages in thread
From: Dave Stevenson @ 2026-10-01 17:35 UTC (permalink / raw)
To: Mattijs Korpershoek
Cc: Laurent Pinchart, Kieran Bingham, Sakari Ailus,
Mauro Carvalho Chehab, Michael Riesch, Maxime Ripard,
linux-media, linux-kernel
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
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC 4/5] media: imx219: Implement .detect() sensor operation
2026-10-01 12:55 [PATCH RFC 0/5] media: Fault-Tolerant V4L2 Mattijs Korpershoek
` (2 preceding siblings ...)
2026-10-01 12:55 ` [PATCH RFC 3/5] media: imx219: Allow driver probe with missing sensor Mattijs Korpershoek
@ 2026-10-01 12:55 ` Mattijs Korpershoek
2026-10-01 12:55 ` [PATCH RFC 5/5] media: imx219: Add status polling using .detect() Mattijs Korpershoek
4 siblings, 0 replies; 11+ messages in thread
From: Mattijs Korpershoek @ 2026-10-01 12:55 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Sakari Ailus, Mauro Carvalho Chehab
Cc: Michael Riesch, Dave Stevenson, Maxime Ripard, linux-media,
linux-kernel, 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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH RFC 5/5] media: imx219: Add status polling using .detect()
2026-10-01 12:55 [PATCH RFC 0/5] media: Fault-Tolerant V4L2 Mattijs Korpershoek
` (3 preceding siblings ...)
2026-10-01 12:55 ` [PATCH RFC 4/5] media: imx219: Implement .detect() sensor operation Mattijs Korpershoek
@ 2026-10-01 12:55 ` Mattijs Korpershoek
2026-10-01 15:58 ` Dave Stevenson
4 siblings, 1 reply; 11+ messages in thread
From: Mattijs Korpershoek @ 2026-10-01 12:55 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham, Sakari Ailus, Mauro Carvalho Chehab
Cc: Michael Riesch, Dave Stevenson, Maxime Ripard, linux-media,
linux-kernel, 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH RFC 5/5] media: imx219: Add status polling using .detect()
2026-10-01 12:55 ` [PATCH RFC 5/5] media: imx219: Add status polling using .detect() Mattijs Korpershoek
@ 2026-10-01 15:58 ` Dave Stevenson
2026-10-01 17:26 ` Dave Stevenson
0 siblings, 1 reply; 11+ messages in thread
From: Dave Stevenson @ 2026-10-01 15:58 UTC (permalink / raw)
To: Mattijs Korpershoek
Cc: Laurent Pinchart, Kieran Bingham, Sakari Ailus,
Mauro Carvalho Chehab, Michael Riesch, Maxime Ripard,
linux-media, linux-kernel
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH RFC 5/5] media: imx219: Add status polling using .detect()
2026-10-01 15:58 ` Dave Stevenson
@ 2026-10-01 17:26 ` Dave Stevenson
0 siblings, 0 replies; 11+ messages in thread
From: Dave Stevenson @ 2026-10-01 17:26 UTC (permalink / raw)
To: Mattijs Korpershoek
Cc: Laurent Pinchart, Kieran Bingham, Sakari Ailus,
Mauro Carvalho Chehab, Michael Riesch, Maxime Ripard,
linux-media, linux-kernel
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
> >
^ permalink raw reply [flat|nested] 11+ messages in thread