diff mbox series

[14/16] media: i2c: max9286: Initialize remotes when bound

Message ID 20210216174146.106639-15-jacopo+renesas@jmondi.org (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series media: i2c: GMSL reliability improvements | expand

Commit Message

Jacopo Mondi Feb. 16, 2021, 5:41 p.m. UTC
With the introduction of the .init() core subdev operation in the
max9271 GMSL serializer, the max9286 deserializer needs to explicitly
initialize the remote devices by calling the .init() subdev operation on
each probed camera.

Call the .init() subdev operation at remote bound time and toggle
the reverse channel amplitude to compensate for the remote ends
noise immunity threshold.

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

Comments

Geert Uytterhoeven Feb. 17, 2021, 8:19 a.m. UTC | #1
On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> With the introduction of the .init() core subdev operation in the
> max9271 GMSL serializer, the max9286 deserializer needs to explicitly
> initialize the remote devices by calling the .init() subdev operation on
> each probed camera.
>
> Call the .init() subdev operation at remote bound time and toggle
> the reverse channel amplitude to compensate for the remote ends
> noise immunity threshold.

remote end's?
remote ends' ... thresholds?

Gr{oetje,eeting}s,

                        Geert
Kieran Bingham Feb. 18, 2021, 4 p.m. UTC | #2
On 16/02/2021 17:41, Jacopo Mondi wrote:
> With the introduction of the .init() core subdev operation in the
> max9271 GMSL serializer, the max9286 deserializer needs to explicitly
> initialize the remote devices by calling the .init() subdev operation on
> each probed camera.
> 
> Call the .init() subdev operation at remote bound time and toggle
> the reverse channel amplitude to compensate for the remote ends
> noise immunity threshold.
> 

Should this patch be merged with the previous one to keep the RDACM2x
and max9286 usage aligned?

I expect it won't compile fail, but it would fail a test (if bisecting
was testing the capture).

Seems to look ok, given the previous patch as a dependency:

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


> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7913b5f2249e..c41284de89b6 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -554,25 +554,39 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	dev_dbg(&priv->client->dev, "Bound %s pad: %u on index %u\n",
>  		subdev->name, src_pad, index);
>  
> +	/*
> +	 * Initialize the remote camera. Increase the channel amplitude
> +	 * to compensate for the remote noise immunity threshold.
> +	 */
> +	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
> +	ret = v4l2_subdev_call(subdev, core, init, 0);
> +	if (ret) {
> +		dev_err(&priv->client->dev,
> +			"Failed to initialize camera device %u\n", index);
> +		return ret;
> +	}
> +
>  	/*
>  	 * 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.
>  	 */
> -	if (priv->bound_sources != priv->source_mask)
> +	if (priv->bound_sources != priv->source_mask) {
> +		/*
> +		 * If not all remotes have probed yet, restore the initial
> +		 * reverse channel amplitude to allow the next camera to probe.
> +		 */
> +		max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);

Oh - wow, now I see why we definitely need to store both the initial and
the current value.



>  		return 0;
> +	}
>  
>  	/*
>  	 * 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);
>  
>  	/*
>
Laurent Pinchart Feb. 22, 2021, 2:03 a.m. UTC | #3
Hi Jacopo,

Thank you for the patch.

On Tue, Feb 16, 2021 at 06:41:44PM +0100, Jacopo Mondi wrote:
> With the introduction of the .init() core subdev operation in the
> max9271 GMSL serializer, the max9286 deserializer needs to explicitly
> initialize the remote devices by calling the .init() subdev operation on
> each probed camera.
> 
> Call the .init() subdev operation at remote bound time and toggle
> the reverse channel amplitude to compensate for the remote ends
> noise immunity threshold.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7913b5f2249e..c41284de89b6 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -554,25 +554,39 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	dev_dbg(&priv->client->dev, "Bound %s pad: %u on index %u\n",
>  		subdev->name, src_pad, index);
>  
> +	/*
> +	 * Initialize the remote camera. Increase the channel amplitude
> +	 * to compensate for the remote noise immunity threshold.
> +	 */
> +	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
> +	ret = v4l2_subdev_call(subdev, core, init, 0);
> +	if (ret) {
> +		dev_err(&priv->client->dev,
> +			"Failed to initialize camera device %u\n", index);
> +		return ret;
> +	}
> +
>  	/*
>  	 * 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.
>  	 */
> -	if (priv->bound_sources != priv->source_mask)
> +	if (priv->bound_sources != priv->source_mask) {
> +		/*
> +		 * If not all remotes have probed yet, restore the initial
> +		 * reverse channel amplitude to allow the next camera to probe.
> +		 */
> +		max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
>  		return 0;
> +	}

Instead of going back and forth, would it make sense to incerase the
channel amplitude here, and call the init() subdev operation in a loop
over all cameras ?

Otherwise the patch looks good to me, but I agree with Kieran that it
should probably be squashed with 13/16.

>  
>  	/*
>  	 * 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);
>  
>  	/*
diff mbox series

Patch

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 7913b5f2249e..c41284de89b6 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -554,25 +554,39 @@  static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	dev_dbg(&priv->client->dev, "Bound %s pad: %u on index %u\n",
 		subdev->name, src_pad, index);
 
+	/*
+	 * Initialize the remote camera. Increase the channel amplitude
+	 * to compensate for the remote noise immunity threshold.
+	 */
+	max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
+	ret = v4l2_subdev_call(subdev, core, init, 0);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"Failed to initialize camera device %u\n", index);
+		return ret;
+	}
+
 	/*
 	 * 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.
 	 */
-	if (priv->bound_sources != priv->source_mask)
+	if (priv->bound_sources != priv->source_mask) {
+		/*
+		 * If not all remotes have probed yet, restore the initial
+		 * reverse channel amplitude to allow the next camera to probe.
+		 */
+		max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
 		return 0;
+	}
 
 	/*
 	 * 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);
 
 	/*