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