Re: [PATCH 1/2] dt-bindings: display: add CIX Sky1 DisplayPort transmitter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] dt-bindings: display: custom vendor properties duplicate
  standard DT bindings
- [Medium] dt-bindings: display: incomplete OF graph ports and missing
  required phys/port
--

commit 7968e7b18915fdc94006fbc915ec20e0e3171502
Author: Luo Liu <Luo.Liu@cixtech.com>

dt-bindings: display: add CIX Sky1 DisplayPort transmitter

This commit adds the Device Tree binding schema for the CIX Sky1 Trilinear
DisplayPort transmitter controller and adds a corresponding MAINTAINERS entry.

> diff --git a/Documentation/devicetree/bindings/display/cix,sky1-dptx.yaml b/Documentation/devicetree/bindings/display/cix,sky1-dptx.yaml
> new file mode 100644
> index 0000000000000..9087b5edd9f02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/cix,sky1-dptx.yaml
> @@ -0,0 +1,108 @@

[ ... ]

> +  phys:
> +    maxItems: 1
> +    description: DisplayPort PHY the controller transmits through.
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/properties/port
> +    description:
> +      Video input from the display pipeline that drives this controller.
> +
> +  cix,adapter-port:
> +    description:
> +      RCSU video adapter port of this transmitter. Sky1 numbers its five
> +      transmitters from 1 to 5. Port 3 is the native DP/eDP output.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 5
> +
> +  cix,dp-max-rate:
> +    description:
> +      Maximum link rate the board is wired for, in units of 10 kbit/s.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [162000, 270000, 540000, 810000]
> +    default: 810000
> +
> +  cix,stream-id:
> +    description:
> +      Index of the video stream this controller uses. Sky1 has two streams,
> +      numbered 0 and 1.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 1
> +    default: 0

[Severity: Medium]
Can standard Devicetree mechanisms replace cix,adapter-port, cix,dp-max-rate,
and cix,stream-id instead of introducing custom vendor properties?

cix,adapter-port encodes a 1-to-5 instance index where port 3 is the native
DP/eDP output with a different RCSU register offset and bit layout from the
USB-C DP transmitters; per writing-bindings.rst, differing programming models
should be distinguished by compatible strings rather than an instance index
property.

cix,dp-max-rate exposes Linux DRM internal software units of 10 kbit/s
without a standard unit suffix, duplicating the standard link-frequencies
property (in Hz) from video-interfaces.yaml on the output graph endpoint.

cix,stream-id hardcodes the input video stream index as a vendor property
instead of modeling the display pipeline to transmitter routing through OF
graph ports and endpoints.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - resets
> +  - cix,adapter-port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    dp0: dp@14064000 {
> +        compatible = "cix,sky1-dptx";
> +        reg = <0x14064000 0x4000>,
> +              <0x14050000 0x400>;
> +        reg-names = "dp", "rcsu";
> +        interrupts = <GIC_SPI 332 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&clk 0>, <&clk 1>, <&clk 2>;
> +        clock-names = "apb", "vid0", "vid1";
> +        resets = <&rst 0>;
> +        cix,adapter-port = <1>;
> +    };

[Severity: Medium]
Should the binding define an output graph port and list phys and the OF graph
ports as required?

The schema only defines a single input port from the display pipeline and
omits the output port connecting this bridge to the downstream sink (such as
a dp-connector, usb-c-connector, or eDP panel), which prevents describing
board-level endpoint properties such as data-lanes and link-frequencies.

In addition, each transmitter sits between a display pipeline and a
DisplayPort PHY (and the driver unconditionally requires phys during probe),
so phys and the OF graph ports should be listed under required and included in
the example.  The unreferenced dp0 label in the example should also be
dropped per writing-schema.rst.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109092623.102291-1-Luo.Liu@cixtech.com?part=1