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@xxxxxxxxxx>
> ---
> 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);
>
> /*
--
Regards,
Laurent Pinchart
