Re: [PATCH v2 2/6] media: dt-bindings: cadence-csi2rx: Convert to DT schema

From: Jack Zhu
Date: Mon Mar 20 2023 - 04:08:59 EST



On 2023/3/12 19:09, Laurent Pinchart wrote:
> Hi Jack,
>
> Thank you for the patch.
>
> On Fri, Mar 10, 2023 at 08:05:49PM +0800, Jack Zhu wrote:
>> Convert DT bindings document for Cadence MIPI-CSI2 RX controller
>> to DT schema format and add new properties.
>
> This would have been easier to review if the patch had been split in
> two, with conversion to YAML first, and then addition of new properties.
> Generally speaking, one patch should contain a single logical change.
>

OK, will split the patch into two patches.

>> Signed-off-by: Jack Zhu <jack.zhu@xxxxxxxxxxxxxxxx>
>> ---
>> .../devicetree/bindings/media/cdns,csi2rx.txt | 100 -----------
>> .../bindings/media/cdns,csi2rx.yaml | 163 ++++++++++++++++++
>> MAINTAINERS | 1 +
>> 3 files changed, 164 insertions(+), 100 deletions(-)
>> delete mode 100644 Documentation/devicetree/bindings/media/cdns,csi2rx.txt
>> create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.txt b/Documentation/devicetree/bindings/media/cdns,csi2rx.txt
>> deleted file mode 100644
>> index 6b02a0657ad9..000000000000
>> --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.txt
>> +++ /dev/null
>> @@ -1,100 +0,0 @@
>> -Cadence MIPI-CSI2 RX controller
>> -===============================
>> -
>> -The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting up to 4 CSI
>> -lanes in input, and 4 different pixel streams in output.
>> -
>> -Required properties:
>> - - compatible: must be set to "cdns,csi2rx" and an SoC-specific compatible
>> - - reg: base address and size of the memory mapped region
>> - - clocks: phandles to the clocks driving the controller
>> - - clock-names: must contain:
>> - * sys_clk: main clock
>> - * p_clk: register bank clock
>> - * pixel_if[0-3]_clk: pixel stream output clock, one for each stream
>> - implemented in hardware, between 0 and 3
>> -
>> -Optional properties:
>> - - phys: phandle to the external D-PHY, phy-names must be provided
>> - - phy-names: must contain "dphy", if the implementation uses an
>> - external D-PHY
>> -
>> -Required subnodes:
>> - - ports: A ports node with one port child node per device input and output
>> - port, in accordance with the video interface bindings defined in
>> - Documentation/devicetree/bindings/media/video-interfaces.txt. The
>> - port nodes are numbered as follows:
>> -
>> - Port Description
>> - -----------------------------
>> - 0 CSI-2 input
>> - 1 Stream 0 output
>> - 2 Stream 1 output
>> - 3 Stream 2 output
>> - 4 Stream 3 output
>> -
>> - The stream output port nodes are optional if they are not
>> - connected to anything at the hardware level or implemented
>> - in the design.Since there is only one endpoint per port,
>> - the endpoints are not numbered.
>> -
>> -
>> -Example:
>> -
>> -csi2rx: csi-bridge@0d060000 {
>> - compatible = "cdns,csi2rx";
>> - reg = <0x0d060000 0x1000>;
>> - clocks = <&byteclock>, <&byteclock>
>> - <&coreclock>, <&coreclock>,
>> - <&coreclock>, <&coreclock>;
>> - clock-names = "sys_clk", "p_clk",
>> - "pixel_if0_clk", "pixel_if1_clk",
>> - "pixel_if2_clk", "pixel_if3_clk";
>> -
>> - ports {
>> - #address-cells = <1>;
>> - #size-cells = <0>;
>> -
>> - port@0 {
>> - reg = <0>;
>> -
>> - csi2rx_in_sensor: endpoint {
>> - remote-endpoint = <&sensor_out_csi2rx>;
>> - clock-lanes = <0>;
>> - data-lanes = <1 2>;
>> - };
>> - };
>> -
>> - port@1 {
>> - reg = <1>;
>> -
>> - csi2rx_out_grabber0: endpoint {
>> - remote-endpoint = <&grabber0_in_csi2rx>;
>> - };
>> - };
>> -
>> - port@2 {
>> - reg = <2>;
>> -
>> - csi2rx_out_grabber1: endpoint {
>> - remote-endpoint = <&grabber1_in_csi2rx>;
>> - };
>> - };
>> -
>> - port@3 {
>> - reg = <3>;
>> -
>> - csi2rx_out_grabber2: endpoint {
>> - remote-endpoint = <&grabber2_in_csi2rx>;
>> - };
>> - };
>> -
>> - port@4 {
>> - reg = <4>;
>> -
>> - csi2rx_out_grabber3: endpoint {
>> - remote-endpoint = <&grabber3_in_csi2rx>;
>> - };
>> - };
>> - };
>> -};
>> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>> new file mode 100644
>> index 000000000000..ed573a67f93e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>> @@ -0,0 +1,163 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/cdns,csi2rx.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Cadence MIPI-CSI2 RX controller
>> +
>> +maintainers:
>> + - Maxime Ripard <mripard@xxxxxxxxxx>
>> +
>> +description:
>> + The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting up to 4 CSI
>> + lanes in input, and 4 different pixel streams in output.
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - cdns,csi2rx
>
> The existing bindings state
>
> - compatible: must be set to "cdns,csi2rx" and an SoC-specific compatible
>
> This should thus be
>
> compatible:
> items:
> - enum:
> - vendor1,device1
> - ...
> - const: cdns,csi2rx
>
> The trouble is that the existing bindings are not used in mainline and
> don't specify any SoC-specific compatible string, so I don't know what
> to indicate for vendor1,device1. One option would be to add the StarFive
> compatible string already:
>
> compatible:
> items:
> - enum:
> - starfive,jh7110-csi2rx
> - const: cdns,csi2rx
>
> The example below should be updated accordingly.
>

Will alter it according your opinion. Thanks.

>> +
>> + reg:
>> + maxItems: 1
>> +
>> + clocks:
>> + items:
>> + - description: CSI2Rx system clock
>> + - description: Gated Register bank clock for APB interface
>> + - description: pixel Clock for Stream interface 0
>> + - description: pixel Clock for Stream interface 1
>> + - description: pixel Clock for Stream interface 2
>> + - description: pixel Clock for Stream interface 3
>> +
>> + clock-names:
>> + items:
>> + - const: sys
>> + - const: reg_bank
>> + - const: pixel_if0
>> + - const: pixel_if1
>> + - const: pixel_if2
>> + - const: pixel_if3
>
> This changes the clock names and breaks compatibility with the driver.
> The existing names must be preserved.
>

OK, will use the existing names.

>> +
>> + resets:
>> + items:
>> + - description: CSI2Rx system reset
>> + - description: Gated Register bank reset for APB interface
>> + - description: pixel reset for Stream interface 0
>> + - description: pixel reset for Stream interface 1
>> + - description: pixel reset for Stream interface 2
>> + - description: pixel reset for Stream interface 3
>> +
>> + reset-names:
>> + items:
>> + - const: sys
>> + - const: reg_bank
>> + - const: pixel_if0
>> + - const: pixel_if1
>> + - const: pixel_if2
>> + - const: pixel_if3
>
> Let's move the addition of the resets and reset-names properties to a
> patch separate from the YAML conversion to make it easier to review them
> independently.

OK, will do. Thank.

>
>> +
>> + phys:
>> + maxItems: 1
>> + description: MIPI D-PHY
>> +
>> + phy-names:
>> + items:
>> + - const: dphy
>> +
>> + ports:
>> + $ref: /schemas/graph.yaml#/properties/ports
>> +
>> + properties:
>> + port@0:
>> + $ref: /schemas/graph.yaml#/$defs/port-base
>> + unevaluatedProperties: false
>> + description:
>> + Input port node, single endpoint describing the CSI-2 transmitter.
>> +
>> + properties:
>> + endpoint:
>> + $ref: video-interfaces.yaml#
>> + unevaluatedProperties: false
>> +
>> + properties:
>> + bus-type:
>> + enum:
>> + - 4
>
> You can simplify this to
>
> bus-type:
> const: 4
>

OK, will fix

>> +
>> + clock-lanes:
>> + maximum: 4
>> +
>> + data-lanes:
>> + minItems: 1
>> + maxItems: 4
>> + items:
>> + maximum: 4
>
> Does the IP core support clock and data lanes remapping ?

Only data lanes remapping is supported.
Will change it to the below:

clock-lanes:
const: 0

>
>> +
>> + required:
>> + - clock-lanes
>> + - data-lanes
>> +
>> + port@1:
>> + $ref: /schemas/graph.yaml#/properties/port
>> + description:
>> + Output port node
>
> This is also a change compared to the existing bindings, and it will
> break backward compatibility. You should have four output ports.

OK, will fix.

>
>> +
>> + required:
>> + - port@0
>> + - port@1
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - clocks
>> + - clock-names
>> + - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + csi2rx: csi@0d060000 {
>
> The csi2rx label is never referenced, you can drop it.

OK, will drop it.

>
>> + compatible = "cdns,csi2rx";
>> + reg = <0x0d060000 0x1000>;
>> + clocks = <&byteclock 7>, <&byteclock 6>,
>> + <&coreclock 8>, <&coreclock 9>,
>> + <&coreclock 10>, <&coreclock 11>;
>> + clock-names = "sys", "reg_bank",
>> + "pixel_if0", "pixel_if1",
>> + "pixel_if2", "pixel_if3";
>> + resets = <&bytereset 9>, <&bytereset 4>,
>> + <&corereset 5>, <&corereset 6>,
>> + <&corereset 7>, <&corereset 8>;
>> + reset-names = "sys", "reg_bank",
>> + "pixel_if0", "pixel_if1",
>> + "pixel_if2", "pixel_if3";
>> + phys = <&csi_phy>;
>> + phy-names = "dphy";
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + port@0 {
>> + reg = <0>;
>> +
>> + csi2rx_in_sensor: endpoint {
>> + remote-endpoint = <&sensor_out_csi2rx>;
>> + clock-lanes = <0>;
>> + data-lanes = <1 2>;
>> + };
>> + };
>> +
>> + port@1 {
>> + reg = <1>;
>> +
>> + csi2rx_out_grabber0: endpoint {
>> + remote-endpoint = <&grabber0_in_csi2rx>;
>> + };
>> + };
>> + };
>> + };
>> +
>> +...
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8ddef8669efb..b2e7ca5603c3 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -4632,6 +4632,7 @@ M: Maxime Ripard <mripard@xxxxxxxxxx>
>> L: linux-media@xxxxxxxxxxxxxxx
>> S: Maintained
>> F: Documentation/devicetree/bindings/media/cdns,*.txt
>> +F: Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>> F: drivers/media/platform/cadence/cdns-csi2*
>>
>> CADENCE NAND DRIVER
>