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

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

Hi Andy,

Thanks for the review, replies inline.

On 15/06/2026 15:43, Andy Shevchenko wrote:
> On Mon, Jun 15, 2026 at 12:37:21AM +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.
> 
> ...
> 
>> +static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
>> +{
>> +     *val = (raw_data * (int)MILLI) >> SYSMON_FRACTIONAL_SHIFT;
> 
> MILLIDEGREE_PER_DEGREE is defined as int.

Will use MILLIDEGREE_PER_DEGREE in both q8p7 conversion functions.

> 
>> +}
>> +
>> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
>> +{
>> +     *raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / (int)MILLI;
> 
> Ditto.
> 
>> +}
> 
> ...
> 
>> +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;
> 
> Due to agnosticism of this function, I dunno if the above can be applied here.

Agreed, this converts millivolts, not millicelsius.
MILLI with the (int) cast is correct here.

> 
>> +     if (format)
>> +             tmp = clamp(tmp, S16_MIN, S16_MAX);
>> +     else
>> +             tmp = clamp(tmp, 0, U16_MAX);
>> +
>> +     *raw_data = (u16)tmp;
>> +}
> 
> ...
> 
>> +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);
>> +     unsigned int reg_val;
>> +     int ret;
>> +
>> +     ret = regmap_read(sysmon->regmap, offset, &reg_val);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return !!(reg_val & BIT(shift));
> 
> regmap_test_bits()?

Will switch to regmap_test_bits(). This simplifies the whole
function to a single return statement.

> 
>> +}
> 
> ...
> 
>> -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)
> 
> I would leave one line (82 characters IIANM).

Will join onto one line.

Thanks,
Salih

> 
> --
> With Best Regards,
> Andy Shevchenko
> 
>