[v2,1/2] media: i2c: imx290: Check for availability in probe()

  Currently, the V4L2 subdevice is also created when the device is not
available/connected. In this case, dmesg shows the following:

[   10.419510] imx290 7-001a: Error writing reg 0x301c: -6
[   10.428981] imx290 7-001a: Error writing reg 0x3020: -6
[   10.442712] imx290 7-001a: Error writing reg 0x3018: -6
[   10.454018] imx290 7-001a: Error writing reg 0x3020: -6

which seems to come from imx290_ctrl_update() after the subdev init is
finished. However, as the errors are ignored, the subdev is initialized
but simply does not work. From userspace perspective, there is no
visible difference between a working and not-working subdevice (except
when trying it out or watching for the error message).

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 v1:
- define operating/standby mode and use it
- read out the standby mode during probe and ensure it is standby
---
 drivers/media/i2c/imx290.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
  

Hi Benjamin,

On Wed, Aug 28, 2024 at 08:13:07PM +0200, Benjamin Bara wrote:
> Currently, the V4L2 subdevice is also created when the device is not
> available/connected. In this case, dmesg shows the following:
> 
> [   10.419510] imx290 7-001a: Error writing reg 0x301c: -6
> [   10.428981] imx290 7-001a: Error writing reg 0x3020: -6
> [   10.442712] imx290 7-001a: Error writing reg 0x3018: -6
> [   10.454018] imx290 7-001a: Error writing reg 0x3020: -6
> 
> which seems to come from imx290_ctrl_update() after the subdev init is
> finished. However, as the errors are ignored, the subdev is initialized
> but simply does not work. From userspace perspective, there is no
> visible difference between a working and not-working subdevice (except
> when trying it out or watching for the error message).
> 
> 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 v1:
> - define operating/standby mode and use it
> - read out the standby mode during probe and ensure it is standby
> ---
>  drivers/media/i2c/imx290.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 4150e6e4b9a6..2a869576600c 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -29,6 +29,8 @@
>  #include <media/v4l2-subdev.h>
>  
>  #define IMX290_STANDBY					CCI_REG8(0x3000)
> +#define IMX290_STANDBY_OPERATING			(0 << 0)
> +#define IMX290_STANDBY_STANDBY				(1 << 0)
>  #define IMX290_REGHOLD					CCI_REG8(0x3001)
>  #define IMX290_XMSTA					CCI_REG8(0x3002)
>  #define IMX290_ADBIT					CCI_REG8(0x3005)
> @@ -1016,7 +1018,7 @@ static int imx290_start_streaming(struct imx290 *imx290,
>  		return ret;
>  	}
>  
> -	cci_write(imx290->regmap, IMX290_STANDBY, 0x00, &ret);
> +	cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_OPERATING, &ret);

Please run

	./scripts/checkpatch.pl --strict --max-line-length=80

the next time. I'll wrap the line this time.

>  
>  	msleep(30);
>  
> @@ -1029,7 +1031,7 @@ static int imx290_stop_streaming(struct imx290 *imx290)
>  {
>  	int ret = 0;
>  
> -	cci_write(imx290->regmap, IMX290_STANDBY, 0x01, &ret);
> +	cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_STANDBY, &ret);
>  
>  	msleep(30);
>  
> @@ -1520,6 +1522,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);
> @@ -1580,6 +1583,16 @@ 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)
> +		goto err_pm;
> +	if (val != IMX290_STANDBY_STANDBY) {
> +		dev_err(dev, "Sensor is not in standby mode\n");
> +		ret = -ENODEV;
> +		goto err_pm;
> +	}
> +
>  	/* Initialize the V4L2 subdev. */
>  	ret = imx290_subdev_init(imx290);
>  	if (ret)
>
  

Hi Sakari,

On Thu, 29 Aug 2024 at 09:05, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> Hi Benjamin,
>
> On Wed, Aug 28, 2024 at 08:13:07PM +0200, Benjamin Bara wrote:
> > Currently, the V4L2 subdevice is also created when the device is not
> > available/connected. In this case, dmesg shows the following:
> >
> > [   10.419510] imx290 7-001a: Error writing reg 0x301c: -6
> > [   10.428981] imx290 7-001a: Error writing reg 0x3020: -6
> > [   10.442712] imx290 7-001a: Error writing reg 0x3018: -6
> > [   10.454018] imx290 7-001a: Error writing reg 0x3020: -6
> >
> > which seems to come from imx290_ctrl_update() after the subdev init is
> > finished. However, as the errors are ignored, the subdev is initialized
> > but simply does not work. From userspace perspective, there is no
> > visible difference between a working and not-working subdevice (except
> > when trying it out or watching for the error message).
> >
> > 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 v1:
> > - define operating/standby mode and use it
> > - read out the standby mode during probe and ensure it is standby
> > ---
> >  drivers/media/i2c/imx290.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 4150e6e4b9a6..2a869576600c 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -29,6 +29,8 @@
> >  #include <media/v4l2-subdev.h>
> >
> >  #define IMX290_STANDBY                                       CCI_REG8(0x3000)
> > +#define IMX290_STANDBY_OPERATING                     (0 << 0)
> > +#define IMX290_STANDBY_STANDBY                               (1 << 0)
> >  #define IMX290_REGHOLD                                       CCI_REG8(0x3001)
> >  #define IMX290_XMSTA                                 CCI_REG8(0x3002)
> >  #define IMX290_ADBIT                                 CCI_REG8(0x3005)
> > @@ -1016,7 +1018,7 @@ static int imx290_start_streaming(struct imx290 *imx290,
> >               return ret;
> >       }
> >
> > -     cci_write(imx290->regmap, IMX290_STANDBY, 0x00, &ret);
> > +     cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_OPERATING, &ret);
>
> Please run
>
>         ./scripts/checkpatch.pl --strict --max-line-length=80
>
> the next time. I'll wrap the line this time.

Sorry. I added the strict max-line check to my .gitconfig.
"b4 prep --check" now warns me.

> >
> >       msleep(30);
> >
> > @@ -1029,7 +1031,7 @@ static int imx290_stop_streaming(struct imx290 *imx290)
> >  {
> >       int ret = 0;
> >
> > -     cci_write(imx290->regmap, IMX290_STANDBY, 0x01, &ret);
> > +     cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_STANDBY, &ret);
> >
> >       msleep(30);
> >
> > @@ -1520,6 +1522,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);
> > @@ -1580,6 +1583,16 @@ 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)
> > +             goto err_pm;
> > +     if (val != IMX290_STANDBY_STANDBY) {
> > +             dev_err(dev, "Sensor is not in standby mode\n");
> > +             ret = -ENODEV;
> > +             goto err_pm;
> > +     }
> > +
> >       /* Initialize the V4L2 subdev. */
> >       ret = imx290_subdev_init(imx290);
> >       if (ret)
> >
>
> --
> Kind regards,
>
> Sakari Ailus

thx & regards
Benjamin
  

On Thu, Aug 29, 2024 at 07:05:34AM +0000, Sakari Ailus wrote:
> the next time. I'll wrap the line this time.

Based on the other review comments I'll wait for v2.

Thanks.
  

On Thu, Aug 29, 2024 at 12:39:36PM +0000, Sakari Ailus wrote:
> On Thu, Aug 29, 2024 at 07:05:34AM +0000, Sakari Ailus wrote:
> > the next time. I'll wrap the line this time.
> 
> Based on the other review comments I'll wait for v2.

Oh well, wrong e-mail. Please ignore.
  

Hi Benjamin,

On Wed, Aug 28, 2024 at 08:13:07PM +0200, Benjamin Bara wrote:
> Currently, the V4L2 subdevice is also created when the device is not
> available/connected. In this case, dmesg shows the following:
> 
> [   10.419510] imx290 7-001a: Error writing reg 0x301c: -6
> [   10.428981] imx290 7-001a: Error writing reg 0x3020: -6
> [   10.442712] imx290 7-001a: Error writing reg 0x3018: -6
> [   10.454018] imx290 7-001a: Error writing reg 0x3020: -6
> 
> which seems to come from imx290_ctrl_update() after the subdev init is
> finished. However, as the errors are ignored, the subdev is initialized
> but simply does not work. From userspace perspective, there is no
> visible difference between a working and not-working subdevice (except
> when trying it out or watching for the error message).
> 
> 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 v1:
> - define operating/standby mode and use it
> - read out the standby mode during probe and ensure it is standby
> ---
>  drivers/media/i2c/imx290.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 4150e6e4b9a6..2a869576600c 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -29,6 +29,8 @@
>  #include <media/v4l2-subdev.h>
>  
>  #define IMX290_STANDBY					CCI_REG8(0x3000)
> +#define IMX290_STANDBY_OPERATING			(0 << 0)
> +#define IMX290_STANDBY_STANDBY				(1 << 0)

This should use BIT() instead (or at least unsigned values). I'll fix that
while at it, too...
  

Hi Benjamin,

Thank you for the patch.

On Wed, Aug 28, 2024 at 08:13:07PM +0200, Benjamin Bara wrote:
> Currently, the V4L2 subdevice is also created when the device is not
> available/connected. In this case, dmesg shows the following:
> 
> [   10.419510] imx290 7-001a: Error writing reg 0x301c: -6
> [   10.428981] imx290 7-001a: Error writing reg 0x3020: -6
> [   10.442712] imx290 7-001a: Error writing reg 0x3018: -6
> [   10.454018] imx290 7-001a: Error writing reg 0x3020: -6
> 
> which seems to come from imx290_ctrl_update() after the subdev init is
> finished.

I think this should also be fixed. There should be no need to write
those registers at probe time. Would moving the
pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() calls to
just before imx290_subdev_init() help ?

> However, as the errors are ignored, the subdev is initialized
> but simply does not work. From userspace perspective, there is no
> visible difference between a working and not-working subdevice (except
> when trying it out or watching for the error message).
> 
> 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 v1:
> - define operating/standby mode and use it
> - read out the standby mode during probe and ensure it is standby
> ---
>  drivers/media/i2c/imx290.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 4150e6e4b9a6..2a869576600c 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -29,6 +29,8 @@
>  #include <media/v4l2-subdev.h>
>  
>  #define IMX290_STANDBY					CCI_REG8(0x3000)
> +#define IMX290_STANDBY_OPERATING			(0 << 0)
> +#define IMX290_STANDBY_STANDBY				(1 << 0)

The datasheet documents the STANDBY field as a single bit, but doesn't
mention an OPERATING value. I would match that, drop
IMX290_STANDBY_OPERATING and write

#define IMX290_STANDBY_STANDBY				BIT(0)

>  #define IMX290_REGHOLD					CCI_REG8(0x3001)
>  #define IMX290_XMSTA					CCI_REG8(0x3002)
>  #define IMX290_ADBIT					CCI_REG8(0x3005)
> @@ -1016,7 +1018,7 @@ static int imx290_start_streaming(struct imx290 *imx290,
>  		return ret;
>  	}
>  
> -	cci_write(imx290->regmap, IMX290_STANDBY, 0x00, &ret);
> +	cci_write(imx291->regmap, IMX290_STANDBY, IMX290_STANDBY_OPERATING, &ret);

This hunk would then be dropped.

>  
>  	msleep(30);
>  
> @@ -1029,7 +1031,7 @@ static int imx290_stop_streaming(struct imx290 *imx290)
>  {
>  	int ret = 0;
>  
> -	cci_write(imx290->regmap, IMX290_STANDBY, 0x01, &ret);
> +	cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_STANDBY, &ret);

And this looks fine.

The change isn't mentioned in the commit message though. I wouldn't ask
for a v3 just to split this, but as you need to address other issues, it
would be nice to have a separate patch in v3.

>  
>  	msleep(30);
>  
> @@ -1520,6 +1522,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);
> @@ -1580,6 +1583,16 @@ 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);

I still wish we had an ID register, but so be it.

> +	if (ret)

Maybe add

		ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");

or something similar ? Up to you.

> +		goto err_pm;
> +	if (val != IMX290_STANDBY_STANDBY) {

I think this check could be dropped though. If the sensor isn't present
or doesn't respond to I2C reads for any other reason, cci_read() will
fail.

> +		dev_err(dev, "Sensor is not in standby mode\n");
> +		ret = -ENODEV;
> +		goto err_pm;
> +	}
> +

My last concern is about accessing hardware at probe time. There are
known cases where this is problematic. They can be split in two
categories, systems that exhibit unwanted side effects when powering the
sensor up, and systems where the sensor can't be accessed at probe time.

The two issues I can think of in the first category is devices that have
a camera privacy light that could cause worries among users if it
flashes at boot time, and devices that agressively optimize boot time.

In the second category, I know that some people use camera serdes
(FPD-Link, GMSL, ...) that are controlled by userspace. As they should
instead use kernel drivers for those components, upstream may not care
too much about this use case. Another issue I was told about was a
device booting in temperatures that were too low for the camera to
operate, which then needed half an hour to heat the device enclosure
before the sensor and serdes could be accessed. That's a bit extreme,
but it sounds like a valid use case to me.

What do we do with those cases ? Detecting devices at probe time does
have value, so I think it should be a policy decision. We may want to
convey some of that information through DT properties (I'm not sure what
would be acceptable there though). In any case, that's quite a bit of
yak shaving, so I'm inclined to accept this series (or rather its next
version), given that quite a few other camera sensor drivers detect the
device at probe time. I would however like feedback on the problem to
try and find a good solution.

>  	/* Initialize the V4L2 subdev. */
>  	ret = imx290_subdev_init(imx290);
>  	if (ret)
>
  

Hi Laurent,

thank you for the feedback!

On Thu, 29 Aug 2024 at 15:19, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Benjamin,
>
> Thank you for the patch.
>
> On Wed, Aug 28, 2024 at 08:13:07PM +0200, Benjamin Bara wrote:
> > Currently, the V4L2 subdevice is also created when the device is not
> > available/connected. In this case, dmesg shows the following:
> >
> > [   10.419510] imx290 7-001a: Error writing reg 0x301c: -6
> > [   10.428981] imx290 7-001a: Error writing reg 0x3020: -6
> > [   10.442712] imx290 7-001a: Error writing reg 0x3018: -6
> > [   10.454018] imx290 7-001a: Error writing reg 0x3020: -6
> >
> > which seems to come from imx290_ctrl_update() after the subdev init is
> > finished.
>
> I think this should also be fixed. There should be no need to write
> those registers at probe time. Would moving the
> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() calls to
> just before imx290_subdev_init() help ?

I guess if I decrease the autosuspend delay (now 1s), it would work -
but it feels like a hack to me. I would prefer to not call
imx290_ctrl_update() at all during probe(). I guess the reason why it is
done is to have sane ctrl values for link_freq, hblank and vblank.
However, as they depend on the mode (which isn't known at that time), it
(at least to me) doesn't make sense to just "assume" the first mode
here.

I would prefer to introduce a FREQ_INDEX_OFF with 0 here and use this as
default, and use the ranges from the datasheet for {v,h}blank already
when creating the controls. When the mode is decided, the blanks will be
adapted to be in range.

I can add an example in the next round, need to implement and test
first.

> > However, as the errors are ignored, the subdev is initialized
> > but simply does not work. From userspace perspective, there is no
> > visible difference between a working and not-working subdevice (except
> > when trying it out or watching for the error message).
> >
> > 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 v1:
> > - define operating/standby mode and use it
> > - read out the standby mode during probe and ensure it is standby
> > ---
> >  drivers/media/i2c/imx290.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 4150e6e4b9a6..2a869576600c 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -29,6 +29,8 @@
> >  #include <media/v4l2-subdev.h>
> >
> >  #define IMX290_STANDBY                                       CCI_REG8(0x3000)
> > +#define IMX290_STANDBY_OPERATING                     (0 << 0)
> > +#define IMX290_STANDBY_STANDBY                               (1 << 0)
>
> The datasheet documents the STANDBY field as a single bit, but doesn't
> mention an OPERATING value. I would match that, drop
> IMX290_STANDBY_OPERATING and write

The imx290 datasheet from Arrow has it on page 35, but I can switch to
your version if preferred :)

> #define IMX290_STANDBY_STANDBY                          BIT(0)
>
> >  #define IMX290_REGHOLD                                       CCI_REG8(0x3001)
> >  #define IMX290_XMSTA                                 CCI_REG8(0x3002)
> >  #define IMX290_ADBIT                                 CCI_REG8(0x3005)
> > @@ -1016,7 +1018,7 @@ static int imx290_start_streaming(struct imx290 *imx290,
> >               return ret;
> >       }
> >
> > -     cci_write(imx290->regmap, IMX290_STANDBY, 0x00, &ret);
> > +     cci_write(imx291->regmap, IMX290_STANDBY, IMX290_STANDBY_OPERATING, &ret);
>
> This hunk would then be dropped.
>
> >
> >       msleep(30);
> >
> > @@ -1029,7 +1031,7 @@ static int imx290_stop_streaming(struct imx290 *imx290)
> >  {
> >       int ret = 0;
> >
> > -     cci_write(imx290->regmap, IMX290_STANDBY, 0x01, &ret);
> > +     cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_STANDBY, &ret);
>
> And this looks fine.
>
> The change isn't mentioned in the commit message though. I wouldn't ask
> for a v3 just to split this, but as you need to address other issues, it
> would be nice to have a separate patch in v3.

Yup, can do that :)

> >
> >       msleep(30);
> >
> > @@ -1520,6 +1522,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);
> > @@ -1580,6 +1583,16 @@ 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);
>
> I still wish we had an ID register, but so be it.

When we implement a SW reset (to be sure that there was a reset in case
a dummy regulator is used), we can probably come up with a mix of
different default values of registers to go through, but not sure if
this is really worth it...

> > +     if (ret)
>
> Maybe add
>
>                 ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
>
> or something similar ? Up to you.

Probably a good idea. I didn't print something here because cci_read()
already does.

> > +             goto err_pm;
> > +     if (val != IMX290_STANDBY_STANDBY) {
>
> I think this check could be dropped though. If the sensor isn't present
> or doesn't respond to I2C reads for any other reason, cci_read() will
> fail.

I added it because Sakari and Alex suggested to read back a value and
compare it to an expectation. Would keep it therefore?

> > +             dev_err(dev, "Sensor is not in standby mode\n");
> > +             ret = -ENODEV;
> > +             goto err_pm;
> > +     }
> > +
>
> My last concern is about accessing hardware at probe time. There are
> known cases where this is problematic. They can be split in two
> categories, systems that exhibit unwanted side effects when powering the
> sensor up, and systems where the sensor can't be accessed at probe time.
>
> The two issues I can think of in the first category is devices that have
> a camera privacy light that could cause worries among users if it
> flashes at boot time, and devices that agressively optimize boot time.
>
> In the second category, I know that some people use camera serdes
> (FPD-Link, GMSL, ...) that are controlled by userspace. As they should
> instead use kernel drivers for those components, upstream may not care
> too much about this use case. Another issue I was told about was a
> device booting in temperatures that were too low for the camera to
> operate, which then needed half an hour to heat the device enclosure
> before the sensor and serdes could be accessed. That's a bit extreme,
> but it sounds like a valid use case to me.
>
> What do we do with those cases ? Detecting devices at probe time does
> have value, so I think it should be a policy decision. We may want to
> convey some of that information through DT properties (I'm not sure what
> would be acceptable there though). In any case, that's quite a bit of
> yak shaving, so I'm inclined to accept this series (or rather its next
> version), given that quite a few other camera sensor drivers detect the
> device at probe time. I would however like feedback on the problem to
> try and find a good solution.

One of the rather "simpler" solutions that come to my mind (without
adding something like a generic "disallow-regulator-during-probe" or
similar DT property) is to check the current state of the used
regulator(s) and keep it during the cam probe. If it is already active:
fine, we can communicate and find out; if not: live with schroedinger's
cam. Probably we should decide at one point in time if dead or alive.

If you think this sounds fine, I can modify the series to do that.

> >       /* Initialize the V4L2 subdev. */
> >       ret = imx290_subdev_init(imx290);
> >       if (ret)
> >
--
Kind regards
Benjamin
  

On Thu, Aug 29, 2024 at 04:19:09PM +0300, Laurent Pinchart wrote:

Hi Laurent,

[...]

> > +		dev_err(dev, "Sensor is not in standby mode\n");
> > +		ret = -ENODEV;
> > +		goto err_pm;
> > +	}
> > +
> 
> My last concern is about accessing hardware at probe time. There are
> known cases where this is problematic. They can be split in two
> categories, systems that exhibit unwanted side effects when powering the
> sensor up, and systems where the sensor can't be accessed at probe time.
> 
> The two issues I can think of in the first category is devices that have
> a camera privacy light that could cause worries among users if it
> flashes at boot time, and devices that agressively optimize boot time.
> 
> In the second category, I know that some people use camera serdes
> (FPD-Link, GMSL, ...) that are controlled by userspace. As they should
> instead use kernel drivers for those components, upstream may not care
> too much about this use case. Another issue I was told about was a
> device booting in temperatures that were too low for the camera to
> operate, which then needed half an hour to heat the device enclosure
> before the sensor and serdes could be accessed. That's a bit extreme,
> but it sounds like a valid use case to me.
> 
> What do we do with those cases ? Detecting devices at probe time does
> have value, so I think it should be a policy decision. We may want to
> convey some of that information through DT properties (I'm not sure what
> would be acceptable there though). In any case, that's quite a bit of
> yak shaving, so I'm inclined to accept this series (or rather its next
> version), given that quite a few other camera sensor drivers detect the
> device at probe time. I would however like feedback on the problem to
> try and find a good solution.
> 

Most of the issues you mentioned applies to other hardware peripherals also IMO.
And it is common for the drivers to read registers and make sure the device is
detected on the bus during probe(). If an usecase doesn't want to read the
registers during probe time, then they _should_not_ build the driver as built-in
rather make it as a loadable module and load it whenever necessary. This applies
to boot time optimization as well.

A DT property wouldn't be feasible as DT is supposed to describe the hardware,
not the usecase.

- Mani
  

Hi Manivannan,

On Thu, Aug 29, 2024 at 10:02:47PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 29, 2024 at 04:19:09PM +0300, Laurent Pinchart wrote:
> 
> Hi Laurent,
> 
> [...]
> 
> > > +		dev_err(dev, "Sensor is not in standby mode\n");
> > > +		ret = -ENODEV;
> > > +		goto err_pm;
> > > +	}
> > > +
> > 
> > My last concern is about accessing hardware at probe time. There are
> > known cases where this is problematic. They can be split in two
> > categories, systems that exhibit unwanted side effects when powering the
> > sensor up, and systems where the sensor can't be accessed at probe time.
> > 
> > The two issues I can think of in the first category is devices that have
> > a camera privacy light that could cause worries among users if it
> > flashes at boot time, and devices that agressively optimize boot time.
> > 
> > In the second category, I know that some people use camera serdes
> > (FPD-Link, GMSL, ...) that are controlled by userspace. As they should
> > instead use kernel drivers for those components, upstream may not care
> > too much about this use case. Another issue I was told about was a
> > device booting in temperatures that were too low for the camera to
> > operate, which then needed half an hour to heat the device enclosure
> > before the sensor and serdes could be accessed. That's a bit extreme,
> > but it sounds like a valid use case to me.
> > 
> > What do we do with those cases ? Detecting devices at probe time does
> > have value, so I think it should be a policy decision. We may want to
> > convey some of that information through DT properties (I'm not sure what
> > would be acceptable there though). In any case, that's quite a bit of
> > yak shaving, so I'm inclined to accept this series (or rather its next
> > version), given that quite a few other camera sensor drivers detect the
> > device at probe time. I would however like feedback on the problem to
> > try and find a good solution.
> 
> Most of the issues you mentioned applies to other hardware peripherals also IMO.
> And it is common for the drivers to read registers and make sure the device is
> detected on the bus during probe().

That's true. I think the problem affects different device types
differently though, and this may (or may not) call for different
solutions.

> If an usecase doesn't want to read the
> registers during probe time, then they _should_not_ build the driver as built-in
> rather make it as a loadable module and load it whenever necessary. This applies
> to boot time optimization as well.

For most of the use cases I listed I agree with you. One exception is
the privacy light issue. Regardless of when the camera sensor driver is
loaded, powering the device at probe time will flash the privacy light.
Doing so later than boot time would probably make the issue even worse,
I would worry more if I saw my webcam privacy light flashing at a random
point after boot time.

> A DT property wouldn't be feasible as DT is supposed to describe the hardware,
> not the usecase.

I think that rule is typically slightly relaxed, by allowing in DT
system descriptions, not just hardware descriptions. Otherwise we
wouldn't allow things like reserved memory ranges. Describing that a
camera sensor has a privacy light, in a way that would allow drivers to
avoid powering up the device at probe time without requiring much
duplicated code in all drivers, would in my opinion be an acceptable DT
usage.
  

Hi Benjamin,

On Thu, Aug 29, 2024 at 05:36:48PM +0200, Benjamin Bara wrote:
> On Thu, 29 Aug 2024 at 15:19, Laurent Pinchart wrote:
> > On Wed, Aug 28, 2024 at 08:13:07PM +0200, Benjamin Bara wrote:
> > > Currently, the V4L2 subdevice is also created when the device is not
> > > available/connected. In this case, dmesg shows the following:
> > >
> > > [   10.419510] imx290 7-001a: Error writing reg 0x301c: -6
> > > [   10.428981] imx290 7-001a: Error writing reg 0x3020: -6
> > > [   10.442712] imx290 7-001a: Error writing reg 0x3018: -6
> > > [   10.454018] imx290 7-001a: Error writing reg 0x3020: -6
> > >
> > > which seems to come from imx290_ctrl_update() after the subdev init is
> > > finished.
> >
> > I think this should also be fixed. There should be no need to write
> > those registers at probe time. Would moving the
> > pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() calls to
> > just before imx290_subdev_init() help ?
> 
> I guess if I decrease the autosuspend delay (now 1s), it would work -
> but it feels like a hack to me. I would prefer to not call

Is decreading the autosuspend delay needed ? If you call
pm_runtime_put_autosuspend() before imx290_subdev_init(), will the
pm_runtime_get_if_in_use() call in imx290_set_ctrl() return a non-zero
value if the device hasn't been suspended yet due to the autosuspend
delay ? My understanding is that it shouldn't, unlike
pm_runtime_get_if_active().

> imx290_ctrl_update() at all during probe(). I guess the reason why it is
> done is to have sane ctrl values for link_freq, hblank and vblank.
> However, as they depend on the mode (which isn't known at that time), it
> (at least to me) doesn't make sense to just "assume" the first mode
> here.

The reason why the function is called at probe time is to centralize in
a single location all the code that computes ranges for the controls.
Otherwise, the logic would need to be duplicated in imx290_ctrl_init().
We could possibly split imx290_ctrl_update() in two functions, one that
computes the ranges and one that updates the controls, and call the
first one from imx290_ctrl_init().

> I would prefer to introduce a FREQ_INDEX_OFF with 0 here and use this as
> default, and use the ranges from the datasheet for {v,h}blank already
> when creating the controls. When the mode is decided, the blanks will be
> adapted to be in range.

That would violate the V4L2 API I'm afraid. Subdevs are supposed to have
a valid configuration at all times, including just after probe before
being configured by users.

> I can add an example in the next round, need to implement and test
> first.
> 
> > > However, as the errors are ignored, the subdev is initialized
> > > but simply does not work. From userspace perspective, there is no
> > > visible difference between a working and not-working subdevice (except
> > > when trying it out or watching for the error message).
> > >
> > > 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 v1:
> > > - define operating/standby mode and use it
> > > - read out the standby mode during probe and ensure it is standby
> > > ---
> > >  drivers/media/i2c/imx290.c | 17 +++++++++++++++--
> > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 4150e6e4b9a6..2a869576600c 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -29,6 +29,8 @@
> > >  #include <media/v4l2-subdev.h>
> > >
> > >  #define IMX290_STANDBY                                       CCI_REG8(0x3000)
> > > +#define IMX290_STANDBY_OPERATING                     (0 << 0)
> > > +#define IMX290_STANDBY_STANDBY                               (1 << 0)
> >
> > The datasheet documents the STANDBY field as a single bit, but doesn't
> > mention an OPERATING value. I would match that, drop
> > IMX290_STANDBY_OPERATING and write
> 
> The imx290 datasheet from Arrow has it on page 35, but I can switch to
> your version if preferred :)
> 
> > #define IMX290_STANDBY_STANDBY                          BIT(0)
> >
> > >  #define IMX290_REGHOLD                                       CCI_REG8(0x3001)
> > >  #define IMX290_XMSTA                                 CCI_REG8(0x3002)
> > >  #define IMX290_ADBIT                                 CCI_REG8(0x3005)
> > > @@ -1016,7 +1018,7 @@ static int imx290_start_streaming(struct imx290 *imx290,
> > >               return ret;
> > >       }
> > >
> > > -     cci_write(imx290->regmap, IMX290_STANDBY, 0x00, &ret);
> > > +     cci_write(imx291->regmap, IMX290_STANDBY, IMX290_STANDBY_OPERATING, &ret);
> >
> > This hunk would then be dropped.
> >
> > >
> > >       msleep(30);
> > >
> > > @@ -1029,7 +1031,7 @@ static int imx290_stop_streaming(struct imx290 *imx290)
> > >  {
> > >       int ret = 0;
> > >
> > > -     cci_write(imx290->regmap, IMX290_STANDBY, 0x01, &ret);
> > > +     cci_write(imx290->regmap, IMX290_STANDBY, IMX290_STANDBY_STANDBY, &ret);
> >
> > And this looks fine.
> >
> > The change isn't mentioned in the commit message though. I wouldn't ask
> > for a v3 just to split this, but as you need to address other issues, it
> > would be nice to have a separate patch in v3.
> 
> Yup, can do that :)
> 
> > >
> > >       msleep(30);
> > >
> > > @@ -1520,6 +1522,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);
> > > @@ -1580,6 +1583,16 @@ 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);
> >
> > I still wish we had an ID register, but so be it.
> 
> When we implement a SW reset (to be sure that there was a reset in case
> a dummy regulator is used), we can probably come up with a mix of
> different default values of registers to go through, but not sure if
> this is really worth it...
> 
> > > +     if (ret)
> >
> > Maybe add
> >
> >                 ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
> >
> > or something similar ? Up to you.
> 
> Probably a good idea. I didn't print something here because cci_read()
> already does.
> 
> > > +             goto err_pm;
> > > +     if (val != IMX290_STANDBY_STANDBY) {
> >
> > I think this check could be dropped though. If the sensor isn't present
> > or doesn't respond to I2C reads for any other reason, cci_read() will
> > fail.
> 
> I added it because Sakari and Alex suggested to read back a value and
> compare it to an expectation. Would keep it therefore?

If we had an ID register that had a more discriminatory value I would
check its contents. In this case, as the goal is to detect if the device
is functional, I don't think the check adds much value, unless you think
we need to protect against an incorrect devices responding to the same
I2C address. I would prefer dropping this, but I don't mind keeping the
check too much.

> > > +             dev_err(dev, "Sensor is not in standby mode\n");
> > > +             ret = -ENODEV;
> > > +             goto err_pm;
> > > +     }
> > > +
> >
> > My last concern is about accessing hardware at probe time. There are
> > known cases where this is problematic. They can be split in two
> > categories, systems that exhibit unwanted side effects when powering the
> > sensor up, and systems where the sensor can't be accessed at probe time.
> >
> > The two issues I can think of in the first category is devices that have
> > a camera privacy light that could cause worries among users if it
> > flashes at boot time, and devices that agressively optimize boot time.
> >
> > In the second category, I know that some people use camera serdes
> > (FPD-Link, GMSL, ...) that are controlled by userspace. As they should
> > instead use kernel drivers for those components, upstream may not care
> > too much about this use case. Another issue I was told about was a
> > device booting in temperatures that were too low for the camera to
> > operate, which then needed half an hour to heat the device enclosure
> > before the sensor and serdes could be accessed. That's a bit extreme,
> > but it sounds like a valid use case to me.
> >
> > What do we do with those cases ? Detecting devices at probe time does
> > have value, so I think it should be a policy decision. We may want to
> > convey some of that information through DT properties (I'm not sure what
> > would be acceptable there though). In any case, that's quite a bit of
> > yak shaving, so I'm inclined to accept this series (or rather its next
> > version), given that quite a few other camera sensor drivers detect the
> > device at probe time. I would however like feedback on the problem to
> > try and find a good solution.
> 
> One of the rather "simpler" solutions that come to my mind (without
> adding something like a generic "disallow-regulator-during-probe" or
> similar DT property) is to check the current state of the used
> regulator(s) and keep it during the cam probe. If it is already active:
> fine, we can communicate and find out; if not: live with schroedinger's
> cam. Probably we should decide at one point in time if dead or alive.
> 
> If you think this sounds fine, I can modify the series to do that.

Hmmmm that's sounds a bit dangerous, and possibly racy. I think we can
add the check for now, and implement a more generic mechanism in a
second step. As discussed in a separate e-mail in the same thread, most
of the use cases I mentioned can be addressed by loading the driver as a
module. The only exception (at the moment) is the privacy light problem.
I'd like to address that more globally.

> > >       /* Initialize the V4L2 subdev. */
> > >       ret = imx290_subdev_init(imx290);
> > >       if (ret)
> > >
  

On Thu, Aug 29, 2024 at 07:48:43PM +0300, Laurent Pinchart wrote:
> Hi Manivannan,
> 
> On Thu, Aug 29, 2024 at 10:02:47PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Aug 29, 2024 at 04:19:09PM +0300, Laurent Pinchart wrote:
> > 
> > Hi Laurent,
> > 
> > [...]
> > 
> > > > +		dev_err(dev, "Sensor is not in standby mode\n");
> > > > +		ret = -ENODEV;
> > > > +		goto err_pm;
> > > > +	}
> > > > +
> > > 
> > > My last concern is about accessing hardware at probe time. There are
> > > known cases where this is problematic. They can be split in two
> > > categories, systems that exhibit unwanted side effects when powering the
> > > sensor up, and systems where the sensor can't be accessed at probe time.
> > > 
> > > The two issues I can think of in the first category is devices that have
> > > a camera privacy light that could cause worries among users if it
> > > flashes at boot time, and devices that agressively optimize boot time.
> > > 
> > > In the second category, I know that some people use camera serdes
> > > (FPD-Link, GMSL, ...) that are controlled by userspace. As they should
> > > instead use kernel drivers for those components, upstream may not care
> > > too much about this use case. Another issue I was told about was a
> > > device booting in temperatures that were too low for the camera to
> > > operate, which then needed half an hour to heat the device enclosure
> > > before the sensor and serdes could be accessed. That's a bit extreme,
> > > but it sounds like a valid use case to me.
> > > 
> > > What do we do with those cases ? Detecting devices at probe time does
> > > have value, so I think it should be a policy decision. We may want to
> > > convey some of that information through DT properties (I'm not sure what
> > > would be acceptable there though). In any case, that's quite a bit of
> > > yak shaving, so I'm inclined to accept this series (or rather its next
> > > version), given that quite a few other camera sensor drivers detect the
> > > device at probe time. I would however like feedback on the problem to
> > > try and find a good solution.
> > 
> > Most of the issues you mentioned applies to other hardware peripherals also IMO.
> > And it is common for the drivers to read registers and make sure the device is
> > detected on the bus during probe().
> 
> That's true. I think the problem affects different device types
> differently though, and this may (or may not) call for different
> solutions.
> 
> > If an usecase doesn't want to read the
> > registers during probe time, then they _should_not_ build the driver as built-in
> > rather make it as a loadable module and load it whenever necessary. This applies
> > to boot time optimization as well.
> 
> For most of the use cases I listed I agree with you. One exception is
> the privacy light issue. Regardless of when the camera sensor driver is
> loaded, powering the device at probe time will flash the privacy light.
> Doing so later than boot time would probably make the issue even worse,
> I would worry more if I saw my webcam privacy light flashing at a random
> point after boot time.
> 

I'm not familiar with the privacy light feature in camera sensors, but is there
no way to prevent it from enabling by default? If that's not possible, it makes
sense to disable it using a DT property as it is a hardware feature.

> > A DT property wouldn't be feasible as DT is supposed to describe the hardware,
> > not the usecase.
> 
> I think that rule is typically slightly relaxed, by allowing in DT
> system descriptions, not just hardware descriptions. Otherwise we
> wouldn't allow things like reserved memory ranges. Describing that a
> camera sensor has a privacy light, in a way that would allow drivers to
> avoid powering up the device at probe time without requiring much
> duplicated code in all drivers, would in my opinion be an acceptable DT
> usage.
> 

Well, I agree with you. As I said above, privacy light is a hardware feature, so
we can enable/disable it using a DT property. My comment about the DT property
applies only to detecting the devices during probe time, which is a
driver/usecase dependent feature.

- Mani
  

On Fri, Aug 30, 2024 at 02:01:07PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 29, 2024 at 07:48:43PM +0300, Laurent Pinchart wrote:
> > On Thu, Aug 29, 2024 at 10:02:47PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Aug 29, 2024 at 04:19:09PM +0300, Laurent Pinchart wrote:
> > > 
> > > Hi Laurent,
> > > 
> > > [...]
> > > 
> > > > > +		dev_err(dev, "Sensor is not in standby mode\n");
> > > > > +		ret = -ENODEV;
> > > > > +		goto err_pm;
> > > > > +	}
> > > > > +
> > > > 
> > > > My last concern is about accessing hardware at probe time. There are
> > > > known cases where this is problematic. They can be split in two
> > > > categories, systems that exhibit unwanted side effects when powering the
> > > > sensor up, and systems where the sensor can't be accessed at probe time.
> > > > 
> > > > The two issues I can think of in the first category is devices that have
> > > > a camera privacy light that could cause worries among users if it
> > > > flashes at boot time, and devices that agressively optimize boot time.
> > > > 
> > > > In the second category, I know that some people use camera serdes
> > > > (FPD-Link, GMSL, ...) that are controlled by userspace. As they should
> > > > instead use kernel drivers for those components, upstream may not care
> > > > too much about this use case. Another issue I was told about was a
> > > > device booting in temperatures that were too low for the camera to
> > > > operate, which then needed half an hour to heat the device enclosure
> > > > before the sensor and serdes could be accessed. That's a bit extreme,
> > > > but it sounds like a valid use case to me.
> > > > 
> > > > What do we do with those cases ? Detecting devices at probe time does
> > > > have value, so I think it should be a policy decision. We may want to
> > > > convey some of that information through DT properties (I'm not sure what
> > > > would be acceptable there though). In any case, that's quite a bit of
> > > > yak shaving, so I'm inclined to accept this series (or rather its next
> > > > version), given that quite a few other camera sensor drivers detect the
> > > > device at probe time. I would however like feedback on the problem to
> > > > try and find a good solution.
> > > 
> > > Most of the issues you mentioned applies to other hardware peripherals also IMO.
> > > And it is common for the drivers to read registers and make sure the device is
> > > detected on the bus during probe().
> > 
> > That's true. I think the problem affects different device types
> > differently though, and this may (or may not) call for different
> > solutions.
> > 
> > > If an usecase doesn't want to read the
> > > registers during probe time, then they _should_not_ build the driver as built-in
> > > rather make it as a loadable module and load it whenever necessary. This applies
> > > to boot time optimization as well.
> > 
> > For most of the use cases I listed I agree with you. One exception is
> > the privacy light issue. Regardless of when the camera sensor driver is
> > loaded, powering the device at probe time will flash the privacy light.
> > Doing so later than boot time would probably make the issue even worse,
> > I would worry more if I saw my webcam privacy light flashing at a random
> > point after boot time.
> 
> I'm not familiar with the privacy light feature in camera sensors, but is there
> no way to prevent it from enabling by default? If that's not possible, it makes
> sense to disable it using a DT property as it is a hardware feature.

The whole point of the privacy light is that it shouldn't be possible to
disable it by software. Otherwise malicious software could try to work
around it. On many devices it is hardwired to one of the camera sensor's
power supplies.

> > > A DT property wouldn't be feasible as DT is supposed to describe the hardware,
> > > not the usecase.
> > 
> > I think that rule is typically slightly relaxed, by allowing in DT
> > system descriptions, not just hardware descriptions. Otherwise we
> > wouldn't allow things like reserved memory ranges. Describing that a
> > camera sensor has a privacy light, in a way that would allow drivers to
> > avoid powering up the device at probe time without requiring much
> > duplicated code in all drivers, would in my opinion be an acceptable DT
> > usage.
> 
> Well, I agree with you. As I said above, privacy light is a hardware feature, so
> we can enable/disable it using a DT property. My comment about the DT property
> applies only to detecting the devices during probe time, which is a
> driver/usecase dependent feature.
  

On Fri, Aug 30, 2024 at 12:25:26PM +0300, Laurent Pinchart wrote:
> On Fri, Aug 30, 2024 at 02:01:07PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Aug 29, 2024 at 07:48:43PM +0300, Laurent Pinchart wrote:
> > > On Thu, Aug 29, 2024 at 10:02:47PM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Aug 29, 2024 at 04:19:09PM +0300, Laurent Pinchart wrote:
> > > > 
> > > > Hi Laurent,
> > > > 
> > > > [...]
> > > > 
> > > > > > +		dev_err(dev, "Sensor is not in standby mode\n");
> > > > > > +		ret = -ENODEV;
> > > > > > +		goto err_pm;
> > > > > > +	}
> > > > > > +
> > > > > 
> > > > > My last concern is about accessing hardware at probe time. There are
> > > > > known cases where this is problematic. They can be split in two
> > > > > categories, systems that exhibit unwanted side effects when powering the
> > > > > sensor up, and systems where the sensor can't be accessed at probe time.
> > > > > 
> > > > > The two issues I can think of in the first category is devices that have
> > > > > a camera privacy light that could cause worries among users if it
> > > > > flashes at boot time, and devices that agressively optimize boot time.
> > > > > 
> > > > > In the second category, I know that some people use camera serdes
> > > > > (FPD-Link, GMSL, ...) that are controlled by userspace. As they should
> > > > > instead use kernel drivers for those components, upstream may not care
> > > > > too much about this use case. Another issue I was told about was a
> > > > > device booting in temperatures that were too low for the camera to
> > > > > operate, which then needed half an hour to heat the device enclosure
> > > > > before the sensor and serdes could be accessed. That's a bit extreme,
> > > > > but it sounds like a valid use case to me.
> > > > > 
> > > > > What do we do with those cases ? Detecting devices at probe time does
> > > > > have value, so I think it should be a policy decision. We may want to
> > > > > convey some of that information through DT properties (I'm not sure what
> > > > > would be acceptable there though). In any case, that's quite a bit of
> > > > > yak shaving, so I'm inclined to accept this series (or rather its next
> > > > > version), given that quite a few other camera sensor drivers detect the
> > > > > device at probe time. I would however like feedback on the problem to
> > > > > try and find a good solution.
> > > > 
> > > > Most of the issues you mentioned applies to other hardware peripherals also IMO.
> > > > And it is common for the drivers to read registers and make sure the device is
> > > > detected on the bus during probe().
> > > 
> > > That's true. I think the problem affects different device types
> > > differently though, and this may (or may not) call for different
> > > solutions.
> > > 
> > > > If an usecase doesn't want to read the
> > > > registers during probe time, then they _should_not_ build the driver as built-in
> > > > rather make it as a loadable module and load it whenever necessary. This applies
> > > > to boot time optimization as well.
> > > 
> > > For most of the use cases I listed I agree with you. One exception is
> > > the privacy light issue. Regardless of when the camera sensor driver is
> > > loaded, powering the device at probe time will flash the privacy light.
> > > Doing so later than boot time would probably make the issue even worse,
> > > I would worry more if I saw my webcam privacy light flashing at a random
> > > point after boot time.
> > 
> > I'm not familiar with the privacy light feature in camera sensors, but is there
> > no way to prevent it from enabling by default? If that's not possible, it makes
> > sense to disable it using a DT property as it is a hardware feature.
> 
> The whole point of the privacy light is that it shouldn't be possible to
> disable it by software. Otherwise malicious software could try to work
> around it. On many devices it is hardwired to one of the camera sensor's
> power supplies.
> 

Ah okay, please forgive my ignorance here. But still I'm not sure about the DT
usage. Maybe it is best to send out a bindings patch and see what the
maintainers have to say?

- Mani
  

On Fri, Aug 30, 2024 at 05:55:29PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Aug 30, 2024 at 12:25:26PM +0300, Laurent Pinchart wrote:
> > On Fri, Aug 30, 2024 at 02:01:07PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Aug 29, 2024 at 07:48:43PM +0300, Laurent Pinchart wrote:
> > > > On Thu, Aug 29, 2024 at 10:02:47PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Thu, Aug 29, 2024 at 04:19:09PM +0300, Laurent Pinchart wrote:
> > > > > 
> > > > > Hi Laurent,
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > +		dev_err(dev, "Sensor is not in standby mode\n");
> > > > > > > +		ret = -ENODEV;
> > > > > > > +		goto err_pm;
> > > > > > > +	}
> > > > > > > +
> > > > > > 
> > > > > > My last concern is about accessing hardware at probe time. There are
> > > > > > known cases where this is problematic. They can be split in two
> > > > > > categories, systems that exhibit unwanted side effects when powering the
> > > > > > sensor up, and systems where the sensor can't be accessed at probe time.
> > > > > > 
> > > > > > The two issues I can think of in the first category is devices that have
> > > > > > a camera privacy light that could cause worries among users if it
> > > > > > flashes at boot time, and devices that agressively optimize boot time.
> > > > > > 
> > > > > > In the second category, I know that some people use camera serdes
> > > > > > (FPD-Link, GMSL, ...) that are controlled by userspace. As they should
> > > > > > instead use kernel drivers for those components, upstream may not care
> > > > > > too much about this use case. Another issue I was told about was a
> > > > > > device booting in temperatures that were too low for the camera to
> > > > > > operate, which then needed half an hour to heat the device enclosure
> > > > > > before the sensor and serdes could be accessed. That's a bit extreme,
> > > > > > but it sounds like a valid use case to me.
> > > > > > 
> > > > > > What do we do with those cases ? Detecting devices at probe time does
> > > > > > have value, so I think it should be a policy decision. We may want to
> > > > > > convey some of that information through DT properties (I'm not sure what
> > > > > > would be acceptable there though). In any case, that's quite a bit of
> > > > > > yak shaving, so I'm inclined to accept this series (or rather its next
> > > > > > version), given that quite a few other camera sensor drivers detect the
> > > > > > device at probe time. I would however like feedback on the problem to
> > > > > > try and find a good solution.
> > > > > 
> > > > > Most of the issues you mentioned applies to other hardware peripherals also IMO.
> > > > > And it is common for the drivers to read registers and make sure the device is
> > > > > detected on the bus during probe().
> > > > 
> > > > That's true. I think the problem affects different device types
> > > > differently though, and this may (or may not) call for different
> > > > solutions.
> > > > 
> > > > > If an usecase doesn't want to read the
> > > > > registers during probe time, then they _should_not_ build the driver as built-in
> > > > > rather make it as a loadable module and load it whenever necessary. This applies
> > > > > to boot time optimization as well.
> > > > 
> > > > For most of the use cases I listed I agree with you. One exception is
> > > > the privacy light issue. Regardless of when the camera sensor driver is
> > > > loaded, powering the device at probe time will flash the privacy light.
> > > > Doing so later than boot time would probably make the issue even worse,
> > > > I would worry more if I saw my webcam privacy light flashing at a random
> > > > point after boot time.
> > > 
> > > I'm not familiar with the privacy light feature in camera sensors, but is there
> > > no way to prevent it from enabling by default? If that's not possible, it makes
> > > sense to disable it using a DT property as it is a hardware feature.
> > 
> > The whole point of the privacy light is that it shouldn't be possible to
> > disable it by software. Otherwise malicious software could try to work
> > around it. On many devices it is hardwired to one of the camera sensor's
> > power supplies.
> 
> Ah okay, please forgive my ignorance here. But still I'm not sure about the DT
> usage. Maybe it is best to send out a bindings patch and see what the
> maintainers have to say?

Yes, that's a good path forward. I don't think it should block this
patch though.