[RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Convert the DT binding documentation for the TI TFP410 DPI-to-DVI
encoder to json-schema.

Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
---
Hi all,

I found some issues while converting this binding and I'd like to know
your opinions on how to tackle them.

1) dtbs_check fails for arch/arm/boot/dts/dove-sbc-a510.dts

  This board uses the TFP410 encoder but it doesn't define any ports for
  it. I can't find any suitable remote endpoints in its description
  either. Maybe this board description should be reworked? The current
  driver won't handle the device if it doesn't define any ports or
  endpoints anyway.

  It also uses the 'powerdown-gpio' property instead of
  'powerdown-gpios'. AFAICT this shouldn't be a problem from the driver
  point of view, but the general standard in DT bindings is to use the
  plural. This is trivial to fix.

2) The definition of ti,deskew in the original binding seems to be
tailored to the current driver and the way it's defined may not be very
DT-friendly.

  This parameter maps to a 3-bit field in a hardware register that takes
  a value from 0 to 7, so the [-4, 3] range described for this would map
  to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.

  Then, the driver parses the parameter (unsigned) and casts it to a
  signed integer to get a number in the [-4, 3] range.

  A vendor-specific property must have a type definition in json-schema,
  so if I translate the original bindings semantics directly, I should
  define ti,deskew as an int32, but this makes dt_binding_check fail if
  the property has a negative value in the example because of the
  internal representation of cells as unsigned integers:

     ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647

  So I can think of two solutions to this:

  a) Keep the ti,deskew property as an uint32 and document the valid
  range ([-4, 3]) in the property description (this is what this patch
  does currently).

  b) Redefine this property to be closer to the datasheet description
  (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
  This would also let us define its range properly using minimum and
  maximum properties for it.

  I think (b) is the right thing to do but I want to know your
  opinion. Besides, I don't have this hardware at hand and if I updated
  the driver I wouldn't be able to test it.

Thanks.

Patch tested with:

make dt_binding_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>
make dtbs_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>

 .../bindings/display/bridge/ti,tfp410.txt     |  66 ----------
 .../bindings/display/bridge/ti,tfp410.yaml    | 121 ++++++++++++++++++
 2 files changed, 121 insertions(+), 66 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
deleted file mode 100644
index 5ff4f64ef8e8..000000000000
--- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
+++ /dev/null
@@ -1,66 +0,0 @@
-TFP410 DPI to DVI encoder
-=========================
-
-Required properties:
-- compatible: "ti,tfp410"
-
-Optional properties:
-- powerdown-gpios: power-down gpio
-- reg: I2C address. If and only if present the device node should be placed
-  into the I2C controller node where the TFP410 I2C is connected to.
-- ti,deskew: data de-skew in 350ps increments, from -4 to +3, as configured
-  through th DK[3:1] pins. This property shall be present only if the TFP410
-  is not connected through I2C.
-
-Required nodes:
-
-This device has two video ports. Their connections are modeled using the OF
-graph bindings specified in [1]. Each port node shall have a single endpoint.
-
-- Port 0 is the DPI input port. Its endpoint subnode shall contain a
-  pclk-sample and bus-width property and a remote-endpoint property as specified
-  in [1].
-  - If pclk-sample is not defined, pclk-sample = 0 should be assumed for
-    backward compatibility.
-  - If bus-width is not defined then bus-width = 24 should be assumed for
-    backward compatibility.
-    bus-width = 24: 24 data lines are connected and single-edge mode
-    bus-width = 12: 12 data lines are connected and dual-edge mode
-
-- Port 1 is the DVI output port. Its endpoint subnode shall contain a
-  remote-endpoint property is specified in [1].
-
-[1] Documentation/devicetree/bindings/media/video-interfaces.txt
-
-
-Example
--------
-
-tfp410: encoder@0 {
-	compatible = "ti,tfp410";
-	powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
-	ti,deskew = <4>;
-
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@0 {
-			reg = <0>;
-
-			tfp410_in: endpoint@0 {
-				pclk-sample = <1>;
-				bus-width = <24>;
-				remote-endpoint = <&dpi_out>;
-			};
-		};
-
-		port@1 {
-			reg = <1>;
-
-			tfp410_out: endpoint@0 {
-				remote-endpoint = <&dvi_connector_in>;
-			};
-		};
-	};
-};
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
new file mode 100644
index 000000000000..79666ee540f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
@@ -0,0 +1,121 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/ti,tfp410.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TFP410 DPI to DVI encoder
+
+maintainers:
+  - Tomi Valkeinen <tomi.valkeinen@ti.com>
+  - Jyri Sarha <jsarha@ti.com>
+
+properties:
+  compatible:
+    const: "ti,tfp410"
+
+  reg:
+    description: I2C address of the device.
+    maxItems: 1
+
+  powerdown-gpios:
+    maxItems: 1
+
+  ti,deskew:
+    description:
+      Data de-skew in 350ps increments, from -4 to +3, as configured
+      through the DK[3:1] pins. This property shall be present only if
+      the TFP410 is not connected through I2C.
+    maxItems: 1
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
+  ports:
+    description:
+      A node containing input and output port nodes with endpoint
+      definitions as documented in
+      Documentation/devicetree/bindings/media/video-interfaces.txt
+    type: object
+
+    properties:
+      port@0:
+        description: DPI input port.
+        type: object
+
+        properties:
+          reg:
+            const: 0
+
+          endpoint:
+            type: object
+
+            properties:
+              pclk-sample:
+                description: Endpoint sampling edge.
+                enum:
+                  - 0  # Falling edge
+                  - 1  # Rising edge
+
+              bus-width:
+                description: Endpoint bus width.
+                enum: [ 12, 24 ]
+
+        required:
+          - endpoint
+
+      port@1:
+        description: DVI output port.
+        type: object
+
+        properties:
+          reg:
+            const: 1
+
+          endpoint:
+            type: object
+
+        required:
+          - endpoint
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    tfp410: encoder {
+        compatible = "ti,tfp410";
+        powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
+        ti,deskew = <3>;
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+                tfp410_in: endpoint {
+                    pclk-sample = <1>;
+                    bus-width = <24>;
+                    remote-endpoint = <&dpi_out>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+                tfp410_out: endpoint {
+                    remote-endpoint = <&dvi_connector_in>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.18.0

[RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Convert the DT binding documentation for the TI TFP410 DPI-to-DVI
encoder to json-schema.

Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
---
Hi all,

I found some issues while converting this binding and I'd like to know
your opinions on how to tackle them.

1) dtbs_check fails for arch/arm/boot/dts/dove-sbc-a510.dts

  This board uses the TFP410 encoder but it doesn't define any ports for
  it. I can't find any suitable remote endpoints in its description
  either. Maybe this board description should be reworked? The current
  driver won't handle the device if it doesn't define any ports or
  endpoints anyway.

  It also uses the 'powerdown-gpio' property instead of
  'powerdown-gpios'. AFAICT this shouldn't be a problem from the driver
  point of view, but the general standard in DT bindings is to use the
  plural. This is trivial to fix.

2) The definition of ti,deskew in the original binding seems to be
tailored to the current driver and the way it's defined may not be very
DT-friendly.

  This parameter maps to a 3-bit field in a hardware register that takes
  a value from 0 to 7, so the [-4, 3] range described for this would map
  to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.

  Then, the driver parses the parameter (unsigned) and casts it to a
  signed integer to get a number in the [-4, 3] range.

  A vendor-specific property must have a type definition in json-schema,
  so if I translate the original bindings semantics directly, I should
  define ti,deskew as an int32, but this makes dt_binding_check fail if
  the property has a negative value in the example because of the
  internal representation of cells as unsigned integers:

     ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647

  So I can think of two solutions to this:

  a) Keep the ti,deskew property as an uint32 and document the valid
  range ([-4, 3]) in the property description (this is what this patch
  does currently).

  b) Redefine this property to be closer to the datasheet description
  (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
  This would also let us define its range properly using minimum and
  maximum properties for it.

  I think (b) is the right thing to do but I want to know your
  opinion. Besides, I don't have this hardware at hand and if I updated
  the driver I wouldn't be able to test it.

Thanks.

Patch tested with:

make dt_binding_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>
make dtbs_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>

 .../bindings/display/bridge/ti,tfp410.txt     |  66 ----------
 .../bindings/display/bridge/ti,tfp410.yaml    | 121 ++++++++++++++++++
 2 files changed, 121 insertions(+), 66 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
deleted file mode 100644
index 5ff4f64ef8e8..000000000000
--- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
+++ /dev/null
@@ -1,66 +0,0 @@
-TFP410 DPI to DVI encoder
-=========================
-
-Required properties:
-- compatible: "ti,tfp410"
-
-Optional properties:
-- powerdown-gpios: power-down gpio
-- reg: I2C address. If and only if present the device node should be placed
-  into the I2C controller node where the TFP410 I2C is connected to.
-- ti,deskew: data de-skew in 350ps increments, from -4 to +3, as configured
-  through th DK[3:1] pins. This property shall be present only if the TFP410
-  is not connected through I2C.
-
-Required nodes:
-
-This device has two video ports. Their connections are modeled using the OF
-graph bindings specified in [1]. Each port node shall have a single endpoint.
-
-- Port 0 is the DPI input port. Its endpoint subnode shall contain a
-  pclk-sample and bus-width property and a remote-endpoint property as specified
-  in [1].
-  - If pclk-sample is not defined, pclk-sample = 0 should be assumed for
-    backward compatibility.
-  - If bus-width is not defined then bus-width = 24 should be assumed for
-    backward compatibility.
-    bus-width = 24: 24 data lines are connected and single-edge mode
-    bus-width = 12: 12 data lines are connected and dual-edge mode
-
-- Port 1 is the DVI output port. Its endpoint subnode shall contain a
-  remote-endpoint property is specified in [1].
-
-[1] Documentation/devicetree/bindings/media/video-interfaces.txt
-
-
-Example
--------
-
-tfp410: encoder@0 {
-	compatible = "ti,tfp410";
-	powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
-	ti,deskew = <4>;
-
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@0 {
-			reg = <0>;
-
-			tfp410_in: endpoint@0 {
-				pclk-sample = <1>;
-				bus-width = <24>;
-				remote-endpoint = <&dpi_out>;
-			};
-		};
-
-		port@1 {
-			reg = <1>;
-
-			tfp410_out: endpoint@0 {
-				remote-endpoint = <&dvi_connector_in>;
-			};
-		};
-	};
-};
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
new file mode 100644
index 000000000000..79666ee540f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
@@ -0,0 +1,121 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/ti,tfp410.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TFP410 DPI to DVI encoder
+
+maintainers:
+  - Tomi Valkeinen <tomi.valkeinen@ti.com>
+  - Jyri Sarha <jsarha@ti.com>
+
+properties:
+  compatible:
+    const: "ti,tfp410"
+
+  reg:
+    description: I2C address of the device.
+    maxItems: 1
+
+  powerdown-gpios:
+    maxItems: 1
+
+  ti,deskew:
+    description:
+      Data de-skew in 350ps increments, from -4 to +3, as configured
+      through the DK[3:1] pins. This property shall be present only if
+      the TFP410 is not connected through I2C.
+    maxItems: 1
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
+  ports:
+    description:
+      A node containing input and output port nodes with endpoint
+      definitions as documented in
+      Documentation/devicetree/bindings/media/video-interfaces.txt
+    type: object
+
+    properties:
+      port@0:
+        description: DPI input port.
+        type: object
+
+        properties:
+          reg:
+            const: 0
+
+          endpoint:
+            type: object
+
+            properties:
+              pclk-sample:
+                description: Endpoint sampling edge.
+                enum:
+                  - 0  # Falling edge
+                  - 1  # Rising edge
+
+              bus-width:
+                description: Endpoint bus width.
+                enum: [ 12, 24 ]
+
+        required:
+          - endpoint
+
+      port@1:
+        description: DVI output port.
+        type: object
+
+        properties:
+          reg:
+            const: 1
+
+          endpoint:
+            type: object
+
+        required:
+          - endpoint
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    tfp410: encoder {
+        compatible = "ti,tfp410";
+        powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
+        ti,deskew = <3>;
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+                tfp410_in: endpoint {
+                    pclk-sample = <1>;
+                    bus-width = <24>;
+                    remote-endpoint = <&dpi_out>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+                tfp410_out: endpoint {
+                    remote-endpoint = <&dvi_connector_in>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Convert the DT binding documentation for the TI TFP410 DPI-to-DVI
encoder to json-schema.

Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
---
Hi all,

I found some issues while converting this binding and I'd like to know
your opinions on how to tackle them.

1) dtbs_check fails for arch/arm/boot/dts/dove-sbc-a510.dts

  This board uses the TFP410 encoder but it doesn't define any ports for
  it. I can't find any suitable remote endpoints in its description
  either. Maybe this board description should be reworked? The current
  driver won't handle the device if it doesn't define any ports or
  endpoints anyway.

  It also uses the 'powerdown-gpio' property instead of
  'powerdown-gpios'. AFAICT this shouldn't be a problem from the driver
  point of view, but the general standard in DT bindings is to use the
  plural. This is trivial to fix.

2) The definition of ti,deskew in the original binding seems to be
tailored to the current driver and the way it's defined may not be very
DT-friendly.

  This parameter maps to a 3-bit field in a hardware register that takes
  a value from 0 to 7, so the [-4, 3] range described for this would map
  to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.

  Then, the driver parses the parameter (unsigned) and casts it to a
  signed integer to get a number in the [-4, 3] range.

  A vendor-specific property must have a type definition in json-schema,
  so if I translate the original bindings semantics directly, I should
  define ti,deskew as an int32, but this makes dt_binding_check fail if
  the property has a negative value in the example because of the
  internal representation of cells as unsigned integers:

     ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647

  So I can think of two solutions to this:

  a) Keep the ti,deskew property as an uint32 and document the valid
  range ([-4, 3]) in the property description (this is what this patch
  does currently).

  b) Redefine this property to be closer to the datasheet description
  (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
  This would also let us define its range properly using minimum and
  maximum properties for it.

  I think (b) is the right thing to do but I want to know your
  opinion. Besides, I don't have this hardware at hand and if I updated
  the driver I wouldn't be able to test it.

Thanks.

Patch tested with:

make dt_binding_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>
make dtbs_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>

 .../bindings/display/bridge/ti,tfp410.txt     |  66 ----------
 .../bindings/display/bridge/ti,tfp410.yaml    | 121 ++++++++++++++++++
 2 files changed, 121 insertions(+), 66 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
deleted file mode 100644
index 5ff4f64ef8e8..000000000000
--- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
+++ /dev/null
@@ -1,66 +0,0 @@
-TFP410 DPI to DVI encoder
-=========================
-
-Required properties:
-- compatible: "ti,tfp410"
-
-Optional properties:
-- powerdown-gpios: power-down gpio
-- reg: I2C address. If and only if present the device node should be placed
-  into the I2C controller node where the TFP410 I2C is connected to.
-- ti,deskew: data de-skew in 350ps increments, from -4 to +3, as configured
-  through th DK[3:1] pins. This property shall be present only if the TFP410
-  is not connected through I2C.
-
-Required nodes:
-
-This device has two video ports. Their connections are modeled using the OF
-graph bindings specified in [1]. Each port node shall have a single endpoint.
-
-- Port 0 is the DPI input port. Its endpoint subnode shall contain a
-  pclk-sample and bus-width property and a remote-endpoint property as specified
-  in [1].
-  - If pclk-sample is not defined, pclk-sample = 0 should be assumed for
-    backward compatibility.
-  - If bus-width is not defined then bus-width = 24 should be assumed for
-    backward compatibility.
-    bus-width = 24: 24 data lines are connected and single-edge mode
-    bus-width = 12: 12 data lines are connected and dual-edge mode
-
-- Port 1 is the DVI output port. Its endpoint subnode shall contain a
-  remote-endpoint property is specified in [1].
-
-[1] Documentation/devicetree/bindings/media/video-interfaces.txt
-
-
-Example
--------
-
-tfp410: encoder@0 {
-	compatible = "ti,tfp410";
-	powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
-	ti,deskew = <4>;
-
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@0 {
-			reg = <0>;
-
-			tfp410_in: endpoint@0 {
-				pclk-sample = <1>;
-				bus-width = <24>;
-				remote-endpoint = <&dpi_out>;
-			};
-		};
-
-		port@1 {
-			reg = <1>;
-
-			tfp410_out: endpoint@0 {
-				remote-endpoint = <&dvi_connector_in>;
-			};
-		};
-	};
-};
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
new file mode 100644
index 000000000000..79666ee540f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
@@ -0,0 +1,121 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/ti,tfp410.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TFP410 DPI to DVI encoder
+
+maintainers:
+  - Tomi Valkeinen <tomi.valkeinen@ti.com>
+  - Jyri Sarha <jsarha@ti.com>
+
+properties:
+  compatible:
+    const: "ti,tfp410"
+
+  reg:
+    description: I2C address of the device.
+    maxItems: 1
+
+  powerdown-gpios:
+    maxItems: 1
+
+  ti,deskew:
+    description:
+      Data de-skew in 350ps increments, from -4 to +3, as configured
+      through the DK[3:1] pins. This property shall be present only if
+      the TFP410 is not connected through I2C.
+    maxItems: 1
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
+  ports:
+    description:
+      A node containing input and output port nodes with endpoint
+      definitions as documented in
+      Documentation/devicetree/bindings/media/video-interfaces.txt
+    type: object
+
+    properties:
+      port@0:
+        description: DPI input port.
+        type: object
+
+        properties:
+          reg:
+            const: 0
+
+          endpoint:
+            type: object
+
+            properties:
+              pclk-sample:
+                description: Endpoint sampling edge.
+                enum:
+                  - 0  # Falling edge
+                  - 1  # Rising edge
+
+              bus-width:
+                description: Endpoint bus width.
+                enum: [ 12, 24 ]
+
+        required:
+          - endpoint
+
+      port@1:
+        description: DVI output port.
+        type: object
+
+        properties:
+          reg:
+            const: 1
+
+          endpoint:
+            type: object
+
+        required:
+          - endpoint
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    tfp410: encoder {
+        compatible = "ti,tfp410";
+        powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
+        ti,deskew = <3>;
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+                tfp410_in: endpoint {
+                    pclk-sample = <1>;
+                    bus-width = <24>;
+                    remote-endpoint = <&dpi_out>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+                tfp410_out: endpoint {
+                    remote-endpoint = <&dvi_connector_in>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Tomi Valkeinen]

On 28/04/2020 12:20, Ricardo Cañuelo wrote:

> 2) The definition of ti,deskew in the original binding seems to be
> tailored to the current driver and the way it's defined may not be very
> DT-friendly.
> 
>    This parameter maps to a 3-bit field in a hardware register that takes
>    a value from 0 to 7, so the [-4, 3] range described for this would map
>    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> 
>    Then, the driver parses the parameter (unsigned) and casts it to a
>    signed integer to get a number in the [-4, 3] range.

Interestingly the current example has ti,deskew = <4>...

>    A vendor-specific property must have a type definition in json-schema,
>    so if I translate the original bindings semantics directly, I should
>    define ti,deskew as an int32, but this makes dt_binding_check fail if
>    the property has a negative value in the example because of the
>    internal representation of cells as unsigned integers:
> 
>       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647

I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?

>    So I can think of two solutions to this:
> 
>    a) Keep the ti,deskew property as an uint32 and document the valid
>    range ([-4, 3]) in the property description (this is what this patch
>    does currently).
> 
>    b) Redefine this property to be closer to the datasheet description
>    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
>    This would also let us define its range properly using minimum and
>    maximum properties for it.
> 
>    I think (b) is the right thing to do but I want to know your
>    opinion. Besides, I don't have this hardware at hand and if I updated
>    the driver I wouldn't be able to test it.

I don't think anyone has used deskew property, so I guess changing it is not out of the question.

Laurent, did you have a board that needs deskew when you added it to tfp410?

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Tomi Valkeinen]

On 28/04/2020 12:20, Ricardo Cañuelo wrote:

> 2) The definition of ti,deskew in the original binding seems to be
> tailored to the current driver and the way it's defined may not be very
> DT-friendly.
> 
>    This parameter maps to a 3-bit field in a hardware register that takes
>    a value from 0 to 7, so the [-4, 3] range described for this would map
>    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> 
>    Then, the driver parses the parameter (unsigned) and casts it to a
>    signed integer to get a number in the [-4, 3] range.

Interestingly the current example has ti,deskew = <4>...

>    A vendor-specific property must have a type definition in json-schema,
>    so if I translate the original bindings semantics directly, I should
>    define ti,deskew as an int32, but this makes dt_binding_check fail if
>    the property has a negative value in the example because of the
>    internal representation of cells as unsigned integers:
> 
>       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647

I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?

>    So I can think of two solutions to this:
> 
>    a) Keep the ti,deskew property as an uint32 and document the valid
>    range ([-4, 3]) in the property description (this is what this patch
>    does currently).
> 
>    b) Redefine this property to be closer to the datasheet description
>    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
>    This would also let us define its range properly using minimum and
>    maximum properties for it.
> 
>    I think (b) is the right thing to do but I want to know your
>    opinion. Besides, I don't have this hardware at hand and if I updated
>    the driver I wouldn't be able to test it.

I don't think anyone has used deskew property, so I guess changing it is not out of the question.

Laurent, did you have a board that needs deskew when you added it to tfp410?

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Tomi Valkeinen]

On 28/04/2020 12:20, Ricardo Cañuelo wrote:

> 2) The definition of ti,deskew in the original binding seems to be
> tailored to the current driver and the way it's defined may not be very
> DT-friendly.
> 
>    This parameter maps to a 3-bit field in a hardware register that takes
>    a value from 0 to 7, so the [-4, 3] range described for this would map
>    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> 
>    Then, the driver parses the parameter (unsigned) and casts it to a
>    signed integer to get a number in the [-4, 3] range.

Interestingly the current example has ti,deskew = <4>...

>    A vendor-specific property must have a type definition in json-schema,
>    so if I translate the original bindings semantics directly, I should
>    define ti,deskew as an int32, but this makes dt_binding_check fail if
>    the property has a negative value in the example because of the
>    internal representation of cells as unsigned integers:
> 
>       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647

I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?

>    So I can think of two solutions to this:
> 
>    a) Keep the ti,deskew property as an uint32 and document the valid
>    range ([-4, 3]) in the property description (this is what this patch
>    does currently).
> 
>    b) Redefine this property to be closer to the datasheet description
>    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
>    This would also let us define its range properly using minimum and
>    maximum properties for it.
> 
>    I think (b) is the right thing to do but I want to know your
>    opinion. Besides, I don't have this hardware at hand and if I updated
>    the driver I wouldn't be able to test it.

I don't think anyone has used deskew property, so I guess changing it is not out of the question.

Laurent, did you have a board that needs deskew when you added it to tfp410?

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Sam Ravnborg]

Hi Ricardo.

Thanks for looking into this bridge binding.
Some comments in the following.

	Sam

On Tue, Apr 28, 2020 at 11:20:48AM +0200, Ricardo Cañuelo wrote:
> Convert the DT binding documentation for the TI TFP410 DPI-to-DVI
> encoder to json-schema.
> 
> Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
> ---
> Hi all,
> 
> I found some issues while converting this binding and I'd like to know
> your opinions on how to tackle them.
> 
> 1) dtbs_check fails for arch/arm/boot/dts/dove-sbc-a510.dts
> 
>   This board uses the TFP410 encoder but it doesn't define any ports for
>   it. I can't find any suitable remote endpoints in its description
>   either. Maybe this board description should be reworked? The current
>   driver won't handle the device if it doesn't define any ports or
>   endpoints anyway.
> 

It is expected that there are a few DT files that will need an update
when they are checked more fomally.
So post a sepearate patch to fix it - preferably before the
conversion.

>   It also uses the 'powerdown-gpio' property instead of
>   'powerdown-gpios'. AFAICT this shouldn't be a problem from the driver
>   point of view, but the general standard in DT bindings is to use the
>   plural. This is trivial to fix.
Use the plural form as everyone else.

> 2) The definition of ti,deskew in the original binding seems to be
> tailored to the current driver and the way it's defined may not be very
> DT-friendly.
> 
>   This parameter maps to a 3-bit field in a hardware register that takes
>   a value from 0 to 7, so the [-4, 3] range described for this would map
>   to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> 
>   Then, the driver parses the parameter (unsigned) and casts it to a
>   signed integer to get a number in the [-4, 3] range.
> 
>   A vendor-specific property must have a type definition in json-schema,
>   so if I translate the original bindings semantics directly, I should
>   define ti,deskew as an int32, but this makes dt_binding_check fail if
>   the property has a negative value in the example because of the
>   internal representation of cells as unsigned integers:
> 
>      ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647

Can you define it as an enum like this:

	enum: [-4, -3, -2, -1, 0, 1, 2, 3]

And then maybe in the descrition describe how they map to 0..7.
The problem is that the binding is an API so we cannot just change
the interpretation of the value 0 etc.


> 
>   So I can think of two solutions to this:
> 
>   a) Keep the ti,deskew property as an uint32 and document the valid
>   range ([-4, 3]) in the property description (this is what this patch
>   does currently).
> 
>   b) Redefine this property to be closer to the datasheet description
>   (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
>   This would also let us define its range properly using minimum and
>   maximum properties for it.
> 
>   I think (b) is the right thing to do but I want to know your
>   opinion. Besides, I don't have this hardware at hand and if I updated
>   the driver I wouldn't be able to test it.
> 
> Thanks.
> 
> Patch tested with:
> 
> make dt_binding_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>
> make dtbs_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>
> 
>  .../bindings/display/bridge/ti,tfp410.txt     |  66 ----------
>  .../bindings/display/bridge/ti,tfp410.yaml    | 121 ++++++++++++++++++
>  2 files changed, 121 insertions(+), 66 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> deleted file mode 100644
> index 5ff4f64ef8e8..000000000000
> --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> +++ /dev/null
> @@ -1,66 +0,0 @@
> -TFP410 DPI to DVI encoder
> -=========================
> -
> -Required properties:
> -- compatible: "ti,tfp410"
> -
> -Optional properties:
> -- powerdown-gpios: power-down gpio
> -- reg: I2C address. If and only if present the device node should be placed
> -  into the I2C controller node where the TFP410 I2C is connected to.
> -- ti,deskew: data de-skew in 350ps increments, from -4 to +3, as configured
> -  through th DK[3:1] pins. This property shall be present only if the TFP410
> -  is not connected through I2C.
> -
> -Required nodes:
> -
> -This device has two video ports. Their connections are modeled using the OF
> -graph bindings specified in [1]. Each port node shall have a single endpoint.
> -
> -- Port 0 is the DPI input port. Its endpoint subnode shall contain a
> -  pclk-sample and bus-width property and a remote-endpoint property as specified
> -  in [1].

> -  - If pclk-sample is not defined, pclk-sample = 0 should be assumed for
> -    backward compatibility.
> -  - If bus-width is not defined then bus-width = 24 should be assumed for
> -    backward compatibility.
> -    bus-width = 24: 24 data lines are connected and single-edge mode
> -    bus-width = 12: 12 data lines are connected and dual-edge mode
These comments are missing in the new binding.


> -
> -- Port 1 is the DVI output port. Its endpoint subnode shall contain a
> -  remote-endpoint property is specified in [1].
> -
> -[1] Documentation/devicetree/bindings/media/video-interfaces.txt
> -
> -
> -Example
> --------
> -
> -tfp410: encoder@0 {
> -	compatible = "ti,tfp410";
> -	powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
> -	ti,deskew = <4>;
> -
> -	ports {
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
> -		port@0 {
> -			reg = <0>;
> -
> -			tfp410_in: endpoint@0 {
> -				pclk-sample = <1>;
> -				bus-width = <24>;
> -				remote-endpoint = <&dpi_out>;
> -			};
> -		};
> -
> -		port@1 {
> -			reg = <1>;
> -
> -			tfp410_out: endpoint@0 {
> -				remote-endpoint = <&dvi_connector_in>;
> -			};
> -		};
> -	};
> -};
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> new file mode 100644
> index 000000000000..79666ee540f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> @@ -0,0 +1,121 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
You need to obtain approval to change the license to include BSD.
The current binfing is GPL-2-0-only as it is default in the tree.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ti,tfp410.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TFP410 DPI to DVI encoder
> +
> +maintainers:
> +  - Tomi Valkeinen <tomi.valkeinen@ti.com>
> +  - Jyri Sarha <jsarha@ti.com>
> +
> +properties:
> +  compatible:
> +    const: "ti,tfp410"
No "" around the compatible name

> +
> +  reg:
> +    description: I2C address of the device.
> +    maxItems: 1
> +
> +  powerdown-gpios:
> +    maxItems: 1
> +
> +  ti,deskew:
> +    description:
> +      Data de-skew in 350ps increments, from -4 to +3, as configured
> +      through the DK[3:1] pins. This property shall be present only if
> +      the TFP410 is not connected through I2C.
> +    maxItems: 1
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
No need for the allOf: - $ref.
Just use a plain $ref


> +
> +  ports:
> +    description:
> +      A node containing input and output port nodes with endpoint
> +      definitions as documented in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt
> +    type: object
> +
> +    properties:
> +      port@0:
> +        description: DPI input port.
> +        type: object
> +
> +        properties:
> +          reg:
> +            const: 0
> +
> +          endpoint:
> +            type: object
> +
> +            properties:
> +              pclk-sample:
> +                description: Endpoint sampling edge.
> +                enum:
> +                  - 0  # Falling edge
> +                  - 1  # Rising edge
> +
> +              bus-width:
> +                description: Endpoint bus width.
> +                enum: [ 12, 24 ]
> +
> +        required:
> +          - endpoint
> +
> +      port@1:
> +        description: DVI output port.
> +        type: object
> +
> +        properties:
> +          reg:
> +            const: 1
> +
> +          endpoint:
> +            type: object
> +
> +        required:
> +          - endpoint
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    tfp410: encoder {
> +        compatible = "ti,tfp410";
> +        powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
> +        ti,deskew = <3>;
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +                tfp410_in: endpoint {
> +                    pclk-sample = <1>;
> +                    bus-width = <24>;
> +                    remote-endpoint = <&dpi_out>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +                tfp410_out: endpoint {
> +                    remote-endpoint = <&dvi_connector_in>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> -- 
> 2.18.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Sam Ravnborg]

Hi Ricardo.

Thanks for looking into this bridge binding.
Some comments in the following.

	Sam

On Tue, Apr 28, 2020 at 11:20:48AM +0200, Ricardo Cañuelo wrote:
> Convert the DT binding documentation for the TI TFP410 DPI-to-DVI
> encoder to json-schema.
> 
> Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
> ---
> Hi all,
> 
> I found some issues while converting this binding and I'd like to know
> your opinions on how to tackle them.
> 
> 1) dtbs_check fails for arch/arm/boot/dts/dove-sbc-a510.dts
> 
>   This board uses the TFP410 encoder but it doesn't define any ports for
>   it. I can't find any suitable remote endpoints in its description
>   either. Maybe this board description should be reworked? The current
>   driver won't handle the device if it doesn't define any ports or
>   endpoints anyway.
> 

It is expected that there are a few DT files that will need an update
when they are checked more fomally.
So post a sepearate patch to fix it - preferably before the
conversion.

>   It also uses the 'powerdown-gpio' property instead of
>   'powerdown-gpios'. AFAICT this shouldn't be a problem from the driver
>   point of view, but the general standard in DT bindings is to use the
>   plural. This is trivial to fix.
Use the plural form as everyone else.

> 2) The definition of ti,deskew in the original binding seems to be
> tailored to the current driver and the way it's defined may not be very
> DT-friendly.
> 
>   This parameter maps to a 3-bit field in a hardware register that takes
>   a value from 0 to 7, so the [-4, 3] range described for this would map
>   to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> 
>   Then, the driver parses the parameter (unsigned) and casts it to a
>   signed integer to get a number in the [-4, 3] range.
> 
>   A vendor-specific property must have a type definition in json-schema,
>   so if I translate the original bindings semantics directly, I should
>   define ti,deskew as an int32, but this makes dt_binding_check fail if
>   the property has a negative value in the example because of the
>   internal representation of cells as unsigned integers:
> 
>      ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647

Can you define it as an enum like this:

	enum: [-4, -3, -2, -1, 0, 1, 2, 3]

And then maybe in the descrition describe how they map to 0..7.
The problem is that the binding is an API so we cannot just change
the interpretation of the value 0 etc.


> 
>   So I can think of two solutions to this:
> 
>   a) Keep the ti,deskew property as an uint32 and document the valid
>   range ([-4, 3]) in the property description (this is what this patch
>   does currently).
> 
>   b) Redefine this property to be closer to the datasheet description
>   (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
>   This would also let us define its range properly using minimum and
>   maximum properties for it.
> 
>   I think (b) is the right thing to do but I want to know your
>   opinion. Besides, I don't have this hardware at hand and if I updated
>   the driver I wouldn't be able to test it.
> 
> Thanks.
> 
> Patch tested with:
> 
> make dt_binding_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>
> make dtbs_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>
> 
>  .../bindings/display/bridge/ti,tfp410.txt     |  66 ----------
>  .../bindings/display/bridge/ti,tfp410.yaml    | 121 ++++++++++++++++++
>  2 files changed, 121 insertions(+), 66 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> deleted file mode 100644
> index 5ff4f64ef8e8..000000000000
> --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> +++ /dev/null
> @@ -1,66 +0,0 @@
> -TFP410 DPI to DVI encoder
> -=========================
> -
> -Required properties:
> -- compatible: "ti,tfp410"
> -
> -Optional properties:
> -- powerdown-gpios: power-down gpio
> -- reg: I2C address. If and only if present the device node should be placed
> -  into the I2C controller node where the TFP410 I2C is connected to.
> -- ti,deskew: data de-skew in 350ps increments, from -4 to +3, as configured
> -  through th DK[3:1] pins. This property shall be present only if the TFP410
> -  is not connected through I2C.
> -
> -Required nodes:
> -
> -This device has two video ports. Their connections are modeled using the OF
> -graph bindings specified in [1]. Each port node shall have a single endpoint.
> -
> -- Port 0 is the DPI input port. Its endpoint subnode shall contain a
> -  pclk-sample and bus-width property and a remote-endpoint property as specified
> -  in [1].

> -  - If pclk-sample is not defined, pclk-sample = 0 should be assumed for
> -    backward compatibility.
> -  - If bus-width is not defined then bus-width = 24 should be assumed for
> -    backward compatibility.
> -    bus-width = 24: 24 data lines are connected and single-edge mode
> -    bus-width = 12: 12 data lines are connected and dual-edge mode
These comments are missing in the new binding.


> -
> -- Port 1 is the DVI output port. Its endpoint subnode shall contain a
> -  remote-endpoint property is specified in [1].
> -
> -[1] Documentation/devicetree/bindings/media/video-interfaces.txt
> -
> -
> -Example
> --------
> -
> -tfp410: encoder@0 {
> -	compatible = "ti,tfp410";
> -	powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
> -	ti,deskew = <4>;
> -
> -	ports {
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
> -		port@0 {
> -			reg = <0>;
> -
> -			tfp410_in: endpoint@0 {
> -				pclk-sample = <1>;
> -				bus-width = <24>;
> -				remote-endpoint = <&dpi_out>;
> -			};
> -		};
> -
> -		port@1 {
> -			reg = <1>;
> -
> -			tfp410_out: endpoint@0 {
> -				remote-endpoint = <&dvi_connector_in>;
> -			};
> -		};
> -	};
> -};
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> new file mode 100644
> index 000000000000..79666ee540f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> @@ -0,0 +1,121 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
You need to obtain approval to change the license to include BSD.
The current binfing is GPL-2-0-only as it is default in the tree.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ti,tfp410.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TFP410 DPI to DVI encoder
> +
> +maintainers:
> +  - Tomi Valkeinen <tomi.valkeinen@ti.com>
> +  - Jyri Sarha <jsarha@ti.com>
> +
> +properties:
> +  compatible:
> +    const: "ti,tfp410"
No "" around the compatible name

> +
> +  reg:
> +    description: I2C address of the device.
> +    maxItems: 1
> +
> +  powerdown-gpios:
> +    maxItems: 1
> +
> +  ti,deskew:
> +    description:
> +      Data de-skew in 350ps increments, from -4 to +3, as configured
> +      through the DK[3:1] pins. This property shall be present only if
> +      the TFP410 is not connected through I2C.
> +    maxItems: 1
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
No need for the allOf: - $ref.
Just use a plain $ref


> +
> +  ports:
> +    description:
> +      A node containing input and output port nodes with endpoint
> +      definitions as documented in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt
> +    type: object
> +
> +    properties:
> +      port@0:
> +        description: DPI input port.
> +        type: object
> +
> +        properties:
> +          reg:
> +            const: 0
> +
> +          endpoint:
> +            type: object
> +
> +            properties:
> +              pclk-sample:
> +                description: Endpoint sampling edge.
> +                enum:
> +                  - 0  # Falling edge
> +                  - 1  # Rising edge
> +
> +              bus-width:
> +                description: Endpoint bus width.
> +                enum: [ 12, 24 ]
> +
> +        required:
> +          - endpoint
> +
> +      port@1:
> +        description: DVI output port.
> +        type: object
> +
> +        properties:
> +          reg:
> +            const: 1
> +
> +          endpoint:
> +            type: object
> +
> +        required:
> +          - endpoint
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    tfp410: encoder {
> +        compatible = "ti,tfp410";
> +        powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
> +        ti,deskew = <3>;
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +                tfp410_in: endpoint {
> +                    pclk-sample = <1>;
> +                    bus-width = <24>;
> +                    remote-endpoint = <&dpi_out>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +                tfp410_out: endpoint {
> +                    remote-endpoint = <&dvi_connector_in>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> -- 
> 2.18.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Sam Ravnborg]

Hi Ricardo.

Thanks for looking into this bridge binding.
Some comments in the following.

	Sam

On Tue, Apr 28, 2020 at 11:20:48AM +0200, Ricardo Cañuelo wrote:
> Convert the DT binding documentation for the TI TFP410 DPI-to-DVI
> encoder to json-schema.
> 
> Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
> ---
> Hi all,
> 
> I found some issues while converting this binding and I'd like to know
> your opinions on how to tackle them.
> 
> 1) dtbs_check fails for arch/arm/boot/dts/dove-sbc-a510.dts
> 
>   This board uses the TFP410 encoder but it doesn't define any ports for
>   it. I can't find any suitable remote endpoints in its description
>   either. Maybe this board description should be reworked? The current
>   driver won't handle the device if it doesn't define any ports or
>   endpoints anyway.
> 

It is expected that there are a few DT files that will need an update
when they are checked more fomally.
So post a sepearate patch to fix it - preferably before the
conversion.

>   It also uses the 'powerdown-gpio' property instead of
>   'powerdown-gpios'. AFAICT this shouldn't be a problem from the driver
>   point of view, but the general standard in DT bindings is to use the
>   plural. This is trivial to fix.
Use the plural form as everyone else.

> 2) The definition of ti,deskew in the original binding seems to be
> tailored to the current driver and the way it's defined may not be very
> DT-friendly.
> 
>   This parameter maps to a 3-bit field in a hardware register that takes
>   a value from 0 to 7, so the [-4, 3] range described for this would map
>   to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> 
>   Then, the driver parses the parameter (unsigned) and casts it to a
>   signed integer to get a number in the [-4, 3] range.
> 
>   A vendor-specific property must have a type definition in json-schema,
>   so if I translate the original bindings semantics directly, I should
>   define ti,deskew as an int32, but this makes dt_binding_check fail if
>   the property has a negative value in the example because of the
>   internal representation of cells as unsigned integers:
> 
>      ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647

Can you define it as an enum like this:

	enum: [-4, -3, -2, -1, 0, 1, 2, 3]

And then maybe in the descrition describe how they map to 0..7.
The problem is that the binding is an API so we cannot just change
the interpretation of the value 0 etc.


> 
>   So I can think of two solutions to this:
> 
>   a) Keep the ti,deskew property as an uint32 and document the valid
>   range ([-4, 3]) in the property description (this is what this patch
>   does currently).
> 
>   b) Redefine this property to be closer to the datasheet description
>   (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
>   This would also let us define its range properly using minimum and
>   maximum properties for it.
> 
>   I think (b) is the right thing to do but I want to know your
>   opinion. Besides, I don't have this hardware at hand and if I updated
>   the driver I wouldn't be able to test it.
> 
> Thanks.
> 
> Patch tested with:
> 
> make dt_binding_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>
> make dtbs_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml>
> 
>  .../bindings/display/bridge/ti,tfp410.txt     |  66 ----------
>  .../bindings/display/bridge/ti,tfp410.yaml    | 121 ++++++++++++++++++
>  2 files changed, 121 insertions(+), 66 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> deleted file mode 100644
> index 5ff4f64ef8e8..000000000000
> --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> +++ /dev/null
> @@ -1,66 +0,0 @@
> -TFP410 DPI to DVI encoder
> -=========================
> -
> -Required properties:
> -- compatible: "ti,tfp410"
> -
> -Optional properties:
> -- powerdown-gpios: power-down gpio
> -- reg: I2C address. If and only if present the device node should be placed
> -  into the I2C controller node where the TFP410 I2C is connected to.
> -- ti,deskew: data de-skew in 350ps increments, from -4 to +3, as configured
> -  through th DK[3:1] pins. This property shall be present only if the TFP410
> -  is not connected through I2C.
> -
> -Required nodes:
> -
> -This device has two video ports. Their connections are modeled using the OF
> -graph bindings specified in [1]. Each port node shall have a single endpoint.
> -
> -- Port 0 is the DPI input port. Its endpoint subnode shall contain a
> -  pclk-sample and bus-width property and a remote-endpoint property as specified
> -  in [1].

> -  - If pclk-sample is not defined, pclk-sample = 0 should be assumed for
> -    backward compatibility.
> -  - If bus-width is not defined then bus-width = 24 should be assumed for
> -    backward compatibility.
> -    bus-width = 24: 24 data lines are connected and single-edge mode
> -    bus-width = 12: 12 data lines are connected and dual-edge mode
These comments are missing in the new binding.


> -
> -- Port 1 is the DVI output port. Its endpoint subnode shall contain a
> -  remote-endpoint property is specified in [1].
> -
> -[1] Documentation/devicetree/bindings/media/video-interfaces.txt
> -
> -
> -Example
> --------
> -
> -tfp410: encoder@0 {
> -	compatible = "ti,tfp410";
> -	powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
> -	ti,deskew = <4>;
> -
> -	ports {
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
> -		port@0 {
> -			reg = <0>;
> -
> -			tfp410_in: endpoint@0 {
> -				pclk-sample = <1>;
> -				bus-width = <24>;
> -				remote-endpoint = <&dpi_out>;
> -			};
> -		};
> -
> -		port@1 {
> -			reg = <1>;
> -
> -			tfp410_out: endpoint@0 {
> -				remote-endpoint = <&dvi_connector_in>;
> -			};
> -		};
> -	};
> -};
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> new file mode 100644
> index 000000000000..79666ee540f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> @@ -0,0 +1,121 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
You need to obtain approval to change the license to include BSD.
The current binfing is GPL-2-0-only as it is default in the tree.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ti,tfp410.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TFP410 DPI to DVI encoder
> +
> +maintainers:
> +  - Tomi Valkeinen <tomi.valkeinen@ti.com>
> +  - Jyri Sarha <jsarha@ti.com>
> +
> +properties:
> +  compatible:
> +    const: "ti,tfp410"
No "" around the compatible name

> +
> +  reg:
> +    description: I2C address of the device.
> +    maxItems: 1
> +
> +  powerdown-gpios:
> +    maxItems: 1
> +
> +  ti,deskew:
> +    description:
> +      Data de-skew in 350ps increments, from -4 to +3, as configured
> +      through the DK[3:1] pins. This property shall be present only if
> +      the TFP410 is not connected through I2C.
> +    maxItems: 1
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
No need for the allOf: - $ref.
Just use a plain $ref


> +
> +  ports:
> +    description:
> +      A node containing input and output port nodes with endpoint
> +      definitions as documented in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt
> +    type: object
> +
> +    properties:
> +      port@0:
> +        description: DPI input port.
> +        type: object
> +
> +        properties:
> +          reg:
> +            const: 0
> +
> +          endpoint:
> +            type: object
> +
> +            properties:
> +              pclk-sample:
> +                description: Endpoint sampling edge.
> +                enum:
> +                  - 0  # Falling edge
> +                  - 1  # Rising edge
> +
> +              bus-width:
> +                description: Endpoint bus width.
> +                enum: [ 12, 24 ]
> +
> +        required:
> +          - endpoint
> +
> +      port@1:
> +        description: DVI output port.
> +        type: object
> +
> +        properties:
> +          reg:
> +            const: 1
> +
> +          endpoint:
> +            type: object
> +
> +        required:
> +          - endpoint
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    tfp410: encoder {
> +        compatible = "ti,tfp410";
> +        powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
> +        ti,deskew = <3>;
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +                tfp410_in: endpoint {
> +                    pclk-sample = <1>;
> +                    bus-width = <24>;
> +                    remote-endpoint = <&dpi_out>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +                tfp410_out: endpoint {
> +                    remote-endpoint = <&dvi_connector_in>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> -- 
> 2.18.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

On mar 28-04-2020 21:21:17, Sam Ravnborg wrote:
> Hi Ricardo.
> 
> Thanks for looking into this bridge binding.
> Some comments in the following.

Thanks for reviewing it, Sam. I agree with your suggestions, some
comments below:

> Can you define it as an enum like this:
> 
> 	enum: [-4, -3, -2, -1, 0, 1, 2, 3]
> 
> And then maybe in the descrition describe how they map to 0..7.
> The problem is that the binding is an API so we cannot just change
> the interpretation of the value 0 etc.

This is similar to what I wanted to do at first, specifying minimum and
maximum values, but it'd have the same problem with the yaml
checker. See, problem is in the yaml checking itself, the actual
parameter parsing in the driver already works with negative integers.

The issue with this particular property is that:

  - it's vendor specific, so it must have a type definition.

  - the actual value is signed, so the proper type definition to use
    should be a signed integer.

  - but then, if you use a negative value for this it will be cast into
    a very large u32 (that's what cells are, after all) beyond the
    maximum value of a positive int32.

  - Range constraints can't be properly checked either, since any
    negative integer interpreted as unsigned will result in a positive
    number beyond the top end of the value range (3 in this case).

So unless there's a way of making the yaml check process aware of
negative integers, I think the only way to pass the checks is to use
unsigned integers and avoid defining the range constraints.

FWIW, the only other similar case I found (adi,ltc2983.yaml) uses uint64
and no range constraints.

Cheers,
Ricardo

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

On mar 28-04-2020 21:21:17, Sam Ravnborg wrote:
> Hi Ricardo.
> 
> Thanks for looking into this bridge binding.
> Some comments in the following.

Thanks for reviewing it, Sam. I agree with your suggestions, some
comments below:

> Can you define it as an enum like this:
> 
> 	enum: [-4, -3, -2, -1, 0, 1, 2, 3]
> 
> And then maybe in the descrition describe how they map to 0..7.
> The problem is that the binding is an API so we cannot just change
> the interpretation of the value 0 etc.

This is similar to what I wanted to do at first, specifying minimum and
maximum values, but it'd have the same problem with the yaml
checker. See, problem is in the yaml checking itself, the actual
parameter parsing in the driver already works with negative integers.

The issue with this particular property is that:

  - it's vendor specific, so it must have a type definition.

  - the actual value is signed, so the proper type definition to use
    should be a signed integer.

  - but then, if you use a negative value for this it will be cast into
    a very large u32 (that's what cells are, after all) beyond the
    maximum value of a positive int32.

  - Range constraints can't be properly checked either, since any
    negative integer interpreted as unsigned will result in a positive
    number beyond the top end of the value range (3 in this case).

So unless there's a way of making the yaml check process aware of
negative integers, I think the only way to pass the checks is to use
unsigned integers and avoid defining the range constraints.

FWIW, the only other similar case I found (adi,ltc2983.yaml) uses uint64
and no range constraints.

Cheers,
Ricardo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

On mar 28-04-2020 21:21:17, Sam Ravnborg wrote:
> Hi Ricardo.
> 
> Thanks for looking into this bridge binding.
> Some comments in the following.

Thanks for reviewing it, Sam. I agree with your suggestions, some
comments below:

> Can you define it as an enum like this:
> 
> 	enum: [-4, -3, -2, -1, 0, 1, 2, 3]
> 
> And then maybe in the descrition describe how they map to 0..7.
> The problem is that the binding is an API so we cannot just change
> the interpretation of the value 0 etc.

This is similar to what I wanted to do at first, specifying minimum and
maximum values, but it'd have the same problem with the yaml
checker. See, problem is in the yaml checking itself, the actual
parameter parsing in the driver already works with negative integers.

The issue with this particular property is that:

  - it's vendor specific, so it must have a type definition.

  - the actual value is signed, so the proper type definition to use
    should be a signed integer.

  - but then, if you use a negative value for this it will be cast into
    a very large u32 (that's what cells are, after all) beyond the
    maximum value of a positive int32.

  - Range constraints can't be properly checked either, since any
    negative integer interpreted as unsigned will result in a positive
    number beyond the top end of the value range (3 in this case).

So unless there's a way of making the yaml check process aware of
negative integers, I think the only way to pass the checks is to use
unsigned integers and avoid defining the range constraints.

FWIW, the only other similar case I found (adi,ltc2983.yaml) uses uint64
and no range constraints.

Cheers,
Ricardo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Tomi, thanks for reviewing the patch.

On mar 28-04-2020 12:49:28, Tomi Valkeinen wrote: 
> I don't quite understand this. We cannot have negative numbers in dts files?
> Or we can, but dt_binding_check doesn't handle them correctly? Or that int32
> is not supported in yaml bindings?

AFAICT, you can have negative numbers in dts files (see [1] and [2]) and
the DT schema certainly supports signed integers, but dt_binding_check
seems to interpret all cells as unsigned 32bit integers because that's
what they are, really. In kernel code this is not a problem because you
can cast the value back to a signed int before you run your own sanity
checks on them.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/159682.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/159681.html

Cheers,
Ricardo

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Tomi, thanks for reviewing the patch.

On mar 28-04-2020 12:49:28, Tomi Valkeinen wrote: 
> I don't quite understand this. We cannot have negative numbers in dts files?
> Or we can, but dt_binding_check doesn't handle them correctly? Or that int32
> is not supported in yaml bindings?

AFAICT, you can have negative numbers in dts files (see [1] and [2]) and
the DT schema certainly supports signed integers, but dt_binding_check
seems to interpret all cells as unsigned 32bit integers because that's
what they are, really. In kernel code this is not a problem because you
can cast the value back to a signed int before you run your own sanity
checks on them.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/159682.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/159681.html

Cheers,
Ricardo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Tomi, thanks for reviewing the patch.

On mar 28-04-2020 12:49:28, Tomi Valkeinen wrote: 
> I don't quite understand this. We cannot have negative numbers in dts files?
> Or we can, but dt_binding_check doesn't handle them correctly? Or that int32
> is not supported in yaml bindings?

AFAICT, you can have negative numbers in dts files (see [1] and [2]) and
the DT schema certainly supports signed integers, but dt_binding_check
seems to interpret all cells as unsigned 32bit integers because that's
what they are, really. In kernel code this is not a problem because you
can cast the value back to a signed int before you run your own sanity
checks on them.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/159682.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/159681.html

Cheers,
Ricardo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Tomi Valkeinen]

Hi Ricardo,

On 06/05/2020 10:21, Ricardo Cañuelo wrote:
> Hi Tomi, thanks for reviewing the patch.
> 
> On mar 28-04-2020 12:49:28, Tomi Valkeinen wrote:
>> I don't quite understand this. We cannot have negative numbers in dts files?
>> Or we can, but dt_binding_check doesn't handle them correctly? Or that int32
>> is not supported in yaml bindings?
> 
> AFAICT, you can have negative numbers in dts files (see [1] and [2]) and

This is also my understanding after some googling. And there's even of_property_read_s32() in the 
kernel.

> the DT schema certainly supports signed integers, but dt_binding_check
> seems to interpret all cells as unsigned 32bit integers because that's
> what they are, really. In kernel code this is not a problem because you

Well, this is in the nitpick category, and maybe not even relevant, but I don't think that's 
correct. They're just bits. Some pieces of SW happen to use u32 containers to store the bits. But 
what the bits mean is not related to the container.

> can cast the value back to a signed int before you run your own sanity
> checks on them.

Doesn't all this just point to a bug or missing feature in dt_binding_check? That's not a reason to 
change the ABI.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Tomi Valkeinen]

Hi Ricardo,

On 06/05/2020 10:21, Ricardo Cañuelo wrote:
> Hi Tomi, thanks for reviewing the patch.
> 
> On mar 28-04-2020 12:49:28, Tomi Valkeinen wrote:
>> I don't quite understand this. We cannot have negative numbers in dts files?
>> Or we can, but dt_binding_check doesn't handle them correctly? Or that int32
>> is not supported in yaml bindings?
> 
> AFAICT, you can have negative numbers in dts files (see [1] and [2]) and

This is also my understanding after some googling. And there's even of_property_read_s32() in the 
kernel.

> the DT schema certainly supports signed integers, but dt_binding_check
> seems to interpret all cells as unsigned 32bit integers because that's
> what they are, really. In kernel code this is not a problem because you

Well, this is in the nitpick category, and maybe not even relevant, but I don't think that's 
correct. They're just bits. Some pieces of SW happen to use u32 containers to store the bits. But 
what the bits mean is not related to the container.

> can cast the value back to a signed int before you run your own sanity
> checks on them.

Doesn't all this just point to a bug or missing feature in dt_binding_check? That's not a reason to 
change the ABI.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Tomi Valkeinen]

Hi Ricardo,

On 06/05/2020 10:21, Ricardo Cañuelo wrote:
> Hi Tomi, thanks for reviewing the patch.
> 
> On mar 28-04-2020 12:49:28, Tomi Valkeinen wrote:
>> I don't quite understand this. We cannot have negative numbers in dts files?
>> Or we can, but dt_binding_check doesn't handle them correctly? Or that int32
>> is not supported in yaml bindings?
> 
> AFAICT, you can have negative numbers in dts files (see [1] and [2]) and

This is also my understanding after some googling. And there's even of_property_read_s32() in the 
kernel.

> the DT schema certainly supports signed integers, but dt_binding_check
> seems to interpret all cells as unsigned 32bit integers because that's
> what they are, really. In kernel code this is not a problem because you

Well, this is in the nitpick category, and maybe not even relevant, but I don't think that's 
correct. They're just bits. Some pieces of SW happen to use u32 containers to store the bits. But 
what the bits mean is not related to the container.

> can cast the value back to a signed int before you run your own sanity
> checks on them.

Doesn't all this just point to a bug or missing feature in dt_binding_check? That's not a reason to 
change the ABI.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Tomi,

On mié 06-05-2020 11:01:07, Tomi Valkeinen wrote: 
> Doesn't all this just point to a bug or missing feature in dt_binding_check?
> That's not a reason to change the ABI.

I agree and I'd vote for "missing feature", but seeing that there aren't
any other examples of this use case in the whole kernel dts collection
(at least I couldn't find any) I thought that maybe it's us who are
going against the norm here.

Maybe Rob can shed some light about this?

Cheers,
Ricardo

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Tomi,

On mié 06-05-2020 11:01:07, Tomi Valkeinen wrote: 
> Doesn't all this just point to a bug or missing feature in dt_binding_check?
> That's not a reason to change the ABI.

I agree and I'd vote for "missing feature", but seeing that there aren't
any other examples of this use case in the whole kernel dts collection
(at least I couldn't find any) I thought that maybe it's us who are
going against the norm here.

Maybe Rob can shed some light about this?

Cheers,
Ricardo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Tomi,

On mié 06-05-2020 11:01:07, Tomi Valkeinen wrote: 
> Doesn't all this just point to a bug or missing feature in dt_binding_check?
> That's not a reason to change the ABI.

I agree and I'd vote for "missing feature", but seeing that there aren't
any other examples of this use case in the whole kernel dts collection
(at least I couldn't find any) I thought that maybe it's us who are
going against the norm here.

Maybe Rob can shed some light about this?

Cheers,
Ricardo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Tomi Valkeinen]

On 06/05/2020 11:28, Ricardo Cañuelo wrote:
> Hi Tomi,
> 
> On mié 06-05-2020 11:01:07, Tomi Valkeinen wrote:
>> Doesn't all this just point to a bug or missing feature in dt_binding_check?
>> That's not a reason to change the ABI.
> 
> I agree and I'd vote for "missing feature", but seeing that there aren't
> any other examples of this use case in the whole kernel dts collection
> (at least I couldn't find any) I thought that maybe it's us who are
> going against the norm here.

A valid point. I'm not aware of anyone using the deskew property. My guess is that Laurent added it 
just because it was in the spec, not because he had a need for it. So I don't think changing the 
binding is totally out of the question.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Tomi Valkeinen]

On 06/05/2020 11:28, Ricardo Cañuelo wrote:
> Hi Tomi,
> 
> On mié 06-05-2020 11:01:07, Tomi Valkeinen wrote:
>> Doesn't all this just point to a bug or missing feature in dt_binding_check?
>> That's not a reason to change the ABI.
> 
> I agree and I'd vote for "missing feature", but seeing that there aren't
> any other examples of this use case in the whole kernel dts collection
> (at least I couldn't find any) I thought that maybe it's us who are
> going against the norm here.

A valid point. I'm not aware of anyone using the deskew property. My guess is that Laurent added it 
just because it was in the spec, not because he had a need for it. So I don't think changing the 
binding is totally out of the question.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Tomi Valkeinen]

On 06/05/2020 11:28, Ricardo Cañuelo wrote:
> Hi Tomi,
> 
> On mié 06-05-2020 11:01:07, Tomi Valkeinen wrote:
>> Doesn't all this just point to a bug or missing feature in dt_binding_check?
>> That's not a reason to change the ABI.
> 
> I agree and I'd vote for "missing feature", but seeing that there aren't
> any other examples of this use case in the whole kernel dts collection
> (at least I couldn't find any) I thought that maybe it's us who are
> going against the norm here.

A valid point. I'm not aware of anyone using the deskew property. My guess is that Laurent added it 
just because it was in the spec, not because he had a need for it. So I don't think changing the 
binding is totally out of the question.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Laurent Pinchart]

Hi Tomi,

On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote:
> On 28/04/2020 12:20, Ricardo Cañuelo wrote:
> 
> > 2) The definition of ti,deskew in the original binding seems to be
> > tailored to the current driver and the way it's defined may not be very
> > DT-friendly.
> > 
> >    This parameter maps to a 3-bit field in a hardware register that takes
> >    a value from 0 to 7, so the [-4, 3] range described for this would map
> >    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> > 
> >    Then, the driver parses the parameter (unsigned) and casts it to a
> >    signed integer to get a number in the [-4, 3] range.
> 
> Interestingly the current example has ti,deskew = <4>...
> 
> >    A vendor-specific property must have a type definition in json-schema,
> >    so if I translate the original bindings semantics directly, I should
> >    define ti,deskew as an int32, but this makes dt_binding_check fail if
> >    the property has a negative value in the example because of the
> >    internal representation of cells as unsigned integers:
> > 
> >       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647
> 
> I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
> dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?
> 
> >    So I can think of two solutions to this:
> > 
> >    a) Keep the ti,deskew property as an uint32 and document the valid
> >    range ([-4, 3]) in the property description (this is what this patch
> >    does currently).
> > 
> >    b) Redefine this property to be closer to the datasheet description
> >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> >    This would also let us define its range properly using minimum and
> >    maximum properties for it.
> > 
> >    I think (b) is the right thing to do but I want to know your
> >    opinion. Besides, I don't have this hardware at hand and if I updated
> >    the driver I wouldn't be able to test it.
> 
> I don't think anyone has used deskew property, so I guess changing it is not out of the question.
> 
> Laurent, did you have a board that needs deskew when you added it to tfp410?

I didn't if I remember correctly, I just mapped it to the hardware
features. The hardware register indeed takes a value between 0 and 7,
and that is mapped to [-4,3] x t(STEP). I don't mind either option.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Laurent Pinchart]

Hi Tomi,

On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote:
> On 28/04/2020 12:20, Ricardo Cañuelo wrote:
> 
> > 2) The definition of ti,deskew in the original binding seems to be
> > tailored to the current driver and the way it's defined may not be very
> > DT-friendly.
> > 
> >    This parameter maps to a 3-bit field in a hardware register that takes
> >    a value from 0 to 7, so the [-4, 3] range described for this would map
> >    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> > 
> >    Then, the driver parses the parameter (unsigned) and casts it to a
> >    signed integer to get a number in the [-4, 3] range.
> 
> Interestingly the current example has ti,deskew = <4>...
> 
> >    A vendor-specific property must have a type definition in json-schema,
> >    so if I translate the original bindings semantics directly, I should
> >    define ti,deskew as an int32, but this makes dt_binding_check fail if
> >    the property has a negative value in the example because of the
> >    internal representation of cells as unsigned integers:
> > 
> >       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647
> 
> I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
> dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?
> 
> >    So I can think of two solutions to this:
> > 
> >    a) Keep the ti,deskew property as an uint32 and document the valid
> >    range ([-4, 3]) in the property description (this is what this patch
> >    does currently).
> > 
> >    b) Redefine this property to be closer to the datasheet description
> >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> >    This would also let us define its range properly using minimum and
> >    maximum properties for it.
> > 
> >    I think (b) is the right thing to do but I want to know your
> >    opinion. Besides, I don't have this hardware at hand and if I updated
> >    the driver I wouldn't be able to test it.
> 
> I don't think anyone has used deskew property, so I guess changing it is not out of the question.
> 
> Laurent, did you have a board that needs deskew when you added it to tfp410?

I didn't if I remember correctly, I just mapped it to the hardware
features. The hardware register indeed takes a value between 0 and 7,
and that is mapped to [-4,3] x t(STEP). I don't mind either option.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Laurent Pinchart]

Hi Tomi,

On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote:
> On 28/04/2020 12:20, Ricardo Cañuelo wrote:
> 
> > 2) The definition of ti,deskew in the original binding seems to be
> > tailored to the current driver and the way it's defined may not be very
> > DT-friendly.
> > 
> >    This parameter maps to a 3-bit field in a hardware register that takes
> >    a value from 0 to 7, so the [-4, 3] range described for this would map
> >    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> > 
> >    Then, the driver parses the parameter (unsigned) and casts it to a
> >    signed integer to get a number in the [-4, 3] range.
> 
> Interestingly the current example has ti,deskew = <4>...
> 
> >    A vendor-specific property must have a type definition in json-schema,
> >    so if I translate the original bindings semantics directly, I should
> >    define ti,deskew as an int32, but this makes dt_binding_check fail if
> >    the property has a negative value in the example because of the
> >    internal representation of cells as unsigned integers:
> > 
> >       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647
> 
> I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
> dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?
> 
> >    So I can think of two solutions to this:
> > 
> >    a) Keep the ti,deskew property as an uint32 and document the valid
> >    range ([-4, 3]) in the property description (this is what this patch
> >    does currently).
> > 
> >    b) Redefine this property to be closer to the datasheet description
> >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> >    This would also let us define its range properly using minimum and
> >    maximum properties for it.
> > 
> >    I think (b) is the right thing to do but I want to know your
> >    opinion. Besides, I don't have this hardware at hand and if I updated
> >    the driver I wouldn't be able to test it.
> 
> I don't think anyone has used deskew property, so I guess changing it is not out of the question.
> 
> Laurent, did you have a board that needs deskew when you added it to tfp410?

I didn't if I remember correctly, I just mapped it to the hardware
features. The hardware register indeed takes a value between 0 and 7,
and that is mapped to [-4,3] x t(STEP). I don't mind either option.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Rob,

What's your opinion on this?

Some context: It's about bindings that define signed integer properties
with range checks that go below and above zero. The schema checker fails
because, apparently, it interprets every cell value as an uint32, which
makes the range check fail for negative numbers.

On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote:
> > On 28/04/2020 12:20, Ricardo Cañuelo wrote:
> > 
> > > 2) The definition of ti,deskew in the original binding seems to be
> > > tailored to the current driver and the way it's defined may not be very
> > > DT-friendly.
> > > 
> > >    This parameter maps to a 3-bit field in a hardware register that takes
> > >    a value from 0 to 7, so the [-4, 3] range described for this would map
> > >    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> > > 
> > >    Then, the driver parses the parameter (unsigned) and casts it to a
> > >    signed integer to get a number in the [-4, 3] range.
> > 
> > Interestingly the current example has ti,deskew = <4>...
> > 
> > >    A vendor-specific property must have a type definition in json-schema,
> > >    so if I translate the original bindings semantics directly, I should
> > >    define ti,deskew as an int32, but this makes dt_binding_check fail if
> > >    the property has a negative value in the example because of the
> > >    internal representation of cells as unsigned integers:
> > > 
> > >       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647
> > 
> > I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
> > dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?
> > 
> > >    So I can think of two solutions to this:
> > > 
> > >    a) Keep the ti,deskew property as an uint32 and document the valid
> > >    range ([-4, 3]) in the property description (this is what this patch
> > >    does currently).
> > > 
> > >    b) Redefine this property to be closer to the datasheet description
> > >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> > >    This would also let us define its range properly using minimum and
> > >    maximum properties for it.
> > > 
> > >    I think (b) is the right thing to do but I want to know your
> > >    opinion. Besides, I don't have this hardware at hand and if I updated
> > >    the driver I wouldn't be able to test it.
> > 
> > I don't think anyone has used deskew property, so I guess changing it is not out of the question.
> > 
> > Laurent, did you have a board that needs deskew when you added it to tfp410?
> 
> I didn't if I remember correctly, I just mapped it to the hardware
> features. The hardware register indeed takes a value between 0 and 7,
> and that is mapped to [-4,3] x t(STEP). I don't mind either option.
> 
> -- 
> Regards,
> 
> Laurent Pinchart

I haven't found any examples of yaml bindings defining signed integer
properties such as this, what's the norm in this case? Do you agree with
any of the proposed solutions? Do you have a better suggestion?

Thanks,
Ricardo

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Rob,

What's your opinion on this?

Some context: It's about bindings that define signed integer properties
with range checks that go below and above zero. The schema checker fails
because, apparently, it interprets every cell value as an uint32, which
makes the range check fail for negative numbers.

On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote:
> > On 28/04/2020 12:20, Ricardo Cañuelo wrote:
> > 
> > > 2) The definition of ti,deskew in the original binding seems to be
> > > tailored to the current driver and the way it's defined may not be very
> > > DT-friendly.
> > > 
> > >    This parameter maps to a 3-bit field in a hardware register that takes
> > >    a value from 0 to 7, so the [-4, 3] range described for this would map
> > >    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> > > 
> > >    Then, the driver parses the parameter (unsigned) and casts it to a
> > >    signed integer to get a number in the [-4, 3] range.
> > 
> > Interestingly the current example has ti,deskew = <4>...
> > 
> > >    A vendor-specific property must have a type definition in json-schema,
> > >    so if I translate the original bindings semantics directly, I should
> > >    define ti,deskew as an int32, but this makes dt_binding_check fail if
> > >    the property has a negative value in the example because of the
> > >    internal representation of cells as unsigned integers:
> > > 
> > >       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647
> > 
> > I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
> > dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?
> > 
> > >    So I can think of two solutions to this:
> > > 
> > >    a) Keep the ti,deskew property as an uint32 and document the valid
> > >    range ([-4, 3]) in the property description (this is what this patch
> > >    does currently).
> > > 
> > >    b) Redefine this property to be closer to the datasheet description
> > >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> > >    This would also let us define its range properly using minimum and
> > >    maximum properties for it.
> > > 
> > >    I think (b) is the right thing to do but I want to know your
> > >    opinion. Besides, I don't have this hardware at hand and if I updated
> > >    the driver I wouldn't be able to test it.
> > 
> > I don't think anyone has used deskew property, so I guess changing it is not out of the question.
> > 
> > Laurent, did you have a board that needs deskew when you added it to tfp410?
> 
> I didn't if I remember correctly, I just mapped it to the hardware
> features. The hardware register indeed takes a value between 0 and 7,
> and that is mapped to [-4,3] x t(STEP). I don't mind either option.
> 
> -- 
> Regards,
> 
> Laurent Pinchart

I haven't found any examples of yaml bindings defining signed integer
properties such as this, what's the norm in this case? Do you agree with
any of the proposed solutions? Do you have a better suggestion?

Thanks,
Ricardo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Rob,

What's your opinion on this?

Some context: It's about bindings that define signed integer properties
with range checks that go below and above zero. The schema checker fails
because, apparently, it interprets every cell value as an uint32, which
makes the range check fail for negative numbers.

On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote:
> > On 28/04/2020 12:20, Ricardo Cañuelo wrote:
> > 
> > > 2) The definition of ti,deskew in the original binding seems to be
> > > tailored to the current driver and the way it's defined may not be very
> > > DT-friendly.
> > > 
> > >    This parameter maps to a 3-bit field in a hardware register that takes
> > >    a value from 0 to 7, so the [-4, 3] range described for this would map
> > >    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> > > 
> > >    Then, the driver parses the parameter (unsigned) and casts it to a
> > >    signed integer to get a number in the [-4, 3] range.
> > 
> > Interestingly the current example has ti,deskew = <4>...
> > 
> > >    A vendor-specific property must have a type definition in json-schema,
> > >    so if I translate the original bindings semantics directly, I should
> > >    define ti,deskew as an int32, but this makes dt_binding_check fail if
> > >    the property has a negative value in the example because of the
> > >    internal representation of cells as unsigned integers:
> > > 
> > >       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647
> > 
> > I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
> > dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?
> > 
> > >    So I can think of two solutions to this:
> > > 
> > >    a) Keep the ti,deskew property as an uint32 and document the valid
> > >    range ([-4, 3]) in the property description (this is what this patch
> > >    does currently).
> > > 
> > >    b) Redefine this property to be closer to the datasheet description
> > >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> > >    This would also let us define its range properly using minimum and
> > >    maximum properties for it.
> > > 
> > >    I think (b) is the right thing to do but I want to know your
> > >    opinion. Besides, I don't have this hardware at hand and if I updated
> > >    the driver I wouldn't be able to test it.
> > 
> > I don't think anyone has used deskew property, so I guess changing it is not out of the question.
> > 
> > Laurent, did you have a board that needs deskew when you added it to tfp410?
> 
> I didn't if I remember correctly, I just mapped it to the hardware
> features. The hardware register indeed takes a value between 0 and 7,
> and that is mapped to [-4,3] x t(STEP). I don't mind either option.
> 
> -- 
> Regards,
> 
> Laurent Pinchart

I haven't found any examples of yaml bindings defining signed integer
properties such as this, what's the norm in this case? Do you agree with
any of the proposed solutions? Do you have a better suggestion?

Thanks,
Ricardo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Sam Ravnborg]

Hi Ricardo.

On Mon, May 11, 2020 at 04:59:11PM +0200, Ricardo Cañuelo wrote:
> Hi Rob,
> 
> What's your opinion on this?

I'm not Rob, but anyway.
> 
> Some context: It's about bindings that define signed integer properties
> with range checks that go below and above zero. The schema checker fails
> because, apparently, it interprets every cell value as an uint32, which
> makes the range check fail for negative numbers.
> 
> > > >    b) Redefine this property to be closer to the datasheet description
> > > >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> > > >    This would also let us define its range properly using minimum and
> > > >    maximum properties for it.
> > > > 
> > > >    I think (b) is the right thing to do but I want to know your
> > > >    opinion. Besides, I don't have this hardware at hand and if I updated
> > > >    the driver I wouldn't be able to test it.

Based on the discussions option b) above seems like the best compromise.

	Sam

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Sam Ravnborg]

Hi Ricardo.

On Mon, May 11, 2020 at 04:59:11PM +0200, Ricardo Cañuelo wrote:
> Hi Rob,
> 
> What's your opinion on this?

I'm not Rob, but anyway.
> 
> Some context: It's about bindings that define signed integer properties
> with range checks that go below and above zero. The schema checker fails
> because, apparently, it interprets every cell value as an uint32, which
> makes the range check fail for negative numbers.
> 
> > > >    b) Redefine this property to be closer to the datasheet description
> > > >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> > > >    This would also let us define its range properly using minimum and
> > > >    maximum properties for it.
> > > > 
> > > >    I think (b) is the right thing to do but I want to know your
> > > >    opinion. Besides, I don't have this hardware at hand and if I updated
> > > >    the driver I wouldn't be able to test it.

Based on the discussions option b) above seems like the best compromise.

	Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Sam Ravnborg]

Hi Ricardo.

On Mon, May 11, 2020 at 04:59:11PM +0200, Ricardo Cañuelo wrote:
> Hi Rob,
> 
> What's your opinion on this?

I'm not Rob, but anyway.
> 
> Some context: It's about bindings that define signed integer properties
> with range checks that go below and above zero. The schema checker fails
> because, apparently, it interprets every cell value as an uint32, which
> makes the range check fail for negative numbers.
> 
> > > >    b) Redefine this property to be closer to the datasheet description
> > > >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> > > >    This would also let us define its range properly using minimum and
> > > >    maximum properties for it.
> > > > 
> > > >    I think (b) is the right thing to do but I want to know your
> > > >    opinion. Besides, I don't have this hardware at hand and if I updated
> > > >    the driver I wouldn't be able to test it.

Based on the discussions option b) above seems like the best compromise.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Rob Herring]

On Mon, May 11, 2020 at 04:59:11PM +0200, Ricardo Cañuelo wrote:
> Hi Rob,
> 
> What's your opinion on this?
> 
> Some context: It's about bindings that define signed integer properties
> with range checks that go below and above zero. The schema checker fails
> because, apparently, it interprets every cell value as an uint32, which
> makes the range check fail for negative numbers.

The real fix is dtc needs to carry the sign thru to the yaml output 
format. In the mean time, perhaps the schema should have an unsigned 
range for signed types.

Rob

> 
> On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> > Hi Tomi,
> > 
> > On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote:
> > > On 28/04/2020 12:20, Ricardo Cañuelo wrote:
> > > 
> > > > 2) The definition of ti,deskew in the original binding seems to be
> > > > tailored to the current driver and the way it's defined may not be very
> > > > DT-friendly.
> > > > 
> > > >    This parameter maps to a 3-bit field in a hardware register that takes
> > > >    a value from 0 to 7, so the [-4, 3] range described for this would map
> > > >    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> > > > 
> > > >    Then, the driver parses the parameter (unsigned) and casts it to a
> > > >    signed integer to get a number in the [-4, 3] range.
> > > 
> > > Interestingly the current example has ti,deskew = <4>...
> > > 
> > > >    A vendor-specific property must have a type definition in json-schema,
> > > >    so if I translate the original bindings semantics directly, I should
> > > >    define ti,deskew as an int32, but this makes dt_binding_check fail if
> > > >    the property has a negative value in the example because of the
> > > >    internal representation of cells as unsigned integers:
> > > > 
> > > >       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647
> > > 
> > > I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
> > > dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?
> > > 
> > > >    So I can think of two solutions to this:
> > > > 
> > > >    a) Keep the ti,deskew property as an uint32 and document the valid
> > > >    range ([-4, 3]) in the property description (this is what this patch
> > > >    does currently).
> > > > 
> > > >    b) Redefine this property to be closer to the datasheet description
> > > >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> > > >    This would also let us define its range properly using minimum and
> > > >    maximum properties for it.
> > > > 
> > > >    I think (b) is the right thing to do but I want to know your
> > > >    opinion. Besides, I don't have this hardware at hand and if I updated
> > > >    the driver I wouldn't be able to test it.
> > > 
> > > I don't think anyone has used deskew property, so I guess changing it is not out of the question.
> > > 
> > > Laurent, did you have a board that needs deskew when you added it to tfp410?
> > 
> > I didn't if I remember correctly, I just mapped it to the hardware
> > features. The hardware register indeed takes a value between 0 and 7,
> > and that is mapped to [-4,3] x t(STEP). I don't mind either option.
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> 
> I haven't found any examples of yaml bindings defining signed integer
> properties such as this, what's the norm in this case? Do you agree with
> any of the proposed solutions? Do you have a better suggestion?
> 
> Thanks,
> Ricardo

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Rob Herring]

On Mon, May 11, 2020 at 04:59:11PM +0200, Ricardo Cañuelo wrote:
> Hi Rob,
> 
> What's your opinion on this?
> 
> Some context: It's about bindings that define signed integer properties
> with range checks that go below and above zero. The schema checker fails
> because, apparently, it interprets every cell value as an uint32, which
> makes the range check fail for negative numbers.

The real fix is dtc needs to carry the sign thru to the yaml output 
format. In the mean time, perhaps the schema should have an unsigned 
range for signed types.

Rob

> 
> On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> > Hi Tomi,
> > 
> > On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote:
> > > On 28/04/2020 12:20, Ricardo Cañuelo wrote:
> > > 
> > > > 2) The definition of ti,deskew in the original binding seems to be
> > > > tailored to the current driver and the way it's defined may not be very
> > > > DT-friendly.
> > > > 
> > > >    This parameter maps to a 3-bit field in a hardware register that takes
> > > >    a value from 0 to 7, so the [-4, 3] range described for this would map
> > > >    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> > > > 
> > > >    Then, the driver parses the parameter (unsigned) and casts it to a
> > > >    signed integer to get a number in the [-4, 3] range.
> > > 
> > > Interestingly the current example has ti,deskew = <4>...
> > > 
> > > >    A vendor-specific property must have a type definition in json-schema,
> > > >    so if I translate the original bindings semantics directly, I should
> > > >    define ti,deskew as an int32, but this makes dt_binding_check fail if
> > > >    the property has a negative value in the example because of the
> > > >    internal representation of cells as unsigned integers:
> > > > 
> > > >       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647
> > > 
> > > I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
> > > dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?
> > > 
> > > >    So I can think of two solutions to this:
> > > > 
> > > >    a) Keep the ti,deskew property as an uint32 and document the valid
> > > >    range ([-4, 3]) in the property description (this is what this patch
> > > >    does currently).
> > > > 
> > > >    b) Redefine this property to be closer to the datasheet description
> > > >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> > > >    This would also let us define its range properly using minimum and
> > > >    maximum properties for it.
> > > > 
> > > >    I think (b) is the right thing to do but I want to know your
> > > >    opinion. Besides, I don't have this hardware at hand and if I updated
> > > >    the driver I wouldn't be able to test it.
> > > 
> > > I don't think anyone has used deskew property, so I guess changing it is not out of the question.
> > > 
> > > Laurent, did you have a board that needs deskew when you added it to tfp410?
> > 
> > I didn't if I remember correctly, I just mapped it to the hardware
> > features. The hardware register indeed takes a value between 0 and 7,
> > and that is mapped to [-4,3] x t(STEP). I don't mind either option.
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> 
> I haven't found any examples of yaml bindings defining signed integer
> properties such as this, what's the norm in this case? Do you agree with
> any of the proposed solutions? Do you have a better suggestion?
> 
> Thanks,
> Ricardo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Rob Herring]

On Mon, May 11, 2020 at 04:59:11PM +0200, Ricardo Cañuelo wrote:
> Hi Rob,
> 
> What's your opinion on this?
> 
> Some context: It's about bindings that define signed integer properties
> with range checks that go below and above zero. The schema checker fails
> because, apparently, it interprets every cell value as an uint32, which
> makes the range check fail for negative numbers.

The real fix is dtc needs to carry the sign thru to the yaml output 
format. In the mean time, perhaps the schema should have an unsigned 
range for signed types.

Rob

> 
> On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> > Hi Tomi,
> > 
> > On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote:
> > > On 28/04/2020 12:20, Ricardo Cañuelo wrote:
> > > 
> > > > 2) The definition of ti,deskew in the original binding seems to be
> > > > tailored to the current driver and the way it's defined may not be very
> > > > DT-friendly.
> > > > 
> > > >    This parameter maps to a 3-bit field in a hardware register that takes
> > > >    a value from 0 to 7, so the [-4, 3] range described for this would map
> > > >    to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111.
> > > > 
> > > >    Then, the driver parses the parameter (unsigned) and casts it to a
> > > >    signed integer to get a number in the [-4, 3] range.
> > > 
> > > Interestingly the current example has ti,deskew = <4>...
> > > 
> > > >    A vendor-specific property must have a type definition in json-schema,
> > > >    so if I translate the original bindings semantics directly, I should
> > > >    define ti,deskew as an int32, but this makes dt_binding_check fail if
> > > >    the property has a negative value in the example because of the
> > > >    internal representation of cells as unsigned integers:
> > > > 
> > > >       ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647
> > > 
> > > I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but 
> > > dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings?
> > > 
> > > >    So I can think of two solutions to this:
> > > > 
> > > >    a) Keep the ti,deskew property as an uint32 and document the valid
> > > >    range ([-4, 3]) in the property description (this is what this patch
> > > >    does currently).
> > > > 
> > > >    b) Redefine this property to be closer to the datasheet description
> > > >    (ie. unsigned integers from 0 to 7) and adapt the driver accordingly.
> > > >    This would also let us define its range properly using minimum and
> > > >    maximum properties for it.
> > > > 
> > > >    I think (b) is the right thing to do but I want to know your
> > > >    opinion. Besides, I don't have this hardware at hand and if I updated
> > > >    the driver I wouldn't be able to test it.
> > > 
> > > I don't think anyone has used deskew property, so I guess changing it is not out of the question.
> > > 
> > > Laurent, did you have a board that needs deskew when you added it to tfp410?
> > 
> > I didn't if I remember correctly, I just mapped it to the hardware
> > features. The hardware register indeed takes a value between 0 and 7,
> > and that is mapped to [-4,3] x t(STEP). I don't mind either option.
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> 
> I haven't found any examples of yaml bindings defining signed integer
> properties such as this, what's the norm in this case? Do you agree with
> any of the proposed solutions? Do you have a better suggestion?
> 
> Thanks,
> Ricardo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Laurent,

On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> I didn't if I remember correctly, I just mapped it to the hardware
> features. The hardware register indeed takes a value between 0 and 7,
> and that is mapped to [-4,3] x t(STEP). I don't mind either option.

I was taking a look at the ti-tfp410.c driver to see if it needs any
changes to support the updated deskew property ranges [0-7], but I don't
fully understand what this does (line 276):

	/* Get the setup and hold time from vendor-specific properties. */
	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
	if (deskew < -4 || deskew > 3)
		return -EINVAL;

	timings->setup_time_ps = min(0, 1200 - 350 * deskew);
	timings->hold_time_ps = min(0, 1300 + 350 * deskew);

It looks like that the driver doesn't really apply the deskew settings
to the device and that this has not been really tested, so it's probably
not a big deal.

I guess what you wanted to do was to adjust the setup and hold times
around 1200 and 1300 ps respectively in increments/decrements of 350 ps
depending on the deskew value, as the datasheet describes. But this code
would set timings->setup_time_ps to 0 regardless of the deskew value,
and timings->hold_time_ps would be either 0 or a very big integer value
if deskew is -4 (both setup_time_ps and hold_time_ps are u32).

Am I missing something? Was this intentional?

Thanks,
Ricardo

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Laurent,

On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> I didn't if I remember correctly, I just mapped it to the hardware
> features. The hardware register indeed takes a value between 0 and 7,
> and that is mapped to [-4,3] x t(STEP). I don't mind either option.

I was taking a look at the ti-tfp410.c driver to see if it needs any
changes to support the updated deskew property ranges [0-7], but I don't
fully understand what this does (line 276):

	/* Get the setup and hold time from vendor-specific properties. */
	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
	if (deskew < -4 || deskew > 3)
		return -EINVAL;

	timings->setup_time_ps = min(0, 1200 - 350 * deskew);
	timings->hold_time_ps = min(0, 1300 + 350 * deskew);

It looks like that the driver doesn't really apply the deskew settings
to the device and that this has not been really tested, so it's probably
not a big deal.

I guess what you wanted to do was to adjust the setup and hold times
around 1200 and 1300 ps respectively in increments/decrements of 350 ps
depending on the deskew value, as the datasheet describes. But this code
would set timings->setup_time_ps to 0 regardless of the deskew value,
and timings->hold_time_ps would be either 0 or a very big integer value
if deskew is -4 (both setup_time_ps and hold_time_ps are u32).

Am I missing something? Was this intentional?

Thanks,
Ricardo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Laurent,

On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> I didn't if I remember correctly, I just mapped it to the hardware
> features. The hardware register indeed takes a value between 0 and 7,
> and that is mapped to [-4,3] x t(STEP). I don't mind either option.

I was taking a look at the ti-tfp410.c driver to see if it needs any
changes to support the updated deskew property ranges [0-7], but I don't
fully understand what this does (line 276):

	/* Get the setup and hold time from vendor-specific properties. */
	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
	if (deskew < -4 || deskew > 3)
		return -EINVAL;

	timings->setup_time_ps = min(0, 1200 - 350 * deskew);
	timings->hold_time_ps = min(0, 1300 + 350 * deskew);

It looks like that the driver doesn't really apply the deskew settings
to the device and that this has not been really tested, so it's probably
not a big deal.

I guess what you wanted to do was to adjust the setup and hold times
around 1200 and 1300 ps respectively in increments/decrements of 350 ps
depending on the deskew value, as the datasheet describes. But this code
would set timings->setup_time_ps to 0 regardless of the deskew value,
and timings->hold_time_ps would be either 0 or a very big integer value
if deskew is -4 (both setup_time_ps and hold_time_ps are u32).

Am I missing something? Was this intentional?

Thanks,
Ricardo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Laurent Pinchart]

Hi Ricardo,

On Wed, May 13, 2020 at 01:09:57PM +0200, Ricardo Cañuelo wrote:
> On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> > I didn't if I remember correctly, I just mapped it to the hardware
> > features. The hardware register indeed takes a value between 0 and 7,
> > and that is mapped to [-4,3] x t(STEP). I don't mind either option.
> 
> I was taking a look at the ti-tfp410.c driver to see if it needs any
> changes to support the updated deskew property ranges [0-7], but I don't
> fully understand what this does (line 276):
> 
> 	/* Get the setup and hold time from vendor-specific properties. */
> 	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
> 	if (deskew < -4 || deskew > 3)
> 		return -EINVAL;
> 
> 	timings->setup_time_ps = min(0, 1200 - 350 * deskew);
> 	timings->hold_time_ps = min(0, 1300 + 350 * deskew);
> 
> It looks like that the driver doesn't really apply the deskew settings
> to the device and that this has not been really tested, so it's probably
> not a big deal.

The driver doesn't apply any setting to the device :-) The ti,deskew
property is meant to report the deskew settings selected by the chip's
configuration pins, not to set a value to be programmed to the device.

> I guess what you wanted to do was to adjust the setup and hold times
> around 1200 and 1300 ps respectively in increments/decrements of 350 ps
> depending on the deskew value, as the datasheet describes. But this code
> would set timings->setup_time_ps to 0 regardless of the deskew value,
> and timings->hold_time_ps would be either 0 or a very big integer value
> if deskew is -4 (both setup_time_ps and hold_time_ps are u32).
> 
> Am I missing something? Was this intentional?

Oops. That's embarassing... It should clearly be a max(), not a min().
And only for hold_time_ps is this required.

Would you like to send a patch, or should I do so ?

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Laurent Pinchart]

Hi Ricardo,

On Wed, May 13, 2020 at 01:09:57PM +0200, Ricardo Cañuelo wrote:
> On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> > I didn't if I remember correctly, I just mapped it to the hardware
> > features. The hardware register indeed takes a value between 0 and 7,
> > and that is mapped to [-4,3] x t(STEP). I don't mind either option.
> 
> I was taking a look at the ti-tfp410.c driver to see if it needs any
> changes to support the updated deskew property ranges [0-7], but I don't
> fully understand what this does (line 276):
> 
> 	/* Get the setup and hold time from vendor-specific properties. */
> 	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
> 	if (deskew < -4 || deskew > 3)
> 		return -EINVAL;
> 
> 	timings->setup_time_ps = min(0, 1200 - 350 * deskew);
> 	timings->hold_time_ps = min(0, 1300 + 350 * deskew);
> 
> It looks like that the driver doesn't really apply the deskew settings
> to the device and that this has not been really tested, so it's probably
> not a big deal.

The driver doesn't apply any setting to the device :-) The ti,deskew
property is meant to report the deskew settings selected by the chip's
configuration pins, not to set a value to be programmed to the device.

> I guess what you wanted to do was to adjust the setup and hold times
> around 1200 and 1300 ps respectively in increments/decrements of 350 ps
> depending on the deskew value, as the datasheet describes. But this code
> would set timings->setup_time_ps to 0 regardless of the deskew value,
> and timings->hold_time_ps would be either 0 or a very big integer value
> if deskew is -4 (both setup_time_ps and hold_time_ps are u32).
> 
> Am I missing something? Was this intentional?

Oops. That's embarassing... It should clearly be a max(), not a min().
And only for hold_time_ps is this required.

Would you like to send a patch, or should I do so ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Laurent Pinchart]

Hi Ricardo,

On Wed, May 13, 2020 at 01:09:57PM +0200, Ricardo Cañuelo wrote:
> On mié 06-05-2020 18:53:20, Laurent Pinchart wrote:
> > I didn't if I remember correctly, I just mapped it to the hardware
> > features. The hardware register indeed takes a value between 0 and 7,
> > and that is mapped to [-4,3] x t(STEP). I don't mind either option.
> 
> I was taking a look at the ti-tfp410.c driver to see if it needs any
> changes to support the updated deskew property ranges [0-7], but I don't
> fully understand what this does (line 276):
> 
> 	/* Get the setup and hold time from vendor-specific properties. */
> 	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
> 	if (deskew < -4 || deskew > 3)
> 		return -EINVAL;
> 
> 	timings->setup_time_ps = min(0, 1200 - 350 * deskew);
> 	timings->hold_time_ps = min(0, 1300 + 350 * deskew);
> 
> It looks like that the driver doesn't really apply the deskew settings
> to the device and that this has not been really tested, so it's probably
> not a big deal.

The driver doesn't apply any setting to the device :-) The ti,deskew
property is meant to report the deskew settings selected by the chip's
configuration pins, not to set a value to be programmed to the device.

> I guess what you wanted to do was to adjust the setup and hold times
> around 1200 and 1300 ps respectively in increments/decrements of 350 ps
> depending on the deskew value, as the datasheet describes. But this code
> would set timings->setup_time_ps to 0 regardless of the deskew value,
> and timings->hold_time_ps would be either 0 or a very big integer value
> if deskew is -4 (both setup_time_ps and hold_time_ps are u32).
> 
> Am I missing something? Was this intentional?

Oops. That's embarassing... It should clearly be a max(), not a min().
And only for hold_time_ps is this required.

Would you like to send a patch, or should I do so ?

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Laurent,

On mié 13-05-2020 17:08:32, Laurent Pinchart wrote:
> The driver doesn't apply any setting to the device :-) The ti,deskew
> property is meant to report the deskew settings selected by the chip's
> configuration pins, not to set a value to be programmed to the device.

Oh, I see, thanks for clarifying it.

> Would you like to send a patch, or should I do so ?

I can take care of it, I'll try to have a new series by tomorrow.

Cheers,
Ricardo

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Laurent,

On mié 13-05-2020 17:08:32, Laurent Pinchart wrote:
> The driver doesn't apply any setting to the device :-) The ti,deskew
> property is meant to report the deskew settings selected by the chip's
> configuration pins, not to set a value to be programmed to the device.

Oh, I see, thanks for clarifying it.

> Would you like to send a patch, or should I do so ?

I can take care of it, I'll try to have a new series by tomorrow.

Cheers,
Ricardo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] dt-bindings: display: ti,tfp410.txt: convert to yaml

[Ricardo Cañuelo]

Hi Laurent,

On mié 13-05-2020 17:08:32, Laurent Pinchart wrote:
> The driver doesn't apply any setting to the device :-) The ti,deskew
> property is meant to report the deskew settings selected by the chip's
> configuration pins, not to set a value to be programmed to the device.

Oh, I see, thanks for clarifying it.

> Would you like to send a patch, or should I do so ?

I can take care of it, I'll try to have a new series by tomorrow.

Cheers,
Ricardo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel