[1/6] media: dt-bindings: media: camss: Add qcom,sc7180-camss binding

Message ID 20240621-b4-sc7180-camss-v1-1-14937929f30e@gmail.com (mailing list archive)
State Superseded
Headers
Series media: qcom: camss: Add sc7180 support |

Commit Message

George Chan via B4 Relay June 21, 2024, 9:40 a.m. UTC
  From: George Chan <gchan9527@gmail.com>

Add bindings for qcom,sc7180-camss in order to support the camera
subsystem for sm7125 as found in the Xiaomi Redmi 9 Pro cellphone.

Signed-off-by: George Chan <gchan9527@gmail.com>
---
 .../bindings/media/qcom,sc7180-camss.yaml          | 324 +++++++++++++++++++++
 1 file changed, 324 insertions(+)
  

Comments

Krzysztof Kozlowski June 21, 2024, 10:02 a.m. UTC | #1
On 21/06/2024 11:40, George Chan via B4 Relay wrote:
> From: George Chan <gchan9527@gmail.com>
> 
> Add bindings for qcom,sc7180-camss in order to support the camera
> subsystem for sm7125 as found in the Xiaomi Redmi 9 Pro cellphone.
> 
> Signed-off-by: George Chan <gchan9527@gmail.com>

Subject: just one media (first). No need to write media: media: ...


A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


> ---
>  .../bindings/media/qcom,sc7180-camss.yaml          | 324 +++++++++++++++++++++
>  1 file changed, 324 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml
> new file mode 100644
> index 000000000000..4dc10c32ee9c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml
> @@ -0,0 +1,324 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm CAMSS ISP

What is CAMSS?

> +
> +maintainers:
> +  - Robert Foss <robert.foss@linaro.org>

For sure this is not true. Robert does not work in Linaro and I doubt he
cares that much about camss.

> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms
> +
> +properties:
> +  compatible:
> +    const: qcom,sc7180-camss
> +
> +  clocks:
> +    minItems: 25

Drop minItems

> +    maxItems: 25
> +
> +  clock-names:
> +    items:
> +      - const: camnoc_axi
> +      - const: cpas_ahb
> +      - const: cphy_rx_src
> +      - const: csi0
> +      - const: csi1
> +      - const: csi2
> +      - const: csiphy0
> +      - const: csiphy0_timer
> +      - const: csiphy1
> +      - const: csiphy1_timer
> +      - const: csiphy2
> +      - const: csiphy2_timer
> +      - const: csiphy3
> +      - const: csiphy3_timer
> +      - const: gcc_camera_ahb
> +      - const: gcc_camera_axi
> +      - const: soc_ahb
> +      - const: vfe0_axi
> +      - const: vfe0
> +      - const: vfe0_cphy_rx
> +      - const: vfe1_axi
> +      - const: vfe1
> +      - const: vfe1_cphy_rx
> +      - const: vfe_lite
> +      - const: vfe_lite_cphy_rx
> +
> +  interrupts:
> +    minItems: 10

Drop minItems

> +    maxItems: 10
> +
> +  interrupt-names:
> +    items:
> +      - const: csid0
> +      - const: csid1
> +      - const: csid2
> +      - const: csiphy0
> +      - const: csiphy1
> +      - const: csiphy2
> +      - const: csiphy3
> +      - const: vfe0
> +      - const: vfe1
> +      - const: vfe_lite
> +
> +  iommus:
> +    maxItems: 4
> +
> +  power-domains:
> +    items:
> +      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
> +      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
> +      - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller.
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    description:
> +      CSI input ports.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +            required:
> +              - data-lanes
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +            required:
> +              - data-lanes
> +
> +      port@2:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +            required:
> +              - data-lanes
> +
> +      port@3:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +            required:
> +              - data-lanes
> +
> +  reg:
> +    minItems: 10

Drop minItems

> +    maxItems: 10
> +
> +  reg-names:
> +    items:
> +      - const: csid0
> +      - const: csid1
> +      - const: csid2
> +      - const: csiphy0
> +      - const: csiphy1
> +      - const: csiphy2
> +      - const: csiphy3
> +      - const: vfe0
> +      - const: vfe1
> +      - const: vfe_lite
> +
> +  vdda-phy-supply:
> +    description:
> +      Phandle to a regulator supply to PHY core block.
> +
> +  vdda-pll-supply:
> +    description:
> +      Phandle to 1.8V regulator supply to PHY refclk pll block.
> +
> +required:
> +  - clock-names
> +  - clocks
> +  - compatible

Keep the list ordered, the same as list properties.

> +  - interrupt-names
> +  - interrupts
> +  - iommus
> +  - power-domains
> +  - reg
> +  - reg-names
> +  - vdda-phy-supply
> +  - vdda-pll-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,camcc-sc7180.h>
> +    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
> +
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      camss: camss@acb3000 {
> +        compatible = "qcom,sc7180-camss";
> +
> +        clocks = <&clock_camcc CAM_CC_CAMNOC_AXI_CLK>,
> +          <&clock_camcc CAM_CC_CPAS_AHB_CLK>,

Missed alignment with previous <.

> +          <&clock_camcc CAM_CC_IFE_0_CSID_CLK>,
> +          <&clock_camcc CAM_CC_IFE_1_CSID_CLK>,
> +          <&clock_camcc CAM_CC_IFE_LITE_CSID_CLK>,
> +          <&clock_camcc CAM_CC_CSIPHY0_CLK>,
> +          <&clock_camcc CAM_CC_CSI0PHYTIMER_CLK>,
> +          <&clock_camcc CAM_CC_CSIPHY1_CLK>,
> +          <&clock_camcc CAM_CC_CSI1PHYTIMER_CLK>,
> +          <&clock_camcc CAM_CC_CSIPHY2_CLK>,
> +          <&clock_camcc CAM_CC_CSI2PHYTIMER_CLK>,
> +          <&clock_camcc CAM_CC_CSIPHY3_CLK>,
> +          <&clock_camcc CAM_CC_CSI3PHYTIMER_CLK>,
> +          <&gcc GCC_CAMERA_AHB_CLK>,
> +          <&gcc GCC_CAMERA_HF_AXI_CLK>,
> +          <&clock_camcc CAM_CC_SOC_AHB_CLK>,
> +          <&clock_camcc CAM_CC_IFE_0_AXI_CLK>,
> +          <&clock_camcc CAM_CC_IFE_0_CLK>,
> +          <&clock_camcc CAM_CC_IFE_0_CPHY_RX_CLK>,
> +          <&clock_camcc CAM_CC_IFE_1_AXI_CLK>,
> +          <&clock_camcc CAM_CC_IFE_1_CLK>,
> +          <&clock_camcc CAM_CC_IFE_1_CPHY_RX_CLK>,
> +          <&clock_camcc CAM_CC_IFE_LITE_CLK>,
> +          <&clock_camcc CAM_CC_IFE_LITE_CPHY_RX_CLK>;
> +
> +        clock-names = "camnoc_axi",
> +          "cpas_ahb",

Same problem.

> +          "csi0",
> +          "csi1",
> +          "csi2",
> +          "csiphy0",
> +          "csiphy0_timer",
> +          "csiphy1",
> +          "csiphy1_timer",
> +          "csiphy2",
> +          "csiphy2_timer",
> +          "csiphy3",
> +          "csiphy3_timer",
> +          "gcc_camera_ahb",
> +          "gcc_camera_axi",
> +          "soc_ahb",
> +          "vfe0_axi",
> +          "vfe0",
> +          "vfe0_cphy_rx",
> +          "vfe1_axi",
> +          "vfe1",
> +          "vfe1_cphy_rx",
> +          "vfe_lite",
> +          "vfe_lite_cphy_rx";
> +
> +        interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> +          <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> +          <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> +          <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
> +          <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
> +          <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
> +          <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> +          <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> +          <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> +          <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;
> +
> +        interrupt-names = "csid0",
> +          "csid1",
> +          "csid2",
> +          "csiphy0",
> +          "csiphy1",
> +          "csiphy2",
> +          "csiphy3",
> +          "vfe0",
> +          "vfe1",
> +          "vfe_lite";
> +
> +        iommus = <&apps_smmu 0x820 0x0>,
> +          <&apps_smmu 0x840 0x0>,
> +          <&apps_smmu 0x860 0x0>;
> +
> +        power-domains = <&camcc IFE_0_GDSC>,
> +          <&camcc IFE_1_GDSC>,
> +          <&camcc TITAN_TOP_GDSC>;
> +
> +        reg = <0 0xacb3000 0 0x1000>,

reg is always the second property. See DTS coding style.

> +          <0 0xacba000 0 0x1000>,
> +          <0 0xacc8000 0 0x1000>,
> +          <0 0xac65000 0 0x1000>,
> +          <0 0xac66000 0 0x1000>,
> +          <0 0xac67000 0 0x1000>,
> +          <0 0xac68000 0 0x1000>,
> +          <0 0xacaf000 0 0x4000>,
> +          <0 0xacb6000 0 0x4000>,
> +          <0 0xacc4000 0 0x4000>;
> +
> +        reg-names = "csid0",

So this will be the third property.



Best regards,
Krzysztof
  
Rob Herring June 21, 2024, 10:29 a.m. UTC | #2
On Fri, 21 Jun 2024 17:40:53 +0800, George Chan wrote:
> Add bindings for qcom,sc7180-camss in order to support the camera
> subsystem for sm7125 as found in the Xiaomi Redmi 9 Pro cellphone.
> 
> Signed-off-by: George Chan <gchan9527@gmail.com>
> ---
>  .../bindings/media/qcom,sc7180-camss.yaml          | 324 +++++++++++++++++++++
>  1 file changed, 324 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clocks: [[4294967295, 6], [4294967295, 12], [4294967295, 37], [4294967295, 44], [4294967295, 50], [4294967295, 22], [4294967295, 14], [4294967295, 23], [4294967295, 16], [4294967295, 24], [4294967295, 18], [4294967295, 25], [4294967295, 20], [4294967295, 10], [4294967295, 11], [4294967295, 77], [4294967295, 33], [4294967295, 34], [4294967295, 36], [4294967295, 40], [4294967295, 41], [4294967295, 43], [4294967295, 47], [4294967295, 49]] is too short
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:2: 'cphy_rx_src' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:3: 'csi0' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:4: 'csi1' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:5: 'csi2' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:6: 'csiphy0' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:7: 'csiphy0_timer' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:8: 'csiphy1' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:9: 'csiphy1_timer' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:10: 'csiphy2' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:11: 'csiphy2_timer' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:12: 'csiphy3' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:13: 'csiphy3_timer' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:14: 'gcc_camera_ahb' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:15: 'gcc_camera_axi' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:16: 'soc_ahb' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:17: 'vfe0_axi' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:18: 'vfe0' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:19: 'vfe0_cphy_rx' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:20: 'vfe1_axi' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:21: 'vfe1' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:22: 'vfe1_cphy_rx' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names:23: 'vfe_lite' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: clock-names: ['camnoc_axi', 'cpas_ahb', 'csi0', 'csi1', 'csi2', 'csiphy0', 'csiphy0_timer', 'csiphy1', 'csiphy1_timer', 'csiphy2', 'csiphy2_timer', 'csiphy3', 'csiphy3_timer', 'gcc_camera_ahb', 'gcc_camera_axi', 'soc_ahb', 'vfe0_axi', 'vfe0', 'vfe0_cphy_rx', 'vfe1_axi', 'vfe1', 'vfe1_cphy_rx', 'vfe_lite', 'vfe_lite_cphy_rx'] is too short
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sc7180-camss.example.dtb: camss@acb3000: iommus: [[4294967295, 2080, 0], [4294967295, 2112, 0], [4294967295, 2144, 0]] is too short
	from schema $id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240621-b4-sc7180-camss-v1-1-14937929f30e@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
  
Bryan O'Donoghue June 21, 2024, 11:24 a.m. UTC | #3
On 21/06/2024 10:40, George Chan via B4 Relay wrote:
> +  power-domains:
> +    items:
> +      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
> +      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
> +      - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller.

Please name these power-domains and require them in your yaml, 
remembering to add them into the DTS.

See

Commit: d89751c61279 ("media: qcom: camss: Add support for named 
power-domains")

---
bod
  
george chan June 22, 2024, 3:24 p.m. UTC | #4
On Fri, Jun 21, 2024 at 6:02 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 21/06/2024 11:40, George Chan via B4 Relay wrote:
> > From: George Chan <gchan9527@gmail.com>
> >
> > Add bindings for qcom,sc7180-camss in order to support the camera
> > subsystem for sm7125 as found in the Xiaomi Redmi 9 Pro cellphone.
> >
> > Signed-off-by: George Chan <gchan9527@gmail.com>
>
> Subject: just one media (first). No need to write media: media: ...
>
>
> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

I found the cause of this, see if I can fix it next round.

> > +title: Qualcomm CAMSS ISP
>
> What is CAMSS?
>

No idea from an outsider, i can suggest one like "title: Qualcomm
Camera SubSystem"

> > +
> > +maintainers:
> > +  - Robert Foss <robert.foss@linaro.org>
>
> For sure this is not true. Robert does not work in Linaro and I doubt he
> cares that much about camss.
>
Well, i might suggest to be like below if no objection

 maintainers:
-  - Robert Foss <robert.foss@linaro.org>
+  - Bryan O'Donoghue <bryan.odonoghue@linaro.org>
...
> > +
> > +description: |
>
> Do not need '|' unless you need to preserve formatting.

How about this? I have no idea what this is, just modify it blindly below.
-description: |
+description:

Then drop all "minItems"
...
> > +required:
> > +  - clock-names
> > +  - clocks
> > +  - compatible
>
> Keep the list ordered, the same as list properties.

Sure for this "required" list
...
>
> Missed alignment with previous <.
>
Sure
...
> > +        reg = <0 0xacb3000 0 0x1000>,
>
> reg is always the second property. See DTS coding style.
>
...
> > +        reg-names = "csid0",
>
> So this will be the third property.
>
>
>
> Best regards,
> Krzysztof
>
Best regards,
George
  
george chan June 22, 2024, 3:31 p.m. UTC | #5
On Fri, Jun 21, 2024 at 7:24 PM Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 21/06/2024 10:40, George Chan via B4 Relay wrote:
> > +  power-domains:
> > +    items:
> > +      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
> > +      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
> > +      - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller.
>
> Please name these power-domains and require them in your yaml,
> remembering to add them into the DTS.

I can borrow the idea from sdm845-venus-v2

@@ -191,6 +190,7 @@ required:
   - interrupts
   - iommus
   - power-domains
+  - power-domain-names
   - reg
...
+        power-domain-names = "ife0",
+                             "ife1",
+                             "top";

I can confirm sc7180 will be the first one to have
"power-domain-names" property on camss node amongst SOCs.
>
> ---
> bod
  
Bryan O'Donoghue June 23, 2024, 11:10 a.m. UTC | #6
On 22/06/2024 16:31, george chan wrote:
> On Fri, Jun 21, 2024 at 7:24 PM Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> wrote:
>>
>> On 21/06/2024 10:40, George Chan via B4 Relay wrote:
>>> +  power-domains:
>>> +    items:
>>> +      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
>>> +      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
>>> +      - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller.
>>
>> Please name these power-domains and require them in your yaml,
>> remembering to add them into the DTS.
> 
> I can borrow the idea from sdm845-venus-v2
> 
> @@ -191,6 +190,7 @@ required:
>     - interrupts
>     - iommus
>     - power-domains
> +  - power-domain-names
>     - reg
> ...
> +        power-domain-names = "ife0",
> +                             "ife1",
> +                             "top";

Something like this

https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/7626cd2a2a509832c214e538827b91c5dbf1bf09

---
bod
  
george chan June 23, 2024, 9:45 p.m. UTC | #7
On Sun, Jun 23, 2024 at 7:10 PM Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> Something like this
>
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/7626cd2a2a509832c214e538827b91c5dbf1bf09
>
Nice, see if sc7180 is the first-runner-up patch hitting the mainline.
  

Patch

diff --git a/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml
new file mode 100644
index 000000000000..4dc10c32ee9c
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml
@@ -0,0 +1,324 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm CAMSS ISP
+
+maintainers:
+  - Robert Foss <robert.foss@linaro.org>
+
+description: |
+  The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms
+
+properties:
+  compatible:
+    const: qcom,sc7180-camss
+
+  clocks:
+    minItems: 25
+    maxItems: 25
+
+  clock-names:
+    items:
+      - const: camnoc_axi
+      - const: cpas_ahb
+      - const: cphy_rx_src
+      - const: csi0
+      - const: csi1
+      - const: csi2
+      - const: csiphy0
+      - const: csiphy0_timer
+      - const: csiphy1
+      - const: csiphy1_timer
+      - const: csiphy2
+      - const: csiphy2_timer
+      - const: csiphy3
+      - const: csiphy3_timer
+      - const: gcc_camera_ahb
+      - const: gcc_camera_axi
+      - const: soc_ahb
+      - const: vfe0_axi
+      - const: vfe0
+      - const: vfe0_cphy_rx
+      - const: vfe1_axi
+      - const: vfe1
+      - const: vfe1_cphy_rx
+      - const: vfe_lite
+      - const: vfe_lite_cphy_rx
+
+  interrupts:
+    minItems: 10
+    maxItems: 10
+
+  interrupt-names:
+    items:
+      - const: csid0
+      - const: csid1
+      - const: csid2
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: csiphy3
+      - const: vfe0
+      - const: vfe1
+      - const: vfe_lite
+
+  iommus:
+    maxItems: 4
+
+  power-domains:
+    items:
+      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
+      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
+      - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller.
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    description:
+      CSI input ports.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - data-lanes
+
+      port@1:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - data-lanes
+
+      port@2:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - data-lanes
+
+      port@3:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - data-lanes
+
+  reg:
+    minItems: 10
+    maxItems: 10
+
+  reg-names:
+    items:
+      - const: csid0
+      - const: csid1
+      - const: csid2
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: csiphy3
+      - const: vfe0
+      - const: vfe1
+      - const: vfe_lite
+
+  vdda-phy-supply:
+    description:
+      Phandle to a regulator supply to PHY core block.
+
+  vdda-pll-supply:
+    description:
+      Phandle to 1.8V regulator supply to PHY refclk pll block.
+
+required:
+  - clock-names
+  - clocks
+  - compatible
+  - interrupt-names
+  - interrupts
+  - iommus
+  - power-domains
+  - reg
+  - reg-names
+  - vdda-phy-supply
+  - vdda-pll-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,camcc-sc7180.h>
+    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      camss: camss@acb3000 {
+        compatible = "qcom,sc7180-camss";
+
+        clocks = <&clock_camcc CAM_CC_CAMNOC_AXI_CLK>,
+          <&clock_camcc CAM_CC_CPAS_AHB_CLK>,
+          <&clock_camcc CAM_CC_IFE_0_CSID_CLK>,
+          <&clock_camcc CAM_CC_IFE_1_CSID_CLK>,
+          <&clock_camcc CAM_CC_IFE_LITE_CSID_CLK>,
+          <&clock_camcc CAM_CC_CSIPHY0_CLK>,
+          <&clock_camcc CAM_CC_CSI0PHYTIMER_CLK>,
+          <&clock_camcc CAM_CC_CSIPHY1_CLK>,
+          <&clock_camcc CAM_CC_CSI1PHYTIMER_CLK>,
+          <&clock_camcc CAM_CC_CSIPHY2_CLK>,
+          <&clock_camcc CAM_CC_CSI2PHYTIMER_CLK>,
+          <&clock_camcc CAM_CC_CSIPHY3_CLK>,
+          <&clock_camcc CAM_CC_CSI3PHYTIMER_CLK>,
+          <&gcc GCC_CAMERA_AHB_CLK>,
+          <&gcc GCC_CAMERA_HF_AXI_CLK>,
+          <&clock_camcc CAM_CC_SOC_AHB_CLK>,
+          <&clock_camcc CAM_CC_IFE_0_AXI_CLK>,
+          <&clock_camcc CAM_CC_IFE_0_CLK>,
+          <&clock_camcc CAM_CC_IFE_0_CPHY_RX_CLK>,
+          <&clock_camcc CAM_CC_IFE_1_AXI_CLK>,
+          <&clock_camcc CAM_CC_IFE_1_CLK>,
+          <&clock_camcc CAM_CC_IFE_1_CPHY_RX_CLK>,
+          <&clock_camcc CAM_CC_IFE_LITE_CLK>,
+          <&clock_camcc CAM_CC_IFE_LITE_CPHY_RX_CLK>;
+
+        clock-names = "camnoc_axi",
+          "cpas_ahb",
+          "csi0",
+          "csi1",
+          "csi2",
+          "csiphy0",
+          "csiphy0_timer",
+          "csiphy1",
+          "csiphy1_timer",
+          "csiphy2",
+          "csiphy2_timer",
+          "csiphy3",
+          "csiphy3_timer",
+          "gcc_camera_ahb",
+          "gcc_camera_axi",
+          "soc_ahb",
+          "vfe0_axi",
+          "vfe0",
+          "vfe0_cphy_rx",
+          "vfe1_axi",
+          "vfe1",
+          "vfe1_cphy_rx",
+          "vfe_lite",
+          "vfe_lite_cphy_rx";
+
+        interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
+          <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
+          <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
+          <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
+          <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
+          <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
+          <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
+          <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
+          <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
+          <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;
+
+        interrupt-names = "csid0",
+          "csid1",
+          "csid2",
+          "csiphy0",
+          "csiphy1",
+          "csiphy2",
+          "csiphy3",
+          "vfe0",
+          "vfe1",
+          "vfe_lite";
+
+        iommus = <&apps_smmu 0x820 0x0>,
+          <&apps_smmu 0x840 0x0>,
+          <&apps_smmu 0x860 0x0>;
+
+        power-domains = <&camcc IFE_0_GDSC>,
+          <&camcc IFE_1_GDSC>,
+          <&camcc TITAN_TOP_GDSC>;
+
+        reg = <0 0xacb3000 0 0x1000>,
+          <0 0xacba000 0 0x1000>,
+          <0 0xacc8000 0 0x1000>,
+          <0 0xac65000 0 0x1000>,
+          <0 0xac66000 0 0x1000>,
+          <0 0xac67000 0 0x1000>,
+          <0 0xac68000 0 0x1000>,
+          <0 0xacaf000 0 0x4000>,
+          <0 0xacb6000 0 0x4000>,
+          <0 0xacc4000 0 0x4000>;
+
+        reg-names = "csid0",
+          "csid1",
+          "csid2",
+          "csiphy0",
+          "csiphy1",
+          "csiphy2",
+          "csiphy3",
+          "vfe0",
+          "vfe1",
+          "vfe_lite";
+
+        vdda-phy-supply = <&vreg_l1a_0p875>;
+        vdda-pll-supply = <&vreg_l26a_1p2>;
+
+        ports {
+          #address-cells = <1>;
+          #size-cells = <0>;
+        };
+      };
+    };