Re: [PATCH v2 2/5] iio: adc: add Versal SysMon driver

Andy Shevchenko
2026-05-04T10:18:55Z

On Sat, May 02, 2026 at 12:19:48PM +0100, Salih Erim wrote:
> Add the AMD/Xilinx Versal System Monitor (SysMon) IIO driver.
>
> The driver is split into a bus-agnostic core module
> (versal-sysmon-core) and a memory-mapped I/O platform driver
> (versal-sysmon). The core uses the regmap API so that different
> bus implementations can share the same IIO logic.
>
> The core provides:
> - Static temperature channels (current max/min, peak max/min)
> - Supply voltage channels parsed from DT container nodes
> - Temperature satellite channels parsed from DT container nodes
> - read_raw for IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_PROCESSED
> - read_label using the DT label property
>
> The MMIO platform driver provides:
> - Memory-mapped register access via custom regmap callbacks
> - NPI unlock before every register write (platform management
> controller may re-lock NPI unpredictably on Versal devices)
>
> Threshold events, oversampling, and I2C bus support are added in
> subsequent patches.

...

IWYU please:

+ array_size.h

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>

+ string.h
+ types.h

(and maybe more that I missed).

...

> +#define SYSMON_CHAN_TEMP(_chan, _address, _ext) { \
> + .type = IIO_TEMP, \
> + .indexed = 1, \
> + .address = _address, \
> + .channel = _chan, \

> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_PROCESSED), \

It's better to use the following style:

.info_mask_separate = \
BIT(IIO_CHAN_INFO_RAW) | \
BIT(IIO_CHAN_INFO_PROCESSED), \

> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 15, \
> + .storagebits = 16, \
> + .endianness = IIO_CPU, \
> + }, \
> + .datasheet_name = _ext, \
> +}

...

> +static void sysmon_q8p7_to_millicelsius(int raw_data, int *val)
> +{
> + *val = ((s16)raw_data * SYSMON_MILLI) >> SYSMON_FRACTIONAL_SHIFT;

No casting is needed, just make the type s16 to begin with.

> +}

...

> +static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
> +{
> + int mantissa, format, exponent;
> +
> + mantissa = FIELD_GET(SYSMON_MANTISSA_MASK, raw_data);
> + exponent = SYSMON_SUPPLY_MANTISSA_BITS - FIELD_GET(SYSMON_MODE_MASK, raw_data);
> + format = FIELD_GET(SYSMON_FMT_MASK, raw_data);
> + /*
> + * When format bit is set the mantissa is two's complement
> + * (per hardware spec); sign-extend to int for correct arithmetic.
> + */
> + if (format)
> + mantissa = (int)(s16)mantissa;

Potential user of FIELD_GET_SIGNED(), but for now just use explicit call to
sign_extend32().

> + *val = (mantissa * SYSMON_MILLI) >> exponent;
> +}

...

> +static int sysmon_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)

Use logical split:

static int sysmon_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int *val, int *val2,
long mask)

OR (to be strict in 80 limit)

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;
> +
> + if (mask != IIO_CHAN_INFO_RAW && mask != IIO_CHAN_INFO_PROCESSED)
> + return -EINVAL;
> +
> + guard(mutex)(&sysmon->lock);
> +
> + switch (chan->type) {
> + case IIO_TEMP:
> + ret = regmap_read(sysmon->regmap, chan->address, &regval);
> + if (ret)
> + return ret;
> + if (mask == IIO_CHAN_INFO_PROCESSED)
> + sysmon_q8p7_to_millicelsius(regval, val);
> + else
> + *val = (int)regval;

Why casting?

> + return IIO_VAL_INT;
> +
> + case IIO_VOLTAGE:
> + ret = regmap_read(sysmon->regmap,
> + (chan->address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_BASE,
> + &regval);
> + if (ret)
> + return ret;
> + if (mask == IIO_CHAN_INFO_PROCESSED)
> + sysmon_supply_rawtoprocessed(regval, val);
> + else
> + *val = (int)regval;

Ditto.

> + return IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> + }
> +}

...

> +/**
> + * sysmon_parse_fw() - Parse firmware nodes and configure IIO channels.
> + * @indio_dev: IIO device instance
> + * @dev: Parent device
> + *
> + * Reads supply-channels and temperature-channels container nodes from
> + * firmware and builds the IIO channel array. Static temperature channels
> + * are prepended, followed by supply and satellite channels from DT.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
> +{
> + unsigned int idx, temp_chan_idx, volt_chan_idx;
> + struct fwnode_handle *supply_node, *temp_node;

> + unsigned int num_supply = 0, num_temp = 0;

Better for maintenance to split these assignments and perform them when they
are really needed.

> + struct iio_chan_spec *sysmon_channels;
> + const char *label;
> + u32 reg;
> + int ret;

> + supply_node = device_get_named_child_node(dev, "supply-channels");

Use what cleanup.h provides you,

> + if (supply_node)
> + num_supply = fwnode_get_child_node_count(supply_node);
> +
> + temp_node = device_get_named_child_node(dev, "temperature-channels");
> + if (temp_node)
> + num_temp = fwnode_get_child_node_count(temp_node);

As per above.

> + sysmon_channels = devm_kcalloc(dev,
> + ARRAY_SIZE(temp_channels) +
> + num_supply + num_temp,

Each of num_supply and num_temp may lead to overflow on 32-bit machines.

> + sizeof(*sysmon_channels), GFP_KERNEL);
> + if (!sysmon_channels) {
> + ret = -ENOMEM;
> + goto err_put;
> + }
> +
> + /* Static temperature channels first (fixed indices) */
> + idx = 0;
> + memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
> + idx += ARRAY_SIZE(temp_channels);
> +
> + /* Supply channels from DT */

> + if (supply_node) {

Why? Do you expect the below loop to go on this being NULL?

> + fwnode_for_each_child_node_scoped(supply_node, child) {
> + ret = fwnode_property_read_u32(child, "reg", &reg);
> + if (ret < 0)
> + goto err_put;
> +
> + if (reg > SYSMON_SUPPLY_IDX_MAX) {
> + ret = -EINVAL;
> + dev_err(dev, "supply reg %u exceeds max %u\n",
> + reg, SYSMON_SUPPLY_IDX_MAX);
> + goto err_put;
> + }
> +
> + ret = fwnode_property_read_string(child, "label",
> + &label);
> + if (ret < 0)
> + goto err_put;
> +
> + sysmon_channels[idx++] = (struct iio_chan_spec) {
> + .type = IIO_VOLTAGE,
> + .indexed = 1,
> + .address = reg,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_PROCESSED),
> + .scan_type = {
> + .realbits = 19,
> + .storagebits = 32,
> + .endianness = IIO_CPU,
> + .sign = fwnode_property_read_bool(child,
> + "bipolar") ? 's' : 'u',
> + },
> + .datasheet_name = label,
> + };
> + }
> + fwnode_handle_put(supply_node);
> + supply_node = NULL;
> + }
> +
> + /* Temperature satellite channels from DT */
> + if (temp_node) {

As per above.

> + fwnode_for_each_child_node_scoped(temp_node, child) {
> + ret = fwnode_property_read_u32(child, "reg", &reg);
> + if (ret < 0)
> + goto err_put;
> +
> + if (reg < 1 || reg > SYSMON_TEMP_SAT_MAX) {
> + ret = -EINVAL;
> + dev_err(dev, "temp reg %u out of range [1..%u]\n",
> + reg, SYSMON_TEMP_SAT_MAX);
> + goto err_put;
> + }
> +
> + ret = fwnode_property_read_string(child, "label",
> + &label);
> + if (ret < 0)
> + goto err_put;
> +
> + 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) |
> + BIT(IIO_CHAN_INFO_PROCESSED),
> + .scan_type = {
> + .sign = 's',
> + .realbits = 15,
> + .storagebits = 16,
> + .endianness = IIO_CPU,
> + },
> + .datasheet_name = label,
> + };
> + }
> + fwnode_handle_put(temp_node);
> + temp_node = NULL;
> + }
> +
> + 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 (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;
> +
> +err_put:
> + fwnode_handle_put(supply_node);
> + fwnode_handle_put(temp_node);
> + return ret;
> +}

...

> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

As per above, please respect and follow IWYU principle.

...

> +static int sysmon_platform_probe(struct platform_device *pdev)
> +{
> + struct sysmon_mmio *mmio;
> + struct regmap *regmap;
> + int irq;
> +
> + mmio = devm_kzalloc(&pdev->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);
> +
> + regmap = devm_regmap_init(&pdev->dev, NULL, mmio,

Using

struct device *dev = &pdev->dev;

at the top of the function makes this code neater and probably a single line.

> + &sysmon_mmio_regmap_config);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + irq = platform_get_irq_optional(pdev, 0);
> +
> + return sysmon_core_probe(&pdev->dev, regmap, irq);
> +}

...

> +#include <linux/iio/iio.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>

Ditto for the header files, use and follow the IWYU principle.

...

> +/* Signed milli scale (MILLI from linux/units.h is unsigned long) */
> +#define SYSMON_MILLI 1000

I think you want a different approach. I already forgot how it's being used,
but there shouldn't be a new (re-)definition just because of this.

--
With Best Regards,
Andy Shevchenko