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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 11 Jun 2026 23:27:37 +0100
Salih Erim <salih.erim@amd.com> wrote:

> Add threshold event support for temperature and supply voltage
> channels.
> 
> Temperature events:
>   - Rising threshold with configurable value
>   - Over-temperature (OT) alarm with separate threshold

Ah. I ask about this below.  If this applies to the same channel
as the main threshold we generally don't support that in IIO and
definitely not by introducing a 'magic' extra channel.
See below.

>   - Per-channel hysteresis as a millicelsius value
>   - Event direction is IIO_EV_DIR_RISING (hysteresis mode)
> 
> Supply voltage events:
>   - Rising/falling threshold per supply channel
>   - Per-channel alarm enable via alarm configuration registers
> 
> The hardware supports both window and hysteresis alarm modes for
> temperature. This driver uses hysteresis mode, where the upper
> threshold triggers the alarm and the lower threshold clears it
> (re-arm point). The hardware has a single ISR bit per temperature
> channel with no indication of which threshold was crossed, so
> hysteresis mode is the natural fit. The lower threshold register
> is computed internally as (upper - hysteresis).
> 
> Hysteresis is stored in the driver as a millicelsius value,
> initialized from the hardware registers at probe. Writing the
> rising threshold or hysteresis recomputes the lower register.
> ALARM_CONFIG is hard-coded to hysteresis mode during init.
> 
> 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, 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.
> 
> Signed-off-by: Salih Erim <salih.erim@amd.com>

Some follow on questions on the temperature channels and one thing
Sashiko noticed that looks real.

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index c875d156dbe..20fd3a87d44 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
> @@ -11,7 +11,9 @@
>  #include <linux/bitops.h>
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
> +#include <linux/devm-helpers.h>
>  #include <linux/err.h>
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
> @@ -19,10 +21,19 @@
>  #include <linux/sysfs.h>
>  #include <linux/units.h>
>  
> +#include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  
>  #include "versal-sysmon.h"
>  
> +/* OT and TEMP hysteresis mode bits in SYSMON_TEMP_EV_CFG */
> +#define SYSMON_OT_HYST_MASK		BIT(0)
> +#define SYSMON_TEMP_HYST_MASK		BIT(1)
> +
> +/* Compute alarm register offset from a channel address */
> +#define SYSMON_ALARM_OFFSET(addr) \
> +	(SYSMON_ALARM_REG + ((addr) / SYSMON_ALARM_BITS_PER_REG) * SYSMON_REG_STRIDE)
> +
>  #define SYSMON_CHAN_TEMP(_chan, _address, _name)		\
>  {								\
>  	.type = IIO_TEMP,					\
> @@ -34,14 +45,87 @@
>  	.datasheet_name = _name,				\
>  }
>  
> +#define SYSMON_CHAN_TEMP_EVENT(_chan, _address, _name, _events)	\
> +{								\
> +	.type = IIO_TEMP,					\
> +	.indexed = 1,						\
> +	.address = _address,					\
> +	.channel = _chan,					\
> +	.event_spec = _events,					\
> +	.num_event_specs = ARRAY_SIZE(_events),			\
> +	.datasheet_name = _name,				\
> +}
> +
> +enum sysmon_alarm_bit {
> +	SYSMON_BIT_ALARM0 = 0,
> +	SYSMON_BIT_ALARM1 = 1,
> +	SYSMON_BIT_ALARM2 = 2,
> +	SYSMON_BIT_ALARM3 = 3,
> +	SYSMON_BIT_ALARM4 = 4,
> +	SYSMON_BIT_OT = 8,
> +	SYSMON_BIT_TEMP = 9,
> +};

>  /* Static temperature channels (always present) */
> -static const struct iio_chan_spec temp_channels[] = {
> +static const struct iio_chan_spec temp_channels_no_events[] = {
>  	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 temperature channels with event support (when IRQ available) */
> +static const struct iio_chan_spec temp_channels_with_events[] = {
> +	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"),
> +	SYSMON_CHAN_TEMP_EVENT(4, SYSMON_ADDR_TEMP_EVENT, "temp",
> +			      sysmon_temp_events),
Is this not an event on channel 0?  Why does it need a separate one?
> +	SYSMON_CHAN_TEMP_EVENT(5, SYSMON_ADDR_OT_EVENT, "ot",

Why two separate channels for events? Are we dealing with two separate
events on the same signal? Generally we don't support that for IIO because
it's largely meaningless except in hwmon usecases - what is the point in two
thresholds if they are reported through the same path?  Just use one and update
it if you want to add another level of detection.

> +			      sysmon_temp_events),
> +};

> +
> +static int sysmon_read_event_config(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir)
> +{
> +	u32 alarm_event_mask = sysmon_get_event_mask(chan->address);
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int imr;
> +	int config_value;
> +	int ret;
> +
> +	ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
> +	if (ret)
> +		return ret;
> +
> +	/* IMR bits are 1=masked, invert to get 1=enabled */
> +	imr = ~imr;
> +
> +	switch (chan->type) {
> +	case IIO_VOLTAGE:
> +		config_value = sysmon_read_alarm_config(sysmon, chan->address);
> +		if (config_value < 0)
> +			return config_value;
> +		return config_value && (imr & alarm_event_mask);
> +
> +	case IIO_TEMP:
> +		return !!(imr & alarm_event_mask);

Sashiko made a perhaps insightful observation here.  When the interrupt
is masked between sending an event and the worker reenabling it does
this give an unexpected value to userspace?  I think that condition
we'd kind of expect this to return 0.
https://sashiko.dev/#/patchset/20260611222738.2035062-1-salih.erim%40amd.com

I think the rest of the feedback is probably false positives or debatable
stuff but this one rang true. Please do take a look at the other stuff
as I may have missed something (maybe the comment about needing to disable
event interrupt generation is true?)

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




> +