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

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

[Potin Lai]

This patch series allow PWR_AVG and VI_AVG set from device tree

Example:
        adm1278@11 {
                compatible = "adi,adm1278";
                ......
                pwr-avg = <128>;
                vi-avg = <128>;
        };

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

 .../bindings/hwmon/adi,adm1275.yaml           | 10 ++++++++
 drivers/hwmon/pmbus/adm1275.c                 | 25 +++++++++++++++++++
 2 files changed, 35 insertions(+)

-- 
2.17.1

[PATCH 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 "pwr-avg" and "vi-avg" from device tree, and setting
sample averaging during probe.

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

diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
index d311e0557401..97faff4e3111 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,30 @@ static int adm1275_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
+	if (data->have_power_sampling &&
+	    of_property_read_u32(client->dev.of_node,
+				"pwr-avg", &avg) == 0) {
+		avg = clamp_val(avg, 1, ADM1275_SAMPLES_AVG_MAX);
+		ret = adm1275_write_pmon_config(data, client,
+						true, ilog2(avg));
+		if (ret < 0) {
+			dev_err(&client->dev, "set pwr-avg failed, ret = %d",
+				ret);
+			return ret;
+		}
+	}
+
+	if (of_property_read_u32(client->dev.of_node, "vi-avg", &avg) == 0) {
+		avg = clamp_val(avg, 1, ADM1275_SAMPLES_AVG_MAX);
+		ret = adm1275_write_pmon_config(data, client,
+						false, ilog2(avg));
+		if (ret < 0) {
+			dev_err(&client->dev, "set vi-avg failed, ret = %d",
+				ret);
+			return ret;
+		}
+	}
+
 	if (voindex < 0)
 		voindex = vindex;
 	if (vindex >= 0) {
-- 
2.17.1

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

[Guenter Roeck]

On 2/23/22 08:38, Potin Lai wrote:
> 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 "pwr-avg" and "vi-avg" from device tree, and setting
> sample averaging during probe.
> 
> Signed-off-by: Potin Lai <potin.lai@quantatw.com>
> ---
>   drivers/hwmon/pmbus/adm1275.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
> index d311e0557401..97faff4e3111 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,30 @@ static int adm1275_probe(struct i2c_client *client)
>   		return -ENODEV;
>   	}
>   
> +	if (data->have_power_sampling &&
> +	    of_property_read_u32(client->dev.of_node,
> +				"pwr-avg", &avg) == 0) {

Those properties will need a better name and will have to be prefixed
by the chip vendor.

> +		avg = clamp_val(avg, 1, ADM1275_SAMPLES_AVG_MAX);

clamp_val() is inappropriate for devicetree properties.
The property definition needs to include a valid range, and that range
needs to be validated and rejected if invalid.

> +		ret = adm1275_write_pmon_config(data, client,
> +						true, ilog2(avg));
> +		if (ret < 0) {
> +			dev_err(&client->dev, "set pwr-avg failed, ret = %d",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	if (of_property_read_u32(client->dev.of_node, "vi-avg", &avg) == 0) {
> +		avg = clamp_val(avg, 1, ADM1275_SAMPLES_AVG_MAX);
> +		ret = adm1275_write_pmon_config(data, client,
> +						false, ilog2(avg));
> +		if (ret < 0) {
> +			dev_err(&client->dev, "set vi-avg failed, ret = %d",

This needs to be more descriptive, eg "Setting voltage average failed, ...".
Same above.

> +				ret);
> +			return ret;
> +		}
> +	}
> +
>   	if (voindex < 0)
>   		voindex = vindex;
>   	if (vindex >= 0) {

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

[Potin Lai]

Add binding information for "pwr-avg" and "vi-avg" properties

Signed-off-by: Potin Lai <potin.lai@quantatw.com>
---
 .../devicetree/bindings/hwmon/adi,adm1275.yaml         | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
index 223393d7cafd..2525a67a880e 100644
--- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
@@ -37,6 +37,14 @@ properties:
     description:
       Shunt resistor value in micro-Ohm.
 
+  vi-avg:
+    description:
+      Sample averaging for current and voltage.
+
+  pwr-avg:
+    description:
+      Sample averaging for power.
+
 required:
   - compatible
   - reg
@@ -53,5 +61,7 @@ examples:
             compatible = "adi,adm1272";
             reg = <0x10>;
             shunt-resistor-micro-ohms = <500>;
+            vi-avg = <128>;
+            pwr-avg = <128>;
         };
     };
-- 
2.17.1

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

[Krzysztof Kozlowski]

On 23/02/2022 17:38, Potin Lai wrote:
> Add binding information for "pwr-avg" and "vi-avg" properties
> 
> Signed-off-by: Potin Lai <potin.lai@quantatw.com>
> ---
>  .../devicetree/bindings/hwmon/adi,adm1275.yaml         | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
> index 223393d7cafd..2525a67a880e 100644
> --- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
> @@ -37,6 +37,14 @@ properties:
>      description:
>        Shunt resistor value in micro-Ohm.
>  
> +  vi-avg:
> +    description:
> +      Sample averaging for current and voltage.

Both do not look like generic properties, so you need:
 - type
 - vendor prefix ("adi,")
 - maximum value?

It would be nice to actually explain what this value means - number of
readings for each averaging (unless it is obvious in the domain...)?

Does the hardware have default value?

> +
> +  pwr-avg:
> +    description:
> +      Sample averaging for power.
> +
>  required:
>    - compatible
>    - reg
> @@ -53,5 +61,7 @@ examples:
>              compatible = "adi,adm1272";
>              reg = <0x10>;
>              shunt-resistor-micro-ohms = <500>;
> +            vi-avg = <128>;
> +            pwr-avg = <128>;
>          };
>      };


Best regards,
Krzysztof

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

[Guenter Roeck]

On 2/23/22 08:38, Potin Lai wrote:
> Add binding information for "pwr-avg" and "vi-avg" properties
> 
> Signed-off-by: Potin Lai <potin.lai@quantatw.com>
> ---
>   .../devicetree/bindings/hwmon/adi,adm1275.yaml         | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
> index 223393d7cafd..2525a67a880e 100644
> --- a/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1275.yaml
> @@ -37,6 +37,14 @@ properties:
>       description:
>         Shunt resistor value in micro-Ohm.
>   
> +  vi-avg:
> +    description:
> +      Sample averaging for current and voltage.
> +
> +  pwr-avg:
> +    description:
> +      Sample averaging for power.
> +

Properties need a better name, prefixed with chip vendor, and the valid range
needs to be provided. Also, the description could be better, eg "Number of samples
to be used to report power values". Also, the chips actually supporting power
sampling need to be listed.

Guenter

>   required:
>     - compatible
>     - reg
> @@ -53,5 +61,7 @@ examples:
>               compatible = "adi,adm1272";
>               reg = <0x10>;
>               shunt-resistor-micro-ohms = <500>;
> +            vi-avg = <128>;
> +            pwr-avg = <128>;
>           };
>       };