Hi Sam,

Thank you for the patch.

On Sun, Feb 16, 2020 at 07:15:10PM +0100, Sam Ravnborg wrote:

Add display-timings.yaml - that references panel-timings.yaml. display-timings.yaml will be used for display bindings when they are converted to meta-schema format.

For now the old display-timing.txt points to the new display-timings.yaml - and all users are left as-is.

v2:

• Updated native-mode description

v3:

• Simpler "^timing" pattern (Rob)
• timing node is of type object (Rob)
• added display-timings to panel-common.yaml
• added yaml document terminator "..."

Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Rob Herring robh@kernel.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Thierry Reding thierry.reding@gmail.com Cc: Oleksandr Suvorov oleksandr.suvorov@toradex.com Cc: devicetree@vger.kernel.org

---

.../bindings/display/panel/display-timing.txt | 124 +----------------- .../display/panel/display-timings.yaml | 77 +++++++++++ .../bindings/display/panel/panel-common.yaml | 8 ++ 3 files changed, 86 insertions(+), 123 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/panel/display-timings.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/display-timing.txt b/Documentation/devicetree/bindings/display/panel/display-timing.txt index 78222ced1874..7f55ad4a40c4 100644 --- a/Documentation/devicetree/bindings/display/panel/display-timing.txt +++ b/Documentation/devicetree/bindings/display/panel/display-timing.txt @@ -1,123 +1 @@

-display-timing bindings

-display-timings node

-required properties:

• • none

-optional properties:

• • native-mode: The native mode for the display, in case multiple modes are

• provided. When omitted, assume the first node is the native.
  

-timing subnode

-required properties:

• • hactive, vactive: display resolution
• • hfront-porch, hback-porch, hsync-len: horizontal display timing parameters
• in pixels
• vfront-porch, vback-porch, vsync-len: vertical display timing parameters in
• lines
• • clock-frequency: display clock in Hz

-optional properties:

• • hsync-active: hsync pulse is active low/high/ignored
• • vsync-active: vsync pulse is active low/high/ignored
• • de-active: data-enable pulse is active low/high/ignored
• • pixelclk-active: with

• 	- active high = drive pixel data on rising edge/
  

• 			sample data on falling edge
  

• 	- active low  = drive pixel data on falling edge/
  

• 			sample data on rising edge
  

• 	- ignored     = ignored
  

• • syncclk-active: with

• 	- active high = drive sync on rising edge/
  

• 			sample sync on falling edge of pixel
  

• 			clock
  

• 	- active low  = drive sync on falling edge/
  

• 			sample sync on rising edge of pixel
  

• 			clock
  

• 	- omitted     = same configuration as pixelclk-active
  

• • interlaced (bool): boolean to enable interlaced mode
• • doublescan (bool): boolean to enable doublescan mode
• • doubleclk (bool): boolean to enable doubleclock mode

-All the optional properties that are not bool follow the following logic:

• <1>: high active
• <0>: low active
• omitted: not used on hardware

-There are different ways of describing the capabilities of a display. The -devicetree representation corresponds to the one commonly found in datasheets -for displays. If a display supports multiple signal timings, the native-mode -can be specified.

-The parameters are defined as:

• +----------+-------------------------------------+----------+-------+
• | | ^ | | |
• | | |vback_porch | | |
• | | v | | |
• +----------#######################################----------+-------+
• | # ^ # | |
• | # | # | |
• | hback # | # hfront | hsync |
• | porch # | hactive # porch | len |
• |<-------->#<-------+--------------------------->#<-------->|<----->|
• | # | # | |
• | # |vactive # | |
• | # | # | |
• | # v # | |
• +----------#######################################----------+-------+
• | | ^ | | |
• | | |vfront_porch | | |
• | | v | | |
• +----------+-------------------------------------+----------+-------+
• | | ^ | | |
• | | |vsync_len | | |
• | | v | | |
• +----------+-------------------------------------+----------+-------+

-Note: In addition to being used as subnode(s) of display-timings, the timing

•  subnode may also be used on its own. This is appropriate if only one mode
  

•  need be conveyed. In this case, the node should be named 'panel-timing'.
  

-Example:

• display-timings {

• native-mode = <&timing0>;
  

• timing0: 1080p24 {
  

• 	/* 1920x1080p24 */
  

• 	clock-frequency = <52000000>;
  

• 	hactive = <1920>;
  

• 	vactive = <1080>;
  

• 	hfront-porch = <25>;
  

• 	hback-porch = <25>;
  

• 	hsync-len = <25>;
  

• 	vback-porch = <2>;
  

• 	vfront-porch = <2>;
  

• 	vsync-len = <2>;
  

• 	hsync-active = <1>;
  

• };
  

• };

-Every required property also supports the use of ranges, so the commonly used -datasheet description with minimum, typical and maximum values can be used.

-Example:

• timing1: timing {

• /* 1920x1080p24 */
  

• clock-frequency = <148500000>;
  

• hactive = <1920>;
  

• vactive = <1080>;
  

• hsync-len = <0 44 60>;
  

• hfront-porch = <80 88 95>;
  

• hback-porch = <100 148 160>;
  

• vfront-porch = <0 4 6>;
  

• vback-porch = <0 36 50>;
  

• vsync-len = <0 5 6>;
  

• };

+See display-timings.yaml in this directory. diff --git a/Documentation/devicetree/bindings/display/panel/display-timings.yaml b/Documentation/devicetree/bindings/display/panel/display-timings.yaml new file mode 100644 index 000000000000..c8c0c9cb0492 --- /dev/null +++ b/Documentation/devicetree/bindings/display/panel/display-timings.yaml @@ -0,0 +1,77 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/panel/display-timings.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: display timing bindings

s/timing/timings/

+maintainers:

• • Thierry Reding thierry.reding@gmail.com
• • Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
• • Sam Ravnborg sam@ravnborg.org

+description: |

• A display panel may be able to handle several display timings,
• with different resolutions.
• The display-timings node makes it possible to specify the timing

s/the timing/the timings/

• and to specify the timing that is native for the display.

+properties:

• $nodename:
• const: display-timings
• native-mode:
• $ref: /schemas/types.yaml#/definitions/phandle
• description: |

•  The default display timing is the one specified as native-mode.
  

•  If no native-mode is specified then the first node is assumed the
  

s/assumed the/assumed to be the/

•  native mode.
  

+patternProperties:

• "^timing":

Should this be "^timing[0-9]*$", or do we want to allow or names ?

• type: object
• allOf:

•  - $ref: panel-timing.yaml#
  

+additionalProperties: false

+examples:

• • |+
• /*

• * Example that specifies panel timing using minimum, typical,
  

• * maximum values as commonly used in datasheet description.
  

• * timing1 is the native-mode.
  

• */
  

• display-timings {

•    native-mode = <&timing1>;
  

Does this compile, as there's no phandle named timing1 ?

•    timing0 {
  

•        /* 1920x1080p24 */
  

•        clock-frequency = <148500000>;
  

•        hactive = <1920>;
  

•        vactive = <1080>;
  

•        hsync-len = <0 44 60>;
  

•        hfront-porch = <80 88 95>;
  

•        hback-porch = <100 148 160>;
  

•        vfront-porch = <0 4 6>;
  

•        vback-porch = <0 36 50>;
  

•        vsync-len = <0 5 6>;
  

•    };
  

•    timing1 {
  

•        /* 1920x1080p24 */
  

•        clock-frequency = <52000000>;
  

•        hactive = <1920>;
  

•        vactive = <1080>;
  

•        hfront-porch = <25>;
  

•        hback-porch = <25>;
  

•        hsync-len = <0 25 25>;
  

•        vback-porch = <2>;
  

•        vfront-porch = <2>;
  

•        vsync-len = <2>;
  

•        hsync-active = <1>;
  

•        pixelclk-active = <1>;
  

•    };
  

• };

+... diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml index 8070c439adbd..ed051ba12084 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml @@ -61,6 +61,14 @@ properties: allOf: - $ref: panel-timing.yaml#

• display-timings:
• description:

•  Some display panels supports several resolutions with different timing.
  

s/timing/timings/

•  The display-timings bindings supports specifying several timings and
  

•  optional specify which is the native mode.
  

s/optional/optionally/

With these small issues fixed,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Great work !

• allOf:

•  - $ref: display-timings.yaml#
  

• # Connectivity port: type: object

On Tue, Feb 18, 2020 at 02:13:45PM -0600, Rob Herring wrote:

On Sun, Feb 16, 2020 at 12:15 PM Sam Ravnborg sam@ravnborg.org wrote:

...

Add data-mapping property that can be used to specify the media format used for the connection betwwen the display controller (connector) and the panel. Signed-off-by: Sam Ravnborg sam@ravnborg.org

Missing blank line.

...

---

.../devicetree/bindings/display/panel/panel-dpi.yaml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml index 40079fc24a63..6a03d2449701 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml @@ -21,6 +21,16 @@ properties: - {} - const: panel-dpi

• data-mapping:
• enum:

•  - rgb24
  

•  - rgb565
  

•  - bgr666
  

•  - lvds666
  

Doesn't lvds666 come from i.MX IPU which as I remember has built-in LVDS block? I'd think this format would be implicit when using the LVDS block and panel. It doesn't seem this is actually used anywhere either.

I must admit that I just copied this list from Oleksandrs original patch. The MEDIA type it identifies(MEDIA_BUS_FMT_RGB666_1X24_CPADHI) looks special. I will drop lvds666 while applying, unless I get other feedback. (Note: travelling, earliest in the weekend)

Btw. anyway I can add data-mapping to panel-common - and then list the allowed enum values in each binding?

I would love to have a central definition of data-mapping, and then let the users only allow the relevant subset so we catch errors in DT files early.

Sam

Hi Sam,

On Tue, Feb 18, 2020 at 11:16:38PM +0100, Sam Ravnborg wrote:

On Tue, Feb 18, 2020 at 02:13:45PM -0600, Rob Herring wrote:

...

On Sun, Feb 16, 2020 at 12:15 PM Sam Ravnborg sam@ravnborg.org wrote:

...

Add data-mapping property that can be used to specify the media format used for the connection betwwen the display controller (connector) and the panel. Signed-off-by: Sam Ravnborg sam@ravnborg.org

Missing blank line.

...

---

.../devicetree/bindings/display/panel/panel-dpi.yaml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml index 40079fc24a63..6a03d2449701 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml @@ -21,6 +21,16 @@ properties: - {} - const: panel-dpi

• data-mapping:
• enum:

•  - rgb24
  

•  - rgb565
  

•  - bgr666
  

•  - lvds666
  

Doesn't lvds666 come from i.MX IPU which as I remember has built-in LVDS block? I'd think this format would be implicit when using the LVDS block and panel. It doesn't seem this is actually used anywhere either.

I must admit that I just copied this list from Oleksandrs original patch. The MEDIA type it identifies(MEDIA_BUS_FMT_RGB666_1X24_CPADHI) looks special. I will drop lvds666 while applying, unless I get other feedback. (Note: travelling, earliest in the weekend)

There are different data mappings defined for LVDS, we should follow them. lvds666 is wrong in any case, and doesn't apply to a DPI panel anyway.

I don't like the name data-mapping much for DPI panels I'm afraid. It made sense for LVDS as it's really about how the different data bits are mapped to LVDS time slots, but for DPI, what we need to describe is the format. I also wonder whether multiple formats wouldn't be required when the panel supports more than one, but that may not apply to panels covered by these bindings.

If a panel expects RGB888 and receives RGB666 with the two LSBs of each component hardwired to GND on the PCB, should DT report RGB888 or RGB666 on the panel side ? I'm tempted by the former, and specifying the latter on the transmitting side.

Please also note that this case is already described by Documentation/devicetree/bindings/media/video-interfaces.txt through two entirely different properties, bus-width and data-shift. I think we should try to standardize mappings between display and capture. This new property should be reconsidered in my opinion, I think it was merged too soon.

Btw. anyway I can add data-mapping to panel-common - and then list the allowed enum values in each binding?

I would love to have a central definition of data-mapping, and then let the users only allow the relevant subset so we catch errors in DT files early.

Hi Sam,

Thank you for the patch.

On Sun, Feb 16, 2020 at 07:15:13PM +0100, Sam Ravnborg wrote:

RFC only - not tested yet!

The panel-dpi compatible is a fallback that allows the DT to specify the timing.

When matching panel-dpi expect the device tree to include the timing information for the display-panel.

Background for this change: There are a lot of panels and new models hits the market very often. It is a lost cause trying to chase them all and users of new panels will often find them in situations that the panel they ues are not

s/ues/use/

supported by the kernel. On top of this a lot of panels are customized based on customer specifications.

Including the panel timing in the device tree allows for a simple way to describe the actual HW and use this description in a generic way in the kernel. This allows uses of proprietary panels, or panels which are not included in the kernel, to specify the timing in the device tree together with all the other HW descriptions. And thus, using the device tree it is then easy to add support for an otherwise unknown panel.

The current support expect panels that do not require any delays for prepare/enable/disable/unprepare.

I've proposed something similar a few times in the past, so I can't disagree :-)

Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maxime Ripard mripard@kernel.org Cc: Oleksandr Suvorov oleksandr.suvorov@toradex.com

---

drivers/gpu/drm/panel/panel-simple.c | 74 +++++++++++++++++++++++++++- 1 file changed, 72 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 82363d05bad4..188526637398 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -351,6 +351,65 @@ static const struct drm_panel_funcs panel_simple_funcs = { .get_timings = panel_simple_get_timings, };

+static struct panel_desc panel_dpi;

Do you need this, can't you set data to 0 in the panel-dpi entry ?

+static int panel_dpi_probe(struct device *dev,

• 	   struct panel_simple *panel)
  

+{

• struct display_timing *timing;
• const struct device_node *np;
• struct panel_desc *desc;
• unsigned int bus_flags;
• struct videomode vm;
• const char *mapping;
• int ret;
• np = dev->of_node;
• desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
• if (!desc)

• return -ENOMEM;
  

• timing = devm_kzalloc(dev, sizeof(*timing), GFP_KERNEL);
• if (!timing)

• return -ENOMEM;
  

• ret = of_get_display_timing(np, "panel-timing", timing);
• if (ret < 0) {

• dev_err(dev, "%pOF: no panel-timing node found for \"panel-dpi\" binding\n",
  

• 	np);
  

• return ret;
  

• }
• desc->timings = timing;
• desc->num_timings = 1;
• of_property_read_u32(np, "width-mm", &desc->size.width);
• of_property_read_u32(np, "height-mm", &desc->size.height);
• of_property_read_string(np, "data-mapping", &mapping);
• if (!strcmp(mapping, "rgb24"))

• desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
  

• else if (!strcmp(mapping, "rgb565"))

• desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
  

• else if (!strcmp(mapping, "bgr666"))

• desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
  

• else if (!strcmp(mapping, "lvds666"))

• desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
  

I think you've dropped lvds666 in the DT bindings, so this should be dropped too. Furthermore, I think the data-mapping property should be reconsidered (see my review of the bindings), so this would need to change too.

• /* Extract bus_flags from display_timing */
• bus_flags = 0;
• vm.flags = timing->flags;
• drm_bus_flags_from_videomode(&vm, &bus_flags);
• desc->bus_flags = bus_flags;
• /* We do not know the connector for the DT node, so guess it */
• desc->connector_type = DRM_MODE_CONNECTOR_DPI;
• panel->desc = desc;
• return 0;

+}

#define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \ (to_check->field.typ >= bounds->field.min && \ to_check->field.typ <= bounds->field.max) @@ -437,8 +496,15 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) return -EPROBE_DEFER; }

• if (!of_get_display_timing(dev->of_node, "panel-timing", &dt))

• panel_simple_parse_panel_timing_node(dev, panel, &dt);
  

• if (desc == &panel_dpi) {

• /* Handle the generic panel-dpi binding */
  

• err = panel_dpi_probe(dev, panel);
  

• if (err)
  

• 	goto free_ddc;
  

• } else {

• if (!of_get_display_timing(dev->of_node, "panel-timing", &dt))
  

• 	panel_simple_parse_panel_timing_node(dev, panel, &dt);
  

• }

  drm_panel_init(&panel->base, dev, &panel_simple_funcs, desc->connector_type);

@@ -3688,6 +3754,10 @@ static const struct of_device_id platform_of_match[] = { }, { .compatible = "winstar,wf35ltiacd", .data = &winstar_wf35ltiacd,

• }, {

• /* Must be the last entry */
  

• .compatible = "panel-dpi",
  

• .data = &panel_dpi,
  

  }, { /* sentinel */ }