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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 2 May 2026 12:19:48 +0100
Salih Erim <salih.erim@amd.com> 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

Various comments inline.  One thing to check.
Is this one strictly a hardware monitoring device? Or does it
get used for more general ADC purposes?  Did you consider an HWMON driver
for it? The above sounds a lot like hwmon. So why IIO for this one?

I wasn't awake enough on v1 to raise this!  Sorry about that.
+CC Guenter and linux-hwmon for that discussion.

Thanks,

Jonathan

> 
> 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.
> 
> Co-developed-by: Michal Simek <michal.simek@amd.com>
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> Signed-off-by: Salih Erim <salih.erim@amd.com>



> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> new file mode 100644
> index 00000000000..37736c2900b
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon-core.c
> @@ -0,0 +1,320 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Versal SysMon core driver
> + *
> + * Copyright (C) 2019 - 2022, Xilinx, Inc.
> + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
> + */
> +
> +#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>
> +
> +#include "versal-sysmon.h"
> +
> +#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),			\
Why do you need raw and processed?  There are a few reasons that can show
up in a driver.
1) Non linear transforms that aren't reversable and we need to be
   able to threshold on the raw.
2) History - don't copy this but some drivers used _PROCESSED and
   reviewers (i.e. mostly me) weren't paying attention. We then needed
   to add _raw for buffered / events and so ended up with both.

1 might apply here I guess?  If so add a comment to that affect above
.info_mask_separate being assigned.  I don't want this to get copied
into most drivers where things aren't non linear.


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

Can we rename _ext.  We still have legacy .extended_name in here
and people might assume that is in use (definitely don't use that!)
_name or something like that instead of _ext?
 
> +}
> +
> +/* Static temperature channels (always present) */
> +static const struct iio_chan_spec temp_channels[] = {
> +	SYSMON_CHAN_TEMP(0, SYSMON_TEMP_MAX, "temp"),
> +	SYSMON_CHAN_TEMP(1, SYSMON_TEMP_MIN, "min"),
> +	SYSMON_CHAN_TEMP(2, SYSMON_TEMP_MAX_MAX, "max_max"),
> +	SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
> +};
> +
> +static void sysmon_q8p7_to_millicelsius(int raw_data, int *val)
> +{
> +	*val = ((s16)raw_data * SYSMON_MILLI) >> SYSMON_FRACTIONAL_SHIFT;
> +}
> +
> +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;
> +
> +	*val = (mantissa * SYSMON_MILLI) >> exponent;
> +}

> +/**
> + * 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;
> +	struct iio_chan_spec *sysmon_channels;
> +	const char *label;
> +	u32 reg;
> +	int ret;
> +
> +	supply_node = device_get_named_child_node(dev, "supply-channels");
> +	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);

Once you have these using __free() look closely at the various exit points.
Most can be direct returns and as this is only called from probe() you can
use dev_err_probe() to simplify things a little.

> diff --git a/drivers/iio/adc/versal-sysmon.c b/drivers/iio/adc/versal-sysmon.c
> new file mode 100644
> index 00000000000..c597934e869
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon.c

> +
> +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,
> +				  &sysmon_mmio_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	irq = platform_get_irq_optional(pdev, 0);

Could you instead use fwnode_irq_get(dev_fwnode(dev), 0) in the
shared code to get  this?  There may be subtle differences in
handling but those will be in strange corner cases that probably
don't apply here.

> +
> +	return sysmon_core_probe(&pdev->dev, regmap, irq);
> +}

> diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
> new file mode 100644
> index 00000000000..fc4d2338328
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon.h
> @@ -0,0 +1,69 @@

> +
> +/* Q8.7 fractional shift */
> +#define SYSMON_FRACTIONAL_SHIFT		7U
> +#define SYSMON_SUPPLY_MANTISSA_BITS	16
> +
> +/* Signed milli scale (MILLI from linux/units.h is unsigned long) */
> +#define SYSMON_MILLI			1000

Cast that one rather than defining the number again.  I see you discussed
this with Andy but no conclusion reached.

> +
> +/**
> + * struct sysmon - Driver data for Versal SysMon
> + * @dev: pointer to device struct
> + * @indio_dev: pointer to the iio device (needed for work callbacks)
> + * @regmap: register map for hardware access
> + * @lock: mutex for serializing user-space access

Normally we talk about what data is protected. The comment below
is better.  Also ignore checkpatch, you don't need it commented down there
- in kernel-doc is absolutely fine.

> + * @irq: interrupt number
> + */
> +struct sysmon {
> +	struct device *dev;

I'm always a bit in two minds about this. You can always get the dev
from the regmap but it can be a little more ugly than strictly necessary.
See how bad it is using regmap_get_device()

> +	struct iio_dev *indio_dev;
This smells backwards. Given this sysmon structure is allocated in the
private area of iio_priv() we should never need to go in this direction.
I don't think you really use this beyond one place where you can easily pass
it to. Anyhow this indio_dev pointer needs to go.

> +	struct regmap *regmap;
> +	/* Serializes access to device registers and state */
> +	struct mutex lock;
> +	int irq;

It's relatively rare to need to keep irq around after probe() and
I don't think you need to here.  Just store it in a local variable
that you pass into the interrupt init functions.

> +};
> +
> +int sysmon_core_probe(struct device *dev, struct regmap *regmap, int irq);
> +
> +#endif /* _VERSAL_SYSMON_H_ */