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

["Erim, Salih" <salih.erim@amd.com>]

Hi Andy,

On 18/06/2026 08:09, Andy Shevchenko wrote:
> On Wed, Jun 17, 2026 at 07:01:46PM +0100, 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.
> 
> A couple of nit-picks below, otherwise
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Thank you for the review!

> 
> ...
> 
>>   #include <linux/cleanup.h>
>>   #include <linux/device.h>
>>   #include <linux/err.h>
>> +#include <linux/interrupt.h>
> 
> + limits.h // U16_MAX, et cetera

Will add.
> 
>> +#include <linux/minmax.h>
>>   #include <linux/module.h>
>>   #include <linux/overflow.h>
>>   #include <linux/property.h>
> 
> ...
> 
>> -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, idx, temp_chan_idx, volt_chan_idx;
>> -     unsigned int num_supply, num_temp;
>>        struct iio_chan_spec *sysmon_channels;
>> +     unsigned int num_supply, num_temp;
> 
> Stray change, and I would expect to see all num_* on one line, and
> all *_idx on another.
> 
>          unsigned int num_chan, num_static, num_supply, num_temp;
>          unsigned int idx, temp_chan_idx, volt_chan_idx;
>          struct iio_chan_spec *sysmon_channels;
> 
> TL;DR:
> in this patch the above should not be modified as the previous one should
> provide already a nice structure.

Accepted. Will restructure in P2 so P4 inherits cleanly.

Thanks,
Salih

> 
>>        const char *label;
>>        u32 reg;
>>        int ret;
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
>