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

[Mattijs Korpershoek]

On Thu, Oct 01, 2026 at 18:26, Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> wrote:

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

Yes, the 2s was an arbitrary decision from me. It's certainly not set in
stone.
You make a very good point about pm_runtime_autosuspend. I'll increase
to 10s for next version unless someone has a better suggestion

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

Oh, I did not think about that at all. Having the privacy led blinking
every poll would indeed be super creepy.
Thanks for bringing that up.

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

Ack.
I'll look into it for v2.

Thanks a lot for the suggestions.
Mattijs

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