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

[Dave Stevenson]

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.

  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
>