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

["Erim, Salih" <salih.erim@amd.com>]

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

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