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@xxxxxxxxxxxxxxx> wrote:
>
> Hi Mattij
>
> On Thu, 1 Oct 2026 at 13:55, Mattijs Korpershoek
> <mkorpershoek@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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
> >