* [PATCH v3 1/7] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema
2019-11-07 20:10 [PATCH v3 0/7] Add LCD panel support to iwg20d Fabrizio Castro
@ 2019-11-07 20:10 ` Fabrizio Castro
2019-11-07 20:20 ` Laurent Pinchart
2019-11-07 20:10 ` [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c Fabrizio Castro
` (5 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-07 20:10 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
Convert the lvds-transmitter binding to DT schema format using
json-schema.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
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..5be163a
--- /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 or decoder 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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 1/7] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema
2019-11-07 20:10 ` [PATCH v3 1/7] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema Fabrizio Castro
@ 2019-11-07 20:20 ` Laurent Pinchart
2019-11-08 9:15 ` Fabrizio Castro
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-07 20:20 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Fabrizio,
Thank you for the patch.
On Thu, Nov 07, 2019 at 08:10:57PM +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>
>
> ---
> 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..5be163a
> --- /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 or decoder compatible with this generic binding, but with
I think "or decoder" should be added in patch 3/7.
> + additional properties not listed here, must define its own binding and
> + list a device specific compatible first followed by the generic compatible
s/compatible/compatible./
> + enum:
> + - lvds-encoder
> +
How is this binding supposed to be used, should LVDS encoder bindings
reference it with $ref ? If so, how do those bindings extend the
compatible property ?
> + 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>;
> + };
> + };
> + };
> + };
> +
> +...
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* RE: [PATCH v3 1/7] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema
2019-11-07 20:20 ` Laurent Pinchart
@ 2019-11-08 9:15 ` Fabrizio Castro
2019-11-08 10:54 ` Laurent Pinchart
0 siblings, 1 reply; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-08 9:15 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hello Laurent,
Thank you for your feedback!
> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 20:21
> Subject: Re: [PATCH v3 1/7] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Thu, Nov 07, 2019 at 08:10:57PM +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>
> >
> > ---
> > 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..5be163a
> > --- /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 or decoder compatible with this generic binding, but with
>
> I think "or decoder" should be added in patch 3/7.
Good catch, will fix.
>
> > + additional properties not listed here, must define its own binding and
> > + list a device specific compatible first followed by the generic compatible
>
> s/compatible/compatible./
Will change
>
> > + enum:
> > + - lvds-encoder
> > +
>
> How is this binding supposed to be used, should LVDS encoder bindings
> reference it with $ref ? If so, how do those bindings extend the
> compatible property ?
I think for the time being we could simply list the compatible devices straight
into this binding and forget about referencing this with ref until we have a
use case that requires an extension that's not suitable for the generic case
(but this is probably highly unlikely as this is for transparent devices, which
means that if there is anything missing is probably worth implementing in the
generic driver as others may benefit from it).
Is this going to require a tidy? For example, should we absorb
Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt ?
Also, I have found an undocumented compatible ("ti,sn75lvds83") used
in a node with "lvds-encoder" as generic fallback, shall we incorporate
that one too?
We should also describe powerdown-gpio, somehow its documentation was
left behind.
Thanks,
Fab
>
> > + 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>;
> > + };
> > + };
> > + };
> > + };
> > +
> > +...
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 1/7] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema
2019-11-08 9:15 ` Fabrizio Castro
@ 2019-11-08 10:54 ` Laurent Pinchart
0 siblings, 0 replies; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-08 10:54 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Fabrizio,
On Fri, Nov 08, 2019 at 09:15:02AM +0000, Fabrizio Castro wrote:
> On 07 November 2019 20:21 Laurent Pinchart wrote:
> > On Thu, Nov 07, 2019 at 08:10:57PM +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>
> > >
> > > ---
> > > 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..5be163a
> > > --- /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 or decoder compatible with this generic binding, but with
> >
> > I think "or decoder" should be added in patch 3/7.
>
> Good catch, will fix.
>
> > > + additional properties not listed here, must define its own binding and
> > > + list a device specific compatible first followed by the generic compatible
> >
> > s/compatible/compatible./
>
> Will change
>
> > > + enum:
> > > + - lvds-encoder
> > > +
> >
> > How is this binding supposed to be used, should LVDS encoder bindings
> > reference it with $ref ? If so, how do those bindings extend the
> > compatible property ?
>
> I think for the time being we could simply list the compatible devices straight
> into this binding and forget about referencing this with ref until we have a
> use case that requires an extension that's not suitable for the generic case
> (but this is probably highly unlikely as this is for transparent devices, which
> means that if there is anything missing is probably worth implementing in the
> generic driver as others may benefit from it).
>
> Is this going to require a tidy? For example, should we absorb
> Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt ?
> Also, I have found an undocumented compatible ("ti,sn75lvds83") used
> in a node with "lvds-encoder" as generic fallback, shall we incorporate
> that one too?
I think we should, yes.
> We should also describe powerdown-gpio, somehow its documentation was
> left behind.
That would be useful, yes.
> > > + 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>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > +...
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c
2019-11-07 20:10 [PATCH v3 0/7] Add LCD panel support to iwg20d Fabrizio Castro
2019-11-07 20:10 ` [PATCH v3 1/7] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema Fabrizio Castro
@ 2019-11-07 20:10 ` Fabrizio Castro
2019-11-07 20:34 ` Laurent Pinchart
2019-11-07 20:10 ` [PATCH v3 3/7] dt-bindings: display: bridge: Repurpose lvds-encoder Fabrizio Castro
` (4 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-07 20:10 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
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.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v2->v3:
* No change
v1->v2:
* No change
---
drivers/gpu/drm/bridge/Kconfig | 8 +-
drivers/gpu/drm/bridge/Makefile | 2 +-
drivers/gpu/drm/bridge/lvds-codec.c | 131 ++++++++++++++++++++++++++++
drivers/gpu/drm/bridge/lvds-encoder.c | 155 ----------------------------------
4 files changed, 136 insertions(+), 160 deletions(-)
create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3436297..9e75ca4e 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 LVDS 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-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
new file mode 100644
index 0000000..d57a8eb
--- /dev/null
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2019 Renesas Electronics Corporation
+ * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ */
+
+#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>
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_panel.h>
+
+struct lvds_codec {
+ struct drm_bridge bridge;
+ struct drm_bridge *panel_bridge;
+ struct gpio_desc *powerdown_gpio;
+};
+
+static int lvds_codec_attach(struct drm_bridge *bridge)
+{
+ struct lvds_codec *lvds_codec = container_of(bridge,
+ struct lvds_codec, bridge);
+
+ return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
+ bridge);
+}
+
+static void lvds_codec_enable(struct drm_bridge *bridge)
+{
+ struct lvds_codec *lvds_codec = container_of(bridge,
+ struct lvds_codec, bridge);
+
+ if (lvds_codec->powerdown_gpio)
+ gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
+}
+
+static void lvds_codec_disable(struct drm_bridge *bridge)
+{
+ struct lvds_codec *lvds_codec = container_of(bridge,
+ struct lvds_codec, bridge);
+
+ if (lvds_codec->powerdown_gpio)
+ gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
+}
+
+static struct drm_bridge_funcs funcs = {
+ .attach = lvds_codec_attach,
+ .enable = lvds_codec_enable,
+ .disable = lvds_codec_disable,
+};
+
+static int lvds_codec_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *panel_node;
+ struct drm_panel *panel;
+ struct lvds_codec *lvds_codec;
+
+ lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
+ if (!lvds_codec)
+ return -ENOMEM;
+
+ lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(lvds_codec->powerdown_gpio))
+ return PTR_ERR(lvds_codec->powerdown_gpio);
+
+ panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
+ if (!panel_node) {
+ dev_dbg(dev, "panel DT node not found\n");
+ return -ENXIO;
+ }
+
+ panel = of_drm_find_panel(panel_node);
+ of_node_put(panel_node);
+ if (IS_ERR(panel)) {
+ dev_dbg(dev, "panel not found, deferring probe\n");
+ return PTR_ERR(panel);
+ }
+
+ lvds_codec->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+ 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,
+ * but we need a bridge attached to our of_node for our user
+ * to look up.
+ */
+ lvds_codec->bridge.of_node = dev->of_node;
+ lvds_codec->bridge.funcs = &funcs;
+ drm_bridge_add(&lvds_codec->bridge);
+
+ platform_set_drvdata(pdev, lvds_codec);
+
+ return 0;
+}
+
+static int lvds_codec_remove(struct platform_device *pdev)
+{
+ struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
+
+ drm_bridge_remove(&lvds_codec->bridge);
+
+ return 0;
+}
+
+static const struct of_device_id lvds_codec_match[] = {
+ { .compatible = "lvds-encoder" },
+ { .compatible = "thine,thc63lvdm83d" },
+ { .compatible = "lvds-decoder" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, lvds_codec_match);
+
+static struct platform_driver lvds_codec_driver = {
+ .probe = lvds_codec_probe,
+ .remove = lvds_codec_remove,
+ .driver = {
+ .name = "lvds-codec",
+ .of_match_table = lvds_codec_match,
+ },
+};
+module_platform_driver(lvds_codec_driver);
+
+MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
+MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
deleted file mode 100644
index e2132a8..0000000
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ /dev/null
@@ -1,155 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
- */
-
-#include <linux/gpio/consumer.h>
-#include <linux/module.h>
-#include <linux/of.h>
-#include <linux/of_graph.h>
-#include <linux/platform_device.h>
-
-#include <drm/drm_bridge.h>
-#include <drm/drm_panel.h>
-
-struct lvds_encoder {
- struct drm_bridge bridge;
- struct drm_bridge *panel_bridge;
- struct gpio_desc *powerdown_gpio;
-};
-
-static int lvds_encoder_attach(struct drm_bridge *bridge)
-{
- struct lvds_encoder *lvds_encoder = container_of(bridge,
- struct lvds_encoder,
- bridge);
-
- return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge,
- bridge);
-}
-
-static void lvds_encoder_enable(struct drm_bridge *bridge)
-{
- struct lvds_encoder *lvds_encoder = container_of(bridge,
- struct lvds_encoder,
- bridge);
-
- if (lvds_encoder->powerdown_gpio)
- gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
-}
-
-static void lvds_encoder_disable(struct drm_bridge *bridge)
-{
- struct lvds_encoder *lvds_encoder = container_of(bridge,
- struct lvds_encoder,
- bridge);
-
- if (lvds_encoder->powerdown_gpio)
- gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
-}
-
-static struct drm_bridge_funcs funcs = {
- .attach = lvds_encoder_attach,
- .enable = lvds_encoder_enable,
- .disable = lvds_encoder_disable,
-};
-
-static int lvds_encoder_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;
-
- lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
- if (!lvds_encoder)
- 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);
-
- if (err != -EPROBE_DEFER)
- dev_err(dev, "powerdown GPIO failure: %d\n", err);
- return err;
- }
-
- /* 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);
- if (!panel_node) {
- dev_dbg(dev, "no remote endpoint for port 1\n");
- return -ENXIO;
- }
-
- panel = of_drm_find_panel(panel_node);
- of_node_put(panel_node);
- if (IS_ERR(panel)) {
- dev_dbg(dev, "panel not found, deferring probe\n");
- return PTR_ERR(panel);
- }
-
- lvds_encoder->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);
-
- /* 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);
-
- platform_set_drvdata(pdev, lvds_encoder);
-
- return 0;
-}
-
-static int lvds_encoder_remove(struct platform_device *pdev)
-{
- struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev);
-
- drm_bridge_remove(&lvds_encoder->bridge);
-
- return 0;
-}
-
-static const struct of_device_id lvds_encoder_match[] = {
- { .compatible = "lvds-encoder" },
- { .compatible = "thine,thc63lvdm83d" },
- {},
-};
-MODULE_DEVICE_TABLE(of, lvds_encoder_match);
-
-static struct platform_driver lvds_encoder_driver = {
- .probe = lvds_encoder_probe,
- .remove = lvds_encoder_remove,
- .driver = {
- .name = "lvds-encoder",
- .of_match_table = lvds_encoder_match,
- },
-};
-module_platform_driver(lvds_encoder_driver);
-
-MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
-MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
-MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c
2019-11-07 20:10 ` [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c Fabrizio Castro
@ 2019-11-07 20:34 ` Laurent Pinchart
2019-11-08 9:22 ` Fabrizio Castro
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-07 20:34 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Fabrizio,
Thank you for the patch.
On Thu, Nov 07, 2019 at 08:10:58PM +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.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v2->v3:
> * No change
> v1->v2:
> * No change
> ---
> drivers/gpu/drm/bridge/Kconfig | 8 +-
> drivers/gpu/drm/bridge/Makefile | 2 +-
> drivers/gpu/drm/bridge/lvds-codec.c | 131 ++++++++++++++++++++++++++++
> drivers/gpu/drm/bridge/lvds-encoder.c | 155 ----------------------------------
> 4 files changed, 136 insertions(+), 160 deletions(-)
It would help if you added the -M1 option to git-format-patch to detect
the rename, the result would be easier to review.
> create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3436297..9e75ca4e 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 LVDS 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-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> new file mode 100644
> index 0000000..d57a8eb
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2019 Renesas Electronics Corporation
> + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + */
> +
> +#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>
> +
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +struct lvds_codec {
> + struct drm_bridge bridge;
> + struct drm_bridge *panel_bridge;
> + struct gpio_desc *powerdown_gpio;
> +};
> +
> +static int lvds_codec_attach(struct drm_bridge *bridge)
> +{
> + struct lvds_codec *lvds_codec = container_of(bridge,
> + struct lvds_codec, bridge);
> +
> + return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> + bridge);
> +}
> +
> +static void lvds_codec_enable(struct drm_bridge *bridge)
> +{
> + struct lvds_codec *lvds_codec = container_of(bridge,
> + struct lvds_codec, bridge);
> +
> + if (lvds_codec->powerdown_gpio)
> + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> +}
> +
> +static void lvds_codec_disable(struct drm_bridge *bridge)
> +{
> + struct lvds_codec *lvds_codec = container_of(bridge,
> + struct lvds_codec, bridge);
> +
> + if (lvds_codec->powerdown_gpio)
> + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> +}
> +
> +static struct drm_bridge_funcs funcs = {
> + .attach = lvds_codec_attach,
> + .enable = lvds_codec_enable,
> + .disable = lvds_codec_disable,
> +};
> +
> +static int lvds_codec_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *panel_node;
> + struct drm_panel *panel;
> + struct lvds_codec *lvds_codec;
> +
> + lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> + if (!lvds_codec)
> + return -ENOMEM;
> +
> + lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(lvds_codec->powerdown_gpio))
> + return PTR_ERR(lvds_codec->powerdown_gpio);
The driver had an error message here, any reason it got removed ?
> +
> + panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
> + if (!panel_node) {
> + dev_dbg(dev, "panel DT node not found\n");
> + return -ENXIO;
> + }
> +
> + panel = of_drm_find_panel(panel_node);
> + of_node_put(panel_node);
> + if (IS_ERR(panel)) {
> + dev_dbg(dev, "panel not found, deferring probe\n");
> + return PTR_ERR(panel);
> + }
> +
> + lvds_codec->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
This was devm_drm_panel_bridge_add_typed(), do you think there's a risk
of breaking userspace ? Of course as noted in the documentation of
devm_drm_panel_bridge_add_typed() the right solution is to fix panel
drivers, but I'm still slightly worried.
Actually, could you split this patch in two, with a patch that only
renames the driver (and the symbols internally) without any functional
change, and another patch that performs the modifications ? That would
be much easier to review and discuss.
> + 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,
> + * but we need a bridge attached to our of_node for our user
> + * to look up.
> + */
> + lvds_codec->bridge.of_node = dev->of_node;
> + lvds_codec->bridge.funcs = &funcs;
> + drm_bridge_add(&lvds_codec->bridge);
> +
> + platform_set_drvdata(pdev, lvds_codec);
> +
> + return 0;
> +}
> +
> +static int lvds_codec_remove(struct platform_device *pdev)
> +{
> + struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> +
> + drm_bridge_remove(&lvds_codec->bridge);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id lvds_codec_match[] = {
> + { .compatible = "lvds-encoder" },
> + { .compatible = "thine,thc63lvdm83d" },
> + { .compatible = "lvds-decoder" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> +
> +static struct platform_driver lvds_codec_driver = {
> + .probe = lvds_codec_probe,
> + .remove = lvds_codec_remove,
> + .driver = {
> + .name = "lvds-codec",
> + .of_match_table = lvds_codec_match,
> + },
> +};
> +module_platform_driver(lvds_codec_driver);
> +
> +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
Maybe "LVDS encoders and decoders" ?
> +MODULE_LICENSE("GPL");
[snip]
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* RE: [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c
2019-11-07 20:34 ` Laurent Pinchart
@ 2019-11-08 9:22 ` Fabrizio Castro
2019-11-08 9:39 ` Jacopo Mondi
2019-11-08 11:02 ` Laurent Pinchart
0 siblings, 2 replies; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-08 9:22 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hello Laurent,
Thank you for your feedback!
> From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 20:35
> Subject: Re: [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Thu, Nov 07, 2019 at 08:10:58PM +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.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v2->v3:
> > * No change
> > v1->v2:
> > * No change
> > ---
> > drivers/gpu/drm/bridge/Kconfig | 8 +-
> > drivers/gpu/drm/bridge/Makefile | 2 +-
> > drivers/gpu/drm/bridge/lvds-codec.c | 131 ++++++++++++++++++++++++++++
> > drivers/gpu/drm/bridge/lvds-encoder.c | 155 ----------------------------------
> > 4 files changed, 136 insertions(+), 160 deletions(-)
>
> It would help if you added the -M1 option to git-format-patch to detect
> the rename, the result would be easier to review.
Will do, thank you for the hint
>
> > create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> > delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 3436297..9e75ca4e 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 LVDS 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-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > new file mode 100644
> > index 0000000..d57a8eb
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > @@ -0,0 +1,131 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2019 Renesas Electronics Corporation
> > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > + */
> > +
> > +#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>
> > +
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_panel.h>
> > +
> > +struct lvds_codec {
> > + struct drm_bridge bridge;
> > + struct drm_bridge *panel_bridge;
> > + struct gpio_desc *powerdown_gpio;
> > +};
> > +
> > +static int lvds_codec_attach(struct drm_bridge *bridge)
> > +{
> > + struct lvds_codec *lvds_codec = container_of(bridge,
> > + struct lvds_codec, bridge);
> > +
> > + return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> > + bridge);
> > +}
> > +
> > +static void lvds_codec_enable(struct drm_bridge *bridge)
> > +{
> > + struct lvds_codec *lvds_codec = container_of(bridge,
> > + struct lvds_codec, bridge);
> > +
> > + if (lvds_codec->powerdown_gpio)
> > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> > +}
> > +
> > +static void lvds_codec_disable(struct drm_bridge *bridge)
> > +{
> > + struct lvds_codec *lvds_codec = container_of(bridge,
> > + struct lvds_codec, bridge);
> > +
> > + if (lvds_codec->powerdown_gpio)
> > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> > +}
> > +
> > +static struct drm_bridge_funcs funcs = {
> > + .attach = lvds_codec_attach,
> > + .enable = lvds_codec_enable,
> > + .disable = lvds_codec_disable,
> > +};
> > +
> > +static int lvds_codec_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *panel_node;
> > + struct drm_panel *panel;
> > + struct lvds_codec *lvds_codec;
> > +
> > + lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> > + if (!lvds_codec)
> > + return -ENOMEM;
> > +
> > + lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > + GPIOD_OUT_HIGH);
> > + if (IS_ERR(lvds_codec->powerdown_gpio))
> > + return PTR_ERR(lvds_codec->powerdown_gpio);
>
> The driver had an error message here, any reason it got removed ?
I am quoting from https://www.spinics.net/lists/devicetree/msg318602.html :
"I know it was there already, but this seems a bit unusual for the
minimal gain of having a printout in the very unlikely case the
gpiod_get() operations fails. I would just return PTR_ERR()."
I am OK with reinstating it, just let me know what you want me to do here.
>
> > +
> > + panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
> > + if (!panel_node) {
> > + dev_dbg(dev, "panel DT node not found\n");
> > + return -ENXIO;
> > + }
> > +
> > + panel = of_drm_find_panel(panel_node);
> > + of_node_put(panel_node);
> > + if (IS_ERR(panel)) {
> > + dev_dbg(dev, "panel not found, deferring probe\n");
> > + return PTR_ERR(panel);
> > + }
> > +
> > + lvds_codec->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
>
> This was devm_drm_panel_bridge_add_typed(), do you think there's a risk
> of breaking userspace ? Of course as noted in the documentation of
> devm_drm_panel_bridge_add_typed() the right solution is to fix panel
> drivers, but I'm still slightly worried.
Things break when the panel doesn't define connector_type, leading to the below
check from devm_drm_panel_bridge_add:
if (WARN_ON(panel->connector_type == DRM_MODE_CONNECTOR_Unknown))
return NULL;
Please advise on the best course of action here.
>
> Actually, could you split this patch in two, with a patch that only
> renames the driver (and the symbols internally) without any functional
> change, and another patch that performs the modifications ? That would
> be much easier to review and discuss.
Will do
>
> > + 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,
> > + * but we need a bridge attached to our of_node for our user
> > + * to look up.
> > + */
> > + lvds_codec->bridge.of_node = dev->of_node;
> > + lvds_codec->bridge.funcs = &funcs;
> > + drm_bridge_add(&lvds_codec->bridge);
> > +
> > + platform_set_drvdata(pdev, lvds_codec);
> > +
> > + return 0;
> > +}
> > +
> > +static int lvds_codec_remove(struct platform_device *pdev)
> > +{
> > + struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> > +
> > + drm_bridge_remove(&lvds_codec->bridge);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id lvds_codec_match[] = {
> > + { .compatible = "lvds-encoder" },
> > + { .compatible = "thine,thc63lvdm83d" },
> > + { .compatible = "lvds-decoder" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> > +
> > +static struct platform_driver lvds_codec_driver = {
> > + .probe = lvds_codec_probe,
> > + .remove = lvds_codec_remove,
> > + .driver = {
> > + .name = "lvds-codec",
> > + .of_match_table = lvds_codec_match,
> > + },
> > +};
> > +module_platform_driver(lvds_codec_driver);
> > +
> > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
>
> Maybe "LVDS encoders and decoders" ?
>
> > +MODULE_LICENSE("GPL");
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c
2019-11-08 9:22 ` Fabrizio Castro
@ 2019-11-08 9:39 ` Jacopo Mondi
2019-11-08 11:06 ` Laurent Pinchart
2019-11-08 11:02 ` Laurent Pinchart
1 sibling, 1 reply; 28+ messages in thread
From: Jacopo Mondi @ 2019-11-08 9:39 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Laurent Pinchart, Neil Armstrong, David Airlie, Daniel Vetter,
Rob Herring, Mark Rutland, Andrzej Hajda, Simon Horman,
Geert Uytterhoeven, Magnus Damm, Peter Rosin, dri-devel,
devicetree, linux-renesas-soc, Chris Paterson, Biju Das,
Kieran Bingham, Jacopo Mondi
[-- Attachment #1: Type: text/plain, Size: 9956 bytes --]
Hello,
On Fri, Nov 08, 2019 at 09:22:56AM +0000, Fabrizio Castro wrote:
> Hello Laurent,
>
> Thank you for your feedback!
>
> > From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> > Sent: 07 November 2019 20:35
> > Subject: Re: [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c
> >
> > Hi Fabrizio,
> >
> > Thank you for the patch.
> >
> > On Thu, Nov 07, 2019 at 08:10:58PM +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.
> > >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > ---
> > > v2->v3:
> > > * No change
> > > v1->v2:
> > > * No change
> > > ---
> > > drivers/gpu/drm/bridge/Kconfig | 8 +-
> > > drivers/gpu/drm/bridge/Makefile | 2 +-
> > > drivers/gpu/drm/bridge/lvds-codec.c | 131 ++++++++++++++++++++++++++++
> > > drivers/gpu/drm/bridge/lvds-encoder.c | 155 ----------------------------------
> > > 4 files changed, 136 insertions(+), 160 deletions(-)
> >
> > It would help if you added the -M1 option to git-format-patch to detect
> > the rename, the result would be easier to review.
>
> Will do, thank you for the hint
>
> >
> > > create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> > > delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> > >
> > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > index 3436297..9e75ca4e 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 LVDS 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-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > new file mode 100644
> > > index 0000000..d57a8eb
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > @@ -0,0 +1,131 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Copyright (C) 2019 Renesas Electronics Corporation
> > > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > + */
> > > +
> > > +#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>
> > > +
> > > +#include <drm/drm_bridge.h>
> > > +#include <drm/drm_panel.h>
> > > +
> > > +struct lvds_codec {
> > > + struct drm_bridge bridge;
> > > + struct drm_bridge *panel_bridge;
> > > + struct gpio_desc *powerdown_gpio;
> > > +};
> > > +
> > > +static int lvds_codec_attach(struct drm_bridge *bridge)
> > > +{
> > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > + struct lvds_codec, bridge);
> > > +
> > > + return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> > > + bridge);
> > > +}
> > > +
> > > +static void lvds_codec_enable(struct drm_bridge *bridge)
> > > +{
> > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > + struct lvds_codec, bridge);
> > > +
> > > + if (lvds_codec->powerdown_gpio)
> > > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> > > +}
> > > +
> > > +static void lvds_codec_disable(struct drm_bridge *bridge)
> > > +{
> > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > + struct lvds_codec, bridge);
> > > +
> > > + if (lvds_codec->powerdown_gpio)
> > > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> > > +}
> > > +
> > > +static struct drm_bridge_funcs funcs = {
> > > + .attach = lvds_codec_attach,
> > > + .enable = lvds_codec_enable,
> > > + .disable = lvds_codec_disable,
> > > +};
> > > +
> > > +static int lvds_codec_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct device_node *panel_node;
> > > + struct drm_panel *panel;
> > > + struct lvds_codec *lvds_codec;
> > > +
> > > + lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> > > + if (!lvds_codec)
> > > + return -ENOMEM;
> > > +
> > > + lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > + GPIOD_OUT_HIGH);
> > > + if (IS_ERR(lvds_codec->powerdown_gpio))
> > > + return PTR_ERR(lvds_codec->powerdown_gpio);
> >
> > The driver had an error message here, any reason it got removed ?
>
> I am quoting from https://www.spinics.net/lists/devicetree/msg318602.html :
> "I know it was there already, but this seems a bit unusual for the
> minimal gain of having a printout in the very unlikely case the
> gpiod_get() operations fails. I would just return PTR_ERR()."
>
> I am OK with reinstating it, just let me know what you want me to do here.
>
Yeah, I suggested that as it seemed to me quite unusual pattern for the
minimal gain of having an error message in an unlikely case. Sorry Fab
for the double effort if Laurent wants it back again.
> >
> > > +
> > > + panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
> > > + if (!panel_node) {
> > > + dev_dbg(dev, "panel DT node not found\n");
> > > + return -ENXIO;
> > > + }
> > > +
> > > + panel = of_drm_find_panel(panel_node);
> > > + of_node_put(panel_node);
> > > + if (IS_ERR(panel)) {
> > > + dev_dbg(dev, "panel not found, deferring probe\n");
> > > + return PTR_ERR(panel);
> > > + }
> > > +
> > > + lvds_codec->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> >
> > This was devm_drm_panel_bridge_add_typed(), do you think there's a risk
> > of breaking userspace ? Of course as noted in the documentation of
> > devm_drm_panel_bridge_add_typed() the right solution is to fix panel
> > drivers, but I'm still slightly worried.
>
> Things break when the panel doesn't define connector_type, leading to the below
> check from devm_drm_panel_bridge_add:
> if (WARN_ON(panel->connector_type == DRM_MODE_CONNECTOR_Unknown))
> return NULL;
>
> Please advise on the best course of action here.
I pointed out that function was described as deprecated and probably
fixing the panel driver would be best. Why are you concerned about
userspace ? is the panel driver that should correctly report its
connector type, isn't it ? In case it's not, sorry again Fab for the
double effort.
>
> >
> > Actually, could you split this patch in two, with a patch that only
> > renames the driver (and the symbols internally) without any functional
> > change, and another patch that performs the modifications ? That would
> > be much easier to review and discuss.
This is more work for something that could be simply addressed by the
reviewer by passing -M10 to git show. For such a simple driver isn't
this fine the way it is ?
>
> Will do
>
> >
> > > + 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,
> > > + * but we need a bridge attached to our of_node for our user
> > > + * to look up.
> > > + */
> > > + lvds_codec->bridge.of_node = dev->of_node;
> > > + lvds_codec->bridge.funcs = &funcs;
> > > + drm_bridge_add(&lvds_codec->bridge);
> > > +
> > > + platform_set_drvdata(pdev, lvds_codec);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int lvds_codec_remove(struct platform_device *pdev)
> > > +{
> > > + struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> > > +
> > > + drm_bridge_remove(&lvds_codec->bridge);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id lvds_codec_match[] = {
> > > + { .compatible = "lvds-encoder" },
> > > + { .compatible = "thine,thc63lvdm83d" },
> > > + { .compatible = "lvds-decoder" },
> > > + {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> > > +
> > > +static struct platform_driver lvds_codec_driver = {
> > > + .probe = lvds_codec_probe,
> > > + .remove = lvds_codec_remove,
> > > + .driver = {
> > > + .name = "lvds-codec",
> > > + .of_match_table = lvds_codec_match,
> > > + },
> > > +};
> > > +module_platform_driver(lvds_codec_driver);
> > > +
> > > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > > +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> >
> > Maybe "LVDS encoders and decoders" ?
> >
> > > +MODULE_LICENSE("GPL");
> >
> > [snip]
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c
2019-11-08 9:39 ` Jacopo Mondi
@ 2019-11-08 11:06 ` Laurent Pinchart
2019-11-08 11:37 ` Jacopo Mondi
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-08 11:06 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Fabrizio Castro, Neil Armstrong, David Airlie, Daniel Vetter,
Rob Herring, Mark Rutland, Andrzej Hajda, Simon Horman,
Geert Uytterhoeven, Magnus Damm, Peter Rosin, dri-devel,
devicetree, linux-renesas-soc, Chris Paterson, Biju Das,
Kieran Bingham, Jacopo Mondi
Hello Jacopo,
On Fri, Nov 08, 2019 at 10:39:27AM +0100, Jacopo Mondi wrote:
> On Fri, Nov 08, 2019 at 09:22:56AM +0000, Fabrizio Castro wrote:
> > On 07 November 2019 20:35 Laurent Pinchart wrote:
> > > On Thu, Nov 07, 2019 at 08:10:58PM +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.
> > > >
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > >
> > > > ---
> > > > v2->v3:
> > > > * No change
> > > > v1->v2:
> > > > * No change
> > > > ---
> > > > drivers/gpu/drm/bridge/Kconfig | 8 +-
> > > > drivers/gpu/drm/bridge/Makefile | 2 +-
> > > > drivers/gpu/drm/bridge/lvds-codec.c | 131 ++++++++++++++++++++++++++++
> > > > drivers/gpu/drm/bridge/lvds-encoder.c | 155 ----------------------------------
> > > > 4 files changed, 136 insertions(+), 160 deletions(-)
> > >
> > > It would help if you added the -M1 option to git-format-patch to detect
> > > the rename, the result would be easier to review.
> >
> > Will do, thank you for the hint
> >
> > > > create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> > > > delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > > index 3436297..9e75ca4e 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 LVDS 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-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > > new file mode 100644
> > > > index 0000000..d57a8eb
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > > @@ -0,0 +1,131 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + * Copyright (C) 2019 Renesas Electronics Corporation
> > > > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > + */
> > > > +
> > > > +#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>
> > > > +
> > > > +#include <drm/drm_bridge.h>
> > > > +#include <drm/drm_panel.h>
> > > > +
> > > > +struct lvds_codec {
> > > > + struct drm_bridge bridge;
> > > > + struct drm_bridge *panel_bridge;
> > > > + struct gpio_desc *powerdown_gpio;
> > > > +};
> > > > +
> > > > +static int lvds_codec_attach(struct drm_bridge *bridge)
> > > > +{
> > > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > > + struct lvds_codec, bridge);
> > > > +
> > > > + return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> > > > + bridge);
> > > > +}
> > > > +
> > > > +static void lvds_codec_enable(struct drm_bridge *bridge)
> > > > +{
> > > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > > + struct lvds_codec, bridge);
> > > > +
> > > > + if (lvds_codec->powerdown_gpio)
> > > > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> > > > +}
> > > > +
> > > > +static void lvds_codec_disable(struct drm_bridge *bridge)
> > > > +{
> > > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > > + struct lvds_codec, bridge);
> > > > +
> > > > + if (lvds_codec->powerdown_gpio)
> > > > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> > > > +}
> > > > +
> > > > +static struct drm_bridge_funcs funcs = {
> > > > + .attach = lvds_codec_attach,
> > > > + .enable = lvds_codec_enable,
> > > > + .disable = lvds_codec_disable,
> > > > +};
> > > > +
> > > > +static int lvds_codec_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct device *dev = &pdev->dev;
> > > > + struct device_node *panel_node;
> > > > + struct drm_panel *panel;
> > > > + struct lvds_codec *lvds_codec;
> > > > +
> > > > + lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> > > > + if (!lvds_codec)
> > > > + return -ENOMEM;
> > > > +
> > > > + lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > > + GPIOD_OUT_HIGH);
> > > > + if (IS_ERR(lvds_codec->powerdown_gpio))
> > > > + return PTR_ERR(lvds_codec->powerdown_gpio);
> > >
> > > The driver had an error message here, any reason it got removed ?
> >
> > I am quoting from https://www.spinics.net/lists/devicetree/msg318602.html :
> > "I know it was there already, but this seems a bit unusual for the
> > minimal gain of having a printout in the very unlikely case the
> > gpiod_get() operations fails. I would just return PTR_ERR()."
> >
> > I am OK with reinstating it, just let me know what you want me to do here.
>
> Yeah, I suggested that as it seemed to me quite unusual pattern for the
> minimal gain of having an error message in an unlikely case. Sorry Fab
> for the double effort if Laurent wants it back again.
>
> > > > +
> > > > + panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
> > > > + if (!panel_node) {
> > > > + dev_dbg(dev, "panel DT node not found\n");
> > > > + return -ENXIO;
> > > > + }
> > > > +
> > > > + panel = of_drm_find_panel(panel_node);
> > > > + of_node_put(panel_node);
> > > > + if (IS_ERR(panel)) {
> > > > + dev_dbg(dev, "panel not found, deferring probe\n");
> > > > + return PTR_ERR(panel);
> > > > + }
> > > > +
> > > > + lvds_codec->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > >
> > > This was devm_drm_panel_bridge_add_typed(), do you think there's a risk
> > > of breaking userspace ? Of course as noted in the documentation of
> > > devm_drm_panel_bridge_add_typed() the right solution is to fix panel
> > > drivers, but I'm still slightly worried.
> >
> > Things break when the panel doesn't define connector_type, leading to the below
> > check from devm_drm_panel_bridge_add:
> > if (WARN_ON(panel->connector_type == DRM_MODE_CONNECTOR_Unknown))
> > return NULL;
> >
> > Please advise on the best course of action here.
>
> I pointed out that function was described as deprecated and probably
> fixing the panel driver would be best. Why are you concerned about
> userspace ? is the panel driver that should correctly report its
> connector type, isn't it ? In case it's not, sorry again Fab for the
> double effort.
I'm concerned that this change may turn a working system in a
non-working system. The issue has to be fixed in panel drivers of
course, but switching from devm_drm_panel_bridge_add_typed() to
devm_drm_panel_bridge_add() should only be done once all the drivers
that are used with lvds-encoder behave properly.
> > > Actually, could you split this patch in two, with a patch that only
> > > renames the driver (and the symbols internally) without any functional
> > > change, and another patch that performs the modifications ? That would
> > > be much easier to review and discuss.
>
> This is more work for something that could be simply addressed by the
> reviewer by passing -M10 to git show. For such a simple driver isn't
> this fine the way it is ?
Don't make it difficult for the reviewer. I've reviewed this patch in my
e-mail client, not in git. The patch itself should be generated with
-M10, but in any case, such renames should not be bundled with other
changes. One logical change by patch is the rule, and we can sometimes
bundle a semi-unrelated minor change (such as a typo or indentation
fix), but certainly not a potentially dangerous functional change that
needs to be carefully reviewed.
> > Will do
> >
> > > > + 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,
> > > > + * but we need a bridge attached to our of_node for our user
> > > > + * to look up.
> > > > + */
> > > > + lvds_codec->bridge.of_node = dev->of_node;
> > > > + lvds_codec->bridge.funcs = &funcs;
> > > > + drm_bridge_add(&lvds_codec->bridge);
> > > > +
> > > > + platform_set_drvdata(pdev, lvds_codec);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int lvds_codec_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> > > > +
> > > > + drm_bridge_remove(&lvds_codec->bridge);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static const struct of_device_id lvds_codec_match[] = {
> > > > + { .compatible = "lvds-encoder" },
> > > > + { .compatible = "thine,thc63lvdm83d" },
> > > > + { .compatible = "lvds-decoder" },
> > > > + {},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> > > > +
> > > > +static struct platform_driver lvds_codec_driver = {
> > > > + .probe = lvds_codec_probe,
> > > > + .remove = lvds_codec_remove,
> > > > + .driver = {
> > > > + .name = "lvds-codec",
> > > > + .of_match_table = lvds_codec_match,
> > > > + },
> > > > +};
> > > > +module_platform_driver(lvds_codec_driver);
> > > > +
> > > > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > > > +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> > >
> > > Maybe "LVDS encoders and decoders" ?
> > >
> > > > +MODULE_LICENSE("GPL");
> > >
> > > [snip]
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c
2019-11-08 11:06 ` Laurent Pinchart
@ 2019-11-08 11:37 ` Jacopo Mondi
2019-11-08 11:40 ` Laurent Pinchart
0 siblings, 1 reply; 28+ messages in thread
From: Jacopo Mondi @ 2019-11-08 11:37 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Fabrizio Castro, Neil Armstrong, David Airlie, Daniel Vetter,
Rob Herring, Mark Rutland, Andrzej Hajda, Simon Horman,
Geert Uytterhoeven, Magnus Damm, Peter Rosin, dri-devel,
devicetree, linux-renesas-soc, Chris Paterson, Biju Das,
Kieran Bingham, Jacopo Mondi
[-- Attachment #1: Type: text/plain, Size: 12251 bytes --]
Hi Laurent,
On Fri, Nov 08, 2019 at 01:06:58PM +0200, Laurent Pinchart wrote:
> Hello Jacopo,
>
> On Fri, Nov 08, 2019 at 10:39:27AM +0100, Jacopo Mondi wrote:
> > On Fri, Nov 08, 2019 at 09:22:56AM +0000, Fabrizio Castro wrote:
> > > On 07 November 2019 20:35 Laurent Pinchart wrote:
> > > > On Thu, Nov 07, 2019 at 08:10:58PM +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.
> > > > >
> > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > >
> > > > > ---
> > > > > v2->v3:
> > > > > * No change
> > > > > v1->v2:
> > > > > * No change
> > > > > ---
> > > > > drivers/gpu/drm/bridge/Kconfig | 8 +-
> > > > > drivers/gpu/drm/bridge/Makefile | 2 +-
> > > > > drivers/gpu/drm/bridge/lvds-codec.c | 131 ++++++++++++++++++++++++++++
> > > > > drivers/gpu/drm/bridge/lvds-encoder.c | 155 ----------------------------------
> > > > > 4 files changed, 136 insertions(+), 160 deletions(-)
> > > >
> > > > It would help if you added the -M1 option to git-format-patch to detect
> > > > the rename, the result would be easier to review.
> > >
> > > Will do, thank you for the hint
> > >
> > > > > create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> > > > > delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> > > > >
> > > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > > > index 3436297..9e75ca4e 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 LVDS 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-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > > > new file mode 100644
> > > > > index 0000000..d57a8eb
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > > > @@ -0,0 +1,131 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > +/*
> > > > > + * Copyright (C) 2019 Renesas Electronics Corporation
> > > > > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > + */
> > > > > +
> > > > > +#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>
> > > > > +
> > > > > +#include <drm/drm_bridge.h>
> > > > > +#include <drm/drm_panel.h>
> > > > > +
> > > > > +struct lvds_codec {
> > > > > + struct drm_bridge bridge;
> > > > > + struct drm_bridge *panel_bridge;
> > > > > + struct gpio_desc *powerdown_gpio;
> > > > > +};
> > > > > +
> > > > > +static int lvds_codec_attach(struct drm_bridge *bridge)
> > > > > +{
> > > > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > > > + struct lvds_codec, bridge);
> > > > > +
> > > > > + return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> > > > > + bridge);
> > > > > +}
> > > > > +
> > > > > +static void lvds_codec_enable(struct drm_bridge *bridge)
> > > > > +{
> > > > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > > > + struct lvds_codec, bridge);
> > > > > +
> > > > > + if (lvds_codec->powerdown_gpio)
> > > > > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> > > > > +}
> > > > > +
> > > > > +static void lvds_codec_disable(struct drm_bridge *bridge)
> > > > > +{
> > > > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > > > + struct lvds_codec, bridge);
> > > > > +
> > > > > + if (lvds_codec->powerdown_gpio)
> > > > > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> > > > > +}
> > > > > +
> > > > > +static struct drm_bridge_funcs funcs = {
> > > > > + .attach = lvds_codec_attach,
> > > > > + .enable = lvds_codec_enable,
> > > > > + .disable = lvds_codec_disable,
> > > > > +};
> > > > > +
> > > > > +static int lvds_codec_probe(struct platform_device *pdev)
> > > > > +{
> > > > > + struct device *dev = &pdev->dev;
> > > > > + struct device_node *panel_node;
> > > > > + struct drm_panel *panel;
> > > > > + struct lvds_codec *lvds_codec;
> > > > > +
> > > > > + lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> > > > > + if (!lvds_codec)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > > > + GPIOD_OUT_HIGH);
> > > > > + if (IS_ERR(lvds_codec->powerdown_gpio))
> > > > > + return PTR_ERR(lvds_codec->powerdown_gpio);
> > > >
> > > > The driver had an error message here, any reason it got removed ?
> > >
> > > I am quoting from https://www.spinics.net/lists/devicetree/msg318602.html :
> > > "I know it was there already, but this seems a bit unusual for the
> > > minimal gain of having a printout in the very unlikely case the
> > > gpiod_get() operations fails. I would just return PTR_ERR()."
> > >
> > > I am OK with reinstating it, just let me know what you want me to do here.
> >
> > Yeah, I suggested that as it seemed to me quite unusual pattern for the
> > minimal gain of having an error message in an unlikely case. Sorry Fab
> > for the double effort if Laurent wants it back again.
> >
> > > > > +
> > > > > + panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
> > > > > + if (!panel_node) {
> > > > > + dev_dbg(dev, "panel DT node not found\n");
> > > > > + return -ENXIO;
> > > > > + }
> > > > > +
> > > > > + panel = of_drm_find_panel(panel_node);
> > > > > + of_node_put(panel_node);
> > > > > + if (IS_ERR(panel)) {
> > > > > + dev_dbg(dev, "panel not found, deferring probe\n");
> > > > > + return PTR_ERR(panel);
> > > > > + }
> > > > > +
> > > > > + lvds_codec->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > >
> > > > This was devm_drm_panel_bridge_add_typed(), do you think there's a risk
> > > > of breaking userspace ? Of course as noted in the documentation of
> > > > devm_drm_panel_bridge_add_typed() the right solution is to fix panel
> > > > drivers, but I'm still slightly worried.
> > >
> > > Things break when the panel doesn't define connector_type, leading to the below
> > > check from devm_drm_panel_bridge_add:
> > > if (WARN_ON(panel->connector_type == DRM_MODE_CONNECTOR_Unknown))
> > > return NULL;
> > >
> > > Please advise on the best course of action here.
> >
> > I pointed out that function was described as deprecated and probably
> > fixing the panel driver would be best. Why are you concerned about
> > userspace ? is the panel driver that should correctly report its
> > connector type, isn't it ? In case it's not, sorry again Fab for the
> > double effort.
>
> I'm concerned that this change may turn a working system in a
> non-working system. The issue has to be fixed in panel drivers of
> course, but switching from devm_drm_panel_bridge_add_typed() to
> devm_drm_panel_bridge_add() should only be done once all the drivers
> that are used with lvds-encoder behave properly.
>
I see.. It's probably most safer then to keep the _typed() version.
However, I understand not breaking working system is of course
desirable, but this would not be a userspace breakage, but a driver
change that requires other associated drivers to be updated
accordingly, like it happens at every release. I however fear a
change like this if blindly ported to a BSP would break it, and yes,
doing that in this single commit won't help identifying where the
issue comes from. Sorry Fabrizio for the bad suggestion, you should
keep the _typed version and eventually switch to the new one in a
separate commit if you feel like to.
> > > > Actually, could you split this patch in two, with a patch that only
> > > > renames the driver (and the symbols internally) without any functional
> > > > change, and another patch that performs the modifications ? That would
> > > > be much easier to review and discuss.
> >
> > This is more work for something that could be simply addressed by the
> > reviewer by passing -M10 to git show. For such a simple driver isn't
> > this fine the way it is ?
>
> Don't make it difficult for the reviewer. I've reviewed this patch in my
> e-mail client, not in git. The patch itself should be generated with
> -M10, but in any case, such renames should not be bundled with other
> changes. One logical change by patch is the rule, and we can sometimes
> bundle a semi-unrelated minor change (such as a typo or indentation
> fix), but certainly not a potentially dangerous functional change that
> needs to be carefully reviewed.
Ack
Thanks
j
>
> > > Will do
> > >
> > > > > + 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,
> > > > > + * but we need a bridge attached to our of_node for our user
> > > > > + * to look up.
> > > > > + */
> > > > > + lvds_codec->bridge.of_node = dev->of_node;
> > > > > + lvds_codec->bridge.funcs = &funcs;
> > > > > + drm_bridge_add(&lvds_codec->bridge);
> > > > > +
> > > > > + platform_set_drvdata(pdev, lvds_codec);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int lvds_codec_remove(struct platform_device *pdev)
> > > > > +{
> > > > > + struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> > > > > +
> > > > > + drm_bridge_remove(&lvds_codec->bridge);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct of_device_id lvds_codec_match[] = {
> > > > > + { .compatible = "lvds-encoder" },
> > > > > + { .compatible = "thine,thc63lvdm83d" },
> > > > > + { .compatible = "lvds-decoder" },
> > > > > + {},
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> > > > > +
> > > > > +static struct platform_driver lvds_codec_driver = {
> > > > > + .probe = lvds_codec_probe,
> > > > > + .remove = lvds_codec_remove,
> > > > > + .driver = {
> > > > > + .name = "lvds-codec",
> > > > > + .of_match_table = lvds_codec_match,
> > > > > + },
> > > > > +};
> > > > > +module_platform_driver(lvds_codec_driver);
> > > > > +
> > > > > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > > > > +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> > > >
> > > > Maybe "LVDS encoders and decoders" ?
> > > >
> > > > > +MODULE_LICENSE("GPL");
> > > >
> > > > [snip]
>
> --
> Regards,
>
> Laurent Pinchart
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c
2019-11-08 11:37 ` Jacopo Mondi
@ 2019-11-08 11:40 ` Laurent Pinchart
0 siblings, 0 replies; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-08 11:40 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Fabrizio Castro, Neil Armstrong, David Airlie, Daniel Vetter,
Rob Herring, Mark Rutland, Andrzej Hajda, Simon Horman,
Geert Uytterhoeven, Magnus Damm, Peter Rosin, dri-devel,
devicetree, linux-renesas-soc, Chris Paterson, Biju Das,
Kieran Bingham, Jacopo Mondi
Hi Jacopo,
On Fri, Nov 08, 2019 at 12:37:37PM +0100, Jacopo Mondi wrote:
> On Fri, Nov 08, 2019 at 01:06:58PM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 08, 2019 at 10:39:27AM +0100, Jacopo Mondi wrote:
> > > On Fri, Nov 08, 2019 at 09:22:56AM +0000, Fabrizio Castro wrote:
> > > > On 07 November 2019 20:35 Laurent Pinchart wrote:
> > > > > On Thu, Nov 07, 2019 at 08:10:58PM +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.
> > > > > >
> > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > > >
> > > > > > ---
> > > > > > v2->v3:
> > > > > > * No change
> > > > > > v1->v2:
> > > > > > * No change
> > > > > > ---
> > > > > > drivers/gpu/drm/bridge/Kconfig | 8 +-
> > > > > > drivers/gpu/drm/bridge/Makefile | 2 +-
> > > > > > drivers/gpu/drm/bridge/lvds-codec.c | 131 ++++++++++++++++++++++++++++
> > > > > > drivers/gpu/drm/bridge/lvds-encoder.c | 155 ----------------------------------
> > > > > > 4 files changed, 136 insertions(+), 160 deletions(-)
> > > > >
> > > > > It would help if you added the -M1 option to git-format-patch to detect
> > > > > the rename, the result would be easier to review.
> > > >
> > > > Will do, thank you for the hint
> > > >
> > > > > > create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> > > > > > delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > > > > index 3436297..9e75ca4e 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 LVDS 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-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > > > > new file mode 100644
> > > > > > index 0000000..d57a8eb
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > > > > @@ -0,0 +1,131 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > +/*
> > > > > > + * Copyright (C) 2019 Renesas Electronics Corporation
> > > > > > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > + */
> > > > > > +
> > > > > > +#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>
> > > > > > +
> > > > > > +#include <drm/drm_bridge.h>
> > > > > > +#include <drm/drm_panel.h>
> > > > > > +
> > > > > > +struct lvds_codec {
> > > > > > + struct drm_bridge bridge;
> > > > > > + struct drm_bridge *panel_bridge;
> > > > > > + struct gpio_desc *powerdown_gpio;
> > > > > > +};
> > > > > > +
> > > > > > +static int lvds_codec_attach(struct drm_bridge *bridge)
> > > > > > +{
> > > > > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > > > > + struct lvds_codec, bridge);
> > > > > > +
> > > > > > + return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> > > > > > + bridge);
> > > > > > +}
> > > > > > +
> > > > > > +static void lvds_codec_enable(struct drm_bridge *bridge)
> > > > > > +{
> > > > > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > > > > + struct lvds_codec, bridge);
> > > > > > +
> > > > > > + if (lvds_codec->powerdown_gpio)
> > > > > > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> > > > > > +}
> > > > > > +
> > > > > > +static void lvds_codec_disable(struct drm_bridge *bridge)
> > > > > > +{
> > > > > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > > > > + struct lvds_codec, bridge);
> > > > > > +
> > > > > > + if (lvds_codec->powerdown_gpio)
> > > > > > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> > > > > > +}
> > > > > > +
> > > > > > +static struct drm_bridge_funcs funcs = {
> > > > > > + .attach = lvds_codec_attach,
> > > > > > + .enable = lvds_codec_enable,
> > > > > > + .disable = lvds_codec_disable,
> > > > > > +};
> > > > > > +
> > > > > > +static int lvds_codec_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > + struct device *dev = &pdev->dev;
> > > > > > + struct device_node *panel_node;
> > > > > > + struct drm_panel *panel;
> > > > > > + struct lvds_codec *lvds_codec;
> > > > > > +
> > > > > > + lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> > > > > > + if (!lvds_codec)
> > > > > > + return -ENOMEM;
> > > > > > +
> > > > > > + lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > > > > + GPIOD_OUT_HIGH);
> > > > > > + if (IS_ERR(lvds_codec->powerdown_gpio))
> > > > > > + return PTR_ERR(lvds_codec->powerdown_gpio);
> > > > >
> > > > > The driver had an error message here, any reason it got removed ?
> > > >
> > > > I am quoting from https://www.spinics.net/lists/devicetree/msg318602.html :
> > > > "I know it was there already, but this seems a bit unusual for the
> > > > minimal gain of having a printout in the very unlikely case the
> > > > gpiod_get() operations fails. I would just return PTR_ERR()."
> > > >
> > > > I am OK with reinstating it, just let me know what you want me to do here.
> > >
> > > Yeah, I suggested that as it seemed to me quite unusual pattern for the
> > > minimal gain of having an error message in an unlikely case. Sorry Fab
> > > for the double effort if Laurent wants it back again.
> > >
> > > > > > +
> > > > > > + panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
> > > > > > + if (!panel_node) {
> > > > > > + dev_dbg(dev, "panel DT node not found\n");
> > > > > > + return -ENXIO;
> > > > > > + }
> > > > > > +
> > > > > > + panel = of_drm_find_panel(panel_node);
> > > > > > + of_node_put(panel_node);
> > > > > > + if (IS_ERR(panel)) {
> > > > > > + dev_dbg(dev, "panel not found, deferring probe\n");
> > > > > > + return PTR_ERR(panel);
> > > > > > + }
> > > > > > +
> > > > > > + lvds_codec->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > > >
> > > > > This was devm_drm_panel_bridge_add_typed(), do you think there's a risk
> > > > > of breaking userspace ? Of course as noted in the documentation of
> > > > > devm_drm_panel_bridge_add_typed() the right solution is to fix panel
> > > > > drivers, but I'm still slightly worried.
> > > >
> > > > Things break when the panel doesn't define connector_type, leading to the below
> > > > check from devm_drm_panel_bridge_add:
> > > > if (WARN_ON(panel->connector_type == DRM_MODE_CONNECTOR_Unknown))
> > > > return NULL;
> > > >
> > > > Please advise on the best course of action here.
> > >
> > > I pointed out that function was described as deprecated and probably
> > > fixing the panel driver would be best. Why are you concerned about
> > > userspace ? is the panel driver that should correctly report its
> > > connector type, isn't it ? In case it's not, sorry again Fab for the
> > > double effort.
> >
> > I'm concerned that this change may turn a working system in a
> > non-working system. The issue has to be fixed in panel drivers of
> > course, but switching from devm_drm_panel_bridge_add_typed() to
> > devm_drm_panel_bridge_add() should only be done once all the drivers
> > that are used with lvds-encoder behave properly.
> >
>
> I see.. It's probably most safer then to keep the _typed() version.
>
> However, I understand not breaking working system is of course
> desirable, but this would not be a userspace breakage, but a driver
> change that requires other associated drivers to be updated
> accordingly, like it happens at every release.
Sure, but we're then expected to do this in lock-step, not push the
first change, wait for someone to complain, and then fix other drivers
:-) If we're confident that panel drivers are already behaving properly
(which, in this case, more or less means that all LVDS panel drivers
should report that they are LVDS panel drivers), then we can already
move forward, but in any case, in a separate patch.
> I however fear a
> change like this if blindly ported to a BSP would break it, and yes,
> doing that in this single commit won't help identifying where the
> issue comes from. Sorry Fabrizio for the bad suggestion, you should
> keep the _typed version and eventually switch to the new one in a
> separate commit if you feel like to.
>
> > > > > Actually, could you split this patch in two, with a patch that only
> > > > > renames the driver (and the symbols internally) without any functional
> > > > > change, and another patch that performs the modifications ? That would
> > > > > be much easier to review and discuss.
> > >
> > > This is more work for something that could be simply addressed by the
> > > reviewer by passing -M10 to git show. For such a simple driver isn't
> > > this fine the way it is ?
> >
> > Don't make it difficult for the reviewer. I've reviewed this patch in my
> > e-mail client, not in git. The patch itself should be generated with
> > -M10, but in any case, such renames should not be bundled with other
> > changes. One logical change by patch is the rule, and we can sometimes
> > bundle a semi-unrelated minor change (such as a typo or indentation
> > fix), but certainly not a potentially dangerous functional change that
> > needs to be carefully reviewed.
>
> Ack
>
> > > > Will do
> > > >
> > > > > > + 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,
> > > > > > + * but we need a bridge attached to our of_node for our user
> > > > > > + * to look up.
> > > > > > + */
> > > > > > + lvds_codec->bridge.of_node = dev->of_node;
> > > > > > + lvds_codec->bridge.funcs = &funcs;
> > > > > > + drm_bridge_add(&lvds_codec->bridge);
> > > > > > +
> > > > > > + platform_set_drvdata(pdev, lvds_codec);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int lvds_codec_remove(struct platform_device *pdev)
> > > > > > +{
> > > > > > + struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> > > > > > +
> > > > > > + drm_bridge_remove(&lvds_codec->bridge);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct of_device_id lvds_codec_match[] = {
> > > > > > + { .compatible = "lvds-encoder" },
> > > > > > + { .compatible = "thine,thc63lvdm83d" },
> > > > > > + { .compatible = "lvds-decoder" },
> > > > > > + {},
> > > > > > +};
> > > > > > +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> > > > > > +
> > > > > > +static struct platform_driver lvds_codec_driver = {
> > > > > > + .probe = lvds_codec_probe,
> > > > > > + .remove = lvds_codec_remove,
> > > > > > + .driver = {
> > > > > > + .name = "lvds-codec",
> > > > > > + .of_match_table = lvds_codec_match,
> > > > > > + },
> > > > > > +};
> > > > > > +module_platform_driver(lvds_codec_driver);
> > > > > > +
> > > > > > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > > > > > +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> > > > >
> > > > > Maybe "LVDS encoders and decoders" ?
> > > > >
> > > > > > +MODULE_LICENSE("GPL");
> > > > >
> > > > > [snip]
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c
2019-11-08 9:22 ` Fabrizio Castro
2019-11-08 9:39 ` Jacopo Mondi
@ 2019-11-08 11:02 ` Laurent Pinchart
1 sibling, 0 replies; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-08 11:02 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Fabrizio,
On Fri, Nov 08, 2019 at 09:22:56AM +0000, Fabrizio Castro wrote:
> On 07 November 2019 20:35 Laurent Pinchart wrote:
> > On Thu, Nov 07, 2019 at 08:10:58PM +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.
> > >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > ---
> > > v2->v3:
> > > * No change
> > > v1->v2:
> > > * No change
> > > ---
> > > drivers/gpu/drm/bridge/Kconfig | 8 +-
> > > drivers/gpu/drm/bridge/Makefile | 2 +-
> > > drivers/gpu/drm/bridge/lvds-codec.c | 131 ++++++++++++++++++++++++++++
> > > drivers/gpu/drm/bridge/lvds-encoder.c | 155 ----------------------------------
> > > 4 files changed, 136 insertions(+), 160 deletions(-)
> >
> > It would help if you added the -M1 option to git-format-patch to detect
> > the rename, the result would be easier to review.
>
> Will do, thank you for the hint
>
> > > create mode 100644 drivers/gpu/drm/bridge/lvds-codec.c
> > > delete mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c
> > >
> > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > index 3436297..9e75ca4e 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 LVDS 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-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > > new file mode 100644
> > > index 0000000..d57a8eb
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> > > @@ -0,0 +1,131 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Copyright (C) 2019 Renesas Electronics Corporation
> > > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > + */
> > > +
> > > +#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>
> > > +
> > > +#include <drm/drm_bridge.h>
> > > +#include <drm/drm_panel.h>
> > > +
> > > +struct lvds_codec {
> > > + struct drm_bridge bridge;
> > > + struct drm_bridge *panel_bridge;
> > > + struct gpio_desc *powerdown_gpio;
> > > +};
> > > +
> > > +static int lvds_codec_attach(struct drm_bridge *bridge)
> > > +{
> > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > + struct lvds_codec, bridge);
> > > +
> > > + return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge,
> > > + bridge);
> > > +}
> > > +
> > > +static void lvds_codec_enable(struct drm_bridge *bridge)
> > > +{
> > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > + struct lvds_codec, bridge);
> > > +
> > > + if (lvds_codec->powerdown_gpio)
> > > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0);
> > > +}
> > > +
> > > +static void lvds_codec_disable(struct drm_bridge *bridge)
> > > +{
> > > + struct lvds_codec *lvds_codec = container_of(bridge,
> > > + struct lvds_codec, bridge);
> > > +
> > > + if (lvds_codec->powerdown_gpio)
> > > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1);
> > > +}
> > > +
> > > +static struct drm_bridge_funcs funcs = {
> > > + .attach = lvds_codec_attach,
> > > + .enable = lvds_codec_enable,
> > > + .disable = lvds_codec_disable,
> > > +};
> > > +
> > > +static int lvds_codec_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct device_node *panel_node;
> > > + struct drm_panel *panel;
> > > + struct lvds_codec *lvds_codec;
> > > +
> > > + lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> > > + if (!lvds_codec)
> > > + return -ENOMEM;
> > > +
> > > + lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> > > + GPIOD_OUT_HIGH);
> > > + if (IS_ERR(lvds_codec->powerdown_gpio))
> > > + return PTR_ERR(lvds_codec->powerdown_gpio);
> >
> > The driver had an error message here, any reason it got removed ?
>
> I am quoting from https://www.spinics.net/lists/devicetree/msg318602.html :
> "I know it was there already, but this seems a bit unusual for the
> minimal gain of having a printout in the very unlikely case the
> gpiod_get() operations fails. I would just return PTR_ERR()."
>
> I am OK with reinstating it, just let me know what you want me to do here.
Debugging probe failures is annoying without proper error messages. As
the GPIO is optional, though, it's probably safe to drop it, but if you
decide to do so, please split that to a separate patch. What bothers me
the most is that the change is hidden and not explained in the commit
message.
> > > +
> > > + panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
> > > + if (!panel_node) {
> > > + dev_dbg(dev, "panel DT node not found\n");
> > > + return -ENXIO;
> > > + }
> > > +
> > > + panel = of_drm_find_panel(panel_node);
> > > + of_node_put(panel_node);
> > > + if (IS_ERR(panel)) {
> > > + dev_dbg(dev, "panel not found, deferring probe\n");
> > > + return PTR_ERR(panel);
> > > + }
> > > +
> > > + lvds_codec->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> >
> > This was devm_drm_panel_bridge_add_typed(), do you think there's a risk
> > of breaking userspace ? Of course as noted in the documentation of
> > devm_drm_panel_bridge_add_typed() the right solution is to fix panel
> > drivers, but I'm still slightly worried.
>
> Things break when the panel doesn't define connector_type, leading to the below
> check from devm_drm_panel_bridge_add:
> if (WARN_ON(panel->connector_type == DRM_MODE_CONNECTOR_Unknown))
> return NULL;
>
> Please advise on the best course of action here.
It's a matter of risk management. Is there a system that uses a buggy
panel driver with lvds-encoder out there ? If you think the risk is
close to 0, this change is fine. Otherwise we need to keep using
devm_drm_panel_bridge_add_typed() for the lvds-encoder case. In any
case, please split this to a separate patch.
> > Actually, could you split this patch in two, with a patch that only
> > renames the driver (and the symbols internally) without any functional
> > change, and another patch that performs the modifications ? That would
> > be much easier to review and discuss.
>
> Will do
>
> > > + 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,
> > > + * but we need a bridge attached to our of_node for our user
> > > + * to look up.
> > > + */
> > > + lvds_codec->bridge.of_node = dev->of_node;
> > > + lvds_codec->bridge.funcs = &funcs;
> > > + drm_bridge_add(&lvds_codec->bridge);
> > > +
> > > + platform_set_drvdata(pdev, lvds_codec);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int lvds_codec_remove(struct platform_device *pdev)
> > > +{
> > > + struct lvds_codec *lvds_codec = platform_get_drvdata(pdev);
> > > +
> > > + drm_bridge_remove(&lvds_codec->bridge);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id lvds_codec_match[] = {
> > > + { .compatible = "lvds-encoder" },
> > > + { .compatible = "thine,thc63lvdm83d" },
> > > + { .compatible = "lvds-decoder" },
> > > + {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, lvds_codec_match);
> > > +
> > > +static struct platform_driver lvds_codec_driver = {
> > > + .probe = lvds_codec_probe,
> > > + .remove = lvds_codec_remove,
> > > + .driver = {
> > > + .name = "lvds-codec",
> > > + .of_match_table = lvds_codec_match,
> > > + },
> > > +};
> > > +module_platform_driver(lvds_codec_driver);
> > > +
> > > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > > +MODULE_DESCRIPTION("Driver for transparent LVDS encoders and LVDS decoders");
> >
> > Maybe "LVDS encoders and decoders" ?
> >
> > > +MODULE_LICENSE("GPL");
> >
> > [snip]
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 3/7] dt-bindings: display: bridge: Repurpose lvds-encoder
2019-11-07 20:10 [PATCH v3 0/7] Add LCD panel support to iwg20d Fabrizio Castro
2019-11-07 20:10 ` [PATCH v3 1/7] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema Fabrizio Castro
2019-11-07 20:10 ` [PATCH v3 2/7] drm/bridge: Repurpose lvds-encoder.c Fabrizio Castro
@ 2019-11-07 20:10 ` Fabrizio Castro
2019-11-07 20:38 ` Laurent Pinchart
2019-11-07 20:11 ` [PATCH v3 4/7] drm: Define DRM_MODE_CONNECTOR_PARALLEL Fabrizio Castro
` (3 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-07 20:10 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
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, therefore adapt the dt-bindings to fit the
new purpose.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v2->v3:
* Extracted conversion to lvds-codec as per Rob's comment
v1->v2:
* Converted to dt-schema as per Neil's comment
---
.../bindings/display/bridge/lvds-codec.yaml | 120 +++++++++++++++++++++
.../bindings/display/bridge/lvds-transmitter.yaml | 91 ----------------
2 files changed, 120 insertions(+), 91 deletions(-)
create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
new file mode 100644
index 0000000..3ebdf96
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
@@ -0,0 +1,120 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Transparent LVDS encoders and LVDS decoders
+
+maintainers:
+ - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
+
+description: |
+ This binding supports transparent LVDS encoders and LVDS 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
+ 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 or decoder 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 # for LVDS encoders
+ - lvds-decoder # for LVDS decoders
+
+ 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: |
+ With LVDS encoders port 0 is for parallel input
+ With LVDS decoders port 0 is for LVDS input
+
+ port@1:
+ type: object
+ description: |
+ With LVDS encoders port 1 is for LVDS output
+ With LVDS decoders port 1 is for parallel 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>;
+ };
+ };
+ };
+ };
+
+ - |
+ 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>;
+ };
+ };
+ };
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
deleted file mode 100644
index 5be163a..0000000
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
+++ /dev/null
@@ -1,91 +0,0 @@
-# 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 or decoder 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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 3/7] dt-bindings: display: bridge: Repurpose lvds-encoder
2019-11-07 20:10 ` [PATCH v3 3/7] dt-bindings: display: bridge: Repurpose lvds-encoder Fabrizio Castro
@ 2019-11-07 20:38 ` Laurent Pinchart
2019-11-08 9:26 ` Fabrizio Castro
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-07 20:38 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Fabrizio,
Thank you for the patch.
On Thu, Nov 07, 2019 at 08:10:59PM +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, therefore adapt the dt-bindings to fit the
> new purpose.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v2->v3:
> * Extracted conversion to lvds-codec as per Rob's comment
> v1->v2:
> * Converted to dt-schema as per Neil's comment
> ---
> .../bindings/display/bridge/lvds-codec.yaml | 120 +++++++++++++++++++++
> .../bindings/display/bridge/lvds-transmitter.yaml | 91 ----------------
-M1 would also help a lot here.
> 2 files changed, 120 insertions(+), 91 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> new file mode 100644
> index 0000000..3ebdf96
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -0,0 +1,120 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Transparent LVDS encoders and LVDS decoders
s/LVDS decoders/decoders/ ?
> +
> +maintainers:
> + - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> +
> +description: |
> + This binding supports transparent LVDS encoders and LVDS decoders that don't
Same here.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + 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 or decoder 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 # for LVDS encoders
> + - lvds-decoder # for LVDS decoders
> +
> + 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: |
> + With LVDS encoders port 0 is for parallel input
> + With LVDS decoders port 0 is for LVDS input
> +
> + port@1:
> + type: object
> + description: |
> + With LVDS encoders port 1 is for LVDS output
> + With LVDS decoders port 1 is for parallel 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>;
> + };
> + };
> + };
> + };
> +
> + - |
> + 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>;
> + };
> + };
> + };
> + };
> +
> +...
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> deleted file mode 100644
> index 5be163a..0000000
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> +++ /dev/null
> @@ -1,91 +0,0 @@
> -# 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 or decoder 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
^ permalink raw reply [flat|nested] 28+ messages in thread* RE: [PATCH v3 3/7] dt-bindings: display: bridge: Repurpose lvds-encoder
2019-11-07 20:38 ` Laurent Pinchart
@ 2019-11-08 9:26 ` Fabrizio Castro
0 siblings, 0 replies; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-08 9:26 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hello Laurent,
Thank you for your feedback!
> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 20:38
> Subject: Re: [PATCH v3 3/7] dt-bindings: display: bridge: Repurpose lvds-encoder
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Thu, Nov 07, 2019 at 08:10:59PM +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, therefore adapt the dt-bindings to fit the
> > new purpose.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v2->v3:
> > * Extracted conversion to lvds-codec as per Rob's comment
> > v1->v2:
> > * Converted to dt-schema as per Neil's comment
> > ---
> > .../bindings/display/bridge/lvds-codec.yaml | 120 +++++++++++++++++++++
> > .../bindings/display/bridge/lvds-transmitter.yaml | 91 ----------------
>
> -M1 would also help a lot here.
Will do
>
> > 2 files changed, 120 insertions(+), 91 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > new file mode 100644
> > index 0000000..3ebdf96
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > @@ -0,0 +1,120 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/lvds-codec.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Transparent LVDS encoders and LVDS decoders
>
> s/LVDS decoders/decoders/ ?
Will replace
>
> > +
> > +maintainers:
> > + - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > +
> > +description: |
> > + This binding supports transparent LVDS encoders and LVDS decoders that don't
>
> Same here.
Will do
Thanks,
Fab
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > + 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 or decoder 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 # for LVDS encoders
> > + - lvds-decoder # for LVDS decoders
> > +
> > + 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: |
> > + With LVDS encoders port 0 is for parallel input
> > + With LVDS decoders port 0 is for LVDS input
> > +
> > + port@1:
> > + type: object
> > + description: |
> > + With LVDS encoders port 1 is for LVDS output
> > + With LVDS decoders port 1 is for parallel 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>;
> > + };
> > + };
> > + };
> > + };
> > +
> > + - |
> > + 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>;
> > + };
> > + };
> > + };
> > + };
> > +
> > +...
> > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> > deleted file mode 100644
> > index 5be163a..0000000
> > --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.yaml
> > +++ /dev/null
> > @@ -1,91 +0,0 @@
> > -# 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 or decoder 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 4/7] drm: Define DRM_MODE_CONNECTOR_PARALLEL
2019-11-07 20:10 [PATCH v3 0/7] Add LCD panel support to iwg20d Fabrizio Castro
` (2 preceding siblings ...)
2019-11-07 20:10 ` [PATCH v3 3/7] dt-bindings: display: bridge: Repurpose lvds-encoder Fabrizio Castro
@ 2019-11-07 20:11 ` Fabrizio Castro
2019-11-07 20:46 ` Laurent Pinchart
2019-11-07 20:11 ` [PATCH v3 5/7] drm/panel: panel-simple: Add connector type for etm0700g0dh6 Fabrizio Castro
` (2 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-07 20:11 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
The existing DRM_MODE_CONNECTOR_ definitions don't seem to
describe the connector for RGB/Parallel embedded displays,
hence add DRM_MODE_CONNECTOR_PARALLEL.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v2->v3:
* New patch
---
drivers/gpu/drm/drm_connector.c | 1 +
include/uapi/drm/drm_mode.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 2166000..b233029 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -93,6 +93,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
{ DRM_MODE_CONNECTOR_DPI, "DPI" },
{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
{ DRM_MODE_CONNECTOR_SPI, "SPI" },
+ { DRM_MODE_CONNECTOR_PARALLEL, "Parallel" },
};
void drm_connector_ida_init(void)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 735c8cf..5852f47 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -362,6 +362,7 @@ enum drm_mode_subconnector {
#define DRM_MODE_CONNECTOR_DPI 17
#define DRM_MODE_CONNECTOR_WRITEBACK 18
#define DRM_MODE_CONNECTOR_SPI 19
+#define DRM_MODE_CONNECTOR_PARALLEL 20
struct drm_mode_get_connector {
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/7] drm: Define DRM_MODE_CONNECTOR_PARALLEL
2019-11-07 20:11 ` [PATCH v3 4/7] drm: Define DRM_MODE_CONNECTOR_PARALLEL Fabrizio Castro
@ 2019-11-07 20:46 ` Laurent Pinchart
2019-11-08 9:32 ` Fabrizio Castro
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-07 20:46 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi, Sam Ravnborg
Hi Fabrizio,
(CC'ing Sam)
Thank you for the patch.
On Thu, Nov 07, 2019 at 08:11:00PM +0000, Fabrizio Castro wrote:
> The existing DRM_MODE_CONNECTOR_ definitions don't seem to
> describe the connector for RGB/Parallel embedded displays,
> hence add DRM_MODE_CONNECTOR_PARALLEL.
Please, no. We already have too many connector types for panels, when
userspace should really not care. DRM_MODE_CONNECTOR_LVDS,
DRM_MODE_CONNECTOR_eDP, DRM_MODE_CONNECTOR_DSI, DRM_MODE_CONNECTOR_DPI
and probably DRM_MODE_CONNECTOR_SPI should have been
DRM_MODE_CONNECTOR_PANEL.
This has been discussed in [1]. Let's instead define a
DRM_MODE_CONNECTOR_PANEL, possibly as an alias to one of the existing
types, and deprecate the other types.
[1] https://www.spinics.net/lists/dri-devel/msg224638.html
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v2->v3:
> * New patch
> ---
> drivers/gpu/drm/drm_connector.c | 1 +
> include/uapi/drm/drm_mode.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 2166000..b233029 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -93,6 +93,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
> { DRM_MODE_CONNECTOR_DPI, "DPI" },
> { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
> { DRM_MODE_CONNECTOR_SPI, "SPI" },
> + { DRM_MODE_CONNECTOR_PARALLEL, "Parallel" },
> };
>
> void drm_connector_ida_init(void)
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 735c8cf..5852f47 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -362,6 +362,7 @@ enum drm_mode_subconnector {
> #define DRM_MODE_CONNECTOR_DPI 17
> #define DRM_MODE_CONNECTOR_WRITEBACK 18
> #define DRM_MODE_CONNECTOR_SPI 19
> +#define DRM_MODE_CONNECTOR_PARALLEL 20
>
> struct drm_mode_get_connector {
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* RE: [PATCH v3 4/7] drm: Define DRM_MODE_CONNECTOR_PARALLEL
2019-11-07 20:46 ` Laurent Pinchart
@ 2019-11-08 9:32 ` Fabrizio Castro
0 siblings, 0 replies; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-08 9:32 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi, Sam Ravnborg
Hi Laurent,
Thank you for your feedback!
> From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 20:47
> Subject: Re: [PATCH v3 4/7] drm: Define DRM_MODE_CONNECTOR_PARALLEL
>
> Hi Fabrizio,
>
> (CC'ing Sam)
>
> Thank you for the patch.
>
> On Thu, Nov 07, 2019 at 08:11:00PM +0000, Fabrizio Castro wrote:
> > The existing DRM_MODE_CONNECTOR_ definitions don't seem to
> > describe the connector for RGB/Parallel embedded displays,
> > hence add DRM_MODE_CONNECTOR_PARALLEL.
>
> Please, no. We already have too many connector types for panels, when
> userspace should really not care. DRM_MODE_CONNECTOR_LVDS,
> DRM_MODE_CONNECTOR_eDP, DRM_MODE_CONNECTOR_DSI, DRM_MODE_CONNECTOR_DPI
> and probably DRM_MODE_CONNECTOR_SPI should have been
> DRM_MODE_CONNECTOR_PANEL.
>
> This has been discussed in [1]. Let's instead define a
> DRM_MODE_CONNECTOR_PANEL, possibly as an alias to one of the existing
> types, and deprecate the other types.
>
> [1] https://www.spinics.net/lists/dri-devel/msg224638.html
Thank you for the pointer and the for the details. That clarifies things a lot.
In my case, as you mentioned in the patch to simple panel, I can use an
existing definition, therefore I think it's best if DRM_MODE_CONNECTOR_PANEL
gets added when there is a valid use case.
Thanks,
Fab
>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v2->v3:
> > * New patch
> > ---
> > drivers/gpu/drm/drm_connector.c | 1 +
> > include/uapi/drm/drm_mode.h | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 2166000..b233029 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -93,6 +93,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
> > { DRM_MODE_CONNECTOR_DPI, "DPI" },
> > { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
> > { DRM_MODE_CONNECTOR_SPI, "SPI" },
> > + { DRM_MODE_CONNECTOR_PARALLEL, "Parallel" },
> > };
> >
> > void drm_connector_ida_init(void)
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 735c8cf..5852f47 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -362,6 +362,7 @@ enum drm_mode_subconnector {
> > #define DRM_MODE_CONNECTOR_DPI 17
> > #define DRM_MODE_CONNECTOR_WRITEBACK 18
> > #define DRM_MODE_CONNECTOR_SPI 19
> > +#define DRM_MODE_CONNECTOR_PARALLEL 20
> >
> > struct drm_mode_get_connector {
> >
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 5/7] drm/panel: panel-simple: Add connector type for etm0700g0dh6
2019-11-07 20:10 [PATCH v3 0/7] Add LCD panel support to iwg20d Fabrizio Castro
` (3 preceding siblings ...)
2019-11-07 20:11 ` [PATCH v3 4/7] drm: Define DRM_MODE_CONNECTOR_PARALLEL Fabrizio Castro
@ 2019-11-07 20:11 ` Fabrizio Castro
2019-11-07 20:48 ` Laurent Pinchart
2019-11-07 20:11 ` [PATCH v3 6/7] ARM: dts: iwg20d-q7-common: Add LCD support Fabrizio Castro
2019-11-07 20:11 ` [PATCH v3 7/7] ARM: shmobile_defconfig: Enable support for panels from EDT Fabrizio Castro
6 siblings, 1 reply; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-07 20:11 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
Add connector type for the etm0700g0dh6 from Emerging Display
Technologies (EDT).
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v2->v3:
* New patch
---
drivers/gpu/drm/panel/panel-simple.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 5d48768..82065ff 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1342,6 +1342,7 @@ static const struct panel_desc edt_etm0700g0dh6 = {
},
.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
+ .connector_type = DRM_MODE_CONNECTOR_PARALLEL,
};
static const struct panel_desc edt_etm0700g0bdh6 = {
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 5/7] drm/panel: panel-simple: Add connector type for etm0700g0dh6
2019-11-07 20:11 ` [PATCH v3 5/7] drm/panel: panel-simple: Add connector type for etm0700g0dh6 Fabrizio Castro
@ 2019-11-07 20:48 ` Laurent Pinchart
2019-11-08 9:51 ` Fabrizio Castro
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-07 20:48 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Fabrizio,
Thank you for the patch.
On Thu, Nov 07, 2019 at 08:11:01PM +0000, Fabrizio Castro wrote:
> Add connector type for the etm0700g0dh6 from Emerging Display
> Technologies (EDT).
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v2->v3:
> * New patch
> ---
> drivers/gpu/drm/panel/panel-simple.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 5d48768..82065ff 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1342,6 +1342,7 @@ static const struct panel_desc edt_etm0700g0dh6 = {
> },
> .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
> + .connector_type = DRM_MODE_CONNECTOR_PARALLEL,
I still think we should have a DRM_MODE_CONNECTOR_PANEL, but regardless,
this panel seems to match DRM_MODE_CONNECTOR_DPI.
> };
>
> static const struct panel_desc edt_etm0700g0bdh6 = {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread* RE: [PATCH v3 5/7] drm/panel: panel-simple: Add connector type for etm0700g0dh6
2019-11-07 20:48 ` Laurent Pinchart
@ 2019-11-08 9:51 ` Fabrizio Castro
0 siblings, 0 replies; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-08 9:51 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Laurent,
Thank you for your feedback!
> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 20:49
> Subject: Re: [PATCH v3 5/7] drm/panel: panel-simple: Add connector type for etm0700g0dh6
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Thu, Nov 07, 2019 at 08:11:01PM +0000, Fabrizio Castro wrote:
> > Add connector type for the etm0700g0dh6 from Emerging Display
> > Technologies (EDT).
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v2->v3:
> > * New patch
> > ---
> > drivers/gpu/drm/panel/panel-simple.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index 5d48768..82065ff 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -1342,6 +1342,7 @@ static const struct panel_desc edt_etm0700g0dh6 = {
> > },
> > .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> > .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
> > + .connector_type = DRM_MODE_CONNECTOR_PARALLEL,
>
> I still think we should have a DRM_MODE_CONNECTOR_PANEL, but regardless,
> this panel seems to match DRM_MODE_CONNECTOR_DPI.
Thank you! I wasn't really sure about which definition to pick, DRM_MODE_CONNECTOR_DPI
will surely work just fine.
Thanks,
Fab
>
> > };
> >
> > static const struct panel_desc edt_etm0700g0bdh6 = {
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 6/7] ARM: dts: iwg20d-q7-common: Add LCD support
2019-11-07 20:10 [PATCH v3 0/7] Add LCD panel support to iwg20d Fabrizio Castro
` (4 preceding siblings ...)
2019-11-07 20:11 ` [PATCH v3 5/7] drm/panel: panel-simple: Add connector type for etm0700g0dh6 Fabrizio Castro
@ 2019-11-07 20:11 ` Fabrizio Castro
2019-11-07 20:55 ` Laurent Pinchart
2019-11-07 20:11 ` [PATCH v3 7/7] ARM: shmobile_defconfig: Enable support for panels from EDT Fabrizio Castro
6 siblings, 1 reply; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-07 20:11 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
The iwg20d comes with a 7" capacitive touch screen, therefore
add support for it.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
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..3428b8d 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 = "lvds-decoder";
+ powerdown = <&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", "simple-panel";
+ 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 = <®_3p3v>;
VDDD-supply = <®_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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 6/7] ARM: dts: iwg20d-q7-common: Add LCD support
2019-11-07 20:11 ` [PATCH v3 6/7] ARM: dts: iwg20d-q7-common: Add LCD support Fabrizio Castro
@ 2019-11-07 20:55 ` Laurent Pinchart
2019-11-08 12:02 ` Fabrizio Castro
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-07 20:55 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Fabrizio,
Thank you for the patch.
On Thu, Nov 07, 2019 at 08:11:02PM +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>
>
> ---
> 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..3428b8d 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 = "lvds-decoder";
A specific compatible string is required.
I think the lvds-decoder driver should error out at probe time if only
one compatible string is listed.
> + powerdown = <&gpio7 25 GPIO_ACTIVE_LOW>;
powerdown-gpios ?
> +
> + 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", "simple-panel";
There's no "simple-panel" compatible string defined anywhere as far as I
can tell.
> + 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;
> + };
Do you need this, with the touchpanel@38 node already listing the
interrupt ?
> +};
> +
> &hsusb {
> status = "okay";
> pinctrl-0 = <&usb0_pins>;
> @@ -147,6 +202,25 @@
> VDDIO-supply = <®_3p3v>;
> VDDD-supply = <®_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
^ permalink raw reply [flat|nested] 28+ messages in thread* RE: [PATCH v3 6/7] ARM: dts: iwg20d-q7-common: Add LCD support
2019-11-07 20:55 ` Laurent Pinchart
@ 2019-11-08 12:02 ` Fabrizio Castro
0 siblings, 0 replies; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-08 12:02 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Laurent,
Thank you for your feedback!
> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 20:55
> Subject: Re: [PATCH v3 6/7] ARM: dts: iwg20d-q7-common: Add LCD support
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Thu, Nov 07, 2019 at 08:11:02PM +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>
> >
> > ---
> > 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..3428b8d 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 = "lvds-decoder";
>
> A specific compatible string is required.
Will document the specific compatible in the binding doc
>
> I think the lvds-decoder driver should error out at probe time if only
> one compatible string is listed.
Ok, will modify the driver accordingly
>
> > + powerdown = <&gpio7 25 GPIO_ACTIVE_LOW>;
>
> powerdown-gpios ?
That's a bug, well spotted.
>
> > +
> > + 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", "simple-panel";
>
> There's no "simple-panel" compatible string defined anywhere as far as I
> can tell.
The usage of "simple-panel" as a compatible is widespread and really confusing.
The fact that you made this comment is good enough for me to say we don't
need it, I'll take it out.
>
> > + 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;
> > + };
>
> Do you need this, with the touchpanel@38 node already listing the
> interrupt ?
Yes, unfortunately we do need this as the bootloader is poking with the gpio.
I can't fix this in the bootloader as we have no control over it.
Thanks,
Fab
>
> > +};
> > +
> > &hsusb {
> > status = "okay";
> > pinctrl-0 = <&usb0_pins>;
> > @@ -147,6 +202,25 @@
> > VDDIO-supply = <®_3p3v>;
> > VDDD-supply = <®_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
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 7/7] ARM: shmobile_defconfig: Enable support for panels from EDT
2019-11-07 20:10 [PATCH v3 0/7] Add LCD panel support to iwg20d Fabrizio Castro
` (5 preceding siblings ...)
2019-11-07 20:11 ` [PATCH v3 6/7] ARM: dts: iwg20d-q7-common: Add LCD support Fabrizio Castro
@ 2019-11-07 20:11 ` Fabrizio Castro
2019-11-07 20:56 ` Laurent Pinchart
6 siblings, 1 reply; 28+ messages in thread
From: Fabrizio Castro @ 2019-11-07 20:11 UTC (permalink / raw)
To: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda
Cc: Fabrizio Castro, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Laurent Pinchart, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Laurent Pinchart,
Kieran Bingham, Jacopo Mondi
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>
---
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 7/7] ARM: shmobile_defconfig: Enable support for panels from EDT
2019-11-07 20:11 ` [PATCH v3 7/7] ARM: shmobile_defconfig: Enable support for panels from EDT Fabrizio Castro
@ 2019-11-07 20:56 ` Laurent Pinchart
2019-11-08 8:38 ` Geert Uytterhoeven
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2019-11-07 20:56 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Neil Armstrong, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Andrzej Hajda, Simon Horman, Geert Uytterhoeven,
Magnus Damm, Peter Rosin, dri-devel, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Fabrizio,
Thank you for the patch.
On Thu, Nov 07, 2019 at 08:11:03PM +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>
>
> ---
> 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
Do these options need to be built-in, or could modules work too ?
> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 7/7] ARM: shmobile_defconfig: Enable support for panels from EDT
2019-11-07 20:56 ` Laurent Pinchart
@ 2019-11-08 8:38 ` Geert Uytterhoeven
0 siblings, 0 replies; 28+ messages in thread
From: Geert Uytterhoeven @ 2019-11-08 8:38 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Fabrizio Castro, Neil Armstrong, David Airlie, Daniel Vetter,
Rob Herring, Mark Rutland, Andrzej Hajda, Simon Horman,
Geert Uytterhoeven, Magnus Damm, Peter Rosin, DRI Development,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-Renesas, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi
Hi Laurent,
On Thu, Nov 7, 2019 at 9:56 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thu, Nov 07, 2019 at 08:11:03PM +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>
> >
> > ---
> > 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
>
> Do these options need to be built-in, or could modules work too ?
All Renesas-specific config options in shmobile_defconfig are builtin,
unlike in multi_v7_defconfig and arm64_defconfig.
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
^ permalink raw reply [flat|nested] 28+ messages in thread