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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 21 May 2026 00:15:12 +0100
"Erim, Salih" <salih.erim@amd.com> wrote:

> 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.

For that we have a vague event with direction IIO_EV_DIR_EITHER.
Not the nicest of interfaces though as requires userspace to be
a little careful, in the case where there is shared event but
separate enables.

Anyhow, I'm happy not supporting it ;)

> 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.
Perfect.

J
> 
> Salih
> 
>