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

[Jia-Wei Chang <jia-wei.chang@mediatek.com>]

Dear Krzysztof,

Thanks for your comments.
Pardon me for not being familiar with upstream rules and my late reply.
I was supposed to have an internal review before submission and I will.

On Mon, 2022-03-07 at 19:57 +0100, Krzysztof Kozlowski wrote:
> On 07/03/2022 13:21, Tim Chang wrote:
> > convert Mediatek cpufreq devicetree binding to YAML.
> 
> Start with capital letter please.

Sure, I will update it in the next version.

> > 
> > 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?

Sorry, I missed it and I will fix it in the next version.

> 
> > 
> > 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: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/cpufreq/cpufreq-mediatek.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2NdpChkMA$
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!xbKG4TgD0MRpMLyGJVBZEGpZFrNOclrcxOCx_APKo5Nmg8nF2x5PcBdE0unvL2O8T_oxCQ$
> >  
> > +
> > +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".

Thanks for your suggestion of title.
Or should I use the origin title "Binding for MediaTek's CPUFreq
driver"?

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

No, mediatek-cpufreq is performing in kernel driver rather than on
hardware.
On the other hand, mediatek-cpufreq-hw is performing on hardware.
That's why "hw" is present in its name.

> 
> 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?

Except for SoC, there's no requirement of hardware binding for
mediatek-cpufreq.
mediatek-cpufreq recognizes the compatible of Mediatek SoC while
probing.

> 
> > +
> > +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.

As mentioned above, only compatible of SoC is required for mediatek-
cpufreq.

> 
> > +  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?

Yes, all of these properties exist in the original bindings.
operating-point-v2 is used to determine the voltage and frequency of
dvfs which is further utilized by mediatek-cpufreq here.

> 
> > +
> > +  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.

Sure, I'll remove description and add const value in the next version.

> 
> 
> Best regards,
> Krzysztof