| 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 |
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
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); > > /* >
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 --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); /*
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(-)