Re: [PATCH v7 5/8] dt-bindings: display: add i.MX6 MIPI DSI host controller doc

[Adrian Ratiu <adrian.ratiu@collabora.com>]

On Wed, 22 Apr 2020, Laurent Pinchart 
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Adrian, 
>

Hi Laurent,
 
> On Wed, Apr 22, 2020 at 03:58:33AM +0300, Laurent Pinchart 
> wrote: 
>> On Tue, Apr 21, 2020 at 07:16:07PM +0300, Adrian Ratiu wrote: 
>> > This provides an example DT binding for the MIPI DSI host 
>> > controller present on the i.MX6 SoC based on Synopsis 
>> > DesignWare v1.01 IP.   Cc: Rob Herring <robh@kernel.org> Cc: 
>> > Neil Armstrong <narmstrong@baylibre.com> Cc: Fabio Estevam 
>> > <festevam@gmail.com> Cc: Laurent Pinchart 
>> > <laurent.pinchart@ideasonboard.com> Cc: 
>> > devicetree@vger.kernel.org Tested-by: Adrian Pop 
>> > <pop.adrian61@gmail.com> Tested-by: Arnaud Ferraris 
>> > <arnaud.ferraris@collabora.com> Signed-off-by: Sjoerd Simons 
>> > <sjoerd.simons@collabora.com> Signed-off-by: Martyn Welch 
>> > <martyn.welch@collabora.com> Signed-off-by: Adrian Ratiu 
>> > <adrian.ratiu@collabora.com> --- Changes since v6: 
>> >   - Added ref to the newly created snps,dw-mipi-dsi.yaml 
>> >   (Laurent) - Moved *-cells properties outside 
>> >   patternProperties (Laurent) - Removed the panel port 
>> >   documentation (Laurent) - Wrapped lines at 80 chars, typo 
>> >   fixes, sort includes (Laurent) 
>> >  Changes since v5: 
>> >   - Fixed missing reg warning (Fabio) - Updated dt-schema and 
>> >   fixed warnings (Rob) 
>> >  Changes since v4: 
>> >   - Fixed yaml binding to pass `make dt_binding_check 
>> >   dtbs_check` and addressed received binding feedback (Rob) 
>> >  Changes since v3: 
>> >   - Added commit message (Neil) - Converted to yaml format 
>> >   (Neil) - Minor dt node + driver fixes (Rob) - Added small 
>> >   panel example to the host controller binding 
>> >  Changes since v2: 
>> >   - Fixed commit tags (Emil) 
>> > --- 
>> >  .../display/imx/fsl,mipi-dsi-imx6.yaml        | 135 
>> >  ++++++++++++++++++ 1 file changed, 135 insertions(+) create 
>> >  mode 100644 
>> >  Documentation/devicetree/bindings/display/imx/fsl,mipi-dsi-imx6.yaml 
>> >  diff --git 
>> > a/Documentation/devicetree/bindings/display/imx/fsl,mipi-dsi-imx6.yaml 
>> > b/Documentation/devicetree/bindings/display/imx/fsl,mipi-dsi-imx6.yaml 
>> > new file mode 100644 index 0000000000000..b73e3ae33a852 --- 
>> > /dev/null +++ 
>> > b/Documentation/devicetree/bindings/display/imx/fsl,mipi-dsi-imx6.yaml 
>> > @@ -0,0 +1,135 @@ +# SPDX-License-Identifier: (GPL-2.0-only 
>> > OR BSD-2-Clause) +%YAML 1.2 +--- +$id: 
>> > http://devicetree.org/schemas/display/imx/fsl,mipi-dsi-imx6.yaml# 
>> > +$schema: http://devicetree.org/meta-schemas/core.yaml# + 
>> > +title: Freescale i.MX6 DW MIPI DSI Host Controller + 
>> > +maintainers: +  - Adrian Ratiu <adrian.ratiu@collabora.com> 
>> > + +description: | +  The i.MX6 DSI host controller is a 
>> > Synopsys DesignWare MIPI DSI v1.01 +  IP block with a 
>> > companion PHY IP. 
> 
> I forgot to mention, if there's a companion PHY, shouldn't it be 
> referenced from the DT bindings ? 
>

I don't think so, that description was copied verbatim from the 
imx6 ref manual IIRC, the physical layer is the same for MIPI DSI 
which does TX as for MIPI CSI which does RX, but looking at the 
ref manual and how drivers are written I don't think it's 
necessary.

This might change if we wanted to unify the DSI and CSI drivers a 
bit, but considering the scope already associated with this patch 
series I'm a bit afraid to open a subject like that =)

>> > +
>> > +  These DT bindings follow the Synopsys DW MIPI DSI bindings defined in
>> > +  Documentation/devicetree/bindings/display/bridge/dw_mipi_dsi.txt with
>> > +  the following device-specific properties.
>> > +
>> > +allOf:
>> > +  - $ref: ../bridge/snps,dw-mipi-dsi.yaml#
>> > +
>> > +properties:
>> > +  '#address-cells':
>> > +    const: 1
>> > +
>> > +  '#size-cells':
>> > +    const: 0
>> > +
>> > +  compatible:
>> > +    items:
>> > +      - const: fsl,imx6q-mipi-dsi
>> > +      - const: snps,dw-mipi-dsi
>> > +
>> > +  reg:
>> > +    maxItems: 1
>> > +
>> > +  interrupts:
>> > +    maxItems: 1
>> > +
>> > +  clocks:
>> > +    items:
>> > +      - description: Module Clock
>> > +      - description: DSI bus clock
>> > +
>> > +  clock-names:
>> > +    items:
>> > +      - const: ref
>> > +      - const: pclk
>> > +
>> > +  fsl,gpr:
>> > +    description:
>> > +      Phandle to the iomuxc-gpr region containing the multiplexer ctrl register.
>> > +    $ref: /schemas/types.yaml#/definitions/phandle
>> > +
>> > +  ports:
>> > +    type: object
>> > +    description: |
>> > +      A node containing DSI input & output port nodes with endpoint
>> > +      definitions as documented in
>> > +      Documentation/devicetree/bindings/media/video-interfaces.txt
>> > +      Documentation/devicetree/bindings/graph.txt
>> > +    properties:
>> > +      port@0:
>> > +        type: object
>> > +        description:
>> > +          DSI input port node, connected to the ltdc rgb output port.
>> > +
>> > +      port@1:
>> > +        type: object
>> > +        description:
>> > +          RGB output port node, connected to a panel or a bridge input port.
>> 
>> Isn't it the other way around, doesn't the bridge take RGB input and
>> output DSI ? And to be precise, it's not about RGB, but about the input
>> being parallel interface (DSI will also carry RGB).
>> 
>> I would add
>> 
>>     required:
>>       - port@0
>>       - port@1
>> 
>> > +
>> > +additionalProperties: false
>> > +
>> > +patternProperties:
>> > +  "^panel@[0-3]$":
>> > +    type: object
>> > +
>> > +required:
>> > +  - "#address-cells"
>> > +  - "#size-cells"
>> > +  - compatible
>> > +  - reg
>> > +  - interrupts
>> > +  - clocks
>> > +  - clock-names
>> > +  - ports
>> > +
>> > +examples:
>> > +  - |+
>> > +    #include <dt-bindings/clock/imx6qdl-clock.h>
>> > +    #include <dt-bindings/gpio/gpio.h>
>> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> > +
>> > +    dsi: dsi@21e0000 {
>> > +        #address-cells = <1>;
>> > +        #size-cells = <0>;
>> > +        compatible = "fsl,imx6q-mipi-dsi", "snps,dw-mipi-dsi";
>> > +        reg = <0x021e0000 0x4000>;
>> > +        interrupts = <0 102 IRQ_TYPE_LEVEL_HIGH>;
>> > +        fsl,gpr = <&gpr>;
>> > +        clocks = <&clks IMX6QDL_CLK_MIPI_CORE_CFG>,
>> > +                 <&clks IMX6QDL_CLK_MIPI_IPG>;
>> > +        clock-names = "ref", "pclk";
>> > +
>> > +        ports {
>> > +            #address-cells = <1>;
>> > +            #size-cells = <0>;
>> 
>> port@0 is missing.
>> 
>> > +            port@1 {
>> > +                reg = <1>;
>> > +                dsi_out: endpoint {
>> > +                    remote-endpoint = <&panel_in>;
>> > +                };
>> > +            };
>> > +        };
>> > +
>> > +        panel@0 {
>> > +            compatible = "sharp,ls032b3sx01";
>> > +            reg = <0>;
>> > +            reset-gpios = <&gpio6 8 GPIO_ACTIVE_LOW>;
>> > +            ports {
>> > +                #address-cells = <1>;
>> > +                #size-cells = <0>;
>> > +                port@0 {
>> > +                    reg = <0>;
>> > +                    panel_in: endpoint {
>> > +                        remote-endpoint = <&dsi_out>;
>> > +                    };
>> > +                };
>> > +            };
>> > +        };
>> > +    };
>> > +
>> > +...
>
> -- 
> Regards,
>
> Laurent Pinchart