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

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

Hi Jonathan,

Thanks for the detailed guidance on the ABI constraints.
I have studied a bit more and have a suggestion inline.

On 20/05/2026 10:37, Jonathan Cameron wrote:

> 
> 
>>>> +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.
>>
> Not quite because that falling attribute is not in line with the ABI.
> 
> This needs a little hammering to fit in the oddly shaped hole of our ABI.
> 
> If you are sticking to hystersis only (not window mode) then you need
> to expose it as rising threshold + hysteresis (not falling threshold as
> there is no even triggered in that direction - unless you are doing something
> nastier like triggering on both edges of the interrupt - in which case this
> is very similar to window mode and for both event directions you'd need
> to set the hysteresis to the difference between the threshold values).
> 
> If you do want to do window mode - that would be fine but you'd need
> to then do a falling event where the enable sets the hysteresis reported
> to 0.  Whether a write to hysteresis would then fail or we'd disable the
> falling direction would need some discussion - the ABI doesn't constrain
> that so it's a case of what is less likely to confuse a user.
> 
> Jonathan

Understood. I'll go with pure hysteresis mode - no falling threshold.

The hardware has a single ISR bit per temperature channel (REG_ISR
bits 8 and 9) that only tells us the temperature is "outside the
threshold(s)," with no indication of direction. In hysteresis mode
the alarm asserts when temperature exceeds the upper threshold and
clears when it drops below the lower, so the event is strictly
rising, and the lower threshold is just the re-arm point.

So the event spec becomes a single entry with everything on
IIO_EV_DIR_RISING:

     { IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING,
       ENABLE | VALUE | HYSTERESIS }

giving thresh_rising_value, thresh_rising_en and
thresh_rising_hysteresis. The pushed event will also use
IIO_EV_DIR_RISING.

Hysteresis will be stored in millicelsius and initialized from
the hardware registers at probe as (upper - lower). Writing the
rising threshold or hysteresis recomputes the lower register
internally. ALARM_CONFIG will be hard-coded to hysteresis mode
during init.

Window mode can be added later without ABI breakage by introducing
a IIO_EV_DIR_FALLING entry and enabling it would switch the hardware
mode.

Salih