lkml.org 
[lkml]   [2022]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/4] dt-bindings: cpufreq: mediatek: transform cpufreq-mediatek into yaml
From
Date
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

\
 
 \ /
  Last update: 2022-03-24 10:39    [W:0.126 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site