Re: [PATCH v4 2/5] iio: adc: add Versal SysMon driver
From: Andy Shevchenko
Date: Sun Jun 07 2026 - 04:06:54 EST
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
> +#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.
> + *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.
> + 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.
> + 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.
> + 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?
> + 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.
> + 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?
> + 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.
> + .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++) {
?
> + 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.
...
+ err.h // covers IS_ERR(), -Exxx
> +#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.
> +#include "versal-sysmon.h"
> +
> +struct sysmon_mmio {
> + void __iomem *base;
__iomem is not defined in the above headers.
> +};
...
> +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.
> +
> + regmap = devm_regmap_init(dev, NULL, mmio, &sysmon_mmio_regmap_config);
NULL is not defined in the above headers.
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + return sysmon_core_probe(dev, regmap);
> +}
...
> +++ b/drivers/iio/adc/versal-sysmon.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * AMD Versal SysMon driver
> + *
> + * Copyright (C) 2019 - 2022, Xilinx, Inc.
> + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef _VERSAL_SYSMON_H_
> +#define _VERSAL_SYSMON_H_
> +
> +#include <linux/bits.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +struct device;
> +struct iio_dev;
> +struct regmap;
> +
> +/* Register offsets (sorted by address) */
> +#define SYSMON_NPI_LOCK 0x000C
> +#define SYSMON_ISR 0x0044
> +#define SYSMON_IDR 0x0050
> +#define SYSMON_TEMP_MAX 0x1030
> +#define SYSMON_TEMP_MIN 0x1034
> +#define SYSMON_SUPPLY_BASE 0x1040
> +#define SYSMON_TEMP_MIN_MIN 0x1F8C
> +#define SYSMON_TEMP_MAX_MAX 0x1F90
> +#define SYSMON_TEMP_SAT_BASE 0x1FAC
> +#define SYSMON_MAX_REG 0x24C0
> +
> +/* NPI unlock value written to SYSMON_NPI_LOCK */
> +#define SYSMON_NPI_UNLOCK_CODE 0xF9E8D7C6
> +
> +/* Register stride: 4 bytes per 32-bit register */
> +#define SYSMON_REG_STRIDE 4
> +
> +#define SYSMON_SUPPLY_IDX_MAX 159
> +#define SYSMON_TEMP_SAT_MAX 64
> +#define SYSMON_INTR_ALL_MASK GENMASK(31, 0)
> +
> +/* Supply voltage conversion register fields */
> +#define SYSMON_MANTISSA_MASK GENMASK(15, 0)
> +#define SYSMON_FMT_MASK BIT(16)
> +#define SYSMON_MODE_MASK GENMASK(18, 17)
> +
> +/* Q8.7 fractional shift */
> +#define SYSMON_FRACTIONAL_SHIFT 7U
> +#define SYSMON_SUPPLY_MANTISSA_BITS 16
> +
> +/**
> + * struct sysmon - Driver data for Versal SysMon
> + * @regmap: register map for hardware access
> + * @lock: protects read-modify-write sequences on threshold registers
> + * and cached state that spans multiple regmap calls
> + */
> +struct sysmon {
> + struct regmap *regmap;
> + /*
> + * Protects read-modify-write sequences on threshold registers
> + * and cached state (oversampling ratios, hysteresis values)
> + * that spans multiple regmap calls.
> + */
> + struct mutex lock;
> +};
> +
> +int sysmon_core_probe(struct device *dev, struct regmap *regmap);
> +
> +#endif /* _VERSAL_SYSMON_H_ */
> --
> 2.48.1
>
--
With Best Regards,
Andy Shevchenko