Re: [PATCH v5 2/5] iio: adc: add Versal SysMon driver
From: Erim, Salih
Date: Wed Jun 10 2026 - 07:53:47 EST
Hi Andy,
Thanks for all reviews.
On 09/06/2026 16:06, Andy Shevchenko wrote:
On Mon, Jun 08, 2026 at 07:37:58PM +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.
Almost there.
...
+static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
+{
+ unsigned int num_supply = 0, num_temp = 0;
Unneeded assignments.
Accepted.
+ 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");
+ 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);
+
+ sysmon_channels = devm_kcalloc(dev,
+ size_add(size_add(ARRAY_SIZE(temp_channels),
+ num_supply), num_temp),
+ sizeof(*sysmon_channels), GFP_KERNEL);
Something happened to indentation of the third line (out of four). Taking into
account nested size_add(), I would rewrite the whole thing as
Accepted. Will use a temporary variable for the channel count.
sysmon_channels = devm_kcalloc(dev,
size_add(num_temp,
size_add(ARRAY_SIZE(temp_channels), num_supply)),
sizeof(*sysmon_channels), GFP_KERNEL);
Or even use temporary variable
unsigned int num_chan;
num_chan = size_add(num_temp, size_add(ARRAY_SIZE(temp_channels), num_supply)),
sysmon_channels = devm_kcalloc(dev, num_chan, sizeof(*sysmon_channels), GFP_KERNEL);
still over 80, but a bit shorter.
+ if (!sysmon_channels)
+ return -ENOMEM;
+
+ /* Static temperature channels first (fixed indices) */
+ idx = 0;
Why?
+ memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
+ idx += ARRAY_SIZE(temp_channels);
Just
Accepted. Will initialize idx directly.
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),
Perfectly one line. Is it going to be expanded in the next changes?
If not, join.
Not expanded. Will join on one line.
+ .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),
Ditto.
Not expanded. Will join on one line.
Regards,
Salih
+ .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;
+}
--
With Best Regards,
Andy Shevchenko