Re: [PATCH v3 01/29] dt-bindings: media: Add sm8550 dt schema

[Krzysztof Kozlowski <krzk@kernel.org>]

On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote:
> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
> 
> Add a schema description for the iris video encoder/decoder
> on sm8550.

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

And subject is neither correct nor complete. You did not add here SM8550
SoC, but SM8550 Iris. Plus what is SM8550? TI SM8550? Samsung SM8550?

You have entire commit subject to say briefly such details without
repeating obvious "dt schema".

> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
>  .../bindings/media/qcom,sm8550-iris.yaml           | 162 +++++++++++++++++++++
>  1 file changed, 162 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> new file mode 100644
> index 000000000000..a7aa6a6190cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> @@ -0,0 +1,162 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/qcom,sm8550-iris.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm IRIS video encode and decode accelerators

IRIS or Iris? Why it is every time written differently?

If IRIS then explain the acronym in description.

> +
> +maintainers:
> +  - Vikash Garodia <quic_vgarodia@quicinc.com>
> +  - Dikshita Agarwal <quic_dikshita@quicinc.com>
> +
> +description: |

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

> +  The Iris video processing unit is a video encode and decode accelerator
> +  present on Qualcomm platforms.
> +
> +allOf:
> +  - $ref: qcom,venus-common.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:

Drop oneOf

> +      - enum:
> +          - qcom,sm8550-iris
> +
> +  power-domains:
> +    maxItems: 4
> +
> +  power-domain-names:
> +    oneOf:

Drop oneOf

> +      - items:
> +          - const: venus
> +          - const: vcodec0
> +          - const: mxc
> +          - const: mmcx
> +
> +  clocks:
> +    maxItems: 3
> +
> +  clock-names:
> +    items:
> +      - const: iface
> +      - const: core
> +      - const: vcodec0_core
> +
> +  interconnects:
> +    maxItems: 2
> +
> +  interconnect-names:
> +    items:
> +      - const: cpu-cfg
> +      - const: video-mem
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    items:
> +      - const: bus
> +
> +  iommus:
> +    maxItems: 2
> +
> +  dma-coherent: true
> +
> +  operating-points-v2: true
> +
> +  opp-table:
> +    type: object
> +
> +required:
> +  - compatible
> +  - power-domain-names
> +  - interconnects
> +  - interconnect-names
> +  - resets
> +  - reset-names
> +  - iommus
> +  - dma-coherent
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,rpmh.h>
> +    #include <dt-bindings/clock/qcom,sm8550-gcc.h>
> +    #include <dt-bindings/clock/qcom,sm8450-videocc.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interconnect/qcom,icc.h>
> +    #include <dt-bindings/interconnect/qcom,sm8550-rpmh.h>
> +    #include <dt-bindings/power/qcom-rpmpd.h>
> +    #include <dt-bindings/power/qcom,rpmhpd.h>
> +
> +    iris: video-codec@aa00000 {

Drop unused label

> +        compatible = "qcom,sm8550-iris";
> +

No blank line here

> +        reg = <0x0aa00000 0xf0000>;
> +        interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
> +
> +        power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>,
> +                        <&videocc VIDEO_CC_MVS0_GDSC>,
> +                        <&rpmhpd RPMHPD_MXC>,
> +                        <&rpmhpd RPMHPD_MMCX>;
> +        power-domain-names = "venus", "vcodec0", "mxc", "mmcx";
> +
> +        clocks = <&gcc GCC_VIDEO_AXI0_CLK>,
> +                 <&videocc VIDEO_CC_MVS0C_CLK>,
> +                 <&videocc VIDEO_CC_MVS0_CLK>;
> +        clock-names = "iface", "core", "vcodec0_core";
> +
> +        interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> +                         &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ALWAYS>,
> +                        <&mmss_noc MASTER_VIDEO QCOM_ICC_TAG_ALWAYS
> +                         &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>;
> +        interconnect-names = "cpu-cfg", "video-mem";
> +
> +        memory-region = <&video_mem>;
> +
> +        resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
> +        reset-names = "bus";
> +
> +        iommus = <&apps_smmu 0x1940 0x0000>,
> +                 <&apps_smmu 0x1947 0x0000>;
> +        dma-coherent;
> +
> +        operating-points-v2 = <&iris_opp_table>;
> +
> +        iris_opp_table: opp-table {
> +            compatible = "operating-points-v2";
> +
> +            opp-240000000 {
> +                opp-hz = /bits/ 64 <240000000>;
> +                required-opps = <&rpmhpd_opp_svs>,
> +                                <&rpmhpd_opp_low_svs>;
> +            };
> +
> +            opp-338000000 {
> +                opp-hz = /bits/ 64 <338000000>;
> +                required-opps = <&rpmhpd_opp_svs>,
> +                                <&rpmhpd_opp_svs>;
> +            };
> +
> +            opp-366000000 {
> +                opp-hz = /bits/ 64 <366000000>;
> +                required-opps = <&rpmhpd_opp_svs_l1>,
> +                                <&rpmhpd_opp_svs_l1>;
> +            };
> +
> +            opp-444000000 {
> +                opp-hz = /bits/ 64 <444000000>;
> +                required-opps = <&rpmhpd_opp_turbo>,
> +                                <&rpmhpd_opp_turbo>;
> +            };
> +
> +            opp-533333334 {
> +                opp-hz = /bits/ 64 <533333334>;
> +                required-opps = <&rpmhpd_opp_turbo_l1>,
> +                                <&rpmhpd_opp_turbo_l1>;
> +           };

Fix the indentation above.

> +        };
> +    };
> +...
> 

Best regards,
Krzysztof