Re: [PATCH v3 2/2] dt-bindings: hwmon: Add sample averaging properties for ADM1275

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/02/2022 17:13, POTIN LAI wrote:
> 
> Krzysztof Kozlowski 於 28/02/2022 10:25 pm 寫道:
>> On 28/02/2022 11:37, Potin Lai wrote:
>>> Add documentation of new properties for sample averaging in PMON_CONFIG
>>> register.
>>>
>>> New properties:
>>> - adi,volt-curr-sample-average
>>> - adi,power-sample-average
>>>
>>> Signed-off-by: Potin Lai <potin.lai@quantatw.com>
>>>
>>> doc
>> You have weirdly formatted commit msg.
> It must be pasted from somewhere accidentally, sorry.
>>> ---
>>>  .../bindings/hwmon/adi,adm1275.yaml           | 39 +++++++++++++++++++
>>>  1 file changed, 39 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>>> index 223393d7cafd..bc4206b257a8 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>>> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>>> @@ -37,6 +37,43 @@ properties:
>>>      description:
>>>        Shunt resistor value in micro-Ohm.
>>>  
>>> +  adi,volt-curr-sample-average:
>>> +    description: |
>>> +      Number of samples to be used to report voltage and current values.
>>> +      If the configured value is not a power of 2, sample averaging number
>>> +      will be configured with smaller and closest power of 2.
>>> +
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 1
>>> +    maximum: 128
>>> +    default: 1
>>> +
>>> +  adi,power-sample-average:
>>> +    description: |
>>> +      Number of samples to be used to report power values.
>>> +      If the configured value is not a power of 2, sample averaging number
>>> +      will be configured with smaller and closest power of 2.
>>> +
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 1
>>> +    maximum: 128
>>> +    default: 1
>>> +
>>> +if:
>> This should be in allOf.
> will add it.
>>
>>> +  not:
>> Remove negation and list devices where it is not allowed.
> will remove it.
>>
>>> +    properties:
>>> +      compatible:
>>> +        contains:
>>> +          enum:
>>> +          - adi,adm1272
>>> +          - adi,adm1278
>>> +          - adi,adm1293
>>> +          - adi,adm1294
>>> +then:
>>> +  properties:
>>> +    adi,power-sample-average:
>>> +      description: This property is not allowed.
>> This does not work. Please test it - add not allowed property to such
>> devices and look for error. I gave you the example how it should be
>> done. Why doing it in a different way which does not work?
>>
> Sorry for misunderstanding from original example. I rechecked the example and made a modification as below, before sending out new patch, would you mind help me review it and let me know if anything improper? Thank you.

Don't take the example by copying and pasting. It has to be adjusted, I
just explained there in example how to disallow a property.

> 
> 
> dependencies:
>   adi,enable-power-sample-average: [ 'adi,power-sample-average' ]
>   adi,power-sample-average: [ 'adi,enable-power-sample-average' ]
> 
> allOf:
>   - if:
>       properties:
>         compatible:
>           contains:
>             enum:
>               - adi,adm1272
>               - adi,adm1278
>               - adi,adm1293
>               - adi,adm1294
>     then:
>       required:
>         - adi,enable-power-sample-average

This does not look correct, because it is not a required property.

You should have "if:then: adi,enable-power-sample-average: false" etc"

>     else:
>       properties:
>         adi,enable-power-sample-average: false
> 
> 


Best regards,
Krzysztof