| Message ID | 20201215170957.92761-1-jacopo+renesas@jmondi.org |
|---|---|
| Headers | show |
| Series | media: i2c: Add RDACM21 camera module | expand |
Hi Jacopo, Thank you for the patch. On Tue, Dec 15, 2020 at 06:09:54PM +0100, Jacopo Mondi wrote: > Document the 'reverse-channel-microvolt' vendor property in the > bindings document of the max9286 driver. > > The newly introduced property allows to specifying the initial > configuration of the GMSL reverse control channel to accommodate > remote serializers pre-programmed with the high threshold power > supply noise immunity enabled. > > 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> > --- > v5->v6: > - Use standard unit suffix 'microvolt' for the custom property > - Drop '$ref' as according to 'example-schema.yaml': > "Vendor specific properties having a standard unit suffix don't need a type." > --- > .../bindings/media/i2c/maxim,max9286.yaml | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > index 9ea827092fdd..b22ba3e0db4a 100644 > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > @@ -51,6 +51,26 @@ properties: > '#gpio-cells': > const: 2 > > + maxim,reverse-channel-microvolt: > + minimum: 30000 > + maximum: 200000 > + default: 170000 > + description: | > + Initial amplitude of the reverse control channel, in micro volts. > + > + The initial amplitude shall be adjusted to a value compatible with the > + configuration of the connected remote serializer. > + > + Some camera modules (for example RDACM20) include an on-board MCU that > + pre-programs the embedded serializer with power supply noise immunity > + (high-threshold) enabled. A typical value of the deserializer's reverse > + channel amplitude to communicate with pre-programmed serializers is > + 170000 micro volts. > + > + A typical value for the reverse channel amplitude to communicate with > + a remote serializer whose high-threshold noise immunity is not enabled > + is 100000 micro volts > + > ports: > type: object > description: | > @@ -221,6 +241,7 @@ required: > - ports > - i2c-mux > - gpio-controller > + - maxim,reverse-channel-microvolt > > additionalProperties: false > > @@ -243,6 +264,8 @@ examples: > gpio-controller; > #gpio-cells = <2>; > > + maxim,reverse-channel-microvolt = <170000>; > + > ports { > #address-cells = <1>; > #size-cells = <0>; -- Regards, Laurent Pinchart
Hi Jacopo, Thank you for the patch. On Tue, Dec 15, 2020 at 06:09:55PM +0100, Jacopo Mondi wrote: > Break out the reverse channel setup configuration procedure to its own > function. > > This change prepares for configuring the reverse channel conditionally > to the remote side high threshold configuration. > > No functional changes intended. > > Reviewed-by: Kieran Bingham <kieran.bingham+renesasa@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/max9286.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index c82c1493e099..1cfc8801c0b2 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -336,6 +336,22 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack) > usleep_range(3000, 5000); > } > > +static void max9286_reverse_channel_setup(struct max9286_priv *priv) > +{ > + /* > + * 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. > + */ > + 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); > + usleep_range(2000, 2500); > +} > + > /* > * max9286_check_video_links() - Make sure video links are detected and locked > * > @@ -941,19 +957,7 @@ static int max9286_setup(struct max9286_priv *priv) > * only. This should be disabled after the mux is initialised. > */ > max9286_configure_i2c(priv, true); > - > - /* > - * 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. > - */ > - 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); > - usleep_range(2000, 2500); > + max9286_reverse_channel_setup(priv); > > /* > * Enable GMSL links, mask unused ones and autodetect link -- Regards, Laurent Pinchart
Hi Jacopo, Thank you for the patch. On Tue, Dec 15, 2020 at 06:09:56PM +0100, 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. > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > 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 1cfc8801c0b2..021309c6dd6f 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -336,19 +336,29 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack) > usleep_range(3000, 5000); > } > > -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 to express values (100 < x < 130) */ > + chan_amplitude = chan_amplitude < 130 > + ? 30 : chan_amplitude - 100; This could also be written chan_amplitude = min(30, chan_amplitude - 100); With or without the change, Reviewed-by: Laurent Pinchart <laurent.pinchart@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 -- Regards, Laurent Pinchart
On Wed, Dec 16, 2020 at 07:14:25PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Dec 15, 2020 at 06:09:56PM +0100, 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. > > > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > 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 1cfc8801c0b2..021309c6dd6f 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -336,19 +336,29 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack) > > usleep_range(3000, 5000); > > } > > > > -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 to express values (100 < x < 130) */ > > + chan_amplitude = chan_amplitude < 130 > > + ? 30 : chan_amplitude - 100; > > This could also be written > > chan_amplitude = min(30, chan_amplitude - 100); s/min/max/ of course. > > With or without the change, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 -- Regards, Laurent Pinchart
On Wed, Dec 16, 2020 at 07:05:34PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Dec 15, 2020 at 06:09:54PM +0100, Jacopo Mondi wrote: > > Document the 'reverse-channel-microvolt' vendor property in the > > bindings document of the max9286 driver. > > > > The newly introduced property allows to specifying the initial > > configuration of the GMSL reverse control channel to accommodate > > remote serializers pre-programmed with the high threshold power > > supply noise immunity enabled. > > > > 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> > > > --- > > v5->v6: > > - Use standard unit suffix 'microvolt' for the custom property > > - Drop '$ref' as according to 'example-schema.yaml': > > "Vendor specific properties having a standard unit suffix don't need a type." > > --- > > .../bindings/media/i2c/maxim,max9286.yaml | 23 +++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > index 9ea827092fdd..b22ba3e0db4a 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > @@ -51,6 +51,26 @@ properties: > > '#gpio-cells': > > const: 2 > > > > + maxim,reverse-channel-microvolt: > > + minimum: 30000 > > + maximum: 200000 > > + default: 170000 > > + description: | > > + Initial amplitude of the reverse control channel, in micro volts. > > + > > + The initial amplitude shall be adjusted to a value compatible with the > > + configuration of the connected remote serializer. > > + > > + Some camera modules (for example RDACM20) include an on-board MCU that > > + pre-programs the embedded serializer with power supply noise immunity > > + (high-threshold) enabled. A typical value of the deserializer's reverse > > + channel amplitude to communicate with pre-programmed serializers is > > + 170000 micro volts. > > + > > + A typical value for the reverse channel amplitude to communicate with > > + a remote serializer whose high-threshold noise immunity is not enabled > > + is 100000 micro volts > > + > > ports: > > type: object > > description: | > > @@ -221,6 +241,7 @@ required: > > - ports > > - i2c-mux > > - gpio-controller > > + - maxim,reverse-channel-microvolt One comment though: You specify a default value above, which isn't very useful when the property is required. Should we either drop the default value, or make the property optional ? > > > > additionalProperties: false > > > > @@ -243,6 +264,8 @@ examples: > > gpio-controller; > > #gpio-cells = <2>; > > > > + maxim,reverse-channel-microvolt = <170000>; > > + > > ports { > > #address-cells = <1>; > > #size-cells = <0>;
On Wed, Dec 16, 2020 at 07:17:05PM +0200, Laurent Pinchart wrote: > On Wed, Dec 16, 2020 at 07:05:34PM +0200, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Tue, Dec 15, 2020 at 06:09:54PM +0100, Jacopo Mondi wrote: > > > Document the 'reverse-channel-microvolt' vendor property in the > > > bindings document of the max9286 driver. > > > > > > The newly introduced property allows to specifying the initial > > > configuration of the GMSL reverse control channel to accommodate > > > remote serializers pre-programmed with the high threshold power > > > supply noise immunity enabled. > > > > > > 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> > > > > > --- > > > v5->v6: > > > - Use standard unit suffix 'microvolt' for the custom property > > > - Drop '$ref' as according to 'example-schema.yaml': > > > "Vendor specific properties having a standard unit suffix don't need a type." > > > --- > > > .../bindings/media/i2c/maxim,max9286.yaml | 23 +++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > > index 9ea827092fdd..b22ba3e0db4a 100644 > > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > > @@ -51,6 +51,26 @@ properties: > > > '#gpio-cells': > > > const: 2 > > > > > > + maxim,reverse-channel-microvolt: > > > + minimum: 30000 > > > + maximum: 200000 > > > + default: 170000 > > > + description: | > > > + Initial amplitude of the reverse control channel, in micro volts. > > > + > > > + The initial amplitude shall be adjusted to a value compatible with the > > > + configuration of the connected remote serializer. > > > + > > > + Some camera modules (for example RDACM20) include an on-board MCU that > > > + pre-programs the embedded serializer with power supply noise immunity > > > + (high-threshold) enabled. A typical value of the deserializer's reverse > > > + channel amplitude to communicate with pre-programmed serializers is > > > + 170000 micro volts. > > > + > > > + A typical value for the reverse channel amplitude to communicate with > > > + a remote serializer whose high-threshold noise immunity is not enabled > > > + is 100000 micro volts > > > + > > > ports: > > > type: object > > > description: | > > > @@ -221,6 +241,7 @@ required: > > > - ports > > > - i2c-mux > > > - gpio-controller > > > + - maxim,reverse-channel-microvolt > > One comment though: You specify a default value above, which isn't very > useful when the property is required. Should we either drop the default > value, or make the property optional ? And generally added properties can't be required unless for some reason DT's without the property are broken. With required dropped, Reviewed-by: Rob Herring <robh@kernel.org> Rob
Hi Rob, Laurent, On Mon, Dec 21, 2020 at 11:58:27AM -0700, Rob Herring wrote: > On Wed, Dec 16, 2020 at 07:17:05PM +0200, Laurent Pinchart wrote: > > > > @@ -221,6 +241,7 @@ required: > > > > - ports > > > > - i2c-mux > > > > - gpio-controller > > > > + - maxim,reverse-channel-microvolt > > > > One comment though: You specify a default value above, which isn't very > > useful when the property is required. Should we either drop the default > > value, or make the property optional ? > > And generally added properties can't be required unless for some reason > DT's without the property are broken. My thinking was to make it required for new DTS and specify a default for the old ones that do not have the property. I'll drop required and keep the default value in next version. Thanks j > > With required dropped, > > Reviewed-by: Rob Herring <robh@kernel.org> > > Rob
Hello, this v6 renames the 'maxim,initial-reverse-channel-mV' property to 'maxim,reverse-channel-microvolt' to use standard suffix as suggested by Rob. v5: https://patchwork.linuxtv.org/project/linux-media/list/?series=3876 v4: https://patchwork.linuxtv.org/project/linux-media/list/?series=3847 v3: https://patchwork.linuxtv.org/project/linux-media/list/?series=3657 Background in v1 cover letter: https://www.spinics.net/lists/linux-renesas-soc/msg52886.html Jacopo Mondi (5): media: i2c: Add driver for RDACM21 camera module dt-bindings: media: max9286: Document 'maxim,reverse-channel-microvolt' media: i2c: max9286: Break-out reverse channel setup media: i2c: max9286: Make channel amplitude programmable media: i2c: max9286: Configure reverse channel amplitude .../bindings/media/i2c/maxim,max9286.yaml | 23 + MAINTAINERS | 12 + drivers/media/i2c/Kconfig | 13 + drivers/media/i2c/Makefile | 2 + drivers/media/i2c/max9286.c | 61 +- drivers/media/i2c/rdacm21.c | 595 ++++++++++++++++++ 6 files changed, 693 insertions(+), 13 deletions(-) create mode 100644 drivers/media/i2c/rdacm21.c -- 2.29.2