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

[Guenter Roeck]

On 3/1/22 05:21, Krzysztof Kozlowski wrote:
> 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@xxxxxxxxxxxx>
> > > > ---
> > > >   .../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?

Valid register values are 0..7, matching dt property values of
BIT(x) or 1 .. 128. Setting the property value to 1 (= 1 sample)
is equivalent to disabling sampling.

> 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.

Agreed.

Guenter