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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sat, Jun 06, 2026 at 06:17:06AM +0100, Salih Erim wrote:
> Add threshold event support for temperature and supply voltage
> channels.
> 
> Temperature events:
>   - Rising threshold with configurable value
>   - Over-temperature (OT) alarm with separate threshold
>   - 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 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
> 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.

...

> +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_t(int, tmp, S16_MIN, S16_MAX);
> +	else
> +		tmp = clamp_t(int, tmp, 0, U16_MAX);

No, please, use clamp().

> +	*raw_data = (u16)tmp;
> +}

...

> +static int sysmon_write_alarm_config(struct sysmon *sysmon,
> +				     unsigned long address, bool enable)
> +{
> +	u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
> +	u32 offset = SYSMON_ALARM_OFFSET(address);
> +
> +	if (enable)
> +		return regmap_set_bits(sysmon->regmap, offset, BIT(shift));
> +
> +	return regmap_clear_bits(sysmon->regmap, offset, BIT(shift));

regmap_assign_bits()

> +}

...

> +	u32 alarm_reg_offset = SYSMON_ALARM_REG + (event * SYSMON_REG_STRIDE);

Unneeded parentheses.

...

> +		for_each_set_bit(bit, &alarm_flag_reg,
> +				 SYSMON_ALARM_BITS_PER_REG) {

I would leave this on a single (83 characters) line.

> +			address = bit + (SYSMON_ALARM_BITS_PER_REG * event);

Unneeded parentheses.

> +			sysmon_push_event(indio_dev, address);
> +			ret = regmap_update_bits(sysmon->regmap, alarm_reg_offset, BIT(bit), 0);

Why not _clear_bits()

> +			if (ret)
> +				return ret;
> +		}

...

> +static void sysmon_unmask_temp(struct sysmon *sysmon, unsigned int isr)
> +{
> +	unsigned int unmask, status;
> +
> +	status = isr & SYSMON_TEMP_INTR_MASK;
> +
> +	unmask = (sysmon->masked_temp ^ status) & sysmon->masked_temp;

Is this the same as

	unmask =  ~status & sysmon->masked_temp;

?

> +	sysmon->masked_temp &= status;
> +
> +	unmask &= ~sysmon->temp_mask;

The above needs a comment explaining the logic.

> +}

...

> +static void sysmon_unmask_worker(struct work_struct *work)
> +{
> +	struct sysmon *sysmon = container_of(work, struct sysmon,
> +					     sysmon_unmask_work.work);

Better to split as

	struct sysmon *sysmon =
		container_of(work, struct sysmon, sysmon_unmask_work.work);

> +	unsigned int isr;
> +
> +	spin_lock_irq(&sysmon->irq_lock);
> +	regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
> +	regmap_write(sysmon->regmap, SYSMON_ISR, isr);
> +	sysmon_unmask_temp(sysmon, isr);
> +	spin_unlock_irq(&sysmon->irq_lock);
> +
> +	if (sysmon->masked_temp)
> +		schedule_delayed_work(&sysmon->sysmon_unmask_work,
> +				      msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
> +	else
> +		regmap_write(sysmon->regmap, SYSMON_STATUS_RESET, 1);
> +}
> +
> +static irqreturn_t sysmon_iio_irq(int irq, void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int isr, imr;
> +
> +	guard(spinlock)(&sysmon->irq_lock);
> +
> +	regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
> +	regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
> +
> +	isr &= ~imr;

> +	regmap_write(sysmon->regmap, SYSMON_ISR, isr);

Is writing 0 necessary?

> +	if (!isr)
> +		return IRQ_NONE;
> +
> +	sysmon_handle_events(indio_dev, isr);
> +	schedule_delayed_work(&sysmon->sysmon_unmask_work,
> +			      msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sysmon_init_interrupt(struct sysmon *sysmon,
> +				 struct device *dev,
> +				 struct iio_dev *indio_dev,
> +				 int irq)
> +{
> +	unsigned int imr;
> +	int ret;
> +
> +	/* Events not supported without IRQ (e.g. I2C path) */
> +	if (!irq)
> +		return 0;
> +
> +	ret = devm_delayed_work_autocancel(dev, &sysmon->sysmon_unmask_work,
> +					   sysmon_unmask_worker);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
> +	if (ret)
> +		return ret;
> +	sysmon->temp_mask = imr & SYSMON_TEMP_INTR_MASK;
> +
> +	return devm_request_irq(dev, irq, sysmon_iio_irq, 0,
> +				"sysmon-irq", indio_dev);



> +}

...

> +static int sysmon_init_hysteresis(struct sysmon *sysmon, int address,

Can address be negative?

> +				  int *hysteresis)

...

> +	ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(sysmon->regmap, lower_off, &lower_reg);
> +	if (ret)
> +		return ret;

Half of the IO accessors have no error checks, these do.
Why is this inconsistency?

...

> +	if (has_irq) {
> +		temp_chans = temp_channels_with_events;
> +		num_static = ARRAY_SIZE(temp_channels_with_events);
> +	} else {
> +		temp_chans = temp_channels_no_events;
> +		num_static = ARRAY_SIZE(temp_channels_no_events);
> +	}
> +
>  	sysmon_channels = devm_kcalloc(dev,
> -				       size_add(ARRAY_SIZE(temp_channels),
> +				       size_add(num_static,
>  						num_supply + num_temp),

It makes inconsistency. Even originally. It should be two calls to size_add().

>  				       sizeof(*sysmon_channels), GFP_KERNEL);
>  	if (!sysmon_channels)
>  		return -ENOMEM;


...

> +	irq = fwnode_irq_get(dev_fwnode(dev), 0);
> +	has_irq = irq > 0;

This misses deferred probe.

...

> -	ret = sysmon_parse_fw(indio_dev, dev);
> +	ret = sysmon_parse_fw(indio_dev, dev, has_irq);

Why do we need has_irq? You can supply irq there as well and check it against 0.

>  	if (ret)
>  		return ret;
>  
> +	if (has_irq) {
> +		/* Set hysteresis mode for both temperature channels */
> +		ret = regmap_set_bits(sysmon->regmap, SYSMON_TEMP_EV_CFG,
> +				      SYSMON_OT_HYST_MASK |
> +				      SYSMON_TEMP_HYST_MASK);
> +		if (ret)
> +			return ret;
> +
> +		/* Initialize cached hysteresis from hardware registers */
> +		ret = sysmon_init_hysteresis(sysmon, SYSMON_ADDR_TEMP_EVENT,
> +					     &sysmon->temp_hysteresis);
> +		if (ret)
> +			return ret;
> +		ret = sysmon_init_hysteresis(sysmon, SYSMON_ADDR_OT_EVENT,
> +					     &sysmon->ot_hysteresis);
> +		if (ret)
> +			return ret;
> +
> +		ret = sysmon_init_interrupt(sysmon, dev, indio_dev, irq);
> +		if (ret)
> +			return ret;
> +	}

-- 
With Best Regards,
Andy Shevchenko