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

[Mattijs Korpershoek]

Hi Dave,

Thank you for the review.

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

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

That's quite some time to wait. Are the 80ms also needed to write registers?

I added this as a safeguard to avoid doing the LP-11 power state change
(and thus writing IMX219_REG_MODE_SELECT).

We could replace the identify module with testing if the REG_MODE_SELECT
write failed ...

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

... or use a cached value. I will give it some thought and change this
for v2.

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

Ack. see below.

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

Ack, I will look into this for v2.

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

I agree, I will read REG_CHIP_ID once in probe(). The read won't abort
the probe. The IMX219_CHIP_ID mismatch will.
This way, we don't report the unknown status.

Mattijs

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