Re: [PATCH v5 4/5] iio: adc: versal-sysmon: add threshold event support
From: Erim, Salih
Date: Wed Jun 10 2026 - 08:10:25 EST
Hi Andy,
On 09/06/2026 18:54, Andy Shevchenko wrote:
On Mon, Jun 08, 2026 at 07:38:00PM +0100, Salih Erim 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
- 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.
...
+#define SYSMON_CHAN_TEMP_EVENT(_chan, _address, _name, _events) {\
Just move { to be on the separate line, it will make the macro look better.
Accepted. Will fix both SYSMON_CHAN_TEMP and SYSMON_CHAN_TEMP_EVENT macros.
#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, \
+}
...
+static int sysmon_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ bool state)
+{
+ u32 offset = SYSMON_ALARM_OFFSET(chan->address);
+ u32 ier = sysmon_get_event_mask(chan->address);
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int alarm_config;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ if (chan->type == IIO_VOLTAGE) {
+ ret = sysmon_write_alarm_config(sysmon, chan->address, state);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(sysmon->regmap, offset, &alarm_config);
+ if (ret)
+ return ret;
+
+ if (alarm_config)
+ return regmap_write(sysmon->regmap, SYSMON_IER, ier);
+
+ return regmap_write(sysmon->regmap, SYSMON_IDR, ier);
+ }
+
+ if (chan->type == IIO_TEMP) {
Still same problem you promised to address. Please, go back to the previous
thread and check again what has been addressed and what's not.
You're right on all counts, and I apologize for wasting your time
reviewing the same issues.
The cascading if statements should have been converted to switch
in v5. I fixed it in the oversampling patch but missed these event
functions. All will use switch(chan->type) in v6, and
read/write_event_value will also use switch(info) for the nested
dispatch.
+ if (state) {
+ ret = regmap_write(sysmon->regmap, SYSMON_IER, ier);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irq, &sysmon->irq_lock)
+ sysmon->temp_mask &= ~ier;
+ } else {
+ ret = regmap_write(sysmon->regmap, SYSMON_IDR, ier);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irq, &sysmon->irq_lock)
+ sysmon->temp_mask |= ier;
+ }
+ }
+
+ return 0;
+}
...
+static int sysmon_update_temp_lower(struct sysmon *sysmon, int address)
+{
+ unsigned int upper_reg;
+ int upper_mc, lower_mc, hysteresis;
+ u32 raw_val;
+ int upper_off, lower_off, ret;
Keep in reversed xmas tree order.
Accepted.
+ upper_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_RISING);
+ if (upper_off < 0)
+ return upper_off;
+ lower_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_FALLING);
+ if (lower_off < 0)
+ return lower_off;
+
+ if (address == SYSMON_ADDR_OT_EVENT)
+ hysteresis = sysmon->ot_hysteresis;
+ else
+ hysteresis = sysmon->temp_hysteresis;
+
+ ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
+
+ lower_mc = upper_mc - hysteresis;
+ sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);
+
+ return regmap_write(sysmon->regmap, lower_off, raw_val);
+}
...
+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;
+ int offset;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ if (chan->type == IIO_TEMP) {
+ if (info == IIO_EV_INFO_VALUE) {
+ /* Only rising threshold is exposed */
+ offset = sysmon_temp_thresh_offset(chan->address,
+ IIO_EV_DIR_RISING);
+ if (offset < 0)
+ return offset;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(reg_val, val);
+
+ return IIO_VAL_INT;
+ }
+ if (info == IIO_EV_INFO_HYSTERESIS) {
+ if (chan->address == SYSMON_ADDR_OT_EVENT)
+ *val = sysmon->ot_hysteresis;
+ else
+ *val = sysmon->temp_hysteresis;
+ return IIO_VAL_INT;
+ }
+ }
+
+ if (chan->type == IIO_VOLTAGE) {
Again, same issue. Are you sure you sent the new version?
That's purely my mistake, I will address them all in new version.
+ offset = sysmon_supply_thresh_offset(chan->address, dir);
+ if (offset < 0)
+ return offset;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_supply_rawtoprocessed(reg_val, val);
+
+ return IIO_VAL_INT;
+ }
+
+ return -EINVAL;
+}
...
+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 raw_val;
+ int offset;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ if (chan->type == IIO_TEMP) {
+ if (info == IIO_EV_INFO_VALUE) {
+ /* Only rising threshold is exposed */
+ offset = sysmon_temp_thresh_offset(chan->address,
+ IIO_EV_DIR_RISING);
+ if (offset < 0)
+ return offset;
+
+ sysmon_millicelsius_to_q8p7(&raw_val, val);
+
+ ret = regmap_write(sysmon->regmap, offset, raw_val);
+ if (ret)
+ return ret;
+
+ /* Recompute lower = upper - hysteresis */
+ return sysmon_update_temp_lower(sysmon, chan->address);
+ }
+ if (info == IIO_EV_INFO_HYSTERESIS) {
Ditto.
Will be fixed in v6.
+ if (val < 0)
+ return -EINVAL;
+
+ if (chan->address == SYSMON_ADDR_OT_EVENT)
+ sysmon->ot_hysteresis = val;
+ else
+ sysmon->temp_hysteresis = val;
+
+ return sysmon_update_temp_lower(sysmon, chan->address);
+ }
+ }
+
+ if (chan->type == IIO_VOLTAGE) {
Ditto.
Will be fixed in v6.
+ offset = sysmon_supply_thresh_offset(chan->address, dir);
+ if (offset < 0)
+ return offset;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_supply_processedtoraw(val, reg_val, &raw_val);
+
+ return regmap_write(sysmon->regmap, offset, raw_val);
+ }
+
+ return -EINVAL;
+}
...
+/*
+ * Versal threshold interrupts are level-sensitive. Active threshold
+ * interrupts are masked in the handler and polled via delayed work
+ * until the condition clears, then unmasked.
+ */
+static void sysmon_unmask_worker(struct work_struct *work)
+{
+ struct sysmon *sysmon =
+ container_of(work, struct sysmon, sysmon_unmask_work.work);
+ unsigned int isr;
+
+ /*
+ * regmap errors are not checked here because the worker and IRQ
+ * handler cannot propagate errors. The MMIO regmap uses fast_io
+ * with direct readl/writel which cannot fail.
OK (but they can fail on HW level to the point of bus errors or so :).
Accepted. Will add error checks to modify flow on failure
instead of just documenting why they're absent.
+ */
+ spin_lock_irq(&sysmon->irq_lock);
+ regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
+ regmap_write(sysmon->regmap, SYSMON_ISR, isr);
+ sysmon_unmask_temp(sysmon, isr);
+ spin_unlock_irq(&sysmon->irq_lock);
+
+ if (sysmon->masked_temp)
+ schedule_delayed_work(&sysmon->sysmon_unmask_work,
+ msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
+ else
+ regmap_write(sysmon->regmap, SYSMON_STATUS_RESET, 1);
+}
...
+static int sysmon_init_interrupt(struct sysmon *sysmon,
+ struct device *dev,
+ struct iio_dev *indio_dev,
+ int irq)
+{
+ unsigned int imr;
+ int ret;
+
+ /* Events not supported without IRQ (e.g. I2C path) */
+ if (!irq)
+ return 0;
+
+ ret = devm_delayed_work_autocancel(dev, &sysmon->sysmon_unmask_work,
+ sysmon_unmask_worker);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+ if (ret)
+ return ret;
+ sysmon->temp_mask = imr & SYSMON_TEMP_INTR_MASK;
+
+ return devm_request_irq(dev, irq, sysmon_iio_irq, 0,
+ "sysmon-irq", indio_dev);
I would do that on a single line, but it's 86 characters long, so up to
Jonathan.
Accepted. Will join on a single line. Happy to split it back if Jonathan prefers.
+}
...
+static int sysmon_init_hysteresis(struct sysmon *sysmon, unsigned int address,
+ int *hysteresis)
+{
+ unsigned int upper_reg, lower_reg;
+ int upper_mc, lower_mc;
+ int upper_off, lower_off;
Reversed xmas tree order.
Accepted.
+ int ret;
+
+ upper_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_RISING);
+ if (upper_off < 0)
+ return upper_off;
+ lower_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_FALLING);
+ if (lower_off < 0)
+ return lower_off;
+
+ ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(sysmon->regmap, lower_off, &lower_reg);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
+ sysmon_q8p7_to_millicelsius(lower_reg, &lower_mc);
+ *hysteresis = upper_mc - lower_mc;
+
+ return 0;
+}
...
sysmon_channels = devm_kcalloc(dev,
- size_add(size_add(ARRAY_SIZE(temp_channels),
+ size_add(size_add(num_static,
num_supply), num_temp),
sizeof(*sysmon_channels), GFP_KERNEL);
Same comment as per previous patch.
Accepted.
if (!sysmon_channels)
return -ENOMEM;
...
--- a/drivers/iio/adc/versal-sysmon.h
+++ b/drivers/iio/adc/versal-sysmon.h
#include <linux/bits.h>
#include <linux/mutex.h>
+#include <linux/spinlock_types.h>
+#include <linux/types.h>
Same comment as per previous round. Really, please double check what you missed
to address.
I removed types.h from P2 as you asked, then added it at P4
assuming the new struct members needed it. They don't --
spinlock_t comes from spinlock_types.h, unsigned int and int are
built-in, struct delayed_work comes from workqueue.h. Will remove
types.h from the header entirely.
Regards,
Salih
+#include <linux/workqueue.h>
--
With Best Regards,
Andy Shevchenko