Re: [RFC PATCH 1/4] dt-bindings: display: Convert common panel bindings to DT schema

From: Rob Herring <robh@kernel.org>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	devicetree@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Sam Ravnborg <sam@ravnborg.org>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [RFC PATCH 1/4] dt-bindings: display: Convert common panel bindings to DT schema
Date: Thu, 20 Jun 2019 08:52:36 -0600	[thread overview]
Message-ID: <CAL_JsqKC-RDjdMQWM6yk_HiWu-WwuU+vUf946t=TDJAxnqMW7Q@mail.gmail.com> (raw)
In-Reply-To: <20190620090122.GB26689@ulmo>

On Thu, Jun 20, 2019 at 3:01 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Wed, Jun 19, 2019 at 03:51:53PM -0600, Rob Herring wrote:
> > Convert the common panel bindings to DT schema consolidating scattered
> > definitions to a single schema file.
> >
> > The 'simple-panel' binding just a collection of properties and not a
> > complete binding itself. All of the 'simple-panel' properties are
> > covered by the panel-common.txt binding with the exception of the
> > 'no-hpd' property, so add that to the schema.
> >
> > As there are lots of references to simple-panel.txt, just keep the file
> > with a reference to panel-common.yaml for now until all the bindings are
> > converted.
> >
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > Note there's still some references to panel-common.txt that I need to
> > update or just go ahead and convert to schema.
> >
> >  .../bindings/display/panel/panel-common.txt   | 101 -------------
> >  .../bindings/display/panel/panel-common.yaml  | 143 ++++++++++++++++++
> >  .../bindings/display/panel/panel.txt          |   4 -
> >  .../bindings/display/panel/simple-panel.txt   |  29 +---
> >  4 files changed, 144 insertions(+), 133 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/display/panel/panel-common.txt
> >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-common.yaml
>
> I know it was this way before, but perhaps remove the redundant panel-
> prefix while at it?

Sure.

> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > new file mode 100644
> > index 000000000000..6fe87254edad
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> > @@ -0,0 +1,143 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/panel/panel-common.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Common Properties for Display Panels
> > +
> > +maintainers:
> > +  - Thierry Reding <thierry.reding@gmail.com>
> > +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > +
> > +description: |
> > +  This document defines device tree properties common to several classes of
> > +  display panels. It doesn't constitue a device tree binding specification by
> > +  itself but is meant to be referenced by device tree bindings.
> > +
> > +  When referenced from panel device tree bindings the properties defined in this
> > +  document are defined as follows. The panel device tree bindings are
> > +  responsible for defining whether each property is required or optional.
> > +
> > +
>
> Are the two blank lines here on purpose?

No.

> The original document had two
> blank lines here, but that was mostly for readability I would guess. The
> YAML format doesn't really need additional formatting for readability,
> so perhaps just remove the extra blank line?
>
> > +properties:
> > +  # Descriptive Properties
> > +  width-mm:
> > +    description: The width-mm and height-mm specify the width and height of the
> > +      physical area where images are displayed. These properties are expressed
> > +      in millimeters and rounded to the closest unit.
> > +
> > +  height-mm:
> > +    description: The width-mm and height-mm specify the width and height of the
> > +      physical area where images are displayed. These properties are expressed
> > +      in millimeters and rounded to the closest unit.
>
> I suppose there's no way in YAML to share the description between both
> the width-mm and height-mm properties? It's a little unfortunate that we
> have to copy, but if there's no better way, guess we'll have to live
> with it.

I could make it a comment instead, but then we loose being able to
parse it. I should probably just reword them to be separate:

"Specifies the height of the physical area where images are displayed.
The property is expressed in millimeters and rounded to the closest
unit."

Also, just realized I need to make these 2 dependencies on either
other (i.e. not valid to only have one).

> > +  label:
> > +    description: |
> > +      The label property specifies a symbolic name for the panel as a
> > +      string suitable for use by humans. It typically contains a name inscribed
> > +      on the system (e.g. as an affixed label) or specified in the system's
> > +      documentation (e.g. in the user's manual).
> > +
> > +      If no such name exists, and unless the property is mandatory according to
> > +      device tree bindings, it shall rather be omitted than constructed of
> > +      non-descriptive information. For instance an LCD panel in a system that
> > +      contains a single panel shall not be labelled "LCD" if that name is not
> > +      inscribed on the system or used in a descriptive fashion in system
> > +      documentation.
> > +
> > +  rotation:
> > +    description:
> > +      Display rotation in degrees counter clockwise (0,90,180,270)
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > +      - enum: [ 0, 90, 180, 270 ]
> > +
> > +  # Display Timings
> > +  panel-timing:
>
> Am I the only one bugged by the redundancy in this property name? What
> else is the timing going to express if not the timing of the panel that
> it's part of. "timing" really would be enough. Anyway, not much we can
> do about it now.

I'm just happy we have a defined name.

Rob