Devicetree
 help / color / mirror / Atom feed
From: "Erim, Salih" <salih.erim@amd.com>
To: jic23@kernel.org, andy@kernel.org
Cc: dlechner@baylibre.com, nuno.sa@analog.com, robh@kernel.org,
	krzk+dt@kernel.org, conor+dt@kernel.org, conall.ogriofa@amd.com,
	michal.simek@amd.com, linux@roeck-us.net, erimsalih@gmail.com,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Andy Shevchenko <andriy.shevchenko@intel.com>
Subject: Re: [PATCH v10 4/5] iio: adc: versal-sysmon: add threshold event support
Date: Tue, 23 Jun 2026 01:43:52 +0100	[thread overview]
Message-ID: <8a5fa9fc-406f-40d4-ba0e-7c9a8caff3cb@amd.com> (raw)
In-Reply-To: <20260618101414.3462934-5-salih.erim@amd.com>

Addressing Sashiko findings on this patch:

- [High] Missing bounds checking in threshold value conversion
   leads to integer overflow and hardware register corruption.

   Agreed. Will add bounds checking for both temperature and
   supply voltage threshold writes in v11.

- [High] Writing a supply voltage threshold silently clears the
   format (FMT) and mode (MODE) bits in the threshold register.

   Not a bug. The threshold registers have separate read and write
   semantics. The upper bits (FMT/MODE) are returned on read but
   only the lower 16-bit mantissa is used on write. Will add a
   comment explaining this.

- [High] The delayed worker sysmon_unmask_worker silently clears
   all pending hardware interrupts, causing concurrent voltage
   alarm events to be permanently dropped.

   The worker only runs when temperature interrupts are masked and
   only processes temperature bits. A concurrent voltage alarm
   could theoretically be cleared but the window is narrow and
   not observed in hardware testing. No change needed.

- [High] Voltage alarm handler incorrectly disables the alarm
   configuration and early returns leak ISR status.

   By design. The alarm bit is cleared to prevent re-triggering
   on level-sensitive interrupts. The event is reported to
   userspace first. Userspace re-enables via write_event_config
   if needed. The early return on regmap failure is theoretical
   as MMIO regmap writes cannot fail. No change needed.

- [High] The hardirq handler sysmon_iio_irq takes a spinlock and
   sleeps when using an I2C regmap.

   Same as P3 finding. The I2C path never registers an IRQ
   handler. Not applicable.

- [High] Race condition between interrupt configuration/teardown
   and the unmask worker.

   The teardown masks all interrupts, clears masked_temp under
   spinlock, then calls cancel_delayed_work_sync. This ordering
   prevents the race. No change needed.

- [Medium] Unconditional execution of the delayed worker resets
   hardware tracking status on voltage alarms.

   STATUS_RESET is only written when masked_temp is 0, meaning
   all temperature conditions have cleared. Intended behaviour.
   No change needed.

Thanks,
Salih

On 18/06/2026 11:14, Salih Erim wrote:
> Add threshold event support for temperature and supply voltage
> channels.
> 
> Temperature events:
>    - Rising threshold with configurable value on the device
>      temperature channel (current max across all satellites)
>    - 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 hardware also provides a separate over-temperature (OT)
> threshold, but it is not exposed through IIO as it serves as a
> hardware safety mechanism for platform shutdown. OT will be
> exposed through the thermal framework in a follow-up series.
> 
> 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
> specs are not attached 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.
> 
> A devm cleanup action masks all interrupts on driver unbind to
> prevent unhandled interrupt storms after the IRQ handler is freed.
> 
> Signed-off-by: Salih Erim <salih.erim@amd.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> ---
> Changes in v10:
>    - Add Reviewed-by tag from Andy Shevchenko
>    - Add limits.h include for U16_MAX, S16_MIN, S16_MAX (Andy)
> 
> Changes in v9:
>    - Add minmax.h include for clamp() (Andy)
>    - Join sysmon_supply_thresh_offset to one line, change address
>      parameter to unsigned long for consistency (Andy)
>    - Combine mask declaration with initialization in
>      sysmon_read_event_config (Andy)
>    - Rename ier to mask in sysmon_write_event_config for
>      consistency with sysmon_read_event_config (Andy)
>    - Remove blank line in sysmon_update_temp_lower between
>      semantically coupled lines (Andy)
>    - Rename unmask to ier (u32) in sysmon_unmask_temp (Andy)
>    - Variable name and type consistency audit across all
>      event functions (Andy)
> 
> Changes in v8:
>    - Use MILLIDEGREE_PER_DEGREE in q8p7 conversion functions (Andy)
>    - Use regmap_test_bits() in sysmon_read_alarm_config (Andy)
>    - Join sysmon_parse_fw signature onto one line (Andy)
>    - Fix devm teardown race: replace devm_delayed_work_autocancel
>      with INIT_DELAYED_WORK; fold cancel_delayed_work_sync into
>      sysmon_disable_interrupts to prevent the worker from
>      re-enabling interrupts after the IRQ handler is freed (Sashiko)
>    - Drop devm-helpers.h include (no longer needed)
> 
> Changes in v7:
>    - Move TEMP threshold event onto channel 0; drop OT as
>      separate IIO channel -- OT is a hardware safety mechanism
>      better suited for the thermal framework follow-up (Jonathan)
>    - Use single temp_channels array; attach event spec to
>      channel 0 at runtime when IRQ is available, matching the
>      pattern used for supply channels (Jonathan)
>    - Remove sysmon_temp_thresh_offset; use SYSMON_TEMP_TH_UP
>      and SYSMON_TEMP_TH_LOW defines directly at call sites
>    - Return administrative state from temp_mask in
>      read_event_config instead of transient hardware IMR
>      (Jonathan, Sashiko)
>    - Add devm_add_action_or_reset to mask all HW interrupts
>      on driver unbind (Sashiko)
>    - Remove SYSMON_CHAN_TEMP_EVENT macro, SYSMON_ADDR_TEMP_EVENT,
>      SYSMON_ADDR_OT_EVENT, SYSMON_BIT_OT, SYSMON_OT_HYST_MASK,
>      OT_TH_LOW/UP registers, ot_hysteresis from struct
>    - Simplify sysmon_get_event_mask, sysmon_update_temp_lower,
>      sysmon_init_hysteresis -- all now operate on single TEMP
>      channel only
> 
> 
> Changes in v6:
>    - Remove types.h from header (not needed at any stage) (Andy)
>    - Macro brace on separate line for SYSMON_CHAN_TEMP_EVENT (Andy)
>    - switch(chan->type) in all event functions instead of cascading
>      if statements (Andy)
>    - switch(info) in read/write_event_value for nested
>      dispatch (Andy)
>    - Reversed xmas tree in sysmon_update_temp_lower and
>      sysmon_init_hysteresis (Andy)
>    - scoped_guard(spinlock_irq) with error check in
>      sysmon_unmask_worker (Andy)
>    - Combined regmap_read error check with || in
>      sysmon_iio_irq (Andy)
>    - Join devm_request_irq on one line (Andy)
>    - Fix fwnode_irq_get() to propagate only -EPROBE_DEFER;
>      treating all negatives as fatal broke probe on I2C nodes
>      without interrupts property
> 
> Changes in v5:
>    - clamp() instead of clamp_t() (Andy)
>    - regmap_assign_bits() instead of separate set/clear (Andy)
>    - Remove unneeded parentheses (2 places) (Andy)
>    - for_each_set_bit on single line (Andy)
>    - regmap_clear_bits() instead of regmap_update_bits() (Andy)
>    - Simplify unmask XOR to ~status & masked_temp (Andy)
>    - Add comment explaining unmask &= ~temp_mask logic (Andy)
>    - Split container_of across two lines (Andy)
>    - Move ISR write after !isr check to avoid writing 0 (Andy)
>    - unsigned int for init_hysteresis address param (Andy)
>    - Add comment explaining error check policy in worker/IRQ (Andy)
>    - Nested size_add() for overflow-safe allocation (Andy)
>    - Propagate negative from fwnode_irq_get() for
>      EPROBE_DEFER (Andy)
>    - Pass irq instead of has_irq to sysmon_parse_fw (Andy)
> 
> Changes in v4:
>    - Merge event channels into static temp array; two arrays
>      (with/without events) selected by has_irq (Jonathan)
>    - Event-only channels have no info_mask; their addresses are
>      logical identifiers, not readable registers
>    - Drop RAW for voltage events, keep PROCESSED only (Jonathan)
>    - Drop scan_type from event channel macro (Jonathan)
>    - Blank lines between call+error-check blocks (Jonathan)
>    - Fit under 80 chars on one line where possible (Jonathan)
>    - default case returns -EINVAL instead of break (Jonathan)
>    - sysmon_handle_event: return early in each case (Jonathan)
>    - guard(spinlock) in sysmon_iio_irq, return IRQ_NONE/IRQ_HANDLED
>      directly (Jonathan)
>    - Take irq_lock in write_event_config for temp_mask updates to
>      synchronize with unmask worker (Sashiko)
> 
> Changes in v3:
>    - IWYU: add new includes, group iio headers with blank line (Andy)
>    - Reduce casts in millicelsius_to_q8p7, consistent style with
>      q8p7_to_millicelsius (Andy)
>    - Use clamp_t with typed constants, remove tmp & U16_MAX (Andy)
>    - Use !! to return 0/1 from read_alarm_config (Andy)
>    - Use regmap_set_bits/clear_bits in write_alarm_config (Andy)
>    - Add comment explaining spinlock is safe (I2C never reaches
>      event code path) (Andy)
>    - Add comment explaining IMR negation logic (Andy)
>    - Split read_event_value/write_event_value parameters logically
>      across lines (Andy)
>    - Move mask/shift after regmap_read error check (Andy)
>    - Remove redundant else in read_event_value and
>      write_event_value (Andy)
>    - Use named constant for hysteresis bit, if-else not ternary
>      (Andy)
>    - Loop variable declared in for() scope (Andy)
>    - Add error checks in sysmon_handle_event (Andy)
>    - Use IRQ_RETVAL() macro (Andy)
>    - Use devm_delayed_work_autocancel instead of manual INIT +
>      devm_add_action (Andy)
>    - Use FIELD_GET/FIELD_PREP for hysteresis register bits
>      (Jonathan)
>    - Split OT vs TEMP handling with FIELD_GET (Jonathan)
>    - Rework hysteresis: store as millicelsius value, hardcode
>      ALARM_CONFIG to hysteresis mode, compute lower threshold
>      from (upper - hysteresis), initialize from HW at probe
>      (Jonathan)
>    - Remove falling threshold for temperature; single event
>      spec per channel with IIO_EV_DIR_RISING (Jonathan)
>    - Push IIO_EV_DIR_RISING events for temperature,
>      IIO_EV_DIR_EITHER for voltage (Jonathan)
> 
> Changes in v2:
>    - Reverse Christmas Tree variable ordering in all functions
>    - Named constants for hysteresis bits: SYSMON_OT_HYST_BIT,
>      SYSMON_TEMP_HYST_BIT instead of magic 0x1/0x2
>    - SYSMON_ALARM_BITS_PER_REG replaces magic number 32
>    - SYSMON_ALARM_OFFSET() helper macro deduplicates alarm register
>      offset computation
>    - BIT() macro for shift expressions in conversion functions
>    - Hysteresis input validated to single-bit range (0 or 1)
>    - Event channels only created when irq > 0 (I2C safety)
>    - Group alarm interrupt stays active while any channel in the
>      group has an alarm enabled
>    - write_event_value returns -EINVAL for unhandled types
>    - IRQ_NONE returned for spurious interrupts
>    - Q8.7 write path uses multiplication instead of left-shift
>      to avoid undefined behavior with negative temperatures
>    - (u16) mask prevents garbage in reserved register bits
>    - regmap_write return values checked for IER/IDR writes
>    - devm cleanup ordering: cancel_work before request_irq
>   drivers/iio/adc/versal-sysmon-core.c | 600 ++++++++++++++++++++++++++-
>   drivers/iio/adc/versal-sysmon.h      |  36 ++
>   2 files changed, 632 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 03a745d3fb4..50b5228aa22 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
> @@ -12,6 +12,9 @@
>   #include <linux/cleanup.h>
>   #include <linux/device.h>
>   #include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/limits.h>
> +#include <linux/minmax.h>
>   #include <linux/module.h>
>   #include <linux/overflow.h>
>   #include <linux/property.h>
> @@ -20,10 +23,18 @@
>   #include <linux/sysfs.h>
>   #include <linux/units.h>
>   
> +#include <linux/iio/events.h>
>   #include <linux/iio/iio.h>
>   
>   #include "versal-sysmon.h"
>   
> +/* TEMP hysteresis mode bit in SYSMON_TEMP_EV_CFG */
> +#define SYSMON_TEMP_HYST_MASK		BIT(1)
> +
> +/* Compute alarm register offset from a channel address */
> +#define SYSMON_ALARM_OFFSET(addr) \
> +	(SYSMON_ALARM_REG + ((addr) / SYSMON_ALARM_BITS_PER_REG) * SYSMON_REG_STRIDE)
> +
>   #define SYSMON_CHAN_TEMP(_chan, _address, _name)		\
>   {								\
>   	.type = IIO_TEMP,					\
> @@ -35,6 +46,45 @@
>   	.datasheet_name = _name,				\
>   }
>   
> +enum sysmon_alarm_bit {
> +	SYSMON_BIT_ALARM0 = 0,
> +	SYSMON_BIT_ALARM1 = 1,
> +	SYSMON_BIT_ALARM2 = 2,
> +	SYSMON_BIT_ALARM3 = 3,
> +	SYSMON_BIT_ALARM4 = 4,
> +	SYSMON_BIT_TEMP = 9,
> +};
> +
> +/* Temperature event specification: rising threshold + hysteresis only */
> +static const struct iio_event_spec sysmon_temp_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> +				 BIT(IIO_EV_INFO_VALUE) |
> +				 BIT(IIO_EV_INFO_HYSTERESIS),
> +	},
> +};
> +
> +/* Supply event specifications */
> +static const struct iio_event_spec sysmon_supply_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
>   /*
>    * Static temperature channels (always present).
>    *
> @@ -52,6 +102,16 @@ static const struct iio_chan_spec temp_channels[] = {
>   	SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
>   };
>   
> +static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
> +{
> +	*val = (raw_data * MILLIDEGREE_PER_DEGREE) >> SYSMON_FRACTIONAL_SHIFT;
> +}
> +
> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
> +{
> +	*raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / MILLIDEGREE_PER_DEGREE;
> +}
> +
>   static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
>   {
>   	int mantissa, format, exponent;
> @@ -69,6 +129,33 @@ static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
>   	*val = (mantissa * (int)MILLI) >> exponent;
>   }
>   
> +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(tmp, S16_MIN, S16_MAX);
> +	else
> +		tmp = clamp(tmp, 0, U16_MAX);
> +
> +	*raw_data = (u16)tmp;
> +}
> +
> +static int sysmon_supply_thresh_offset(unsigned long address, enum iio_event_direction dir)
> +{
> +	if (dir == IIO_EV_DIR_RISING)
> +		return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_UP;
> +	if (dir == IIO_EV_DIR_FALLING)
> +		return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_LOW;
> +
> +	return -EINVAL;
> +}
> +
>   static int sysmon_read_raw(struct iio_dev *indio_dev,
>   			   struct iio_chan_spec const *chan,
>   			   int *val, int *val2, long mask)
> @@ -115,6 +202,256 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
>   	}
>   }
>   
> +static u32 sysmon_get_event_mask(const struct iio_chan_spec *chan)
> +{
> +	if (chan->type == IIO_TEMP)
> +		return BIT(SYSMON_BIT_TEMP);
> +
> +	return BIT(chan->address / SYSMON_ALARM_BITS_PER_REG);
> +}
> +
> +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);
> +
> +	return regmap_test_bits(sysmon->regmap, offset, BIT(shift));
> +}
> +
> +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);
> +
> +	return regmap_assign_bits(sysmon->regmap, offset, BIT(shift), enable);
> +}
> +
> +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)
> +{
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	u32 mask = sysmon_get_event_mask(chan);
> +	unsigned int imr;
> +	int config_value;
> +	int ret;
> +
> +	ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
> +	if (ret)
> +		return ret;
> +
> +	/* IMR bits are 1=masked, invert to get 1=enabled */
> +	imr = ~imr;
> +
> +	switch (chan->type) {
> +	case IIO_VOLTAGE:
> +		config_value = sysmon_read_alarm_config(sysmon, chan->address);
> +		if (config_value < 0)
> +			return config_value;
> +		return config_value && (imr & mask);
> +
> +	case IIO_TEMP:
> +		/*
> +		 * Return the administrative state, not the hardware IMR.
> +		 * The IRQ handler temporarily masks the interrupt during
> +		 * the polling window; reading IMR would show it as disabled.
> +		 * temp_mask bit is set when administratively disabled.
> +		 */
> +		return !(sysmon->temp_mask & mask);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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);
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	u32 mask = sysmon_get_event_mask(chan);
> +	unsigned int alarm_config;
> +	int ret;
> +
> +	guard(mutex)(&sysmon->lock);
> +
> +	switch (chan->type) {
> +	case 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, mask);
> +
> +		return regmap_write(sysmon->regmap, SYSMON_IDR, mask);
> +
> +	case IIO_TEMP:
> +		if (state) {
> +			ret = regmap_write(sysmon->regmap, SYSMON_IER, mask);
> +			if (ret)
> +				return ret;
> +
> +			scoped_guard(spinlock_irq, &sysmon->irq_lock)
> +				sysmon->temp_mask &= ~mask;
> +		} else {
> +			ret = regmap_write(sysmon->regmap, SYSMON_IDR, mask);
> +			if (ret)
> +				return ret;
> +
> +			scoped_guard(spinlock_irq, &sysmon->irq_lock)
> +				sysmon->temp_mask |= mask;
> +		}
> +		return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +/*
> + * Recompute the lower threshold register from upper threshold and
> + * cached hysteresis. Called when either upper threshold or hysteresis
> + * is written.
> + */
> +static int sysmon_update_temp_lower(struct sysmon *sysmon)
> +{
> +	unsigned int upper_reg;
> +	int upper_mc, lower_mc;
> +	u32 raw_val;
> +	int ret;
> +
> +	ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, &upper_reg);
> +	if (ret)
> +		return ret;
> +
> +	sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
> +	lower_mc = upper_mc - sysmon->temp_hysteresis;
> +	sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);
> +
> +	return regmap_write(sysmon->regmap, SYSMON_TEMP_TH_LOW, raw_val);
> +}
> +
> +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;
> +	int offset;
> +	int ret;
> +
> +	guard(mutex)(&sysmon->lock);
> +
> +	switch (chan->type) {
> +	case IIO_TEMP:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, &reg_val);
> +			if (ret)
> +				return ret;
> +
> +			sysmon_q8p7_to_millicelsius(reg_val, val);
> +
> +			return IIO_VAL_INT;
> +
> +		case IIO_EV_INFO_HYSTERESIS:
> +			*val = sysmon->temp_hysteresis;
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case 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_rawtoprocessed(reg_val, val);
> +
> +		return IIO_VAL_INT;
> +
> +	default:
> +		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 raw_val;
> +	int offset;
> +	int ret;
> +
> +	guard(mutex)(&sysmon->lock);
> +
> +	switch (chan->type) {
> +	case IIO_TEMP:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			sysmon_millicelsius_to_q8p7(&raw_val, val);
> +
> +			ret = regmap_write(sysmon->regmap, SYSMON_TEMP_TH_UP, raw_val);
> +			if (ret)
> +				return ret;
> +
> +			/* Recompute lower = upper - hysteresis */
> +			return sysmon_update_temp_lower(sysmon);
> +
> +		case IIO_EV_INFO_HYSTERESIS:
> +			if (val < 0)
> +				return -EINVAL;
> +
> +			sysmon->temp_hysteresis = val;
> +
> +			return sysmon_update_temp_lower(sysmon);
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case 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);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>   static int sysmon_read_label(struct iio_dev *indio_dev,
>   			     struct iio_chan_spec const *chan,
>   			     char *label)
> @@ -128,20 +465,242 @@ static int sysmon_read_label(struct iio_dev *indio_dev,
>   static const struct iio_info sysmon_iio_info = {
>   	.read_raw = sysmon_read_raw,
>   	.read_label = sysmon_read_label,
> +	.read_event_config = sysmon_read_event_config,
> +	.write_event_config = sysmon_write_event_config,
> +	.read_event_value = sysmon_read_event_value,
> +	.write_event_value = sysmon_write_event_value,
>   };
>   
> +static void sysmon_push_event(struct iio_dev *indio_dev, u32 address)
> +{
> +	const struct iio_chan_spec *chan;
> +	enum iio_event_direction dir;
> +
> +	for (unsigned int i = 0; i < indio_dev->num_channels; i++) {
> +		if (indio_dev->channels[i].address != address)
> +			continue;
> +
> +		chan = &indio_dev->channels[i];
> +		/* Temp uses hysteresis mode (rising only), voltage uses window */
> +		dir = (chan->type == IIO_TEMP) ? IIO_EV_DIR_RISING :
> +						 IIO_EV_DIR_EITHER;
> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(chan->type,
> +						    chan->channel,
> +						    IIO_EV_TYPE_THRESH,
> +						    dir),
> +			       iio_get_time_ns(indio_dev));
> +	}
> +}
> +
> +static int sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
> +{
> +	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;
> +	int ret;
> +
> +	switch (event) {
> +	case SYSMON_BIT_TEMP:
> +		sysmon_push_event(indio_dev, SYSMON_TEMP_MAX);
> +
> +		ret = regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_TEMP));
> +		if (ret)
> +			return ret;
> +
> +		sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
> +		return 0;
> +
> +	case SYSMON_BIT_ALARM0:
> +	case SYSMON_BIT_ALARM1:
> +	case SYSMON_BIT_ALARM2:
> +	case SYSMON_BIT_ALARM3:
> +	case SYSMON_BIT_ALARM4:
> +		ret = regmap_read(sysmon->regmap, alarm_flag_offset, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		alarm_flag_reg = 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);
> +			ret = regmap_clear_bits(sysmon->regmap, alarm_reg_offset, BIT(bit));
> +			if (ret)
> +				return ret;
> +		}
> +
> +		return regmap_write(sysmon->regmap, alarm_flag_offset, alarm_flag_reg);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static void sysmon_handle_events(struct iio_dev *indio_dev,
> +				 unsigned long events)
> +{
> +	unsigned int bit;
> +
> +	for_each_set_bit(bit, &events, SYSMON_NO_OF_EVENTS)
> +		sysmon_handle_event(indio_dev, bit);
> +}
> +
> +static void sysmon_unmask_temp(struct sysmon *sysmon, unsigned int isr)
> +{
> +	unsigned int status;
> +	u32 ier;
> +
> +	status = isr & SYSMON_TEMP_INTR_MASK;
> +
> +	ier = ~status & sysmon->masked_temp;
> +	sysmon->masked_temp &= status;
> +
> +	/* Only unmask if not administratively disabled by userspace */
> +	ier &= ~sysmon->temp_mask;
> +
> +	regmap_write(sysmon->regmap, SYSMON_IER, ier);
> +}
> +
> +/*
> + * Versal threshold interrupts are level-sensitive. Active threshold
> + * interrupts are masked in the handler and polled via delayed work
> + * until the condition clears, then unmasked.
> + */
> +static void sysmon_unmask_worker(struct work_struct *work)
> +{
> +	struct sysmon *sysmon =
> +		container_of(work, struct sysmon, sysmon_unmask_work.work);
> +	unsigned int isr;
> +
> +	/*
> +	 * If the ISR read fails, skip processing to avoid acting
> +	 * on undefined data.
> +	 */
> +	scoped_guard(spinlock_irq, &sysmon->irq_lock) {
> +		if (regmap_read(sysmon->regmap, SYSMON_ISR, &isr))
> +			break;
> +		regmap_write(sysmon->regmap, SYSMON_ISR, isr);
> +		sysmon_unmask_temp(sysmon, isr);
> +	}
> +
> +	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);
> +
> +	if (regmap_read(sysmon->regmap, SYSMON_ISR, &isr) ||
> +	    regmap_read(sysmon->regmap, SYSMON_IMR, &imr))
> +		return IRQ_NONE;
> +
> +	isr &= ~imr;
> +	if (!isr)
> +		return IRQ_NONE;
> +
> +	regmap_write(sysmon->regmap, SYSMON_ISR, isr);
> +
> +	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 void sysmon_disable_interrupts(void *data)
> +{
> +	struct sysmon *sysmon = data;
> +
> +	regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
> +
> +	scoped_guard(spinlock_irq, &sysmon->irq_lock)
> +		sysmon->masked_temp = 0;
> +
> +	cancel_delayed_work_sync(&sysmon->sysmon_unmask_work);
> +}
> +
> +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;
> +
> +	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_request_irq(dev, irq, sysmon_iio_irq, 0, "sysmon-irq", indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, sysmon_disable_interrupts, sysmon);
> +}
> +
> +/*
> + * Initialize the cached hysteresis for a temperature channel from the
> + * current hardware threshold registers: hysteresis = upper - lower.
> + */
> +static int sysmon_init_hysteresis(struct sysmon *sysmon, int *hysteresis)
> +{
> +	unsigned int upper_reg, lower_reg;
> +	int upper_mc, lower_mc;
> +	int ret;
> +
> +	ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, &upper_reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_LOW, &lower_reg);
> +	if (ret)
> +		return ret;
> +
> +	sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
> +	sysmon_q8p7_to_millicelsius(lower_reg, &lower_mc);
> +	*hysteresis = upper_mc - lower_mc;
> +
> +	return 0;
> +}
> +
>   /**
>    * sysmon_parse_fw() - Parse firmware nodes and configure IIO channels.
>    * @indio_dev: IIO device instance
>    * @dev: Parent device
> + * @irq: IRQ number (positive enables event channels, 0 disables)
>    *
>    * Reads voltage-channels and temperature-channels container nodes from
>    * firmware and builds the IIO channel array. Static temperature channels
> - * are prepended, followed by supply and satellite channels from DT.
> + * and event channels are prepended, followed by supply and satellite
> + * channels from DT.
> + *
> + * Event channels and per-channel event specs are only added when the
> + * device has an IRQ. I2C devices have no interrupt line, and the I2C
> + * regmap cannot be called from atomic context, so events are not
> + * supported on that path.
>    *
>    * Return: 0 on success, negative errno on failure.
>    */
> -static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
> +static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev, int irq)
>   {
>   	unsigned int num_chan, num_static, num_supply, num_temp;
>   	unsigned int idx, temp_chan_idx, volt_chan_idx;
> @@ -164,8 +723,14 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
>   	if (!sysmon_channels)
>   		return -ENOMEM;
>   
> -	/* Static temperature channels first */
>   	memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
> +
> +	/* Attach event spec to channel 0 when IRQ is available */
> +	if (irq > 0) {
> +		sysmon_channels[0].event_spec = sysmon_temp_events;
> +		sysmon_channels[0].num_event_specs = ARRAY_SIZE(sysmon_temp_events);
> +	}
> +
>   	idx = num_static;
>   
>   	/* Supply channels from DT */
> @@ -190,6 +755,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
>   			.indexed = 1,
>   			.address = reg,
>   			.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +			.event_spec = irq > 0 ?
> +				sysmon_supply_events : NULL,
> +			.num_event_specs = irq > 0 ?
> +				ARRAY_SIZE(sysmon_supply_events) : 0,
>   			.datasheet_name = label,
>   		};
>   	}
> @@ -255,6 +824,7 @@ int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap)
>   {
>   	struct iio_dev *indio_dev;
>   	struct sysmon *sysmon;
> +	int irq;
>   	int ret;
>   
>   	indio_dev = devm_iio_device_alloc(dev, sizeof(*sysmon));
> @@ -267,6 +837,7 @@ int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap)
>   	ret = devm_mutex_init(dev, &sysmon->lock);
>   	if (ret)
>   		return ret;
> +	spin_lock_init(&sysmon->irq_lock);
>   
>   	/* Disable all interrupts and clear pending status */
>   	ret = regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
> @@ -276,13 +847,34 @@ int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap)
>   	if (ret)
>   		return ret;
>   
> +	irq = fwnode_irq_get(dev_fwnode(dev), 0);
> +	if (irq == -EPROBE_DEFER)
> +		return dev_err_probe(dev, irq, "failed to get IRQ\n");
> +
>   	indio_dev->name = "versal-sysmon";
>   	indio_dev->modes = INDIO_DIRECT_MODE;
>   
> -	ret = sysmon_parse_fw(indio_dev, dev);
> +	ret = sysmon_parse_fw(indio_dev, dev, irq);
>   	if (ret)
>   		return ret;
>   
> +	if (irq > 0) {
> +		/* Set hysteresis mode for temperature threshold */
> +		ret = regmap_set_bits(sysmon->regmap, SYSMON_TEMP_EV_CFG,
> +				      SYSMON_TEMP_HYST_MASK);
> +		if (ret)
> +			return ret;
> +
> +		/* Initialize cached hysteresis from hardware registers */
> +		ret = sysmon_init_hysteresis(sysmon, &sysmon->temp_hysteresis);
> +		if (ret)
> +			return ret;
> +
> +		ret = sysmon_init_interrupt(sysmon, dev, indio_dev, irq);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	return devm_iio_device_register(dev, indio_dev);
>   }
>   EXPORT_SYMBOL_NS_GPL(devm_versal_sysmon_core_probe, "VERSAL_SYSMON");
> diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
> index e27a5357575..9fe2793757a 100644
> --- a/drivers/iio/adc/versal-sysmon.h
> +++ b/drivers/iio/adc/versal-sysmon.h
> @@ -11,6 +11,8 @@
>   
>   #include <linux/bits.h>
>   #include <linux/mutex.h>
> +#include <linux/spinlock_types.h>
> +#include <linux/workqueue.h>
>   
>   struct device;
>   struct regmap;
> @@ -18,12 +20,22 @@ struct regmap;
>   /* Register offsets (sorted by address) */
>   #define SYSMON_NPI_LOCK			0x000C
>   #define SYSMON_ISR			0x0044
> +#define SYSMON_IMR			0x0048
> +#define SYSMON_IER			0x004C
>   #define SYSMON_IDR			0x0050
>   #define SYSMON_TEMP_MAX			0x1030
>   #define SYSMON_TEMP_MIN			0x1034
>   #define SYSMON_SUPPLY_BASE		0x1040
> +#define SYSMON_ALARM_FLAG		0x1018
> +#define SYSMON_ALARM_REG		0x1940
> +#define SYSMON_TEMP_TH_LOW		0x1970
> +#define SYSMON_TEMP_TH_UP		0x1974
> +#define SYSMON_SUPPLY_TH_LOW		0x1980
> +#define SYSMON_SUPPLY_TH_UP		0x1C80
> +#define SYSMON_TEMP_EV_CFG		0x1F84
>   #define SYSMON_TEMP_MIN_MIN		0x1F8C
>   #define SYSMON_TEMP_MAX_MAX		0x1F90
> +#define SYSMON_STATUS_RESET		0x1F94
>   #define SYSMON_TEMP_SAT_BASE		0x1FAC
>   #define SYSMON_MAX_REG			0x24C0
>   
> @@ -35,8 +47,12 @@ struct regmap;
>   
>   #define SYSMON_SUPPLY_IDX_MAX		159
>   #define SYSMON_TEMP_SAT_MAX		64
> +#define SYSMON_NO_OF_EVENTS		32
>   #define SYSMON_INTR_ALL_MASK		GENMASK(31, 0)
>   
> +/* ISR/IMR temperature alarm mask (bit 9) */
> +#define SYSMON_TEMP_INTR_MASK		BIT(9)
> +
>   /* Supply voltage conversion register fields */
>   #define SYSMON_MANTISSA_MASK		GENMASK(15, 0)
>   #define SYSMON_FMT_MASK			BIT(16)
> @@ -46,11 +62,21 @@ struct regmap;
>   #define SYSMON_FRACTIONAL_SHIFT		7U
>   #define SYSMON_SUPPLY_MANTISSA_BITS	16
>   
> +/* Bits per alarm register */
> +#define SYSMON_ALARM_BITS_PER_REG	32
> +
> +#define SYSMON_UNMASK_WORK_DELAY_MS	500
> +
>   /**
>    * struct sysmon - Driver data for Versal SysMon
>    * @regmap: register map for hardware access
>    * @lock: protects read-modify-write sequences on threshold registers
>    *        and cached state that spans multiple regmap calls
> + * @irq_lock: protects interrupt mask register updates (MMIO path only)
> + * @masked_temp: currently masked temperature alarm bits
> + * @temp_mask: temperature interrupt configuration mask
> + * @temp_hysteresis: cached DEVICE_TEMP hysteresis in millicelsius
> + * @sysmon_unmask_work: re-enables events after alarm condition clears
>    */
>   struct sysmon {
>   	struct regmap *regmap;
> @@ -60,6 +86,16 @@ struct sysmon {
>   	 * that spans multiple regmap calls.
>   	 */
>   	struct mutex lock;
> +	/*
> +	 * Protects interrupt mask register updates.  Only used on the
> +	 * MMIO path (fast_io regmap); I2C has no IRQ and never reaches
> +	 * the event code that takes this lock.
> +	 */
> +	spinlock_t irq_lock;
> +	unsigned int masked_temp;
> +	unsigned int temp_mask;
> +	int temp_hysteresis;
> +	struct delayed_work sysmon_unmask_work;
>   };
>   
>   int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap);


  parent reply	other threads:[~2026-06-23  0:44 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-18 10:14 [PATCH v10 0/5] iio: adc: add Versal SysMon driver Salih Erim
2026-06-18 10:14 ` [PATCH v10 1/5] dt-bindings: iio: adc: add xlnx,versal-sysmon binding Salih Erim
2026-06-18 10:14 ` [PATCH v10 2/5] iio: adc: add Versal SysMon driver Salih Erim
2026-06-18 10:14 ` [PATCH v10 3/5] iio: adc: versal-sysmon: add I2C driver Salih Erim
2026-06-18 10:28   ` sashiko-bot
2026-06-23  0:26   ` Erim, Salih
2026-06-18 10:14 ` [PATCH v10 4/5] iio: adc: versal-sysmon: add threshold event support Salih Erim
2026-06-18 10:28   ` sashiko-bot
2026-06-21 16:22   ` Jonathan Cameron
2026-06-22 23:59     ` Erim, Salih
2026-06-23  0:43   ` Erim, Salih [this message]
2026-06-18 10:14 ` [PATCH v10 5/5] iio: adc: versal-sysmon: add oversampling support Salih Erim
2026-06-18 10:25   ` sashiko-bot
2026-06-21 16:28   ` Jonathan Cameron
2026-06-23  0:06     ` Erim, Salih
2026-06-23  0:34   ` Erim, Salih

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8a5fa9fc-406f-40d4-ba0e-7c9a8caff3cb@amd.com \
    --to=salih.erim@amd.com \
    --cc=andriy.shevchenko@intel.com \
    --cc=andy@kernel.org \
    --cc=conall.ogriofa@amd.com \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dlechner@baylibre.com \
    --cc=erimsalih@gmail.com \
    --cc=jic23@kernel.org \
    --cc=krzk+dt@kernel.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=michal.simek@amd.com \
    --cc=nuno.sa@analog.com \
    --cc=robh@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox