[v1,1/3] dt-bindings: display: add panel-timing.yaml

Add meta-schema variant of panel-timing and
reference it from panel-common.yaml.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
Cc: devicetree@vger.kernel.org
---
 .../bindings/display/panel/panel-common.yaml  |   7 +-
 .../bindings/display/panel/panel-timing.yaml  | 253 ++++++++++++++++++
 2 files changed, 256 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/panel/panel-timing.yaml

On Mon, Jan 20, 2020 at 10:07 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Add meta-schema variant of panel-timing and
> reference it from panel-common.yaml.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>

There some typos:

> diff --git a/Documentation/devicetree/bindings/display/panel/panel-timing.yaml b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml
> new file mode 100644
> index 000000000000..59891c7a58ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml
> @@ -0,0 +1,253 @@
...
> +description: |
> +  There are different ways of describing the timing data data of a panel. The
-----------------------------------------------------------------------^^^^^^^^^
s/data data/data/

...
> +  vactive:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Verical panel resolution in pixels
---------^^^^^^
Vertical

Anyway, those are minor issues, so

Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>

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

--
Best regards
Oleksandr Suvorov

Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500
4800 (main line)

Hi,

On Mon, Jan 20, 2020 at 09:06:39PM +0100, Sam Ravnborg wrote:
> Add meta-schema variant of panel-timing and
> reference it from panel-common.yaml.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/display/panel/panel-common.yaml  |   7 +-
>  .../bindings/display/panel/panel-timing.yaml  | 253 ++++++++++++++++++
>  2 files changed, 256 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-timing.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> index ef8d8cdfcede..8070c439adbd 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> @@ -54,13 +54,12 @@ properties:
>
>    # Display Timings
>    panel-timing:
> -    type: object
>      description:
>        Most display panels are restricted to a single resolution and
>        require specific display timings. The panel-timing subnode expresses those
> -      timings as specified in the timing subnode section of the display timing
> -      bindings defined in
> -      Documentation/devicetree/bindings/display/panel/display-timing.txt.
> +      timings.
> +    allOf:
> +      - $ref: panel-timing.yaml#
>
>    # Connectivity
>    port:
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-timing.yaml b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml
> new file mode 100644
> index 000000000000..59891c7a58ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml
> @@ -0,0 +1,253 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/panel-timing.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: panel timing bindings
> +
> +maintainers:
> +  - Thierry Reding <thierry.reding@gmail.com>
> +  - Sam Ravnborg <sam@ravnborg.org>
> +
> +description: |
> +  There are different ways of describing the timing data data of a panel. The
> +  devicetree representation corresponds to the one commonly found in datasheets
> +  for panels. If a panel supports multiple signal timings, the native-mode
> +  can be specified.
> +
> +  The parameters are defined as seen in the following illustration.
> +
> +  +----------+-------------------------------------+----------+-------+
> +  |          |        ^                            |          |       |
> +  |          |        |vback_porch                 |          |       |
> +  |          |        v                            |          |       |
> +  +----------#######################################----------+-------+
> +  |          #        ^                            #          |       |
> +  |          #        |                            #          |       |
> +  |  hback   #        |                            #  hfront  | hsync |
> +  |   porch  #        |       hactive              #  porch   |  len  |
> +  |<-------->#<-------+--------------------------->#<-------->|<----->|
> +  |          #        |                            #          |       |
> +  |          #        |vactive                     #          |       |
> +  |          #        |                            #          |       |
> +  |          #        v                            #          |       |
> +  +----------#######################################----------+-------+
> +  |          |        ^                            |          |       |
> +  |          |        |vfront_porch                |          |       |
> +  |          |        v                            |          |       |
> +  +----------+-------------------------------------+----------+-------+
> +  |          |        ^                            |          |       |
> +  |          |        |vsync_len                   |          |       |
> +  |          |        v                            |          |       |
> +  +----------+-------------------------------------+----------+-------+
> +
> +
> +  The following is the panel timings shown with time on the x-axis.
> +  This matches the timing diagrams often found in data sheets.
> +
> +              Active                 Front           Sync           Back
> +              Region                 Porch                          Porch
> +  <-----------------------><----------------><-------------><-------------->
> +    //////////////////////|
> +   ////////////////////// |
> +  //////////////////////  |..................               ................
> +                                             _______________
> +
> +  Timing can be specified either as a typical value or as a tuple
> +  of min, typ, max values.
> +
> +properties:
> +
> +  clock-frequency:
> +   $ref: /schemas/types.yaml#/definitions/uint32
> +   description: |
> +     Panel clock in Hz
> +
> +  hactive:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +       Horizontal panel resolution in pixels
> +
> +  vactive:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Verical panel resolution in pixels
> +
> +  hfront-porch:
> +    description: |
> +      Horizontal front porch panel timing
> +    oneOf:
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - maxItems: 1
> +          items:
> +            description: typical number of pixels

You have that on a number of occasions, but that could just be an
uint32, right?

> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - minItems: 3
> +          maxItems: 3
> +          items:
> +            description: min, typ, max number of pixels

When minItems and maxitems are equal, you can just set one, the other
one will be filled by the DT schemas tools.

> +  hsync-active:
> +    description: |
> +      Horizontal sync pulse.
> +      If omitted then it is not used by the hardware
> +    oneOf:
> +      - const: 0
> +        description: active low
> +      - const: 1
> +        description: active high

You probably should use an enum here (and in other similar
places). oneOf / anyOf / allOF errors are pretty cryptic, while it
will it's really better with an enum.

Thanks!
Maxime

On Mon, Jan 20, 2020 at 2:07 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Add meta-schema variant of panel-timing and
> reference it from panel-common.yaml.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/display/panel/panel-common.yaml  |   7 +-
>  .../bindings/display/panel/panel-timing.yaml  | 253 ++++++++++++++++++
>  2 files changed, 256 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-timing.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> index ef8d8cdfcede..8070c439adbd 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> @@ -54,13 +54,12 @@ properties:
>
>    # Display Timings
>    panel-timing:
> -    type: object
>      description:
>        Most display panels are restricted to a single resolution and
>        require specific display timings. The panel-timing subnode expresses those
> -      timings as specified in the timing subnode section of the display timing
> -      bindings defined in
> -      Documentation/devicetree/bindings/display/panel/display-timing.txt.
> +      timings.
> +    allOf:
> +      - $ref: panel-timing.yaml#
>
>    # Connectivity
>    port:
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-timing.yaml b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml
> new file mode 100644
> index 000000000000..59891c7a58ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml
> @@ -0,0 +1,253 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

Some of this comes from the existing binding files, do you have rights
to change the license (it would be nice to get it).

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/panel-timing.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: panel timing bindings
> +
> +maintainers:
> +  - Thierry Reding <thierry.reding@gmail.com>
> +  - Sam Ravnborg <sam@ravnborg.org>
> +
> +description: |
> +  There are different ways of describing the timing data data of a panel. The
> +  devicetree representation corresponds to the one commonly found in datasheets
> +  for panels. If a panel supports multiple signal timings, the native-mode
> +  can be specified.
> +
> +  The parameters are defined as seen in the following illustration.
> +
> +  +----------+-------------------------------------+----------+-------+
> +  |          |        ^                            |          |       |
> +  |          |        |vback_porch                 |          |       |
> +  |          |        v                            |          |       |
> +  +----------#######################################----------+-------+
> +  |          #        ^                            #          |       |
> +  |          #        |                            #          |       |
> +  |  hback   #        |                            #  hfront  | hsync |
> +  |   porch  #        |       hactive              #  porch   |  len  |
> +  |<-------->#<-------+--------------------------->#<-------->|<----->|
> +  |          #        |                            #          |       |
> +  |          #        |vactive                     #          |       |
> +  |          #        |                            #          |       |
> +  |          #        v                            #          |       |
> +  +----------#######################################----------+-------+
> +  |          |        ^                            |          |       |
> +  |          |        |vfront_porch                |          |       |
> +  |          |        v                            |          |       |
> +  +----------+-------------------------------------+----------+-------+
> +  |          |        ^                            |          |       |
> +  |          |        |vsync_len                   |          |       |
> +  |          |        v                            |          |       |
> +  +----------+-------------------------------------+----------+-------+
> +
> +
> +  The following is the panel timings shown with time on the x-axis.
> +  This matches the timing diagrams often found in data sheets.
> +
> +              Active                 Front           Sync           Back
> +              Region                 Porch                          Porch
> +  <-----------------------><----------------><-------------><-------------->
> +    //////////////////////|
> +   ////////////////////// |
> +  //////////////////////  |..................               ................
> +                                             _______________
> +
> +  Timing can be specified either as a typical value or as a tuple
> +  of min, typ, max values.
> +
> +properties:
> +
> +  clock-frequency:
> +   $ref: /schemas/types.yaml#/definitions/uint32

There's already a type definition for this.

> +   description: |

Drop '|' unless you have multiple paragraphs or need to maintain formatting.

> +     Panel clock in Hz
> +
> +  hactive:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +       Horizontal panel resolution in pixels
> +
> +  vactive:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Verical panel resolution in pixels

typo

> +
> +  hfront-porch:
> +    description: |
> +      Horizontal front porch panel timing
> +    oneOf:
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - maxItems: 1
> +          items:
> +            description: typical number of pixels
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - minItems: 3
> +          maxItems: 3
> +          items:
> +            description: min, typ, max number of pixels

I think you can describe it like this (test with 2 items though):

allOf:
  - $ref: /schemas/types.yaml#/definitions/uint32-array
not:
  maxItems: 2
items:
  - description: ...
  - description: ...
  - description: ...

> +
> +  hback-porch:
> +    description: |
> +      Horizontal back porch timing
> +    oneOf:
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - maxItems: 1
> +          items:
> +            description: typical number of pixels
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - minItems: 3
> +          maxItems: 3
> +          items:
> +            description: min, typ, max number of pixels
> +
> +  hsync-len:
> +    description: |
> +      Horizontal sync length panel timing
> +    oneOf:
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - maxItems: 1
> +          items:
> +            description: typical number of pixels
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - minItems: 3
> +          maxItems: 3
> +          items:
> +            description: min, typ, max number of pixels
> +
> +  vfront-porch:
> +    description: |
> +      Vertical front porch panel timing
> +    oneOf:
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - maxItems: 1
> +          items:
> +            description: typical number of lines
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - minItems: 3
> +          maxItems: 3
> +          items:
> +            description: min, typ, max number of lines
> +
> +  vback-porch:
> +    description: |
> +      Vertical back porch panel timing
> +    oneOf:
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - maxItems: 1
> +          items:
> +            description: typical number of lines
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - minItems: 3
> +          maxItems: 3
> +          items:
> +            description: min, typ, max number of lines
> +
> +  vsync-len:
> +    description: |
> +      Vertical sync length panel timing
> +    oneOf:
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - maxItems: 1
> +          items:
> +            description: typical number of lines
> +      - allOf:
> +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> +        - minItems: 3
> +          maxItems: 3
> +          items:
> +            description: min, typ, max number of lines
> +
> +  hsync-active:
> +    description: |
> +      Horizontal sync pulse.
> +      If omitted then it is not used by the hardware
> +    oneOf:
> +      - const: 0
> +        description: active low
> +      - const: 1
> +        description: active high
> +
> +  vsync-active:
> +    description: |
> +      Vertical sync pulse.
> +      If omitted then it is not used by the hardware
> +    oneOf:
> +      - const: 0
> +        description: active low
> +      - const: 1
> +        description: active high
> +
> +  de-active:
> +    description: |
> +      Data enable.
> +      If omitted then it is not used by the hardware
> +    oneOf:
> +      - const: 0
> +        description: active low
> +      - const: 1
> +        description: active high
> +
> +  pixelclk-active:
> +    description: |
> +      Data driving on rising or falling edge
> +    oneOf:
> +      - const: 0
> +        description: |
> +          Drive pixel data on falling edge and
> +          sample data on rising edge
> +      - const: 1
> +        description: |
> +          Drive pixel data on rising edge and
> +          sample data on falling edge
> +
> +  syncclk-active:
> +    description: |
> +      Drive sync on rising or sample sync on falling edge.
> +      If not specified then the same as pixelclk-active
> +    oneOf:
> +      - const: 0
> +        description: |
> +          Drive sync on falling edge and
> +          sample sync on rising edge of pixel clock
> +      - const: 1
> +        description: |
> +          Drive sync on rising edge and
> +          sample sync on falling edge of pixel clock
> +
> +  interlaced:
> +    type: boolean
> +    description: Enable interlaced mode
> +
> +  doublescan:
> +    type: boolean
> +    description: Enable double scan mode
> +
> +  doubleclk:
> +    type: boolean
> +    description: Enable double clock mode
> +
> +required:
> + - clock-frequency
> + - hactive
> + - vactive
> + - hfront-porch
> + - hback-porch
> + - hsync-len
> + - vfront-porch
> + - vback-porch
> + - vsync-len
> +
> +additionalProperties: false
> --
> 2.20.1
>

Hi Oleksandr.

> 
> There some typos:
> 
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-timing.yaml b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml
> > new file mode 100644
> > index 000000000000..59891c7a58ee
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml
> > @@ -0,0 +1,253 @@
> ...
> > +description: |
> > +  There are different ways of describing the timing data data of a panel. The
> -----------------------------------------------------------------------^^^^^^^^^
> s/data data/data/
> 
> ...
> > +  vactive:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      Verical panel resolution in pixels
> ---------^^^^^^
> Vertical

Fixed in v2 - thanks.

> 
> Anyway, those are minor issues, so
> 
> Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>

	Sam

Hi Maxime.

Thanks for your comments!
> 
> > +      - allOf:
> > +        - $ref: /schemas/types.yaml#/definitions/uint32-array
> > +        - minItems: 3
> > +          maxItems: 3
> > +          items:
> > +            description: min, typ, max number of pixels
> 
> When minItems and maxitems are equal, you can just set one, the other
> one will be filled by the DT schemas tools.

I tried to drop minItems: 3
But it just did not work - I could not make it
accept <xx> and <xx xx xx>.

And the schema should, and does, reject <xx xx> and <xx xx xx xx>

I do not know why - I just did a trial-and-error on this.

> > +  hsync-active:
> > +    description: |
> > +      Horizontal sync pulse.
> > +      If omitted then it is not used by the hardware
> > +    oneOf:
> > +      - const: 0
> > +        description: active low
> > +      - const: 1
> > +        description: active high
> 
> You probably should use an enum here (and in other similar
> places). oneOf / anyOf / allOF errors are pretty cryptic, while it
> will it's really better with an enum.

Yep - enum: [0, 1] is much nicer.

	Sam

Hi Oleksandr

Thanks for the feedback on the typos.

> 
> Anyway, those are minor issues, so
> 
> Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>

I did a number of other changes so I dropped this r-b,
as you really did not review the v2 version.
v2 coming later today.

	Sam