Re: [PATCH 5/6] dt-bindings: display: add Unisoc's mipi dsi&dphy bindings

[Rob Herring <robh+dt@kernel.org>]

On Tue, Dec 1, 2020 at 6:56 AM Kevin Tang <kevin3.tang@gmail.com> wrote:
>
> Hi Rob,
>
> Rob Herring <robh+dt@kernel.org> 于2020年12月1日周二 上午4:31写道：
>>
>> On Mon, Nov 30, 2020 at 7:29 AM Kevin Tang <kevin3.tang@gmail.com> wrote:
>> >
>> > From: Kevin Tang <kevin.tang@unisoc.com>
>> >
>> > Adds MIPI DSI Master and MIPI DSI-PHY (D-PHY)
>> > support for Unisoc's display subsystem.
>> >
>> > Cc: Orson Zhai <orsonzhai@gmail.com>
>> > Cc: Chunyan Zhang <zhang.lyra@gmail.com>
>> > Signed-off-by: Kevin Tang <kevin.tang@unisoc.com>
>> > ---
>> >  .../display/sprd/sprd,sharkl3-dsi-host.yaml        | 107 +++++++++++++++++++++
>> >  .../display/sprd/sprd,sharkl3-dsi-phy.yaml         |  84 ++++++++++++++++
>> >  2 files changed, 191 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
>> >  create mode 100644 Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-phy.yaml
>> >
>> > diff --git a/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml b/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
>> > new file mode 100644
>> > index 0000000..fe0e89d
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
>> > @@ -0,0 +1,107 @@
>> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> > +%YAML 1.2
>> > +---
>> > +$id: http://devicetree.org/schemas/display/sprd/sprd,sharkl3-dsi-host.yaml#
>> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> > +
>> > +title: Unisoc MIPI DSI Controller
>> > +
>> > +maintainers:
>> > +  - Kevin Tang <kevin.tang@unisoc.com>
>> > +
>> > +properties:
>> > +  compatible:
>> > +    const: sprd,sharkl3-dsi-host
>> > +
>> > +  reg:
>> > +    maxItems: 1
>> > +    description:
>> > +      Physical base address and length of the registers set for the device.
>> > +
>> > +  interrupts:
>> > +    maxItems: 2
>> > +    description:
>> > +      Should contain DSI interrupt.
>> > +
>> > +  clocks:
>> > +    minItems: 1
>> > +
>> > +  clock-names:
>> > +    items:
>> > +      - const: clk_src_96m
>> > +
>> > +  power-domains:
>> > +    maxItems: 1
>> > +    description: A phandle to DSIM power domain node
>> > +
>> > +  ports:
>> > +    type: object
>> > +
>> > +    properties:
>> > +      "#address-cells":
>> > +        const: 1
>> > +
>> > +      "#size-cells":
>> > +        const: 0
>> > +
>> > +      port@0:
>> > +        type: object
>> > +        description:
>> > +          A port node with endpoint definitions as defined in
>> > +          Documentation/devicetree/bindings/media/video-interfaces.txt.
>> > +          That port should be the input endpoint, usually coming from
>> > +          the associated DPU.
>> > +      port@1:
>> > +        type: object
>> > +        description:
>> > +          A port node with endpoint definitions as defined in
>> > +          Documentation/devicetree/bindings/media/video-interfaces.txt.
>> > +          That port should be the output endpoint, usually output to
>> > +          the associated DPHY.
>> > +
>> > +    required:
>> > +      - "#address-cells"
>> > +      - "#size-cells"
>> > +      - port@0
>> > +      - port@1
>> > +
>> > +    additionalProperties: false
>> > +
>> > +required:
>> > +  - compatible
>> > +  - reg
>> > +  - interrupts
>> > +  - clocks
>> > +  - clock-names
>> > +  - ports
>> > +
>> > +additionalProperties: false
>> > +
>> > +examples:
>> > +  - |
>> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> > +    #include <dt-bindings/clock/sprd,sc9860-clk.h>
>> > +    dsi: dsi@63100000 {
>> > +        compatible = "sprd,sharkl3-dsi-host";
>> > +        reg = <0x63100000 0x1000>;
>> > +        interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>,
>> > +          <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>> > +        clock-names = "clk_src_96m";
>> > +        clocks = <&pll CLK_TWPLL_96M>;
>> > +        ports {
>> > +            #address-cells = <1>;
>> > +            #size-cells = <0>;
>> > +            port@0 {
>> > +                reg = <0>;
>> > +                dsi_in: endpoint {
>> > +                    remote-endpoint = <&dpu_out>;
>> > +                };
>> > +            };
>> > +            port@1 {
>> > +                reg = <1>;
>> > +                dsi_out: endpoint {
>> > +                    remote-endpoint = <&dphy_in>;
>> > +                };
>> > +            };
>> > +        };
>> > +    };
>> > diff --git a/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-phy.yaml b/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-phy.yaml
>> > new file mode 100644
>> > index 0000000..b4715d5
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-phy.yaml
>> > @@ -0,0 +1,84 @@
>> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> > +%YAML 1.2
>> > +---
>> > +$id: http://devicetree.org/schemas/display/sprd/sprd,sharkl3-dsi-phy.yaml#
>> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> > +
>> > +title: Unisoc MIPI DSI-PHY (D-PHY)
>> > +
>> > +maintainers:
>> > +  - Kevin Tang <kevin.tang@unisoc.com>
>> > +
>> > +properties:
>> > +  compatible:
>> > +    const: sprd,sharkl3-dsi-phy
>> > +
>> > +  reg:
>> > +    maxItems: 1
>> > +    description:
>> > +      Must be the dsi controller base address.
>> > +
>> > +  ports:
>> > +    type: object
>> > +
>> > +    properties:
>> > +      "#address-cells":
>> > +        const: 1
>> > +
>> > +      "#size-cells":
>> > +        const: 0
>> > +
>> > +      port@0:
>> > +        type: object
>> > +        description:
>> > +          A port node with endpoint definitions as defined in
>> > +          Documentation/devicetree/bindings/media/video-interfaces.txt.
>> > +          That port should be the output endpoint, usually output to
>> > +          the associated panel.
>> > +      port@1:
>>
>> For PHYs, we use the PHY binding, not the graph binding. Please follow
>> what practically every other DSI PHY does.
>
> It seems that the dphy driver does not need to exist alone, so i remove dphy and dsi graph binding, merge the dphy driver into the dsi driver.

Maybe not now, but what about in the future? PHYs tend to change with
every process node and digital blocks like the DSI controller may not.

If the DPHY is a separate h/w block then it should be a separate node
in the DT. Whether you have 1 driver or 2 is independent of the DT
binding. Drivers and DT nodes don't have to be 1:1. It's just
convenient when they are.

Rob