[PATCH v2 0/2] wmon: (adm1275) Add sample averaging binding support

[PATCH v2 0/2] wmon: (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 = /bits/ 8 <0x07>;
		adi,power-sample-average = /bits/ 8 <0x07>;
	};

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

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 property for ADM1275

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

-- 
2.17.1

[PATCH v2 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.

- adi,power-sample-average
- adi,volt-curr-sample-average

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

diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
index d311e0557401..4fc1421b7540 100644
--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -83,6 +83,8 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 };
 #define ADM1278_VI_AVG_MASK		GENMASK(ADM1278_VI_AVG_SHIFT + 2, \
 						ADM1278_VI_AVG_SHIFT)
 
+#define ADM1275_SAMPLES_AVG_MAX_INDEX	7
+
 struct adm1275_data {
 	int id;
 	bool have_oc_fault;
@@ -475,6 +477,7 @@ static int adm1275_probe(struct i2c_client *client)
 	int vindex = -1, voindex = -1, cindex = -1, pindex = -1;
 	int tindex = -1;
 	u32 shunt;
+	u8 avgindex;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_READ_BYTE_DATA
@@ -756,6 +759,34 @@ static int adm1275_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
+	if (data->have_power_sampling &&
+	    of_property_read_u8(client->dev.of_node,
+				"adi,power-sample-average", &avgindex) == 0) {
+		if (avgindex > ADM1275_SAMPLES_AVG_MAX_INDEX)
+			return -EINVAL;
+		ret = adm1275_write_pmon_config(data, client, true, avgindex);
+		if (ret < 0) {
+			dev_err(&client->dev,
+				"Setting power sample averaging failed with error %d",
+				ret);
+			return ret;
+		}
+	}
+
+	if (of_property_read_u8(client->dev.of_node,
+				"adi,volt-curr-sample-average",
+				&avgindex) == 0) {
+		if (avgindex > ADM1275_SAMPLES_AVG_MAX_INDEX)
+			return -EINVAL;
+		ret = adm1275_write_pmon_config(data, client, false, avgindex);
+		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 v2 2/2] dt-bindings: hwmon: Add sample averaging property for ADM1275

[Potin Lai]

Add new properties for binding sample averaging in PMON_CONFIG register

- adi,volt-curr-sample-average
- adi,power-sample-average

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..325f6827648f 100644
--- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
@@ -37,6 +37,48 @@ properties:
     description:
       Shunt resistor value in micro-Ohm.
 
+  adi,volt-curr-sample-average:
+    description: |
+      A value to configure VI_AVG in PMON_CONFIG register to indicate a
+      number of samples to be used to report voltage and currentvalues.
+      If set to 7, the 128 samples averaging would be used.
+
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum:
+      - 0 # 1 sample averaging
+      - 1 # 2 sample averaging
+      - 2 # 4 sample averaging
+      - 3 # 8 sample averaging
+      - 4 # 16 sample averaging
+      - 5 # 32 sample averaging
+      - 6 # 64 sample averaging
+      - 7 # 128 sample averaging
+    default: 0
+
+  adi,power-sample-average:
+    description: |
+      A value to configure PWR_AVG in PMON_CONFIG register to indicate a
+      number of samples to be used to report power values.
+      If set to 7, the 128 samples averaging would be used.
+
+      The chip supports power sample averaging:
+        "adi,adm1272"
+        "adi,adm1278"
+        "adi,adm1293"
+        "adi,adm1294"
+
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum:
+      - 0 # 1 sample averaging
+      - 1 # 2 sample averaging
+      - 2 # 4 sample averaging
+      - 3 # 8 sample averaging
+      - 4 # 16 sample averaging
+      - 5 # 32 sample averaging
+      - 6 # 64 sample averaging
+      - 7 # 128 sample averaging
+    default: 0
+
 required:
   - compatible
   - reg
@@ -53,5 +95,7 @@ examples:
             compatible = "adi,adm1272";
             reg = <0x10>;
             shunt-resistor-micro-ohms = <500>;
+            adi,volt-curr-sample-average = /bits/ 8 <0x07>;
+            adi,power-sample-average = /bits/ 8 <0x07>;
         };
     };
-- 
2.17.1

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

[Krzysztof Kozlowski]

On 24/02/2022 16:43, Potin Lai wrote:
> Add new properties for binding sample averaging in PMON_CONFIG register
> 
> - adi,volt-curr-sample-average
> - adi,power-sample-average
> 
> 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..325f6827648f 100644
> --- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
> @@ -37,6 +37,48 @@ properties:
>      description:
>        Shunt resistor value in micro-Ohm.
>  
> +  adi,volt-curr-sample-average:
> +    description: |
> +      A value to configure VI_AVG in PMON_CONFIG register to indicate a
> +      number of samples to be used to report voltage and currentvalues.

missing space after current.

> +      If set to 7, the 128 samples averaging would be used.
> +
> +    $ref: /schemas/types.yaml#/definitions/uint8

Make it a uint32.

The previous usage of this field was more appropriate. Instead of
keeping register values in DT, it's better to keep logical value. What
if in next cheap the register values have calculation method?

This should be like in v1 - enum for number of samples to take in averaging.

> +    enum:
> +      - 0 # 1 sample averaging
> +      - 1 # 2 sample averaging
> +      - 2 # 4 sample averaging
> +      - 3 # 8 sample averaging
> +      - 4 # 16 sample averaging
> +      - 5 # 32 sample averaging
> +      - 6 # 64 sample averaging
> +      - 7 # 128 sample averaging
> +    default: 0
> +
> +  adi,power-sample-average:
> +    description: |
> +      A value to configure PWR_AVG in PMON_CONFIG register to indicate a
> +      number of samples to be used to report power values.
> +      If set to 7, the 128 samples averaging would be used.

The same.

> +
> +      The chip supports power sample averaging:
> +        "adi,adm1272"
> +        "adi,adm1278"
> +        "adi,adm1293"
> +        "adi,adm1294"

This should be in if-block like here:
https://lore.kernel.org/linux-devicetree/YheqjZQHq0T%2FRSIz@robh.at.kernel.org/T/#m0672807a08c95aba2bccb927d37ff24fde471b8b


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 25/02/2022 08:06, Krzysztof Kozlowski wrote:
> On 24/02/2022 16:43, Potin Lai wrote:
>> Add new properties for binding sample averaging in PMON_CONFIG register
>>
>> - adi,volt-curr-sample-average
>> - adi,power-sample-average
>>
>> 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..325f6827648f 100644
>> --- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>> @@ -37,6 +37,48 @@ properties:
>>      description:
>>        Shunt resistor value in micro-Ohm.
>>  
>> +  adi,volt-curr-sample-average:
>> +    description: |
>> +      A value to configure VI_AVG in PMON_CONFIG register to indicate a
>> +      number of samples to be used to report voltage and currentvalues.
> 
> missing space after current.
> 
>> +      If set to 7, the 128 samples averaging would be used.
>> +
>> +    $ref: /schemas/types.yaml#/definitions/uint8
> 
> Make it a uint32.
> 
> The previous usage of this field was more appropriate. Instead of
> keeping register values in DT, it's better to keep logical value. What
> if in next cheap the register values have calculation method?
> 
> This should be like in v1 - enum for number of samples to take in averaging.
> 

One more thought: this field could also stay in current approach if you
change the meaning from "value to configure VI_AVG" to something like
"the exponent used to determine the number of samples, where the base is 2".

This approach would allow you to skip the "ilog" in the code. It sill
won't be that easily scalable if another chip comes with different
formula, but I think that's unlikely.

Best regards,
Krzysztof

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

[Guenter Roeck]

On 2/24/22 23:31, Krzysztof Kozlowski wrote:
> On 25/02/2022 08:06, Krzysztof Kozlowski wrote:
>> On 24/02/2022 16:43, Potin Lai wrote:
>>> Add new properties for binding sample averaging in PMON_CONFIG register
>>>
>>> - adi,volt-curr-sample-average
>>> - adi,power-sample-average
>>>
>>> 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..325f6827648f 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>>> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>>> @@ -37,6 +37,48 @@ properties:
>>>       description:
>>>         Shunt resistor value in micro-Ohm.
>>>   
>>> +  adi,volt-curr-sample-average:
>>> +    description: |
>>> +      A value to configure VI_AVG in PMON_CONFIG register to indicate a
>>> +      number of samples to be used to report voltage and currentvalues.
>>
>> missing space after current.
>>
>>> +      If set to 7, the 128 samples averaging would be used.
>>> +
>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>
>> Make it a uint32.
>>
>> The previous usage of this field was more appropriate. Instead of
>> keeping register values in DT, it's better to keep logical value. What
>> if in next cheap the register values have calculation method?
>>
>> This should be like in v1 - enum for number of samples to take in averaging.
>>
> 
> One more thought: this field could also stay in current approach if you
> change the meaning from "value to configure VI_AVG" to something like
> "the exponent used to determine the number of samples, where the base is 2".
> 
> This approach would allow you to skip the "ilog" in the code. It sill
> won't be that easily scalable if another chip comes with different
> formula, but I think that's unlikely.
> 

The standard hwmon ABI expects the number of samples, it isn't always a
power of 2, and the driver already implements it (with ilog2) as sysfs
attribute. I don't really see the point of "optimizing" something
like this to be chip specific just to avoid some error checking.

Guenter

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

[Krzysztof Kozlowski]

On 25/02/2022 09:41, Guenter Roeck wrote:
> On 2/24/22 23:31, Krzysztof Kozlowski wrote:
>> On 25/02/2022 08:06, Krzysztof Kozlowski wrote:
>>> On 24/02/2022 16:43, Potin Lai wrote:
>>>> Add new properties for binding sample averaging in PMON_CONFIG register
>>>>
>>>> - adi,volt-curr-sample-average
>>>> - adi,power-sample-average
>>>>
>>>> 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..325f6827648f 100644
>>>> --- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>>>> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
>>>> @@ -37,6 +37,48 @@ properties:
>>>>       description:
>>>>         Shunt resistor value in micro-Ohm.
>>>>   
>>>> +  adi,volt-curr-sample-average:
>>>> +    description: |
>>>> +      A value to configure VI_AVG in PMON_CONFIG register to indicate a
>>>> +      number of samples to be used to report voltage and currentvalues.
>>>
>>> missing space after current.
>>>
>>>> +      If set to 7, the 128 samples averaging would be used.
>>>> +
>>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>>
>>> Make it a uint32.
>>>
>>> The previous usage of this field was more appropriate. Instead of
>>> keeping register values in DT, it's better to keep logical value. What
>>> if in next cheap the register values have calculation method?
>>>
>>> This should be like in v1 - enum for number of samples to take in averaging.
>>>
>>
>> One more thought: this field could also stay in current approach if you
>> change the meaning from "value to configure VI_AVG" to something like
>> "the exponent used to determine the number of samples, where the base is 2".
>>
>> This approach would allow you to skip the "ilog" in the code. It sill
>> won't be that easily scalable if another chip comes with different
>> formula, but I think that's unlikely.
>>
> 
> The standard hwmon ABI expects the number of samples, it isn't always a
> power of 2, and the driver already implements it (with ilog2) as sysfs
> attribute. I don't really see the point of "optimizing" something
> like this to be chip specific just to avoid some error checking.

Thanks for confirming. +1



Best regards,
Krzysztof