[PATCH v4 00/13] Add LCD panel support to iwg20d

[PATCH v4 00/13] Add LCD panel support to iwg20d

[Fabrizio Castro]

The iW-RainboW-G20D-Qseven RZ/G1M,G1N Qseven Development Platform
comes with a 7" capacitive display kit from Emerging Display
Technologies Corporation (EDT). This series adds all that's
necessary for supporting it.

Thanks,
Fab

v3->v4:
* Reworked according to Laurent's and Jacopo's comments
v2->v3:
* Split the dt-schema patch in two patches as per Rob's comment
* Made fixes to the dt-schema according to Rob's comment
* Made fixes to the lvds-codec driver according to Jacopo's comments
* Added two new patches:
  * drm: Define DRM_MODE_CONNECTOR_PARALLEL
  * drm/panel: panel-simple: Add connector type for etm0700g0dh6
v1->v2:
* Convert dt-bindings to dt-schema

Fabrizio Castro (13):
  dt-bindings: display: bridge: Convert lvds-transmitter binding to
    json-schema
  dt-bindings: display: bridge: lvds-transmitter: Document
    powerdown-gpios
  dt-bindings: display: bridge: lvds-transmitter: Absorb ti,ds90c185.txt
  dt-bindings: display: bridge: lvds-transmitter: Document
    "ti,sn75lvds83"
  drm/bridge: Repurpose lvds-encoder.c
  drm/bridge: lvds-codec: Add "lvds-decoder" support
  drm/bridge: lvds-codec: Simplify panel DT node localisation
  dt-bindings: display: bridge: Repurpose lvds-encoder
  dt-bindings: display: bridge: lvds-codec: Document ti,ds90cf384a
  ARM: dts: iwg20d-q7-common: Add LCD support
  ARM: shmobile_defconfig: Enable support for panels from EDT
  [HACK] drm/bridge: lvds-codec: Enforce device specific compatible
    strings
  [HACK] dt-bindings: display: bridge: lvds-codec: Absorb
    thine,thc63lvdm83d.txt

 .../bindings/display/bridge/lvds-codec.yaml        | 131 +++++++++++++++
 .../bindings/display/bridge/lvds-transmitter.txt   |  66 --------
 .../bindings/display/bridge/thine,thc63lvdm83d.txt |  50 ------
 .../bindings/display/bridge/ti,ds90c185.txt        |  55 -------
 arch/arm/boot/dts/iwg20d-q7-common.dtsi            |  85 ++++++++++
 arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi           |   1 -
 arch/arm/configs/shmobile_defconfig                |   3 +
 drivers/gpu/drm/bridge/Kconfig                     |   8 +-
 drivers/gpu/drm/bridge/Makefile                    |   2 +-
 .../drm/bridge/{lvds-encoder.c => lvds-codec.c}    | 179 +++++++++++++--------
 10 files changed, 333 insertions(+), 247 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
 rename drivers/gpu/drm/bridge/{lvds-encoder.c => lvds-codec.c} (25%)

-- 
2.7.4

[PATCH v4 01/13] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema

[Fabrizio Castro]

Convert the lvds-transmitter binding to DT schema format using
json-schema.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* Fixed the description of property "compatible" according to Laurent's
  comments
v2->v3:
* Extracted conversion to dt-schema as per Rob's comment
v1->v2:
* Converted to dt-schema as per Neil's comment
---
 .../bindings/display/bridge/lvds-transmitter.txt   | 66 ----------------
 .../bindings/display/bridge/lvds-transmitter.yaml  | 91 ++++++++++++++++++++++
 2 files changed, 91 insertions(+), 66 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
 create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
deleted file mode 100644
index 60091db..0000000
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ /dev/null
@@ -1,66 +0,0 @@
-Parallel to LVDS Encoder
-------------------------
-
-This binding supports the parallel to LVDS encoders that don't require any
-configuration.
-
-LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
-incompatible data link layers have been used over time to transmit image data
-to LVDS panels. This binding targets devices compatible with the following
-specifications only.
-
-[JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
-1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
-[LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
-Semiconductor
-[VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
-Electronics Standards Association (VESA)
-
-Those devices have been marketed under the FPD-Link and FlatLink brand names
-among others.
-
-
-Required properties:
-
-- compatible: Must be "lvds-encoder"
-
-  Any encoder compatible with this generic binding, but with additional
-  properties not listed here, must list a device specific compatible first
-  followed by this generic compatible.
-
-Required nodes:
-
-This device has two video ports. Their connections are modeled using the OF
-graph bindings specified in Documentation/devicetree/bindings/graph.txt.
-
-- Video port 0 for parallel input
-- Video port 1 for LVDS output
-
-
-Example
--------
-
-lvds-encoder {
-	compatible = "lvds-encoder";
-
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@0 {
-			reg = <0>;
-
-			lvds_enc_in: endpoint {
-				remote-endpoint = <&display_out_rgb>;
-			};
-		};
-
-		port@1 {
-			reg = <1>;
-
-			lvds_enc_out: endpoint {
-				remote-endpoint = <&lvds_panel_in>;
-			};
-		};
-	};
-};
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
new file mode 100644
index 0000000..b5dd0da
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
@@ -0,0 +1,91 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/lvds-transmitter.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Parallel to LVDS Encoder
+
+maintainers:
+  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
+
+description: |
+  This binding supports the parallel to LVDS encoders that don't require any
+  configuration.
+
+  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
+  incompatible data link layers have been used over time to transmit image data
+  to LVDS panels. This binding targets devices compatible with the following
+  specifications only.
+
+  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
+  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
+  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
+  Semiconductor
+  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
+  Electronics Standards Association (VESA)
+
+  Those devices have been marketed under the FPD-Link and FlatLink brand names
+  among others.
+
+properties:
+  compatible:
+    description: |
+      Any encoder compatible with this generic binding, but with additional
+      properties not listed here, must define its own binding and list a device
+      specific compatible first followed by the generic compatible.
+    enum:
+      - lvds-encoder
+
+  ports:
+    type: object
+    description: |
+      This device has two video ports. Their connections are modeled using the
+      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
+    properties:
+      port@0:
+        type: object
+        description: |
+          Port 0 is for parallel input
+
+      port@1:
+        type: object
+        description: |
+          Port 1 is for LVDS output
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - ports
+
+examples:
+  - |
+    lvds-encoder {
+      compatible = "lvds-encoder";
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+
+          lvds_enc_in: endpoint {
+            remote-endpoint = <&display_out_rgb>;
+          };
+        };
+
+        port@1 {
+          reg = <1>;
+
+          lvds_enc_out: endpoint {
+            remote-endpoint = <&lvds_panel_in>;
+          };
+        };
+      };
+    };
+
+...
-- 
2.7.4

Re: [PATCH v4 01/13] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema

[Rob Herring]

On Wed, 13 Nov 2019 15:51:20 +0000, Fabrizio Castro wrote:
> Convert the lvds-transmitter binding to DT schema format using
> json-schema.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * Fixed the description of property "compatible" according to Laurent's
>   comments
> v2->v3:
> * Extracted conversion to dt-schema as per Rob's comment
> v1->v2:
> * Converted to dt-schema as per Neil's comment
> ---
>  .../bindings/display/bridge/lvds-transmitter.txt   | 66 ----------------
>  .../bindings/display/bridge/lvds-transmitter.yaml  | 91 ++++++++++++++++++++++
>  2 files changed, 91 insertions(+), 66 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 01/13] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:20PM +0000, Fabrizio Castro wrote:
> Convert the lvds-transmitter binding to DT schema format using
> json-schema.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3->v4:
> * Fixed the description of property "compatible" according to Laurent's
>   comments
> v2->v3:
> * Extracted conversion to dt-schema as per Rob's comment
> v1->v2:
> * Converted to dt-schema as per Neil's comment
> ---
>  .../bindings/display/bridge/lvds-transmitter.txt   | 66 ----------------
>  .../bindings/display/bridge/lvds-transmitter.yaml  | 91 ++++++++++++++++++++++
>  2 files changed, 91 insertions(+), 66 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> deleted file mode 100644
> index 60091db..0000000
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> +++ /dev/null
> @@ -1,66 +0,0 @@
> -Parallel to LVDS Encoder
> -------------------------
> -
> -This binding supports the parallel to LVDS encoders that don't require any
> -configuration.
> -
> -LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> -incompatible data link layers have been used over time to transmit image data
> -to LVDS panels. This binding targets devices compatible with the following
> -specifications only.
> -
> -[JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
> -1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
> -[LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
> -Semiconductor
> -[VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> -Electronics Standards Association (VESA)
> -
> -Those devices have been marketed under the FPD-Link and FlatLink brand names
> -among others.
> -
> -
> -Required properties:
> -
> -- compatible: Must be "lvds-encoder"
> -
> -  Any encoder compatible with this generic binding, but with additional
> -  properties not listed here, must list a device specific compatible first
> -  followed by this generic compatible.
> -
> -Required nodes:
> -
> -This device has two video ports. Their connections are modeled using the OF
> -graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> -
> -- Video port 0 for parallel input
> -- Video port 1 for LVDS output
> -
> -
> -Example
> --------
> -
> -lvds-encoder {
> -	compatible = "lvds-encoder";
> -
> -	ports {
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
> -		port@0 {
> -			reg = <0>;
> -
> -			lvds_enc_in: endpoint {
> -				remote-endpoint = <&display_out_rgb>;
> -			};
> -		};
> -
> -		port@1 {
> -			reg = <1>;
> -
> -			lvds_enc_out: endpoint {
> -				remote-endpoint = <&lvds_panel_in>;
> -			};
> -		};
> -	};
> -};
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> new file mode 100644
> index 0000000..b5dd0da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> @@ -0,0 +1,91 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/lvds-transmitter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Parallel to LVDS Encoder
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> +
> +description: |
> +  This binding supports the parallel to LVDS encoders that don't require any
> +  configuration.
> +
> +  LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> +  incompatible data link layers have been used over time to transmit image data
> +  to LVDS panels. This binding targets devices compatible with the following
> +  specifications only.
> +
> +  [JEIDA] "Digital Interface Standards for Monitor", JEIDA-59-1999, February
> +  1999 (Version 1.0), Japan Electronic Industry Development Association (JEIDA)
> +  [LDI] "Open LVDS Display Interface", May 1999 (Version 0.95), National
> +  Semiconductor
> +  [VESA] "VESA Notebook Panel Standard", October 2007 (Version 1.0), Video
> +  Electronics Standards Association (VESA)
> +
> +  Those devices have been marketed under the FPD-Link and FlatLink brand names
> +  among others.
> +
> +properties:
> +  compatible:
> +    description: |
> +      Any encoder compatible with this generic binding, but with additional
> +      properties not listed here, must define its own binding and list a device
> +      specific compatible first followed by the generic compatible.
> +    enum:
> +      - lvds-encoder
> +
> +  ports:
> +    type: object
> +    description: |
> +      This device has two video ports. Their connections are modeled using the
> +      OF graph bindings specified in Documentation/devicetree/bindings/graph.txt
> +    properties:
> +      port@0:
> +        type: object
> +        description: |
> +          Port 0 is for parallel input
> +
> +      port@1:
> +        type: object
> +        description: |
> +          Port 1 is for LVDS output
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - ports
> +
> +examples:
> +  - |
> +    lvds-encoder {
> +      compatible = "lvds-encoder";
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +
> +          lvds_enc_in: endpoint {
> +            remote-endpoint = <&display_out_rgb>;
> +          };
> +        };
> +
> +        port@1 {
> +          reg = <1>;
> +
> +          lvds_enc_out: endpoint {
> +            remote-endpoint = <&lvds_panel_in>;
> +          };
> +        };
> +      };
> +    };
> +
> +...
> -- 
> 2.7.4
> 

-- 
Regards,

Laurent Pinchart

[PATCH v4 02/13] dt-bindings: display: bridge: lvds-transmitter: Document powerdown-gpios

[Fabrizio Castro]

Add documentation for property powerdown-gpios. The property is
optional.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* New patch
---
 .../devicetree/bindings/display/bridge/lvds-transmitter.yaml         | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
index b5dd0da..2484737 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
@@ -57,6 +57,11 @@ properties:
       - port@0
       - port@1
 
+  powerdown-gpios:
+    description:
+      The GPIO used to control the power down line of this device.
+    maxItems: 1
+
 required:
   - compatible
   - ports
-- 
2.7.4

Re: [PATCH v4 02/13] dt-bindings: display: bridge: lvds-transmitter: Document powerdown-gpios

[Rob Herring]

On Wed, 13 Nov 2019 15:51:21 +0000, Fabrizio Castro wrote:
> Add documentation for property powerdown-gpios. The property is
> optional.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * New patch
> ---
>  .../devicetree/bindings/display/bridge/lvds-transmitter.yaml         | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 02/13] dt-bindings: display: bridge: lvds-transmitter: Document powerdown-gpios

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:21PM +0000, Fabrizio Castro wrote:
> Add documentation for property powerdown-gpios. The property is
> optional.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3->v4:
> * New patch
> ---
>  .../devicetree/bindings/display/bridge/lvds-transmitter.yaml         | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> index b5dd0da..2484737 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> @@ -57,6 +57,11 @@ properties:
>        - port@0
>        - port@1
>  
> +  powerdown-gpios:
> +    description:
> +      The GPIO used to control the power down line of this device.
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - ports

-- 
Regards,

Laurent Pinchart

[PATCH v4 03/13] dt-bindings: display: bridge: lvds-transmitter: Absorb ti,ds90c185.txt

[Fabrizio Castro]

ti,ds90c185.txt documents LVDS encoders using the same driver
as the one documented by lvds-transmitter.yaml.
Since the properties listed in ti,ds90c185.txt are the same
as the ones listed in lvds-transmitter.yaml, absorb the dt-binding
into lvds-transmitter.yaml.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* New patch
---
 .../bindings/display/bridge/lvds-transmitter.yaml  | 14 +++---
 .../bindings/display/bridge/ti,ds90c185.txt        | 55 ----------------------
 2 files changed, 8 insertions(+), 61 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
index 2484737..a8326ce 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
@@ -31,11 +31,13 @@ description: |
 properties:
   compatible:
     description: |
-      Any encoder compatible with this generic binding, but with additional
-      properties not listed here, must define its own binding and list a device
-      specific compatible first followed by the generic compatible.
-    enum:
-      - lvds-encoder
+      Must list the device specific compatible string first, followed by the
+      generic compatible string.
+    items:
+      - enum:
+        - ti,ds90c185       # For the TI DS90C185 FPD-Link Serializer
+        - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
+      - const: lvds-encoder # Generic LVDS encoder compatible fallback
 
   ports:
     type: object
@@ -69,7 +71,7 @@ required:
 examples:
   - |
     lvds-encoder {
-      compatible = "lvds-encoder";
+      compatible = "ti,ds90c185", "lvds-encoder";
 
       ports {
         #address-cells = <1>;
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
deleted file mode 100644
index e575f99..0000000
--- a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
+++ /dev/null
@@ -1,55 +0,0 @@
-Texas Instruments FPD-Link (LVDS) Serializer
---------------------------------------------
-
-The DS90C185 and DS90C187 are low-power serializers for portable
-battery-powered applications that reduces the size of the RGB
-interface between the host GPU and the display.
-
-Required properties:
-
-- compatible: Should be
-  "ti,ds90c185", "lvds-encoder"  for the TI DS90C185 FPD-Link Serializer
-  "ti,ds90c187", "lvds-encoder"  for the TI DS90C187 FPD-Link Serializer
-
-Optional properties:
-
-- powerdown-gpios: Power down control GPIO (the PDB pin, active-low)
-
-Required nodes:
-
-The devices have two video ports. Their connections are modeled using the OF
-graph bindings specified in Documentation/devicetree/bindings/graph.txt.
-
-- Video port 0 for parallel input
-- Video port 1 for LVDS output
-
-
-Example
--------
-
-lvds-encoder {
-	compatible = "ti,ds90c185", "lvds-encoder";
-
-	powerdown-gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
-
-	ports {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		port@0 {
-			reg = <0>;
-
-			lvds_enc_in: endpoint {
-				remote-endpoint = <&lcdc_out_rgb>;
-			};
-		};
-
-		port@1 {
-			reg = <1>;
-
-			lvds_enc_out: endpoint {
-				remote-endpoint = <&lvds_panel_in>;
-			};
-		};
-	};
-};
-- 
2.7.4

Re: [PATCH v4 03/13] dt-bindings: display: bridge: lvds-transmitter: Absorb ti,ds90c185.txt

[Rob Herring]

On Wed, 13 Nov 2019 15:51:22 +0000, Fabrizio Castro wrote:
> ti,ds90c185.txt documents LVDS encoders using the same driver
> as the one documented by lvds-transmitter.yaml.
> Since the properties listed in ti,ds90c185.txt are the same
> as the ones listed in lvds-transmitter.yaml, absorb the dt-binding
> into lvds-transmitter.yaml.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * New patch
> ---
>  .../bindings/display/bridge/lvds-transmitter.yaml  | 14 +++---
>  .../bindings/display/bridge/ti,ds90c185.txt        | 55 ----------------------
>  2 files changed, 8 insertions(+), 61 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 03/13] dt-bindings: display: bridge: lvds-transmitter: Absorb ti,ds90c185.txt

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:22PM +0000, Fabrizio Castro wrote:
> ti,ds90c185.txt documents LVDS encoders using the same driver
> as the one documented by lvds-transmitter.yaml.
> Since the properties listed in ti,ds90c185.txt are the same
> as the ones listed in lvds-transmitter.yaml, absorb the dt-binding
> into lvds-transmitter.yaml.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3->v4:
> * New patch
> ---
>  .../bindings/display/bridge/lvds-transmitter.yaml  | 14 +++---
>  .../bindings/display/bridge/ti,ds90c185.txt        | 55 ----------------------
>  2 files changed, 8 insertions(+), 61 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> index 2484737..a8326ce 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> @@ -31,11 +31,13 @@ description: |
>  properties:
>    compatible:
>      description: |
> -      Any encoder compatible with this generic binding, but with additional
> -      properties not listed here, must define its own binding and list a device
> -      specific compatible first followed by the generic compatible.
> -    enum:
> -      - lvds-encoder
> +      Must list the device specific compatible string first, followed by the
> +      generic compatible string.
> +    items:
> +      - enum:
> +        - ti,ds90c185       # For the TI DS90C185 FPD-Link Serializer
> +        - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
> +      - const: lvds-encoder # Generic LVDS encoder compatible fallback
>  
>    ports:
>      type: object
> @@ -69,7 +71,7 @@ required:
>  examples:
>    - |
>      lvds-encoder {
> -      compatible = "lvds-encoder";
> +      compatible = "ti,ds90c185", "lvds-encoder";
>  
>        ports {
>          #address-cells = <1>;
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> deleted file mode 100644
> index e575f99..0000000
> --- a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> +++ /dev/null
> @@ -1,55 +0,0 @@
> -Texas Instruments FPD-Link (LVDS) Serializer
> ---------------------------------------------
> -
> -The DS90C185 and DS90C187 are low-power serializers for portable
> -battery-powered applications that reduces the size of the RGB
> -interface between the host GPU and the display.
> -
> -Required properties:
> -
> -- compatible: Should be
> -  "ti,ds90c185", "lvds-encoder"  for the TI DS90C185 FPD-Link Serializer
> -  "ti,ds90c187", "lvds-encoder"  for the TI DS90C187 FPD-Link Serializer
> -
> -Optional properties:
> -
> -- powerdown-gpios: Power down control GPIO (the PDB pin, active-low)
> -
> -Required nodes:
> -
> -The devices have two video ports. Their connections are modeled using the OF
> -graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> -
> -- Video port 0 for parallel input
> -- Video port 1 for LVDS output
> -
> -
> -Example
> --------
> -
> -lvds-encoder {
> -	compatible = "ti,ds90c185", "lvds-encoder";
> -
> -	powerdown-gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
> -
> -	ports {
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
> -		port@0 {
> -			reg = <0>;
> -
> -			lvds_enc_in: endpoint {
> -				remote-endpoint = <&lcdc_out_rgb>;
> -			};
> -		};
> -
> -		port@1 {
> -			reg = <1>;
> -
> -			lvds_enc_out: endpoint {
> -				remote-endpoint = <&lvds_panel_in>;
> -			};
> -		};
> -	};
> -};
> -- 
> 2.7.4
> 

-- 
Regards,

Laurent Pinchart

[PATCH v4 04/13] dt-bindings: display: bridge: lvds-transmitter: Document "ti,sn75lvds83"

[Fabrizio Castro]

Compatible string "ti,sn75lvds83" is being used by device tree
rk3188-bqedison2qc.dts, but it's not documented anywhere, therefore
document it within lvds-transmitter.yaml.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* New patch
---
 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
index a8326ce..27de616 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
@@ -37,6 +37,7 @@ properties:
       - enum:
         - ti,ds90c185       # For the TI DS90C185 FPD-Link Serializer
         - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
+        - ti,sn75lvds83     # For the TI SN75LVDS83 FlatLink transmitter
       - const: lvds-encoder # Generic LVDS encoder compatible fallback
 
   ports:
-- 
2.7.4

Re: [PATCH v4 04/13] dt-bindings: display: bridge: lvds-transmitter: Document "ti,sn75lvds83"

[Rob Herring]

On Wed, 13 Nov 2019 15:51:23 +0000, Fabrizio Castro wrote:
> Compatible string "ti,sn75lvds83" is being used by device tree
> rk3188-bqedison2qc.dts, but it's not documented anywhere, therefore
> document it within lvds-transmitter.yaml.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * New patch
> ---
>  Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 04/13] dt-bindings: display: bridge: lvds-transmitter: Document "ti,sn75lvds83"

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:23PM +0000, Fabrizio Castro wrote:
> Compatible string "ti,sn75lvds83" is being used by device tree
> rk3188-bqedison2qc.dts, but it's not documented anywhere, therefore
> document it within lvds-transmitter.yaml.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3->v4:
> * New patch
> ---
>  Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> index a8326ce..27de616 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> @@ -37,6 +37,7 @@ properties:
>        - enum:
>          - ti,ds90c185       # For the TI DS90C185 FPD-Link Serializer
>          - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
> +        - ti,sn75lvds83     # For the TI SN75LVDS83 FlatLink transmitter
>        - const: lvds-encoder # Generic LVDS encoder compatible fallback
>  
>    ports:

-- 
Regards,

Laurent Pinchart

[PATCH v4 05/13] drm/bridge: Repurpose lvds-encoder.c

[Fabrizio Castro]

lvds-encoder.c implementation is also suitable for LVDS decoders,
not just LVDS encoders.
Instead of creating a new driver for addressing support for
transparent LVDS decoders, repurpose lvds-encoder.c for the greater
good with this patch.

This patch only "rebrands" the lvds-encoder.c driver, to make it
suitable for hosting LVDS decoders support. The actual support for
LVDS decoders will come with a later patch.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* The patch now only renames the driver (with related data structures
  and build options), changes the description of the module, and
  refreshes the copyright
v2->v3:
* No change
v1->v2:
* No change
---
 drivers/gpu/drm/bridge/Kconfig                     |  8 +-
 drivers/gpu/drm/bridge/Makefile                    |  2 +-
 .../drm/bridge/{lvds-encoder.c => lvds-codec.c}    | 97 +++++++++++-----------
 3 files changed, 53 insertions(+), 54 deletions(-)
 rename drivers/gpu/drm/bridge/{lvds-encoder.c => lvds-codec.c} (45%)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3436297..fbbea46 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
 	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
 	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
 
-config DRM_LVDS_ENCODER
-	tristate "Transparent parallel to LVDS encoder support"
+config DRM_LVDS_CODEC
+	tristate "Transparent LVDS encoders and decoders support"
 	depends on OF
 	select DRM_KMS_HELPER
 	select DRM_PANEL_BRIDGE
 	help
-	  Support for transparent parallel to LVDS encoders that don't require
-	  any configuration.
+	  Support for transparent LVDS encoders and decoders that don't
+	  require any configuration.
 
 config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
 	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 4934fcf..8a9178a 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -2,7 +2,7 @@
 obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
 obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
 obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
-obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
+obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
 obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
 obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
 obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-codec.c
similarity index 45%
rename from drivers/gpu/drm/bridge/lvds-encoder.c
rename to drivers/gpu/drm/bridge/lvds-codec.c
index e2132a8..b5801a2 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
+ * Copyright (C) 2019 Renesas Electronics Corporation
  * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
  */
 
@@ -12,65 +13,62 @@
 #include <drm/drm_bridge.h>
 #include <drm/drm_panel.h>
 
-struct lvds_encoder {
+struct lvds_codec {
 	struct drm_bridge bridge;
 	struct drm_bridge *panel_bridge;
 	struct gpio_desc *powerdown_gpio;
 };
 
-static int lvds_encoder_attach(struct drm_bridge *bridge)
+static int lvds_codec_attach(struct drm_bridge *bridge)
 {
-	struct lvds_encoder *lvds_encoder = container_of(bridge,
-							 struct lvds_encoder,
-							 bridge);
+	struct lvds_codec *lvds_codec = container_of(bridge,
+						     struct lvds_codec, bridge);
 
-	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
+	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
 				 bridge);
 }
 
-static void lvds_encoder_enable(struct drm_bridge *bridge)
+static void lvds_codec_enable(struct drm_bridge *bridge)
 {
-	struct lvds_encoder *lvds_encoder = container_of(bridge,
-							 struct lvds_encoder,
-							 bridge);
+	struct lvds_codec *lvds_codec = container_of(bridge,
+						     struct lvds_codec, bridge);
 
-	if (lvds_encoder->powerdown_gpio)
-		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
+	if (lvds_codec->powerdown_gpio)
+		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
 }
 
-static void lvds_encoder_disable(struct drm_bridge *bridge)
+static void lvds_codec_disable(struct drm_bridge *bridge)
 {
-	struct lvds_encoder *lvds_encoder = container_of(bridge,
-							 struct lvds_encoder,
-							 bridge);
+	struct lvds_codec *lvds_codec = container_of(bridge,
+						     struct lvds_codec, bridge);
 
-	if (lvds_encoder->powerdown_gpio)
-		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
+	if (lvds_codec->powerdown_gpio)
+		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
 }
 
 static struct drm_bridge_funcs funcs = {
-	.attach = lvds_encoder_attach,
-	.enable = lvds_encoder_enable,
-	.disable = lvds_encoder_disable,
+	.attach = lvds_codec_attach,
+	.enable = lvds_codec_enable,
+	.disable = lvds_codec_disable,
 };
 
-static int lvds_encoder_probe(struct platform_device *pdev)
+static int lvds_codec_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *port;
 	struct device_node *endpoint;
 	struct device_node *panel_node;
 	struct drm_panel *panel;
-	struct lvds_encoder *lvds_encoder;
+	struct lvds_codec *lvds_codec;
 
-	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
-	if (!lvds_encoder)
+	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
+	if (!lvds_codec)
 		return -ENOMEM;
 
-	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
-							       GPIOD_OUT_HIGH);
-	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
-		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
+	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
+							     GPIOD_OUT_HIGH);
+	if (IS_ERR(lvds_codec->powerdown_gpio)) {
+		int err = PTR_ERR(lvds_codec->powerdown_gpio);
 
 		if (err != -EPROBE_DEFER)
 			dev_err(dev, "powerdown GPIO failure: %d\n", err);
@@ -105,51 +103,52 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 		return PTR_ERR(panel);
 	}
 
-	lvds_encoder->panel_bridge =
+	lvds_codec->panel_bridge =
 		devm_drm_panel_bridge_add_typed(dev, panel,
 						DRM_MODE_CONNECTOR_LVDS);
-	if (IS_ERR(lvds_encoder->panel_bridge))
-		return PTR_ERR(lvds_encoder->panel_bridge);
+	if (IS_ERR(lvds_codec->panel_bridge))
+		return PTR_ERR(lvds_codec->panel_bridge);
 
-	/* The panel_bridge bridge is attached to the panel's of_node,
+	/*
+	 * The panel_bridge bridge is attached to the panel's of_node,
 	 * but we need a bridge attached to our of_node for our user
 	 * to look up.
 	 */
-	lvds_encoder->bridge.of_node = dev->of_node;
-	lvds_encoder->bridge.funcs = &funcs;
-	drm_bridge_add(&lvds_encoder->bridge);
+	lvds_codec->bridge.of_node = dev->of_node;
+	lvds_codec->bridge.funcs = &funcs;
+	drm_bridge_add(&lvds_codec->bridge);
 
-	platform_set_drvdata(pdev, lvds_encoder);
+	platform_set_drvdata(pdev, lvds_codec);
 
 	return 0;
 }
 
-static int lvds_encoder_remove(struct platform_device *pdev)
+static int lvds_codec_remove(struct platform_device *pdev)
 {
-	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
+	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
 
-	drm_bridge_remove(&lvds_encoder->bridge);
+	drm_bridge_remove(&lvds_codec->bridge);
 
 	return 0;
 }
 
-static const struct of_device_id lvds_encoder_match[] = {
+static const struct of_device_id lvds_codec_match[] = {
 	{ .compatible = "lvds-encoder" },
 	{ .compatible = "thine,thc63lvdm83d" },
 	{},
 };
-MODULE_DEVICE_TABLE(of, lvds_encoder_match);
+MODULE_DEVICE_TABLE(of, lvds_codec_match);
 
-static struct platform_driver lvds_encoder_driver = {
-	.probe	= lvds_encoder_probe,
-	.remove	= lvds_encoder_remove,
+static struct platform_driver lvds_codec_driver = {
+	.probe	= lvds_codec_probe,
+	.remove	= lvds_codec_remove,
 	.driver		= {
-		.name		= "lvds-encoder",
-		.of_match_table	= lvds_encoder_match,
+		.name		= "lvds-codec",
+		.of_match_table	= lvds_codec_match,
 	},
 };
-module_platform_driver(lvds_encoder_driver);
+module_platform_driver(lvds_codec_driver);
 
 MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
-MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
+MODULE_DESCRIPTION("LVDS encoders and decoders");
 MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH v4 05/13] drm/bridge: Repurpose lvds-encoder.c

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:24PM +0000, Fabrizio Castro wrote:
> lvds-encoder.c implementation is also suitable for LVDS decoders,
> not just LVDS encoders.
> Instead of creating a new driver for addressing support for
> transparent LVDS decoders, repurpose lvds-encoder.c for the greater
> good with this patch.
> 
> This patch only "rebrands" the lvds-encoder.c driver, to make it
> suitable for hosting LVDS decoders support. The actual support for
> LVDS decoders will come with a later patch.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3->v4:
> * The patch now only renames the driver (with related data structures
>   and build options), changes the description of the module, and
>   refreshes the copyright
> v2->v3:
> * No change
> v1->v2:
> * No change
> ---
>  drivers/gpu/drm/bridge/Kconfig                     |  8 +-
>  drivers/gpu/drm/bridge/Makefile                    |  2 +-
>  .../drm/bridge/{lvds-encoder.c => lvds-codec.c}    | 97 +++++++++++-----------
>  3 files changed, 53 insertions(+), 54 deletions(-)
>  rename drivers/gpu/drm/bridge/{lvds-encoder.c => lvds-codec.c} (45%)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3436297..fbbea46 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC
>  	  Support for non-programmable RGB to VGA DAC bridges, such as ADI
>  	  ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs.
>  
> -config DRM_LVDS_ENCODER
> -	tristate "Transparent parallel to LVDS encoder support"
> +config DRM_LVDS_CODEC
> +	tristate "Transparent LVDS encoders and decoders support"
>  	depends on OF
>  	select DRM_KMS_HELPER
>  	select DRM_PANEL_BRIDGE
>  	help
> -	  Support for transparent parallel to LVDS encoders that don't require
> -	  any configuration.
> +	  Support for transparent LVDS encoders and decoders that don't
> +	  require any configuration.
>  
>  config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
>  	tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw"
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 4934fcf..8a9178a 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -2,7 +2,7 @@
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> -obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> +obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-codec.c
> similarity index 45%
> rename from drivers/gpu/drm/bridge/lvds-encoder.c
> rename to drivers/gpu/drm/bridge/lvds-codec.c
> index e2132a8..b5801a2 100644
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> + * Copyright (C) 2019 Renesas Electronics Corporation
>   * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>   */
>  
> @@ -12,65 +13,62 @@
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_panel.h>
>  
> -struct lvds_encoder {
> +struct lvds_codec {
>  	struct drm_bridge bridge;
>  	struct drm_bridge *panel_bridge;
>  	struct gpio_desc *powerdown_gpio;
>  };
>  
> -static int lvds_encoder_attach(struct drm_bridge *bridge)
> +static int lvds_codec_attach(struct drm_bridge *bridge)
>  {
> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> -							 struct lvds_encoder,
> -							 bridge);
> +	struct lvds_codec *lvds_codec = container_of(bridge,
> +						     struct lvds_codec, bridge);
>  
> -	return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
> +	return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
>  				 bridge);
>  }
>  
> -static void lvds_encoder_enable(struct drm_bridge *bridge)
> +static void lvds_codec_enable(struct drm_bridge *bridge)
>  {
> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> -							 struct lvds_encoder,
> -							 bridge);
> +	struct lvds_codec *lvds_codec = container_of(bridge,
> +						     struct lvds_codec, bridge);
>  
> -	if (lvds_encoder->powerdown_gpio)
> -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> +	if (lvds_codec->powerdown_gpio)
> +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
>  }
>  
> -static void lvds_encoder_disable(struct drm_bridge *bridge)
> +static void lvds_codec_disable(struct drm_bridge *bridge)
>  {
> -	struct lvds_encoder *lvds_encoder = container_of(bridge,
> -							 struct lvds_encoder,
> -							 bridge);
> +	struct lvds_codec *lvds_codec = container_of(bridge,
> +						     struct lvds_codec, bridge);
>  
> -	if (lvds_encoder->powerdown_gpio)
> -		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> +	if (lvds_codec->powerdown_gpio)
> +		gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
>  }
>  
>  static struct drm_bridge_funcs funcs = {
> -	.attach = lvds_encoder_attach,
> -	.enable = lvds_encoder_enable,
> -	.disable = lvds_encoder_disable,
> +	.attach = lvds_codec_attach,
> +	.enable = lvds_codec_enable,
> +	.disable = lvds_codec_disable,
>  };
>  
> -static int lvds_encoder_probe(struct platform_device *pdev)
> +static int lvds_codec_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *port;
>  	struct device_node *endpoint;
>  	struct device_node *panel_node;
>  	struct drm_panel *panel;
> -	struct lvds_encoder *lvds_encoder;
> +	struct lvds_codec *lvds_codec;
>  
> -	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
> -	if (!lvds_encoder)
> +	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> +	if (!lvds_codec)
>  		return -ENOMEM;
>  
> -	lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> -							       GPIOD_OUT_HIGH);
> -	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
> -		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
> +	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> +							     GPIOD_OUT_HIGH);
> +	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> +		int err = PTR_ERR(lvds_codec->powerdown_gpio);
>  
>  		if (err != -EPROBE_DEFER)
>  			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> @@ -105,51 +103,52 @@ static int lvds_encoder_probe(struct platform_device *pdev)
>  		return PTR_ERR(panel);
>  	}
>  
> -	lvds_encoder->panel_bridge =
> +	lvds_codec->panel_bridge =
>  		devm_drm_panel_bridge_add_typed(dev, panel,
>  						DRM_MODE_CONNECTOR_LVDS);
> -	if (IS_ERR(lvds_encoder->panel_bridge))
> -		return PTR_ERR(lvds_encoder->panel_bridge);
> +	if (IS_ERR(lvds_codec->panel_bridge))
> +		return PTR_ERR(lvds_codec->panel_bridge);
>  
> -	/* The panel_bridge bridge is attached to the panel's of_node,
> +	/*
> +	 * The panel_bridge bridge is attached to the panel's of_node,
>  	 * but we need a bridge attached to our of_node for our user
>  	 * to look up.
>  	 */
> -	lvds_encoder->bridge.of_node = dev->of_node;
> -	lvds_encoder->bridge.funcs = &funcs;
> -	drm_bridge_add(&lvds_encoder->bridge);
> +	lvds_codec->bridge.of_node = dev->of_node;
> +	lvds_codec->bridge.funcs = &funcs;
> +	drm_bridge_add(&lvds_codec->bridge);
>  
> -	platform_set_drvdata(pdev, lvds_encoder);
> +	platform_set_drvdata(pdev, lvds_codec);
>  
>  	return 0;
>  }
>  
> -static int lvds_encoder_remove(struct platform_device *pdev)
> +static int lvds_codec_remove(struct platform_device *pdev)
>  {
> -	struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
> +	struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
>  
> -	drm_bridge_remove(&lvds_encoder->bridge);
> +	drm_bridge_remove(&lvds_codec->bridge);
>  
>  	return 0;
>  }
>  
> -static const struct of_device_id lvds_encoder_match[] = {
> +static const struct of_device_id lvds_codec_match[] = {
>  	{ .compatible = "lvds-encoder" },
>  	{ .compatible = "thine,thc63lvdm83d" },
>  	{},
>  };
> -MODULE_DEVICE_TABLE(of, lvds_encoder_match);
> +MODULE_DEVICE_TABLE(of, lvds_codec_match);
>  
> -static struct platform_driver lvds_encoder_driver = {
> -	.probe	= lvds_encoder_probe,
> -	.remove	= lvds_encoder_remove,
> +static struct platform_driver lvds_codec_driver = {
> +	.probe	= lvds_codec_probe,
> +	.remove	= lvds_codec_remove,
>  	.driver		= {
> -		.name		= "lvds-encoder",
> -		.of_match_table	= lvds_encoder_match,
> +		.name		= "lvds-codec",
> +		.of_match_table	= lvds_codec_match,
>  	},
>  };
> -module_platform_driver(lvds_encoder_driver);
> +module_platform_driver(lvds_codec_driver);
>  
>  MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
> +MODULE_DESCRIPTION("LVDS encoders and decoders");
>  MODULE_LICENSE("GPL");

-- 
Regards,

Laurent Pinchart

[PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Fabrizio Castro]

Add support for transparent LVDS decoders by adding a new
compatible string ("lvds-decoder") to the driver.
This patch also adds member connector_type to struct lvds_codec,
and that's because LVDS decoders have a different connector type
from LVDS encoders. We fill this new member up with the data
matching the compatible string.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* New patch
---
 drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
index b5801a2..c32e125 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -7,6 +7,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_graph.h>
 #include <linux/platform_device.h>
 
@@ -17,6 +18,7 @@ struct lvds_codec {
 	struct drm_bridge bridge;
 	struct drm_bridge *panel_bridge;
 	struct gpio_desc *powerdown_gpio;
+	u32 connector_type;
 };
 
 static int lvds_codec_attach(struct drm_bridge *bridge)
@@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
 	if (!lvds_codec)
 		return -ENOMEM;
 
+	lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
 	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
 							     GPIOD_OUT_HIGH);
 	if (IS_ERR(lvds_codec->powerdown_gpio)) {
@@ -105,7 +108,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
 
 	lvds_codec->panel_bridge =
 		devm_drm_panel_bridge_add_typed(dev, panel,
-						DRM_MODE_CONNECTOR_LVDS);
+						lvds_codec->connector_type);
 	if (IS_ERR(lvds_codec->panel_bridge))
 		return PTR_ERR(lvds_codec->panel_bridge);
 
@@ -133,8 +136,18 @@ static int lvds_codec_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id lvds_codec_match[] = {
-	{ .compatible = "lvds-encoder" },
-	{ .compatible = "thine,thc63lvdm83d" },
+	{
+		.compatible = "lvds-decoder",
+		.data = (void *)DRM_MODE_CONNECTOR_DPI,
+	},
+	{
+		.compatible = "lvds-encoder",
+		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
+	},
+	{
+		.compatible = "thine,thc63lvdm83d",
+		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, lvds_codec_match);
-- 
2.7.4

Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> Add support for transparent LVDS decoders by adding a new
> compatible string ("lvds-decoder") to the driver.
> This patch also adds member connector_type to struct lvds_codec,
> and that's because LVDS decoders have a different connector type
> from LVDS encoders. We fill this new member up with the data
> matching the compatible string.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3->v4:
> * New patch
> ---
>  drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> index b5801a2..c32e125 100644
> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -7,6 +7,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
>  
> @@ -17,6 +18,7 @@ struct lvds_codec {
>  	struct drm_bridge bridge;
>  	struct drm_bridge *panel_bridge;
>  	struct gpio_desc *powerdown_gpio;
> +	u32 connector_type;
>  };
>  
>  static int lvds_codec_attach(struct drm_bridge *bridge)
> @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  	if (!lvds_codec)
>  		return -ENOMEM;
>  
> +	lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
>  	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>  							     GPIOD_OUT_HIGH);
>  	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> @@ -105,7 +108,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  
>  	lvds_codec->panel_bridge =
>  		devm_drm_panel_bridge_add_typed(dev, panel,
> -						DRM_MODE_CONNECTOR_LVDS);
> +						lvds_codec->connector_type);
>  	if (IS_ERR(lvds_codec->panel_bridge))
>  		return PTR_ERR(lvds_codec->panel_bridge);
>  
> @@ -133,8 +136,18 @@ static int lvds_codec_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id lvds_codec_match[] = {
> -	{ .compatible = "lvds-encoder" },
> -	{ .compatible = "thine,thc63lvdm83d" },
> +	{
> +		.compatible = "lvds-decoder",
> +		.data = (void *)DRM_MODE_CONNECTOR_DPI,
> +	},
> +	{
> +		.compatible = "lvds-encoder",
> +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> +	},
> +	{
> +		.compatible = "thine,thc63lvdm83d",
> +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> +	},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, lvds_codec_match);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Laurent Pinchart]

Hi Fabrizio,

On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> Add support for transparent LVDS decoders by adding a new
> compatible string ("lvds-decoder") to the driver.
> This patch also adds member connector_type to struct lvds_codec,
> and that's because LVDS decoders have a different connector type
> from LVDS encoders. We fill this new member up with the data
> matching the compatible string.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * New patch
> ---
>  drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> index b5801a2..c32e125 100644
> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -7,6 +7,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
>  
> @@ -17,6 +18,7 @@ struct lvds_codec {
>  	struct drm_bridge bridge;
>  	struct drm_bridge *panel_bridge;
>  	struct gpio_desc *powerdown_gpio;
> +	u32 connector_type;
>  };
>  
>  static int lvds_codec_attach(struct drm_bridge *bridge)
> @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  	if (!lvds_codec)
>  		return -ENOMEM;
>  
> +	lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);

I'm now getting a compilation failure here:

drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’:
drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
  lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
                               ^

The fix should be simple:

	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);

I'm bothered by the fact that I've compiled this before without any
issue, so this really puzzles me. Do you get the same warning ?

>  	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>  							     GPIOD_OUT_HIGH);
>  	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> @@ -105,7 +108,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  
>  	lvds_codec->panel_bridge =
>  		devm_drm_panel_bridge_add_typed(dev, panel,
> -						DRM_MODE_CONNECTOR_LVDS);
> +						lvds_codec->connector_type);
>  	if (IS_ERR(lvds_codec->panel_bridge))
>  		return PTR_ERR(lvds_codec->panel_bridge);
>  
> @@ -133,8 +136,18 @@ static int lvds_codec_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id lvds_codec_match[] = {
> -	{ .compatible = "lvds-encoder" },
> -	{ .compatible = "thine,thc63lvdm83d" },
> +	{
> +		.compatible = "lvds-decoder",
> +		.data = (void *)DRM_MODE_CONNECTOR_DPI,
> +	},
> +	{
> +		.compatible = "lvds-encoder",
> +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> +	},
> +	{
> +		.compatible = "thine,thc63lvdm83d",
> +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> +	},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, lvds_codec_match);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Laurent Pinchart]

Hi Fabrizio,

Ping ?

On Fri, Dec 13, 2019 at 07:10:38PM +0200, Laurent Pinchart wrote:
> On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> > Add support for transparent LVDS decoders by adding a new
> > compatible string ("lvds-decoder") to the driver.
> > This patch also adds member connector_type to struct lvds_codec,
> > and that's because LVDS decoders have a different connector type
> > from LVDS encoders. We fill this new member up with the data
> > matching the compatible string.
> > 
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > 
> > ---
> > v3->v4:
> > * New patch
> > ---
> >  drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > index b5801a2..c32e125 100644
> > --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <linux/of_graph.h>
> >  #include <linux/platform_device.h>
> >  
> > @@ -17,6 +18,7 @@ struct lvds_codec {
> >  	struct drm_bridge bridge;
> >  	struct drm_bridge *panel_bridge;
> >  	struct gpio_desc *powerdown_gpio;
> > +	u32 connector_type;
> >  };
> >  
> >  static int lvds_codec_attach(struct drm_bridge *bridge)
> > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >  	if (!lvds_codec)
> >  		return -ENOMEM;
> >  
> > +	lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> 
> I'm now getting a compilation failure here:
> 
> drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’:
> drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
>                                ^
> 
> The fix should be simple:
> 
> 	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> 
> I'm bothered by the fact that I've compiled this before without any
> issue, so this really puzzles me. Do you get the same warning ?
> 
> >  	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> >  							     GPIOD_OUT_HIGH);
> >  	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > @@ -105,7 +108,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >  
> >  	lvds_codec->panel_bridge =
> >  		devm_drm_panel_bridge_add_typed(dev, panel,
> > -						DRM_MODE_CONNECTOR_LVDS);
> > +						lvds_codec->connector_type);
> >  	if (IS_ERR(lvds_codec->panel_bridge))
> >  		return PTR_ERR(lvds_codec->panel_bridge);
> >  
> > @@ -133,8 +136,18 @@ static int lvds_codec_remove(struct platform_device *pdev)
> >  }
> >  
> >  static const struct of_device_id lvds_codec_match[] = {
> > -	{ .compatible = "lvds-encoder" },
> > -	{ .compatible = "thine,thc63lvdm83d" },
> > +	{
> > +		.compatible = "lvds-decoder",
> > +		.data = (void *)DRM_MODE_CONNECTOR_DPI,
> > +	},
> > +	{
> > +		.compatible = "lvds-encoder",
> > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > +	},
> > +	{
> > +		.compatible = "thine,thc63lvdm83d",
> > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > +	},
> >  	{},
> >  };
> >  MODULE_DEVICE_TABLE(of, lvds_codec_match);

-- 
Regards,

Laurent Pinchart

RE: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Fabrizio Castro]

Hi Laurent,

Thank you for your feedback!

> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 13 December 2019 17:11
> Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support
> 
> Hi Fabrizio,
> 
> On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> > Add support for transparent LVDS decoders by adding a new
> > compatible string ("lvds-decoder") to the driver.
> > This patch also adds member connector_type to struct lvds_codec,
> > and that's because LVDS decoders have a different connector type
> > from LVDS encoders. We fill this new member up with the data
> > matching the compatible string.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v3->v4:
> > * New patch
> > ---
> >  drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > index b5801a2..c32e125 100644
> > --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <linux/of_graph.h>
> >  #include <linux/platform_device.h>
> >
> > @@ -17,6 +18,7 @@ struct lvds_codec {
> >  	struct drm_bridge bridge;
> >  	struct drm_bridge *panel_bridge;
> >  	struct gpio_desc *powerdown_gpio;
> > +	u32 connector_type;
> >  };
> >
> >  static int lvds_codec_attach(struct drm_bridge *bridge)
> > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >  	if (!lvds_codec)
> >  		return -ENOMEM;
> >
> > +	lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> 
> I'm now getting a compilation failure here:
> 
> drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’:
> drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
>                                ^
> 
> The fix should be simple:
> 
> 	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> 
> I'm bothered by the fact that I've compiled this before without any
> issue, so this really puzzles me. Do you get the same warning ?

The warning appears when compiling for arm64, understandably so.
We must have compiled this for arm only the first time around.

I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long).

What's your preference?

Thanks,
Fab

> 
> >  	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> >  							     GPIOD_OUT_HIGH);
> >  	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > @@ -105,7 +108,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >
> >  	lvds_codec->panel_bridge =
> >  		devm_drm_panel_bridge_add_typed(dev, panel,
> > -						DRM_MODE_CONNECTOR_LVDS);
> > +						lvds_codec->connector_type);
> >  	if (IS_ERR(lvds_codec->panel_bridge))
> >  		return PTR_ERR(lvds_codec->panel_bridge);
> >
> > @@ -133,8 +136,18 @@ static int lvds_codec_remove(struct platform_device *pdev)
> >  }
> >
> >  static const struct of_device_id lvds_codec_match[] = {
> > -	{ .compatible = "lvds-encoder" },
> > -	{ .compatible = "thine,thc63lvdm83d" },
> > +	{
> > +		.compatible = "lvds-decoder",
> > +		.data = (void *)DRM_MODE_CONNECTOR_DPI,
> > +	},
> > +	{
> > +		.compatible = "lvds-encoder",
> > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > +	},
> > +	{
> > +		.compatible = "thine,thc63lvdm83d",
> > +		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > +	},
> >  	{},
> >  };
> >  MODULE_DEVICE_TABLE(of, lvds_codec_match);
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Geert Uytterhoeven]

Hi Fabrizio,

On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Sent: 13 December 2019 17:11
> > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support
> >
> > On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> > > Add support for transparent LVDS decoders by adding a new
> > > compatible string ("lvds-decoder") to the driver.
> > > This patch also adds member connector_type to struct lvds_codec,
> > > and that's because LVDS decoders have a different connector type
> > > from LVDS encoders. We fill this new member up with the data
> > > matching the compatible string.
> > >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > ---
> > > v3->v4:
> > > * New patch
> > > ---
> > >  drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++---
> > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > index b5801a2..c32e125 100644
> > > --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > @@ -7,6 +7,7 @@
> > >  #include <linux/gpio/consumer.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > > +#include <linux/of_device.h>
> > >  #include <linux/of_graph.h>
> > >  #include <linux/platform_device.h>
> > >
> > > @@ -17,6 +18,7 @@ struct lvds_codec {
> > >     struct drm_bridge bridge;
> > >     struct drm_bridge *panel_bridge;
> > >     struct gpio_desc *powerdown_gpio;
> > > +   u32 connector_type;
> > >  };
> > >
> > >  static int lvds_codec_attach(struct drm_bridge *bridge)
> > > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > >     if (!lvds_codec)
> > >             return -ENOMEM;
> > >
> > > +   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> >
> > I'm now getting a compilation failure here:
> >
> > drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’:
> > drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> >   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> >                                ^
> >
> > The fix should be simple:
> >
> >       lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> >
> > I'm bothered by the fact that I've compiled this before without any
> > issue, so this really puzzles me. Do you get the same warning ?
>
> The warning appears when compiling for arm64, understandably so.
> We must have compiled this for arm only the first time around.
>
> I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long).

Just casting to uintptr_t should be sufficient.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Fabrizio Castro]

Hi Geert,

Thank you for your feedback!

> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Geert Uytterhoeven
> Sent: 17 December 2019 12:21
> Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support
> 
> Hi Fabrizio,
> 
> On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Sent: 13 December 2019 17:11
> > > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support
> > >
> > > On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> > > > Add support for transparent LVDS decoders by adding a new
> > > > compatible string ("lvds-decoder") to the driver.
> > > > This patch also adds member connector_type to struct lvds_codec,
> > > > and that's because LVDS decoders have a different connector type
> > > > from LVDS encoders. We fill this new member up with the data
> > > > matching the compatible string.
> > > >
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > >
> > > > ---
> > > > v3->v4:
> > > > * New patch
> > > > ---
> > > >  drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++---
> > > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > > index b5801a2..c32e125 100644
> > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > > @@ -7,6 +7,7 @@
> > > >  #include <linux/gpio/consumer.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > > >  #include <linux/of_graph.h>
> > > >  #include <linux/platform_device.h>
> > > >
> > > > @@ -17,6 +18,7 @@ struct lvds_codec {
> > > >     struct drm_bridge bridge;
> > > >     struct drm_bridge *panel_bridge;
> > > >     struct gpio_desc *powerdown_gpio;
> > > > +   u32 connector_type;
> > > >  };
> > > >
> > > >  static int lvds_codec_attach(struct drm_bridge *bridge)
> > > > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > > >     if (!lvds_codec)
> > > >             return -ENOMEM;
> > > >
> > > > +   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > >
> > > I'm now getting a compilation failure here:
> > >
> > > drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’:
> > > drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> > >   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > >                                ^
> > >
> > > The fix should be simple:
> > >
> > >       lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> > >
> > > I'm bothered by the fact that I've compiled this before without any
> > > issue, so this really puzzles me. Do you get the same warning ?
> >
> > The warning appears when compiling for arm64, understandably so.
> > We must have compiled this for arm only the first time around.
> >
> > I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long).
> 
> Just casting to uintptr_t should be sufficient.

It should be sufficient for the compiler, but I have seen examples where people
preferred to be explicit, like in:
drivers/mailbox/mtk-cmdq-mailbox.c
drivers/leds/leds-pm8058.c

Since the kernel is increasing its tightness with respect to warnings, I personally prefer
(u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you
don't like (u32)(uintptr_t).

Cheers,
Fab

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Geert Uytterhoeven]

Hi Fabrizio,

On Tue, Dec 17, 2019 at 1:31 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> > From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Geert Uytterhoeven
> > Sent: 17 December 2019 12:21
> > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support
> >
> > On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro
> > <fabrizio.castro@bp.renesas.com> wrote:
> > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Sent: 13 December 2019 17:11
> > > > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support
> > > >
> > > > On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> > > > > Add support for transparent LVDS decoders by adding a new
> > > > > compatible string ("lvds-decoder") to the driver.
> > > > > This patch also adds member connector_type to struct lvds_codec,
> > > > > and that's because LVDS decoders have a different connector type
> > > > > from LVDS encoders. We fill this new member up with the data
> > > > > matching the compatible string.
> > > > >
> > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

> > > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c

> > > > > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > > > >     if (!lvds_codec)
> > > > >             return -ENOMEM;
> > > > >
> > > > > +   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > > >
> > > > I'm now getting a compilation failure here:
> > > >
> > > > drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’:
> > > > drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> > > >   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > > >                                ^
> > > >
> > > > The fix should be simple:
> > > >
> > > >       lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> > > >
> > > > I'm bothered by the fact that I've compiled this before without any
> > > > issue, so this really puzzles me. Do you get the same warning ?
> > >
> > > The warning appears when compiling for arm64, understandably so.
> > > We must have compiled this for arm only the first time around.
> > >
> > > I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long).
> >
> > Just casting to uintptr_t should be sufficient.
>
> It should be sufficient for the compiler, but I have seen examples where people
> preferred to be explicit, like in:
> drivers/mailbox/mtk-cmdq-mailbox.c
> drivers/leds/leds-pm8058.c
>
> Since the kernel is increasing its tightness with respect to warnings, I personally prefer
> (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you
> don't like (u32)(uintptr_t).

It depends. I try to have as few casts as possible ("casts are evil").

While adding the extra (u32) cast makes it clearer that the intended
result is a u32 (for now), it will cause silent truncation on 64-bit if
connector_type is ever enlarged to unsigned long, and larger values are
used.

In this particular case this is unlikely, though, as unsigned long would
still be 32-bit on 32-bit platforms, so the larger values cannot be
used.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Laurent Pinchart]

Hello,

On Tue, Dec 17, 2019 at 01:38:51PM +0100, Geert Uytterhoeven wrote:
> On Tue, Dec 17, 2019 at 1:31 PM Fabrizio Castro wrote:
> > On 17 December 2019 12:21, Geert Uytterhoeven wrote:
> >> On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro wrote:
> >>> On 13 December 2019 17:11, Laurent Pinchart wrote:
> >>>> On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> >>>>> Add support for transparent LVDS decoders by adding a new
> >>>>> compatible string ("lvds-decoder") to the driver.
> >>>>> This patch also adds member connector_type to struct lvds_codec,
> >>>>> and that's because LVDS decoders have a different connector type
> >>>>> from LVDS encoders. We fill this new member up with the data
> >>>>> matching the compatible string.
> >>>>>
> >>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >>>>>
> >>>>> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> >>>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> >>>>>
> >>>>> @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >>>>>     if (!lvds_codec)
> >>>>>             return -ENOMEM;
> >>>>>
> >>>>> +   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> >>>>
> >>>> I'm now getting a compilation failure here:
> >>>>
> >>>> drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’:
> >>>> drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> >>>>   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> >>>>                                ^
> >>>>
> >>>> The fix should be simple:
> >>>>
> >>>>       lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> >>>>
> >>>> I'm bothered by the fact that I've compiled this before without any
> >>>> issue, so this really puzzles me. Do you get the same warning ?
> >>>
> >>> The warning appears when compiling for arm64, understandably so.
> >>> We must have compiled this for arm only the first time around.
> >>>
> >>> I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long).
> >>
> >> Just casting to uintptr_t should be sufficient.
> >
> > It should be sufficient for the compiler, but I have seen examples where people
> > preferred to be explicit, like in:
> > drivers/mailbox/mtk-cmdq-mailbox.c
> > drivers/leds/leds-pm8058.c
> >
> > Since the kernel is increasing its tightness with respect to warnings, I personally prefer
> > (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you
> > don't like (u32)(uintptr_t).
> 
> It depends. I try to have as few casts as possible ("casts are evil").
> 
> While adding the extra (u32) cast makes it clearer that the intended
> result is a u32 (for now), it will cause silent truncation on 64-bit if
> connector_type is ever enlarged to unsigned long, and larger values are
> used.
> 
> In this particular case this is unlikely, though, as unsigned long would
> still be 32-bit on 32-bit platforms, so the larger values cannot be
> used.

I also try to add as few casts as possible, so (uintptr_t) would be my
preference.

Fabrizio, could you submit a new version of this patch with the problem
fixed (and with the casts you decide to use, but using uintptr_t instead
of unsigned long in any case) ?

-- 
Regards,

Laurent Pinchart

RE: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Fabrizio Castro]

Hi guys,

> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 17 December 2019 13:54
> Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support
> 
> Hello,
> 
> On Tue, Dec 17, 2019 at 01:38:51PM +0100, Geert Uytterhoeven wrote:
> > On Tue, Dec 17, 2019 at 1:31 PM Fabrizio Castro wrote:
> > > On 17 December 2019 12:21, Geert Uytterhoeven wrote:
> > >> On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro wrote:
> > >>> On 13 December 2019 17:11, Laurent Pinchart wrote:
> > >>>> On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> > >>>>> Add support for transparent LVDS decoders by adding a new
> > >>>>> compatible string ("lvds-decoder") to the driver.
> > >>>>> This patch also adds member connector_type to struct lvds_codec,
> > >>>>> and that's because LVDS decoders have a different connector type
> > >>>>> from LVDS encoders. We fill this new member up with the data
> > >>>>> matching the compatible string.
> > >>>>>
> > >>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >>>>>
> > >>>>> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > >>>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > >>>>>
> > >>>>> @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > >>>>>     if (!lvds_codec)
> > >>>>>             return -ENOMEM;
> > >>>>>
> > >>>>> +   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > >>>>
> > >>>> I'm now getting a compilation failure here:
> > >>>>
> > >>>> drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’:
> > >>>> drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> > >>>>   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > >>>>                                ^
> > >>>>
> > >>>> The fix should be simple:
> > >>>>
> > >>>>       lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> > >>>>
> > >>>> I'm bothered by the fact that I've compiled this before without any
> > >>>> issue, so this really puzzles me. Do you get the same warning ?
> > >>>
> > >>> The warning appears when compiling for arm64, understandably so.
> > >>> We must have compiled this for arm only the first time around.
> > >>>
> > >>> I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long).
> > >>
> > >> Just casting to uintptr_t should be sufficient.
> > >
> > > It should be sufficient for the compiler, but I have seen examples where people
> > > preferred to be explicit, like in:
> > > drivers/mailbox/mtk-cmdq-mailbox.c
> > > drivers/leds/leds-pm8058.c
> > >
> > > Since the kernel is increasing its tightness with respect to warnings, I personally prefer
> > > (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you
> > > don't like (u32)(uintptr_t).
> >
> > It depends. I try to have as few casts as possible ("casts are evil").
> >
> > While adding the extra (u32) cast makes it clearer that the intended
> > result is a u32 (for now), it will cause silent truncation on 64-bit if
> > connector_type is ever enlarged to unsigned long, and larger values are
> > used.
> >
> > In this particular case this is unlikely, though, as unsigned long would
> > still be 32-bit on 32-bit platforms, so the larger values cannot be
> > used.
> 
> I also try to add as few casts as possible, so (uintptr_t) would be my
> preference.
> 
> Fabrizio, could you submit a new version of this patch with the problem
> fixed (and with the casts you decide to use, but using uintptr_t instead
> of unsigned long in any case) ?

Will send a new version tomorrow with (uintptr_t)

Thanks,
Fab

> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Laurent Pinchart]

On Tue, Dec 17, 2019 at 10:06:14PM +0000, Fabrizio Castro wrote:
> On 17 December 2019 13:54, Laurent Pinchart wrote:
> > On Tue, Dec 17, 2019 at 01:38:51PM +0100, Geert Uytterhoeven wrote:
> >> On Tue, Dec 17, 2019 at 1:31 PM Fabrizio Castro wrote:
> >>> On 17 December 2019 12:21, Geert Uytterhoeven wrote:
> >>>> On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro wrote:
> >>>>> On 13 December 2019 17:11, Laurent Pinchart wrote:
> >>>>>> On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> >>>>>>> Add support for transparent LVDS decoders by adding a new
> >>>>>>> compatible string ("lvds-decoder") to the driver.
> >>>>>>> This patch also adds member connector_type to struct lvds_codec,
> >>>>>>> and that's because LVDS decoders have a different connector type
> >>>>>>> from LVDS encoders. We fill this new member up with the data
> >>>>>>> matching the compatible string.
> >>>>>>>
> >>>>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >>>>>>>
> >>>>>>> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> >>>>>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> >>>>>>>
> >>>>>>> @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >>>>>>>     if (!lvds_codec)
> >>>>>>>             return -ENOMEM;
> >>>>>>>
> >>>>>>> +   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> >>>>>>
> >>>>>> I'm now getting a compilation failure here:
> >>>>>>
> >>>>>> drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’:
> >>>>>> drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> >>>>>>   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> >>>>>>                                ^
> >>>>>>
> >>>>>> The fix should be simple:
> >>>>>>
> >>>>>>       lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> >>>>>>
> >>>>>> I'm bothered by the fact that I've compiled this before without any
> >>>>>> issue, so this really puzzles me. Do you get the same warning ?
> >>>>>
> >>>>> The warning appears when compiling for arm64, understandably so.
> >>>>> We must have compiled this for arm only the first time around.
> >>>>>
> >>>>> I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long).
> >>>>
> >>>> Just casting to uintptr_t should be sufficient.
> >>>
> >>> It should be sufficient for the compiler, but I have seen examples where people
> >>> preferred to be explicit, like in:
> >>> drivers/mailbox/mtk-cmdq-mailbox.c
> >>> drivers/leds/leds-pm8058.c
> >>>
> >>> Since the kernel is increasing its tightness with respect to warnings, I personally prefer
> >>> (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you
> >>> don't like (u32)(uintptr_t).
> >>
> >> It depends. I try to have as few casts as possible ("casts are evil").
> >>
> >> While adding the extra (u32) cast makes it clearer that the intended
> >> result is a u32 (for now), it will cause silent truncation on 64-bit if
> >> connector_type is ever enlarged to unsigned long, and larger values are
> >> used.
> >>
> >> In this particular case this is unlikely, though, as unsigned long would
> >> still be 32-bit on 32-bit platforms, so the larger values cannot be
> >> used.
> > 
> > I also try to add as few casts as possible, so (uintptr_t) would be my
> > preference.
> > 
> > Fabrizio, could you submit a new version of this patch with the problem
> > fixed (and with the casts you decide to use, but using uintptr_t instead
> > of unsigned long in any case) ?
> 
> Will send a new version tomorrow with (uintptr_t)

Done as v4.1 :-)

-- 
Regards,

Laurent Pinchart

RE: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support

[Fabrizio Castro]

> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 17 December 2019 23:09
> Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support
> 
> On Tue, Dec 17, 2019 at 10:06:14PM +0000, Fabrizio Castro wrote:
> > On 17 December 2019 13:54, Laurent Pinchart wrote:
> > > On Tue, Dec 17, 2019 at 01:38:51PM +0100, Geert Uytterhoeven wrote:
> > >> On Tue, Dec 17, 2019 at 1:31 PM Fabrizio Castro wrote:
> > >>> On 17 December 2019 12:21, Geert Uytterhoeven wrote:
> > >>>> On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro wrote:
> > >>>>> On 13 December 2019 17:11, Laurent Pinchart wrote:
> > >>>>>> On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote:
> > >>>>>>> Add support for transparent LVDS decoders by adding a new
> > >>>>>>> compatible string ("lvds-decoder") to the driver.
> > >>>>>>> This patch also adds member connector_type to struct lvds_codec,
> > >>>>>>> and that's because LVDS decoders have a different connector type
> > >>>>>>> from LVDS encoders. We fill this new member up with the data
> > >>>>>>> matching the compatible string.
> > >>>>>>>
> > >>>>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >>>>>>>
> > >>>>>>> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > >>>>>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > >>>>>>>
> > >>>>>>> @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > >>>>>>>     if (!lvds_codec)
> > >>>>>>>             return -ENOMEM;
> > >>>>>>>
> > >>>>>>> +   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > >>>>>>
> > >>>>>> I'm now getting a compilation failure here:
> > >>>>>>
> > >>>>>> drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’:
> > >>>>>> drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-
> cast]
> > >>>>>>   lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > >>>>>>                                ^
> > >>>>>>
> > >>>>>> The fix should be simple:
> > >>>>>>
> > >>>>>>       lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> > >>>>>>
> > >>>>>> I'm bothered by the fact that I've compiled this before without any
> > >>>>>> issue, so this really puzzles me. Do you get the same warning ?
> > >>>>>
> > >>>>> The warning appears when compiling for arm64, understandably so.
> > >>>>> We must have compiled this for arm only the first time around.
> > >>>>>
> > >>>>> I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long).
> > >>>>
> > >>>> Just casting to uintptr_t should be sufficient.
> > >>>
> > >>> It should be sufficient for the compiler, but I have seen examples where people
> > >>> preferred to be explicit, like in:
> > >>> drivers/mailbox/mtk-cmdq-mailbox.c
> > >>> drivers/leds/leds-pm8058.c
> > >>>
> > >>> Since the kernel is increasing its tightness with respect to warnings, I personally prefer
> > >>> (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you
> > >>> don't like (u32)(uintptr_t).
> > >>
> > >> It depends. I try to have as few casts as possible ("casts are evil").
> > >>
> > >> While adding the extra (u32) cast makes it clearer that the intended
> > >> result is a u32 (for now), it will cause silent truncation on 64-bit if
> > >> connector_type is ever enlarged to unsigned long, and larger values are
> > >> used.
> > >>
> > >> In this particular case this is unlikely, though, as unsigned long would
> > >> still be 32-bit on 32-bit platforms, so the larger values cannot be
> > >> used.
> > >
> > > I also try to add as few casts as possible, so (uintptr_t) would be my
> > > preference.
> > >
> > > Fabrizio, could you submit a new version of this patch with the problem
> > > fixed (and with the casts you decide to use, but using uintptr_t instead
> > > of unsigned long in any case) ?
> >
> > Will send a new version tomorrow with (uintptr_t)
> 
> Done as v4.1 :-)

Thank you!

Fab

> 
> --
> Regards,
> 
> Laurent Pinchart

[PATCH v4 07/13] drm/bridge: lvds-codec: Simplify panel DT node localisation

[Fabrizio Castro]

The probe function needs to get ahold of the panel device tree
node, and it achieves that by using a combination of
of_graph_get_port_by_id, of_get_child_by_name, and
of_graph_get_remote_port_parent. We can achieve the same goal
by replacing those calls with a call to of_graph_get_remote_node
these days.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* New patch
---
 drivers/gpu/drm/bridge/lvds-codec.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
index c32e125..784bbd3 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -57,8 +57,6 @@ static struct drm_bridge_funcs funcs = {
 static int lvds_codec_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *port;
-	struct device_node *endpoint;
 	struct device_node *panel_node;
 	struct drm_panel *panel;
 	struct lvds_codec *lvds_codec;
@@ -79,23 +77,9 @@ static int lvds_codec_probe(struct platform_device *pdev)
 	}
 
 	/* Locate the panel DT node. */
-	port = of_graph_get_port_by_id(dev->of_node, 1);
-	if (!port) {
-		dev_dbg(dev, "port 1 not found\n");
-		return -ENXIO;
-	}
-
-	endpoint = of_get_child_by_name(port, "endpoint");
-	of_node_put(port);
-	if (!endpoint) {
-		dev_dbg(dev, "no endpoint for port 1\n");
-		return -ENXIO;
-	}
-
-	panel_node = of_graph_get_remote_port_parent(endpoint);
-	of_node_put(endpoint);
+	panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
 	if (!panel_node) {
-		dev_dbg(dev, "no remote endpoint for port 1\n");
+		dev_dbg(dev, "panel DT node not found\n");
 		return -ENXIO;
 	}
 
-- 
2.7.4

Re: [PATCH v4 07/13] drm/bridge: lvds-codec: Simplify panel DT node localisation

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:26PM +0000, Fabrizio Castro wrote:
> The probe function needs to get ahold of the panel device tree
> node, and it achieves that by using a combination of
> of_graph_get_port_by_id, of_get_child_by_name, and
> of_graph_get_remote_port_parent. We can achieve the same goal
> by replacing those calls with a call to of_graph_get_remote_node
> these days.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3->v4:
> * New patch
> ---
>  drivers/gpu/drm/bridge/lvds-codec.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> index c32e125..784bbd3 100644
> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -57,8 +57,6 @@ static struct drm_bridge_funcs funcs = {
>  static int lvds_codec_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *port;
> -	struct device_node *endpoint;
>  	struct device_node *panel_node;
>  	struct drm_panel *panel;
>  	struct lvds_codec *lvds_codec;
> @@ -79,23 +77,9 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Locate the panel DT node. */
> -	port = of_graph_get_port_by_id(dev->of_node, 1);
> -	if (!port) {
> -		dev_dbg(dev, "port 1 not found\n");
> -		return -ENXIO;
> -	}
> -
> -	endpoint = of_get_child_by_name(port, "endpoint");
> -	of_node_put(port);
> -	if (!endpoint) {
> -		dev_dbg(dev, "no endpoint for port 1\n");
> -		return -ENXIO;
> -	}
> -
> -	panel_node = of_graph_get_remote_port_parent(endpoint);
> -	of_node_put(endpoint);
> +	panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
>  	if (!panel_node) {
> -		dev_dbg(dev, "no remote endpoint for port 1\n");
> +		dev_dbg(dev, "panel DT node not found\n");
>  		return -ENXIO;
>  	}
>  

-- 
Regards,

Laurent Pinchart

[PATCH v4 08/13] dt-bindings: display: bridge: Repurpose lvds-encoder

[Fabrizio Castro]

In an effort to repurpose lvds-encoder.c to also serve the
function of LVDS decoders, we ended up defining a new "generic"
compatible string ("lvds-decoder"), therefore adapt the dt schema
to allow for the new compatible string.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* Improved title and description according to Laurent's comments
* Reworked definition of the compatible property
v2->v3:
* Extracted conversion to lvds-codec as per Rob's comment
v1->v2:
* Converted to dt-schema as per Neil's comment
---
 .../{lvds-transmitter.yaml => lvds-codec.yaml}     | 54 +++++++++++++++++-----
 1 file changed, 42 insertions(+), 12 deletions(-)
 rename Documentation/devicetree/bindings/display/bridge/{lvds-transmitter.yaml => lvds-codec.yaml} (61%)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
similarity index 61%
rename from Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
rename to Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
index 27de616..0ecc8a4 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
@@ -1,17 +1,17 @@
 # SPDX-License-Identifier: GPL-2.0
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/display/bridge/lvds-transmitter.yaml#
+$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Parallel to LVDS Encoder
+title: Transparent LVDS encoders and decoders
 
 maintainers:
   - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
 
 description: |
-  This binding supports the parallel to LVDS encoders that don't require any
-  configuration.
+  This binding supports transparent LVDS encoders and decoders that don't
+  require any configuration.
 
   LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
   incompatible data link layers have been used over time to transmit image data
@@ -33,12 +33,14 @@ properties:
     description: |
       Must list the device specific compatible string first, followed by the
       generic compatible string.
-    items:
-      - enum:
-        - ti,ds90c185       # For the TI DS90C185 FPD-Link Serializer
-        - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
-        - ti,sn75lvds83     # For the TI SN75LVDS83 FlatLink transmitter
-      - const: lvds-encoder # Generic LVDS encoder compatible fallback
+    oneOf:
+      - items:
+        - enum:
+          - ti,ds90c185       # For the TI DS90C185 FPD-Link Serializer
+          - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
+          - ti,sn75lvds83     # For the TI SN75LVDS83 FlatLink transmitter
+        - const: lvds-encoder # Generic LVDS encoder compatible fallback
+      - const: lvds-decoder   # Generic LVDS decoders compatible fallback
 
   ports:
     type: object
@@ -49,12 +51,14 @@ properties:
       port@0:
         type: object
         description: |
-          Port 0 is for parallel input
+          With LVDS encoders port 0 is for parallel input
+          With LVDS decoders port 0 is for LVDS input
 
       port@1:
         type: object
         description: |
-          Port 1 is for LVDS output
+          With LVDS encoders port 1 is for LVDS output
+          With LVDS decoders port 1 is for parallel output
 
     required:
       - port@0
@@ -96,4 +100,30 @@ examples:
       };
     };
 
+  - |
+    lvds-decoder {
+      compatible = "lvds-decoder";
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+
+          lvds_dec_in: endpoint {
+            remote-endpoint = <&display_out_lvds>;
+          };
+        };
+
+        port@1 {
+          reg = <1>;
+
+          lvds_dec_out: endpoint {
+            remote-endpoint = <&rgb_panel_in>;
+          };
+        };
+      };
+    };
+
 ...
-- 
2.7.4

Re: [PATCH v4 08/13] dt-bindings: display: bridge: Repurpose lvds-encoder

[Rob Herring]

On Wed, 13 Nov 2019 15:51:27 +0000, Fabrizio Castro wrote:
> In an effort to repurpose lvds-encoder.c to also serve the
> function of LVDS decoders, we ended up defining a new "generic"
> compatible string ("lvds-decoder"), therefore adapt the dt schema
> to allow for the new compatible string.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * Improved title and description according to Laurent's comments
> * Reworked definition of the compatible property
> v2->v3:
> * Extracted conversion to lvds-codec as per Rob's comment
> v1->v2:
> * Converted to dt-schema as per Neil's comment
> ---
>  .../{lvds-transmitter.yaml => lvds-codec.yaml}     | 54 +++++++++++++++++-----
>  1 file changed, 42 insertions(+), 12 deletions(-)
>  rename Documentation/devicetree/bindings/display/bridge/{lvds-transmitter.yaml => lvds-codec.yaml} (61%)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 08/13] dt-bindings: display: bridge: Repurpose lvds-encoder

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:27PM +0000, Fabrizio Castro wrote:
> In an effort to repurpose lvds-encoder.c to also serve the
> function of LVDS decoders, we ended up defining a new "generic"
> compatible string ("lvds-decoder"), therefore adapt the dt schema
> to allow for the new compatible string.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * Improved title and description according to Laurent's comments
> * Reworked definition of the compatible property
> v2->v3:
> * Extracted conversion to lvds-codec as per Rob's comment
> v1->v2:
> * Converted to dt-schema as per Neil's comment
> ---
>  .../{lvds-transmitter.yaml => lvds-codec.yaml}     | 54 +++++++++++++++++-----
>  1 file changed, 42 insertions(+), 12 deletions(-)
>  rename Documentation/devicetree/bindings/display/bridge/{lvds-transmitter.yaml => lvds-codec.yaml} (61%)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> similarity index 61%
> rename from Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> rename to Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> index 27de616..0ecc8a4 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -1,17 +1,17 @@
>  # SPDX-License-Identifier: GPL-2.0
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/display/bridge/lvds-transmitter.yaml#
> +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Parallel to LVDS Encoder
> +title: Transparent LVDS encoders and decoders
>  
>  maintainers:
>    - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>  
>  description: |
> -  This binding supports the parallel to LVDS encoders that don't require any
> -  configuration.
> +  This binding supports transparent LVDS encoders and decoders that don't
> +  require any configuration.
>  
>    LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
>    incompatible data link layers have been used over time to transmit image data
> @@ -33,12 +33,14 @@ properties:
>      description: |
>        Must list the device specific compatible string first, followed by the
>        generic compatible string.
> -    items:
> -      - enum:
> -        - ti,ds90c185       # For the TI DS90C185 FPD-Link Serializer
> -        - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
> -        - ti,sn75lvds83     # For the TI SN75LVDS83 FlatLink transmitter
> -      - const: lvds-encoder # Generic LVDS encoder compatible fallback
> +    oneOf:
> +      - items:
> +        - enum:
> +          - ti,ds90c185       # For the TI DS90C185 FPD-Link Serializer
> +          - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
> +          - ti,sn75lvds83     # For the TI SN75LVDS83 FlatLink transmitter
> +        - const: lvds-encoder # Generic LVDS encoder compatible fallback
> +      - const: lvds-decoder   # Generic LVDS decoders compatible fallback
>  
>    ports:
>      type: object
> @@ -49,12 +51,14 @@ properties:
>        port@0:
>          type: object
>          description: |
> -          Port 0 is for parallel input
> +          With LVDS encoders port 0 is for parallel input
> +          With LVDS decoders port 0 is for LVDS input

How about

          For LVDS encoders, port 0 is the parallel input
          For LVDS decoders, port 0 is the LVDS input

>  
>        port@1:
>          type: object
>          description: |
> -          Port 1 is for LVDS output
> +          With LVDS encoders port 1 is for LVDS output
> +          With LVDS decoders port 1 is for parallel output

And similarly here ?

If you're fine with this change there's no need to resubmit, I'll change
this when applying, and

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>      required:
>        - port@0
> @@ -96,4 +100,30 @@ examples:
>        };
>      };
>  
> +  - |
> +    lvds-decoder {
> +      compatible = "lvds-decoder";
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +
> +          lvds_dec_in: endpoint {
> +            remote-endpoint = <&display_out_lvds>;
> +          };
> +        };
> +
> +        port@1 {
> +          reg = <1>;
> +
> +          lvds_dec_out: endpoint {
> +            remote-endpoint = <&rgb_panel_in>;
> +          };
> +        };
> +      };
> +    };
> +
>  ...

-- 
Regards,

Laurent Pinchart

RE: [PATCH v4 08/13] dt-bindings: display: bridge: Repurpose lvds-encoder

[Fabrizio Castro]

Hi Laurent,

Thank you for your feedback!

> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 19 November 2019 00:08
> Subject: Re: [PATCH v4 08/13] dt-bindings: display: bridge: Repurpose lvds-encoder
> 
> Hi Fabrizio,
> 
> Thank you for the patch.
> 
> On Wed, Nov 13, 2019 at 03:51:27PM +0000, Fabrizio Castro wrote:
> > In an effort to repurpose lvds-encoder.c to also serve the
> > function of LVDS decoders, we ended up defining a new "generic"
> > compatible string ("lvds-decoder"), therefore adapt the dt schema
> > to allow for the new compatible string.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v3->v4:
> > * Improved title and description according to Laurent's comments
> > * Reworked definition of the compatible property
> > v2->v3:
> > * Extracted conversion to lvds-codec as per Rob's comment
> > v1->v2:
> > * Converted to dt-schema as per Neil's comment
> > ---
> >  .../{lvds-transmitter.yaml => lvds-codec.yaml}     | 54 +++++++++++++++++-----
> >  1 file changed, 42 insertions(+), 12 deletions(-)
> >  rename Documentation/devicetree/bindings/display/bridge/{lvds-transmitter.yaml => lvds-codec.yaml} (61%)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > similarity index 61%
> > rename from Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> > rename to Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > index 27de616..0ecc8a4 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > @@ -1,17 +1,17 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  %YAML 1.2
> >  ---
> > -$id: http://devicetree.org/schemas/display/bridge/lvds-transmitter.yaml#
> > +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> >  $schema: http://devicetree.org/meta-schemas/core.yaml#
> >
> > -title: Parallel to LVDS Encoder
> > +title: Transparent LVDS encoders and decoders
> >
> >  maintainers:
> >    - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> >  description: |
> > -  This binding supports the parallel to LVDS encoders that don't require any
> > -  configuration.
> > +  This binding supports transparent LVDS encoders and decoders that don't
> > +  require any configuration.
> >
> >    LVDS is a physical layer specification defined in ANSI/TIA/EIA-644-A. Multiple
> >    incompatible data link layers have been used over time to transmit image data
> > @@ -33,12 +33,14 @@ properties:
> >      description: |
> >        Must list the device specific compatible string first, followed by the
> >        generic compatible string.
> > -    items:
> > -      - enum:
> > -        - ti,ds90c185       # For the TI DS90C185 FPD-Link Serializer
> > -        - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
> > -        - ti,sn75lvds83     # For the TI SN75LVDS83 FlatLink transmitter
> > -      - const: lvds-encoder # Generic LVDS encoder compatible fallback
> > +    oneOf:
> > +      - items:
> > +        - enum:
> > +          - ti,ds90c185       # For the TI DS90C185 FPD-Link Serializer
> > +          - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
> > +          - ti,sn75lvds83     # For the TI SN75LVDS83 FlatLink transmitter
> > +        - const: lvds-encoder # Generic LVDS encoder compatible fallback
> > +      - const: lvds-decoder   # Generic LVDS decoders compatible fallback
> >
> >    ports:
> >      type: object
> > @@ -49,12 +51,14 @@ properties:
> >        port@0:
> >          type: object
> >          description: |
> > -          Port 0 is for parallel input
> > +          With LVDS encoders port 0 is for parallel input
> > +          With LVDS decoders port 0 is for LVDS input
> 
> How about
> 
>           For LVDS encoders, port 0 is the parallel input
>           For LVDS decoders, port 0 is the LVDS input

It's ok with me

> 
> >
> >        port@1:
> >          type: object
> >          description: |
> > -          Port 1 is for LVDS output
> > +          With LVDS encoders port 1 is for LVDS output
> > +          With LVDS decoders port 1 is for parallel output
> 
> And similarly here ?

Here too

> 
> If you're fine with this change there's no need to resubmit, I'll change
> this when applying, and

Yes please.

Thanks,
Fab

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >
> >      required:
> >        - port@0
> > @@ -96,4 +100,30 @@ examples:
> >        };
> >      };
> >
> > +  - |
> > +    lvds-decoder {
> > +      compatible = "lvds-decoder";
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 {
> > +          reg = <0>;
> > +
> > +          lvds_dec_in: endpoint {
> > +            remote-endpoint = <&display_out_lvds>;
> > +          };
> > +        };
> > +
> > +        port@1 {
> > +          reg = <1>;
> > +
> > +          lvds_dec_out: endpoint {
> > +            remote-endpoint = <&rgb_panel_in>;
> > +          };
> > +        };
> > +      };
> > +    };
> > +
> >  ...
> 
> --
> Regards,
> 
> Laurent Pinchart

[PATCH v4 09/13] dt-bindings: display: bridge: lvds-codec: Document ti,ds90cf384a

[Fabrizio Castro]

The DS90CF384A from TI is a transparent LVDS receiver (decoder),
and therefore it is compatible with the lvds-codec driver and
bindings.

Document the ti,ds90cf384a compatible string with the dt-bindings.
No driver change required.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* New patch
---
 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
index 0ecc8a4..21f8c6e 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
@@ -40,7 +40,10 @@ properties:
           - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
           - ti,sn75lvds83     # For the TI SN75LVDS83 FlatLink transmitter
         - const: lvds-encoder # Generic LVDS encoder compatible fallback
-      - const: lvds-decoder   # Generic LVDS decoders compatible fallback
+      - items:
+        - enum:
+          - ti,ds90cf384a     # For the DS90CF384A FPD-Link LVDS Receiver
+        - const: lvds-decoder # Generic LVDS decoders compatible fallback
 
   ports:
     type: object
@@ -102,7 +105,7 @@ examples:
 
   - |
     lvds-decoder {
-      compatible = "lvds-decoder";
+      compatible = "ti,ds90cf384a", "lvds-decoder";
 
       ports {
         #address-cells = <1>;
-- 
2.7.4

Re: [PATCH v4 09/13] dt-bindings: display: bridge: lvds-codec: Document ti,ds90cf384a

[Rob Herring]

On Wed, 13 Nov 2019 15:51:28 +0000, Fabrizio Castro wrote:
> The DS90CF384A from TI is a transparent LVDS receiver (decoder),
> and therefore it is compatible with the lvds-codec driver and
> bindings.
> 
> Document the ti,ds90cf384a compatible string with the dt-bindings.
> No driver change required.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v3->v4:
> * New patch
> ---
>  Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 09/13] dt-bindings: display: bridge: lvds-codec: Document ti,ds90cf384a

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:28PM +0000, Fabrizio Castro wrote:
> The DS90CF384A from TI is a transparent LVDS receiver (decoder),
> and therefore it is compatible with the lvds-codec driver and
> bindings.
> 
> Document the ti,ds90cf384a compatible string with the dt-bindings.
> No driver change required.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3->v4:
> * New patch
> ---
>  Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> index 0ecc8a4..21f8c6e 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -40,7 +40,10 @@ properties:
>            - ti,ds90c187       # For the TI DS90C187 FPD-Link Serializer
>            - ti,sn75lvds83     # For the TI SN75LVDS83 FlatLink transmitter
>          - const: lvds-encoder # Generic LVDS encoder compatible fallback
> -      - const: lvds-decoder   # Generic LVDS decoders compatible fallback
> +      - items:
> +        - enum:
> +          - ti,ds90cf384a     # For the DS90CF384A FPD-Link LVDS Receiver
> +        - const: lvds-decoder # Generic LVDS decoders compatible fallback
>  
>    ports:
>      type: object
> @@ -102,7 +105,7 @@ examples:
>  
>    - |
>      lvds-decoder {
> -      compatible = "lvds-decoder";
> +      compatible = "ti,ds90cf384a", "lvds-decoder";
>  
>        ports {
>          #address-cells = <1>;

-- 
Regards,

Laurent Pinchart

[PATCH v4 10/13] ARM: dts: iwg20d-q7-common: Add LCD support

[Fabrizio Castro]

The iwg20d comes with a 7" capacitive touch screen, therefore
add support for it.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* Reworked compatibility of DT node lvds-receiver
* s/powerdown/powerdown-gpios/g in lvds-receiver
* Reworked compatibility of DT node panel
v2->v3:
* No change
v1->v2:
* No change
---
 arch/arm/boot/dts/iwg20d-q7-common.dtsi  | 85 ++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi |  1 -
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/iwg20d-q7-common.dtsi b/arch/arm/boot/dts/iwg20d-q7-common.dtsi
index ae75a1db..ab5ae93 100644
--- a/arch/arm/boot/dts/iwg20d-q7-common.dtsi
+++ b/arch/arm/boot/dts/iwg20d-q7-common.dtsi
@@ -46,6 +46,49 @@
 		clock-frequency = <26000000>;
 	};
 
+	lcd_backlight: backlight {
+		compatible = "pwm-backlight";
+
+		pwms = <&pwm3 0 5000000 0>;
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <7>;
+		enable-gpios = <&gpio5 14 GPIO_ACTIVE_HIGH>;
+	};
+
+	lvds-receiver {
+		compatible = "ti,ds90cf384a", "lvds-decoder";
+		powerdown-gpios = <&gpio7 25 GPIO_ACTIVE_LOW>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				lvds_receiver_in: endpoint {
+					remote-endpoint = <&lvds0_out>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				lvds_receiver_out: endpoint {
+					remote-endpoint = <&panel_in>;
+				};
+			};
+		};
+	};
+
+	panel {
+		compatible = "edt,etm0700g0dh6";
+		backlight = <&lcd_backlight>;
+
+		port {
+			panel_in: endpoint {
+				remote-endpoint = <&lvds_receiver_out>;
+			};
+		};
+	};
+
 	reg_1p5v: 1p5v {
 		compatible = "regulator-fixed";
 		regulator-name = "1P5V";
@@ -120,6 +163,18 @@
 	status = "okay";
 };
 
+&du {
+	status = "okay";
+};
+
+&gpio2 {
+	touch-interrupt {
+		gpio-hog;
+		gpios = <12 GPIO_ACTIVE_LOW>;
+		input;
+	};
+};
+
 &hsusb {
 	status = "okay";
 	pinctrl-0 = <&usb0_pins>;
@@ -147,6 +202,25 @@
 		VDDIO-supply = <&reg_3p3v>;
 		VDDD-supply = <&reg_1p5v>;
 	};
+
+	touch: touchpanel@38 {
+		compatible = "edt,edt-ft5406";
+		reg = <0x38>;
+		interrupt-parent = <&gpio2>;
+		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
+	};
+};
+
+&lvds0 {
+	status = "okay";
+
+	ports {
+		port@1 {
+			lvds0_out: endpoint {
+				remote-endpoint = <&lvds_receiver_in>;
+			};
+		};
+	};
 };
 
 &pci0 {
@@ -180,6 +254,11 @@
 		function = "i2c2";
 	};
 
+	pwm3_pins: pwm3 {
+		groups = "pwm3";
+		function = "pwm3";
+	};
+
 	scif0_pins: scif0 {
 		groups = "scif0_data_d";
 		function = "scif0";
@@ -218,6 +297,12 @@
 	};
 };
 
+&pwm3 {
+	pinctrl-0 = <&pwm3_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
 &rcar_sound {
 	pinctrl-0 = <&sound_pins>;
 	pinctrl-names = "default";
diff --git a/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi b/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
index 0e99df2..ede2e0c 100644
--- a/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
+++ b/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
@@ -39,7 +39,6 @@
 &du {
 	pinctrl-0 = <&du_pins>;
 	pinctrl-names = "default";
-	status = "okay";
 
 	ports {
 		port@0 {
-- 
2.7.4

Re: [PATCH v4 10/13] ARM: dts: iwg20d-q7-common: Add LCD support

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:29PM +0000, Fabrizio Castro wrote:
> The iwg20d comes with a 7" capacitive touch screen, therefore
> add support for it.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I expect Geert to pick this up.

> ---
> v3->v4:
> * Reworked compatibility of DT node lvds-receiver
> * s/powerdown/powerdown-gpios/g in lvds-receiver
> * Reworked compatibility of DT node panel
> v2->v3:
> * No change
> v1->v2:
> * No change
> ---
>  arch/arm/boot/dts/iwg20d-q7-common.dtsi  | 85 ++++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi |  1 -
>  2 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/iwg20d-q7-common.dtsi b/arch/arm/boot/dts/iwg20d-q7-common.dtsi
> index ae75a1db..ab5ae93 100644
> --- a/arch/arm/boot/dts/iwg20d-q7-common.dtsi
> +++ b/arch/arm/boot/dts/iwg20d-q7-common.dtsi
> @@ -46,6 +46,49 @@
>  		clock-frequency = <26000000>;
>  	};
>  
> +	lcd_backlight: backlight {
> +		compatible = "pwm-backlight";
> +
> +		pwms = <&pwm3 0 5000000 0>;
> +		brightness-levels = <0 4 8 16 32 64 128 255>;
> +		default-brightness-level = <7>;
> +		enable-gpios = <&gpio5 14 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	lvds-receiver {
> +		compatible = "ti,ds90cf384a", "lvds-decoder";
> +		powerdown-gpios = <&gpio7 25 GPIO_ACTIVE_LOW>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				lvds_receiver_in: endpoint {
> +					remote-endpoint = <&lvds0_out>;
> +				};
> +			};
> +			port@1 {
> +				reg = <1>;
> +				lvds_receiver_out: endpoint {
> +					remote-endpoint = <&panel_in>;
> +				};
> +			};
> +		};
> +	};
> +
> +	panel {
> +		compatible = "edt,etm0700g0dh6";
> +		backlight = <&lcd_backlight>;
> +
> +		port {
> +			panel_in: endpoint {
> +				remote-endpoint = <&lvds_receiver_out>;
> +			};
> +		};
> +	};
> +
>  	reg_1p5v: 1p5v {
>  		compatible = "regulator-fixed";
>  		regulator-name = "1P5V";
> @@ -120,6 +163,18 @@
>  	status = "okay";
>  };
>  
> +&du {
> +	status = "okay";
> +};
> +
> +&gpio2 {
> +	touch-interrupt {
> +		gpio-hog;
> +		gpios = <12 GPIO_ACTIVE_LOW>;
> +		input;
> +	};
> +};
> +
>  &hsusb {
>  	status = "okay";
>  	pinctrl-0 = <&usb0_pins>;
> @@ -147,6 +202,25 @@
>  		VDDIO-supply = <&reg_3p3v>;
>  		VDDD-supply = <&reg_1p5v>;
>  	};
> +
> +	touch: touchpanel@38 {
> +		compatible = "edt,edt-ft5406";
> +		reg = <0x38>;
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
> +	};
> +};
> +
> +&lvds0 {
> +	status = "okay";
> +
> +	ports {
> +		port@1 {
> +			lvds0_out: endpoint {
> +				remote-endpoint = <&lvds_receiver_in>;
> +			};
> +		};
> +	};
>  };
>  
>  &pci0 {
> @@ -180,6 +254,11 @@
>  		function = "i2c2";
>  	};
>  
> +	pwm3_pins: pwm3 {
> +		groups = "pwm3";
> +		function = "pwm3";
> +	};
> +
>  	scif0_pins: scif0 {
>  		groups = "scif0_data_d";
>  		function = "scif0";
> @@ -218,6 +297,12 @@
>  	};
>  };
>  
> +&pwm3 {
> +	pinctrl-0 = <&pwm3_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};
> +
>  &rcar_sound {
>  	pinctrl-0 = <&sound_pins>;
>  	pinctrl-names = "default";
> diff --git a/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi b/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
> index 0e99df2..ede2e0c 100644
> --- a/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
> +++ b/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi
> @@ -39,7 +39,6 @@
>  &du {
>  	pinctrl-0 = <&du_pins>;
>  	pinctrl-names = "default";
> -	status = "okay";
>  
>  	ports {
>  		port@0 {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 10/13] ARM: dts: iwg20d-q7-common: Add LCD support

[Geert Uytterhoeven]

On Tue, Nov 19, 2019 at 1:10 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Nov 13, 2019 at 03:51:29PM +0000, Fabrizio Castro wrote:
> > The iwg20d comes with a 7" capacitive touch screen, therefore
> > add support for it.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I expect Geert to pick this up.

Thanks, queued in renesas-devel for v5.6.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v4 11/13] ARM: shmobile_defconfig: Enable support for panels from EDT

[Fabrizio Castro]

The iwg20d comes with an LCD panel from Emerging Display
Technologies Corporation (EDT), therefore enable what's
required to support it.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v3->v4:
* No change
v2->v3:
* No change
v1->v2:
* No change
---
 arch/arm/configs/shmobile_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig
index c6c7035..ab416a5 100644
--- a/arch/arm/configs/shmobile_defconfig
+++ b/arch/arm/configs/shmobile_defconfig
@@ -66,6 +66,7 @@ CONFIG_INPUT_EVDEV=y
 CONFIG_KEYBOARD_GPIO=y
 # CONFIG_INPUT_MOUSE is not set
 CONFIG_INPUT_TOUCHSCREEN=y
+CONFIG_TOUCHSCREEN_EDT_FT5X06=y
 CONFIG_TOUCHSCREEN_ST1232=y
 CONFIG_INPUT_MISC=y
 CONFIG_INPUT_ADXL34X=y
@@ -125,7 +126,9 @@ CONFIG_VIDEO_ADV7604=y
 CONFIG_VIDEO_ML86V7667=y
 CONFIG_DRM=y
 CONFIG_DRM_RCAR_DU=y
+CONFIG_DRM_PANEL_SIMPLE=y
 CONFIG_DRM_DUMB_VGA_DAC=y
+CONFIG_DRM_LVDS_CODEC=y
 CONFIG_DRM_SII902X=y
 CONFIG_DRM_I2C_ADV7511=y
 CONFIG_DRM_I2C_ADV7511_AUDIO=y
-- 
2.7.4

Re: [PATCH v4 11/13] ARM: shmobile_defconfig: Enable support for panels from EDT

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:30PM +0000, Fabrizio Castro wrote:
> The iwg20d comes with an LCD panel from Emerging Display
> Technologies Corporation (EDT), therefore enable what's
> required to support it.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I expect Geert to pick this up.

> ---
> v3->v4:
> * No change
> v2->v3:
> * No change
> v1->v2:
> * No change
> ---
>  arch/arm/configs/shmobile_defconfig | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig
> index c6c7035..ab416a5 100644
> --- a/arch/arm/configs/shmobile_defconfig
> +++ b/arch/arm/configs/shmobile_defconfig
> @@ -66,6 +66,7 @@ CONFIG_INPUT_EVDEV=y
>  CONFIG_KEYBOARD_GPIO=y
>  # CONFIG_INPUT_MOUSE is not set
>  CONFIG_INPUT_TOUCHSCREEN=y
> +CONFIG_TOUCHSCREEN_EDT_FT5X06=y
>  CONFIG_TOUCHSCREEN_ST1232=y
>  CONFIG_INPUT_MISC=y
>  CONFIG_INPUT_ADXL34X=y
> @@ -125,7 +126,9 @@ CONFIG_VIDEO_ADV7604=y
>  CONFIG_VIDEO_ML86V7667=y
>  CONFIG_DRM=y
>  CONFIG_DRM_RCAR_DU=y
> +CONFIG_DRM_PANEL_SIMPLE=y
>  CONFIG_DRM_DUMB_VGA_DAC=y
> +CONFIG_DRM_LVDS_CODEC=y
>  CONFIG_DRM_SII902X=y
>  CONFIG_DRM_I2C_ADV7511=y
>  CONFIG_DRM_I2C_ADV7511_AUDIO=y

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 11/13] ARM: shmobile_defconfig: Enable support for panels from EDT

[Geert Uytterhoeven]

On Tue, Nov 19, 2019 at 1:12 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Nov 13, 2019 at 03:51:30PM +0000, Fabrizio Castro wrote:
> > The iwg20d comes with an LCD panel from Emerging Display
> > Technologies Corporation (EDT), therefore enable what's
> > required to support it.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I expect Geert to pick this up.

Thanks, queued in renesas-devel for v5.6.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings

[Fabrizio Castro]

The lvds-codec driver is a generic stub for transparent LVDS
encoders and decoders.
It's good practice to list a device specific compatible string
before the generic fallback (if any) in the DT node for the relevant
LVDS encoder/decoder, and it's also required by the dt-bindings.
A notable exception to the generic fallback mechanism is the case
of "thine,thc63lvdm83d", as documented in:
Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
This patch enforces the adoption of a device specific compatible
string (as fist string in the list), by using markers for the
compatible string we match against and the index of the matching
compatible string in the list.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
Hi Laurent,

I don't think we need to do anything in the driver to address your
comment, as we can "enforce" this with the bindings (please see the
next patch, as it would help with the "enforcing" of the compatible
string for the thine device).
I am sending this patch only so that you can see what a possible
solution in the driver could look like.

v3->v4:
* New patch addressing the below comment from Laurent:
"I think the lvds-decoder driver should error out at probe time if only
one compatible string is listed."
---
 drivers/gpu/drm/bridge/lvds-codec.c | 55 +++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
index 784bbd3..145c25d 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -14,11 +14,16 @@
 #include <drm/drm_bridge.h>
 #include <drm/drm_panel.h>
 
+struct lvds_codec_data {
+	u32 connector_type;
+	bool device_specific;
+};
+
 struct lvds_codec {
 	struct drm_bridge bridge;
 	struct drm_bridge *panel_bridge;
 	struct gpio_desc *powerdown_gpio;
-	u32 connector_type;
+	const struct lvds_codec_data *data;
 };
 
 static int lvds_codec_attach(struct drm_bridge *bridge)
@@ -65,7 +70,30 @@ static int lvds_codec_probe(struct platform_device *pdev)
 	if (!lvds_codec)
 		return -ENOMEM;
 
-	lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
+	lvds_codec->data = of_device_get_match_data(&pdev->dev);
+	if (!lvds_codec->data)
+		return -EINVAL;
+
+	/*
+	 * If we haven't matched a device specific compatible string, we need
+	 * to work out if the generic compatible string we matched against was
+	 * listed first in the compatible property.
+	 */
+	if (!lvds_codec->data->device_specific) {
+		const struct of_device_id *match;
+		int compatible_index;
+
+		match = of_match_node(dev->driver->of_match_table,
+				      dev->of_node);
+		compatible_index = of_property_match_string(dev->of_node,
+							    "compatible",
+							    match->compatible);
+		if (compatible_index == 0) {
+			dev_err(dev, "Device specific compatible needed\n");
+			return -EINVAL;
+		}
+	}
+
 	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
 							     GPIOD_OUT_HIGH);
 	if (IS_ERR(lvds_codec->powerdown_gpio)) {
@@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
 
 	lvds_codec->panel_bridge =
 		devm_drm_panel_bridge_add_typed(dev, panel,
-						lvds_codec->connector_type);
+					lvds_codec->data->connector_type);
 	if (IS_ERR(lvds_codec->panel_bridge))
 		return PTR_ERR(lvds_codec->panel_bridge);
 
@@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct lvds_codec_data lvds_codec_decoder_data = {
+	.connector_type = DRM_MODE_CONNECTOR_DPI,
+	.device_specific = false,
+};
+
+static const struct lvds_codec_data lvds_codec_encoder_data = {
+	.connector_type = DRM_MODE_CONNECTOR_LVDS,
+	.device_specific = false,
+};
+
+static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = {
+	.connector_type = DRM_MODE_CONNECTOR_LVDS,
+	.device_specific = true,
+};
+
 static const struct of_device_id lvds_codec_match[] = {
 	{
 		.compatible = "lvds-decoder",
-		.data = (void *)DRM_MODE_CONNECTOR_DPI,
+		.data = &lvds_codec_decoder_data,
 	},
 	{
 		.compatible = "lvds-encoder",
-		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
+		.data = &lvds_codec_encoder_data,
 	},
 	{
 		.compatible = "thine,thc63lvdm83d",
-		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
+		.data = &lvds_codec_thc63lvdm83d_data,
 	},
 	{},
 };
-- 
2.7.4

Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote:
> The lvds-codec driver is a generic stub for transparent LVDS
> encoders and decoders.
> It's good practice to list a device specific compatible string

s/good practice/mandatory/

> before the generic fallback (if any) in the DT node for the relevant
> LVDS encoder/decoder, and it's also required by the dt-bindings.
> A notable exception to the generic fallback mechanism is the case
> of "thine,thc63lvdm83d", as documented in:
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> This patch enforces the adoption of a device specific compatible
> string (as fist string in the list), by using markers for the

s/fist/first/

> compatible string we match against and the index of the matching
> compatible string in the list.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> Hi Laurent,
> 
> I don't think we need to do anything in the driver to address your
> comment, as we can "enforce" this with the bindings (please see the
> next patch, as it would help with the "enforcing" of the compatible
> string for the thine device).
> I am sending this patch only so that you can see what a possible
> solution in the driver could look like.
> 
> v3->v4:
> * New patch addressing the below comment from Laurent:
> "I think the lvds-decoder driver should error out at probe time if only
> one compatible string is listed."
> ---
>  drivers/gpu/drm/bridge/lvds-codec.c | 55 +++++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> index 784bbd3..145c25d 100644
> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -14,11 +14,16 @@
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_panel.h>
>  
> +struct lvds_codec_data {
> +	u32 connector_type;
> +	bool device_specific;
> +};
> +
>  struct lvds_codec {
>  	struct drm_bridge bridge;
>  	struct drm_bridge *panel_bridge;
>  	struct gpio_desc *powerdown_gpio;
> -	u32 connector_type;
> +	const struct lvds_codec_data *data;
>  };
>  
>  static int lvds_codec_attach(struct drm_bridge *bridge)
> @@ -65,7 +70,30 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  	if (!lvds_codec)
>  		return -ENOMEM;
>  
> -	lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> +	lvds_codec->data = of_device_get_match_data(&pdev->dev);
> +	if (!lvds_codec->data)
> +		return -EINVAL;
> +
> +	/*
> +	 * If we haven't matched a device specific compatible string, we need
> +	 * to work out if the generic compatible string we matched against was
> +	 * listed first in the compatible property.
> +	 */

Can't we do this unconditionally, and thus drop the lvds_codec_data
structure ?

> +	if (!lvds_codec->data->device_specific) {
> +		const struct of_device_id *match;
> +		int compatible_index;
> +
> +		match = of_match_node(dev->driver->of_match_table,
> +				      dev->of_node);
> +		compatible_index = of_property_match_string(dev->of_node,
> +							    "compatible",
> +							    match->compatible);
> +		if (compatible_index == 0) {
> +			dev_err(dev, "Device specific compatible needed\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>  							     GPIOD_OUT_HIGH);
>  	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  
>  	lvds_codec->panel_bridge =
>  		devm_drm_panel_bridge_add_typed(dev, panel,
> -						lvds_codec->connector_type);
> +					lvds_codec->data->connector_type);
>  	if (IS_ERR(lvds_codec->panel_bridge))
>  		return PTR_ERR(lvds_codec->panel_bridge);
>  
> @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct lvds_codec_data lvds_codec_decoder_data = {
> +	.connector_type = DRM_MODE_CONNECTOR_DPI,
> +	.device_specific = false,
> +};
> +
> +static const struct lvds_codec_data lvds_codec_encoder_data = {
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> +	.device_specific = false,
> +};
> +
> +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = {
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> +	.device_specific = true,
> +};
> +
>  static const struct of_device_id lvds_codec_match[] = {
>  	{
>  		.compatible = "lvds-decoder",
> -		.data = (void *)DRM_MODE_CONNECTOR_DPI,
> +		.data = &lvds_codec_decoder_data,
>  	},
>  	{
>  		.compatible = "lvds-encoder",
> -		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> +		.data = &lvds_codec_encoder_data,
>  	},
>  	{
>  		.compatible = "thine,thc63lvdm83d",
> -		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> +		.data = &lvds_codec_thc63lvdm83d_data,
>  	},
>  	{},
>  };

-- 
Regards,

Laurent Pinchart

RE: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings

[Fabrizio Castro]

Hi Laurent,

Thank you for your feedback!

> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 19 November 2019 00:16
> Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings
> 
> Hi Fabrizio,
> 
> Thank you for the patch.
> 
> On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote:
> > The lvds-codec driver is a generic stub for transparent LVDS
> > encoders and decoders.
> > It's good practice to list a device specific compatible string
> 
> s/good practice/mandatory/

Will fix

> 
> > before the generic fallback (if any) in the DT node for the relevant
> > LVDS encoder/decoder, and it's also required by the dt-bindings.
> > A notable exception to the generic fallback mechanism is the case
> > of "thine,thc63lvdm83d", as documented in:
> > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> > This patch enforces the adoption of a device specific compatible
> > string (as fist string in the list), by using markers for the
> 
> s/fist/first/

Well spotted

> 
> > compatible string we match against and the index of the matching
> > compatible string in the list.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > Hi Laurent,
> >
> > I don't think we need to do anything in the driver to address your
> > comment, as we can "enforce" this with the bindings (please see the
> > next patch, as it would help with the "enforcing" of the compatible
> > string for the thine device).
> > I am sending this patch only so that you can see what a possible
> > solution in the driver could look like.
> >
> > v3->v4:
> > * New patch addressing the below comment from Laurent:
> > "I think the lvds-decoder driver should error out at probe time if only
> > one compatible string is listed."
> > ---
> >  drivers/gpu/drm/bridge/lvds-codec.c | 55 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 49 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > index 784bbd3..145c25d 100644
> > --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > @@ -14,11 +14,16 @@
> >  #include <drm/drm_bridge.h>
> >  #include <drm/drm_panel.h>
> >
> > +struct lvds_codec_data {
> > +	u32 connector_type;
> > +	bool device_specific;
> > +};
> > +
> >  struct lvds_codec {
> >  	struct drm_bridge bridge;
> >  	struct drm_bridge *panel_bridge;
> >  	struct gpio_desc *powerdown_gpio;
> > -	u32 connector_type;
> > +	const struct lvds_codec_data *data;
> >  };
> >
> >  static int lvds_codec_attach(struct drm_bridge *bridge)
> > @@ -65,7 +70,30 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >  	if (!lvds_codec)
> >  		return -ENOMEM;
> >
> > -	lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > +	lvds_codec->data = of_device_get_match_data(&pdev->dev);
> > +	if (!lvds_codec->data)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * If we haven't matched a device specific compatible string, we need
> > +	 * to work out if the generic compatible string we matched against was
> > +	 * listed first in the compatible property.
> > +	 */
> 
> Can't we do this unconditionally, and thus drop the lvds_codec_data
> structure ?

I don't think so, and the reason for this is that we have a corner case for
thine,thc63lvdm83d. Here is what's allowed (according to the documentation)
from what's supported upstream (+ this series):
"ti,ds90c185", "lvds-encoder"
"ti,ds90c187", "lvds-encoder"
"ti,sn75lvds83", "lvds-encoder"
"ti,ds90cf384a", "lvds-decoder"
"thine,thc63lvdm83d"

As you can see from the examples above, in most cases it's enough to say it's
all good when we match a compatible string with index > 0, but for the thine
device you _have_ to match the string with index 0 as that's what's currently
documented (please see thine,thc63lvdm83d.txt) and that's what's supported
by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts).

This patch "classifies" compatible strings, and it considers a good match
device specific compatible strings, or generic compatible strings as long
as they are not listed first.

These days you can leverage the yaml files to validate the device trees,
therefore we should be focusing on writing yaml files in such a way we only
pass the checks we mean to, and by checks I mean:
make dtbs_check

or more specifically, for this series:
make dtbs_check  DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml

and that's of course on top of make dt_binding_check.

It's a very common requirement to have a part number specific compatible
string first followed by a generic (fallback) compatible string in the device trees,
most drivers for Renesas SoCs have similar requirements.

If we start doing this here, we'll end up doing it elsewhere as well, and I really
think we shouldn't, but others may see things differently, so I'll wait for others
(and yourself with further comments) to jump in before doing any more work
on this patch.

Thanks,
Fab

> 
> > +	if (!lvds_codec->data->device_specific) {
> > +		const struct of_device_id *match;
> > +		int compatible_index;
> > +
> > +		match = of_match_node(dev->driver->of_match_table,
> > +				      dev->of_node);
> > +		compatible_index = of_property_match_string(dev->of_node,
> > +							    "compatible",
> > +							    match->compatible);
> > +		if (compatible_index == 0) {
> > +			dev_err(dev, "Device specific compatible needed\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >  	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> >  							     GPIOD_OUT_HIGH);
> >  	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >
> >  	lvds_codec->panel_bridge =
> >  		devm_drm_panel_bridge_add_typed(dev, panel,
> > -						lvds_codec->connector_type);
> > +					lvds_codec->data->connector_type);
> >  	if (IS_ERR(lvds_codec->panel_bridge))
> >  		return PTR_ERR(lvds_codec->panel_bridge);
> >
> > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >
> > +static const struct lvds_codec_data lvds_codec_decoder_data = {
> > +	.connector_type = DRM_MODE_CONNECTOR_DPI,
> > +	.device_specific = false,
> > +};
> > +
> > +static const struct lvds_codec_data lvds_codec_encoder_data = {
> > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> > +	.device_specific = false,
> > +};
> > +
> > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = {
> > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> > +	.device_specific = true,
> > +};
> > +
> >  static const struct of_device_id lvds_codec_match[] = {
> >  	{
> >  		.compatible = "lvds-decoder",
> > -		.data = (void *)DRM_MODE_CONNECTOR_DPI,
> > +		.data = &lvds_codec_decoder_data,
> >  	},
> >  	{
> >  		.compatible = "lvds-encoder",
> > -		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > +		.data = &lvds_codec_encoder_data,
> >  	},
> >  	{
> >  		.compatible = "thine,thc63lvdm83d",
> > -		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > +		.data = &lvds_codec_thc63lvdm83d_data,
> >  	},
> >  	{},
> >  };
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings

[Laurent Pinchart]

Hi Fabrizio,

On Tue, Nov 19, 2019 at 11:17:34AM +0000, Fabrizio Castro wrote:
> On 19 November 2019 00:16 Laurent Pinchart wrote:
> > On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote:
> > > The lvds-codec driver is a generic stub for transparent LVDS
> > > encoders and decoders.
> > > It's good practice to list a device specific compatible string
> > 
> > s/good practice/mandatory/
> 
> Will fix
> 
> > > before the generic fallback (if any) in the DT node for the relevant
> > > LVDS encoder/decoder, and it's also required by the dt-bindings.
> > > A notable exception to the generic fallback mechanism is the case
> > > of "thine,thc63lvdm83d", as documented in:
> > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> > > This patch enforces the adoption of a device specific compatible
> > > string (as fist string in the list), by using markers for the
> > 
> > s/fist/first/
> 
> Well spotted
> 
> > 
> > > compatible string we match against and the index of the matching
> > > compatible string in the list.
> > >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > ---
> > > Hi Laurent,
> > >
> > > I don't think we need to do anything in the driver to address your
> > > comment, as we can "enforce" this with the bindings (please see the
> > > next patch, as it would help with the "enforcing" of the compatible
> > > string for the thine device).
> > > I am sending this patch only so that you can see what a possible
> > > solution in the driver could look like.
> > >
> > > v3->v4:
> > > * New patch addressing the below comment from Laurent:
> > > "I think the lvds-decoder driver should error out at probe time if only
> > > one compatible string is listed."
> > > ---
> > >  drivers/gpu/drm/bridge/lvds-codec.c | 55 +++++++++++++++++++++++++++++++++----
> > >  1 file changed, 49 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > index 784bbd3..145c25d 100644
> > > --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > @@ -14,11 +14,16 @@
> > >  #include <drm/drm_bridge.h>
> > >  #include <drm/drm_panel.h>
> > >
> > > +struct lvds_codec_data {
> > > +	u32 connector_type;
> > > +	bool device_specific;
> > > +};
> > > +
> > >  struct lvds_codec {
> > >  	struct drm_bridge bridge;
> > >  	struct drm_bridge *panel_bridge;
> > >  	struct gpio_desc *powerdown_gpio;
> > > -	u32 connector_type;
> > > +	const struct lvds_codec_data *data;
> > >  };
> > >
> > >  static int lvds_codec_attach(struct drm_bridge *bridge)
> > > @@ -65,7 +70,30 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > >  	if (!lvds_codec)
> > >  		return -ENOMEM;
> > >
> > > -	lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > > +	lvds_codec->data = of_device_get_match_data(&pdev->dev);
> > > +	if (!lvds_codec->data)
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * If we haven't matched a device specific compatible string, we need
> > > +	 * to work out if the generic compatible string we matched against was
> > > +	 * listed first in the compatible property.
> > > +	 */
> > 
> > Can't we do this unconditionally, and thus drop the lvds_codec_data
> > structure ?
> 
> I don't think so, and the reason for this is that we have a corner case for
> thine,thc63lvdm83d. Here is what's allowed (according to the documentation)
> from what's supported upstream (+ this series):
> "ti,ds90c185", "lvds-encoder"
> "ti,ds90c187", "lvds-encoder"
> "ti,sn75lvds83", "lvds-encoder"
> "ti,ds90cf384a", "lvds-decoder"
> "thine,thc63lvdm83d"
> 
> As you can see from the examples above, in most cases it's enough to say it's
> all good when we match a compatible string with index > 0, but for the thine
> device you _have_ to match the string with index 0 as that's what's currently
> documented (please see thine,thc63lvdm83d.txt) and that's what's supported
> by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts).

How about the following logic ?

	if (match_index("lvds-encoder") == 0 ||
	    match_index("lvds-decoder") == 0)
		return -EINVAL;

?

> This patch "classifies" compatible strings, and it considers a good match
> device specific compatible strings, or generic compatible strings as long
> as they are not listed first.
> 
> These days you can leverage the yaml files to validate the device trees,
> therefore we should be focusing on writing yaml files in such a way we only
> pass the checks we mean to, and by checks I mean:
> make dtbs_check
> 
> or more specifically, for this series:
> make dtbs_check  DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> 
> and that's of course on top of make dt_binding_check.

Sure, but that doesn't prevent anyone ignoring the validation.

> It's a very common requirement to have a part number specific compatible
> string first followed by a generic (fallback) compatible string in the device trees,
> most drivers for Renesas SoCs have similar requirements.
> 
> If we start doing this here, we'll end up doing it elsewhere as well, and I really
> think we shouldn't, but others may see things differently, so I'll wait for others
> (and yourself with further comments) to jump in before doing any more work
> on this patch.

I agree with this argument, it would set a precedent, and is probably
not worth duplicating similar code in all drivers. I wonder if this is
something we could handle with core helpers, but maybe it's overkill.

> > > +	if (!lvds_codec->data->device_specific) {
> > > +		const struct of_device_id *match;
> > > +		int compatible_index;
> > > +
> > > +		match = of_match_node(dev->driver->of_match_table,
> > > +				      dev->of_node);
> > > +		compatible_index = of_property_match_string(dev->of_node,
> > > +							    "compatible",
> > > +							    match->compatible);
> > > +		if (compatible_index == 0) {
> > > +			dev_err(dev, "Device specific compatible needed\n");
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> > > +
> > >  	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > >  							     GPIOD_OUT_HIGH);
> > >  	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > >
> > >  	lvds_codec->panel_bridge =
> > >  		devm_drm_panel_bridge_add_typed(dev, panel,
> > > -						lvds_codec->connector_type);
> > > +					lvds_codec->data->connector_type);
> > >  	if (IS_ERR(lvds_codec->panel_bridge))
> > >  		return PTR_ERR(lvds_codec->panel_bridge);
> > >
> > > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev)
> > >  	return 0;
> > >  }
> > >
> > > +static const struct lvds_codec_data lvds_codec_decoder_data = {
> > > +	.connector_type = DRM_MODE_CONNECTOR_DPI,
> > > +	.device_specific = false,
> > > +};
> > > +
> > > +static const struct lvds_codec_data lvds_codec_encoder_data = {
> > > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > +	.device_specific = false,
> > > +};
> > > +
> > > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = {
> > > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > +	.device_specific = true,
> > > +};
> > > +
> > >  static const struct of_device_id lvds_codec_match[] = {
> > >  	{
> > >  		.compatible = "lvds-decoder",
> > > -		.data = (void *)DRM_MODE_CONNECTOR_DPI,
> > > +		.data = &lvds_codec_decoder_data,
> > >  	},
> > >  	{
> > >  		.compatible = "lvds-encoder",
> > > -		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > > +		.data = &lvds_codec_encoder_data,
> > >  	},
> > >  	{
> > >  		.compatible = "thine,thc63lvdm83d",
> > > -		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > > +		.data = &lvds_codec_thc63lvdm83d_data,
> > >  	},
> > >  	{},
> > >  };

-- 
Regards,

Laurent Pinchart

RE: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings

[Fabrizio Castro]

Hi Laurent,

Thank you for your feedback!

> From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 19 November 2019 21:52
> Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings
> 
> Hi Fabrizio,
> 
> On Tue, Nov 19, 2019 at 11:17:34AM +0000, Fabrizio Castro wrote:
> > On 19 November 2019 00:16 Laurent Pinchart wrote:
> > > On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote:
> > > > The lvds-codec driver is a generic stub for transparent LVDS
> > > > encoders and decoders.
> > > > It's good practice to list a device specific compatible string
> > >
> > > s/good practice/mandatory/
> >
> > Will fix
> >
> > > > before the generic fallback (if any) in the DT node for the relevant
> > > > LVDS encoder/decoder, and it's also required by the dt-bindings.
> > > > A notable exception to the generic fallback mechanism is the case
> > > > of "thine,thc63lvdm83d", as documented in:
> > > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> > > > This patch enforces the adoption of a device specific compatible
> > > > string (as fist string in the list), by using markers for the
> > >
> > > s/fist/first/
> >
> > Well spotted
> >
> > >
> > > > compatible string we match against and the index of the matching
> > > > compatible string in the list.
> > > >
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > >
> > > > ---
> > > > Hi Laurent,
> > > >
> > > > I don't think we need to do anything in the driver to address your
> > > > comment, as we can "enforce" this with the bindings (please see the
> > > > next patch, as it would help with the "enforcing" of the compatible
> > > > string for the thine device).
> > > > I am sending this patch only so that you can see what a possible
> > > > solution in the driver could look like.
> > > >
> > > > v3->v4:
> > > > * New patch addressing the below comment from Laurent:
> > > > "I think the lvds-decoder driver should error out at probe time if only
> > > > one compatible string is listed."
> > > > ---
> > > >  drivers/gpu/drm/bridge/lvds-codec.c | 55 +++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 49 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > > index 784bbd3..145c25d 100644
> > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > > @@ -14,11 +14,16 @@
> > > >  #include <drm/drm_bridge.h>
> > > >  #include <drm/drm_panel.h>
> > > >
> > > > +struct lvds_codec_data {
> > > > +	u32 connector_type;
> > > > +	bool device_specific;
> > > > +};
> > > > +
> > > >  struct lvds_codec {
> > > >  	struct drm_bridge bridge;
> > > >  	struct drm_bridge *panel_bridge;
> > > >  	struct gpio_desc *powerdown_gpio;
> > > > -	u32 connector_type;
> > > > +	const struct lvds_codec_data *data;
> > > >  };
> > > >
> > > >  static int lvds_codec_attach(struct drm_bridge *bridge)
> > > > @@ -65,7 +70,30 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > > >  	if (!lvds_codec)
> > > >  		return -ENOMEM;
> > > >
> > > > -	lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > > > +	lvds_codec->data = of_device_get_match_data(&pdev->dev);
> > > > +	if (!lvds_codec->data)
> > > > +		return -EINVAL;
> > > > +
> > > > +	/*
> > > > +	 * If we haven't matched a device specific compatible string, we need
> > > > +	 * to work out if the generic compatible string we matched against was
> > > > +	 * listed first in the compatible property.
> > > > +	 */
> > >
> > > Can't we do this unconditionally, and thus drop the lvds_codec_data
> > > structure ?
> >
> > I don't think so, and the reason for this is that we have a corner case for
> > thine,thc63lvdm83d. Here is what's allowed (according to the documentation)
> > from what's supported upstream (+ this series):
> > "ti,ds90c185", "lvds-encoder"
> > "ti,ds90c187", "lvds-encoder"
> > "ti,sn75lvds83", "lvds-encoder"
> > "ti,ds90cf384a", "lvds-decoder"
> > "thine,thc63lvdm83d"
> >
> > As you can see from the examples above, in most cases it's enough to say it's
> > all good when we match a compatible string with index > 0, but for the thine
> > device you _have_ to match the string with index 0 as that's what's currently
> > documented (please see thine,thc63lvdm83d.txt) and that's what's supported
> > by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts).
> 
> How about the following logic ?
> 
> 	if (match_index("lvds-encoder") == 0 ||
> 	    match_index("lvds-decoder") == 0)
> 		return -EINVAL;
> 
> 

Now I see what you mean

> 
> > This patch "classifies" compatible strings, and it considers a good match
> > device specific compatible strings, or generic compatible strings as long
> > as they are not listed first.
> >
> > These days you can leverage the yaml files to validate the device trees,
> > therefore we should be focusing on writing yaml files in such a way we only
> > pass the checks we mean to, and by checks I mean:
> > make dtbs_check
> >
> > or more specifically, for this series:
> > make dtbs_check  DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> >
> > and that's of course on top of make dt_binding_check.
> 
> Sure, but that doesn't prevent anyone ignoring the validation.
> 
> > It's a very common requirement to have a part number specific compatible
> > string first followed by a generic (fallback) compatible string in the device trees,
> > most drivers for Renesas SoCs have similar requirements.
> >
> > If we start doing this here, we'll end up doing it elsewhere as well, and I really
> > think we shouldn't, but others may see things differently, so I'll wait for others
> > (and yourself with further comments) to jump in before doing any more work
> > on this patch.
> 
> I agree with this argument, it would set a precedent, and is probably
> not worth duplicating similar code in all drivers. I wonder if this is
> something we could handle with core helpers, but maybe it's overkill.

I was hoping others would comment as well, but perhaps this topic is not too exciting.

Geert, what do you think about this? Is this something we should enforce
in drivers?

Thanks,
Fab

> 
> > > > +	if (!lvds_codec->data->device_specific) {
> > > > +		const struct of_device_id *match;
> > > > +		int compatible_index;
> > > > +
> > > > +		match = of_match_node(dev->driver->of_match_table,
> > > > +				      dev->of_node);
> > > > +		compatible_index = of_property_match_string(dev->of_node,
> > > > +							    "compatible",
> > > > +							    match->compatible);
> > > > +		if (compatible_index == 0) {
> > > > +			dev_err(dev, "Device specific compatible needed\n");
> > > > +			return -EINVAL;
> > > > +		}
> > > > +	}
> > > > +
> > > >  	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > >  							     GPIOD_OUT_HIGH);
> > > >  	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > > > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > > >
> > > >  	lvds_codec->panel_bridge =
> > > >  		devm_drm_panel_bridge_add_typed(dev, panel,
> > > > -						lvds_codec->connector_type);
> > > > +					lvds_codec->data->connector_type);
> > > >  	if (IS_ERR(lvds_codec->panel_bridge))
> > > >  		return PTR_ERR(lvds_codec->panel_bridge);
> > > >
> > > > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static const struct lvds_codec_data lvds_codec_decoder_data = {
> > > > +	.connector_type = DRM_MODE_CONNECTOR_DPI,
> > > > +	.device_specific = false,
> > > > +};
> > > > +
> > > > +static const struct lvds_codec_data lvds_codec_encoder_data = {
> > > > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > > +	.device_specific = false,
> > > > +};
> > > > +
> > > > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = {
> > > > +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > > +	.device_specific = true,
> > > > +};
> > > > +
> > > >  static const struct of_device_id lvds_codec_match[] = {
> > > >  	{
> > > >  		.compatible = "lvds-decoder",
> > > > -		.data = (void *)DRM_MODE_CONNECTOR_DPI,
> > > > +		.data = &lvds_codec_decoder_data,
> > > >  	},
> > > >  	{
> > > >  		.compatible = "lvds-encoder",
> > > > -		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > > > +		.data = &lvds_codec_encoder_data,
> > > >  	},
> > > >  	{
> > > >  		.compatible = "thine,thc63lvdm83d",
> > > > -		.data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > > > +		.data = &lvds_codec_thc63lvdm83d_data,
> > > >  	},
> > > >  	{},
> > > >  };
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings

[Geert Uytterhoeven]

Hi Fabrizio,

On Thu, Nov 21, 2019 at 5:00 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> > From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> > Sent: 19 November 2019 21:52
> > Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings
> >
> > On Tue, Nov 19, 2019 at 11:17:34AM +0000, Fabrizio Castro wrote:
> > > On 19 November 2019 00:16 Laurent Pinchart wrote:
> > > > On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote:
> > > > > The lvds-codec driver is a generic stub for transparent LVDS
> > > > > encoders and decoders.
> > > > > It's good practice to list a device specific compatible string
> > > > > before the generic fallback (if any) in the DT node for the relevant
> > > > > LVDS encoder/decoder, and it's also required by the dt-bindings.
> > > > > A notable exception to the generic fallback mechanism is the case
> > > > > of "thine,thc63lvdm83d", as documented in:
> > > > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> > > > > This patch enforces the adoption of a device specific compatible
> > > > > string (as fist string in the list), by using markers for the
> > > > > compatible string we match against and the index of the matching
> > > > > compatible string in the list.
> > > > >
> > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > >
> > > > > ---
> > > > > Hi Laurent,
> > > > >
> > > > > I don't think we need to do anything in the driver to address your
> > > > > comment, as we can "enforce" this with the bindings (please see the
> > > > > next patch, as it would help with the "enforcing" of the compatible
> > > > > string for the thine device).
> > > > > I am sending this patch only so that you can see what a possible
> > > > > solution in the driver could look like.
> > > > >
> > > > > v3->v4:
> > > > > * New patch addressing the below comment from Laurent:
> > > > > "I think the lvds-decoder driver should error out at probe time if only
> > > > > one compatible string is listed."

> > > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c

> > > > > @@ -65,7 +70,30 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > > > >         if (!lvds_codec)
> > > > >                 return -ENOMEM;
> > > > >
> > > > > -       lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > > > > +       lvds_codec->data = of_device_get_match_data(&pdev->dev);
> > > > > +       if (!lvds_codec->data)
> > > > > +               return -EINVAL;
> > > > > +
> > > > > +       /*
> > > > > +        * If we haven't matched a device specific compatible string, we need
> > > > > +        * to work out if the generic compatible string we matched against was
> > > > > +        * listed first in the compatible property.
> > > > > +        */
> > > >
> > > > Can't we do this unconditionally, and thus drop the lvds_codec_data
> > > > structure ?
> > >
> > > I don't think so, and the reason for this is that we have a corner case for
> > > thine,thc63lvdm83d. Here is what's allowed (according to the documentation)
> > > from what's supported upstream (+ this series):
> > > "ti,ds90c185", "lvds-encoder"
> > > "ti,ds90c187", "lvds-encoder"
> > > "ti,sn75lvds83", "lvds-encoder"
> > > "ti,ds90cf384a", "lvds-decoder"
> > > "thine,thc63lvdm83d"
> > >
> > > As you can see from the examples above, in most cases it's enough to say it's
> > > all good when we match a compatible string with index > 0, but for the thine
> > > device you _have_ to match the string with index 0 as that's what's currently
> > > documented (please see thine,thc63lvdm83d.txt) and that's what's supported
> > > by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts).
> >
> > How about the following logic ?
> >
> >       if (match_index("lvds-encoder") == 0 ||
> >           match_index("lvds-decoder") == 0)
> >               return -EINVAL;
> >
> >
>
> Now I see what you mean
>
> > > This patch "classifies" compatible strings, and it considers a good match
> > > device specific compatible strings, or generic compatible strings as long
> > > as they are not listed first.
> > >
> > > These days you can leverage the yaml files to validate the device trees,
> > > therefore we should be focusing on writing yaml files in such a way we only
> > > pass the checks we mean to, and by checks I mean:
> > > make dtbs_check
> > >
> > > or more specifically, for this series:
> > > make dtbs_check  DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > >
> > > and that's of course on top of make dt_binding_check.
> >
> > Sure, but that doesn't prevent anyone ignoring the validation.
> >
> > > It's a very common requirement to have a part number specific compatible
> > > string first followed by a generic (fallback) compatible string in the device trees,
> > > most drivers for Renesas SoCs have similar requirements.
> > >
> > > If we start doing this here, we'll end up doing it elsewhere as well, and I really
> > > think we shouldn't, but others may see things differently, so I'll wait for others
> > > (and yourself with further comments) to jump in before doing any more work
> > > on this patch.
> >
> > I agree with this argument, it would set a precedent, and is probably
> > not worth duplicating similar code in all drivers. I wonder if this is
> > something we could handle with core helpers, but maybe it's overkill.
>
> I was hoping others would comment as well, but perhaps this topic is not too exciting.
>
> Geert, what do you think about this? Is this something we should enforce
> in drivers?

So IIUIC, you want to enforce the presence of both specific and generic
compatible values (in that order) in the driver (except for
"thine,thc63lvdm83d", as that predates the introduction of the generic
compatible value)?
However, the driver would not really care about the actual hardware-specific
value, as it would still match against the generic one, and the
hardware-specific one may not even be listed in the driver's match table?

By definition, you can have one or more compatible values listed in a
device node, from most-specific to least-specific.  Typically the driver
cannot know if a more specific value is missing, but YAML DT binding
validation can.

In this case it is a bit special, as there is a generic one involved, so
you can assume there should be a more specific one, too.
If you want to handle this in the core, you probably need to add an
"is_generic" flag to struct of_device_id.

Rob/Mark?

> > > > > +       if (!lvds_codec->data->device_specific) {
> > > > > +               const struct of_device_id *match;
> > > > > +               int compatible_index;
> > > > > +
> > > > > +               match = of_match_node(dev->driver->of_match_table,
> > > > > +                                     dev->of_node);
> > > > > +               compatible_index = of_property_match_string(dev->of_node,
> > > > > +                                                           "compatible",
> > > > > +                                                           match->compatible);
> > > > > +               if (compatible_index == 0) {
> > > > > +                       dev_err(dev, "Device specific compatible needed\n");
> > > > > +                       return -EINVAL;

-ENODEV?
So a "more generic" driver can take over?

> > > > > +               }
> > > > > +       }
> > > > > +
> > > > >         lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > > >                                                              GPIOD_OUT_HIGH);
> > > > >         if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > > > > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > > > >
> > > > >         lvds_codec->panel_bridge =
> > > > >                 devm_drm_panel_bridge_add_typed(dev, panel,
> > > > > -                                               lvds_codec->connector_type);
> > > > > +                                       lvds_codec->data->connector_type);
> > > > >         if (IS_ERR(lvds_codec->panel_bridge))
> > > > >                 return PTR_ERR(lvds_codec->panel_bridge);
> > > > >
> > > > > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev)
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +static const struct lvds_codec_data lvds_codec_decoder_data = {
> > > > > +       .connector_type = DRM_MODE_CONNECTOR_DPI,
> > > > > +       .device_specific = false,
> > > > > +};
> > > > > +
> > > > > +static const struct lvds_codec_data lvds_codec_encoder_data = {
> > > > > +       .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > > > +       .device_specific = false,
> > > > > +};
> > > > > +
> > > > > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = {
> > > > > +       .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > > > +       .device_specific = true,
> > > > > +};
> > > > > +
> > > > >  static const struct of_device_id lvds_codec_match[] = {
> > > > >         {
> > > > >                 .compatible = "lvds-decoder",
> > > > > -               .data = (void *)DRM_MODE_CONNECTOR_DPI,
> > > > > +               .data = &lvds_codec_decoder_data,
> > > > >         },
> > > > >         {
> > > > >                 .compatible = "lvds-encoder",
> > > > > -               .data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > > > > +               .data = &lvds_codec_encoder_data,
> > > > >         },
> > > > >         {
> > > > >                 .compatible = "thine,thc63lvdm83d",
> > > > > -               .data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > > > > +               .data = &lvds_codec_thc63lvdm83d_data,
> > > > >         },
> > > > >         {},
> > > > >  };

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings

[Fabrizio Castro]

Hi Geert,

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 22 November 2019 08:17
> Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings
> 
> Hi Fabrizio,
> 
> On Thu, Nov 21, 2019 at 5:00 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > > From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> > > Sent: 19 November 2019 21:52
> > > Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings
> > >
> > > On Tue, Nov 19, 2019 at 11:17:34AM +0000, Fabrizio Castro wrote:
> > > > On 19 November 2019 00:16 Laurent Pinchart wrote:
> > > > > On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote:
> > > > > > The lvds-codec driver is a generic stub for transparent LVDS
> > > > > > encoders and decoders.
> > > > > > It's good practice to list a device specific compatible string
> > > > > > before the generic fallback (if any) in the DT node for the relevant
> > > > > > LVDS encoder/decoder, and it's also required by the dt-bindings.
> > > > > > A notable exception to the generic fallback mechanism is the case
> > > > > > of "thine,thc63lvdm83d", as documented in:
> > > > > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> > > > > > This patch enforces the adoption of a device specific compatible
> > > > > > string (as fist string in the list), by using markers for the
> > > > > > compatible string we match against and the index of the matching
> > > > > > compatible string in the list.
> > > > > >
> > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > > >
> > > > > > ---
> > > > > > Hi Laurent,
> > > > > >
> > > > > > I don't think we need to do anything in the driver to address your
> > > > > > comment, as we can "enforce" this with the bindings (please see the
> > > > > > next patch, as it would help with the "enforcing" of the compatible
> > > > > > string for the thine device).
> > > > > > I am sending this patch only so that you can see what a possible
> > > > > > solution in the driver could look like.
> > > > > >
> > > > > > v3->v4:
> > > > > > * New patch addressing the below comment from Laurent:
> > > > > > "I think the lvds-decoder driver should error out at probe time if only
> > > > > > one compatible string is listed."
> 
> > > > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > > > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> 
> > > > > > @@ -65,7 +70,30 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > > > > >         if (!lvds_codec)
> > > > > >                 return -ENOMEM;
> > > > > >
> > > > > > -       lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> > > > > > +       lvds_codec->data = of_device_get_match_data(&pdev->dev);
> > > > > > +       if (!lvds_codec->data)
> > > > > > +               return -EINVAL;
> > > > > > +
> > > > > > +       /*
> > > > > > +        * If we haven't matched a device specific compatible string, we need
> > > > > > +        * to work out if the generic compatible string we matched against was
> > > > > > +        * listed first in the compatible property.
> > > > > > +        */
> > > > >
> > > > > Can't we do this unconditionally, and thus drop the lvds_codec_data
> > > > > structure ?
> > > >
> > > > I don't think so, and the reason for this is that we have a corner case for
> > > > thine,thc63lvdm83d. Here is what's allowed (according to the documentation)
> > > > from what's supported upstream (+ this series):
> > > > "ti,ds90c185", "lvds-encoder"
> > > > "ti,ds90c187", "lvds-encoder"
> > > > "ti,sn75lvds83", "lvds-encoder"
> > > > "ti,ds90cf384a", "lvds-decoder"
> > > > "thine,thc63lvdm83d"
> > > >
> > > > As you can see from the examples above, in most cases it's enough to say it's
> > > > all good when we match a compatible string with index > 0, but for the thine
> > > > device you _have_ to match the string with index 0 as that's what's currently
> > > > documented (please see thine,thc63lvdm83d.txt) and that's what's supported
> > > > by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts).
> > >
> > > How about the following logic ?
> > >
> > >       if (match_index("lvds-encoder") == 0 ||
> > >           match_index("lvds-decoder") == 0)
> > >               return -EINVAL;
> > >
> > >
> >
> > Now I see what you mean
> >
> > > > This patch "classifies" compatible strings, and it considers a good match
> > > > device specific compatible strings, or generic compatible strings as long
> > > > as they are not listed first.
> > > >
> > > > These days you can leverage the yaml files to validate the device trees,
> > > > therefore we should be focusing on writing yaml files in such a way we only
> > > > pass the checks we mean to, and by checks I mean:
> > > > make dtbs_check
> > > >
> > > > or more specifically, for this series:
> > > > make dtbs_check  DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > > >
> > > > and that's of course on top of make dt_binding_check.
> > >
> > > Sure, but that doesn't prevent anyone ignoring the validation.
> > >
> > > > It's a very common requirement to have a part number specific compatible
> > > > string first followed by a generic (fallback) compatible string in the device trees,
> > > > most drivers for Renesas SoCs have similar requirements.
> > > >
> > > > If we start doing this here, we'll end up doing it elsewhere as well, and I really
> > > > think we shouldn't, but others may see things differently, so I'll wait for others
> > > > (and yourself with further comments) to jump in before doing any more work
> > > > on this patch.
> > >
> > > I agree with this argument, it would set a precedent, and is probably
> > > not worth duplicating similar code in all drivers. I wonder if this is
> > > something we could handle with core helpers, but maybe it's overkill.
> >
> > I was hoping others would comment as well, but perhaps this topic is not too exciting.
> >
> > Geert, what do you think about this? Is this something we should enforce
> > in drivers?
> 
> So IIUIC, you want to enforce the presence of both specific and generic
> compatible values (in that order) in the driver (except for
> "thine,thc63lvdm83d", as that predates the introduction of the generic
> compatible value)?

Yeah, this is what Laurent would want ideally.

> However, the driver would not really care about the actual hardware-specific
> value, as it would still match against the generic one, and the
> hardware-specific one may not even be listed in the driver's match table?

Exactly.

> 
> By definition, you can have one or more compatible values listed in a
> device node, from most-specific to least-specific.  Typically the driver
> cannot know if a more specific value is missing, but YAML DT binding
> validation can.
> 
> In this case it is a bit special, as there is a generic one involved, so
> you can assume there should be a more specific one, too.
> If you want to handle this in the core, you probably need to add an
> "is_generic" flag to struct of_device_id.

That's actually an interesting way of looking at this.
Laurent?

Thanks,
Fab

> 
> Rob/Mark?
> 
> > > > > > +       if (!lvds_codec->data->device_specific) {
> > > > > > +               const struct of_device_id *match;
> > > > > > +               int compatible_index;
> > > > > > +
> > > > > > +               match = of_match_node(dev->driver->of_match_table,
> > > > > > +                                     dev->of_node);
> > > > > > +               compatible_index = of_property_match_string(dev->of_node,
> > > > > > +                                                           "compatible",
> > > > > > +                                                           match->compatible);
> > > > > > +               if (compatible_index == 0) {
> > > > > > +                       dev_err(dev, "Device specific compatible needed\n");
> > > > > > +                       return -EINVAL;
> 
> -ENODEV?
> So a "more generic" driver can take over?
> 
> > > > > > +               }
> > > > > > +       }
> > > > > > +
> > > > > >         lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > > > >                                                              GPIOD_OUT_HIGH);
> > > > > >         if (IS_ERR(lvds_codec->powerdown_gpio)) {
> > > > > > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > > > > >
> > > > > >         lvds_codec->panel_bridge =
> > > > > >                 devm_drm_panel_bridge_add_typed(dev, panel,
> > > > > > -                                               lvds_codec->connector_type);
> > > > > > +                                       lvds_codec->data->connector_type);
> > > > > >         if (IS_ERR(lvds_codec->panel_bridge))
> > > > > >                 return PTR_ERR(lvds_codec->panel_bridge);
> > > > > >
> > > > > > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev)
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > +static const struct lvds_codec_data lvds_codec_decoder_data = {
> > > > > > +       .connector_type = DRM_MODE_CONNECTOR_DPI,
> > > > > > +       .device_specific = false,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct lvds_codec_data lvds_codec_encoder_data = {
> > > > > > +       .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > > > > +       .device_specific = false,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = {
> > > > > > +       .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > > > > +       .device_specific = true,
> > > > > > +};
> > > > > > +
> > > > > >  static const struct of_device_id lvds_codec_match[] = {
> > > > > >         {
> > > > > >                 .compatible = "lvds-decoder",
> > > > > > -               .data = (void *)DRM_MODE_CONNECTOR_DPI,
> > > > > > +               .data = &lvds_codec_decoder_data,
> > > > > >         },
> > > > > >         {
> > > > > >                 .compatible = "lvds-encoder",
> > > > > > -               .data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > > > > > +               .data = &lvds_codec_encoder_data,
> > > > > >         },
> > > > > >         {
> > > > > >                 .compatible = "thine,thc63lvdm83d",
> > > > > > -               .data = (void *)DRM_MODE_CONNECTOR_LVDS,
> > > > > > +               .data = &lvds_codec_thc63lvdm83d_data,
> > > > > >         },
> > > > > >         {},
> > > > > >  };
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings

[Laurent Pinchart]

Hi Fabrizio,

On Mon, Nov 25, 2019 at 11:17:25AM +0000, Fabrizio Castro wrote:
> On 22 November 2019 08:17 Geert Uytterhoeven wrote:
> > On Thu, Nov 21, 2019 at 5:00 PM Fabrizio Castro wrote:
> >> On 19 November 2019 21:52 Laurent Pinchart wrote:
> >>> On Tue, Nov 19, 2019 at 11:17:34AM +0000, Fabrizio Castro wrote:
> >>>> On 19 November 2019 00:16 Laurent Pinchart wrote:
> >>>>> On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote:
> >>>>>> The lvds-codec driver is a generic stub for transparent LVDS
> >>>>>> encoders and decoders.
> >>>>>> It's good practice to list a device specific compatible string
> >>>>>> before the generic fallback (if any) in the DT node for the relevant
> >>>>>> LVDS encoder/decoder, and it's also required by the dt-bindings.
> >>>>>> A notable exception to the generic fallback mechanism is the case
> >>>>>> of "thine,thc63lvdm83d", as documented in:
> >>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> >>>>>> This patch enforces the adoption of a device specific compatible
> >>>>>> string (as fist string in the list), by using markers for the
> >>>>>> compatible string we match against and the index of the matching
> >>>>>> compatible string in the list.
> >>>>>>
> >>>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >>>>>>
> >>>>>> ---
> >>>>>> Hi Laurent,
> >>>>>>
> >>>>>> I don't think we need to do anything in the driver to address your
> >>>>>> comment, as we can "enforce" this with the bindings (please see the
> >>>>>> next patch, as it would help with the "enforcing" of the compatible
> >>>>>> string for the thine device).
> >>>>>> I am sending this patch only so that you can see what a possible
> >>>>>> solution in the driver could look like.
> >>>>>>
> >>>>>> v3->v4:
> >>>>>> * New patch addressing the below comment from Laurent:
> >>>>>> "I think the lvds-decoder driver should error out at probe time if only
> >>>>>> one compatible string is listed."
> >>>>>>
> >>>>>> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> >>>>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> >>>>>>
> >>>>>> @@ -65,7 +70,30 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >>>>>>         if (!lvds_codec)
> >>>>>>                 return -ENOMEM;
> >>>>>>
> >>>>>> -       lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> >>>>>> +       lvds_codec->data = of_device_get_match_data(&pdev->dev);
> >>>>>> +       if (!lvds_codec->data)
> >>>>>> +               return -EINVAL;
> >>>>>> +
> >>>>>> +       /*
> >>>>>> +        * If we haven't matched a device specific compatible string, we need
> >>>>>> +        * to work out if the generic compatible string we matched against was
> >>>>>> +        * listed first in the compatible property.
> >>>>>> +        */
> >>>>>
> >>>>> Can't we do this unconditionally, and thus drop the lvds_codec_data
> >>>>> structure ?
> >>>>
> >>>> I don't think so, and the reason for this is that we have a corner case for
> >>>> thine,thc63lvdm83d. Here is what's allowed (according to the documentation)
> >>>> from what's supported upstream (+ this series):
> >>>> "ti,ds90c185", "lvds-encoder"
> >>>> "ti,ds90c187", "lvds-encoder"
> >>>> "ti,sn75lvds83", "lvds-encoder"
> >>>> "ti,ds90cf384a", "lvds-decoder"
> >>>> "thine,thc63lvdm83d"
> >>>>
> >>>> As you can see from the examples above, in most cases it's enough to say it's
> >>>> all good when we match a compatible string with index > 0, but for the thine
> >>>> device you _have_ to match the string with index 0 as that's what's currently
> >>>> documented (please see thine,thc63lvdm83d.txt) and that's what's supported
> >>>> by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts).
> >>>
> >>> How about the following logic ?
> >>>
> >>>       if (match_index("lvds-encoder") == 0 ||
> >>>           match_index("lvds-decoder") == 0)
> >>>               return -EINVAL;
> >>>
> >>>
> >>
> >> Now I see what you mean
> >>
> >>>> This patch "classifies" compatible strings, and it considers a good match
> >>>> device specific compatible strings, or generic compatible strings as long
> >>>> as they are not listed first.
> >>>>
> >>>> These days you can leverage the yaml files to validate the device trees,
> >>>> therefore we should be focusing on writing yaml files in such a way we only
> >>>> pass the checks we mean to, and by checks I mean:
> >>>> make dtbs_check
> >>>>
> >>>> or more specifically, for this series:
> >>>> make dtbs_check  DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> >>>>
> >>>> and that's of course on top of make dt_binding_check.
> >>>
> >>> Sure, but that doesn't prevent anyone ignoring the validation.
> >>>
> >>>> It's a very common requirement to have a part number specific compatible
> >>>> string first followed by a generic (fallback) compatible string in the device trees,
> >>>> most drivers for Renesas SoCs have similar requirements.
> >>>>
> >>>> If we start doing this here, we'll end up doing it elsewhere as well, and I really
> >>>> think we shouldn't, but others may see things differently, so I'll wait for others
> >>>> (and yourself with further comments) to jump in before doing any more work
> >>>> on this patch.
> >>>
> >>> I agree with this argument, it would set a precedent, and is probably
> >>> not worth duplicating similar code in all drivers. I wonder if this is
> >>> something we could handle with core helpers, but maybe it's overkill.
> >>
> >> I was hoping others would comment as well, but perhaps this topic is not too exciting.
> >>
> >> Geert, what do you think about this? Is this something we should enforce
> >> in drivers?
> > 
> > So IIUIC, you want to enforce the presence of both specific and generic
> > compatible values (in that order) in the driver (except for
> > "thine,thc63lvdm83d", as that predates the introduction of the generic
> > compatible value)?
> 
> Yeah, this is what Laurent would want ideally.
> 
> > However, the driver would not really care about the actual hardware-specific
> > value, as it would still match against the generic one, and the
> > hardware-specific one may not even be listed in the driver's match table?
> 
> Exactly.
> 
> > By definition, you can have one or more compatible values listed in a
> > device node, from most-specific to least-specific.  Typically the driver
> > cannot know if a more specific value is missing, but YAML DT binding
> > validation can.
> > 
> > In this case it is a bit special, as there is a generic one involved, so
> > you can assume there should be a more specific one, too.
> > If you want to handle this in the core, you probably need to add an
> > "is_generic" flag to struct of_device_id.
> 
> That's actually an interesting way of looking at this.
> Laurent?

I like the idea, it's better than doing it in each driver.

> > Rob/Mark?

Ping ?

> >>>>>> +       if (!lvds_codec->data->device_specific) {
> >>>>>> +               const struct of_device_id *match;
> >>>>>> +               int compatible_index;
> >>>>>> +
> >>>>>> +               match = of_match_node(dev->driver->of_match_table,
> >>>>>> +                                     dev->of_node);
> >>>>>> +               compatible_index = of_property_match_string(dev->of_node,
> >>>>>> +                                                           "compatible",
> >>>>>> +                                                           match->compatible);
> >>>>>> +               if (compatible_index == 0) {
> >>>>>> +                       dev_err(dev, "Device specific compatible needed\n");
> >>>>>> +                       return -EINVAL;
> > 
> > -ENODEV?
> > So a "more generic" driver can take over?
> > 
> >>>>>> +               }
> >>>>>> +       }
> >>>>>> +
> >>>>>>         lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> >>>>>>                                                              GPIOD_OUT_HIGH);
> >>>>>>         if (IS_ERR(lvds_codec->powerdown_gpio)) {
> >>>>>> @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >>>>>>
> >>>>>>         lvds_codec->panel_bridge =
> >>>>>>                 devm_drm_panel_bridge_add_typed(dev, panel,
> >>>>>> -                                               lvds_codec->connector_type);
> >>>>>> +                                       lvds_codec->data->connector_type);
> >>>>>>         if (IS_ERR(lvds_codec->panel_bridge))
> >>>>>>                 return PTR_ERR(lvds_codec->panel_bridge);
> >>>>>>
> >>>>>> @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev)
> >>>>>>         return 0;
> >>>>>>  }
> >>>>>>
> >>>>>> +static const struct lvds_codec_data lvds_codec_decoder_data = {
> >>>>>> +       .connector_type = DRM_MODE_CONNECTOR_DPI,
> >>>>>> +       .device_specific = false,
> >>>>>> +};
> >>>>>> +
> >>>>>> +static const struct lvds_codec_data lvds_codec_encoder_data = {
> >>>>>> +       .connector_type = DRM_MODE_CONNECTOR_LVDS,
> >>>>>> +       .device_specific = false,
> >>>>>> +};
> >>>>>> +
> >>>>>> +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = {
> >>>>>> +       .connector_type = DRM_MODE_CONNECTOR_LVDS,
> >>>>>> +       .device_specific = true,
> >>>>>> +};
> >>>>>> +
> >>>>>>  static const struct of_device_id lvds_codec_match[] = {
> >>>>>>         {
> >>>>>>                 .compatible = "lvds-decoder",
> >>>>>> -               .data = (void *)DRM_MODE_CONNECTOR_DPI,
> >>>>>> +               .data = &lvds_codec_decoder_data,
> >>>>>>         },
> >>>>>>         {
> >>>>>>                 .compatible = "lvds-encoder",
> >>>>>> -               .data = (void *)DRM_MODE_CONNECTOR_LVDS,
> >>>>>> +               .data = &lvds_codec_encoder_data,
> >>>>>>         },
> >>>>>>         {
> >>>>>>                 .compatible = "thine,thc63lvdm83d",
> >>>>>> -               .data = (void *)DRM_MODE_CONNECTOR_LVDS,
> >>>>>> +               .data = &lvds_codec_thc63lvdm83d_data,
> >>>>>>         },
> >>>>>>         {},
> >>>>>>  };

-- 
Regards,

Laurent Pinchart

[PATCH v4 13/13] [HACK] dt-bindings: display: bridge: lvds-codec: Absorb thine,thc63lvdm83d.txt

[Fabrizio Castro]

At this point in time, compatible string "thine,thc63lvdm83d" is
backed by the lvds-codec driver, and the documentation contained
in thine,thc63lvdm83d.txt is basically the same as the one
contained in lvds-codec.yaml (generic fallback compatible string
aside), therefore absorb thine,thc63lvdm83d.txt.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
Hi Laurent,

what do you think about this patch?

Thanks,
Fab

v3->v4:
* New patch
---
 .../bindings/display/bridge/lvds-codec.yaml        |  5 +--
 .../bindings/display/bridge/thine,thc63lvdm83d.txt | 50 ----------------------
 2 files changed, 2 insertions(+), 53 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
index 21f8c6e..420bfce 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
@@ -30,9 +30,6 @@ description: |
 
 properties:
   compatible:
-    description: |
-      Must list the device specific compatible string first, followed by the
-      generic compatible string.
     oneOf:
       - items:
         - enum:
@@ -44,6 +41,8 @@ properties:
         - enum:
           - ti,ds90cf384a     # For the DS90CF384A FPD-Link LVDS Receiver
         - const: lvds-decoder # Generic LVDS decoders compatible fallback
+      - enum:
+        - thine,thc63lvdm83d  # For the THC63LVDM83D LVDS serializer
 
   ports:
     type: object
diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
deleted file mode 100644
index fee3c88..0000000
--- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
+++ /dev/null
@@ -1,50 +0,0 @@
-THine Electronics THC63LVDM83D LVDS serializer
-----------------------------------------------
-
-The THC63LVDM83D is an LVDS serializer designed to support pixel data
-transmission between a host and a flat panel.
-
-Required properties:
-
-- compatible: Should be "thine,thc63lvdm83d"
-
-Optional properties:
-
-- powerdown-gpios: Power down control GPIO (the /PWDN pin, active low).
-
-Required nodes:
-
-The THC63LVDM83D has two video ports. Their connections are modeled using the
-OFgraph bindings specified in Documentation/devicetree/bindings/graph.txt.
-
-- Video port 0 for CMOS/TTL input
-- Video port 1 for LVDS output
-
-
-Example
--------
-
-	lvds_enc: encoder@0 {
-		compatible = "thine,thc63lvdm83d";
-
-		ports {
-			#address-cells = <1>;
-			#size-cells = <0>;
-
-			port@0 {
-				reg = <0>;
-
-				lvds_enc_in: endpoint@0 {
-					remote-endpoint = <&rgb_out>;
-				};
-			};
-
-			port@1 {
-				reg = <1>;
-
-				lvds_enc_out: endpoint@0 {
-					remote-endpoint = <&panel_in>;
-				};
-			};
-		};
-	};
-- 
2.7.4

Re: [PATCH v4 13/13] [HACK] dt-bindings: display: bridge: lvds-codec: Absorb thine,thc63lvdm83d.txt

[Rob Herring]

On Wed, Nov 13, 2019 at 03:51:32PM +0000, Fabrizio Castro wrote:
> At this point in time, compatible string "thine,thc63lvdm83d" is
> backed by the lvds-codec driver, and the documentation contained
> in thine,thc63lvdm83d.txt is basically the same as the one
> contained in lvds-codec.yaml (generic fallback compatible string
> aside), therefore absorb thine,thc63lvdm83d.txt.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> Hi Laurent,
> 
> what do you think about this patch?
> 
> Thanks,
> Fab
> 
> v3->v4:
> * New patch
> ---
>  .../bindings/display/bridge/lvds-codec.yaml        |  5 +--
>  .../bindings/display/bridge/thine,thc63lvdm83d.txt | 50 ----------------------
>  2 files changed, 2 insertions(+), 53 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> index 21f8c6e..420bfce 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -30,9 +30,6 @@ description: |
>  
>  properties:
>    compatible:
> -    description: |
> -      Must list the device specific compatible string first, followed by the
> -      generic compatible string.

Probably better to just omit this from the start. 

In any case:

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 13/13] [HACK] dt-bindings: display: bridge: lvds-codec: Absorb thine,thc63lvdm83d.txt

[Laurent Pinchart]

Hi Fabrizio,

Thank you for the patch.

On Wed, Nov 13, 2019 at 03:51:32PM +0000, Fabrizio Castro wrote:
> At this point in time, compatible string "thine,thc63lvdm83d" is
> backed by the lvds-codec driver, and the documentation contained
> in thine,thc63lvdm83d.txt is basically the same as the one
> contained in lvds-codec.yaml (generic fallback compatible string
> aside), therefore absorb thine,thc63lvdm83d.txt.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> Hi Laurent,
> 
> what do you think about this patch?

I think it's good :-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

It however doesn't address my concern that resulted in patch 12/13. A
DTB could still contain only lvds-encoder or lvds-decoder, even if the
DT bindings forbid it. Adding a check in the driver would enforce the
use of a device-specific compatible string. It adds quite a bit of
complexity though :-S

> v3->v4:
> * New patch
> ---
>  .../bindings/display/bridge/lvds-codec.yaml        |  5 +--
>  .../bindings/display/bridge/thine,thc63lvdm83d.txt | 50 ----------------------
>  2 files changed, 2 insertions(+), 53 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> index 21f8c6e..420bfce 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -30,9 +30,6 @@ description: |
>  
>  properties:
>    compatible:
> -    description: |
> -      Must list the device specific compatible string first, followed by the
> -      generic compatible string.
>      oneOf:
>        - items:
>          - enum:
> @@ -44,6 +41,8 @@ properties:
>          - enum:
>            - ti,ds90cf384a     # For the DS90CF384A FPD-Link LVDS Receiver
>          - const: lvds-decoder # Generic LVDS decoders compatible fallback
> +      - enum:
> +        - thine,thc63lvdm83d  # For the THC63LVDM83D LVDS serializer
>  
>    ports:
>      type: object
> diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> deleted file mode 100644
> index fee3c88..0000000
> --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> +++ /dev/null
> @@ -1,50 +0,0 @@
> -THine Electronics THC63LVDM83D LVDS serializer
> -----------------------------------------------
> -
> -The THC63LVDM83D is an LVDS serializer designed to support pixel data
> -transmission between a host and a flat panel.
> -
> -Required properties:
> -
> -- compatible: Should be "thine,thc63lvdm83d"
> -
> -Optional properties:
> -
> -- powerdown-gpios: Power down control GPIO (the /PWDN pin, active low).
> -
> -Required nodes:
> -
> -The THC63LVDM83D has two video ports. Their connections are modeled using the
> -OFgraph bindings specified in Documentation/devicetree/bindings/graph.txt.
> -
> -- Video port 0 for CMOS/TTL input
> -- Video port 1 for LVDS output
> -
> -
> -Example
> --------
> -
> -	lvds_enc: encoder@0 {
> -		compatible = "thine,thc63lvdm83d";
> -
> -		ports {
> -			#address-cells = <1>;
> -			#size-cells = <0>;
> -
> -			port@0 {
> -				reg = <0>;
> -
> -				lvds_enc_in: endpoint@0 {
> -					remote-endpoint = <&rgb_out>;
> -				};
> -			};
> -
> -			port@1 {
> -				reg = <1>;
> -
> -				lvds_enc_out: endpoint@0 {
> -					remote-endpoint = <&panel_in>;
> -				};
> -			};
> -		};
> -	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 00/13] Add LCD panel support to iwg20d

[Neil Armstrong]

Hi,

On 13/11/2019 16:51, Fabrizio Castro wrote:
> The iW-RainboW-G20D-Qseven RZ/G1M,G1N Qseven Development Platform
> comes with a 7" capacitive display kit from Emerging Display
> Technologies Corporation (EDT). This series adds all that's
> necessary for supporting it.
> 
> Thanks,
> Fab
> 
> v3->v4:
> * Reworked according to Laurent's and Jacopo's comments
> v2->v3:
> * Split the dt-schema patch in two patches as per Rob's comment
> * Made fixes to the dt-schema according to Rob's comment
> * Made fixes to the lvds-codec driver according to Jacopo's comments
> * Added two new patches:
>   * drm: Define DRM_MODE_CONNECTOR_PARALLEL
>   * drm/panel: panel-simple: Add connector type for etm0700g0dh6
> v1->v2:
> * Convert dt-bindings to dt-schema
> 
> Fabrizio Castro (13):
>   dt-bindings: display: bridge: Convert lvds-transmitter binding to
>     json-schema
>   dt-bindings: display: bridge: lvds-transmitter: Document
>     powerdown-gpios
>   dt-bindings: display: bridge: lvds-transmitter: Absorb ti,ds90c185.txt
>   dt-bindings: display: bridge: lvds-transmitter: Document
>     "ti,sn75lvds83"
>   drm/bridge: Repurpose lvds-encoder.c
>   drm/bridge: lvds-codec: Add "lvds-decoder" support
>   drm/bridge: lvds-codec: Simplify panel DT node localisation
>   dt-bindings: display: bridge: Repurpose lvds-encoder
>   dt-bindings: display: bridge: lvds-codec: Document ti,ds90cf384a
>   ARM: dts: iwg20d-q7-common: Add LCD support
>   ARM: shmobile_defconfig: Enable support for panels from EDT
>   [HACK] drm/bridge: lvds-codec: Enforce device specific compatible
>     strings
>   [HACK] dt-bindings: display: bridge: lvds-codec: Absorb
>     thine,thc63lvdm83d.txt
> 
>  .../bindings/display/bridge/lvds-codec.yaml        | 131 +++++++++++++++
>  .../bindings/display/bridge/lvds-transmitter.txt   |  66 --------
>  .../bindings/display/bridge/thine,thc63lvdm83d.txt |  50 ------
>  .../bindings/display/bridge/ti,ds90c185.txt        |  55 -------
>  arch/arm/boot/dts/iwg20d-q7-common.dtsi            |  85 ++++++++++
>  arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi           |   1 -
>  arch/arm/configs/shmobile_defconfig                |   3 +
>  drivers/gpu/drm/bridge/Kconfig                     |   8 +-
>  drivers/gpu/drm/bridge/Makefile                    |   2 +-
>  .../drm/bridge/{lvds-encoder.c => lvds-codec.c}    | 179 +++++++++++++--------
>  10 files changed, 333 insertions(+), 247 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
>  delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
>  rename drivers/gpu/drm/bridge/{lvds-encoder.c => lvds-codec.c} (25%)
> 

Applying patches 1->9 and 13 to drm-misc-next with correct SoB.

Also applied change on patch 8 while applying.

And checked the diff is the same as laurent's PR.

Neil

RE: [PATCH v4 00/13] Add LCD panel support to iwg20d

[Fabrizio Castro]

Hi Neil,


> From: Neil Armstrong <narmstrong@baylibre.com>
> Sent: 18 December 2019 11:53
> Subject: Re: [PATCH v4 00/13] Add LCD panel support to iwg20d
> 
> Hi,
> 
> On 13/11/2019 16:51, Fabrizio Castro wrote:
> > The iW-RainboW-G20D-Qseven RZ/G1M,G1N Qseven Development Platform
> > comes with a 7" capacitive display kit from Emerging Display
> > Technologies Corporation (EDT). This series adds all that's
> > necessary for supporting it.
> >
> > Thanks,
> > Fab
> >
> > v3->v4:
> > * Reworked according to Laurent's and Jacopo's comments
> > v2->v3:
> > * Split the dt-schema patch in two patches as per Rob's comment
> > * Made fixes to the dt-schema according to Rob's comment
> > * Made fixes to the lvds-codec driver according to Jacopo's comments
> > * Added two new patches:
> >   * drm: Define DRM_MODE_CONNECTOR_PARALLEL
> >   * drm/panel: panel-simple: Add connector type for etm0700g0dh6
> > v1->v2:
> > * Convert dt-bindings to dt-schema
> >
> > Fabrizio Castro (13):
> >   dt-bindings: display: bridge: Convert lvds-transmitter binding to
> >     json-schema
> >   dt-bindings: display: bridge: lvds-transmitter: Document
> >     powerdown-gpios
> >   dt-bindings: display: bridge: lvds-transmitter: Absorb ti,ds90c185.txt
> >   dt-bindings: display: bridge: lvds-transmitter: Document
> >     "ti,sn75lvds83"
> >   drm/bridge: Repurpose lvds-encoder.c
> >   drm/bridge: lvds-codec: Add "lvds-decoder" support
> >   drm/bridge: lvds-codec: Simplify panel DT node localisation
> >   dt-bindings: display: bridge: Repurpose lvds-encoder
> >   dt-bindings: display: bridge: lvds-codec: Document ti,ds90cf384a
> >   ARM: dts: iwg20d-q7-common: Add LCD support
> >   ARM: shmobile_defconfig: Enable support for panels from EDT
> >   [HACK] drm/bridge: lvds-codec: Enforce device specific compatible
> >     strings
> >   [HACK] dt-bindings: display: bridge: lvds-codec: Absorb
> >     thine,thc63lvdm83d.txt
> >
> >  .../bindings/display/bridge/lvds-codec.yaml        | 131 +++++++++++++++
> >  .../bindings/display/bridge/lvds-transmitter.txt   |  66 --------
> >  .../bindings/display/bridge/thine,thc63lvdm83d.txt |  50 ------
> >  .../bindings/display/bridge/ti,ds90c185.txt        |  55 -------
> >  arch/arm/boot/dts/iwg20d-q7-common.dtsi            |  85 ++++++++++
> >  arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi           |   1 -
> >  arch/arm/configs/shmobile_defconfig                |   3 +
> >  drivers/gpu/drm/bridge/Kconfig                     |   8 +-
> >  drivers/gpu/drm/bridge/Makefile                    |   2 +-
> >  .../drm/bridge/{lvds-encoder.c => lvds-codec.c}    | 179 +++++++++++++--------
> >  10 files changed, 333 insertions(+), 247 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> >  delete mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> >  delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> >  rename drivers/gpu/drm/bridge/{lvds-encoder.c => lvds-codec.c} (25%)
> >
> 
> Applying patches 1->9 and 13 to drm-misc-next with correct SoB.
> 
> Also applied change on patch 8 while applying.
> 
> And checked the diff is the same as laurent's PR.

Thank you.

Fab

> 
> Neil