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

[Erim, Salih]

Hi Jonathan,

Thanks for reviews, replies are inline.

On 12/06/2026 13:52, Jonathan Cameron wrote:
> On Thu, 11 Jun 2026 23:27:37 +0100
> Salih Erim <salih.erim@xxxxxxx> 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@xxxxxxx>
>
> 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?

The hardware has two independent threshold register pairs on the
same DEVICE_TEMP_MAX measurement: a TEMP threshold (ISR bit 9)
and an OT threshold (ISR bit 8), each with its own hysteresis.
We modelled them as separate event-only channels because of the
independent HW registers.

However, you're right that they don't necessarily need separate
channels. Both monitor the same value that channel 0 reads, so
the TEMP event spec belongs directly on channel 0.

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

Yes, both are thresholds on the same physical measurement. OT is
a higher-severity threshold that can trigger the platform
management controller to initiate a hardware shutdown sequence.

If you agree, I'd propose for v7:
  - Move TEMP threshold event spec onto channel 0 directly
  - Drop OT as a separate IIO channel, since it's a hardware
    safety mechanism better suited for the thermal framework
    as a critical trip point (planned for the follow-up
    thermal series)

Happy to take a different direction if you prefer.

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

Agreed. read_event_config currently reads the hardware IMR which
shows transient masking state during the 500ms polling window.
Will fix to return the administrative state from temp_mask instead.

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

Good point. Will investigate whether a devm_add_action to
write SYSMON_IDR is needed on unbind and add it if so.

Reviewed the remaining Sashiko findings:

  - Integer overflow in threshold conversions: for temperature,
    the Q8.7 register range is -256C to +255C, so any
    reasonable millicelsius input fits after the shift. For
    supply, val * scale can overflow int32 above ~32V, but
    supply rails on Versal are well under 4V. Extreme sysfs
    inputs are outside the hardware range.

  - I2C + IRQ panic on misconfigured DT: if an I2C node
    incorrectly specifies an interrupts property, the driver
    would register a hardirq handler on a sleeping regmap.
    The binding does not list interrupts for I2C, so this
    would be a DT authoring error.

Thanks,
Salih

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