diff mbox series

[RESEND,v7] dt-bindings: display: Add idk-2121wr binding

Message ID 20200306152031.14212-1-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State New, archived
Headers show
Series [RESEND,v7] dt-bindings: display: Add idk-2121wr binding | expand

Commit Message

Lad, Prabhakar March 6, 2020, 3:20 p.m. UTC
From: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Add binding for the idk-2121wr LVDS panel from Advantech.

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>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Apologies for flooding in I missed to add the ML email-ids for the earlier
version so resending it.

Hi All,

This patch is part of series [1] ("Add dual-LVDS panel support to EK874),
all the patches have been accepted from it except this one. I have fixed
Rob's comments in this version of the patch.

[1] https://patchwork.kernel.org/cover/11297589/

v6->7
 * Added reference to lvds.yaml
 * Changed maintainer to myself
 * Switched to dual license
 * Dropped required properties except for ports as rest are already listed
   in lvds.panel
 * Dropped Reviewed-by tag of Laurent, due to the changes made it might not
   be valid.

v5->v6:
 * No change

v4->v5:
* No change

v3->v4:
* Absorbed patch "dt-bindings: display: Add bindings for LVDS
  bus-timings"
* Big restructuring after Rob's and Laurent's comments

v2->v3:
* New patch

 .../display/panel/advantech,idk-2121wr.yaml        | 120 +++++++++++++++++++++
 1 file changed, 120 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml

Comments

Rob Herring (Arm) March 9, 2020, 8:32 p.m. UTC | #1
On Fri,  6 Mar 2020 15:20:31 +0000, Lad Prabhakar wrote:
> From: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> Add binding for the idk-2121wr LVDS panel from Advantech.
> 
> 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>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Apologies for flooding in I missed to add the ML email-ids for the earlier
> version so resending it.
> 
> Hi All,
> 
> This patch is part of series [1] ("Add dual-LVDS panel support to EK874),
> all the patches have been accepted from it except this one. I have fixed
> Rob's comments in this version of the patch.
> 
> [1] https://patchwork.kernel.org/cover/11297589/
> 
> v6->7
>  * Added reference to lvds.yaml
>  * Changed maintainer to myself
>  * Switched to dual license
>  * Dropped required properties except for ports as rest are already listed
>    in lvds.panel
>  * Dropped Reviewed-by tag of Laurent, due to the changes made it might not
>    be valid.
> 
> v5->v6:
>  * No change
> 
> v4->v5:
> * No change
> 
> v3->v4:
> * Absorbed patch "dt-bindings: display: Add bindings for LVDS
>   bus-timings"
> * Big restructuring after Rob's and Laurent's comments
> 
> v2->v3:
> * New patch
> 
>  .../display/panel/advantech,idk-2121wr.yaml        | 120 +++++++++++++++++++++
>  1 file changed, 120 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.example.dt.yaml: panel-lvds: 'port' is a required property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.example.dt.yaml: panel-lvds: 'port' is a required property

See https://patchwork.ozlabs.org/patch/1250384
Please check and re-submit.
Lad, Prabhakar March 9, 2020, 9:23 p.m. UTC | #2
Hi Rob,

On Mon, Mar 9, 2020 at 8:32 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri,  6 Mar 2020 15:20:31 +0000, Lad Prabhakar wrote:
> > From: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > Add binding for the idk-2121wr LVDS panel from Advantech.
> >
> > 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>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > Apologies for flooding in I missed to add the ML email-ids for the earlier
> > version so resending it.
> >
> > Hi All,
> >
> > This patch is part of series [1] ("Add dual-LVDS panel support to EK874),
> > all the patches have been accepted from it except this one. I have fixed
> > Rob's comments in this version of the patch.
> >
> > [1] https://patchwork.kernel.org/cover/11297589/
> >
> > v6->7
> >  * Added reference to lvds.yaml
> >  * Changed maintainer to myself
> >  * Switched to dual license
> >  * Dropped required properties except for ports as rest are already listed
> >    in lvds.panel
> >  * Dropped Reviewed-by tag of Laurent, due to the changes made it might not
> >    be valid.
> >
> > v5->v6:
> >  * No change
> >
> > v4->v5:
> > * No change
> >
> > v3->v4:
> > * Absorbed patch "dt-bindings: display: Add bindings for LVDS
> >   bus-timings"
> > * Big restructuring after Rob's and Laurent's comments
> >
> > v2->v3:
> > * New patch
> >
> >  .../display/panel/advantech,idk-2121wr.yaml        | 120 +++++++++++++++++++++
> >  1 file changed, 120 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
> >
>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.example.dt.yaml: panel-lvds: 'port' is a required property
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.example.dt.yaml: panel-lvds: 'port' is a required property
>
This panel is a dual channel LVDS, as a result the root port is called as
ports instead of port and the child node port@0 and port@1 are used for
even and odd pixels, hence binding has required property as ports instead
of port.

Cheers,
--Prabhakar

> See https://patchwork.ozlabs.org/patch/1250384
> Please check and re-submit.
Sam Ravnborg March 9, 2020, 9:47 p.m. UTC | #3
Hi Prabhakar

On Mon, Mar 09, 2020 at 09:23:24PM +0000, Lad, Prabhakar wrote:
> Hi Rob,
> 
> On Mon, Mar 9, 2020 at 8:32 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri,  6 Mar 2020 15:20:31 +0000, Lad Prabhakar wrote:
> > > From: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > Add binding for the idk-2121wr LVDS panel from Advantech.
> > >
> > > 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>
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > Apologies for flooding in I missed to add the ML email-ids for the earlier
> > > version so resending it.
> > >
> > > Hi All,
> > >
> > > This patch is part of series [1] ("Add dual-LVDS panel support to EK874),
> > > all the patches have been accepted from it except this one. I have fixed
> > > Rob's comments in this version of the patch.
> > >
> > > [1] https://patchwork.kernel.org/cover/11297589/
> > >
> > > v6->7
> > >  * Added reference to lvds.yaml
> > >  * Changed maintainer to myself
> > >  * Switched to dual license
> > >  * Dropped required properties except for ports as rest are already listed
> > >    in lvds.panel
> > >  * Dropped Reviewed-by tag of Laurent, due to the changes made it might not
> > >    be valid.
> > >
> > > v5->v6:
> > >  * No change
> > >
> > > v4->v5:
> > > * No change
> > >
> > > v3->v4:
> > > * Absorbed patch "dt-bindings: display: Add bindings for LVDS
> > >   bus-timings"
> > > * Big restructuring after Rob's and Laurent's comments
> > >
> > > v2->v3:
> > > * New patch
> > >
> > >  .../display/panel/advantech,idk-2121wr.yaml        | 120 +++++++++++++++++++++
> > >  1 file changed, 120 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
> > >
> >
> > My bot found errors running 'make dt_binding_check' on your patch:
> >
> > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.example.dt.yaml: panel-lvds: 'port' is a required property
> > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.example.dt.yaml: panel-lvds: 'port' is a required property
> >
> This panel is a dual channel LVDS, as a result the root port is called as
> ports instead of port and the child node port@0 and port@1 are used for
> even and odd pixels, hence binding has required property as ports instead
> of port.

What goes wrong is that you have a ref to lvds.yaml - and thus you get
also required from that file.

So basically - I think this binding should not have a ref to lvds.yaml -
as the binding needs to be different.

	Sam
Lad, Prabhakar March 9, 2020, 10:28 p.m. UTC | #4
Hi Sam,

On Mon, Mar 9, 2020 at 9:47 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Prabhakar
>
> On Mon, Mar 09, 2020 at 09:23:24PM +0000, Lad, Prabhakar wrote:
> > Hi Rob,
> >
> > On Mon, Mar 9, 2020 at 8:32 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Fri,  6 Mar 2020 15:20:31 +0000, Lad Prabhakar wrote:
> > > > From: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > >
> > > > Add binding for the idk-2121wr LVDS panel from Advantech.
> > > >
> > > > 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>
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > Apologies for flooding in I missed to add the ML email-ids for the earlier
> > > > version so resending it.
> > > >
> > > > Hi All,
> > > >
> > > > This patch is part of series [1] ("Add dual-LVDS panel support to EK874),
> > > > all the patches have been accepted from it except this one. I have fixed
> > > > Rob's comments in this version of the patch.
> > > >
> > > > [1] https://patchwork.kernel.org/cover/11297589/
> > > >
> > > > v6->7
> > > >  * Added reference to lvds.yaml
> > > >  * Changed maintainer to myself
> > > >  * Switched to dual license
> > > >  * Dropped required properties except for ports as rest are already listed
> > > >    in lvds.panel
> > > >  * Dropped Reviewed-by tag of Laurent, due to the changes made it might not
> > > >    be valid.
> > > >
> > > > v5->v6:
> > > >  * No change
> > > >
> > > > v4->v5:
> > > > * No change
> > > >
> > > > v3->v4:
> > > > * Absorbed patch "dt-bindings: display: Add bindings for LVDS
> > > >   bus-timings"
> > > > * Big restructuring after Rob's and Laurent's comments
> > > >
> > > > v2->v3:
> > > > * New patch
> > > >
> > > >  .../display/panel/advantech,idk-2121wr.yaml        | 120 +++++++++++++++++++++
> > > >  1 file changed, 120 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
> > > >
> > >
> > > My bot found errors running 'make dt_binding_check' on your patch:
> > >
> > > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.example.dt.yaml: panel-lvds: 'port' is a required property
> > > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.example.dt.yaml: panel-lvds: 'port' is a required property
> > >
> > This panel is a dual channel LVDS, as a result the root port is called as
> > ports instead of port and the child node port@0 and port@1 are used for
> > even and odd pixels, hence binding has required property as ports instead
> > of port.
>
> What goes wrong is that you have a ref to lvds.yaml - and thus you get
> also required from that file.
>
Agreed.

> So basically - I think this binding should not have a ref to lvds.yaml -
> as the binding needs to be different.
>
Yes makes sense, will post a v8 dropping the reference to lvds.yaml

Cheers,
--Prabhakar

>         Sam
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
new file mode 100644
index 0000000..b05df05
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/advantech,idk-2121wr.yaml
@@ -0,0 +1,120 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/advantech,idk-2121wr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Advantech IDK-2121WR 21.5" Full-HD dual-LVDS panel
+
+maintainers:
+  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
+  - Thierry Reding <thierry.reding@gmail.com>
+
+description: |
+  The IDK-2121WR from Advantech is a Full-HD dual-LVDS panel.
+  A dual-LVDS interface is a dual-link connection with even pixels traveling
+  on one link, and with odd pixels traveling on the other link.
+
+  The panel expects odd pixels on the first port, and even pixels on the
+  second port, therefore the ports must be marked accordingly (with either
+  dual-lvds-odd-pixels or dual-lvds-even-pixels).
+
+allOf:
+  - $ref: lvds.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: advantech,idk-2121wr
+      - {} # panel-lvds, but not listed here to avoid false select
+
+  width-mm:
+    const: 476
+
+  height-mm:
+    const: 268
+
+  data-mapping:
+    const: vesa-24
+
+  ports:
+    type: object
+    properties:
+      port@0:
+        type: object
+        description: The sink for odd pixels.
+        properties:
+          reg:
+            const: 0
+
+          dual-lvds-odd-pixels: true
+
+        required:
+          - reg
+          - dual-lvds-odd-pixels
+
+      port@1:
+        type: object
+        description: The sink for even pixels.
+        properties:
+          reg:
+            const: 1
+
+          dual-lvds-even-pixels: true
+
+        required:
+          - reg
+          - dual-lvds-even-pixels
+
+  panel-timing: true
+
+additionalProperties: false
+
+required:
+  - ports
+
+examples:
+  - |+
+    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>;
+          dual-lvds-odd-pixels;
+          panel_in0: endpoint {
+            remote-endpoint = <&lvds0_out>;
+          };
+        };
+
+        port@1 {
+          reg = <1>;
+          dual-lvds-even-pixels;
+          panel_in1: endpoint {
+            remote-endpoint = <&lvds1_out>;
+          };
+        };
+      };
+    };
+
+...