Re: [PATCH v7 2/5] iio: adc: add Versal SysMon driver
From: Erim, Salih
Date: Mon Jun 15 2026 - 11:45:53 EST
Hi Andy,
Thanks for the review, replies inline.
On 15/06/2026 15:22, Andy Shevchenko wrote:
On Mon, Jun 15, 2026 at 12:37:19AM +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 hardware also provides four aggregate temperature registers that
are always present regardless of the device tree configuration: the
current max and min across all active satellites, and the peak and
trough values recorded since the last hardware reset.
The driver is split into two 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.
...
+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 = MILLI;
Since this is about temperature, wouldn't be better to use
MILLIDEGREE_PER_DEGREE
here?
Agreed, MILLIDEGREE_PER_DEGREE is semantically correct here.
Will change in v8.
+ *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 = sign_extend32(regval, 15);
+ 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 +
+ 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_chan, idx, temp_chan_idx, volt_chan_idx;
+ unsigned int num_supply, num_temp;
+ 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");
+ 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");
+ num_temp = fwnode_get_child_node_count(temp_node);
+
+ num_chan = size_add(num_temp, size_add(ARRAY_SIZE(temp_channels), num_supply));
+ overflow.h
Accepted, Will add.
+ sysmon_channels = devm_kcalloc(dev, num_chan, sizeof(*sysmon_channels), GFP_KERNEL);
+ if (!sysmon_channels)
+ return -ENOMEM;
+
+ /* Static temperature channels first */
+ memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
+ idx = ARRAY_SIZE(temp_channels);
+
+ /* Supply channels from DT */
+ fwnode_for_each_child_node_scoped(supply_node, child) {
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "missing reg for supply channel\n");
+
+ if (reg > SYSMON_SUPPLY_IDX_MAX)
+ return dev_err_probe(dev, -EINVAL,
+ "supply reg %u exceeds max %u\n",
+ reg, SYSMON_SUPPLY_IDX_MAX);
+
+ ret = fwnode_property_read_string(child, "label", &label);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "missing label for supply channel\n");
+
+ sysmon_channels[idx++] = (struct iio_chan_spec) {
+ .type = IIO_VOLTAGE,
+ .indexed = 1,
+ .address = reg,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .datasheet_name = label,
+ };
+ }
+
+ /* Temperature satellite channels from DT */
+ fwnode_for_each_child_node_scoped(temp_node, child) {
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret)
+ 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)
+ 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,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+ .datasheet_name = label,
+ };
+ }
+
+ indio_dev->num_channels = idx;
+ indio_dev->info = &sysmon_iio_info;
+
+ /*
+ * Assign per-type sequential channel numbers.
+ * IIO sysfs uses type prefix (in_tempN, in_voltageN)
+ * so numbers only need to be unique within each type.
+ */
+ temp_chan_idx = 0;
+ volt_chan_idx = 0;
+ for (unsigned int idx = 0; idx < indio_dev->num_channels; idx++) {
+ if (sysmon_channels[idx].type == IIO_TEMP)
+ sysmon_channels[idx].channel = temp_chan_idx++;
+ else
+ sysmon_channels[idx].channel = volt_chan_idx++;
+ }
+
+ indio_dev->channels = sysmon_channels;
+
+ return 0;
+}
...
+/**
+ * sysmon_core_probe() - Initialize Versal SysMon core
It is managed, please name it accordingly: devm_sysmon_core_probe().
Will rename to devm_sysmon_core_probe() and update callers.
Thanks,
Salih
+ * @dev: Parent device
+ * @regmap: Register map for hardware access
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
--
With Best Regards,
Andy Shevchenko