Re: [PATCH v2 4/5] iio: adc: versal-sysmon: add threshold event support
From: Erim, Salih
Date: Sun May 17 2026 - 19:48:34 EST
Hi Andy,
On 04/05/2026 11:52, Andy Shevchenko wrote:
On Sat, May 02, 2026 at 12:19:50PM +0100, Salih Erim 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.
...
#include <linux/bitfield.h>
#include <linux/bits.h>
#include <linux/cleanup.h>
+#include <linux/iio/events.h>
#include <linux/iio/iio.h>
It's better from the start make it as a separate group of headers...
Accepted. Will group linux/iio/* headers together seperated by blank lines from the rest.
+#include <linux/interrupt.h>
+#include <linux/limits.h>
#include <linux/module.h>
#include <linux/property.h>
#include <linux/regmap.h>
...like here (after blank line!)
...those linux/iio/*...
+blank line.
#include "versal-sysmon.h"
Also follow IWYU.
Accepted.
...
static void sysmon_q8p7_to_millicelsius(int raw_data, int *val)
{
*val = ((s16)raw_data * SYSMON_MILLI) >> SYSMON_FRACTIONAL_SHIFT;
}
+static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
+{
+ *raw_data = (u16)((val * (int)BIT(SYSMON_FRACTIONAL_SHIFT)) / SYSMON_MILLI);
+}
Too many explicit castings... Think how to reduce amount of them. Also this
SYSMON_MILLI shouldn't be defined.
Besides that it's inconsistent to use right-shift in one case and BIT() in
the other.
Accpeted. Will make both directions consistent (use right-shift or BIT in both) and reduce casts by using proper types in parameters.
...
+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) / SYSMON_MILLI;
+
+ if (format)
+ tmp = clamp(tmp, (int)S16_MIN, (int)S16_MAX);
+ else
+ tmp = clamp(tmp, 0, (int)U16_MAX);
No, the use of clamp() assumes it should not have explicit casts.
Will use clamp_t(int, ...) or declare typed contants to avoid the casts.
+ *raw_data = tmp & U16_MAX;
Why?! The previous clamp() guarantees that, no?
Correct, clamp already bounds it. Will remove the mask.
+}
...
+static int sysmon_read_alarm_config(struct sysmon *sysmon,
+ unsigned long address)
+{
+ u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
+ u32 offset = SYSMON_ALARM_OFFSET(address);
+ unsigned int reg_val;
+ int ret;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ return reg_val & BIT(shift);
This won't work in case shift becomes 31. Because the returned value will be
considered by the callers as negative error code.
Good catch. Will use !!(reg_val & BIT(shift)) to ensure the return 0 or 1.
+}
...
+static int sysmon_write_alarm_config(struct sysmon *sysmon,
+ unsigned long address, u32 val)
+{
+ u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
+ u32 offset = SYSMON_ALARM_OFFSET(address);
+
+ return regmap_update_bits(sysmon->regmap, offset,
+ BIT(shift), val << shift);
Looks like regmap_set_bits().
The val parameter is 0 or 1, so this needs to both set and clear.
Will use regmap_set_bits() when val is true and regmap_clear_bits() when val is false.
+}
...
+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;
+ blank line and a comment why imr is negated.
Accepted. IMR is the interrupt mask register where 1 = masked. Negating gives the enabled interrupts. Will add a comment.
+ imr = ~imr;
+
+ if (chan->type == 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);
+ }
+
+ return !!(imr & alarm_event_mask);
+}
...
+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);
+ guard(spinlock_irqsave)(&sysmon->irq_lock);
This is wrong (imagine I²C driver calling this...).
Events are only created when irq > 0, and the I2C driver passes irq = 0 so event channels are never registered. This function cannot be reached from the I2C path. Will add a comment clarifying this. However if you prefer, can restructure to avoid spinlock in this entirely.
+
+ 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);
+ else
+ return regmap_write(sysmon->regmap, SYSMON_IDR, ier);
+ } else if (chan->type == IIO_TEMP) {
+ if (state) {
+ ret = regmap_write(sysmon->regmap, SYSMON_IER, ier);
+ if (ret)
+ return ret;
+ sysmon->temp_mask &= ~ier;
+ } else {
+ ret = regmap_write(sysmon->regmap, SYSMON_IDR, ier);
+ if (ret)
+ return ret;
+ sysmon->temp_mask |= ier;
+ }
+ }
+
+ return 0;
+}
...
+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)
Logical split, please (move int val to the next line, or int val2 to the
previous).
Accepted.
+{
+ 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, ®_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;
No need to calculate mask if regmap_read() fails.
Accepted. Will move mask/shift computation after the regmap_read() error check.
+ shift = (chan->address == SYSMON_ADDR_OT_EVENT) ? 0 : 1;
Ditto.
+ ret = regmap_read(sysmon->regmap, SYSMON_TEMP_EV_CFG,
+ ®_val);
+ if (ret)
+ return ret;
+ *val = (reg_val & mask) >> shift;
+ return IIO_VAL_INT;
+ }
+ } else if (chan->type == IIO_VOLTAGE) {
Redundant 'else'.
Accepted. Will remove.
+ 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 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;
Similar comments as per above.
Accepted.
+ if (val & ~1)
BIT(0), but still magic.
Will use a named constant or add a comment. The intent is to validate that val is a single-bit value (0 or 1).
+ return -EINVAL;
+ return regmap_update_bits(sysmon->regmap,
+ SYSMON_TEMP_EV_CFG,
+ mask, val << shift);
Also seems like a regmap_set_bits(). and instead of that ugly ternary just make
it if-else or similar.
Accepted. Will use if-else with regmap_set_bits()/regmap_clear_bits() and remove the ternary.
+ }
+ } else if (chan->type == IIO_VOLTAGE) {
Redundant 'else'.
+ 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;
+}
...
+static void sysmon_push_event(struct iio_dev *indio_dev, u32 address)
+{
+ const struct iio_chan_spec *chan;
+ unsigned int i;
Seems not used outside of the loop, hence...
Accepted.
+ for (i = 0; i < indio_dev->num_channels; i++) {
for (unsigned int i = 0; i < indio_dev->num_channels; i++) {
+ if (indio_dev->channels[i].address != address)
+ continue;
+
+ chan = &indio_dev->channels[i];
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(chan->type,
+ chan->channel,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_EITHER),
+ iio_get_time_ns(indio_dev));
+ }
+}
...
+static void sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
+{
Why no checks for error from IO accessors in this function?
THis runs from the IRQ handler under spinlock. Will add error checks and bail out on failure.
+ u32 alarm_flag_offset = SYSMON_ALARM_FLAG + (event * SYSMON_REG_STRIDE);
+ u32 alarm_reg_offset = SYSMON_ALARM_REG + (event * SYSMON_REG_STRIDE);
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned long alarm_flag_reg;
+ unsigned int reg_val;
+ u32 address, bit;
+
+ switch (event) {
+ case SYSMON_BIT_TEMP:
+ sysmon_push_event(indio_dev, SYSMON_ADDR_TEMP_EVENT);
+ regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_TEMP));
+ sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
+ break;
+
+ case SYSMON_BIT_OT:
+ sysmon_push_event(indio_dev, SYSMON_ADDR_OT_EVENT);
+ regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_OT));
+ sysmon->masked_temp |= BIT(SYSMON_BIT_OT);
+ break;
+
+ case SYSMON_BIT_ALARM0:
+ case SYSMON_BIT_ALARM1:
+ case SYSMON_BIT_ALARM2:
+ case SYSMON_BIT_ALARM3:
+ case SYSMON_BIT_ALARM4:
+ regmap_read(sysmon->regmap, alarm_flag_offset, ®_val);
+ alarm_flag_reg = (unsigned long)reg_val;
+
+ for_each_set_bit(bit, &alarm_flag_reg,
+ SYSMON_ALARM_BITS_PER_REG) {
+ address = bit + (SYSMON_ALARM_BITS_PER_REG * event);
+ sysmon_push_event(indio_dev, address);
+ regmap_update_bits(sysmon->regmap, alarm_reg_offset,
+ BIT(bit), 0);
+ }
+ regmap_write(sysmon->regmap, alarm_flag_offset, alarm_flag_reg);
+ break;
+
+ default:
+ break;
+ }
+}
...
+ return isr ? IRQ_HANDLED : IRQ_NONE;
There is a macro for this. IIRC IRQ_RETVAL().
Accepted. Will use IRQ_RETVAL().
...
+static int sysmon_init_interrupt(struct sysmon *sysmon)
+{
+ unsigned int imr;
+ int ret;
+
+ /* Events not supported without IRQ (e.g. I2C path) */
+ if (sysmon->irq <= 0)
When can '=' happen?
irq == 0 comes from the I2C backend, which passes 0 explicitly to sysmon_core_probe() to indicate no IRQ is available. platform_get_irq_optional() itself returns only positive values or negative error codes. Will change to if (!sysmon->irq) for clarity.
+ return 0;
+
+ INIT_DELAYED_WORK(&sysmon->sysmon_unmask_work, sysmon_unmask_worker);
+
+ ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+ if (ret)
+ return ret;
+ sysmon->temp_mask = imr & SYSMON_TEMP_INTR_MASK;
+
+ ret = devm_add_action_or_reset(sysmon->dev, sysmon_cancel_work,
+ sysmon);
+ if (ret)
+ return ret;
Don't we have include/linux/devm-helpers.h?
Yes. Will use devm_delayed_work_autocancel() instead of manual INIT_DELAYED_WORK + devm_add_action_or_reset.
+ return devm_request_irq(sysmon->dev, sysmon->irq,
+ sysmon_iio_irq, 0, "sysmon-irq",
+ sysmon->indio_dev);
+}
--
With Best Regards,
Andy Shevchenko
All items will be addressed in v3.
Thanks,
Salih.