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

[Andy Shevchenko]

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?

> +			*val2 = BIT(SYSMON_FRACTIONAL_SHIFT);
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +		if (mask != IIO_CHAN_INFO_RAW)
> +			return -EINVAL;
> +
> +		ret = regmap_read(sysmon->regmap, chan->address, &regval);
> +		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, &regval);
> +		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

> +	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", &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", &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().

> + * @dev: Parent device
> + * @regmap: Register map for hardware access
> + *
> + * Return: 0 on success, negative errno on failure.
> + */

-- 
With Best Regards,
Andy Shevchenko