Re: [PATCH 13/13] dt-bindings: cpufreq: qcom-hw: Add bindings for 8998

From: Rob Herring
Date: Tue Dec 08 2020 - 13:12:03 EST


On Thu, Nov 26, 2020 at 07:45:59PM +0100, AngeloGioacchino Del Regno wrote:
> The OSM programming addition has been done under the
> qcom,cpufreq-hw-8998 compatible name: specify the requirement
> of two additional register spaces for this functionality.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxxx>
> ---
> .../bindings/cpufreq/qcom,cpufreq-hw.yaml | 31 ++++++++++++++++---
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml b/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
> index 94a56317b14b..f64cea73037e 100644
> --- a/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
> +++ b/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
> @@ -23,17 +23,21 @@ properties:
> - qcom,cpufreq-epss
>
> reg:
> + description: Base address and size of the RBCPR register region

That doesn't make sense given you have 2 regions.

> minItems: 2
> maxItems: 2

maxItems: 4

>
> reg-names:
> description:
> - Frequency domain register region for each domain.
> - items:
> - - const: "freq-domain0"
> - - const: "freq-domain1"
> + Frequency domain register region for each domain. If OSM programming
> + does not happen in the bootloader and has to be done in this driver,
> + then also the OSM domain region osm-domain[0-1] has to be provided.

Don't write free form text for what can be expressed as schema.

> + minItems: 2
> + maxItems: 2

You obviously haven't tried this change with 8998. It will fail with
more than 2. What you need here is:

minItems: 2
maxItems: 4

items:
- const: "freq-domain0"
- const: "freq-domain1"
- const: "osm-domain0"
- const: "osm-domain1"

And then...

>
> clock-names:
> + minItems: 2
> + maxItems: 2
> - const: xo
> - const: ref
>
> @@ -53,9 +57,28 @@ properties:
> property with phandle to a cpufreq_hw followed by the Domain ID(0/1)
> in the CPU DT node.
>
> +allOf:
> + - if:
> + properties:
> + reg-names:
> + contains:
> + const: qcom,cpufreq-hw-8998
> + then:
> + properties:
> + reg:
> + minItems: 4
> + maxItems: 4
> + reg-names:

...here just:

minItems: 4

And you'll need an 'else' clause with 'maxItems: 2' for reg and
reg-names.

> + items:
> + - const: "freq-domain0"
> + - const: "freq-domain1"
> + - const: "osm-domain0"
> + - const: "osm-domain1"
> +
> required:
> - compatible
> - reg
> + - reg-names

You can't make something that was optional now required. (Unless it was
a mistake and all existing users always had 'reg-names'.)

> - clock-names
> - clocks
> - "#freq-domain-cells"
> --
> 2.29.2
>