Re: [PATCH 1/4] dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml

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

On 07/03/2022 13:21, Tim Chang wrote:
> convert Mediatek cpufreq devicetree binding to YAML.

Start with capital letter please.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
> ---
>  .../bindings/cpufreq/cpufreq-mediatek.yaml    | 131 ++++++++++++++++++
>  1 file changed, 131 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml

You wrote "convert" but where is the removal of TXT?

> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> new file mode 100644
> index 000000000000..584946eb3790
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.yaml
> @@ -0,0 +1,131 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/cpufreq/cpufreq-mediatek.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek CPUFREQ driver Device Tree Bindings

Please remove "driver Device Tree Bindings" because the title should
describe the hardware. Therefore it could be something like "Mediatek
SoC CPU frequency and voltage scaling".

How is it related to cpufreq-mediatek-hw.yaml? The names/title look
unfortunately too similar.

In general this does not look like proper bindings (see also below lack
of compatible). Bindings describe the hardware, so what is exactly the
hardware here?

> +
> +maintainers:
> +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> +
> +description: |
> +  CPUFREQ is used for scaling clock frequency of CPUs.
> +  The module cooperates with CCI DEVFREQ to manage frequency for some Mediatek
> +  SoCs.
> +
> +properties:

How is this schema going to be applied? I don't see here select neither
compatible.

> +  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: "cpu"
> +      - const: "intermediate"
> +
> +  operating-points-v2:
> +    description:
> +      For details, please refer to
> +      Documentation/devicetree/bindings/opp/opp-v2.yaml
> +
> +  opp-table: true

You have operating-points-v2. What is this for? Did it exist in original
bindings?

> +
> +  proc-supply:
> +    description:
> +      Phandle of the regulator for CPU cluster that provides the supply
> +      voltage.
> +
> +  sram-supply:
> +    description:
> +      Phandle of the regulator for sram of CPU cluster that provides the supply
> +      voltage. When present, the cpufreq 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.
> +
> +  "#cooling-cells":
> +    description:
> +      For details, please refer to
> +      Documentation/devicetree/bindings/thermal/thermal-cooling-devices.yaml

Skip description, it's obvious. Instead add here const with value.


Best regards,
Krzysztof