Re: [PATCH v4 2/5] iio: adc: add Versal SysMon driver
From: Erim, Salih
Date: Sun Jun 07 2026 - 16:46:05 EST
Hi Andy,
Thanks for review, replies are inline.
On 07/06/2026 09:06, Andy Shevchenko wrote:
On Sat, Jun 06, 2026 at 06:17:04AM +0100, Salih Erim wrote:
Add the core driver and MMIO platform driver for the AMD/Xilinx Versal
System Monitor (SysMon) block.
The SysMon block resides in the platform management controller (PMC) and
provides on-chip voltage and temperature monitoring through a 10-bit,
200 kSPS ADC. It can monitor up to 160 voltage channels and 64
temperature satellites distributed across the SoC, with a consistent
sample rate of 8 kSPS per channel regardless of how many channels are
enabled.
The driver is split into three compilation units:
- versal-sysmon-core: Channel parsing, IIO registration, read_raw
- versal-sysmon: MMIO platform driver with custom regmap accessors
Voltage results are stored in a 19-bit modified floating-point format
and converted to millivolts. Temperature results are stored in Q8.7
signed fixed-point Celsius format and converted to millicelsius.
The MMIO regmap backend uses a custom reg_write accessor that
automatically unlocks the NPI (NoC programming interface) lock
register before each write, as required by the hardware. The regmap
is configured with fast_io since the underlying MMIO accessors are
safe to call from atomic context.
...
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+ err.h
Accepted.
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+
+#include "versal-sysmon.h"
...
+static int sysmon_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int regval;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ switch (chan->type) {
+ case IIO_TEMP:
+ if (mask == IIO_CHAN_INFO_SCALE) {
+ /* Q8.7 to millicelsius: raw * 1000 / 128 */
+ *val = (int)MILLI;
Casting is not required here.
Accepted. Will drop (int) cast in the scale assignment.
+ *val2 = BIT(SYSMON_FRACTIONAL_SHIFT);
+ return IIO_VAL_FRACTIONAL;
+ }
+ if (mask != IIO_CHAN_INFO_RAW)
+ return -EINVAL;
+
+ ret = regmap_read(sysmon->regmap, chan->address, ®val);
+ if (ret)
+ return ret;
+
+ *val = (s16)regval;
Basically you want to use sign_extend32(regval, 15) for consistency.
Accepted.
+ return IIO_VAL_INT;
+
+ case IIO_VOLTAGE:
+ if (mask != IIO_CHAN_INFO_PROCESSED)
+ return -EINVAL;
+
+ ret = regmap_read(sysmon->regmap,
+ (chan->address * SYSMON_REG_STRIDE) +
Unneeded parentheses.
Accepted. Will remove throughout.
+ SYSMON_SUPPLY_BASE, ®val);
+ if (ret)
+ return ret;
+
+ sysmon_supply_rawtoprocessed(regval, val);
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+ }
+}
...
+static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
+{
+ unsigned int num_supply = 0, num_temp = 0;
+ unsigned int idx, temp_chan_idx, volt_chan_idx;
+ struct iio_chan_spec *sysmon_channels;
+ const char *label;
+ u32 reg;
+ int ret;
+
+ struct fwnode_handle *supply_node __free(fwnode_handle) =
+ device_get_named_child_node(dev, "voltage-channels");
+ if (supply_node)
Do you need this check? IIRC the below is NULL-aware.
You're right, fwnode_get_child_node_count() handles NULL
(fwnode_get_next_child_node returns NULL for NULL fwnode).
Will drop both checks.
>
+ num_supply = fwnode_get_child_node_count(supply_node);
+
+ struct fwnode_handle *temp_node __free(fwnode_handle) =
+ device_get_named_child_node(dev, "temperature-channels");
+ if (temp_node)
Same Q here?
Will drop this check too.
+ num_temp = fwnode_get_child_node_count(temp_node);
+
+ sysmon_channels = devm_kcalloc(dev,
+ size_add(ARRAY_SIZE(temp_channels),
+ num_supply + num_temp),
size_add() should be called twice.
Accepted.
+ sizeof(*sysmon_channels), GFP_KERNEL);
+ if (!sysmon_channels)
+ return -ENOMEM;
...
+ /* Temperature satellite channels from DT */
+ fwnode_for_each_child_node_scoped(temp_node, child) {
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret < 0)
Can fwnode APIs like this return positive? If so, what is the meaning of that
and why do we ignore it?
They cannot return positive. fwnode_property_read_u32() returns
0 on success or negative errno. Will change to if (ret) for
consistency.
+ return dev_err_probe(dev, ret,
+ "missing reg for temp channel\n");
+
+ if (reg < 1 || reg > SYSMON_TEMP_SAT_MAX)
+ return dev_err_probe(dev, -EINVAL,
+ "temp reg %u out of range [1..%u]\n",
+ reg, SYSMON_TEMP_SAT_MAX);
+
+ ret = fwnode_property_read_string(child, "label", &label);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "missing label for temp channel\n");
+
+ sysmon_channels[idx++] = (struct iio_chan_spec) {
+ .type = IIO_TEMP,
+ .indexed = 1,
+ .address = SYSMON_TEMP_SAT_BASE +
+ ((reg - 1) * SYSMON_REG_STRIDE),
Too many parentheses.
Accepted. (reg - 1) needs parens for precedence, will remove the
outer pair.
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_type =
+ BIT(IIO_CHAN_INFO_SCALE),
+ .datasheet_name = label,
+ };
+ }
...
+ for (idx = 0; idx < indio_dev->num_channels; idx++) {
for (unsigned int idx = 0; idx < indio_dev->num_channels; idx++) {
?
Accepted.
+ if (sysmon_channels[idx].type == IIO_TEMP)
+ sysmon_channels[idx].channel = temp_chan_idx++;
+ else
+ sysmon_channels[idx].channel = volt_chan_idx++;
+ }
...
+/**
+ * sysmon_core_probe() - Initialize Versal SysMon core
+ * @dev: Parent device
+ * @regmap: Register map for hardware access
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int sysmon_core_probe(struct device *dev, struct regmap *regmap)
I'm wondering if the @regmap is the same as you can get from @dev via
respective API.
Yes, dev_get_regmap(dev, NULL) would work since devm_regmap_init
registers it with devres. However, the explicit parameter makes
the dependency clear and avoids coupling core_probe to the devres
registration order. Happy to change if you prefer dev_get_regmap().
...
+ err.h // covers IS_ERR(), -Exxx
Accepted.
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+ types.h // covers NULL, __iomem
IWYU, please.
Accepted.
+#include "versal-sysmon.h"
+
+struct sysmon_mmio {
+ void __iomem *base;
__iomem is not defined in the above headers.
Accepted, covered by types.h above.
+};
...
+static int sysmon_platform_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct sysmon_mmio *mmio;
+ struct regmap *regmap;
+
+ mmio = devm_kzalloc(dev, sizeof(*mmio), GFP_KERNEL);
+ if (!mmio)
+ return -ENOMEM;
+
+ mmio->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(mmio->base))
+ return PTR_ERR(mmio->base);
IS_ERR() is not provided in the headers included.
Accepted, covered by err.h above.
+
+ regmap = devm_regmap_init(dev, NULL, mmio, &sysmon_mmio_regmap_config);
NULL is not defined in the above headers.
Accepted, covered by types.h above.
Salih
...