Re: [PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 07/03/2022 13:25, Tim Chang wrote:
> add devicetree binding of mtk cci devfreq on MediaTek SoC.

Start with capital letter.

> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>

This does not match your From. Please fix this in all your submissions.

> ---
>  .../devicetree/bindings/devfreq/mtk-cci.yaml  | 73 +++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> new file mode 100644
> index 000000000000..e64ac4c56758
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/devfreq/mtk-cci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Cache Coherent Interconnect (CCI) Devfreq driver Device Tree Bindings

Similarly to your other patches - the title describes hardware. Please
fix it in all your submissions of all your series.

Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so this
maybe "bus frequency scaling"? Although later you call the device node
as cci.

> +
> +maintainers:
> +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> +
> +description: |
> +  This module is used to create CCI DEVFREQ.
> +  The performance will depend on both CCI frequency and CPU frequency.
> +  For MT8186, CCI co-buck with Little core.
> +  Contain CCI opp table for voltage and frequency scaling.

Half of this description (first and last sentence) does not describe the
actual hardware. Please describe hardware, not driver.

> +
> +properties:
> +  compatible:
> +    const: "mediatek,mt8186-cci"

No need for quotes.

> +
> +  clocks:
> +    items:
> +      - description:
> +          The first one is the multiplexer for clock input of CPU cluster.
> +      - description:
> +          The other is used as an intermediate clock source when the original
> +          CPU is under transition and not stable yet.
> +
> +  clock-names:
> +    items:
> +      - const: "cci"
> +      - const: "intermediate"

No need for quotes.

> +
> +  operating-points-v2:
> +    description:
> +      For details, please refer to
> +      Documentation/devicetree/bindings/opp/opp-v2.yaml
> +
> +  opp-table: true

Same comments as your CPU freq bindings apply.

> +
> +  proc-supply:
> +    description:
> +      Phandle of the regulator for CCI that provides the supply voltage.
> +
> +  sram-supply:
> +    description:
> +      Phandle of the regulator for sram of CCI that provides the supply
> +      voltage. When present, the cci devfreq driver needs to do
> +      "voltage tracking" to step by step scale up/down Vproc and Vsram to fit
> +      SoC specific needs. When absent, the voltage scaling flow is handled by
> +      hardware, hence no software "voltage tracking" is needed.
> +
> +required:
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - operating-points-v2
> +  - proc-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mt8186-clk.h>
> +    cci: cci {

Node names should be generic and describe type of device. Are you sure
this is a CCI? Maybe "interconnect" suits it better?

> +      compatible = "mediatek,mt8186-cci";
> +      clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys CLK_APMIXED_MAINPLL>;
> +      clock-names = "cci", "intermediate";
> +      operating-points-v2 = <&cci_opp>;
> +      proc-supply = <&mt6358_vproc12_reg>;
> +      sram-supply = <&mt6358_vsram_proc12_reg>;
> +    };


Best regards,
Krzysztof