[PATCH v4 0/2] hwmon: (adm1275) Add sample averaging binding support

[PATCH v4 0/2] hwmon: (adm1275) Add sample averaging binding support

[Potin Lai]

This patch series allow user config PWR_AVG and VI_AVG in PMON_CONF
register by adding properties in device tree.

Example:
	adm1278@11 {
		compatible = "adi,adm1278";
		......
		adi,volt-curr-sample-average = <128>;
		adi,power-sample-average = <128>;
    adi,power-sample-average-enable;
	};

LINK: [v1] https://lore.kernel.org/all/20220223163817.30583-1-potin.lai@quantatw.com/
LINK: [v2] https://lore.kernel.org/all/20220224154329.9755-1-potin.lai@quantatw.com/
LINK: [v3] https://lore.kernel.org/all/20220228103716.10774-1-potin.lai@quantatw.com/

Changes v3 --> v4:
- add "adi,power-sample-average-enable" property
- add sample number cehcking in driver, only allow listed value
- remove info logging, add error log when invalid number detected

Changes v2 --> v3:
- change property type back to u32, use logical value instead of register
  value
- fix typo in properties description
- add if-block to descript "adi,power-sample-average" not alloed if
  compatible not in the enum list

Changes v1 --> v2:
- use more descriptive property name
- change property type from u32 to u8 
- add property value check, valid range between 1 and 7

Potin Lai (2):
  hwmon: (adm1275) Allow setting sample averaging
  dt-bindings: hwmon: Add sample averaging properties for ADM1275

 .../bindings/hwmon/adi,adm1275.yaml           | 44 +++++++++++++++++++
 drivers/hwmon/pmbus/adm1275.c                 | 40 +++++++++++++++++
 2 files changed, 84 insertions(+)

-- 
2.17.1

[PATCH v4 1/2] hwmon: (adm1275) Allow setting sample averaging

[Potin Lai]

Current driver assume PWR_AVG and VI_AVG as 1 by default, and user needs
to set sample averaging via sysfs manually.

This patch parses the properties below from device tree, and setting
sample averaging during probe. Input value must be one of value in the
list [1, 2, 4, 8, 16, 32, 64, 128].

Add property called "adi,power-sample-average-enable" for enabling
configuration of power sample averaging number.

Signed-off-by: Potin Lai <potin.lai@quantatw.com>
---
 drivers/hwmon/pmbus/adm1275.c | 40 +++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
index d311e0557401..0afeaa1319cd 100644
--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -475,6 +475,7 @@ static int adm1275_probe(struct i2c_client *client)
 	int vindex = -1, voindex = -1, cindex = -1, pindex = -1;
 	int tindex = -1;
 	u32 shunt;
+	u32 avg;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_READ_BYTE_DATA
@@ -756,6 +757,45 @@ static int adm1275_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
+	if (data->have_power_sampling &&
+	    of_property_read_bool(client->dev.of_node,
+				 "adi,power-sample-average-enable") &&
+	    of_property_read_u32(client->dev.of_node,
+				 "adi,power-sample-average", &avg) == 0) {
+		if (!avg || avg > ADM1275_SAMPLES_AVG_MAX ||
+		    BIT(__fls(avg)) != avg) {
+			dev_err(&client->dev,
+				"Invalid number of power samples");
+			return -EINVAL;
+		}
+		ret = adm1275_write_pmon_config(data, client, true,
+						ilog2(avg));
+		if (ret < 0) {
+			dev_err(&client->dev,
+				"Setting power sample averaging failed with error %d",
+				ret);
+			return ret;
+		}
+	}
+
+	if (of_property_read_u32(client->dev.of_node,
+				"adi,volt-curr-sample-average", &avg) == 0) {
+		if (!avg || avg > ADM1275_SAMPLES_AVG_MAX ||
+		    BIT(__fls(avg)) != avg) {
+			dev_err(&client->dev,
+				"Invalid number of volt and curr samples");
+			return -EINVAL;
+		}
+		ret = adm1275_write_pmon_config(data, client, false,
+						ilog2(avg));
+		if (ret < 0) {
+			dev_err(&client->dev,
+				"Setting voltage and current sample averaging failed with error %d",
+				ret);
+			return ret;
+		}
+	}
+
 	if (voindex < 0)
 		voindex = vindex;
 	if (vindex >= 0) {
-- 
2.17.1

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

[Potin Lai]

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@quantatw.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
+
+dependencies:
+  adi,power-sample-average-enable: ['adi,power-sample-average']
+  adi,power-sample-average: ['adi,power-sample-average-enable']
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,adm1075
+              - adi,adm1275
+              - adi,adm1276
+    then:
+      properties:
+        adi,power-sample-average-enable: false
+
 required:
   - compatible
   - reg
@@ -53,5 +94,8 @@ examples:
             compatible = "adi,adm1272";
             reg = <0x10>;
             shunt-resistor-micro-ohms = <500>;
+            adi,volt-curr-sample-average = <128>;
+            adi,power-sample-average = <128>;
+            adi,power-sample-average-enable;
         };
     };
-- 
2.17.1

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

[Krzysztof Kozlowski]

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@quantatw.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?


Best regards,
Krzysztof

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

[POTIN LAI]

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


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.


Potin
>
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

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

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@quantatw.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?
> 

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