diff mbox series

[v3,6/7] media: i2c: imx290: Check for availability in probe()

Message ID 20240902-imx290-avail-v3-6-b32a12799fed@skidata.com (mailing list archive)
State Superseded
Delegated to: Sakari Ailus
Headers
Series media: i2c: imx290: check for availability in probe() |

Checks

Context Check Description
media-ci/HTML_report success Link
media-ci/report success Link
media-ci/bisect success Link
media-ci/doc success Link
media-ci/build success Link
media-ci/static-upstream success Link
media-ci/abi success Link
media-ci/media-patchstyle success Link
media-ci/checkpatch success Link

Commit Message

Benjamin Bara Sept. 2, 2024, 3:57 p.m. UTC 
From: Benjamin Bara <benjamin.bara@skidata.com>

Currently, the V4L2 subdevice is also created when the device is not
available/connected. From userspace perspective, there is no visible
difference between a working and not-working subdevice (except when
trying it out).

This commit adds a simple availability check before starting with the
subdev initialization to error out instead.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
Changes since v2:
- the new 1/8 is split out
- use dev_err_probe() (thx Laurent)
---
 drivers/media/i2c/imx290.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Dave Stevenson Sept. 2, 2024, 6:22 p.m. UTC | #1 
Hi Benjamin

On Mon, 2 Sept 2024 at 16:58, <bbara93@gmail.com> wrote:
>
> From: Benjamin Bara <benjamin.bara@skidata.com>
>
> Currently, the V4L2 subdevice is also created when the device is not
> available/connected. From userspace perspective, there is no visible
> difference between a working and not-working subdevice (except when
> trying it out).
>
> This commit adds a simple availability check before starting with the
> subdev initialization to error out instead.
>
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> Changes since v2:
> - the new 1/8 is split out
> - use dev_err_probe() (thx Laurent)
> ---
>  drivers/media/i2c/imx290.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 9610e9fd2059..6b292bbb0856 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -1571,6 +1571,7 @@ static int imx290_probe(struct i2c_client *client)
>  {
>         struct device *dev = &client->dev;
>         struct imx290 *imx290;
> +       u64 val;
>         int ret;
>
>         imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
> @@ -1631,6 +1632,17 @@ static int imx290_probe(struct i2c_client *client)
>         pm_runtime_set_autosuspend_delay(dev, 1000);
>         pm_runtime_use_autosuspend(dev);
>
> +       /* Make sure the sensor is available before V4L2 subdev init. */
> +       ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> +       if (ret) {
> +               ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
> +               goto err_pm;
> +       }
> +       if (val != IMX290_STANDBY_STANDBY) {

As Laurent commented on v2, this is a slightly unsafe check. If the
device isn't controlled via a regulator then there's no guarantee that
the sensor will be in standby.
The cci_read call will already have returned an error if the sensor
isn't present which will be 99.999% of the error cases.

If you want to catch the case where it's not in standby, why not put
it into standby as a recovery mechanism. It'd be a better user
experience than just bombing out of the probe.

  Dave

> +               ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
> +               goto err_pm;
> +       }
> +
>         /* Initialize the V4L2 subdev. */
>         ret = imx290_subdev_init(imx290);
>         if (ret)
>
> --
> 2.46.0
>
>
Laurent Pinchart Sept. 2, 2024, 8:01 p.m. UTC | #2 
On Mon, Sep 02, 2024 at 07:22:43PM +0100, Dave Stevenson wrote:
> Hi Benjamin
> 
> On Mon, 2 Sept 2024 at 16:58, <bbara93@gmail.com> wrote:
> >
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > Currently, the V4L2 subdevice is also created when the device is not
> > available/connected. From userspace perspective, there is no visible
> > difference between a working and not-working subdevice (except when
> > trying it out).
> >
> > This commit adds a simple availability check before starting with the
> > subdev initialization to error out instead.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > Changes since v2:
> > - the new 1/8 is split out
> > - use dev_err_probe() (thx Laurent)
> > ---
> >  drivers/media/i2c/imx290.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 9610e9fd2059..6b292bbb0856 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -1571,6 +1571,7 @@ static int imx290_probe(struct i2c_client *client)
> >  {
> >         struct device *dev = &client->dev;
> >         struct imx290 *imx290;
> > +       u64 val;
> >         int ret;
> >
> >         imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
> > @@ -1631,6 +1632,17 @@ static int imx290_probe(struct i2c_client *client)
> >         pm_runtime_set_autosuspend_delay(dev, 1000);
> >         pm_runtime_use_autosuspend(dev);
> >
> > +       /* Make sure the sensor is available before V4L2 subdev init. */
> > +       ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > +       if (ret) {
> > +               ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
> > +               goto err_pm;
> > +       }
> > +       if (val != IMX290_STANDBY_STANDBY) {
> 
> As Laurent commented on v2, this is a slightly unsafe check. If the
> device isn't controlled via a regulator then there's no guarantee that
> the sensor will be in standby.
> The cci_read call will already have returned an error if the sensor
> isn't present which will be 99.999% of the error cases.
> 
> If you want to catch the case where it's not in standby, why not put
> it into standby as a recovery mechanism. It'd be a better user
> experience than just bombing out of the probe.

I would also just drop the value check. I don't think it would really
catch real world issues.

> > +               ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
> > +               goto err_pm;
> > +       }
> > +
> >         /* Initialize the V4L2 subdev. */
> >         ret = imx290_subdev_init(imx290);
> >         if (ret)
Benjamin Bara Sept. 2, 2024, 8:03 p.m. UTC | #3 
Hi Dave!

On Mon, 2 Sept 2024 at 20:22, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
> On Mon, 2 Sept 2024 at 16:58, <bbara93@gmail.com> wrote:
> >
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > Currently, the V4L2 subdevice is also created when the device is not
> > available/connected. From userspace perspective, there is no visible
> > difference between a working and not-working subdevice (except when
> > trying it out).
> >
> > This commit adds a simple availability check before starting with the
> > subdev initialization to error out instead.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > Changes since v2:
> > - the new 1/8 is split out
> > - use dev_err_probe() (thx Laurent)
> > ---
> >  drivers/media/i2c/imx290.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 9610e9fd2059..6b292bbb0856 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -1571,6 +1571,7 @@ static int imx290_probe(struct i2c_client *client)
> >  {
> >         struct device *dev = &client->dev;
> >         struct imx290 *imx290;
> > +       u64 val;
> >         int ret;
> >
> >         imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
> > @@ -1631,6 +1632,17 @@ static int imx290_probe(struct i2c_client *client)
> >         pm_runtime_set_autosuspend_delay(dev, 1000);
> >         pm_runtime_use_autosuspend(dev);
> >
> > +       /* Make sure the sensor is available before V4L2 subdev init. */
> > +       ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > +       if (ret) {
> > +               ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
> > +               goto err_pm;
> > +       }
> > +       if (val != IMX290_STANDBY_STANDBY) {
>
> As Laurent commented on v2, this is a slightly unsafe check. If the
> device isn't controlled via a regulator then there's no guarantee that
> the sensor will be in standby.
> The cci_read call will already have returned an error if the sensor
> isn't present which will be 99.999% of the error cases.
>
> If you want to catch the case where it's not in standby, why not put
> it into standby as a recovery mechanism. It'd be a better user
> experience than just bombing out of the probe.

Thanks for the idea - no idea why it didn't come to my mind. I would
also prefer setting STANDBY instead of reading the value.

Kind regards
Benjamin

>   Dave
>
> > +               ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
> > +               goto err_pm;
> > +       }
> > +
> >         /* Initialize the V4L2 subdev. */
> >         ret = imx290_subdev_init(imx290);
> >         if (ret)
> >
> > --
> > 2.46.0
> >
> >
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 9610e9fd2059..6b292bbb0856 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1571,6 +1571,7 @@  static int imx290_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct imx290 *imx290;
+	u64 val;
 	int ret;
 
 	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
@@ -1631,6 +1632,17 @@  static int imx290_probe(struct i2c_client *client)
 	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 
+	/* Make sure the sensor is available before V4L2 subdev init. */
+	ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
+	if (ret) {
+		ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
+		goto err_pm;
+	}
+	if (val != IMX290_STANDBY_STANDBY) {
+		ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
+		goto err_pm;
+	}
+
 	/* Initialize the V4L2 subdev. */
 	ret = imx290_subdev_init(imx290);
 	if (ret)