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

Jonathan Cameron
2026-05-04T17:44:26Z

On Sat, 2 May 2026 12:19:50 +0100
Salih Erim <salih.erim@amd.com> 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.
>
> Signed-off-by: Salih Erim <salih.erim@amd.com>
A few minor comments inline to add to what Andy found.

> drivers/iio/adc/versal-sysmon-core.c | 539 ++++++++++++++++++++++++++-
> drivers/iio/adc/versal-sysmon.h | 36 ++
> 2 files changed, 574 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 37736c2900b..857fe21db7a 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c

>
> +/* OT and TEMP hysteresis bit positions in SYSMON_TEMP_EV_CFG */
> +#define SYSMON_OT_HYST_BIT BIT(0)
> +#define SYSMON_TEMP_HYST_BIT BIT(1)

You use a mix of these defines and manual shift. Use FIELD_GET()
/FIELD_PREP() to avoid that.
>
> +#define SYSMON_CHAN_TEMP_EVENT(_chan, _address, _ext, _events) { \
> + .type = IIO_TEMP, \
> + .indexed = 1, \
> + .address = _address, \
> + .channel = _chan, \
> + .event_spec = _events, \
> + .num_event_specs = ARRAY_SIZE(_events), \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 15, \
> + .storagebits = 16, \
> + .endianness = IIO_CPU, \
> + }, \
> + .datasheet_name = _ext, \

As before - consider renaming this.

> +}

> +
> +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;
> + 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;

Not a massive amount of sharing for OT_EVENT vs others. Maybe just split it
and then you can use FIELD_GET() and get shift handling included via the mask.

ret = regmap_read(sysmon->regmap, SYSMON_TEMP_EV_CFG,
&reg_val);
if (ret)
return ret;
if (chan->addres == SYSMBO_ADDR_OT_EVENT) {
*val = FIELD_GET(SYSMON_OT_HYST_BIT, reg_val);
else
*val = FIELD_GET(YSMON_TEMP_HYST_BIT, reg_val);

> + *val = (reg_val & mask) >> shift;
}
> + shift = (chan->address == SYSMON_ADDR_OT_EVENT) ? 0 : 1;
> + ret = regmap_read(sysmon->regmap, SYSMON_TEMP_EV_CFG,
> + &reg_val);
> + if (ret)
> + return ret;
> + *val = (reg_val & mask) >> shift;
> + return IIO_VAL_INT;
> + }

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

Also similar to above, I'd split the two cases and use FIELD_PREP()

> + return -EINVAL;
> + return regmap_update_bits(sysmon->regmap,
> + SYSMON_TEMP_EV_CFG,
> + mask, val << shift);
> + }
> + } else if (chan->type == 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);
> + return regmap_write(sysmon->regmap, offset, raw_val);
> + }
> +
> + return -EINVAL;
> +}