[PATCH v3] hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

[PATCH v3] hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

[Conor Dooley]

From: Lars Randers <lranders@mail.dk>

Add a driver for the temperature and voltage sensors on PolarFire SoC.
The temperature reports how hot the die is, and the voltages are the
SoC's 1.05, 1.8 and 2.5 volt rails respectively.

The hardware supports alarms in theory, but there is an erratum that
prevents clearing them once triggered, so no support is added for them.

The hardware measures voltage with 16 bits, of which 1 is a sign bit and
the remainder holds the voltage as a fixed point integer value. It's
improbable that the hardware will work if the voltages are negative, so
the driver ignores the sign bits.

There's no dt support etc here because this is the child of a simple-mfd
syscon.

Signed-off-by: Lars Randers <lranders@mail.dk>
Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
v3:
- clamp interval values
- note that 0-8 ms is the range for update_interval
- return -ENODATA for invalid data
- do write bounds checking once
- comment on MMIO regmap return value checks

v2:
- Fix some minor things pointed out by Sashiko including inaccurate
  comments, bounds checking of values read from sysfs and Kconfig
  dependencies.
- Make update_interval use milliseconds instead of microseconds
  (I'll add update_interval_us support when that lands, there's a
  proposed workaround for the erratum circulating internally, so it'll
  probably come alongside alarm support).

CC: Guenter Roeck <linux@roeck-us.net>
CC: Jonathan Corbet <corbet@lwn.net>
CC: Shuah Khan <skhan@linuxfoundation.org>
CC: Conor Dooley <conor.dooley@microchip.com>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: linux-hwmon@vger.kernel.org
CC: linux-doc@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-riscv@lists.infradead.org
CC: Valentina.FernandezAlanis@microchip.com
---
 Documentation/hwmon/index.rst    |   1 +
 Documentation/hwmon/tvs-mpfs.rst |  53 +++++
 MAINTAINERS                      |   1 +
 drivers/hwmon/Kconfig            |  13 ++
 drivers/hwmon/Makefile           |   1 +
 drivers/hwmon/tvs-mpfs.c         | 388 +++++++++++++++++++++++++++++++
 6 files changed, 457 insertions(+)
 create mode 100644 Documentation/hwmon/tvs-mpfs.rst
 create mode 100644 drivers/hwmon/tvs-mpfs.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 8b655e5d6b68..84a5339e1d6f 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -262,6 +262,7 @@ Hardware Monitoring Kernel Drivers
    tps53679
    tps546d24
    tsc1641
+   tvs-mpfs
    twl4030-madc-hwmon
    ucd9000
    ucd9200
diff --git a/Documentation/hwmon/tvs-mpfs.rst b/Documentation/hwmon/tvs-mpfs.rst
new file mode 100644
index 000000000000..1035812f363a
--- /dev/null
+++ b/Documentation/hwmon/tvs-mpfs.rst
@@ -0,0 +1,53 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver tvs-mpfs
+======================
+
+Supported chips:
+
+  * PolarFire SoC
+
+Authors:
+
+   - Conor Dooley <conor.dooley@microchip.com>
+   - Lars Randers <lranders@mail.dk>
+
+Description
+-----------
+
+This driver implements support for the temperature and voltage sensors on
+PolarFire SoC. The temperature reports how hot the die is, and the voltages are
+the SoC's 1.05, 1.8 and 2.5 volt rails respectively.
+
+
+Usage Notes
+-----------
+
+update_interval has a permitted range of 0 to 8 milliseconds.
+
+Temperatures are read in millidegrees Celsius, but the hardware measures in
+degrees Kelvin, storing the result as 11.4 fixed point data, for a maximum
+value of 2047.9375 degrees Kelvin.
+
+Voltages are read in millivolts. The hardware measures in millivolts, storing
+the value as 12.3 fixed point data, for a maximum of 4095.875 millivolts.
+The minimum value reportable by the driver is 0 volts, although the hardware
+is capable of measuring negative values.
+
+Sysfs entries
+-------------
+
+The following attributes are supported. update_interval is read-write, as are
+the enables. All other attributes are read only.
+
+======================= ====================================================
+temp1_label		Fixed name for channel.
+temp1_input		Measured temperature for channel.
+temp1_enable		Enable/disable for channel.
+
+in[0-2]_label		Fixed name for channel.
+in[0-2]_input		Measured voltage for channel.
+in[0-2]_enable		Enable/disable for channel.
+
+update_interval		The interval at which the chip will update readings.
+======================= ====================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 2fb1c75afd16..a492cf5ad0fc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22938,6 +22938,7 @@ F:	drivers/char/hw_random/mpfs-rng.c
 F:	drivers/clk/microchip/clk-mpfs*.c
 F:	drivers/firmware/microchip/mpfs-auto-update.c
 F:	drivers/gpio/gpio-mpfs.c
+F:	drivers/hwmon/tvs-mpfs.c
 F:	drivers/i2c/busses/i2c-microchip-corei2c.c
 F:	drivers/mailbox/mailbox-mpfs.c
 F:	drivers/pci/controller/plda/pcie-microchip-host.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 14e4cea48acc..2b9622b1db95 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -930,6 +930,19 @@ config SENSORS_JC42
 	  This driver can also be built as a module. If so, the module
 	  will be called jc42.
 
+config SENSORS_POLARFIRE_SOC_TVS
+	tristate "PolarFire SoC (MPFS) temperature and voltage sensor"
+	depends on POLARFIRE_SOC_SYSCONS || COMPILE_TEST
+	depends on MFD_SYSCON
+	help
+	  This driver adds support for the PolarFire SoC (MPFS) Temperature and
+	  Voltage Sensor.
+
+	  To compile this driver as a module, choose M here. the
+	  module will be called tvs-mpfs.
+
+	  If unsure, say N.
+
 config SENSORS_POWERZ
 	tristate "ChargerLAB POWER-Z USB-C tester"
 	depends on USB
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 4788996aa137..b58d249e4cf4 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
+obj-$(CONFIG_SENSORS_POLARFIRE_SOC_TVS)  += tvs-mpfs.o
 obj-$(CONFIG_SENSORS_POWERZ)	+= powerz.o
 obj-$(CONFIG_SENSORS_POWR1220)  += powr1220.o
 obj-$(CONFIG_SENSORS_PT5161L)	+= pt5161l.o
diff --git a/drivers/hwmon/tvs-mpfs.c b/drivers/hwmon/tvs-mpfs.c
new file mode 100644
index 000000000000..48de8d98b344
--- /dev/null
+++ b/drivers/hwmon/tvs-mpfs.c
@@ -0,0 +1,388 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Author: Lars Randers <lranders@mail.dk>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/err.h>
+#include <linux/freezer.h>
+#include <linux/hwmon.h>
+#include <linux/io.h>
+#include <linux/kthread.h>
+#include <linux/mfd/syscon.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define MPFS_TVS_CTRL 0x08
+#define MPFS_TVS_OUTPUT0 0x24
+#define MPFS_TVS_OUTPUT1 0x28
+
+#define MPFS_TVS_CTRL_TEMP_VALID	BIT(19)
+#define MPFS_TVS_CTRL_V2P5_VALID	BIT(18)
+#define MPFS_TVS_CTRL_V1P8_VALID	BIT(17)
+#define MPFS_TVS_CTRL_V1P05_VALID	BIT(16)
+
+#define MPFS_TVS_CTRL_TEMP_ENABLE	BIT(3)
+#define MPFS_TVS_CTRL_V2P5_ENABLE	BIT(2)
+#define MPFS_TVS_CTRL_V1P8_ENABLE	BIT(1)
+#define MPFS_TVS_CTRL_V1P05_ENABLE	BIT(0)
+#define MPFS_TVS_CTRL_ENABLE_ALL	GENMASK(3, 0)
+
+/*
+ * For all of these the value in millivolts is stored in 16 bits, with an upper
+ * sign bit and a lower 3 bits of decimal. These masks discard the sign bit and
+ * decimal places, because if Linux is running these voltages cannot be negative
+ * and so avoid having to convert to two's complement.
+ */
+#define MPFS_OUTPUT0_V1P8_MASK	GENMASK(30, 19)
+#define MPFS_OUTPUT0_V1P05_MASK	GENMASK(14, 3)
+#define MPFS_OUTPUT1_V2P5_MASK	GENMASK(14, 3)
+
+/*
+ * The register map claims that the temperature is stored in bits 31:16, but
+ * application note "AN4682: PolarFire FPGA Temperature and Voltage Sensor"
+ * says that 31 is reserved. Temperature is in kelvin, so what's probably a
+ * sign bit has no value anyway.
+ */
+#define MPFS_OUTPUT1_TEMP_MASK GENMASK(30, 16)
+
+#define MPFS_TVS_INTERVAL_MASK GENMASK(15, 8)
+#define MPFS_TVS_INTERVAL_OFFSET 8
+/* The interval register is in increments of 32 us */
+#define MPFS_TVS_INTERVAL_SCALE 32
+/* with 254 usable increments of 32 us available, 8 ms is the integer limit */
+#define MPFS_TVS_INTERVAL_MAX_MS 8U
+
+/* 273.1875 in 11.4 fixed-point notation */
+#define MPFS_TVS_K_TO_C 0x1113
+
+enum mpfs_tvs_sensors {
+	SENSOR_V1P05 = 0,
+	SENSOR_V1P8,
+	SENSOR_V2P5,
+};
+
+static const char * const mpfs_tvs_voltage_labels[] = { "1P05", "1P8", "2P5" };
+
+struct mpfs_tvs {
+	struct regmap *regmap;
+};
+
+static int mpfs_tvs_voltage_read(struct mpfs_tvs *data, u32 attr,
+				 int channel, long *val)
+{
+	u32 tmp, control;
+
+	if (attr != hwmon_in_input && attr != hwmon_in_enable)
+		return -EOPNOTSUPP;
+
+	regmap_read(data->regmap, MPFS_TVS_CTRL, &control);
+
+	switch (channel) {
+	case SENSOR_V2P5:
+		if (attr == hwmon_in_enable) {
+			*val = FIELD_GET(MPFS_TVS_CTRL_V2P5_ENABLE, control);
+			break;
+		}
+
+		if (!(control & MPFS_TVS_CTRL_V2P5_VALID))
+			return -ENODATA;
+
+		regmap_read(data->regmap, MPFS_TVS_OUTPUT1, &tmp);
+		*val = FIELD_GET(MPFS_OUTPUT1_V2P5_MASK, tmp);
+		break;
+	case SENSOR_V1P8:
+		if (attr == hwmon_in_enable) {
+			*val = FIELD_GET(MPFS_TVS_CTRL_V1P8_ENABLE, control);
+			break;
+		}
+
+		if (!(control & MPFS_TVS_CTRL_V1P8_VALID))
+			return -ENODATA;
+
+		regmap_read(data->regmap, MPFS_TVS_OUTPUT0, &tmp);
+		*val = FIELD_GET(MPFS_OUTPUT0_V1P8_MASK, tmp);
+		break;
+	case SENSOR_V1P05:
+		if (attr == hwmon_in_enable) {
+			*val = FIELD_GET(MPFS_TVS_CTRL_V1P05_ENABLE, control);
+			break;
+		}
+
+		if (!(control & MPFS_TVS_CTRL_V1P05_VALID))
+			return -ENODATA;
+
+		regmap_read(data->regmap, MPFS_TVS_OUTPUT0, &tmp);
+		*val = FIELD_GET(MPFS_OUTPUT0_V1P05_MASK, tmp);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int mpfs_tvs_voltage_write(struct mpfs_tvs *data, u32 attr,
+				  int channel, long val)
+{
+	u32 tmp;
+
+	if (attr != hwmon_in_enable)
+		return -EOPNOTSUPP;
+
+	if (val > 1 || val < 0)
+		return -EINVAL;
+
+	switch (channel) {
+	case SENSOR_V2P5:
+		tmp = FIELD_PREP(MPFS_TVS_CTRL_V2P5_ENABLE, val);
+		regmap_update_bits(data->regmap, MPFS_TVS_CTRL,
+				   MPFS_TVS_CTRL_V2P5_ENABLE, tmp);
+		break;
+	case SENSOR_V1P8:
+		tmp = FIELD_PREP(MPFS_TVS_CTRL_V1P8_ENABLE, val);
+		regmap_update_bits(data->regmap, MPFS_TVS_CTRL,
+				   MPFS_TVS_CTRL_V1P8_ENABLE, tmp);
+		break;
+	case SENSOR_V1P05:
+		tmp = FIELD_PREP(MPFS_TVS_CTRL_V1P05_ENABLE, val);
+		regmap_update_bits(data->regmap, MPFS_TVS_CTRL,
+				   MPFS_TVS_CTRL_V1P05_ENABLE, tmp);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int mpfs_tvs_temp_read(struct mpfs_tvs *data, u32 attr, long *val)
+{
+	u32 tmp, control;
+
+	if (attr != hwmon_temp_input && attr != hwmon_temp_enable)
+		return -EOPNOTSUPP;
+
+	regmap_read(data->regmap, MPFS_TVS_CTRL, &control);
+
+	if (attr == hwmon_temp_enable) {
+		*val = FIELD_GET(MPFS_TVS_CTRL_TEMP_ENABLE, control);
+		return 0;
+	}
+
+	if (!(control & MPFS_TVS_CTRL_TEMP_VALID))
+		return -ENODATA;
+
+	regmap_read(data->regmap, MPFS_TVS_OUTPUT1, &tmp);
+	*val = FIELD_GET(MPFS_OUTPUT1_TEMP_MASK, tmp);
+	*val -= MPFS_TVS_K_TO_C;
+	*val = (1000 * *val) >> 4; /* fixed point (11.4) to millidegrees */
+
+	return 0;
+}
+
+static int mpfs_tvs_temp_write(struct mpfs_tvs *data, u32 attr, long val)
+{
+	u32 tmp;
+
+	if (attr != hwmon_temp_enable)
+		return -EOPNOTSUPP;
+
+	if (val > 1 || val < 0)
+		return -EINVAL;
+
+	tmp = FIELD_PREP(MPFS_TVS_CTRL_TEMP_ENABLE, val);
+	regmap_update_bits(data->regmap, MPFS_TVS_CTRL,
+			   MPFS_TVS_CTRL_TEMP_ENABLE, tmp);
+
+	return 0;
+}
+
+static int mpfs_tvs_interval_read(struct mpfs_tvs *data, u32 attr, long *val)
+{
+	u32 tmp;
+
+	if (attr != hwmon_chip_update_interval)
+		return -EOPNOTSUPP;
+
+	regmap_read(data->regmap, MPFS_TVS_CTRL, &tmp);
+	*val = FIELD_GET(MPFS_TVS_INTERVAL_MASK, tmp);
+	*val *= MPFS_TVS_INTERVAL_SCALE;
+	*val /= 1000;
+
+	return 0;
+}
+
+static int mpfs_tvs_interval_write(struct mpfs_tvs *data, u32 attr, long val)
+{
+	unsigned long temp = val;
+
+	if (attr != hwmon_chip_update_interval)
+		return -EOPNOTSUPP;
+
+	temp = clamp(temp, 0U, MPFS_TVS_INTERVAL_MAX_MS);
+
+	temp *= 1000;
+	temp /= MPFS_TVS_INTERVAL_SCALE;
+
+	temp <<= MPFS_TVS_INTERVAL_OFFSET;
+	regmap_update_bits(data->regmap, MPFS_TVS_CTRL,
+			   MPFS_TVS_INTERVAL_MASK, temp);
+
+	return 0;
+}
+
+static umode_t mpfs_tvs_is_visible(const void *data,
+				   enum hwmon_sensor_types type,
+				   u32 attr, int channel)
+{
+	if (type == hwmon_chip && attr == hwmon_chip_update_interval)
+		return 0644;
+
+	if (type == hwmon_temp) {
+		switch (attr) {
+		case hwmon_temp_enable:
+			return 0644;
+		case hwmon_temp_input:
+		case hwmon_temp_label:
+			return 0444;
+		default:
+			return 0;
+		}
+	}
+
+	if (type == hwmon_in) {
+		switch (attr) {
+		case hwmon_in_enable:
+			return 0644;
+		case hwmon_in_input:
+		case hwmon_in_label:
+			return 0444;
+		default:
+			return 0;
+		}
+	}
+
+	return 0;
+}
+
+static int mpfs_tvs_read(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long *val)
+{
+	struct mpfs_tvs *data = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_temp:
+		return mpfs_tvs_temp_read(data, attr, val);
+	case hwmon_in:
+		return mpfs_tvs_voltage_read(data, attr, channel, val);
+	case hwmon_chip:
+		return mpfs_tvs_interval_read(data, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int mpfs_tvs_write(struct device *dev, enum hwmon_sensor_types type,
+			  u32 attr, int channel, long val)
+{
+	struct mpfs_tvs *data = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_temp:
+		return mpfs_tvs_temp_write(data, attr, val);
+	case hwmon_in:
+		return mpfs_tvs_voltage_write(data, attr, channel, val);
+	case hwmon_chip:
+		return mpfs_tvs_interval_write(data, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int mpfs_tvs_read_labels(struct device *dev,
+				enum hwmon_sensor_types type,
+				u32 attr, int channel,
+				const char **str)
+{
+	switch (type) {
+	case hwmon_temp:
+		*str = "Die Temp";
+		return 0;
+	case hwmon_in:
+		*str = mpfs_tvs_voltage_labels[channel];
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_ops mpfs_tvs_ops = {
+	.is_visible = mpfs_tvs_is_visible,
+	.read_string = mpfs_tvs_read_labels,
+	.read = mpfs_tvs_read,
+	.write = mpfs_tvs_write,
+};
+
+static const struct hwmon_channel_info *mpfs_tvs_info[] = {
+	HWMON_CHANNEL_INFO(chip,
+			   HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_ENABLE),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_ENABLE,
+			   HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_ENABLE,
+			   HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_ENABLE),
+	NULL
+};
+
+static const struct hwmon_chip_info mpfs_tvs_chip_info = {
+	.ops = &mpfs_tvs_ops,
+	.info = mpfs_tvs_info,
+};
+
+static int mpfs_tvs_probe(struct platform_device *pdev)
+{
+	struct device *hwmon_dev;
+	struct mpfs_tvs *data;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->regmap = device_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(data->regmap),
+				     "Failed to find syscon regmap\n");
+
+	/*
+	 * It's an MMIO regmap with no resources, there's nothing that can fail
+	 * and return an error
+	 */
+	regmap_write(data->regmap, MPFS_TVS_CTRL, MPFS_TVS_CTRL_ENABLE_ALL);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "mpfs_tvs",
+							 data,
+							 &mpfs_tvs_chip_info,
+							 NULL);
+	if (IS_ERR(hwmon_dev))
+		return dev_err_probe(&pdev->dev, PTR_ERR(hwmon_dev),
+				     "hwmon device registration failed.\n");
+
+	return 0;
+}
+
+static struct platform_driver mpfs_tvs_driver = {
+	.probe = mpfs_tvs_probe,
+	.driver = {
+		.name = "mpfs-tvs",
+	},
+};
+module_platform_driver(mpfs_tvs_driver);
+
+MODULE_AUTHOR("Lars Randers <lranders@mail.dk>");
+MODULE_DESCRIPTION("PolarFire SoC temperature & voltage sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.53.0

Re: [PATCH v3] hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Negative values written to `update_interval` are incorrectly clamped to the maximum interval instead of returning an error or clamping to 0.
- [Medium] Reading `update_interval` inconsistently under-reports the configured value due to integer division truncation.
- [Medium] Hardware sensors are enabled in `probe` but are not disabled on probe failure or driver unbind.
--

commit 91a403f5c5eef9fbcfcab3857179dc2f03ad3c89
Author: Lars Randers <lranders@mail.dk>

hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

Add a driver for the temperature and voltage sensors on PolarFire SoC.
The temperature reports how hot the die is, and the voltages are the
SoC's 1.05, 1.8 and 2.5 volt rails respectively.

> diff --git a/drivers/hwmon/tvs-mpfs.c b/drivers/hwmon/tvs-mpfs.c
> new file mode 100644
> index 0000000000000..48de8d98b3444
> --- /dev/null
> +++ b/drivers/hwmon/tvs-mpfs.c

[ ... ]

> +static int mpfs_tvs_interval_read(struct mpfs_tvs *data, u32 attr, long *val)
> +{
> +	u32 tmp;
> +
> +	if (attr != hwmon_chip_update_interval)
> +		return -EOPNOTSUPP;
> +
> +	regmap_read(data->regmap, MPFS_TVS_CTRL, &tmp);
> +	*val = FIELD_GET(MPFS_TVS_INTERVAL_MASK, tmp);
> +	*val *= MPFS_TVS_INTERVAL_SCALE;
> +	*val /= 1000;

[Severity: Medium]
Will this return an incorrectly truncated value for certain intervals?

Since integer division is used both here and in mpfs_tvs_interval_write(), 
the truncation can cause the readback value to be lower than what was written.
For example, writing 7 ms calculates `(7 * 1000) / 32 = 218`. Reading it back
computes `(218 * 32) / 1000 = 6` ms. 

Should DIV_ROUND_CLOSEST be used for these conversions to ensure the value
survives the round trip?

> +
> +	return 0;
> +}
> +
> +static int mpfs_tvs_interval_write(struct mpfs_tvs *data, u32 attr, long val)
> +{
> +	unsigned long temp = val;
> +
> +	if (attr != hwmon_chip_update_interval)
> +		return -EOPNOTSUPP;
> +
> +	temp = clamp(temp, 0U, MPFS_TVS_INTERVAL_MAX_MS);

[Severity: Medium]
Does this correctly handle negative input values?

The parameter `val` is signed, but it is assigned to `unsigned long temp`. If
a negative value is passed (e.g., -1), it undergoes an unsigned conversion and
becomes a large positive number like ULONG_MAX. 

When clamp() evaluates this against 8 ms, it incorrectly clamps to 8 instead of
returning -EINVAL or properly clamping to 0.

> +
> +	temp *= 1000;
> +	temp /= MPFS_TVS_INTERVAL_SCALE;

[Severity: Medium]
Similar to mpfs_tvs_interval_read(), does this integer truncation prevent
accurately restoring the configured value? Should DIV_ROUND_CLOSEST be
used here as well?

> +
> +	temp <<= MPFS_TVS_INTERVAL_OFFSET;
> +	regmap_update_bits(data->regmap, MPFS_TVS_CTRL,
> +			   MPFS_TVS_INTERVAL_MASK, temp);
> +
> +	return 0;
> +}

[ ... ]

> +static int mpfs_tvs_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	/*
> +	 * It's an MMIO regmap with no resources, there's nothing that can fail
> +	 * and return an error
> +	 */
> +	regmap_write(data->regmap, MPFS_TVS_CTRL, MPFS_TVS_CTRL_ENABLE_ALL);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "mpfs_tvs",
> +							 data,
> +							 &mpfs_tvs_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(hwmon_dev),
> +				     "hwmon device registration failed.\n");

[Severity: Medium]
Does the hardware remain enabled if hwmon device registration fails, or if the
driver is unbound?

The hardware sensors are enabled here by writing MPFS_TVS_CTRL_ENABLE_ALL, but
there doesn't seem to be an error path or devm action to disable them if
devm_hwmon_device_register_with_info() fails. 

Could this lead to unnecessary power consumption when the driver fails to probe
or is not actively loaded?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-blank-footprint-5504b819baec@spud?part=1

Re: [PATCH v3] hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3443 bytes --]

On Thu, Jun 11, 2026 at 03:13:16PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] Negative values written to `update_interval` are incorrectly clamped to the maximum interval instead of returning an error or clamping to 0.
> - [Medium] Reading `update_interval` inconsistently under-reports the configured value due to integer division truncation.
> - [Medium] Hardware sensors are enabled in `probe` but are not disabled on probe failure or driver unbind.
> --
> 
> commit 91a403f5c5eef9fbcfcab3857179dc2f03ad3c89
> Author: Lars Randers <lranders@mail.dk>
> 
> hwmon: add a driver for the temp/voltage sensor on PolarFire SoC
> 
> Add a driver for the temperature and voltage sensors on PolarFire SoC.
> The temperature reports how hot the die is, and the voltages are the
> SoC's 1.05, 1.8 and 2.5 volt rails respectively.
> 
> > diff --git a/drivers/hwmon/tvs-mpfs.c b/drivers/hwmon/tvs-mpfs.c
> > new file mode 100644
> > index 0000000000000..48de8d98b3444
> > --- /dev/null
> > +++ b/drivers/hwmon/tvs-mpfs.c
> 
> [ ... ]
> 
> > +static int mpfs_tvs_interval_read(struct mpfs_tvs *data, u32 attr, long *val)
> > +{
> > +	u32 tmp;
> > +
> > +	if (attr != hwmon_chip_update_interval)
> > +		return -EOPNOTSUPP;
> > +
> > +	regmap_read(data->regmap, MPFS_TVS_CTRL, &tmp);
> > +	*val = FIELD_GET(MPFS_TVS_INTERVAL_MASK, tmp);
> > +	*val *= MPFS_TVS_INTERVAL_SCALE;
> > +	*val /= 1000;
> 
> [Severity: Medium]
> Will this return an incorrectly truncated value for certain intervals?
> 
> Since integer division is used both here and in mpfs_tvs_interval_write(), 
> the truncation can cause the readback value to be lower than what was written.
> For example, writing 7 ms calculates `(7 * 1000) / 32 = 218`. Reading it back
> computes `(218 * 32) / 1000 = 6` ms. 
> 
> Should DIV_ROUND_CLOSEST be used for these conversions to ensure the value
> survives the round trip?

Hmm, this is actually a good point that I overlooked.
What is the desired rounding behaviour here?
IOW, should asking for 7 ms produce the closest interval less than 7 ms or
the closest interval greater than 7 ms?

A consistent rounding is better than rounding to the closest IMO, if both
update_interval and update_interval_us are going to be supported

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int mpfs_tvs_interval_write(struct mpfs_tvs *data, u32 attr, long val)
> > +{
> > +	unsigned long temp = val;
> > +
> > +	if (attr != hwmon_chip_update_interval)
> > +		return -EOPNOTSUPP;
> > +
> > +	temp = clamp(temp, 0U, MPFS_TVS_INTERVAL_MAX_MS);
> 
> [Severity: Medium]
> Does this correctly handle negative input values?
> 
> The parameter `val` is signed, but it is assigned to `unsigned long temp`. If
> a negative value is passed (e.g., -1), it undergoes an unsigned conversion and
> becomes a large positive number like ULONG_MAX. 
> 
> When clamp() evaluates this against 8 ms, it incorrectly clamps to 8 instead of
> returning -EINVAL or properly clamping to 0.

I can definitely clamp this without casting. I considered that inputting
-1 to get the maximum was not really problematic given the documented
range and that common sense doesn't permit negative times, but since I
have to submit a v4 anyway, I'll just make this operate on the signed
value.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3] hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

[Guenter Roeck]

On Thu, Jun 11, 2026 at 05:21:41PM +0100, Conor Dooley wrote:
> On Thu, Jun 11, 2026 at 03:13:16PM +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> > - [Medium] Negative values written to `update_interval` are incorrectly clamped to the maximum interval instead of returning an error or clamping to 0.
> > - [Medium] Reading `update_interval` inconsistently under-reports the configured value due to integer division truncation.
> > - [Medium] Hardware sensors are enabled in `probe` but are not disabled on probe failure or driver unbind.
> > --
> > 
> > commit 91a403f5c5eef9fbcfcab3857179dc2f03ad3c89
> > Author: Lars Randers <lranders@mail.dk>
> > 
> > hwmon: add a driver for the temp/voltage sensor on PolarFire SoC
> > 
> > Add a driver for the temperature and voltage sensors on PolarFire SoC.
> > The temperature reports how hot the die is, and the voltages are the
> > SoC's 1.05, 1.8 and 2.5 volt rails respectively.
> > 
> > > diff --git a/drivers/hwmon/tvs-mpfs.c b/drivers/hwmon/tvs-mpfs.c
> > > new file mode 100644
> > > index 0000000000000..48de8d98b3444
> > > --- /dev/null
> > > +++ b/drivers/hwmon/tvs-mpfs.c
> > 
> > [ ... ]
> > 
> > > +static int mpfs_tvs_interval_read(struct mpfs_tvs *data, u32 attr, long *val)
> > > +{
> > > +	u32 tmp;
> > > +
> > > +	if (attr != hwmon_chip_update_interval)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	regmap_read(data->regmap, MPFS_TVS_CTRL, &tmp);
> > > +	*val = FIELD_GET(MPFS_TVS_INTERVAL_MASK, tmp);
> > > +	*val *= MPFS_TVS_INTERVAL_SCALE;
> > > +	*val /= 1000;
> > 
> > [Severity: Medium]
> > Will this return an incorrectly truncated value for certain intervals?
> > 
> > Since integer division is used both here and in mpfs_tvs_interval_write(), 
> > the truncation can cause the readback value to be lower than what was written.
> > For example, writing 7 ms calculates `(7 * 1000) / 32 = 218`. Reading it back
> > computes `(218 * 32) / 1000 = 6` ms. 
> > 
> > Should DIV_ROUND_CLOSEST be used for these conversions to ensure the value
> > survives the round trip?
> 
> Hmm, this is actually a good point that I overlooked.
> What is the desired rounding behaviour here?
> IOW, should asking for 7 ms produce the closest interval less than 7 ms or
> the closest interval greater than 7 ms?
> 
> A consistent rounding is better than rounding to the closest IMO, if both
> update_interval and update_interval_us are going to be supported
> 

Not sure what you mean with "consistent rounding". Effectively the current
code is rounding down. I personally like using DIV_ROUND_CLOSEST() for both
read and write conversions because it gives the most accurate result. So
(7 * 1000) / 32 = 218.75 would result in 219 and (219 * 32) / 1000 = 7.008
would result in 7.

> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int mpfs_tvs_interval_write(struct mpfs_tvs *data, u32 attr, long val)
> > > +{
> > > +	unsigned long temp = val;
> > > +
> > > +	if (attr != hwmon_chip_update_interval)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	temp = clamp(temp, 0U, MPFS_TVS_INTERVAL_MAX_MS);
> > 
> > [Severity: Medium]
> > Does this correctly handle negative input values?
> > 
> > The parameter `val` is signed, but it is assigned to `unsigned long temp`. If
> > a negative value is passed (e.g., -1), it undergoes an unsigned conversion and
> > becomes a large positive number like ULONG_MAX. 
> > 
> > When clamp() evaluates this against 8 ms, it incorrectly clamps to 8 instead of
> > returning -EINVAL or properly clamping to 0.
> 
> I can definitely clamp this without casting. I considered that inputting
> -1 to get the maximum was not really problematic given the documented
> range and that common sense doesn't permit negative times, but since I
> have to submit a v4 anyway, I'll just make this operate on the signed
> value.

Please do that. I don't think anyone would expect that writing -1
would result in programming the maximum.

Thanks,
Guenter

Re: [PATCH v3] hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3688 bytes --]

On Thu, Jun 11, 2026 at 09:41:49AM -0700, Guenter Roeck wrote:
> On Thu, Jun 11, 2026 at 05:21:41PM +0100, Conor Dooley wrote:
> > On Thu, Jun 11, 2026 at 03:13:16PM +0000, sashiko-bot@kernel.org wrote:
> > > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> > > - [Medium] Negative values written to `update_interval` are incorrectly clamped to the maximum interval instead of returning an error or clamping to 0.
> > > - [Medium] Reading `update_interval` inconsistently under-reports the configured value due to integer division truncation.
> > > - [Medium] Hardware sensors are enabled in `probe` but are not disabled on probe failure or driver unbind.
> > > --
> > > 
> > > commit 91a403f5c5eef9fbcfcab3857179dc2f03ad3c89
> > > Author: Lars Randers <lranders@mail.dk>
> > > 
> > > hwmon: add a driver for the temp/voltage sensor on PolarFire SoC
> > > 
> > > Add a driver for the temperature and voltage sensors on PolarFire SoC.
> > > The temperature reports how hot the die is, and the voltages are the
> > > SoC's 1.05, 1.8 and 2.5 volt rails respectively.
> > > 
> > > > diff --git a/drivers/hwmon/tvs-mpfs.c b/drivers/hwmon/tvs-mpfs.c
> > > > new file mode 100644
> > > > index 0000000000000..48de8d98b3444
> > > > --- /dev/null
> > > > +++ b/drivers/hwmon/tvs-mpfs.c
> > > 
> > > [ ... ]
> > > 
> > > > +static int mpfs_tvs_interval_read(struct mpfs_tvs *data, u32 attr, long *val)
> > > > +{
> > > > +	u32 tmp;
> > > > +
> > > > +	if (attr != hwmon_chip_update_interval)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	regmap_read(data->regmap, MPFS_TVS_CTRL, &tmp);
> > > > +	*val = FIELD_GET(MPFS_TVS_INTERVAL_MASK, tmp);
> > > > +	*val *= MPFS_TVS_INTERVAL_SCALE;
> > > > +	*val /= 1000;
> > > 
> > > [Severity: Medium]
> > > Will this return an incorrectly truncated value for certain intervals?
> > > 
> > > Since integer division is used both here and in mpfs_tvs_interval_write(), 
> > > the truncation can cause the readback value to be lower than what was written.
> > > For example, writing 7 ms calculates `(7 * 1000) / 32 = 218`. Reading it back
> > > computes `(218 * 32) / 1000 = 6` ms. 
> > > 
> > > Should DIV_ROUND_CLOSEST be used for these conversions to ensure the value
> > > survives the round trip?
> > 
> > Hmm, this is actually a good point that I overlooked.
> > What is the desired rounding behaviour here?
> > IOW, should asking for 7 ms produce the closest interval less than 7 ms or
> > the closest interval greater than 7 ms?
> > 
> > A consistent rounding is better than rounding to the closest IMO, if both
> > update_interval and update_interval_us are going to be supported
> > 
> 
> Not sure what you mean with "consistent rounding". Effectively the current

What I meant was that it would round to an interval that would either be
at least (always down) or at most (always up) as frequently as requested.
If, for example, the interval was set to 7400 or 7600 us via
update_interval_us, and then read back from update_interval, that the
rounding used for that would do the same thing as operating entirely via
update_interval and always display an interval that was at least as or
as most as frequent as the configured value.
Rounding closest would of course produce different results for 7400 and
7600, with one being more frequent and one less frequent than reality.

> code is rounding down. I personally like using DIV_ROUND_CLOSEST() for both
> read and write conversions because it gives the most accurate result. So
> (7 * 1000) / 32 = 218.75 would result in 219 and (219 * 32) / 1000 = 7.008
> would result in 7.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3] hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

[Guenter Roeck]

On 6/11/26 09:57, Conor Dooley wrote:
> On Thu, Jun 11, 2026 at 09:41:49AM -0700, Guenter Roeck wrote:
>> On Thu, Jun 11, 2026 at 05:21:41PM +0100, Conor Dooley wrote:
>>> On Thu, Jun 11, 2026 at 03:13:16PM +0000, sashiko-bot@kernel.org wrote:
>>>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>>>> - [Medium] Negative values written to `update_interval` are incorrectly clamped to the maximum interval instead of returning an error or clamping to 0.
>>>> - [Medium] Reading `update_interval` inconsistently under-reports the configured value due to integer division truncation.
>>>> - [Medium] Hardware sensors are enabled in `probe` but are not disabled on probe failure or driver unbind.
>>>> --
>>>>
>>>> commit 91a403f5c5eef9fbcfcab3857179dc2f03ad3c89
>>>> Author: Lars Randers <lranders@mail.dk>
>>>>
>>>> hwmon: add a driver for the temp/voltage sensor on PolarFire SoC
>>>>
>>>> Add a driver for the temperature and voltage sensors on PolarFire SoC.
>>>> The temperature reports how hot the die is, and the voltages are the
>>>> SoC's 1.05, 1.8 and 2.5 volt rails respectively.
>>>>
>>>>> diff --git a/drivers/hwmon/tvs-mpfs.c b/drivers/hwmon/tvs-mpfs.c
>>>>> new file mode 100644
>>>>> index 0000000000000..48de8d98b3444
>>>>> --- /dev/null
>>>>> +++ b/drivers/hwmon/tvs-mpfs.c
>>>>
>>>> [ ... ]
>>>>
>>>>> +static int mpfs_tvs_interval_read(struct mpfs_tvs *data, u32 attr, long *val)
>>>>> +{
>>>>> +	u32 tmp;
>>>>> +
>>>>> +	if (attr != hwmon_chip_update_interval)
>>>>> +		return -EOPNOTSUPP;
>>>>> +
>>>>> +	regmap_read(data->regmap, MPFS_TVS_CTRL, &tmp);
>>>>> +	*val = FIELD_GET(MPFS_TVS_INTERVAL_MASK, tmp);
>>>>> +	*val *= MPFS_TVS_INTERVAL_SCALE;
>>>>> +	*val /= 1000;
>>>>
>>>> [Severity: Medium]
>>>> Will this return an incorrectly truncated value for certain intervals?
>>>>
>>>> Since integer division is used both here and in mpfs_tvs_interval_write(),
>>>> the truncation can cause the readback value to be lower than what was written.
>>>> For example, writing 7 ms calculates `(7 * 1000) / 32 = 218`. Reading it back
>>>> computes `(218 * 32) / 1000 = 6` ms.
>>>>
>>>> Should DIV_ROUND_CLOSEST be used for these conversions to ensure the value
>>>> survives the round trip?
>>>
>>> Hmm, this is actually a good point that I overlooked.
>>> What is the desired rounding behaviour here?
>>> IOW, should asking for 7 ms produce the closest interval less than 7 ms or
>>> the closest interval greater than 7 ms?
>>>
>>> A consistent rounding is better than rounding to the closest IMO, if both
>>> update_interval and update_interval_us are going to be supported
>>>
>>
>> Not sure what you mean with "consistent rounding". Effectively the current
> 
> What I meant was that it would round to an interval that would either be
> at least (always down) or at most (always up) as frequently as requested.
> If, for example, the interval was set to 7400 or 7600 us via
> update_interval_us, and then read back from update_interval, that the
> rounding used for that would do the same thing as operating entirely via
> update_interval and always display an interval that was at least as or
> as most as frequent as the configured value.
> Rounding closest would of course produce different results for 7400 and
> 7600, with one being more frequent and one less frequent than reality.
> 

I must admit that I don't understand what you are trying to say. Either
case, your driver, your decision to make if and how you want to round.

Thanks,
Guenter