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

Andy Shevchenko
2026-05-04T10:52:52Z

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

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

...

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

...

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

> + *raw_data = tmp & U16_MAX;

Why?! The previous clamp() guarantees that, no?

> +}

...

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

> +}

...

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

> +}

...

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

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

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

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

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

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

> + if (val & ~1)

BIT(0), but still magic.

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

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

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

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

...

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

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

> + return devm_request_irq(sysmon->dev, sysmon->irq,
> + sysmon_iio_irq, 0, "sysmon-irq",
> + sysmon->indio_dev);
> +}

--
With Best Regards,
Andy Shevchenko