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

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

Hi Jonathan,

On 28/05/2026 14:01, Jonathan Cameron wrote:
> 
> 
> On Wed, 27 May 2026 12:42:10 +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
>>    - 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>
> Hi Salih
> 
> Main thing in here is related to earlier question on _PROCESSED + _RAW.
> I can't see a reason to have _RAW.

Agreed. Will drop RAW, keep PROCESSED only.

> 
> Various minor things inline. In many case they apply in other places I haven't
> called out so look for repeats
> 
> Jonathan
> 
>>   drivers/iio/adc/versal-sysmon-core.c | 655 ++++++++++++++++++++++++++-
>>   drivers/iio/adc/versal-sysmon.h      |  48 +-
>>   2 files changed, 697 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
>> index ebe052f6982..04977c9c887 100644
>> --- a/drivers/iio/adc/versal-sysmon-core.c
>> +++ b/drivers/iio/adc/versal-sysmon-core.c
>> @@ -11,6 +11,8 @@
>>   #include <linux/bitops.h>
>>   #include <linux/cleanup.h>
>>   #include <linux/device.h>
>> +#include <linux/devm-helpers.h>
>> +#include <linux/interrupt.h>
>>   #include <linux/module.h>
>>   #include <linux/property.h>
>>   #include <linux/regmap.h>
>> @@ -18,10 +20,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)
>> +
>>   /*
>>    * Both RAW and PROCESSED are exposed: RAW is needed for event thresholds
>>    * (which operate in hardware register format), PROCESSED gives userspace
>> @@ -44,6 +55,62 @@
>>        .datasheet_name = _name,                                \
>>   }
>>
>> +#define SYSMON_CHAN_TEMP_EVENT(_chan, _address, _name, _events) {\
>> +     .type = IIO_TEMP,                                       \
>> +     .indexed = 1,                                           \
> 
> Why do you needs separate channel for events?  Can't we add the event
> spec to existing channels? For the constant ones you may need to
> have two arrays to pick between depending on whether the irq is available or not.

Accepted. Will merge event specs into the static temp channels
and use two arrays (with/without events) based on has_irq.

> 
> 
>> +     .address = _address,                                    \
>> +     .channel = _chan,                                       \
>> +     .event_spec = _events,                                  \
>> +     .num_event_specs = ARRAY_SIZE(_events),                 \
>> +     .scan_type = {                                          \
>> +             .sign = 's',                                    \
>> +             .realbits = 15,                                 \
>> +             .storagebits = 16,                              \
>> +             .endianness = IIO_CPU,                          \
>> +     },                                                      \
>> +     .datasheet_name = _name,                                \
>> +}
>> +
>          *raw_data = (u16)tmp;
>> +}
> 
>> +
>> +/*
>> + * Recompute the lower threshold register from upper threshold and
>> + * cached hysteresis. Called when either upper threshold or hysteresis
>> + * is written.
>> + */
>> +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;
>> +
>> +     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);
> 
> So all the manipulation is in the units of _PROCESSED. Hence
> I'd drop _RAW.

Accepted.
> 
>> +
>> +     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;
> I'd add a blank line here.
>> +                     ret = regmap_read(sysmon->regmap, offset, &reg_val);
>> +                     if (ret)
>> +                             return ret;
> here
>> +                     sysmon_q8p7_to_millicelsius(reg_val, val);
> and here
> 
> Generally keep block of call + error check for one thing separate from code
> before and after.  Slightly nicer to read.
> 
> Lots of other places this applies but it's a very minor thing.

Accepted. Will add blank lines throughout between call+error-check
blocks.
> 
>> +                     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) {
>> +             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);
> 
> Under 80 chars on one line.  Check for cases of this and feel free to go a bit
> over if it helps readability.

Accepted.

> 
>> +                     return sysmon_update_temp_lower(sysmon, chan->address);
> 
>> +             }
>> +             if (info == IIO_EV_INFO_HYSTERESIS) {
>> +                     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);
> 
> As above.

Accepted.

> 
>> +             }
>> +     }
>> +
>> +     if (chan->type == IIO_VOLTAGE) {
>> +             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;
>> +}
> 
>> +static int sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
>> +{
>> +     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;
>> +     int ret;
>> +
>> +     switch (event) {
>> +     case SYSMON_BIT_TEMP:
>> +             sysmon_push_event(indio_dev, SYSMON_ADDR_TEMP_EVENT);
>> +             ret = regmap_write(sysmon->regmap, SYSMON_IDR,
>> +                                BIT(SYSMON_BIT_TEMP));
>> +             if (ret)
>> +                     return ret;
>> +             sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
>> +             break;
>> +
>> +     case SYSMON_BIT_OT:
>> +             sysmon_push_event(indio_dev, SYSMON_ADDR_OT_EVENT);
>> +             ret = regmap_write(sysmon->regmap, SYSMON_IDR,
>> +                                BIT(SYSMON_BIT_OT));
>> +             if (ret)
>> +                     return ret;
>> +             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:
>> +             ret = regmap_read(sysmon->regmap, alarm_flag_offset, &reg_val);
>> +             if (ret)
>> +                     return ret;
>> +             alarm_flag_reg = 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);
>> +                     ret = regmap_update_bits(sysmon->regmap,
>> +                                              alarm_reg_offset,
>> +                                              BIT(bit), 0);
>> +                     if (ret)
>> +                             return ret;
>> +             }
>> +             ret = regmap_write(sysmon->regmap, alarm_flag_offset,
>> +                                alarm_flag_reg);
>> +             if (ret)
>> +                     return ret;
>> +             break;
>                  return regmap_write();
>> +
>> +     default:
>> +             break;
> 
> Why is this not an error?

Good point. Will return -EINVAL from the default case.

> 
>> +     }
>> +
>> +     return 0;
> Might as well return early in the various paths.

Accepted. Will restructure to return from each case directly.

>> +}
> ...
> 
>> +static irqreturn_t sysmon_iio_irq(int irq, void *data)
>> +{
>> +     struct iio_dev *indio_dev = data;
>> +     struct sysmon *sysmon;
>> +     unsigned int isr, imr;
>> +
>> +     sysmon = iio_priv(indio_dev);
>> +     spin_lock(&sysmon->irq_lock);
> 
> guard() here would eman you can just return if (!isr)

Accepted.

All items will be addressed in v4.

Salih

> 
>> +
>> +     regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
>> +     regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
>> +
>> +     isr &= ~imr;
>> +     regmap_write(sysmon->regmap, SYSMON_ISR, isr);
>> +
>> +     if (isr) {
>> +             sysmon_handle_events(indio_dev, isr);
>> +             schedule_delayed_work(&sysmon->sysmon_unmask_work,
>> +                                   msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
>> +     }
>> +
>> +     spin_unlock(&sysmon->irq_lock);
>> +
>> +     return IRQ_RETVAL(isr);
>> +}