[PATCH v4 00/17] media: gmsl: Reliability improvements

[PATCH v4 00/17] media: gmsl: Reliability improvements

[Jacopo Mondi]

Hello,
   series following:
v1: https://patchwork.linuxtv.org/project/linux-media/list/?series=4650
v2: https://patchwork.linuxtv.org/project/linux-media/list/?series=4861
v3: https://patchwork.linuxtv.org/project/linux-media/list/?series=4904

This series is a minor rework that builds on top of the comments received on v3.

I've re-order patches for better consumption:
[01/17] -> [05/17]: max9286 style fixes
[06/17] -> [07/17]: max9271 minor fixes
[08/17] -> [10/17]: rdamc21 fixes: these patches are relevant for RDACM21
 		    stability
[11/17] -> [15/17]: rdacm20 fixes
[16/17] -> [17/17]: GMSL initialization series rework, also relevant for syste,
                    stability

The only part where consensus still has to be reached is the last two patches.
Unfortunately, Sakari's suggestion of moving the remotes initialization to
s_stream() time did not work, and this version is the only one I've found that
gurantees a reliable initialization sequence. I've cc-ed Hans and Sakari to
continue the discussion.

Run quite some tests with Eagle and RDACM21: 1378 boot cycles with 100% boot
success (thanks Kieran for the board access).

Thanks
   j

Jacopo Mondi (17):
  media: i2c: max9286: Adjust parameters indent
  media: i2c: max9286: Rename reverse_channel_mv
  media: i2c: max9286: Cache channel amplitude
  media: i2c: max9286: Define high channel amplitude
  media: i2c: max9286: Rework comments in .bound()
  media: i2c: max9271: Check max9271_write() return
  media: i2c: max9271: Introduce wake_up() function
  media: i2c: rdacm21: Add dealy after OV490 reset
  media: i2c: rdacm21: Fix OV10640 powerup
  media: i2c: rdacm21: Power up OV10640 before OV490
  media: i2c: rdacm20: Enable noise immunity
  media: i2c: rdacm20: Embed 'serializer' field
  media: i2c: rdacm20: Report camera module name
  media: i2c: rdacm20: Check return values
  media: i2c: rdacm20: Re-work ov10635 reset
  media: v4l2-subdev: De-deprecate init() subdev op
  media: gmsl: Reimplement initialization sequence

 drivers/media/i2c/max9271.c |  42 +++++++++--
 drivers/media/i2c/max9271.h |   9 +++
 drivers/media/i2c/max9286.c |  56 +++++++++------
 drivers/media/i2c/rdacm20.c | 135 +++++++++++++++++++++---------------
 drivers/media/i2c/rdacm21.c | 124 +++++++++++++++++++++------------
 include/media/v4l2-subdev.h |  15 +++-
 6 files changed, 253 insertions(+), 128 deletions(-)

--
2.31.1

[PATCH v4 01/17] media: i2c: max9286: Adjust parameters indent

[Jacopo Mondi]

The parameters to max9286_i2c_mux_configure() fits on the previous
line. Adjust it.

Cosmetic change only.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 6fd4d59fcc72..1d9951215868 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -287,9 +287,8 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
 
 	priv->mux_channel = chan;
 
-	max9286_i2c_mux_configure(priv,
-				  MAX9286_FWDCCEN(chan) |
-				  MAX9286_REVCCEN(chan));
+	max9286_i2c_mux_configure(priv, MAX9286_FWDCCEN(chan) |
+					MAX9286_REVCCEN(chan));
 
 	return 0;
 }
-- 
2.31.1

[PATCH v4 02/17] media: i2c: max9286: Rename reverse_channel_mv

[Jacopo Mondi]

Rename the reverse_channel_mv variable to init_rev_chan_mv as
the next patch will cache the reverse channel amplitude in
a new driver variable.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1d9951215868..82ec05e96cb7 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -163,7 +163,8 @@ struct max9286_priv {
 	unsigned int mux_channel;
 	bool mux_open;
 
-	u32 reverse_channel_mv;
+	/* The initial reverse control channel amplitude. */
+	u32 init_rev_chan_mv;
 
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixelrate;
@@ -563,7 +564,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
-	if (priv->reverse_channel_mv < 170)
+	if (priv->init_rev_chan_mv < 170)
 		max9286_reverse_channel_setup(priv, 170);
 	max9286_check_config_link(priv, priv->source_mask);
 
@@ -971,7 +972,7 @@ static int max9286_setup(struct max9286_priv *priv)
 	 * only. This should be disabled after the mux is initialised.
 	 */
 	max9286_configure_i2c(priv, true);
-	max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
+	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
 
 	/*
 	 * Enable GMSL links, mask unused ones and autodetect link
@@ -1236,9 +1237,9 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	if (of_property_read_u32(dev->of_node,
 				 "maxim,reverse-channel-microvolt",
 				 &reverse_channel_microvolt))
-		priv->reverse_channel_mv = 170;
+		priv->init_rev_chan_mv = 170;
 	else
-		priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
+		priv->init_rev_chan_mv = reverse_channel_microvolt / 1000U;
 
 	priv->route_mask = priv->source_mask;
 
-- 
2.31.1

[PATCH v4 03/17] media: i2c: max9286: Cache channel amplitude

[Jacopo Mondi]

Cache the current channel amplitude in a driver variable
to skip updating it if the newly requested value is the same
as the currently configured one.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 82ec05e96cb7..ed1cdefe7c30 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -165,6 +165,7 @@ struct max9286_priv {
 
 	/* The initial reverse control channel amplitude. */
 	u32 init_rev_chan_mv;
+	u32 rev_chan_mv;
 
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixelrate;
@@ -341,8 +342,15 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
 static void max9286_reverse_channel_setup(struct max9286_priv *priv,
 					  unsigned int chan_amplitude)
 {
+	u8 chan_config;
+
+	if (priv->rev_chan_mv == chan_amplitude)
+		return;
+
+	priv->rev_chan_mv = chan_amplitude;
+
 	/* Reverse channel transmission time: default to 1. */
-	u8 chan_config = MAX9286_REV_TRF(1);
+	chan_config = MAX9286_REV_TRF(1);
 
 	/*
 	 * Reverse channel setup.
@@ -564,8 +572,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
-	if (priv->init_rev_chan_mv < 170)
-		max9286_reverse_channel_setup(priv, 170);
+	max9286_reverse_channel_setup(priv, 170);
 	max9286_check_config_link(priv, priv->source_mask);
 
 	/*
-- 
2.31.1

[PATCH v4 04/17] media: i2c: max9286: Define high channel amplitude

[Jacopo Mondi]

Provide a macro to define the reverse channel amplitude to
be used to compensate the remote serializer noise immunity.

While at it, update a comment.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index ed1cdefe7c30..9124d5fa6ea3 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -113,6 +113,7 @@
 #define MAX9286_REV_TRF(n)		((n) << 4)
 #define MAX9286_REV_AMP(n)		((((n) - 30) / 10) << 1) /* in mV */
 #define MAX9286_REV_AMP_X		BIT(0)
+#define MAX9286_REV_AMP_HIGH		170
 /* Register 0x3f */
 #define MAX9286_EN_REV_CFG		BIT(6)
 #define MAX9286_REV_FLEN(n)		((n) - 20)
@@ -567,12 +568,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	 * channels:
 	 *
 	 * - Increase the reverse channel amplitude to compensate for the
-	 *   remote ends high threshold, if not done already
+	 *   remote ends high threshold
 	 * - Verify all configuration links are properly detected
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
-	max9286_reverse_channel_setup(priv, 170);
+	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
 	max9286_check_config_link(priv, priv->source_mask);
 
 	/*
-- 
2.31.1

[PATCH v4 05/17] media: i2c: max9286: Rework comments in .bound()

[Jacopo Mondi]

Rephrase a comment in .bound() callback to make it clear we register
a subdev notifier and remove a redundant comment about disabling i2c
auto-ack.

No functional changes intended.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 9124d5fa6ea3..e1c7173f2d00 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -556,9 +556,9 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 		subdev->name, src_pad, index);
 
 	/*
-	 * We can only register v4l2_async_notifiers, which do not provide a
-	 * means to register a complete callback. bound_sources allows us to
-	 * identify when all remote serializers have completed their probe.
+	 * As we register a subdev notifiers we won't get a .complete() callback
+	 * here, so we have to use bound_sources to identify when all remote
+	 * serializers have probed.
 	 */
 	if (priv->bound_sources != priv->source_mask)
 		return 0;
@@ -575,11 +575,6 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	 */
 	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
 	max9286_check_config_link(priv, priv->source_mask);
-
-	/*
-	 * Re-configure I2C with local acknowledge disabled after cameras have
-	 * probed.
-	 */
 	max9286_configure_i2c(priv, false);
 
 	return max9286_set_pixelrate(priv);
-- 
2.31.1

[PATCH v4 06/17] media: i2c: max9271: Check max9271_write() return

[Jacopo Mondi]

Check the return value of the max9271_write() function in the
max9271 library driver.

While at it, modify an existing condition to be made identical
to other checks.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9271.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index c495582dcff6..2c7dc7fb9846 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -106,7 +106,10 @@ int max9271_set_serial_link(struct max9271_device *dev, bool enable)
 	 * Short delays here appear to show bit-errors in the writes following.
 	 * Therefore a conservative delay seems best here.
 	 */
-	max9271_write(dev, 0x04, val);
+	ret = max9271_write(dev, 0x04, val);
+	if (ret < 0)
+		return ret;
+
 	usleep_range(5000, 8000);
 
 	return 0;
@@ -118,7 +121,7 @@ int max9271_configure_i2c(struct max9271_device *dev, u8 i2c_config)
 	int ret;
 
 	ret = max9271_write(dev, 0x0d, i2c_config);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	/* The delay required after an I2C bus configuration change is not
@@ -143,7 +146,10 @@ int max9271_set_high_threshold(struct max9271_device *dev, bool enable)
 	 * Enable or disable reverse channel high threshold to increase
 	 * immunity to power supply noise.
 	 */
-	max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
+	ret = max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
+	if (ret < 0)
+		return ret;
+
 	usleep_range(2000, 2500);
 
 	return 0;
@@ -152,6 +158,8 @@ EXPORT_SYMBOL_GPL(max9271_set_high_threshold);
 
 int max9271_configure_gmsl_link(struct max9271_device *dev)
 {
+	int ret;
+
 	/*
 	 * Configure the GMSL link:
 	 *
@@ -162,16 +170,24 @@ int max9271_configure_gmsl_link(struct max9271_device *dev)
 	 *
 	 * TODO: Make the GMSL link configuration parametric.
 	 */
-	max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
-		      MAX9271_EDC_1BIT_PARITY);
+	ret = max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
+			    MAX9271_EDC_1BIT_PARITY);
+	if (ret < 0)
+		return ret;
+
 	usleep_range(5000, 8000);
 
 	/*
 	 * Adjust spread spectrum to +4% and auto-detect pixel clock
 	 * and serial link rate.
 	 */
-	max9271_write(dev, 0x02, MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
-		      MAX9271_PCLK_AUTODETECT | MAX9271_SERIAL_AUTODETECT);
+	ret = max9271_write(dev, 0x02,
+			    MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
+			    MAX9271_PCLK_AUTODETECT |
+			    MAX9271_SERIAL_AUTODETECT);
+	if (ret < 0)
+		return ret;
+
 	usleep_range(5000, 8000);
 
 	return 0;
-- 
2.31.1

[PATCH v4 07/17] media: i2c: max9271: Introduce wake_up() function

[Jacopo Mondi]

The MAX9271 chip manual prescribes a delay of 5 milliseconds
after the chip exits from low power state.

Add a new function to the max9271 library driver that wakes up the chip
with a dummy i2c transaction and implements the correct delay of 5
milliseconds after the chip exits from low power state.

Use the newly introduced function in the rdacm20 and rdacm21 camera
drivers. The former was not respecting the required delay while the
latter was waiting for a too-short timeout.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9271.c | 12 ++++++++++++
 drivers/media/i2c/max9271.h |  9 +++++++++
 drivers/media/i2c/rdacm20.c |  4 +---
 drivers/media/i2c/rdacm21.c |  5 +----
 4 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index 2c7dc7fb9846..ff86c8c4ea61 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -80,6 +80,18 @@ static int max9271_pclk_detect(struct max9271_device *dev)
 	return -EIO;
 }
 
+void max9271_wake_up(struct max9271_device *dev)
+{
+	/*
+	 * Use the chip default address as this function has to be called
+	 * before any other one.
+	 */
+	dev->client->addr = MAX9271_DEFAULT_ADDR;
+	i2c_smbus_read_byte(dev->client);
+	usleep_range(5000, 8000);
+}
+EXPORT_SYMBOL_GPL(max9271_wake_up);
+
 int max9271_set_serial_link(struct max9271_device *dev, bool enable)
 {
 	int ret;
diff --git a/drivers/media/i2c/max9271.h b/drivers/media/i2c/max9271.h
index d78fb21441e9..dc5e4e70ba6f 100644
--- a/drivers/media/i2c/max9271.h
+++ b/drivers/media/i2c/max9271.h
@@ -85,6 +85,15 @@ struct max9271_device {
 	struct i2c_client *client;
 };
 
+/**
+ * max9271_wake_up() - Wake up the serializer by issuing an i2c transaction
+ * @dev: The max9271 device
+ *
+ * This function shall be called before any other interaction with the
+ * serializer.
+ */
+void max9271_wake_up(struct max9271_device *dev);
+
 /**
  * max9271_set_serial_link() - Enable/disable serial link
  * @dev: The max9271 device
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 90eb73f0e6e9..c1a717153303 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -455,9 +455,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	unsigned int retry = 3;
 	int ret;
 
-	/* Verify communication with the MAX9271: ping to wakeup. */
-	dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
-	i2c_smbus_read_byte(dev->serializer->client);
+	max9271_wake_up(dev->serializer);
 
 	/* Serial link disabled during config as it needs a valid pixel clock. */
 	ret = max9271_set_serial_link(dev->serializer, false);
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 179d107f494c..553e3f03752b 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -450,10 +450,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
 {
 	int ret;
 
-	/* Verify communication with the MAX9271: ping to wakeup. */
-	dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
-	i2c_smbus_read_byte(dev->serializer.client);
-	usleep_range(3000, 5000);
+	max9271_wake_up(&dev->serializer);
 
 	/* Enable reverse channel and disable the serial link. */
 	ret = max9271_set_serial_link(&dev->serializer, false);
-- 
2.31.1

[PATCH v4 08/17] media: i2c: rdacm21: Add dealy after OV490 reset

[Jacopo Mondi]

Add a delay after the OV490 chip is put in reset state. The reset
signal shall be held for at least 250 useconds.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm21.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 553e3f03752b..6be8ce130e78 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -469,7 +469,10 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
 	if (ret)
 		return ret;
 
-	/* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */
+	/*
+	 * Enable GPIO1 and hold OV490 in reset during max9271 configuration.
+	 * The reset signal has to be asserted for at least 250 useconds.
+	 */
 	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
@@ -477,6 +480,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
 	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
+	usleep_range(250, 500);
 
 	ret = max9271_configure_gmsl_link(&dev->serializer);
 	if (ret)
-- 
2.31.1

Re: [PATCH v4 08/17] media: i2c: rdacm21: Add dealy after OV490 reset

[Kieran Bingham]

Hi Jacopo,

There's still a s/dealy/delay/ in $SUBJECT

On 12/04/2021 10:34, Jacopo Mondi wrote:
> Add a delay after the OV490 chip is put in reset state. The reset
> signal shall be held for at least 250 useconds.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I added this on v3...

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/rdacm21.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 553e3f03752b..6be8ce130e78 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -469,7 +469,10 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  	if (ret)
>  		return ret;
>  
> -	/* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */
> +	/*
> +	 * Enable GPIO1 and hold OV490 in reset during max9271 configuration.
> +	 * The reset signal has to be asserted for at least 250 useconds.


Is it worth mentioning here that it is asserted to active low? Just to
make it clear that holding it low for 250 uS is the desired effect?

It might not be worth it - but perhaps that was the reason for some
confusion here.

Eitherway RB tag still stands I think.

--
Kieran

> +	 */
>  	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
> @@ -477,6 +480,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
> +	usleep_range(250, 500);
>  
>  	ret = max9271_configure_gmsl_link(&dev->serializer);
>  	if (ret)
> 

Re: [PATCH v4 08/17] media: i2c: rdacm21: Add dealy after OV490 reset

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Apr 12, 2021 at 11:34:42AM +0200, Jacopo Mondi wrote:
> Add a delay after the OV490 chip is put in reset state. The reset
> signal shall be held for at least 250 useconds.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

With s/dealy/delay/ in the subject line,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/rdacm21.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 553e3f03752b..6be8ce130e78 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -469,7 +469,10 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  	if (ret)
>  		return ret;
>  
> -	/* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */
> +	/*
> +	 * Enable GPIO1 and hold OV490 in reset during max9271 configuration.
> +	 * The reset signal has to be asserted for at least 250 useconds.
> +	 */
>  	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
> @@ -477,6 +480,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>  	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>  	if (ret)
>  		return ret;
> +	usleep_range(250, 500);
>  
>  	ret = max9271_configure_gmsl_link(&dev->serializer);
>  	if (ret)

-- 
Regards,

Laurent Pinchart

[PATCH v4 09/17] media: i2c: rdacm21: Fix OV10640 powerup

[Jacopo Mondi]

The OV10640 image sensor powerdown signal is controlled by the first
line of the OV490 GPIO pad #1, but the pad #0 identifier
OV490_GPIO_OUTPUT_VALUE0 was erroneously used. As a result the image
sensor powerdown signal was never asserted but was left floating and
kept high by an internal pull-up resistor, causing sporadic failures
during the image sensor startup phase.

Fix this by using the correct GPIO pad identifier and wait the mandatory
1.5 millisecond delay after the powerup lane is asserted. The reset
delay is not characterized in the chip manual if not as "255 XVCLK +
initialization". Wait for at least 3 milliseconds to guarantee the SCCB
bus is available.

While at it also fix the reset sequence, as the reset line was released
before the powerdown one, and the line was not cycled.

This commit fixes a sporadic start-up error triggered by a failure to
read the OV10640 chip ID:
rdacm21 8-0054: OV10640 ID mismatch: (0x01)

Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm21.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 6be8ce130e78..7c0a4a84340a 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -333,13 +333,19 @@ static int ov10640_initialize(struct rdacm21_device *dev)
 {
 	u8 val;
 
-	/* Power-up OV10640 by setting RESETB and PWDNB pins high. */
+	/* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
 	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
 	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
 	ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
 	ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
+
+	/* Power up OV10640 and then reset it. */
+	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE1, OV490_SPWDN0);
+	usleep_range(1500, 3000);
+
+	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, 0x00);
+	usleep_range(1500, 3000);
 	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
-	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
 	usleep_range(3000, 5000);
 
 	/* Read OV10640 ID to test communications. */
-- 
2.31.1

[PATCH v4 10/17] media: i2c: rdacm21: Power up OV10640 before OV490

[Jacopo Mondi]

The current RDACM21 initialization routine powers up the OV10640 image
sensor after the OV490 ISP. The ISP is programmed with a firmare loaded
from an embedded serial flash that (most probably) tries to interact and
program also the image sensor connected to the ISP.

As described in commit ("media: i2c: rdacm21: Fix OV10640 powerup") the
image sensor powerdown signal is kept high by an internal pull up
resistor and occasionally fails to startup correctly if the powerdown
line is not asserted explicitely. Failures in the OV10640 startup causes
the OV490 firmware to fail to boot correctly resulting in the camera
module initialization to fail consequentially.

Fix this by powering up the OV10640 image sensor before testing the
OV490 firmware boot completion, by splitting the ov10640_initialize()
function in an ov10640_power_up() one and an ov10640_check_id() one.

Also make sure the OV10640 identification procedure gives enough time to
the image sensor to resume after the programming phase performed by the
OV490 firmware by repeating the ID read procedure.

This commit fixes a sporadic start-up error triggered by a failure to
detect the OV490 firmware boot completion:
rdacm21 8-0054: Timeout waiting for firmware boot

Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm21.c | 46 ++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 7c0a4a84340a..43c41cb800a4 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -69,6 +69,7 @@
 #define OV490_ISP_VSIZE_LOW		0x80820062
 #define OV490_ISP_VSIZE_HIGH		0x80820063
 
+#define OV10640_PID_TIMEOUT		20
 #define OV10640_ID_HIGH			0xa6
 #define OV10640_CHIP_ID			0x300a
 #define OV10640_PIXEL_RATE		55000000
@@ -329,10 +330,8 @@ static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
 	.pad		= &rdacm21_subdev_pad_ops,
 };
 
-static int ov10640_initialize(struct rdacm21_device *dev)
+static void ov10640_power_up(struct rdacm21_device *dev)
 {
-	u8 val;
-
 	/* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
 	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
 	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
@@ -347,18 +346,35 @@ static int ov10640_initialize(struct rdacm21_device *dev)
 	usleep_range(1500, 3000);
 	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
 	usleep_range(3000, 5000);
+}
 
-	/* Read OV10640 ID to test communications. */
-	ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
-	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
-	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & 0xff);
-
-	/* Trigger SCCB slave transaction and give it some time to complete. */
-	ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
-	usleep_range(1000, 1500);
+static int ov10640_check_id(struct rdacm21_device *dev)
+{
+	unsigned int i;
+	u8 val;
 
-	ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
-	if (val != OV10640_ID_HIGH) {
+	/* Read OV10640 ID to test communications. */
+	for (i = 0; i < OV10640_PID_TIMEOUT; ++i) {
+		ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR,
+				OV490_SCCB_SLAVE_READ);
+		ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH,
+				OV10640_CHIP_ID >> 8);
+		ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW,
+				OV10640_CHIP_ID & 0xff);
+
+		/*
+		 * Trigger SCCB slave transaction and give it some time
+		 * to complete.
+		 */
+		ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
+		usleep_range(1000, 1500);
+
+		ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
+		if (val == OV10640_ID_HIGH)
+			break;
+		usleep_range(1000, 1500);
+	}
+	if (i == OV10640_PID_TIMEOUT) {
 		dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
 		return -ENODEV;
 	}
@@ -374,6 +390,8 @@ static int ov490_initialize(struct rdacm21_device *dev)
 	unsigned int i;
 	int ret;
 
+	ov10640_power_up(dev);
+
 	/*
 	 * Read OV490 Id to test communications. Give it up to 40msec to
 	 * exit from reset.
@@ -411,7 +429,7 @@ static int ov490_initialize(struct rdacm21_device *dev)
 		return -ENODEV;
 	}
 
-	ret = ov10640_initialize(dev);
+	ret = ov10640_check_id(dev);
 	if (ret)
 		return ret;
 
-- 
2.31.1

Re: [PATCH v4 10/17] media: i2c: rdacm21: Power up OV10640 before OV490

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Apr 12, 2021 at 11:34:44AM +0200, Jacopo Mondi wrote:
> The current RDACM21 initialization routine powers up the OV10640 image
> sensor after the OV490 ISP. The ISP is programmed with a firmare loaded
> from an embedded serial flash that (most probably) tries to interact and
> program also the image sensor connected to the ISP.
> 
> As described in commit ("media: i2c: rdacm21: Fix OV10640 powerup") the

s/[()]//g

> image sensor powerdown signal is kept high by an internal pull up
> resistor and occasionally fails to startup correctly if the powerdown
> line is not asserted explicitely. Failures in the OV10640 startup causes
> the OV490 firmware to fail to boot correctly resulting in the camera
> module initialization to fail consequentially.
> 
> Fix this by powering up the OV10640 image sensor before testing the
> OV490 firmware boot completion, by splitting the ov10640_initialize()
> function in an ov10640_power_up() one and an ov10640_check_id() one.
> 
> Also make sure the OV10640 identification procedure gives enough time to
> the image sensor to resume after the programming phase performed by the
> OV490 firmware by repeating the ID read procedure.
> 
> This commit fixes a sporadic start-up error triggered by a failure to
> detect the OV490 firmware boot completion:
> rdacm21 8-0054: Timeout waiting for firmware boot
> 
> Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/rdacm21.c | 46 ++++++++++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 7c0a4a84340a..43c41cb800a4 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -69,6 +69,7 @@
>  #define OV490_ISP_VSIZE_LOW		0x80820062
>  #define OV490_ISP_VSIZE_HIGH		0x80820063
>  
> +#define OV10640_PID_TIMEOUT		20
>  #define OV10640_ID_HIGH			0xa6
>  #define OV10640_CHIP_ID			0x300a
>  #define OV10640_PIXEL_RATE		55000000
> @@ -329,10 +330,8 @@ static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
>  	.pad		= &rdacm21_subdev_pad_ops,
>  };
>  
> -static int ov10640_initialize(struct rdacm21_device *dev)
> +static void ov10640_power_up(struct rdacm21_device *dev)
>  {
> -	u8 val;
> -
>  	/* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
>  	ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
>  	ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> @@ -347,18 +346,35 @@ static int ov10640_initialize(struct rdacm21_device *dev)
>  	usleep_range(1500, 3000);
>  	ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
>  	usleep_range(3000, 5000);
> +}
>  
> -	/* Read OV10640 ID to test communications. */
> -	ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
> -	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
> -	ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & 0xff);
> -
> -	/* Trigger SCCB slave transaction and give it some time to complete. */
> -	ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> -	usleep_range(1000, 1500);
> +static int ov10640_check_id(struct rdacm21_device *dev)
> +{
> +	unsigned int i;
> +	u8 val;
>  
> -	ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
> -	if (val != OV10640_ID_HIGH) {
> +	/* Read OV10640 ID to test communications. */
> +	for (i = 0; i < OV10640_PID_TIMEOUT; ++i) {
> +		ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR,
> +				OV490_SCCB_SLAVE_READ);
> +		ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH,
> +				OV10640_CHIP_ID >> 8);
> +		ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW,
> +				OV10640_CHIP_ID & 0xff);
> +
> +		/*
> +		 * Trigger SCCB slave transaction and give it some time
> +		 * to complete.
> +		 */
> +		ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> +		usleep_range(1000, 1500);
> +
> +		ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
> +		if (val == OV10640_ID_HIGH)
> +			break;
> +		usleep_range(1000, 1500);
> +	}
> +	if (i == OV10640_PID_TIMEOUT) {
>  		dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
>  		return -ENODEV;
>  	}
> @@ -374,6 +390,8 @@ static int ov490_initialize(struct rdacm21_device *dev)
>  	unsigned int i;
>  	int ret;
>  
> +	ov10640_power_up(dev);
> +
>  	/*
>  	 * Read OV490 Id to test communications. Give it up to 40msec to
>  	 * exit from reset.
> @@ -411,7 +429,7 @@ static int ov490_initialize(struct rdacm21_device *dev)
>  		return -ENODEV;
>  	}
>  
> -	ret = ov10640_initialize(dev);
> +	ret = ov10640_check_id(dev);
>  	if (ret)
>  		return ret;
>  

-- 
Regards,

Laurent Pinchart

[PATCH v4 11/17] media: i2c: rdacm20: Enable noise immunity

[Jacopo Mondi]

Enable the noise immunity threshold at the end of the rdacm20
initialization routine.

The rdacm20 camera module has been so far tested with a startup
delay that allowed the embedded MCU to program the serializer. If
the initialization routine is run before the MCU programs the
serializer and the image sensor and their addresses gets changed
by the rdacm20 driver it is required to manually enable the noise
immunity threshold to make the communication on the control channel
more reliable.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index c1a717153303..5e0314a2b1ca 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -539,7 +539,19 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 
 	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
 
-	return 0;
+	/*
+	 * Set reverse channel high threshold to increase noise immunity.
+	 *
+	 * This should be compensated by increasing the reverse channel
+	 * amplitude on the remote deserializer side.
+	 *
+	 * TODO Inspect the embedded MCU programming sequence to make sure
+	 * there are no conflicts with the configuration applied here.
+	 *
+	 * TODO Clarify the embedded MCU startup delay to avoid write
+	 * collisions on the I2C bus.
+	 */
+	return max9271_set_high_threshold(&dev->serializer, true);
 }
 
 static int rdacm20_probe(struct i2c_client *client)
-- 
2.31.1

Re: [PATCH v4 11/17] media: i2c: rdacm20: Enable noise immunity

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Apr 12, 2021 at 11:34:45AM +0200, Jacopo Mondi wrote:
> Enable the noise immunity threshold at the end of the rdacm20
> initialization routine.
> 
> The rdacm20 camera module has been so far tested with a startup
> delay that allowed the embedded MCU to program the serializer. If
> the initialization routine is run before the MCU programs the
> serializer and the image sensor and their addresses gets changed
> by the rdacm20 driver it is required to manually enable the noise
> immunity threshold to make the communication on the control channel
> more reliable.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/rdacm20.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index c1a717153303..5e0314a2b1ca 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -539,7 +539,19 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  
>  	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
>  
> -	return 0;
> +	/*
> +	 * Set reverse channel high threshold to increase noise immunity.
> +	 *
> +	 * This should be compensated by increasing the reverse channel
> +	 * amplitude on the remote deserializer side.
> +	 *
> +	 * TODO Inspect the embedded MCU programming sequence to make sure
> +	 * there are no conflicts with the configuration applied here.
> +	 *
> +	 * TODO Clarify the embedded MCU startup delay to avoid write
> +	 * collisions on the I2C bus.
> +	 */
> +	return max9271_set_high_threshold(&dev->serializer, true);
>  }
>  
>  static int rdacm20_probe(struct i2c_client *client)

-- 
Regards,

Laurent Pinchart

[PATCH v4 12/17] media: i2c: rdacm20: Embed 'serializer' field

[Jacopo Mondi]

There's no reason to allocate dynamically the 'serializer' field in
the driver structure.

Embed the field and adjust all its users in the driver.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 5e0314a2b1ca..029af8fd7485 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -312,7 +312,7 @@ static const struct ov10635_reg {
 
 struct rdacm20_device {
 	struct device			*dev;
-	struct max9271_device		*serializer;
+	struct max9271_device		serializer;
 	struct i2c_client		*sensor;
 	struct v4l2_subdev		sd;
 	struct media_pad		pad;
@@ -399,7 +399,7 @@ static int rdacm20_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct rdacm20_device *dev = sd_to_rdacm20(sd);
 
-	return max9271_set_serial_link(dev->serializer, enable);
+	return max9271_set_serial_link(&dev->serializer, enable);
 }
 
 static int rdacm20_enum_mbus_code(struct v4l2_subdev *sd,
@@ -455,10 +455,10 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	unsigned int retry = 3;
 	int ret;
 
-	max9271_wake_up(dev->serializer);
+	max9271_wake_up(&dev->serializer);
 
 	/* Serial link disabled during config as it needs a valid pixel clock. */
-	ret = max9271_set_serial_link(dev->serializer, false);
+	ret = max9271_set_serial_link(&dev->serializer, false);
 	if (ret)
 		return ret;
 
@@ -466,35 +466,35 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	 *  Ensure that we have a good link configuration before attempting to
 	 *  identify the device.
 	 */
-	max9271_configure_i2c(dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
-					       MAX9271_I2CSLVTO_1024US |
-					       MAX9271_I2CMSTBT_105KBPS);
+	max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
+						MAX9271_I2CSLVTO_1024US |
+						MAX9271_I2CMSTBT_105KBPS);
 
-	max9271_configure_gmsl_link(dev->serializer);
+	max9271_configure_gmsl_link(&dev->serializer);
 
-	ret = max9271_verify_id(dev->serializer);
+	ret = max9271_verify_id(&dev->serializer);
 	if (ret < 0)
 		return ret;
 
-	ret = max9271_set_address(dev->serializer, dev->addrs[0]);
+	ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
 	if (ret < 0)
 		return ret;
-	dev->serializer->client->addr = dev->addrs[0];
+	dev->serializer.client->addr = dev->addrs[0];
 
 	/*
 	 * Reset the sensor by cycling the OV10635 reset signal connected to the
 	 * MAX9271 GPIO1 and verify communication with the OV10635.
 	 */
-	ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
+	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
 
-	ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
+	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
 	usleep_range(10000, 15000);
 
-	ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
+	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
 	usleep_range(10000, 15000);
@@ -564,13 +564,7 @@ static int rdacm20_probe(struct i2c_client *client)
 	if (!dev)
 		return -ENOMEM;
 	dev->dev = &client->dev;
-
-	dev->serializer = devm_kzalloc(&client->dev, sizeof(*dev->serializer),
-				       GFP_KERNEL);
-	if (!dev->serializer)
-		return -ENOMEM;
-
-	dev->serializer->client = client;
+	dev->serializer.client = client;
 
 	ret = of_property_read_u32_array(client->dev.of_node, "reg",
 					 dev->addrs, 2);
-- 
2.31.1

[PATCH v4 13/17] media: i2c: rdacm20: Report camera module name

[Jacopo Mondi]

When the device is identified the driver currently reports the
names of the chips embedded in the camera module.

Report the name of the camera module itself instead.
Cosmetic change only.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 029af8fd7485..f1819bfa165d 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -537,7 +537,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	if (ret)
 		return ret;
 
-	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
+	dev_info(dev->dev, "Identified RDACM20 camera module\n");
 
 	/*
 	 * Set reverse channel high threshold to increase noise immunity.
-- 
2.31.1

[PATCH v4 14/17] media: i2c: rdacm20: Check return values

[Jacopo Mondi]

The camera module initialization routine does not check the return
value of a few functions. Fix that.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index f1819bfa165d..e6fed86a3281 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -466,11 +466,16 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	 *  Ensure that we have a good link configuration before attempting to
 	 *  identify the device.
 	 */
-	max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
-						MAX9271_I2CSLVTO_1024US |
-						MAX9271_I2CMSTBT_105KBPS);
+	ret = max9271_configure_i2c(&dev->serializer,
+				    MAX9271_I2CSLVSH_469NS_234NS |
+				    MAX9271_I2CSLVTO_1024US |
+				    MAX9271_I2CMSTBT_105KBPS);
+	if (ret)
+		return ret;
 
-	max9271_configure_gmsl_link(&dev->serializer);
+	ret = max9271_configure_gmsl_link(&dev->serializer);
+	if (ret)
+		return ret;
 
 	ret = max9271_verify_id(&dev->serializer);
 	if (ret < 0)
-- 
2.31.1

[PATCH v4 15/17] media: i2c: rdacm20: Re-work ov10635 reset

[Jacopo Mondi]

The OV10635 image sensor embedded in the camera module is currently
reset after the MAX9271 initialization with two long delays that were
most probably not correctly characterized.

Re-work the image sensor reset procedure by holding the chip in reset
during the MAX9271 configuration, removing the long sleep delays and
only wait after the chip exits from reset for 350-500 microseconds
interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
characterized in the chip manual.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index e6fed86a3281..cb725c2778c0 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -473,6 +473,19 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	if (ret)
 		return ret;
 
+	/*
+	 * Hold OV10635 in reset during max9271 configuration. The reset signal
+	 * has to be asserted for at least 200 microseconds.
+	 */
+	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
+	if (ret)
+		return ret;
+
+	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
+	if (ret)
+		return ret;
+	usleep_range(200, 500);
+
 	ret = max9271_configure_gmsl_link(&dev->serializer);
 	if (ret)
 		return ret;
@@ -487,22 +500,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	dev->serializer.client->addr = dev->addrs[0];
 
 	/*
-	 * Reset the sensor by cycling the OV10635 reset signal connected to the
-	 * MAX9271 GPIO1 and verify communication with the OV10635.
+	 * Release ov10635 from reset and initialize it. The image sensor
+	 * requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz)
+	 * before being available. Stay safe and wait up to 500 micro-seconds.
 	 */
-	ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
-	if (ret)
-		return ret;
-
-	ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
-	if (ret)
-		return ret;
-	usleep_range(10000, 15000);
-
 	ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
 	if (ret)
 		return ret;
-	usleep_range(10000, 15000);
+	usleep_range(100, 500);
 
 again:
 	ret = ov10635_read16(dev, OV10635_PID);
-- 
2.31.1

[PATCH v4 16/17] media: v4l2-subdev: De-deprecate init() subdev op

[Jacopo Mondi]

The init() subdev core operation is deemed to be deprecated for new
subdevice drivers. However it could prove useful for complex
architectures to defer operation that require access to the
communication bus if said bus is not available (or fully configured)
at the time when the subdevice probe() function is run.

As an example, the GMSL architecture requires the GMSL configuration
link to be configured on the host side after the remote subdevice
has completed its probe function. After the configuration on the host
side has been performed, the subdevice registers can be accessed through
the communication bus.

In particular:

	HOST			REMOTE

	probe()
	   |
	   ---------------------> |
				  probe() {
				     bus config()
				  }
	   |<--------------------|
	v4l2 async bound {
	    bus config()
	    call subdev init()
	   |-------------------->|
				 init() {
				     access register on the bus()
				}
	   |<-------------------
	}

In the GMSL use case the bus configuration requires the enablement of the
noise immunity threshold on the remote side which ensures reliability
of communications in electrically noisy environments. After the subdevice
has enabled the threshold at the end of its probe() sequence the host
side shall compensate it with an higher signal amplitude. Once this
sequence has completed the bus can be accessed with noise protection
enabled and all the operations that require a considerable number of
transactions on the bus (such as the image sensor configuration
sequence) are run in the subdevice init() operation implementation.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d0e9a5bdb08b..3068d9940669 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
  *	each pin being configured.  This function could be called at times
  *	other than just subdevice initialization.
  *
- * @init: initialize the sensor registers to some sort of reasonable default
- *	values. Do not use for new drivers and should be removed in existing
- *	drivers.
+ * @init: initialize the subdevice registers to some sort of reasonable default
+ *	values. Do not use for new drivers (and should be removed in existing
+ *	ones) for regular architectures where the image sensor is connected to
+ *	the host receiver. For more complex architectures where the subdevice
+ *	initialization should be deferred to the completion of the probe
+ *	sequence of some intermediate component, or the communication bus
+ *	requires configurations on the host side that depend on the completion
+ *	of the probe sequence of the remote subdevices, the usage of this
+ *	operation could be considered to allow the devices along the pipeline to
+ *	probe and register in the media graph and to defer any operation that
+ *	require actual access to the communication bus to their init() function
+ *	implementation.
  *
  * @load_fw: load firmware.
  *
--
2.31.1

Re: [PATCH v4 16/17] media: v4l2-subdev: De-deprecate init() subdev op

[Jacopo Mondi]

Hi,

with feedback from media maintainers I can resend the series which is
now fully reviewed.

Thanks
   j

On Mon, Apr 12, 2021 at 11:34:50AM +0200, Jacopo Mondi wrote:
> The init() subdev core operation is deemed to be deprecated for new
> subdevice drivers. However it could prove useful for complex
> architectures to defer operation that require access to the
> communication bus if said bus is not available (or fully configured)
> at the time when the subdevice probe() function is run.
>
> As an example, the GMSL architecture requires the GMSL configuration
> link to be configured on the host side after the remote subdevice
> has completed its probe function. After the configuration on the host
> side has been performed, the subdevice registers can be accessed through
> the communication bus.
>
> In particular:
>
> 	HOST			REMOTE
>
> 	probe()
> 	   |
> 	   ---------------------> |
> 				  probe() {
> 				     bus config()
> 				  }
> 	   |<--------------------|
> 	v4l2 async bound {
> 	    bus config()
> 	    call subdev init()
> 	   |-------------------->|
> 				 init() {
> 				     access register on the bus()
> 				}
> 	   |<-------------------
> 	}
>
> In the GMSL use case the bus configuration requires the enablement of the
> noise immunity threshold on the remote side which ensures reliability
> of communications in electrically noisy environments. After the subdevice
> has enabled the threshold at the end of its probe() sequence the host
> side shall compensate it with an higher signal amplitude. Once this
> sequence has completed the bus can be accessed with noise protection
> enabled and all the operations that require a considerable number of
> transactions on the bus (such as the image sensor configuration
> sequence) are run in the subdevice init() operation implementation.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d0e9a5bdb08b..3068d9940669 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
>   *	each pin being configured.  This function could be called at times
>   *	other than just subdevice initialization.
>   *
> - * @init: initialize the sensor registers to some sort of reasonable default
> - *	values. Do not use for new drivers and should be removed in existing
> - *	drivers.
> + * @init: initialize the subdevice registers to some sort of reasonable default
> + *	values. Do not use for new drivers (and should be removed in existing
> + *	ones) for regular architectures where the image sensor is connected to
> + *	the host receiver. For more complex architectures where the subdevice
> + *	initialization should be deferred to the completion of the probe
> + *	sequence of some intermediate component, or the communication bus
> + *	requires configurations on the host side that depend on the completion
> + *	of the probe sequence of the remote subdevices, the usage of this
> + *	operation could be considered to allow the devices along the pipeline to
> + *	probe and register in the media graph and to defer any operation that
> + *	require actual access to the communication bus to their init() function
> + *	implementation.
>   *
>   * @load_fw: load firmware.
>   *
> --
> 2.31.1
>

Re: [PATCH v4 16/17] media: v4l2-subdev: De-deprecate init() subdev op

[Jacopo Mondi]

Hello linux-media,

On Mon, Apr 12, 2021 at 11:34:50AM +0200, Jacopo Mondi wrote:
> The init() subdev core operation is deemed to be deprecated for new
> subdevice drivers. However it could prove useful for complex
> architectures to defer operation that require access to the
> communication bus if said bus is not available (or fully configured)
> at the time when the subdevice probe() function is run.
>
> As an example, the GMSL architecture requires the GMSL configuration
> link to be configured on the host side after the remote subdevice
> has completed its probe function. After the configuration on the host
> side has been performed, the subdevice registers can be accessed through
> the communication bus.
>
> In particular:
>
> 	HOST			REMOTE
>
> 	probe()
> 	   |
> 	   ---------------------> |
> 				  probe() {
> 				     bus config()
> 				  }
> 	   |<--------------------|
> 	v4l2 async bound {
> 	    bus config()
> 	    call subdev init()
> 	   |-------------------->|
> 				 init() {
> 				     access register on the bus()
> 				}
> 	   |<-------------------
> 	}
>
> In the GMSL use case the bus configuration requires the enablement of the
> noise immunity threshold on the remote side which ensures reliability
> of communications in electrically noisy environments. After the subdevice
> has enabled the threshold at the end of its probe() sequence the host
> side shall compensate it with an higher signal amplitude. Once this
> sequence has completed the bus can be accessed with noise protection
> enabled and all the operations that require a considerable number of
> transactions on the bus (such as the image sensor configuration
> sequence) are run in the subdevice init() operation implementation.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

This change is key for the whole series to get in, and requires the
approval of linux-media maintainers as it use a function now deemed as
deprecated.

Could I get an ack/nack please ?

Thanks
   j

> ---
>  include/media/v4l2-subdev.h | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d0e9a5bdb08b..3068d9940669 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
>   *	each pin being configured.  This function could be called at times
>   *	other than just subdevice initialization.
>   *
> - * @init: initialize the sensor registers to some sort of reasonable default
> - *	values. Do not use for new drivers and should be removed in existing
> - *	drivers.
> + * @init: initialize the subdevice registers to some sort of reasonable default
> + *	values. Do not use for new drivers (and should be removed in existing
> + *	ones) for regular architectures where the image sensor is connected to
> + *	the host receiver. For more complex architectures where the subdevice
> + *	initialization should be deferred to the completion of the probe
> + *	sequence of some intermediate component, or the communication bus
> + *	requires configurations on the host side that depend on the completion
> + *	of the probe sequence of the remote subdevices, the usage of this
> + *	operation could be considered to allow the devices along the pipeline to
> + *	probe and register in the media graph and to defer any operation that
> + *	require actual access to the communication bus to their init() function
> + *	implementation.
>   *
>   * @load_fw: load firmware.
>   *
> --
> 2.31.1
>

Re: [PATCH v4 16/17] media: v4l2-subdev: De-deprecate init() subdev op

[Hans Verkuil]

On 12/04/2021 11:34, Jacopo Mondi wrote:
> The init() subdev core operation is deemed to be deprecated for new
> subdevice drivers. However it could prove useful for complex
> architectures to defer operation that require access to the
> communication bus if said bus is not available (or fully configured)
> at the time when the subdevice probe() function is run.
> 
> As an example, the GMSL architecture requires the GMSL configuration
> link to be configured on the host side after the remote subdevice
> has completed its probe function. After the configuration on the host
> side has been performed, the subdevice registers can be accessed through
> the communication bus.
> 
> In particular:
> 
> 	HOST			REMOTE
> 
> 	probe()
> 	   |
> 	   ---------------------> |
> 				  probe() {
> 				     bus config()
> 				  }
> 	   |<--------------------|
> 	v4l2 async bound {
> 	    bus config()
> 	    call subdev init()
> 	   |-------------------->|
> 				 init() {
> 				     access register on the bus()
> 				}
> 	   |<-------------------
> 	}
> 
> In the GMSL use case the bus configuration requires the enablement of the
> noise immunity threshold on the remote side which ensures reliability
> of communications in electrically noisy environments. After the subdevice
> has enabled the threshold at the end of its probe() sequence the host
> side shall compensate it with an higher signal amplitude. Once this
> sequence has completed the bus can be accessed with noise protection
> enabled and all the operations that require a considerable number of
> transactions on the bus (such as the image sensor configuration
> sequence) are run in the subdevice init() operation implementation.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d0e9a5bdb08b..3068d9940669 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
>   *	each pin being configured.  This function could be called at times
>   *	other than just subdevice initialization.
>   *
> - * @init: initialize the sensor registers to some sort of reasonable default
> - *	values. Do not use for new drivers and should be removed in existing
> - *	drivers.
> + * @init: initialize the subdevice registers to some sort of reasonable default
> + *	values. Do not use for new drivers (and should be removed in existing
> + *	ones) for regular architectures where the image sensor is connected to
> + *	the host receiver. For more complex architectures where the subdevice
> + *	initialization should be deferred to the completion of the probe
> + *	sequence of some intermediate component, or the communication bus
> + *	requires configurations on the host side that depend on the completion
> + *	of the probe sequence of the remote subdevices, the usage of this
> + *	operation could be considered to allow the devices along the pipeline to
> + *	probe and register in the media graph and to defer any operation that
> + *	require actual access to the communication bus to their init() function
> + *	implementation.

I don't like de-deprecating init. It was deprecated for a good reason, and
I'd like to keep it that way.

There are two alternatives: one is a bit quick-and-dirty, the other is a hint
towards a more generic solution (just a hint since it will require more research):

1) Quick-and-dirty: use the core callback op to create a custom INIT callback.
This depends on this patch:

https://patchwork.linuxtv.org/project/linux-media/patch/20210610214305.4170835-8-arnd@kernel.org/

This will make it clear to the reader that this is a highly specific interaction
between two drivers that are tightly coupled. It works in the current situation,
but not if we want to make this more generic.

2) Subdev drivers can implement the registered() op which is called by
v4l2_device_register_subdev(). This in turn is called from v4l2_async_match_notify().

What you want is that when max9286 calls v4l2_async_subdev_notifier_register, it
can set a flag or something indicating that initialization has to be postponed.
Then, when v4l2_async_match_notify() calls the register() callback, that flag can
be read. If false, then the register() callback will initialize the device, if
true then that won't happen. Instead, it will do that when the max9286 calls a
post_register() callback.

This is a lot more work (and research, since this is just a brainstorm from my
side), but it is a way towards making this a generic solution.

Regards,

	Hans

>   *
>   * @load_fw: load firmware.
>   *
> --
> 2.31.1
> 

Re: [PATCH v4 16/17] media: v4l2-subdev: De-deprecate init() subdev op

[Jacopo Mondi]

Hi Hans,
   thanks for your reply

On Mon, Jun 14, 2021 at 10:51:25AM +0200, Hans Verkuil wrote:
> On 12/04/2021 11:34, Jacopo Mondi wrote:
> > The init() subdev core operation is deemed to be deprecated for new
> > subdevice drivers. However it could prove useful for complex
> > architectures to defer operation that require access to the
> > communication bus if said bus is not available (or fully configured)
> > at the time when the subdevice probe() function is run.
> >
> > As an example, the GMSL architecture requires the GMSL configuration
> > link to be configured on the host side after the remote subdevice
> > has completed its probe function. After the configuration on the host
> > side has been performed, the subdevice registers can be accessed through
> > the communication bus.
> >
> > In particular:
> >
> > 	HOST			REMOTE
> >
> > 	probe()
> > 	   |
> > 	   ---------------------> |
> > 				  probe() {
> > 				     bus config()
> > 				  }
> > 	   |<--------------------|
> > 	v4l2 async bound {
> > 	    bus config()
> > 	    call subdev init()
> > 	   |-------------------->|
> > 				 init() {
> > 				     access register on the bus()
> > 				}
> > 	   |<-------------------
> > 	}
> >
> > In the GMSL use case the bus configuration requires the enablement of the
> > noise immunity threshold on the remote side which ensures reliability
> > of communications in electrically noisy environments. After the subdevice
> > has enabled the threshold at the end of its probe() sequence the host
> > side shall compensate it with an higher signal amplitude. Once this
> > sequence has completed the bus can be accessed with noise protection
> > enabled and all the operations that require a considerable number of
> > transactions on the bus (such as the image sensor configuration
> > sequence) are run in the subdevice init() operation implementation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index d0e9a5bdb08b..3068d9940669 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> >   *	each pin being configured.  This function could be called at times
> >   *	other than just subdevice initialization.
> >   *
> > - * @init: initialize the sensor registers to some sort of reasonable default
> > - *	values. Do not use for new drivers and should be removed in existing
> > - *	drivers.
> > + * @init: initialize the subdevice registers to some sort of reasonable default
> > + *	values. Do not use for new drivers (and should be removed in existing
> > + *	ones) for regular architectures where the image sensor is connected to
> > + *	the host receiver. For more complex architectures where the subdevice
> > + *	initialization should be deferred to the completion of the probe
> > + *	sequence of some intermediate component, or the communication bus
> > + *	requires configurations on the host side that depend on the completion
> > + *	of the probe sequence of the remote subdevices, the usage of this
> > + *	operation could be considered to allow the devices along the pipeline to
> > + *	probe and register in the media graph and to defer any operation that
> > + *	require actual access to the communication bus to their init() function
> > + *	implementation.
>
> I don't like de-deprecating init. It was deprecated for a good reason, and
> I'd like to keep it that way.

I see, fair enough :)

>
> There are two alternatives: one is a bit quick-and-dirty, the other is a hint
> towards a more generic solution (just a hint since it will require more research):
>
> 1) Quick-and-dirty: use the core callback op to create a custom INIT callback.
> This depends on this patch:
>
> https://patchwork.linuxtv.org/project/linux-media/patch/20210610214305.4170835-8-arnd@kernel.org/
>
> This will make it clear to the reader that this is a highly specific interaction
> between two drivers that are tightly coupled. It works in the current situation,
> but not if we want to make this more generic.

Depends what you mean with 'generic' :) I think such a solution would
slightly abuse a generic API like 'command' is, but the GMSL
deserializers/serializers are tighly coupled by definition, so this is
less a concern, as long as we have a single driver for the whole
camera module. If we're going to split it in 3 subdev drivers then
yes, they will all have to implement .command() and they can be used
with in isolation with a generic receiver driver.

>
> 2) Subdev drivers can implement the registered() op which is called by
> v4l2_device_register_subdev(). This in turn is called from v4l2_async_match_notify().
>
> What you want is that when max9286 calls v4l2_async_subdev_notifier_register, it
> can set a flag or something indicating that initialization has to be postponed.
> Then, when v4l2_async_match_notify() calls the register() callback, that flag can
> be read. If false, then the register() callback will initialize the device, if
> true then that won't happen. Instead, it will do that when the max9286 calls a
> post_register() callback.

2 questions to help me better understand this:
1) s/register()/registered() in this paragraph ?
2) $ git grep post_register drivers/media/ include/media/
   gives me back nothing.

   Are you suggesting a new operation ?

Thanks
   j

>
> This is a lot more work (and research, since this is just a brainstorm from my
> side), but it is a way towards making this a generic solution.
>
> Regards,
>
> 	Hans
>
> >   *
> >   * @load_fw: load firmware.
> >   *
> > --
> > 2.31.1
> >
>

Re: [PATCH v4 16/17] media: v4l2-subdev: De-deprecate init() subdev op

[Hans Verkuil]

On 14/06/2021 11:45, Jacopo Mondi wrote:
> Hi Hans,
>    thanks for your reply
> 
> On Mon, Jun 14, 2021 at 10:51:25AM +0200, Hans Verkuil wrote:
>> On 12/04/2021 11:34, Jacopo Mondi wrote:
>>> The init() subdev core operation is deemed to be deprecated for new
>>> subdevice drivers. However it could prove useful for complex
>>> architectures to defer operation that require access to the
>>> communication bus if said bus is not available (or fully configured)
>>> at the time when the subdevice probe() function is run.
>>>
>>> As an example, the GMSL architecture requires the GMSL configuration
>>> link to be configured on the host side after the remote subdevice
>>> has completed its probe function. After the configuration on the host
>>> side has been performed, the subdevice registers can be accessed through
>>> the communication bus.
>>>
>>> In particular:
>>>
>>> 	HOST			REMOTE
>>>
>>> 	probe()
>>> 	   |
>>> 	   ---------------------> |
>>> 				  probe() {
>>> 				     bus config()
>>> 				  }
>>> 	   |<--------------------|
>>> 	v4l2 async bound {
>>> 	    bus config()
>>> 	    call subdev init()
>>> 	   |-------------------->|
>>> 				 init() {
>>> 				     access register on the bus()
>>> 				}
>>> 	   |<-------------------
>>> 	}
>>>
>>> In the GMSL use case the bus configuration requires the enablement of the
>>> noise immunity threshold on the remote side which ensures reliability
>>> of communications in electrically noisy environments. After the subdevice
>>> has enabled the threshold at the end of its probe() sequence the host
>>> side shall compensate it with an higher signal amplitude. Once this
>>> sequence has completed the bus can be accessed with noise protection
>>> enabled and all the operations that require a considerable number of
>>> transactions on the bus (such as the image sensor configuration
>>> sequence) are run in the subdevice init() operation implementation.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  include/media/v4l2-subdev.h | 15 ++++++++++++---
>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>> index d0e9a5bdb08b..3068d9940669 100644
>>> --- a/include/media/v4l2-subdev.h
>>> +++ b/include/media/v4l2-subdev.h
>>> @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
>>>   *	each pin being configured.  This function could be called at times
>>>   *	other than just subdevice initialization.
>>>   *
>>> - * @init: initialize the sensor registers to some sort of reasonable default
>>> - *	values. Do not use for new drivers and should be removed in existing
>>> - *	drivers.
>>> + * @init: initialize the subdevice registers to some sort of reasonable default
>>> + *	values. Do not use for new drivers (and should be removed in existing
>>> + *	ones) for regular architectures where the image sensor is connected to
>>> + *	the host receiver. For more complex architectures where the subdevice
>>> + *	initialization should be deferred to the completion of the probe
>>> + *	sequence of some intermediate component, or the communication bus
>>> + *	requires configurations on the host side that depend on the completion
>>> + *	of the probe sequence of the remote subdevices, the usage of this
>>> + *	operation could be considered to allow the devices along the pipeline to
>>> + *	probe and register in the media graph and to defer any operation that
>>> + *	require actual access to the communication bus to their init() function
>>> + *	implementation.
>>
>> I don't like de-deprecating init. It was deprecated for a good reason, and
>> I'd like to keep it that way.
> 
> I see, fair enough :)
> 
>>
>> There are two alternatives: one is a bit quick-and-dirty, the other is a hint
>> towards a more generic solution (just a hint since it will require more research):
>>
>> 1) Quick-and-dirty: use the core callback op to create a custom INIT callback.
>> This depends on this patch:
>>
>> https://patchwork.linuxtv.org/project/linux-media/patch/20210610214305.4170835-8-arnd@kernel.org/
>>
>> This will make it clear to the reader that this is a highly specific interaction
>> between two drivers that are tightly coupled. It works in the current situation,
>> but not if we want to make this more generic.
> 
> Depends what you mean with 'generic' :) I think such a solution would
> slightly abuse a generic API like 'command' is, but the GMSL
> deserializers/serializers are tighly coupled by definition, so this is
> less a concern, as long as we have a single driver for the whole
> camera module. If we're going to split it in 3 subdev drivers then
> yes, they will all have to implement .command() and they can be used
> with in isolation with a generic receiver driver.
> 
>>
>> 2) Subdev drivers can implement the registered() op which is called by
>> v4l2_device_register_subdev(). This in turn is called from v4l2_async_match_notify().
>>
>> What you want is that when max9286 calls v4l2_async_subdev_notifier_register, it
>> can set a flag or something indicating that initialization has to be postponed.
>> Then, when v4l2_async_match_notify() calls the register() callback, that flag can
>> be read. If false, then the register() callback will initialize the device, if
>> true then that won't happen. Instead, it will do that when the max9286 calls a
>> post_register() callback.
> 
> 2 questions to help me better understand this:
> 1) s/register()/registered() in this paragraph ?

Yes, sorry.

> 2) $ git grep post_register drivers/media/ include/media/
>    gives me back nothing.
> 
>    Are you suggesting a new operation ?

Yes, that would be a new op.

Regards,

	Hans

> 
> Thanks
>    j
> 
>>
>> This is a lot more work (and research, since this is just a brainstorm from my
>> side), but it is a way towards making this a generic solution.
>>
>> Regards,
>>
>> 	Hans
>>
>>>   *
>>>   * @load_fw: load firmware.
>>>   *
>>> --
>>> 2.31.1
>>>
>>

Re: [PATCH v4 16/17] media: v4l2-subdev: De-deprecate init() subdev op

[Jacopo Mondi]

Hi again Hans,
   I pondered a bit on your suggestion and I leave these here as
a potential discussion starter

On Mon, Jun 14, 2021 at 11:55:30AM +0200, Hans Verkuil wrote:
> On 14/06/2021 11:45, Jacopo Mondi wrote:
> > Hi Hans,
> >    thanks for your reply
> >
> > On Mon, Jun 14, 2021 at 10:51:25AM +0200, Hans Verkuil wrote:
> >> On 12/04/2021 11:34, Jacopo Mondi wrote:
> >>> The init() subdev core operation is deemed to be deprecated for new
> >>> subdevice drivers. However it could prove useful for complex
> >>> architectures to defer operation that require access to the
> >>> communication bus if said bus is not available (or fully configured)
> >>> at the time when the subdevice probe() function is run.
> >>>
> >>> As an example, the GMSL architecture requires the GMSL configuration
> >>> link to be configured on the host side after the remote subdevice
> >>> has completed its probe function. After the configuration on the host
> >>> side has been performed, the subdevice registers can be accessed through
> >>> the communication bus.
> >>>
> >>> In particular:
> >>>
> >>> 	HOST			REMOTE
> >>>
> >>> 	probe()
> >>> 	   |
> >>> 	   ---------------------> |
> >>> 				  probe() {
> >>> 				     bus config()
> >>> 				  }
> >>> 	   |<--------------------|
> >>> 	v4l2 async bound {
> >>> 	    bus config()
> >>> 	    call subdev init()
> >>> 	   |-------------------->|
> >>> 				 init() {
> >>> 				     access register on the bus()
> >>> 				}
> >>> 	   |<-------------------
> >>> 	}
> >>>
> >>> In the GMSL use case the bus configuration requires the enablement of the
> >>> noise immunity threshold on the remote side which ensures reliability
> >>> of communications in electrically noisy environments. After the subdevice
> >>> has enabled the threshold at the end of its probe() sequence the host
> >>> side shall compensate it with an higher signal amplitude. Once this
> >>> sequence has completed the bus can be accessed with noise protection
> >>> enabled and all the operations that require a considerable number of
> >>> transactions on the bus (such as the image sensor configuration
> >>> sequence) are run in the subdevice init() operation implementation.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>  include/media/v4l2-subdev.h | 15 ++++++++++++---
> >>>  1 file changed, 12 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>> index d0e9a5bdb08b..3068d9940669 100644
> >>> --- a/include/media/v4l2-subdev.h
> >>> +++ b/include/media/v4l2-subdev.h
> >>> @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> >>>   *	each pin being configured.  This function could be called at times
> >>>   *	other than just subdevice initialization.
> >>>   *
> >>> - * @init: initialize the sensor registers to some sort of reasonable default
> >>> - *	values. Do not use for new drivers and should be removed in existing
> >>> - *	drivers.
> >>> + * @init: initialize the subdevice registers to some sort of reasonable default
> >>> + *	values. Do not use for new drivers (and should be removed in existing
> >>> + *	ones) for regular architectures where the image sensor is connected to
> >>> + *	the host receiver. For more complex architectures where the subdevice
> >>> + *	initialization should be deferred to the completion of the probe
> >>> + *	sequence of some intermediate component, or the communication bus
> >>> + *	requires configurations on the host side that depend on the completion
> >>> + *	of the probe sequence of the remote subdevices, the usage of this
> >>> + *	operation could be considered to allow the devices along the pipeline to
> >>> + *	probe and register in the media graph and to defer any operation that
> >>> + *	require actual access to the communication bus to their init() function
> >>> + *	implementation.
> >>
> >> I don't like de-deprecating init. It was deprecated for a good reason, and
> >> I'd like to keep it that way.
> >
> > I see, fair enough :)
> >
> >>
> >> There are two alternatives: one is a bit quick-and-dirty, the other is a hint
> >> towards a more generic solution (just a hint since it will require more research):
> >>
> >> 1) Quick-and-dirty: use the core callback op to create a custom INIT callback.
> >> This depends on this patch:
> >>
> >> https://patchwork.linuxtv.org/project/linux-media/patch/20210610214305.4170835-8-arnd@kernel.org/
> >>
> >> This will make it clear to the reader that this is a highly specific interaction
> >> between two drivers that are tightly coupled. It works in the current situation,
> >> but not if we want to make this more generic.
> >
> > Depends what you mean with 'generic' :) I think such a solution would
> > slightly abuse a generic API like 'command' is, but the GMSL
> > deserializers/serializers are tighly coupled by definition, so this is
> > less a concern, as long as we have a single driver for the whole
> > camera module. If we're going to split it in 3 subdev drivers then
> > yes, they will all have to implement .command() and they can be used
> > with in isolation with a generic receiver driver.
> >
> >>
> >> 2) Subdev drivers can implement the registered() op which is called by
> >> v4l2_device_register_subdev(). This in turn is called from v4l2_async_match_notify().
> >>
> >> What you want is that when max9286 calls v4l2_async_subdev_notifier_register, it
> >> can set a flag or something indicating that initialization has to be postponed.
> >> Then, when v4l2_async_match_notify() calls the register() callback, that flag can
> >> be read. If false, then the register() callback will initialize the device, if
> >> true then that won't happen. Instead, it will do that when the max9286 calls a
> >> post_register() callback.
> >
> > 2 questions to help me better understand this:
> > 1) s/register()/registered() in this paragraph ?
>
> Yes, sorry.
>
> > 2) $ git grep post_register drivers/media/ include/media/
> >    gives me back nothing.
> >
> >    Are you suggesting a new operation ?
>
> Yes, that would be a new op.

Thanks for the clarification. Let me start this by setting up the
goals I would like to reach:

- Conditionally allow to run later parts of the probe/initialization
  routine of a subdevice: a subdevice driver should be made capable to
  post-pone the execution of parts (if not all) of its
  probe/initialization routines to a later point in time, in example,
  when all other subdevices on the same bus have been probed.

  In the context of the use case at hand (GMSL) this means deferring
  the completion of the camera devices initialization sequence that
  heavily access the i2c-over-GMSL channel to a later time when all
  others camera modules have probed and have enabled their high-noise
  immunity so that the deserializer can now increase the channel signal
  amplitude.

- Initialization deferring should be made conditional so that when
  used when host receivers that has not deferring requirements the
  subdevice drivers work without changes

Implementing such a solution requires two mechanisms in place:
A) A way to discern if initialization should be deferred
b) A callback mechanism that automatically executes at the end of the
   probe sequences

In detail:

A)
Can be made protocol dependent. I haven't been able to figure out a
generic way for a host receiver to set a flag on the subdev to be
inspected at probe/registered() time. Assuming such a generic mechanism
does not exist, a per-use-case solution can be considered. In example
for GMSL we have a deserializer property that specifies the initial
amplitude of the control channel. If it's lower that a certain
threshold it has to be increased at 'complete' time. The property can
be duplicated on the camera module side such that:
- If the channel amplitude is low, we know initialization should be
  post-poned
- If it's high already (or not present for non-GMSL use case)
  initialization can be performed at probe() time.

This is a very good match for GMSL. The initial deserializer channel
amplitude only depend on the connected camera module. In example
RDACM20 comes pre-programmed by an on-board MCU such that the deser
can be started with high amplitude and the camera module can be
programmed at probe() time. RDACM21 requires the deser to be started
with low amp, all camera modules have to complete probe and enable
their noise threshold so that the deser amplitude can be increased.
The only drawback I see is that the same property should be specified
in both endpoints. This happens already, in example, for the media bus
configuration endpoint properties.

B)
The registered() internal callback is executed immediately once the
async subdev has been matched against a notifier. For the use case at
hand it is not useful as it does not allow to return control to the
host driver before calling into the subdev.  The "right" time to
trigger deferred initialization could be once the whole chain of async
subdev and notifiers has completed, and we already have notifier ops
complete() callback for that purpose, only executed on the root
notifier. Adding a 'complete()' internal operation to be called on the
list of the top v4l2_dev subdevices at the same time (or replace the
complete notifier operation completely) might a good way forward ?

Sorry for the long email, it mostly served me to clarify my thoughts.

Thanks
   j

>
> Regards,
>
> 	Hans
>
> >
> > Thanks
> >    j
> >
> >>
> >> This is a lot more work (and research, since this is just a brainstorm from my
> >> side), but it is a way towards making this a generic solution.
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >>>   *
> >>>   * @load_fw: load firmware.
> >>>   *
> >>> --
> >>> 2.31.1
> >>>
> >>
>

[PATCH v4 17/17] media: gmsl: Reimplement initialization sequence

[Jacopo Mondi]

The current probe() procedure of the RDACM20 and RDACM21 GMSL cameras is
performed with the embedded MAX9271 serializer's noise immunity
threshold disabled. Once the camera has been initialized by probing the
embedded chips, the threshold is enabled and then compensated on the
deserializer's side by increasing the reverse channel signal amplitude
once all cameras have bound.

The probe routine is thus run without noise immunity activated which
in noisy environment conditions makes the probe sequence less reliable as
the chips configuration requires a relatively high amount of i2c
transactions.

Break chip initialization in two:
- At probe time only configure the serializer's reverse channel with
  noise immunity activated, to reduce the number of transactions
  performed without noise immunity protection enabled
- Move the chips initialization to the .init() core subdev operation
  called by the deserializer after all camera have probed and
  have increased their noise immunity threshold

The initialization routine looks like the following:

            MAX9286                  RDACM20/21

            probe()
               |
               ---------------------> |
                                      probe() {
                                         enable_threshold()
                                      }
               |<--------------------|
            v4l2 async bound {
                compensate_amplitude()
                call subdev init()
               |-------------------->|
                                     init() {
                                         access camera registers()
                                    }
               |<-------------------
            }

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 19 +++++++++---
 drivers/media/i2c/rdacm20.c | 59 ++++++++++++++++++++++---------------
 drivers/media/i2c/rdacm21.c | 59 ++++++++++++++++++++++---------------
 3 files changed, 87 insertions(+), 50 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index e1c7173f2d00..aafe55f30a0a 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -563,17 +563,28 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	if (priv->bound_sources != priv->source_mask)
 		return 0;

+	/*
+	 * Initialize all the remote camera. Increase the channel amplitude
+	 * to compensate for the remote noise immunity threshold.
+	 */
+	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
+	for_each_source(priv, source) {
+		ret = v4l2_subdev_call(source->sd, core, init, 0);
+		if (ret) {
+			dev_err(&priv->client->dev,
+				"Failed to initialize camera device %u\n",
+				index);
+			return ret;
+		}
+	}
+
 	/*
 	 * All enabled sources have probed and enabled their reverse control
 	 * channels:
-	 *
-	 * - Increase the reverse channel amplitude to compensate for the
-	 *   remote ends high threshold
 	 * - Verify all configuration links are properly detected
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
-	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
 	max9286_check_config_link(priv, priv->source_mask);
 	max9286_configure_i2c(priv, false);

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index cb725c2778c0..ce3e05ca6389 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -435,33 +435,12 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }

-static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
-	.s_stream	= rdacm20_s_stream,
-};
-
-static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
-	.enum_mbus_code = rdacm20_enum_mbus_code,
-	.get_fmt	= rdacm20_get_fmt,
-	.set_fmt	= rdacm20_get_fmt,
-};
-
-static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
-	.video		= &rdacm20_video_ops,
-	.pad		= &rdacm20_subdev_pad_ops,
-};
-
-static int rdacm20_initialize(struct rdacm20_device *dev)
+static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
 {
+	struct rdacm20_device *dev = sd_to_rdacm20(sd);
 	unsigned int retry = 3;
 	int ret;

-	max9271_wake_up(&dev->serializer);
-
-	/* Serial link disabled during config as it needs a valid pixel clock. */
-	ret = max9271_set_serial_link(&dev->serializer, false);
-	if (ret)
-		return ret;
-
 	/*
 	 *  Ensure that we have a good link configuration before attempting to
 	 *  identify the device.
@@ -549,6 +528,40 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

 	dev_info(dev->dev, "Identified RDACM20 camera module\n");

+	return 0;
+}
+
+static const struct v4l2_subdev_core_ops rdacm20_core_ops = {
+	.init           = rdacm20_init,
+};
+
+static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
+	.s_stream	= rdacm20_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
+	.enum_mbus_code = rdacm20_enum_mbus_code,
+	.get_fmt	= rdacm20_get_fmt,
+	.set_fmt	= rdacm20_get_fmt,
+};
+
+static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
+	.core		= &rdacm20_core_ops,
+	.video		= &rdacm20_video_ops,
+	.pad		= &rdacm20_subdev_pad_ops,
+};
+
+static int rdacm20_initialize(struct rdacm20_device *dev)
+{
+	int ret;
+
+	max9271_wake_up(&dev->serializer);
+
+	/* Serial link disabled during config as it needs a valid pixel clock. */
+	ret = max9271_set_serial_link(&dev->serializer, false);
+	if (ret)
+		return ret;
+
 	/*
 	 * Set reverse channel high threshold to increase noise immunity.
 	 *
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 43c41cb800a4..7232722e59cd 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -315,21 +315,6 @@ static int rdacm21_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }

-static const struct v4l2_subdev_video_ops rdacm21_video_ops = {
-	.s_stream	= rdacm21_s_stream,
-};
-
-static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
-	.enum_mbus_code = rdacm21_enum_mbus_code,
-	.get_fmt	= rdacm21_get_fmt,
-	.set_fmt	= rdacm21_get_fmt,
-};
-
-static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
-	.video		= &rdacm21_video_ops,
-	.pad		= &rdacm21_subdev_pad_ops,
-};
-
 static void ov10640_power_up(struct rdacm21_device *dev)
 {
 	/* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
@@ -470,17 +455,11 @@ static int ov490_initialize(struct rdacm21_device *dev)
 	return 0;
 }

-static int rdacm21_initialize(struct rdacm21_device *dev)
+static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
 {
+	struct rdacm21_device *dev = sd_to_rdacm21(sd);
 	int ret;

-	max9271_wake_up(&dev->serializer);
-
-	/* Enable reverse channel and disable the serial link. */
-	ret = max9271_set_serial_link(&dev->serializer, false);
-	if (ret)
-		return ret;
-
 	/* Configure I2C bus at 105Kbps speed and configure GMSL. */
 	ret = max9271_configure_i2c(&dev->serializer,
 				    MAX9271_I2CSLVSH_469NS_234NS |
@@ -531,6 +510,40 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
 	if (ret)
 		return ret;

+	return 0;
+}
+
+static const struct v4l2_subdev_core_ops rdacm21_core_ops = {
+	.init		= rdacm21_init,
+};
+
+static const struct v4l2_subdev_video_ops rdacm21_video_ops = {
+	.s_stream	= rdacm21_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
+	.enum_mbus_code = rdacm21_enum_mbus_code,
+	.get_fmt	= rdacm21_get_fmt,
+	.set_fmt	= rdacm21_get_fmt,
+};
+
+static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
+	.core		= &rdacm21_core_ops,
+	.video		= &rdacm21_video_ops,
+	.pad		= &rdacm21_subdev_pad_ops,
+};
+
+static int rdacm21_initialize(struct rdacm21_device *dev)
+{
+	int ret;
+
+	max9271_wake_up(&dev->serializer);
+
+	/* Enable reverse channel and disable the serial link. */
+	ret = max9271_set_serial_link(&dev->serializer, false);
+	if (ret)
+		return ret;
+
 	/*
 	 * Set reverse channel high threshold to increase noise immunity.
 	 *
--
2.31.1