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

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

[Salih Erim]

Hi Andy,

Thanks for the review, replies inline.

On 5/4/2026 11:18 AM, Andy Shevchenko wrote:
> 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).

Accepted. Will audit all three files (core, MMIO, header) for IWYU
and add missing includes.

> 
> 
> ...
> 
>> +#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),                   \

Accepted.

> 
> 
>> +     .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.

Accepted. Will change the parameter type to s16.

> 
>> +}
> 
> ...
> 
>> +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().

Accepted. Will use sign_extend32(mantissa, 15).

> 
>> +     *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:

Accepted. Will use:

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

> 
> 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?

Unneccessary, will remove.

> 
>> +             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.

Accepted.
> 
>> +     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,

Accepted. Will use __free(fwnode_handle) for both supply_node and 
temp_node, eliminating the goto err_put pattern.

> 
>> +     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.

Will use size_add() for the allocation size computation.

> 
>> +                                    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?

With _free(fwnode_handle), the NULL guard becomes unnecessary since 
fwnode_for_each_child_node_scoped handles NULL parent. Will remove.

> 
>> +             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.

Will audit the MMIO driver and header for IWYU as well.

> 
> ...
> 
>> +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.

Accepted.

> 
>> +                               &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.

The issue is that MILLI from units.h is 1000UL, which causes unsigned
promotion when multiplied with signed int values (e.g. negative
temperatures or two's complement mantissa). There is
MILLIDEGREE_PER_DEGREE (signed 1000) but using it for voltage
channel conversions would be semantically wrong. Will either use
(int)MILLI at the call sites or just use literal 1000 -- happy to
go with whichever you prefer.

> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 
All items will be addressed in v3.

Salih

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

[Jonathan Cameron]

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_ */

Re: [PATCH v2 4/5] iio: adc: versal-sysmon: add threshold event support

[Jonathan Cameron]

On Sat, 2 May 2026 12:19:50 +0100
Salih Erim <salih.erim@amd.com> wrote:

> Add threshold event support for temperature and supply voltage
> channels.
> 
> Temperature events:
>   - Rising/falling threshold with configurable values
>   - Over-temperature (OT) alarm with separate thresholds
>   - Per-channel hysteresis configuration
> 
> Supply voltage events:
>   - Rising/falling threshold per supply channel
>   - Per-channel alarm enable via alarm configuration registers
> 
> The interrupt handler masks active threshold interrupts (which are
> level-sensitive) and schedules a delayed worker to poll for condition
> clear before unmasking. When no hardware IRQ is available (irq <= 0),
> event channels are not created and interrupt init is skipped, since
> the I2C regmap backend cannot be called from atomic context.
> 
> When disabling a supply channel alarm, the group interrupt remains
> active if any other channel in the same alarm group still has an
> alarm enabled.
> 
> Named constants replace magic numbers for hysteresis bit positions
> (SYSMON_OT_HYST_BIT, SYSMON_TEMP_HYST_BIT) and alarm register width
> (SYSMON_ALARM_BITS_PER_REG).
> 
> Hysteresis values are validated to single-bit range (0 or 1) before
> writing to the hardware register.
> 
> Signed-off-by: Salih Erim <salih.erim@amd.com>
A few minor comments inline to add to what Andy found.

>  drivers/iio/adc/versal-sysmon-core.c | 539 ++++++++++++++++++++++++++-
>  drivers/iio/adc/versal-sysmon.h      |  36 ++
>  2 files changed, 574 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 37736c2900b..857fe21db7a 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c

>  
> +/* OT and TEMP hysteresis bit positions in SYSMON_TEMP_EV_CFG */
> +#define SYSMON_OT_HYST_BIT		BIT(0)
> +#define SYSMON_TEMP_HYST_BIT		BIT(1)

You use a mix of these defines and manual shift.  Use FIELD_GET()
/FIELD_PREP() to avoid that.
>  
> +#define SYSMON_CHAN_TEMP_EVENT(_chan, _address, _ext, _events) {	\
> +	.type = IIO_TEMP,					\
> +	.indexed = 1,						\
> +	.address = _address,					\
> +	.channel = _chan,					\
> +	.event_spec = _events,					\
> +	.num_event_specs = ARRAY_SIZE(_events),			\
> +	.scan_type = {						\
> +		.sign = 's',					\
> +		.realbits = 15,					\
> +		.storagebits = 16,				\
> +		.endianness = IIO_CPU,				\
> +	},							\
> +	.datasheet_name = _ext,					\

As before - consider renaming this.

> +}

> +
> +static int sysmon_read_event_value(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   enum iio_event_type type,
> +				   enum iio_event_direction dir,
> +				   enum iio_event_info info, int *val,
> +				   int *val2)
> +{
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	u32 mask, shift;
> +	int offset;
> +	int ret;
> +
> +	guard(mutex)(&sysmon->lock);
> +
> +	if (chan->type == IIO_TEMP) {
> +		if (info == IIO_EV_INFO_VALUE) {
> +			offset = sysmon_temp_thresh_offset(chan->address, dir);
> +			if (offset < 0)
> +				return offset;
> +			ret = regmap_read(sysmon->regmap, offset, &reg_val);
> +			if (ret)
> +				return ret;
> +			sysmon_q8p7_to_millicelsius(reg_val, val);
> +			return IIO_VAL_INT;
> +		}
> +		if (info == IIO_EV_INFO_HYSTERESIS) {
> +			mask = (chan->address == SYSMON_ADDR_OT_EVENT) ?
> +				SYSMON_OT_HYST_BIT : SYSMON_TEMP_HYST_BIT;

Not a massive amount of sharing for OT_EVENT vs others. Maybe just split it
and then you can use FIELD_GET() and get shift handling included via the mask.

			ret = regmap_read(sysmon->regmap, SYSMON_TEMP_EV_CFG,
				  	  &reg_val);
			if (ret)
				return ret;
			if (chan->addres == SYSMBO_ADDR_OT_EVENT) {
				*val = FIELD_GET(SYSMON_OT_HYST_BIT, reg_val);
			else
				*val = FIELD_GET(YSMON_TEMP_HYST_BIT, reg_val);



> +			*val = (reg_val & mask) >> shift;
			}
> +			shift = (chan->address == SYSMON_ADDR_OT_EVENT) ? 0 : 1;
> +			ret = regmap_read(sysmon->regmap, SYSMON_TEMP_EV_CFG,
> +					  &reg_val);
> +			if (ret)
> +				return ret;
> +			*val = (reg_val & mask) >> shift;
> +			return IIO_VAL_INT;
> +		}

> +
> +static int sysmon_write_event_value(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir,
> +				    enum iio_event_info info, int val, int val2)
> +{
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	u32 mask, shift;
> +	u32 raw_val;
> +	int offset;
> +	int ret;
> +
> +	guard(mutex)(&sysmon->lock);
> +
> +	if (chan->type == IIO_TEMP) {
> +		if (info == IIO_EV_INFO_VALUE) {
> +			offset = sysmon_temp_thresh_offset(chan->address, dir);
> +			if (offset < 0)
> +				return offset;
> +			sysmon_millicelsius_to_q8p7(&raw_val, val);
> +			return regmap_write(sysmon->regmap, offset, raw_val);
> +		}
> +		if (info == IIO_EV_INFO_HYSTERESIS) {
> +			mask = (chan->address == SYSMON_ADDR_OT_EVENT) ?
> +				SYSMON_OT_HYST_BIT : SYSMON_TEMP_HYST_BIT;
> +			shift = (chan->address == SYSMON_ADDR_OT_EVENT) ? 0 : 1;
> +			if (val & ~1)

Just to confirm - this only has hysteresis values of 0 or 1?  That's unusually
small given hysteresis should be in same units as _raw.

Also similar to above, I'd split the two cases and use FIELD_PREP()

> +				return -EINVAL;
> +			return regmap_update_bits(sysmon->regmap,
> +						  SYSMON_TEMP_EV_CFG,
> +						  mask, val << shift);
> +		}
> +	} else if (chan->type == IIO_VOLTAGE) {
> +		offset = sysmon_supply_thresh_offset(chan->address, dir);
> +		if (offset < 0)
> +			return offset;
> +		ret = regmap_read(sysmon->regmap, offset, &reg_val);
> +		if (ret)
> +			return ret;
> +		sysmon_supply_processedtoraw(val, reg_val, &raw_val);
> +		return regmap_write(sysmon->regmap, offset, raw_val);
> +	}
> +
> +	return -EINVAL;
> +}

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

[Guenter Roeck]

On 5/4/26 10:32, Jonathan Cameron wrote:
> 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.
> 

This very much sounds like a hardware monitoring device to me.

Guenter

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

[Andy Shevchenko]

On Mon, May 04, 2026 at 04:50:05PM +0100, Salih Erim wrote:

> On 5/4/2026 11:18 AM, Andy Shevchenko wrote:
> > On Sat, May 02, 2026 at 12:19:48PM +0100, Salih Erim wrote:

...

> > > +     if (format)
> > > +             mantissa = (int)(s16)mantissa;
> > 
> > Potential user of FIELD_GET_SIGNED(), but for now just use explicit call to
> > sign_extend32().
> 
> Accepted. Will use sign_extend32(mantissa, 15).
> 
> > > +     *val = (mantissa * SYSMON_MILLI) >> exponent;

The idea is to think about this (and other) user and try to come up with the
solution that will not decrease readability and at the same time give more
context. Perhaps you need an additional patch that introduces the multiplier
for millivolts (as it's done for degrees).

...

> > > +/* 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.
> 
> The issue is that MILLI from units.h is 1000UL, which causes unsigned
> promotion when multiplied with signed int values (e.g. negative
> temperatures or two's complement mantissa). There is
> MILLIDEGREE_PER_DEGREE (signed 1000) but using it for voltage
> channel conversions would be semantically wrong. Will either use
> (int)MILLI at the call sites or just use literal 1000 -- happy to
> go with whichever you prefer.

Let's wait for resolution WRT hwmon vs. IIO.


-- 
With Best Regards,
Andy Shevchenko

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

[Salih Erim]

Hi Guenter and Jonathan,

On 5/4/2026 8:26 PM, Guenter Roeck wrote:
> 
> 
> On 5/4/26 10:32, Jonathan Cameron wrote:
>> 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.
>>
> 
> This very much sounds like a hardware monitoring device to me.

The device is indeed used for hardware monitoring, but the hardware
characteristics push it towards IIO:

- The predecessor (Zynq UltraScale+ AMS, xilinx-ams.c) is already
   in drivers/iio/adc/ upstream. This driver is the direct successor
   for the Versal generation.

- The supply voltage encoding is a modified floating-point format
   with per-register exponent and format bits. This non-linear
   encoding doesn't map well to hwmon's linear in*_input model.

- The device has configurable threshold events with per-channel
   alarm registers, hysteresis bits, and level-sensitive interrupt
   masking/unmasking -- which maps directly to the IIO event
   infrastructure.

- Oversampling is hardware-configurable per channel type with
   per-channel averaging enable registers.

- Up to 160 voltage and 64 temperature channels are dynamically
   configured from DT, which fits IIO's dynamic channel model
   better than hwmon's compile-time attribute groups.

- The follow-up thermal driver uses the IIO consumer API
   (iio_channel_read) to aggregate temperature data across
   multiple satellites into thermal zones. The iio-hwmon bridge
   then exposes the same data to hwmon userspace.

So the architecture is: IIO driver (provider) -> iio-hwmon bridge
(hwmon exposure) + IIO consumer (thermal zones). This gives both
hwmon and thermal framework access through a single IIO provider.
> 
> Guenter
> 

Salih.