Hi Jonathan,

Thanks for the review.

On 04/05/2026 18:44, Jonathan Cameron wrote:

> 
> 
> On Sat, 2 May 2026 12:19:50 +0100
> Salih Erim <salih.erim@amd.com> 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.
>>
>> Signed-off-by: Salih Erim <salih.erim@amd.com>
> A few minor comments inline to add to what Andy found.
> 
>>   drivers/iio/adc/versal-sysmon-core.c | 539 ++++++++++++++++++++++++++-
>>   drivers/iio/adc/versal-sysmon.h      |  36 ++
>>   2 files changed, 574 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
>> index 37736c2900b..857fe21db7a 100644
>> --- a/drivers/iio/adc/versal-sysmon-core.c
>> +++ b/drivers/iio/adc/versal-sysmon-core.c
> 
>>
>> +/* OT and TEMP hysteresis bit positions in SYSMON_TEMP_EV_CFG */
>> +#define SYSMON_OT_HYST_BIT           BIT(0)
>> +#define SYSMON_TEMP_HYST_BIT         BIT(1)
> 
> You use a mix of these defines and manual shift.  Use FIELD_GET()
> /FIELD_PREP() to avoid that.

Agreed. Will use FIELD_GET()/FIELD_PREP() consistently with the
existing BIT() defines as masks.

>>
>> +#define SYSMON_CHAN_TEMP_EVENT(_chan, _address, _ext, _events) {     \
>> +     .type = IIO_TEMP,                                       \
>> +     .indexed = 1,                                           \
>> +     .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 = _ext,                                 \
> 
> As before - consider renaming this.

Will rename _ext to _name, same as P2.

> 
>> +}
> 
>> +
>> +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;
>> +     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, &reg_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;
> 
> Not a massive amount of sharing for OT_EVENT vs others. Maybe just split it
> and then you can use FIELD_GET() and get shift handling included via the mask.

Agreed, the if/else with FIELD_GET() is cleaner than the ternary.
Will apply to both read and write paths.

> 
>                          ret = regmap_read(sysmon->regmap, SYSMON_TEMP_EV_CFG,
>                                            &reg_val);
>                          if (ret)
>                                  return ret;
>                          if (chan->addres == SYSMBO_ADDR_OT_EVENT) {
>                                  *val = FIELD_GET(SYSMON_OT_HYST_BIT, reg_val);
>                          else
>                                  *val = FIELD_GET(YSMON_TEMP_HYST_BIT, reg_val);
> 
> 
> 
>> +                     *val = (reg_val & mask) >> shift;
>                          }
>> +                     shift = (chan->address == SYSMON_ADDR_OT_EVENT) ? 0 : 1;
>> +                     ret = regmap_read(sysmon->regmap, SYSMON_TEMP_EV_CFG,
>> +                                       &reg_val);
>> +                     if (ret)
>> +                             return ret;
>> +                     *val = (reg_val & mask) >> shift;
>> +                     return IIO_VAL_INT;
>> +             }
> 
>> +
>> +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;
>> +                     if (val & ~1)
> 
> Just to confirm - this only has hysteresis values of 0 or 1?  That's unusually
> small given hysteresis should be in same units as _raw.

You're right, this is wrong. The current code exposes a mode-select
bit from ALARM_CONFIG (0 = window mode, 1 = hysteresis mode), not an
actual hysteresis value.

The hardware has independent upper and lower threshold registers for
each temperature alarm (DEVICE_TEMP and OT), plus a mode bit in
ALARM_CONFIG that selects between window mode (alarm on crossing
either threshold) and hysteresis mode (upper triggers, lower clears).
Since the hardware has a single alarm bit per temperature channel,
even in window mode you can't distinguish which threshold was
crossed. Hysteresis mode maps naturally to the IIO event model.

I'll rework this as follows:
- Hard-code ALARM_CONFIG to hysteresis mode during init (both
   DEVICE_TEMP and OT)
- Expose hysteresis as a writable value in millicelsius, stored in
   the driver
- Keep both rising (upper) and falling (lower) thresholds writable
- Couple the three attributes:
     Write rising    -> recompute lower = upper - stored_hysteresis
     Write falling   -> update stored_hysteresis = upper - lower
     Write hysteresis -> recompute lower = upper - new_hysteresis
- Read hysteresis returns the stored value

This keeps full user control over both thresholds while exposing
hysteresis as a proper temperature value, matching IIO semantics.
Window mode support could be added later if needed without ABI
changes.

> 
> Also similar to above, I'd split the two cases and use FIELD_PREP()

This block will be replaced by the hysteresis rework described above.

Best regards,
Salih


> 
>> +                             return -EINVAL;
>> +                     return regmap_update_bits(sysmon->regmap,
>> +                                               SYSMON_TEMP_EV_CFG,
>> +                                               mask, val << shift);
>> +             }
>> +     } else 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;
>> +}
> 
>