Re: [PATCH 01/18] dt-bindings: opp: opp-v2-kryo-cpu: support Qualcomm Krait SoCs

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 14/06/2023 19:01, Krzysztof Kozlowski wrote:
> On 12/06/2023 07:39, Dmitry Baryshkov wrote:
>> Exted the opp-v2-kryo-cpu.yaml to support defining OPP tables for the
>> previous generation of Qualcomm CPUs, 32-bit Krait-based platforms.
>>
>> It makes no sense to use 'operating-points-v2-kryo-cpu' compatibility
>> node for the Krait cores. Add support for the Krait-specific
>> 'operating-points-v2-krait-cpu' compatibility string and the relevant
>> opp-microvolt subclasses properties.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   .../devicetree/bindings/opp/opp-v2-kryo-cpu.yaml      | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml b/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
>> index bbbad31ae4ca..93ec778bf333 100644
>> --- a/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
>> @@ -26,7 +26,9 @@ description: |
>>   
>>   properties:
>>     compatible:
>> -    const: operating-points-v2-kryo-cpu
>> +    enum:
>> +      - operating-points-v2-krait-cpu
>> +      - operating-points-v2-kryo-cpu
>>   
>>     nvmem-cells:
>>       description: |
>> @@ -63,14 +65,15 @@ patternProperties:
>>             5:  MSM8996SG, speedbin 1
>>             6:  MSM8996SG, speedbin 2
>>             7-31:  unused
>> -        enum: [0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7,
>> -               0x9, 0xd, 0xe, 0xf,
>> -               0x10, 0x20, 0x30, 0x70]
> 
> Why?
> 
>> +        $ref: /schemas/types.yaml#/definitions/uint32
> 
> You are changing the type. No. It should be fixed instead (enum applies
> to items).

Currenlty this bindings are only usable for msm8996/msm8996pro. As such 
we listed opp-supported-hw values that are applicable to this platform. 
This series adds support for apq8064 platform, which will add new items 
to this enum. I think it is not very sensible to list all of them here.

However granted there is already a good enough base type definition, I 
think it would be better to drop the $ref, drop the enum, add ': true' 
(is it necessary if we have a description already?) and expand 
documentation.

> 
>>   
>>         clock-latency-ns: true
>>   
>>         required-opps: true
>>   
>> +    patternProperties:
>> +      '^opp-microvolt-speed[0-9]+-pvs[0-9]+$': true
> 
> I don't think it is a common property, so it needs description and
> specific type. Specifically "pvs[0-9]" something entirely new.

Ack.

> 
> 
> 
> Best regards,
> Krzysztof
> 

-- 
With best wishes
Dmitry