[PATCH v2 00/16] Variety of fixes and new features for mr75203 driver

[PATCH v2 00/16] Variety of fixes and new features for mr75203 driver

[Eliav Farber]

List of fixes:
 - Fix for VM sensor allocation
 - Fix multi-channel voltage reading
 - Protect from negative voltage value
 - Fix temperature equation and coefficients

List of new features:
 - Add option to skip reset controller
 - Add VM active channel support
 - Add VM pre-scalar support
 - Add parsing of thermal coefficients from device-tree
 - Add debugfs to updater temperature coefficients on the fly

Changes between v1 and v2:
 *) Fix compilation error for patch 08/16:
    "warning: ISO C90 forbids variable length array"

Eliav Farber (16):
  hwmon: (mr75203) fix VM sensor allocation when "intel,vm-map" not
    defined
  hwmon: (mr75203) update pvt->v_num to the actual number of used
    sensors
  hwmon: (mr75203) update Moortec PVT controller intel,vm-map property
  hwmon: (mr75203) add Moortec PVT controller reset-control-skip
    property
  hwmon: (mr75203) add option to skip reset controller
  hwmon: (mr75203) fix multi-channel voltage reading
  hwmon: (mr75203) add VM active channels property for Moortec PVT
    controller
  hwmon: (mr75203) add VM active channel support
  hwmon: (mr75203) add VM pre-scalar property for Moortec PVT controller
  hwmon: (mr75203) add VM pre-scalar support
  hwmon: (mr75203) add protection for negative voltage value
  hwmon: (mr75203) modify the temperature equation
  hwmon: (mr75203) add thermal coefficient properties for Moortec PVT
    controller
  hwmon: (mr75203) parse thermal coefficients from device-tree
  hwmon: (mr75203) fix coding style space errors
  hwmon: (mr75203) add debugfs to read and write temperature
    coefficients

 .../bindings/hwmon/moortec,mr75203.yaml       |  60 ++-
 drivers/hwmon/mr75203.c                       | 467 +++++++++++++++---
 2 files changed, 461 insertions(+), 66 deletions(-)

-- 
2.37.1

[PATCH v2 01/16] hwmon: (mr75203) fix VM sensor allocation when "intel,vm-map" not defined

[Eliav Farber]

Fix a bug that in case "intel,vm-map" is missing 'num' is set to 0,
and no voltage channel infos are allocated.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 046523d47c29..0e29877a1a9c 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -580,8 +580,6 @@ static int mr75203_probe(struct platform_device *pdev)
 	}
 
 	if (vm_num) {
-		u32 num = vm_num;
-
 		ret = pvt_get_regmap(pdev, "vm", pvt);
 		if (ret)
 			return ret;
@@ -594,30 +592,28 @@ static int mr75203_probe(struct platform_device *pdev)
 		ret = device_property_read_u8_array(dev, "intel,vm-map",
 						    pvt->vm_idx, vm_num);
 		if (ret) {
-			num = 0;
+			/*
+			 * Incase intel,vm-map property is not defined, we
+			 * assume incremental channel numbers.
+			 */
+			for (i = 0; i < vm_num; i++)
+				pvt->vm_idx[i] = i;
 		} else {
 			for (i = 0; i < vm_num; i++)
 				if (pvt->vm_idx[i] >= vm_num ||
-				    pvt->vm_idx[i] == 0xff) {
-					num = i;
+				    pvt->vm_idx[i] == 0xff)
 					break;
-				}
-		}
 
-		/*
-		 * Incase intel,vm-map property is not defined, we assume
-		 * incremental channel numbers.
-		 */
-		for (i = num; i < vm_num; i++)
-			pvt->vm_idx[i] = i;
+			vm_num = i;
+		}
 
-		in_config = devm_kcalloc(dev, num + 1,
+		in_config = devm_kcalloc(dev, vm_num + 1,
 					 sizeof(*in_config), GFP_KERNEL);
 		if (!in_config)
 			return -ENOMEM;
 
-		memset32(in_config, HWMON_I_INPUT, num);
-		in_config[num] = 0;
+		memset32(in_config, HWMON_I_INPUT, vm_num);
+		in_config[vm_num] = 0;
 		pvt_in.config = in_config;
 
 		pvt_info[index++] = &pvt_in;
-- 
2.37.1

Re: [PATCH v2 01/16] hwmon: (mr75203) fix VM sensor allocation when "intel,vm-map" not defined

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:06AM +0000, Eliav Farber wrote:
> Fix a bug that in case "intel,vm-map" is missing 'num' is set to 0,
> and no voltage channel infos are allocated.
> 

"intel,vm-map" is listed as required property in moortec,mr75203.yaml.
If it is missing, the probe function should fail.

Guenter

> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  drivers/hwmon/mr75203.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index 046523d47c29..0e29877a1a9c 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -580,8 +580,6 @@ static int mr75203_probe(struct platform_device *pdev)
>  	}
>  
>  	if (vm_num) {
> -		u32 num = vm_num;
> -
>  		ret = pvt_get_regmap(pdev, "vm", pvt);
>  		if (ret)
>  			return ret;
> @@ -594,30 +592,28 @@ static int mr75203_probe(struct platform_device *pdev)
>  		ret = device_property_read_u8_array(dev, "intel,vm-map",
>  						    pvt->vm_idx, vm_num);
>  		if (ret) {
> -			num = 0;
> +			/*
> +			 * Incase intel,vm-map property is not defined, we
> +			 * assume incremental channel numbers.
> +			 */
> +			for (i = 0; i < vm_num; i++)
> +				pvt->vm_idx[i] = i;
>  		} else {
>  			for (i = 0; i < vm_num; i++)
>  				if (pvt->vm_idx[i] >= vm_num ||
> -				    pvt->vm_idx[i] == 0xff) {
> -					num = i;
> +				    pvt->vm_idx[i] == 0xff)
>  					break;
> -				}
> -		}
>  
> -		/*
> -		 * Incase intel,vm-map property is not defined, we assume
> -		 * incremental channel numbers.
> -		 */
> -		for (i = num; i < vm_num; i++)
> -			pvt->vm_idx[i] = i;
> +			vm_num = i;
> +		}
>  
> -		in_config = devm_kcalloc(dev, num + 1,
> +		in_config = devm_kcalloc(dev, vm_num + 1,
>  					 sizeof(*in_config), GFP_KERNEL);
>  		if (!in_config)
>  			return -ENOMEM;
>  
> -		memset32(in_config, HWMON_I_INPUT, num);
> -		in_config[num] = 0;
> +		memset32(in_config, HWMON_I_INPUT, vm_num);
> +		in_config[vm_num] = 0;
>  		pvt_in.config = in_config;
>  
>  		pvt_info[index++] = &pvt_in;

Re: [PATCH v2 01/16] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Farber, Eliav]

On 8/18/2022 10:40 PM, Guenter Roeck wrote:
> "intel,vm-map" is listed as required property in moortec,mr75203.yaml.
> If it is missing, the probe function should fail.

Indeed according to moortec,mr75203.yaml "intel,vm-map" is listed as 
required
but the code indicates otherwise:

/*
  * Incase intel,vm-map property is not defined, we assume
  * incremental channel numbers.
  */

The probe function takes care in case it is missing and does not fail.
It also seems less reasonable that an Intel proprietary parameter will be
required and not optional.
So in patch 03 I fixed the moortec,mr75203.yaml documentation and 
changed it to
be optional.

--
Regards, Eliav

[PATCH v2 02/16] hwmon: (mr75203) update pvt->v_num to the actual number of used sensors

[Eliav Farber]

This issue is relevant when intel,vm-map is set, and defines a lower
number of VMs than actually supported.

This change is needed for all places that use pvt->v_num later on in the
code.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 0e29877a1a9c..f89f7bb5d698 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -605,6 +605,7 @@ static int mr75203_probe(struct platform_device *pdev)
 					break;
 
 			vm_num = i;
+			pvt->v_num = i;
 		}
 
 		in_config = devm_kcalloc(dev, vm_num + 1,
-- 
2.37.1

Re: [PATCH v2 02/16] hwmon: (mr75203) update pvt->v_num to the actual number of used sensors

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:07AM +0000, Eliav Farber wrote:
> This issue is relevant when intel,vm-map is set, and defines a lower
> number of VMs than actually supported.
> 
> This change is needed for all places that use pvt->v_num later on in the
> code.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  drivers/hwmon/mr75203.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index 0e29877a1a9c..f89f7bb5d698 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -605,6 +605,7 @@ static int mr75203_probe(struct platform_device *pdev)
>  					break;
>  
>  			vm_num = i;
> +			pvt->v_num = i;

This code is changed to no longer set pvt->v_num in the previous patch.
Do not send patches which fix issues introduced in an earlier patch of
the same series. Besides, again, regarding "when intel,vm-map is set":
That is not an optional property.

Guenter

>  		}
>  
>  		in_config = devm_kcalloc(dev, vm_num + 1,

[PATCH v2 03/16] hwmon: (mr75203) update Moortec PVT controller intel,vm-map property

[Eliav Farber]

The "intel,vm-map property" is optional and not required.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index 6f3e3c01f717..f9e849cc73e0 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -48,12 +48,12 @@ required:
   - compatible
   - reg
   - reg-names
-  - intel,vm-map
   - clocks
   - resets
   - "#thermal-sensor-cells"
 
-additionalProperties: false
+additionalProperties:
+  - intel,vm-map
 
 examples:
   - |
-- 
2.37.1

Re: [PATCH v2 03/16] hwmon: (mr75203) update Moortec PVT controller intel,vm-map property

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:08AM +0000, Eliav Farber wrote:
> The "intel,vm-map property" is optional and not required.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>

Ah. This patch should come first, before making associated
code changes, and there would need to be an explanation
_why_ it is not a mandatory property. Also, some action
is taken in the previous patches if the property is not
there. That indicates that the driver is expected to
use some defaults if the property is indeed not provided,
and that expected default should be documented.

Guenter

> ---
>  Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> index 6f3e3c01f717..f9e849cc73e0 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -48,12 +48,12 @@ required:
>    - compatible
>    - reg
>    - reg-names
> -  - intel,vm-map
>    - clocks
>    - resets
>    - "#thermal-sensor-cells"
>  
> -additionalProperties: false
> +additionalProperties:
> +  - intel,vm-map
>  
>  examples:
>    - |

Re: [PATCH v2 03/16] hwmon: (mr75203) update Moortec PVT controller intel, vm-map property

[Farber, Eliav]

On 8/18/2022 10:47 PM, Guenter Roeck wrote:
> Ah. This patch should come first, before making associated
> code changes, and there would need to be an explanation
> _why_ it is not a mandatory property. Also, some action
> is taken in the previous patches if the property is not
> there. That indicates that the driver is expected to
> use some defaults if the property is indeed not provided,
> and that expected default should be documented. 
Ack.
I should have been more clear and explained that driver actually handles
the case that "intel,vm-map" does not exist (including an explicit comment
- "Incase intel,vm-map property is not defined ...") and that it is
actually optional.

--
Regards, Eliav

[PATCH v2 04/16] hwmon: (mr75203) add Moortec PVT controller reset-control-skip property

[Eliav Farber]

Adding a "reset-control-skip" bool property to the mr75203 node will
avoid looking up and obtaining a reference to a reset controller.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 .../devicetree/bindings/hwmon/moortec,mr75203.yaml          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index f9e849cc73e0..da9c3cdcb6f0 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -44,6 +44,11 @@ properties:
   "#thermal-sensor-cells":
     const: 1
 
+  reset-control-skip:
+    description:
+      reset-control-skip bool property defines if obtaining a
+      reference to a reset controller should be skipped.
+
 required:
   - compatible
   - reg
@@ -54,6 +59,7 @@ required:
 
 additionalProperties:
   - intel,vm-map
+  - reset-control-skip
 
 examples:
   - |
-- 
2.37.1

Re: [PATCH v2 04/16] hwmon: (mr75203) add Moortec PVT controller reset-control-skip property

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:09AM +0000, Eliav Farber wrote:
> Adding a "reset-control-skip" bool property to the mr75203 node will
> avoid looking up and obtaining a reference to a reset controller.
> 

This seems overly complex. WHy not just declare the "resets"
property optional ?

Guenter

> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml          | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> index f9e849cc73e0..da9c3cdcb6f0 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -44,6 +44,11 @@ properties:
>    "#thermal-sensor-cells":
>      const: 1
>  
> +  reset-control-skip:
> +    description:
> +      reset-control-skip bool property defines if obtaining a
> +      reference to a reset controller should be skipped.
> +
>  required:
>    - compatible
>    - reg
> @@ -54,6 +59,7 @@ required:
>  
>  additionalProperties:
>    - intel,vm-map
> +  - reset-control-skip
>  
>  examples:
>    - |

Re: [PATCH v2 04/16] hwmon: (mr75203) add Moortec PVT controller reset-control-skip property

[Farber, Eliav]

On 8/18/2022 11:01 PM, Guenter Roeck wrote:
> On Wed, Aug 17, 2022 at 05:43:09AM +0000, Eliav Farber wrote:
>> Adding a "reset-control-skip" bool property to the mr75203 node will
>> avoid looking up and obtaining a reference to a reset controller.
>>
>
> This seems overly complex. WHy not just declare the "resets"
> property optional ? 
Fixed. I declared "resets" property  as optional, and modified the driver
to support it being optional instead of the change I did.
Will be part of next version.

--
Thanks, Eliav

[PATCH v2 05/16] hwmon: (mr75203) add option to skip reset controller

[Eliav Farber]

Adding a "reset-control-skip" bool property to the mr75203 node will
avoid looking up and obtaining a reference to a reset controller.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index f89f7bb5d698..bec63b611eb4 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
@@ -525,14 +526,19 @@ static int mr75203_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pvt->rst = devm_reset_control_get_exclusive(dev, NULL);
-	if (IS_ERR(pvt->rst))
-		return dev_err_probe(dev, PTR_ERR(pvt->rst),
-				     "failed to get reset control\n");
+	if (of_property_read_bool(dev->of_node, "reset-control-skip")) {
+		dev_info(dev, "skipping reset-control\n");
+	} else {
+		pvt->rst = devm_reset_control_get_exclusive(dev, NULL);
+		if (IS_ERR(pvt->rst))
+			return dev_err_probe(dev, PTR_ERR(pvt->rst),
+					     "failed to get reset control\n");
 
-	ret = pvt_reset_control_deassert(dev, pvt);
-	if (ret)
-		return dev_err_probe(dev, ret, "cannot deassert reset control\n");
+		ret = pvt_reset_control_deassert(dev, pvt);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "cannot deassert reset control\n");
+	}
 
 	ret = regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
 	if(ret < 0)
-- 
2.37.1

[PATCH v2 06/16] hwmon: (mr75203) fix multi-channel voltage reading

[Eliav Farber]

- Fix voltage reading to support number of channels in VM IP (CH_NUM).
- Configure the ip-polling register to enable polling for all channels.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index bec63b611eb4..4419e481d47c 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -69,8 +69,9 @@
 
 /* VM Individual Macro Register */
 #define VM_COM_REG_SIZE	0x200
-#define VM_SDIF_DONE(n)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (n))
-#define VM_SDIF_DATA(n)	(VM_COM_REG_SIZE + 0x40 + 0x200 * (n))
+#define VM_SDIF_DONE(vm)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (vm))
+#define VM_SDIF_DATA(vm, ch)	\
+	(VM_COM_REG_SIZE + 0x40 + 0x200 * (vm) + 0x4 * (ch))
 
 /* SDA Slave Register */
 #define IP_CTRL			0x00
@@ -116,6 +117,7 @@ struct pvt_device {
 	u32			t_num;
 	u32			p_num;
 	u32			v_num;
+	u32			c_num;
 	u32			ip_freq;
 	u8			*vm_idx;
 };
@@ -181,12 +183,14 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 	struct regmap *v_map = pvt->v_map;
 	u32 n, stat;
 	u8 vm_idx;
+	u8 ch_idx;
 	int ret;
 
-	if (channel >= pvt->v_num)
+	if (channel >= pvt->v_num * pvt->c_num)
 		return -EINVAL;
 
-	vm_idx = pvt->vm_idx[channel];
+	vm_idx = pvt->vm_idx[channel / pvt->c_num];
+	ch_idx = channel % pvt->c_num;
 
 	switch (attr) {
 	case hwmon_in_input:
@@ -197,7 +201,7 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 		if (ret)
 			return ret;
 
-		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx), &n);
+		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx, ch_idx), &n);
 		if(ret < 0)
 			return ret;
 
@@ -386,6 +390,20 @@ static int pvt_init(struct pvt_device *pvt)
 		if (ret)
 			return ret;
 
+		val = GENMASK(pvt->c_num - 1, 0) | VM_CH_INIT |
+		      IP_POLL << SDIF_ADDR_SFT |
+		      SDIF_WRN_W | SDIF_PROG;
+		ret = regmap_write(v_map, SDIF_W, val);
+		if (ret < 0)
+			return ret;
+
+		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
+					       val, !(val & SDIF_BUSY),
+					       PVT_POLL_DELAY_US,
+					       PVT_POLL_TIMEOUT_US);
+		if (ret)
+			return ret;
+
 		val = CFG1_VOL_MEAS_MODE | CFG1_PARALLEL_OUT |
 		      CFG1_14_BIT | IP_CFG << SDIF_ADDR_SFT |
 		      SDIF_WRN_W | SDIF_PROG;
@@ -501,7 +519,7 @@ static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt
 static int mr75203_probe(struct platform_device *pdev)
 {
 	const struct hwmon_channel_info **pvt_info;
-	u32 ts_num, vm_num, pd_num, val, index, i;
+	u32 ts_num, vm_num, pd_num, ch_num, val, index, i;
 	struct device *dev = &pdev->dev;
 	u32 *temp_config, *in_config;
 	struct device *hwmon_dev;
@@ -547,9 +565,11 @@ static int mr75203_probe(struct platform_device *pdev)
 	ts_num = (val & TS_NUM_MSK) >> TS_NUM_SFT;
 	pd_num = (val & PD_NUM_MSK) >> PD_NUM_SFT;
 	vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
+	ch_num = (val & CH_NUM_MSK) >> CH_NUM_SFT;
 	pvt->t_num = ts_num;
 	pvt->p_num = pd_num;
 	pvt->v_num = vm_num;
+	pvt->c_num = ch_num;
 	val = 0;
 	if (ts_num)
 		val++;
@@ -586,6 +606,8 @@ static int mr75203_probe(struct platform_device *pdev)
 	}
 
 	if (vm_num) {
+		u32 total_ch = ch_num * vm_num;
+
 		ret = pvt_get_regmap(pdev, "vm", pvt);
 		if (ret)
 			return ret;
@@ -614,13 +636,13 @@ static int mr75203_probe(struct platform_device *pdev)
 			pvt->v_num = i;
 		}
 
-		in_config = devm_kcalloc(dev, vm_num + 1,
+		in_config = devm_kcalloc(dev, total_ch + 1,
 					 sizeof(*in_config), GFP_KERNEL);
 		if (!in_config)
 			return -ENOMEM;
 
-		memset32(in_config, HWMON_I_INPUT, vm_num);
-		in_config[vm_num] = 0;
+		memset32(in_config, HWMON_I_INPUT, total_ch);
+		in_config[total_ch] = 0;
 		pvt_in.config = in_config;
 
 		pvt_info[index++] = &pvt_in;
-- 
2.37.1

Re: [PATCH v2 06/16] hwmon: (mr75203) fix multi-channel voltage reading

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:11AM +0000, Eliav Farber wrote:
> - Fix voltage reading to support number of channels in VM IP (CH_NUM).
> - Configure the ip-polling register to enable polling for all channels.
> 

That fails to explain what is actually wrong in the current code.
Also, one fix per patch, please.

> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  drivers/hwmon/mr75203.c | 40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index bec63b611eb4..4419e481d47c 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -69,8 +69,9 @@
>  
>  /* VM Individual Macro Register */
>  #define VM_COM_REG_SIZE	0x200
> -#define VM_SDIF_DONE(n)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (n))
> -#define VM_SDIF_DATA(n)	(VM_COM_REG_SIZE + 0x40 + 0x200 * (n))
> +#define VM_SDIF_DONE(vm)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (vm))
> +#define VM_SDIF_DATA(vm, ch)	\
> +	(VM_COM_REG_SIZE + 0x40 + 0x200 * (vm) + 0x4 * (ch))
>  
>  /* SDA Slave Register */
>  #define IP_CTRL			0x00
> @@ -116,6 +117,7 @@ struct pvt_device {
>  	u32			t_num;
>  	u32			p_num;
>  	u32			v_num;
> +	u32			c_num;
>  	u32			ip_freq;
>  	u8			*vm_idx;
>  };
> @@ -181,12 +183,14 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
>  	struct regmap *v_map = pvt->v_map;
>  	u32 n, stat;
>  	u8 vm_idx;
> +	u8 ch_idx;
>  	int ret;
>  
> -	if (channel >= pvt->v_num)
> +	if (channel >= pvt->v_num * pvt->c_num)
>  		return -EINVAL;
>  
> -	vm_idx = pvt->vm_idx[channel];
> +	vm_idx = pvt->vm_idx[channel / pvt->c_num];
> +	ch_idx = channel % pvt->c_num;
>  
>  	switch (attr) {
>  	case hwmon_in_input:
> @@ -197,7 +201,7 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
>  		if (ret)
>  			return ret;
>  
> -		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx), &n);
> +		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx, ch_idx), &n);
>  		if(ret < 0)
>  			return ret;
>  
> @@ -386,6 +390,20 @@ static int pvt_init(struct pvt_device *pvt)
>  		if (ret)
>  			return ret;
>  
> +		val = GENMASK(pvt->c_num - 1, 0) | VM_CH_INIT |
> +		      IP_POLL << SDIF_ADDR_SFT |
> +		      SDIF_WRN_W | SDIF_PROG;
> +		ret = regmap_write(v_map, SDIF_W, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
> +					       val, !(val & SDIF_BUSY),
> +					       PVT_POLL_DELAY_US,
> +					       PVT_POLL_TIMEOUT_US);
> +		if (ret)
> +			return ret;
> +
>  		val = CFG1_VOL_MEAS_MODE | CFG1_PARALLEL_OUT |
>  		      CFG1_14_BIT | IP_CFG << SDIF_ADDR_SFT |
>  		      SDIF_WRN_W | SDIF_PROG;
> @@ -501,7 +519,7 @@ static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt
>  static int mr75203_probe(struct platform_device *pdev)
>  {
>  	const struct hwmon_channel_info **pvt_info;
> -	u32 ts_num, vm_num, pd_num, val, index, i;
> +	u32 ts_num, vm_num, pd_num, ch_num, val, index, i;
>  	struct device *dev = &pdev->dev;
>  	u32 *temp_config, *in_config;
>  	struct device *hwmon_dev;
> @@ -547,9 +565,11 @@ static int mr75203_probe(struct platform_device *pdev)
>  	ts_num = (val & TS_NUM_MSK) >> TS_NUM_SFT;
>  	pd_num = (val & PD_NUM_MSK) >> PD_NUM_SFT;
>  	vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
> +	ch_num = (val & CH_NUM_MSK) >> CH_NUM_SFT;
>  	pvt->t_num = ts_num;
>  	pvt->p_num = pd_num;
>  	pvt->v_num = vm_num;
> +	pvt->c_num = ch_num;
>  	val = 0;
>  	if (ts_num)
>  		val++;
> @@ -586,6 +606,8 @@ static int mr75203_probe(struct platform_device *pdev)
>  	}
>  
>  	if (vm_num) {
> +		u32 total_ch = ch_num * vm_num;
> +
>  		ret = pvt_get_regmap(pdev, "vm", pvt);
>  		if (ret)
>  			return ret;
> @@ -614,13 +636,13 @@ static int mr75203_probe(struct platform_device *pdev)
>  			pvt->v_num = i;
>  		}
>  
> -		in_config = devm_kcalloc(dev, vm_num + 1,
> +		in_config = devm_kcalloc(dev, total_ch + 1,
>  					 sizeof(*in_config), GFP_KERNEL);
>  		if (!in_config)
>  			return -ENOMEM;
>  
> -		memset32(in_config, HWMON_I_INPUT, vm_num);
> -		in_config[vm_num] = 0;
> +		memset32(in_config, HWMON_I_INPUT, total_ch);
> +		in_config[total_ch] = 0;
>  		pvt_in.config = in_config;
>  
>  		pvt_info[index++] = &pvt_in;

Re: [PATCH v2 06/16] hwmon: (mr75203) fix multi-channel voltage reading

[Farber, Eliav]

On 8/18/2022 11:03 PM, Guenter Roeck wrote:
> On Wed, Aug 17, 2022 at 05:43:11AM +0000, Eliav Farber wrote:
>> - Fix voltage reading to support number of channels in VM IP (CH_NUM).
>> - Configure the ip-polling register to enable polling for all channels.
>>
>
> That fails to explain what is actually wrong in the current code.
> Also, one fix per patch, please.
I moved the configuration of the ip-polling register to a separate patch.

The problem in the current code is that it allocates in_config according
to the total number of voltage monitors and not according to the total
number of channels in all voltage monitors.
Therefore it didn’t create enough sysfs to read all inputs.
Also pvr_read_in() only tries to access the first channel in each voltage
monitor.
I will add this explanation to next version.

--
Thanks, Eliav

[PATCH v2 07/16] hwmon: (mr75203) add VM active channels property for Moortec PVT controller

[Eliav Farber]

Add optional "vm-active-channels" property to define the number of
active channels per VM.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 .../devicetree/bindings/hwmon/moortec,mr75203.yaml       | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index da9c3cdcb6f0..6111b5069b3c 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -49,6 +49,13 @@ properties:
       reset-control-skip bool property defines if obtaining a
       reference to a reset controller should be skipped.
 
+  vm-active-channels:
+    description:
+      vm-active-channels defines the number of channels per VM
+      that are actually used and are connected to some source.
+      A value of 0 means that the entire VM sensor is nou used.
+    $ref: /schemas/types.yaml#definitions/uint8-array
+
 required:
   - compatible
   - reg
@@ -60,6 +67,7 @@ required:
 additionalProperties:
   - intel,vm-map
   - reset-control-skip
+  - vm-active-channels
 
 examples:
   - |
@@ -73,5 +81,6 @@ examples:
         intel,vm-map = [03 01 04 ff ff];
         clocks = <&osc0>;
         resets = <&rcu0 0x40 7>;
+        vm-active-channels = [08 10 02];
         #thermal-sensor-cells = <1>;
     };
-- 
2.37.1

Re: [PATCH v2 07/16] hwmon: (mr75203) add VM active channels property for Moortec PVT controller

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:12AM +0000, Eliav Farber wrote:
> Add optional "vm-active-channels" property to define the number of
> active channels per VM.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml       | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> index da9c3cdcb6f0..6111b5069b3c 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -49,6 +49,13 @@ properties:
>        reset-control-skip bool property defines if obtaining a
>        reference to a reset controller should be skipped.
>  
> +  vm-active-channels:
> +    description:
> +      vm-active-channels defines the number of channels per VM
> +      that are actually used and are connected to some source.
> +      A value of 0 means that the entire VM sensor is nou used.

not ?

> +    $ref: /schemas/types.yaml#definitions/uint8-array
> +
>  required:
>    - compatible
>    - reg
> @@ -60,6 +67,7 @@ required:
>  additionalProperties:
>    - intel,vm-map
>    - reset-control-skip
> +  - vm-active-channels
>  
>  examples:
>    - |
> @@ -73,5 +81,6 @@ examples:
>          intel,vm-map = [03 01 04 ff ff];
>          clocks = <&osc0>;
>          resets = <&rcu0 0x40 7>;
> +        vm-active-channels = [08 10 02];

Is that how properties are defined nowadays ? I am left with
no clues how this is supposed to be interpreted by a driver.
What does "08 10 02" mean ? How does that refer to "the number of
active channels per VM" ?

Also, I am not a devicetree expert, but I am quite sure that all
those chip specific properties would need a vendor prefix.

Guenter

>          #thermal-sensor-cells = <1>;
>      };

[PATCH v2 08/16] hwmon: (mr75203) add VM active channel support

[Eliav Farber]

Add active channel support per VM, which is useful when not all VM
channels are used.
Number of active channel is read from device-tree.
When absent in device-tree, all channels are assumed to be used.
Setting number of active channels to 0, means that entire VM sesnor is
not used (this can partially replace the "intel,vm-map" functionality).

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 94 +++++++++++++++++++++++++++++++----------
 1 file changed, 72 insertions(+), 22 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 4419e481d47c..2e6139c09efc 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -30,6 +30,8 @@
 #define CH_NUM_MSK	GENMASK(31, 24)
 #define CH_NUM_SFT	24
 
+#define VM_NUM_MAX	(VM_NUM_MSK >> VM_NUM_SFT)
+
 /* Macro Common Register */
 #define CLK_SYNTH		0x00
 #define CLK_SYNTH_LO_SFT	0
@@ -107,6 +109,11 @@
 #define PVT_N_CONST		90
 #define PVT_R_CONST		245805
 
+struct voltage_device {
+	u8 vm_map;
+	u8 ch_map;
+};
+
 struct pvt_device {
 	struct regmap		*c_map;
 	struct regmap		*t_map;
@@ -114,12 +121,13 @@ struct pvt_device {
 	struct regmap		*v_map;
 	struct clk		*clk;
 	struct reset_control	*rst;
+	struct voltage_device	*vd;
 	u32			t_num;
 	u32			p_num;
 	u32			v_num;
-	u32			c_num;
 	u32			ip_freq;
-	u8			*vm_idx;
+	u8			vm_ch_max;
+	u8			vm_ch_total;
 };
 
 static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
@@ -186,11 +194,11 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 	u8 ch_idx;
 	int ret;
 
-	if (channel >= pvt->v_num * pvt->c_num)
+	if (channel >= pvt->vm_ch_total)
 		return -EINVAL;
 
-	vm_idx = pvt->vm_idx[channel / pvt->c_num];
-	ch_idx = channel % pvt->c_num;
+	vm_idx = pvt->vd[channel].vm_map;
+	ch_idx = pvt->vd[channel].ch_map;
 
 	switch (attr) {
 	case hwmon_in_input:
@@ -390,7 +398,7 @@ static int pvt_init(struct pvt_device *pvt)
 		if (ret)
 			return ret;
 
-		val = GENMASK(pvt->c_num - 1, 0) | VM_CH_INIT |
+		val = GENMASK(pvt->vm_ch_max - 1, 0) | VM_CH_INIT |
 		      IP_POLL << SDIF_ADDR_SFT |
 		      SDIF_WRN_W | SDIF_PROG;
 		ret = regmap_write(v_map, SDIF_W, val);
@@ -519,7 +527,7 @@ static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt
 static int mr75203_probe(struct platform_device *pdev)
 {
 	const struct hwmon_channel_info **pvt_info;
-	u32 ts_num, vm_num, pd_num, ch_num, val, index, i;
+	u32 ts_num, vm_num, pd_num, ch_num, val, index, i, j, k;
 	struct device *dev = &pdev->dev;
 	u32 *temp_config, *in_config;
 	struct device *hwmon_dev;
@@ -569,7 +577,6 @@ static int mr75203_probe(struct platform_device *pdev)
 	pvt->t_num = ts_num;
 	pvt->p_num = pd_num;
 	pvt->v_num = vm_num;
-	pvt->c_num = ch_num;
 	val = 0;
 	if (ts_num)
 		val++;
@@ -606,43 +613,86 @@ static int mr75203_probe(struct platform_device *pdev)
 	}
 
 	if (vm_num) {
-		u32 total_ch = ch_num * vm_num;
+		u8 vm_idx[VM_NUM_MAX];
+		u8 vm_active_ch[VM_NUM_MAX];
 
 		ret = pvt_get_regmap(pdev, "vm", pvt);
 		if (ret)
 			return ret;
 
-		pvt->vm_idx = devm_kcalloc(dev, vm_num, sizeof(*pvt->vm_idx),
-					   GFP_KERNEL);
-		if (!pvt->vm_idx)
-			return -ENOMEM;
-
-		ret = device_property_read_u8_array(dev, "intel,vm-map",
-						    pvt->vm_idx, vm_num);
+		ret = device_property_read_u8_array(dev, "intel,vm-map", vm_idx,
+						    vm_num);
 		if (ret) {
 			/*
 			 * Incase intel,vm-map property is not defined, we
 			 * assume incremental channel numbers.
 			 */
 			for (i = 0; i < vm_num; i++)
-				pvt->vm_idx[i] = i;
+				vm_idx[i] = i;
 		} else {
 			for (i = 0; i < vm_num; i++)
-				if (pvt->vm_idx[i] >= vm_num ||
-				    pvt->vm_idx[i] == 0xff)
+				if (vm_idx[i] >= vm_num || vm_idx[i] == 0xff)
 					break;
 
 			vm_num = i;
 			pvt->v_num = i;
 		}
 
-		in_config = devm_kcalloc(dev, total_ch + 1,
+		ret = device_property_read_u8_array(dev, "vm-active-channels",
+						    vm_active_ch, vm_num);
+		if (ret) {
+			/*
+			 * Incase vm-active-channels property is not defined,
+			 * we assume each VM sensor has all of its channels
+			 * active.
+			 */
+			for (i = 0; i < vm_num; i++)
+				vm_active_ch[i] = ch_num;
+
+			pvt->vm_ch_max = ch_num;
+			pvt->vm_ch_total = ch_num * vm_num;
+		} else {
+			for (i = 0; i < vm_num; i++) {
+				if (vm_active_ch[i] > ch_num) {
+					dev_err(dev,
+						"invalid active channels: %u\n",
+						vm_active_ch[i]);
+					return -EINVAL;
+				}
+
+				pvt->vm_ch_total += vm_active_ch[i];
+
+				if (vm_active_ch[i] > pvt->vm_ch_max)
+					pvt->vm_ch_max = vm_active_ch[i];
+			}
+		}
+
+		/*
+		 * Map between the channel-number to VM-index and channel-index.
+		 * Example - 3 VMs, vm_active_ch = [05 02 04]:
+		 * vm_map = [0 0 0 0 0 1 1 2 2 2 2]
+		 * ch_map = [0 1 2 3 4 0 1 0 1 2 3]
+		 */
+		pvt->vd = devm_kcalloc(dev, pvt->vm_ch_total, sizeof(*pvt->vd),
+				       GFP_KERNEL);
+		if (!pvt->vd)
+			return -ENOMEM;
+
+		k = 0;
+		for (i = 0; i < vm_num; i++)
+			for (j = 0; j < vm_active_ch[i]; j++) {
+				pvt->vd[k].vm_map = vm_idx[i];
+				pvt->vd[k].ch_map = j;
+				k++;
+			}
+
+		in_config = devm_kcalloc(dev, pvt->vm_ch_total + 1,
 					 sizeof(*in_config), GFP_KERNEL);
 		if (!in_config)
 			return -ENOMEM;
 
-		memset32(in_config, HWMON_I_INPUT, total_ch);
-		in_config[total_ch] = 0;
+		memset32(in_config, HWMON_I_INPUT, pvt->vm_ch_total);
+		in_config[pvt->vm_ch_total] = 0;
 		pvt_in.config = in_config;
 
 		pvt_info[index++] = &pvt_in;
-- 
2.37.1

[PATCH v2 09/16] hwmon: (mr75203) add VM pre-scalar property for Moortec PVT controller

[Eliav Farber]

vm-pre-scalar-ch# is a per channel optional parameter that can be
used to normalzie the voltage output results.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 .../devicetree/bindings/hwmon/moortec,mr75203.yaml        | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index 6111b5069b3c..e2a55001eefc 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -56,6 +56,12 @@ properties:
       A value of 0 means that the entire VM sensor is nou used.
     $ref: /schemas/types.yaml#definitions/uint8-array
 
+  vm-pre-scalar-ch#:
+    description:
+      vm-active-channels defines the pre-scalar per channel value
+      used to normalzie the voltage output results.
+    $ref: /schemas/types.yaml#definitions/uint32
+
 required:
   - compatible
   - reg
@@ -68,6 +74,7 @@ additionalProperties:
   - intel,vm-map
   - reset-control-skip
   - vm-active-channels
+  - vm-pre-scalar-ch#
 
 examples:
   - |
@@ -82,5 +89,6 @@ examples:
         clocks = <&osc0>;
         resets = <&rcu0 0x40 7>;
         vm-active-channels = [08 10 02];
+        vm-pre-scalar-ch5 = <2>;
         #thermal-sensor-cells = <1>;
     };
-- 
2.37.1

Re: [PATCH v2 09/16] hwmon: (mr75203) add VM pre-scalar property for Moortec PVT controller

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:14AM +0000, Eliav Farber wrote:
> vm-pre-scalar-ch# is a per channel optional parameter that can be
> used to normalzie the voltage output results.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml        | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> index 6111b5069b3c..e2a55001eefc 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -56,6 +56,12 @@ properties:
>        A value of 0 means that the entire VM sensor is nou used.
>      $ref: /schemas/types.yaml#definitions/uint8-array
>  
> +  vm-pre-scalar-ch#:

Is that how such properties are implemented ? Seems to me that
results in a lot of decode complexity.

Why not use an array property like the other properties ?

Guenter

> +    description:
> +      vm-active-channels defines the pre-scalar per channel value
> +      used to normalzie the voltage output results.
> +    $ref: /schemas/types.yaml#definitions/uint32
> +
>  required:
>    - compatible
>    - reg
> @@ -68,6 +74,7 @@ additionalProperties:
>    - intel,vm-map
>    - reset-control-skip
>    - vm-active-channels
> +  - vm-pre-scalar-ch#
>  
>  examples:
>    - |
> @@ -82,5 +89,6 @@ examples:
>          clocks = <&osc0>;
>          resets = <&rcu0 0x40 7>;
>          vm-active-channels = [08 10 02];
> +        vm-pre-scalar-ch5 = <2>;
>          #thermal-sensor-cells = <1>;
>      };

Re: [PATCH v2 09/16] hwmon: (mr75203) add VM pre-scalar property for Moortec PVT controller

[Farber, Eliav]

On 8/18/2022 11:11 PM, Guenter Roeck wrote:
> Is that how such properties are implemented ? Seems to me that
> results in a lot of decode complexity.
>
> Why not use an array property like the other properties ?

Each VM has up to 16 inputs and there might be more than one VM.
Assuming an example of 2 VMs, and channels 5 and 6 in first VM have pre-
scalar of 2, while channel 2 in the second VM has pre-scalar of 3 and
channel 11 has pre-scalar of 2, the alternative was to do something like
this:
vm-pre-scalar-0=[01 01 01 01 01 02 02 01 01 01 01 01 01 01 01 01];
vm-pre-scalar-1=[01 01 03 01 01 01 01 01 01 01 01 02 01 01 01 01];
Most of the inputs are 01, which are anyway the default.
I don't see a difference in decoding complexity between the different
approaches but if you prefer this I'll modify my patches.

[PATCH v2 10/16] hwmon: (mr75203) add VM pre-scalar support

[Eliav Farber]

Add pre-scalar support to normalzie the voltage output results for
channels the use pre-scalar units to get the measurement to be within
the range that the sensor supports.
The pre-scalar value is used if it exists in device-tree, otherwise
default value of 1 is used.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 2e6139c09efc..24a00339cfd8 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -112,8 +112,12 @@
 struct voltage_device {
 	u8 vm_map;
 	u8 ch_map;
+	u32 pre_scaler;
 };
 
+#define PRE_SCALR_PROPERTY_NAME	32
+#define PRE_SCALR_DEFAULT_VAL	1
+
 struct pvt_device {
 	struct regmap		*c_map;
 	struct regmap		*t_map;
@@ -215,7 +219,9 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 
 		n &= SAMPLE_DATA_MSK;
 		/* Convert the N bitstream count into voltage */
-		*val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS;
+		*val = pvt->vd[channel].pre_scaler;
+		*val *= (PVT_N_CONST * n - PVT_R_CONST);
+		*val >>= PVT_CONV_BITS;
 
 		return 0;
 	default:
@@ -527,6 +533,7 @@ static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt
 static int mr75203_probe(struct platform_device *pdev)
 {
 	const struct hwmon_channel_info **pvt_info;
+	const struct device_node *np = pdev->dev.of_node;
 	u32 ts_num, vm_num, pd_num, ch_num, val, index, i, j, k;
 	struct device *dev = &pdev->dev;
 	u32 *temp_config, *in_config;
@@ -552,7 +559,7 @@ static int mr75203_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	if (of_property_read_bool(dev->of_node, "reset-control-skip")) {
+	if (of_property_read_bool(np, "reset-control-skip")) {
 		dev_info(dev, "skipping reset-control\n");
 	} else {
 		pvt->rst = devm_reset_control_get_exclusive(dev, NULL);
@@ -615,6 +622,8 @@ static int mr75203_probe(struct platform_device *pdev)
 	if (vm_num) {
 		u8 vm_idx[VM_NUM_MAX];
 		u8 vm_active_ch[VM_NUM_MAX];
+		char prop_name[PRE_SCALR_PROPERTY_NAME] = {0};
+		u32 pre_scaler;
 
 		ret = pvt_get_regmap(pdev, "vm", pvt);
 		if (ret)
@@ -686,6 +695,21 @@ static int mr75203_probe(struct platform_device *pdev)
 				k++;
 			}
 
+		/*
+		 * Incase vm-pre-scalar-ch# property is not defined, we assume
+		 * default pre-scaler of 1.
+		 */
+		for (i = 0; i < pvt->vm_ch_total; i++) {
+			snprintf(prop_name, sizeof(prop_name),
+				 "vm-pre-scalar-ch%u", i);
+
+			ret = of_property_read_u32(np, prop_name, &pre_scaler);
+			if (ret)
+				pvt->vd[i].pre_scaler = PRE_SCALR_DEFAULT_VAL;
+			else
+				pvt->vd[i].pre_scaler = pre_scaler;
+		}
+
 		in_config = devm_kcalloc(dev, pvt->vm_ch_total + 1,
 					 sizeof(*in_config), GFP_KERNEL);
 		if (!in_config)
-- 
2.37.1

[PATCH v2 11/16] hwmon: (mr75203) add protection for negative voltage value

[Eliav Farber]

This change makes sure the returned voltage vlaue is 0 or positive.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 24a00339cfd8..e3191f590167 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -218,6 +218,13 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 			return ret;
 
 		n &= SAMPLE_DATA_MSK;
+
+		/* Voltage can't be negative */
+		if (PVT_N_CONST * n < PVT_R_CONST) {
+			*val = 0;
+			return 0;
+		}
+
 		/* Convert the N bitstream count into voltage */
 		*val = pvt->vd[channel].pre_scaler;
 		*val *= (PVT_N_CONST * n - PVT_R_CONST);
-- 
2.37.1

Re: [PATCH v2 11/16] hwmon: (mr75203) add protection for negative voltage value

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:16AM +0000, Eliav Farber wrote:
> This change makes sure the returned voltage vlaue is 0 or positive.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  drivers/hwmon/mr75203.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index 24a00339cfd8..e3191f590167 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -218,6 +218,13 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
>  			return ret;
>  
>  		n &= SAMPLE_DATA_MSK;
> +
> +		/* Voltage can't be negative */

Who says, and what does that mean ? Under which conditions would
the value be negative, and why would that be a problem / bug ?
After all, negative voltages do exist.

Guenter

> +		if (PVT_N_CONST * n < PVT_R_CONST) {
> +			*val = 0;
> +			return 0;
> +		}
> +
>  		/* Convert the N bitstream count into voltage */
>  		*val = pvt->vd[channel].pre_scaler;
>  		*val *= (PVT_N_CONST * n - PVT_R_CONST);

[PATCH v2 12/16] hwmon: (mr75203) modify the temperature equation

[Eliav Farber]

Modify the equation and coefficients to convert the digital output to
temperature according to series 5 of the Moortec Embedded Temperature
Sensor (METS) datasheet:
T = G + H * (n / cal5 - 0.5) + J * F

The G, H and J coefficients are multiplied by 1000 to get the temperature
in milli-Celsius.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index e3191f590167..e500897585e4 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -102,9 +102,10 @@
 
 #define PVT_POLL_DELAY_US	20
 #define PVT_POLL_TIMEOUT_US	20000
-#define PVT_H_CONST		100000
-#define PVT_CAL5_CONST		2047
-#define PVT_G_CONST		40000
+#define PVT_H_CONST		60000
+#define PVT_G_CONST		200000
+#define PVT_J_CONST		-100
+#define PVT_CAL5_CONST		4094
 #define PVT_CONV_BITS		10
 #define PVT_N_CONST		90
 #define PVT_R_CONST		245805
@@ -158,7 +159,6 @@ static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
 	struct regmap *t_map = pvt->t_map;
 	u32 stat, nbs;
 	int ret;
-	u64 tmp;
 
 	switch (attr) {
 	case hwmon_temp_input:
@@ -176,12 +176,13 @@ static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
 		nbs &= SAMPLE_DATA_MSK;
 
 		/*
-		 * Convert the register value to
-		 * degrees centigrade temperature
+		 * Convert the register value to degrees centigrade temperature:
+		 * T = G + H * (n / cal5 - 0.5) + J * F
 		 */
-		tmp = nbs * PVT_H_CONST;
-		do_div(tmp, PVT_CAL5_CONST);
-		*val = tmp - PVT_G_CONST - pvt->ip_freq;
+		*val = PVT_G_CONST;
+		*val += PVT_H_CONST * nbs / PVT_CAL5_CONST;
+		*val -= PVT_H_CONST / 2;
+		*val += PVT_J_CONST * pvt->ip_freq / HZ_PER_MHZ;
 
 		return 0;
 	default:
@@ -313,7 +314,7 @@ static int pvt_init(struct pvt_device *pvt)
 		    (key >> 1) << CLK_SYNTH_HI_SFT |
 		    (key >> 1) << CLK_SYNTH_HOLD_SFT | CLK_SYNTH_EN;
 
-	pvt->ip_freq = sys_freq * 100 / (key + 2);
+	pvt->ip_freq = clk_get_rate(pvt->clk) / (key + 2);
 
 	if (t_num) {
 		ret = regmap_write(t_map, SDIF_SMPL_CTRL, 0x0);
-- 
2.37.1

Re: [PATCH v2 12/16] hwmon: (mr75203) modify the temperature equation

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:17AM +0000, Eliav Farber wrote:
> Modify the equation and coefficients to convert the digital output to
> temperature according to series 5 of the Moortec Embedded Temperature
> Sensor (METS) datasheet:
> T = G + H * (n / cal5 - 0.5) + J * F
> 
> The G, H and J coefficients are multiplied by 1000 to get the temperature
> in milli-Celsius.
> 

This is, at the very least, confusing. It doesn't explain the discrepancy
to the old code nor the change in constant values. I have no idea if this
change would result in erroneous readings on some other system where
the existing calculation may be the correct one.

On top of that, it seems overflow-prune in 32 bit systems.

Guenter

> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  drivers/hwmon/mr75203.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index e3191f590167..e500897585e4 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -102,9 +102,10 @@
>  
>  #define PVT_POLL_DELAY_US	20
>  #define PVT_POLL_TIMEOUT_US	20000
> -#define PVT_H_CONST		100000
> -#define PVT_CAL5_CONST		2047
> -#define PVT_G_CONST		40000
> +#define PVT_H_CONST		60000
> +#define PVT_G_CONST		200000
> +#define PVT_J_CONST		-100
> +#define PVT_CAL5_CONST		4094
>  #define PVT_CONV_BITS		10
>  #define PVT_N_CONST		90
>  #define PVT_R_CONST		245805
> @@ -158,7 +159,6 @@ static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
>  	struct regmap *t_map = pvt->t_map;
>  	u32 stat, nbs;
>  	int ret;
> -	u64 tmp;
>  
>  	switch (attr) {
>  	case hwmon_temp_input:
> @@ -176,12 +176,13 @@ static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
>  		nbs &= SAMPLE_DATA_MSK;
>  
>  		/*
> -		 * Convert the register value to
> -		 * degrees centigrade temperature
> +		 * Convert the register value to degrees centigrade temperature:
> +		 * T = G + H * (n / cal5 - 0.5) + J * F
>  		 */
> -		tmp = nbs * PVT_H_CONST;
> -		do_div(tmp, PVT_CAL5_CONST);
> -		*val = tmp - PVT_G_CONST - pvt->ip_freq;
> +		*val = PVT_G_CONST;
> +		*val += PVT_H_CONST * nbs / PVT_CAL5_CONST;
> +		*val -= PVT_H_CONST / 2;
> +		*val += PVT_J_CONST * pvt->ip_freq / HZ_PER_MHZ;
>  
>  		return 0;
>  	default:
> @@ -313,7 +314,7 @@ static int pvt_init(struct pvt_device *pvt)
>  		    (key >> 1) << CLK_SYNTH_HI_SFT |
>  		    (key >> 1) << CLK_SYNTH_HOLD_SFT | CLK_SYNTH_EN;
>  
> -	pvt->ip_freq = sys_freq * 100 / (key + 2);
> +	pvt->ip_freq = clk_get_rate(pvt->clk) / (key + 2);
>  
>  	if (t_num) {
>  		ret = regmap_write(t_map, SDIF_SMPL_CTRL, 0x0);

Re: [PATCH v2 12/16] hwmon: (mr75203) modify the temperature equation

[Farber, Eliav]

On 8/18/2022 11:23 PM, Guenter Roeck wrote:
> On Wed, Aug 17, 2022 at 05:43:17AM +0000, Eliav Farber wrote:
>> Modify the equation and coefficients to convert the digital output to
>> temperature according to series 5 of the Moortec Embedded Temperature
>> Sensor (METS) datasheet:
>> T = G + H * (n / cal5 - 0.5) + J * F
>>
>> The G, H and J coefficients are multiplied by 1000 to get the 
>> temperature
>> in milli-Celsius.
>>
>
> This is, at the very least, confusing. It doesn't explain the discrepancy
> to the old code nor the change in constant values. I have no idea if this
> change would result in erroneous readings on some other system where
> the existing calculation may be the correct one.

When I tested the driver it was also not clear to me why the equation
and coefficients in the code don't match the specifications in the data
sheet.
I reached out to Maxlinear engineers (@rtanwar) and they also couldn't
explain the discrepancy.
After further correspondence I aligned both the equation and coefficients
in the driver code to the equation and coefficients defined in series 5
of the Moortec Embedded Temperature Sensor (METS) datasheet which they
provided.

> On top of that, it seems overflow-prune in 32 bit systems.

I'll check if it can overflow, and if it can I'll fix in next version.

--
Regards, Eliav

Re: [PATCH v2 12/16] hwmon: (mr75203) modify the temperature equation

[Guenter Roeck]

On Fri, Aug 19, 2022 at 10:44:06AM +0300, Farber, Eliav wrote:
> On 8/18/2022 11:23 PM, Guenter Roeck wrote:
> > On Wed, Aug 17, 2022 at 05:43:17AM +0000, Eliav Farber wrote:
> > > Modify the equation and coefficients to convert the digital output to
> > > temperature according to series 5 of the Moortec Embedded Temperature
> > > Sensor (METS) datasheet:
> > > T = G + H * (n / cal5 - 0.5) + J * F
> > > 
> > > The G, H and J coefficients are multiplied by 1000 to get the
> > > temperature
> > > in milli-Celsius.
> > > 
> > 
> > This is, at the very least, confusing. It doesn't explain the discrepancy
> > to the old code nor the change in constant values. I have no idea if this
> > change would result in erroneous readings on some other system where
> > the existing calculation may be the correct one.
> 
> When I tested the driver it was also not clear to me why the equation
> and coefficients in the code don't match the specifications in the data
> sheet.
> I reached out to Maxlinear engineers (@rtanwar) and they also couldn't
> explain the discrepancy.
> After further correspondence I aligned both the equation and coefficients
> in the driver code to the equation and coefficients defined in series 5
> of the Moortec Embedded Temperature Sensor (METS) datasheet which they
> provided.
> 

At least some of the discrepancy is because the original code is more
optimized and avoids overflow. Either case, the above needs to be explained
in the commit description.

> > On top of that, it seems overflow-prune in 32 bit systems.
> 
> I'll check if it can overflow, and if it can I'll fix in next version.
> 
> --
> Regards, Eliav
> 

[PATCH v2 13/16] hwmon: (mr75203) add thermal coefficient properties for Moortec PVT controller

[Eliav Farber]

Add optional "ts-coeff-g", "ts-coeff-h", "ts-coeff-cal5" and
"ts-coeff-j" properties to be used instead of defaults for the
thermal equasion.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 .../bindings/hwmon/moortec,mr75203.yaml       | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index e2a55001eefc..867664bd937f 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -62,6 +62,30 @@ properties:
       used to normalzie the voltage output results.
     $ref: /schemas/types.yaml#definitions/uint32
 
+  ts-coeff-g:
+    description:
+      G coefficient for thermal equation.
+    maxItems: 1
+    $ref: /schemas/types.yaml#definitions/uint32
+
+  ts-coeff-h:
+    description:
+      H coefficient for thermal equation.
+    maxItems: 1
+    $ref: /schemas/types.yaml#definitions/uint32
+
+  ts-coeff-cal5:
+    description:
+      cal5 coefficient for thermal equation (can't be 0).
+    maxItems: 1
+    $ref: /schemas/types.yaml#definitions/uint32
+
+  ts-coeff-j:
+    description:
+      J coefficient for thermal equation.
+    maxItems: 1
+    $ref: /schemas/types.yaml#definitions/int32
+
 required:
   - compatible
   - reg
@@ -75,6 +99,10 @@ additionalProperties:
   - reset-control-skip
   - vm-active-channels
   - vm-pre-scalar-ch#
+  - ts-coeff-g
+  - ts-coeff-h
+  - ts-coeff-cal5
+  - ts-coeff-j
 
 examples:
   - |
@@ -90,5 +118,10 @@ examples:
         resets = <&rcu0 0x40 7>;
         vm-active-channels = [08 10 02];
         vm-pre-scalar-ch5 = <2>;
+        ts-coeff-g = <57400>;
+        ts-coeff-h = <249400>;
+        ts-coeff-cal5 = <4096>;
+        ts-coeff-j = <0>;
+
         #thermal-sensor-cells = <1>;
     };
-- 
2.37.1

Re: [PATCH v2 13/16] hwmon: (mr75203) add thermal coefficient properties for Moortec PVT controller

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:18AM +0000, Eliav Farber wrote:
> Add optional "ts-coeff-g", "ts-coeff-h", "ts-coeff-cal5" and
> "ts-coeff-j" properties to be used instead of defaults for the
> thermal equasion.
> 
Vendor prefix again, and shouldn;t there be some note about the
to-be-used defaults ?

Guenter

> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  .../bindings/hwmon/moortec,mr75203.yaml       | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> index e2a55001eefc..867664bd937f 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -62,6 +62,30 @@ properties:
>        used to normalzie the voltage output results.
>      $ref: /schemas/types.yaml#definitions/uint32
>  
> +  ts-coeff-g:
> +    description:
> +      G coefficient for thermal equation.
> +    maxItems: 1
> +    $ref: /schemas/types.yaml#definitions/uint32
> +
> +  ts-coeff-h:
> +    description:
> +      H coefficient for thermal equation.
> +    maxItems: 1
> +    $ref: /schemas/types.yaml#definitions/uint32
> +
> +  ts-coeff-cal5:
> +    description:
> +      cal5 coefficient for thermal equation (can't be 0).
> +    maxItems: 1
> +    $ref: /schemas/types.yaml#definitions/uint32
> +
> +  ts-coeff-j:
> +    description:
> +      J coefficient for thermal equation.
> +    maxItems: 1
> +    $ref: /schemas/types.yaml#definitions/int32
> +
>  required:
>    - compatible
>    - reg
> @@ -75,6 +99,10 @@ additionalProperties:
>    - reset-control-skip
>    - vm-active-channels
>    - vm-pre-scalar-ch#
> +  - ts-coeff-g
> +  - ts-coeff-h
> +  - ts-coeff-cal5
> +  - ts-coeff-j
>  
>  examples:
>    - |
> @@ -90,5 +118,10 @@ examples:
>          resets = <&rcu0 0x40 7>;
>          vm-active-channels = [08 10 02];
>          vm-pre-scalar-ch5 = <2>;
> +        ts-coeff-g = <57400>;
> +        ts-coeff-h = <249400>;
> +        ts-coeff-cal5 = <4096>;
> +        ts-coeff-j = <0>;
> +
>          #thermal-sensor-cells = <1>;
>      };

Re: [PATCH v2 13/16] hwmon: (mr75203) add thermal coefficient properties for Moortec PVT controller

[Farber, Eliav]

On 8/18/2022 11:25 PM, Guenter Roeck wrote:
> On Wed, Aug 17, 2022 at 05:43:18AM +0000, Eliav Farber wrote:
>> Add optional "ts-coeff-g", "ts-coeff-h", "ts-coeff-cal5" and
>> "ts-coeff-j" properties to be used instead of defaults for the
>> thermal equasion.
>>
> Vendor prefix again, and shouldn;t there be some note about the
> to-be-used defaults ?
Can you please explain why to add a vendor prefix to these properties
(and also to all other properties I added in this series)?
All the properties I added are not specific to our SOC, and any other
vendor using the same mr75203 ip block in their SOC, can also use the new
properties.

Regarding defaults, these properties are optional, so if they are absent
in device tree, the current defaults in code are used.

--
Thanks, Eliav

Re: [PATCH v2 13/16] hwmon: (mr75203) add thermal coefficient properties for Moortec PVT controller

[Guenter Roeck]

On Mon, Aug 22, 2022 at 04:24:20PM +0300, Farber, Eliav wrote:
> On 8/18/2022 11:25 PM, Guenter Roeck wrote:
> > On Wed, Aug 17, 2022 at 05:43:18AM +0000, Eliav Farber wrote:
> > > Add optional "ts-coeff-g", "ts-coeff-h", "ts-coeff-cal5" and
> > > "ts-coeff-j" properties to be used instead of defaults for the
> > > thermal equasion.
> > > 
> > Vendor prefix again, and shouldn;t there be some note about the
> > to-be-used defaults ?
> Can you please explain why to add a vendor prefix to these properties
> (and also to all other properties I added in this series)?
> All the properties I added are not specific to our SOC, and any other
> vendor using the same mr75203 ip block in their SOC, can also use the new
> properties.

To me it seems that the properties are very mr75203 and thus vendor
specific.

However, I am not a DT maintainer. Stepping back to let DT maintainers
respond and state their preferences.

> 
> Regarding defaults, these properties are optional, so if they are absent
> in device tree, the current defaults in code are used.
> 
Same as before. Question was if DT should say what the defaults are.
I'll leave it up to DT maintainers.

Thanks,
Guenter

Re: [PATCH v2 13/16] hwmon: (mr75203) add thermal coefficient properties for Moortec PVT controller

[Farber, Eliav]

On 8/22/2022 7:25 PM, Guenter Roeck wrote:
> On Mon, Aug 22, 2022 at 04:24:20PM +0300, Farber, Eliav wrote:
>> On 8/18/2022 11:25 PM, Guenter Roeck wrote:
>> > On Wed, Aug 17, 2022 at 05:43:18AM +0000, Eliav Farber wrote:
>> > > Add optional "ts-coeff-g", "ts-coeff-h", "ts-coeff-cal5" and
>> > > "ts-coeff-j" properties to be used instead of defaults for the
>> > > thermal equasion.
>> > >
>> > Vendor prefix again, and shouldn;t there be some note about the
>> > to-be-used defaults ?
>> Can you please explain why to add a vendor prefix to these properties
>> (and also to all other properties I added in this series)?
>> All the properties I added are not specific to our SOC, and any other
>> vendor using the same mr75203 ip block in their SOC, can also use the 
>> new
>> properties.
>
> To me it seems that the properties are very mr75203 and thus vendor
> specific.
>
> However, I am not a DT maintainer. Stepping back to let DT maintainers
> respond and state their preferences.

I added "moortec" prefix to all the new properties I added in this
series.
Will be part of v3.

--
Thanks, Eliav

[PATCH v2 14/16] hwmon: (mr75203) parse thermal coefficients from device-tree

[Eliav Farber]

Use thermal coefficients from the device tree if they exist.
Otherwise, use default values.

The equation used in the driver is:
  T = G + H * (n / cal5 - 0.5) + J * F

With this change we can support also Mode 1 Conversion, which
uses A instead of G, and B instead of H.

We can also support the series 6 equation that has different
coefficients and has a slightly different format:
  T = G + H * (n / cal5 - 0.5)
by setting J to 0.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 44 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index e500897585e4..e54a4d1803e4 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -131,6 +131,10 @@ struct pvt_device {
 	u32			p_num;
 	u32			v_num;
 	u32			ip_freq;
+	u32			ts_coeff_h;
+	u32			ts_coeff_g;
+	s32			ts_coeff_j;
+	u32			ts_coeff_cal5;
 	u8			vm_ch_max;
 	u8			vm_ch_total;
 };
@@ -179,10 +183,10 @@ static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
 		 * Convert the register value to degrees centigrade temperature:
 		 * T = G + H * (n / cal5 - 0.5) + J * F
 		 */
-		*val = PVT_G_CONST;
-		*val += PVT_H_CONST * nbs / PVT_CAL5_CONST;
-		*val -= PVT_H_CONST / 2;
-		*val += PVT_J_CONST * pvt->ip_freq / HZ_PER_MHZ;
+		*val = pvt->ts_coeff_g;
+		*val += pvt->ts_coeff_h * nbs / pvt->ts_coeff_cal5;
+		*val -= pvt->ts_coeff_h / 2;
+		*val += pvt->ts_coeff_j * pvt->ip_freq / HZ_PER_MHZ;
 
 		return 0;
 	default:
@@ -619,6 +623,38 @@ static int mr75203_probe(struct platform_device *pdev)
 		memset32(temp_config, HWMON_T_INPUT, ts_num);
 		pvt_temp.config = temp_config;
 		pvt_info[index++] = &pvt_temp;
+
+		/*
+		 * Incase ts-coeff-h/g/j/cal5 property is not defined, use
+		 * default value.
+		 */
+		ret = of_property_read_u32(np, "ts-coeff-h", &pvt->ts_coeff_h);
+		if (ret)
+			pvt->ts_coeff_h = PVT_H_CONST;
+
+		ret = of_property_read_u32(np, "ts-coeff-g", &pvt->ts_coeff_g);
+		if (ret)
+			pvt->ts_coeff_g = PVT_G_CONST;
+
+		ret = of_property_read_s32(np, "ts-coeff-j", &pvt->ts_coeff_j);
+		if (ret)
+			pvt->ts_coeff_j = PVT_J_CONST;
+
+		ret = of_property_read_u32(np, "ts-coeff-cal5",
+					   &pvt->ts_coeff_cal5);
+		if (ret) {
+			pvt->ts_coeff_cal5 = PVT_CAL5_CONST;
+		} else {
+			if (pvt->ts_coeff_cal5 == 0) {
+				dev_err(dev, "invalid ts-coeff-cal5 (%u)\n",
+					pvt->ts_coeff_cal5);
+				return -EINVAL;
+			}
+		}
+
+		dev_dbg(dev, "ts-coeff: h = %u, g = %u, j = %d, cal5 = %u\n",
+			pvt->ts_coeff_h, pvt->ts_coeff_g, pvt->ts_coeff_j,
+			pvt->ts_coeff_cal5);
 	}
 
 	if (pd_num) {
-- 
2.37.1

Re: [PATCH v2 14/16] hwmon: (mr75203) parse thermal coefficients from device-tree

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:19AM +0000, Eliav Farber wrote:
> Use thermal coefficients from the device tree if they exist.
> Otherwise, use default values.
> 
> The equation used in the driver is:
>   T = G + H * (n / cal5 - 0.5) + J * F
> 
> With this change we can support also Mode 1 Conversion, which
> uses A instead of G, and B instead of H.
> 
> We can also support the series 6 equation that has different
> coefficients and has a slightly different format:
>   T = G + H * (n / cal5 - 0.5)
> by setting J to 0.
> 

The calculation was just changed to use new defaults in a previous
patch. This patch makes it quite clear that the coefficients
are implementation (?) dependent. So the previous patch just changes
the defaults to (presumably) the coefficients used in your system.
That is inappropriate. Adding non-default corefficients is ok
and makes sense is supported by the chip, but changing defaults
isn't.

Guenter

> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  drivers/hwmon/mr75203.c | 44 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index e500897585e4..e54a4d1803e4 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -131,6 +131,10 @@ struct pvt_device {
>  	u32			p_num;
>  	u32			v_num;
>  	u32			ip_freq;
> +	u32			ts_coeff_h;
> +	u32			ts_coeff_g;
> +	s32			ts_coeff_j;
> +	u32			ts_coeff_cal5;
>  	u8			vm_ch_max;
>  	u8			vm_ch_total;
>  };
> @@ -179,10 +183,10 @@ static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
>  		 * Convert the register value to degrees centigrade temperature:
>  		 * T = G + H * (n / cal5 - 0.5) + J * F
>  		 */
> -		*val = PVT_G_CONST;
> -		*val += PVT_H_CONST * nbs / PVT_CAL5_CONST;
> -		*val -= PVT_H_CONST / 2;
> -		*val += PVT_J_CONST * pvt->ip_freq / HZ_PER_MHZ;
> +		*val = pvt->ts_coeff_g;
> +		*val += pvt->ts_coeff_h * nbs / pvt->ts_coeff_cal5;
> +		*val -= pvt->ts_coeff_h / 2;
> +		*val += pvt->ts_coeff_j * pvt->ip_freq / HZ_PER_MHZ;
>  
>  		return 0;
>  	default:
> @@ -619,6 +623,38 @@ static int mr75203_probe(struct platform_device *pdev)
>  		memset32(temp_config, HWMON_T_INPUT, ts_num);
>  		pvt_temp.config = temp_config;
>  		pvt_info[index++] = &pvt_temp;
> +
> +		/*
> +		 * Incase ts-coeff-h/g/j/cal5 property is not defined, use
> +		 * default value.
> +		 */
> +		ret = of_property_read_u32(np, "ts-coeff-h", &pvt->ts_coeff_h);
> +		if (ret)
> +			pvt->ts_coeff_h = PVT_H_CONST;
> +
> +		ret = of_property_read_u32(np, "ts-coeff-g", &pvt->ts_coeff_g);
> +		if (ret)
> +			pvt->ts_coeff_g = PVT_G_CONST;
> +
> +		ret = of_property_read_s32(np, "ts-coeff-j", &pvt->ts_coeff_j);
> +		if (ret)
> +			pvt->ts_coeff_j = PVT_J_CONST;
> +
> +		ret = of_property_read_u32(np, "ts-coeff-cal5",
> +					   &pvt->ts_coeff_cal5);
> +		if (ret) {
> +			pvt->ts_coeff_cal5 = PVT_CAL5_CONST;
> +		} else {
> +			if (pvt->ts_coeff_cal5 == 0) {
> +				dev_err(dev, "invalid ts-coeff-cal5 (%u)\n",
> +					pvt->ts_coeff_cal5);
> +				return -EINVAL;
> +			}
> +		}
> +
> +		dev_dbg(dev, "ts-coeff: h = %u, g = %u, j = %d, cal5 = %u\n",
> +			pvt->ts_coeff_h, pvt->ts_coeff_g, pvt->ts_coeff_j,
> +			pvt->ts_coeff_cal5);
>  	}
>  
>  	if (pd_num) {

Re: [PATCH v2 14/16] hwmon: (mr75203) parse thermal coefficients from device-tree

[Farber, Eliav]

On 8/18/2022 11:28 PM, Guenter Roeck wrote:
> The calculation was just changed to use new defaults in a previous
> patch. This patch makes it quite clear that the coefficients
> are implementation (?) dependent. So the previous patch just changes
> the defaults to (presumably) the coefficients used in your system.
> That is inappropriate. Adding non-default corefficients is ok
> and makes sense is supported by the chip, but changing defaults
> isn't.
The calculation was changed in previous patch to match series 5 of the
Moortec Embedded Temperature Sensor (METS) datasheet.
In our SOC we use series 6 which has a slightly different equation and
different coefficients.
I did the changes in steps.
With this last change, both series 5 and 6 are supported, in addition to
calibrated vs. non-calibrated modes.
In addition the data sheet just recommends default values but they also
specifically mention that actual values might vary from product to product.

--
Regards, Eliav

Re: [PATCH v2 14/16] hwmon: (mr75203) parse thermal coefficients from device-tree

[Guenter Roeck]

On Fri, Aug 19, 2022 at 10:57:58AM +0300, Farber, Eliav wrote:
> On 8/18/2022 11:28 PM, Guenter Roeck wrote:
> > The calculation was just changed to use new defaults in a previous
> > patch. This patch makes it quite clear that the coefficients
> > are implementation (?) dependent. So the previous patch just changes
> > the defaults to (presumably) the coefficients used in your system.
> > That is inappropriate. Adding non-default corefficients is ok
> > and makes sense is supported by the chip, but changing defaults
> > isn't.
> The calculation was changed in previous patch to match series 5 of the
> Moortec Embedded Temperature Sensor (METS) datasheet.
> In our SOC we use series 6 which has a slightly different equation and
> different coefficients.
> I did the changes in steps.
> With this last change, both series 5 and 6 are supported, in addition to
> calibrated vs. non-calibrated modes.
> In addition the data sheet just recommends default values but they also
> specifically mention that actual values might vary from product to product.
> 
Please mention all this in the commit description.

Guenter

> --
> Regards, Eliav

Re: [PATCH v2 14/16] hwmon: (mr75203) parse thermal coefficients from device-tree

[Guenter Roeck]

On Fri, Aug 19, 2022 at 10:57:58AM +0300, Farber, Eliav wrote:
> On 8/18/2022 11:28 PM, Guenter Roeck wrote:
> > The calculation was just changed to use new defaults in a previous
> > patch. This patch makes it quite clear that the coefficients
> > are implementation (?) dependent. So the previous patch just changes
> > the defaults to (presumably) the coefficients used in your system.
> > That is inappropriate. Adding non-default corefficients is ok
> > and makes sense is supported by the chip, but changing defaults
> > isn't.
> The calculation was changed in previous patch to match series 5 of the
> Moortec Embedded Temperature Sensor (METS) datasheet.
> In our SOC we use series 6 which has a slightly different equation and
> different coefficients.

If the coefficients are different based on the series, it would probably
make sense to create a separate devicetree compatible property for series 6
instead or requiring the user to list the actual coefficients. Those can
still be present, but the code should be able to use the defaults for
each series.

Guenter

> I did the changes in steps.
> With this last change, both series 5 and 6 are supported, in addition to
> calibrated vs. non-calibrated modes.
> In addition the data sheet just recommends default values but they also
> specifically mention that actual values might vary from product to product.
> 
> --
> Regards, Eliav

Re: [PATCH v2 14/16] hwmon: (mr75203) parse thermal coefficients from device-tree

[Farber, Eliav]

On 8/19/2022 2:38 PM, Guenter Roeck wrote:
> On Fri, Aug 19, 2022 at 10:57:58AM +0300, Farber, Eliav wrote:
>> On 8/18/2022 11:28 PM, Guenter Roeck wrote:
>> > The calculation was just changed to use new defaults in a previous
>> > patch. This patch makes it quite clear that the coefficients
>> > are implementation (?) dependent. So the previous patch just changes
>> > the defaults to (presumably) the coefficients used in your system.
>> > That is inappropriate. Adding non-default corefficients is ok
>> > and makes sense is supported by the chip, but changing defaults
>> > isn't.
>> The calculation was changed in previous patch to match series 5 of the
>> Moortec Embedded Temperature Sensor (METS) datasheet.
>> In our SOC we use series 6 which has a slightly different equation and
>> different coefficients.
>
> If the coefficients are different based on the series, it would probably
> make sense to create a separate devicetree compatible property for 
> series 6
> instead or requiring the user to list the actual coefficients. Those can
> still be present, but the code should be able to use the defaults for
> each series. 
There is a different set of coefficients for series 5 and for series 6,
so it would make sense to add a single property (e.g. series) instead
of adding 4 properties, one for each coefficient.
But that would not always be enough.
The Moortec datasheet explicitly says that coefficients can vary between
product and product, and be different from the default values.
That is the situation in our SOC.
The coefficients we use are slightly different from the defaults for
series 6.
So just adding a single series property would not be enough, and we would
anyway want to have the option to specifically determine the coefficient
values.
Do you suggest to add both, also series and also coefficients? (and I can
fail the probe in case both are set, to avoid conflicts).

--
Thanks, Eliav

Re: [PATCH v2 14/16] hwmon: (mr75203) parse thermal coefficients from device-tree

[Guenter Roeck]

On Mon, Aug 22, 2022 at 04:41:07PM +0300, Farber, Eliav wrote:
> On 8/19/2022 2:38 PM, Guenter Roeck wrote:
> > On Fri, Aug 19, 2022 at 10:57:58AM +0300, Farber, Eliav wrote:
> > > On 8/18/2022 11:28 PM, Guenter Roeck wrote:
> > > > The calculation was just changed to use new defaults in a previous
> > > > patch. This patch makes it quite clear that the coefficients
> > > > are implementation (?) dependent. So the previous patch just changes
> > > > the defaults to (presumably) the coefficients used in your system.
> > > > That is inappropriate. Adding non-default corefficients is ok
> > > > and makes sense is supported by the chip, but changing defaults
> > > > isn't.
> > > The calculation was changed in previous patch to match series 5 of the
> > > Moortec Embedded Temperature Sensor (METS) datasheet.
> > > In our SOC we use series 6 which has a slightly different equation and
> > > different coefficients.
> > 
> > If the coefficients are different based on the series, it would probably
> > make sense to create a separate devicetree compatible property for
> > series 6
> > instead or requiring the user to list the actual coefficients. Those can
> > still be present, but the code should be able to use the defaults for
> > each series.
> There is a different set of coefficients for series 5 and for series 6,
> so it would make sense to add a single property (e.g. series) instead
> of adding 4 properties, one for each coefficient.
> But that would not always be enough.
> The Moortec datasheet explicitly says that coefficients can vary between
> product and product, and be different from the default values.
> That is the situation in our SOC.
> The coefficients we use are slightly different from the defaults for
> series 6.
> So just adding a single series property would not be enough, and we would
> anyway want to have the option to specifically determine the coefficient
> values.
> Do you suggest to add both, also series and also coefficients? (and I can
> fail the probe in case both are set, to avoid conflicts).
> 

It should not be necessary to provide explicit default values for any of the
series. Yes, default values can be overwritten with explicit coefficient
properties, but it should not be necessary to provide those if the defaults
are used. So I would expect separate compatible properties for each of the
supported series plus separate coefficient properties.

However, since we are discussiong DT properties here, I'll step back and let
DT maintainers decide.

Thanks,
Guenter

Re: [PATCH v2 14/16] hwmon: (mr75203) parse thermal coefficients from device-tree

[Farber, Eliav]

On 8/22/2022 7:31 PM, Guenter Roeck wrote:
> It should not be necessary to provide explicit default values for any 
> of the
> series. Yes, default values can be overwritten with explicit coefficient
> properties, but it should not be necessary to provide those if the 
> defaults
> are used. So I would expect separate compatible properties for each of 
> the
> supported series plus separate coefficient properties.
I added a "moortec,ts-series" so that user will not need to provide all
4 coefficients.
The values of the "moortec,ts-series" can be 5 (default) or 6.
I didn't do it as a compatible property because the the driver is for
the Moortec controller (mr75203) while series 5 or 6 are only relevant
for the thermal sensor (mr74137).

--
Thanks, Eliav

[PATCH v2 15/16] hwmon: (mr75203) fix coding style space errors

[Eliav Farber]

Fix: "ERROR: space required before the open parenthesis '('"

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index e54a4d1803e4..2898565afaab 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -174,7 +174,7 @@ static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
 			return ret;
 
 		ret = regmap_read(t_map, SDIF_DATA(channel), &nbs);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		nbs &= SAMPLE_DATA_MSK;
@@ -219,7 +219,7 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 			return ret;
 
 		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx, ch_idx), &n);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		n &= SAMPLE_DATA_MSK;
@@ -322,19 +322,19 @@ static int pvt_init(struct pvt_device *pvt)
 
 	if (t_num) {
 		ret = regmap_write(t_map, SDIF_SMPL_CTRL, 0x0);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_write(t_map, SDIF_HALT, 0x0);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_write(t_map, CLK_SYNTH, clk_synth);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_write(t_map, SDIF_DISABLE, 0x0);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
@@ -347,7 +347,7 @@ static int pvt_init(struct pvt_device *pvt)
 		val = CFG0_MODE_2 | CFG0_PARALLEL_OUT | CFG0_12_BIT |
 		      IP_CFG << SDIF_ADDR_SFT | SDIF_WRN_W | SDIF_PROG;
 		ret = regmap_write(t_map, SDIF_W, val);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
@@ -360,7 +360,7 @@ static int pvt_init(struct pvt_device *pvt)
 		val = POWER_DELAY_CYCLE_256 | IP_TMR << SDIF_ADDR_SFT |
 			      SDIF_WRN_W | SDIF_PROG;
 		ret = regmap_write(t_map, SDIF_W, val);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
@@ -374,39 +374,39 @@ static int pvt_init(struct pvt_device *pvt)
 		      IP_CTRL << SDIF_ADDR_SFT |
 		      SDIF_WRN_W | SDIF_PROG;
 		ret = regmap_write(t_map, SDIF_W, val);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 	}
 
 	if (p_num) {
 		ret = regmap_write(p_map, SDIF_HALT, 0x0);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_write(p_map, SDIF_DISABLE, BIT(p_num) - 1);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_write(p_map, CLK_SYNTH, clk_synth);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 	}
 
 	if (v_num) {
 		ret = regmap_write(v_map, SDIF_SMPL_CTRL, 0x0);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_write(v_map, SDIF_HALT, 0x0);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_write(v_map, CLK_SYNTH, clk_synth);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_write(v_map, SDIF_DISABLE, 0x0);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
@@ -434,7 +434,7 @@ static int pvt_init(struct pvt_device *pvt)
 		      CFG1_14_BIT | IP_CFG << SDIF_ADDR_SFT |
 		      SDIF_WRN_W | SDIF_PROG;
 		ret = regmap_write(v_map, SDIF_W, val);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
@@ -447,7 +447,7 @@ static int pvt_init(struct pvt_device *pvt)
 		val = POWER_DELAY_CYCLE_64 | IP_TMR << SDIF_ADDR_SFT |
 		      SDIF_WRN_W | SDIF_PROG;
 		ret = regmap_write(v_map, SDIF_W, val);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 
 		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
@@ -461,7 +461,7 @@ static int pvt_init(struct pvt_device *pvt)
 		      IP_CTRL << SDIF_ADDR_SFT |
 		      SDIF_WRN_W | SDIF_PROG;
 		ret = regmap_write(v_map, SDIF_W, val);
-		if(ret < 0)
+		if (ret < 0)
 			return ret;
 	}
 
@@ -586,7 +586,7 @@ static int mr75203_probe(struct platform_device *pdev)
 	}
 
 	ret = regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
-	if(ret < 0)
+	if (ret < 0)
 		return ret;
 
 	ts_num = (val & TS_NUM_MSK) >> TS_NUM_SFT;
-- 
2.37.1

[PATCH v2 16/16] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Eliav Farber]

This change adds debugfs to read and write TS coefficients - g, h, j and
cal5.

The coefficients can vary between product and product, so to calibrate
them it can be very useful to to be able to modify them on the fly.

e.g.

cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
4096

echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/hwmon/mr75203.c | 196 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 196 insertions(+)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 2898565afaab..ce34a44237e8 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -9,6 +9,7 @@
  */
 #include <linux/bits.h>
 #include <linux/clk.h>
+#include <linux/debugfs.h>
 #include <linux/hwmon.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
@@ -127,6 +128,7 @@ struct pvt_device {
 	struct clk		*clk;
 	struct reset_control	*rst;
 	struct voltage_device	*vd;
+	struct dentry		*dbgfs_dir;
 	u32			t_num;
 	u32			p_num;
 	u32			v_num;
@@ -139,6 +141,198 @@ struct pvt_device {
 	u8			vm_ch_total;
 };
 
+static ssize_t pvt_ts_coeff_h_read(struct file *file,
+				   char __user *user_buf,
+				   size_t count, loff_t *ppos)
+{
+	struct pvt_device *pvt = file->private_data;
+	char buf[16];
+	unsigned int len;
+
+	len = sprintf(buf, "%u\n", pvt->ts_coeff_h);
+
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t pvt_ts_coeff_h_write(struct file *file,
+				    const char __user *user_buf,
+				    size_t count, loff_t *ppos)
+{
+	struct pvt_device *pvt = file->private_data;
+	int ret;
+	u32 coeff;
+
+	ret = kstrtou32_from_user(user_buf, count, 0, &coeff);
+	if (ret)
+		return ret;
+
+	pvt->ts_coeff_h = coeff;
+
+	return count;
+}
+
+static const struct file_operations pvt_ts_coeff_h_fops = {
+	.read = pvt_ts_coeff_h_read,
+	.write = pvt_ts_coeff_h_write,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
+static ssize_t pvt_ts_coeff_g_read(struct file *file,
+				   char __user *user_buf,
+				   size_t count, loff_t *ppos)
+{
+	struct pvt_device *pvt = file->private_data;
+	char buf[16];
+	unsigned int len;
+
+	len = sprintf(buf, "%u\n", pvt->ts_coeff_g);
+
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t pvt_ts_coeff_g_write(struct file *file,
+				    const char __user *user_buf,
+				    size_t count, loff_t *ppos)
+{
+	struct pvt_device *pvt = file->private_data;
+	int ret;
+	u32 coeff;
+
+	ret = kstrtou32_from_user(user_buf, count, 0, &coeff);
+	if (ret)
+		return ret;
+
+	pvt->ts_coeff_g = coeff;
+
+	return count;
+}
+
+static const struct file_operations pvt_ts_coeff_g_fops = {
+	.read = pvt_ts_coeff_g_read,
+	.write = pvt_ts_coeff_g_write,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
+static ssize_t pvt_ts_coeff_j_read(struct file *file,
+				   char __user *user_buf,
+				   size_t count, loff_t *ppos)
+{
+	struct pvt_device *pvt = file->private_data;
+	char buf[16];
+	unsigned int len;
+
+	len = sprintf(buf, "%d\n", pvt->ts_coeff_j);
+
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t pvt_ts_coeff_j_write(struct file *file,
+				    const char __user *user_buf,
+				    size_t count, loff_t *ppos)
+{
+	struct pvt_device *pvt = file->private_data;
+	int ret;
+	s32 coeff;
+
+	ret = kstrtos32_from_user(user_buf, count, 0, &coeff);
+	if (ret)
+		return ret;
+
+	pvt->ts_coeff_j = coeff;
+
+	return count;
+}
+
+static const struct file_operations pvt_ts_coeff_j_fops = {
+	.read = pvt_ts_coeff_j_read,
+	.write = pvt_ts_coeff_j_write,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
+static ssize_t pvt_ts_coeff_cal5_read(struct file *file,
+				      char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+	struct pvt_device *pvt = file->private_data;
+	char buf[16];
+	unsigned int len;
+
+	len = sprintf(buf, "%u\n", pvt->ts_coeff_cal5);
+
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t pvt_ts_coeff_cal5_write(struct file *file,
+				       const char __user *user_buf,
+				       size_t count, loff_t *ppos)
+{
+	struct pvt_device *pvt = file->private_data;
+	int ret;
+	u32 coeff;
+
+	ret = kstrtou32_from_user(user_buf, count, 0, &coeff);
+	if (ret)
+		return ret;
+
+	if (coeff == 0)
+		return -EINVAL;
+
+	pvt->ts_coeff_cal5 = coeff;
+
+	return count;
+}
+
+static const struct file_operations pvt_ts_coeff_cal5_fops = {
+	.read = pvt_ts_coeff_cal5_read,
+	.write = pvt_ts_coeff_cal5_write,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
+static void devm_pvt_ts_dbgfs_remove(void *data)
+{
+	struct pvt_device *pvt = (struct pvt_device *)data;
+
+	debugfs_remove_recursive(pvt->dbgfs_dir);
+	pvt->dbgfs_dir = NULL;
+}
+
+static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
+{
+	int ret;
+
+	pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
+	if (!pvt->dbgfs_dir) {
+		dev_err(dev, "Failed to create dbgfs_dir\n");
+		return -EINVAL;
+	}
+
+	debugfs_create_file("ts_coeff_h", 0644, pvt->dbgfs_dir, pvt,
+			    &pvt_ts_coeff_h_fops);
+	debugfs_create_file("ts_coeff_g", 0644, pvt->dbgfs_dir, pvt,
+			    &pvt_ts_coeff_g_fops);
+	debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt,
+			    &pvt_ts_coeff_j_fops);
+	debugfs_create_file("ts_coeff_cal5", 0644, pvt->dbgfs_dir,  pvt,
+			    &pvt_ts_coeff_cal5_fops);
+
+	ret = devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, pvt);
+	if (ret) {
+		dev_err(dev, "failed to add action to remove pvt dbgfs (%d)\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
 			      u32 attr, int channel)
 {
@@ -655,6 +849,8 @@ static int mr75203_probe(struct platform_device *pdev)
 		dev_dbg(dev, "ts-coeff: h = %u, g = %u, j = %d, cal5 = %u\n",
 			pvt->ts_coeff_h, pvt->ts_coeff_g, pvt->ts_coeff_j,
 			pvt->ts_coeff_cal5);
+
+		pvt_ts_dbgfs_create(pvt, dev);
 	}
 
 	if (pd_num) {
-- 
2.37.1

Re: [PATCH v2 16/16] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Guenter Roeck]

On Wed, Aug 17, 2022 at 05:43:21AM +0000, Eliav Farber wrote:
> This change adds debugfs to read and write TS coefficients - g, h, j and
> cal5.
> 
> The coefficients can vary between product and product, so to calibrate
> them it can be very useful to to be able to modify them on the fly.
> 
> e.g.
> 
> cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
> 4096
> 
> echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g
> 

What happens if you write 0 into all those attributes, or 0xffffffff ?

Guenter

> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  drivers/hwmon/mr75203.c | 196 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 196 insertions(+)
> 
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index 2898565afaab..ce34a44237e8 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -9,6 +9,7 @@
>   */
>  #include <linux/bits.h>
>  #include <linux/clk.h>
> +#include <linux/debugfs.h>
>  #include <linux/hwmon.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> @@ -127,6 +128,7 @@ struct pvt_device {
>  	struct clk		*clk;
>  	struct reset_control	*rst;
>  	struct voltage_device	*vd;
> +	struct dentry		*dbgfs_dir;
>  	u32			t_num;
>  	u32			p_num;
>  	u32			v_num;
> @@ -139,6 +141,198 @@ struct pvt_device {
>  	u8			vm_ch_total;
>  };
>  
> +static ssize_t pvt_ts_coeff_h_read(struct file *file,
> +				   char __user *user_buf,
> +				   size_t count, loff_t *ppos)
> +{
> +	struct pvt_device *pvt = file->private_data;
> +	char buf[16];
> +	unsigned int len;
> +
> +	len = sprintf(buf, "%u\n", pvt->ts_coeff_h);
> +
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}
> +
> +static ssize_t pvt_ts_coeff_h_write(struct file *file,
> +				    const char __user *user_buf,
> +				    size_t count, loff_t *ppos)
> +{
> +	struct pvt_device *pvt = file->private_data;
> +	int ret;
> +	u32 coeff;
> +
> +	ret = kstrtou32_from_user(user_buf, count, 0, &coeff);
> +	if (ret)
> +		return ret;
> +
> +	pvt->ts_coeff_h = coeff;
> +
> +	return count;
> +}
> +
> +static const struct file_operations pvt_ts_coeff_h_fops = {
> +	.read = pvt_ts_coeff_h_read,
> +	.write = pvt_ts_coeff_h_write,
> +	.open = simple_open,
> +	.owner = THIS_MODULE,
> +	.llseek = default_llseek,
> +};
> +
> +static ssize_t pvt_ts_coeff_g_read(struct file *file,
> +				   char __user *user_buf,
> +				   size_t count, loff_t *ppos)
> +{
> +	struct pvt_device *pvt = file->private_data;
> +	char buf[16];
> +	unsigned int len;
> +
> +	len = sprintf(buf, "%u\n", pvt->ts_coeff_g);
> +
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}
> +
> +static ssize_t pvt_ts_coeff_g_write(struct file *file,
> +				    const char __user *user_buf,
> +				    size_t count, loff_t *ppos)
> +{
> +	struct pvt_device *pvt = file->private_data;
> +	int ret;
> +	u32 coeff;
> +
> +	ret = kstrtou32_from_user(user_buf, count, 0, &coeff);
> +	if (ret)
> +		return ret;
> +
> +	pvt->ts_coeff_g = coeff;
> +
> +	return count;
> +}
> +
> +static const struct file_operations pvt_ts_coeff_g_fops = {
> +	.read = pvt_ts_coeff_g_read,
> +	.write = pvt_ts_coeff_g_write,
> +	.open = simple_open,
> +	.owner = THIS_MODULE,
> +	.llseek = default_llseek,
> +};
> +
> +static ssize_t pvt_ts_coeff_j_read(struct file *file,
> +				   char __user *user_buf,
> +				   size_t count, loff_t *ppos)
> +{
> +	struct pvt_device *pvt = file->private_data;
> +	char buf[16];
> +	unsigned int len;
> +
> +	len = sprintf(buf, "%d\n", pvt->ts_coeff_j);
> +
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}
> +
> +static ssize_t pvt_ts_coeff_j_write(struct file *file,
> +				    const char __user *user_buf,
> +				    size_t count, loff_t *ppos)
> +{
> +	struct pvt_device *pvt = file->private_data;
> +	int ret;
> +	s32 coeff;
> +
> +	ret = kstrtos32_from_user(user_buf, count, 0, &coeff);
> +	if (ret)
> +		return ret;
> +
> +	pvt->ts_coeff_j = coeff;
> +
> +	return count;
> +}
> +
> +static const struct file_operations pvt_ts_coeff_j_fops = {
> +	.read = pvt_ts_coeff_j_read,
> +	.write = pvt_ts_coeff_j_write,
> +	.open = simple_open,
> +	.owner = THIS_MODULE,
> +	.llseek = default_llseek,
> +};
> +
> +static ssize_t pvt_ts_coeff_cal5_read(struct file *file,
> +				      char __user *user_buf,
> +				      size_t count, loff_t *ppos)
> +{
> +	struct pvt_device *pvt = file->private_data;
> +	char buf[16];
> +	unsigned int len;
> +
> +	len = sprintf(buf, "%u\n", pvt->ts_coeff_cal5);
> +
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}
> +
> +static ssize_t pvt_ts_coeff_cal5_write(struct file *file,
> +				       const char __user *user_buf,
> +				       size_t count, loff_t *ppos)
> +{
> +	struct pvt_device *pvt = file->private_data;
> +	int ret;
> +	u32 coeff;
> +
> +	ret = kstrtou32_from_user(user_buf, count, 0, &coeff);
> +	if (ret)
> +		return ret;
> +
> +	if (coeff == 0)
> +		return -EINVAL;
> +
> +	pvt->ts_coeff_cal5 = coeff;
> +
> +	return count;
> +}
> +
> +static const struct file_operations pvt_ts_coeff_cal5_fops = {
> +	.read = pvt_ts_coeff_cal5_read,
> +	.write = pvt_ts_coeff_cal5_write,
> +	.open = simple_open,
> +	.owner = THIS_MODULE,
> +	.llseek = default_llseek,
> +};
> +
> +static void devm_pvt_ts_dbgfs_remove(void *data)
> +{
> +	struct pvt_device *pvt = (struct pvt_device *)data;
> +
> +	debugfs_remove_recursive(pvt->dbgfs_dir);
> +	pvt->dbgfs_dir = NULL;
> +}
> +
> +static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
> +{
> +	int ret;
> +
> +	pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
> +	if (!pvt->dbgfs_dir) {
> +		dev_err(dev, "Failed to create dbgfs_dir\n");
> +		return -EINVAL;
> +	}
> +
> +	debugfs_create_file("ts_coeff_h", 0644, pvt->dbgfs_dir, pvt,
> +			    &pvt_ts_coeff_h_fops);
> +	debugfs_create_file("ts_coeff_g", 0644, pvt->dbgfs_dir, pvt,
> +			    &pvt_ts_coeff_g_fops);
> +	debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt,
> +			    &pvt_ts_coeff_j_fops);
> +	debugfs_create_file("ts_coeff_cal5", 0644, pvt->dbgfs_dir,  pvt,
> +			    &pvt_ts_coeff_cal5_fops);
> +
> +	ret = devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, pvt);
> +	if (ret) {
> +		dev_err(dev, "failed to add action to remove pvt dbgfs (%d)\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
>  			      u32 attr, int channel)
>  {
> @@ -655,6 +849,8 @@ static int mr75203_probe(struct platform_device *pdev)
>  		dev_dbg(dev, "ts-coeff: h = %u, g = %u, j = %d, cal5 = %u\n",
>  			pvt->ts_coeff_h, pvt->ts_coeff_g, pvt->ts_coeff_j,
>  			pvt->ts_coeff_cal5);
> +
> +		pvt_ts_dbgfs_create(pvt, dev);
>  	}
>  
>  	if (pd_num) {

Re: [PATCH v2 16/16] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Farber, Eliav]

On 8/19/2022 2:11 AM, Guenter Roeck wrote:
> On Wed, Aug 17, 2022 at 05:43:21AM +0000, Eliav Farber wrote:
>> This change adds debugfs to read and write TS coefficients - g, h, j and
>> cal5.
>>
>> The coefficients can vary between product and product, so to calibrate
>> them it can be very useful to to be able to modify them on the fly.
>>
>> e.g.
>>
>> cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
>> 4096
>>
>> echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g
>>
>
> What happens if you write 0 into all those attributes, or 0xffffffff ?
The driver equation is:
T = G + H * (n / cal5 - 0.5) + J * F
So I added protection for cal5 not being 0.
Besides that there is no limitation on what these values can be.
I can't really think of any other logical limitation I can apply.

--
Regards, Eliav

Re: [PATCH v2 16/16] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Guenter Roeck]

On Mon, Aug 22, 2022 at 04:59:43PM +0300, Farber, Eliav wrote:
> On 8/19/2022 2:11 AM, Guenter Roeck wrote:
> > On Wed, Aug 17, 2022 at 05:43:21AM +0000, Eliav Farber wrote:
> > > This change adds debugfs to read and write TS coefficients - g, h, j and
> > > cal5.
> > > 
> > > The coefficients can vary between product and product, so to calibrate
> > > them it can be very useful to to be able to modify them on the fly.
> > > 
> > > e.g.
> > > 
> > > cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
> > > 4096
> > > 
> > > echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g
> > > 
> > 
> > What happens if you write 0 into all those attributes, or 0xffffffff ?
> The driver equation is:
> T = G + H * (n / cal5 - 0.5) + J * F
> So I added protection for cal5 not being 0.
> Besides that there is no limitation on what these values can be.
> I can't really think of any other logical limitation I can apply.
> 
There needs to be an overflow protection. I am quite sure that 0xffffffff
would result in overflows and thus in quite random reported values.

Thanks,
Guenter

> --
> Regards, Eliav

Re: [PATCH v2 16/16] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Farber, Eliav]

On 8/22/2022 7:28 PM, Guenter Roeck wrote:
> There needs to be an overflow protection. I am quite sure that 0xffffffff
> would result in overflows and thus in quite random reported values. 
Added overflow protection.
Will be part of v3.

--
Thanks, Eliav