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

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

[bbara93]

Hi!

This series introduces i2c communication with the imx290 sensor during
probe s.t. the v4l2 subdev is not initialized when the chip is not
reachable.

The datasheets show that INCKSEL* registers have a different default
value after reset on imx290[1] and imx327[2], however I am not sure if
this is a sufficient identification option - therefore I just removed
the current CHIP_ID register for now.

Thank you all for the feedback and the discussion, I updated the series
to contain some of the ideas that came up.

For now, I kept reading back the content of the STANDBY register, as
suggested by Sakari and Alexander. If not wanted (as suggested by
Laurent), I can re-add my "optional read" commit from the first version
of this series instead.

*Overview:*
Patch 1/7 is a split from the old 1/2 which defines the values of the
STANDBY register and uses them.
Patch 2/7 is new and defines the whole supported range of the controls.
Patch 3/7 is the old 2/2 to drop the CHIP_ID register.
Patch 4+5/7 are new and target the avoidable communication during
probe(). I decided to use a variant that also works on machines without
runtime PM active, and falls back to the values of 2/7 instead.
Additionally, imx290_runtime_suspend() is using v4l_subdev, and
therefore depends on the subdevice. If we move the autosuspend stuff
before the subdevice creation, we basically have a race between the
delay and the subdevice creation. Although it is not very close, I don't
think it is a good idea to potentially autosuspend before the subdev is
created.
Patch 6/7 is the old 1/2.
Patch 7/7 is a new PoC to decide to check the availability based on the
power state of the sensor during probe().

Note: dummy-regulators, which are used when no supplies are set in the
DT, are always-on.
Note2: The "off" mode is currently only active after probe(). If this
approach is of interest, I would also ensure that it is active once
streaming has stopped - need to dig deeper if it is necessary to store
the "old current" before stopping, therefore only "initial" for now.

thanks & regards
Benjamin

[1] https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf
[2] https://e2e.ti.com/cfs-file/__key/communityserver-discussions-components-files/138/IMX327LQR_2D00_C_5F00_TechnicalDatasheet_5F00_E_5F00_Rev0.2.pdf

---
Changes in v3:
- probably better readable in the overview
- 1/2 -> 1+6/7
- 2/2 -> 3/7 (added R-b - thx for that)
- others are new based on the discussions/suggestions
- Link to v2: https://lore.kernel.org/r/20240828-imx290-avail-v2-0-bd320ac8e8fa@skidata.com

Changes in v2:
- dropped 1/2 to ignore the read value in cci_read() (old 2/2 -> new 1/2)
- 1/2: read-back standby mode instead and ensure that it is really in standby
- new 2/2: drop chip_id register definition which seems to be incorrect
- Link to v1: https://lore.kernel.org/r/20240807-imx290-avail-v1-0-666c130c7601@skidata.com

---
Benjamin Bara (7):
      media: i2c: imx290: Define standby mode values
      media: i2c: imx290: Define absolute control ranges
      media: i2c: imx290: Remove CHIP_ID reg definition
      media: i2c: imx290: Introduce initial "off" mode & link freq
      media: i2c: imx290: Avoid communication during probe()
      media: i2c: imx290: Check for availability in probe()
      media: i2c: imx290: Implement a "privacy mode" for probe()

 drivers/media/i2c/imx290.c | 153 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 124 insertions(+), 29 deletions(-)
---
base-commit: eec5d86d5bac6b3e972eb9c1898af3c08303c52d
change-id: 20240807-imx290-avail-85795c27d988

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

[PATCH v3 1/7] media: i2c: imx290: Define standby mode values

[bbara93]

From: Benjamin Bara <benjamin.bara@skidata.com>

The imx290 datasheet states that the IMX290_STANDBY register has two
values: 0 for operating and 1 for standby. Define and use them.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
Changes since v2:
- new, split out from the previous 1/2
---
 drivers/media/i2c/imx290.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 4150e6e4b9a6..1c97f9650eb4 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			0x00
+#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,8 @@ 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);
 
 	msleep(30);
 
@@ -1029,7 +1032,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);
 

-- 
2.46.0

Re: [PATCH v3 1/7] media: i2c: imx290: Define standby mode values

[Laurent Pinchart]

Hi Benjamin,

On Mon, Sep 02, 2024 at 05:57:26PM +0200, bbara93@gmail.com wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> The imx290 datasheet states that the IMX290_STANDBY register has two
> values: 0 for operating and 1 for standby. Define and use them.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> Changes since v2:
> - new, split out from the previous 1/2
> ---
>  drivers/media/i2c/imx290.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 4150e6e4b9a6..1c97f9650eb4 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			0x00
> +#define IMX290_STANDBY_STANDBY				BIT(0)

The convention, for single-bit fields, is to define a macro to describe
the bit, but not a macro to describe the bit not being set.

>  #define IMX290_REGHOLD					CCI_REG8(0x3001)
>  #define IMX290_XMSTA					CCI_REG8(0x3002)
>  #define IMX290_ADBIT					CCI_REG8(0x3005)
> @@ -1016,7 +1018,8 @@ 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);

I would thus rather drop this change.

>  
>  	msleep(30);
>  
> @@ -1029,7 +1032,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 keep this one.

>  
>  	msleep(30);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/7] media: i2c: imx290: Define standby mode values

[Benjamin Bara]

Hi Laurent!

Thanks for your feedback!

On Mon, 2 Sept 2024 at 21:56, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Sep 02, 2024 at 05:57:26PM +0200, bbara93@gmail.com wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > The imx290 datasheet states that the IMX290_STANDBY register has two
> > values: 0 for operating and 1 for standby. Define and use them.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > Changes since v2:
> > - new, split out from the previous 1/2
> > ---
> >  drivers/media/i2c/imx290.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 4150e6e4b9a6..1c97f9650eb4 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                     0x00
> > +#define IMX290_STANDBY_STANDBY                               BIT(0)
>
> The convention, for single-bit fields, is to define a macro to describe
> the bit, but not a macro to describe the bit not being set.
>
> >  #define IMX290_REGHOLD                                       CCI_REG8(0x3001)
> >  #define IMX290_XMSTA                                 CCI_REG8(0x3002)
> >  #define IMX290_ADBIT                                 CCI_REG8(0x3005)
> > @@ -1016,7 +1018,8 @@ 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);
>
> I would thus rather drop this change.
>
> >
> >       msleep(30);
> >
> > @@ -1029,7 +1032,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 keep this one.
>
> >
> >       msleep(30);
> >

Thanks, will do in the next version.

Kind regards
Benjamin

>
> --
> Regards,
>
> Laurent Pinchart

[PATCH v3 2/7] media: i2c: imx290: Define absolute control ranges

[bbara93]

From: Benjamin Bara <benjamin.bara@skidata.com>

For now, the driver activates the first mode (1080p) as current active
mode in probe(). This e.g. means that one cannot set VBLANK below 45
(vmax_min - height), although theoretically the minimum is 30 (720p
mode). Define the absolute possible/supported ranges to have them
available later.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
Changes since v2:
- new
---
 drivers/media/i2c/imx290.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 1c97f9650eb4..466492bab600 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -499,6 +499,10 @@ static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
 };
 
 /* Mode configs */
+#define WIDTH_720P	1280
+#define HEIGHT_720P	720
+#define MINIMUM_WIDTH	WIDTH_720P
+#define MINIMUM_HEIGHT	HEIGHT_720P
 static const struct imx290_mode imx290_modes_2lanes[] = {
 	{
 		.width = 1920,
@@ -512,8 +516,8 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 		.clk_cfg = imx290_1080p_clock_config,
 	},
 	{
-		.width = 1280,
-		.height = 720,
+		.width = WIDTH_720P,
+		.height = HEIGHT_720P,
 		.hmax_min = 3300,
 		.vmax_min = 750,
 		.link_freq_index = FREQ_INDEX_720P,
@@ -537,8 +541,8 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 		.clk_cfg = imx290_1080p_clock_config,
 	},
 	{
-		.width = 1280,
-		.height = 720,
+		.width = WIDTH_720P,
+		.height = HEIGHT_720P,
 		.hmax_min = 3300,
 		.vmax_min = 750,
 		.link_freq_index = FREQ_INDEX_720P,
@@ -846,6 +850,30 @@ static const char * const imx290_test_pattern_menu[] = {
 	"000/555h Toggle Pattern",
 };
 
+/* absolute supported control ranges */
+#define HBLANK_MAX	(IMX290_HMAX_MAX - MINIMUM_WIDTH)
+#define VBLANK_MAX	(IMX290_VMAX_MAX - MINIMUM_HEIGHT)
+static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
+{
+	const struct imx290_mode *modes = imx290_modes_ptr(imx290);
+	unsigned int min = UINT_MAX;
+	int i;
+
+	for (i = 0; i < imx290_modes_num(imx290); i++) {
+		unsigned int tmp;
+
+		if (v)
+			tmp = modes[i].hmax_min - modes[i].width;
+		else
+			tmp = modes[i].vmax_min - modes[i].height;
+
+		if (tmp < min)
+			min = tmp;
+	}
+
+	return min;
+}
+
 static void imx290_ctrl_update(struct imx290 *imx290,
 			       const struct imx290_mode *mode)
 {

-- 
2.46.0

Re: [PATCH v3 2/7] media: i2c: imx290: Define absolute control ranges

[Dave Stevenson]

Hi Benjamin

On Mon, 2 Sept 2024 at 16:58, <bbara93@gmail.com> wrote:
>
> From: Benjamin Bara <benjamin.bara@skidata.com>
>
> For now, the driver activates the first mode (1080p) as current active
> mode in probe(). This e.g. means that one cannot set VBLANK below 45
> (vmax_min - height), although theoretically the minimum is 30 (720p
> mode). Define the absolute possible/supported ranges to have them
> available later.

Currently the driver will set the ranges for VBLANK and HBLANK
whenever the mode changes.

How is it helpful to fake these numbers? Seeing as they aren't
reflecting anything useful, they may as well all be 0.

> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> Changes since v2:
> - new
> ---
>  drivers/media/i2c/imx290.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 1c97f9650eb4..466492bab600 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -499,6 +499,10 @@ static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
>  };
>
>  /* Mode configs */
> +#define WIDTH_720P     1280
> +#define HEIGHT_720P    720
> +#define MINIMUM_WIDTH  WIDTH_720P
> +#define MINIMUM_HEIGHT HEIGHT_720P
>  static const struct imx290_mode imx290_modes_2lanes[] = {
>         {
>                 .width = 1920,
> @@ -512,8 +516,8 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>                 .clk_cfg = imx290_1080p_clock_config,
>         },
>         {
> -               .width = 1280,
> -               .height = 720,
> +               .width = WIDTH_720P,
> +               .height = HEIGHT_720P,
>                 .hmax_min = 3300,
>                 .vmax_min = 750,
>                 .link_freq_index = FREQ_INDEX_720P,
> @@ -537,8 +541,8 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>                 .clk_cfg = imx290_1080p_clock_config,
>         },
>         {
> -               .width = 1280,
> -               .height = 720,
> +               .width = WIDTH_720P,
> +               .height = HEIGHT_720P,
>                 .hmax_min = 3300,
>                 .vmax_min = 750,
>                 .link_freq_index = FREQ_INDEX_720P,
> @@ -846,6 +850,30 @@ static const char * const imx290_test_pattern_menu[] = {
>         "000/555h Toggle Pattern",
>  };
>
> +/* absolute supported control ranges */
> +#define HBLANK_MAX     (IMX290_HMAX_MAX - MINIMUM_WIDTH)
> +#define VBLANK_MAX     (IMX290_VMAX_MAX - MINIMUM_HEIGHT)
> +static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> +{

This function is never used in this patch. I'm surprised the compiler
doesn't throw an error on a static function not being used.
You first use it in patch 4 "Introduce initial "off" mode & link freq"

> +       const struct imx290_mode *modes = imx290_modes_ptr(imx290);
> +       unsigned int min = UINT_MAX;
> +       int i;
> +
> +       for (i = 0; i < imx290_modes_num(imx290); i++) {
> +               unsigned int tmp;
> +
> +               if (v)
> +                       tmp = modes[i].hmax_min - modes[i].width;

if (v)
   return h

With the complete series my sensor comes up with controls defined as
vertical_blanking 0x009e0901 (int)    : min=280 max=261423 step=1
default=280 value=280
horizontal_blanking 0x009e0902 (int)    : min=30 max=64255 step=1
default=30 value=30

Set the mode to 1080p and I get
vertical_blanking 0x009e0901 (int)    : min=45 max=261063 step=1
default=45 value=1169
horizontal_blanking 0x009e0902 (int)    : min=280 max=63615 step=1
default=280 value=280

  Dave

> +               else
> +                       tmp = modes[i].vmax_min - modes[i].height;
> +
> +               if (tmp < min)
> +                       min = tmp;
> +       }
> +
> +       return min;
> +}
> +
>  static void imx290_ctrl_update(struct imx290 *imx290,
>                                const struct imx290_mode *mode)
>  {
>
> --
> 2.46.0
>
>

Re: [PATCH v3 2/7] media: i2c: imx290: Define absolute control ranges

[Benjamin Bara]

Hi Dave!

On Mon, 2 Sept 2024 at 20:00, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
> On Mon, 2 Sept 2024 at 16:58, <bbara93@gmail.com> wrote:
> >
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > For now, the driver activates the first mode (1080p) as current active
> > mode in probe(). This e.g. means that one cannot set VBLANK below 45
> > (vmax_min - height), although theoretically the minimum is 30 (720p
> > mode). Define the absolute possible/supported ranges to have them
> > available later.
>
> Currently the driver will set the ranges for VBLANK and HBLANK
> whenever the mode changes.
>
> How is it helpful to fake these numbers? Seeing as they aren't
> reflecting anything useful, they may as well all be 0.
>
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > Changes since v2:
> > - new
> > ---
> >  drivers/media/i2c/imx290.c | 36 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 1c97f9650eb4..466492bab600 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -499,6 +499,10 @@ static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
> >  };
> >
> >  /* Mode configs */
> > +#define WIDTH_720P     1280
> > +#define HEIGHT_720P    720
> > +#define MINIMUM_WIDTH  WIDTH_720P
> > +#define MINIMUM_HEIGHT HEIGHT_720P
> >  static const struct imx290_mode imx290_modes_2lanes[] = {
> >         {
> >                 .width = 1920,
> > @@ -512,8 +516,8 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> >                 .clk_cfg = imx290_1080p_clock_config,
> >         },
> >         {
> > -               .width = 1280,
> > -               .height = 720,
> > +               .width = WIDTH_720P,
> > +               .height = HEIGHT_720P,
> >                 .hmax_min = 3300,
> >                 .vmax_min = 750,
> >                 .link_freq_index = FREQ_INDEX_720P,
> > @@ -537,8 +541,8 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> >                 .clk_cfg = imx290_1080p_clock_config,
> >         },
> >         {
> > -               .width = 1280,
> > -               .height = 720,
> > +               .width = WIDTH_720P,
> > +               .height = HEIGHT_720P,
> >                 .hmax_min = 3300,
> >                 .vmax_min = 750,
> >                 .link_freq_index = FREQ_INDEX_720P,
> > @@ -846,6 +850,30 @@ static const char * const imx290_test_pattern_menu[] = {
> >         "000/555h Toggle Pattern",
> >  };
> >
> > +/* absolute supported control ranges */
> > +#define HBLANK_MAX     (IMX290_HMAX_MAX - MINIMUM_WIDTH)
> > +#define VBLANK_MAX     (IMX290_VMAX_MAX - MINIMUM_HEIGHT)
> > +static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > +{
>
> This function is never used in this patch. I'm surprised the compiler
> doesn't throw an error on a static function not being used.
> You first use it in patch 4 "Introduce initial "off" mode & link freq"
>
> > +       const struct imx290_mode *modes = imx290_modes_ptr(imx290);
> > +       unsigned int min = UINT_MAX;
> > +       int i;
> > +
> > +       for (i = 0; i < imx290_modes_num(imx290); i++) {
> > +               unsigned int tmp;
> > +
> > +               if (v)
> > +                       tmp = modes[i].hmax_min - modes[i].width;
>
> if (v)
>    return h
>
> With the complete series my sensor comes up with controls defined as
> vertical_blanking 0x009e0901 (int)    : min=280 max=261423 step=1
> default=280 value=280
> horizontal_blanking 0x009e0902 (int)    : min=30 max=64255 step=1
> default=30 value=30
>
> Set the mode to 1080p and I get
> vertical_blanking 0x009e0901 (int)    : min=45 max=261063 step=1
> default=45 value=1169
> horizontal_blanking 0x009e0902 (int)    : min=280 max=63615 step=1
> default=280 value=280

The idea here is to have VBLANK=30 available in the initial "after
probe" state of the sensor. VBLANK=30 is a valid value for 720p mode,
but it cannot be set after probe, because the driver (not the user)
decided that 1080 mode is active. The idea is to relax the ranges while
the mode is not set. Once the mode is known, the values are tightened
to the real mode-dependent values.

Kind regards
Benjamin

>   Dave
>
> > +               else
> > +                       tmp = modes[i].vmax_min - modes[i].height;
> > +
> > +               if (tmp < min)
> > +                       min = tmp;
> > +       }
> > +
> > +       return min;
> > +}
> > +
> >  static void imx290_ctrl_update(struct imx290 *imx290,
> >                                const struct imx290_mode *mode)
> >  {
> >
> > --
> > 2.46.0
> >
> >

Re: [PATCH v3 2/7] media: i2c: imx290: Define absolute control ranges

[Laurent Pinchart]

On Mon, Sep 02, 2024 at 09:43:36PM +0200, Benjamin Bara wrote:
> Hi Dave!
> 
> On Mon, 2 Sept 2024 at 20:00, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> > On Mon, 2 Sept 2024 at 16:58, <bbara93@gmail.com> wrote:
> > >
> > > From: Benjamin Bara <benjamin.bara@skidata.com>
> > >
> > > For now, the driver activates the first mode (1080p) as current active
> > > mode in probe(). This e.g. means that one cannot set VBLANK below 45
> > > (vmax_min - height), although theoretically the minimum is 30 (720p
> > > mode). Define the absolute possible/supported ranges to have them
> > > available later.
> >
> > Currently the driver will set the ranges for VBLANK and HBLANK
> > whenever the mode changes.
> >
> > How is it helpful to fake these numbers? Seeing as they aren't
> > reflecting anything useful, they may as well all be 0.
> >
> > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > ---
> > > Changes since v2:
> > > - new
> > > ---
> > >  drivers/media/i2c/imx290.c | 36 ++++++++++++++++++++++++++++++++----
> > >  1 file changed, 32 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 1c97f9650eb4..466492bab600 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -499,6 +499,10 @@ static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
> > >  };
> > >
> > >  /* Mode configs */
> > > +#define WIDTH_720P     1280
> > > +#define HEIGHT_720P    720

That's pure obfuscation :-) Just use the values directly.

> > > +#define MINIMUM_WIDTH  WIDTH_720P
> > > +#define MINIMUM_HEIGHT HEIGHT_720P

Driver-specific macros should have a driver-specific prefix.

> > >  static const struct imx290_mode imx290_modes_2lanes[] = {
> > >         {
> > >                 .width = 1920,
> > > @@ -512,8 +516,8 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> > >                 .clk_cfg = imx290_1080p_clock_config,
> > >         },
> > >         {
> > > -               .width = 1280,
> > > -               .height = 720,
> > > +               .width = WIDTH_720P,
> > > +               .height = HEIGHT_720P,
> > >                 .hmax_min = 3300,
> > >                 .vmax_min = 750,
> > >                 .link_freq_index = FREQ_INDEX_720P,
> > > @@ -537,8 +541,8 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > >                 .clk_cfg = imx290_1080p_clock_config,
> > >         },
> > >         {
> > > -               .width = 1280,
> > > -               .height = 720,
> > > +               .width = WIDTH_720P,
> > > +               .height = HEIGHT_720P,
> > >                 .hmax_min = 3300,
> > >                 .vmax_min = 750,
> > >                 .link_freq_index = FREQ_INDEX_720P,
> > > @@ -846,6 +850,30 @@ static const char * const imx290_test_pattern_menu[] = {
> > >         "000/555h Toggle Pattern",
> > >  };
> > >
> > > +/* absolute supported control ranges */
> > > +#define HBLANK_MAX     (IMX290_HMAX_MAX - MINIMUM_WIDTH)
> > > +#define VBLANK_MAX     (IMX290_VMAX_MAX - MINIMUM_HEIGHT)

Here too.

> > > +static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > > +{
> >
> > This function is never used in this patch. I'm surprised the compiler
> > doesn't throw an error on a static function not being used.
> > You first use it in patch 4 "Introduce initial "off" mode & link freq"
> >
> > > +       const struct imx290_mode *modes = imx290_modes_ptr(imx290);
> > > +       unsigned int min = UINT_MAX;
> > > +       int i;
> > > +
> > > +       for (i = 0; i < imx290_modes_num(imx290); i++) {
> > > +               unsigned int tmp;
> > > +
> > > +               if (v)
> > > +                       tmp = modes[i].hmax_min - modes[i].width;
> >
> > if (v)
> >    return h
> >
> > With the complete series my sensor comes up with controls defined as
> > vertical_blanking 0x009e0901 (int)    : min=280 max=261423 step=1
> > default=280 value=280
> > horizontal_blanking 0x009e0902 (int)    : min=30 max=64255 step=1
> > default=30 value=30
> >
> > Set the mode to 1080p and I get
> > vertical_blanking 0x009e0901 (int)    : min=45 max=261063 step=1
> > default=45 value=1169
> > horizontal_blanking 0x009e0902 (int)    : min=280 max=63615 step=1
> > default=280 value=280
> 
> The idea here is to have VBLANK=30 available in the initial "after
> probe" state of the sensor. VBLANK=30 is a valid value for 720p mode,
> but it cannot be set after probe, because the driver (not the user)
> decided that 1080 mode is active. The idea is to relax the ranges while
> the mode is not set. Once the mode is known, the values are tightened
> to the real mode-dependent values.
> 
> Kind regards
> Benjamin
> 
> >   Dave
> >
> > > +               else
> > > +                       tmp = modes[i].vmax_min - modes[i].height;
> > > +
> > > +               if (tmp < min)
> > > +                       min = tmp;
> > > +       }
> > > +
> > > +       return min;
> > > +}
> > > +
> > >  static void imx290_ctrl_update(struct imx290 *imx290,
> > >                                const struct imx290_mode *mode)
> > >  {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/7] media: i2c: imx290: Define absolute control ranges

[Benjamin Bara]

Hi Laurent!

On Mon, 2 Sept 2024 at 22:06, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Sep 02, 2024 at 09:43:36PM +0200, Benjamin Bara wrote:
> > On Mon, 2 Sept 2024 at 20:00, Dave Stevenson
> > <dave.stevenson@raspberrypi.com> wrote:
> > > On Mon, 2 Sept 2024 at 16:58, <bbara93@gmail.com> wrote:
> > > >
> > > > From: Benjamin Bara <benjamin.bara@skidata.com>
> > > >
> > > > For now, the driver activates the first mode (1080p) as current active
> > > > mode in probe(). This e.g. means that one cannot set VBLANK below 45
> > > > (vmax_min - height), although theoretically the minimum is 30 (720p
> > > > mode). Define the absolute possible/supported ranges to have them
> > > > available later.
> > >
> > > Currently the driver will set the ranges for VBLANK and HBLANK
> > > whenever the mode changes.
> > >
> > > How is it helpful to fake these numbers? Seeing as they aren't
> > > reflecting anything useful, they may as well all be 0.
> > >
> > > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > > ---
> > > > Changes since v2:
> > > > - new
> > > > ---
> > > >  drivers/media/i2c/imx290.c | 36 ++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 32 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index 1c97f9650eb4..466492bab600 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -499,6 +499,10 @@ static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
> > > >  };
> > > >
> > > >  /* Mode configs */
> > > > +#define WIDTH_720P     1280
> > > > +#define HEIGHT_720P    720
>
> That's pure obfuscation :-) Just use the values directly.
>
> > > > +#define MINIMUM_WIDTH  WIDTH_720P
> > > > +#define MINIMUM_HEIGHT HEIGHT_720P
>
> Driver-specific macros should have a driver-specific prefix.
>
> > > >  static const struct imx290_mode imx290_modes_2lanes[] = {
> > > >         {
> > > >                 .width = 1920,
> > > > @@ -512,8 +516,8 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> > > >                 .clk_cfg = imx290_1080p_clock_config,
> > > >         },
> > > >         {
> > > > -               .width = 1280,
> > > > -               .height = 720,
> > > > +               .width = WIDTH_720P,
> > > > +               .height = HEIGHT_720P,
> > > >                 .hmax_min = 3300,
> > > >                 .vmax_min = 750,
> > > >                 .link_freq_index = FREQ_INDEX_720P,
> > > > @@ -537,8 +541,8 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > > >                 .clk_cfg = imx290_1080p_clock_config,
> > > >         },
> > > >         {
> > > > -               .width = 1280,
> > > > -               .height = 720,
> > > > +               .width = WIDTH_720P,
> > > > +               .height = HEIGHT_720P,
> > > >                 .hmax_min = 3300,
> > > >                 .vmax_min = 750,
> > > >                 .link_freq_index = FREQ_INDEX_720P,
> > > > @@ -846,6 +850,30 @@ static const char * const imx290_test_pattern_menu[] = {
> > > >         "000/555h Toggle Pattern",
> > > >  };
> > > >
> > > > +/* absolute supported control ranges */
> > > > +#define HBLANK_MAX     (IMX290_HMAX_MAX - MINIMUM_WIDTH)
> > > > +#define VBLANK_MAX     (IMX290_VMAX_MAX - MINIMUM_HEIGHT)
>
> Here too.

Thanks for the feedback but I will drop this patch. As we need to decide
on a link frequency, we need to decide on one of the two "all pixel"
modes the imx290 supports and therefore already know the mode-specific,
tight ranges starting from probe time.

Kind regards
Benjamin

> > > > +static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > > > +{
> > >
> > > This function is never used in this patch. I'm surprised the compiler
> > > doesn't throw an error on a static function not being used.
> > > You first use it in patch 4 "Introduce initial "off" mode & link freq"
> > >
> > > > +       const struct imx290_mode *modes = imx290_modes_ptr(imx290);
> > > > +       unsigned int min = UINT_MAX;
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < imx290_modes_num(imx290); i++) {
> > > > +               unsigned int tmp;
> > > > +
> > > > +               if (v)
> > > > +                       tmp = modes[i].hmax_min - modes[i].width;
> > >
> > > if (v)
> > >    return h
> > >
> > > With the complete series my sensor comes up with controls defined as
> > > vertical_blanking 0x009e0901 (int)    : min=280 max=261423 step=1
> > > default=280 value=280
> > > horizontal_blanking 0x009e0902 (int)    : min=30 max=64255 step=1
> > > default=30 value=30
> > >
> > > Set the mode to 1080p and I get
> > > vertical_blanking 0x009e0901 (int)    : min=45 max=261063 step=1
> > > default=45 value=1169
> > > horizontal_blanking 0x009e0902 (int)    : min=280 max=63615 step=1
> > > default=280 value=280
> >
> > The idea here is to have VBLANK=30 available in the initial "after
> > probe" state of the sensor. VBLANK=30 is a valid value for 720p mode,
> > but it cannot be set after probe, because the driver (not the user)
> > decided that 1080 mode is active. The idea is to relax the ranges while
> > the mode is not set. Once the mode is known, the values are tightened
> > to the real mode-dependent values.
> >
> > Kind regards
> > Benjamin
> >
> > >   Dave
> > >
> > > > +               else
> > > > +                       tmp = modes[i].vmax_min - modes[i].height;
> > > > +
> > > > +               if (tmp < min)
> > > > +                       min = tmp;
> > > > +       }
> > > > +
> > > > +       return min;
> > > > +}
> > > > +
> > > >  static void imx290_ctrl_update(struct imx290 *imx290,
> > > >                                const struct imx290_mode *mode)
> > > >  {
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v3 2/7] media: i2c: imx290: Define absolute control ranges

[Benjamin Bara]

Hi Dave!

On Mon, 2 Sept 2024 at 20:00, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
> On Mon, 2 Sept 2024 at 16:58, <bbara93@gmail.com> wrote:
> >
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > For now, the driver activates the first mode (1080p) as current active
> > mode in probe(). This e.g. means that one cannot set VBLANK below 45
> > (vmax_min - height), although theoretically the minimum is 30 (720p
> > mode). Define the absolute possible/supported ranges to have them
> > available later.
>
> Currently the driver will set the ranges for VBLANK and HBLANK
> whenever the mode changes.
>
> How is it helpful to fake these numbers? Seeing as they aren't
> reflecting anything useful, they may as well all be 0.
>
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > Changes since v2:
> > - new
> > ---
> >  drivers/media/i2c/imx290.c | 36 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 1c97f9650eb4..466492bab600 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -499,6 +499,10 @@ static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
> >  };
> >
> >  /* Mode configs */
> > +#define WIDTH_720P     1280
> > +#define HEIGHT_720P    720
> > +#define MINIMUM_WIDTH  WIDTH_720P
> > +#define MINIMUM_HEIGHT HEIGHT_720P
> >  static const struct imx290_mode imx290_modes_2lanes[] = {
> >         {
> >                 .width = 1920,
> > @@ -512,8 +516,8 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> >                 .clk_cfg = imx290_1080p_clock_config,
> >         },
> >         {
> > -               .width = 1280,
> > -               .height = 720,
> > +               .width = WIDTH_720P,
> > +               .height = HEIGHT_720P,
> >                 .hmax_min = 3300,
> >                 .vmax_min = 750,
> >                 .link_freq_index = FREQ_INDEX_720P,
> > @@ -537,8 +541,8 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> >                 .clk_cfg = imx290_1080p_clock_config,
> >         },
> >         {
> > -               .width = 1280,
> > -               .height = 720,
> > +               .width = WIDTH_720P,
> > +               .height = HEIGHT_720P,
> >                 .hmax_min = 3300,
> >                 .vmax_min = 750,
> >                 .link_freq_index = FREQ_INDEX_720P,
> > @@ -846,6 +850,30 @@ static const char * const imx290_test_pattern_menu[] = {
> >         "000/555h Toggle Pattern",
> >  };
> >
> > +/* absolute supported control ranges */
> > +#define HBLANK_MAX     (IMX290_HMAX_MAX - MINIMUM_WIDTH)
> > +#define VBLANK_MAX     (IMX290_VMAX_MAX - MINIMUM_HEIGHT)
> > +static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > +{
>
> This function is never used in this patch. I'm surprised the compiler
> doesn't throw an error on a static function not being used.
> You first use it in patch 4 "Introduce initial "off" mode & link freq"

I think I couldn't really transport my intention behind this function
and going to try to explain my thought process a little bit:

My basic understanding of sensors and blanking times is that the sensors
cannot "just forward image data" but also need some time for internal
stuff. My expectation would be that this time should be the same for two
different modes (720p and 1080p) of the same type ("all pixel readout").
I would even expect that in this type of mode, most of the "internal
time" is used for transferring data, therefore I would also expect that
the minimum blanking times for 720p to be lower (or at least equal) than
1080p. This would make my intention quite easy: Just activate the lowest
resolution during probe, because it has the widest allowed blanking time
ranges, and then we can "freely configure", independent of the mode
later used.

However, this is not the case here: The minimum VBLANK of the 1080p mode
is lower than the minimum VBLANK of the 720p mode. Together with
HBLANK_min_720p < HBLANK_min_1080p, we need a "virtual, invalid" mode to
support both minimas.

What would make things simpler here would be allowing us to control the
total times instead of the blanking times. In this case, we can directly
use the minimum defined {H,V}MAX of the mode and default to the mode
with the lowest possible resolution instead. This would enable a freely
configurable frame duration with full supported ranges and valid modes.

Kind regards
Benjamin

> > +       const struct imx290_mode *modes = imx290_modes_ptr(imx290);
> > +       unsigned int min = UINT_MAX;
> > +       int i;
> > +
> > +       for (i = 0; i < imx290_modes_num(imx290); i++) {
> > +               unsigned int tmp;
> > +
> > +               if (v)
> > +                       tmp = modes[i].hmax_min - modes[i].width;
>
> if (v)
>    return h
>
> With the complete series my sensor comes up with controls defined as
> vertical_blanking 0x009e0901 (int)    : min=280 max=261423 step=1
> default=280 value=280
> horizontal_blanking 0x009e0902 (int)    : min=30 max=64255 step=1
> default=30 value=30
>
> Set the mode to 1080p and I get
> vertical_blanking 0x009e0901 (int)    : min=45 max=261063 step=1
> default=45 value=1169
> horizontal_blanking 0x009e0902 (int)    : min=280 max=63615 step=1
> default=280 value=280
>
>   Dave
>
> > +               else
> > +                       tmp = modes[i].vmax_min - modes[i].height;
> > +
> > +               if (tmp < min)
> > +                       min = tmp;
> > +       }
> > +
> > +       return min;
> > +}
> > +
> >  static void imx290_ctrl_update(struct imx290 *imx290,
> >                                const struct imx290_mode *mode)
> >  {
> >
> > --
> > 2.46.0
> >
> >

[PATCH v3 3/7] media: i2c: imx290: Remove CHIP_ID reg definition

[bbara93]

From: Benjamin Bara <benjamin.bara@skidata.com>

This register is not described in the public available imx290 datasheet.
Additionally, a read returns '0x07d0' for an imx327lqr and also for an
imx462, which means it cannot be used to distinguish between those two
imx290 derivatives.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
Changes since v2:
- picked up R-b of Laurent (thx!)
---
 drivers/media/i2c/imx290.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 466492bab600..6812e7cb9e23 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -80,7 +80,6 @@
 #define IMX290_ADBIT2					CCI_REG8(0x317c)
 #define IMX290_ADBIT2_10BIT				0x12
 #define IMX290_ADBIT2_12BIT				0x00
-#define IMX290_CHIP_ID					CCI_REG16_LE(0x319a)
 #define IMX290_ADBIT3					CCI_REG8(0x31ec)
 #define IMX290_ADBIT3_10BIT				0x37
 #define IMX290_ADBIT3_12BIT				0x0e

-- 
2.46.0

[PATCH v3 4/7] media: i2c: imx290: Introduce initial "off" mode & link freq

[bbara93]

From: Benjamin Bara <benjamin.bara@skidata.com>

To be compliant to the V4L2 API, the driver currently "randomly" decides
on one of the two supported modes which also implies a link frequency.

Add a new mode and frequency which symbolize that the sensor is not in
use. This can be used as a default value during probe() and enables us
to avoid communication with the sensor.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
Changes since v2:
- new
---
 drivers/media/i2c/imx290.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 6812e7cb9e23..ece4d66001f5 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -425,14 +425,17 @@ static const struct imx290_csi_cfg imx290_csi_297mhz = {
 /* supported link frequencies */
 #define FREQ_INDEX_1080P	0
 #define FREQ_INDEX_720P		1
+#define FREQ_INDEX_OFF		2
 static const s64 imx290_link_freq_2lanes[] = {
 	[FREQ_INDEX_1080P] = 445500000,
 	[FREQ_INDEX_720P] = 297000000,
+	[FREQ_INDEX_OFF] = 0,
 };
 
 static const s64 imx290_link_freq_4lanes[] = {
 	[FREQ_INDEX_1080P] = 222750000,
 	[FREQ_INDEX_720P] = 148500000,
+	[FREQ_INDEX_OFF] = 0,
 };
 
 /*
@@ -552,6 +555,10 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 	},
 };
 
+static const struct imx290_mode imx290_mode_off = {
+	.link_freq_index = FREQ_INDEX_OFF,
+};
+
 static inline const struct imx290_mode *imx290_modes_ptr(const struct imx290 *imx290)
 {
 	if (imx290->nlanes == 2)
@@ -876,10 +883,19 @@ static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
 static void imx290_ctrl_update(struct imx290 *imx290,
 			       const struct imx290_mode *mode)
 {
-	unsigned int hblank_min = mode->hmax_min - mode->width;
-	unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
-	unsigned int vblank_min = mode->vmax_min - mode->height;
-	unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
+	unsigned int hblank_min, hblank_max, vblank_min, vblank_max;
+
+	if (mode == &imx290_mode_off) {
+		hblank_min = imx290_get_blank_min(imx290, false);
+		hblank_max = HBLANK_MAX;
+		vblank_min = imx290_get_blank_min(imx290, true);
+		vblank_max = VBLANK_MAX;
+	} else {
+		hblank_min = mode->hmax_min - mode->width;
+		hblank_max = IMX290_HMAX_MAX - mode->width;
+		vblank_min = mode->vmax_min - mode->height;
+		vblank_max = IMX290_VMAX_MAX - mode->height;
+	}
 
 	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
 
@@ -932,7 +948,8 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 	imx290->link_freq =
 		v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
 				       V4L2_CID_LINK_FREQ,
-				       imx290_link_freqs_num(imx290) - 1, 0,
+				       imx290_link_freqs_num(imx290) - 1,
+				       FREQ_INDEX_OFF,
 				       imx290_link_freqs_ptr(imx290));
 	if (imx290->link_freq)
 		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
@@ -1278,7 +1295,7 @@ static int imx290_subdev_init(struct imx290 *imx290)
 	struct v4l2_subdev_state *state;
 	int ret;
 
-	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
+	imx290->current_mode = &imx290_mode_off;
 
 	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
 	imx290->sd.internal_ops = &imx290_internal_ops;

-- 
2.46.0

Re: [PATCH v3 4/7] media: i2c: imx290: Introduce initial "off" mode & link freq

[Laurent Pinchart]

Hi Benjamin,

Thank you for the patch.

On Mon, Sep 02, 2024 at 05:57:29PM +0200, bbara93@gmail.com wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> To be compliant to the V4L2 API, the driver currently "randomly" decides
> on one of the two supported modes which also implies a link frequency.
> 
> Add a new mode and frequency which symbolize that the sensor is not in
> use. This can be used as a default value during probe() and enables us
> to avoid communication with the sensor.

I really doin't like this change. I would like to instead move away from
modes and make the driver freely configurable. Furthermore, the concept
of an initial unconfigured state isn't valid in V4L2. The driver must
fully initialize the whole device state at probe time.

> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> Changes since v2:
> - new
> ---
>  drivers/media/i2c/imx290.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 6812e7cb9e23..ece4d66001f5 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -425,14 +425,17 @@ static const struct imx290_csi_cfg imx290_csi_297mhz = {
>  /* supported link frequencies */
>  #define FREQ_INDEX_1080P	0
>  #define FREQ_INDEX_720P		1
> +#define FREQ_INDEX_OFF		2
>  static const s64 imx290_link_freq_2lanes[] = {
>  	[FREQ_INDEX_1080P] = 445500000,
>  	[FREQ_INDEX_720P] = 297000000,
> +	[FREQ_INDEX_OFF] = 0,
>  };
>  
>  static const s64 imx290_link_freq_4lanes[] = {
>  	[FREQ_INDEX_1080P] = 222750000,
>  	[FREQ_INDEX_720P] = 148500000,
> +	[FREQ_INDEX_OFF] = 0,
>  };
>  
>  /*
> @@ -552,6 +555,10 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  	},
>  };
>  
> +static const struct imx290_mode imx290_mode_off = {
> +	.link_freq_index = FREQ_INDEX_OFF,
> +};
> +
>  static inline const struct imx290_mode *imx290_modes_ptr(const struct imx290 *imx290)
>  {
>  	if (imx290->nlanes == 2)
> @@ -876,10 +883,19 @@ static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
>  static void imx290_ctrl_update(struct imx290 *imx290,
>  			       const struct imx290_mode *mode)
>  {
> -	unsigned int hblank_min = mode->hmax_min - mode->width;
> -	unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> -	unsigned int vblank_min = mode->vmax_min - mode->height;
> -	unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
> +	unsigned int hblank_min, hblank_max, vblank_min, vblank_max;
> +
> +	if (mode == &imx290_mode_off) {
> +		hblank_min = imx290_get_blank_min(imx290, false);
> +		hblank_max = HBLANK_MAX;
> +		vblank_min = imx290_get_blank_min(imx290, true);
> +		vblank_max = VBLANK_MAX;
> +	} else {
> +		hblank_min = mode->hmax_min - mode->width;
> +		hblank_max = IMX290_HMAX_MAX - mode->width;
> +		vblank_min = mode->vmax_min - mode->height;
> +		vblank_max = IMX290_VMAX_MAX - mode->height;
> +	}
>  
>  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
>  
> @@ -932,7 +948,8 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  	imx290->link_freq =
>  		v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
>  				       V4L2_CID_LINK_FREQ,
> -				       imx290_link_freqs_num(imx290) - 1, 0,
> +				       imx290_link_freqs_num(imx290) - 1,
> +				       FREQ_INDEX_OFF,
>  				       imx290_link_freqs_ptr(imx290));
>  	if (imx290->link_freq)
>  		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> @@ -1278,7 +1295,7 @@ static int imx290_subdev_init(struct imx290 *imx290)
>  	struct v4l2_subdev_state *state;
>  	int ret;
>  
> -	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> +	imx290->current_mode = &imx290_mode_off;
>  
>  	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
>  	imx290->sd.internal_ops = &imx290_internal_ops;
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 4/7] media: i2c: imx290: Introduce initial "off" mode & link freq

[Benjamin Bara]

Hi Laurent!

On Mon, 2 Sept 2024 at 21:58, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Sep 02, 2024 at 05:57:29PM +0200, bbara93@gmail.com wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > To be compliant to the V4L2 API, the driver currently "randomly" decides
> > on one of the two supported modes which also implies a link frequency.
> >
> > Add a new mode and frequency which symbolize that the sensor is not in
> > use. This can be used as a default value during probe() and enables us
> > to avoid communication with the sensor.
>
> I really doin't like this change. I would like to instead move away from
> modes and make the driver freely configurable.

Which controls do you want to have freely configurable? At least on the
imx290 the exposure limits depend on the blanking, and the blanking
limits depend on the format. As the format is defined by the mode on
imx290, I think this will be quite hard with the current controls.

> Furthermore, the concept of an initial unconfigured state isn't valid
> in V4L2. The driver must fully initialize the whole device state at
> probe time.

I understand that and it makes sense to me. But given the dependencies
from above and the fact that the format is currently part of this
"state", it makes the "freely configurable" intention even harder :-(

Kind regards
Benjamin

> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > Changes since v2:
> > - new
> > ---
> >  drivers/media/i2c/imx290.c | 29 +++++++++++++++++++++++------
> >  1 file changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 6812e7cb9e23..ece4d66001f5 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -425,14 +425,17 @@ static const struct imx290_csi_cfg imx290_csi_297mhz = {
> >  /* supported link frequencies */
> >  #define FREQ_INDEX_1080P     0
> >  #define FREQ_INDEX_720P              1
> > +#define FREQ_INDEX_OFF               2
> >  static const s64 imx290_link_freq_2lanes[] = {
> >       [FREQ_INDEX_1080P] = 445500000,
> >       [FREQ_INDEX_720P] = 297000000,
> > +     [FREQ_INDEX_OFF] = 0,
> >  };
> >
> >  static const s64 imx290_link_freq_4lanes[] = {
> >       [FREQ_INDEX_1080P] = 222750000,
> >       [FREQ_INDEX_720P] = 148500000,
> > +     [FREQ_INDEX_OFF] = 0,
> >  };
> >
> >  /*
> > @@ -552,6 +555,10 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> >       },
> >  };
> >
> > +static const struct imx290_mode imx290_mode_off = {
> > +     .link_freq_index = FREQ_INDEX_OFF,
> > +};
> > +
> >  static inline const struct imx290_mode *imx290_modes_ptr(const struct imx290 *imx290)
> >  {
> >       if (imx290->nlanes == 2)
> > @@ -876,10 +883,19 @@ static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> >  static void imx290_ctrl_update(struct imx290 *imx290,
> >                              const struct imx290_mode *mode)
> >  {
> > -     unsigned int hblank_min = mode->hmax_min - mode->width;
> > -     unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> > -     unsigned int vblank_min = mode->vmax_min - mode->height;
> > -     unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
> > +     unsigned int hblank_min, hblank_max, vblank_min, vblank_max;
> > +
> > +     if (mode == &imx290_mode_off) {
> > +             hblank_min = imx290_get_blank_min(imx290, false);
> > +             hblank_max = HBLANK_MAX;
> > +             vblank_min = imx290_get_blank_min(imx290, true);
> > +             vblank_max = VBLANK_MAX;
> > +     } else {
> > +             hblank_min = mode->hmax_min - mode->width;
> > +             hblank_max = IMX290_HMAX_MAX - mode->width;
> > +             vblank_min = mode->vmax_min - mode->height;
> > +             vblank_max = IMX290_VMAX_MAX - mode->height;
> > +     }
> >
> >       __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> >
> > @@ -932,7 +948,8 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> >       imx290->link_freq =
> >               v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
> >                                      V4L2_CID_LINK_FREQ,
> > -                                    imx290_link_freqs_num(imx290) - 1, 0,
> > +                                    imx290_link_freqs_num(imx290) - 1,
> > +                                    FREQ_INDEX_OFF,
> >                                      imx290_link_freqs_ptr(imx290));
> >       if (imx290->link_freq)
> >               imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > @@ -1278,7 +1295,7 @@ static int imx290_subdev_init(struct imx290 *imx290)
> >       struct v4l2_subdev_state *state;
> >       int ret;
> >
> > -     imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> > +     imx290->current_mode = &imx290_mode_off;
> >
> >       v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> >       imx290->sd.internal_ops = &imx290_internal_ops;
> >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v3 4/7] media: i2c: imx290: Introduce initial "off" mode & link freq

[Laurent Pinchart]

On Mon, Sep 02, 2024 at 10:55:04PM +0200, Benjamin Bara wrote:
> On Mon, 2 Sept 2024 at 21:58, Laurent Pinchart wrote:
> > On Mon, Sep 02, 2024 at 05:57:29PM +0200, bbara93@gmail.com wrote:
> > > From: Benjamin Bara <benjamin.bara@skidata.com>
> > >
> > > To be compliant to the V4L2 API, the driver currently "randomly" decides
> > > on one of the two supported modes which also implies a link frequency.
> > >
> > > Add a new mode and frequency which symbolize that the sensor is not in
> > > use. This can be used as a default value during probe() and enables us
> > > to avoid communication with the sensor.
> >
> > I really doin't like this change. I would like to instead move away from
> > modes and make the driver freely configurable.
> 
> Which controls do you want to have freely configurable? At least on the
> imx290 the exposure limits depend on the blanking, and the blanking
> limits depend on the format. As the format is defined by the mode on
> imx290, I think this will be quite hard with the current controls.

I want to make the format freely configurable.

> > Furthermore, the concept of an initial unconfigured state isn't valid
> > in V4L2. The driver must fully initialize the whole device state at
> > probe time.
> 
> I understand that and it makes sense to me. But given the dependencies
> from above and the fact that the format is currently part of this
> "state", it makes the "freely configurable" intention even harder :-(

Why can't we simply initialize the controls with limits that correspond
to the default format ? I don't understand what issue this is trying to
solve.

> > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > ---
> > > Changes since v2:
> > > - new
> > > ---
> > >  drivers/media/i2c/imx290.c | 29 +++++++++++++++++++++++------
> > >  1 file changed, 23 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 6812e7cb9e23..ece4d66001f5 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -425,14 +425,17 @@ static const struct imx290_csi_cfg imx290_csi_297mhz = {
> > >  /* supported link frequencies */
> > >  #define FREQ_INDEX_1080P     0
> > >  #define FREQ_INDEX_720P              1
> > > +#define FREQ_INDEX_OFF               2
> > >  static const s64 imx290_link_freq_2lanes[] = {
> > >       [FREQ_INDEX_1080P] = 445500000,
> > >       [FREQ_INDEX_720P] = 297000000,
> > > +     [FREQ_INDEX_OFF] = 0,
> > >  };
> > >
> > >  static const s64 imx290_link_freq_4lanes[] = {
> > >       [FREQ_INDEX_1080P] = 222750000,
> > >       [FREQ_INDEX_720P] = 148500000,
> > > +     [FREQ_INDEX_OFF] = 0,
> > >  };
> > >
> > >  /*
> > > @@ -552,6 +555,10 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > >       },
> > >  };
> > >
> > > +static const struct imx290_mode imx290_mode_off = {
> > > +     .link_freq_index = FREQ_INDEX_OFF,
> > > +};
> > > +
> > >  static inline const struct imx290_mode *imx290_modes_ptr(const struct imx290 *imx290)
> > >  {
> > >       if (imx290->nlanes == 2)
> > > @@ -876,10 +883,19 @@ static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > >  static void imx290_ctrl_update(struct imx290 *imx290,
> > >                              const struct imx290_mode *mode)
> > >  {
> > > -     unsigned int hblank_min = mode->hmax_min - mode->width;
> > > -     unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> > > -     unsigned int vblank_min = mode->vmax_min - mode->height;
> > > -     unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
> > > +     unsigned int hblank_min, hblank_max, vblank_min, vblank_max;
> > > +
> > > +     if (mode == &imx290_mode_off) {
> > > +             hblank_min = imx290_get_blank_min(imx290, false);
> > > +             hblank_max = HBLANK_MAX;
> > > +             vblank_min = imx290_get_blank_min(imx290, true);
> > > +             vblank_max = VBLANK_MAX;
> > > +     } else {
> > > +             hblank_min = mode->hmax_min - mode->width;
> > > +             hblank_max = IMX290_HMAX_MAX - mode->width;
> > > +             vblank_min = mode->vmax_min - mode->height;
> > > +             vblank_max = IMX290_VMAX_MAX - mode->height;
> > > +     }
> > >
> > >       __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> > >
> > > @@ -932,7 +948,8 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > >       imx290->link_freq =
> > >               v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
> > >                                      V4L2_CID_LINK_FREQ,
> > > -                                    imx290_link_freqs_num(imx290) - 1, 0,
> > > +                                    imx290_link_freqs_num(imx290) - 1,
> > > +                                    FREQ_INDEX_OFF,
> > >                                      imx290_link_freqs_ptr(imx290));
> > >       if (imx290->link_freq)
> > >               imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > @@ -1278,7 +1295,7 @@ static int imx290_subdev_init(struct imx290 *imx290)
> > >       struct v4l2_subdev_state *state;
> > >       int ret;
> > >
> > > -     imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> > > +     imx290->current_mode = &imx290_mode_off;
> > >
> > >       v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> > >       imx290->sd.internal_ops = &imx290_internal_ops;
> > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 4/7] media: i2c: imx290: Introduce initial "off" mode & link freq

[Dave Stevenson]

On Tue, 3 Sept 2024 at 14:01, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Sep 02, 2024 at 10:55:04PM +0200, Benjamin Bara wrote:
> > On Mon, 2 Sept 2024 at 21:58, Laurent Pinchart wrote:
> > > On Mon, Sep 02, 2024 at 05:57:29PM +0200, bbara93@gmail.com wrote:
> > > > From: Benjamin Bara <benjamin.bara@skidata.com>
> > > >
> > > > To be compliant to the V4L2 API, the driver currently "randomly" decides
> > > > on one of the two supported modes which also implies a link frequency.
> > > >
> > > > Add a new mode and frequency which symbolize that the sensor is not in
> > > > use. This can be used as a default value during probe() and enables us
> > > > to avoid communication with the sensor.
> > >
> > > I really doin't like this change. I would like to instead move away from
> > > modes and make the driver freely configurable.
> >
> > Which controls do you want to have freely configurable? At least on the
> > imx290 the exposure limits depend on the blanking, and the blanking
> > limits depend on the format. As the format is defined by the mode on
> > imx290, I think this will be quite hard with the current controls.
>
> I want to make the format freely configurable.

Isn't this partly limited by the discussion led by Jacopo in Dublin
back in 2022 [1]?

How do we shift from the current list of 2 modes, to a single defined
mode that the selection API can then crop?
Changing the list of enumerated modes is likely to break existing
users of this driver, and AIUI it isn't currently permitted for
selecting a new enumerated mode to update the selection rectangles.

It also needs someone to sit down and fully digest the window cropping
mode section of the datasheet, and hope that the sensor actually
behaves as documented. I know I don't have the time to do that at
present.

[1] https://www.spinics.net/lists/linux-media/msg218231.html

> > > Furthermore, the concept of an initial unconfigured state isn't valid
> > > in V4L2. The driver must fully initialize the whole device state at
> > > probe time.
> >
> > I understand that and it makes sense to me. But given the dependencies
> > from above and the fact that the format is currently part of this
> > "state", it makes the "freely configurable" intention even harder :-(
>
> Why can't we simply initialize the controls with limits that correspond
> to the default format ? I don't understand what issue this is trying to
> solve.

If I'm following the full discussion correctly now, it's trying to
avoid those couple of register writes during probe due to updating
VBLANK and HBLANK control ranges. Adding all this "dummy mode" code
seems overkill to achieve that.

I see 3 simpler approaches:
- Move the pm_runtime calls so that the range updates are when the
sensor is powered down. Ack on that needing very careful handling of
what is initialised when.
- Move the lines
    state = v4l2_subdev_lock_and_get_active_state(&imx290->sd);
    imx290_ctrl_update(imx290, imx290->current_mode);
    v4l2_subdev_unlock_state(state);
out of imx290_subdev_init to the end of imx290_probe, after the pm_runtime_put
- Add a flag to the state (in addition to the pm_runtime handling) to
denote that we're between stream_on and stream_off, and therefore
imx290_set_ctrl should update registers.

A quick test of the 2nd option (with patches 1, 3, and 6) appears to
achieve that aim - I only see the one read from patch 6.
IMHO trying to address the case where runtime PM is disabled is fairly
redundant, but others may disagree. Option 3 would achieve that with
minimal extra overhead though.

  Dave

> > > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > > ---
> > > > Changes since v2:
> > > > - new
> > > > ---
> > > >  drivers/media/i2c/imx290.c | 29 +++++++++++++++++++++++------
> > > >  1 file changed, 23 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index 6812e7cb9e23..ece4d66001f5 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -425,14 +425,17 @@ static const struct imx290_csi_cfg imx290_csi_297mhz = {
> > > >  /* supported link frequencies */
> > > >  #define FREQ_INDEX_1080P     0
> > > >  #define FREQ_INDEX_720P              1
> > > > +#define FREQ_INDEX_OFF               2
> > > >  static const s64 imx290_link_freq_2lanes[] = {
> > > >       [FREQ_INDEX_1080P] = 445500000,
> > > >       [FREQ_INDEX_720P] = 297000000,
> > > > +     [FREQ_INDEX_OFF] = 0,
> > > >  };
> > > >
> > > >  static const s64 imx290_link_freq_4lanes[] = {
> > > >       [FREQ_INDEX_1080P] = 222750000,
> > > >       [FREQ_INDEX_720P] = 148500000,
> > > > +     [FREQ_INDEX_OFF] = 0,
> > > >  };
> > > >
> > > >  /*
> > > > @@ -552,6 +555,10 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > > >       },
> > > >  };
> > > >
> > > > +static const struct imx290_mode imx290_mode_off = {
> > > > +     .link_freq_index = FREQ_INDEX_OFF,
> > > > +};
> > > > +
> > > >  static inline const struct imx290_mode *imx290_modes_ptr(const struct imx290 *imx290)
> > > >  {
> > > >       if (imx290->nlanes == 2)
> > > > @@ -876,10 +883,19 @@ static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > > >  static void imx290_ctrl_update(struct imx290 *imx290,
> > > >                              const struct imx290_mode *mode)
> > > >  {
> > > > -     unsigned int hblank_min = mode->hmax_min - mode->width;
> > > > -     unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> > > > -     unsigned int vblank_min = mode->vmax_min - mode->height;
> > > > -     unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
> > > > +     unsigned int hblank_min, hblank_max, vblank_min, vblank_max;
> > > > +
> > > > +     if (mode == &imx290_mode_off) {
> > > > +             hblank_min = imx290_get_blank_min(imx290, false);
> > > > +             hblank_max = HBLANK_MAX;
> > > > +             vblank_min = imx290_get_blank_min(imx290, true);
> > > > +             vblank_max = VBLANK_MAX;
> > > > +     } else {
> > > > +             hblank_min = mode->hmax_min - mode->width;
> > > > +             hblank_max = IMX290_HMAX_MAX - mode->width;
> > > > +             vblank_min = mode->vmax_min - mode->height;
> > > > +             vblank_max = IMX290_VMAX_MAX - mode->height;
> > > > +     }
> > > >
> > > >       __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> > > >
> > > > @@ -932,7 +948,8 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > > >       imx290->link_freq =
> > > >               v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
> > > >                                      V4L2_CID_LINK_FREQ,
> > > > -                                    imx290_link_freqs_num(imx290) - 1, 0,
> > > > +                                    imx290_link_freqs_num(imx290) - 1,
> > > > +                                    FREQ_INDEX_OFF,
> > > >                                      imx290_link_freqs_ptr(imx290));
> > > >       if (imx290->link_freq)
> > > >               imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > @@ -1278,7 +1295,7 @@ static int imx290_subdev_init(struct imx290 *imx290)
> > > >       struct v4l2_subdev_state *state;
> > > >       int ret;
> > > >
> > > > -     imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> > > > +     imx290->current_mode = &imx290_mode_off;
> > > >
> > > >       v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> > > >       imx290->sd.internal_ops = &imx290_internal_ops;
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
>

[PATCH v3 5/7] media: i2c: imx290: Avoid communication during probe()

[bbara93]

From: Benjamin Bara <benjamin.bara@skidata.com>

As we don't know the mode during probe(), it doesn't make sense to
update the sensors' registers with assumptions. Avoid the communication
in this case.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
Changes since v2:
- new
---
 drivers/media/i2c/imx290.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index ece4d66001f5..9610e9fd2059 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -769,6 +769,10 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 	if (!pm_runtime_get_if_in_use(imx290->dev))
 		return 0;
 
+	/* V4L2 controls values will be applied only when mode is known */
+	if (imx290->current_mode == &imx290_mode_off)
+		return 0;
+
 	state = v4l2_subdev_get_locked_active_state(&imx290->sd);
 	format = v4l2_subdev_state_get_format(state, 0);
 

-- 
2.46.0

Re: [PATCH v3 5/7] media: i2c: imx290: Avoid communication during probe()

[Laurent Pinchart]

On Mon, Sep 02, 2024 at 05:57:30PM +0200, bbara93@gmail.com wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> As we don't know the mode during probe(), it doesn't make sense to
> update the sensors' registers with assumptions. Avoid the communication
> in this case.

That doesn't seem right. I think you can fix the problem by
moving initialization of the controls at probe time after the device
gets runtime-suspended. Please try it, and if it doesn't work, let's
figure out why.

> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> Changes since v2:
> - new
> ---
>  drivers/media/i2c/imx290.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index ece4d66001f5..9610e9fd2059 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -769,6 +769,10 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  	if (!pm_runtime_get_if_in_use(imx290->dev))
>  		return 0;
>  
> +	/* V4L2 controls values will be applied only when mode is known */
> +	if (imx290->current_mode == &imx290_mode_off)
> +		return 0;
> +
>  	state = v4l2_subdev_get_locked_active_state(&imx290->sd);
>  	format = v4l2_subdev_state_get_format(state, 0);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 5/7] media: i2c: imx290: Avoid communication during probe()

[Benjamin Bara]

Hi Laurent!

On Mon, 2 Sept 2024 at 22:00, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Sep 02, 2024 at 05:57:30PM +0200, bbara93@gmail.com wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > As we don't know the mode during probe(), it doesn't make sense to
> > update the sensors' registers with assumptions. Avoid the communication
> > in this case.
>
> That doesn't seem right. I think you can fix the problem by
> moving initialization of the controls at probe time after the device
> gets runtime-suspended. Please try it, and if it doesn't work, let's
> figure out why.

Will do for the next iteration.

Thanks & kind regards
Benjamin

> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > Changes since v2:
> > - new
> > ---
> >  drivers/media/i2c/imx290.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index ece4d66001f5..9610e9fd2059 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -769,6 +769,10 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> >       if (!pm_runtime_get_if_in_use(imx290->dev))
> >               return 0;
> >
> > +     /* V4L2 controls values will be applied only when mode is known */
> > +     if (imx290->current_mode == &imx290_mode_off)
> > +             return 0;
> > +
> >       state = v4l2_subdev_get_locked_active_state(&imx290->sd);
> >       format = v4l2_subdev_state_get_format(state, 0);
> >
>
> --
> Regards,
>
> Laurent Pinchart

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

[bbara93]

From: Benjamin Bara <benjamin.bara@skidata.com>

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

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

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

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

-- 
2.46.0

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

[Dave Stevenson]

Hi Benjamin

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

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

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

  Dave

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

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

[Laurent Pinchart]

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

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

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

-- 
Regards,

Laurent Pinchart

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

[Benjamin Bara]

Hi Dave!

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

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

Kind regards
Benjamin

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

[PATCH v3 7/7] media: i2c: imx290: Implement a "privacy mode" for probe()

[bbara93]

From: Benjamin Bara <benjamin.bara@skidata.com>

Currently, we have a trade-off between potentially enabling the privacy
LED and reading out the connection state of the sensor during probe().

To have a somewhat defined policy for now, make a decision based on the
power supplies of the sensor. If they are enabled anyways, communicate
with the powered sensor for an availability check. Otherwise, create the
subdevice without knowing whether the sensor is connected or not.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
Changes since v2:
- new
---
 drivers/media/i2c/imx290.c | 82 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 25 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 6b292bbb0856..338b2c5ea547 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1354,6 +1354,17 @@ static void imx290_subdev_cleanup(struct imx290 *imx290)
  * Power management
  */
 
+static bool is_imx290_power_on(struct imx290 *imx)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(imx->supplies); i++)
+		if (!regulator_is_enabled(imx->supplies[i].consumer))
+			return false;
+
+	return true;
+}
+
 static int imx290_power_on(struct imx290 *imx290)
 {
 	int ret;
@@ -1571,6 +1582,7 @@ static int imx290_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct imx290 *imx290;
+	bool power_on;
 	u64 val;
 	int ret;
 
@@ -1611,36 +1623,54 @@ static int imx290_probe(struct i2c_client *client)
 		return ret;
 
 	/*
-	 * Enable power management. The driver supports runtime PM, but needs to
-	 * work when runtime PM is disabled in the kernel. To that end, power
-	 * the sensor on manually here.
+	 * Privacy mode: if the regulators are not enabled, avoid enabling them.
+	 * In case the regulators are enabled, we still want to make sure that
+	 * the regulators know that they have another consumer, therefore run
+	 * the powering sequence.
 	 */
-	ret = imx290_power_on(imx290);
-	if (ret < 0) {
-		dev_err(dev, "Could not power on the device\n");
-		return ret;
+	power_on = is_imx290_power_on(imx290);
+	dev_dbg(dev, "%s: power on: %d\n", __func__, power_on);
+	if (power_on) {
+		/*
+		 * Enable power management. The driver supports runtime PM, but
+		 * needs to work when runtime PM is disabled in the kernel. To
+		 * that end, power the sensor on manually here.
+		 */
+		ret = imx290_power_on(imx290);
+		if (ret < 0) {
+			dev_err(dev, "Could not power on the device\n");
+			return ret;
+		}
+
+		/*
+		 * Enable runtime PM with autosuspend. As the device has been
+		 * powered manually, mark it as active, and increase the usage
+		 * count without resuming the device.
+		 */
+		pm_runtime_set_active(dev);
+		pm_runtime_get_noresume(dev);
 	}
 
-	/*
-	 * Enable runtime PM with autosuspend. As the device has been powered
-	 * manually, mark it as active, and increase the usage count without
-	 * resuming the device.
-	 */
-	pm_runtime_set_active(dev);
-	pm_runtime_get_noresume(dev);
 	pm_runtime_enable(dev);
 	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 
-	/* Make sure the sensor is available before V4L2 subdev init. */
-	ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
-	if (ret) {
-		ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
-		goto err_pm;
-	}
-	if (val != IMX290_STANDBY_STANDBY) {
-		ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
-		goto err_pm;
+	/*
+	 * Make sure the sensor is available before V4L2 subdev init.
+	 * This only works when the sensor is powered.
+	 */
+	if (power_on) {
+		ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
+		if (ret) {
+			ret = dev_err_probe(dev, -ENODEV,
+					    "Failed to detect sensor\n");
+			goto err_pm;
+		}
+		if (val != IMX290_STANDBY_STANDBY) {
+			ret = dev_err_probe(dev, -ENODEV,
+					    "Sensor is not in standby\n");
+			goto err_pm;
+		}
 	}
 
 	/* Initialize the V4L2 subdev. */
@@ -1666,8 +1696,10 @@ static int imx290_probe(struct i2c_client *client)
 	 * Decrease the PM usage count. The device will get suspended after the
 	 * autosuspend delay, turning the power off.
 	 */
-	pm_runtime_mark_last_busy(dev);
-	pm_runtime_put_autosuspend(dev);
+	if (power_on) {
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_autosuspend(dev);
+	}
 
 	return 0;
 

-- 
2.46.0

Re: [PATCH v3 7/7] media: i2c: imx290: Implement a "privacy mode" for probe()

[Dave Stevenson]

Hi Benjamin

On Mon, 2 Sept 2024 at 16:59, <bbara93@gmail.com> wrote:
>
> From: Benjamin Bara <benjamin.bara@skidata.com>
>
> Currently, we have a trade-off between potentially enabling the privacy
> LED and reading out the connection state of the sensor during probe().
>
> To have a somewhat defined policy for now, make a decision based on the
> power supplies of the sensor. If they are enabled anyways, communicate
> with the powered sensor for an availability check. Otherwise, create the
> subdevice without knowing whether the sensor is connected or not.

Almost all the camera modules used on Raspberry Pi have regulators
controlled via a GPIO, but no privacy LED. The preference from us is
very definitely to query the sensor during probe where possible to
flag up any connectivity issues, and indeed I've had a number of
support threads with imx290 where it's just not been connected but it
probed fully and showed up in libcamera.

How can I opt in to patch 6 checking basic I2C to the sensor during
probe when I have a controllable regulator? (This is where the
discussions over a dtbinding for privacy LEDs and not powering up
sensors during probe comes in).

Thanks
  Dave

> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> Changes since v2:
> - new
> ---
>  drivers/media/i2c/imx290.c | 82 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 57 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 6b292bbb0856..338b2c5ea547 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -1354,6 +1354,17 @@ static void imx290_subdev_cleanup(struct imx290 *imx290)
>   * Power management
>   */
>
> +static bool is_imx290_power_on(struct imx290 *imx)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(imx->supplies); i++)
> +               if (!regulator_is_enabled(imx->supplies[i].consumer))
> +                       return false;
> +
> +       return true;
> +}
> +
>  static int imx290_power_on(struct imx290 *imx290)
>  {
>         int ret;
> @@ -1571,6 +1582,7 @@ static int imx290_probe(struct i2c_client *client)
>  {
>         struct device *dev = &client->dev;
>         struct imx290 *imx290;
> +       bool power_on;
>         u64 val;
>         int ret;
>
> @@ -1611,36 +1623,54 @@ static int imx290_probe(struct i2c_client *client)
>                 return ret;
>
>         /*
> -        * Enable power management. The driver supports runtime PM, but needs to
> -        * work when runtime PM is disabled in the kernel. To that end, power
> -        * the sensor on manually here.
> +        * Privacy mode: if the regulators are not enabled, avoid enabling them.
> +        * In case the regulators are enabled, we still want to make sure that
> +        * the regulators know that they have another consumer, therefore run
> +        * the powering sequence.
>          */
> -       ret = imx290_power_on(imx290);
> -       if (ret < 0) {
> -               dev_err(dev, "Could not power on the device\n");
> -               return ret;
> +       power_on = is_imx290_power_on(imx290);
> +       dev_dbg(dev, "%s: power on: %d\n", __func__, power_on);
> +       if (power_on) {
> +               /*
> +                * Enable power management. The driver supports runtime PM, but
> +                * needs to work when runtime PM is disabled in the kernel. To
> +                * that end, power the sensor on manually here.
> +                */
> +               ret = imx290_power_on(imx290);
> +               if (ret < 0) {
> +                       dev_err(dev, "Could not power on the device\n");
> +                       return ret;
> +               }
> +
> +               /*
> +                * Enable runtime PM with autosuspend. As the device has been
> +                * powered manually, mark it as active, and increase the usage
> +                * count without resuming the device.
> +                */
> +               pm_runtime_set_active(dev);
> +               pm_runtime_get_noresume(dev);
>         }
>
> -       /*
> -        * Enable runtime PM with autosuspend. As the device has been powered
> -        * manually, mark it as active, and increase the usage count without
> -        * resuming the device.
> -        */
> -       pm_runtime_set_active(dev);
> -       pm_runtime_get_noresume(dev);
>         pm_runtime_enable(dev);
>         pm_runtime_set_autosuspend_delay(dev, 1000);
>         pm_runtime_use_autosuspend(dev);
>
> -       /* Make sure the sensor is available before V4L2 subdev init. */
> -       ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> -       if (ret) {
> -               ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
> -               goto err_pm;
> -       }
> -       if (val != IMX290_STANDBY_STANDBY) {
> -               ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
> -               goto err_pm;
> +       /*
> +        * Make sure the sensor is available before V4L2 subdev init.
> +        * This only works when the sensor is powered.
> +        */
> +       if (power_on) {
> +               ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> +               if (ret) {
> +                       ret = dev_err_probe(dev, -ENODEV,
> +                                           "Failed to detect sensor\n");
> +                       goto err_pm;
> +               }
> +               if (val != IMX290_STANDBY_STANDBY) {
> +                       ret = dev_err_probe(dev, -ENODEV,
> +                                           "Sensor is not in standby\n");
> +                       goto err_pm;
> +               }
>         }
>
>         /* Initialize the V4L2 subdev. */
> @@ -1666,8 +1696,10 @@ static int imx290_probe(struct i2c_client *client)
>          * Decrease the PM usage count. The device will get suspended after the
>          * autosuspend delay, turning the power off.
>          */
> -       pm_runtime_mark_last_busy(dev);
> -       pm_runtime_put_autosuspend(dev);
> +       if (power_on) {
> +               pm_runtime_mark_last_busy(dev);
> +               pm_runtime_put_autosuspend(dev);
> +       }
>
>         return 0;
>
>
> --
> 2.46.0
>
>

Re: [PATCH v3 7/7] media: i2c: imx290: Implement a "privacy mode" for probe()

[Benjamin Bara]

Hi Dave!

On Mon, 2 Sept 2024 at 20:10, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
> On Mon, 2 Sept 2024 at 16:59, <bbara93@gmail.com> wrote:
> >
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > Currently, we have a trade-off between potentially enabling the privacy
> > LED and reading out the connection state of the sensor during probe().
> >
> > To have a somewhat defined policy for now, make a decision based on the
> > power supplies of the sensor. If they are enabled anyways, communicate
> > with the powered sensor for an availability check. Otherwise, create the
> > subdevice without knowing whether the sensor is connected or not.
>
> Almost all the camera modules used on Raspberry Pi have regulators
> controlled via a GPIO, but no privacy LED. The preference from us is
> very definitely to query the sensor during probe where possible to
> flag up any connectivity issues, and indeed I've had a number of
> support threads with imx290 where it's just not been connected but it
> probed fully and showed up in libcamera.
>
> How can I opt in to patch 6 checking basic I2C to the sensor during
> probe when I have a controllable regulator? (This is where the
> discussions over a dtbinding for privacy LEDs and not powering up
> sensors during probe comes in).

When you want to probe only during boot time, you can use the
"regulator-boot-on" DT binding on your controllable regulator. This
enables the regulator while it is probed and disables it later if not
used (in comparison to "always-on"). Should also work for modules.

Unfortunately, I don't have a clean solution (which also autosuspends)
for "any probe time". I think it is not possible to enable a regulator
from user space without having a consuming DT node. A somewhat clean
workaround might be CONFIG_REGULATOR_USERSPACE_CONSUMER, which gives you
the possibility to change the state of a regulator via sysfs (after
creating a DT node). This gives you the possibility to enable it any
time. However, the userspace-consumer driver gets the regulators
exclusive, which means you cannot add the sensor driver as consumer and
therefore cannot use the autosuspend feature of the imx290. Not really
"nice", but probably "feasible" if you have special constraints when you
are allowed to probe (e.g. the temperature as mentioned by Laurent). A
DT binding would be easier for this case.

Kind regards
Benjamin

> Thanks
>   Dave
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > Changes since v2:
> > - new
> > ---
> >  drivers/media/i2c/imx290.c | 82 ++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 57 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 6b292bbb0856..338b2c5ea547 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -1354,6 +1354,17 @@ static void imx290_subdev_cleanup(struct imx290 *imx290)
> >   * Power management
> >   */
> >
> > +static bool is_imx290_power_on(struct imx290 *imx)
> > +{
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(imx->supplies); i++)
> > +               if (!regulator_is_enabled(imx->supplies[i].consumer))
> > +                       return false;
> > +
> > +       return true;
> > +}
> > +
> >  static int imx290_power_on(struct imx290 *imx290)
> >  {
> >         int ret;
> > @@ -1571,6 +1582,7 @@ static int imx290_probe(struct i2c_client *client)
> >  {
> >         struct device *dev = &client->dev;
> >         struct imx290 *imx290;
> > +       bool power_on;
> >         u64 val;
> >         int ret;
> >
> > @@ -1611,36 +1623,54 @@ static int imx290_probe(struct i2c_client *client)
> >                 return ret;
> >
> >         /*
> > -        * Enable power management. The driver supports runtime PM, but needs to
> > -        * work when runtime PM is disabled in the kernel. To that end, power
> > -        * the sensor on manually here.
> > +        * Privacy mode: if the regulators are not enabled, avoid enabling them.
> > +        * In case the regulators are enabled, we still want to make sure that
> > +        * the regulators know that they have another consumer, therefore run
> > +        * the powering sequence.
> >          */
> > -       ret = imx290_power_on(imx290);
> > -       if (ret < 0) {
> > -               dev_err(dev, "Could not power on the device\n");
> > -               return ret;
> > +       power_on = is_imx290_power_on(imx290);
> > +       dev_dbg(dev, "%s: power on: %d\n", __func__, power_on);
> > +       if (power_on) {
> > +               /*
> > +                * Enable power management. The driver supports runtime PM, but
> > +                * needs to work when runtime PM is disabled in the kernel. To
> > +                * that end, power the sensor on manually here.
> > +                */
> > +               ret = imx290_power_on(imx290);
> > +               if (ret < 0) {
> > +                       dev_err(dev, "Could not power on the device\n");
> > +                       return ret;
> > +               }
> > +
> > +               /*
> > +                * Enable runtime PM with autosuspend. As the device has been
> > +                * powered manually, mark it as active, and increase the usage
> > +                * count without resuming the device.
> > +                */
> > +               pm_runtime_set_active(dev);
> > +               pm_runtime_get_noresume(dev);
> >         }
> >
> > -       /*
> > -        * Enable runtime PM with autosuspend. As the device has been powered
> > -        * manually, mark it as active, and increase the usage count without
> > -        * resuming the device.
> > -        */
> > -       pm_runtime_set_active(dev);
> > -       pm_runtime_get_noresume(dev);
> >         pm_runtime_enable(dev);
> >         pm_runtime_set_autosuspend_delay(dev, 1000);
> >         pm_runtime_use_autosuspend(dev);
> >
> > -       /* Make sure the sensor is available before V4L2 subdev init. */
> > -       ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > -       if (ret) {
> > -               ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
> > -               goto err_pm;
> > -       }
> > -       if (val != IMX290_STANDBY_STANDBY) {
> > -               ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
> > -               goto err_pm;
> > +       /*
> > +        * Make sure the sensor is available before V4L2 subdev init.
> > +        * This only works when the sensor is powered.
> > +        */
> > +       if (power_on) {
> > +               ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > +               if (ret) {
> > +                       ret = dev_err_probe(dev, -ENODEV,
> > +                                           "Failed to detect sensor\n");
> > +                       goto err_pm;
> > +               }
> > +               if (val != IMX290_STANDBY_STANDBY) {
> > +                       ret = dev_err_probe(dev, -ENODEV,
> > +                                           "Sensor is not in standby\n");
> > +                       goto err_pm;
> > +               }
> >         }
> >
> >         /* Initialize the V4L2 subdev. */
> > @@ -1666,8 +1696,10 @@ static int imx290_probe(struct i2c_client *client)
> >          * Decrease the PM usage count. The device will get suspended after the
> >          * autosuspend delay, turning the power off.
> >          */
> > -       pm_runtime_mark_last_busy(dev);
> > -       pm_runtime_put_autosuspend(dev);
> > +       if (power_on) {
> > +               pm_runtime_mark_last_busy(dev);
> > +               pm_runtime_put_autosuspend(dev);
> > +       }
> >
> >         return 0;
> >
> >
> > --
> > 2.46.0
> >
> >

Re: [PATCH v3 7/7] media: i2c: imx290: Implement a "privacy mode" for probe()

[Laurent Pinchart]

On Mon, Sep 02, 2024 at 09:49:33PM +0200, Benjamin Bara wrote:
> On Mon, 2 Sept 2024 at 20:10, Dave Stevenson wrote:
> > On Mon, 2 Sept 2024 at 16:59, <bbara93@gmail.com> wrote:
> > >
> > > From: Benjamin Bara <benjamin.bara@skidata.com>
> > >
> > > Currently, we have a trade-off between potentially enabling the privacy
> > > LED and reading out the connection state of the sensor during probe().
> > >
> > > To have a somewhat defined policy for now, make a decision based on the
> > > power supplies of the sensor. If they are enabled anyways, communicate
> > > with the powered sensor for an availability check. Otherwise, create the
> > > subdevice without knowing whether the sensor is connected or not.
> >
> > Almost all the camera modules used on Raspberry Pi have regulators
> > controlled via a GPIO, but no privacy LED. The preference from us is
> > very definitely to query the sensor during probe where possible to
> > flag up any connectivity issues, and indeed I've had a number of
> > support threads with imx290 where it's just not been connected but it
> > probed fully and showed up in libcamera.
> >
> > How can I opt in to patch 6 checking basic I2C to the sensor during
> > probe when I have a controllable regulator? (This is where the
> > discussions over a dtbinding for privacy LEDs and not powering up
> > sensors during probe comes in).
> 
> When you want to probe only during boot time, you can use the
> "regulator-boot-on" DT binding on your controllable regulator. This
> enables the regulator while it is probed and disables it later if not
> used (in comparison to "always-on"). Should also work for modules.

This seems like a hack, I'm sorry :-( I think we should instead have a
DT property standardized for camera sensors that tell whether or not
there is a privacy LED, and skip the detection in that case.

> Unfortunately, I don't have a clean solution (which also autosuspends)
> for "any probe time". I think it is not possible to enable a regulator
> from user space without having a consuming DT node. A somewhat clean
> workaround might be CONFIG_REGULATOR_USERSPACE_CONSUMER, which gives you
> the possibility to change the state of a regulator via sysfs (after
> creating a DT node). This gives you the possibility to enable it any
> time. However, the userspace-consumer driver gets the regulators
> exclusive, which means you cannot add the sensor driver as consumer and
> therefore cannot use the autosuspend feature of the imx290. Not really
> "nice", but probably "feasible" if you have special constraints when you
> are allowed to probe (e.g. the temperature as mentioned by Laurent). A
> DT binding would be easier for this case.
> 
> > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > ---
> > > Changes since v2:
> > > - new
> > > ---
> > >  drivers/media/i2c/imx290.c | 82 ++++++++++++++++++++++++++++++++--------------
> > >  1 file changed, 57 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 6b292bbb0856..338b2c5ea547 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -1354,6 +1354,17 @@ static void imx290_subdev_cleanup(struct imx290 *imx290)
> > >   * Power management
> > >   */
> > >
> > > +static bool is_imx290_power_on(struct imx290 *imx)
> > > +{
> > > +       unsigned int i;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(imx->supplies); i++)
> > > +               if (!regulator_is_enabled(imx->supplies[i].consumer))
> > > +                       return false;
> > > +
> > > +       return true;
> > > +}
> > > +
> > >  static int imx290_power_on(struct imx290 *imx290)
> > >  {
> > >         int ret;
> > > @@ -1571,6 +1582,7 @@ static int imx290_probe(struct i2c_client *client)
> > >  {
> > >         struct device *dev = &client->dev;
> > >         struct imx290 *imx290;
> > > +       bool power_on;
> > >         u64 val;
> > >         int ret;
> > >
> > > @@ -1611,36 +1623,54 @@ static int imx290_probe(struct i2c_client *client)
> > >                 return ret;
> > >
> > >         /*
> > > -        * Enable power management. The driver supports runtime PM, but needs to
> > > -        * work when runtime PM is disabled in the kernel. To that end, power
> > > -        * the sensor on manually here.
> > > +        * Privacy mode: if the regulators are not enabled, avoid enabling them.
> > > +        * In case the regulators are enabled, we still want to make sure that
> > > +        * the regulators know that they have another consumer, therefore run
> > > +        * the powering sequence.
> > >          */
> > > -       ret = imx290_power_on(imx290);
> > > -       if (ret < 0) {
> > > -               dev_err(dev, "Could not power on the device\n");
> > > -               return ret;
> > > +       power_on = is_imx290_power_on(imx290);
> > > +       dev_dbg(dev, "%s: power on: %d\n", __func__, power_on);
> > > +       if (power_on) {
> > > +               /*
> > > +                * Enable power management. The driver supports runtime PM, but
> > > +                * needs to work when runtime PM is disabled in the kernel. To
> > > +                * that end, power the sensor on manually here.
> > > +                */
> > > +               ret = imx290_power_on(imx290);
> > > +               if (ret < 0) {
> > > +                       dev_err(dev, "Could not power on the device\n");
> > > +                       return ret;
> > > +               }
> > > +
> > > +               /*
> > > +                * Enable runtime PM with autosuspend. As the device has been
> > > +                * powered manually, mark it as active, and increase the usage
> > > +                * count without resuming the device.
> > > +                */
> > > +               pm_runtime_set_active(dev);
> > > +               pm_runtime_get_noresume(dev);
> > >         }
> > >
> > > -       /*
> > > -        * Enable runtime PM with autosuspend. As the device has been powered
> > > -        * manually, mark it as active, and increase the usage count without
> > > -        * resuming the device.
> > > -        */
> > > -       pm_runtime_set_active(dev);
> > > -       pm_runtime_get_noresume(dev);
> > >         pm_runtime_enable(dev);
> > >         pm_runtime_set_autosuspend_delay(dev, 1000);
> > >         pm_runtime_use_autosuspend(dev);
> > >
> > > -       /* Make sure the sensor is available before V4L2 subdev init. */
> > > -       ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > > -       if (ret) {
> > > -               ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
> > > -               goto err_pm;
> > > -       }
> > > -       if (val != IMX290_STANDBY_STANDBY) {
> > > -               ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
> > > -               goto err_pm;
> > > +       /*
> > > +        * Make sure the sensor is available before V4L2 subdev init.
> > > +        * This only works when the sensor is powered.
> > > +        */
> > > +       if (power_on) {
> > > +               ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > > +               if (ret) {
> > > +                       ret = dev_err_probe(dev, -ENODEV,
> > > +                                           "Failed to detect sensor\n");
> > > +                       goto err_pm;
> > > +               }
> > > +               if (val != IMX290_STANDBY_STANDBY) {
> > > +                       ret = dev_err_probe(dev, -ENODEV,
> > > +                                           "Sensor is not in standby\n");
> > > +                       goto err_pm;
> > > +               }
> > >         }
> > >
> > >         /* Initialize the V4L2 subdev. */
> > > @@ -1666,8 +1696,10 @@ static int imx290_probe(struct i2c_client *client)
> > >          * Decrease the PM usage count. The device will get suspended after the
> > >          * autosuspend delay, turning the power off.
> > >          */
> > > -       pm_runtime_mark_last_busy(dev);
> > > -       pm_runtime_put_autosuspend(dev);
> > > +       if (power_on) {
> > > +               pm_runtime_mark_last_busy(dev);
> > > +               pm_runtime_put_autosuspend(dev);
> > > +       }
> > >
> > >         return 0;
> > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 7/7] media: i2c: imx290: Implement a "privacy mode" for probe()

[Benjamin Bara]

Hi Laurent!

On Mon, 2 Sept 2024 at 22:04, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Sep 02, 2024 at 09:49:33PM +0200, Benjamin Bara wrote:
> > On Mon, 2 Sept 2024 at 20:10, Dave Stevenson wrote:
> > > On Mon, 2 Sept 2024 at 16:59, <bbara93@gmail.com> wrote:
> > > >
> > > > From: Benjamin Bara <benjamin.bara@skidata.com>
> > > >
> > > > Currently, we have a trade-off between potentially enabling the privacy
> > > > LED and reading out the connection state of the sensor during probe().
> > > >
> > > > To have a somewhat defined policy for now, make a decision based on the
> > > > power supplies of the sensor. If they are enabled anyways, communicate
> > > > with the powered sensor for an availability check. Otherwise, create the
> > > > subdevice without knowing whether the sensor is connected or not.
> > >
> > > Almost all the camera modules used on Raspberry Pi have regulators
> > > controlled via a GPIO, but no privacy LED. The preference from us is
> > > very definitely to query the sensor during probe where possible to
> > > flag up any connectivity issues, and indeed I've had a number of
> > > support threads with imx290 where it's just not been connected but it
> > > probed fully and showed up in libcamera.
> > >
> > > How can I opt in to patch 6 checking basic I2C to the sensor during
> > > probe when I have a controllable regulator? (This is where the
> > > discussions over a dtbinding for privacy LEDs and not powering up
> > > sensors during probe comes in).
> >
> > When you want to probe only during boot time, you can use the
> > "regulator-boot-on" DT binding on your controllable regulator. This
> > enables the regulator while it is probed and disables it later if not
> > used (in comparison to "always-on"). Should also work for modules.
>
> This seems like a hack, I'm sorry :-( I think we should instead have a
> DT property standardized for camera sensors that tell whether or not
> there is a privacy LED, and skip the detection in that case.

No prob, I didn't really expect this to be final and fully understand
it. I'll simply drop it for the next round. I also think that the DT
binding "has-privacy-led" is the much cleaner solution and also aligns
with the HW description constraint.

Thanks & regards
Benjamin

> > Unfortunately, I don't have a clean solution (which also autosuspends)
> > for "any probe time". I think it is not possible to enable a regulator
> > from user space without having a consuming DT node. A somewhat clean
> > workaround might be CONFIG_REGULATOR_USERSPACE_CONSUMER, which gives you
> > the possibility to change the state of a regulator via sysfs (after
> > creating a DT node). This gives you the possibility to enable it any
> > time. However, the userspace-consumer driver gets the regulators
> > exclusive, which means you cannot add the sensor driver as consumer and
> > therefore cannot use the autosuspend feature of the imx290. Not really
> > "nice", but probably "feasible" if you have special constraints when you
> > are allowed to probe (e.g. the temperature as mentioned by Laurent). A
> > DT binding would be easier for this case.
> >
> > > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > > ---
> > > > Changes since v2:
> > > > - new
> > > > ---
> > > >  drivers/media/i2c/imx290.c | 82 ++++++++++++++++++++++++++++++++--------------
> > > >  1 file changed, 57 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index 6b292bbb0856..338b2c5ea547 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -1354,6 +1354,17 @@ static void imx290_subdev_cleanup(struct imx290 *imx290)
> > > >   * Power management
> > > >   */
> > > >
> > > > +static bool is_imx290_power_on(struct imx290 *imx)
> > > > +{
> > > > +       unsigned int i;
> > > > +
> > > > +       for (i = 0; i < ARRAY_SIZE(imx->supplies); i++)
> > > > +               if (!regulator_is_enabled(imx->supplies[i].consumer))
> > > > +                       return false;
> > > > +
> > > > +       return true;
> > > > +}
> > > > +
> > > >  static int imx290_power_on(struct imx290 *imx290)
> > > >  {
> > > >         int ret;
> > > > @@ -1571,6 +1582,7 @@ static int imx290_probe(struct i2c_client *client)
> > > >  {
> > > >         struct device *dev = &client->dev;
> > > >         struct imx290 *imx290;
> > > > +       bool power_on;
> > > >         u64 val;
> > > >         int ret;
> > > >
> > > > @@ -1611,36 +1623,54 @@ static int imx290_probe(struct i2c_client *client)
> > > >                 return ret;
> > > >
> > > >         /*
> > > > -        * Enable power management. The driver supports runtime PM, but needs to
> > > > -        * work when runtime PM is disabled in the kernel. To that end, power
> > > > -        * the sensor on manually here.
> > > > +        * Privacy mode: if the regulators are not enabled, avoid enabling them.
> > > > +        * In case the regulators are enabled, we still want to make sure that
> > > > +        * the regulators know that they have another consumer, therefore run
> > > > +        * the powering sequence.
> > > >          */
> > > > -       ret = imx290_power_on(imx290);
> > > > -       if (ret < 0) {
> > > > -               dev_err(dev, "Could not power on the device\n");
> > > > -               return ret;
> > > > +       power_on = is_imx290_power_on(imx290);
> > > > +       dev_dbg(dev, "%s: power on: %d\n", __func__, power_on);
> > > > +       if (power_on) {
> > > > +               /*
> > > > +                * Enable power management. The driver supports runtime PM, but
> > > > +                * needs to work when runtime PM is disabled in the kernel. To
> > > > +                * that end, power the sensor on manually here.
> > > > +                */
> > > > +               ret = imx290_power_on(imx290);
> > > > +               if (ret < 0) {
> > > > +                       dev_err(dev, "Could not power on the device\n");
> > > > +                       return ret;
> > > > +               }
> > > > +
> > > > +               /*
> > > > +                * Enable runtime PM with autosuspend. As the device has been
> > > > +                * powered manually, mark it as active, and increase the usage
> > > > +                * count without resuming the device.
> > > > +                */
> > > > +               pm_runtime_set_active(dev);
> > > > +               pm_runtime_get_noresume(dev);
> > > >         }
> > > >
> > > > -       /*
> > > > -        * Enable runtime PM with autosuspend. As the device has been powered
> > > > -        * manually, mark it as active, and increase the usage count without
> > > > -        * resuming the device.
> > > > -        */
> > > > -       pm_runtime_set_active(dev);
> > > > -       pm_runtime_get_noresume(dev);
> > > >         pm_runtime_enable(dev);
> > > >         pm_runtime_set_autosuspend_delay(dev, 1000);
> > > >         pm_runtime_use_autosuspend(dev);
> > > >
> > > > -       /* Make sure the sensor is available before V4L2 subdev init. */
> > > > -       ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > > > -       if (ret) {
> > > > -               ret = dev_err_probe(dev, -ENODEV, "Failed to detect sensor\n");
> > > > -               goto err_pm;
> > > > -       }
> > > > -       if (val != IMX290_STANDBY_STANDBY) {
> > > > -               ret = dev_err_probe(dev, -ENODEV, "Sensor is not in standby\n");
> > > > -               goto err_pm;
> > > > +       /*
> > > > +        * Make sure the sensor is available before V4L2 subdev init.
> > > > +        * This only works when the sensor is powered.
> > > > +        */
> > > > +       if (power_on) {
> > > > +               ret = cci_read(imx290->regmap, IMX290_STANDBY, &val, NULL);
> > > > +               if (ret) {
> > > > +                       ret = dev_err_probe(dev, -ENODEV,
> > > > +                                           "Failed to detect sensor\n");
> > > > +                       goto err_pm;
> > > > +               }
> > > > +               if (val != IMX290_STANDBY_STANDBY) {
> > > > +                       ret = dev_err_probe(dev, -ENODEV,
> > > > +                                           "Sensor is not in standby\n");
> > > > +                       goto err_pm;
> > > > +               }
> > > >         }
> > > >
> > > >         /* Initialize the V4L2 subdev. */
> > > > @@ -1666,8 +1696,10 @@ static int imx290_probe(struct i2c_client *client)
> > > >          * Decrease the PM usage count. The device will get suspended after the
> > > >          * autosuspend delay, turning the power off.
> > > >          */
> > > > -       pm_runtime_mark_last_busy(dev);
> > > > -       pm_runtime_put_autosuspend(dev);
> > > > +       if (power_on) {
> > > > +               pm_runtime_mark_last_busy(dev);
> > > > +               pm_runtime_put_autosuspend(dev);
> > > > +       }
> > > >
> > > >         return 0;
> > > >
>
> --
> Regards,
>
> Laurent Pinchart

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

[Dave Stevenson]

Hi Benjamin

On Mon, 2 Sept 2024 at 16:57, <bbara93@gmail.com> wrote:
>
> Hi!
>
> This series introduces i2c communication with the imx290 sensor during
> probe s.t. the v4l2 subdev is not initialized when the chip is not
> reachable.
>
> The datasheets show that INCKSEL* registers have a different default
> value after reset on imx290[1] and imx327[2], however I am not sure if
> this is a sufficient identification option - therefore I just removed
> the current CHIP_ID register for now.
>
> Thank you all for the feedback and the discussion, I updated the series
> to contain some of the ideas that came up.
>
> For now, I kept reading back the content of the STANDBY register, as
> suggested by Sakari and Alexander. If not wanted (as suggested by
> Laurent), I can re-add my "optional read" commit from the first version
> of this series instead.
>
> *Overview:*
> Patch 1/7 is a split from the old 1/2 which defines the values of the
> STANDBY register and uses them.
> Patch 2/7 is new and defines the whole supported range of the controls.
> Patch 3/7 is the old 2/2 to drop the CHIP_ID register.
> Patch 4+5/7 are new and target the avoidable communication during
> probe(). I decided to use a variant that also works on machines without
> runtime PM active, and falls back to the values of 2/7 instead.

I was reading through patches 2, 4, 5, and 7 and really not seeing
what you're trying to avoid here. Adding an option to avoid powering
up the sensor is one thing, but we've got masses of other changes to
fake HBLANK, VBLANK, and LINK_FREQ values, even though we appear to
still have a pad format defined as 1920x1080 1x10 set on the pad. All
those controls are therefore declaring invalid ranges at that point.

If it's solely trying to stop imx290_set_ctrl sending register writes
when not streaming, isn't it simpler to have a basic bool in the state
structure to denote whether we've passed through
imx290_set_stream(subdev, 1)? Set it in the same place as
pm_runtime_resume_and_get is called as it's mimicking exactly the same
behaviour.

> Additionally, imx290_runtime_suspend() is using v4l_subdev, and
> therefore depends on the subdevice. If we move the autosuspend stuff
> before the subdevice creation, we basically have a race between the
> delay and the subdevice creation. Although it is not very close, I don't
> think it is a good idea to potentially autosuspend before the subdev is
> created.
> Patch 6/7 is the old 1/2.
> Patch 7/7 is a new PoC to decide to check the availability based on the
> power state of the sensor during probe().
>
> Note: dummy-regulators, which are used when no supplies are set in the
> DT, are always-on.
> Note2: The "off" mode is currently only active after probe(). If this
> approach is of interest, I would also ensure that it is active once
> streaming has stopped - need to dig deeper if it is necessary to store
> the "old current" before stopping, therefore only "initial" for now.

What extra data do you see sent after stopping? On any system with
runtime PM, imx290_set_ctrl will stop at the "if
(!pm_runtime_get_if_in_use(imx290->dev))" check.

  Dave

> thanks & regards
> Benjamin
>
> [1] https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf
> [2] https://e2e.ti.com/cfs-file/__key/communityserver-discussions-components-files/138/IMX327LQR_2D00_C_5F00_TechnicalDatasheet_5F00_E_5F00_Rev0.2.pdf
>
> ---
> Changes in v3:
> - probably better readable in the overview
> - 1/2 -> 1+6/7
> - 2/2 -> 3/7 (added R-b - thx for that)
> - others are new based on the discussions/suggestions
> - Link to v2: https://lore.kernel.org/r/20240828-imx290-avail-v2-0-bd320ac8e8fa@skidata.com
>
> Changes in v2:
> - dropped 1/2 to ignore the read value in cci_read() (old 2/2 -> new 1/2)
> - 1/2: read-back standby mode instead and ensure that it is really in standby
> - new 2/2: drop chip_id register definition which seems to be incorrect
> - Link to v1: https://lore.kernel.org/r/20240807-imx290-avail-v1-0-666c130c7601@skidata.com
>
> ---
> Benjamin Bara (7):
>       media: i2c: imx290: Define standby mode values
>       media: i2c: imx290: Define absolute control ranges
>       media: i2c: imx290: Remove CHIP_ID reg definition
>       media: i2c: imx290: Introduce initial "off" mode & link freq
>       media: i2c: imx290: Avoid communication during probe()
>       media: i2c: imx290: Check for availability in probe()
>       media: i2c: imx290: Implement a "privacy mode" for probe()
>
>  drivers/media/i2c/imx290.c | 153 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 124 insertions(+), 29 deletions(-)
> ---
> base-commit: eec5d86d5bac6b3e972eb9c1898af3c08303c52d
> change-id: 20240807-imx290-avail-85795c27d988
>
> Best regards,
> --
> Benjamin Bara <benjamin.bara@skidata.com>
>
>

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

[Benjamin Bara]

Hi Dave!

Thank you for your feedback!

On Mon, 2 Sept 2024 at 19:55, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
> On Mon, 2 Sept 2024 at 16:57, <bbara93@gmail.com> wrote:
> > This series introduces i2c communication with the imx290 sensor during
> > probe s.t. the v4l2 subdev is not initialized when the chip is not
> > reachable.
> >
> > The datasheets show that INCKSEL* registers have a different default
> > value after reset on imx290[1] and imx327[2], however I am not sure if
> > this is a sufficient identification option - therefore I just removed
> > the current CHIP_ID register for now.
> >
> > Thank you all for the feedback and the discussion, I updated the series
> > to contain some of the ideas that came up.
> >
> > For now, I kept reading back the content of the STANDBY register, as
> > suggested by Sakari and Alexander. If not wanted (as suggested by
> > Laurent), I can re-add my "optional read" commit from the first version
> > of this series instead.
> >
> > *Overview:*
> > Patch 1/7 is a split from the old 1/2 which defines the values of the
> > STANDBY register and uses them.
> > Patch 2/7 is new and defines the whole supported range of the controls.
> > Patch 3/7 is the old 2/2 to drop the CHIP_ID register.
> > Patch 4+5/7 are new and target the avoidable communication during
> > probe(). I decided to use a variant that also works on machines without
> > runtime PM active, and falls back to the values of 2/7 instead.
>
> I was reading through patches 2, 4, 5, and 7 and really not seeing
> what you're trying to avoid here. Adding an option to avoid powering
> up the sensor is one thing, but we've got masses of other changes to
> fake HBLANK, VBLANK, and LINK_FREQ values, even though we appear to
> still have a pad format defined as 1920x1080 1x10 set on the pad. All
> those controls are therefore declaring invalid ranges at that point.

I guess I missed the pad format. My idea with 2+4 was to have "more free
control ranges" while we don't have a mode set yet - but it is probably
not a really good idea. Patch 5, as you said, can also be replaced with
a bool. As Laurent brought up the point with the privacy LED, patch 7 is
just an example how we can avoid powering the sensor up during probe
time.

> If it's solely trying to stop imx290_set_ctrl sending register writes
> when not streaming, isn't it simpler to have a basic bool in the state
> structure to denote whether we've passed through
 imx290_set_stream(subdev, 1)? Set it in the same place as
> pm_runtime_resume_and_get is called as it's mimicking exactly the same
> behaviour.
>
> > Additionally, imx290_runtime_suspend() is using v4l_subdev, and
> > therefore depends on the subdevice. If we move the autosuspend stuff
> > before the subdevice creation, we basically have a race between the
> > delay and the subdevice creation. Although it is not very close, I don't
> > think it is a good idea to potentially autosuspend before the subdev is
> > created.
> > Patch 6/7 is the old 1/2.
> > Patch 7/7 is a new PoC to decide to check the availability based on the
> > power state of the sensor during probe().
> >
> > Note: dummy-regulators, which are used when no supplies are set in the
> > DT, are always-on.
> > Note2: The "off" mode is currently only active after probe(). If this
> > approach is of interest, I would also ensure that it is active once
> > streaming has stopped - need to dig deeper if it is necessary to store
> > the "old current" before stopping, therefore only "initial" for now.
>
> What extra data do you see sent after stopping? On any system with
> runtime PM, imx290_set_ctrl will stop at the "if
> (!pm_runtime_get_if_in_use(imx290->dev))" check.

This is rather related to patch 2+4. As I already expected that it has
downsides to have an extra (invalid) "off mode", I didn't set it
everytime streaming is stopped. I guess dropping 2+4 and replacing 5
with a bool is probably a better idea to have this.

Thanks
Benjamin

>
>   Dave
>
> > thanks & regards
> > Benjamin
> >
> > [1] https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf
> > [2] https://e2e.ti.com/cfs-file/__key/communityserver-discussions-components-files/138/IMX327LQR_2D00_C_5F00_TechnicalDatasheet_5F00_E_5F00_Rev0.2.pdf
> >
> > ---
> > Changes in v3:
> > - probably better readable in the overview
> > - 1/2 -> 1+6/7
> > - 2/2 -> 3/7 (added R-b - thx for that)
> > - others are new based on the discussions/suggestions
> > - Link to v2: https://lore.kernel.org/r/20240828-imx290-avail-v2-0-bd320ac8e8fa@skidata.com
> >
> > Changes in v2:
> > - dropped 1/2 to ignore the read value in cci_read() (old 2/2 -> new 1/2)
> > - 1/2: read-back standby mode instead and ensure that it is really in standby
> > - new 2/2: drop chip_id register definition which seems to be incorrect
> > - Link to v1: https://lore.kernel.org/r/20240807-imx290-avail-v1-0-666c130c7601@skidata.com
> >
> > ---
> > Benjamin Bara (7):
> >       media: i2c: imx290: Define standby mode values
> >       media: i2c: imx290: Define absolute control ranges
> >       media: i2c: imx290: Remove CHIP_ID reg definition
> >       media: i2c: imx290: Introduce initial "off" mode & link freq
> >       media: i2c: imx290: Avoid communication during probe()
> >       media: i2c: imx290: Check for availability in probe()
> >       media: i2c: imx290: Implement a "privacy mode" for probe()
> >
> >  drivers/media/i2c/imx290.c | 153 ++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 124 insertions(+), 29 deletions(-)
> > ---
> > base-commit: eec5d86d5bac6b3e972eb9c1898af3c08303c52d
> > change-id: 20240807-imx290-avail-85795c27d988
> >
> > Best regards,
> > --
> > Benjamin Bara <benjamin.bara@skidata.com>
> >
> >