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

[Jonathan Cameron <jic23@kernel.org>]

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