Message-ID: <20191208144533.GA14311@pendragon.ideasonboard.com>
Date:   Sun, 8 Dec 2019 16:45:33 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Rob Clark <robdclark@...il.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
        aarch64-laptops@...ts.linaro.org,
        Vasily Khoruzhick <anarsoul@...il.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Jeffrey Hugo <jhugo@...eaurora.org>,
        Rob Clark <robdclark@...omium.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Sam Ravnborg <sam@...nborg.org>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] dt-bindings: display: panel: document panel-id

Hi Rob,

Thank you for the patch.

On Sat, Dec 07, 2019 at 12:35:50PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@...omium.org>
> 
> For devices that have one of several possible panels installed, the
> panel-id property gives firmware a generic way to locate and enable the
> panel node corresponding to the installed panel.  Example of how to use
> this property:
> 
>     ivo_panel {
>         compatible = "ivo,m133nwf4-r0";
>         panel-id = <0xc5>;
>         status = "disabled";
> 
>         ports {
>             port {
>                 ivo_panel_in_edp: endpoint {
>                     remote-endpoint = <&sn65dsi86_out_ivo>;
>                 };
>             };
>         };
>     };
> 
>     boe_panel {
>         compatible = "boe,nv133fhm-n61";
>         panel-id = <0xc4>;
>         status = "disabled";
> 
>         ports {
>             port {
>                 boe_panel_in_edp: endpoint {
>                     remote-endpoint = <&sn65dsi86_out_boe>;
>                 };
>             };
>         };
>     };
> 
>     sn65dsi86: bridge@2c {
>         compatible = "ti,sn65dsi86";
> 
>         ports {
>             #address-cells = <1>;
>             #size-cells = <0>;
> 
>             port@0 {
>                 reg = <0>;
>                 sn65dsi86_in_a: endpoint {
>                     remote-endpoint = <&dsi0_out>;
>                 };
>             };
> 
>             port@1 {
>                 reg = <1>;
> 
>                 sn65dsi86_out_boe: endpoint@c4 {
>                     remote-endpoint = <&boe_panel_in_edp>;
>                 };
> 
>                 sn65dsi86_out_ivo: endpoint@c5 {
>                     remote-endpoint = <&ivo_panel_in_edp>;
>                 };
>             };
>         };
>     };
> 
> Signed-off-by: Rob Clark <robdclark@...omium.org>
> ---
>  .../bindings/display/panel/panel-common.yaml  | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> index ef8d8cdfcede..6113319b91dd 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
> @@ -75,6 +75,32 @@ properties:
>        in the device graph bindings defined in
>        Documentation/devicetree/bindings/graph.txt.
>  
> +  panel-id:
> +    description:
> +      To support the case where one of several different panels can be installed
> +      on a device, the panel-id property can be used by the firmware to identify
> +      which panel should have it's status changed to "ok".  This property is not
> +      used by the HLOS itself.

If your firmware can modify the status property of a panel, it can also
add DT nodes. As discussed before, I don't think this belongs to DT.
Even if panel-id isn't used by the operating system, you have Linux
kernel patches in this series that show that this isn't transparent.

This needs to be handled in the firmware (or, if not possible, in a
kernel board driver). The above DT fragment, visible to the kernel,
doesn't describe the actual hardware. Furthermore, you would require all
bridge drivers to be patched to support this method, which shows again
that the issue isn't handled in the right place.

Finally, unless I'm mistaken, this series is meant to support display
for an ACPI-based ARM machine. Using DT as a stop-gap measure because
ACPI support isn't there yet is fine out-of-tree, and fine by me in-tree
provided that the DT bindings are clean, but not when DT is abused like
this.

I'm sorry, but this is a NACK from me. Please handle this transparently
in the firmware if you want DT-based boot, or with ACPI.

> +
> +      For a device with multiple potential panels, a node for each potential
> +      should be defined with status = "disabled", and an appropriate panel-id
> +      property.  The video data producer should be setup with endpoints going to
> +      each possible panel.  The firmware will find the dt node with a panel-id
> +      matching the actual panel installed, and change it's status to "ok".
> +
> +      The exact method the firmware uses to determine the panel-id of the installed
> +      panel is outside the scope of this binding, but a few examples are
> +
> +      1) u-boot module reading a value from a u-boot env var
> +      2) EFI driver module reading a value from an EFI variable
> +      3) device specific firmware reading some device specific GPIOs or
> +         e-fuse
> +
> +      The panel-id values are an opaque integer.  They can be sparse.  The only
> +      important thing is that each possible panel in the system has a unique
> +      panel-id, and that the values configured in the device's DTB match the
> +      values that the firmware is looking for.
> +
>    ddc-i2c-bus:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description:

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists