| Message ID | 20220830192212.28570-1-farbere@amazon.com |
|---|---|
| Headers | show |
| Series | Variety of fixes and new features for mr75203 driver | expand |
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;
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
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;
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..
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
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.
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.
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.
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(-)