[PATCH 0/5] media: i2c: max9286: Add configuration properties

[PATCH 0/5] media: i2c: max9286: Add configuration properties

[Jacopo Mondi]

Hello,
   this small series applies on top of Kieran's
[PATCH v7 0/2] MAX9286 GMSL support
and instruments the driver and expands its dt bindings to support
configuring a few deserializer parameters to enable support for different
remote serializers.

In particular for our use case RDACM20 and RDACM21 camera modules require
different configurations of the deserializer reverse channel amplitude and
disabling of the device overlap window.

Hyun has reported he needs to disable the deserializer overlap window to
have this driver work with max96705 serializer.

The series expands the device bindings to require two additional properties
to control overlap window and channel amplitude, and instruments the driver
to parse those properties and use them to configure it parameter.

What 'overlap window' controls is not totally clear at the moment and it's
poorly documented. In all our cases it could stay disabled, but as long as
its precise meaning it's not clarified, a required property ensures that
all current DTB are fully specified, so that in future we select a default
value, old users continue to work as intended/

The reverse channel amplitude describes the initially programmed signal
amplitude of the low bandwidth control channel. The amplitude is made
configurable to accommodate different serializers that might need an initial
different amplitude configuration to establish reliable communications.

On top of the series, a small fixup to be brought in next max9286 version.

Tested on R-Car R8A77970 Eagle with RDACM21.
I would have liked to test the same on Salvator-X with RDACM20. Kieran could
we sync and test it?

RDACM21 and RDACM20 drivers available at
git://jmondi.org/linux #gmsl/jmondi/platform/rdacm21
with integration in Eagle and Salvator-x from Kieran's platform branch, with
additional properties added by this series.


Jacopo Mondi (5):
  media: i2c: max9286: Put of node on error
  dt-bindings: media: max9286: Add overlap window
  media: i2c: max9286: Parse overlap window value
  dt-bindings: media: max9286: Add reverse channel amplitude
  media: i2c: max9286: Parse channel amplitude

 .../bindings/media/i2c/maxim,max9286.yaml     | 32 +++++++++
 MAINTAINERS                                   |  1 +
 drivers/media/i2c/max9286.c                   | 66 +++++++++++++++++--
 include/dt-bindings/media/maxim-gmsl.h        |  9 +++
 4 files changed, 103 insertions(+), 5 deletions(-)
 create mode 100644 include/dt-bindings/media/maxim-gmsl.h

--
2.25.1

[PATCH 1/5] media: i2c: max9286: Put of node on error

[Jacopo Mondi]

Put the device of node in case of dt parsing error.

Fixes: 9eed4185c7a0 ("media: i2c: Add MAX9286 driver")
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index a20829297ef6..06edd8bd3e82 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1046,6 +1046,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
 	if (!i2c_mux) {
 		dev_err(dev, "Failed to find i2c-mux node\n");
+		of_node_put(dev->of_node);
 		return -EINVAL;
 	}
 
-- 
2.25.1

[PATCH 2/5] dt-bindings: media: max9286: Add overlap window

[Jacopo Mondi]

The MAX9286 chip exposes a way to control the 'overlap window'
parameter, most probably used in calculation of the frame
synchronization interval.

When used in conjunction with some serializers, the overlap window has to
be disabled in order to correctly achieve frame sync locking.

As the exact meaning of that control is not documented in the chip's
manual, require all DTS users to specify the value of the window. When,
and if, in future the meaning of control gets clarified and a default
behaviour (window enabled or disabled) can be established, a new boolean
property could supersede this one while being sure that older DTB are
fully specified to avoid confusion.

Provide a few convenience macros for the window disabled and window
default value.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../devicetree/bindings/media/i2c/maxim,max9286.yaml  | 11 +++++++++++
 MAINTAINERS                                           |  1 +
 include/dt-bindings/media/maxim-gmsl.h                |  9 +++++++++
 3 files changed, 21 insertions(+)
 create mode 100644 include/dt-bindings/media/maxim-gmsl.h

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index f9d3e5712c59..ee8e0418b3f0 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -46,6 +46,14 @@ properties:
     description: GPIO connected to the \#PWDN pin with inverted polarity
     maxItems: 1
 
+  # Until the overlap window control gets not clarified, require dts
+  # to set its value explicitly,
+  maxim,overlap-window:
+    description: Overlap window duration, in pixel clock cycles.
+    maxItems: 1
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
   ports:
     type: object
     description: |
@@ -146,6 +154,7 @@ properties:
 required:
   - compatible
   - reg
+  - maxim,overlap-window
   - ports
   - i2c-mux
 
@@ -154,6 +163,7 @@ additionalProperties: false
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/media/maxim-gmsl.h>
 
     i2c@e66d8000 {
       #address-cells = <1>;
@@ -166,6 +176,7 @@ examples:
         reg = <0x2c>;
         poc-supply = <&camera_poc_12v>;
         enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
+        maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
 
         ports {
           #address-cells = <1>;
diff --git a/MAINTAINERS b/MAINTAINERS
index 21a9ff4fe684..3d2455085c80 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10190,6 +10190,7 @@ M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+F:	include/dt-bindings/media/maxim-gmsl.h
 F:	drivers/media/i2c/max9286.c
 
 MAX9860 MONO AUDIO VOICE CODEC DRIVER
diff --git a/include/dt-bindings/media/maxim-gmsl.h b/include/dt-bindings/media/maxim-gmsl.h
new file mode 100644
index 000000000000..47945ffc3a4d
--- /dev/null
+++ b/include/dt-bindings/media/maxim-gmsl.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
+#define _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
+
+/* MAX9286 default overlap values. */
+#define MAX9286_OVLP_WINDOW_DISABLED	<0>
+#define MAX9286_OVLP_WINDOW_DEFAULT	<0x1680>
+
+#endif /* _DT_BINDINGS_MEDIA_MAXIM_GMSL_H */
-- 
2.25.1

[PATCH 3/5] media: i2c: max9286: Parse overlap window value

[Jacopo Mondi]

Parse the 'maxim,overlap-window' property value and cache its
content to later program registers 0x63 and 0x64.

As specified by the bindings documentation, the property is mandatory as
long as a default value cannot be established to guarantee DTB backward
compatibility.

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

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 06edd8bd3e82..0357515860b2 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -117,6 +117,9 @@
 #define MAX9286_REV_FLEN(n)		((n) - 20)
 /* Register 0x49 */
 #define MAX9286_VIDEO_DETECT_MASK	0x0f
+/* Register 0x64 */
+#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
+
 /* Register 0x69 */
 #define MAX9286_LFLTBMONMASKED		BIT(7)
 #define MAX9286_LOCKMONMASKED		BIT(6)
@@ -164,6 +167,8 @@ struct max9286_priv {
 	unsigned int csi2_data_lanes;
 	struct max9286_source sources[MAX9286_NUM_GMSL];
 	struct v4l2_async_notifier notifier;
+
+	u32 overlap_window;
 };
 
 static struct max9286_source *next_source(struct max9286_priv *priv,
@@ -895,6 +900,11 @@ static int max9286_setup(struct max9286_priv *priv)
 	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
 		      MAX9286_FSYNCMETH_AUTO);
 
+	/* Configure overlap window. */
+	max9286_write(priv, 0x63, priv->overlap_window);
+	max9286_write(priv, 0x64, (priv->overlap_window >> 8) &
+				  MAX9286_OVLP_WINDOWH_MASK);
+
 	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
 	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
 		      MAX9286_HVSRC_D14);
@@ -1041,8 +1051,24 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	struct device_node *i2c_mux;
 	struct device_node *node = NULL;
 	unsigned int i2c_mux_mask = 0;
+	int ret;
 
 	of_node_get(dev->of_node);
+
+	/*
+	 * FIXM: Require overlap window value to be specified by DTS as long as
+	 * the control function is not clarified. As soon as a default
+	 * behaviour can be established drop this requirement, while older
+	 * DTBs are guaranteed to be fully specified.
+	 */
+	ret = of_property_read_u32(dev->of_node, "maxim,overlap-window",
+				   &priv->overlap_window);
+	if (ret) {
+		dev_err(dev, "Missing property \"maxim,overlap-window\"\n");
+		of_node_put(dev->of_node);
+		return -EINVAL;
+	}
+
 	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
 	if (!i2c_mux) {
 		dev_err(dev, "Failed to find i2c-mux node\n");
-- 
2.25.1

[PATCH 4/5] dt-bindings: media: max9286: Add reverse channel amplitude

[Jacopo Mondi]

The MAX9286 chip exposes registers to control the reverse channel
amplitude signal. The channel amplitude has to be configured according
to the connected remote serializer settings, in order to guarantee
reliable communications.

Serializer might be pre-programmed and initialize with their reverse
channel noise threshold level increased. While this is intended to
increase the signal/noise immunity ratio on the channel, the
deserializer should be initialized accordingly, with its channel
amplitude increased to 170mV.

Add to the bindings documentation a required property to allow DTS users
to specify the initial setting of the deserializer reverse channel and
accommodate different serializer models.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/media/i2c/maxim,max9286.yaml     | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index ee8e0418b3f0..a1c56734a727 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -54,6 +54,25 @@ properties:
     allOf:
       - $ref: /schemas/types.yaml#/definitions/uint32
 
+  maxim,reverse-channel-amplitude:
+    description: |
+      The reverse channel amplitude initial value, in milliVolts. If the remote
+      serializer is pre-programmed with an high reverse channel noise threshold,
+      the deserializer channel amplitude shall initially be increased to 170mV
+      to allow the two to communicate reliably. Likewise, if the remote
+      serializer probes without an increased reverse channel noise threshold,
+      the deserializer initial reverse channel amplitude should be set to 100mV
+      to be later increased to 170mV after serializers have increased their
+      reverse channel noise threshold.
+    maxItems: 1
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    # The property can be easily expanded to support more values if needed,
+    # but that's what's supported today by the driver.
+    oneOf:
+      - const: 100
+      - const: 170
+
   ports:
     type: object
     description: |
@@ -155,6 +174,7 @@ required:
   - compatible
   - reg
   - maxim,overlap-window
+  - maxim,reverse-channel-amplitude
   - ports
   - i2c-mux
 
@@ -177,6 +197,7 @@ examples:
         poc-supply = <&camera_poc_12v>;
         enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
         maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
+        maxim,reverse-channel-amplitude = <170>;
 
         ports {
           #address-cells = <1>;
-- 
2.25.1

[PATCH 5/5] media: i2c: max9286: Parse channel amplitude

[Jacopo Mondi]

Parse the 'maxim,reverse-channel-amplitude' property value and cache its
content to later program the initial reverse channel amplitude.

Only support 100mV and 170mV values for the moment. The property could
be easily expanded to support more values.

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

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 0357515860b2..24af8002535e 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -168,6 +168,7 @@ struct max9286_priv {
 	struct max9286_source sources[MAX9286_NUM_GMSL];
 	struct v4l2_async_notifier notifier;
 
+	u32 reverse_chan_amp;
 	u32 overlap_window;
 };
 
@@ -479,10 +480,15 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	 * All enabled sources have probed and enabled their reverse control
 	 * channels:
 	 *
-	 * - Verify all configuration links are properly detected
+	 * - Increase reverse channel amplitude to 170mV if not initially
+	 *   compensated
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
+	if (priv->reverse_chan_amp == 100)
+		max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) |
+			      MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X);
+
 	max9286_check_config_link(priv, priv->source_mask);
 
 	/*
@@ -830,6 +836,8 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
 
 static int max9286_setup(struct max9286_priv *priv)
 {
+	u8 chan_amp = MAX9286_REV_TRF(1);
+
 	/*
 	 * Link ordering values for all enabled links combinations. Orders must
 	 * be assigned sequentially from 0 to the number of enabled links
@@ -869,12 +877,18 @@ static int max9286_setup(struct max9286_priv *priv)
 	 *
 	 * - 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.
+	 * - Set initial reverse channel amplitude according the DTS property.
+	 *   If the initial channel amplitude is 100mV it should be increase
+	 *   later after the serializers high threshold have been enabled.
+	 *   If the initial value is 170mV the serializer has been
+	 *   pre-programmed and we can compensate immediately.
 	 */
 	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 (priv->reverse_chan_amp == 100)
+		chan_amp |= MAX9286_REV_AMP(100);
+	else
+		chan_amp |= MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X;
+	max9286_write(priv, 0x3b, chan_amp);
 	usleep_range(2000, 2500);
 
 	/*
@@ -1069,6 +1083,21 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 		return -EINVAL;
 	}
 
+	ret = of_property_read_u32(dev->of_node, "maxim,reverse-channel-amplitude",
+				   &priv->reverse_chan_amp);
+	if (ret) {
+		dev_err(dev,
+			"Missing property \"maxim,reverse-channel-amplitude\"\n");
+		of_node_put(dev->of_node);
+		return -EINVAL;
+	}
+	if (priv->reverse_chan_amp != 100 && priv->reverse_chan_amp != 170) {
+		dev_err(dev, "Unsupported  channel amplitude %umV\n",
+			priv->reverse_chan_amp);
+		of_node_put(dev->of_node);
+		return -EINVAL;
+	}
+
 	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
 	if (!i2c_mux) {
 		dev_err(dev, "Failed to find i2c-mux node\n");
-- 
2.25.1

Re: [PATCH 1/5] media: i2c: max9286: Put of node on error

[Kieran Bingham]

Hi Jacopo

On 16/03/2020 20:27, Jacopo Mondi wrote:
> Put the device of node in case of dt parsing error.
> 

Ooops, it does look like this probably leaks - but isn't it also leaking
in other code paths in this function too?

If we fix here, we should fix all leaks of this usage. (and perhaps
identify if there are leaks of other refcnts too ;-S )


> Fixes: 9eed4185c7a0 ("media: i2c: Add MAX9286 driver")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index a20829297ef6..06edd8bd3e82 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -1046,6 +1046,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>  	if (!i2c_mux) {
>  		dev_err(dev, "Failed to find i2c-mux node\n");
> +		of_node_put(dev->of_node);
>  		return -EINVAL;
>  	}
>  
> 

Re: [PATCH 2/5] dt-bindings: media: max9286: Add overlap window

[Kieran Bingham]

Hi Jacopo,

On 16/03/2020 20:27, Jacopo Mondi wrote:
> The MAX9286 chip exposes a way to control the 'overlap window'
> parameter, most probably used in calculation of the frame
> synchronization interval.
> 
> When used in conjunction with some serializers, the overlap window has to
> be disabled in order to correctly achieve frame sync locking.
> 
> As the exact meaning of that control is not documented in the chip's
> manual, require all DTS users to specify the value of the window. When,
> and if, in future the meaning of control gets clarified and a default

/in future/in the future/
/of control/of the control/

> behaviour (window enabled or disabled) can be established, a new boolean
> property could supersede this one while being sure that older DTB are

/DTB/DTBs/

> fully specified to avoid confusion.
> 
> Provide a few convenience macros for the window disabled and window
> default value.

Well it's not the best solution (putting hardcode values into the DTB)
but I agree, without documentation these are almost 'magic values for
the hardware' which is unfortunate.

I do fear this may be the wrong place still though.

This is dependent upon the 'serializer' connected, so is it a property
of the serializer that the max9286 should query at probe time to see
what it has connected?

But that's just back to the whole topic of bus-parameter negotiations
between the serializer and deserializer ...


But with this solution, any dtb segment or overlay for the serializer
needs to modify the max9286 depending on it's requirements ... that
feels a bit odd. Possible, I think, but odd.

Do we have any precedence in existing DT to reference on this topic?




> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/media/i2c/maxim,max9286.yaml  | 11 +++++++++++
>  MAINTAINERS                                           |  1 +
>  include/dt-bindings/media/maxim-gmsl.h                |  9 +++++++++
>  3 files changed, 21 insertions(+)
>  create mode 100644 include/dt-bindings/media/maxim-gmsl.h
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index f9d3e5712c59..ee8e0418b3f0 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -46,6 +46,14 @@ properties:
>      description: GPIO connected to the \#PWDN pin with inverted polarity
>      maxItems: 1
>  
> +  # Until the overlap window control gets not clarified, require dts
> +  # to set its value explicitly,
> +  maxim,overlap-window:
> +    description: Overlap window duration, in pixel clock cycles.
> +    maxItems: 1
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +
>    ports:
>      type: object
>      description: |
> @@ -146,6 +154,7 @@ properties:
>  required:
>    - compatible
>    - reg
> +  - maxim,overlap-window
>    - ports
>    - i2c-mux
>  
> @@ -154,6 +163,7 @@ additionalProperties: false
>  examples:
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/media/maxim-gmsl.h>
>  
>      i2c@e66d8000 {
>        #address-cells = <1>;
> @@ -166,6 +176,7 @@ examples:
>          reg = <0x2c>;
>          poc-supply = <&camera_poc_12v>;
>          enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
> +        maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
>  
>          ports {
>            #address-cells = <1>;
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 21a9ff4fe684..3d2455085c80 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10190,6 +10190,7 @@ M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>  L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +F:	include/dt-bindings/media/maxim-gmsl.h
>  F:	drivers/media/i2c/max9286.c
>  
>  MAX9860 MONO AUDIO VOICE CODEC DRIVER
> diff --git a/include/dt-bindings/media/maxim-gmsl.h b/include/dt-bindings/media/maxim-gmsl.h
> new file mode 100644
> index 000000000000..47945ffc3a4d
> --- /dev/null
> +++ b/include/dt-bindings/media/maxim-gmsl.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> +#define _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> +
> +/* MAX9286 default overlap values. */
> +#define MAX9286_OVLP_WINDOW_DISABLED	<0>
> +#define MAX9286_OVLP_WINDOW_DEFAULT	<0x1680>
> +
> +#endif /* _DT_BINDINGS_MEDIA_MAXIM_GMSL_H */
> 

Re: [PATCH 3/5] media: i2c: max9286: Parse overlap window value

[Kieran Bingham]

Hi Jacopo,

On 16/03/2020 20:27, Jacopo Mondi wrote:
> Parse the 'maxim,overlap-window' property value and cache its
> content to later program registers 0x63 and 0x64.
> 
> As specified by the bindings documentation, the property is mandatory as
> long as a default value cannot be established to guarantee DTB backward
> compatibility.

Well, we don't yet have the DTB bindings 'in' I don't believe so I don't
think we need to worry about backwards compatibility yet...

Oh, or do you mean in the future it wouldn't have to be mandatory
perhaps ...

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 06edd8bd3e82..0357515860b2 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -117,6 +117,9 @@
>  #define MAX9286_REV_FLEN(n)		((n) - 20)
>  /* Register 0x49 */
>  #define MAX9286_VIDEO_DETECT_MASK	0x0f
> +/* Register 0x64 */
> +#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
> +
>  /* Register 0x69 */
>  #define MAX9286_LFLTBMONMASKED		BIT(7)
>  #define MAX9286_LOCKMONMASKED		BIT(6)
> @@ -164,6 +167,8 @@ struct max9286_priv {
>  	unsigned int csi2_data_lanes;
>  	struct max9286_source sources[MAX9286_NUM_GMSL];
>  	struct v4l2_async_notifier notifier;
> +
> +	u32 overlap_window;
>  };
>  
>  static struct max9286_source *next_source(struct max9286_priv *priv,
> @@ -895,6 +900,11 @@ static int max9286_setup(struct max9286_priv *priv)
>  	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
>  		      MAX9286_FSYNCMETH_AUTO);
>  
> +	/* Configure overlap window. */
> +	max9286_write(priv, 0x63, priv->overlap_window);
> +	max9286_write(priv, 0x64, (priv->overlap_window >> 8) &
> +				  MAX9286_OVLP_WINDOWH_MASK);
> +
>  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
>  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
>  		      MAX9286_HVSRC_D14);
> @@ -1041,8 +1051,24 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	struct device_node *i2c_mux;
>  	struct device_node *node = NULL;
>  	unsigned int i2c_mux_mask = 0;
> +	int ret;
>  
>  	of_node_get(dev->of_node);
> +
> +	/*
> +	 * FIXM: Require overlap window value to be specified by DTS as long as

/FIXM/FIXME/

> +	 * the control function is not clarified. As soon as a default
> +	 * behaviour can be established drop this requirement, while older

/established/established,/
/requirement,/requirement/


> +	 * DTBs are guaranteed to be fully specified.
> +	 */
> +	ret = of_property_read_u32(dev->of_node, "maxim,overlap-window",
> +				   &priv->overlap_window);
> +	if (ret) {
> +		dev_err(dev, "Missing property \"maxim,overlap-window\"\n");
> +		of_node_put(dev->of_node);
> +		return -EINVAL;
> +	}
> +

Other wise, this looks fine except for my concerns and wondering if this
could be approached by defining a property of the requirements in the
serializer and parsing that in some form.


--
Kieran

>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>  	if (!i2c_mux) {
>  		dev_err(dev, "Failed to find i2c-mux node\n");
> 

Re: [PATCH 4/5] dt-bindings: media: max9286: Add reverse channel amplitude

[Kieran Bingham]

Hi Jacopo,

On 16/03/2020 20:27, Jacopo Mondi wrote:
> The MAX9286 chip exposes registers to control the reverse channel
> amplitude signal. The channel amplitude has to be configured according
> to the connected remote serializer settings, in order to guarantee
> reliable communications.
> 
> Serializer might be pre-programmed and initialize with their reverse
> channel noise threshold level increased. While this is intended to
> increase the signal/noise immunity ratio on the channel, the
> deserializer should be initialized accordingly, with its channel
> amplitude increased to 170mV.
> 
> Add to the bindings documentation a required property to allow DTS users
> to specify the initial setting of the deserializer reverse channel and
> accommodate different serializer models.
> 

Same comments really as the other property, that these are highly
dependent upon the serializer which is connected, so I wonder if there's
a better way to describe that association / communication requirement.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../bindings/media/i2c/maxim,max9286.yaml     | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index ee8e0418b3f0..a1c56734a727 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -54,6 +54,25 @@ properties:
>      allOf:
>        - $ref: /schemas/types.yaml#/definitions/uint32
>  
> +  maxim,reverse-channel-amplitude:
> +    description: |
> +      The reverse channel amplitude initial value, in milliVolts. If the remote

Should there be a newline after milliVolts. ? That seems like a brief,
followed by a description but all flowing together?

> +      serializer is pre-programmed with an high reverse channel noise threshold,

/an/a/

> +      the deserializer channel amplitude shall initially be increased to 170mV
> +      to allow the two to communicate reliably. Likewise, if the remote
> +      serializer probes without an increased reverse channel noise threshold,
> +      the deserializer initial reverse channel amplitude should be set to 100mV
> +      to be later increased to 170mV after serializers have increased their

/after/after the/

> +      reverse channel noise threshold.
> +    maxItems: 1
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +    # The property can be easily expanded to support more values if needed,
> +    # but that's what's supported today by the driver.

Stating "that's what's supported today" seems a bit redundant...

> +    oneOf:
> +      - const: 100
> +      - const: 170
> +
>    ports:
>      type: object
>      description: |
> @@ -155,6 +174,7 @@ required:
>    - compatible
>    - reg
>    - maxim,overlap-window
> +  - maxim,reverse-channel-amplitude
>    - ports
>    - i2c-mux
>  
> @@ -177,6 +197,7 @@ examples:
>          poc-supply = <&camera_poc_12v>;
>          enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
>          maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
> +        maxim,reverse-channel-amplitude = <170>;
>  
>          ports {
>            #address-cells = <1>;
> 

Re: [PATCH 5/5] media: i2c: max9286: Parse channel amplitude

[Kieran Bingham]

Hi Jacopo,

On 16/03/2020 20:27, Jacopo Mondi wrote:
> Parse the 'maxim,reverse-channel-amplitude' property value and cache its
> content to later program the initial reverse channel amplitude.
> 
> Only support 100mV and 170mV values for the moment. The property could
> be easily expanded to support more values.

Can we (in the future) support arbitrary values from a range, or only
from a fixed list?

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 39 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 0357515860b2..24af8002535e 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -168,6 +168,7 @@ struct max9286_priv {
>  	struct max9286_source sources[MAX9286_NUM_GMSL];
>  	struct v4l2_async_notifier notifier;
>  
> +	u32 reverse_chan_amp;
>  	u32 overlap_window;
>  };
>  
> @@ -479,10 +480,15 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	 * All enabled sources have probed and enabled their reverse control
>  	 * channels:
>  	 *
> -	 * - Verify all configuration links are properly detected
> +	 * - Increase reverse channel amplitude to 170mV if not initially
> +	 *   compensated
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> +	if (priv->reverse_chan_amp == 100)
> +		max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) |
> +			      MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X);
> +
>  	max9286_check_config_link(priv, priv->source_mask);
>  
>  	/*
> @@ -830,6 +836,8 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
>  
>  static int max9286_setup(struct max9286_priv *priv)
>  {
> +	u8 chan_amp = MAX9286_REV_TRF(1);
> +
>  	/*
>  	 * Link ordering values for all enabled links combinations. Orders must
>  	 * be assigned sequentially from 0 to the number of enabled links
> @@ -869,12 +877,18 @@ static int max9286_setup(struct max9286_priv *priv)
>  	 *
>  	 * - 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.
> +	 * - Set initial reverse channel amplitude according the DTS property.
> +	 *   If the initial channel amplitude is 100mV it should be increase
> +	 *   later after the serializers high threshold have been enabled.
> +	 *   If the initial value is 170mV the serializer has been
> +	 *   pre-programmed and we can compensate immediately.>  	 */
>  	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 (priv->reverse_chan_amp == 100)
> +		chan_amp |= MAX9286_REV_AMP(100);
> +	else
> +		chan_amp |= MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X;
> +	max9286_write(priv, 0x3b, chan_amp);
>  	usleep_range(2000, 2500);
>  
>  	/*
> @@ -1069,6 +1083,21 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  		return -EINVAL;
>  	}
>  
> +	ret = of_property_read_u32(dev->of_node, "maxim,reverse-channel-amplitude",
> +				   &priv->reverse_chan_amp);
> +	if (ret) {
> +		dev_err(dev,
> +			"Missing property \"maxim,reverse-channel-amplitude\"\n");
> +		of_node_put(dev->of_node);
> +		return -EINVAL;
> +	}
> +	if (priv->reverse_chan_amp != 100 && priv->reverse_chan_amp != 170) {
> +		dev_err(dev, "Unsupported  channel amplitude %umV\n",
> +			priv->reverse_chan_amp);
> +		of_node_put(dev->of_node);
> +		return -EINVAL;
> +	}
> +
>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>  	if (!i2c_mux) {
>  		dev_err(dev, "Failed to find i2c-mux node\n");
> 

Re: [PATCH 1/5] media: i2c: max9286: Put of node on error

[Jacopo Mondi]

Hi Kieran,

On Wed, Mar 18, 2020 at 09:32:45AM +0000, Kieran Bingham wrote:
> Hi Jacopo
>
> On 16/03/2020 20:27, Jacopo Mondi wrote:
> > Put the device of node in case of dt parsing error.
> >
>
> Ooops, it does look like this probably leaks - but isn't it also leaking
> in other code paths in this function too?

I checked and got confused by the very last
        of_node_put(node);

which should be coupled with and additional
        of_node_put(dev->of_node);

I don't see any additional leak, am I mistaken ?

>
> If we fix here, we should fix all leaks of this usage. (and perhaps
> identify if there are leaks of other refcnts too ;-S )
>
>
> > Fixes: 9eed4185c7a0 ("media: i2c: Add MAX9286 driver")
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index a20829297ef6..06edd8bd3e82 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -1046,6 +1046,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
> >  	if (!i2c_mux) {
> >  		dev_err(dev, "Failed to find i2c-mux node\n");
> > +		of_node_put(dev->of_node);
> >  		return -EINVAL;
> >  	}
> >
> >
>

Re: [PATCH 2/5] dt-bindings: media: max9286: Add overlap window

[Jacopo Mondi]

Hi Kieran,

On Wed, Mar 18, 2020 at 09:45:14AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 16/03/2020 20:27, Jacopo Mondi wrote:
> > The MAX9286 chip exposes a way to control the 'overlap window'
> > parameter, most probably used in calculation of the frame
> > synchronization interval.
> >
> > When used in conjunction with some serializers, the overlap window has to
> > be disabled in order to correctly achieve frame sync locking.
> >
> > As the exact meaning of that control is not documented in the chip's
> > manual, require all DTS users to specify the value of the window. When,
> > and if, in future the meaning of control gets clarified and a default
>
> /in future/in the future/
> /of control/of the control/
>
> > behaviour (window enabled or disabled) can be established, a new boolean
> > property could supersede this one while being sure that older DTB are
>
> /DTB/DTBs/
>
> > fully specified to avoid confusion.
> >
> > Provide a few convenience macros for the window disabled and window
> > default value.
>
> Well it's not the best solution (putting hardcode values into the DTB)
> but I agree, without documentation these are almost 'magic values for
> the hardware' which is unfortunate.

>
> I do fear this may be the wrong place still though.
>
> This is dependent upon the 'serializer' connected, so is it a property
> of the serializer that the max9286 should query at probe time to see
> what it has connected?

I see two distinct issues here:
1) overlap window: this is really a property of the deserializer and
should be here specified
2) reverse channel amplitude: this depends on the serializer
configuration. we could make this a serializer property and have the
deserializer read it from the remote end,  which, if I'm not
mistaken is a no-go (not sure I recall why, but I refrained to do so
as I recall it was considered bad practice).

This should work by ideally rewriting the deser properties when
loading overlays, or simply configuring the deserializer correctly
depending on which serializer is connected in your system. I don't see
it working differently than any other endpoint property. If you have a
parallel video port configured with an active high vsync polarity and
you want to connect a camera with a low active signal state you should
change you video input port ni your .dts or rewrite it with an overlay
in case of dynamic loading.

>
> But that's just back to the whole topic of bus-parameter negotiations
> between the serializer and deserializer ...
>

The real solution here would be to have operations to call on the
remote to get the bus configuration and act accordingly...

I recently proposed
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=256127
which works on remote pads though, and we have one pad per gmsl
channel, while this should be queried from the device. Yes we can bend
this to interrogate the remote side on the first connected channel and
ignore the pad argument in the callee, but maybe we can do better than
proposing an API and working it around immediately ? :)

>
> But with this solution, any dtb segment or overlay for the serializer
> needs to modify the max9286 depending on it's requirements ... that
> feels a bit odd. Possible, I think, but odd.

Really not someone with any dt overlay experience here, but re-writing
properties of nodes defined in the DTS seems like an intentended
feature of overlays... Is anyone with more experience listenting (says
Jacopo looking at Geert ?)

Thanks
   j
>
> Do we have any precedence in existing DT to reference on this topic?
>
>
>
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  .../devicetree/bindings/media/i2c/maxim,max9286.yaml  | 11 +++++++++++
> >  MAINTAINERS                                           |  1 +
> >  include/dt-bindings/media/maxim-gmsl.h                |  9 +++++++++
> >  3 files changed, 21 insertions(+)
> >  create mode 100644 include/dt-bindings/media/maxim-gmsl.h
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > index f9d3e5712c59..ee8e0418b3f0 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > @@ -46,6 +46,14 @@ properties:
> >      description: GPIO connected to the \#PWDN pin with inverted polarity
> >      maxItems: 1
> >
> > +  # Until the overlap window control gets not clarified, require dts
> > +  # to set its value explicitly,
> > +  maxim,overlap-window:
> > +    description: Overlap window duration, in pixel clock cycles.
> > +    maxItems: 1
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > +
> >    ports:
> >      type: object
> >      description: |
> > @@ -146,6 +154,7 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > +  - maxim,overlap-window
> >    - ports
> >    - i2c-mux
> >
> > @@ -154,6 +163,7 @@ additionalProperties: false
> >  examples:
> >    - |
> >      #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/media/maxim-gmsl.h>
> >
> >      i2c@e66d8000 {
> >        #address-cells = <1>;
> > @@ -166,6 +176,7 @@ examples:
> >          reg = <0x2c>;
> >          poc-supply = <&camera_poc_12v>;
> >          enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
> > +        maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
> >
> >          ports {
> >            #address-cells = <1>;
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 21a9ff4fe684..3d2455085c80 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10190,6 +10190,7 @@ M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >  L:	linux-media@vger.kernel.org
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +F:	include/dt-bindings/media/maxim-gmsl.h
> >  F:	drivers/media/i2c/max9286.c
> >
> >  MAX9860 MONO AUDIO VOICE CODEC DRIVER
> > diff --git a/include/dt-bindings/media/maxim-gmsl.h b/include/dt-bindings/media/maxim-gmsl.h
> > new file mode 100644
> > index 000000000000..47945ffc3a4d
> > --- /dev/null
> > +++ b/include/dt-bindings/media/maxim-gmsl.h
> > @@ -0,0 +1,9 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > +#define _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > +
> > +/* MAX9286 default overlap values. */
> > +#define MAX9286_OVLP_WINDOW_DISABLED	<0>
> > +#define MAX9286_OVLP_WINDOW_DEFAULT	<0x1680>
> > +
> > +#endif /* _DT_BINDINGS_MEDIA_MAXIM_GMSL_H */
> >
>

Re: [PATCH 3/5] media: i2c: max9286: Parse overlap window value

[Jacopo Mondi]

Hi Kieran,

On Wed, Mar 18, 2020 at 09:50:22AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 16/03/2020 20:27, Jacopo Mondi wrote:
> > Parse the 'maxim,overlap-window' property value and cache its
> > content to later program registers 0x63 and 0x64.
> >
> > As specified by the bindings documentation, the property is mandatory as
> > long as a default value cannot be established to guarantee DTB backward
> > compatibility.
>
> Well, we don't yet have the DTB bindings 'in' I don't believe so I don't
> think we need to worry about backwards compatibility yet...
>
> Oh, or do you mean in the future it wouldn't have to be mandatory
> perhaps ...

I meant in future. As we don't know now what an opportune default is
for this un-documented setting, I think having all DTBs using this
first version fully specified will make any decision we take in future
easier to implement in a back-ward compatible way.

>
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 06edd8bd3e82..0357515860b2 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -117,6 +117,9 @@
> >  #define MAX9286_REV_FLEN(n)		((n) - 20)
> >  /* Register 0x49 */
> >  #define MAX9286_VIDEO_DETECT_MASK	0x0f
> > +/* Register 0x64 */
> > +#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
> > +
> >  /* Register 0x69 */
> >  #define MAX9286_LFLTBMONMASKED		BIT(7)
> >  #define MAX9286_LOCKMONMASKED		BIT(6)
> > @@ -164,6 +167,8 @@ struct max9286_priv {
> >  	unsigned int csi2_data_lanes;
> >  	struct max9286_source sources[MAX9286_NUM_GMSL];
> >  	struct v4l2_async_notifier notifier;
> > +
> > +	u32 overlap_window;
> >  };
> >
> >  static struct max9286_source *next_source(struct max9286_priv *priv,
> > @@ -895,6 +900,11 @@ static int max9286_setup(struct max9286_priv *priv)
> >  	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> >  		      MAX9286_FSYNCMETH_AUTO);
> >
> > +	/* Configure overlap window. */
> > +	max9286_write(priv, 0x63, priv->overlap_window);
> > +	max9286_write(priv, 0x64, (priv->overlap_window >> 8) &
> > +				  MAX9286_OVLP_WINDOWH_MASK);
> > +
> >  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> >  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
> >  		      MAX9286_HVSRC_D14);
> > @@ -1041,8 +1051,24 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  	struct device_node *i2c_mux;
> >  	struct device_node *node = NULL;
> >  	unsigned int i2c_mux_mask = 0;
> > +	int ret;
> >
> >  	of_node_get(dev->of_node);
> > +
> > +	/*
> > +	 * FIXM: Require overlap window value to be specified by DTS as long as
>
> /FIXM/FIXME/
>
> > +	 * the control function is not clarified. As soon as a default
> > +	 * behaviour can be established drop this requirement, while older
>
> /established/established,/
> /requirement,/requirement/
>
>
> > +	 * DTBs are guaranteed to be fully specified.
> > +	 */
> > +	ret = of_property_read_u32(dev->of_node, "maxim,overlap-window",
> > +				   &priv->overlap_window);
> > +	if (ret) {
> > +		dev_err(dev, "Missing property \"maxim,overlap-window\"\n");
> > +		of_node_put(dev->of_node);
> > +		return -EINVAL;
> > +	}
> > +
>
> Other wise, this looks fine except for my concerns and wondering if this
> could be approached by defining a property of the requirements in the
> serializer and parsing that in some form.

As per my previous reply, I had the same concern but mostly for the
reverse channel setting. This is a configuration that applies solely
to the deserializer (at least, that's my understanding not knowing
exactly what this property controls)

>
>
> --
> Kieran
>
> >  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
> >  	if (!i2c_mux) {
> >  		dev_err(dev, "Failed to find i2c-mux node\n");
> >
>

Re: [PATCH 5/5] media: i2c: max9286: Parse channel amplitude

[Jacopo Mondi]

Hi Kieran,

On Wed, Mar 18, 2020 at 09:57:48AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 16/03/2020 20:27, Jacopo Mondi wrote:
> > Parse the 'maxim,reverse-channel-amplitude' property value and cache its
> > content to later program the initial reverse channel amplitude.
> >
> > Only support 100mV and 170mV values for the moment. The property could
> > be easily expanded to support more values.
>
> Can we (in the future) support arbitrary values from a range, or only
> from a fixed list?

Good question. The 0x3b register of the deserializer is not documented
in my datasheet version, I got this from the application developer
guide that reports

        Increase reverse amplitude from 100mV to
        170mV. This compensates for the higher
        threshold of step 5.

and reports the following list of supported values in the 0x3b
register description.

        Reverse channel amplitude
        000 = 30mV
        001 = 40mV
        010 = 50mV
        011 = 60mV
        100 = 70mV
        101 = 80mV
        110 = 90mV
        111 = 100mV

with an optional +100mV boost option.

Going forward we can add more values to the list of supported ones in
the bindings and control their configuration in the driver.

Maybe worth noting it down with a fixme note ?

>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 39 ++++++++++++++++++++++++++++++++-----
> >  1 file changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 0357515860b2..24af8002535e 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -168,6 +168,7 @@ struct max9286_priv {
> >  	struct max9286_source sources[MAX9286_NUM_GMSL];
> >  	struct v4l2_async_notifier notifier;
> >
> > +	u32 reverse_chan_amp;
> >  	u32 overlap_window;
> >  };
> >
> > @@ -479,10 +480,15 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> >  	 * All enabled sources have probed and enabled their reverse control
> >  	 * channels:
> >  	 *
> > -	 * - Verify all configuration links are properly detected
> > +	 * - Increase reverse channel amplitude to 170mV if not initially
> > +	 *   compensated
> >  	 * - Disable auto-ack as communication on the control channel are now
> >  	 *   stable.
> >  	 */
> > +	if (priv->reverse_chan_amp == 100)
> > +		max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) |
> > +			      MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X);
> > +
> >  	max9286_check_config_link(priv, priv->source_mask);
> >
> >  	/*
> > @@ -830,6 +836,8 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
> >
> >  static int max9286_setup(struct max9286_priv *priv)
> >  {
> > +	u8 chan_amp = MAX9286_REV_TRF(1);
> > +
> >  	/*
> >  	 * Link ordering values for all enabled links combinations. Orders must
> >  	 * be assigned sequentially from 0 to the number of enabled links
> > @@ -869,12 +877,18 @@ static int max9286_setup(struct max9286_priv *priv)
> >  	 *
> >  	 * - 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.
> > +	 * - Set initial reverse channel amplitude according the DTS property.
> > +	 *   If the initial channel amplitude is 100mV it should be increase
> > +	 *   later after the serializers high threshold have been enabled.
> > +	 *   If the initial value is 170mV the serializer has been
> > +	 *   pre-programmed and we can compensate immediately.>  	 */
> >  	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 (priv->reverse_chan_amp == 100)
> > +		chan_amp |= MAX9286_REV_AMP(100);
> > +	else
> > +		chan_amp |= MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X;
> > +	max9286_write(priv, 0x3b, chan_amp);
> >  	usleep_range(2000, 2500);
> >
> >  	/*
> > @@ -1069,6 +1083,21 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  		return -EINVAL;
> >  	}
> >
> > +	ret = of_property_read_u32(dev->of_node, "maxim,reverse-channel-amplitude",
> > +				   &priv->reverse_chan_amp);
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"Missing property \"maxim,reverse-channel-amplitude\"\n");
> > +		of_node_put(dev->of_node);
> > +		return -EINVAL;
> > +	}
> > +	if (priv->reverse_chan_amp != 100 && priv->reverse_chan_amp != 170) {
> > +		dev_err(dev, "Unsupported  channel amplitude %umV\n",
> > +			priv->reverse_chan_amp);
> > +		of_node_put(dev->of_node);
> > +		return -EINVAL;
> > +	}
> > +
> >  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
> >  	if (!i2c_mux) {
> >  		dev_err(dev, "Failed to find i2c-mux node\n");
> >
>

Re: [PATCH 2/5] dt-bindings: media: max9286: Add overlap window

[Hyun Kwon]

Hi Jacopo,

On Wed, 2020-03-18 at 07:19:39 -0700, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Wed, Mar 18, 2020 at 09:45:14AM +0000, Kieran Bingham wrote:
> > Hi Jacopo,
> >
> > On 16/03/2020 20:27, Jacopo Mondi wrote:
> > > The MAX9286 chip exposes a way to control the 'overlap window'
> > > parameter, most probably used in calculation of the frame
> > > synchronization interval.
> > >
> > > When used in conjunction with some serializers, the overlap window has to
> > > be disabled in order to correctly achieve frame sync locking.
> > >
> > > As the exact meaning of that control is not documented in the chip's
> > > manual, require all DTS users to specify the value of the window. When,
> > > and if, in future the meaning of control gets clarified and a default
> >
> > /in future/in the future/
> > /of control/of the control/
> >
> > > behaviour (window enabled or disabled) can be established, a new boolean
> > > property could supersede this one while being sure that older DTB are
> >
> > /DTB/DTBs/
> >
> > > fully specified to avoid confusion.
> > >
> > > Provide a few convenience macros for the window disabled and window
> > > default value.
> >
> > Well it's not the best solution (putting hardcode values into the DTB)
> > but I agree, without documentation these are almost 'magic values for
> > the hardware' which is unfortunate.
> 
> >
> > I do fear this may be the wrong place still though.
> >
> > This is dependent upon the 'serializer' connected, so is it a property
> > of the serializer that the max9286 should query at probe time to see
> > what it has connected?
> 
> I see two distinct issues here:
> 1) overlap window: this is really a property of the deserializer and
> should be here specified
> 2) reverse channel amplitude: this depends on the serializer
> configuration. we could make this a serializer property and have the
> deserializer read it from the remote end,  which, if I'm not
> mistaken is a no-go (not sure I recall why, but I refrained to do so
> as I recall it was considered bad practice).
> 
> This should work by ideally rewriting the deser properties when
> loading overlays, or simply configuring the deserializer correctly
> depending on which serializer is connected in your system. I don't see
> it working differently than any other endpoint property. If you have a
> parallel video port configured with an active high vsync polarity and
> you want to connect a camera with a low active signal state you should
> change you video input port ni your .dts or rewrite it with an overlay
> in case of dynamic loading.
> 
> >
> > But that's just back to the whole topic of bus-parameter negotiations
> > between the serializer and deserializer ...
> >
> 
> The real solution here would be to have operations to call on the
> remote to get the bus configuration and act accordingly...
> 
> I recently proposed
> https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=256127
> which works on remote pads though, and we have one pad per gmsl
> channel, while this should be queried from the device. Yes we can bend
> this to interrogate the remote side on the first connected channel and
> ignore the pad argument in the callee, but maybe we can do better than
> proposing an API and working it around immediately ? :)
> 

What should be worked around here? To me, this seems better solution than
device tree because those are not really hardened properties, and some even
have to be passed between ser - deser. So adding a gmsl config to the pad
config would do, correct? I can (and would like to) try that out if you don't
ee any obvious issue. :-)

Thanks,
-hyun

> >
> > But with this solution, any dtb segment or overlay for the serializer
> > needs to modify the max9286 depending on it's requirements ... that
> > feels a bit odd. Possible, I think, but odd.
> 
> Really not someone with any dt overlay experience here, but re-writing
> properties of nodes defined in the DTS seems like an intentended
> feature of overlays... Is anyone with more experience listenting (says
> Jacopo looking at Geert ?)
> 
> Thanks
>    j
> >
> > Do we have any precedence in existing DT to reference on this topic?
> >
> >
> >
> >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  .../devicetree/bindings/media/i2c/maxim,max9286.yaml  | 11 +++++++++++
> > >  MAINTAINERS                                           |  1 +
> > >  include/dt-bindings/media/maxim-gmsl.h                |  9 +++++++++
> > >  3 files changed, 21 insertions(+)
> > >  create mode 100644 include/dt-bindings/media/maxim-gmsl.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > index f9d3e5712c59..ee8e0418b3f0 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > @@ -46,6 +46,14 @@ properties:
> > >      description: GPIO connected to the \#PWDN pin with inverted polarity
> > >      maxItems: 1
> > >
> > > +  # Until the overlap window control gets not clarified, require dts
> > > +  # to set its value explicitly,
> > > +  maxim,overlap-window:
> > > +    description: Overlap window duration, in pixel clock cycles.
> > > +    maxItems: 1
> > > +    allOf:
> > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > >    ports:
> > >      type: object
> > >      description: |
> > > @@ -146,6 +154,7 @@ properties:
> > >  required:
> > >    - compatible
> > >    - reg
> > > +  - maxim,overlap-window
> > >    - ports
> > >    - i2c-mux
> > >
> > > @@ -154,6 +163,7 @@ additionalProperties: false
> > >  examples:
> > >    - |
> > >      #include <dt-bindings/gpio/gpio.h>
> > > +    #include <dt-bindings/media/maxim-gmsl.h>
> > >
> > >      i2c@e66d8000 {
> > >        #address-cells = <1>;
> > > @@ -166,6 +176,7 @@ examples:
> > >          reg = <0x2c>;
> > >          poc-supply = <&camera_poc_12v>;
> > >          enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
> > > +        maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
> > >
> > >          ports {
> > >            #address-cells = <1>;
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 21a9ff4fe684..3d2455085c80 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -10190,6 +10190,7 @@ M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >  L:	linux-media@vger.kernel.org
> > >  S:	Maintained
> > >  F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > +F:	include/dt-bindings/media/maxim-gmsl.h
> > >  F:	drivers/media/i2c/max9286.c
> > >
> > >  MAX9860 MONO AUDIO VOICE CODEC DRIVER
> > > diff --git a/include/dt-bindings/media/maxim-gmsl.h b/include/dt-bindings/media/maxim-gmsl.h
> > > new file mode 100644
> > > index 000000000000..47945ffc3a4d
> > > --- /dev/null
> > > +++ b/include/dt-bindings/media/maxim-gmsl.h
> > > @@ -0,0 +1,9 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > > +#define _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > > +
> > > +/* MAX9286 default overlap values. */
> > > +#define MAX9286_OVLP_WINDOW_DISABLED	<0>
> > > +#define MAX9286_OVLP_WINDOW_DEFAULT	<0x1680>
> > > +
> > > +#endif /* _DT_BINDINGS_MEDIA_MAXIM_GMSL_H */
> > >
> >

Re: [PATCH 2/5] dt-bindings: media: max9286: Add overlap window

[Jacopo Mondi]

Hi Hyun,

On Wed, Mar 18, 2020 at 06:08:13PM -0700, Hyun Kwon wrote:
> Hi Jacopo,
>
> On Wed, 2020-03-18 at 07:19:39 -0700, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Wed, Mar 18, 2020 at 09:45:14AM +0000, Kieran Bingham wrote:
> > > Hi Jacopo,
> > >
> > > On 16/03/2020 20:27, Jacopo Mondi wrote:
> > > > The MAX9286 chip exposes a way to control the 'overlap window'
> > > > parameter, most probably used in calculation of the frame
> > > > synchronization interval.
> > > >
> > > > When used in conjunction with some serializers, the overlap window has to
> > > > be disabled in order to correctly achieve frame sync locking.
> > > >
> > > > As the exact meaning of that control is not documented in the chip's
> > > > manual, require all DTS users to specify the value of the window. When,
> > > > and if, in future the meaning of control gets clarified and a default
> > >
> > > /in future/in the future/
> > > /of control/of the control/
> > >
> > > > behaviour (window enabled or disabled) can be established, a new boolean
> > > > property could supersede this one while being sure that older DTB are
> > >
> > > /DTB/DTBs/
> > >
> > > > fully specified to avoid confusion.
> > > >
> > > > Provide a few convenience macros for the window disabled and window
> > > > default value.
> > >
> > > Well it's not the best solution (putting hardcode values into the DTB)
> > > but I agree, without documentation these are almost 'magic values for
> > > the hardware' which is unfortunate.
> >
> > >
> > > I do fear this may be the wrong place still though.
> > >
> > > This is dependent upon the 'serializer' connected, so is it a property
> > > of the serializer that the max9286 should query at probe time to see
> > > what it has connected?
> >
> > I see two distinct issues here:
> > 1) overlap window: this is really a property of the deserializer and
> > should be here specified
> > 2) reverse channel amplitude: this depends on the serializer
> > configuration. we could make this a serializer property and have the
> > deserializer read it from the remote end,  which, if I'm not
> > mistaken is a no-go (not sure I recall why, but I refrained to do so
> > as I recall it was considered bad practice).
> >
> > This should work by ideally rewriting the deser properties when
> > loading overlays, or simply configuring the deserializer correctly
> > depending on which serializer is connected in your system. I don't see
> > it working differently than any other endpoint property. If you have a
> > parallel video port configured with an active high vsync polarity and
> > you want to connect a camera with a low active signal state you should
> > change you video input port ni your .dts or rewrite it with an overlay
> > in case of dynamic loading.
> >
> > >
> > > But that's just back to the whole topic of bus-parameter negotiations
> > > between the serializer and deserializer ...
> > >
> >
> > The real solution here would be to have operations to call on the
> > remote to get the bus configuration and act accordingly...
> >
> > I recently proposed
> > https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=256127
> > which works on remote pads though, and we have one pad per gmsl
> > channel, while this should be queried from the device. Yes we can bend
> > this to interrogate the remote side on the first connected channel and
> > ignore the pad argument in the callee, but maybe we can do better than
> > proposing an API and working it around immediately ? :)
> >
>
> What should be worked around here? To me, this seems better solution than
> device tree because those are not really hardened properties, and some even
> have to be passed between ser - deser. So adding a gmsl config to the pad
> config would do, correct? I can (and would like to) try that out if you don't
> ee any obvious issue. :-)

As you know I'm not yet sure which properties should come from DTS and
which ones negotiated, but for sure some of them fit better in a
ser/deser negotiation and the reverse channel amplitude setting is
indeed one of them.

Why my get_mbus_config() should be "worked around": the proposed
operation is a pad operation. The deserializer has 4 input pads, one
for each gmsl input channel. Currently we configure the reverse
channel amplitude once, then probe all the four channels, and finally
we compensate it. I said "work it around" as in this scenario we have
to query the first enabled remote channel, and apply the reported
settings for all the channels.

BUT I'm now thinking we should actually change how we setup reverse
channel in the deserializer and make it a per-channel procedure, which
is what the programming guide suggests as well.

Using get_mbus_config() with the current implementation would look like

        config = get_mbus_config(first_enabled_channel)
        configure_reverse_channel(config)

        for_each_channel(c):
                setup_serializer(c)

        if (config.amplitude < 170)
                increase_amplitude(config)

And I'm really not thrilled by this.
What we should do is actually

        for_each_channel(c):
                config = get_mbus_config(c)
                configure_reverse_channel(config);

                setup_serializer(c)

                if (config.amplitude < 170)
                        increase_amplitude(config)

And I would like this much better, it also gives get_mbus_config() one
more user which would help having that operation accepted in mainline.

I will give this a go! What do you think ?

Thanks
   j

>
> Thanks,
> -hyun
>
> > >
> > > But with this solution, any dtb segment or overlay for the serializer
> > > needs to modify the max9286 depending on it's requirements ... that
> > > feels a bit odd. Possible, I think, but odd.
> >
> > Really not someone with any dt overlay experience here, but re-writing
> > properties of nodes defined in the DTS seems like an intentended
> > feature of overlays... Is anyone with more experience listenting (says
> > Jacopo looking at Geert ?)
> >
> > Thanks
> >    j
> > >
> > > Do we have any precedence in existing DT to reference on this topic?
> > >
> > >
> > >
> > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  .../devicetree/bindings/media/i2c/maxim,max9286.yaml  | 11 +++++++++++
> > > >  MAINTAINERS                                           |  1 +
> > > >  include/dt-bindings/media/maxim-gmsl.h                |  9 +++++++++
> > > >  3 files changed, 21 insertions(+)
> > > >  create mode 100644 include/dt-bindings/media/maxim-gmsl.h
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > > index f9d3e5712c59..ee8e0418b3f0 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > > @@ -46,6 +46,14 @@ properties:
> > > >      description: GPIO connected to the \#PWDN pin with inverted polarity
> > > >      maxItems: 1
> > > >
> > > > +  # Until the overlap window control gets not clarified, require dts
> > > > +  # to set its value explicitly,
> > > > +  maxim,overlap-window:
> > > > +    description: Overlap window duration, in pixel clock cycles.
> > > > +    maxItems: 1
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > > +
> > > >    ports:
> > > >      type: object
> > > >      description: |
> > > > @@ -146,6 +154,7 @@ properties:
> > > >  required:
> > > >    - compatible
> > > >    - reg
> > > > +  - maxim,overlap-window
> > > >    - ports
> > > >    - i2c-mux
> > > >
> > > > @@ -154,6 +163,7 @@ additionalProperties: false
> > > >  examples:
> > > >    - |
> > > >      #include <dt-bindings/gpio/gpio.h>
> > > > +    #include <dt-bindings/media/maxim-gmsl.h>
> > > >
> > > >      i2c@e66d8000 {
> > > >        #address-cells = <1>;
> > > > @@ -166,6 +176,7 @@ examples:
> > > >          reg = <0x2c>;
> > > >          poc-supply = <&camera_poc_12v>;
> > > >          enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
> > > > +        maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
> > > >
> > > >          ports {
> > > >            #address-cells = <1>;
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 21a9ff4fe684..3d2455085c80 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -10190,6 +10190,7 @@ M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > >  L:	linux-media@vger.kernel.org
> > > >  S:	Maintained
> > > >  F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > > +F:	include/dt-bindings/media/maxim-gmsl.h
> > > >  F:	drivers/media/i2c/max9286.c
> > > >
> > > >  MAX9860 MONO AUDIO VOICE CODEC DRIVER
> > > > diff --git a/include/dt-bindings/media/maxim-gmsl.h b/include/dt-bindings/media/maxim-gmsl.h
> > > > new file mode 100644
> > > > index 000000000000..47945ffc3a4d
> > > > --- /dev/null
> > > > +++ b/include/dt-bindings/media/maxim-gmsl.h
> > > > @@ -0,0 +1,9 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +#ifndef _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > > > +#define _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > > > +
> > > > +/* MAX9286 default overlap values. */
> > > > +#define MAX9286_OVLP_WINDOW_DISABLED	<0>
> > > > +#define MAX9286_OVLP_WINDOW_DEFAULT	<0x1680>
> > > > +
> > > > +#endif /* _DT_BINDINGS_MEDIA_MAXIM_GMSL_H */
> > > >
> > >

Re: [PATCH 5/5] media: i2c: max9286: Parse channel amplitude

[Kieran Bingham]

On 18/03/2020 14:32, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Wed, Mar 18, 2020 at 09:57:48AM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 16/03/2020 20:27, Jacopo Mondi wrote:
>>> Parse the 'maxim,reverse-channel-amplitude' property value and cache its
>>> content to later program the initial reverse channel amplitude.
>>>
>>> Only support 100mV and 170mV values for the moment. The property could
>>> be easily expanded to support more values.
>>
>> Can we (in the future) support arbitrary values from a range, or only
>> from a fixed list?
> 
> Good question. The 0x3b register of the deserializer is not documented
> in my datasheet version, I got this from the application developer
> guide that reports
> 
>         Increase reverse amplitude from 100mV to
>         170mV. This compensates for the higher
>         threshold of step 5.
> 
> and reports the following list of supported values in the 0x3b
> register description.
> 
>         Reverse channel amplitude
>         000 = 30mV
>         001 = 40mV
>         010 = 50mV
>         011 = 60mV
>         100 = 70mV
>         101 = 80mV
>         110 = 90mV
>         111 = 100mV
> 
> with an optional +100mV boost option.

Ok, so we have two supported 'ranges'
 30-100mV and 130-200mV.

Indeed it's probably best not to express that as a single range :-)


> Going forward we can add more values to the list of supported ones in
> the bindings and control their configuration in the driver.
> 
> Maybe worth noting it down with a fixme note ?

I wonder if it should be expressed as a supported range, with a boost,
which matches the hardware, and presumably will match the requirements
on the serializer side too ?

Presumably if the boost is needed, we 'know' when it's needed? although
that doesn't quite fit either. I see you're going from 100mV to 70mV so
you're actually onnly applying a 'boost' of 70mV not 100 ?

Hrm ... I'll have to do more digging to understand what's going here.

--
Kieran


> 
>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/media/i2c/max9286.c | 39 ++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 34 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>> index 0357515860b2..24af8002535e 100644
>>> --- a/drivers/media/i2c/max9286.c
>>> +++ b/drivers/media/i2c/max9286.c
>>> @@ -168,6 +168,7 @@ struct max9286_priv {
>>>  	struct max9286_source sources[MAX9286_NUM_GMSL];
>>>  	struct v4l2_async_notifier notifier;
>>>
>>> +	u32 reverse_chan_amp;
>>>  	u32 overlap_window;
>>>  };
>>>
>>> @@ -479,10 +480,15 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>>>  	 * All enabled sources have probed and enabled their reverse control
>>>  	 * channels:
>>>  	 *
>>> -	 * - Verify all configuration links are properly detected
>>> +	 * - Increase reverse channel amplitude to 170mV if not initially
>>> +	 *   compensated
>>>  	 * - Disable auto-ack as communication on the control channel are now
>>>  	 *   stable.
>>>  	 */
>>> +	if (priv->reverse_chan_amp == 100)
>>> +		max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) |
>>> +			      MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X);
>>> +
>>>  	max9286_check_config_link(priv, priv->source_mask);
>>>
>>>  	/*
>>> @@ -830,6 +836,8 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
>>>
>>>  static int max9286_setup(struct max9286_priv *priv)
>>>  {
>>> +	u8 chan_amp = MAX9286_REV_TRF(1);
>>> +
>>>  	/*
>>>  	 * Link ordering values for all enabled links combinations. Orders must
>>>  	 * be assigned sequentially from 0 to the number of enabled links
>>> @@ -869,12 +877,18 @@ static int max9286_setup(struct max9286_priv *priv)
>>>  	 *
>>>  	 * - 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.
>>> +	 * - Set initial reverse channel amplitude according the DTS property.
>>> +	 *   If the initial channel amplitude is 100mV it should be increase
>>> +	 *   later after the serializers high threshold have been enabled.
>>> +	 *   If the initial value is 170mV the serializer has been
>>> +	 *   pre-programmed and we can compensate immediately.>  	 */
>>>  	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 (priv->reverse_chan_amp == 100)
>>> +		chan_amp |= MAX9286_REV_AMP(100);
>>> +	else
>>> +		chan_amp |= MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X;
>>> +	max9286_write(priv, 0x3b, chan_amp);
>>>  	usleep_range(2000, 2500);
>>>
>>>  	/*
>>> @@ -1069,6 +1083,21 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>>  		return -EINVAL;
>>>  	}
>>>
>>> +	ret = of_property_read_u32(dev->of_node, "maxim,reverse-channel-amplitude",
>>> +				   &priv->reverse_chan_amp);
>>> +	if (ret) {
>>> +		dev_err(dev,
>>> +			"Missing property \"maxim,reverse-channel-amplitude\"\n");
>>> +		of_node_put(dev->of_node);
>>> +		return -EINVAL;
>>> +	}
>>> +	if (priv->reverse_chan_amp != 100 && priv->reverse_chan_amp != 170) {
>>> +		dev_err(dev, "Unsupported  channel amplitude %umV\n",
>>> +			priv->reverse_chan_amp);
>>> +		of_node_put(dev->of_node);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>>>  	if (!i2c_mux) {
>>>  		dev_err(dev, "Failed to find i2c-mux node\n");
>>>
>>