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

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

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, &reg_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, &reg_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, &reg_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
> 
>