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

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

Hi Andy,

On 04/05/2026 11:52, Andy Shevchenko wrote:

> 
> On Sat, May 02, 2026 at 12:19:50PM +0100, Salih Erim 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.
> 
> ...
> 
>>   #include <linux/bitfield.h>
>>   #include <linux/bits.h>
>>   #include <linux/cleanup.h>
> 
>> +#include <linux/iio/events.h>
>>   #include <linux/iio/iio.h>
> 
> It's better from the start make it as a separate group of headers...

Accepted. Will group linux/iio/* headers together seperated by blank 
lines from the rest.

> 
>> +#include <linux/interrupt.h>
>> +#include <linux/limits.h>
>>   #include <linux/module.h>
>>   #include <linux/property.h>
>>   #include <linux/regmap.h>
>>
> 
> ...like here (after blank line!)
> 
> ...those linux/iio/*...
> +blank line.
> 
>>   #include "versal-sysmon.h"
> 
> Also follow IWYU.

Accepted.

> 
> ...
> 
>>   static void sysmon_q8p7_to_millicelsius(int raw_data, int *val)
>>   {
>>        *val = ((s16)raw_data * SYSMON_MILLI) >> SYSMON_FRACTIONAL_SHIFT;
>>   }
>>
>> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
>> +{
>> +     *raw_data = (u16)((val * (int)BIT(SYSMON_FRACTIONAL_SHIFT)) / SYSMON_MILLI);
>> +}
> 
> Too many explicit castings... Think how to reduce amount of them. Also this
> SYSMON_MILLI shouldn't be defined.
> 
> Besides that it's inconsistent to use right-shift in one case and BIT() in
> the other.

Accpeted. Will make both directions consistent (use right-shift or BIT 
in both) and reduce casts by using proper types in parameters.

> 
> ...
> 
>> +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) / SYSMON_MILLI;
>> +
>> +     if (format)
>> +             tmp = clamp(tmp, (int)S16_MIN, (int)S16_MAX);
>> +     else
>> +             tmp = clamp(tmp, 0, (int)U16_MAX);
> 
> No, the use of clamp() assumes it should not have explicit casts.

Will use clamp_t(int, ...) or declare typed contants to avoid the casts.

> 
>> +     *raw_data = tmp & U16_MAX;
> 
> Why?! The previous clamp() guarantees that, no?

Correct, clamp already bounds it. Will remove the mask.

> 
>> +}
> 
> ...
> 
>> +static int sysmon_read_alarm_config(struct sysmon *sysmon,
>> +                                 unsigned long address)
>> +{
>> +     u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
>> +     u32 offset = SYSMON_ALARM_OFFSET(address);
>> +     unsigned int reg_val;
>> +     int ret;
>> +
>> +     ret = regmap_read(sysmon->regmap, offset, &reg_val);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return reg_val & BIT(shift);
> 
> This won't work in case shift becomes 31. Because the returned value will be
> considered by the callers as negative error code.

Good catch. Will use !!(reg_val & BIT(shift)) to ensure the return 0 or 1.

> 
>> +}
> 
> ...
> 
>> +static int sysmon_write_alarm_config(struct sysmon *sysmon,
>> +                                  unsigned long address, u32 val)
>> +{
>> +     u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
>> +     u32 offset = SYSMON_ALARM_OFFSET(address);
>> +
>> +     return regmap_update_bits(sysmon->regmap, offset,
>> +                               BIT(shift), val << shift);
> 
> Looks like regmap_set_bits().

The val parameter is 0 or 1, so this needs to both set and clear.
Will use regmap_set_bits() when val is true and regmap_clear_bits() when 
val is false.

> 
>> +}
> 
> ...
> 
>> +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)
>> +{
>> +     u32 alarm_event_mask = sysmon_get_event_mask(chan->address);
>> +     struct sysmon *sysmon = iio_priv(indio_dev);
>> +     unsigned int imr;
>> +     int config_value;
>> +     int ret;
>> +
>> +     ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
>> +     if (ret)
>> +             return ret;
> 
> + blank line and a comment why imr is negated.

Accepted. IMR is the interrupt mask register where 1 = masked. Negating 
gives the enabled interrupts. Will add a comment.

> 
>> +     imr = ~imr;
>> +
>> +     if (chan->type == IIO_VOLTAGE) {
>> +             config_value = sysmon_read_alarm_config(sysmon, chan->address);
>> +             if (config_value < 0)
>> +                     return config_value;
>> +             return config_value && (imr & alarm_event_mask);
>> +     }
>> +
>> +     return !!(imr & alarm_event_mask);
>> +}
> 
> ...
> 
>> +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);
>> +     u32 ier = sysmon_get_event_mask(chan->address);
>> +     struct sysmon *sysmon = iio_priv(indio_dev);
>> +     unsigned int alarm_config;
>> +     int ret;
>> +
>> +     guard(mutex)(&sysmon->lock);
> 
>> +     guard(spinlock_irqsave)(&sysmon->irq_lock);
> 
> This is wrong (imagine I²C driver calling this...).

Events are only created when irq > 0, and the I2C driver passes irq = 0 
so event channels are never registered. This function cannot be reached 
from the I2C path. Will add a comment clarifying this. However if you 
prefer, can restructure to avoid spinlock in this entirely.

> 
>> +
>> +     if (chan->type == 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);
>> +             else
>> +                     return regmap_write(sysmon->regmap, SYSMON_IDR, ier);
>> +     } else if (chan->type == IIO_TEMP) {
>> +             if (state) {
>> +                     ret = regmap_write(sysmon->regmap, SYSMON_IER, ier);
>> +                     if (ret)
>> +                             return ret;
>> +                     sysmon->temp_mask &= ~ier;
>> +             } else {
>> +                     ret = regmap_write(sysmon->regmap, SYSMON_IDR, ier);
>> +                     if (ret)
>> +                             return ret;
>> +                     sysmon->temp_mask |= ier;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
> 
> ...
> 
>> +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)
> 
> Logical split, please (move int val to the next line, or int val2 to the
> previous).

Accepted.

> 
>> +{
>> +     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;
> 
> No need to calculate mask if regmap_read() fails.

Accepted. Will move mask/shift computation after the regmap_read() error 
check.

> 
>> +                     shift = (chan->address == SYSMON_ADDR_OT_EVENT) ? 0 : 1;
> 
> Ditto.
> 
>> +                     ret = regmap_read(sysmon->regmap, SYSMON_TEMP_EV_CFG,
>> +                                       &reg_val);
>> +                     if (ret)
>> +                             return ret;
>> +                     *val = (reg_val & mask) >> shift;
>> +                     return IIO_VAL_INT;
>> +             }
> 
>> +     } else if (chan->type == IIO_VOLTAGE) {
> 
> Redundant 'else'.

Accepted. Will remove.

> 
>> +             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;
>> +     }
>> +
>> +     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 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;
> 
> Similar comments as per above.

Accepted.

> 
>> +                     if (val & ~1)
> 
> BIT(0), but still magic.

Will use a named constant or add a comment. The intent is to validate 
that val is a single-bit value (0 or 1).

> 
>> +                             return -EINVAL;
>> +                     return regmap_update_bits(sysmon->regmap,
>> +                                               SYSMON_TEMP_EV_CFG,
>> +                                               mask, val << shift);
> 
> Also seems like a regmap_set_bits(). and instead of that ugly ternary just make
> it if-else or similar.

Accepted. Will use if-else with regmap_set_bits()/regmap_clear_bits() 
and remove the ternary.

> 
>> +             }
>> +     } else if (chan->type == IIO_VOLTAGE) {
> 
> Redundant 'else'.
> 
>> +             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;
>> +}
> 
> ...
> 
>> +static void sysmon_push_event(struct iio_dev *indio_dev, u32 address)
>> +{
>> +     const struct iio_chan_spec *chan;
> 
>> +     unsigned int i;
> 
> Seems not used outside of the loop, hence...

Accepted.
> 
>> +     for (i = 0; i < indio_dev->num_channels; i++) {
> 
>          for (unsigned int i = 0; i < indio_dev->num_channels; i++) {
> 
>> +             if (indio_dev->channels[i].address != address)
>> +                     continue;
>> +
>> +             chan = &indio_dev->channels[i];
>> +             iio_push_event(indio_dev,
>> +                            IIO_UNMOD_EVENT_CODE(chan->type,
>> +                                                 chan->channel,
>> +                                                 IIO_EV_TYPE_THRESH,
>> +                                                 IIO_EV_DIR_EITHER),
>> +                            iio_get_time_ns(indio_dev));
>> +     }
>> +}
> 
> ...
> 
>> +static void sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
>> +{
> 
> Why no checks for error from IO accessors in this function?

THis runs from the IRQ handler under spinlock. Will add error checks and 
bail out on failure.
> 
>> +     u32 alarm_flag_offset = SYSMON_ALARM_FLAG + (event * SYSMON_REG_STRIDE);
>> +     u32 alarm_reg_offset = SYSMON_ALARM_REG + (event * SYSMON_REG_STRIDE);
>> +     struct sysmon *sysmon = iio_priv(indio_dev);
>> +     unsigned long alarm_flag_reg;
>> +     unsigned int reg_val;
>> +     u32 address, bit;
>> +
>> +     switch (event) {
>> +     case SYSMON_BIT_TEMP:
>> +             sysmon_push_event(indio_dev, SYSMON_ADDR_TEMP_EVENT);
>> +             regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_TEMP));
>> +             sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
>> +             break;
>> +
>> +     case SYSMON_BIT_OT:
>> +             sysmon_push_event(indio_dev, SYSMON_ADDR_OT_EVENT);
>> +             regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_OT));
>> +             sysmon->masked_temp |= BIT(SYSMON_BIT_OT);
>> +             break;
>> +
>> +     case SYSMON_BIT_ALARM0:
>> +     case SYSMON_BIT_ALARM1:
>> +     case SYSMON_BIT_ALARM2:
>> +     case SYSMON_BIT_ALARM3:
>> +     case SYSMON_BIT_ALARM4:
>> +             regmap_read(sysmon->regmap, alarm_flag_offset, &reg_val);
>> +             alarm_flag_reg = (unsigned long)reg_val;
>> +
>> +             for_each_set_bit(bit, &alarm_flag_reg,
>> +                              SYSMON_ALARM_BITS_PER_REG) {
>> +                     address = bit + (SYSMON_ALARM_BITS_PER_REG * event);
>> +                     sysmon_push_event(indio_dev, address);
>> +                     regmap_update_bits(sysmon->regmap, alarm_reg_offset,
>> +                                        BIT(bit), 0);
>> +             }
>> +             regmap_write(sysmon->regmap, alarm_flag_offset, alarm_flag_reg);
>> +             break;
>> +
>> +     default:
>> +             break;
>> +     }
>> +}
> 
> ...
> 
>> +     return isr ? IRQ_HANDLED : IRQ_NONE;
> 
> There is a macro for this. IIRC IRQ_RETVAL().

Accepted. Will use IRQ_RETVAL().

> 
> ...
> 
>> +static int sysmon_init_interrupt(struct sysmon *sysmon)
>> +{
>> +     unsigned int imr;
>> +     int ret;
>> +
>> +     /* Events not supported without IRQ (e.g. I2C path) */
>> +     if (sysmon->irq <= 0)
> 
> When can '=' happen?

irq == 0 comes from the I2C backend, which passes 0 explicitly to 
sysmon_core_probe() to indicate no IRQ is available. 
platform_get_irq_optional() itself returns only positive values or 
negative error codes. Will change to if (!sysmon->irq) for clarity.

> 
>> +             return 0;
>> +
>> +     INIT_DELAYED_WORK(&sysmon->sysmon_unmask_work, sysmon_unmask_worker);
>> +
>> +     ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
>> +     if (ret)
>> +             return ret;
>> +     sysmon->temp_mask = imr & SYSMON_TEMP_INTR_MASK;
>> +
>> +     ret = devm_add_action_or_reset(sysmon->dev, sysmon_cancel_work,
>> +                                    sysmon);
>> +     if (ret)
>> +             return ret;
> 
> Don't we have include/linux/devm-helpers.h?

Yes. Will use devm_delayed_work_autocancel() instead of manual 
INIT_DELAYED_WORK + devm_add_action_or_reset.
> 
>> +     return devm_request_irq(sysmon->dev, sysmon->irq,
>> +                             sysmon_iio_irq, 0, "sysmon-irq",
>> +                             sysmon->indio_dev);
>> +}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

All items will be addressed in v3.

Thanks,
Salih.