[PATCH 0/2] media: i2c: imx290: check for availability in probe()

[PATCH 0/2] media: i2c: imx290: check for availability in probe()

[Benjamin Bara]

Hi!

First commit is optional and just adds the possibility to do a
cci_read() without caring about the read value. If not wanted, I can
remove it.

Second commit tries to communicate with the sensor (reading back the
STANDBY register) to find out if the sensor is available at probe time.
Currently, the first device communication is happening after the v4l2
subdev is initialized - and the communication errors are then basically
ignored.

thanks & regards
Benjamin

---
Benjamin Bara (2):
      media: v4l2-cci: Allow "empty read"
      media: i2c: imx290: Check for availability in probe()

 drivers/media/i2c/imx290.c         | 5 +++++
 drivers/media/v4l2-core/v4l2-cci.c | 5 ++++-
 include/media/v4l2-cci.h           | 2 +-
 3 files changed, 10 insertions(+), 2 deletions(-)
---
base-commit: eec5d86d5bac6b3e972eb9c1898af3c08303c52d
change-id: 20240807-imx290-avail-85795c27d988

Best regards,
-- 
Benjamin Bara <benjamin.bara@skidata.com>

[PATCH 1/2] media: v4l2-cci: Allow "empty read"

[Benjamin Bara]

Make the read pointer optional for cases where only the return value is
of interest. This could be the case for availability checks during
probe().

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/media/v4l2-core/v4l2-cci.c | 5 ++++-
 include/media/v4l2-cci.h           | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-cci.c b/drivers/media/v4l2-core/v4l2-cci.c
index 1ff94affbaf3..c402e0377a57 100644
--- a/drivers/media/v4l2-core/v4l2-cci.c
+++ b/drivers/media/v4l2-core/v4l2-cci.c
@@ -30,7 +30,8 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)
 	 * to a valid value whenever this function returns 0 but smatch
 	 * can't figure that out currently.
 	 */
-	*val = 0;
+	if (val)
+		*val = 0;
 
 	if (err && *err)
 		return *err;
@@ -45,6 +46,8 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)
 			reg, ret);
 		goto out;
 	}
+	if (!val)
+		goto out;
 
 	switch (len) {
 	case 1:
diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h
index 4e96e90ee636..ef5e0d875e68 100644
--- a/include/media/v4l2-cci.h
+++ b/include/media/v4l2-cci.h
@@ -60,7 +60,7 @@ struct cci_reg_sequence {
  *
  * @map: Register map to read from
  * @reg: Register address to read, use CCI_REG#() macros to encode reg width
- * @val: Pointer to store read value
+ * @val: Optional pointer to store read value
  * @err: Optional pointer to store errors, if a previous error is set
  *       then the read will be skipped
  *

-- 
2.46.0

[PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Benjamin Bara]

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>
---
 drivers/media/i2c/imx290.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 4150e6e4b9a6..a86076e42a36 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1580,6 +1580,11 @@ 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, NULL, NULL);
+	if (ret)
+		goto err_pm;
+
 	/* Initialize the V4L2 subdev. */
 	ret = imx290_subdev_init(imx290);
 	if (ret)

-- 
2.46.0

Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Alexander Stein]

Hi Benjamin,

Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> 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.

There is already a patch reading the ID register at [1]. This also reads the
ID register. But I don't have any documentation regarding that register,
neither address nor values definitions. If there is known information about
that I would prefer reading the ID and compare it to expected values.

Best regards,
Alexander

[1] https://gitlab.com/ideasonboard/nxp/linux/-/commit/85ce725f1de7c16133bfb92b2ab0d3d84efcdb47

> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/media/i2c/imx290.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 4150e6e4b9a6..a86076e42a36 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -1580,6 +1580,11 @@ 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, NULL, NULL);
> +	if (ret)
> +		goto err_pm;
> +
>  	/* Initialize the V4L2 subdev. */
>  	ret = imx290_subdev_init(imx290);
>  	if (ret)
> 
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Sakari Ailus]

Hi,

On Wed, Aug 07, 2024 at 10:33:51AM +0200, Alexander Stein wrote:
> Hi Benjamin,
> 
> Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> > 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.
> 
> There is already a patch reading the ID register at [1]. This also reads the
> ID register. But I don't have any documentation regarding that register,
> neither address nor values definitions. If there is known information about
> that I would prefer reading the ID and compare it to expected values.
> 
> Best regards,
> Alexander
> 
> [1] https://gitlab.com/ideasonboard/nxp/linux/-/commit/85ce725f1de7c16133bfb92b2ab0d3d84efcdb47

I'd also prefer reading a register and indeed comparing the read value with
the expected value.

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Benjamin Bara]

Hi!

On Wed, 7 Aug 2024 at 10:43, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> Hi,
>
> On Wed, Aug 07, 2024 at 10:33:51AM +0200, Alexander Stein wrote:
> > Hi Benjamin,
> >
> > Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> > > 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.
> >
> > There is already a patch reading the ID register at [1]. This also reads the
> > ID register. But I don't have any documentation regarding that register,
> > neither address nor values definitions. If there is known information about
> > that I would prefer reading the ID and compare it to expected values.
> >
> > Best regards,
> > Alexander
> >
> > [1] https://gitlab.com/ideasonboard/nxp/linux/-/commit/85ce725f1de7c16133bfb92b2ab0d3d84efcdb47
>
> I'd also prefer reading a register and indeed comparing the read value with
> the expected value.

Sure - I can drop the first patch and check if it contains a "1" (standby).
Probably we learn more about the CHIP_ID, then I can switch to it instead.

Best regards
Benjamin

> --
> Regards,
>
> Sakari Ailus

Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Benjamin Bara]

Hi Alexander,

On Wed, 7 Aug 2024 at 10:33, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
> Hi Benjamin,
>
> Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> > 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.
>
> There is already a patch reading the ID register at [1]. This also reads the
> ID register. But I don't have any documentation regarding that register,
> neither address nor values definitions. If there is known information about
> that I would prefer reading the ID and compare it to expected values.

Thanks for the link - it seems like Laurent has dropped the patch for
the more recent kernel versions on their GitLab.

This was also my initial intention, but similar to you, I don't have a
docu describing this register, so I am not sure where the info is coming
from and if it really contains the identification/type info. Probably
Laurent has more infos on that.

Best regards
Benjamin

> Best regards,
> Alexander
>
> [1] https://gitlab.com/ideasonboard/nxp/linux/-/commit/85ce725f1de7c16133bfb92b2ab0d3d84efcdb47
>
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> >  drivers/media/i2c/imx290.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 4150e6e4b9a6..a86076e42a36 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -1580,6 +1580,11 @@ 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, NULL, NULL);
> > +     if (ret)
> > +             goto err_pm;
> > +
> >       /* Initialize the V4L2 subdev. */
> >       ret = imx290_subdev_init(imx290);
> >       if (ret)
> >
> >
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Laurent Pinchart]

Hi Benjamin,

On Wed, Aug 07, 2024 at 10:47:39AM +0200, Benjamin Bara wrote:
> On Wed, 7 Aug 2024 at 10:33, Alexander Stein wrote:
> > Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> > > 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.
> >
> > There is already a patch reading the ID register at [1]. This also reads the
> > ID register. But I don't have any documentation regarding that register,
> > neither address nor values definitions. If there is known information about
> > that I would prefer reading the ID and compare it to expected values.
> 
> Thanks for the link - it seems like Laurent has dropped the patch for
> the more recent kernel versions on their GitLab.

It was a patch that I wrote as a test, and I decided not to upstream it
as it had limited value to me. The downside with reading registers at
probe time is that you have to power up the sensor. This can have
undesired side effects, such as flashing a privacy LED on at boot time
in devices that have one. There's also the increase in boot time due to
the power up sequence, which one may want to avoid.

The imx290 driver already powers up the device unconditionally at probe
time, so reading the version register wouldn't be much of an issue I
suppose. I would be fine merging that patch.

> This was also my initial intention, but similar to you, I don't have a
> docu describing this register, so I am not sure where the info is coming
> from and if it really contains the identification/type info. Probably
> Laurent has more infos on that.

That's a good question. I don't see a mention of that register in the
IMX290 datasheet I've found online
(https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf).
Looking at the git history, the IMX290_CHIP_ID register macro was
introduced in an unrelated commit, without an explanation. I don't
recall where it comes from, but I don't think I've added it randomly. It
may have come from an out-of-tree driver.

I don't have an IMX290 plugged in at the moment, what's the value of the
register ?

> > [1] https://gitlab.com/ideasonboard/nxp/linux/-/commit/85ce725f1de7c16133bfb92b2ab0d3d84efcdb47
> >
> > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > ---
> > >  drivers/media/i2c/imx290.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 4150e6e4b9a6..a86076e42a36 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -1580,6 +1580,11 @@ 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, NULL, NULL);
> > > +     if (ret)
> > > +             goto err_pm;
> > > +
> > >       /* Initialize the V4L2 subdev. */
> > >       ret = imx290_subdev_init(imx290);
> > >       if (ret)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Benjamin Bara]

Hi!

On Wed, 7 Aug 2024 at 11:50, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Aug 07, 2024 at 10:47:39AM +0200, Benjamin Bara wrote:
> > On Wed, 7 Aug 2024 at 10:33, Alexander Stein wrote:
> > > Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> > > > 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.
> > >
> > > There is already a patch reading the ID register at [1]. This also reads the
> > > ID register. But I don't have any documentation regarding that register,
> > > neither address nor values definitions. If there is known information about
> > > that I would prefer reading the ID and compare it to expected values.
> >
> > Thanks for the link - it seems like Laurent has dropped the patch for
> > the more recent kernel versions on their GitLab.
>
> It was a patch that I wrote as a test, and I decided not to upstream it
> as it had limited value to me. The downside with reading registers at
> probe time is that you have to power up the sensor. This can have
> undesired side effects, such as flashing a privacy LED on at boot time
> in devices that have one. There's also the increase in boot time due to
> the power up sequence, which one may want to avoid.
>
> The imx290 driver already powers up the device unconditionally at probe
> time, so reading the version register wouldn't be much of an issue I
> suppose. I would be fine merging that patch.
>
> > This was also my initial intention, but similar to you, I don't have a
> > docu describing this register, so I am not sure where the info is coming
> > from and if it really contains the identification/type info. Probably
> > Laurent has more infos on that.
>
> That's a good question. I don't see a mention of that register in the
> IMX290 datasheet I've found online
> (https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf).
> Looking at the git history, the IMX290_CHIP_ID register macro was
> introduced in an unrelated commit, without an explanation. I don't
> recall where it comes from, but I don't think I've added it randomly. It
> may have come from an out-of-tree driver.

Thanks for the info!

> I don't have an IMX290 plugged in at the moment, what's the value of the
> register ?

I currently have an imx462 available, which is not "officially supported" yet,
but basically an imx290 derivative. With your patch applied:

[   10.424187] imx290 7-001a: chip ID 0x07d0

Best regards
Benjamin

>
> > > [1] https://gitlab.com/ideasonboard/nxp/linux/-/commit/85ce725f1de7c16133bfb92b2ab0d3d84efcdb47
> > >
> > > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > > ---
> > > >  drivers/media/i2c/imx290.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index 4150e6e4b9a6..a86076e42a36 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -1580,6 +1580,11 @@ 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, NULL, NULL);
> > > > +     if (ret)
> > > > +             goto err_pm;
> > > > +
> > > >       /* Initialize the V4L2 subdev. */
> > > >       ret = imx290_subdev_init(imx290);
> > > >       if (ret)

Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Alexander Stein]

Hi,

Am Mittwoch, 7. August 2024, 13:07:24 CEST schrieb Benjamin Bara:
> On Wed, 7 Aug 2024 at 11:50, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Wed, Aug 07, 2024 at 10:47:39AM +0200, Benjamin Bara wrote:
> > > On Wed, 7 Aug 2024 at 10:33, Alexander Stein wrote:
> > > > Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> > > > > 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.
> > > >
> > > > There is already a patch reading the ID register at [1]. This also reads the
> > > > ID register. But I don't have any documentation regarding that register,
> > > > neither address nor values definitions. If there is known information about
> > > > that I would prefer reading the ID and compare it to expected values.
> > >
> > > Thanks for the link - it seems like Laurent has dropped the patch for
> > > the more recent kernel versions on their GitLab.
> >
> > It was a patch that I wrote as a test, and I decided not to upstream it
> > as it had limited value to me. The downside with reading registers at
> > probe time is that you have to power up the sensor. This can have
> > undesired side effects, such as flashing a privacy LED on at boot time
> > in devices that have one. There's also the increase in boot time due to
> > the power up sequence, which one may want to avoid.
> >
> > The imx290 driver already powers up the device unconditionally at probe
> > time, so reading the version register wouldn't be much of an issue I
> > suppose. I would be fine merging that patch.
> >
> > > This was also my initial intention, but similar to you, I don't have a
> > > docu describing this register, so I am not sure where the info is coming
> > > from and if it really contains the identification/type info. Probably
> > > Laurent has more infos on that.
> >
> > That's a good question. I don't see a mention of that register in the
> > IMX290 datasheet I've found online
> > (https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf).
> > Looking at the git history, the IMX290_CHIP_ID register macro was
> > introduced in an unrelated commit, without an explanation. I don't
> > recall where it comes from, but I don't think I've added it randomly. It
> > may have come from an out-of-tree driver.
> 
> Thanks for the info!
> 
> > I don't have an IMX290 plugged in at the moment, what's the value of the
> > register ?
> 
> I currently have an imx462 available, which is not "officially supported" yet,
> but basically an imx290 derivative. With your patch applied:
> 
> [   10.424187] imx290 7-001a: chip ID 0x07d0

Okay, this is from a imx327lqr:

[   15.265086] imx290 3-001a: chip ID 0x07d0

Doesn't look like an ID register to me.

Best regards,
Alexabder

> 
> >
> > > > [1] https://gitlab.com/ideasonboard/nxp/linux/-/commit/85ce725f1de7c16133bfb92b2ab0d3d84efcdb47
> > > >
> > > > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > > > ---
> > > > >  drivers/media/i2c/imx290.c | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > > index 4150e6e4b9a6..a86076e42a36 100644
> > > > > --- a/drivers/media/i2c/imx290.c
> > > > > +++ b/drivers/media/i2c/imx290.c
> > > > > @@ -1580,6 +1580,11 @@ 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, NULL, NULL);
> > > > > +     if (ret)
> > > > > +             goto err_pm;
> > > > > +
> > > > >       /* Initialize the V4L2 subdev. */
> > > > >       ret = imx290_subdev_init(imx290);
> > > > >       if (ret)
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Laurent Pinchart]

On Wed, Aug 07, 2024 at 02:12:04PM +0200, Alexander Stein wrote:
> Am Mittwoch, 7. August 2024, 13:07:24 CEST schrieb Benjamin Bara:
> > On Wed, 7 Aug 2024 at 11:50, Laurent Pinchart wrote:
> > > On Wed, Aug 07, 2024 at 10:47:39AM +0200, Benjamin Bara wrote:
> > > > On Wed, 7 Aug 2024 at 10:33, Alexander Stein wrote:
> > > > > Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> > > > > > 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.
> > > > >
> > > > > There is already a patch reading the ID register at [1]. This also reads the
> > > > > ID register. But I don't have any documentation regarding that register,
> > > > > neither address nor values definitions. If there is known information about
> > > > > that I would prefer reading the ID and compare it to expected values.
> > > >
> > > > Thanks for the link - it seems like Laurent has dropped the patch for
> > > > the more recent kernel versions on their GitLab.
> > >
> > > It was a patch that I wrote as a test, and I decided not to upstream it
> > > as it had limited value to me. The downside with reading registers at
> > > probe time is that you have to power up the sensor. This can have
> > > undesired side effects, such as flashing a privacy LED on at boot time
> > > in devices that have one. There's also the increase in boot time due to
> > > the power up sequence, which one may want to avoid.
> > >
> > > The imx290 driver already powers up the device unconditionally at probe
> > > time, so reading the version register wouldn't be much of an issue I
> > > suppose. I would be fine merging that patch.
> > >
> > > > This was also my initial intention, but similar to you, I don't have a
> > > > docu describing this register, so I am not sure where the info is coming
> > > > from and if it really contains the identification/type info. Probably
> > > > Laurent has more infos on that.
> > >
> > > That's a good question. I don't see a mention of that register in the
> > > IMX290 datasheet I've found online
> > > (https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf).
> > > Looking at the git history, the IMX290_CHIP_ID register macro was
> > > introduced in an unrelated commit, without an explanation. I don't
> > > recall where it comes from, but I don't think I've added it randomly. It
> > > may have come from an out-of-tree driver.
> > 
> > Thanks for the info!
> > 
> > > I don't have an IMX290 plugged in at the moment, what's the value of the
> > > register ?
> > 
> > I currently have an imx462 available, which is not "officially supported" yet,
> > but basically an imx290 derivative. With your patch applied:
> > 
> > [   10.424187] imx290 7-001a: chip ID 0x07d0
> 
> Okay, this is from a imx327lqr:
> 
> [   15.265086] imx290 3-001a: chip ID 0x07d0
> 
> Doesn't look like an ID register to me.

Indeed, it's quite suspicious.

I wonder if we could find a more applicable register. Chip ID registers
are usually located at the beginning or end of the register space, we
could have a look there.

> > > > > [1] https://gitlab.com/ideasonboard/nxp/linux/-/commit/85ce725f1de7c16133bfb92b2ab0d3d84efcdb47
> > > > >
> > > > > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > > > > ---
> > > > > >  drivers/media/i2c/imx290.c | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > > > index 4150e6e4b9a6..a86076e42a36 100644
> > > > > > --- a/drivers/media/i2c/imx290.c
> > > > > > +++ b/drivers/media/i2c/imx290.c
> > > > > > @@ -1580,6 +1580,11 @@ 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, NULL, NULL);
> > > > > > +     if (ret)
> > > > > > +             goto err_pm;
> > > > > > +
> > > > > >       /* Initialize the V4L2 subdev. */
> > > > > >       ret = imx290_subdev_init(imx290);
> > > > > >       if (ret)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Alexander Stein]

Am Mittwoch, 7. August 2024, 14:16:48 CEST schrieb Laurent Pinchart:
> ********************
> Achtung externe E-Mail: Öffnen Sie Anhänge und Links nur, wenn Sie wissen, dass diese aus einer sicheren Quelle stammen und sicher sind. Leiten Sie die E-Mail im Zweifelsfall zur Prüfung an den IT-Helpdesk weiter.
> Attention external email: Open attachments and links only if you know that they are from a secure source and are safe. In doubt forward the email to the IT-Helpdesk to check it.
> ********************
> 
> On Wed, Aug 07, 2024 at 02:12:04PM +0200, Alexander Stein wrote:
> > Am Mittwoch, 7. August 2024, 13:07:24 CEST schrieb Benjamin Bara:
> > > On Wed, 7 Aug 2024 at 11:50, Laurent Pinchart wrote:
> > > > On Wed, Aug 07, 2024 at 10:47:39AM +0200, Benjamin Bara wrote:
> > > > > On Wed, 7 Aug 2024 at 10:33, Alexander Stein wrote:
> > > > > > Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> > > > > > > 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.
> > > > > >
> > > > > > There is already a patch reading the ID register at [1]. This also reads the
> > > > > > ID register. But I don't have any documentation regarding that register,
> > > > > > neither address nor values definitions. If there is known information about
> > > > > > that I would prefer reading the ID and compare it to expected values.
> > > > >
> > > > > Thanks for the link - it seems like Laurent has dropped the patch for
> > > > > the more recent kernel versions on their GitLab.
> > > >
> > > > It was a patch that I wrote as a test, and I decided not to upstream it
> > > > as it had limited value to me. The downside with reading registers at
> > > > probe time is that you have to power up the sensor. This can have
> > > > undesired side effects, such as flashing a privacy LED on at boot time
> > > > in devices that have one. There's also the increase in boot time due to
> > > > the power up sequence, which one may want to avoid.
> > > >
> > > > The imx290 driver already powers up the device unconditionally at probe
> > > > time, so reading the version register wouldn't be much of an issue I
> > > > suppose. I would be fine merging that patch.
> > > >
> > > > > This was also my initial intention, but similar to you, I don't have a
> > > > > docu describing this register, so I am not sure where the info is coming
> > > > > from and if it really contains the identification/type info. Probably
> > > > > Laurent has more infos on that.
> > > >
> > > > That's a good question. I don't see a mention of that register in the
> > > > IMX290 datasheet I've found online
> > > > (https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf).
> > > > Looking at the git history, the IMX290_CHIP_ID register macro was
> > > > introduced in an unrelated commit, without an explanation. I don't
> > > > recall where it comes from, but I don't think I've added it randomly. It
> > > > may have come from an out-of-tree driver.
> > > 
> > > Thanks for the info!
> > > 
> > > > I don't have an IMX290 plugged in at the moment, what's the value of the
> > > > register ?
> > > 
> > > I currently have an imx462 available, which is not "officially supported" yet,
> > > but basically an imx290 derivative. With your patch applied:
> > > 
> > > [   10.424187] imx290 7-001a: chip ID 0x07d0
> > 
> > Okay, this is from a imx327lqr:
> > 
> > [   15.265086] imx290 3-001a: chip ID 0x07d0
> > 
> > Doesn't look like an ID register to me.
> 
> Indeed, it's quite suspicious.
> 
> I wonder if we could find a more applicable register. Chip ID registers
> are usually located at the beginning or end of the register space, we
> could have a look there.

Dumping all registers (8-Bit reads) from 0x3001 till 0x3480 only has a few
non-zero registers:
> # cat /sys/kernel/debug/regmap/3-001a/range
> 3000-3480
> # cat /sys/kernel/debug/regmap/3-001a/registers  | grep -v ": 00"
> 3000: 01
> 3020: 01
> 303c: 08
> 303e: 38
> 303f: 04
> 3040: 08
> 3042: 80
> 3043: 07
> 319a: d0
> 319b: 07
> 3418: 38
> 3419: 04
> 3472: 80
> 3473: 07

Note I am on a Vision Components imx327, which might block some reads.
Laurent is also aware of that behaviour. But maybe this list gives an
indicator.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/2] media: i2c: imx290: Check for availability in probe()

[Alexander Stein]

Am Mittwoch, 7. August 2024, 14:39:01 CEST schrieb Alexander Stein:
> ********************
> Achtung externe E-Mail: Öffnen Sie Anhänge und Links nur, wenn Sie wissen, dass diese aus einer sicheren Quelle stammen und sicher sind. Leiten Sie die E-Mail im Zweifelsfall zur Prüfung an den IT-Helpdesk weiter.
> Attention external email: Open attachments and links only if you know that they are from a secure source and are safe. In doubt forward the email to the IT-Helpdesk to check it.
> ********************
> 
> Am Mittwoch, 7. August 2024, 14:16:48 CEST schrieb Laurent Pinchart:
> > ********************
> > Achtung externe E-Mail: Öffnen Sie Anhänge und Links nur, wenn Sie wissen, dass diese aus einer sicheren Quelle stammen und sicher sind. Leiten Sie die E-Mail im Zweifelsfall zur Prüfung an den IT-Helpdesk weiter.
> > Attention external email: Open attachments and links only if you know that they are from a secure source and are safe. In doubt forward the email to the IT-Helpdesk to check it.
> > ********************
> > 
> > On Wed, Aug 07, 2024 at 02:12:04PM +0200, Alexander Stein wrote:
> > > Am Mittwoch, 7. August 2024, 13:07:24 CEST schrieb Benjamin Bara:
> > > > On Wed, 7 Aug 2024 at 11:50, Laurent Pinchart wrote:
> > > > > On Wed, Aug 07, 2024 at 10:47:39AM +0200, Benjamin Bara wrote:
> > > > > > On Wed, 7 Aug 2024 at 10:33, Alexander Stein wrote:
> > > > > > > Am Mittwoch, 7. August 2024, 10:10:28 CEST schrieb Benjamin Bara:
> > > > > > > > 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.
> > > > > > >
> > > > > > > There is already a patch reading the ID register at [1]. This also reads the
> > > > > > > ID register. But I don't have any documentation regarding that register,
> > > > > > > neither address nor values definitions. If there is known information about
> > > > > > > that I would prefer reading the ID and compare it to expected values.
> > > > > >
> > > > > > Thanks for the link - it seems like Laurent has dropped the patch for
> > > > > > the more recent kernel versions on their GitLab.
> > > > >
> > > > > It was a patch that I wrote as a test, and I decided not to upstream it
> > > > > as it had limited value to me. The downside with reading registers at
> > > > > probe time is that you have to power up the sensor. This can have
> > > > > undesired side effects, such as flashing a privacy LED on at boot time
> > > > > in devices that have one. There's also the increase in boot time due to
> > > > > the power up sequence, which one may want to avoid.
> > > > >
> > > > > The imx290 driver already powers up the device unconditionally at probe
> > > > > time, so reading the version register wouldn't be much of an issue I
> > > > > suppose. I would be fine merging that patch.
> > > > >
> > > > > > This was also my initial intention, but similar to you, I don't have a
> > > > > > docu describing this register, so I am not sure where the info is coming
> > > > > > from and if it really contains the identification/type info. Probably
> > > > > > Laurent has more infos on that.
> > > > >
> > > > > That's a good question. I don't see a mention of that register in the
> > > > > IMX290 datasheet I've found online
> > > > > (https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf).
> > > > > Looking at the git history, the IMX290_CHIP_ID register macro was
> > > > > introduced in an unrelated commit, without an explanation. I don't
> > > > > recall where it comes from, but I don't think I've added it randomly. It
> > > > > may have come from an out-of-tree driver.
> > > > 
> > > > Thanks for the info!
> > > > 
> > > > > I don't have an IMX290 plugged in at the moment, what's the value of the
> > > > > register ?
> > > > 
> > > > I currently have an imx462 available, which is not "officially supported" yet,
> > > > but basically an imx290 derivative. With your patch applied:
> > > > 
> > > > [   10.424187] imx290 7-001a: chip ID 0x07d0
> > > 
> > > Okay, this is from a imx327lqr:
> > > 
> > > [   15.265086] imx290 3-001a: chip ID 0x07d0
> > > 
> > > Doesn't look like an ID register to me.
> > 
> > Indeed, it's quite suspicious.
> > 
> > I wonder if we could find a more applicable register. Chip ID registers
> > are usually located at the beginning or end of the register space, we
> > could have a look there.
> 
> Dumping all registers (8-Bit reads) from 0x3001 till 0x3480 only has a few
> non-zero registers:
> > # cat /sys/kernel/debug/regmap/3-001a/range
> > 3000-3480
> > # cat /sys/kernel/debug/regmap/3-001a/registers  | grep -v ": 00"
> > 3000: 01
> > 3020: 01
> > 303c: 08
> > 303e: 38
> > 303f: 04
> > 3040: 08
> > 3042: 80
> > 3043: 07
> > 319a: d0
> > 319b: 07
> > 3418: 38
> > 3419: 04
> > 3472: 80
> > 3473: 07
> 
> Note I am on a Vision Components imx327, which might block some reads.
> Laurent is also aware of that behaviour. But maybe this list gives an
> indicator.

Ah, just for the records. I hacked the kernel to get these debugfs entries
for cci regmaps. They are not available by default.

Cheers
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/