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

[Erim, Salih]

Hi Jonathan,

Thank you for the review.

On 21/06/2026 17:22, Jonathan Cameron wrote:
> On Thu, 18 Jun 2026 11:14:13 +0100
> Salih Erim <salih.erim@xxxxxxx> wrote:
>
> > Add threshold event support for temperature and supply voltage
> > channels.
> >
> > Temperature events:
> >    - Rising threshold with configurable value on the device
> >      temperature channel (current max across all satellites)
> >    - 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 hardware also provides a separate over-temperature (OT)
> > threshold, but it is not exposed through IIO as it serves as a
> > hardware safety mechanism for platform shutdown. OT will be
> > exposed through the thermal framework in a follow-up series.
> >
> > 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
> > specs are not attached 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.
> >
> > A devm cleanup action masks all interrupts on driver unbind to
> > prevent unhandled interrupt storms after the IRQ handler is freed.
> >
> > Signed-off-by: Salih Erim <salih.erim@xxxxxxx>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
>
> There is some stuff from Sashiko that looks plausible.
> https://sashiko.dev/#/patchset/20260618101414.3462934-1-salih.erim%40amd.com
>
> Whilst the out of range temp thresholds might not be a bug
> that causes anything particular bad to happen it would be nice
> to report an error to userspace if a write is for something we
> can't support.
>
> There are some things that I can't figure out without data sheet
> diving so I'll leave those for you to sanity check + some I think
> we addressed in earlier discussions.
> For a few of the things it raises I talk about them inline.

Thanks for the guidance on handling Sashiko findings. I have
reviewed all of them and will reply on-list with my assessment.

> Note I didn't spot anything else (and probably wouldn't have
> spotted these :(
>
> Jonathan
>
>
> > ---
> > Changes in v10:
> >    - Add Reviewed-by tag from Andy Shevchenko
> >    - Add limits.h include for U16_MAX, S16_MIN, S16_MAX (Andy)
> >
> > Changes in v9:
> >    - Add minmax.h include for clamp() (Andy)
> >    - Join sysmon_supply_thresh_offset to one line, change address
> >      parameter to unsigned long for consistency (Andy)
> >    - Combine mask declaration with initialization in
> >      sysmon_read_event_config (Andy)
> >    - Rename ier to mask in sysmon_write_event_config for
> >      consistency with sysmon_read_event_config (Andy)
> >    - Remove blank line in sysmon_update_temp_lower between
> >      semantically coupled lines (Andy)
> >    - Rename unmask to ier (u32) in sysmon_unmask_temp (Andy)
> >    - Variable name and type consistency audit across all
> >      event functions (Andy)
> >
> > Changes in v8:
> >    - Use MILLIDEGREE_PER_DEGREE in q8p7 conversion functions (Andy)
> >    - Use regmap_test_bits() in sysmon_read_alarm_config (Andy)
> >    - Join sysmon_parse_fw signature onto one line (Andy)
> >    - Fix devm teardown race: replace devm_delayed_work_autocancel
> >      with INIT_DELAYED_WORK; fold cancel_delayed_work_sync into
> >      sysmon_disable_interrupts to prevent the worker from
> >      re-enabling interrupts after the IRQ handler is freed (Sashiko)
> >    - Drop devm-helpers.h include (no longer needed)
> >
> > Changes in v7:
> >    - Move TEMP threshold event onto channel 0; drop OT as
> >      separate IIO channel -- OT is a hardware safety mechanism
> >      better suited for the thermal framework follow-up (Jonathan)
> >    - Use single temp_channels array; attach event spec to
> >      channel 0 at runtime when IRQ is available, matching the
> >      pattern used for supply channels (Jonathan)
> >    - Remove sysmon_temp_thresh_offset; use SYSMON_TEMP_TH_UP
> >      and SYSMON_TEMP_TH_LOW defines directly at call sites
> >    - Return administrative state from temp_mask in
> >      read_event_config instead of transient hardware IMR
> >      (Jonathan, Sashiko)
> >    - Add devm_add_action_or_reset to mask all HW interrupts
> >      on driver unbind (Sashiko)
> >    - Remove SYSMON_CHAN_TEMP_EVENT macro, SYSMON_ADDR_TEMP_EVENT,
> >      SYSMON_ADDR_OT_EVENT, SYSMON_BIT_OT, SYSMON_OT_HYST_MASK,
> >      OT_TH_LOW/UP registers, ot_hysteresis from struct
> >    - Simplify sysmon_get_event_mask, sysmon_update_temp_lower,
> >      sysmon_init_hysteresis -- all now operate on single TEMP
> >      channel only
> >
> >
> > Changes in v6:
> >    - Remove types.h from header (not needed at any stage) (Andy)
> >    - Macro brace on separate line for SYSMON_CHAN_TEMP_EVENT (Andy)
> >    - switch(chan->type) in all event functions instead of cascading
> >      if statements (Andy)
> >    - switch(info) in read/write_event_value for nested
> >      dispatch (Andy)
> >    - Reversed xmas tree in sysmon_update_temp_lower and
> >      sysmon_init_hysteresis (Andy)
> >    - scoped_guard(spinlock_irq) with error check in
> >      sysmon_unmask_worker (Andy)
> >    - Combined regmap_read error check with || in
> >      sysmon_iio_irq (Andy)
> >    - Join devm_request_irq on one line (Andy)
> >    - Fix fwnode_irq_get() to propagate only -EPROBE_DEFER;
> >      treating all negatives as fatal broke probe on I2C nodes
> >      without interrupts property
> >
> > Changes in v5:
> >    - clamp() instead of clamp_t() (Andy)
> >    - regmap_assign_bits() instead of separate set/clear (Andy)
> >    - Remove unneeded parentheses (2 places) (Andy)
> >    - for_each_set_bit on single line (Andy)
> >    - regmap_clear_bits() instead of regmap_update_bits() (Andy)
> >    - Simplify unmask XOR to ~status & masked_temp (Andy)
> >    - Add comment explaining unmask &= ~temp_mask logic (Andy)
> >    - Split container_of across two lines (Andy)
> >    - Move ISR write after !isr check to avoid writing 0 (Andy)
> >    - unsigned int for init_hysteresis address param (Andy)
> >    - Add comment explaining error check policy in worker/IRQ (Andy)
> >    - Nested size_add() for overflow-safe allocation (Andy)
> >    - Propagate negative from fwnode_irq_get() for
> >      EPROBE_DEFER (Andy)
> >    - Pass irq instead of has_irq to sysmon_parse_fw (Andy)
> >
> > Changes in v4:
> >    - Merge event channels into static temp array; two arrays
> >      (with/without events) selected by has_irq (Jonathan)
> >    - Event-only channels have no info_mask; their addresses are
> >      logical identifiers, not readable registers
> >    - Drop RAW for voltage events, keep PROCESSED only (Jonathan)
> >    - Drop scan_type from event channel macro (Jonathan)
> >    - Blank lines between call+error-check blocks (Jonathan)
> >    - Fit under 80 chars on one line where possible (Jonathan)
> >    - default case returns -EINVAL instead of break (Jonathan)
> >    - sysmon_handle_event: return early in each case (Jonathan)
> >    - guard(spinlock) in sysmon_iio_irq, return IRQ_NONE/IRQ_HANDLED
> >      directly (Jonathan)
> >    - Take irq_lock in write_event_config for temp_mask updates to
> >      synchronize with unmask worker (Sashiko)
> >
> > Changes in v3:
> >    - IWYU: add new includes, group iio headers with blank line (Andy)
> >    - Reduce casts in millicelsius_to_q8p7, consistent style with
> >      q8p7_to_millicelsius (Andy)
> >    - Use clamp_t with typed constants, remove tmp & U16_MAX (Andy)
> >    - Use !! to return 0/1 from read_alarm_config (Andy)
> >    - Use regmap_set_bits/clear_bits in write_alarm_config (Andy)
> >    - Add comment explaining spinlock is safe (I2C never reaches
> >      event code path) (Andy)
> >    - Add comment explaining IMR negation logic (Andy)
> >    - Split read_event_value/write_event_value parameters logically
> >      across lines (Andy)
> >    - Move mask/shift after regmap_read error check (Andy)
> >    - Remove redundant else in read_event_value and
> >      write_event_value (Andy)
> >    - Use named constant for hysteresis bit, if-else not ternary
> >      (Andy)
> >    - Loop variable declared in for() scope (Andy)
> >    - Add error checks in sysmon_handle_event (Andy)
> >    - Use IRQ_RETVAL() macro (Andy)
> >    - Use devm_delayed_work_autocancel instead of manual INIT +
> >      devm_add_action (Andy)
> >    - Use FIELD_GET/FIELD_PREP for hysteresis register bits
> >      (Jonathan)
> >    - Split OT vs TEMP handling with FIELD_GET (Jonathan)
> >    - Rework hysteresis: store as millicelsius value, hardcode
> >      ALARM_CONFIG to hysteresis mode, compute lower threshold
> >      from (upper - hysteresis), initialize from HW at probe
> >      (Jonathan)
> >    - Remove falling threshold for temperature; single event
> >      spec per channel with IIO_EV_DIR_RISING (Jonathan)
> >    - Push IIO_EV_DIR_RISING events for temperature,
> >      IIO_EV_DIR_EITHER for voltage (Jonathan)
> >
> > Changes in v2:
> >    - Reverse Christmas Tree variable ordering in all functions
> >    - Named constants for hysteresis bits: SYSMON_OT_HYST_BIT,
> >      SYSMON_TEMP_HYST_BIT instead of magic 0x1/0x2
> >    - SYSMON_ALARM_BITS_PER_REG replaces magic number 32
> >    - SYSMON_ALARM_OFFSET() helper macro deduplicates alarm register
> >      offset computation
> >    - BIT() macro for shift expressions in conversion functions
> >    - Hysteresis input validated to single-bit range (0 or 1)
> >    - Event channels only created when irq > 0 (I2C safety)
> >    - Group alarm interrupt stays active while any channel in the
> >      group has an alarm enabled
> >    - write_event_value returns -EINVAL for unhandled types
> >    - IRQ_NONE returned for spurious interrupts
> >    - Q8.7 write path uses multiplication instead of left-shift
> >      to avoid undefined behavior with negative temperatures
> >    - (u16) mask prevents garbage in reserved register bits
> >    - regmap_write return values checked for IER/IDR writes
> >    - devm cleanup ordering: cancel_work before request_irq
> >   drivers/iio/adc/versal-sysmon-core.c | 600 ++++++++++++++++++++++++++-
> >   drivers/iio/adc/versal-sysmon.h      |  36 ++
> >   2 files changed, 632 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> > index 03a745d3fb4..50b5228aa22 100644
> > --- a/drivers/iio/adc/versal-sysmon-core.c
> > +++ b/drivers/iio/adc/versal-sysmon-core.c
>
>
> >   /*
> >    * Static temperature channels (always present).
> >    *
> > @@ -52,6 +102,16 @@ static const struct iio_chan_spec temp_channels[] = {
> >        SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
> >   };
> >
> > +static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
> > +{
> > +     *val = (raw_data * MILLIDEGREE_PER_DEGREE) >> SYSMON_FRACTIONAL_SHIFT;
> > +}
> > +
> > +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
> > +{
> > +     *raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / MILLIDEGREE_PER_DEGREE;
> > +}
> > +
> >   static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
> >   {
> >        int mantissa, format, exponent;
> > @@ -69,6 +129,33 @@ static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
> >        *val = (mantissa * (int)MILLI) >> exponent;
> >   }
> >
> > +static void sysmon_supply_processedtoraw(int val, u32 reg_val, u32 *raw_data)
> > +{
> > +     int exponent = FIELD_GET(SYSMON_MODE_MASK, reg_val);
> > +     int format = FIELD_GET(SYSMON_FMT_MASK, reg_val);
> > +     int scale, tmp;
> > +
> > +     scale = BIT(SYSMON_SUPPLY_MANTISSA_BITS - exponent);
> > +     tmp = (val * scale) / (int)MILLI;
>
> See below. Overflow issue is if val is large enough that this overflows
> before tmp is clamped, possibly giving unexpected values.

Agreed. Will add input validation to prevent the overflow
before the clamp.

> > +
> > +     if (format)
> > +             tmp = clamp(tmp, S16_MIN, S16_MAX);
> > +     else
> > +             tmp = clamp(tmp, 0, U16_MAX);
> > +
> > +     *raw_data = (u16)tmp;
> > +}
>
> ...
>
> > +
> > +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);
> > +
> > +     switch (chan->type) {
> > +     case IIO_TEMP:
> > +             switch (info) {
> > +             case IIO_EV_INFO_VALUE:
> > +                     ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, &reg_val);
> > +                     if (ret)
> > +                             return ret;
> > +
> > +                     sysmon_q8p7_to_millicelsius(reg_val, val);
> > +
> > +                     return IIO_VAL_INT;
> > +
> > +             case IIO_EV_INFO_HYSTERESIS:
> > +                     *val = sysmon->temp_hysteresis;
> > +                     return IIO_VAL_INT;
> > +
> > +             default:
> > +                     return -EINVAL;
> > +             }
> > +
> > +     case 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;
> > +
> > +     default:
> > +             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);
> > +
> > +     switch (chan->type) {
> > +     case IIO_TEMP:
> > +             switch (info) {
> > +             case IIO_EV_INFO_VALUE:
> > +                     sysmon_millicelsius_to_q8p7(&raw_val, val);
> In this path, sashiko is asking whether it is possible for val to be sufficiently large
> or negative that the calculation is going to given rather unexpected results.
>
> Given in the read direction you assume it is suitable for passing in a U16, should we
> have a check here? + error out if it is out of range?

Agreed. Will add a bounds check on val before the Q8.7 conversion
and return -EINVAL if out of the representable range.

> > +
> > +                     ret = regmap_write(sysmon->regmap, SYSMON_TEMP_TH_UP, raw_val);
> > +                     if (ret)
> > +                             return ret;
> > +
> > +                     /* Recompute lower = upper - hysteresis */
> > +                     return sysmon_update_temp_lower(sysmon);
> > +
> > +             case IIO_EV_INFO_HYSTERESIS:
> > +                     if (val < 0)
> > +                             return -EINVAL;
> > +
> > +                     sysmon->temp_hysteresis = val;
> > +
> > +                     return sysmon_update_temp_lower(sysmon);
> > +
> > +             default:
> > +                     return -EINVAL;
> > +             }
> > +
> > +     case 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);
>
> There is another sashiko report about potential out of range val
> here. Probably also want to check for that just as a way to improve
> useability.

Will add a bounds check before calling sysmon_supply_processedtoraw()
as well.

> > +
> > +             return regmap_write(sysmon->regmap, offset, raw_val);
> There is also a comment on whether this is wiping out the fields in the
> upper bits of the register. If it isn't (maybe they are read only?)
> then a comment here would be good.

The threshold registers have separate read and write semantics,
the upper bits (FMT/MODE) are returned on read but ignored on
write; only the lower 16-bit mantissa is used. Will add a comment
to make this clear.

Regards,
Salih
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}