Re: [PATCH v8 4/5] iio: adc: versal-sysmon: add threshold event support
From: Erim, Salih
Date: Wed Jun 17 2026 - 12:43:30 EST
Hi Andy,
All accepted, all will be fixed in v9.
On 17/06/2026 16:38, Andy Shevchenko wrote:
On Tue, Jun 16, 2026 at 02:15:58PM +0100, Salih Erim wrote:
Add threshold event support for temperature and supply voltage
channels.
Temperature events:
- Rising threshold with configurable value on the device
temperature channel (current max across all satellites)
- 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 hardware also provides a separate over-temperature (OT)
threshold, but it is not exposed through IIO as it serves as a
hardware safety mechanism for platform shutdown. OT will be
exposed through the thermal framework in a follow-up series.
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
specs are not attached 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.
A devm cleanup action masks all interrupts on driver unbind to
prevent unhandled interrupt storms after the IRQ handler is freed.
...
+static void sysmon_supply_processedtoraw(int val, u32 reg_val, u32 *raw_data)
+{
+ int exponent = FIELD_GET(SYSMON_MODE_MASK, reg_val);
+ int format = FIELD_GET(SYSMON_FMT_MASK, reg_val);
+ int scale, tmp;
+
+ scale = BIT(SYSMON_SUPPLY_MANTISSA_BITS - exponent);
+ tmp = (val * scale) / (int)MILLI;
+
+ if (format)
+ tmp = clamp(tmp, S16_MIN, S16_MAX);
+ else
+ tmp = clamp(tmp, 0, U16_MAX);
Double check that minmax.h is included.
Accepted. Will add <linux/minmax.h>
+ *raw_data = (u16)tmp;
+}
...
+static int sysmon_supply_thresh_offset(int address,
+ enum iio_event_direction dir)
Make it a single line. OTOH why is 'address' signed? Perhaps u32?
Or for some reason unsigned long as per _alarm_config()?
Accepted. Will Join to one line. Changed to unsigned long for consistency with sysmon_read_alarm_config() and sysmon_write_alarm_config().
+{
+ if (dir == IIO_EV_DIR_RISING)
+ return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_UP;
+ if (dir == IIO_EV_DIR_FALLING)
+ return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_LOW;
+
+ return -EINVAL;
+}
...
+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)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int imr;
+ int config_value;
+ u32 mask;
+ int ret;
+
+ mask = sysmon_get_event_mask(chan);
Just make it together, as we don't validate the value of 'mask'.
Accepted.
struct sysmon *sysmon = iio_priv(indio_dev);
u32 mask = sysmon_get_event_mask(chan);
...
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 & mask);
+
+ case IIO_TEMP:
+ /*
+ * Return the administrative state, not the hardware IMR.
+ * The IRQ handler temporarily masks the interrupt during
+ * the polling window; reading IMR would show it as disabled.
+ * temp_mask bit is set when administratively disabled.
+ */
+ return !(sysmon->temp_mask & mask);
+
+ default:
+ return -EINVAL;
+ }
+}
...
+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);
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ u32 ier = sysmon_get_event_mask(chan);
Here you call the variable 'ier'. Please, make this consistent in the related
APIs (see above).
Accepted. Will rename to 'mask' in write_event_config to match read_event_config
+ unsigned int alarm_config;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ switch (chan->type) {
+ case 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);
+
+ case IIO_TEMP:
+ 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;
+
+ default:
+ return -EINVAL;
+ }
+}
...
+static int sysmon_update_temp_lower(struct sysmon *sysmon)
+{
+ unsigned int upper_reg;
+ int upper_mc, lower_mc;
+ u32 raw_val;
+ int ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, &upper_reg);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
+
^^^
+ lower_mc = upper_mc - sysmon->temp_hysteresis;
Either add a blank line here, or remove the one above as these three is kinda
semantically coupled.
Accepted. Will removed the blank line, the three lines are a single conversion sequence.
+ sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);
+
+ return regmap_write(sysmon->regmap, SYSMON_TEMP_TH_LOW, raw_val);
+}
...
+static void sysmon_unmask_temp(struct sysmon *sysmon, unsigned int isr)
+{
+ unsigned int unmask, status;
As per above perhaps name 'unmask' as 'u32 ier'? Or did I miss the use case?
Accepted. Will rename to 'u32 ier', this is the value written to the IER register, so the name fits.
+ status = isr & SYSMON_TEMP_INTR_MASK;
+
+ unmask = ~status & sysmon->masked_temp;
+ sysmon->masked_temp &= status;
+
+ /* Only unmask if not administratively disabled by userspace */
+ unmask &= ~sysmon->temp_mask;
+
+ regmap_write(sysmon->regmap, SYSMON_IER, unmask);
+}
Also looking at all this, please double check variable names in all functions
and make types and names consistent across the whole driver code.
Accepted. Will audited. To be consistent: 'mask' for the event interrupt bitmask in read/write_event_config, 'ier' for values written to the IER
register in sysmon_unmask_temp.
+ }
...
- num_chan = size_add(num_temp, size_add(ARRAY_SIZE(temp_channels), num_supply));
+ num_static = ARRAY_SIZE(temp_channels);
+ num_chan = size_add(num_temp, size_add(num_static, num_supply));
At glance I don't see any additional arguments, can we introduce num_static in
the previous patch to reduce a churn here?
Accepted. Will move num_static introduction to P2.
Thanks,
Salih
--
With Best Regards,
Andy Shevchenko