| Message ID | 20201015182710.54795-1-jacopo+renesas@jmondi.org |
|---|---|
| Headers | show |
| Series | media: i2c: Add support for RDACM21 camera module | expand |
Hi Jacopo, On 15/10/2020 19:27, Jacopo Mondi wrote: > Adjust reverse channel amplitude according to the presence of > the 'high-threshold" DTS property. > > If no high threshold compensation is required, start with a low > amplitude (100mV) and increase it after the remote serializers > have probed and have enabled noise immunity on their reverse > channels. > > If high threshold compensation is required, configure the reverse > channel with a 170mV amplitude before the remote serializers have > probed. > > This change is required for both rdacm20 and rdacm21 camera modules > to be correctly probed when used in combination with the max9286 > deserializer. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/max9286.c | 74 +++++++++++++++++++++++-------------- > 1 file changed, 47 insertions(+), 27 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 163e102199e3..4b59a9e0228a 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -163,6 +163,8 @@ struct max9286_priv { > unsigned int mux_channel; > bool mux_open; > > + bool high_threshold; > + > struct v4l2_ctrl_handler ctrls; > struct v4l2_ctrl *pixelrate; > > @@ -436,6 +438,32 @@ static int max9286_check_config_link(struct max9286_priv *priv, > return 0; > } > > +static void max9286_reverse_channel_setup(struct max9286_priv *priv, > + unsigned int chan_amplitude) This looks like you're adding a new function - how about we add this function here, in the first place when you add it in 3/7 > +{ > + /* Reverse channel transmission time: default to 1. */ > + u8 chan_config = MAX9286_REV_TRF(1); > + > + /* > + * Reverse channel setup. > + * > + * - Enable custom reverse channel configuration (through register 0x3f) > + * and set the first pulse length to 35 clock cycles. > + * - Adjust reverse channel amplitude: values > 130 are programmed > + * using the additional +100mV REV_AMP_X boost flag > + */ > + max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35)); > + > + if (chan_amplitude > 100) { > + /* It is not possible express values (100 < x < 130) */ > + chan_amplitude = chan_amplitude < 130 > + ? 30 : chan_amplitude - 100; > + chan_config |= MAX9286_REV_AMP_X; > + } > + max9286_write(priv, 0x3b, chan_config | MAX9286_REV_AMP(chan_amplitude)); > + usleep_range(2000, 2500); > +} > + > /* ----------------------------------------------------------------------------- > * V4L2 Subdev > */ > @@ -531,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > * All enabled sources have probed and enabled their reverse control > * channels: > * > + * - Increase the reverse channel amplitude to compensate for the > + * remote ends high threshold, if not done already > * - Verify all configuration links are properly detected > * - Disable auto-ack as communication on the control channel are now > * stable. > */ > + if (!priv->high_threshold) > + max9286_reverse_channel_setup(priv, 170); is it troublesome to re-set it if it's already set? I guess it's just unnecessary. so that's fine. > max9286_check_config_link(priv, priv->source_mask); > > /* > @@ -906,32 +938,6 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv) > * Probe/Remove > */ > > -static void max9286_reverse_channel_setup(struct max9286_priv *priv, > - unsigned int chan_amplitude) > -{ > - /* Reverse channel transmission time: default to 1. */ > - u8 chan_config = MAX9286_REV_TRF(1); > - > - /* > - * Reverse channel setup. > - * > - * - Enable custom reverse channel configuration (through register 0x3f) > - * and set the first pulse length to 35 clock cycles. > - * - Adjust reverse channel amplitude: values > 130 are programmed > - * using the additional +100mV REV_AMP_X boost flag > - */ > - max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35)); > - > - if (chan_amplitude > 100) { > - /* It is not possible express values (100 < x < 130) */ > - chan_amplitude = chan_amplitude < 130 > - ? 30 : chan_amplitude - 100; > - chan_config |= MAX9286_REV_AMP_X; > - } > - max9286_write(priv, 0x3b, chan_config | MAX9286_REV_AMP(chan_amplitude)); > - usleep_range(2000, 2500); > -} > - > static int max9286_setup(struct max9286_priv *priv) > { > /* > @@ -967,7 +973,15 @@ 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, 170); > + > + /* > + * Compensate the remote end high threshold with a larger channel > + * amplitude if necessary. > + */ > + if (priv->high_threshold) > + max9286_reverse_channel_setup(priv, 170); > + else > + max9286_reverse_channel_setup(priv, 100); Hrm... ternery is more concise here, but is it helpful? max9286_reverse_channel_setup(priv, priv->high_threshold ? 170 : 100); The high-threshold could also be parsed in max9286_reverse_channel_setup(), but I like it being passed in. > > /* > * Enable GMSL links, mask unused ones and autodetect link > @@ -1235,6 +1249,12 @@ static int max9286_parse_dt(struct max9286_priv *priv) > } > of_node_put(node); > > + /* > + * Parse 'high_threshold' property to configure the reverse channel > + * amplitude. > + */ > + priv->high_threshold = device_property_present(dev, "high_threshold"); > + Oh, I think I like this, it's a neat way to express what it needs to do from the DT depending on the attached cameras. It's sort of dependant upon the cameras though, I guess making this something that we query from the remote endpoint isn't so easy ...? > priv->route_mask = priv->source_mask; > > return 0; >
Hi Jacopo, On 15/10/2020 19:27, Jacopo Mondi wrote: > Instrument the function that configures the reverse channel with a > programmable amplitude value. > > This change serves to prepare to adjust the reverse channel amplitude > depending on the remote end high-threshold configuration. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/max9286.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 89a7248f5c25..163e102199e3 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -906,19 +906,29 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv) > * Probe/Remove > */ > > -static void max9286_reverse_channel_setup(struct max9286_priv *priv) > +static void max9286_reverse_channel_setup(struct max9286_priv *priv, > + unsigned int chan_amplitude) > { > + /* Reverse channel transmission time: default to 1. */ > + u8 chan_config = MAX9286_REV_TRF(1); > + > /* > * Reverse channel setup. > * > * - Enable custom reverse channel configuration (through register 0x3f) > * and set the first pulse length to 35 clock cycles. > - * - Increase the reverse channel amplitude to 170mV to accommodate the > - * high threshold enabled by the serializer driver. > + * - Adjust reverse channel amplitude: values > 130 are programmed > + * using the additional +100mV REV_AMP_X boost flag > */ > max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35)); > - max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) | > - MAX9286_REV_AMP_X); > + Should we also clamp to min/max values at all? Probably not needed, as it's only an internal helper. > + if (chan_amplitude > 100) { > + /* It is not possible express values (100 < x < 130) */ 'possible to express' > + chan_amplitude = chan_amplitude < 130 > + ? 30 : chan_amplitude - 100; We could round < 115 to 100, and >= 115 to 130, but that's probably more effort that it's worth, so I think this is fine. I think it's really helpful to codify this parameter though: Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > + chan_config |= MAX9286_REV_AMP_X; > + } > + max9286_write(priv, 0x3b, chan_config | MAX9286_REV_AMP(chan_amplitude)); > usleep_range(2000, 2500); > } > > @@ -957,7 +967,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); > + max9286_reverse_channel_setup(priv, 170); > > /* > * Enable GMSL links, mask unused ones and autodetect link >
Hello! On 15.10.2020 21:27, Jacopo Mondi wrote: > Instrument the function that configures the reverse channel with a > programmable amplitude value. > > This change serves to prepare to adjust the reverse channel amplitude > depending on the remote end high-threshold configuration. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/max9286.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 89a7248f5c25..163e102199e3 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -906,19 +906,29 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv) > * Probe/Remove > */ > > -static void max9286_reverse_channel_setup(struct max9286_priv *priv) > +static void max9286_reverse_channel_setup(struct max9286_priv *priv, > + unsigned int chan_amplitude) > { > + /* Reverse channel transmission time: default to 1. */ > + u8 chan_config = MAX9286_REV_TRF(1); > + > /* > * Reverse channel setup. > * > * - Enable custom reverse channel configuration (through register 0x3f) > * and set the first pulse length to 35 clock cycles. > - * - Increase the reverse channel amplitude to 170mV to accommodate the > - * high threshold enabled by the serializer driver. > + * - Adjust reverse channel amplitude: values > 130 are programmed > + * using the additional +100mV REV_AMP_X boost flag > */ > max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35)); > - max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) | > - MAX9286_REV_AMP_X); > + > + if (chan_amplitude > 100) { > + /* It is not possible express values (100 < x < 130) */ "To express", perhaps? > + chan_amplitude = chan_amplitude < 130 > + ? 30 : chan_amplitude - 100; > + chan_config |= MAX9286_REV_AMP_X; > + } > + max9286_write(priv, 0x3b, chan_config | MAX9286_REV_AMP(chan_amplitude)); > usleep_range(2000, 2500); > } > [...] MBR, Sergei
Hi Kieran, On Thu, Oct 15, 2020 at 08:52:01PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 15/10/2020 19:27, Jacopo Mondi wrote: > > Adjust reverse channel amplitude according to the presence of > > the 'high-threshold" DTS property. > > > > If no high threshold compensation is required, start with a low > > amplitude (100mV) and increase it after the remote serializers > > have probed and have enabled noise immunity on their reverse > > channels. > > > > If high threshold compensation is required, configure the reverse > > channel with a 170mV amplitude before the remote serializers have > > probed. > > > > This change is required for both rdacm20 and rdacm21 camera modules > > to be correctly probed when used in combination with the max9286 > > deserializer. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/max9286.c | 74 +++++++++++++++++++++++-------------- > > 1 file changed, 47 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > index 163e102199e3..4b59a9e0228a 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -163,6 +163,8 @@ struct max9286_priv { > > unsigned int mux_channel; > > bool mux_open; > > > > + bool high_threshold; > > + > > struct v4l2_ctrl_handler ctrls; > > struct v4l2_ctrl *pixelrate; > > > > @@ -436,6 +438,32 @@ static int max9286_check_config_link(struct max9286_priv *priv, > > return 0; > > } > > > > +static void max9286_reverse_channel_setup(struct max9286_priv *priv, > > + unsigned int chan_amplitude) > > This looks like you're adding a new function - how about we add this > function here, in the first place when you add it in 3/7 > I'll move it up in 3/7 > > +{ > > + /* Reverse channel transmission time: default to 1. */ > > + u8 chan_config = MAX9286_REV_TRF(1); > > + > > + /* > > + * Reverse channel setup. > > + * > > + * - Enable custom reverse channel configuration (through register 0x3f) > > + * and set the first pulse length to 35 clock cycles. > > + * - Adjust reverse channel amplitude: values > 130 are programmed > > + * using the additional +100mV REV_AMP_X boost flag > > + */ > > + max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35)); > > + > > + if (chan_amplitude > 100) { > > + /* It is not possible express values (100 < x < 130) */ > > + chan_amplitude = chan_amplitude < 130 > > + ? 30 : chan_amplitude - 100; > > + chan_config |= MAX9286_REV_AMP_X; > > + } > > + max9286_write(priv, 0x3b, chan_config | MAX9286_REV_AMP(chan_amplitude)); > > + usleep_range(2000, 2500); > > +} > > + > > /* ----------------------------------------------------------------------------- > > * V4L2 Subdev > > */ > > @@ -531,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > > * All enabled sources have probed and enabled their reverse control > > * channels: > > * > > + * - Increase the reverse channel amplitude to compensate for the > > + * remote ends high threshold, if not done already > > * - Verify all configuration links are properly detected > > * - Disable auto-ack as communication on the control channel are now > > * stable. > > */ > > + if (!priv->high_threshold) > > + max9286_reverse_channel_setup(priv, 170); > > is it troublesome to re-set it if it's already set? I guess it's just > unnecessary. so that's fine. > > > max9286_check_config_link(priv, priv->source_mask); > > > > /* > > @@ -906,32 +938,6 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv) > > * Probe/Remove > > */ > > > > -static void max9286_reverse_channel_setup(struct max9286_priv *priv, > > - unsigned int chan_amplitude) > > -{ > > - /* Reverse channel transmission time: default to 1. */ > > - u8 chan_config = MAX9286_REV_TRF(1); > > - > > - /* > > - * Reverse channel setup. > > - * > > - * - Enable custom reverse channel configuration (through register 0x3f) > > - * and set the first pulse length to 35 clock cycles. > > - * - Adjust reverse channel amplitude: values > 130 are programmed > > - * using the additional +100mV REV_AMP_X boost flag > > - */ > > - max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35)); > > - > > - if (chan_amplitude > 100) { > > - /* It is not possible express values (100 < x < 130) */ > > - chan_amplitude = chan_amplitude < 130 > > - ? 30 : chan_amplitude - 100; > > - chan_config |= MAX9286_REV_AMP_X; > > - } > > - max9286_write(priv, 0x3b, chan_config | MAX9286_REV_AMP(chan_amplitude)); > > - usleep_range(2000, 2500); > > -} > > - > > static int max9286_setup(struct max9286_priv *priv) > > { > > /* > > @@ -967,7 +973,15 @@ 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, 170); > > + > > + /* > > + * Compensate the remote end high threshold with a larger channel > > + * amplitude if necessary. > > + */ > > + if (priv->high_threshold) > > + max9286_reverse_channel_setup(priv, 170); > > + else > > + max9286_reverse_channel_setup(priv, 100); > > Hrm... ternery is more concise here, but is it helpful? > Ternary is nicer, you're right! > max9286_reverse_channel_setup(priv, priv->high_threshold ? 170 : 100); > > The high-threshold could also be parsed in > max9286_reverse_channel_setup(), but I like it being passed in. > > > > > /* > > * Enable GMSL links, mask unused ones and autodetect link > > @@ -1235,6 +1249,12 @@ static int max9286_parse_dt(struct max9286_priv *priv) > > } > > of_node_put(node); > > > > + /* > > + * Parse 'high_threshold' property to configure the reverse channel > > + * amplitude. > > + */ > > + priv->high_threshold = device_property_present(dev, "high_threshold"); > > + > > Oh, I think I like this, it's a neat way to express what it needs to do > from the DT depending on the attached cameras. > > It's sort of dependant upon the cameras though, I guess making this > something that we query from the remote endpoint isn't so easy ...? That's the real question. Is it fair to express as a deserializer property a setting of the remote serializer(s) ? What if the remotes have different configurations (we had to play with pre-programmed and non-pre-programmed RDACM20s in the past iirc). I've detailed a few possible way forward and their pros and cons in the v1 cover letter [1] and I'm happy to discuss them as I'm not sure this is the best possible way forward. Thanks j [1] For reference: https://www.spinics.net/lists/linux-renesas-soc/msg52886.html > > > > > priv->route_mask = priv->source_mask; > > > > return 0; > > >