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

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

Hi Andy,

All accepted, all will be fixed in v9.
On 17/06/2026 16:38, Andy Shevchenko wrote:
> On Tue, Jun 16, 2026 at 02:15:58PM +0100, Salih Erim 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.
> 
> ...
> 
>> +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;
>> +
>> +     if (format)
>> +             tmp = clamp(tmp, S16_MIN, S16_MAX);
>> +     else
>> +             tmp = clamp(tmp, 0, U16_MAX);
> 
> Double check that minmax.h is included.

Accepted. Will add <linux/minmax.h>

> 
>> +     *raw_data = (u16)tmp;
>> +}
> 
> ...
> 
>> +static int sysmon_supply_thresh_offset(int address,
>> +                                    enum iio_event_direction dir)
> 
> Make it a single line. OTOH why is 'address' signed? Perhaps u32?
> Or for some reason unsigned long as per _alarm_config()?

Accepted. Will Join to one line. Changed to unsigned long for 
consistency with sysmon_read_alarm_config() and sysmon_write_alarm_config().

> 
>> +{
>> +     if (dir == IIO_EV_DIR_RISING)
>> +             return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_UP;
>> +     if (dir == IIO_EV_DIR_FALLING)
>> +             return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_LOW;
>> +
>> +     return -EINVAL;
>> +}
> 
> ...
> 
>> +static int sysmon_read_event_config(struct iio_dev *indio_dev,
>> +                                 const struct iio_chan_spec *chan,
>> +                                 enum iio_event_type type,
>> +                                 enum iio_event_direction dir)
>> +{
>> +     struct sysmon *sysmon = iio_priv(indio_dev);
>> +     unsigned int imr;
>> +     int config_value;
> 
>> +     u32 mask;
>> +     int ret;
>> +
>> +     mask = sysmon_get_event_mask(chan);
> 
> Just make it together, as we don't validate the value of 'mask'.

Accepted.
> 
>          struct sysmon *sysmon = iio_priv(indio_dev);
>          u32 mask = sysmon_get_event_mask(chan);
>          ...
>          int ret;
> 
>> +     ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* IMR bits are 1=masked, invert to get 1=enabled */
>> +     imr = ~imr;
>> +
>> +     switch (chan->type) {
>> +     case IIO_VOLTAGE:
>> +             config_value = sysmon_read_alarm_config(sysmon, chan->address);
>> +             if (config_value < 0)
>> +                     return config_value;
>> +             return config_value && (imr & mask);
>> +
>> +     case IIO_TEMP:
>> +             /*
>> +              * Return the administrative state, not the hardware IMR.
>> +              * The IRQ handler temporarily masks the interrupt during
>> +              * the polling window; reading IMR would show it as disabled.
>> +              * temp_mask bit is set when administratively disabled.
>> +              */
>> +             return !(sysmon->temp_mask & mask);
>> +
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +}
> 
> ...
> 
>> +static int sysmon_write_event_config(struct iio_dev *indio_dev,
>> +                                  const struct iio_chan_spec *chan,
>> +                                  enum iio_event_type type,
>> +                                  enum iio_event_direction dir,
>> +                                  bool state)
>> +{
>> +     u32 offset = SYSMON_ALARM_OFFSET(chan->address);
>> +     struct sysmon *sysmon = iio_priv(indio_dev);
>> +     u32 ier = sysmon_get_event_mask(chan);
> 
> Here you call the variable 'ier'. Please, make this consistent in the related
> APIs (see above).

Accepted. Will rename to 'mask' in write_event_config to match 
read_event_config

> 
>> +     unsigned int alarm_config;
>> +     int ret;
>> +
>> +     guard(mutex)(&sysmon->lock);
>> +
>> +     switch (chan->type) {
>> +     case IIO_VOLTAGE:
>> +             ret = sysmon_write_alarm_config(sysmon, chan->address, state);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             ret = regmap_read(sysmon->regmap, offset, &alarm_config);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             if (alarm_config)
>> +                     return regmap_write(sysmon->regmap, SYSMON_IER, ier);
>> +
>> +             return regmap_write(sysmon->regmap, SYSMON_IDR, ier);
>> +
>> +     case IIO_TEMP:
>> +             if (state) {
>> +                     ret = regmap_write(sysmon->regmap, SYSMON_IER, ier);
>> +                     if (ret)
>> +                             return ret;
>> +
>> +                     scoped_guard(spinlock_irq, &sysmon->irq_lock)
>> +                             sysmon->temp_mask &= ~ier;
>> +             } else {
>> +                     ret = regmap_write(sysmon->regmap, SYSMON_IDR, ier);
>> +                     if (ret)
>> +                             return ret;
>> +
>> +                     scoped_guard(spinlock_irq, &sysmon->irq_lock)
>> +                             sysmon->temp_mask |= ier;
>> +             }
>> +             return 0;
>> +
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +}
> 
> ...
> 
>> +static int sysmon_update_temp_lower(struct sysmon *sysmon)
>> +{
>> +     unsigned int upper_reg;
>> +     int upper_mc, lower_mc;
>> +     u32 raw_val;
>> +     int ret;
>> +
>> +     ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, &upper_reg);
>> +     if (ret)
>> +             return ret;
>> +
>> +     sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
> 
>> +
> 
> ^^^
> 
>> +     lower_mc = upper_mc - sysmon->temp_hysteresis;
> 
> Either add a blank line here, or remove the one above as these three is kinda
> semantically coupled.

Accepted. Will removed the blank line, the three lines are a single 
conversion sequence.
> 
>> +     sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);
>> +
>> +     return regmap_write(sysmon->regmap, SYSMON_TEMP_TH_LOW, raw_val);
>> +}
> 
> ...
> 
>> +static void sysmon_unmask_temp(struct sysmon *sysmon, unsigned int isr)
>> +{
>> +     unsigned int unmask, status;
> 
> As per above perhaps name 'unmask' as 'u32 ier'? Or did I miss the use case?

Accepted. Will rename to 'u32 ier', this is the value written to the IER 
register, so the name fits.

> 
>> +     status = isr & SYSMON_TEMP_INTR_MASK;
>> +
>> +     unmask = ~status & sysmon->masked_temp;
>> +     sysmon->masked_temp &= status;
>> +
>> +     /* Only unmask if not administratively disabled by userspace */
>> +     unmask &= ~sysmon->temp_mask;
>> +
>> +     regmap_write(sysmon->regmap, SYSMON_IER, unmask);
>> +}
> 
> Also looking at all this, please double check variable names in all functions
> and make types and names consistent across the whole driver code.

Accepted. Will audited. To be consistent: 'mask' for the event interrupt 
bitmask in read/write_event_config, 'ier' for values written to the IER
register in sysmon_unmask_temp.
> 
>> +     }
> 
> ...
> 
>> -     num_chan = size_add(num_temp, size_add(ARRAY_SIZE(temp_channels), num_supply));
>> +     num_static = ARRAY_SIZE(temp_channels);
>> +     num_chan = size_add(num_temp, size_add(num_static, num_supply));
> 
> At glance I don't see any additional arguments, can we introduce num_static in
> the previous patch to reduce a churn here?

Accepted. Will move num_static introduction to P2.

Thanks,
Salih
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
>