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 v3 and v4:
*) Provide a Fixes tag for all fixes in the series.
*) Start series with fixes.
*) New patch to add description in moortec,mr75203.yaml.
*) New patch to add moortec to vendor-prefixes.
*) Fix moortec,mr75203.yaml checker errors.
*) Remove validation of device-tree parameters.
*) Fix per patch specific comments (detailed in each patch).
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 (21):
hwmon: (mr75203) fix coding style space errors
dt-bindings: hwmon: (mr75203) fix "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 and vm_num to the actual number of
used sensors
hwmon: (mr75203) fix voltage equation for negative source input
hwmon: (mr75203) fix multi-channel voltage reading
hwmon: (mr75203) enable polling for all VM channels
dt-bindings: hwmon: (mr75203) add description for Moortec's PVT
controller
dt-bindings: hwmon: (mr75203) change "resets" property to be optional
hwmon: (mr75203) skip reset-control deassert for SOCs that don't
support it
dt-bindings: vendor-prefixes: add vendor prefix for Moortec
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-x2" property
hwmon: (mr75203) add VM pre-scaler x2 support
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
.../bindings/hwmon/moortec,mr75203.yaml | 97 ++++-
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
drivers/hwmon/mr75203.c | 387 +++++++++++++++---
3 files changed, 421 insertions(+), 65 deletions(-)
--
2.37.1
Add support for mr76006 pre-scaler which provides divide-by-2 scaling
of the input voltage, so that it can be presented to the VM for
measurement within its range (the VM input range is limited from -0.1V
to 1V).
The driver reads from the device-tree all the channels that use the
mr76006 pre-scaler and multiplies the voltage result by a factor of 2,
to represent to the user with the actual voltage input source.
Channels that are not in the device-tree are multiplied by a factor
of 1.
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> V3:
- Replace of_property_count_u8_elems() with device_property_count_u8().
- Remove unnecessary blank line.
- Remove code that validated the YAML.
V3 -> V2:
- Modify code according to new property format.
drivers/hwmon/mr75203.c | 55 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 5aa694fd9118..66f151082c11 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -17,6 +17,7 @@
#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,17 +109,24 @@
#define PVT_N_CONST 90
#define PVT_R_CONST 245805
+#define PRE_SCALER_X1 1
+#define PRE_SCALER_X2 2
+
/**
* struct voltage_device - VM single input parameters.
* @vm_map: Map channel number to VM index.
* @ch_map: Map channel number to channel index.
+ * @pre_scaler: Pre scaler value (1 or 2) used to normalize the voltage output
+ * result.
*
* The structure provides mapping between channel-number (0..N-1) to VM-index
* (0..num_vm-1) and channel-index (0..ch_num-1) where N = num_vm * ch_num.
+ * It also provides normalization factor for the VM equation.
*/
struct voltage_device {
u32 vm_map;
u32 ch_map;
+ u32 pre_scaler;
};
/**
@@ -207,8 +215,8 @@ 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, pre_scaler;
u8 vm_idx, ch_idx;
- u32 n, stat;
int ret;
if (channel >= pvt->vm_channels.total)
@@ -232,7 +240,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 * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS);
+ pre_scaler = pvt->vd[channel].pre_scaler;
+ *val = pre_scaler * (PVT_N_CONST * (long)n - PVT_R_CONST) /
+ (1 << PVT_CONV_BITS);
return 0;
default:
@@ -594,6 +604,43 @@ 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)
+{
+ u8 *pre_scaler_ch_list;
+ int i, ret, num_ch;
+ u32 channel;
+
+ /* Set default pre-scaler value to be 1. */
+ for (i = 0; i < pvt->vm_channels.total; i++)
+ pvt->vd[i].pre_scaler = PRE_SCALER_X1;
+
+ /* Get number of channels configured in "moortec,vm-pre-scaler-x2". */
+ num_ch = device_property_count_u8(dev, "moortec,vm-pre-scaler-x2");
+ if (num_ch <= 0)
+ return 0;
+
+ pre_scaler_ch_list = kcalloc(num_ch, 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-x2",
+ pre_scaler_ch_list, num_ch);
+ if (ret)
+ goto out;
+
+ for (i = 0; i < num_ch; i++) {
+ channel = pre_scaler_ch_list[i];
+ pvt->vd[channel].pre_scaler = PRE_SCALER_X2;
+ }
+
+out:
+ kfree(pre_scaler_ch_list);
+
+ return ret;
+}
+
static int mr75203_probe(struct platform_device *pdev)
{
u32 ts_num, vm_num, pd_num, ch_num, val, index, i;
@@ -709,6 +756,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
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 <[email protected]>
---
V4 -> V3:
- Add 'multipleOf: 1000' instead of plain text.
- Add minimum/maximum for some of the new properties.
V3 -> V2:
- Add "moortec" prefix to property name.
.../bindings/hwmon/moortec,mr75203.yaml | 37 +++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index 28656e9d9059..5e3d060290a9 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -100,6 +100,41 @@ properties:
default: 5
$ref: /schemas/types.yaml#/definitions/uint32
+ moortec,ts-coeff-g:
+ description:
+ G coefficient for temperature equation.
+ Default for series 5 = 60000
+ Default for series 6 = 57400
+ multipleOf: 1000
+ minimum: 1000
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ moortec,ts-coeff-h:
+ description:
+ H coefficient for temperature equation.
+ Default for series 5 = 200000
+ Default for series 6 = 249400
+ multipleOf: 1000
+ minimum: 1000
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ moortec,ts-coeff-cal5:
+ description:
+ cal5 coefficient for temperature equation.
+ Default for series 5 = 4094
+ Default for series 6 = 4096
+ minimum: 1
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ moortec,ts-coeff-j:
+ description:
+ J coefficient for temperature equation.
+ Default for series 5 = -100
+ Default for series 6 = 0
+ multipleOf: 1000
+ maximum: 0
+ $ref: /schemas/types.yaml#/definitions/int32
+
required:
- compatible
- reg
@@ -123,5 +158,7 @@ examples:
resets = <&rcu0 0x40 7>;
moortec,vm-active-channels = /bits/ 8 <0x10 0x05>;
moortec,vm-pre-scaler-x2 = /bits/ 8 <5 6 20>;
+ moortec,ts-coeff-g = <61400>;
+ moortec,ts-coeff-h = <253700>;
#thermal-sensor-cells = <1>;
};
--
2.37.1
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.
Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for Moortec MR75203 PVT controller")
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> V3:
- Keep lines sorted according to length.
V3 -> V2:
- Remove configuration of ip-polling register to a separate commit.
- Explain the fix.
drivers/hwmon/mr75203.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 78dc471e843c..69f38c05b02d 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;
};
@@ -178,14 +180,15 @@ 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;
+ u8 vm_idx, ch_idx;
u32 n, stat;
- u8 vm_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;
@@ -499,8 +502,8 @@ static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt
static int mr75203_probe(struct platform_device *pdev)
{
+ u32 ts_num, vm_num, pd_num, ch_num, val, index, i;
const struct hwmon_channel_info **pvt_info;
- u32 ts_num, vm_num, pd_num, val, index, i;
struct device *dev = &pdev->dev;
u32 *temp_config, *in_config;
struct device *hwmon_dev;
@@ -541,9 +544,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++;
@@ -580,7 +585,7 @@ static int mr75203_probe(struct platform_device *pdev)
}
if (vm_num) {
- u32 num = vm_num;
+ u32 total_ch;
ret = pvt_get_regmap(pdev, "vm", pvt);
if (ret)
@@ -604,20 +609,20 @@ static int mr75203_probe(struct platform_device *pdev)
for (i = 0; i < vm_num; i++)
if (pvt->vm_idx[i] >= vm_num ||
pvt->vm_idx[i] == 0xff) {
- num = i;
pvt->v_num = i;
vm_num = i;
break;
}
}
- in_config = devm_kcalloc(dev, num + 1,
+ total_ch = ch_num * vm_num;
+ in_config = devm_kcalloc(dev, total_ch + 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, total_ch);
+ in_config[total_ch] = 0;
pvt_in.config = in_config;
pvt_info[index++] = &pvt_in;
--
2.37.1
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 “moortec,ts-series” property in
the device tree.
If absent, series 5 is assumed to be the default.
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> V3:
- Replace of_property_read_u32() with device_property_read_u32().
- Use switch-case instead of if-else.
V3 -> V2:
- New patch to support temperature sensor series 6 instead of having to
set all 4 coefficients.
drivers/hwmon/mr75203.c | 66 ++++++++++++++++++++++++++++++++++++++---
1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index f98fb0f3d2fd..ba5abd4065b3 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -115,6 +115,15 @@
#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
@@ -147,6 +156,13 @@ struct voltage_channels {
u8 max;
};
+struct temp_coeff {
+ u32 h;
+ u32 g;
+ u32 cal5;
+ s32 j;
+};
+
struct pvt_device {
struct regmap *c_map;
struct regmap *t_map;
@@ -156,6 +172,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;
@@ -186,10 +203,12 @@ 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_SERIES5_G_CONST +
- PVT_SERIES5_H_CONST * (s64)nbs / PVT_SERIES5_CAL5_CONST -
- PVT_SERIES5_H_CONST / 2 +
- PVT_SERIES5_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_mC, PVT_TEMP_MAX_mC);
}
@@ -658,6 +677,41 @@ 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;
+ u32 series;
+ int ret;
+
+ /* Incase ts-series property is not defined, use default 5. */
+ ret = device_property_read_u32(dev, "moortec,ts-series", &series);
+ if (ret)
+ series = TEMPERATURE_SENSOR_SERIES_5;
+
+ switch (series) {
+ case 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;
+ break;
+ case 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;
+ break;
+ default:
+ 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)
{
u32 ts_num, vm_num, pd_num, ch_num, val, index, i;
@@ -728,6 +782,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
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 <[email protected]>
---
V4 -> V3:
- Replace of_property_read_u32() with device_property_read_u32().
- Fix "Code shouldn't be a YAML validator".
- Read directly to ts_coeff-> parameter to avoid conditional if.
drivers/hwmon/mr75203.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index ba5abd4065b3..8baa99a9ea83 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -709,6 +709,15 @@ 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. */
+ device_property_read_u32(dev, "moortec,ts-coeff-h", &ts_coeff->h);
+ device_property_read_u32(dev, "moortec,ts-coeff-g", &ts_coeff->g);
+ device_property_read_u32(dev, "moortec,ts-coeff-j", &ts_coeff->j);
+ device_property_read_u32(dev, "moortec,ts-coeff-cal5", &ts_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
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 unclear where the current equation and
coefficients came from.
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> V3:
- Change 'not clear' to 'unclear' in commit message.
- Add _mC prefix to temperature mix/max defines.
- Add SERIES5 to coefficient defines.
V3 -> V2:
- Protect from overflow.
- Add temperature clamping.
- Add better documentation.
drivers/hwmon/mr75203.c | 33 +++++++++++++++++++++++++--------
1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 66f151082c11..f98fb0f3d2fd 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -102,13 +102,19 @@
#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_CONV_BITS 10
#define PVT_N_CONST 90
#define PVT_R_CONST 245805
+#define PVT_TEMP_MIN_mC -40000
+#define PVT_TEMP_MAX_mC 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
+
#define PRE_SCALER_X1 1
#define PRE_SCALER_X2 2
@@ -174,13 +180,26 @@ 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_SERIES5_G_CONST +
+ PVT_SERIES5_H_CONST * (s64)nbs / PVT_SERIES5_CAL5_CONST -
+ PVT_SERIES5_H_CONST / 2 +
+ PVT_SERIES5_J_CONST * (s64)pvt->ip_freq / HZ_PER_MHZ;
+
+ return clamp_val(tmp, PVT_TEMP_MIN_mC, PVT_TEMP_MAX_mC);
+}
+
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:
@@ -201,9 +220,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:
@@ -327,7 +344,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
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 <[email protected]>
---
V4 -> V3:
- Remove constraints in free-form text descriptions.
V3 -> V2:
- New patch to introduce "moortec,ts-series" property.
.../devicetree/bindings/hwmon/moortec,mr75203.yaml | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index de92a21d5552..28656e9d9059 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -91,6 +91,15 @@ properties:
Each channel should not appear more than once.
$ref: /schemas/types.yaml#/definitions/uint8-array
+ moortec,ts-series:
+ description:
+ Definition of the temperature equation and coefficients that shall be
+ used to convert the digital output to value in milli-Celsius.
+ minimum: 5
+ maximum: 6
+ default: 5
+ $ref: /schemas/types.yaml#/definitions/uint32
+
required:
- compatible
- reg
--
2.37.1
Bug - in case "intel,vm-map" is missing in device-tree ,'num' is set
to 0, and no voltage channel infos are allocated.
The reason num is set to 0 when "intel,vm-map" is missing is to set the
entire pvt->vm_idx[] with incremental channel numbers, but it didn't
take into consideration that same num is used later in devm_kcalloc().
If "intel,vm-map" does exist there is no need to set the unspecified
channels with incremental numbers, because the unspecified channels
can't be accessed in pvt_read_in() which is the only other place besides
the probe functions that uses pvt->vm_idx[].
This change fixes the bug by moving the incremental channel numbers
setting to be done only if "intel,vm-map" property is defined (starting
loop from 0), and removing 'num = 0'.
Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for Moortec MR75203 PVT controller")
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> v3:
- Simplify the fix by not removing the local num variable (it is removed as
part of a later commit).
drivers/hwmon/mr75203.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 8adfbb15453f..0ead576694a1 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -594,7 +594,12 @@ 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 ||
@@ -604,13 +609,6 @@ static int mr75203_probe(struct platform_device *pdev)
}
}
- /*
- * 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;
-
in_config = devm_kcalloc(dev, num + 1,
sizeof(*in_config), GFP_KERNEL);
if (!in_config)
--
2.37.1
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 or vm_num
later on in the code.
Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for Moortec MR75203 PVT controller")
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> v3:
- Update also vm_num to actual number of VMs.
drivers/hwmon/mr75203.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 0ead576694a1..a209f5d95f4b 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -605,6 +605,8 @@ static int mr75203_probe(struct platform_device *pdev)
if (pvt->vm_idx[i] >= vm_num ||
pvt->vm_idx[i] == 0xff) {
num = i;
+ pvt->v_num = i;
+ vm_num = i;
break;
}
}
--
2.37.1
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 <[email protected]>
---
V4 -> V3:
- Fix DT checker errors.
V3 -> V2:
- Add "moortec" prefix to property name.
- Add explanation why this change is needed.
.../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 9454576ebb73..2aa4c3618596 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -70,6 +70,15 @@ properties:
"#thermal-sensor-cells":
const: 1
+ moortec,vm-active-channels:
+ description:
+ Defines the number of channels per VM that are actually used and are
+ connected to some input source.
+ Maximum number of items - number of VMs.
+ Maximum value of each item - number of channels.
+ Minimum value of each item - 0 (which means entire VM sensor is nou used).
+ $ref: /schemas/types.yaml#/definitions/uint8-array
+
required:
- compatible
- reg
@@ -91,5 +100,6 @@ examples:
intel,vm-map = [03 01 04 ff ff];
clocks = <&osc0>;
resets = <&rcu0 0x40 7>;
+ moortec,vm-active-channels = /bits/ 8 <0x10 0x05>;
#thermal-sensor-cells = <1>;
};
--
2.37.1
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 <[email protected]>
---
V4 -> V3:
- Convert comments to kernel doc.
- Repalce for loop with memset.
- Add {} to outer for loop.
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 | 121 ++++++++++++++++++++++++++++++++--------
1 file changed, 99 insertions(+), 22 deletions(-)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index c94e89c94cb3..5aa694fd9118 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,31 @@
#define PVT_N_CONST 90
#define PVT_R_CONST 245805
+/**
+ * struct voltage_device - VM single input parameters.
+ * @vm_map: Map channel number to VM index.
+ * @ch_map: Map channel number to channel index.
+ *
+ * The structure provides mapping between channel-number (0..N-1) to VM-index
+ * (0..num_vm-1) and channel-index (0..ch_num-1) where N = num_vm * ch_num.
+ */
+struct voltage_device {
+ u32 vm_map;
+ u32 ch_map;
+};
+
+/**
+ * struct voltage_channels - VM channel count.
+ * @total: Total number of channels in all VMs.
+ * @max: Maximum number of channels among all VMs.
+ *
+ * The structure provides channel count information across all VMs.
+ */
+struct voltage_channels {
+ u32 total;
+ u8 max;
+};
+
struct pvt_device {
struct regmap *c_map;
struct regmap *t_map;
@@ -113,12 +140,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 +211,11 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
u32 n, stat;
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 +415,7 @@ static int pvt_init(struct pvt_device *pvt)
if (ret)
return ret;
- val = (BIT(pvt->c_num) - 1) | VM_CH_INIT |
+ val = (BIT(pvt->vm_channels.max) - 1) | VM_CH_INIT |
IP_POLL << SDIF_ADDR_SFT | SDIF_WRN_W | SDIF_PROG;
ret = regmap_write(v_map, SDIF_W, val);
if (ret < 0)
@@ -513,6 +540,60 @@ 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 "moortec,vm-active-channels" property is not defined,
+ * we assume each VM sensor has all of its channels active.
+ */
+ memset(vm_active_ch, ch_num, vm_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, "moortec,vm_active_ch" = <5 2 4>:
+ * 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)
{
u32 ts_num, vm_num, pd_num, ch_num, val, index, i;
@@ -564,7 +645,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++;
@@ -601,44 +681,41 @@ static int mr75203_probe(struct platform_device *pdev)
}
if (vm_num) {
- u32 total_ch;
+ 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) {
pvt->v_num = i;
vm_num = i;
break;
}
}
- total_ch = ch_num * vm_num;
- 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
According to Moortec Embedded Voltage Monitor (MEVM) series 3 data
sheet, the minimum input signal is -100mv and maximum input signal
is +1000mv.
The equation used to convert the digital word to voltage uses mixed
types (*val signed and n unsigned), and on 64 bit machines also has
different size, since sizeof(u32) = 4 and sizeof(long) = 8.
So when measuring a negative input, n will be small enough, such that
PVT_N_CONST * n < PVT_R_CONST, and the result of
(PVT_N_CONST * n - PVT_R_CONST) will overflow to a very big positive
32 bit number. Then when storing the result in *val it will be the same
value just in 64 bit (instead of it representing a negative number which
will what happen when sizeof(long) = 4).
When -1023 <= (PVT_N_CONST * n - PVT_R_CONST) <= -1
dividing the number by 1024 should result of in 0, but because ">> 10"
is used it results in -1 (0xf...fffff).
This change fixes the sign problem and supports negative values by
casting n to long and replacing the shift right with div operation.
Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for Moortec MR75203 PVT controller")
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> V3:
- Remove unrelated change (add of empty line).
V3 -> V2:
- Fix equation to support negative values instead of limiting value to
zero.
drivers/hwmon/mr75203.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index a209f5d95f4b..78dc471e843c 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -202,7 +202,7 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
n &= SAMPLE_DATA_MSK;
/* Convert the N bitstream count into voltage */
- *val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS;
+ *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS);
return 0;
default:
--
2.37.1
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 <[email protected]>
---
V3 -> V2:
- Move configuration of ip-polling register from previous patch to a
separate commit.
V4 -> V3:
- Replace GENMASK(pvt->c_num - 1, 0) with (BIT(pvt->c_num) - 1).
drivers/hwmon/mr75203.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 69f38c05b02d..5f2b11a2bf5f 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -388,6 +388,19 @@ static int pvt_init(struct pvt_device *pvt)
if (ret)
return ret;
+ val = (BIT(pvt->c_num) - 1) | 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
Fix: "ERROR: space required before the open parenthesis '('"
Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for Moortec MR75203 PVT controller")
Signed-off-by: Eliav Farber <[email protected]>
---
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 046523d47c29..8adfbb15453f 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -155,7 +155,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;
@@ -197,7 +197,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), &n);
- if(ret < 0)
+ if (ret < 0)
return ret;
n &= SAMPLE_DATA_MSK;
@@ -291,19 +291,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,
@@ -316,7 +316,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,
@@ -329,7 +329,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,
@@ -343,39 +343,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,
@@ -389,7 +389,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,
@@ -402,7 +402,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,
@@ -416,7 +416,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;
}
@@ -535,7 +535,7 @@ static int mr75203_probe(struct platform_device *pdev)
return dev_err_probe(dev, ret, "cannot deassert reset control\n");
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
Change "resets" property to be optional instead of required, for SOCs
that don't support a reset controller.
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> V3:
- Fix "reset" to "resets".
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 5e41aed5891a..9454576ebb73 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -75,7 +75,6 @@ required:
- reg
- reg-names
- clocks
- - resets
- "#thermal-sensor-cells"
additionalProperties: false
--
2.37.1
Add support for mr76006 pre-scaler which provides divide-by-2 scaling of
the input voltage, so that it can be presented to the VM for measurement
within its range (the VM input range is limited to -0.1V to 1V).
The new "moortec,vm-pre-scaler-x2" property lists the channels that use
the mr76006 pre-scaler.
The driver will use this list to multiply the voltage result by 2, to
present to the user with the actual voltage input source.
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> V3:
- Rename "moortec,vm-pre-scaler" to "moortec,vm-pre-scaler-x2".
- Added mximum number if items in description.
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 | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index 2aa4c3618596..de92a21d5552 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -79,6 +79,18 @@ properties:
Minimum value of each item - 0 (which means entire VM sensor is nou used).
$ref: /schemas/types.yaml#/definitions/uint8-array
+ 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
+
required:
- compatible
- reg
@@ -101,5 +113,6 @@ examples:
clocks = <&osc0>;
resets = <&rcu0 0x40 7>;
moortec,vm-active-channels = /bits/ 8 <0x10 0x05>;
+ moortec,vm-pre-scaler-x2 = /bits/ 8 <5 6 20>;
#thermal-sensor-cells = <1>;
};
--
2.37.1
This changes adds a detailed description for the mr75203 controller and
for some of the analog IPs controlled by it.
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> V3:
- New patch to add description.
.../bindings/hwmon/moortec,mr75203.yaml | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
index 8ea97e774364..5e41aed5891a 100644
--- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -9,6 +9,32 @@ title: Moortec Semiconductor MR75203 PVT Controller bindings
maintainers:
- Rahul Tanwar <[email protected]>
+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 (mr75203) 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.
+
+ Some of the units have number of series, each series can have slightly
+ different characteristics.
+
+ The mr75203 binding describes configuration for the controller unit, but also
+ for some of the analog IPs.
+
properties:
compatible:
const: moortec,mr75203
--
2.37.1
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".
Fixes: 748022ef093f ("hwmon: Add DT bindings schema for PVT controller")
Signed-off-by: Eliav Farber <[email protected]>
---
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 b79f069a04c2..8ea97e774364 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
Add device-tree vendor prefix for Moortec Semiconductor Ltd.
Website: https://moortec.com/
Moortec were acquired by Synopsys so link above leads to:
https://www.synopsys.com/solutions/silicon-lifecycle-management.html
Signed-off-by: Eliav Farber <[email protected]>
---
V4 -> V3:
- New patch to add moortec to vendor-prefixes.
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 2f0151e9f6be..9bf7c2cd7e89 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -819,6 +819,8 @@ patternProperties:
description: MNT Research GmbH
"^modtronix,.*":
description: Modtronix Engineering
+ "^moortec,.*":
+ description: Moortec Semiconductor Ltd.
"^mosaixtech,.*":
description: Mosaix Technologies, Inc.
"^motorola,.*":
--
2.37.1
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 <[email protected]>
---
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 5f2b11a2bf5f..c94e89c94cb3 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -541,14 +541,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
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 <[email protected]>
---
V4 -> V3:
- Remove check of the debugfs_create_dir() return value.
- Use debugfs_create_u32() instead of debugfs_create_file().
- Return devm_add_action_or_reset() without checking return value and printing
an error message on failure.
drivers/hwmon/mr75203.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 8baa99a9ea83..e3670075fae4 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>
@@ -170,6 +171,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;
@@ -179,6 +181,30 @@ struct pvt_device {
u32 ip_freq;
};
+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)
+{
+ pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
+
+ debugfs_create_u32("ts_coeff_h", 0644, pvt->dbgfs_dir,
+ &pvt->ts_coeff.h);
+ debugfs_create_u32("ts_coeff_g", 0644, pvt->dbgfs_dir,
+ &pvt->ts_coeff.g);
+ debugfs_create_u32("ts_coeff_j", 0644, pvt->dbgfs_dir,
+ &pvt->ts_coeff.j);
+ debugfs_create_u32("ts_coeff_cal5", 0644, pvt->dbgfs_dir,
+ &pvt->ts_coeff.cal5);
+
+ return devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, pvt);
+}
+
static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
u32 attr, int channel)
{
@@ -803,6 +829,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
On Tue, Sep 06, 2022 at 08:33:40AM +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.
>
> The equation used to convert the digital word to voltage uses mixed
> types (*val signed and n unsigned), and on 64 bit machines also has
> different size, since sizeof(u32) = 4 and sizeof(long) = 8.
>
> So when measuring a negative input, n will be small enough, such that
> PVT_N_CONST * n < PVT_R_CONST, and the result of
> (PVT_N_CONST * n - PVT_R_CONST) will overflow to a very big positive
> 32 bit number. Then when storing the result in *val it will be the same
> value just in 64 bit (instead of it representing a negative number which
> will what happen when sizeof(long) = 4).
>
> When -1023 <= (PVT_N_CONST * n - PVT_R_CONST) <= -1
> dividing the number by 1024 should result of in 0, but because ">> 10"
> is used it results in -1 (0xf...fffff).
>
> This change fixes the sign problem and supports negative values by
> casting n to long and replacing the shift right with div operation.
This is really downside of C...
...
> - *val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS;
> + *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS);
Wondering if we can use BIT(PVT_CONV_BITS) for two (quite unlikely to happen,
I hope) purposes:
1) Somebody copies such code where PVT_CONV_BITS analogue can be 31,
which is according to C standard is UB (undefined behaviour).
2) It makes shorter the line and also drops the pattern where some
dumb robot may propose a patch to basically revert the division
change.
--
With Best Regards,
Andy Shevchenko
On 9/6/2022 3:03 PM, Andy Shevchenko wrote:
> On Tue, Sep 06, 2022 at 08:33:40AM +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.
>>
>> The equation used to convert the digital word to voltage uses mixed
>> types (*val signed and n unsigned), and on 64 bit machines also has
>> different size, since sizeof(u32) = 4 and sizeof(long) = 8.
>>
>> So when measuring a negative input, n will be small enough, such that
>> PVT_N_CONST * n < PVT_R_CONST, and the result of
>> (PVT_N_CONST * n - PVT_R_CONST) will overflow to a very big positive
>> 32 bit number. Then when storing the result in *val it will be the same
>> value just in 64 bit (instead of it representing a negative number which
>> will what happen when sizeof(long) = 4).
>>
>> When -1023 <= (PVT_N_CONST * n - PVT_R_CONST) <= -1
>> dividing the number by 1024 should result of in 0, but because ">> 10"
>> is used it results in -1 (0xf...fffff).
>>
>> This change fixes the sign problem and supports negative values by
>> casting n to long and replacing the shift right with div operation.
>
> This is really downside of C...
>
> ...
>
>> - *val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS;
>> + *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 <<
>> PVT_CONV_BITS);
>
> Wondering if we can use BIT(PVT_CONV_BITS) for two (quite unlikely to
> happen,
> I hope) purposes:
>
> 1) Somebody copies such code where PVT_CONV_BITS analogue can be 31,
> which is according to C standard is UB (undefined behaviour).
>
> 2) It makes shorter the line and also drops the pattern where some
> dumb robot may propose a patch to basically revert the division
> change.
I originally tried to use BIT(PVT_CONV_BITS) but it gave a different
result.
e.g.
If n = 2720
*val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS) = 0
*val = (PVT_N_CONST * (long)n - PVT_R_CONST) / BIT(PVT_CONV_BITS) =
18014398509481983
I can try fitting it in one line, either by adding a define for
(1 << PVT_CONV_BITS) or exceeding 80 characters, but keep in mind that
in a later patch (#15) it gets even longer (and I must use more than
one line) since it is multiplied by a pre-scaler factor.
--
Regards, Eliav
On Tue, Sep 06, 2022 at 08:33:41AM +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.
...
> + total_ch = ch_num * vm_num;
> + in_config = devm_kcalloc(dev, total_ch + 1,
> sizeof(*in_config), GFP_KERNEL);
Strictly speaking this should be `size_add(size_mul(...) ...)` construction
from overflow.h.
total_ch = size_mul(ch_num, vm_num);
in_config = devm_kcalloc(dev, size_add(total_ch, 1),
sizeof(*in_config), GFP_KERNEL);
Alternatively before doing all these, add a check
if (array3_size(ch_num, vm_num, sizeof(*in_config)) < SIZE_MAX - sizeof(*in_config))
return -EOVERFLOW;
But this is a bit monstrous. Seems like the above looks and feels better.
Also for backporting purposes perhaps it's fine to do without using those macro
helpers.
> if (!in_config)
> return -ENOMEM;
--
With Best Regards,
Andy Shevchenko
On Tue, Sep 06, 2022 at 08:33:35AM +0000, Eliav Farber wrote:
> 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
For all code patches (means no DT ones)
Reviewed-by: Andy Shevchenko <[email protected]>
> ---------
>
> Changes between v3 and v4:
> *) Provide a Fixes tag for all fixes in the series.
> *) Start series with fixes.
> *) New patch to add description in moortec,mr75203.yaml.
> *) New patch to add moortec to vendor-prefixes.
> *) Fix moortec,mr75203.yaml checker errors.
> *) Remove validation of device-tree parameters.
> *) Fix per patch specific comments (detailed in each patch).
>
> 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 (21):
> hwmon: (mr75203) fix coding style space errors
> dt-bindings: hwmon: (mr75203) fix "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 and vm_num to the actual number of
> used sensors
> hwmon: (mr75203) fix voltage equation for negative source input
> hwmon: (mr75203) fix multi-channel voltage reading
> hwmon: (mr75203) enable polling for all VM channels
> dt-bindings: hwmon: (mr75203) add description for Moortec's PVT
> controller
> dt-bindings: hwmon: (mr75203) change "resets" property to be optional
> hwmon: (mr75203) skip reset-control deassert for SOCs that don't
> support it
> dt-bindings: vendor-prefixes: add vendor prefix for Moortec
> 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-x2" property
> hwmon: (mr75203) add VM pre-scaler x2 support
> 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
>
> .../bindings/hwmon/moortec,mr75203.yaml | 97 ++++-
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> drivers/hwmon/mr75203.c | 387 +++++++++++++++---
> 3 files changed, 421 insertions(+), 65 deletions(-)
>
> --
> 2.37.1
>
--
With Best Regards,
Andy Shevchenko
On Tue, Sep 06, 2022 at 04:27:13PM +0300, Farber, Eliav wrote:
> On 9/6/2022 3:03 PM, Andy Shevchenko wrote:
> > On Tue, Sep 06, 2022 at 08:33:40AM +0000, Eliav Farber wrote:
...
> > > -???????????? *val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS;
> > > +???????????? *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 <<
> > > PVT_CONV_BITS);
> >
> > Wondering if we can use BIT(PVT_CONV_BITS) for two (quite unlikely to
> > happen,
> > I hope) purposes:
> >
> > 1) Somebody copies such code where PVT_CONV_BITS analogue can be 31,
> > ? which is according to C standard is UB (undefined behaviour).
> >
> > 2) It makes shorter the line and also drops the pattern where some
> > ? dumb robot may propose a patch to basically revert the division
> > ? change.
> I originally tried to use BIT(PVT_CONV_BITS) but it gave a different
> result.
> e.g.
> If n = 2720
> *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS) = 0
> *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / BIT(PVT_CONV_BITS) =
> 18014398509481983
>
> I can try fitting it in one line, either by adding a define for
> (1 << PVT_CONV_BITS) or exceeding 80 characters, but keep in mind that
> in a later patch (#15) it gets even longer (and I must use more than
> one line) since it is multiplied by a pre-scaler factor.
Don't get me wrong, it's not about style, it's about preventing
followup "fixes" of this. All the problems here due to (hidden)
unsigned type(s).
What you can do is to add a good comment on top of that line
explaining why division instead of right shift and why BIT()
may not be used (because it's unsigned).
--
With Best Regards,
Andy Shevchenko
On Tue, Sep 06, 2022 at 08:33:37AM +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".
>
> Fixes: 748022ef093f ("hwmon: Add DT bindings schema for PVT controller")
> Signed-off-by: Eliav Farber <[email protected]>
> ---
> V3 -> V2:
> - Change this patch to be first in the series.
> - Add explanation why "intel,vm-map" is not required.
>
I don't see how this change warrants dropping Rob's Acked-by tag.
Am I missing something ?
Guenter
> 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 b79f069a04c2..8ea97e774364 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"
On Tue, Sep 06, 2022 at 08:33:36AM +0000, Eliav Farber wrote:
> Fix: "ERROR: space required before the open parenthesis '('"
>
> Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for Moortec MR75203 PVT controller")
Coding style "fixes" do not fix the code. I consider using the Fixes: tag
for those to close to an abuse of that tag (and it would be great if that
was spelled out somewhere). As it is, I can not with good conscience apply
this patch to the mainline kernel (especially not for -rc5), meaning the
entire series will have to wait for the next release window unless there
are no conflicts.
Guenter
> Signed-off-by: Eliav Farber <[email protected]>
> ---
> 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 046523d47c29..8adfbb15453f 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -155,7 +155,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;
> @@ -197,7 +197,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), &n);
> - if(ret < 0)
> + if (ret < 0)
> return ret;
>
> n &= SAMPLE_DATA_MSK;
> @@ -291,19 +291,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,
> @@ -316,7 +316,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,
> @@ -329,7 +329,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,
> @@ -343,39 +343,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,
> @@ -389,7 +389,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,
> @@ -402,7 +402,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,
> @@ -416,7 +416,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;
> }
>
> @@ -535,7 +535,7 @@ static int mr75203_probe(struct platform_device *pdev)
> return dev_err_probe(dev, ret, "cannot deassert reset control\n");
>
> 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;
On Tue, Sep 06, 2022 at 08:33:47AM +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 <[email protected]>
> ---
> V4 -> V3:
> - Fix DT checker errors.
>
> V3 -> V2:
> - Add "moortec" prefix to property name.
> - Add explanation why this change is needed.
>
> .../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 9454576ebb73..2aa4c3618596 100644
> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> @@ -70,6 +70,15 @@ properties:
> "#thermal-sensor-cells":
> const: 1
>
> + moortec,vm-active-channels:
> + description:
> + Defines the number of channels per VM that are actually used and are
> + connected to some input source.
> + Maximum number of items - number of VMs.
> + Maximum value of each item - number of channels.
> + Minimum value of each item - 0 (which means entire VM sensor is nou used).
s/nou/not/
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> +
> required:
> - compatible
> - reg
> @@ -91,5 +100,6 @@ examples:
> intel,vm-map = [03 01 04 ff ff];
> clocks = <&osc0>;
> resets = <&rcu0 0x40 7>;
> + moortec,vm-active-channels = /bits/ 8 <0x10 0x05>;
> #thermal-sensor-cells = <1>;
> };
On 9/6/2022 5:10 PM, Andy Shevchenko wrote:
> On Tue, Sep 06, 2022 at 08:33:41AM +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.
>
> ...
>
>> + total_ch = ch_num * vm_num;
>> + in_config = devm_kcalloc(dev, total_ch + 1,
>> sizeof(*in_config), GFP_KERNEL);
>
> Strictly speaking this should be `size_add(size_mul(...) ...)`
> construction
> from overflow.h.
>
> total_ch = size_mul(ch_num, vm_num);
> in_config = devm_kcalloc(dev, size_add(total_ch, 1),
> sizeof(*in_config), GFP_KERNEL);
>
> Alternatively before doing all these, add a check
>
> if (array3_size(ch_num, vm_num, sizeof(*in_config)) <
> SIZE_MAX - sizeof(*in_config))
> return -EOVERFLOW;
>
> But this is a bit monstrous. Seems like the above looks and feels better.
>
> Also for backporting purposes perhaps it's fine to do without using
> those macro
> helpers.
According to the driver code total_ch is a u32 variable while vm_num
and ch_num are both limited to a value of 31:
#define VM_NUM_MSK GENMASK(20, 16)
#define VM_NUM_SFT 16
#define CH_NUM_MSK GENMASK(31, 24)
#define CH_NUM_SFT 24
In addition the PVT Controller Series 3+ Specification mentions that
the actual maximum values are even smaller – 8 for vm_num and 16 for
ch_num.
Therefore we are very far from a scenario of an overflow.
Do you still think overflow protection in necessary?
--
Thanks, Eliav
On 9/6/2022 8:08 PM, Guenter Roeck wrote:
> On Tue, Sep 06, 2022 at 08:33:47AM +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 <[email protected]>
>> ---
>> V4 -> V3:
>> - Fix DT checker errors.
>>
>> V3 -> V2:
>> - Add "moortec" prefix to property name.
>> - Add explanation why this change is needed.
>>
>> .../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 9454576ebb73..2aa4c3618596 100644
>> --- a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
>> @@ -70,6 +70,15 @@ properties:
>> "#thermal-sensor-cells":
>> const: 1
>>
>> + moortec,vm-active-channels:
>> + description:
>> + Defines the number of channels per VM that are actually used
>> and are
>> + connected to some input source.
>> + Maximum number of items - number of VMs.
>> + Maximum value of each item - number of channels.
>> + Minimum value of each item - 0 (which means entire VM sensor
>> is nou used).
>
> s/nou/not/
Typo fixed in v5.
--
Thanks, Eliav
On 9/6/2022 7:53 PM, Guenter Roeck wrote:
> On Tue, Sep 06, 2022 at 08:33:37AM +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".
>>
>> Fixes: 748022ef093f ("hwmon: Add DT bindings schema for PVT controller")
>> Signed-off-by: Eliav Farber <[email protected]>
>> ---
>> V3 -> V2:
>> - Change this patch to be first in the series.
>> - Add explanation why "intel,vm-map" is not required.
>>
>
> I don't see how this change warrants dropping Rob's Acked-by tag.
> Am I missing something ?
My apology. I wasn’t aware I had to keep the Acked-by tag.
I'll add it in v5.
--
Regards, Eliav
On 9/6/2022 7:52 PM, Guenter Roeck wrote:
> On Tue, Sep 06, 2022 at 08:33:36AM +0000, Eliav Farber wrote:
>> Fix: "ERROR: space required before the open parenthesis '('"
>>
>> Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for
>> Moortec MR75203 PVT controller")
>
> Coding style "fixes" do not fix the code. I consider using the Fixes: tag
> for those to close to an abuse of that tag (and it would be great if that
> was spelled out somewhere). As it is, I can not with good conscience
> apply
> this patch to the mainline kernel (especially not for -rc5), meaning the
> entire series will have to wait for the next release window unless there
> are no conflicts.
Because as you mentioned it is not a functional fix in the code I’ll
remove the Fixes tag in v5.
I checked older kernel versions and patch applies without conflicts.
That’s also why I moved it to be first in the series, so it will be
before any of my other changes that night cause merge problems for
other branches.
--
Thanks, Eliav
On Wed, Sep 07, 2022 at 08:15:36AM +0300, Farber, Eliav wrote:
> On 9/6/2022 5:10 PM, Andy Shevchenko wrote:
> > On Tue, Sep 06, 2022 at 08:33:41AM +0000, Eliav Farber wrote:
...
> > > + total_ch = ch_num * vm_num;
> > > + in_config = devm_kcalloc(dev, total_ch + 1,
> > > sizeof(*in_config), GFP_KERNEL);
> >
> > Strictly speaking this should be `size_add(size_mul(...) ...)`
> > construction
> > from overflow.h.
> >
> > total_ch = size_mul(ch_num, vm_num);
> > in_config = devm_kcalloc(dev, size_add(total_ch, 1),
> > sizeof(*in_config), GFP_KERNEL);
> >
> > Alternatively before doing all these, add a check
> >
> > if (array3_size(ch_num, vm_num, sizeof(*in_config)) <
> > SIZE_MAX - sizeof(*in_config))
> > return -EOVERFLOW;
> >
> > But this is a bit monstrous. Seems like the above looks and feels better.
> >
> > Also for backporting purposes perhaps it's fine to do without using
> > those macro
> > helpers.
> According to the driver code total_ch is a u32 variable while vm_num
> and ch_num are both limited to a value of 31:
>
> #define VM_NUM_MSK GENMASK(20, 16)
> #define VM_NUM_SFT 16
> #define CH_NUM_MSK GENMASK(31, 24)
> #define CH_NUM_SFT 24
>
> In addition the PVT Controller Series 3+ Specification mentions that
> the actual maximum values are even smaller – 8 for vm_num and 16 for
> ch_num.
> Therefore we are very far from a scenario of an overflow.
> Do you still think overflow protection in necessary?
Like I said "Strictly..." Means it's up to you, but allocations are
usually be protected against the overflows.
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 07, 2022 at 09:50:00AM +0300, Farber, Eliav wrote:
> On 9/6/2022 7:52 PM, Guenter Roeck wrote:
> > On Tue, Sep 06, 2022 at 08:33:36AM +0000, Eliav Farber wrote:
> > > Fix: "ERROR: space required before the open parenthesis '('"
> > >
> > > Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for
> > > Moortec MR75203 PVT controller")
> >
> > Coding style "fixes" do not fix the code. I consider using the Fixes: tag
> > for those to close to an abuse of that tag (and it would be great if that
> > was spelled out somewhere). As it is, I can not with good conscience
> > apply
> > this patch to the mainline kernel (especially not for -rc5), meaning the
> > entire series will have to wait for the next release window unless there
> > are no conflicts.
> Because as you mentioned it is not a functional fix in the code I’ll
> remove the Fixes tag in v5.
> I checked older kernel versions and patch applies without conflicts.
> That’s also why I moved it to be first in the series, so it will be
> before any of my other changes that night cause merge problems for
> other branches.
If it's not a fix, it should go _after_ real fixes.
And by nature this kind of patch is usually at the end of the series.
--
With Best Regards,
Andy Shevchenko
On 9/6/22 23:28, Farber, Eliav wrote:
> On 9/6/2022 7:53 PM, Guenter Roeck wrote:
>> On Tue, Sep 06, 2022 at 08:33:37AM +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".
>>>
>>> Fixes: 748022ef093f ("hwmon: Add DT bindings schema for PVT controller")
>>> Signed-off-by: Eliav Farber <[email protected]>
>>> ---
>>> V3 -> V2:
>>> - Change this patch to be first in the series.
>>> - Add explanation why "intel,vm-map" is not required.
>>>
>>
>> I don't see how this change warrants dropping Rob's Acked-by tag.
>> Am I missing something ?
>
> My apology. I wasn’t aware I had to keep the Acked-by tag.
> I'll add it in v5.
>
"have" is such a strong word. Just keep in mind that unnecessarily
dropping tags tends to result in irritated reviewers.
Thanks,
Guenter
On 9/7/22 07:42, Andy Shevchenko wrote:
> On Wed, Sep 07, 2022 at 09:50:00AM +0300, Farber, Eliav wrote:
>> On 9/6/2022 7:52 PM, Guenter Roeck wrote:
>>> On Tue, Sep 06, 2022 at 08:33:36AM +0000, Eliav Farber wrote:
>>>> Fix: "ERROR: space required before the open parenthesis '('"
>>>>
>>>> Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for
>>>> Moortec MR75203 PVT controller")
>>>
>>> Coding style "fixes" do not fix the code. I consider using the Fixes: tag
>>> for those to close to an abuse of that tag (and it would be great if that
>>> was spelled out somewhere). As it is, I can not with good conscience
>>> apply
>>> this patch to the mainline kernel (especially not for -rc5), meaning the
>>> entire series will have to wait for the next release window unless there
>>> are no conflicts.
>> Because as you mentioned it is not a functional fix in the code I’ll
>> remove the Fixes tag in v5.
>> I checked older kernel versions and patch applies without conflicts.
>> That’s also why I moved it to be first in the series, so it will be
>> before any of my other changes that night cause merge problems for
>> other branches.
>
> If it's not a fix, it should go _after_ real fixes.
> And by nature this kind of patch is usually at the end of the series.
>
I don't care where it is, really, as long as it is after the real fixes.
And obviously I would not want to see a patch fixing something introduced
in a previous patch of the same series.
Thanks,
Guenter
On Tue, 06 Sep 2022 08:33:44 +0000, Eliav Farber wrote:
> Change "resets" property to be optional instead of required, for SOCs
> that don't support a reset controller.
>
> Signed-off-by: Eliav Farber <[email protected]>
> ---
> V4 -> V3:
> - Fix "reset" to "resets".
>
> 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(-)
>
Acked-by: Rob Herring <[email protected]>
On Tue, 06 Sep 2022 08:33:43 +0000, Eliav Farber wrote:
> This changes adds a detailed description for the mr75203 controller and
> for some of the analog IPs controlled by it.
>
> Signed-off-by: Eliav Farber <[email protected]>
> ---
> V4 -> V3:
> - New patch to add description.
>
> .../bindings/hwmon/moortec,mr75203.yaml | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
Acked-by: Rob Herring <[email protected]>