Re: [PATCH 4/6] iio: adc: versal-sysmon: add over-temperature alarm events

[Jonathan Cameron]

On Mon, 2 Nov 2026 17:51:10 +0530
Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxx> wrote:

> The SysMon has two independent temperature comparators. The driver
> exposes only DEVICE_TEMP on interrupt bit 9; the over-temperature alarm
> on bit 8, with its own threshold pair at OT_TEMP_TH_LOWER and
> OT_TEMP_TH_UPPER, is not reported to userspace at all.
> 
> The two are structurally identical: an upper threshold, a lower
> threshold acting as the hysteresis floor, and a hysteresis mode bit in
> ALARM_CONFIG, bit 0 for OT and bit 1 for DEVICE_TEMP. Per AM006 the
> over-temperature shutdown response is a PMC function configured at
> design time, so the kernel's role here is threshold configuration and
> notification rather than acting on the alarm.
> 
> IIO identifies an event by channel, type and direction, and both
> comparators are rising thresholds on the same aggregate temperature, so
> they cannot share a channel. Add an event-only channel labelled "ot"
> carrying the OT thresholds.

So the reason we don't normally do multiple threshold levels is they
have minimal use for an IIO interface - as you've discovered they've
reported the same on the events interface.

If this wasn't an existing driver, the support of this is something that
would strongly push in favour of an hwmon driver where the concept of
warn and critical thresholds exists.  That's there precisely because
the critical is often wired to a pmic or similar.

However, that isn't normally a problem even in an IIO driver
because that threshold should be a design time concern with the
limits coming from firmware.   So why is this even settable by
userspace? Feels like an interface to create the bad type of smoke.

We have a few times discussed how we would perhaps add another
layer of indexing to events, but it is hard given the the compact
event codes and I'm not sure I'm seeing a usecase that justifies
that complexity here.

I also can't work out where your new channel turns up. What does
the userspace ABI look like for this and the existing event controls?

Thanks,

Jonathan


> 
> The channel is appended after the device tree derived channels because
> io-channels phandle arguments are raw array positions, and placing it
> earlier would silently repoint existing consumers. It is created only
> when the device has an interrupt, as without one it would carry no
> events.
> 
> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxx>
>
W
> @@ -880,6 +932,9 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev, int ir
>  
>  	num_static = ARRAY_SIZE(temp_channels);
>  	num_chan = size_add(num_temp, size_add(num_static, num_supply));
> +	/* The over-temperature channel exists only when events do */
> +	if (irq > 0)
> +		num_chan = size_add(num_chan, 1);
>  	sysmon_channels = devm_kcalloc(dev, num_chan, sizeof(*sysmon_channels), GFP_KERNEL);
>  	if (!sysmon_channels)
>  		return -ENOMEM;
> @@ -960,6 +1015,25 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev, int ir
>  		};
>  	}
>  
> +	/*
> +	 * Over-temperature alarm. The OT comparator watches the same
> +	 * aggregate temperature as channel 0 but against its own threshold
> +	 * pair, and IIO cannot express two rising thresholds on one channel.
> +	 * It has no reading of its own, so only event attributes and a label
> +	 * are exposed. Appended last, because io-channels phandle arguments
> +	 * are raw array positions and inserting it earlier would silently
> +	 * repoint every existing consumer.
> +	 */
> +	if (irq > 0)
> +		sysmon_channels[idx++] = (struct iio_chan_spec) {
> +			.type = IIO_TEMP,
> +			.indexed = 1,

indexed without an index? I'm not even sure what this
turns up as. There are already 4 indexed temperature channels
so why isn't this just landing on the index 0 one?

> +			.address = SYSMON_OT_TH_UP,
> +			.event_spec = sysmon_temp_events,
> +			.num_event_specs = ARRAY_SIZE(sysmon_temp_events),
> +			.datasheet_name = "ot",
> +		};
> +