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

["Farber, Eliav" <farbere@amazon.com>]

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