[PATCH v3 00/19] Variety of fixes and new features for mr75203 driver

[PATCH v3 00/19] Variety of fixes and new features for mr75203 driver

[Eliav Farber]

List of fixes:
 - Fix "intel,vm-map" property to be optional.
 - Fix VM sensor allocation when "intel,vm-map" not defined.
 - Fix multi-channel voltage reading.
 - Fix voltage equation for negative source input.
 - Modify the temperature equation according to series 5 datasheet.
 - Fix coding style issue.

List of new features:
- Modify "reset" property to be optional.
- Add optional "moortec,vm-active-channels" property to define the number
  of active channels per VM.
- Add support for mr76006 pre-scaler to multiply the voltage result by 2.
- Add support for series 6 temperature equation.
- Add coefficient properties to fine tune the temperature equation.
- Add debugfs to read and write temperature coefficients

---------

Changes between v2 and v3:
*) Add "moortec" prefix to all new device-tree properties.
*) Change order of patches.
*) Add explanations to better understand the changes.
*) Change "reset" property to be optional and remove the
  "reset-control-skip" property.
*) Split the patch for "fix multi-channel voltage reading" to two
   patches.
*) Change pre-scaler property format and fix typo (scalar --> scaler).
*) Fix voltage equation to support negative values instead of limiting
   value to zero.
*) Temperature equation - protect from overflow and add clamping.
*) Add new "moortec,ts-series" property to select between temperature
   equation of series 5 or series 6.

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

---------

Eliav Farber (19):
  dt-bindings: hwmon: (mr75203) update "intel,vm-map" property to be
    optional
  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
  dt-bindings: hwmon: (mr75203) change "reset" property to be optional
  hwmon: (mr75203) skip reset-control deassert for SOCs that don't
    support it
  hwmon: (mr75203) fix multi-channel voltage reading
  hwmon: (mr75203) enable polling for all VM channels
  dt-bindings: hwmon: (mr75203) add "moortec,vm-active-channels"
    property
  hwmon: (mr75203) add VM active channel support
  dt-bindings: hwmon: (mr75203) add "moortec,vm-pre-scaler" property
  hwmon: (mr75203) add VM pre-scaler support
  hwmon: (mr75203) fix voltage equation for negative source input
  hwmon: (mr75203) modify the temperature equation according to series 5
    datasheet
  dt-bindings: hwmon: (mr75203) add "moortec,ts-series" property
  hwmon: (mr75203) add support for series 6 temperature equation
  dt-bindings: hwmon: (mr75203) add coefficient properties for the
    thermal equation
  hwmon: (mr75203) parse temperature coefficients from device-tree
  hwmon: (mr75203) add debugfs to read and write temperature
    coefficients
  hwmon: (mr75203) fix coding style space errors

 .../bindings/hwmon/moortec,mr75203.yaml       |  67 ++-
 drivers/hwmon/mr75203.c                       | 568 ++++++++++++++++--
 2 files changed, 569 insertions(+), 66 deletions(-)

-- 
2.37.1

[PATCH v3 01/19] dt-bindings: hwmon: (mr75203) update "intel,vm-map" property to be optional

[Eliav Farber]

Change "intel,vm-map" property to be optional instead of required.

The driver implementation indicates it is not mandatory to have
"intel,vm-map" in the device tree:
 - probe doesn't fail in case it is absent.
 - explicit comment in code - "Incase intel,vm-map property is not
   defined, we assume incremental channel numbers".

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- Change this patch to be first in the series.
- Add explanation why "intel,vm-map" is not required.

 Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml | 1 -
 1 file changed, 1 deletion(-)

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

Re: [PATCH v3 01/19] dt-bindings: hwmon: (mr75203) update "intel,vm-map" property to be optional

[Rob Herring]

On Tue, 30 Aug 2022 19:21:54 +0000, Eliav Farber wrote:
> Change "intel,vm-map" property to be optional instead of required.
> 
> The driver implementation indicates it is not mandatory to have
> "intel,vm-map" in the device tree:
>  - probe doesn't fail in case it is absent.
>  - explicit comment in code - "Incase intel,vm-map property is not
>    defined, we assume incremental channel numbers".
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> V3 -> V2:
> - Change this patch to be first in the series.
> - Add explanation why "intel,vm-map" is not required.
> 
>  Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml | 1 -
>  1 file changed, 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel,vm-map" not defined

[Eliav Farber]

Bug fix - in case "intel,vm-map" is missing in device-tree ,'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 v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel,vm-map" not defined

[Guenter Roeck]

On 8/30/22 12:21, Eliav Farber wrote:
> Bug fix - in case "intel,vm-map" is missing in device-tree ,'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;
> +		}
>   

So this is actually a functional change: In the old code, unspecified channel
numbers (num ... vm_num) were filled with incremental channel numbers.
This is no longer the case.

> -		in_config = devm_kcalloc(dev, num + 1,
> +		in_config = devm_kcalloc(dev, vm_num + 1,
>   					 sizeof(*in_config), GFP_KERNEL);

The relevant difference (and maybe bug in the existing code ?) seems to be
this change. Have you considered leaving everything else in place and only
changing this code as well as the num -> vm_num changes below ?

Thanks,
Guenter

>   		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 v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Farber, Eliav]

On 8/31/2022 5:39 AM, Guenter Roeck wrote:
> On 8/30/22 12:21, Eliav Farber wrote:
>> Bug fix - in case "intel,vm-map" is missing in device-tree ,'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;
>> +             }
>>
>
> So this is actually a functional change: In the old code, unspecified 
> channel
> numbers (num ... vm_num) were filled with incremental channel numbers.
> This is no longer the case.
>
The only place that uses pvt->vm_idx[] besides setting it in the probe
function is pvr_read_in().
It is quite clear from pvr_read_in() that unspecified channel numbers
(num ... vm_num) are not accessed, therefore there is also no need to
set them.

     if (channel >= pvt->v_num)
         return -EINVAL;

     vm_idx = pvt->vm_idx[channel];

Therefore I removed the setting of unspecified channels, and only if
“intel,vm-map” property is not defined, I set the specified channels
in incremental order.

>> -             in_config = devm_kcalloc(dev, num + 1,
>> +             in_config = devm_kcalloc(dev, vm_num + 1,
>>                                        sizeof(*in_config), GFP_KERNEL);
>
> The relevant difference (and maybe bug in the existing code ?) seems 
> to be
> this change. Have you considered leaving everything else in place and 
> only
> changing this code as well as the num -> vm_num changes below ?

Yes, using vm_num instead of num (which will be 0 if “intel,vm-map”
property is not defined) is the actual fix.
Both changes seemed to me to be coupled but if you think it will be
more clear to split this patch to two, first that removes unnecessary
setting of unspecified channels, and second that changes num to vm_num
for in_config, then sure I will do it.

--
Thanks, Eliav

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel,vm-map" not defined

[Guenter Roeck]

On 8/30/22 12:21, Eliav Farber wrote:
> Bug fix - in case "intel,vm-map" is missing in device-tree ,'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;

So all vm_idx values from 0x00 to 0xfe would be acceptable ?
Does the chip really have that many registers (0x200 + 0x40 + 0x200 * 0xfe) ?
Is that documented somewhere ?

Thanks,
Guenter

> -				}
> -		}
>   
> -		/*
> -		 * 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 v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Farber, Eliav]

On 8/31/2022 8:36 AM, Guenter Roeck wrote:
> On 8/30/22 12:21, Eliav Farber wrote:
>> Bug fix - in case "intel,vm-map" is missing in device-tree ,'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;
>
> So all vm_idx values from 0x00 to 0xfe would be acceptable ?
> Does the chip really have that many registers (0x200 + 0x40 + 0x200 * 
> 0xfe) ?
> Is that documented somewhere ? 
According to the code vm_num is limited to 32 because the mask is
only 5 bits:

#define VM_NUM_MSK    GENMASK(20, 16)
#define VM_NUM_SFT    16
vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;

In practice according to the data sheet I have:
0 <= VM instances <= 8

--
Regards, Eliav

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Farber, Eliav]

On 8/31/2022 8:49 AM, Farber, Eliav wrote:
> On 8/31/2022 8:36 AM, Guenter Roeck wrote:
>> On 8/30/22 12:21, Eliav Farber wrote:
>>> Bug fix - in case "intel,vm-map" is missing in device-tree ,'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;
>>
>> So all vm_idx values from 0x00 to 0xfe would be acceptable ?
>> Does the chip really have that many registers (0x200 + 0x40 + 0x200 * 
>> 0xfe) ?
>> Is that documented somewhere ? 
> According to the code vm_num is limited to 32 because the mask is
> only 5 bits:
>
For 5 bit mask, maximum is of course 31 and not 32.
> #define VM_NUM_MSK    GENMASK(20, 16)
> #define VM_NUM_SFT    16
> vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>
> In practice according to the data sheet I have:
> 0 <= VM instances <= 8
>
> -- 
> Regards, Eliav

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Guenter Roeck]

On 8/30/22 22:49, Farber, Eliav wrote:
> On 8/31/2022 8:36 AM, Guenter Roeck wrote:
>> On 8/30/22 12:21, Eliav Farber wrote:
>>> Bug fix - in case "intel,vm-map" is missing in device-tree ,'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;
>>
>> So all vm_idx values from 0x00 to 0xfe would be acceptable ?
>> Does the chip really have that many registers (0x200 + 0x40 + 0x200 * 0xfe) ?
>> Is that documented somewhere ? 
> According to the code vm_num is limited to 32 because the mask is
> only 5 bits:
> 
> #define VM_NUM_MSK    GENMASK(20, 16)
> #define VM_NUM_SFT    16
> vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
> 
> In practice according to the data sheet I have:
> 0 <= VM instances <= 8
> 
Sorry, my bad. I misread the patch and thought the first part of
the if statement was removed.

Anyway, what is the difference between specifying an vm_idx value of
0xff and not specifying anything ? Or, in other words, taking the dt
example, the difference between
	intel,vm-map = [03 01 04 ff ff];
and
	intel,vm-map = [03 01 04];

Thanks,
Guenter

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Farber, Eliav]

On 8/31/2022 2:48 PM, Guenter Roeck wrote:
> On 8/30/22 22:49, Farber, Eliav wrote:
>> On 8/31/2022 8:36 AM, Guenter Roeck wrote:
>>> On 8/30/22 12:21, Eliav Farber wrote:
>>>> Bug fix - in case "intel,vm-map" is missing in device-tree ,'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;
>>>
>>> So all vm_idx values from 0x00 to 0xfe would be acceptable ?
>>> Does the chip really have that many registers (0x200 + 0x40 + 0x200 
>>> * 0xfe) ?
>>> Is that documented somewhere ?
>> According to the code vm_num is limited to 32 because the mask is
>> only 5 bits:
>>
>> #define VM_NUM_MSK    GENMASK(20, 16)
>> #define VM_NUM_SFT    16
>> vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>>
>> In practice according to the data sheet I have:
>> 0 <= VM instances <= 8
>>
> Sorry, my bad. I misread the patch and thought the first part of
> the if statement was removed.
>
> Anyway, what is the difference between specifying an vm_idx value of
> 0xff and not specifying anything ? Or, in other words, taking the dt
> example, the difference between
>        intel,vm-map = [03 01 04 ff ff];
> and
>        intel,vm-map = [03 01 04]; 

The actual number of VMs is read from a HW register:
     ret = regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
     ...
     vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;

Also, using:
     ret = device_property_read_u8_array(dev, "intel,vm-map", vm_idx,
                         vm_num);
in the driver will fail if vm_num > sizeof array in device-tree.

So, if for example vm_num = 5, but you will want to map only 3 of them
you most set property to be:
     intel,vm-map = [03 01 04 ff ff];
otherwise if you set:
     intel,vm-map = [03 01 04];
it will assume the property doesn't, and will continue the flow in code
as if it doesn’t exist (which is not what the user wanted, and before my
fix also has a bug).

--
Regards, Eliav

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Guenter Roeck]

On Thu, Sep 01, 2022 at 11:39:58AM +0300, Farber, Eliav wrote:
> On 8/31/2022 2:48 PM, Guenter Roeck wrote:
> > On 8/30/22 22:49, Farber, Eliav wrote:
> > > On 8/31/2022 8:36 AM, Guenter Roeck wrote:
> > > > On 8/30/22 12:21, Eliav Farber wrote:
> > > > > Bug fix - in case "intel,vm-map" is missing in device-tree
> > > > > ,'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;
> > > > 
> > > > So all vm_idx values from 0x00 to 0xfe would be acceptable ?
> > > > Does the chip really have that many registers (0x200 + 0x40 +
> > > > 0x200 * 0xfe) ?
> > > > Is that documented somewhere ?
> > > According to the code vm_num is limited to 32 because the mask is
> > > only 5 bits:
> > > 
> > > #define VM_NUM_MSK    GENMASK(20, 16)
> > > #define VM_NUM_SFT    16
> > > vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
> > > 
> > > In practice according to the data sheet I have:
> > > 0 <= VM instances <= 8
> > > 
> > Sorry, my bad. I misread the patch and thought the first part of
> > the if statement was removed.
> > 
> > Anyway, what is the difference between specifying an vm_idx value of
> > 0xff and not specifying anything ? Or, in other words, taking the dt
> > example, the difference between
> >        intel,vm-map = [03 01 04 ff ff];
> > and
> >        intel,vm-map = [03 01 04];
> 
> The actual number of VMs is read from a HW register:
>     ret = regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
>     ...
>     vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
> 
> Also, using:
>     ret = device_property_read_u8_array(dev, "intel,vm-map", vm_idx,
>                         vm_num);
> in the driver will fail if vm_num > sizeof array in device-tree.
> 
> So, if for example vm_num = 5, but you will want to map only 3 of them
> you most set property to be:
>     intel,vm-map = [03 01 04 ff ff];
> otherwise if you set:
>     intel,vm-map = [03 01 04];
> it will assume the property doesn't, and will continue the flow in code
> as if it doesn’t exist (which is not what the user wanted, and before my
> fix also has a bug).

There should be some error handling to catch this case (ie if the number
of entries does not match the expected count), or if a value in the array
is larger or equal to vm_num. Today the latter is silently handled as end
of entries (similar to 0xff), but that should result in an error.
This would avoid situations like
	intel,vm-map = [01 02 03 04 05];
ie where the person writing the devicetree file accidentally entered
index values starting with 1 instead of 0. A mismatch between vm_num
and the number of entries in the array is silently handled as if there
was no property at all, which is at the very least misleading and
most definitely unexpected and should also result in an error.

Also, what happens if the devicetree content is something like the
following ? Would that be valid ?
	intel,vm-map = [00 01 01 01 01 01];

Thanks,
Guenter

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Farber, Eliav]

On 9/1/2022 5:44 PM, Guenter Roeck wrote:
> On Thu, Sep 01, 2022 at 11:39:58AM +0300, Farber, Eliav wrote:
>> On 8/31/2022 2:48 PM, Guenter Roeck wrote:
>> > On 8/30/22 22:49, Farber, Eliav wrote:
>> > > On 8/31/2022 8:36 AM, Guenter Roeck wrote:
>> > > > On 8/30/22 12:21, Eliav Farber wrote:
>> > > > > Bug fix - in case "intel,vm-map" is missing in device-tree
>> > > > > ,'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;
>> > > >
>> > > > So all vm_idx values from 0x00 to 0xfe would be acceptable ?
>> > > > Does the chip really have that many registers (0x200 + 0x40 +
>> > > > 0x200 * 0xfe) ?
>> > > > Is that documented somewhere ?
>> > > According to the code vm_num is limited to 32 because the mask is
>> > > only 5 bits:
>> > >
>> > > #define VM_NUM_MSK    GENMASK(20, 16)
>> > > #define VM_NUM_SFT    16
>> > > vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>> > >
>> > > In practice according to the data sheet I have:
>> > > 0 <= VM instances <= 8
>> > >
>> > Sorry, my bad. I misread the patch and thought the first part of
>> > the if statement was removed.
>> >
>> > Anyway, what is the difference between specifying an vm_idx value of
>> > 0xff and not specifying anything ? Or, in other words, taking the dt
>> > example, the difference between
>> >        intel,vm-map = [03 01 04 ff ff];
>> > and
>> >        intel,vm-map = [03 01 04];
>>
>> The actual number of VMs is read from a HW register:
>>     ret = regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
>>     ...
>>     vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>>
>> Also, using:
>>     ret = device_property_read_u8_array(dev, "intel,vm-map", vm_idx,
>>                         vm_num);
>> in the driver will fail if vm_num > sizeof array in device-tree.
>>
>> So, if for example vm_num = 5, but you will want to map only 3 of them
>> you most set property to be:
>>     intel,vm-map = [03 01 04 ff ff];
>> otherwise if you set:
>>     intel,vm-map = [03 01 04];
>> it will assume the property doesn't, and will continue the flow in code
>> as if it doesn’t exist (which is not what the user wanted, and before my
>> fix also has a bug).
>
> There should be some error handling to catch this case (ie if the number
> of entries does not match the expected count), or if a value in the array
> is larger or equal to vm_num. Today the latter is silently handled as end
> of entries (similar to 0xff), but that should result in an error.
> This would avoid situations like
>        intel,vm-map = [01 02 03 04 05];
> ie where the person writing the devicetree file accidentally entered
> index values starting with 1 instead of 0. A mismatch between vm_num
> and the number of entries in the array is silently handled as if there
> was no property at all, which is at the very least misleading and
> most definitely unexpected and should also result in an error.


I assume it is possible to tell according to the return value, if property
doesn’t exist at all, or if it does exists and size of array in
device-tree is smaller than vm_num.
In [PATCH v3 17/19] Andy wrote that “code shouldn't be a YAML validator.
Drop this and make sure you have correct DT schema” so I’m a bit confused
if code should validate “intel,bm-map” or if it is the user responsibility.
As this property was not added by me, I prefer not to fix it as part of
this series of patches.


> Also, what happens if the devicetree content is something like the
> following ? Would that be valid ?
>        intel,vm-map = [00 01 01 01 01 01];

If device-tree content would be:
     intel,vm-map = [00 01 01 01 01 01];
and assuming 16 channels for each VM, the hwmon sub-system will expose 90
sysfs to read voltage values.
In practice 16 – 31, 32 – 47, 48 – 63, 64 – 89 will all report the same
input signals for VM1.

--
Regards, Eliav

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Guenter Roeck]

On 9/1/22 08:24, Farber, Eliav wrote:
> On 9/1/2022 5:44 PM, Guenter Roeck wrote:
>> On Thu, Sep 01, 2022 at 11:39:58AM +0300, Farber, Eliav wrote:
>>> On 8/31/2022 2:48 PM, Guenter Roeck wrote:
>>> > On 8/30/22 22:49, Farber, Eliav wrote:
>>> > > On 8/31/2022 8:36 AM, Guenter Roeck wrote:
>>> > > > On 8/30/22 12:21, Eliav Farber wrote:
>>> > > > > Bug fix - in case "intel,vm-map" is missing in device-tree
>>> > > > > ,'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;
>>> > > >
>>> > > > So all vm_idx values from 0x00 to 0xfe would be acceptable ?
>>> > > > Does the chip really have that many registers (0x200 + 0x40 +
>>> > > > 0x200 * 0xfe) ?
>>> > > > Is that documented somewhere ?
>>> > > According to the code vm_num is limited to 32 because the mask is
>>> > > only 5 bits:
>>> > >
>>> > > #define VM_NUM_MSK    GENMASK(20, 16)
>>> > > #define VM_NUM_SFT    16
>>> > > vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>>> > >
>>> > > In practice according to the data sheet I have:
>>> > > 0 <= VM instances <= 8
>>> > >
>>> > Sorry, my bad. I misread the patch and thought the first part of
>>> > the if statement was removed.
>>> >
>>> > Anyway, what is the difference between specifying an vm_idx value of
>>> > 0xff and not specifying anything ? Or, in other words, taking the dt
>>> > example, the difference between
>>> >        intel,vm-map = [03 01 04 ff ff];
>>> > and
>>> >        intel,vm-map = [03 01 04];
>>>
>>> The actual number of VMs is read from a HW register:
>>>     ret = regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
>>>     ...
>>>     vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>>>
>>> Also, using:
>>>     ret = device_property_read_u8_array(dev, "intel,vm-map", vm_idx,
>>>                         vm_num);
>>> in the driver will fail if vm_num > sizeof array in device-tree.
>>>
>>> So, if for example vm_num = 5, but you will want to map only 3 of them
>>> you most set property to be:
>>>     intel,vm-map = [03 01 04 ff ff];
>>> otherwise if you set:
>>>     intel,vm-map = [03 01 04];
>>> it will assume the property doesn't, and will continue the flow in code
>>> as if it doesn’t exist (which is not what the user wanted, and before my
>>> fix also has a bug).
>>
>> There should be some error handling to catch this case (ie if the number
>> of entries does not match the expected count), or if a value in the array
>> is larger or equal to vm_num. Today the latter is silently handled as end
>> of entries (similar to 0xff), but that should result in an error.
>> This would avoid situations like
>>        intel,vm-map = [01 02 03 04 05];
>> ie where the person writing the devicetree file accidentally entered
>> index values starting with 1 instead of 0. A mismatch between vm_num
>> and the number of entries in the array is silently handled as if there
>> was no property at all, which is at the very least misleading and
>> most definitely unexpected and should also result in an error.
> 
> 
> I assume it is possible to tell according to the return value, if property
> doesn’t exist at all, or if it does exists and size of array in
> device-tree is smaller than vm_num.
> In [PATCH v3 17/19] Andy wrote that “code shouldn't be a YAML validator.
> Drop this and make sure you have correct DT schema” so I’m a bit confused
> if code should validate “intel,bm-map” or if it is the user responsibility.
> As this property was not added by me, I prefer not to fix it as part of
> this series of patches.
> 

You are changing the driver all over the place with 19 patches, including
this code, but you don't want to add code that validates the devicetree
data ? That seems odd.

> 
>> Also, what happens if the devicetree content is something like the
>> following ? Would that be valid ?
>>        intel,vm-map = [00 01 01 01 01 01];
> 
> If device-tree content would be:
>      intel,vm-map = [00 01 01 01 01 01];
> and assuming 16 channels for each VM, the hwmon sub-system will expose 90
> sysfs to read voltage values.
> In practice 16 – 31, 32 – 47, 48 – 63, 64 – 89 will all report the same
> input signals for VM1.
> 

Does that make any sense, and is there a valid reason to have a mapping
table like the one in this example ?

Thanks,
Guenter

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Farber, Eliav]

On 9/1/2022 8:11 PM, Guenter Roeck wrote:
> CAUTION: This email originated from outside of the organization. Do 
> not click links or open attachments unless you can confirm the sender 
> and know the content is safe.
>
>
>
> On 9/1/22 08:24, Farber, Eliav wrote:
>> On 9/1/2022 5:44 PM, Guenter Roeck wrote:
>>> On Thu, Sep 01, 2022 at 11:39:58AM +0300, Farber, Eliav wrote:
>>>> On 8/31/2022 2:48 PM, Guenter Roeck wrote:
>>>> > On 8/30/22 22:49, Farber, Eliav wrote:
>>>> > > On 8/31/2022 8:36 AM, Guenter Roeck wrote:
>>>> > > > On 8/30/22 12:21, Eliav Farber wrote:
>>>> > > > > Bug fix - in case "intel,vm-map" is missing in device-tree
>>>> > > > > ,'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;
>>>> > > >
>>>> > > > So all vm_idx values from 0x00 to 0xfe would be acceptable ?
>>>> > > > Does the chip really have that many registers (0x200 + 0x40 +
>>>> > > > 0x200 * 0xfe) ?
>>>> > > > Is that documented somewhere ?
>>>> > > According to the code vm_num is limited to 32 because the mask is
>>>> > > only 5 bits:
>>>> > >
>>>> > > #define VM_NUM_MSK    GENMASK(20, 16)
>>>> > > #define VM_NUM_SFT    16
>>>> > > vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>>>> > >
>>>> > > In practice according to the data sheet I have:
>>>> > > 0 <= VM instances <= 8
>>>> > >
>>>> > Sorry, my bad. I misread the patch and thought the first part of
>>>> > the if statement was removed.
>>>> >
>>>> > Anyway, what is the difference between specifying an vm_idx value of
>>>> > 0xff and not specifying anything ? Or, in other words, taking the dt
>>>> > example, the difference between
>>>> >        intel,vm-map = [03 01 04 ff ff];
>>>> > and
>>>> >        intel,vm-map = [03 01 04];
>>>>
>>>> The actual number of VMs is read from a HW register:
>>>>     ret = regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
>>>>     ...
>>>>     vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>>>>
>>>> Also, using:
>>>>     ret = device_property_read_u8_array(dev, "intel,vm-map", vm_idx,
>>>>                         vm_num);
>>>> in the driver will fail if vm_num > sizeof array in device-tree.
>>>>
>>>> So, if for example vm_num = 5, but you will want to map only 3 of them
>>>> you most set property to be:
>>>>     intel,vm-map = [03 01 04 ff ff];
>>>> otherwise if you set:
>>>>     intel,vm-map = [03 01 04];
>>>> it will assume the property doesn't, and will continue the flow in 
>>>> code
>>>> as if it doesn’t exist (which is not what the user wanted, and 
>>>> before my
>>>> fix also has a bug).
>>>
>>> There should be some error handling to catch this case (ie if the 
>>> number
>>> of entries does not match the expected count), or if a value in the 
>>> array
>>> is larger or equal to vm_num. Today the latter is silently handled 
>>> as end
>>> of entries (similar to 0xff), but that should result in an error.
>>> This would avoid situations like
>>>        intel,vm-map = [01 02 03 04 05];
>>> ie where the person writing the devicetree file accidentally entered
>>> index values starting with 1 instead of 0. A mismatch between vm_num
>>> and the number of entries in the array is silently handled as if there
>>> was no property at all, which is at the very least misleading and
>>> most definitely unexpected and should also result in an error.
>>
>>
>> I assume it is possible to tell according to the return value, if 
>> property
>> doesn’t exist at all, or if it does exists and size of array in
>> device-tree is smaller than vm_num.
>> In [PATCH v3 17/19] Andy wrote that “code shouldn't be a YAML validator.
>> Drop this and make sure you have correct DT schema” so I’m a bit 
>> confused
>> if code should validate “intel,bm-map” or if it is the user 
>> responsibility.
>> As this property was not added by me, I prefer not to fix it as part of
>> this series of patches.
>>
>
> You are changing the driver all over the place with 19 patches, including
> this code, but you don't want to add code that validates the devicetree
> data ? That seems odd.
>
OK. I have added patch #20 to validate that same VM index doesn't appear
more than once in intel,vm-map.

u32 vm_mask = 0;

for (i = 0; i < vm_num; i++) {
     if (vm_idx[i] >= vm_num || vm_idx[i] == 0xff)
         break;

     if (vm_mask & BIT(vm_idx[i])) {
         dev_err(dev, "Same VM appears more than once in intel,vm-map\n",
             vm_idx[i]);
         return EINVAL;
     }

     vm_mask |= BIT(vm_idx[i]);
}


>>
>>> Also, what happens if the devicetree content is something like the
>>> following ? Would that be valid ?
>>>        intel,vm-map = [00 01 01 01 01 01];
>>
>> If device-tree content would be:
>>      intel,vm-map = [00 01 01 01 01 01];
>> and assuming 16 channels for each VM, the hwmon sub-system will 
>> expose 90
>> sysfs to read voltage values.
>> In practice 16 – 31, 32 – 47, 48 – 63, 64 – 89 will all report the same
>> input signals for VM1.
>>
>
> Does that make any sense, and is there a valid reason to have a mapping
> table like the one in this example ?

I can't find any sense in having such a mapping.
Anyway the new patch will not allow it to happen.

--
Regards, Eliav

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Guenter Roeck]

On 9/1/22 11:36, Farber, Eliav wrote:
> On 9/1/2022 8:11 PM, Guenter Roeck wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On 9/1/22 08:24, Farber, Eliav wrote:
>>> On 9/1/2022 5:44 PM, Guenter Roeck wrote:
>>>> On Thu, Sep 01, 2022 at 11:39:58AM +0300, Farber, Eliav wrote:
>>>>> On 8/31/2022 2:48 PM, Guenter Roeck wrote:
>>>>> > On 8/30/22 22:49, Farber, Eliav wrote:
>>>>> > > On 8/31/2022 8:36 AM, Guenter Roeck wrote:
>>>>> > > > On 8/30/22 12:21, Eliav Farber wrote:
>>>>> > > > > Bug fix - in case "intel,vm-map" is missing in device-tree
>>>>> > > > > ,'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;
>>>>> > > >
>>>>> > > > So all vm_idx values from 0x00 to 0xfe would be acceptable ?
>>>>> > > > Does the chip really have that many registers (0x200 + 0x40 +
>>>>> > > > 0x200 * 0xfe) ?
>>>>> > > > Is that documented somewhere ?
>>>>> > > According to the code vm_num is limited to 32 because the mask is
>>>>> > > only 5 bits:
>>>>> > >
>>>>> > > #define VM_NUM_MSK    GENMASK(20, 16)
>>>>> > > #define VM_NUM_SFT    16
>>>>> > > vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>>>>> > >
>>>>> > > In practice according to the data sheet I have:
>>>>> > > 0 <= VM instances <= 8
>>>>> > >
>>>>> > Sorry, my bad. I misread the patch and thought the first part of
>>>>> > the if statement was removed.
>>>>> >
>>>>> > Anyway, what is the difference between specifying an vm_idx value of
>>>>> > 0xff and not specifying anything ? Or, in other words, taking the dt
>>>>> > example, the difference between
>>>>> >        intel,vm-map = [03 01 04 ff ff];
>>>>> > and
>>>>> >        intel,vm-map = [03 01 04];
>>>>>
>>>>> The actual number of VMs is read from a HW register:
>>>>>     ret = regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
>>>>>     ...
>>>>>     vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>>>>>
>>>>> Also, using:
>>>>>     ret = device_property_read_u8_array(dev, "intel,vm-map", vm_idx,
>>>>>                         vm_num);
>>>>> in the driver will fail if vm_num > sizeof array in device-tree.
>>>>>
>>>>> So, if for example vm_num = 5, but you will want to map only 3 of them
>>>>> you most set property to be:
>>>>>     intel,vm-map = [03 01 04 ff ff];
>>>>> otherwise if you set:
>>>>>     intel,vm-map = [03 01 04];
>>>>> it will assume the property doesn't, and will continue the flow in code
>>>>> as if it doesn’t exist (which is not what the user wanted, and before my
>>>>> fix also has a bug).
>>>>
>>>> There should be some error handling to catch this case (ie if the number
>>>> of entries does not match the expected count), or if a value in the array
>>>> is larger or equal to vm_num. Today the latter is silently handled as end
>>>> of entries (similar to 0xff), but that should result in an error.
>>>> This would avoid situations like
>>>>        intel,vm-map = [01 02 03 04 05];
>>>> ie where the person writing the devicetree file accidentally entered
>>>> index values starting with 1 instead of 0. A mismatch between vm_num
>>>> and the number of entries in the array is silently handled as if there
>>>> was no property at all, which is at the very least misleading and
>>>> most definitely unexpected and should also result in an error.
>>>
>>>
>>> I assume it is possible to tell according to the return value, if property
>>> doesn’t exist at all, or if it does exists and size of array in
>>> device-tree is smaller than vm_num.
>>> In [PATCH v3 17/19] Andy wrote that “code shouldn't be a YAML validator.
>>> Drop this and make sure you have correct DT schema” so I’m a bit confused
>>> if code should validate “intel,bm-map” or if it is the user responsibility.
>>> As this property was not added by me, I prefer not to fix it as part of
>>> this series of patches.
>>>
>>
>> You are changing the driver all over the place with 19 patches, including
>> this code, but you don't want to add code that validates the devicetree
>> data ? That seems odd.
>>
> OK. I have added patch #20 to validate that same VM index doesn't appear
> more than once in intel,vm-map.
> 
> u32 vm_mask = 0;
> 
> for (i = 0; i < vm_num; i++) {
>      if (vm_idx[i] >= vm_num || vm_idx[i] == 0xff)

I think "vm_idx[i] >= vm_num && vm_idx[i] != 0xff)
should also be invalid, ie.

	if (vm_idx[i] == 0xff)
		break;
	if (vm_idx[i] >= vm_num)
		return -EINVAL;

Thanks,
Guenter

>          break;
> 
>      if (vm_mask & BIT(vm_idx[i])) {
>          dev_err(dev, "Same VM appears more than once in intel,vm-map\n",
>              vm_idx[i]);
>          return EINVAL;
>      }
> 
>      vm_mask |= BIT(vm_idx[i]);
> }
> 
> 
>>>
>>>> Also, what happens if the devicetree content is something like the
>>>> following ? Would that be valid ?
>>>>        intel,vm-map = [00 01 01 01 01 01];
>>>
>>> If device-tree content would be:
>>>      intel,vm-map = [00 01 01 01 01 01];
>>> and assuming 16 channels for each VM, the hwmon sub-system will expose 90
>>> sysfs to read voltage values.
>>> In practice 16 – 31, 32 – 47, 48 – 63, 64 – 89 will all report the same
>>> input signals for VM1.
>>>
>>
>> Does that make any sense, and is there a valid reason to have a mapping
>> table like the one in this example ?
> 
> I can't find any sense in having such a mapping.
> Anyway the new patch will not allow it to happen.
> 
> -- 
> Regards, Eliav
> 

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Andy Shevchenko]

On Thu, Sep 01, 2022 at 06:24:51PM +0300, Farber, Eliav wrote:
> On 9/1/2022 5:44 PM, Guenter Roeck wrote:
> > On Thu, Sep 01, 2022 at 11:39:58AM +0300, Farber, Eliav wrote:

...

> > There should be some error handling to catch this case (ie if the number
> > of entries does not match the expected count), or if a value in the array
> > is larger or equal to vm_num. Today the latter is silently handled as end
> > of entries (similar to 0xff), but that should result in an error.
> > This would avoid situations like
> >        intel,vm-map = [01 02 03 04 05];
> > ie where the person writing the devicetree file accidentally entered
> > index values starting with 1 instead of 0. A mismatch between vm_num
> > and the number of entries in the array is silently handled as if there
> > was no property at all, which is at the very least misleading and
> > most definitely unexpected and should also result in an error.
> 
> I assume it is possible to tell according to the return value, if property
> doesn’t exist at all, or if it does exists and size of array in
> device-tree is smaller than vm_num.
> In [PATCH v3 17/19] Andy wrote that “code shouldn't be a YAML validator.
> Drop this and make sure you have correct DT schema” so I’m a bit confused
> if code should validate “intel,bm-map” or if it is the user responsibility.
> As this property was not added by me, I prefer not to fix it as part of
> this series of patches.

I'm just a messenger of Rob's words. If I'm mistaken I would like Rob to
correct me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel,vm-map" not defined

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:21:55PM +0000, Eliav Farber wrote:
> Bug fix - in case "intel,vm-map" is missing in device-tree ,'num' is set
> to 0, and no voltage channel infos are allocated.

Care to provide a Fixes tag for all fixes in your series. Also don't forget to
start series with fixes followed by cleanups and new features..

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Farber, Eliav]

On 8/31/2022 12:38 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:21:55PM +0000, Eliav Farber wrote:
>> Bug fix - in case "intel,vm-map" is missing in device-tree ,'num' is set
>> to 0, and no voltage channel infos are allocated.
>
> Care to provide a Fixes tag for all fixes in your series. Also don't 
> forget to
> start series with fixes followed by cleanups and new features.. 
For v4 I provided a Fixes tag where it was relevant.
I also changed the order of some patches so that all fixes will be first.

--
Thanks, Eliav

Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

[Andy Shevchenko]

On Fri, Sep 02, 2022 at 03:08:41PM +0300, Farber, Eliav wrote:
> On 8/31/2022 12:38 PM, Andy Shevchenko wrote:
> > On Tue, Aug 30, 2022 at 07:21:55PM +0000, Eliav Farber wrote:
> > > Bug fix - in case "intel,vm-map" is missing in device-tree ,'num' is set
> > > to 0, and no voltage channel infos are allocated.
> > 
> > Care to provide a Fixes tag for all fixes in your series. Also don't
> > forget to
> > start series with fixes followed by cleanups and new features..
> For v4 I provided a Fixes tag where it was relevant.

Thanks!

> I also changed the order of some patches so that all fixes will be first.

Note, fixes should prepend the other patches in the series.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 03/19] 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 in device-tree, 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 v3 03/19] hwmon: (mr75203) update pvt->v_num to the actual number of used sensors

[Guenter Roeck]

On 8/30/22 12:21, Eliav Farber wrote:
> This issue is relevant when "intel,vm-map" is set in device-tree, 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;

How about the existing assignment in the probe function ?

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

Re: [PATCH v3 03/19] hwmon: (mr75203) update pvt->v_num to the actual number of used sensors

[Farber, Eliav]

On 8/31/2022 5:41 AM, Guenter Roeck wrote:
> On 8/30/22 12:21, Eliav Farber wrote:
>> This issue is relevant when "intel,vm-map" is set in device-tree, 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;
>
> How about the existing assignment in the probe function ? 
The probe function uses a local variable vm_num which is also updated
(just one line earlier in code) in the previous patch.

--
Thanks, Eliav

Re: [PATCH v3 03/19] hwmon: (mr75203) update pvt->v_num to the actual number of used sensors

[Guenter Roeck]

On 8/30/22 21:50, Farber, Eliav wrote:
> On 8/31/2022 5:41 AM, Guenter Roeck wrote:
>> On 8/30/22 12:21, Eliav Farber wrote:
>>> This issue is relevant when "intel,vm-map" is set in device-tree, 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;
>>
>> How about the existing assignment in the probe function ? 
> The probe function uses a local variable vm_num which is also updated
> (just one line earlier in code) in the previous patch.
> 

I meant
	pvt->v_num = vm_num;
at line 536 of the current code, but I guess the idea is to overwrite that.

Thanks,
Guenter

[PATCH v3 04/19] dt-bindings: hwmon: (mr75203) change "reset" property to be optional

[Eliav Farber]

Change "reset" property to be optional instead of required, for SOCs that
don't support a reset controller.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> v2:
- Change "reset" property to be optional instead of adding new
  "reset-control-skip" property.

 Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index 6abde48b746e..2ec4b9da4b92 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -49,7 +49,6 @@ required:
   - reg
   - reg-names
   - clocks
-  - resets
   - "#thermal-sensor-cells"
 
 additionalProperties: false
-- 
2.37.1

Re: [PATCH v3 04/19] dt-bindings: hwmon: (mr75203) change "reset" property to be optional

[Philipp Zabel]

On Di, 2022-08-30 at 19:21 +0000, Eliav Farber wrote:
> Change "reset" property to be optional instead of required, for SOCs that
> don't support a reset controller.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> V3 -> v2:
> - Change "reset" property to be optional instead of adding new
>   "reset-control-skip" property.
> 
>  Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> index 6abde48b746e..2ec4b9da4b92 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -49,7 +49,6 @@ required:
>    - reg
>    - reg-names
>    - clocks
> -  - resets

Is this just for mr76006? Or does mr75203 work without reset as well?

If it is the former, maybe a new compatible should be added and resets
should be kept required

if:
  properties:
    compatible:
      contains:
        const: moortec,mr75203

regards
Philipp

Re: [PATCH v3 04/19] dt-bindings: hwmon: (mr75203) change "reset" property to be optional

[Farber, Eliav]

On 8/31/2022 11:21 AM, Philipp Zabel wrote:
> On Di, 2022-08-30 at 19:21 +0000, Eliav Farber wrote:
>> Change "reset" property to be optional instead of required, for SOCs 
>> that
>> don't support a reset controller.
>>
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>> V3 -> v2:
>> - Change "reset" property to be optional instead of adding new
>>   "reset-control-skip" property.
>>
>>  Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml 
>> b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> index 6abde48b746e..2ec4b9da4b92 100644
>> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> @@ -49,7 +49,6 @@ required:
>>    - reg
>>    - reg-names
>>    - clocks
>> -  - resets
>
> Is this just for mr76006? Or does mr75203 work without reset as well?
>
> If it is the former, maybe a new compatible should be added and resets
> should be kept required

mr75203 also works without a reset.
As I replied in PATCH v3 14/19, series 5/6 is relevant only for the
temperature sensor.
The “reset” property is relevant to the controller.

--
Regards, Eliav

Re: [PATCH v3 04/19] dt-bindings: hwmon: (mr75203) change "reset" property to be optional

[Philipp Zabel]

On Mi, 2022-08-31 at 12:43 +0300, Farber, Eliav wrote:
> On 8/31/2022 11:21 AM, Philipp Zabel wrote:
> > On Di, 2022-08-30 at 19:21 +0000, Eliav Farber wrote:
> > > Change "reset" property to be optional instead of required, for SOCs 
> > > that
> > > don't support a reset controller.
> > > 
> > > Signed-off-by: Eliav Farber <farbere@amazon.com>
> > > ---
> > > V3 -> v2:
> > > - Change "reset" property to be optional instead of adding new
> > >   "reset-control-skip" property.
> > > 
> > >  Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git 
> > > a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml 
> > > b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> > > index 6abde48b746e..2ec4b9da4b92 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> > > +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> > > @@ -49,7 +49,6 @@ required:
> > >    - reg
> > >    - reg-names
> > >    - clocks
> > > -  - resets
> > 
> > Is this just for mr76006? Or does mr75203 work without reset as well?
> > 
> > If it is the former, maybe a new compatible should be added and resets
> > should be kept required
> 
> mr75203 also works without a reset.
> As I replied in PATCH v3 14/19, series 5/6 is relevant only for the
> temperature sensor.
> The “reset” property is relevant to the controller.

In that case,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v3 04/19] dt-bindings: hwmon: (mr75203) change "reset" property to be optional

[Rob Herring]

On Tue, Aug 30, 2022 at 07:21:57PM +0000, Eliav Farber wrote:
> Change "reset" property to be optional instead of required, for SOCs that

'resets'

And subject too.

> don't support a reset controller.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> V3 -> v2:
> - Change "reset" property to be optional instead of adding new
>   "reset-control-skip" property.
> 
>  Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> index 6abde48b746e..2ec4b9da4b92 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -49,7 +49,6 @@ required:
>    - reg
>    - reg-names
>    - clocks
> -  - resets
>    - "#thermal-sensor-cells"
>  
>  additionalProperties: false
> -- 
> 2.37.1
> 
> 

Re: [PATCH v3 04/19] dt-bindings: hwmon: (mr75203) change "reset" property to be optional

[Farber, Eliav]

On 9/2/2022 10:51 PM, Rob Herring wrote:
> On Tue, Aug 30, 2022 at 07:21:57PM +0000, Eliav Farber wrote:
>> Change "reset" property to be optional instead of required, for SOCs 
>> that
>
> 'resets'
>
> And subject too.
Fixed in v4.

[PATCH v3 05/19] hwmon: (mr75203) skip reset-control deassert for SOCs that don't support it

[Eliav Farber]

Don't fail the probe function and don't deassert the reset controller if
a "reset" property doesn't exist in the device tree.

Change is done for SOCs that don't support a reset controller.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> v2:
- Change "reset" property to be optional instead of skipping it.

 drivers/hwmon/mr75203.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index f89f7bb5d698..901030125127 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -525,14 +525,17 @@ static int mr75203_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pvt->rst = devm_reset_control_get_exclusive(dev, NULL);
+	pvt->rst = devm_reset_control_get_optional_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");
+	if (pvt->rst) {
+		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

Re: [PATCH v3 05/19] hwmon: (mr75203) skip reset-control deassert for SOCs that don't support it

[Philipp Zabel]

Hi Eliav,

On Di, 2022-08-30 at 19:21 +0000, Eliav Farber wrote:
> Don't fail the probe function and don't deassert the reset controller if
> a "reset" property doesn't exist in the device tree.
> 
> Change is done for SOCs that don't support a reset controller.

Not strictly related to this patch, but reading the context I wonder
whether it is required that clock/reset are running/deasserted all the
time. Would it make sense to implement some kind of runtime PM?

> Signed-off-by: Eliav Farber <farbere@amazon.com>

This change is

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

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

[Eliav Farber]

Fix voltage allocation and reading to support all channels in all VMs.
Prior to this change allocation and reading were done only for the first
channel in each VM.
This change counts the total number of channels for allocation, and takes
into account the channel offset when reading the sample data register.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- Remove configuration of ip-polling register to a separate commit.
- Explain the fix.

 drivers/hwmon/mr75203.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 901030125127..639f5a300170 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -68,8 +68,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
@@ -115,6 +116,7 @@ struct pvt_device {
 	u32			t_num;
 	u32			p_num;
 	u32			v_num;
+	u32			c_num;
 	u32			ip_freq;
 	u8			*vm_idx;
 };
@@ -179,13 +181,14 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 	struct pvt_device *pvt = dev_get_drvdata(dev);
 	struct regmap *v_map = pvt->v_map;
 	u32 n, stat;
-	u8 vm_idx;
+	u8 vm_idx, 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:
@@ -196,7 +199,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;
 
@@ -500,7 +503,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;
@@ -544,9 +547,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++;
@@ -583,6 +588,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;
@@ -611,13 +618,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 v3 06/19] hwmon: (mr75203) fix multi-channel voltage reading

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:21:59PM +0000, Eliav Farber wrote:
> Fix voltage allocation and reading to support all channels in all VMs.
> Prior to this change allocation and reading were done only for the first
> channel in each VM.
> This change counts the total number of channels for allocation, and takes
> into account the channel offset when reading the sample data register.

...

>  	struct pvt_device *pvt = dev_get_drvdata(dev);
>  	struct regmap *v_map = pvt->v_map;
>  	u32 n, stat;
> -	u8 vm_idx;
> +	u8 vm_idx, ch_idx;

Can you keep it sorted by line length?

>  	int ret;

...

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

Ditto.

-- 
With Best Regards,
Andy Shevchenko

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

[Farber, Eliav]

On 8/31/2022 12:46 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:21:59PM +0000, Eliav Farber wrote:
>> Fix voltage allocation and reading to support all channels in all VMs.
>> Prior to this change allocation and reading were done only for the first
>> channel in each VM.
>> This change counts the total number of channels for allocation, and 
>> takes
>> into account the channel offset when reading the sample data register.
>
> ...
>
>>       struct pvt_device *pvt = dev_get_drvdata(dev);
>>       struct regmap *v_map = pvt->v_map;
>>       u32 n, stat;
>> -     u8 vm_idx;
>> +     u8 vm_idx, ch_idx;
>
> Can you keep it sorted by line length?

Fixed.

>>       int ret;
>
> ...
>
>>       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;
>
> Ditto. 
Fixed.

--
Thanks, Eliav

[PATCH v3 07/19] hwmon: (mr75203) enable polling for all VM channels

[Eliav Farber]

Configure ip-polling register to enable polling for all voltage monitor
channels.
This enables reading the voltage values for all inputs other than just
input 0.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- Move configuration of ip-polling register from previous patch to a
  separate commit.

 drivers/hwmon/mr75203.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 639f5a300170..f561806c400d 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -388,6 +388,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;
-- 
2.37.1

Re: [PATCH v3 07/19] hwmon: (mr75203) enable polling for all VM channels

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:22:00PM +0000, Eliav Farber wrote:
> Configure ip-polling register to enable polling for all voltage monitor
> channels.
> This enables reading the voltage values for all inputs other than just
> input 0.

...

> +		val = GENMASK(pvt->c_num - 1, 0) | VM_CH_INIT |

I believe in this case (BIT(pvt->c_num) - 1) is better, but not sure
if c_num can be 32.

> +		      IP_POLL << SDIF_ADDR_SFT |
> +		      SDIF_WRN_W | SDIF_PROG;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/19] hwmon: (mr75203) enable polling for all VM channels

[Farber, Eliav]

On 8/31/2022 2:21 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:22:00PM +0000, Eliav Farber wrote:
>> Configure ip-polling register to enable polling for all voltage monitor
>> channels.
>> This enables reading the voltage values for all inputs other than just
>> input 0.
>
> ...
>
>> +             val = GENMASK(pvt->c_num - 1, 0) | VM_CH_INIT |
>
> I believe in this case (BIT(pvt->c_num) - 1) is better, but not sure
> if c_num can be 32. 
c_num can't be 32.
I fix it in v4.

--
Thanks, Eliav

[PATCH v3 08/19] dt-bindings: hwmon: (mr75203) add "moortec,vm-active-channels" property

[Eliav Farber]

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

This shall be useful to avoid exposing sysfs for reading inputs that are
not connected to any voltage source.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- Add "moortec" prefix to property name.
- Add explanation why this change is needed.

 .../devicetree/bindings/hwmon/moortec,mr75203.yaml    | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index 2ec4b9da4b92..69cc6caceb2c 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -44,6 +44,16 @@ properties:
   "#thermal-sensor-cells":
     const: 1
 
+  moortec,vm-active-channels:
+    description:
+      moortec,vm-active-channels defines the number of channels per VM that are
+      actually used and are connected to some source.
+      Number of values in the array should be the number of VMs.
+      A value of 0 means that the entire VM sensor is nou used.
+    maximum: 16
+    default: 16
+    $ref: /schemas/types.yaml#definitions/uint8-array
+
 required:
   - compatible
   - reg
@@ -65,5 +75,6 @@ examples:
         intel,vm-map = [03 01 04 ff ff];
         clocks = <&osc0>;
         resets = <&rcu0 0x40 7>;
+        moortec,vm-active-channels = <0x10 0x05>;
         #thermal-sensor-cells = <1>;
     };
-- 
2.37.1

Re: [PATCH v3 08/19] dt-bindings: hwmon: (mr75203) add "moortec,vm-active-channels" property

[Rob Herring]

On Tue, 30 Aug 2022 19:22:01 +0000, Eliav Farber wrote:
> Add optional "moortec,vm-active-channels" property to define the number
> of active channels per VM.
> 
> This shall be useful to avoid exposing sysfs for reading inputs that are
> not connected to any voltage source.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> V3 -> V2:
> - Add "moortec" prefix to property name.
> - Add explanation why this change is needed.
> 
>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml    | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml: error checking schema file
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml: ignoring, error in schema: properties: moortec,vm-active-channels
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/moortec,mr75203.example.dtb: pvt@e0680000: 'moortec,vm-active-channels' does not match any of the regexes: '^#.*', '^(at25|bm|devbus|dmacap|dsa|exynos|fsi[ab]|gpio-fan|gpio-key|gpio|gpmc|hdmi|i2c-gpio),.*', '^(keypad|m25p|max8952|max8997|max8998|mpmc),.*', '^(pinctrl-single|#pinctrl-single|PowerPC),.*', '^(pl022|pxa-mmc|rcar_sound|rotary-encoder|s5m8767|sdhci),.*', '^(simple-audio-card|st-plgpio|st-spics|ts),.*', '^100ask,.*', '^70mai,.*', '^8dev,.*', '^GEFanuc,.*', '^ORCL,.*', '^SUNW,.*', '^[a-zA-Z0-9#_][a-zA-Z0-9+\\-._@]{0,63}$', '^[a-zA-Z0-9+\\-._]*@[0-9a-zA-Z,]*$', '^abb,.*', '^abilis,.*', '^abracon,.*', '^abt,.*', '^acer,.*', '^acme,.*', '^actions,.*', '^active-semi,.*', '^ad,.*', '^adafruit,.*', '^adapteva,.*', '^adaptrum,.*', '^adh,.*', '^adi,.*', '^advantech,.*', '^aeroflexgaisler,.*', '^aesop,.*', '^airoha,.*', '^al,.*', '^alcatel,.*', '^allegro,.*', '^allo,.*', '^allwinner,.*', '^alphascale,.*', '^al
 ps,.*', '^alt,.*', '^altr,.*', '^amarula,.*', '^amazon,.*', '^amcc,.*', '^amd,.*', '^amediatech,.*', '^amlogic,.*', '^ampere,.*', '^ampire,.*', '^ams,.*', '^amstaos,.*', '^analogix,.*', '^andestech,.*', '^anvo,.*', '^apm,.*', '^apple,.*', '^aptina,.*', '^arasan,.*', '^archermind,.*', '^arctic,.*', '^arcx,.*', '^aries,.*', '^arm,.*', '^armadeus,.*', '^arrow,.*', '^artesyn,.*', '^asahi-kasei,.*', '^asc,.*', '^asix,.*', '^aspeed,.*', '^asrock,.*', '^asus,.*', '^atheros,.*', '^atlas,.*', '^atmel,.*', '^auo,.*', '^auvidea,.*', '^avago,.*', '^avia,.*', '^avic,.*', '^avnet,.*', '^awinic,.*', '^axentia,.*', '^axis,.*', '^azoteq,.*', '^azw,.*', '^baikal,.*', '^bananapi,.*', '^beacon,.*', '^beagle,.*', '^bhf,.*', '^bitmain,.*', '^blutek,.*', '^boe,.*', '^bosch,.*', '^boundary,.*', '^brcm,.*', '^broadmobi,.*', '^bsh,.*', '^bticino,.*', '^buffalo,.*', '^bur,.*', '^bytedance,.*', '^calamp,.*', '^calaosystems,.*', '^calxeda,.*', '^canaan,.*', '^caninos,.*', '^capella,.*', '^cascoda,.*', '^catalys
 t,.*', '^cavium,.*', '^cdns,.*', '^cdtech,.*', '^cellwise,.*', '^ceva,.*', '^checkpoint,.*', '^chefree,.*', '^chipidea,.*', '^chipone,.*', '^chipspark,.*', '^chrontel,.*', '^chrp,.*', '^chunghwa,.*', '^chuwi,.*', '^ciaa,.*', '^cirrus,.*', '^cisco,.*', '^cloudengines,.*', '^cnm,.*', '^cnxt,.*', '^colorfly,.*', '^compulab,.*', '^congatec,.*', '^coreriver,.*', '^corpro,.*', '^cortina,.*', '^cosmic,.*', '^crane,.*', '^creative,.*', '^crystalfontz,.*', '^csky,.*', '^csq,.*', '^ctera,.*', '^ctu,.*', '^cubietech,.*', '^cui,.*', '^cypress,.*', '^cyx,.*', '^cznic,.*', '^dallas,.*', '^dataimage,.*', '^davicom,.*', '^dell,.*', '^delta,.*', '^densitron,.*', '^denx,.*', '^devantech,.*', '^dfi,.*', '^dh,.*', '^difrnce,.*', '^digi,.*', '^digilent,.*', '^dioo,.*', '^dlc,.*', '^dlg,.*', '^dlink,.*', '^dmo,.*', '^domintech,.*', '^dongwoon,.*', '^dptechnics,.*', '^dragino,.*', '^ds,.*', '^dserve,.*', '^dynaimage,.*', '^ea,.*', '^ebang,.*', '^ebbg,.*', '^ebs-systart,.*', '^ebv,.*', '^eckelmann,.*', '^e
 dimax,.*', '^edt,.*', '^eeti,.*', '^einfochips,.*', '^eink,.*', '^elan,.*', '^element14,.*', '^elgin,.*', '^elida,.*', '^elimo,.*', '^elpida,.*', '^embest,.*', '^emlid,.*', '^emmicro,.*', '^empire-electronix,.*', '^emtrion,.*', '^enclustra,.*', '^endless,.*', '^ene,.*', '^energymicro,.*', '^engicam,.*', '^engleder,.*', '^epcos,.*', '^epfl,.*', '^epson,.*', '^esp,.*', '^est,.*', '^ettus,.*', '^eukrea,.*', '^everest,.*', '^everspin,.*', '^evervision,.*', '^exar,.*', '^excito,.*', '^exegin,.*', '^ezchip,.*', '^facebook,.*', '^fairphone,.*', '^faraday,.*', '^fastrax,.*', '^fcs,.*', '^feixin,.*', '^feiyang,.*', '^fii,.*', '^firefly,.*', '^focaltech,.*', '^forlinx,.*', '^frida,.*', '^friendlyarm,.*', '^fsl,.*', '^fujitsu,.*', '^fxtec,.*', '^gardena,.*', '^gateworks,.*', '^gcw,.*', '^ge,.*', '^geekbuying,.*', '^gef,.*', '^gemei,.*', '^geniatech,.*', '^giantec,.*', '^giantplus,.*', '^globalscale,.*', '^globaltop,.*', '^gmt,.*', '^goodix,.*', '^google,.*', '^grinn,.*', '^grmn,.*', '^gumstix,
 .*', '^gw,.*', '^hannstar,.*', '^haochuangyi,.*', '^haoyu,.*', '^hardkernel,.*', '^hechuang,.*', '^hideep,.*', '^himax,.*', '^hirschmann,.*', '^hisi,.*', '^hisilicon,.*', '^hit,.*', '^hitex,.*', '^holt,.*', '^holtek,.*', '^honestar,.*', '^honeywell,.*', '^hoperun,.*', '^hp,.*', '^hpe,.*', '^hsg,.*', '^huawei,.*', '^hugsun,.*', '^hwacom,.*', '^hxt,.*', '^hycon,.*', '^hydis,.*', '^hynix,.*', '^hyundai,.*', '^i2se,.*', '^ibm,.*', '^icplus,.*', '^idt,.*', '^ifi,.*', '^ilitek,.*', '^imagis,.*', '^img,.*', '^imi,.*', '^incircuit,.*', '^inet-tek,.*', '^infineon,.*', '^inforce,.*', '^ingenic,.*', '^ingrasys,.*', '^injoinic,.*', '^innolux,.*', '^inside-secure,.*', '^insignal,.*', '^inspur,.*', '^intel,.*', '^intercontrol,.*', '^invensense,.*', '^inventec,.*', '^inversepath,.*', '^iom,.*', '^isee,.*', '^isil,.*', '^issi,.*', '^ite,.*', '^itead,.*', '^itian,.*', '^ivo,.*', '^iwave,.*', '^jdi,.*', '^jedec,.*', '^jesurun,.*', '^jethome,.*', '^jianda,.*', '^joz,.*', '^kam,.*', '^karo,.*', '^keith
 koep,.*', '^keymile,.*', '^khadas,.*', '^kiebackpeter,.*', '^kinetic,.*', '^kingdisplay,.*', '^kingnovel,.*', '^kionix,.*', '^kobo,.*', '^kobol,.*', '^koe,.*', '^kontron,.*', '^kosagi,.*', '^kvg,.*', '^kyo,.*', '^lacie,.*', '^laird,.*', '^lamobo,.*', '^lantiq,.*', '^lattice,.*', '^leadtek,.*', '^leez,.*', '^lego,.*', '^lemaker,.*', '^lenovo,.*', '^lg,.*', '^lgphilips,.*', '^libretech,.*', '^licheepi,.*', '^linaro,.*', '^linksprite,.*', '^linksys,.*', '^linutronix,.*', '^linux,.*', '^linx,.*', '^liteon,.*', '^litex,.*', '^lltc,.*', '^logicpd,.*', '^logictechno,.*', '^longcheer,.*', '^lontium,.*', '^loongson,.*', '^lsi,.*', '^lwn,.*', '^lxa,.*', '^m5stack,.*', '^macnica,.*', '^mantix,.*', '^mapleboard,.*', '^marvell,.*', '^maxbotix,.*', '^maxim,.*', '^mbvl,.*', '^mcube,.*', '^meas,.*', '^mecer,.*', '^mediatek,.*', '^megachips,.*', '^mele,.*', '^melexis,.*', '^melfas,.*', '^mellanox,.*', '^memsic,.*', '^menlo,.*', '^mentor,.*', '^meraki,.*', '^merrii,.*', '^micrel,.*', '^microchip,.*',
  '^microcrystal,.*', '^micron,.*', '^microsoft,.*', '^microsys,.*', '^mikroe,.*', '^mikrotik,.*', '^miniand,.*', '^minix,.*', '^miramems,.*', '^mitsubishi,.*', '^mixel,.*', '^miyoo,.*', '^mntre,.*', '^modtronix,.*', '^mosaixtech,.*', '^motorola,.*', '^moxa,.*', '^mpl,.*', '^mps,.*', '^mqmaker,.*', '^mrvl,.*', '^mscc,.*', '^msi,.*', '^mstar,.*', '^mti,.*', '^multi-inno,.*', '^mundoreader,.*', '^murata,.*', '^mxic,.*', '^mxicy,.*', '^myir,.*', '^national,.*', '^nec,.*', '^neonode,.*', '^netgear,.*', '^netlogic,.*', '^netron-dy,.*', '^netronix,.*', '^netxeon,.*', '^neweast,.*', '^newhaven,.*', '^nexbox,.*', '^nextthing,.*', '^ni,.*', '^nintendo,.*', '^nlt,.*', '^nokia,.*', '^nordic,.*', '^novtech,.*', '^nutsboard,.*', '^nuvoton,.*', '^nvd,.*', '^nvidia,.*', '^nxp,.*', '^oceanic,.*', '^ocs,.*', '^oct,.*', '^okaya,.*', '^oki,.*', '^olimex,.*', '^olpc,.*', '^oneplus,.*', '^onion,.*', '^onnn,.*', '^ontat,.*', '^opalkelly,.*', '^opencores,.*', '^openembed,.*', '^openrisc,.*', '^option,.*', 
 '^oranth,.*', '^orisetech,.*', '^ortustech,.*', '^osddisplays,.*', '^osmc,.*', '^ouya,.*', '^overkiz,.*', '^ovti,.*', '^oxsemi,.*', '^ozzmaker,.*', '^panasonic,.*', '^parade,.*', '^parallax,.*', '^pda,.*', '^pericom,.*', '^pervasive,.*', '^phicomm,.*', '^phytec,.*', '^picochip,.*', '^pine64,.*', '^pineriver,.*', '^pixcir,.*', '^plantower,.*', '^plathome,.*', '^plda,.*', '^plx,.*', '^ply,.*', '^pni,.*', '^pocketbook,.*', '^polaroid,.*', '^portwell,.*', '^poslab,.*', '^pov,.*', '^powertip,.*', '^powervr,.*', '^primux,.*', '^probox2,.*', '^prt,.*', '^pulsedlight,.*', '^purism,.*', '^qca,.*', '^qcom,.*', '^qemu,.*', '^qi,.*', '^qiaodian,.*', '^qihua,.*', '^qishenglong,.*', '^qnap,.*', '^quanta,.*', '^radxa,.*', '^raidsonic,.*', '^ralink,.*', '^ramtron,.*', '^raspberrypi,.*', '^raydium,.*', '^rda,.*', '^realtek,.*', '^remarkable,.*', '^renesas,.*', '^rervision,.*', '^revotics,.*', '^rex,.*', '^richtek,.*', '^ricoh,.*', '^rikomagic,.*', '^riot,.*', '^riscv,.*', '^rockchip,.*', '^rocktech,
 .*', '^rohm,.*', '^ronbo,.*', '^roofull,.*', '^roseapplepi,.*', '^samsung,.*', '^samtec,.*', '^sancloud,.*', '^sandisk,.*', '^satoz,.*', '^sbs,.*', '^schindler,.*', '^seagate,.*', '^seeed,.*', '^seirobotics,.*', '^semtech,.*', '^senseair,.*', '^sensirion,.*', '^sensortek,.*', '^sercomm,.*', '^sff,.*', '^sgd,.*', '^sgmicro,.*', '^sgx,.*', '^sharp,.*', '^shift,.*', '^shimafuji,.*', '^shiratech,.*', '^si-en,.*', '^si-linux,.*', '^siemens,.*', '^sifive,.*', '^sigma,.*', '^sii,.*', '^sil,.*', '^silabs,.*', '^silan,.*', '^silead,.*', '^silergy,.*', '^silex-insight,.*', '^siliconfile,.*', '^siliconmitus,.*', '^silvaco,.*', '^simtek,.*', '^sinlinx,.*', '^sinovoip,.*', '^sinowealth,.*', '^sipeed,.*', '^sirf,.*', '^sis,.*', '^sitronix,.*', '^skov,.*', '^skyworks,.*', '^smartlabs,.*', '^smsc,.*', '^snps,.*', '^sochip,.*', '^socionext,.*', '^solidrun,.*', '^solomon,.*', '^sony,.*', '^spansion,.*', '^sparkfun,.*', '^spinalhdl,.*', '^sprd,.*', '^ssi,.*', '^sst,.*', '^sstar,.*', '^st,.*', '^st-eri
 csson,.*', '^starfive,.*', '^starry,.*', '^startek,.*', '^ste,.*', '^stericsson,.*', '^storlink,.*', '^storm,.*', '^storopack,.*', '^summit,.*', '^sunchip,.*', '^sundance,.*', '^sunplus,.*', '^supermicro,.*', '^swir,.*', '^syna,.*', '^synology,.*', '^synopsys,.*', '^tbs,.*', '^tbs-biometrics,.*', '^tcg,.*', '^tcl,.*', '^tcs,.*', '^tdo,.*', '^team-source-display,.*', '^technexion,.*', '^technologic,.*', '^techstar,.*', '^teltonika,.*', '^tempo,.*', '^terasic,.*', '^tesla,.*', '^tfc,.*', '^thead,.*', '^thine,.*', '^thingyjp,.*', '^thundercomm,.*', '^ti,.*', '^tianma,.*', '^tlm,.*', '^tmt,.*', '^topeet,.*', '^topic,.*', '^toppoly,.*', '^topwise,.*', '^toradex,.*', '^toshiba,.*', '^toumaz,.*', '^tpk,.*', '^tplink,.*', '^tpo,.*', '^tq,.*', '^traverse,.*', '^tronfy,.*', '^tronsmart,.*', '^truly,.*', '^tsd,.*', '^tyan,.*', '^u-blox,.*', '^u-boot,.*', '^ubnt,.*', '^ucrobotics,.*', '^udoo,.*', '^ugoos,.*', '^uniwest,.*', '^upisemi,.*', '^urt,.*', '^usi,.*', '^utoo,.*', '^v3,.*', '^vaisala,.*
 ', '^vamrs,.*', '^variscite,.*', '^vdl,.*', '^vertexcom,.*', '^via,.*', '^vicor,.*', '^videostrong,.*', '^virtio,.*', '^virtual,.*', '^vishay,.*', '^visionox,.*', '^vitesse,.*', '^vivante,.*', '^vivax,.*', '^vocore,.*', '^voipac,.*', '^vot,.*', '^vxt,.*', '^wanchanglong,.*', '^wand,.*', '^waveshare,.*', '^wd,.*', '^we,.*', '^welltech,.*', '^wetek,.*', '^wexler,.*', '^whwave,.*', '^wi2wi,.*', '^wiligear,.*', '^willsemi,.*', '^winbond,.*', '^wingtech,.*', '^winlink,.*', '^winstar,.*', '^wirelesstag,.*', '^wits,.*', '^wlf,.*', '^wm,.*', '^wobo,.*', '^x-powers,.*', '^xen,.*', '^xes,.*', '^xiaomi,.*', '^xillybus,.*', '^xingbangda,.*', '^xinpeng,.*', '^xiphera,.*', '^xlnx,.*', '^xnano,.*', '^xunlong,.*', '^xylon,.*', '^yadro,.*', '^yamaha,.*', '^yes-optoelectronics,.*', '^yic,.*', '^ylm,.*', '^yna,.*', '^yones-toptech,.*', '^ys,.*', '^ysoft,.*', '^zarlink,.*', '^zealz,.*', '^zeitec,.*', '^zidoo,.*', '^zii,.*', '^zinitix,.*', '^zkmagic,.*', '^zte,.*', '^zyxel,.*', 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/vendor-prefixes.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/moortec,mr75203.example.dtb: pvt@e0680000: intel,vm-map: b'\x03\x01\x04\xff\xff' is not of type 'object', 'array', 'boolean', 'null'
	From schema: /usr/local/lib/python3.10/dist-packages/dtschema/schemas/dt-core.yaml
Documentation/devicetree/bindings/hwmon/moortec,mr75203.example.dtb:0:0: /example-0/pvt@e0680000: failed to match any schema with compatible: ['moortec,mr75203']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 08/19] dt-bindings: hwmon: (mr75203) add "moortec, vm-active-channels" property

[Rob Herring]

On Wed, Aug 31, 2022 at 6:53 AM Farber, Eliav <farbere@amazon.com> wrote:
>
> On 8/31/2022 2:39 PM, Rob Herring wrote:
>
> On Tue, 30 Aug 2022 19:22:01 +0000, Eliav Farber wrote:
>
> Add optional "moortec,vm-active-channels" property to define the number
> of active channels per VM.
>
> This shall be useful to avoid exposing sysfs for reading inputs that are
> not connected to any voltage source.
>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> V3 -> V2:
> - Add "moortec" prefix to property name.
> - Add explanation why this change is needed.
>
>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml    | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> I used dt_binding_check on my changes (I ported it to my kernel).
> The error is related to "intel-vm-map" which I did not add.

The error is the vendor prefix is not defined in vendor-prefixes.yaml.

> I don't mind fixing it if you wish.
> It requires changing:
>   intel,vm-map = [03 01 04 ff ff];
> to:
>   intel,vm-map = /bits/8 <0x03 0x01 0x04 0xff 0xff>;

That is not the issue. The issue is the type is unknown because your
schema fails and we can't get the type from it. Once your schema
passes, this should go away.

Rob

Re: [PATCH v3 08/19] dt-bindings: hwmon: (mr75203) add "moortec, vm-active-channels" property

[Farber, Eliav]

On 8/31/2022 3:17 PM, Rob Herring wrote:
> On Wed, Aug 31, 2022 at 6:53 AM Farber, Eliav <farbere@amazon.com> wrote:
>>
>> On 8/31/2022 2:39 PM, Rob Herring wrote:
>>
>> On Tue, 30 Aug 2022 19:22:01 +0000, Eliav Farber wrote:
>>
>> Add optional "moortec,vm-active-channels" property to define the number
>> of active channels per VM.
>>
>> This shall be useful to avoid exposing sysfs for reading inputs that are
>> not connected to any voltage source.
>>
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>> V3 -> V2:
>> - Add "moortec" prefix to property name.
>> - Add explanation why this change is needed.
>>
>>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml    | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>>
>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>
>> I used dt_binding_check on my changes (I ported it to my kernel).
>> The error is related to "intel-vm-map" which I did not add.
>
> The error is the vendor prefix is not defined in vendor-prefixes.yaml.

I fixed the vendor prefix error (will be part of v4).

>> I don't mind fixing it if you wish.
>> It requires changing:
>>   intel,vm-map = [03 01 04 ff ff];
>> to:
>>   intel,vm-map = /bits/8 <0x03 0x01 0x04 0xff 0xff>;
>
> That is not the issue. The issue is the type is unknown because your
> schema fails and we can't get the type from it. Once your schema
> passes, this should go away.
Even after fixing the vendor prefix error I still see this:
moortec,mr75203.yaml: ignoring, error in schema: properties: intel,vm-map
moortec,mr75203.example.dtb: pvt@e0680000: intel,vm-map: 
b'\x03\x01\x04\xff\xff' is not of type 'object', 'array', 'boolean', 'null'

--
Regards, Eliav

Re: [PATCH v3 08/19] dt-bindings: hwmon: (mr75203) add "moortec, vm-active-channels" property

[Rob Herring]

On Wed, Aug 31, 2022 at 12:48 PM Farber, Eliav <farbere@amazon.com> wrote:
>
> On 8/31/2022 3:17 PM, Rob Herring wrote:
> > On Wed, Aug 31, 2022 at 6:53 AM Farber, Eliav <farbere@amazon.com> wrote:
> >>
> >> On 8/31/2022 2:39 PM, Rob Herring wrote:
> >>
> >> On Tue, 30 Aug 2022 19:22:01 +0000, Eliav Farber wrote:
> >>
> >> Add optional "moortec,vm-active-channels" property to define the number
> >> of active channels per VM.
> >>
> >> This shall be useful to avoid exposing sysfs for reading inputs that are
> >> not connected to any voltage source.
> >>
> >> Signed-off-by: Eliav Farber <farbere@amazon.com>
> >> ---
> >> V3 -> V2:
> >> - Add "moortec" prefix to property name.
> >> - Add explanation why this change is needed.
> >>
> >>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml    | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >>
> >> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> >> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >>
> >> I used dt_binding_check on my changes (I ported it to my kernel).
> >> The error is related to "intel-vm-map" which I did not add.
> >
> > The error is the vendor prefix is not defined in vendor-prefixes.yaml.
>
> I fixed the vendor prefix error (will be part of v4).
>
> >> I don't mind fixing it if you wish.
> >> It requires changing:
> >>   intel,vm-map = [03 01 04 ff ff];
> >> to:
> >>   intel,vm-map = /bits/8 <0x03 0x01 0x04 0xff 0xff>;
> >
> > That is not the issue. The issue is the type is unknown because your
> > schema fails and we can't get the type from it. Once your schema
> > passes, this should go away.
> Even after fixing the vendor prefix error I still see this:
> moortec,mr75203.yaml: ignoring, error in schema: properties: intel,vm-map

You still have an error in the schema. You should see a more specific
reason before this message.

> moortec,mr75203.example.dtb: pvt@e0680000: intel,vm-map:
> b'\x03\x01\x04\xff\xff' is not of type 'object', 'array', 'boolean', 'null'
>
> --
> Regards, Eliav

Re: [PATCH v3 08/19] dt-bindings: hwmon: (mr75203) add "moortec, vm-active-channels" property

[Farber, Eliav]

On 8/31/2022 10:19 PM, Rob Herring wrote:
> On Wed, Aug 31, 2022 at 12:48 PM Farber, Eliav <farbere@amazon.com> 
> wrote:
>>
>> On 8/31/2022 3:17 PM, Rob Herring wrote:
>> > On Wed, Aug 31, 2022 at 6:53 AM Farber, Eliav <farbere@amazon.com> 
>> wrote:
>> >>
>> >> On 8/31/2022 2:39 PM, Rob Herring wrote:
>> >>
>> >> On Tue, 30 Aug 2022 19:22:01 +0000, Eliav Farber wrote:
>> >>
>> >> Add optional "moortec,vm-active-channels" property to define the 
>> number
>> >> of active channels per VM.
>> >>
>> >> This shall be useful to avoid exposing sysfs for reading inputs 
>> that are
>> >> not connected to any voltage source.
>> >>
>> >> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> >> ---
>> >> V3 -> V2:
>> >> - Add "moortec" prefix to property name.
>> >> - Add explanation why this change is needed.
>> >>
>> >>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml | 11 +++++++++++
>> >>  1 file changed, 11 insertions(+)
>> >>
>> >>
>> >> My bot found errors running 'make DT_CHECKER_FLAGS=-m 
>> dt_binding_check'
>> >> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>> >>
>> >> I used dt_binding_check on my changes (I ported it to my kernel).
>> >> The error is related to "intel-vm-map" which I did not add.
>> >
>> > The error is the vendor prefix is not defined in vendor-prefixes.yaml.
>>
>> I fixed the vendor prefix error (will be part of v4).
>>
>> >> I don't mind fixing it if you wish.
>> >> It requires changing:
>> >>   intel,vm-map = [03 01 04 ff ff];
>> >> to:
>> >>   intel,vm-map = /bits/8 <0x03 0x01 0x04 0xff 0xff>;
>> >
>> > That is not the issue. The issue is the type is unknown because your
>> > schema fails and we can't get the type from it. Once your schema
>> > passes, this should go away.
>> Even after fixing the vendor prefix error I still see this:
>> moortec,mr75203.yaml: ignoring, error in schema: properties: 
>> intel,vm-map
>
> You still have an error in the schema. You should see a more specific
> reason before this message. 

Thanks, I found the problem.
I'm using an old version, and I'm missing this commit you did:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml?h=v6.0-rc3&id=d69c6ddd019f31081cc0232fa8ad8ea1cabdf22c

[PATCH v3 09/19] hwmon: (mr75203) add VM active channel support

[Eliav Farber]

Add active channel support per voltage monitor.
The number of active channels is read from the device-tree.
When absent in device-tree, all channels are assumed to be used.

This shall be useful to expose sysfs only for inputs that are connected
to a voltage source.

Setting number of active channels to 0, means that entire VM sensor is
not used.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- Refactor the code changes (move code to a new function and group
  parameters in dedicated structure).

V2 -> V1:
- Fix compilation error for patch 08/16:
  "warning: ISO C90 forbids variable length array"

 drivers/hwmon/mr75203.c | 107 ++++++++++++++++++++++++++++++++--------
 1 file changed, 86 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index f561806c400d..6925e8490587 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -29,6 +29,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
@@ -106,6 +108,16 @@
 #define PVT_N_CONST		90
 #define PVT_R_CONST		245805
 
+struct voltage_device {
+	u32 vm_map;	/* Map channel number to VM index */
+	u32 ch_map;	/* Map channel number to channel index */
+};
+
+struct voltage_channels {
+	u32 total;	/* Total number of channels in all VMs */
+	u8 max;		/* Maximum number of channels among all VMs */
+};
+
 struct pvt_device {
 	struct regmap		*c_map;
 	struct regmap		*t_map;
@@ -113,12 +125,12 @@ struct pvt_device {
 	struct regmap		*v_map;
 	struct clk		*clk;
 	struct reset_control	*rst;
+	struct voltage_device	*vd;
+	struct voltage_channels	vm_channels;
 	u32			t_num;
 	u32			p_num;
 	u32			v_num;
-	u32			c_num;
 	u32			ip_freq;
-	u8			*vm_idx;
 };
 
 static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
@@ -184,11 +196,11 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 	u8 vm_idx, ch_idx;
 	int ret;
 
-	if (channel >= pvt->v_num * pvt->c_num)
+	if (channel >= pvt->vm_channels.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:
@@ -388,7 +400,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_channels.max - 1, 0) | VM_CH_INIT |
 		      IP_POLL << SDIF_ADDR_SFT |
 		      SDIF_WRN_W | SDIF_PROG;
 		ret = regmap_write(v_map, SDIF_W, val);
@@ -514,6 +526,62 @@ static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt
 	return devm_add_action_or_reset(dev, pvt_reset_control_assert, pvt);
 }
 
+static int pvt_get_active_channel(struct device *dev, struct pvt_device *pvt,
+				  u32 vm_num, u32 ch_num, u8 *vm_idx)
+{
+	u8 vm_active_ch[VM_NUM_MAX];
+	int ret, i, j, k;
+
+	ret = device_property_read_u8_array(dev, "moortec,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_channels.max = ch_num;
+		pvt->vm_channels.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_channels.total += vm_active_ch[i];
+
+			if (vm_active_ch[i] > pvt->vm_channels.max)
+				pvt->vm_channels.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_channels.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++;
+		}
+
+	return 0;
+}
+
 static int mr75203_probe(struct platform_device *pdev)
 {
 	const struct hwmon_channel_info **pvt_info;
@@ -565,7 +633,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++;
@@ -602,43 +669,41 @@ static int mr75203_probe(struct platform_device *pdev)
 	}
 
 	if (vm_num) {
-		u32 total_ch = ch_num * vm_num;
+		u8 vm_idx[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 = pvt_get_active_channel(dev, pvt, vm_num, ch_num, vm_idx);
+		if (ret)
+			return ret;
+
+		in_config = devm_kcalloc(dev, pvt->vm_channels.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_channels.total);
+		in_config[pvt->vm_channels.total] = 0;
 		pvt_in.config = in_config;
 
 		pvt_info[index++] = &pvt_in;
-- 
2.37.1

Re: [PATCH v3 09/19] hwmon: (mr75203) add VM active channel support

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:22:02PM +0000, Eliav Farber wrote:
> Add active channel support per voltage monitor.
> The number of active channels is read from the device-tree.
> When absent in device-tree, all channels are assumed to be used.
> 
> This shall be useful to expose sysfs only for inputs that are connected
> to a voltage source.
> 
> Setting number of active channels to 0, means that entire VM sensor is
> not used.

...

> +struct voltage_device {
> +	u32 vm_map;	/* Map channel number to VM index */
> +	u32 ch_map;	/* Map channel number to channel index */
> +};
> +
> +struct voltage_channels {
> +	u32 total;	/* Total number of channels in all VMs */
> +	u8 max;		/* Maximum number of channels among all VMs */
> +};

Why not convert them to kernel doc?

...

> +	ret = device_property_read_u8_array(dev, "moortec,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;

NIH memset().

> +		pvt->vm_channels.max = ch_num;
> +		pvt->vm_channels.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_channels.total += vm_active_ch[i];
> +
> +			if (vm_active_ch[i] > pvt->vm_channels.max)
> +				pvt->vm_channels.max = vm_active_ch[i];
> +		}
> +	}

...

> +	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++;

How is it different from moving this outside the inner loop as

	k += vm_active_ch[i];

?

> +		}

Missed outer {}.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/19] hwmon: (mr75203) add VM active channel support

[Farber, Eliav]

On 8/31/2022 2:48 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:22:02PM +0000, Eliav Farber wrote:
>> Add active channel support per voltage monitor.
>> The number of active channels is read from the device-tree.
>> When absent in device-tree, all channels are assumed to be used.
>>
>> This shall be useful to expose sysfs only for inputs that are connected
>> to a voltage source.
>>
>> Setting number of active channels to 0, means that entire VM sensor is
>> not used.
>
> ...
>
>> +struct voltage_device {
>> +     u32 vm_map;     /* Map channel number to VM index */
>> +     u32 ch_map;     /* Map channel number to channel index */
>> +};
>> +
>> +struct voltage_channels {
>> +     u32 total;      /* Total number of channels in all VMs */
>> +     u8 max;         /* Maximum number of channels among all VMs */
>> +};
>
> Why not convert them to kernel doc?
>
Fixed in v4.


>> +     ret = device_property_read_u8_array(dev, 
>> "moortec,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;
>
> NIH memset().

Fixed in v4.


>> +             pvt->vm_channels.max = ch_num;
>> +             pvt->vm_channels.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_channels.total += vm_active_ch[i];
>> +
>> +                     if (vm_active_ch[i] > pvt->vm_channels.max)
>> +                             pvt->vm_channels.max = vm_active_ch[i];
>> +             }
>> +     }
>
> ...
>
>> +     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++;
>
> How is it different from moving this outside the inner loop as
>
>        k += vm_active_ch[i];
>
> ?

k is used inside the inner loop, so increasing it outside the inner loop
will result in a different incorrect setting of vm_map and ch_map.

>> +             }
>
> Missed outer {}.

Fixed in v4.


--
Regards, Eliav

[PATCH v3 10/19] dt-bindings: hwmon: (mr75203) add "moortec,vm-pre-scaler" property

[Eliav Farber]

Add support for mr76006 pre-scaler which provides divide-by-2 scaling of
input voltage, which can then be presented for VM for measurement within
its range (the VM input range is limited to -0.1V to 1V).

The new "moortec,vm-pre-scaler" property lists the channels that use a
pre-scaler.

The driver will use this list to multiply the voltage result by 2, to
present to the user the actual voltage input source.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- Add "moortec" prefix to property name.
- Change property format to be a single u8 array.
- Fix typo: scalar --> scaler.

 .../devicetree/bindings/hwmon/moortec,mr75203.yaml    | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index 69cc6caceb2c..4c983d8f8fe7 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -54,6 +54,16 @@ properties:
     default: 16
     $ref: /schemas/types.yaml#definitions/uint8-array
 
+  moortec,vm-pre-scaler:
+    description:
+      moortec,vm-pre-scaler property is an array of channels that use a mr76006
+      pre-scaler to divides the input source by 2.
+      The pre-scaler is used for input sources that exceed the VM input range.
+      The driver uses this information to present to the user the actual value
+      of the voltage source.
+    default: 1
+    $ref: /schemas/types.yaml#definitions/uint8-array
+
 required:
   - compatible
   - reg
@@ -76,5 +86,6 @@ examples:
         clocks = <&osc0>;
         resets = <&rcu0 0x40 7>;
         moortec,vm-active-channels = <0x10 0x05>;
+        moortec,vm-pre-scaler = <5 6>;
         #thermal-sensor-cells = <1>;
     };
-- 
2.37.1

Re: [PATCH v3 10/19] dt-bindings: hwmon: (mr75203) add "moortec,vm-pre-scaler" property

[Rob Herring]

On Tue, Aug 30, 2022 at 07:22:03PM +0000, Eliav Farber wrote:
> Add support for mr76006 pre-scaler which provides divide-by-2 scaling of
> input voltage, which can then be presented for VM for measurement within
> its range (the VM input range is limited to -0.1V to 1V).
> 
> The new "moortec,vm-pre-scaler" property lists the channels that use a
> pre-scaler.
> 
> The driver will use this list to multiply the voltage result by 2, to
> present to the user the actual voltage input source.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> V3 -> V2:
> - Add "moortec" prefix to property name.
> - Change property format to be a single u8 array.
> - Fix typo: scalar --> scaler.
> 
>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml    | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> index 69cc6caceb2c..4c983d8f8fe7 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -54,6 +54,16 @@ properties:
>      default: 16
>      $ref: /schemas/types.yaml#definitions/uint8-array
>  
> +  moortec,vm-pre-scaler:
> +    description:
> +      moortec,vm-pre-scaler property is an array of channels that use a mr76006
> +      pre-scaler to divides the input source by 2.

to divide the

You don't need the property name in the description. The entries are the 
pre-scaler values for each channel? The array index is the channel? If 
so, then 'an array of pre-scaler values for each channel ...'.

> +      The pre-scaler is used for input sources that exceed the VM input range.
> +      The driver uses this information to present to the user the actual value
> +      of the voltage source.
> +    default: 1

It's an array, so a scalar default doesn't make sense.

> +    $ref: /schemas/types.yaml#definitions/uint8-array

Constraints? I assume there's a finite number of channels to set the 
array size bounds for example.

> +
>  required:
>    - compatible
>    - reg
> @@ -76,5 +86,6 @@ examples:
>          clocks = <&osc0>;
>          resets = <&rcu0 0x40 7>;
>          moortec,vm-active-channels = <0x10 0x05>;
> +        moortec,vm-pre-scaler = <5 6>;
>          #thermal-sensor-cells = <1>;
>      };
> -- 
> 2.37.1
> 
> 

Re: [PATCH v3 10/19] dt-bindings: hwmon: (mr75203) add "moortec, vm-pre-scaler" property

[Farber, Eliav]

On 9/2/2022 10:57 PM, Rob Herring wrote:
> On Tue, Aug 30, 2022 at 07:22:03PM +0000, Eliav Farber wrote:
>> Add support for mr76006 pre-scaler which provides divide-by-2 scaling of
>> input voltage, which can then be presented for VM for measurement within
>> its range (the VM input range is limited to -0.1V to 1V).
>>
>> The new "moortec,vm-pre-scaler" property lists the channels that use a
>> pre-scaler.
>>
>> The driver will use this list to multiply the voltage result by 2, to
>> present to the user the actual voltage input source.
>>
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>> V3 -> V2:
>> - Add "moortec" prefix to property name.
>> - Change property format to be a single u8 array.
>> - Fix typo: scalar --> scaler.
>>
>>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml    | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml 
>> b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> index 69cc6caceb2c..4c983d8f8fe7 100644
>> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> @@ -54,6 +54,16 @@ properties:
>>      default: 16
>>      $ref: /schemas/types.yaml#definitions/uint8-array
>>
>> +  moortec,vm-pre-scaler:
>> +    description:
>> +      moortec,vm-pre-scaler property is an array of channels that 
>> use a mr76006
>> +      pre-scaler to divides the input source by 2.
>
> to divide the

Fixed in v4.

> You don't need the property name in the description. The entries are the
> pre-scaler values for each channel? The array index is the channel? If
> so, then 'an array of pre-scaler values for each channel ...'.

Removed property name in the description.

The entries are channel numbers that use a pre-scaler.
Assume 2 VMs, with 16 channels each (so 32 channels in total, numbered
from 0 to 31) and assume: moortec,vm-pre-scaler = /bits/ 8 <5 6 20>;
This means that only channels 5 6 and 20 use a pre-scaler, and the driver
will use a factor of 2 only for these channels.

For v4 I renamed the property name to be "moortec,vm-pre-scaler-x2".
And I changed binding to:

moortec,vm-pre-scaler-x2:
description:
   Defines the channels that use a mr76006 pre-scaler to divide the input
   source by 2.
   The pre-scaler is used for input sources that exceed the VM input range.
   The driver uses this information to present to the user with the actual
   value of the voltage source.
   For channels that are not listed, no pre-scaler is assumed.
   Maximum number of items - total number of channels in all VMs.
   Each channel should not appear more than once.
$ref: /schemas/types.yaml#/definitions/uint8-array

>> +      The pre-scaler is used for input sources that exceed the VM 
>> input range.
>> +      The driver uses this information to present to the user the 
>> actual value
>> +      of the voltage source.
>> +    default: 1
>
> It's an array, so a scalar default doesn't make sense.
What I meant was the in-case channel is not defined the default pre-
scaler value is 1.

>> +    $ref: /schemas/types.yaml#definitions/uint8-array
>
> Constraints? I assume there's a finite number of channels to set the
> array size bounds for example. 
Added some constraints in the new description above.

--
Thanks, Eliav

[PATCH v3 11/19] hwmon: (mr75203) add VM pre-scaler support

[Eliav Farber]

Add mr76006 pre-scaler support to normalize the voltage output result for
channels that use pre-scaler units to get the measurement to be within
the range that the sensor supports.

For channels that are listed in the device-tree to have a pre-scaler, the
voltage result is multiplied by a factor of 2, to represent to the user
the actual voltage source which is measured.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- Modify code according to new property format.

 drivers/hwmon/mr75203.c | 64 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 6925e8490587..1cd5ff6eacce 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -13,10 +13,12 @@
 #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>
 #include <linux/reset.h>
+#include <linux/slab.h>
 
 /* PVT Common register */
 #define PVT_IP_CONFIG	0x04
@@ -108,9 +110,13 @@
 #define PVT_N_CONST		90
 #define PVT_R_CONST		245805
 
+#define PRE_SCALER_X1	1
+#define PRE_SCALER_X2	2
+
 struct voltage_device {
 	u32 vm_map;	/* Map channel number to VM index */
 	u32 ch_map;	/* Map channel number to channel index */
+	u32 pre_scaler;
 };
 
 struct voltage_channels {
@@ -192,7 +198,7 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 {
 	struct pvt_device *pvt = dev_get_drvdata(dev);
 	struct regmap *v_map = pvt->v_map;
-	u32 n, stat;
+	u32 n, stat, pre_scaler;
 	u8 vm_idx, ch_idx;
 	int ret;
 
@@ -217,7 +223,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;
+		pre_scaler = pvt->vd[channel].pre_scaler;
+		*val = pre_scaler * (PVT_N_CONST * n - PVT_R_CONST) >>
+			PVT_CONV_BITS;
 
 		return 0;
 	default:
@@ -582,6 +590,54 @@ static int pvt_get_active_channel(struct device *dev, struct pvt_device *pvt,
 	return 0;
 }
 
+static int pvt_get_pre_scaler(struct device *dev, struct pvt_device *pvt)
+{
+	const struct device_node *np = dev->of_node;
+	u32 total_channels = pvt->vm_channels.total;
+	u32 channel;
+	u8 *pre_scaler_ch_list;
+	int i, ret, num_ch;
+
+	/* Set default pre-scaler value to be 1. */
+	for (i = 0; i < total_channels; i++)
+		pvt->vd[i].pre_scaler = PRE_SCALER_X1;
+
+	/* Get number of channels configured in "moortec,vm-pre-scaler". */
+	num_ch = of_property_count_u8_elems(np, "moortec,vm-pre-scaler");
+	if (num_ch <= 0)
+		return 0;
+
+	pre_scaler_ch_list = kcalloc(total_channels,
+				     sizeof(*pre_scaler_ch_list), GFP_KERNEL);
+	if (!pre_scaler_ch_list)
+		return -ENOMEM;
+
+	/* Get list of all channels that have pre-scaler of 2. */
+	ret = device_property_read_u8_array(dev, "moortec,vm-pre-scaler",
+					    pre_scaler_ch_list, num_ch);
+	if (ret)
+		goto out;
+
+	for (i = 0; i < num_ch; i++) {
+		channel = pre_scaler_ch_list[i];
+
+		if (channel >= total_channels) {
+			dev_err(dev,
+				"invalid channel (%u) in pre-scaler list\n",
+				channel);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		pvt->vd[channel].pre_scaler = PRE_SCALER_X2;
+	}
+
+out:
+	kfree(pre_scaler_ch_list);
+
+	return ret;
+}
+
 static int mr75203_probe(struct platform_device *pdev)
 {
 	const struct hwmon_channel_info **pvt_info;
@@ -697,6 +753,10 @@ static int mr75203_probe(struct platform_device *pdev)
 		if (ret)
 			return ret;
 
+		ret = pvt_get_pre_scaler(dev, pvt);
+		if (ret)
+			return ret;
+
 		in_config = devm_kcalloc(dev, pvt->vm_channels.total + 1,
 					 sizeof(*in_config), GFP_KERNEL);
 		if (!in_config)
-- 
2.37.1

Re: [PATCH v3 11/19] hwmon: (mr75203) add VM pre-scaler support

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:22:04PM +0000, Eliav Farber wrote:
> Add mr76006 pre-scaler support to normalize the voltage output result for
> channels that use pre-scaler units to get the measurement to be within
> the range that the sensor supports.
> 
> For channels that are listed in the device-tree to have a pre-scaler, the
> voltage result is multiplied by a factor of 2, to represent to the user
> the actual voltage source which is measured.

...

> +static int pvt_get_pre_scaler(struct device *dev, struct pvt_device *pvt)
> +{
> +	const struct device_node *np = dev->of_node;
> +	u32 total_channels = pvt->vm_channels.total;
> +	u32 channel;
> +	u8 *pre_scaler_ch_list;
> +	int i, ret, num_ch;
> +
> +	/* Set default pre-scaler value to be 1. */
> +	for (i = 0; i < total_channels; i++)
> +		pvt->vd[i].pre_scaler = PRE_SCALER_X1;
> +
> +	/* Get number of channels configured in "moortec,vm-pre-scaler". */
> +	num_ch = of_property_count_u8_elems(np, "moortec,vm-pre-scaler");

of_ ?!

> +	if (num_ch <= 0)
> +		return 0;
> +
> +	pre_scaler_ch_list = kcalloc(total_channels,
> +				     sizeof(*pre_scaler_ch_list), GFP_KERNEL);
> +	if (!pre_scaler_ch_list)
> +		return -ENOMEM;
> +
> +	/* Get list of all channels that have pre-scaler of 2. */
> +	ret = device_property_read_u8_array(dev, "moortec,vm-pre-scaler",
> +					    pre_scaler_ch_list, num_ch);
> +	if (ret)
> +		goto out;
> +
> +	for (i = 0; i < num_ch; i++) {
> +		channel = pre_scaler_ch_list[i];

> +

Unnecessary blank line.

> +		if (channel >= total_channels) {
> +			dev_err(dev,
> +				"invalid channel (%u) in pre-scaler list\n",
> +				channel);
> +			ret = -EINVAL;

> +			goto out;

Wouldn't

			break;

suffice? (I understand the point, up to you)

> +		}
> +
> +		pvt->vd[channel].pre_scaler = PRE_SCALER_X2;
> +	}
> +
> +out:

out_free:

> +	kfree(pre_scaler_ch_list);
> +
> +	return ret;
> +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 11/19] hwmon: (mr75203) add VM pre-scaler support

[Farber, Eliav]

On 8/31/2022 3:02 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:22:04PM +0000, Eliav Farber wrote:
>> +static int pvt_get_pre_scaler(struct device *dev, struct pvt_device 
>> *pvt)
>> +{
>> +     const struct device_node *np = dev->of_node;
>> +     u32 total_channels = pvt->vm_channels.total;
>> +     u32 channel;
>> +     u8 *pre_scaler_ch_list;
>> +     int i, ret, num_ch;
>> +
>> +     /* Set default pre-scaler value to be 1. */
>> +     for (i = 0; i < total_channels; i++)
>> +             pvt->vd[i].pre_scaler = PRE_SCALER_X1;
>> +
>> +     /* Get number of channels configured in 
>> "moortec,vm-pre-scaler". */
>> +     num_ch = of_property_count_u8_elems(np, "moortec,vm-pre-scaler");
>
> of_ ?!
>
Replaced of_property_count_u8_elems() with
device_property_count_u8().

>> +     if (num_ch <= 0)
>> +             return 0;
>> +
>> +     pre_scaler_ch_list = kcalloc(total_channels,
>> +                                  sizeof(*pre_scaler_ch_list), 
>> GFP_KERNEL);
>> +     if (!pre_scaler_ch_list)
>> +             return -ENOMEM;
>> +
>> +     /* Get list of all channels that have pre-scaler of 2. */
>> +     ret = device_property_read_u8_array(dev, "moortec,vm-pre-scaler",
>> +                                         pre_scaler_ch_list, num_ch);
>> +     if (ret)
>> +             goto out;
>> +
>> +     for (i = 0; i < num_ch; i++) {
>> +             channel = pre_scaler_ch_list[i];
>
>> +
>
> Unnecessary blank line.

Blank line removed.


>> +             if (channel >= total_channels) {
>> +                     dev_err(dev,
>> +                             "invalid channel (%u) in pre-scaler 
>> list\n",
>> +                             channel);
>> +                     ret = -EINVAL;
>
>> +                     goto out;
>
> Wouldn't
>
>                        break;
>
> suffice? (I understand the point, up to you)
I prefer to exit the moment I detect a problem.
For now I can use a break but in the future someone else can add new code
in between that will set ret to 0 and instead of failing driver flow will
continue with incomplete pre-scaler value.
So I prefer keeping it as it.

--
Best regards, Eliav

[PATCH v3 12/19] hwmon: (mr75203) fix voltage equation for negative source input

[Eliav Farber]

According to Moortec Embedded Voltage Monitor (MEVM) series 3 data sheet,
the minimum input signal is -100mv and maximum input signal is +1000mv.
When n was small enough, such that PVT_N_CONST * n < PVT_R_CONST it
resulted in n overflowing to a very large number (since n is u32 type).

This change fixes the problem by casting n to long and replacing shift
right with div operation.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- Fix equation to support negative values instead of limiting value to
  zero.

 drivers/hwmon/mr75203.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 1cd5ff6eacce..d1f090a9baac 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -222,10 +222,11 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 			return ret;
 
 		n &= SAMPLE_DATA_MSK;
+
 		/* Convert the N bitstream count into voltage */
 		pre_scaler = pvt->vd[channel].pre_scaler;
-		*val = pre_scaler * (PVT_N_CONST * n - PVT_R_CONST) >>
-			PVT_CONV_BITS;
+		*val = pre_scaler * (PVT_N_CONST * (long)n - PVT_R_CONST) /
+			(1 << PVT_CONV_BITS);
 
 		return 0;
 	default:
-- 
2.37.1

Re: [PATCH v3 12/19] hwmon: (mr75203) fix voltage equation for negative source input

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:22:05PM +0000, Eliav Farber wrote:
> According to Moortec Embedded Voltage Monitor (MEVM) series 3 data sheet,
> the minimum input signal is -100mv and maximum input signal is +1000mv.
> When n was small enough, such that PVT_N_CONST * n < PVT_R_CONST it
> resulted in n overflowing to a very large number (since n is u32 type).
> 
> This change fixes the problem by casting n to long and replacing shift
> right with div operation.

Fixes tag?

...

>  		n &= SAMPLE_DATA_MSK;
> +

Unrelated change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 12/19] hwmon: (mr75203) fix voltage equation for negative source input

[Farber, Eliav]

On 8/31/2022 3:04 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:22:05PM +0000, Eliav Farber wrote:
>> According to Moortec Embedded Voltage Monitor (MEVM) series 3 data 
>> sheet,
>> the minimum input signal is -100mv and maximum input signal is +1000mv.
>> When n was small enough, such that PVT_N_CONST * n < PVT_R_CONST it
>> resulted in n overflowing to a very large number (since n is u32 type).
>>
>> This change fixes the problem by casting n to long and replacing shift
>> right with div operation.
>
> Fixes tag?

For v4 I modified the commit message to (hopefully) be more
understandable:

"
According to Moortec Embedded Voltage Monitor (MEVM) series 3 data
sheet, the minimum input signal is -100mv and maximum input signal
is +1000mv.

On 64 bit machines sizeof(u32) = 4 and sizeof(long) = 8.
So when measuring a negative input and n is small enough, such that
PVT_N_CONST * n < PVT_R_CONST, it results in n overflowing to a very
large number which is not negative (because 4 MSB bytes of val are 0).

This change fixes the sign problem and supports negative values by
casting n to long and replacing shift right with div operation.
"


> ...
>
>>               n &= SAMPLE_DATA_MSK;
>> +
>
> Unrelated change.

Removed.

--
Thanks, Eliav

Re: [PATCH v3 12/19] hwmon: (mr75203) fix voltage equation for negative source input

[Andy Shevchenko]

On Thu, Sep 01, 2022 at 03:47:39PM +0300, Farber, Eliav wrote:
> On 8/31/2022 3:04 PM, Andy Shevchenko wrote:
> > On Tue, Aug 30, 2022 at 07:22:05PM +0000, Eliav Farber wrote:
> > > According to Moortec Embedded Voltage Monitor (MEVM) series 3 data
> > > sheet,
> > > the minimum input signal is -100mv and maximum input signal is +1000mv.
> > > When n was small enough, such that PVT_N_CONST * n < PVT_R_CONST it
> > > resulted in n overflowing to a very large number (since n is u32 type).
> > > 
> > > This change fixes the problem by casting n to long and replacing shift
> > > right with div operation.
> > 
> > Fixes tag?
> 
> For v4 I modified the commit message to (hopefully) be more
> understandable:
> 
> "
> According to Moortec Embedded Voltage Monitor (MEVM) series 3 data
> sheet, the minimum input signal is -100mv and maximum input signal
> is +1000mv.
> 
> On 64 bit machines sizeof(u32) = 4 and sizeof(long) = 8.
> So when measuring a negative input and n is small enough, such that
> PVT_N_CONST * n < PVT_R_CONST, it results in n overflowing to a very
> large number which is not negative (because 4 MSB bytes of val are 0).
> 
> This change fixes the sign problem and supports negative values by
> casting n to long and replacing shift right with div operation.
> "

What I meant is to add the tag of the commit which this one is fixing.
We have Fixes tag format for that. You may see how it's done by looking
into Git history: git log --grep Fixes:

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 13/19] hwmon: (mr75203) modify the temperature equation according to series 5 datasheet

[Eliav Farber]

Modify the equation and coefficients used 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

Where:
*) G = 60, H = 200, cal5 = 4094, J = -0.1.
*) F = frequency clock in MHz.
*) n is the digital output.

In code, the G, H and J coefficients are multiplied by a factor of 1000
to get the temperature in milli-Celsius.
Final result is clamped in case it exceeds min/max thresholds.

Change is done since it is not clear where the current equation and
coefficients came from.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- Protect from overflow.
- Add temperature clamping.
- Add better documentation.

 drivers/hwmon/mr75203.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index d1f090a9baac..5b8ca1ee4a54 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -103,13 +103,17 @@
 
 #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		200000
+#define PVT_G_CONST		60000
+#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
 
+#define PVT_TEMP_MIN		-40000
+#define PVT_TEMP_MAX		125000
+
 #define PRE_SCALER_X1	1
 #define PRE_SCALER_X2	2
 
@@ -157,13 +161,24 @@ static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
 	return 0;
 }
 
+static long pvt_calc_temp(struct pvt_device *pvt, u32 nbs)
+{
+	/*
+	 * Convert the register value to degrees centigrade temperature:
+	 * T = G + H * (n / cal5 - 0.5) + J * F
+	 */
+	s64 tmp = PVT_G_CONST + PVT_H_CONST * (s64)nbs / PVT_CAL5_CONST -
+		PVT_H_CONST / 2 + PVT_J_CONST * (s64)pvt->ip_freq / HZ_PER_MHZ;
+
+	return clamp_val(tmp, PVT_TEMP_MIN, PVT_TEMP_MAX);
+}
+
 static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
 {
 	struct pvt_device *pvt = dev_get_drvdata(dev);
 	struct regmap *t_map = pvt->t_map;
 	u32 stat, nbs;
 	int ret;
-	u64 tmp;
 
 	switch (attr) {
 	case hwmon_temp_input:
@@ -184,9 +199,7 @@ static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
 		 * Convert the register value to
 		 * degrees centigrade temperature
 		 */
-		tmp = nbs * PVT_H_CONST;
-		do_div(tmp, PVT_CAL5_CONST);
-		*val = tmp - PVT_G_CONST - pvt->ip_freq;
+		*val = pvt_calc_temp(pvt, nbs);
 
 		return 0;
 	default:
@@ -311,7 +324,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 v3 13/19] hwmon: (mr75203) modify the temperature equation according to series 5 datasheet

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:22:06PM +0000, Eliav Farber wrote:
> Modify the equation and coefficients used 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
> 
> Where:
> *) G = 60, H = 200, cal5 = 4094, J = -0.1.
> *) F = frequency clock in MHz.
> *) n is the digital output.
> 
> In code, the G, H and J coefficients are multiplied by a factor of 1000
> to get the temperature in milli-Celsius.
> Final result is clamped in case it exceeds min/max thresholds.
> 
> Change is done since it is not clear where the current equation and

not clear -> unclear

> coefficients came from.

...

> +#define PVT_TEMP_MIN		-40000
> +#define PVT_TEMP_MAX		125000

Unit suffix? _mC perhaps would be enough.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 13/19] hwmon: (mr75203) modify the temperature equation according to series 5 datasheet

[Farber, Eliav]

On 8/31/2022 3:06 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:22:06PM +0000, Eliav Farber wrote:
>> Modify the equation and coefficients used 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
>>
>> Where:
>> *) G = 60, H = 200, cal5 = 4094, J = -0.1.
>> *) F = frequency clock in MHz.
>> *) n is the digital output.
>>
>> In code, the G, H and J coefficients are multiplied by a factor of 1000
>> to get the temperature in milli-Celsius.
>> Final result is clamped in case it exceeds min/max thresholds.
>>
>> Change is done since it is not clear where the current equation and
>
> not clear -> unclear
>
Fixed.

>> coefficients came from.
>
> ...
>
>> +#define PVT_TEMP_MIN         -40000
>> +#define PVT_TEMP_MAX         125000
>
> Unit suffix? _mC perhaps would be enough.

Fixed. Added _mC suffix as you suggested.

--
Thanks, Eliav

[PATCH v3 14/19] dt-bindings: hwmon: (mr75203) add "moortec,ts-series" property

[Eliav Farber]

Add optional "moortec,ts-series" property to define the temperature
equation and coefficients that shall be used to convert the digital
output to value in milli-Celsius.
Supported series: 5 (default) and 6.

Series 5:
  T = G + H * (n / cal5 - 0.5) + J * F
Where: G = 60, H = 200, cal5 = 4094, J = -0.1, F = frequency clock in MHz

Series 6:
   T = G + H * (n / cal5 - 0.5)
Where: G = 57.4, H = 249.4, cal5 = 4096

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- New patch to introduce "moortec,ts-series" property.

 .../devicetree/bindings/hwmon/moortec,mr75203.yaml     | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index 4c983d8f8fe7..ec2dbe7da9c2 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -64,6 +64,16 @@ properties:
     default: 1
     $ref: /schemas/types.yaml#definitions/uint8-array
 
+  moortec,ts-series:
+    description:
+      moortec,ts-series defines the temperature equation and coefficients that
+      shall be used to convert the digital output to value in milli-Celsius.
+      Supported series are 5 and 6.
+    minimum: 5
+    maximum: 6
+    default: 5
+    $ref: /schemas/types.yaml#definitions/uint32
+
 required:
   - compatible
   - reg
-- 
2.37.1

Re: [PATCH v3 14/19] dt-bindings: hwmon: (mr75203) add "moortec,ts-series" property

[Philipp Zabel]

On Di, 2022-08-30 at 19:22 +0000, Eliav Farber wrote:
> Add optional "moortec,ts-series" property to define the temperature
> equation and coefficients that shall be used to convert the digital
> output to value in milli-Celsius.
> Supported series: 5 (default) and 6.

Is this the difference between mr75xxx and mr76xxx series?
If so, should be a compatible "moortec,mr76006" instead?
If the temperature equation could be derived from the compatible, this
property would not be necessary.

regards
Philipp

Re: [PATCH v3 14/19] dt-bindings: hwmon: (mr75203) add "moortec, ts-series" property

[Farber, Eliav]

On 8/31/2022 11:23 AM, Philipp Zabel wrote:
> On Di, 2022-08-30 at 19:22 +0000, Eliav Farber wrote:
>> Add optional "moortec,ts-series" property to define the temperature
>> equation and coefficients that shall be used to convert the digital
>> output to value in milli-Celsius.
>> Supported series: 5 (default) and 6.
>
> Is this the difference between mr75xxx and mr76xxx series?
> If so, should be a compatible "moortec,mr76006" instead?
> If the temperature equation could be derived from the compatible, this
> property would not be necessary.
The PVT (Process, Voltage, Temperature) monitoring logic can be
constructed from many different sub-blocks:
*) CONTROLLER (mr75203) - controlling TS, PD and VM.
*) TS (mr74137) - for measuring temperature in ring.
*) PD (mr74139) - for measuring IO based transistors.
*) VM (mr74138) - for measuring voltage rails across the SoC.
*) Ring oscillators (mr76007/mr76008)
*) Pre-scalers (mr76006)

Besides mr75203 which is digital all other IPs are analog.
There is a single mr75203 and there can be several or none of the other
units.

The kernel driver is only for the controller (mr75203).
The series 5 or 6 is relevant for the TS (mr74137) and not for the
controller (mr75203).
Each of the analog units can have a different series number (for example
we use series 3 of the VM).

That is why I didn't change the compatible of mr75203, and instead added
a TS-series parameter.

--
Regards, Eliav

Re: [PATCH v3 14/19] dt-bindings: hwmon: (mr75203) add "moortec, ts-series" property

[Philipp Zabel]

On Mi, 2022-08-31 at 12:23 +0300, Farber, Eliav wrote:
> On 8/31/2022 11:23 AM, Philipp Zabel wrote:
> > On Di, 2022-08-30 at 19:22 +0000, Eliav Farber wrote:
> > > Add optional "moortec,ts-series" property to define the temperature
> > > equation and coefficients that shall be used to convert the digital
> > > output to value in milli-Celsius.
> > > Supported series: 5 (default) and 6.
> > 
> > Is this the difference between mr75xxx and mr76xxx series?
> > If so, should be a compatible "moortec,mr76006" instead?
> > If the temperature equation could be derived from the compatible, this
> > property would not be necessary.
> The PVT (Process, Voltage, Temperature) monitoring logic can be
> constructed from many different sub-blocks:
> *) CONTROLLER (mr75203) - controlling TS, PD and VM.
> *) TS (mr74137) - for measuring temperature in ring.
> *) PD (mr74139) - for measuring IO based transistors.
> *) VM (mr74138) - for measuring voltage rails across the SoC.
> *) Ring oscillators (mr76007/mr76008)
> *) Pre-scalers (mr76006)
>
> Besides mr75203 which is digital all other IPs are analog.
> There is a single mr75203 and there can be several or none of the other
> units.

Thank you for the explanation, I think this information would be nice
to have in a description in moortec,mr75203.yaml.

> The kernel driver is only for the controller (mr75203).
> The series 5 or 6 is relevant for the TS (mr74137) and not for the
> controller (mr75203).
> Each of the analog units can have a different series number (for example
> we use series 3 of the VM).
> 
> That is why I didn't change the compatible of mr75203, and instead added
> a TS-series parameter.

I see, I mistakenly assumed mr76006 referred to a new controller
version. This also invalidates my comment on patch 4.

regards
Philipp

Re: [PATCH v3 14/19] dt-bindings: hwmon: (mr75203) add "moortec, ts-series" property

[Farber, Eliav]

On 8/31/2022 12:42 PM, Philipp Zabel wrote:
> On Mi, 2022-08-31 at 12:23 +0300, Farber, Eliav wrote:
>> On 8/31/2022 11:23 AM, Philipp Zabel wrote:
>> > On Di, 2022-08-30 at 19:22 +0000, Eliav Farber wrote:
>> > > Add optional "moortec,ts-series" property to define the temperature
>> > > equation and coefficients that shall be used to convert the digital
>> > > output to value in milli-Celsius.
>> > > Supported series: 5 (default) and 6.
>> >
>> > Is this the difference between mr75xxx and mr76xxx series?
>> > If so, should be a compatible "moortec,mr76006" instead?
>> > If the temperature equation could be derived from the compatible, this
>> > property would not be necessary.
>> The PVT (Process, Voltage, Temperature) monitoring logic can be
>> constructed from many different sub-blocks:
>> *) CONTROLLER (mr75203) - controlling TS, PD and VM.
>> *) TS (mr74137) - for measuring temperature in ring.
>> *) PD (mr74139) - for measuring IO based transistors.
>> *) VM (mr74138) - for measuring voltage rails across the SoC.
>> *) Ring oscillators (mr76007/mr76008)
>> *) Pre-scalers (mr76006)
>>
>> Besides mr75203 which is digital all other IPs are analog.
>> There is a single mr75203 and there can be several or none of the other
>> units.
>
> Thank you for the explanation, I think this information would be nice
> to have in a description in moortec,mr75203.yaml. 

For v4 I added a new patch which adds this description in
moortec,mr75203.yaml:

description: |
   A Moortec PVT (Process, Voltage, Temperature) monitoring logic design can
   include many different units.
   Such a design will usually consists of several Moortec's embedded 
analog IPs,
   and a single Moortec controller to configure and control the IPs.

   Some of the Moortec's analog hard IPs that can be used in a design:
   *) Temperature Sensor (TS) - used to monitor core temperature (e.g. 
mr74137).
   *) Voltage Monitor (VM) - used to monitor voltage levels (e.g. mr74138).
   *) Process Detector (PD) - used to assess silicon speed (e.g. mr74139).
   *) Delay Chain - ring oscillator connected to the PD, used to measure IO
      based transistors (e.g. mr76008 ring oscillator at 1.1V, mr76007 ring
      oscillator at 1.8V).
   *) Pre Scaler - provides divide-by-X scaling of input voltage, which 
can then
      be presented for VM for measurement within its range (e.g. mr76006 -
      divide by 2 pre-scaler).

   TS, VM & PD also include a digital interface, which consists of 
configuration
   inputs and measurement outputs.
   The mr75203 binding describes configuration for the controller unit, 
but also
   for some of the analog IPs.

--
Regards, Eliav

Re: [PATCH v3 14/19] dt-bindings: hwmon: (mr75203) add "moortec,ts-series" property

[Rob Herring]

On Tue, Aug 30, 2022 at 07:22:07PM +0000, Eliav Farber wrote:
> Add optional "moortec,ts-series" property to define the temperature
> equation and coefficients that shall be used to convert the digital
> output to value in milli-Celsius.
> Supported series: 5 (default) and 6.
> 
> Series 5:
>   T = G + H * (n / cal5 - 0.5) + J * F
> Where: G = 60, H = 200, cal5 = 4094, J = -0.1, F = frequency clock in MHz
> 
> Series 6:
>    T = G + H * (n / cal5 - 0.5)
> Where: G = 57.4, H = 249.4, cal5 = 4096
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> V3 -> V2:
> - New patch to introduce "moortec,ts-series" property.
> 
>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml     | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> index 4c983d8f8fe7..ec2dbe7da9c2 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -64,6 +64,16 @@ properties:
>      default: 1
>      $ref: /schemas/types.yaml#definitions/uint8-array
>  
> +  moortec,ts-series:
> +    description:
> +      moortec,ts-series defines the temperature equation and coefficients that
> +      shall be used to convert the digital output to value in milli-Celsius.
> +      Supported series are 5 and 6.

No need to state constraints in free-form text descriptions.

> +    minimum: 5
> +    maximum: 6
> +    default: 5
> +    $ref: /schemas/types.yaml#definitions/uint32
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.37.1
> 
> 

Re: [PATCH v3 14/19] dt-bindings: hwmon: (mr75203) add "moortec, ts-series" property

[Farber, Eliav]

On 9/2/2022 10:59 PM, Rob Herring wrote:
> On Tue, Aug 30, 2022 at 07:22:07PM +0000, Eliav Farber wrote:
>> Add optional "moortec,ts-series" property to define the temperature
>> equation and coefficients that shall be used to convert the digital
>> output to value in milli-Celsius.
>> Supported series: 5 (default) and 6.
>>
>> Series 5:
>>   T = G + H * (n / cal5 - 0.5) + J * F
>> Where: G = 60, H = 200, cal5 = 4094, J = -0.1, F = frequency clock in 
>> MHz
>>
>> Series 6:
>>    T = G + H * (n / cal5 - 0.5)
>> Where: G = 57.4, H = 249.4, cal5 = 4096
>>
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>> V3 -> V2:
>> - New patch to introduce "moortec,ts-series" property.
>>
>>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml     | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml 
>> b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> index 4c983d8f8fe7..ec2dbe7da9c2 100644
>> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> @@ -64,6 +64,16 @@ properties:
>>      default: 1
>>      $ref: /schemas/types.yaml#definitions/uint8-array
>>
>> +  moortec,ts-series:
>> +    description:
>> +      moortec,ts-series defines the temperature equation and 
>> coefficients that
>> +      shall be used to convert the digital output to value in 
>> milli-Celsius.
>> +      Supported series are 5 and 6.
>
> No need to state constraints in free-form text descriptions.

Fixed in v4.

[PATCH v3 15/19] hwmon: (mr75203) add support for series 6 temperature equation

[Eliav Farber]

The current equation used in code is aligned to series 5:
T = G + H * (n / cal5 - 0.5) + J * F
Where:
G = 60, H = 200, cal5 = 4094, J = -0.1, F = frequency clock in MHz

Series 6 has a slightly different equation:
T = G + H * (n / cal5 - 0.5)
and a different set of coefficients:
G = 57.4, H = 249.4, cal5 = 4096

This change supports equation and coefficients for both series.
(for series 6, J is set to 0).

The series is determined according to “ts-series” property in device
tree.
If absent, series 5 is assumed to be the default.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- New patch to support temperature sensor series 6 instead of having to
  set all 4 coefficients.

 drivers/hwmon/mr75203.c | 69 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 5b8ca1ee4a54..6a035fd115ca 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -103,10 +103,6 @@
 
 #define PVT_POLL_DELAY_US	20
 #define PVT_POLL_TIMEOUT_US	20000
-#define PVT_H_CONST		200000
-#define PVT_G_CONST		60000
-#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
@@ -114,9 +110,31 @@
 #define PVT_TEMP_MIN		-40000
 #define PVT_TEMP_MAX		125000
 
+/* Temperature coefficients for series 5 */
+#define PVT_SERIES5_H_CONST	200000
+#define PVT_SERIES5_G_CONST	60000
+#define PVT_SERIES5_J_CONST	-100
+#define PVT_SERIES5_CAL5_CONST	4094
+
+/* Temperature coefficients for series 6 */
+#define PVT_SERIES6_H_CONST	249400
+#define PVT_SERIES6_G_CONST	57400
+#define PVT_SERIES6_J_CONST	0
+#define PVT_SERIES6_CAL5_CONST	4096
+
+#define TEMPERATURE_SENSOR_SERIES_5	5
+#define TEMPERATURE_SENSOR_SERIES_6	6
+
 #define PRE_SCALER_X1	1
 #define PRE_SCALER_X2	2
 
+struct temp_coeff {
+	u32 h;
+	u32 g;
+	u32 cal5;
+	s32 j;
+};
+
 struct voltage_device {
 	u32 vm_map;	/* Map channel number to VM index */
 	u32 ch_map;	/* Map channel number to channel index */
@@ -137,6 +155,7 @@ struct pvt_device {
 	struct reset_control	*rst;
 	struct voltage_device	*vd;
 	struct voltage_channels	vm_channels;
+	struct temp_coeff	ts_coeff;
 	u32			t_num;
 	u32			p_num;
 	u32			v_num;
@@ -167,8 +186,9 @@ static long pvt_calc_temp(struct pvt_device *pvt, u32 nbs)
 	 * Convert the register value to degrees centigrade temperature:
 	 * T = G + H * (n / cal5 - 0.5) + J * F
 	 */
-	s64 tmp = PVT_G_CONST + PVT_H_CONST * (s64)nbs / PVT_CAL5_CONST -
-		PVT_H_CONST / 2 + PVT_J_CONST * (s64)pvt->ip_freq / HZ_PER_MHZ;
+	struct temp_coeff *ts_coeff = &pvt->ts_coeff;
+	s64 tmp = ts_coeff->g + ts_coeff->h * (s64)nbs / ts_coeff->cal5 -
+		ts_coeff->h / 2 + ts_coeff->j * (s64)pvt->ip_freq / HZ_PER_MHZ;
 
 	return clamp_val(tmp, PVT_TEMP_MIN, PVT_TEMP_MAX);
 }
@@ -652,6 +672,39 @@ static int pvt_get_pre_scaler(struct device *dev, struct pvt_device *pvt)
 	return ret;
 }
 
+static int pvt_set_temp_coeff(struct device *dev, struct pvt_device *pvt)
+{
+	struct temp_coeff *ts_coeff = &pvt->ts_coeff;
+	const struct device_node *np = dev->of_node;
+	u32 series;
+	int ret;
+
+	/* Incase ts-series property is not defined, use default 5. */
+	ret = of_property_read_u32(np, "moortec,ts-series", &series);
+	if (ret)
+		series = TEMPERATURE_SENSOR_SERIES_5;
+
+	if (series == TEMPERATURE_SENSOR_SERIES_5) {
+		ts_coeff->h = PVT_SERIES5_H_CONST;
+		ts_coeff->g = PVT_SERIES5_G_CONST;
+		ts_coeff->j = PVT_SERIES5_J_CONST;
+		ts_coeff->cal5 = PVT_SERIES5_CAL5_CONST;
+	} else if (series == TEMPERATURE_SENSOR_SERIES_6) {
+		ts_coeff->h = PVT_SERIES6_H_CONST;
+		ts_coeff->g = PVT_SERIES6_G_CONST;
+		ts_coeff->j = PVT_SERIES6_J_CONST;
+		ts_coeff->cal5 = PVT_SERIES6_CAL5_CONST;
+	} else {
+		dev_err(dev, "invalid temperature sensor series (%u)\n",
+			series);
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "temperature sensor series = %u\n", series);
+
+	return 0;
+}
+
 static int mr75203_probe(struct platform_device *pdev)
 {
 	const struct hwmon_channel_info **pvt_info;
@@ -722,6 +775,10 @@ static int mr75203_probe(struct platform_device *pdev)
 		if (ret)
 			return ret;
 
+		ret = pvt_set_temp_coeff(dev, pvt);
+		if (ret)
+			return ret;
+
 		temp_config = devm_kcalloc(dev, ts_num + 1,
 					   sizeof(*temp_config), GFP_KERNEL);
 		if (!temp_config)
-- 
2.37.1

Re: [PATCH v3 15/19] hwmon: (mr75203) add support for series 6 temperature equation

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:22:08PM +0000, Eliav Farber wrote:
> The current equation used in code is aligned to series 5:
> T = G + H * (n / cal5 - 0.5) + J * F
> Where:
> G = 60, H = 200, cal5 = 4094, J = -0.1, F = frequency clock in MHz
> 
> Series 6 has a slightly different equation:
> T = G + H * (n / cal5 - 0.5)
> and a different set of coefficients:
> G = 57.4, H = 249.4, cal5 = 4096
> 
> This change supports equation and coefficients for both series.
> (for series 6, J is set to 0).
> 
> The series is determined according to “ts-series” property in device
> tree.
> If absent, series 5 is assumed to be the default.

...

> -#define PVT_H_CONST		200000
> -#define PVT_G_CONST		60000
> -#define PVT_J_CONST		-100
> -#define PVT_CAL5_CONST		4094

You just introduced them patch before. Please, avoid ping-pong style in
the same series.

...

> +	ret = of_property_read_u32(np, "moortec,ts-series", &series);

of_ ?!

Be consistent. Either you use OF everywhere, or device property APIs.

...

> +	if (ret)
> +		series = TEMPERATURE_SENSOR_SERIES_5;
> +
> +	if (series == TEMPERATURE_SENSOR_SERIES_5) {
> +		ts_coeff->h = PVT_SERIES5_H_CONST;
> +		ts_coeff->g = PVT_SERIES5_G_CONST;
> +		ts_coeff->j = PVT_SERIES5_J_CONST;
> +		ts_coeff->cal5 = PVT_SERIES5_CAL5_CONST;
> +	} else if (series == TEMPERATURE_SENSOR_SERIES_6) {
> +		ts_coeff->h = PVT_SERIES6_H_CONST;
> +		ts_coeff->g = PVT_SERIES6_G_CONST;
> +		ts_coeff->j = PVT_SERIES6_J_CONST;
> +		ts_coeff->cal5 = PVT_SERIES6_CAL5_CONST;
> +	} else {
> +		dev_err(dev, "invalid temperature sensor series (%u)\n",
> +			series);
> +		return -EINVAL;
> +	}

switch-case?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 15/19] hwmon: (mr75203) add support for series 6 temperature equation

[Farber, Eliav]

On 8/31/2022 3:08 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:22:08PM +0000, Eliav Farber wrote:
>> The current equation used in code is aligned to series 5:
>> T = G + H * (n / cal5 - 0.5) + J * F
>> Where:
>> G = 60, H = 200, cal5 = 4094, J = -0.1, F = frequency clock in MHz
>>
>> Series 6 has a slightly different equation:
>> T = G + H * (n / cal5 - 0.5)
>> and a different set of coefficients:
>> G = 57.4, H = 249.4, cal5 = 4096
>>
>> This change supports equation and coefficients for both series.
>> (for series 6, J is set to 0).
>>
>> The series is determined according to “ts-series” property in device
>> tree.
>> If absent, series 5 is assumed to be the default.
>
> ...
>
>> -#define PVT_H_CONST          200000
>> -#define PVT_G_CONST          60000
>> -#define PVT_J_CONST          -100
>> -#define PVT_CAL5_CONST               4094
>
> You just introduced them patch before. Please, avoid ping-pong style in
> the same series.


Fixed for v4.
I now introduce these defines in patch 13 to avoid modifying the above
ones and then remove them:
/* Temperature coefficients for series 5 */
#define PVT_SERIES5_H_CONST    200000
#define PVT_SERIES5_G_CONST    60000
#define PVT_SERIES5_J_CONST    -100
#define PVT_SERIES5_CAL5_CONST    4094

> ...
>
>> +     ret = of_property_read_u32(np, "moortec,ts-series", &series);
>
> of_ ?!
>
> Be consistent. Either you use OF everywhere, or device property APIs.
>

Fixed for v4.
Using device_property_read_u32() instead.

> ...
>
>> +     if (ret)
>> +             series = TEMPERATURE_SENSOR_SERIES_5;
>> +
>> +     if (series == TEMPERATURE_SENSOR_SERIES_5) {
>> +             ts_coeff->h = PVT_SERIES5_H_CONST;
>> +             ts_coeff->g = PVT_SERIES5_G_CONST;
>> +             ts_coeff->j = PVT_SERIES5_J_CONST;
>> +             ts_coeff->cal5 = PVT_SERIES5_CAL5_CONST;
>> +     } else if (series == TEMPERATURE_SENSOR_SERIES_6) {
>> +             ts_coeff->h = PVT_SERIES6_H_CONST;
>> +             ts_coeff->g = PVT_SERIES6_G_CONST;
>> +             ts_coeff->j = PVT_SERIES6_J_CONST;
>> +             ts_coeff->cal5 = PVT_SERIES6_CAL5_CONST;
>> +     } else {
>> +             dev_err(dev, "invalid temperature sensor series (%u)\n",
>> +                     series);
>> +             return -EINVAL;
>> +     }
>
> switch-case? 

Changed to switch-case (will be part of v4).

--
Thanks, Eliav

[PATCH v3 16/19] dt-bindings: hwmon: (mr75203) add coefficient properties for the thermal equation

[Eliav Farber]

Add optional temperature coefficient properties:
 *) moortec,ts-coeff-g
 *) moortec,ts-coeff-h
 *) moortec,ts-coeff-cal5
 *) moortec,ts-coeff-j
If defined they shall be used instead of defaults.

The coefficients were added to device tree on top of the series property
(which can be used to select between series 5 and series 6), because
coefficients can vary between product and product, and code defaults might
not be accurate enough.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V3 -> V2:
- Add "moortec" prefix to property name.

 .../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 ec2dbe7da9c2..a92da6064285 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -74,6 +74,37 @@ properties:
     default: 5
     $ref: /schemas/types.yaml#definitions/uint32
 
+  moortec,ts-coeff-g:
+    description:
+      G coefficient for temperature equation.
+      Value should be multiplied by factor 1000.
+      Default for series 5 = 60000
+      Default for series 6 = 57400
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  moortec,ts-coeff-h:
+    description:
+      H coefficient for temperature equation.
+      Value should be multiplied by factor 1000.
+      Default for series 5 = 200000
+      Default for series 6 = 249400
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  moortec,ts-coeff-cal5:
+    description:
+      cal5 coefficient for temperature equation (can't be 0).
+      Default for series 5 = 4094
+      Default for series 6 = 4096
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  moortec,ts-coeff-j:
+    description:
+      J coefficient for temperature equation.
+      Value should be multiplied by factor 1000.
+      Default for series 5 = -100
+      Default for series 6 = 0
+    $ref: /schemas/types.yaml#/definitions/int32
+
 required:
   - compatible
   - reg
@@ -97,5 +128,7 @@ examples:
         resets = <&rcu0 0x40 7>;
         moortec,vm-active-channels = <0x10 0x05>;
         moortec,vm-pre-scaler = <5 6>;
+        moortec,ts-coeff-g = <61400>;
+        moortec,ts-coeff-h = <253700>;
         #thermal-sensor-cells = <1>;
     };
-- 
2.37.1

Re: [PATCH v3 16/19] dt-bindings: hwmon: (mr75203) add coefficient properties for the thermal equation

[Rob Herring]

On Tue, Aug 30, 2022 at 07:22:09PM +0000, Eliav Farber wrote:
> Add optional temperature coefficient properties:
>  *) moortec,ts-coeff-g
>  *) moortec,ts-coeff-h
>  *) moortec,ts-coeff-cal5
>  *) moortec,ts-coeff-j
> If defined they shall be used instead of defaults.
> 
> The coefficients were added to device tree on top of the series property
> (which can be used to select between series 5 and series 6), because
> coefficients can vary between product and product, and code defaults might
> not be accurate enough.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> V3 -> V2:
> - Add "moortec" prefix to property name.
> 
>  .../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 ec2dbe7da9c2..a92da6064285 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -74,6 +74,37 @@ properties:
>      default: 5
>      $ref: /schemas/types.yaml#definitions/uint32
>  
> +  moortec,ts-coeff-g:
> +    description:
> +      G coefficient for temperature equation.
> +      Value should be multiplied by factor 1000.

If you just multiply the values here, you can specify 'multipleOf: 1000'

Either way, some constraints would be nice. Or is 0 - 2^32 valid?


> +      Default for series 5 = 60000
> +      Default for series 6 = 57400
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  moortec,ts-coeff-h:
> +    description:
> +      H coefficient for temperature equation.
> +      Value should be multiplied by factor 1000.
> +      Default for series 5 = 200000
> +      Default for series 6 = 249400
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  moortec,ts-coeff-cal5:
> +    description:
> +      cal5 coefficient for temperature equation (can't be 0).

minimum: 1

> +      Default for series 5 = 4094
> +      Default for series 6 = 4096
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  moortec,ts-coeff-j:
> +    description:
> +      J coefficient for temperature equation.
> +      Value should be multiplied by factor 1000.
> +      Default for series 5 = -100
> +      Default for series 6 = 0
> +    $ref: /schemas/types.yaml#/definitions/int32
> +
>  required:
>    - compatible
>    - reg
> @@ -97,5 +128,7 @@ examples:
>          resets = <&rcu0 0x40 7>;
>          moortec,vm-active-channels = <0x10 0x05>;
>          moortec,vm-pre-scaler = <5 6>;
> +        moortec,ts-coeff-g = <61400>;
> +        moortec,ts-coeff-h = <253700>;
>          #thermal-sensor-cells = <1>;
>      };
> -- 
> 2.37.1
> 
> 

Re: [PATCH v3 16/19] dt-bindings: hwmon: (mr75203) add coefficient properties for the thermal equation

[Farber, Eliav]

On 9/2/2022 11:03 PM, Rob Herring wrote:
> On Tue, Aug 30, 2022 at 07:22:09PM +0000, Eliav Farber wrote:
>> Add optional temperature coefficient properties:
>>  *) moortec,ts-coeff-g
>>  *) moortec,ts-coeff-h
>>  *) moortec,ts-coeff-cal5
>>  *) moortec,ts-coeff-j
>> If defined they shall be used instead of defaults.
>>
>> The coefficients were added to device tree on top of the series property
>> (which can be used to select between series 5 and series 6), because
>> coefficients can vary between product and product, and code defaults 
>> might
>> not be accurate enough.
>>
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>> V3 -> V2:
>> - Add "moortec" prefix to property name.
>>
>>  .../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 ec2dbe7da9c2..a92da6064285 100644
>> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> @@ -74,6 +74,37 @@ properties:
>>      default: 5
>>      $ref: /schemas/types.yaml#definitions/uint32
>>
>> +  moortec,ts-coeff-g:
>> +    description:
>> +      G coefficient for temperature equation.
>> +      Value should be multiplied by factor 1000.
>
> If you just multiply the values here, you can specify 'multipleOf: 1000'
>
Fixed in v4.

> Either way, some constraints would be nice. Or is 0 - 2^32 valid?

g and h can't be 0, and since they must be multiple of 1000 I added:
minimum: 1000
j is negative or 0, so I added:
maximum: 0

>
>> +      Default for series 5 = 60000
>> +      Default for series 6 = 57400
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  moortec,ts-coeff-h:
>> +    description:
>> +      H coefficient for temperature equation.
>> +      Value should be multiplied by factor 1000.
>> +      Default for series 5 = 200000
>> +      Default for series 6 = 249400
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  moortec,ts-coeff-cal5:
>> +    description:
>> +      cal5 coefficient for temperature equation (can't be 0).
>
> minimum: 1 
Fixed in v4.

[PATCH v3 17/19] hwmon: (mr75203) parse temperature coefficients from device-tree

[Eliav Farber]

Use thermal coefficients from the device tree if they exist.
Otherwise, use default values according to the series (5 or 6).
All coefficients can be used or only part of them.

The coefficients shall be used for fine tuning the default values since
coefficients can vary between product and product.

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

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 6a035fd115ca..d9fc5d225868 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -676,7 +676,8 @@ static int pvt_set_temp_coeff(struct device *dev, struct pvt_device *pvt)
 {
 	struct temp_coeff *ts_coeff = &pvt->ts_coeff;
 	const struct device_node *np = dev->of_node;
-	u32 series;
+	u32 series, coeff_h, coeff_g, coeff_cal5;
+	s32 coeff_j;
 	int ret;
 
 	/* Incase ts-series property is not defined, use default 5. */
@@ -702,6 +703,32 @@ static int pvt_set_temp_coeff(struct device *dev, struct pvt_device *pvt)
 
 	dev_dbg(dev, "temperature sensor series = %u\n", series);
 
+	/* Override ts-coeff-h/g/j/cal5 if they are defined. */
+	ret = of_property_read_u32(np, "moortec,ts-coeff-h", &coeff_h);
+	if (!ret)
+		ts_coeff->h = coeff_h;
+
+	ret = of_property_read_u32(np, "moortec,ts-coeff-g", &coeff_g);
+	if (!ret)
+		ts_coeff->g = coeff_g;
+
+	ret = of_property_read_s32(np, "moortec,ts-coeff-j", &coeff_j);
+	if (!ret)
+		ts_coeff->j = coeff_j;
+
+	ret = of_property_read_u32(np, "moortec,ts-coeff-cal5", &coeff_cal5);
+	if (!ret) {
+		if (coeff_cal5 == 0) {
+			dev_err(dev, "moortec,ts-coeff-cal5 can't be 0\n");
+			return -EINVAL;
+		}
+
+		ts_coeff->cal5 = coeff_cal5;
+	}
+
+	dev_dbg(dev, "ts-coeff: h = %u, g = %u, j = %d, cal5 = %u\n",
+		ts_coeff->h, ts_coeff->g, ts_coeff->j, ts_coeff->cal5);
+
 	return 0;
 }
 
-- 
2.37.1

Re: [PATCH v3 17/19] hwmon: (mr75203) parse temperature coefficients from device-tree

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:22:10PM +0000, Eliav Farber wrote:
> Use thermal coefficients from the device tree if they exist.
> Otherwise, use default values according to the series (5 or 6).
> All coefficients can be used or only part of them.
> 
> The coefficients shall be used for fine tuning the default values since
> coefficients can vary between product and product.

...

> +	ret = of_property_read_u32(np, "moortec,ts-coeff-h", &coeff_h);

of_ ?! Ditto for the rest.

> +	if (!ret)
> +		ts_coeff->h = coeff_h;

...

> +	ret = of_property_read_s32(np, "moortec,ts-coeff-j", &coeff_j);
> +	if (!ret)
> +		ts_coeff->j = coeff_j;

You may avoid conditional:

	_property_read_s32(..., "moortec,ts-coeff-j", &ts_coeff->j);


...

> +	ret = of_property_read_u32(np, "moortec,ts-coeff-cal5", &coeff_cal5);
> +	if (!ret) {

> +		if (coeff_cal5 == 0) {
> +			dev_err(dev, "moortec,ts-coeff-cal5 can't be 0\n");
> +			return -EINVAL;
> +		}

Code shouldn't be a YAML validator. Drop this and make sure you have correct
DT schema.

> +		ts_coeff->cal5 = coeff_cal5;
> +	}

Also see above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 17/19] hwmon: (mr75203) parse temperature coefficients from device-tree

[Farber, Eliav]

On 8/31/2022 3:11 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:22:10PM +0000, Eliav Farber wrote:
>> Use thermal coefficients from the device tree if they exist.
>> Otherwise, use default values according to the series (5 or 6).
>> All coefficients can be used or only part of them.
>>
>> The coefficients shall be used for fine tuning the default values since
>> coefficients can vary between product and product.
>
> ...
>
>> +     ret = of_property_read_u32(np, "moortec,ts-coeff-h", &coeff_h);
>
> of_ ?! Ditto for the rest.


Fixed for v4.
I replaced it with device_property_read_u32().

>> +     if (!ret)
>> +             ts_coeff->h = coeff_h;
>
> ...
>
>> +     ret = of_property_read_s32(np, "moortec,ts-coeff-j", &coeff_j);
>> +     if (!ret)
>> +             ts_coeff->j = coeff_j;
>
> You may avoid conditional:
>
>        _property_read_s32(..., "moortec,ts-coeff-j", &ts_coeff->j);


Fixed for v4.
I removed the condition.

> ...
>
>> +     ret = of_property_read_u32(np, "moortec,ts-coeff-cal5", 
>> &coeff_cal5);
>> +     if (!ret) {
>
>> +             if (coeff_cal5 == 0) {
>> +                     dev_err(dev, "moortec,ts-coeff-cal5 can't be 
>> 0\n");
>> +                     return -EINVAL;
>> +             }
>
> Code shouldn't be a YAML validator. Drop this and make sure you have 
> correct
> DT schema.


Fixed for v4.
I dropped the validation check.
The YAML already mentions that it can't be 0.

>> +             ts_coeff->cal5 = coeff_cal5;
>> +     }

--
Thanks, Eliav

[PATCH v3 18/19] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Eliav Farber]

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

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

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 d9fc5d225868..8c30f0521452 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>
@@ -153,6 +154,7 @@ struct pvt_device {
 	struct regmap		*v_map;
 	struct clk		*clk;
 	struct reset_control	*rst;
+	struct dentry		*dbgfs_dir;
 	struct voltage_device	*vd;
 	struct voltage_channels	vm_channels;
 	struct temp_coeff	ts_coeff;
@@ -162,6 +164,198 @@ struct pvt_device {
 	u32			ip_freq;
 };
 
+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)
 {
@@ -814,6 +1008,8 @@ 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;
+
+		pvt_ts_dbgfs_create(pvt, dev);
 	}
 
 	if (pd_num) {
-- 
2.37.1

Re: [PATCH v3 18/19] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:22:11PM +0000, Eliav Farber wrote:
> This change adds debugfs to read and write temperature sensor coefficients
> - g, h, j and cal5.
> 
> The coefficients can vary between product and product, so it can be very
> useful to be able to modify them on the fly during the calibration
> process.
> 
> e.g.:
> 
> cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
> 4096
> 
> echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g

...

> +	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;
> +	}

No, just don't check the return value of debugfs API calls.

> +	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);

debugfs has helpers for POD types, use them and shrink your code by ~80%.

...

> +	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;

return devm_add_...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 18/19] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Farber, Eliav]

On 8/31/2022 3:14 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:22:11PM +0000, Eliav Farber wrote:
>> This change adds debugfs to read and write temperature sensor 
>> coefficients
>> - g, h, j and cal5.
>>
>> The coefficients can vary between product and product, so it can be very
>> useful to be able to modify them on the fly during the calibration
>> process.
>>
>> e.g.:
>>
>> cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
>> 4096
>>
>> echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g
>
> ...
>
>> +     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;
>> +     }
>
> No, just don't check the return value of debugfs API calls.
>
Do you mean that I should just do:
debugfs_create_dir(dev_name(dev), NULL);
Can you please explain why it's OK to ignore the return value?

>> +     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);
>
> debugfs has helpers for POD types, use them and shrink your code by ~80%.
>
Fixed for v4.
I used debugfs_create_u32() for ts_coeff_h, ts_coeff_g and ts_coeff_j.
For ts_coeff_cal5 I still use debugfs_create_file() because I must make
sure it is not set to 0.

>> +     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;
>
> return devm_add_...
Fixed for v4.

--
Thanks, Eliav

Re: [PATCH v3 18/19] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Guenter Roeck]

On Thu, Sep 01, 2022 at 09:54:21AM +0300, Farber, Eliav wrote:
> On 8/31/2022 3:14 PM, Andy Shevchenko wrote:
> > On Tue, Aug 30, 2022 at 07:22:11PM +0000, Eliav Farber wrote:
> > > This change adds debugfs to read and write temperature sensor
> > > coefficients
> > > - g, h, j and cal5.
> > > 
> > > The coefficients can vary between product and product, so it can be very
> > > useful to be able to modify them on the fly during the calibration
> > > process.
> > > 
> > > e.g.:
> > > 
> > > cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
> > > 4096
> > > 
> > > echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g
> > 
> > ...
> > 
> > > +     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;
> > > +     }
> > 
> > No, just don't check the return value of debugfs API calls.
> > 
> Do you mean that I should just do:
> debugfs_create_dir(dev_name(dev), NULL);
> Can you please explain why it's OK to ignore the return value?
> 

Because that is how debugfs code is supposed to be implemented.
Other debugfs code checks if the dir parameter passed to it is NULL
and do nothing if it is. This reduces code size overall by avoiding
unnecessary error checks.

Guenter

Re: [PATCH v3 18/19] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Andy Shevchenko]

On Thu, Sep 01, 2022 at 09:54:21AM +0300, Farber, Eliav wrote:
> On 8/31/2022 3:14 PM, Andy Shevchenko wrote:
> > On Tue, Aug 30, 2022 at 07:22:11PM +0000, Eliav Farber wrote:

...

> > > +     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;
> > > +     }
> > 
> > No, just don't check the return value of debugfs API calls.
> > 
> Do you mean that I should just do:
> debugfs_create_dir(dev_name(dev), NULL);
> Can you please explain why it's OK to ignore the return value?

Author of the debugfs is speaking:
https://patchwork.kernel.org/project/linux-wireless/patch/20190122152151.16139-38-gregkh@linuxfoundation.org/

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 19/19] 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 8c30f0521452..6a19f3d2a708 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -404,7 +404,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;
@@ -445,7 +445,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;
@@ -542,19 +542,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,
@@ -567,7 +567,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,
@@ -580,7 +580,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,
@@ -594,39 +594,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,
@@ -654,7 +654,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,
@@ -667,7 +667,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,
@@ -681,7 +681,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;
 	}
 
@@ -967,7 +967,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

Re: [PATCH v3 19/19] hwmon: (mr75203) fix coding style space errors

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 07:22:12PM +0000, Eliav Farber wrote:
> Fix: "ERROR: space required before the open parenthesis '('"

This patch may have other fixes like adding new blank lines (noted in one
of the patches in the series), etc.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 19/19] hwmon: (mr75203) fix coding style space errors

[Farber, Eliav]

On 8/31/2022 3:15 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:22:12PM +0000, Eliav Farber wrote:
>> Fix: "ERROR: space required before the open parenthesis '('"
>
> This patch may have other fixes like adding new blank lines (noted in one
> of the patches in the series), etc.
This patch fixed a specific space error which existed before my changes
and repeated many time.
I fixed the blank line I added a previous patch (but is it isn’t an error
reported by checkpatch).

--
Regards, Eliav

Re: [PATCH v3 19/19] hwmon: (mr75203) fix coding style space errors

[Guenter Roeck]

On Thu, Sep 01, 2022 at 05:21:43PM +0300, Farber, Eliav wrote:
> On 8/31/2022 3:15 PM, Andy Shevchenko wrote:
> > On Tue, Aug 30, 2022 at 07:22:12PM +0000, Eliav Farber wrote:
> > > Fix: "ERROR: space required before the open parenthesis '('"
> > 
> > This patch may have other fixes like adding new blank lines (noted in one
> > of the patches in the series), etc.
> This patch fixed a specific space error which existed before my changes
> and repeated many time.
> I fixed the blank line I added a previous patch (but is it isn’t an error
> reported by checkpatch).

That should really be fixed where it was introduced, not be introduced
and fixed here.

Guenter

Re: [PATCH v3 19/19] hwmon: (mr75203) fix coding style space errors

[Farber, Eliav]

On 9/1/2022 5:46 PM, Guenter Roeck wrote:
> On Thu, Sep 01, 2022 at 05:21:43PM +0300, Farber, Eliav wrote:
>> On 8/31/2022 3:15 PM, Andy Shevchenko wrote:
>> > On Tue, Aug 30, 2022 at 07:22:12PM +0000, Eliav Farber wrote:
>> > > Fix: "ERROR: space required before the open parenthesis '('"
>> >
>> > This patch may have other fixes like adding new blank lines (noted 
>> in one
>> > of the patches in the series), etc.
>> This patch fixed a specific space error which existed before my changes
>> and repeated many time.
>> I fixed the blank line I added a previous patch (but is it isn’t an 
>> error
>> reported by checkpatch).
>
> That should really be fixed where it was introduced, not be introduced
> and fixed here.


So what do you suggest?
I can drop the patch from this series and ignore it or move it to be the
first patch in the series, or publish it separately later on.
I had it because it was annoying seeing existing checkpatch errors when
I came to check my change.

--
Thanks, Eliav

Re: [PATCH v3 19/19] hwmon: (mr75203) fix coding style space errors

[Guenter Roeck]

On 9/1/22 08:31, Farber, Eliav wrote:
> On 9/1/2022 5:46 PM, Guenter Roeck wrote:
>> On Thu, Sep 01, 2022 at 05:21:43PM +0300, Farber, Eliav wrote:
>>> On 8/31/2022 3:15 PM, Andy Shevchenko wrote:
>>> > On Tue, Aug 30, 2022 at 07:22:12PM +0000, Eliav Farber wrote:
>>> > > Fix: "ERROR: space required before the open parenthesis '('"
>>> >
>>> > This patch may have other fixes like adding new blank lines (noted in one
>>> > of the patches in the series), etc.
>>> This patch fixed a specific space error which existed before my changes
>>> and repeated many time.
>>> I fixed the blank line I added a previous patch (but is it isn’t an error
>>> reported by checkpatch).
>>
>> That should really be fixed where it was introduced, not be introduced
>> and fixed here.
> 
> 
> So what do you suggest?
> I can drop the patch from this series and ignore it or move it to be the
> first patch in the series, or publish it separately later on.
> I had it because it was annoying seeing existing checkpatch errors when
> I came to check my change.
> 

Sorry, you lost me. I referred to "I fixed the blank line I added
a previous patch". That should not be fixed in this patch but be dropped
from the patch where you introduced it. Did I misunderstand your comment ?

Guenter

Re: [PATCH v3 19/19] hwmon: (mr75203) fix coding style space errors

[Farber, Eliav]

On 9/1/2022 8:09 PM, Guenter Roeck wrote:
> On 9/1/22 08:31, Farber, Eliav wrote:
>> On 9/1/2022 5:46 PM, Guenter Roeck wrote:
>>> On Thu, Sep 01, 2022 at 05:21:43PM +0300, Farber, Eliav wrote:
>>>> On 8/31/2022 3:15 PM, Andy Shevchenko wrote:
>>>> > On Tue, Aug 30, 2022 at 07:22:12PM +0000, Eliav Farber wrote:
>>>> > > Fix: "ERROR: space required before the open parenthesis '('"
>>>> >
>>>> > This patch may have other fixes like adding new blank lines 
>>>> (noted in one
>>>> > of the patches in the series), etc.
>>>> This patch fixed a specific space error which existed before my 
>>>> changes
>>>> and repeated many time.
>>>> I fixed the blank line I added a previous patch (but is it isn’t an 
>>>> error
>>>> reported by checkpatch).
>>>
>>> That should really be fixed where it was introduced, not be introduced
>>> and fixed here.
>>
>>
>> So what do you suggest?
>> I can drop the patch from this series and ignore it or move it to be the
>> first patch in the series, or publish it separately later on.
>> I had it because it was annoying seeing existing checkpatch errors when
>> I came to check my change.
>>
>
> Sorry, you lost me. I referred to "I fixed the blank line I added
> a previous patch". That should not be fixed in this patch but be dropped
> from the patch where you introduced it. Did I misunderstand your 
> comment ?


Indeed I fixed the blank line in the original patch in which it was
introduced.
Patch #19 only addresses previously existing space related checkpatch
errors.

--
Thanks, Eliav