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@xxxxxxxxxx> > > > > --- > > > > 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
