* [PATCH v2 1/9] dt-bindings: panel: lvds: Add dual-link LVDS display support
2019-08-15 11:04 [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Fabrizio Castro
@ 2019-08-15 11:04 ` Fabrizio Castro
2019-08-15 11:45 ` Laurent Pinchart
2019-08-15 11:04 ` [PATCH v2 2/9] dt-bindings: display: Add bindings for Advantech IDK-2121WR Fabrizio Castro
` (8 subsequent siblings)
9 siblings, 1 reply; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 11:04 UTC (permalink / raw)
To: Laurent Pinchart, Thierry Reding, David Airlie, Daniel Vetter,
Rob Herring, Mark Rutland
Cc: Fabrizio Castro, Sam Ravnborg, dri-devel, devicetree,
linux-kernel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-renesas-soc, Kieran Bingham, Jacopo Mondi
Dual-link LVDS displays have two ports, therefore document this
with the bindings.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v1->v2:
* Reworked the description of the ports property
* lvds0_panel_in in the example has been renamed to panel_in0
* lvds1_panel_in in the example has been renamed to panel_in1
Laurent,
in linux-next they are now working with:
Documentation/devicetree/bindings/display/panel/lvds.yaml
What should I do here?
Thanks,
Fab
.../bindings/display/panel/panel-lvds.txt | 95 ++++++++++++++++------
1 file changed, 71 insertions(+), 24 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.txt b/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
index 250850a..5231243 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
+++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
@@ -41,7 +41,12 @@ Required nodes:
- panel-timing: See panel-common.txt.
- ports: See panel-common.txt. These bindings require a single port subnode
- corresponding to the panel LVDS input.
+ (for a single link panel) corresponding to the panel LVDS input, or two port
+ subnodes (for a dual link panel) corresponding to the panel LVDS inputs.
+ Dual-link LVDS panels expect even pixels (0, 2, 4, etc.) and odd pixels (1, 3,
+ 5, etc.) on different input ports, it's up to the panel-specific bindings to
+ specify what port is expecting even pixels, and what port is expecting odd
+ pixels.
LVDS data mappings are defined as follows.
@@ -92,30 +97,72 @@ CTL3: 0
Example
-------
-panel {
- compatible = "mitsubishi,aa121td01", "panel-lvds";
-
- width-mm = <261>;
- height-mm = <163>;
-
- data-mapping = "jeida-24";
-
- panel-timing {
- /* 1280x800 @60Hz */
- clock-frequency = <71000000>;
- hactive = <1280>;
- vactive = <800>;
- hsync-len = <70>;
- hfront-porch = <20>;
- hback-porch = <70>;
- vsync-len = <5>;
- vfront-porch = <3>;
- vback-porch = <15>;
+Single port:
+ panel {
+ compatible = "mitsubishi,aa121td01", "panel-lvds";
+
+ width-mm = <261>;
+ height-mm = <163>;
+
+ data-mapping = "jeida-24";
+
+ panel-timing {
+ /* 1280x800 @60Hz */
+ clock-frequency = <71000000>;
+ hactive = <1280>;
+ vactive = <800>;
+ hsync-len = <70>;
+ hfront-porch = <20>;
+ hback-porch = <70>;
+ vsync-len = <5>;
+ vfront-porch = <3>;
+ vback-porch = <15>;
+ };
+
+ port {
+ panel_in: endpoint {
+ remote-endpoint = <&lvds_encoder>;
+ };
+ };
};
- port {
- panel_in: endpoint {
- remote-endpoint = <&lvds_encoder>;
+Two ports:
+ panel {
+ compatible = "advantech,idk-2121wr", "panel-lvds";
+
+ width-mm = <476>;
+ height-mm = <268>;
+
+ data-mapping = "vesa-24";
+
+ panel-timing {
+ clock-frequency = <148500000>;
+ hactive = <1920>;
+ vactive = <1080>;
+ hsync-len = <44>;
+ hfront-porch = <88>;
+ hback-porch = <148>;
+ vfront-porch = <4>;
+ vback-porch = <36>;
+ vsync-len = <5>;
+ };
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ panel_in0: endpoint {
+ remote-endpoint = <&lvds0_out>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ panel_in1: endpoint {
+ remote-endpoint = <&lvds1_out>;
+ };
+ };
};
};
-};
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 1/9] dt-bindings: panel: lvds: Add dual-link LVDS display support
2019-08-15 11:04 ` [PATCH v2 1/9] dt-bindings: panel: lvds: Add dual-link LVDS display support Fabrizio Castro
@ 2019-08-15 11:45 ` Laurent Pinchart
2019-08-15 13:37 ` Fabrizio Castro
0 siblings, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2019-08-15 11:45 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Thierry Reding, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Sam Ravnborg, dri-devel, devicetree, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc, Kieran Bingham, Jacopo Mondi
Hi Fabrizio,
On Thu, Aug 15, 2019 at 12:04:25PM +0100, Fabrizio Castro wrote:
> Dual-link LVDS displays have two ports, therefore document this
> with the bindings.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * Reworked the description of the ports property
> * lvds0_panel_in in the example has been renamed to panel_in0
> * lvds1_panel_in in the example has been renamed to panel_in1
>
> Laurent,
>
> in linux-next they are now working with:
> Documentation/devicetree/bindings/display/panel/lvds.yaml
Documentation/devicetree/bindings/display/panel/lvds.yaml is in
drm-misc-next, so I would advise rebasing on top of that.
> What should I do here?
>
> .../bindings/display/panel/panel-lvds.txt | 95 ++++++++++++++++------
> 1 file changed, 71 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.txt b/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> index 250850a..5231243 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> @@ -41,7 +41,12 @@ Required nodes:
>
> - panel-timing: See panel-common.txt.
> - ports: See panel-common.txt. These bindings require a single port subnode
> - corresponding to the panel LVDS input.
> + (for a single link panel) corresponding to the panel LVDS input, or two port
> + subnodes (for a dual link panel) corresponding to the panel LVDS inputs.
> + Dual-link LVDS panels expect even pixels (0, 2, 4, etc.) and odd pixels (1, 3,
> + 5, etc.) on different input ports, it's up to the panel-specific bindings to
> + specify what port is expecting even pixels, and what port is expecting odd
> + pixels.
>
>
> LVDS data mappings are defined as follows.
> @@ -92,30 +97,72 @@ CTL3: 0
> Example
> -------
>
> -panel {
> - compatible = "mitsubishi,aa121td01", "panel-lvds";
> -
> - width-mm = <261>;
> - height-mm = <163>;
> -
> - data-mapping = "jeida-24";
> -
> - panel-timing {
> - /* 1280x800 @60Hz */
> - clock-frequency = <71000000>;
> - hactive = <1280>;
> - vactive = <800>;
> - hsync-len = <70>;
> - hfront-porch = <20>;
> - hback-porch = <70>;
> - vsync-len = <5>;
> - vfront-porch = <3>;
> - vback-porch = <15>;
> +Single port:
> + panel {
> + compatible = "mitsubishi,aa121td01", "panel-lvds";
> +
> + width-mm = <261>;
> + height-mm = <163>;
> +
> + data-mapping = "jeida-24";
> +
> + panel-timing {
> + /* 1280x800 @60Hz */
> + clock-frequency = <71000000>;
> + hactive = <1280>;
> + vactive = <800>;
> + hsync-len = <70>;
> + hfront-porch = <20>;
> + hback-porch = <70>;
> + vsync-len = <5>;
> + vfront-porch = <3>;
> + vback-porch = <15>;
> + };
> +
> + port {
> + panel_in: endpoint {
> + remote-endpoint = <&lvds_encoder>;
> + };
> + };
> };
>
> - port {
> - panel_in: endpoint {
> - remote-endpoint = <&lvds_encoder>;
> +Two ports:
> + panel {
> + compatible = "advantech,idk-2121wr", "panel-lvds";
> +
> + width-mm = <476>;
> + height-mm = <268>;
> +
> + data-mapping = "vesa-24";
> +
> + panel-timing {
> + clock-frequency = <148500000>;
> + hactive = <1920>;
> + vactive = <1080>;
> + hsync-len = <44>;
> + hfront-porch = <88>;
> + hback-porch = <148>;
> + vfront-porch = <4>;
> + vback-porch = <36>;
> + vsync-len = <5>;
> + };
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + panel_in0: endpoint {
> + remote-endpoint = <&lvds0_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + panel_in1: endpoint {
> + remote-endpoint = <&lvds1_out>;
> + };
> + };
> };
> };
> -};
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH v2 1/9] dt-bindings: panel: lvds: Add dual-link LVDS display support
2019-08-15 11:45 ` Laurent Pinchart
@ 2019-08-15 13:37 ` Fabrizio Castro
0 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 13:37 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Sam Ravnborg, dri-devel, devicetree, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc, Kieran Bingham, Jacopo Mondi
Hi Laurent,
Thank you for your feedback!
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 15 August 2019 12:45
> Subject: Re: [PATCH v2 1/9] dt-bindings: panel: lvds: Add dual-link LVDS display support
>
> Hi Fabrizio,
>
> On Thu, Aug 15, 2019 at 12:04:25PM +0100, Fabrizio Castro wrote:
> > Dual-link LVDS displays have two ports, therefore document this
> > with the bindings.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * Reworked the description of the ports property
> > * lvds0_panel_in in the example has been renamed to panel_in0
> > * lvds1_panel_in in the example has been renamed to panel_in1
> >
> > Laurent,
> >
> > in linux-next they are now working with:
> > Documentation/devicetree/bindings/display/panel/lvds.yaml
>
> Documentation/devicetree/bindings/display/panel/lvds.yaml is in
> drm-misc-next, so I would advise rebasing on top of that.
Will do.
Thanks,
Fab
>
> > What should I do here?
> >
> > .../bindings/display/panel/panel-lvds.txt | 95 ++++++++++++++++------
> > 1 file changed, 71 insertions(+), 24 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> b/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> > index 250850a..5231243 100644
> > --- a/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> > @@ -41,7 +41,12 @@ Required nodes:
> >
> > - panel-timing: See panel-common.txt.
> > - ports: See panel-common.txt. These bindings require a single port subnode
> > - corresponding to the panel LVDS input.
> > + (for a single link panel) corresponding to the panel LVDS input, or two port
> > + subnodes (for a dual link panel) corresponding to the panel LVDS inputs.
> > + Dual-link LVDS panels expect even pixels (0, 2, 4, etc.) and odd pixels (1, 3,
> > + 5, etc.) on different input ports, it's up to the panel-specific bindings to
> > + specify what port is expecting even pixels, and what port is expecting odd
> > + pixels.
> >
> >
> > LVDS data mappings are defined as follows.
> > @@ -92,30 +97,72 @@ CTL3: 0
> > Example
> > -------
> >
> > -panel {
> > - compatible = "mitsubishi,aa121td01", "panel-lvds";
> > -
> > - width-mm = <261>;
> > - height-mm = <163>;
> > -
> > - data-mapping = "jeida-24";
> > -
> > - panel-timing {
> > - /* 1280x800 @60Hz */
> > - clock-frequency = <71000000>;
> > - hactive = <1280>;
> > - vactive = <800>;
> > - hsync-len = <70>;
> > - hfront-porch = <20>;
> > - hback-porch = <70>;
> > - vsync-len = <5>;
> > - vfront-porch = <3>;
> > - vback-porch = <15>;
> > +Single port:
> > + panel {
> > + compatible = "mitsubishi,aa121td01", "panel-lvds";
> > +
> > + width-mm = <261>;
> > + height-mm = <163>;
> > +
> > + data-mapping = "jeida-24";
> > +
> > + panel-timing {
> > + /* 1280x800 @60Hz */
> > + clock-frequency = <71000000>;
> > + hactive = <1280>;
> > + vactive = <800>;
> > + hsync-len = <70>;
> > + hfront-porch = <20>;
> > + hback-porch = <70>;
> > + vsync-len = <5>;
> > + vfront-porch = <3>;
> > + vback-porch = <15>;
> > + };
> > +
> > + port {
> > + panel_in: endpoint {
> > + remote-endpoint = <&lvds_encoder>;
> > + };
> > + };
> > };
> >
> > - port {
> > - panel_in: endpoint {
> > - remote-endpoint = <&lvds_encoder>;
> > +Two ports:
> > + panel {
> > + compatible = "advantech,idk-2121wr", "panel-lvds";
> > +
> > + width-mm = <476>;
> > + height-mm = <268>;
> > +
> > + data-mapping = "vesa-24";
> > +
> > + panel-timing {
> > + clock-frequency = <148500000>;
> > + hactive = <1920>;
> > + vactive = <1080>;
> > + hsync-len = <44>;
> > + hfront-porch = <88>;
> > + hback-porch = <148>;
> > + vfront-porch = <4>;
> > + vback-porch = <36>;
> > + vsync-len = <5>;
> > + };
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > + panel_in0: endpoint {
> > + remote-endpoint = <&lvds0_out>;
> > + };
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > + panel_in1: endpoint {
> > + remote-endpoint = <&lvds1_out>;
> > + };
> > + };
> > };
> > };
> > -};
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 2/9] dt-bindings: display: Add bindings for Advantech IDK-2121WR
2019-08-15 11:04 [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Fabrizio Castro
2019-08-15 11:04 ` [PATCH v2 1/9] dt-bindings: panel: lvds: Add dual-link LVDS display support Fabrizio Castro
@ 2019-08-15 11:04 ` Fabrizio Castro
2019-08-15 11:47 ` Laurent Pinchart
2019-08-15 11:04 ` [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings Fabrizio Castro
` (7 subsequent siblings)
9 siblings, 1 reply; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 11:04 UTC (permalink / raw)
To: Laurent Pinchart, Thierry Reding, David Airlie, Daniel Vetter,
Rob Herring, Mark Rutland
Cc: Fabrizio Castro, Sam Ravnborg, dri-devel, devicetree,
linux-kernel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-renesas-soc, Kieran Bingham, Jacopo Mondi
This panel is handled through the generic lvds-panel bindings,
so only needs its additional compatible specified.
Some panel-specific documentation can be found here:
https://buy.advantech.eu/Displays/Embedded-LCD-Kits-High-Brightness/model-IDK-2121WR-K2FHA2E.htm
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v1->v2:
* Reworked according to Laurent's feedback
* Renamed lvds0_panel_in to panel_in0
* Renamed lvds1_panel_in to panel_in1
Laurent,
Should this be a .yaml file already?
Thanks,
Fab
.../display/panel/advantech,idk-2121wr.txt | 56 ++++++++++++++++++++++
1 file changed, 56 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
diff --git a/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
new file mode 100644
index 0000000..6ee1d1b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
@@ -0,0 +1,56 @@
+Advantech Co., Ltd. IDK-2121WR 21.5" LVDS panel
+===============================================
+
+Required properties:
+- compatible: should be "advantech,idk-2121wr" followed by "panel-lvds"
+
+This binding is compatible with the lvds-panel binding, which is specified
+in panel-lvds.txt in this directory.
+The panel operates in dual-link mode and thus requires two port nodes,
+the first port node expects odd pixels (1, 3, 5, etc.) and the second port
+expects even pixels (0, 2, 4, etc.).
+
+Example
+-------
+
+ panel {
+ compatible = "advantech,idk-2121wr", "panel-lvds";
+
+ width-mm = <476>;
+ height-mm = <268>;
+
+ data-mapping = "vesa-24";
+
+ panel-timing {
+ clock-frequency = <148500000>;
+ hactive = <1920>;
+ vactive = <1080>;
+ hsync-len = <44>;
+ hfront-porch = <88>;
+ hback-porch = <148>;
+ vfront-porch = <4>;
+ vback-porch = <36>;
+ vsync-len = <5>;
+ };
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ /* Odd pixels */
+ reg = <0>;
+ panel_in0: endpoint {
+ remote-endpoint = <&lvds0_out>;
+ };
+ };
+
+ port@1 {
+ /* Even pixels */
+ reg = <1>;
+ panel_in1: endpoint {
+ remote-endpoint = <&lvds1_out>;
+ };
+ };
+ };
+ };
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 2/9] dt-bindings: display: Add bindings for Advantech IDK-2121WR
2019-08-15 11:04 ` [PATCH v2 2/9] dt-bindings: display: Add bindings for Advantech IDK-2121WR Fabrizio Castro
@ 2019-08-15 11:47 ` Laurent Pinchart
2019-08-15 13:38 ` Fabrizio Castro
0 siblings, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2019-08-15 11:47 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Thierry Reding, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Sam Ravnborg, dri-devel, devicetree, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc, Kieran Bingham, Jacopo Mondi
Hi Fabrizio,
On Thu, Aug 15, 2019 at 12:04:26PM +0100, Fabrizio Castro wrote:
> This panel is handled through the generic lvds-panel bindings,
> so only needs its additional compatible specified.
>
> Some panel-specific documentation can be found here:
> https://buy.advantech.eu/Displays/Embedded-LCD-Kits-High-Brightness/model-IDK-2121WR-K2FHA2E.htm
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * Reworked according to Laurent's feedback
> * Renamed lvds0_panel_in to panel_in0
> * Renamed lvds1_panel_in to panel_in1
>
> Laurent,
>
> Should this be a .yaml file already?
It's not a strict requirement, but I'm sure Rob would really appreciate.
I've converted a DT binding to yaml recently (for a panel too), and I
have to say I'm impressed by the validation yaml brings, both for DT
sources and for DT bindings. It even validates the example in the
bindings, which is great. Documentation/devicetree/writing-schema.md
should give you a few pointers to get started (in particular make sure
you run make dt_binding_check for your new bindings).
> .../display/panel/advantech,idk-2121wr.txt | 56 ++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
>
> diff --git a/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
> new file mode 100644
> index 0000000..6ee1d1b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
> @@ -0,0 +1,56 @@
> +Advantech Co., Ltd. IDK-2121WR 21.5" LVDS panel
> +===============================================
> +
> +Required properties:
> +- compatible: should be "advantech,idk-2121wr" followed by "panel-lvds"
> +
> +This binding is compatible with the lvds-panel binding, which is specified
> +in panel-lvds.txt in this directory.
> +The panel operates in dual-link mode and thus requires two port nodes,
> +the first port node expects odd pixels (1, 3, 5, etc.) and the second port
> +expects even pixels (0, 2, 4, etc.).
> +
> +Example
> +-------
> +
> + panel {
> + compatible = "advantech,idk-2121wr", "panel-lvds";
> +
> + width-mm = <476>;
> + height-mm = <268>;
> +
> + data-mapping = "vesa-24";
> +
> + panel-timing {
> + clock-frequency = <148500000>;
> + hactive = <1920>;
> + vactive = <1080>;
> + hsync-len = <44>;
> + hfront-porch = <88>;
> + hback-porch = <148>;
> + vfront-porch = <4>;
> + vback-porch = <36>;
> + vsync-len = <5>;
> + };
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + /* Odd pixels */
> + reg = <0>;
> + panel_in0: endpoint {
> + remote-endpoint = <&lvds0_out>;
> + };
> + };
> +
> + port@1 {
> + /* Even pixels */
> + reg = <1>;
> + panel_in1: endpoint {
> + remote-endpoint = <&lvds1_out>;
> + };
> + };
> + };
> + };
> --
> 2.7.4
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH v2 2/9] dt-bindings: display: Add bindings for Advantech IDK-2121WR
2019-08-15 11:47 ` Laurent Pinchart
@ 2019-08-15 13:38 ` Fabrizio Castro
0 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 13:38 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Sam Ravnborg, dri-devel, devicetree, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc, Kieran Bingham, Jacopo Mondi
Hi Laurent,
Thank you for your feedback!
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 15 August 2019 12:47
> Subject: Re: [PATCH v2 2/9] dt-bindings: display: Add bindings for Advantech IDK-2121WR
>
> Hi Fabrizio,
>
> On Thu, Aug 15, 2019 at 12:04:26PM +0100, Fabrizio Castro wrote:
> > This panel is handled through the generic lvds-panel bindings,
> > so only needs its additional compatible specified.
> >
> > Some panel-specific documentation can be found here:
> > https://buy.advantech.eu/Displays/Embedded-LCD-Kits-High-Brightness/model-IDK-2121WR-K2FHA2E.htm
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * Reworked according to Laurent's feedback
> > * Renamed lvds0_panel_in to panel_in0
> > * Renamed lvds1_panel_in to panel_in1
> >
> > Laurent,
> >
> > Should this be a .yaml file already?
>
> It's not a strict requirement, but I'm sure Rob would really appreciate.
> I've converted a DT binding to yaml recently (for a panel too), and I
> have to say I'm impressed by the validation yaml brings, both for DT
> sources and for DT bindings. It even validates the example in the
> bindings, which is great. Documentation/devicetree/writing-schema.md
> should give you a few pointers to get started (in particular make sure
> you run make dt_binding_check for your new bindings).
Will give this a try.
Thanks,
Fab
>
> > .../display/panel/advantech,idk-2121wr.txt | 56 ++++++++++++++++++++++
> > 1 file changed, 56 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
> b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
> > new file mode 100644
> > index 0000000..6ee1d1b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.txt
> > @@ -0,0 +1,56 @@
> > +Advantech Co., Ltd. IDK-2121WR 21.5" LVDS panel
> > +===============================================
> > +
> > +Required properties:
> > +- compatible: should be "advantech,idk-2121wr" followed by "panel-lvds"
> > +
> > +This binding is compatible with the lvds-panel binding, which is specified
> > +in panel-lvds.txt in this directory.
> > +The panel operates in dual-link mode and thus requires two port nodes,
> > +the first port node expects odd pixels (1, 3, 5, etc.) and the second port
> > +expects even pixels (0, 2, 4, etc.).
> > +
> > +Example
> > +-------
> > +
> > + panel {
> > + compatible = "advantech,idk-2121wr", "panel-lvds";
> > +
> > + width-mm = <476>;
> > + height-mm = <268>;
> > +
> > + data-mapping = "vesa-24";
> > +
> > + panel-timing {
> > + clock-frequency = <148500000>;
> > + hactive = <1920>;
> > + vactive = <1080>;
> > + hsync-len = <44>;
> > + hfront-porch = <88>;
> > + hback-porch = <148>;
> > + vfront-porch = <4>;
> > + vback-porch = <36>;
> > + vsync-len = <5>;
> > + };
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + /* Odd pixels */
> > + reg = <0>;
> > + panel_in0: endpoint {
> > + remote-endpoint = <&lvds0_out>;
> > + };
> > + };
> > +
> > + port@1 {
> > + /* Even pixels */
> > + reg = <1>;
> > + panel_in1: endpoint {
> > + remote-endpoint = <&lvds1_out>;
> > + };
> > + };
> > + };
> > + };
> > --
> > 2.7.4
> >
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 11:04 [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Fabrizio Castro
2019-08-15 11:04 ` [PATCH v2 1/9] dt-bindings: panel: lvds: Add dual-link LVDS display support Fabrizio Castro
2019-08-15 11:04 ` [PATCH v2 2/9] dt-bindings: display: Add bindings for Advantech IDK-2121WR Fabrizio Castro
@ 2019-08-15 11:04 ` Fabrizio Castro
2019-08-15 13:18 ` Laurent Pinchart
2019-08-15 11:04 ` [PATCH v2 4/9] drm/timings: Add link flags Fabrizio Castro
` (6 subsequent siblings)
9 siblings, 1 reply; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 11:04 UTC (permalink / raw)
To: Laurent Pinchart, Andrzej Hajda, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Sean Paul
Cc: Fabrizio Castro, Laurent Pinchart, Eric Anholt, linux-kernel,
dri-devel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-renesas-soc, Kieran Bingham, Jacopo Mondi
The information represented by drm_bridge_timings is also
needed by panels, therefore rename drm_bridge_timings to
drm_timings.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
---
v1->v2:
* new patch
I have copied the license from include/drm/drm_bridge.h as that's
where the struct originally came from. What's the right SPDX license
to use in this case?
drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 ++--
drivers/gpu/drm/bridge/sii902x.c | 2 +-
drivers/gpu/drm/bridge/thc63lvd1024.c | 2 +-
drivers/gpu/drm/bridge/ti-tfp410.c | 6 ++--
drivers/gpu/drm/pl111/pl111_display.c | 2 +-
include/drm/drm_bridge.h | 40 ++---------------------
include/drm/drm_timings.h | 60 +++++++++++++++++++++++++++++++++++
7 files changed, 71 insertions(+), 47 deletions(-)
create mode 100644 include/drm/drm_timings.h
diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index d32885b..bb1d928 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -228,7 +228,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
* NOTE: the ADV7123EP seems to have other timings and need a new timings
* set if used.
*/
-static const struct drm_bridge_timings default_dac_timings = {
+static const struct drm_timings default_dac_timings = {
/* Timing specifications, datasheet page 7 */
.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
.setup_time_ps = 500,
@@ -239,7 +239,7 @@ static const struct drm_bridge_timings default_dac_timings = {
* Information taken from the THS8134, THS8134A, THS8134B datasheet named
* "SLVS205D", dated May 1990, revised March 2000.
*/
-static const struct drm_bridge_timings ti_ths8134_dac_timings = {
+static const struct drm_timings ti_ths8134_dac_timings = {
/* From timing diagram, datasheet page 9 */
.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
/* From datasheet, page 12 */
@@ -252,7 +252,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = {
* Information taken from the THS8135 datasheet named "SLAS343B", dated
* May 2001, revised April 2013.
*/
-static const struct drm_bridge_timings ti_ths8135_dac_timings = {
+static const struct drm_timings ti_ths8135_dac_timings = {
/* From timing diagram, datasheet page 14 */
.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
/* From datasheet, page 16 */
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index dd7aa46..0c63065 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -914,7 +914,7 @@ static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
return 0;
}
-static const struct drm_bridge_timings default_sii902x_timings = {
+static const struct drm_timings default_sii902x_timings = {
.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE
| DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE
| DRM_BUS_FLAG_DE_HIGH,
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
index 3d74129b..9047a9e 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -34,7 +34,7 @@ struct thc63_dev {
struct drm_bridge bridge;
struct drm_bridge *next;
- struct drm_bridge_timings timings;
+ struct drm_timings timings;
};
static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index dbf35c7..c086b06c 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -32,7 +32,7 @@ struct tfp410 {
struct delayed_work hpd_work;
struct gpio_desc *powerdown;
- struct drm_bridge_timings timings;
+ struct drm_timings timings;
struct device *dev;
};
@@ -190,7 +190,7 @@ static irqreturn_t tfp410_hpd_irq_thread(int irq, void *arg)
return IRQ_HANDLED;
}
-static const struct drm_bridge_timings tfp410_default_timings = {
+static const struct drm_timings tfp410_default_timings = {
.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE
| DRM_BUS_FLAG_DE_HIGH,
.setup_time_ps = 1200,
@@ -199,7 +199,7 @@ static const struct drm_bridge_timings tfp410_default_timings = {
static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
{
- struct drm_bridge_timings *timings = &dvi->timings;
+ struct drm_timings *timings = &dvi->timings;
struct device_node *ep;
u32 pclk_sample = 0;
u32 bus_width = 24;
diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
index 15d2755..c82b21f 100644
--- a/drivers/gpu/drm/pl111/pl111_display.c
+++ b/drivers/gpu/drm/pl111/pl111_display.c
@@ -188,7 +188,7 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
}
if (bridge) {
- const struct drm_bridge_timings *btimings = bridge->timings;
+ const struct drm_timings *btimings = bridge->timings;
/*
* Here is when things get really fun. Sometimes the bridge
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 7616f65..8270a38 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -27,9 +27,9 @@
#include <linux/ctype.h>
#include <drm/drm_mode_object.h>
#include <drm/drm_modes.h>
+#include <drm/drm_timings.h>
struct drm_bridge;
-struct drm_bridge_timings;
struct drm_panel;
/**
@@ -337,42 +337,6 @@ struct drm_bridge_funcs {
};
/**
- * struct drm_bridge_timings - timing information for the bridge
- */
-struct drm_bridge_timings {
- /**
- * @input_bus_flags:
- *
- * Tells what additional settings for the pixel data on the bus
- * this bridge requires (like pixel signal polarity). See also
- * &drm_display_info->bus_flags.
- */
- u32 input_bus_flags;
- /**
- * @setup_time_ps:
- *
- * Defines the time in picoseconds the input data lines must be
- * stable before the clock edge.
- */
- u32 setup_time_ps;
- /**
- * @hold_time_ps:
- *
- * Defines the time in picoseconds taken for the bridge to sample the
- * input signal after the clock edge.
- */
- u32 hold_time_ps;
- /**
- * @dual_link:
- *
- * True if the bus operates in dual-link mode. The exact meaning is
- * dependent on the bus type. For LVDS buses, this indicates that even-
- * and odd-numbered pixels are received on separate links.
- */
- bool dual_link;
-};
-
-/**
* struct drm_bridge - central DRM bridge control structure
*/
struct drm_bridge {
@@ -393,7 +357,7 @@ struct drm_bridge {
*
* the timing specification for the bridge, if any (may be NULL)
*/
- const struct drm_bridge_timings *timings;
+ const struct drm_timings *timings;
/** @funcs: control functions */
const struct drm_bridge_funcs *funcs;
/** @driver_private: pointer to the bridge driver's internal context */
diff --git a/include/drm/drm_timings.h b/include/drm/drm_timings.h
new file mode 100644
index 0000000..4af8814
--- /dev/null
+++ b/include/drm/drm_timings.h
@@ -0,0 +1,60 @@
+/*
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that copyright
+ * notice and this permission notice appear in supporting documentation, and
+ * that the name of the copyright holders not be used in advertising or
+ * publicity pertaining to distribution of the software without specific,
+ * written prior permission. The copyright holders make no representations
+ * about the suitability of this software for any purpose. It is provided "as
+ * is" without express or implied warranty.
+ *
+ * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
+ * OF THIS SOFTWARE.
+ */
+
+#ifndef __DRM_TIMINGS_H__
+#define __DRM_TIMINGS_H__
+
+/**
+ * struct drm_timings - timing information
+ */
+struct drm_timings {
+ /**
+ * @input_bus_flags:
+ *
+ * Tells what additional settings for the pixel data on the bus
+ * are required (like pixel signal polarity). See also
+ * &drm_display_info->bus_flags.
+ */
+ u32 input_bus_flags;
+ /**
+ * @setup_time_ps:
+ *
+ * Defines the time in picoseconds the input data lines must be
+ * stable before the clock edge.
+ */
+ u32 setup_time_ps;
+ /**
+ * @hold_time_ps:
+ *
+ * Defines the time in picoseconds taken for the bridge to sample the
+ * input signal after the clock edge.
+ */
+ u32 hold_time_ps;
+ /**
+ * @dual_link:
+ *
+ * True if the bus operates in dual-link mode. The exact meaning is
+ * dependent on the bus type. For LVDS buses, this indicates that even-
+ * and odd-numbered pixels are received on separate links.
+ */
+ bool dual_link;
+};
+
+#endif /* __DRM_TIMINGS_H__ */
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 11:04 ` [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings Fabrizio Castro
@ 2019-08-15 13:18 ` Laurent Pinchart
2019-08-15 13:50 ` Fabrizio Castro
2019-08-15 14:04 ` Greg Kroah-Hartman
0 siblings, 2 replies; 40+ messages in thread
From: Laurent Pinchart @ 2019-08-15 13:18 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Andrzej Hajda, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Sean Paul, Eric Anholt, linux-kernel, dri-devel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc, Kieran Bingham, Jacopo Mondi,
Greg Kroah-Hartman
Hi Fabrizio,
(CC'ing Greg as the architect of the SPDX move)
On Thu, Aug 15, 2019 at 12:04:27PM +0100, Fabrizio Castro wrote:
> The information represented by drm_bridge_timings is also
> needed by panels, therefore rename drm_bridge_timings to
> drm_timings.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
>
> ---
> v1->v2:
> * new patch
>
> I have copied the license from include/drm/drm_bridge.h as that's
> where the struct originally came from. What's the right SPDX license
> to use in this case?
https://wiki.spdx.org/view/Legal_Team/Decisions/Dealing_with_Public_Domain_within_SPDX_Files
Greg, any idea on how we should handle this ?
> drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 ++--
> drivers/gpu/drm/bridge/sii902x.c | 2 +-
> drivers/gpu/drm/bridge/thc63lvd1024.c | 2 +-
> drivers/gpu/drm/bridge/ti-tfp410.c | 6 ++--
> drivers/gpu/drm/pl111/pl111_display.c | 2 +-
> include/drm/drm_bridge.h | 40 ++---------------------
> include/drm/drm_timings.h | 60 +++++++++++++++++++++++++++++++++++
> 7 files changed, 71 insertions(+), 47 deletions(-)
> create mode 100644 include/drm/drm_timings.h
>
> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> index d32885b..bb1d928 100644
> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> @@ -228,7 +228,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
> * NOTE: the ADV7123EP seems to have other timings and need a new timings
> * set if used.
> */
> -static const struct drm_bridge_timings default_dac_timings = {
> +static const struct drm_timings default_dac_timings = {
> /* Timing specifications, datasheet page 7 */
> .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> .setup_time_ps = 500,
> @@ -239,7 +239,7 @@ static const struct drm_bridge_timings default_dac_timings = {
> * Information taken from the THS8134, THS8134A, THS8134B datasheet named
> * "SLVS205D", dated May 1990, revised March 2000.
> */
> -static const struct drm_bridge_timings ti_ths8134_dac_timings = {
> +static const struct drm_timings ti_ths8134_dac_timings = {
> /* From timing diagram, datasheet page 9 */
> .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> /* From datasheet, page 12 */
> @@ -252,7 +252,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = {
> * Information taken from the THS8135 datasheet named "SLAS343B", dated
> * May 2001, revised April 2013.
> */
> -static const struct drm_bridge_timings ti_ths8135_dac_timings = {
> +static const struct drm_timings ti_ths8135_dac_timings = {
> /* From timing diagram, datasheet page 14 */
> .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> /* From datasheet, page 16 */
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index dd7aa46..0c63065 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -914,7 +914,7 @@ static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
> return 0;
> }
>
> -static const struct drm_bridge_timings default_sii902x_timings = {
> +static const struct drm_timings default_sii902x_timings = {
> .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE
> | DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE
> | DRM_BUS_FLAG_DE_HIGH,
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> index 3d74129b..9047a9e 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -34,7 +34,7 @@ struct thc63_dev {
> struct drm_bridge bridge;
> struct drm_bridge *next;
>
> - struct drm_bridge_timings timings;
> + struct drm_timings timings;
> };
>
> static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index dbf35c7..c086b06c 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -32,7 +32,7 @@ struct tfp410 {
> struct delayed_work hpd_work;
> struct gpio_desc *powerdown;
>
> - struct drm_bridge_timings timings;
> + struct drm_timings timings;
>
> struct device *dev;
> };
> @@ -190,7 +190,7 @@ static irqreturn_t tfp410_hpd_irq_thread(int irq, void *arg)
> return IRQ_HANDLED;
> }
>
> -static const struct drm_bridge_timings tfp410_default_timings = {
> +static const struct drm_timings tfp410_default_timings = {
> .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE
> | DRM_BUS_FLAG_DE_HIGH,
> .setup_time_ps = 1200,
> @@ -199,7 +199,7 @@ static const struct drm_bridge_timings tfp410_default_timings = {
>
> static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
> {
> - struct drm_bridge_timings *timings = &dvi->timings;
> + struct drm_timings *timings = &dvi->timings;
> struct device_node *ep;
> u32 pclk_sample = 0;
> u32 bus_width = 24;
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index 15d2755..c82b21f 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
> @@ -188,7 +188,7 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
> }
>
> if (bridge) {
> - const struct drm_bridge_timings *btimings = bridge->timings;
> + const struct drm_timings *btimings = bridge->timings;
>
> /*
> * Here is when things get really fun. Sometimes the bridge
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 7616f65..8270a38 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -27,9 +27,9 @@
> #include <linux/ctype.h>
> #include <drm/drm_mode_object.h>
> #include <drm/drm_modes.h>
> +#include <drm/drm_timings.h>
>
> struct drm_bridge;
> -struct drm_bridge_timings;
> struct drm_panel;
>
> /**
> @@ -337,42 +337,6 @@ struct drm_bridge_funcs {
> };
>
> /**
> - * struct drm_bridge_timings - timing information for the bridge
> - */
> -struct drm_bridge_timings {
> - /**
> - * @input_bus_flags:
> - *
> - * Tells what additional settings for the pixel data on the bus
> - * this bridge requires (like pixel signal polarity). See also
> - * &drm_display_info->bus_flags.
> - */
> - u32 input_bus_flags;
> - /**
> - * @setup_time_ps:
> - *
> - * Defines the time in picoseconds the input data lines must be
> - * stable before the clock edge.
> - */
> - u32 setup_time_ps;
> - /**
> - * @hold_time_ps:
> - *
> - * Defines the time in picoseconds taken for the bridge to sample the
> - * input signal after the clock edge.
> - */
> - u32 hold_time_ps;
> - /**
> - * @dual_link:
> - *
> - * True if the bus operates in dual-link mode. The exact meaning is
> - * dependent on the bus type. For LVDS buses, this indicates that even-
> - * and odd-numbered pixels are received on separate links.
> - */
> - bool dual_link;
> -};
> -
> -/**
> * struct drm_bridge - central DRM bridge control structure
> */
> struct drm_bridge {
> @@ -393,7 +357,7 @@ struct drm_bridge {
> *
> * the timing specification for the bridge, if any (may be NULL)
> */
> - const struct drm_bridge_timings *timings;
> + const struct drm_timings *timings;
> /** @funcs: control functions */
> const struct drm_bridge_funcs *funcs;
> /** @driver_private: pointer to the bridge driver's internal context */
> diff --git a/include/drm/drm_timings.h b/include/drm/drm_timings.h
> new file mode 100644
> index 0000000..4af8814
> --- /dev/null
> +++ b/include/drm/drm_timings.h
> @@ -0,0 +1,60 @@
> +/*
> + * Permission to use, copy, modify, distribute, and sell this software and its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission. The copyright holders make no representations
> + * about the suitability of this software for any purpose. It is provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +#ifndef __DRM_TIMINGS_H__
> +#define __DRM_TIMINGS_H__
> +
> +/**
> + * struct drm_timings - timing information
This dangerously relates to video timings. I would name the structure
drm_bus_timings, or drm_bus_params (or something similar) as it contains
more than timings.
> + */
> +struct drm_timings {
> + /**
> + * @input_bus_flags:
> + *
> + * Tells what additional settings for the pixel data on the bus
> + * are required (like pixel signal polarity). See also
> + * &drm_display_info->bus_flags.
> + */
> + u32 input_bus_flags;
> + /**
> + * @setup_time_ps:
> + *
> + * Defines the time in picoseconds the input data lines must be
> + * stable before the clock edge.
> + */
> + u32 setup_time_ps;
> + /**
> + * @hold_time_ps:
> + *
> + * Defines the time in picoseconds taken for the bridge to sample the
> + * input signal after the clock edge.
> + */
> + u32 hold_time_ps;
> + /**
> + * @dual_link:
> + *
> + * True if the bus operates in dual-link mode. The exact meaning is
> + * dependent on the bus type. For LVDS buses, this indicates that even-
> + * and odd-numbered pixels are received on separate links.
> + */
> + bool dual_link;
> +};
> +
> +#endif /* __DRM_TIMINGS_H__ */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 13:18 ` Laurent Pinchart
@ 2019-08-15 13:50 ` Fabrizio Castro
2019-08-15 14:04 ` Greg Kroah-Hartman
1 sibling, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 13:50 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Andrzej Hajda, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Sean Paul, Eric Anholt, linux-kernel, dri-devel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc, Kieran Bingham, Jacopo Mondi,
Greg Kroah-Hartman
Hello Laurent,
Thank you for your feedback!
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 15 August 2019 14:19
> Subject: Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
>
> Hi Fabrizio,
>
> (CC'ing Greg as the architect of the SPDX move)
>
> On Thu, Aug 15, 2019 at 12:04:27PM +0100, Fabrizio Castro wrote:
> > The information represented by drm_bridge_timings is also
> > needed by panels, therefore rename drm_bridge_timings to
> > drm_timings.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
> >
> > ---
> > v1->v2:
> > * new patch
> >
> > I have copied the license from include/drm/drm_bridge.h as that's
> > where the struct originally came from. What's the right SPDX license
> > to use in this case?
>
> https://wiki.spdx.org/view/Legal_Team/Decisions/Dealing_with_Public_Domain_within_SPDX_Files
>
> Greg, any idea on how we should handle this ?
>
> > drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 ++--
> > drivers/gpu/drm/bridge/sii902x.c | 2 +-
> > drivers/gpu/drm/bridge/thc63lvd1024.c | 2 +-
> > drivers/gpu/drm/bridge/ti-tfp410.c | 6 ++--
> > drivers/gpu/drm/pl111/pl111_display.c | 2 +-
> > include/drm/drm_bridge.h | 40 ++---------------------
> > include/drm/drm_timings.h | 60 +++++++++++++++++++++++++++++++++++
> > 7 files changed, 71 insertions(+), 47 deletions(-)
> > create mode 100644 include/drm/drm_timings.h
> >
> > diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > index d32885b..bb1d928 100644
> > --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> > @@ -228,7 +228,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
> > * NOTE: the ADV7123EP seems to have other timings and need a new timings
> > * set if used.
> > */
> > -static const struct drm_bridge_timings default_dac_timings = {
> > +static const struct drm_timings default_dac_timings = {
> > /* Timing specifications, datasheet page 7 */
> > .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> > .setup_time_ps = 500,
> > @@ -239,7 +239,7 @@ static const struct drm_bridge_timings default_dac_timings = {
> > * Information taken from the THS8134, THS8134A, THS8134B datasheet named
> > * "SLVS205D", dated May 1990, revised March 2000.
> > */
> > -static const struct drm_bridge_timings ti_ths8134_dac_timings = {
> > +static const struct drm_timings ti_ths8134_dac_timings = {
> > /* From timing diagram, datasheet page 9 */
> > .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> > /* From datasheet, page 12 */
> > @@ -252,7 +252,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = {
> > * Information taken from the THS8135 datasheet named "SLAS343B", dated
> > * May 2001, revised April 2013.
> > */
> > -static const struct drm_bridge_timings ti_ths8135_dac_timings = {
> > +static const struct drm_timings ti_ths8135_dac_timings = {
> > /* From timing diagram, datasheet page 14 */
> > .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> > /* From datasheet, page 16 */
> > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> > index dd7aa46..0c63065 100644
> > --- a/drivers/gpu/drm/bridge/sii902x.c
> > +++ b/drivers/gpu/drm/bridge/sii902x.c
> > @@ -914,7 +914,7 @@ static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
> > return 0;
> > }
> >
> > -static const struct drm_bridge_timings default_sii902x_timings = {
> > +static const struct drm_timings default_sii902x_timings = {
> > .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE
> > | DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE
> > | DRM_BUS_FLAG_DE_HIGH,
> > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > index 3d74129b..9047a9e 100644
> > --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -34,7 +34,7 @@ struct thc63_dev {
> > struct drm_bridge bridge;
> > struct drm_bridge *next;
> >
> > - struct drm_bridge_timings timings;
> > + struct drm_timings timings;
> > };
> >
> > static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> > diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> > index dbf35c7..c086b06c 100644
> > --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> > +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> > @@ -32,7 +32,7 @@ struct tfp410 {
> > struct delayed_work hpd_work;
> > struct gpio_desc *powerdown;
> >
> > - struct drm_bridge_timings timings;
> > + struct drm_timings timings;
> >
> > struct device *dev;
> > };
> > @@ -190,7 +190,7 @@ static irqreturn_t tfp410_hpd_irq_thread(int irq, void *arg)
> > return IRQ_HANDLED;
> > }
> >
> > -static const struct drm_bridge_timings tfp410_default_timings = {
> > +static const struct drm_timings tfp410_default_timings = {
> > .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE
> > | DRM_BUS_FLAG_DE_HIGH,
> > .setup_time_ps = 1200,
> > @@ -199,7 +199,7 @@ static const struct drm_bridge_timings tfp410_default_timings = {
> >
> > static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
> > {
> > - struct drm_bridge_timings *timings = &dvi->timings;
> > + struct drm_timings *timings = &dvi->timings;
> > struct device_node *ep;
> > u32 pclk_sample = 0;
> > u32 bus_width = 24;
> > diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> > index 15d2755..c82b21f 100644
> > --- a/drivers/gpu/drm/pl111/pl111_display.c
> > +++ b/drivers/gpu/drm/pl111/pl111_display.c
> > @@ -188,7 +188,7 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
> > }
> >
> > if (bridge) {
> > - const struct drm_bridge_timings *btimings = bridge->timings;
> > + const struct drm_timings *btimings = bridge->timings;
> >
> > /*
> > * Here is when things get really fun. Sometimes the bridge
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 7616f65..8270a38 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -27,9 +27,9 @@
> > #include <linux/ctype.h>
> > #include <drm/drm_mode_object.h>
> > #include <drm/drm_modes.h>
> > +#include <drm/drm_timings.h>
> >
> > struct drm_bridge;
> > -struct drm_bridge_timings;
> > struct drm_panel;
> >
> > /**
> > @@ -337,42 +337,6 @@ struct drm_bridge_funcs {
> > };
> >
> > /**
> > - * struct drm_bridge_timings - timing information for the bridge
> > - */
> > -struct drm_bridge_timings {
> > - /**
> > - * @input_bus_flags:
> > - *
> > - * Tells what additional settings for the pixel data on the bus
> > - * this bridge requires (like pixel signal polarity). See also
> > - * &drm_display_info->bus_flags.
> > - */
> > - u32 input_bus_flags;
> > - /**
> > - * @setup_time_ps:
> > - *
> > - * Defines the time in picoseconds the input data lines must be
> > - * stable before the clock edge.
> > - */
> > - u32 setup_time_ps;
> > - /**
> > - * @hold_time_ps:
> > - *
> > - * Defines the time in picoseconds taken for the bridge to sample the
> > - * input signal after the clock edge.
> > - */
> > - u32 hold_time_ps;
> > - /**
> > - * @dual_link:
> > - *
> > - * True if the bus operates in dual-link mode. The exact meaning is
> > - * dependent on the bus type. For LVDS buses, this indicates that even-
> > - * and odd-numbered pixels are received on separate links.
> > - */
> > - bool dual_link;
> > -};
> > -
> > -/**
> > * struct drm_bridge - central DRM bridge control structure
> > */
> > struct drm_bridge {
> > @@ -393,7 +357,7 @@ struct drm_bridge {
> > *
> > * the timing specification for the bridge, if any (may be NULL)
> > */
> > - const struct drm_bridge_timings *timings;
> > + const struct drm_timings *timings;
> > /** @funcs: control functions */
> > const struct drm_bridge_funcs *funcs;
> > /** @driver_private: pointer to the bridge driver's internal context */
> > diff --git a/include/drm/drm_timings.h b/include/drm/drm_timings.h
> > new file mode 100644
> > index 0000000..4af8814
> > --- /dev/null
> > +++ b/include/drm/drm_timings.h
> > @@ -0,0 +1,60 @@
> > +/*
> > + * Permission to use, copy, modify, distribute, and sell this software and its
> > + * documentation for any purpose is hereby granted without fee, provided that
> > + * the above copyright notice appear in all copies and that both that copyright
> > + * notice and this permission notice appear in supporting documentation, and
> > + * that the name of the copyright holders not be used in advertising or
> > + * publicity pertaining to distribution of the software without specific,
> > + * written prior permission. The copyright holders make no representations
> > + * about the suitability of this software for any purpose. It is provided "as
> > + * is" without express or implied warranty.
> > + *
> > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > + * OF THIS SOFTWARE.
> > + */
> > +
> > +#ifndef __DRM_TIMINGS_H__
> > +#define __DRM_TIMINGS_H__
> > +
> > +/**
> > + * struct drm_timings - timing information
>
> This dangerously relates to video timings. I would name the structure
> drm_bus_timings, or drm_bus_params (or something similar) as it contains
> more than timings.
Will rename to drm_bus_timings.
Thanks,
Fab
>
> > + */
> > +struct drm_timings {
> > + /**
> > + * @input_bus_flags:
> > + *
> > + * Tells what additional settings for the pixel data on the bus
> > + * are required (like pixel signal polarity). See also
> > + * &drm_display_info->bus_flags.
> > + */
> > + u32 input_bus_flags;
> > + /**
> > + * @setup_time_ps:
> > + *
> > + * Defines the time in picoseconds the input data lines must be
> > + * stable before the clock edge.
> > + */
> > + u32 setup_time_ps;
> > + /**
> > + * @hold_time_ps:
> > + *
> > + * Defines the time in picoseconds taken for the bridge to sample the
> > + * input signal after the clock edge.
> > + */
> > + u32 hold_time_ps;
> > + /**
> > + * @dual_link:
> > + *
> > + * True if the bus operates in dual-link mode. The exact meaning is
> > + * dependent on the bus type. For LVDS buses, this indicates that even-
> > + * and odd-numbered pixels are received on separate links.
> > + */
> > + bool dual_link;
> > +};
> > +
> > +#endif /* __DRM_TIMINGS_H__ */
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 13:18 ` Laurent Pinchart
2019-08-15 13:50 ` Fabrizio Castro
@ 2019-08-15 14:04 ` Greg Kroah-Hartman
2019-08-15 14:14 ` Laurent Pinchart
1 sibling, 1 reply; 40+ messages in thread
From: Greg Kroah-Hartman @ 2019-08-15 14:04 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Fabrizio Castro, Andrzej Hajda, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Sean Paul, Eric Anholt,
linux-kernel, dri-devel, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, linux-renesas-soc, Kieran Bingham,
Jacopo Mondi
On Thu, Aug 15, 2019 at 04:18:38PM +0300, Laurent Pinchart wrote:
> Hi Fabrizio,
>
> (CC'ing Greg as the architect of the SPDX move)
_one of_, not the one that did the most of he work, that would be Thomas :)
> On Thu, Aug 15, 2019 at 12:04:27PM +0100, Fabrizio Castro wrote:
> > The information represented by drm_bridge_timings is also
> > needed by panels, therefore rename drm_bridge_timings to
> > drm_timings.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
> >
> > ---
> > v1->v2:
> > * new patch
> >
> > I have copied the license from include/drm/drm_bridge.h as that's
> > where the struct originally came from. What's the right SPDX license
> > to use in this case?
>
> https://wiki.spdx.org/view/Legal_Team/Decisions/Dealing_with_Public_Domain_within_SPDX_Files
>
> Greg, any idea on how we should handle this ?
Ugh, what lunacy. But drm_bridge.h is NOT under any "public domain"
license, so why is that an issue here? This looks like a "normal" bsd 3
clause license to me, right?
So I would just use "BSD-3-Clause" as the SPDX license here, if I were
doing this patch...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 14:04 ` Greg Kroah-Hartman
@ 2019-08-15 14:14 ` Laurent Pinchart
2019-08-15 14:31 ` Fabrizio Castro
0 siblings, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2019-08-15 14:14 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Fabrizio Castro, Andrzej Hajda, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Sean Paul, Eric Anholt,
linux-kernel, dri-devel, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, linux-renesas-soc, Kieran Bingham,
Jacopo Mondi
Hi Greg,
On Thu, Aug 15, 2019 at 04:04:00PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 15, 2019 at 04:18:38PM +0300, Laurent Pinchart wrote:
> > Hi Fabrizio,
> >
> > (CC'ing Greg as the architect of the SPDX move)
>
> _one of_, not the one that did the most of he work, that would be Thomas :)
>
> > On Thu, Aug 15, 2019 at 12:04:27PM +0100, Fabrizio Castro wrote:
> > > The information represented by drm_bridge_timings is also
> > > needed by panels, therefore rename drm_bridge_timings to
> > > drm_timings.
> > >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
> > >
> > > ---
> > > v1->v2:
> > > * new patch
> > >
> > > I have copied the license from include/drm/drm_bridge.h as that's
> > > where the struct originally came from. What's the right SPDX license
> > > to use in this case?
> >
> > https://wiki.spdx.org/view/Legal_Team/Decisions/Dealing_with_Public_Domain_within_SPDX_Files
> >
> > Greg, any idea on how we should handle this ?
>
> Ugh, what lunacy. But drm_bridge.h is NOT under any "public domain"
> license, so why is that an issue here? This looks like a "normal" bsd 3
> clause license to me, right?
You're right, I overread part of the text in drm_bridge.h, it seems to
indeed be covered by a BSD 3 clause license. Sorry for the noise.
> So I would just use "BSD-3-Clause" as the SPDX license here, if I were
> doing this patch...
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 14:14 ` Laurent Pinchart
@ 2019-08-15 14:31 ` Fabrizio Castro
2019-08-15 14:53 ` Greg Kroah-Hartman
0 siblings, 1 reply; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 14:31 UTC (permalink / raw)
To: Laurent Pinchart, Greg Kroah-Hartman
Cc: Andrzej Hajda, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Sean Paul, Eric Anholt, linux-kernel, dri-devel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc, Kieran Bingham, Jacopo Mondi
Hi Greg, hi Laurent,
Thank you for your feedback!
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 15 August 2019 15:15
> Subject: Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
>
> Hi Greg,
>
> On Thu, Aug 15, 2019 at 04:04:00PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 15, 2019 at 04:18:38PM +0300, Laurent Pinchart wrote:
> > > Hi Fabrizio,
> > >
> > > (CC'ing Greg as the architect of the SPDX move)
> >
> > _one of_, not the one that did the most of he work, that would be Thomas :)
> >
> > > On Thu, Aug 15, 2019 at 12:04:27PM +0100, Fabrizio Castro wrote:
> > > > The information represented by drm_bridge_timings is also
> > > > needed by panels, therefore rename drm_bridge_timings to
> > > > drm_timings.
> > > >
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
> > > >
> > > > ---
> > > > v1->v2:
> > > > * new patch
> > > >
> > > > I have copied the license from include/drm/drm_bridge.h as that's
> > > > where the struct originally came from. What's the right SPDX license
> > > > to use in this case?
> > >
> > > https://wiki.spdx.org/view/Legal_Team/Decisions/Dealing_with_Public_Domain_within_SPDX_Files
> > >
> > > Greg, any idea on how we should handle this ?
> >
> > Ugh, what lunacy. But drm_bridge.h is NOT under any "public domain"
> > license, so why is that an issue here? This looks like a "normal" bsd 3
> > clause license to me, right?
>
> You're right, I overread part of the text in drm_bridge.h, it seems to
> indeed be covered by a BSD 3 clause license. Sorry for the noise.
Mmm... This is the template for the BSD-3-Clause:
Copyright (c) <YEAR>, <OWNER>
All rights reserved.
Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:
Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.
Neither the name of the <ORGANIZATION> nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission.
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
And this is the license coming from include/drm/drm_bridge.h:
/*
* Copyright (c) 2016 Intel Corporation
*
* Permission to use, copy, modify, distribute, and sell this software and its
* documentation for any purpose is hereby granted without fee, provided that
* the above copyright notice appear in all copies and that both that copyright
* notice and this permission notice appear in supporting documentation, and
* that the name of the copyright holders not be used in advertising or
* publicity pertaining to distribution of the software without specific,
* written prior permission. The copyright holders make no representations
* about the suitability of this software for any purpose. It is provided "as
* is" without express or implied warranty.
*
* THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
* INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
* EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
* CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
* DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
* TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
* OF THIS SOFTWARE.
*/
Perhaps I am completely wrong here, and I am not a lawyer, but the wording seems different enough to me...
I am happy to use "BSD-3-Clause" though. Laurent please double check.
Thanks!
Fab
>
> > So I would just use "BSD-3-Clause" as the SPDX license here, if I were
> > doing this patch...
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 14:31 ` Fabrizio Castro
@ 2019-08-15 14:53 ` Greg Kroah-Hartman
2019-08-15 15:01 ` Fabrizio Castro
2019-08-15 18:06 ` Laurent Pinchart
0 siblings, 2 replies; 40+ messages in thread
From: Greg Kroah-Hartman @ 2019-08-15 14:53 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Laurent Pinchart, Andrzej Hajda, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Sean Paul, Eric Anholt,
linux-kernel, dri-devel, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, linux-renesas-soc, Kieran Bingham,
Jacopo Mondi
On Thu, Aug 15, 2019 at 02:31:26PM +0000, Fabrizio Castro wrote:
> Hi Greg, hi Laurent,
>
> Thank you for your feedback!
>
> > From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> > Sent: 15 August 2019 15:15
> > Subject: Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
> >
> > Hi Greg,
> >
> > On Thu, Aug 15, 2019 at 04:04:00PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Aug 15, 2019 at 04:18:38PM +0300, Laurent Pinchart wrote:
> > > > Hi Fabrizio,
> > > >
> > > > (CC'ing Greg as the architect of the SPDX move)
> > >
> > > _one of_, not the one that did the most of he work, that would be Thomas :)
> > >
> > > > On Thu, Aug 15, 2019 at 12:04:27PM +0100, Fabrizio Castro wrote:
> > > > > The information represented by drm_bridge_timings is also
> > > > > needed by panels, therefore rename drm_bridge_timings to
> > > > > drm_timings.
> > > > >
> > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > > Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
> > > > >
> > > > > ---
> > > > > v1->v2:
> > > > > * new patch
> > > > >
> > > > > I have copied the license from include/drm/drm_bridge.h as that's
> > > > > where the struct originally came from. What's the right SPDX license
> > > > > to use in this case?
> > > >
> > > > https://wiki.spdx.org/view/Legal_Team/Decisions/Dealing_with_Public_Domain_within_SPDX_Files
> > > >
> > > > Greg, any idea on how we should handle this ?
> > >
> > > Ugh, what lunacy. But drm_bridge.h is NOT under any "public domain"
> > > license, so why is that an issue here? This looks like a "normal" bsd 3
> > > clause license to me, right?
> >
> > You're right, I overread part of the text in drm_bridge.h, it seems to
> > indeed be covered by a BSD 3 clause license. Sorry for the noise.
>
> Mmm... This is the template for the BSD-3-Clause:
>
> Copyright (c) <YEAR>, <OWNER>
> All rights reserved.
>
> Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:
>
> Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
> Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.
> Neither the name of the <ORGANIZATION> nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission.
> THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>
> And this is the license coming from include/drm/drm_bridge.h:
>
> /*
> * Copyright (c) 2016 Intel Corporation
> *
> * Permission to use, copy, modify, distribute, and sell this software and its
> * documentation for any purpose is hereby granted without fee, provided that
> * the above copyright notice appear in all copies and that both that copyright
> * notice and this permission notice appear in supporting documentation, and
> * that the name of the copyright holders not be used in advertising or
> * publicity pertaining to distribution of the software without specific,
> * written prior permission. The copyright holders make no representations
> * about the suitability of this software for any purpose. It is provided "as
> * is" without express or implied warranty.
> *
> * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> * OF THIS SOFTWARE.
> */
>
> Perhaps I am completely wrong here, and I am not a lawyer, but the wording seems different enough to me...
> I am happy to use "BSD-3-Clause" though. Laurent please double check.
Please talk to your lawyers about this, we are not them...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 14:53 ` Greg Kroah-Hartman
@ 2019-08-15 15:01 ` Fabrizio Castro
2019-08-15 18:06 ` Laurent Pinchart
1 sibling, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 15:01 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Laurent Pinchart, Andrzej Hajda, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Sean Paul, Eric Anholt,
linux-kernel, dri-devel, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, linux-renesas-soc, Kieran Bingham,
Jacopo Mondi
Hi Greg, hi Laurent,
Thank you for your feedback!
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: 15 August 2019 15:53
> Subject: Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
>
> On Thu, Aug 15, 2019 at 02:31:26PM +0000, Fabrizio Castro wrote:
> > Hi Greg, hi Laurent,
> >
> > Thank you for your feedback!
> >
> > > From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> > > Sent: 15 August 2019 15:15
> > > Subject: Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
> > >
> > > Hi Greg,
> > >
> > > On Thu, Aug 15, 2019 at 04:04:00PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Aug 15, 2019 at 04:18:38PM +0300, Laurent Pinchart wrote:
> > > > > Hi Fabrizio,
> > > > >
> > > > > (CC'ing Greg as the architect of the SPDX move)
> > > >
> > > > _one of_, not the one that did the most of he work, that would be Thomas :)
> > > >
> > > > > On Thu, Aug 15, 2019 at 12:04:27PM +0100, Fabrizio Castro wrote:
> > > > > > The information represented by drm_bridge_timings is also
> > > > > > needed by panels, therefore rename drm_bridge_timings to
> > > > > > drm_timings.
> > > > > >
> > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > > > Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
> > > > > >
> > > > > > ---
> > > > > > v1->v2:
> > > > > > * new patch
> > > > > >
> > > > > > I have copied the license from include/drm/drm_bridge.h as that's
> > > > > > where the struct originally came from. What's the right SPDX license
> > > > > > to use in this case?
> > > > >
> > > > > https://wiki.spdx.org/view/Legal_Team/Decisions/Dealing_with_Public_Domain_within_SPDX_Files
> > > > >
> > > > > Greg, any idea on how we should handle this ?
> > > >
> > > > Ugh, what lunacy. But drm_bridge.h is NOT under any "public domain"
> > > > license, so why is that an issue here? This looks like a "normal" bsd 3
> > > > clause license to me, right?
> > >
> > > You're right, I overread part of the text in drm_bridge.h, it seems to
> > > indeed be covered by a BSD 3 clause license. Sorry for the noise.
> >
> > Mmm... This is the template for the BSD-3-Clause:
> >
> > Copyright (c) <YEAR>, <OWNER>
> > All rights reserved.
> >
> > Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following
> conditions are met:
> >
> > Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
> > Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the
> documentation and/or other materials provided with the distribution.
> > Neither the name of the <ORGANIZATION> nor the names of its contributors may be used to endorse or promote products derived
> from this software without specific prior written permission.
> > THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED
> WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
> WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >
> > And this is the license coming from include/drm/drm_bridge.h:
> >
> > /*
> > * Copyright (c) 2016 Intel Corporation
> > *
> > * Permission to use, copy, modify, distribute, and sell this software and its
> > * documentation for any purpose is hereby granted without fee, provided that
> > * the above copyright notice appear in all copies and that both that copyright
> > * notice and this permission notice appear in supporting documentation, and
> > * that the name of the copyright holders not be used in advertising or
> > * publicity pertaining to distribution of the software without specific,
> > * written prior permission. The copyright holders make no representations
> > * about the suitability of this software for any purpose. It is provided "as
> > * is" without express or implied warranty.
> > *
> > * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > * OF THIS SOFTWARE.
> > */
> >
> > Perhaps I am completely wrong here, and I am not a lawyer, but the wording seems different enough to me...
> > I am happy to use "BSD-3-Clause" though. Laurent please double check.
>
> Please talk to your lawyers about this, we are not them...
I am really sorry for the trouble (and the waste of time)!
I'll try and use "BSD-3-Clause" for the next version and I'll see what happens.
Thanks,
Fab
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 14:53 ` Greg Kroah-Hartman
2019-08-15 15:01 ` Fabrizio Castro
@ 2019-08-15 18:06 ` Laurent Pinchart
2019-08-15 19:05 ` Greg Kroah-Hartman
1 sibling, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2019-08-15 18:06 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Fabrizio Castro, Andrzej Hajda, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Sean Paul, Eric Anholt,
linux-kernel, dri-devel, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, linux-renesas-soc, Kieran Bingham,
Jacopo Mondi
Hi Greg,
On Thu, Aug 15, 2019 at 04:53:00PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 15, 2019 at 02:31:26PM +0000, Fabrizio Castro wrote:
> > On 15 August 2019 15:15, Laurent Pinchart wrote:
> > > On Thu, Aug 15, 2019 at 04:04:00PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Aug 15, 2019 at 04:18:38PM +0300, Laurent Pinchart wrote:
> > > > > Hi Fabrizio,
> > > > >
> > > > > (CC'ing Greg as the architect of the SPDX move)
> > > >
> > > > _one of_, not the one that did the most of he work, that would be Thomas :)
> > > >
> > > > > On Thu, Aug 15, 2019 at 12:04:27PM +0100, Fabrizio Castro wrote:
> > > > > > The information represented by drm_bridge_timings is also
> > > > > > needed by panels, therefore rename drm_bridge_timings to
> > > > > > drm_timings.
> > > > > >
> > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > > > Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
> > > > > >
> > > > > > ---
> > > > > > v1->v2:
> > > > > > * new patch
> > > > > >
> > > > > > I have copied the license from include/drm/drm_bridge.h as that's
> > > > > > where the struct originally came from. What's the right SPDX license
> > > > > > to use in this case?
> > > > >
> > > > > https://wiki.spdx.org/view/Legal_Team/Decisions/Dealing_with_Public_Domain_within_SPDX_Files
> > > > >
> > > > > Greg, any idea on how we should handle this ?
> > > >
> > > > Ugh, what lunacy. But drm_bridge.h is NOT under any "public domain"
> > > > license, so why is that an issue here? This looks like a "normal" bsd 3
> > > > clause license to me, right?
> > >
> > > You're right, I overread part of the text in drm_bridge.h, it seems to
> > > indeed be covered by a BSD 3 clause license. Sorry for the noise.
> >
> > Mmm... This is the template for the BSD-3-Clause:
> >
> > Copyright (c) <YEAR>, <OWNER>
> > All rights reserved.
> >
> > Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:
> >
> > Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
> > Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.
> > Neither the name of the <ORGANIZATION> nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission.
> > THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >
> > And this is the license coming from include/drm/drm_bridge.h:
> >
> > /*
> > * Copyright (c) 2016 Intel Corporation
> > *
> > * Permission to use, copy, modify, distribute, and sell this software and its
> > * documentation for any purpose is hereby granted without fee, provided that
> > * the above copyright notice appear in all copies and that both that copyright
> > * notice and this permission notice appear in supporting documentation, and
> > * that the name of the copyright holders not be used in advertising or
> > * publicity pertaining to distribution of the software without specific,
> > * written prior permission. The copyright holders make no representations
> > * about the suitability of this software for any purpose. It is provided "as
> > * is" without express or implied warranty.
> > *
> > * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > * OF THIS SOFTWARE.
> > */
> >
> > Perhaps I am completely wrong here, and I am not a lawyer, but the wording seems different enough to me...
> > I am happy to use "BSD-3-Clause" though. Laurent please double check.
>
> Please talk to your lawyers about this, we are not them...
I don't think that's fair though. Fabrizio is reworking kernel code, and
as part of that wondered what SPDX tag to apply to a new file that
contains code moved from an existing file that has no SPDX tag, but the
above copyright notice. He's not trying to change a license, or reword
it. As SPDX is the preferred way of expressing licenses in the kernel,
he legitimately asked for help, and I think we should provide an
official answer for this (which could be not to use SPDX but copy the
license text).
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 18:06 ` Laurent Pinchart
@ 2019-08-15 19:05 ` Greg Kroah-Hartman
2019-08-16 8:11 ` Fabrizio Castro
0 siblings, 1 reply; 40+ messages in thread
From: Greg Kroah-Hartman @ 2019-08-15 19:05 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Fabrizio Castro, Andrzej Hajda, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Sean Paul, Eric Anholt,
linux-kernel, dri-devel, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, linux-renesas-soc, Kieran Bingham,
Jacopo Mondi
On Thu, Aug 15, 2019 at 09:06:41PM +0300, Laurent Pinchart wrote:
> Hi Greg,
>
> On Thu, Aug 15, 2019 at 04:53:00PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 15, 2019 at 02:31:26PM +0000, Fabrizio Castro wrote:
> > > On 15 August 2019 15:15, Laurent Pinchart wrote:
> > > > On Thu, Aug 15, 2019 at 04:04:00PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Thu, Aug 15, 2019 at 04:18:38PM +0300, Laurent Pinchart wrote:
> > > > > > Hi Fabrizio,
> > > > > >
> > > > > > (CC'ing Greg as the architect of the SPDX move)
> > > > >
> > > > > _one of_, not the one that did the most of he work, that would be Thomas :)
> > > > >
> > > > > > On Thu, Aug 15, 2019 at 12:04:27PM +0100, Fabrizio Castro wrote:
> > > > > > > The information represented by drm_bridge_timings is also
> > > > > > > needed by panels, therefore rename drm_bridge_timings to
> > > > > > > drm_timings.
> > > > > > >
> > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > > > > Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
> > > > > > >
> > > > > > > ---
> > > > > > > v1->v2:
> > > > > > > * new patch
> > > > > > >
> > > > > > > I have copied the license from include/drm/drm_bridge.h as that's
> > > > > > > where the struct originally came from. What's the right SPDX license
> > > > > > > to use in this case?
> > > > > >
> > > > > > https://wiki.spdx.org/view/Legal_Team/Decisions/Dealing_with_Public_Domain_within_SPDX_Files
> > > > > >
> > > > > > Greg, any idea on how we should handle this ?
> > > > >
> > > > > Ugh, what lunacy. But drm_bridge.h is NOT under any "public domain"
> > > > > license, so why is that an issue here? This looks like a "normal" bsd 3
> > > > > clause license to me, right?
> > > >
> > > > You're right, I overread part of the text in drm_bridge.h, it seems to
> > > > indeed be covered by a BSD 3 clause license. Sorry for the noise.
> > >
> > > Mmm... This is the template for the BSD-3-Clause:
> > >
> > > Copyright (c) <YEAR>, <OWNER>
> > > All rights reserved.
> > >
> > > Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:
> > >
> > > Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
> > > Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.
> > > Neither the name of the <ORGANIZATION> nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission.
> > > THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > >
> > > And this is the license coming from include/drm/drm_bridge.h:
> > >
> > > /*
> > > * Copyright (c) 2016 Intel Corporation
> > > *
> > > * Permission to use, copy, modify, distribute, and sell this software and its
> > > * documentation for any purpose is hereby granted without fee, provided that
> > > * the above copyright notice appear in all copies and that both that copyright
> > > * notice and this permission notice appear in supporting documentation, and
> > > * that the name of the copyright holders not be used in advertising or
> > > * publicity pertaining to distribution of the software without specific,
> > > * written prior permission. The copyright holders make no representations
> > > * about the suitability of this software for any purpose. It is provided "as
> > > * is" without express or implied warranty.
> > > *
> > > * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > > * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > > * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > > * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > > * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > > * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > > * OF THIS SOFTWARE.
> > > */
> > >
> > > Perhaps I am completely wrong here, and I am not a lawyer, but the wording seems different enough to me...
> > > I am happy to use "BSD-3-Clause" though. Laurent please double check.
> >
> > Please talk to your lawyers about this, we are not them...
>
> I don't think that's fair though. Fabrizio is reworking kernel code, and
> as part of that wondered what SPDX tag to apply to a new file that
> contains code moved from an existing file that has no SPDX tag, but the
> above copyright notice. He's not trying to change a license, or reword
> it. As SPDX is the preferred way of expressing licenses in the kernel,
> he legitimately asked for help, and I think we should provide an
> official answer for this (which could be not to use SPDX but copy the
> license text).
Ah, ok, that makes more sense, didn't realize that.
Fabrizio, just copy the license text as-is to the new file if you are
copying from an existing one. For all of these "we have to read the
text" files that are left in the kernel, we still have a ways to go to
convert them. But, if you leave the text identical, when we match one
and fix it, the tools will catch the other identical ones as well, so
that does not create any extra work.
hope this helps,
greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
2019-08-15 19:05 ` Greg Kroah-Hartman
@ 2019-08-16 8:11 ` Fabrizio Castro
0 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-16 8:11 UTC (permalink / raw)
To: Greg Kroah-Hartman, Laurent Pinchart
Cc: Andrzej Hajda, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Sean Paul, Eric Anholt, linux-kernel, dri-devel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc, Kieran Bingham, Jacopo Mondi
Hi Greg, hi Laurent,
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: 15 August 2019 20:05
> Subject: Re: [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings
>
> On Thu, Aug 15, 2019 at 09:06:41PM +0300, Laurent Pinchart wrote:
> > Hi Greg,
> >
> > On Thu, Aug 15, 2019 at 04:53:00PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Aug 15, 2019 at 02:31:26PM +0000, Fabrizio Castro wrote:
> > > > On 15 August 2019 15:15, Laurent Pinchart wrote:
> > > > > On Thu, Aug 15, 2019 at 04:04:00PM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Thu, Aug 15, 2019 at 04:18:38PM +0300, Laurent Pinchart wrote:
> > > > > > > Hi Fabrizio,
> > > > > > >
> > > > > > > (CC'ing Greg as the architect of the SPDX move)
> > > > > >
> > > > > > _one of_, not the one that did the most of he work, that would be Thomas :)
> > > > > >
> > > > > > > On Thu, Aug 15, 2019 at 12:04:27PM +0100, Fabrizio Castro wrote:
> > > > > > > > The information represented by drm_bridge_timings is also
> > > > > > > > needed by panels, therefore rename drm_bridge_timings to
> > > > > > > > drm_timings.
> > > > > > > >
> > > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > > > > > Link: https://www.spinics.net/lists/linux-renesas-soc/msg43271.html
> > > > > > > >
> > > > > > > > ---
> > > > > > > > v1->v2:
> > > > > > > > * new patch
> > > > > > > >
> > > > > > > > I have copied the license from include/drm/drm_bridge.h as that's
> > > > > > > > where the struct originally came from. What's the right SPDX license
> > > > > > > > to use in this case?
> > > > > > >
> > > > > > > https://wiki.spdx.org/view/Legal_Team/Decisions/Dealing_with_Public_Domain_within_SPDX_Files
> > > > > > >
> > > > > > > Greg, any idea on how we should handle this ?
> > > > > >
> > > > > > Ugh, what lunacy. But drm_bridge.h is NOT under any "public domain"
> > > > > > license, so why is that an issue here? This looks like a "normal" bsd 3
> > > > > > clause license to me, right?
> > > > >
> > > > > You're right, I overread part of the text in drm_bridge.h, it seems to
> > > > > indeed be covered by a BSD 3 clause license. Sorry for the noise.
> > > >
> > > > Mmm... This is the template for the BSD-3-Clause:
> > > >
> > > > Copyright (c) <YEAR>, <OWNER>
> > > > All rights reserved.
> > > >
> > > > Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following
> conditions are met:
> > > >
> > > > Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
> > > > Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in
> the documentation and/or other materials provided with the distribution.
> > > > Neither the name of the <ORGANIZATION> nor the names of its contributors may be used to endorse or promote products
> derived from this software without specific prior written permission.
> > > > THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED
> WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
> WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > > >
> > > > And this is the license coming from include/drm/drm_bridge.h:
> > > >
> > > > /*
> > > > * Copyright (c) 2016 Intel Corporation
> > > > *
> > > > * Permission to use, copy, modify, distribute, and sell this software and its
> > > > * documentation for any purpose is hereby granted without fee, provided that
> > > > * the above copyright notice appear in all copies and that both that copyright
> > > > * notice and this permission notice appear in supporting documentation, and
> > > > * that the name of the copyright holders not be used in advertising or
> > > > * publicity pertaining to distribution of the software without specific,
> > > > * written prior permission. The copyright holders make no representations
> > > > * about the suitability of this software for any purpose. It is provided "as
> > > > * is" without express or implied warranty.
> > > > *
> > > > * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > > > * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > > > * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > > > * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > > > * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > > > * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > > > * OF THIS SOFTWARE.
> > > > */
> > > >
> > > > Perhaps I am completely wrong here, and I am not a lawyer, but the wording seems different enough to me...
> > > > I am happy to use "BSD-3-Clause" though. Laurent please double check.
> > >
> > > Please talk to your lawyers about this, we are not them...
> >
> > I don't think that's fair though. Fabrizio is reworking kernel code, and
> > as part of that wondered what SPDX tag to apply to a new file that
> > contains code moved from an existing file that has no SPDX tag, but the
> > above copyright notice. He's not trying to change a license, or reword
> > it. As SPDX is the preferred way of expressing licenses in the kernel,
> > he legitimately asked for help, and I think we should provide an
> > official answer for this (which could be not to use SPDX but copy the
> > license text).
>
> Ah, ok, that makes more sense, didn't realize that.
>
> Fabrizio, just copy the license text as-is to the new file if you are
> copying from an existing one. For all of these "we have to read the
> text" files that are left in the kernel, we still have a ways to go to
> convert them. But, if you leave the text identical, when we match one
> and fix it, the tools will catch the other identical ones as well, so
> that does not create any extra work.
>
> hope this helps,
It does! Thank you both guys!
Cheers,
Fab
>
> greg k-h
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 4/9] drm/timings: Add link flags
2019-08-15 11:04 [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Fabrizio Castro
` (2 preceding siblings ...)
2019-08-15 11:04 ` [PATCH v2 3/9] drm: Rename drm_bridge_timings to drm_timings Fabrizio Castro
@ 2019-08-15 11:04 ` Fabrizio Castro
2019-08-15 12:00 ` Laurent Pinchart
2019-08-15 11:04 ` [PATCH v2 5/9] drm/panel: Add timings field to drm_panel Fabrizio Castro
` (5 subsequent siblings)
9 siblings, 1 reply; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 11:04 UTC (permalink / raw)
To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard, Sean Paul,
David Airlie, Daniel Vetter
Cc: Fabrizio Castro, dri-devel, linux-kernel, Simon Horman,
Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc,
Kieran Bingham, Jacopo Mondi
We need more information to describe dual-LVDS links, therefore
introduce link_flags.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v1->v2:
* new patch
include/drm/drm_timings.h | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/include/drm/drm_timings.h b/include/drm/drm_timings.h
index 4af8814..58fbf1b 100644
--- a/include/drm/drm_timings.h
+++ b/include/drm/drm_timings.h
@@ -1,4 +1,6 @@
/*
+ * Copyright (C) 2019 Renesas Electronics Corporation
+ *
* Permission to use, copy, modify, distribute, and sell this software and its
* documentation for any purpose is hereby granted without fee, provided that
* the above copyright notice appear in all copies and that both that copyright
@@ -21,6 +23,24 @@
#ifndef __DRM_TIMINGS_H__
#define __DRM_TIMINGS_H__
+#include <linux/bits.h>
+
+/**
+ * enum drm_link_flags - link_flags for &drm_timings
+ *
+ * This enum defines the details of the link.
+ *
+ * @DRM_LINK_DUAL_LVDS_ODD_EVEN: Dual-LVDS link, with odd pixels (1,3,5,
+ * etc.) coming through the first port, and
+ * even pixels (0,2,4,etc.) coming through
+ * the second port. If not specified for a
+ * dual-LVDS panel, it is assumed that even
+ * pixels are coming through the first port
+ */
+enum drm_link_flags {
+ DRM_LINK_DUAL_LVDS_ODD_EVEN = BIT(0),
+};
+
/**
* struct drm_timings - timing information
*/
@@ -55,6 +75,12 @@ struct drm_timings {
* and odd-numbered pixels are received on separate links.
*/
bool dual_link;
+ /**
+ * @link_flags
+ *
+ * Provides detailed information about the link.
+ */
+ enum drm_link_flags link_flags;
};
#endif /* __DRM_TIMINGS_H__ */
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 4/9] drm/timings: Add link flags
2019-08-15 11:04 ` [PATCH v2 4/9] drm/timings: Add link flags Fabrizio Castro
@ 2019-08-15 12:00 ` Laurent Pinchart
2019-08-15 15:40 ` Fabrizio Castro
0 siblings, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2019-08-15 12:00 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, dri-devel, linux-kernel, Simon Horman,
Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc,
Kieran Bingham, Jacopo Mondi
Hi Fabrizio,
Thank you for the patch.
On Thu, Aug 15, 2019 at 12:04:28PM +0100, Fabrizio Castro wrote:
> We need more information to describe dual-LVDS links, therefore
> introduce link_flags.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * new patch
>
> include/drm/drm_timings.h | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/include/drm/drm_timings.h b/include/drm/drm_timings.h
> index 4af8814..58fbf1b 100644
> --- a/include/drm/drm_timings.h
> +++ b/include/drm/drm_timings.h
> @@ -1,4 +1,6 @@
> /*
> + * Copyright (C) 2019 Renesas Electronics Corporation
> + *
> * Permission to use, copy, modify, distribute, and sell this software and its
> * documentation for any purpose is hereby granted without fee, provided that
> * the above copyright notice appear in all copies and that both that copyright
> @@ -21,6 +23,24 @@
> #ifndef __DRM_TIMINGS_H__
> #define __DRM_TIMINGS_H__
>
> +#include <linux/bits.h>
> +
> +/**
> + * enum drm_link_flags - link_flags for &drm_timings
> + *
> + * This enum defines the details of the link.
> + *
> + * @DRM_LINK_DUAL_LVDS_ODD_EVEN: Dual-LVDS link, with odd pixels (1,3,5,
> + * etc.) coming through the first port, and
> + * even pixels (0,2,4,etc.) coming through
> + * the second port. If not specified for a
> + * dual-LVDS panel, it is assumed that even
> + * pixels are coming through the first port
> + */
> +enum drm_link_flags {
The text will be easier to read if you inline it here.
/**
* @DRM_LINK_DUAL_LVDS_ODD_EVEN: Dual-LVDS link, with odd pixels (1,3,5,
* etc.) coming through the first port, and even pixels (0,2,4,etc.)
...
> + DRM_LINK_DUAL_LVDS_ODD_EVEN = BIT(0),
I would remove the dual_link field and add a DRM_LINK_DUAL_LVDS (or
alternatively two flags, dual lvds odd-even and dual lvds even-odd).
> +};
> +
> /**
> * struct drm_timings - timing information
> */
> @@ -55,6 +75,12 @@ struct drm_timings {
> * and odd-numbered pixels are received on separate links.
> */
> bool dual_link;
> + /**
> + * @link_flags
> + *
> + * Provides detailed information about the link.
I think this calls for a bit more information than "detailed
information". What information do you want to store in this field ?
> + */
> + enum drm_link_flags link_flags;
> };
>
> #endif /* __DRM_TIMINGS_H__ */
> --
> 2.7.4
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH v2 4/9] drm/timings: Add link flags
2019-08-15 12:00 ` Laurent Pinchart
@ 2019-08-15 15:40 ` Fabrizio Castro
0 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 15:40 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, dri-devel, linux-kernel, Simon Horman,
Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc,
Kieran Bingham, Jacopo Mondi
Hi Laurent,
Thank you for the feedback!
I think we need to come a conclusion on here:
https://patchwork.kernel.org/patch/11095547/
before taking the comments on this patch any further.
Thanks,
Fab
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 15 August 2019 13:00
> Subject: Re: [PATCH v2 4/9] drm/timings: Add link flags
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Thu, Aug 15, 2019 at 12:04:28PM +0100, Fabrizio Castro wrote:
> > We need more information to describe dual-LVDS links, therefore
> > introduce link_flags.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * new patch
> >
> > include/drm/drm_timings.h | 26 ++++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/include/drm/drm_timings.h b/include/drm/drm_timings.h
> > index 4af8814..58fbf1b 100644
> > --- a/include/drm/drm_timings.h
> > +++ b/include/drm/drm_timings.h
> > @@ -1,4 +1,6 @@
> > /*
> > + * Copyright (C) 2019 Renesas Electronics Corporation
> > + *
> > * Permission to use, copy, modify, distribute, and sell this software and its
> > * documentation for any purpose is hereby granted without fee, provided that
> > * the above copyright notice appear in all copies and that both that copyright
> > @@ -21,6 +23,24 @@
> > #ifndef __DRM_TIMINGS_H__
> > #define __DRM_TIMINGS_H__
> >
> > +#include <linux/bits.h>
> > +
> > +/**
> > + * enum drm_link_flags - link_flags for &drm_timings
> > + *
> > + * This enum defines the details of the link.
> > + *
> > + * @DRM_LINK_DUAL_LVDS_ODD_EVEN: Dual-LVDS link, with odd pixels (1,3,5,
> > + * etc.) coming through the first port, and
> > + * even pixels (0,2,4,etc.) coming through
> > + * the second port. If not specified for a
> > + * dual-LVDS panel, it is assumed that even
> > + * pixels are coming through the first port
> > + */
> > +enum drm_link_flags {
>
> The text will be easier to read if you inline it here.
>
> /**
> * @DRM_LINK_DUAL_LVDS_ODD_EVEN: Dual-LVDS link, with odd pixels (1,3,5,
> * etc.) coming through the first port, and even pixels (0,2,4,etc.)
> ...
>
> > + DRM_LINK_DUAL_LVDS_ODD_EVEN = BIT(0),
>
> I would remove the dual_link field and add a DRM_LINK_DUAL_LVDS (or
> alternatively two flags, dual lvds odd-even and dual lvds even-odd).
>
> > +};
> > +
> > /**
> > * struct drm_timings - timing information
> > */
> > @@ -55,6 +75,12 @@ struct drm_timings {
> > * and odd-numbered pixels are received on separate links.
> > */
> > bool dual_link;
> > + /**
> > + * @link_flags
> > + *
> > + * Provides detailed information about the link.
>
> I think this calls for a bit more information than "detailed
> information". What information do you want to store in this field ?
>
> > + */
> > + enum drm_link_flags link_flags;
> > };
> >
> > #endif /* __DRM_TIMINGS_H__ */
> > --
> > 2.7.4
> >
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 5/9] drm/panel: Add timings field to drm_panel
2019-08-15 11:04 [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Fabrizio Castro
` (3 preceding siblings ...)
2019-08-15 11:04 ` [PATCH v2 4/9] drm/timings: Add link flags Fabrizio Castro
@ 2019-08-15 11:04 ` Fabrizio Castro
2019-08-15 12:03 ` Laurent Pinchart
2019-08-15 14:13 ` Sam Ravnborg
2019-08-15 11:04 ` [PATCH v2 6/9] drm: rcar-du: lvds: Fix companion's mode Fabrizio Castro
` (4 subsequent siblings)
9 siblings, 2 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 11:04 UTC (permalink / raw)
To: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard, Sean Paul,
Thierry Reding, David Airlie, Daniel Vetter
Cc: Fabrizio Castro, Sam Ravnborg, dri-devel, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc, Kieran Bingham, Jacopo Mondi
We need to know if the panel supports dual-link, similarly
to bridges, therefore add a reference to drm_timings in
drm_panel.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v1->v2:
* new patch
include/drm/drm_panel.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 8c738c0..cd6ff07 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -26,6 +26,7 @@
#include <linux/errno.h>
#include <linux/list.h>
+#include <drm/drm_timings.h>
struct device_node;
struct drm_connector;
@@ -81,6 +82,7 @@ struct drm_panel_funcs {
* struct drm_panel - DRM panel object
* @drm: DRM device owning the panel
* @connector: DRM connector that the panel is attached to
+ * @timings: timing information
* @dev: parent device of the panel
* @link: link from panel device (supplier) to DRM device (consumer)
* @funcs: operations that can be performed on the panel
@@ -89,6 +91,7 @@ struct drm_panel_funcs {
struct drm_panel {
struct drm_device *drm;
struct drm_connector *connector;
+ const struct drm_timings *timings;
struct device *dev;
const struct drm_panel_funcs *funcs;
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 5/9] drm/panel: Add timings field to drm_panel
2019-08-15 11:04 ` [PATCH v2 5/9] drm/panel: Add timings field to drm_panel Fabrizio Castro
@ 2019-08-15 12:03 ` Laurent Pinchart
2019-08-15 13:49 ` Fabrizio Castro
2019-08-15 14:13 ` Sam Ravnborg
1 sibling, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2019-08-15 12:03 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, Thierry Reding,
David Airlie, Daniel Vetter, Sam Ravnborg, dri-devel,
linux-kernel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-renesas-soc, Kieran Bingham, Jacopo Mondi
Hi Fabrizio,
Thank you for the patch.
On Thu, Aug 15, 2019 at 12:04:29PM +0100, Fabrizio Castro wrote:
> We need to know if the panel supports dual-link, similarly
> to bridges, therefore add a reference to drm_timings in
> drm_panel.
Panels may also need to report setup/hold time, so it's not about
dual-link only. I would make this explicit in the commit message.
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * new patch
>
> include/drm/drm_panel.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 8c738c0..cd6ff07 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -26,6 +26,7 @@
>
> #include <linux/errno.h>
> #include <linux/list.h>
> +#include <drm/drm_timings.h>
You can just add a forward-declaration of struct drm_timing.
>
> struct device_node;
> struct drm_connector;
> @@ -81,6 +82,7 @@ struct drm_panel_funcs {
> * struct drm_panel - DRM panel object
> * @drm: DRM device owning the panel
> * @connector: DRM connector that the panel is attached to
> + * @timings: timing information
> * @dev: parent device of the panel
> * @link: link from panel device (supplier) to DRM device (consumer)
> * @funcs: operations that can be performed on the panel
> @@ -89,6 +91,7 @@ struct drm_panel_funcs {
> struct drm_panel {
> struct drm_device *drm;
> struct drm_connector *connector;
> + const struct drm_timings *timings;
> struct device *dev;
>
> const struct drm_panel_funcs *funcs;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH v2 5/9] drm/panel: Add timings field to drm_panel
2019-08-15 12:03 ` Laurent Pinchart
@ 2019-08-15 13:49 ` Fabrizio Castro
0 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 13:49 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, Thierry Reding,
David Airlie, Daniel Vetter, Sam Ravnborg, dri-devel,
linux-kernel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-renesas-soc, 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: 15 August 2019 13:03
> Subject: Re: [PATCH v2 5/9] drm/panel: Add timings field to drm_panel
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Thu, Aug 15, 2019 at 12:04:29PM +0100, Fabrizio Castro wrote:
> > We need to know if the panel supports dual-link, similarly
> > to bridges, therefore add a reference to drm_timings in
> > drm_panel.
>
> Panels may also need to report setup/hold time, so it's not about
> dual-link only. I would make this explicit in the commit message.
Ok
>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * new patch
> >
> > include/drm/drm_panel.h | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index 8c738c0..cd6ff07 100644
> > --- a/include/drm/drm_panel.h
> > +++ b/include/drm/drm_panel.h
> > @@ -26,6 +26,7 @@
> >
> > #include <linux/errno.h>
> > #include <linux/list.h>
> > +#include <drm/drm_timings.h>
>
> You can just add a forward-declaration of struct drm_timing.
Ok
Thanks,
Fab
>
> >
> > struct device_node;
> > struct drm_connector;
> > @@ -81,6 +82,7 @@ struct drm_panel_funcs {
> > * struct drm_panel - DRM panel object
> > * @drm: DRM device owning the panel
> > * @connector: DRM connector that the panel is attached to
> > + * @timings: timing information
> > * @dev: parent device of the panel
> > * @link: link from panel device (supplier) to DRM device (consumer)
> > * @funcs: operations that can be performed on the panel
> > @@ -89,6 +91,7 @@ struct drm_panel_funcs {
> > struct drm_panel {
> > struct drm_device *drm;
> > struct drm_connector *connector;
> > + const struct drm_timings *timings;
> > struct device *dev;
> >
> > const struct drm_panel_funcs *funcs;
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 5/9] drm/panel: Add timings field to drm_panel
2019-08-15 11:04 ` [PATCH v2 5/9] drm/panel: Add timings field to drm_panel Fabrizio Castro
2019-08-15 12:03 ` Laurent Pinchart
@ 2019-08-15 14:13 ` Sam Ravnborg
2019-08-15 14:48 ` Fabrizio Castro
1 sibling, 1 reply; 40+ messages in thread
From: Sam Ravnborg @ 2019-08-15 14:13 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard, Sean Paul,
Thierry Reding, David Airlie, Daniel Vetter, dri-devel,
linux-kernel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-renesas-soc, Kieran Bingham, Jacopo Mondi
Hi Fabrizio
On Thu, Aug 15, 2019 at 12:04:29PM +0100, Fabrizio Castro wrote:
> We need to know if the panel supports dual-link, similarly
> to bridges, therefore add a reference to drm_timings in
> drm_panel.
Why do we need to know this?
Why is it needed in drm_panel and not in some driver specific struct?
I cannot see the full series, as I was copied only on some mails.
Awaiting dri-devel moderator before I can see the rest.
Sam
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * new patch
>
> include/drm/drm_panel.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 8c738c0..cd6ff07 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -26,6 +26,7 @@
>
> #include <linux/errno.h>
> #include <linux/list.h>
> +#include <drm/drm_timings.h>
>
> struct device_node;
> struct drm_connector;
> @@ -81,6 +82,7 @@ struct drm_panel_funcs {
> * struct drm_panel - DRM panel object
> * @drm: DRM device owning the panel
> * @connector: DRM connector that the panel is attached to
> + * @timings: timing information
> * @dev: parent device of the panel
> * @link: link from panel device (supplier) to DRM device (consumer)
> * @funcs: operations that can be performed on the panel
> @@ -89,6 +91,7 @@ struct drm_panel_funcs {
> struct drm_panel {
> struct drm_device *drm;
> struct drm_connector *connector;
> + const struct drm_timings *timings;
> struct device *dev;
>
> const struct drm_panel_funcs *funcs;
> --
> 2.7.4
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH v2 5/9] drm/panel: Add timings field to drm_panel
2019-08-15 14:13 ` Sam Ravnborg
@ 2019-08-15 14:48 ` Fabrizio Castro
0 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 14:48 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Laurent Pinchart, Maarten Lankhorst, Maxime Ripard, Sean Paul,
Thierry Reding, David Airlie, Daniel Vetter, dri-devel,
linux-kernel, Simon Horman, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-renesas-soc, Kieran Bingham, Jacopo Mondi
Hi Sam,
Thank you for your feedback!
> From: Sam Ravnborg <sam@ravnborg.org>
> Sent: 15 August 2019 15:14
> Subject: Re: [PATCH v2 5/9] drm/panel: Add timings field to drm_panel
>
> Hi Fabrizio
>
> On Thu, Aug 15, 2019 at 12:04:29PM +0100, Fabrizio Castro wrote:
> > We need to know if the panel supports dual-link, similarly
> > to bridges, therefore add a reference to drm_timings in
> > drm_panel.
>
> Why do we need to know this?
The encoders connected to a dual-LVDS panels may need to do special
arrangements for the dual-link setup, like initializing a companion
encoder, and working out which pixels (even or odd) to send to which
port (first LVDS input port or second LVDS input port). At least, this is
within the scope of the implementation of this series, which is currently
being discussed.
> Why is it needed in drm_panel and not in some driver specific struct?
The other fields are still applicable to panels, so I think it makes sense
for this to be included in struct drm_panels.
>
> I cannot see the full series, as I was copied only on some mails.
> Awaiting dri-devel moderator before I can see the rest.
I am sorry about this, some people don't want to be bothered by the whole
thing, I'll make sure I add you to the recipients list for the next iterations
of this series.
Thanks,
Fab
>
> Sam
>
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * new patch
> >
> > include/drm/drm_panel.h | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index 8c738c0..cd6ff07 100644
> > --- a/include/drm/drm_panel.h
> > +++ b/include/drm/drm_panel.h
> > @@ -26,6 +26,7 @@
> >
> > #include <linux/errno.h>
> > #include <linux/list.h>
> > +#include <drm/drm_timings.h>
> >
> > struct device_node;
> > struct drm_connector;
> > @@ -81,6 +82,7 @@ struct drm_panel_funcs {
> > * struct drm_panel - DRM panel object
> > * @drm: DRM device owning the panel
> > * @connector: DRM connector that the panel is attached to
> > + * @timings: timing information
> > * @dev: parent device of the panel
> > * @link: link from panel device (supplier) to DRM device (consumer)
> > * @funcs: operations that can be performed on the panel
> > @@ -89,6 +91,7 @@ struct drm_panel_funcs {
> > struct drm_panel {
> > struct drm_device *drm;
> > struct drm_connector *connector;
> > + const struct drm_timings *timings;
> > struct device *dev;
> >
> > const struct drm_panel_funcs *funcs;
> > --
> > 2.7.4
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 6/9] drm: rcar-du: lvds: Fix companion's mode
2019-08-15 11:04 [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Fabrizio Castro
` (4 preceding siblings ...)
2019-08-15 11:04 ` [PATCH v2 5/9] drm/panel: Add timings field to drm_panel Fabrizio Castro
@ 2019-08-15 11:04 ` Fabrizio Castro
2019-08-15 11:55 ` Laurent Pinchart
2019-08-15 11:04 ` [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support Fabrizio Castro
` (3 subsequent siblings)
9 siblings, 1 reply; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 11:04 UTC (permalink / raw)
To: Laurent Pinchart, David Airlie, Daniel Vetter
Cc: Fabrizio Castro, Kieran Bingham, dri-devel, linux-renesas-soc,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
Jacopo Mondi
The companion encoder needs to be told to use the same
mode as the primary encoder.
Fixes: e9e8798ab7b8 ("drm: rcar-du: lvds: Add support for dual-link mode")
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v1->v2:
* reworked according to Laurent's feedback
drivers/gpu/drm/rcar-du/rcar_lvds.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3fe0b86..41d28f4 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -603,6 +603,11 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge,
lvds->display_mode = *adjusted_mode;
rcar_lvds_get_lvds_mode(lvds);
+ if (lvds->companion) {
+ struct rcar_lvds *companion_lvds = bridge_to_rcar_lvds(
+ lvds->companion);
+ companion_lvds->mode = lvds->mode;
+ }
}
static int rcar_lvds_attach(struct drm_bridge *bridge)
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 6/9] drm: rcar-du: lvds: Fix companion's mode
2019-08-15 11:04 ` [PATCH v2 6/9] drm: rcar-du: lvds: Fix companion's mode Fabrizio Castro
@ 2019-08-15 11:55 ` Laurent Pinchart
2019-08-15 13:42 ` Fabrizio Castro
0 siblings, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2019-08-15 11:55 UTC (permalink / raw)
To: Fabrizio Castro
Cc: David Airlie, Daniel Vetter, Kieran Bingham, dri-devel,
linux-renesas-soc, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, Jacopo Mondi
Hi Fabrizio,
Thank you for the patch.
On Thu, Aug 15, 2019 at 12:04:30PM +0100, Fabrizio Castro wrote:
> The companion encoder needs to be told to use the same
> mode as the primary encoder.
>
> Fixes: e9e8798ab7b8 ("drm: rcar-du: lvds: Add support for dual-link mode")
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * reworked according to Laurent's feedback
>
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 3fe0b86..41d28f4 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -603,6 +603,11 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge,
> lvds->display_mode = *adjusted_mode;
>
> rcar_lvds_get_lvds_mode(lvds);
> + if (lvds->companion) {
> + struct rcar_lvds *companion_lvds = bridge_to_rcar_lvds(
> + lvds->companion);
> + companion_lvds->mode = lvds->mode;
How about calling rcar_lvds_mode_set() on the companion instead ?
> + }
> }
>
> static int rcar_lvds_attach(struct drm_bridge *bridge)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH v2 6/9] drm: rcar-du: lvds: Fix companion's mode
2019-08-15 11:55 ` Laurent Pinchart
@ 2019-08-15 13:42 ` Fabrizio Castro
0 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 13:42 UTC (permalink / raw)
To: Laurent Pinchart
Cc: David Airlie, Daniel Vetter, Kieran Bingham, dri-devel,
linux-renesas-soc, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, 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: 15 August 2019 12:55
> Subject: Re: [PATCH v2 6/9] drm: rcar-du: lvds: Fix companion's mode
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Thu, Aug 15, 2019 at 12:04:30PM +0100, Fabrizio Castro wrote:
> > The companion encoder needs to be told to use the same
> > mode as the primary encoder.
> >
> > Fixes: e9e8798ab7b8 ("drm: rcar-du: lvds: Add support for dual-link mode")
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * reworked according to Laurent's feedback
> >
> > drivers/gpu/drm/rcar-du/rcar_lvds.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 3fe0b86..41d28f4 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -603,6 +603,11 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge,
> > lvds->display_mode = *adjusted_mode;
> >
> > rcar_lvds_get_lvds_mode(lvds);
> > + if (lvds->companion) {
> > + struct rcar_lvds *companion_lvds = bridge_to_rcar_lvds(
> > + lvds->companion);
> > + companion_lvds->mode = lvds->mode;
>
> How about calling rcar_lvds_mode_set() on the companion instead ?
Can do, will send a new version.
Cheers,
Fab
>
> > + }
> > }
> >
> > static int rcar_lvds_attach(struct drm_bridge *bridge)
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support
2019-08-15 11:04 [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Fabrizio Castro
` (5 preceding siblings ...)
2019-08-15 11:04 ` [PATCH v2 6/9] drm: rcar-du: lvds: Fix companion's mode Fabrizio Castro
@ 2019-08-15 11:04 ` Fabrizio Castro
2019-08-15 13:08 ` Laurent Pinchart
2019-08-15 11:04 ` [PATCH v2 8/9] drm/panel: lvds: Add support for the IDK-2121WR Fabrizio Castro
` (2 subsequent siblings)
9 siblings, 1 reply; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 11:04 UTC (permalink / raw)
To: Laurent Pinchart, David Airlie, Daniel Vetter
Cc: Fabrizio Castro, Kieran Bingham, dri-devel, linux-renesas-soc,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
Jacopo Mondi
This patch adds support for dual-LVDS panels.
It's very important that we coordinate the efforts of both the
primary encoder and the companion encoder to get the right
picture on the panel, therefore this patch adds some code
to work out if even and odd pixels need swapping.
When the encoders are connected to a LVDS panel, the assumption
is that by default the panel expects even pixels (0, 2, 4, etc.)
on the first input port, and odd pixels (1, 3, 5, etc.) on the
seconds port. When DRM_LINK_DUAL_LVDS_ODD_EVEN is found among the
link flags, the display expects odd pixels on the first input
port, and even pixels on the second port. As a result, the way
the encoders are connected to the panel may trigger pixel (data)
swapping.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v1->v2:
* new patch, resulting from Laurent's feedback
drivers/gpu/drm/rcar-du/rcar_lvds.c | 121 ++++++++++++++++++++++++------------
1 file changed, 81 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 41d28f4..5c24884 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -22,6 +22,8 @@
#include <drm/drm_bridge.h>
#include <drm/drm_panel.h>
#include <drm/drm_probe_helper.h>
+#include <drm/drm_timings.h>
+#include <drm/drm_of.h>
#include "rcar_lvds.h"
#include "rcar_lvds_regs.h"
@@ -69,6 +71,7 @@ struct rcar_lvds {
struct drm_bridge *companion;
bool dual_link;
+ bool stripe_swap_data;
};
#define bridge_to_rcar_lvds(b) \
@@ -439,12 +442,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
- /*
- * Configure vertical stripe based on the mode of operation of
- * the connected device.
- */
- rcar_lvds_write(lvds, LVDSTRIPE,
- lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
+ u32 lvdstripe = 0;
+
+ if (lvds->dual_link)
+ /*
+ * Configure vertical stripe based on the mode of
+ * operation of the connected device.
+ *
+ * ST_SWAP from LVD1STRIPE is reserved, do not set
+ * in the companion LVDS
+ */
+ lvdstripe = LVDSTRIPE_ST_ON |
+ (lvds->companion && lvds->stripe_swap_data ?
+ LVDSTRIPE_ST_SWAP : 0);
+ rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
}
/*
@@ -706,13 +717,31 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
return ret;
}
+static int rcar_lvds_get_remote_port_reg(struct device_node *np)
+{
+ struct device_node *endpoint_node, *remote_endpoint;
+ struct of_endpoint endpoint;
+
+ endpoint_node = of_graph_get_endpoint_by_regs(np, 1, 0);
+ if (!endpoint_node)
+ return -ENODEV;
+ remote_endpoint = of_graph_get_remote_endpoint(endpoint_node);
+ if (!remote_endpoint) {
+ of_node_put(endpoint_node);
+ return -ENODEV;
+ }
+ of_graph_parse_endpoint(remote_endpoint, &endpoint);
+ of_node_put(endpoint_node);
+ of_node_put(remote_endpoint);
+
+ return endpoint.port;
+}
+
static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
{
struct device_node *local_output = NULL;
- struct device_node *remote_input = NULL;
struct device_node *remote = NULL;
- struct device_node *node;
- bool is_bridge = false;
+ const struct drm_timings *timings = NULL;
int ret = 0;
local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
@@ -740,45 +769,57 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
goto done;
}
- remote_input = of_graph_get_remote_endpoint(local_output);
+ ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
+ &lvds->panel, &lvds->next_bridge);
+ if (ret) {
+ ret = -EPROBE_DEFER;
+ goto done;
+ }
+ if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
+ if (lvds->next_bridge)
+ timings = lvds->next_bridge->timings;
+ else
+ timings = lvds->panel->timings;
+ if (timings)
+ lvds->dual_link = timings->dual_link;
+ }
- for_each_endpoint_of_node(remote, node) {
- if (node != remote_input) {
+ if (lvds->dual_link) {
+ ret = rcar_lvds_parse_dt_companion(lvds);
+ if (lvds->companion && timings) {
+ int our_port, comp_port;
+ bool odd_even_flag = timings->link_flags &
+ DRM_LINK_DUAL_LVDS_ODD_EVEN;
+ our_port = rcar_lvds_get_remote_port_reg(
+ lvds->dev->of_node);
+ if (our_port < 0) {
+ ret = our_port;
+ goto done;
+ }
+ comp_port = rcar_lvds_get_remote_port_reg(
+ lvds->companion->of_node);
+ if (comp_port < 0) {
+ ret = comp_port;
+ goto done;
+ }
/*
- * We've found one endpoint other than the input, this
- * must be a bridge.
+ * We need to match the port where we generate even
+ * pixels (0, 2, 4, etc.) to the port where the sink
+ * expects to receive even pixels, same thing for the
+ * odd pixels. Swap the generation of even and odd
+ * pixels if the connection requires it.
+ * By default (when DRM_LINK_DUAL_LVDS_ODD_EVEN is not
+ * specified) the sink expects even pixels on the
+ * first input port, and odd pixels on the second port.
*/
- is_bridge = true;
- of_node_put(node);
- break;
+ if (((comp_port - our_port > 0) && odd_even_flag) ||
+ ((comp_port - our_port < 0) && !odd_even_flag))
+ lvds->stripe_swap_data = true;
}
}
- if (is_bridge) {
- lvds->next_bridge = of_drm_find_bridge(remote);
- if (!lvds->next_bridge) {
- ret = -EPROBE_DEFER;
- goto done;
- }
-
- if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
- lvds->dual_link = lvds->next_bridge->timings
- ? lvds->next_bridge->timings->dual_link
- : false;
- } else {
- lvds->panel = of_drm_find_panel(remote);
- if (IS_ERR(lvds->panel)) {
- ret = PTR_ERR(lvds->panel);
- goto done;
- }
- }
-
- if (lvds->dual_link)
- ret = rcar_lvds_parse_dt_companion(lvds);
-
done:
of_node_put(local_output);
- of_node_put(remote_input);
of_node_put(remote);
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support
2019-08-15 11:04 ` [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support Fabrizio Castro
@ 2019-08-15 13:08 ` Laurent Pinchart
2019-08-15 15:36 ` Fabrizio Castro
0 siblings, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2019-08-15 13:08 UTC (permalink / raw)
To: Fabrizio Castro
Cc: David Airlie, Daniel Vetter, Kieran Bingham, dri-devel,
linux-renesas-soc, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, Jacopo Mondi
Hi Fabrizio,
Thank you for the patch.
On Thu, Aug 15, 2019 at 12:04:31PM +0100, Fabrizio Castro wrote:
> This patch adds support for dual-LVDS panels.
>
> It's very important that we coordinate the efforts of both the
> primary encoder and the companion encoder to get the right
> picture on the panel, therefore this patch adds some code
> to work out if even and odd pixels need swapping.
> When the encoders are connected to a LVDS panel, the assumption
> is that by default the panel expects even pixels (0, 2, 4, etc.)
> on the first input port, and odd pixels (1, 3, 5, etc.) on the
> seconds port. When DRM_LINK_DUAL_LVDS_ODD_EVEN is found among the
> link flags, the display expects odd pixels on the first input
> port, and even pixels on the second port. As a result, the way
> the encoders are connected to the panel may trigger pixel (data)
> swapping.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> ---
> v1->v2:
> * new patch, resulting from Laurent's feedback
>
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 121 ++++++++++++++++++++++++------------
> 1 file changed, 81 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 41d28f4..5c24884 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -22,6 +22,8 @@
> #include <drm/drm_bridge.h>
> #include <drm/drm_panel.h>
> #include <drm/drm_probe_helper.h>
> +#include <drm/drm_timings.h>
> +#include <drm/drm_of.h>
Please keep the headers alphabetically sorted.
>
> #include "rcar_lvds.h"
> #include "rcar_lvds_regs.h"
> @@ -69,6 +71,7 @@ struct rcar_lvds {
>
> struct drm_bridge *companion;
> bool dual_link;
> + bool stripe_swap_data;
> };
>
> #define bridge_to_rcar_lvds(b) \
> @@ -439,12 +442,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
>
> if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> - /*
> - * Configure vertical stripe based on the mode of operation of
> - * the connected device.
> - */
> - rcar_lvds_write(lvds, LVDSTRIPE,
> - lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
> + u32 lvdstripe = 0;
> +
> + if (lvds->dual_link)
> + /*
> + * Configure vertical stripe based on the mode of
> + * operation of the connected device.
> + *
> + * ST_SWAP from LVD1STRIPE is reserved, do not set
> + * in the companion LVDS
> + */
> + lvdstripe = LVDSTRIPE_ST_ON |
> + (lvds->companion && lvds->stripe_swap_data ?
> + LVDSTRIPE_ST_SWAP : 0);
Let's sort out the alignment.
lvdstripe = LVDSTRIPE_ST_ON
| (lvds->companion && lvds->stripe_swap_data ?
LVDSTRIPE_ST_SWAP : 0);
> + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
> }
>
> /*
> @@ -706,13 +717,31 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> return ret;
> }
>
> +static int rcar_lvds_get_remote_port_reg(struct device_node *np)
> +{
> + struct device_node *endpoint_node, *remote_endpoint;
> + struct of_endpoint endpoint;
> +
> + endpoint_node = of_graph_get_endpoint_by_regs(np, 1, 0);
> + if (!endpoint_node)
> + return -ENODEV;
> + remote_endpoint = of_graph_get_remote_endpoint(endpoint_node);
> + if (!remote_endpoint) {
> + of_node_put(endpoint_node);
> + return -ENODEV;
> + }
> + of_graph_parse_endpoint(remote_endpoint, &endpoint);
> + of_node_put(endpoint_node);
> + of_node_put(remote_endpoint);
> +
> + return endpoint.port;
> +}
> +
> static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> {
> struct device_node *local_output = NULL;
> - struct device_node *remote_input = NULL;
> struct device_node *remote = NULL;
> - struct device_node *node;
> - bool is_bridge = false;
> + const struct drm_timings *timings = NULL;
> int ret = 0;
>
> local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> @@ -740,45 +769,57 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> goto done;
> }
>
> - remote_input = of_graph_get_remote_endpoint(local_output);
> + ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
> + &lvds->panel, &lvds->next_bridge);
> + if (ret) {
> + ret = -EPROBE_DEFER;
> + goto done;
> + }
> + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> + if (lvds->next_bridge)
> + timings = lvds->next_bridge->timings;
> + else
> + timings = lvds->panel->timings;
I wonder if we should use devm_drm_panel_bridge_add() (or
drm_panel_bridge_add()) and use the bridge API only. It would require a
small change in the drm_panel_bridge to copy the timings pointer, but
apart from that I think it should be fine. If it creates too much churn
due to connector handling then we can skip it for now and I'll handle it
later (but I'd appreciate if you could copy the timings pointer in
drm_panel_bridge already).
> + if (timings)
> + lvds->dual_link = timings->dual_link;
> + }
>
> - for_each_endpoint_of_node(remote, node) {
> - if (node != remote_input) {
> + if (lvds->dual_link) {
> + ret = rcar_lvds_parse_dt_companion(lvds);
> + if (lvds->companion && timings) {
> + int our_port, comp_port;
> + bool odd_even_flag = timings->link_flags &
> + DRM_LINK_DUAL_LVDS_ODD_EVEN;
> + our_port = rcar_lvds_get_remote_port_reg(
> + lvds->dev->of_node);
> + if (our_port < 0) {
> + ret = our_port;
> + goto done;
> + }
> + comp_port = rcar_lvds_get_remote_port_reg(
> + lvds->companion->of_node);
> + if (comp_port < 0) {
> + ret = comp_port;
> + goto done;
> + }
> /*
> - * We've found one endpoint other than the input, this
> - * must be a bridge.
> + * We need to match the port where we generate even
> + * pixels (0, 2, 4, etc.) to the port where the sink
> + * expects to receive even pixels, same thing for the
> + * odd pixels. Swap the generation of even and odd
> + * pixels if the connection requires it.
> + * By default (when DRM_LINK_DUAL_LVDS_ODD_EVEN is not
> + * specified) the sink expects even pixels on the
> + * first input port, and odd pixels on the second port.
I see what you're trying to do, but I'm not sure I like it much :-S
Peeking into the remove DT node like that creates a dependency between
this driver and the DT bindings of all possible remote nodes. For this
to work, you would need to ensure that the odd/even mapping to ports is
common to all dual-link devices, and thus document that globally in the
DT bindings. I'm not sure if there's a way around it as hardware
connections could indeed switch the two lanes, so we need to model that
somehow. It could be modelled with a swap property in DT, but that would
still require a standard mapping of odd-even pixels to ports, so maybe
the easiest option is to document globally that port 0 on the sink is
for even pixels, and port 1 for odd pixels, and remove the
DRM_LINK_DUAL_LVDS_ODD_EVEN flag completely. But what will then happen
if you panel has more than two ports (for audio for instance, or for
other types of video links) ? It may not be possible to always use port
0 and 1 for the LVDS even and odd pixels in DT bindings of a particular
panel or bridge.
A creative solution is needed here.
> */
> - is_bridge = true;
> - of_node_put(node);
> - break;
> + if (((comp_port - our_port > 0) && odd_even_flag) ||
> + ((comp_port - our_port < 0) && !odd_even_flag))
> + lvds->stripe_swap_data = true;
> }
> }
>
> - if (is_bridge) {
> - lvds->next_bridge = of_drm_find_bridge(remote);
> - if (!lvds->next_bridge) {
> - ret = -EPROBE_DEFER;
> - goto done;
> - }
> -
> - if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> - lvds->dual_link = lvds->next_bridge->timings
> - ? lvds->next_bridge->timings->dual_link
> - : false;
> - } else {
> - lvds->panel = of_drm_find_panel(remote);
> - if (IS_ERR(lvds->panel)) {
> - ret = PTR_ERR(lvds->panel);
> - goto done;
> - }
> - }
> -
> - if (lvds->dual_link)
> - ret = rcar_lvds_parse_dt_companion(lvds);
> -
> done:
> of_node_put(local_output);
> - of_node_put(remote_input);
> of_node_put(remote);
>
> /*
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support
2019-08-15 13:08 ` Laurent Pinchart
@ 2019-08-15 15:36 ` Fabrizio Castro
2019-08-20 16:04 ` Laurent Pinchart
0 siblings, 1 reply; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 15:36 UTC (permalink / raw)
To: Laurent Pinchart
Cc: David Airlie, Daniel Vetter, Kieran Bingham, dri-devel,
linux-renesas-soc, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, 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: 15 August 2019 14:09
> Subject: Re: [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Thu, Aug 15, 2019 at 12:04:31PM +0100, Fabrizio Castro wrote:
> > This patch adds support for dual-LVDS panels.
> >
> > It's very important that we coordinate the efforts of both the
> > primary encoder and the companion encoder to get the right
> > picture on the panel, therefore this patch adds some code
> > to work out if even and odd pixels need swapping.
> > When the encoders are connected to a LVDS panel, the assumption
> > is that by default the panel expects even pixels (0, 2, 4, etc.)
> > on the first input port, and odd pixels (1, 3, 5, etc.) on the
> > seconds port. When DRM_LINK_DUAL_LVDS_ODD_EVEN is found among the
> > link flags, the display expects odd pixels on the first input
> > port, and even pixels on the second port. As a result, the way
> > the encoders are connected to the panel may trigger pixel (data)
> > swapping.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * new patch, resulting from Laurent's feedback
> >
> > drivers/gpu/drm/rcar-du/rcar_lvds.c | 121 ++++++++++++++++++++++++------------
> > 1 file changed, 81 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 41d28f4..5c24884 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -22,6 +22,8 @@
> > #include <drm/drm_bridge.h>
> > #include <drm/drm_panel.h>
> > #include <drm/drm_probe_helper.h>
> > +#include <drm/drm_timings.h>
> > +#include <drm/drm_of.h>
>
> Please keep the headers alphabetically sorted.
Ok
>
> >
> > #include "rcar_lvds.h"
> > #include "rcar_lvds_regs.h"
> > @@ -69,6 +71,7 @@ struct rcar_lvds {
> >
> > struct drm_bridge *companion;
> > bool dual_link;
> > + bool stripe_swap_data;
> > };
> >
> > #define bridge_to_rcar_lvds(b) \
> > @@ -439,12 +442,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> > rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
> >
> > if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > - /*
> > - * Configure vertical stripe based on the mode of operation of
> > - * the connected device.
> > - */
> > - rcar_lvds_write(lvds, LVDSTRIPE,
> > - lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
> > + u32 lvdstripe = 0;
> > +
> > + if (lvds->dual_link)
> > + /*
> > + * Configure vertical stripe based on the mode of
> > + * operation of the connected device.
> > + *
> > + * ST_SWAP from LVD1STRIPE is reserved, do not set
> > + * in the companion LVDS
> > + */
> > + lvdstripe = LVDSTRIPE_ST_ON |
> > + (lvds->companion && lvds->stripe_swap_data ?
> > + LVDSTRIPE_ST_SWAP : 0);
>
> Let's sort out the alignment.
>
> lvdstripe = LVDSTRIPE_ST_ON
> | (lvds->companion && lvds->stripe_swap_data ?
> LVDSTRIPE_ST_SWAP : 0);
Ok
>
> > + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
> > }
> >
> > /*
> > @@ -706,13 +717,31 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> > return ret;
> > }
> >
> > +static int rcar_lvds_get_remote_port_reg(struct device_node *np)
> > +{
> > + struct device_node *endpoint_node, *remote_endpoint;
> > + struct of_endpoint endpoint;
> > +
> > + endpoint_node = of_graph_get_endpoint_by_regs(np, 1, 0);
> > + if (!endpoint_node)
> > + return -ENODEV;
> > + remote_endpoint = of_graph_get_remote_endpoint(endpoint_node);
> > + if (!remote_endpoint) {
> > + of_node_put(endpoint_node);
> > + return -ENODEV;
> > + }
> > + of_graph_parse_endpoint(remote_endpoint, &endpoint);
> > + of_node_put(endpoint_node);
> > + of_node_put(remote_endpoint);
> > +
> > + return endpoint.port;
> > +}
> > +
> > static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > {
> > struct device_node *local_output = NULL;
> > - struct device_node *remote_input = NULL;
> > struct device_node *remote = NULL;
> > - struct device_node *node;
> > - bool is_bridge = false;
> > + const struct drm_timings *timings = NULL;
> > int ret = 0;
> >
> > local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> > @@ -740,45 +769,57 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > goto done;
> > }
> >
> > - remote_input = of_graph_get_remote_endpoint(local_output);
> > + ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
> > + &lvds->panel, &lvds->next_bridge);
> > + if (ret) {
> > + ret = -EPROBE_DEFER;
> > + goto done;
> > + }
> > + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > + if (lvds->next_bridge)
> > + timings = lvds->next_bridge->timings;
> > + else
> > + timings = lvds->panel->timings;
>
> I wonder if we should use devm_drm_panel_bridge_add() (or
> drm_panel_bridge_add()) and use the bridge API only. It would require a
> small change in the drm_panel_bridge to copy the timings pointer, but
> apart from that I think it should be fine. If it creates too much churn
> due to connector handling then we can skip it for now and I'll handle it
> later (but I'd appreciate if you could copy the timings pointer in
> drm_panel_bridge already).
Will look into this.
>
> > + if (timings)
> > + lvds->dual_link = timings->dual_link;
> > + }
> >
> > - for_each_endpoint_of_node(remote, node) {
> > - if (node != remote_input) {
> > + if (lvds->dual_link) {
> > + ret = rcar_lvds_parse_dt_companion(lvds);
> > + if (lvds->companion && timings) {
> > + int our_port, comp_port;
> > + bool odd_even_flag = timings->link_flags &
> > + DRM_LINK_DUAL_LVDS_ODD_EVEN;
> > + our_port = rcar_lvds_get_remote_port_reg(
> > + lvds->dev->of_node);
> > + if (our_port < 0) {
> > + ret = our_port;
> > + goto done;
> > + }
> > + comp_port = rcar_lvds_get_remote_port_reg(
> > + lvds->companion->of_node);
> > + if (comp_port < 0) {
> > + ret = comp_port;
> > + goto done;
> > + }
> > /*
> > - * We've found one endpoint other than the input, this
> > - * must be a bridge.
> > + * We need to match the port where we generate even
> > + * pixels (0, 2, 4, etc.) to the port where the sink
> > + * expects to receive even pixels, same thing for the
> > + * odd pixels. Swap the generation of even and odd
> > + * pixels if the connection requires it.
> > + * By default (when DRM_LINK_DUAL_LVDS_ODD_EVEN is not
> > + * specified) the sink expects even pixels on the
> > + * first input port, and odd pixels on the second port.
>
> I see what you're trying to do, but I'm not sure I like it much :-S
>
> Peeking into the remove DT node like that creates a dependency between
> this driver and the DT bindings of all possible remote nodes. For this
> to work, you would need to ensure that the odd/even mapping to ports is
> common to all dual-link devices, and thus document that globally in the
> DT bindings. I'm not sure if there's a way around it as hardware
> connections could indeed switch the two lanes, so we need to model that
> somehow. It could be modelled with a swap property in DT, but that would
> still require a standard mapping of odd-even pixels to ports, so maybe
> the easiest option is to document globally that port 0 on the sink is
> for even pixels, and port 1 for odd pixels, and remove the
> DRM_LINK_DUAL_LVDS_ODD_EVEN flag completely. But what will then happen
> if you panel has more than two ports (for audio for instance, or for
> other types of video links) ? It may not be possible to always use port
> 0 and 1 for the LVDS even and odd pixels in DT bindings of a particular
> panel or bridge.
This is the stickiest point of the whole series. The implementation within this
series allows for any number of ports on the sink, the LVDS ports don't have
to be port 0 and port 1, it's enough that the port for the even pixels comes
before the port of the odd pixels (but the logic can be inverted by means of
DRM_LINK_DUAL_LVDS_ODD_EVEN), and if you swap the lvds0 and lvds1
OF graph connections around, the pixels will be swapped automatically.
But of course, there is the dependency between the driver and dt-bindings
you were mentioning, and of top of that every driver would need to work
things out independently at this point in time.
>
> A creative solution is needed here.
I may have an idea. What if we marked both ends of each OF graph link
with either "even-pixels;" or "odd-pixels;", and exported a function that
given the of_node of two endpoints returned if the link requires swapping?
There'd be no need for the flag at that point, the numbering of the ports
would not matter, and the DT would be comprehensive and very easy to read.
Let me please know your thoughts.
Thanks you for the patience
Fab
>
> > */
> > - is_bridge = true;
> > - of_node_put(node);
> > - break;
> > + if (((comp_port - our_port > 0) && odd_even_flag) ||
> > + ((comp_port - our_port < 0) && !odd_even_flag))
> > + lvds->stripe_swap_data = true;
> > }
> > }
> >
> > - if (is_bridge) {
> > - lvds->next_bridge = of_drm_find_bridge(remote);
> > - if (!lvds->next_bridge) {
> > - ret = -EPROBE_DEFER;
> > - goto done;
> > - }
> > -
> > - if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > - lvds->dual_link = lvds->next_bridge->timings
> > - ? lvds->next_bridge->timings->dual_link
> > - : false;
> > - } else {
> > - lvds->panel = of_drm_find_panel(remote);
> > - if (IS_ERR(lvds->panel)) {
> > - ret = PTR_ERR(lvds->panel);
> > - goto done;
> > - }
> > - }
> > -
> > - if (lvds->dual_link)
> > - ret = rcar_lvds_parse_dt_companion(lvds);
> > -
> > done:
> > of_node_put(local_output);
> > - of_node_put(remote_input);
> > of_node_put(remote);
> >
> > /*
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support
2019-08-15 15:36 ` Fabrizio Castro
@ 2019-08-20 16:04 ` Laurent Pinchart
2019-08-21 17:00 ` Fabrizio Castro
0 siblings, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2019-08-20 16:04 UTC (permalink / raw)
To: Fabrizio Castro
Cc: David Airlie, Daniel Vetter, Kieran Bingham, dri-devel,
linux-renesas-soc, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, Jacopo Mondi
Hi Fabrizio,
On Thu, Aug 15, 2019 at 03:36:53PM +0000, Fabrizio Castro wrote:
> On 15 August 2019 14:09 Laurent Pinchart wrote:
> > On Thu, Aug 15, 2019 at 12:04:31PM +0100, Fabrizio Castro wrote:
> > > This patch adds support for dual-LVDS panels.
> > >
> > > It's very important that we coordinate the efforts of both the
> > > primary encoder and the companion encoder to get the right
> > > picture on the panel, therefore this patch adds some code
> > > to work out if even and odd pixels need swapping.
> > > When the encoders are connected to a LVDS panel, the assumption
> > > is that by default the panel expects even pixels (0, 2, 4, etc.)
> > > on the first input port, and odd pixels (1, 3, 5, etc.) on the
> > > seconds port. When DRM_LINK_DUAL_LVDS_ODD_EVEN is found among the
> > > link flags, the display expects odd pixels on the first input
> > > port, and even pixels on the second port. As a result, the way
> > > the encoders are connected to the panel may trigger pixel (data)
> > > swapping.
> > >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > ---
> > > v1->v2:
> > > * new patch, resulting from Laurent's feedback
> > >
> > > drivers/gpu/drm/rcar-du/rcar_lvds.c | 121 ++++++++++++++++++++++++------------
> > > 1 file changed, 81 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > index 41d28f4..5c24884 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > @@ -22,6 +22,8 @@
> > > #include <drm/drm_bridge.h>
> > > #include <drm/drm_panel.h>
> > > #include <drm/drm_probe_helper.h>
> > > +#include <drm/drm_timings.h>
> > > +#include <drm/drm_of.h>
> >
> > Please keep the headers alphabetically sorted.
>
> Ok
>
> > >
> > > #include "rcar_lvds.h"
> > > #include "rcar_lvds_regs.h"
> > > @@ -69,6 +71,7 @@ struct rcar_lvds {
> > >
> > > struct drm_bridge *companion;
> > > bool dual_link;
> > > + bool stripe_swap_data;
> > > };
> > >
> > > #define bridge_to_rcar_lvds(b) \
> > > @@ -439,12 +442,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> > > rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
> > >
> > > if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > > - /*
> > > - * Configure vertical stripe based on the mode of operation of
> > > - * the connected device.
> > > - */
> > > - rcar_lvds_write(lvds, LVDSTRIPE,
> > > - lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
> > > + u32 lvdstripe = 0;
> > > +
> > > + if (lvds->dual_link)
> > > + /*
> > > + * Configure vertical stripe based on the mode of
> > > + * operation of the connected device.
> > > + *
> > > + * ST_SWAP from LVD1STRIPE is reserved, do not set
> > > + * in the companion LVDS
> > > + */
> > > + lvdstripe = LVDSTRIPE_ST_ON |
> > > + (lvds->companion && lvds->stripe_swap_data ?
> > > + LVDSTRIPE_ST_SWAP : 0);
> >
> > Let's sort out the alignment.
> >
> > lvdstripe = LVDSTRIPE_ST_ON
> > | (lvds->companion && lvds->stripe_swap_data ?
> > LVDSTRIPE_ST_SWAP : 0);
>
> Ok
>
> > > + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
> > > }
> > >
> > > /*
> > > @@ -706,13 +717,31 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> > > return ret;
> > > }
> > >
> > > +static int rcar_lvds_get_remote_port_reg(struct device_node *np)
> > > +{
> > > + struct device_node *endpoint_node, *remote_endpoint;
> > > + struct of_endpoint endpoint;
> > > +
> > > + endpoint_node = of_graph_get_endpoint_by_regs(np, 1, 0);
> > > + if (!endpoint_node)
> > > + return -ENODEV;
> > > + remote_endpoint = of_graph_get_remote_endpoint(endpoint_node);
> > > + if (!remote_endpoint) {
> > > + of_node_put(endpoint_node);
> > > + return -ENODEV;
> > > + }
> > > + of_graph_parse_endpoint(remote_endpoint, &endpoint);
> > > + of_node_put(endpoint_node);
> > > + of_node_put(remote_endpoint);
> > > +
> > > + return endpoint.port;
> > > +}
> > > +
> > > static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > > {
> > > struct device_node *local_output = NULL;
> > > - struct device_node *remote_input = NULL;
> > > struct device_node *remote = NULL;
> > > - struct device_node *node;
> > > - bool is_bridge = false;
> > > + const struct drm_timings *timings = NULL;
> > > int ret = 0;
> > >
> > > local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> > > @@ -740,45 +769,57 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > > goto done;
> > > }
> > >
> > > - remote_input = of_graph_get_remote_endpoint(local_output);
> > > + ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
> > > + &lvds->panel, &lvds->next_bridge);
> > > + if (ret) {
> > > + ret = -EPROBE_DEFER;
> > > + goto done;
> > > + }
> > > + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > > + if (lvds->next_bridge)
> > > + timings = lvds->next_bridge->timings;
> > > + else
> > > + timings = lvds->panel->timings;
> >
> > I wonder if we should use devm_drm_panel_bridge_add() (or
> > drm_panel_bridge_add()) and use the bridge API only. It would require a
> > small change in the drm_panel_bridge to copy the timings pointer, but
> > apart from that I think it should be fine. If it creates too much churn
> > due to connector handling then we can skip it for now and I'll handle it
> > later (but I'd appreciate if you could copy the timings pointer in
> > drm_panel_bridge already).
>
> Will look into this.
>
> > > + if (timings)
> > > + lvds->dual_link = timings->dual_link;
> > > + }
> > >
> > > - for_each_endpoint_of_node(remote, node) {
> > > - if (node != remote_input) {
> > > + if (lvds->dual_link) {
> > > + ret = rcar_lvds_parse_dt_companion(lvds);
> > > + if (lvds->companion && timings) {
> > > + int our_port, comp_port;
> > > + bool odd_even_flag = timings->link_flags &
> > > + DRM_LINK_DUAL_LVDS_ODD_EVEN;
> > > + our_port = rcar_lvds_get_remote_port_reg(
> > > + lvds->dev->of_node);
> > > + if (our_port < 0) {
> > > + ret = our_port;
> > > + goto done;
> > > + }
> > > + comp_port = rcar_lvds_get_remote_port_reg(
> > > + lvds->companion->of_node);
> > > + if (comp_port < 0) {
> > > + ret = comp_port;
> > > + goto done;
> > > + }
> > > /*
> > > - * We've found one endpoint other than the input, this
> > > - * must be a bridge.
> > > + * We need to match the port where we generate even
> > > + * pixels (0, 2, 4, etc.) to the port where the sink
> > > + * expects to receive even pixels, same thing for the
> > > + * odd pixels. Swap the generation of even and odd
> > > + * pixels if the connection requires it.
> > > + * By default (when DRM_LINK_DUAL_LVDS_ODD_EVEN is not
> > > + * specified) the sink expects even pixels on the
> > > + * first input port, and odd pixels on the second port.
> >
> > I see what you're trying to do, but I'm not sure I like it much :-S
> >
> > Peeking into the remove DT node like that creates a dependency between
> > this driver and the DT bindings of all possible remote nodes. For this
> > to work, you would need to ensure that the odd/even mapping to ports is
> > common to all dual-link devices, and thus document that globally in the
> > DT bindings. I'm not sure if there's a way around it as hardware
> > connections could indeed switch the two lanes, so we need to model that
> > somehow. It could be modelled with a swap property in DT, but that would
> > still require a standard mapping of odd-even pixels to ports, so maybe
> > the easiest option is to document globally that port 0 on the sink is
> > for even pixels, and port 1 for odd pixels, and remove the
> > DRM_LINK_DUAL_LVDS_ODD_EVEN flag completely. But what will then happen
> > if you panel has more than two ports (for audio for instance, or for
> > other types of video links) ? It may not be possible to always use port
> > 0 and 1 for the LVDS even and odd pixels in DT bindings of a particular
> > panel or bridge.
>
> This is the stickiest point of the whole series. The implementation within this
> series allows for any number of ports on the sink, the LVDS ports don't have
> to be port 0 and port 1, it's enough that the port for the even pixels comes
> before the port of the odd pixels (but the logic can be inverted by means of
> DRM_LINK_DUAL_LVDS_ODD_EVEN), and if you swap the lvds0 and lvds1
> OF graph connections around, the pixels will be swapped automatically.
> But of course, there is the dependency between the driver and dt-bindings
> you were mentioning, and of top of that every driver would need to work
> things out independently at this point in time.
>
> > A creative solution is needed here.
>
> I may have an idea. What if we marked both ends of each OF graph link
> with either "even-pixels;" or "odd-pixels;", and exported a function that
> given the of_node of two endpoints returned if the link requires swapping?
> There'd be no need for the flag at that point, the numbering of the ports
> would not matter, and the DT would be comprehensive and very easy to read.
>
> Let me please know your thoughts.
Taking one step back to look at the big picture, what we need is for the
source to know what pixel (odd or even) to send on each LVDS output. For
that it needs to know what pixel is expected by the sink the the inputs
connected to the source's outputs. From each source output we can easily
locate the DT nodes corresponding to the connected sink's input ports,
but that doesn't give us enough information. From there, we could
- Infer the odd/even pixel expected by the source based on the port
numbers. This would require common DT bindings for all dual-link LVDS
sinks that specify the port ordering (for instance the bindings could
mandate that lowest numbered port correspond to even pixels).
- Read the odd/even pixel expected by the source from an explicit DT
property, as proposed above. This would also require common DT
bindings for all dual-link LVDS sinks that define these new
properties. This would I think be better than implicitly infering it
from DT port numbers.
- Retrieve the information from the sink drm_bridge at runtime. This
would require a way to query the bridge for the mapping from port
number to odd/even pixel. The DRM_LINK_DUAL_LVDS_ODD_EVEN flag could
be used for that, and would then be defined as "the lowest numbered
port corresponds to odd pixels and the highest numbered port
corresponds to even pixels".
The second and third options would both work I think. The third one is
roughly what you've implemented, except that I think the flag
description should be clarified.
Sorry for circling back to your original proposal, I didn't understand
it properly :-)
> Thanks you for the patience
>
> > > */
> > > - is_bridge = true;
> > > - of_node_put(node);
> > > - break;
> > > + if (((comp_port - our_port > 0) && odd_even_flag) ||
> > > + ((comp_port - our_port < 0) && !odd_even_flag))
> > > + lvds->stripe_swap_data = true;
> > > }
> > > }
> > >
> > > - if (is_bridge) {
> > > - lvds->next_bridge = of_drm_find_bridge(remote);
> > > - if (!lvds->next_bridge) {
> > > - ret = -EPROBE_DEFER;
> > > - goto done;
> > > - }
> > > -
> > > - if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > > - lvds->dual_link = lvds->next_bridge->timings
> > > - ? lvds->next_bridge->timings->dual_link
> > > - : false;
> > > - } else {
> > > - lvds->panel = of_drm_find_panel(remote);
> > > - if (IS_ERR(lvds->panel)) {
> > > - ret = PTR_ERR(lvds->panel);
> > > - goto done;
> > > - }
> > > - }
> > > -
> > > - if (lvds->dual_link)
> > > - ret = rcar_lvds_parse_dt_companion(lvds);
> > > -
> > > done:
> > > of_node_put(local_output);
> > > - of_node_put(remote_input);
> > > of_node_put(remote);
> > >
> > > /*
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support
2019-08-20 16:04 ` Laurent Pinchart
@ 2019-08-21 17:00 ` Fabrizio Castro
0 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-21 17:00 UTC (permalink / raw)
To: Laurent Pinchart
Cc: David Airlie, Daniel Vetter, Kieran Bingham, dri-devel,
linux-renesas-soc, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, Jacopo Mondi
Hi Laurent,
Thank you for getting back to me.
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 20 August 2019 17:04
> Subject: Re: [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support
>
> Hi Fabrizio,
>
> On Thu, Aug 15, 2019 at 03:36:53PM +0000, Fabrizio Castro wrote:
> > On 15 August 2019 14:09 Laurent Pinchart wrote:
> > > On Thu, Aug 15, 2019 at 12:04:31PM +0100, Fabrizio Castro wrote:
> > > > This patch adds support for dual-LVDS panels.
> > > >
> > > > It's very important that we coordinate the efforts of both the
> > > > primary encoder and the companion encoder to get the right
> > > > picture on the panel, therefore this patch adds some code
> > > > to work out if even and odd pixels need swapping.
> > > > When the encoders are connected to a LVDS panel, the assumption
> > > > is that by default the panel expects even pixels (0, 2, 4, etc.)
> > > > on the first input port, and odd pixels (1, 3, 5, etc.) on the
> > > > seconds port. When DRM_LINK_DUAL_LVDS_ODD_EVEN is found among the
> > > > link flags, the display expects odd pixels on the first input
> > > > port, and even pixels on the second port. As a result, the way
> > > > the encoders are connected to the panel may trigger pixel (data)
> > > > swapping.
> > > >
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > >
> > > > ---
> > > > v1->v2:
> > > > * new patch, resulting from Laurent's feedback
> > > >
> > > > drivers/gpu/drm/rcar-du/rcar_lvds.c | 121 ++++++++++++++++++++++++------------
> > > > 1 file changed, 81 insertions(+), 40 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > > index 41d28f4..5c24884 100644
> > > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > > @@ -22,6 +22,8 @@
> > > > #include <drm/drm_bridge.h>
> > > > #include <drm/drm_panel.h>
> > > > #include <drm/drm_probe_helper.h>
> > > > +#include <drm/drm_timings.h>
> > > > +#include <drm/drm_of.h>
> > >
> > > Please keep the headers alphabetically sorted.
> >
> > Ok
> >
> > > >
> > > > #include "rcar_lvds.h"
> > > > #include "rcar_lvds_regs.h"
> > > > @@ -69,6 +71,7 @@ struct rcar_lvds {
> > > >
> > > > struct drm_bridge *companion;
> > > > bool dual_link;
> > > > + bool stripe_swap_data;
> > > > };
> > > >
> > > > #define bridge_to_rcar_lvds(b) \
> > > > @@ -439,12 +442,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> > > > rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
> > > >
> > > > if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > > > - /*
> > > > - * Configure vertical stripe based on the mode of operation of
> > > > - * the connected device.
> > > > - */
> > > > - rcar_lvds_write(lvds, LVDSTRIPE,
> > > > - lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
> > > > + u32 lvdstripe = 0;
> > > > +
> > > > + if (lvds->dual_link)
> > > > + /*
> > > > + * Configure vertical stripe based on the mode of
> > > > + * operation of the connected device.
> > > > + *
> > > > + * ST_SWAP from LVD1STRIPE is reserved, do not set
> > > > + * in the companion LVDS
> > > > + */
> > > > + lvdstripe = LVDSTRIPE_ST_ON |
> > > > + (lvds->companion && lvds->stripe_swap_data ?
> > > > + LVDSTRIPE_ST_SWAP : 0);
> > >
> > > Let's sort out the alignment.
> > >
> > > lvdstripe = LVDSTRIPE_ST_ON
> > > | (lvds->companion && lvds->stripe_swap_data ?
> > > LVDSTRIPE_ST_SWAP : 0);
> >
> > Ok
> >
> > > > + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
> > > > }
> > > >
> > > > /*
> > > > @@ -706,13 +717,31 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> > > > return ret;
> > > > }
> > > >
> > > > +static int rcar_lvds_get_remote_port_reg(struct device_node *np)
> > > > +{
> > > > + struct device_node *endpoint_node, *remote_endpoint;
> > > > + struct of_endpoint endpoint;
> > > > +
> > > > + endpoint_node = of_graph_get_endpoint_by_regs(np, 1, 0);
> > > > + if (!endpoint_node)
> > > > + return -ENODEV;
> > > > + remote_endpoint = of_graph_get_remote_endpoint(endpoint_node);
> > > > + if (!remote_endpoint) {
> > > > + of_node_put(endpoint_node);
> > > > + return -ENODEV;
> > > > + }
> > > > + of_graph_parse_endpoint(remote_endpoint, &endpoint);
> > > > + of_node_put(endpoint_node);
> > > > + of_node_put(remote_endpoint);
> > > > +
> > > > + return endpoint.port;
> > > > +}
> > > > +
> > > > static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > > > {
> > > > struct device_node *local_output = NULL;
> > > > - struct device_node *remote_input = NULL;
> > > > struct device_node *remote = NULL;
> > > > - struct device_node *node;
> > > > - bool is_bridge = false;
> > > > + const struct drm_timings *timings = NULL;
> > > > int ret = 0;
> > > >
> > > > local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> > > > @@ -740,45 +769,57 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > > > goto done;
> > > > }
> > > >
> > > > - remote_input = of_graph_get_remote_endpoint(local_output);
> > > > + ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
> > > > + &lvds->panel, &lvds->next_bridge);
> > > > + if (ret) {
> > > > + ret = -EPROBE_DEFER;
> > > > + goto done;
> > > > + }
> > > > + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > > > + if (lvds->next_bridge)
> > > > + timings = lvds->next_bridge->timings;
> > > > + else
> > > > + timings = lvds->panel->timings;
> > >
> > > I wonder if we should use devm_drm_panel_bridge_add() (or
> > > drm_panel_bridge_add()) and use the bridge API only. It would require a
> > > small change in the drm_panel_bridge to copy the timings pointer, but
> > > apart from that I think it should be fine. If it creates too much churn
> > > due to connector handling then we can skip it for now and I'll handle it
> > > later (but I'd appreciate if you could copy the timings pointer in
> > > drm_panel_bridge already).
> >
> > Will look into this.
> >
> > > > + if (timings)
> > > > + lvds->dual_link = timings->dual_link;
> > > > + }
> > > >
> > > > - for_each_endpoint_of_node(remote, node) {
> > > > - if (node != remote_input) {
> > > > + if (lvds->dual_link) {
> > > > + ret = rcar_lvds_parse_dt_companion(lvds);
> > > > + if (lvds->companion && timings) {
> > > > + int our_port, comp_port;
> > > > + bool odd_even_flag = timings->link_flags &
> > > > + DRM_LINK_DUAL_LVDS_ODD_EVEN;
> > > > + our_port = rcar_lvds_get_remote_port_reg(
> > > > + lvds->dev->of_node);
> > > > + if (our_port < 0) {
> > > > + ret = our_port;
> > > > + goto done;
> > > > + }
> > > > + comp_port = rcar_lvds_get_remote_port_reg(
> > > > + lvds->companion->of_node);
> > > > + if (comp_port < 0) {
> > > > + ret = comp_port;
> > > > + goto done;
> > > > + }
> > > > /*
> > > > - * We've found one endpoint other than the input, this
> > > > - * must be a bridge.
> > > > + * We need to match the port where we generate even
> > > > + * pixels (0, 2, 4, etc.) to the port where the sink
> > > > + * expects to receive even pixels, same thing for the
> > > > + * odd pixels. Swap the generation of even and odd
> > > > + * pixels if the connection requires it.
> > > > + * By default (when DRM_LINK_DUAL_LVDS_ODD_EVEN is not
> > > > + * specified) the sink expects even pixels on the
> > > > + * first input port, and odd pixels on the second port.
> > >
> > > I see what you're trying to do, but I'm not sure I like it much :-S
> > >
> > > Peeking into the remove DT node like that creates a dependency between
> > > this driver and the DT bindings of all possible remote nodes. For this
> > > to work, you would need to ensure that the odd/even mapping to ports is
> > > common to all dual-link devices, and thus document that globally in the
> > > DT bindings. I'm not sure if there's a way around it as hardware
> > > connections could indeed switch the two lanes, so we need to model that
> > > somehow. It could be modelled with a swap property in DT, but that would
> > > still require a standard mapping of odd-even pixels to ports, so maybe
> > > the easiest option is to document globally that port 0 on the sink is
> > > for even pixels, and port 1 for odd pixels, and remove the
> > > DRM_LINK_DUAL_LVDS_ODD_EVEN flag completely. But what will then happen
> > > if you panel has more than two ports (for audio for instance, or for
> > > other types of video links) ? It may not be possible to always use port
> > > 0 and 1 for the LVDS even and odd pixels in DT bindings of a particular
> > > panel or bridge.
> >
> > This is the stickiest point of the whole series. The implementation within this
> > series allows for any number of ports on the sink, the LVDS ports don't have
> > to be port 0 and port 1, it's enough that the port for the even pixels comes
> > before the port of the odd pixels (but the logic can be inverted by means of
> > DRM_LINK_DUAL_LVDS_ODD_EVEN), and if you swap the lvds0 and lvds1
> > OF graph connections around, the pixels will be swapped automatically.
> > But of course, there is the dependency between the driver and dt-bindings
> > you were mentioning, and of top of that every driver would need to work
> > things out independently at this point in time.
> >
> > > A creative solution is needed here.
> >
> > I may have an idea. What if we marked both ends of each OF graph link
> > with either "even-pixels;" or "odd-pixels;", and exported a function that
> > given the of_node of two endpoints returned if the link requires swapping?
> > There'd be no need for the flag at that point, the numbering of the ports
> > would not matter, and the DT would be comprehensive and very easy to read.
> >
> > Let me please know your thoughts.
>
> Taking one step back to look at the big picture, what we need is for the
> source to know what pixel (odd or even) to send on each LVDS output. For
> that it needs to know what pixel is expected by the sink the the inputs
> connected to the source's outputs. From each source output we can easily
> locate the DT nodes corresponding to the connected sink's input ports,
> but that doesn't give us enough information. From there, we could
>
> - Infer the odd/even pixel expected by the source based on the port
> numbers. This would require common DT bindings for all dual-link LVDS
> sinks that specify the port ordering (for instance the bindings could
> mandate that lowest numbered port correspond to even pixels).
>
> - Read the odd/even pixel expected by the source from an explicit DT
> property, as proposed above. This would also require common DT
> bindings for all dual-link LVDS sinks that define these new
> properties. This would I think be better than implicitly infering it
> from DT port numbers.
>
> - Retrieve the information from the sink drm_bridge at runtime. This
> would require a way to query the bridge for the mapping from port
> number to odd/even pixel. The DRM_LINK_DUAL_LVDS_ODD_EVEN flag could
> be used for that, and would then be defined as "the lowest numbered
> port corresponds to odd pixels and the highest numbered port
> corresponds to even pixels".
>
> The second and third options would both work I think. The third one is
> roughly what you've implemented, except that I think the flag
> description should be clarified.
I think I like the second option better, I will give it a shot. I was thinking,
perhaps we could drop the 'dual_link' member altogether (from
drm_<whatever>_timings) in this case, as if we found information about
even and odd pixels in the DT we could work out if we the configuration is
dual-link, so there'd be no need to mark this in drivers.
What do you think?
Thanks,
Fab
>
> Sorry for circling back to your original proposal, I didn't understand
> it properly :-)
>
> > Thanks you for the patience
> >
> > > > */
> > > > - is_bridge = true;
> > > > - of_node_put(node);
> > > > - break;
> > > > + if (((comp_port - our_port > 0) && odd_even_flag) ||
> > > > + ((comp_port - our_port < 0) && !odd_even_flag))
> > > > + lvds->stripe_swap_data = true;
> > > > }
> > > > }
> > > >
> > > > - if (is_bridge) {
> > > > - lvds->next_bridge = of_drm_find_bridge(remote);
> > > > - if (!lvds->next_bridge) {
> > > > - ret = -EPROBE_DEFER;
> > > > - goto done;
> > > > - }
> > > > -
> > > > - if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > > > - lvds->dual_link = lvds->next_bridge->timings
> > > > - ? lvds->next_bridge->timings->dual_link
> > > > - : false;
> > > > - } else {
> > > > - lvds->panel = of_drm_find_panel(remote);
> > > > - if (IS_ERR(lvds->panel)) {
> > > > - ret = PTR_ERR(lvds->panel);
> > > > - goto done;
> > > > - }
> > > > - }
> > > > -
> > > > - if (lvds->dual_link)
> > > > - ret = rcar_lvds_parse_dt_companion(lvds);
> > > > -
> > > > done:
> > > > of_node_put(local_output);
> > > > - of_node_put(remote_input);
> > > > of_node_put(remote);
> > > >
> > > > /*
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 8/9] drm/panel: lvds: Add support for the IDK-2121WR
2019-08-15 11:04 [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Fabrizio Castro
` (6 preceding siblings ...)
2019-08-15 11:04 ` [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support Fabrizio Castro
@ 2019-08-15 11:04 ` Fabrizio Castro
2019-08-15 11:04 ` [PATCH v2 9/9] arm64: dts: renesas: Add EK874 board with idk-2121wr display support Fabrizio Castro
2019-08-15 14:15 ` [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Sam Ravnborg
9 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 11:04 UTC (permalink / raw)
To: Laurent Pinchart, Thierry Reding, David Airlie, Daniel Vetter
Cc: Fabrizio Castro, Sam Ravnborg, dri-devel, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc, Kieran Bingham, Jacopo Mondi
The IDK-2121WR from Advantech is a dual-LVDS display.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v1->v2:
* new patch
drivers/gpu/drm/panel/panel-lvds.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
index 1ec57d0..2cd41757 100644
--- a/drivers/gpu/drm/panel/panel-lvds.c
+++ b/drivers/gpu/drm/panel/panel-lvds.c
@@ -22,6 +22,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_panel.h>
+#include <drm/drm_timings.h>
struct panel_lvds {
struct drm_panel panel;
@@ -259,6 +260,7 @@ static int panel_lvds_probe(struct platform_device *pdev)
/* Register the panel. */
drm_panel_init(&lvds->panel);
lvds->panel.dev = lvds->dev;
+ lvds->panel.timings = of_device_get_match_data(lvds->dev);
lvds->panel.funcs = &panel_lvds_funcs;
ret = drm_panel_add(&lvds->panel);
@@ -287,7 +289,13 @@ static int panel_lvds_remove(struct platform_device *pdev)
return 0;
}
+static const struct drm_timings advantech_idk_2121wr = {
+ .dual_link = true,
+ .link_flags = DRM_LINK_DUAL_LVDS_ODD_EVEN,
+};
+
static const struct of_device_id panel_lvds_of_table[] = {
+ { .compatible = "advantech,idk-2121wr", .data = &advantech_idk_2121wr},
{ .compatible = "panel-lvds", },
{ /* Sentinel */ },
};
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v2 9/9] arm64: dts: renesas: Add EK874 board with idk-2121wr display support
2019-08-15 11:04 [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Fabrizio Castro
` (7 preceding siblings ...)
2019-08-15 11:04 ` [PATCH v2 8/9] drm/panel: lvds: Add support for the IDK-2121WR Fabrizio Castro
@ 2019-08-15 11:04 ` Fabrizio Castro
2019-08-19 12:03 ` Geert Uytterhoeven
2019-08-15 14:15 ` [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Sam Ravnborg
9 siblings, 1 reply; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 11:04 UTC (permalink / raw)
To: Geert Uytterhoeven, Laurent Pinchart, Rob Herring, Mark Rutland
Cc: Fabrizio Castro, Simon Horman, Magnus Damm, linux-renesas-soc,
devicetree, Chris Paterson, Biju Das, Kieran Bingham,
Jacopo Mondi, xu_shunji, ebiharaml
The EK874 is advertised as compatible with panel IDK-2121WR from
Advantech, however the panel isn't sold alongside the board.
A new dts, adding everything that's required to get the panel to
to work with the EK874, is the most convenient way to support the
EK874 when it's connected to the IDK-2121WR.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
v1->v2:
* Added comment for lvds-connector-en-gpio
* Renamed &lvds0_panel_in to panel_in0
* Renamed &lvds1_panel_in to panel_in1
Geert,
at this point in time no other board uses this display, can we
postpone the creation of the .dtsi file to when there'll be another
user (if any)?
Thanks,
Fab
arch/arm64/boot/dts/renesas/Makefile | 3 +-
.../boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts | 116 +++++++++++++++++++++
2 files changed, 118 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
diff --git a/arch/arm64/boot/dts/renesas/Makefile b/arch/arm64/boot/dts/renesas/Makefile
index 42b74c2..ce48478 100644
--- a/arch/arm64/boot/dts/renesas/Makefile
+++ b/arch/arm64/boot/dts/renesas/Makefile
@@ -1,7 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
dtb-$(CONFIG_ARCH_R8A774A1) += r8a774a1-hihope-rzg2m.dtb
dtb-$(CONFIG_ARCH_R8A774A1) += r8a774a1-hihope-rzg2m-ex.dtb
-dtb-$(CONFIG_ARCH_R8A774C0) += r8a774c0-cat874.dtb r8a774c0-ek874.dtb
+dtb-$(CONFIG_ARCH_R8A774C0) += r8a774c0-cat874.dtb r8a774c0-ek874.dtb \
+ r8a774c0-ek874-idk-2121wr.dtb
dtb-$(CONFIG_ARCH_R8A7795) += r8a7795-salvator-x.dtb r8a7795-h3ulcb.dtb
dtb-$(CONFIG_ARCH_R8A7795) += r8a7795-h3ulcb-kf.dtb
dtb-$(CONFIG_ARCH_R8A7795) += r8a7795-salvator-xs.dtb
diff --git a/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
new file mode 100644
index 0000000..df8a60b
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for the Silicon Linux RZ/G2E evaluation kit (EK874),
+ * connected to an Advantech IDK-2121WR 21.5" LVDS panel
+ *
+ * Copyright (C) 2019 Renesas Electronics Corp.
+ */
+
+#include "r8a774c0-ek874.dts"
+
+/ {
+ backlight: backlight {
+ compatible = "pwm-backlight";
+ pwms = <&pwm5 0 50000>;
+
+ brightness-levels = <0 4 8 16 32 64 128 255>;
+ default-brightness-level = <6>;
+
+ power-supply = <®_12p0v>;
+ enable-gpios = <&gpio6 12 GPIO_ACTIVE_HIGH>;
+ };
+
+ panel-lvds {
+ compatible = "advantech,idk-2121wr", "panel-lvds";
+
+ width-mm = <476>;
+ height-mm = <268>;
+
+ data-mapping = "vesa-24";
+
+ panel-timing {
+ clock-frequency = <148500000>;
+ hactive = <1920>;
+ vactive = <1080>;
+ hsync-len = <44>;
+ hfront-porch = <88>;
+ hback-porch = <148>;
+ vfront-porch = <4>;
+ vback-porch = <36>;
+ vsync-len = <5>;
+ };
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ panel_in0: endpoint {
+ remote-endpoint = <&lvds0_out>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ panel_in1: endpoint {
+ remote-endpoint = <&lvds1_out>;
+ };
+ };
+ };
+ };
+};
+
+&gpio0 {
+ /*
+ * When GP0_17 is low LVDS[01] are connected to the LVDS connector
+ * When GP0_17 is high LVDS[01] are connected to the LT8918L
+ */
+ lvds-connector-en-gpio{
+ gpio-hog;
+ gpios = <17 GPIO_ACTIVE_HIGH>;
+ output-low;
+ line-name = "lvds-connector-en-gpio";
+ };
+};
+
+&lvds0 {
+ renesas,swap-data;
+
+ ports {
+ port@1 {
+ lvds0_out: endpoint {
+ remote-endpoint = <&panel_in0>;
+ };
+ };
+ };
+};
+
+&lvds1 {
+ status = "okay";
+
+ clocks = <&cpg CPG_MOD 727>, <&x13_clk>, <&extal_clk>;
+ clock-names = "fck", "dclkin.0", "extal";
+
+ ports {
+ port@1 {
+ lvds1_out: endpoint {
+ remote-endpoint = <&panel_in1>;
+ };
+ };
+ };
+};
+
+&pfc {
+ pwm5_pins: pwm5 {
+ groups = "pwm5_a";
+ function = "pwm5";
+ };
+};
+
+&pwm5 {
+ pinctrl-0 = <&pwm5_pins>;
+ pinctrl-names = "default";
+
+ status = "okay";
+};
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 9/9] arm64: dts: renesas: Add EK874 board with idk-2121wr display support
2019-08-15 11:04 ` [PATCH v2 9/9] arm64: dts: renesas: Add EK874 board with idk-2121wr display support Fabrizio Castro
@ 2019-08-19 12:03 ` Geert Uytterhoeven
2019-08-20 13:59 ` Fabrizio Castro
0 siblings, 1 reply; 40+ messages in thread
From: Geert Uytterhoeven @ 2019-08-19 12:03 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Geert Uytterhoeven, Laurent Pinchart, Rob Herring, Mark Rutland,
Simon Horman, Magnus Damm, Linux-Renesas,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Chris Paterson, Biju Das, Kieran Bingham, Jacopo Mondi,
xu_shunji, ebiharaml
Hi Fabrizio,
On Thu, Aug 15, 2019 at 1:05 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> The EK874 is advertised as compatible with panel IDK-2121WR from
> Advantech, however the panel isn't sold alongside the board.
> A new dts, adding everything that's required to get the panel to
> to work with the EK874, is the most convenient way to support the
> EK874 when it's connected to the IDK-2121WR.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Thanks for your patch!
> Geert,
>
> at this point in time no other board uses this display, can we
> postpone the creation of the .dtsi file to when there'll be another
> user (if any)?
OK.
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
> +&lvds0 {
> + renesas,swap-data;
Wasn't that property dropped?
I'd be grateful if anyone could review the panel parts.
Thanks!
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] 40+ messages in thread* RE: [PATCH v2 9/9] arm64: dts: renesas: Add EK874 board with idk-2121wr display support
2019-08-19 12:03 ` Geert Uytterhoeven
@ 2019-08-20 13:59 ` Fabrizio Castro
0 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-20 13:59 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Geert Uytterhoeven, Laurent Pinchart, Rob Herring, Mark Rutland,
Simon Horman, Magnus Damm, Linux-Renesas,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Chris Paterson, Biju Das, Kieran Bingham, Jacopo Mondi,
xu_shunji, ebiharaml
Hello Geert,
Thank you for your feedback!
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 19 August 2019 13:03
> Subject: Re: [PATCH v2 9/9] arm64: dts: renesas: Add EK874 board with idk-2121wr display support
>
> Hi Fabrizio,
>
> On Thu, Aug 15, 2019 at 1:05 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > The EK874 is advertised as compatible with panel IDK-2121WR from
> > Advantech, however the panel isn't sold alongside the board.
> > A new dts, adding everything that's required to get the panel to
> > to work with the EK874, is the most convenient way to support the
> > EK874 when it's connected to the IDK-2121WR.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> Thanks for your patch!
>
> > Geert,
> >
> > at this point in time no other board uses this display, can we
> > postpone the creation of the .dtsi file to when there'll be another
> > user (if any)?
>
> OK.
>
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
>
> > +&lvds0 {
> > + renesas,swap-data;
>
> Wasn't that property dropped?
Yeah, leftover from the previous series, sorry about that. Anyhow, we better decide
how to implement the swapping before doing anything with the DT related patch.
Thanks!
Fab
>
> I'd be grateful if anyone could review the panel parts.
> Thanks!
>
> 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] 40+ messages in thread
* Re: [PATCH v2 0/9] Add dual-LVDS panel support to EK874
2019-08-15 11:04 [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Fabrizio Castro
` (8 preceding siblings ...)
2019-08-15 11:04 ` [PATCH v2 9/9] arm64: dts: renesas: Add EK874 board with idk-2121wr display support Fabrizio Castro
@ 2019-08-15 14:15 ` Sam Ravnborg
2019-08-15 14:32 ` Fabrizio Castro
9 siblings, 1 reply; 40+ messages in thread
From: Sam Ravnborg @ 2019-08-15 14:15 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Laurent Pinchart, Geert Uytterhoeven, Thierry Reding,
David Airlie, Daniel Vetter, Rob Herring, Mark Rutland,
Andrzej Hajda, Maarten Lankhorst, Maxime Ripard, Sean Paul,
Simon Horman, Magnus Damm, Eric Anholt, Kieran Bingham,
dri-devel, devicetree, linux-kernel, linux-renesas-soc,
Chris Paterson, Biju Das, Jacopo Mondi, xu_shunji, ebiharaml
Hi Fabrizio
> it appears that Rob has been busy converting the dt-bindings relevant to this
> series, and his changes are now found in linux-next. Most notably
> Documentation/devicetree/bindings/display/panel/panel-lvds.txt has now become
> Documentation/devicetree/bindings/display/panel/lvds.yaml
>
> You have already taken patch:
> https://patchwork.kernel.org/patch/11072749/
>
> as such the patch I am sending you is still related to:
> Documentation/devicetree/bindings/display/panel/panel-lvds.txt
>
> Patch "dt-bindings: display: Add bindings for Advantech IDK-2121WR" is still
> assuming the format is .txt, as I am not too sure about what the protocol is in
> this case? Shall we take this patch and convert it later to .yaml or shall I
> convert it to .yaml straight away?
>
> Please, let me know what's the best course of action.
It is preferred that all new display and panel bindings uses the new
meta-schema format.
And that the writers uses the available tools to verify the bindings
submission.
This is one way to avoid simple mistakes in examples.
Sam
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH v2 0/9] Add dual-LVDS panel support to EK874
2019-08-15 14:15 ` [PATCH v2 0/9] Add dual-LVDS panel support to EK874 Sam Ravnborg
@ 2019-08-15 14:32 ` Fabrizio Castro
0 siblings, 0 replies; 40+ messages in thread
From: Fabrizio Castro @ 2019-08-15 14:32 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Laurent Pinchart, Geert Uytterhoeven, Thierry Reding,
David Airlie, Daniel Vetter, Rob Herring, Mark Rutland,
Andrzej Hajda, Maarten Lankhorst, Maxime Ripard, Sean Paul,
Simon Horman, Magnus Damm, Eric Anholt, Kieran Bingham,
dri-devel, devicetree, linux-kernel, linux-renesas-soc,
Chris Paterson, Biju Das, Jacopo Mondi, xu_shunji, ebiharaml
Hi Sam,
Thank you for your feedback!
> From: Sam Ravnborg <sam@ravnborg.org>
> Sent: 15 August 2019 15:16
> Subject: Re: [PATCH v2 0/9] Add dual-LVDS panel support to EK874
>
> Hi Fabrizio
>
> > it appears that Rob has been busy converting the dt-bindings relevant to this
> > series, and his changes are now found in linux-next. Most notably
> > Documentation/devicetree/bindings/display/panel/panel-lvds.txt has now become
> > Documentation/devicetree/bindings/display/panel/lvds.yaml
> >
> > You have already taken patch:
> > https://patchwork.kernel.org/patch/11072749/
> >
> > as such the patch I am sending you is still related to:
> > Documentation/devicetree/bindings/display/panel/panel-lvds.txt
> >
> > Patch "dt-bindings: display: Add bindings for Advantech IDK-2121WR" is still
> > assuming the format is .txt, as I am not too sure about what the protocol is in
> > this case? Shall we take this patch and convert it later to .yaml or shall I
> > convert it to .yaml straight away?
> >
> > Please, let me know what's the best course of action.
>
> It is preferred that all new display and panel bindings uses the new
> meta-schema format.
> And that the writers uses the available tools to verify the bindings
> submission.
> This is one way to avoid simple mistakes in examples.
Will give the meta-schema format a shot.
Thanks,
Fab
>
> Sam
^ permalink raw reply [flat|nested] 40+ messages in thread