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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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