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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bf1fa181-d2a7-eab9-4045-cf53ae3ce2f2@kernel.org>
Date:   Tue, 1 Mar 2022 14:21:57 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     POTIN LAI <potin.lai@...ntatw.com>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Rob Herring <robh+dt@...nel.org>
Cc:     Patrick Williams <patrick@...cx.xyz>, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v4 2/2] dt-bindings: hwmon: Add sample averaging
 properties for ADM1275

On 01/03/2022 13:42, POTIN LAI wrote:
> 
> Krzysztof Kozlowski 於 1/03/2022 7:16 pm 寫道:
>> On 01/03/2022 11:39, 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
>>> - adi,power-sample-average-enable
>>>
>>> Signed-off-by: Potin Lai <potin.lai@...ntatw.com>
>>> ---
>>>  .../bindings/hwmon/adi,adm1275.yaml           | 44 +++++++++++++++++++
>>>  1 file changed, 44 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>>> index 223393d7cafd..1b612dc06992 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>>> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>>> @@ -37,6 +37,47 @@ 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
>>> +    enum: [1, 2, 4, 8, 16, 32, 64, 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
>>> +    enum: [1, 2, 4, 8, 16, 32, 64, 128]
>>> +    default: 1
>>> +
>>> +  adi,power-sample-average-enable:
>>> +    description: Enable sample averaging for power reading.
>>> +    type: boolean
>> Why do you need this property? Voltage/current sampling is enabled in
>> your driver with presence of adi,volt-curr-sample-average. Why power
>> sampling is different?
> For "adi,power-sample-average", adm1075, adm1275 & adm127 don't have config reg for power sample average, so I add boolean type property to enable it
> But for "adi,power-sample-average-enable", all chips have ability of configuring, so it doesn't need a property to enable or disable.

So the reason to add separate property is that this feature can be
disabled. Since your driver does not disable it, it seems it is a
default state to have it disabled and you have to enable it, right?
Where is the enable code? I see you only write the sample averaging
value with adm1275_write_pmon_config(). There is no enable...

But wait, the power averaging is being disabled by writing 0 to
register, which is not allowed by bindings. How one can disable it?

I don't see any usage of "adi,power-sample-average-enable", neither in
driver nor in hardware. I also do not see the need for it, the purpose.

Then second part, you added default value of 1 to
adi,volt-curr-sample-average and adi,power-sample-average. If the
property is missing, then the default of 1 is applied, right? But
datasheet says that default is 128!

The bindings neither match hardware nor driver. They look entirely
independent. This is wrong. They should instead be strongly related to
the hardware, describe the hardware. Then the driver should implement
proper logic for it.

> Does example means that I can set any type (not just boolean?) of property to false if not allowed?
> Could I write as below?
> 
> allOf:
>   - if:
>       properties:
>         compatible:
>           contains:
>             enum:
>               - chips_not_support
>     then:
>       properties:
>         adi,power-sample-average: false
> 
> Sorry, I am not quite understand the example of set property not allowed, if I still get it wrong, please advise more detailed, thank you.

I gave you the example:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/example-schema.yaml?#n221
and there is no boolean in this example. You have a compatible which
does not allow some property, so you need "then:properties:foobar:false"

Best regards,
Krzysztof