Re: [PATCH v10 2/6] dt-bindings: opp: v2-qcom-level: Document CPR3 open/closed loop volt adjustment

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 18.02.2023 00:13, Rob Herring wrote:
> On Fri, Feb 17, 2023 at 12:08:25PM +0100, Konrad Dybcio wrote:
>> CPR3 and newer can be fed per-OPP voltage adjustment values for both
>> open- and closed-loop paths to make better decisions about settling
>> on the final voltage offset target. Document these properties.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  .../devicetree/bindings/opp/opp-v2-qcom-level.yaml         | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-level.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-level.yaml
>> index a30ef93213c0..93cc88434dfe 100644
>> --- a/Documentation/devicetree/bindings/opp/opp-v2-qcom-level.yaml
>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-level.yaml
>> @@ -34,6 +34,20 @@ patternProperties:
>>          minItems: 1
>>          maxItems: 2
>>  
>> +      qcom,opp-cloop-vadj:
>> +        description: |
>> +          A value representing the closed-loop voltage adjustment value
> 
> A value?
> 
>> +          associated with this OPP node.
>> +        $ref: /schemas/types.yaml#/definitions/int32-array
>> +        maxItems: 2
> 
> Or 2 values?
Right, this description doesn't make any sense if you're just
looking at the documentation without looking at the driver..

Generally, each CPR3 instance can have multiple "threads"
(each one of which regulates voltage for some on-SoC IP or
part of it). The nth entry in the qcom,opp-[co]loop-vadj
array corresponds to a voltage offset for the nth thread.

If the nth entry in the array is missing, the driver assumes
the arr[0] one is "global" to this CPR3 instance at this OPP
level and applies it to all threads. ...and looking at it
again, this is sorta just bad design, especially if you
take into account that there's no known user of CPR3 that
employs more than 2 threads.

I'll remove that from the driver and make the description clearer.


Also, only noticed now.. "qcom,sdm630-cprh" was not documented,
so that's to be fixed for the next submission as well!


Konrad
> 
>> +
>> +      qcom,opp-oloop-vadj:
>> +        description: |
>> +          A value representing the open-loop voltage adjustment value
>> +          associated with this OPP node.
>> +        $ref: /schemas/types.yaml#/definitions/int32-array
>> +        maxItems: 2
>> +
>>      required:
>>        - opp-level
>>        - qcom,opp-fuse-level
>>
>> -- 
>> 2.39.1
>>