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

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

Hi Andy,

Thanks, replies are inline.
On 07/06/2026 08:31, Andy Shevchenko wrote:
> 
> On Sat, Jun 06, 2026 at 06:17:06AM +0100, Salih Erim wrote:
>> Add threshold event support for temperature and supply voltage
>> channels.
>>
>> Temperature events:
>>    - Rising threshold with configurable value
>>    - Over-temperature (OT) alarm with separate threshold
>>    - 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 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
>> 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.
> 
> ...
> 
>> +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_t(int, tmp, S16_MIN, S16_MAX);
>> +     else
>> +             tmp = clamp_t(int, tmp, 0, U16_MAX);
> 
> No, please, use clamp().

Accepted.

> 
>> +     *raw_data = (u16)tmp;
>> +}
> 
> ...
> 
>> +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);
>> +
>> +     if (enable)
>> +             return regmap_set_bits(sysmon->regmap, offset, BIT(shift));
>> +
>> +     return regmap_clear_bits(sysmon->regmap, offset, BIT(shift));
> 
> regmap_assign_bits()

Accepted.
> 
>> +}
> 
> ...
> 
>> +     u32 alarm_reg_offset = SYSMON_ALARM_REG + (event * SYSMON_REG_STRIDE);
> 
> Unneeded parentheses.

Accepted. Will remove in both places.
> 
> ...
> 
>> +             for_each_set_bit(bit, &alarm_flag_reg,
>> +                              SYSMON_ALARM_BITS_PER_REG) {
> 
> I would leave this on a single (83 characters) line.

Accepted.

> 
>> +                     address = bit + (SYSMON_ALARM_BITS_PER_REG * event);
> 
> Unneeded parentheses.

Accepted.

> 
>> +                     sysmon_push_event(indio_dev, address);
>> +                     ret = regmap_update_bits(sysmon->regmap, alarm_reg_offset, BIT(bit), 0);
> 
> Why not _clear_bits()

Accepted.

> 
>> +                     if (ret)
>> +                             return ret;
>> +             }
> 
> ...
> 
>> +static void sysmon_unmask_temp(struct sysmon *sysmon, unsigned int isr)
>> +{
>> +     unsigned int unmask, status;
>> +
>> +     status = isr & SYSMON_TEMP_INTR_MASK;
>> +
>> +     unmask = (sysmon->masked_temp ^ status) & sysmon->masked_temp;
> 
> Is this the same as
> 
>          unmask =  ~status & sysmon->masked_temp;
> 
> ?

Yes, that's equivalent, Will simplify.
> 
>> +     sysmon->masked_temp &= status;
>> +
>> +     unmask &= ~sysmon->temp_mask;
> 
> The above needs a comment explaining the logic.

Accepted. Will add a comment explaining that we only unmask
interrupts that have cleared in hardware and are not
administratively disabled by userspace (via temp_mask).

> 
>> +}
> 
> ...
> 
>> +static void sysmon_unmask_worker(struct work_struct *work)
>> +{
>> +     struct sysmon *sysmon = container_of(work, struct sysmon,
>> +                                          sysmon_unmask_work.work);
> 
> Better to split as
> 
>          struct sysmon *sysmon =
>                  container_of(work, struct sysmon, sysmon_unmask_work.work);

Accepted.

> 
>> +     unsigned int isr;
>> +
>> +     spin_lock_irq(&sysmon->irq_lock);
>> +     regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
>> +     regmap_write(sysmon->regmap, SYSMON_ISR, isr);
>> +     sysmon_unmask_temp(sysmon, isr);
>> +     spin_unlock_irq(&sysmon->irq_lock);
>> +
>> +     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);
>> +
>> +     regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
>> +     regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
>> +
>> +     isr &= ~imr;
> 
>> +     regmap_write(sysmon->regmap, SYSMON_ISR, isr);
> 
> Is writing 0 necessary?

No. Will move the ISR write after the if (!isr) early return.

> 
>> +     if (!isr)
>> +             return IRQ_NONE;
>> +
>> +     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 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;
>> +
>> +     ret = devm_delayed_work_autocancel(dev, &sysmon->sysmon_unmask_work,
>> +                                        sysmon_unmask_worker);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
>> +     if (ret)
>> +             return ret;
>> +     sysmon->temp_mask = imr & SYSMON_TEMP_INTR_MASK;
>> +
>> +     return devm_request_irq(dev, irq, sysmon_iio_irq, 0,
>> +                             "sysmon-irq", indio_dev);
> 
> 
> 
>> +}
> 
> ...
> 
>> +static int sysmon_init_hysteresis(struct sysmon *sysmon, int address,
> 
> Can address be negative?

No. Will change to unsigned int.

> 
>> +                               int *hysteresis)
> 
> ...
> 
>> +     ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = regmap_read(sysmon->regmap, lower_off, &lower_reg);
>> +     if (ret)
>> +             return ret;
> 
> Half of the IO accessors have no error checks, these do.
> Why is this inconsistency?

The regmap calls in sysmon_unmask_worker and sysmon_iio_irq
have no error checks because they run in contexts where errors
cannot be propagated (workqueue, hardirq). The init path checks
errors because it can return them to the caller. Will add a
comment explaining this.

> 
> ...
> 
>> +     if (has_irq) {
>> +             temp_chans = temp_channels_with_events;
>> +             num_static = ARRAY_SIZE(temp_channels_with_events);
>> +     } else {
>> +             temp_chans = temp_channels_no_events;
>> +             num_static = ARRAY_SIZE(temp_channels_no_events);
>> +     }
>> +
>>        sysmon_channels = devm_kcalloc(dev,
>> -                                    size_add(ARRAY_SIZE(temp_channels),
>> +                                    size_add(num_static,
>>                                                num_supply + num_temp),
> 
> It makes inconsistency. Even originally. It should be two calls to size_add().

Accepted.

> 
>>                                       sizeof(*sysmon_channels), GFP_KERNEL);
>>        if (!sysmon_channels)
>>                return -ENOMEM;
> 
> 
> ...
> 
>> +     irq = fwnode_irq_get(dev_fwnode(dev), 0);
>> +     has_irq = irq > 0;
> 
> This misses deferred probe.

Accepted. Will check for negative return and propagate.

> 
> ...
> 
>> -     ret = sysmon_parse_fw(indio_dev, dev);
>> +     ret = sysmon_parse_fw(indio_dev, dev, has_irq);
> 
> Why do we need has_irq? You can supply irq there as well and check it against 0.

Accepted. Will pass irq directly and check > 0 inside
sysmon_parse_fw.

Regards,
Salih

> 
>>        if (ret)
>>                return ret;
>>
>> +     if (has_irq) {
>> +             /* Set hysteresis mode for both temperature channels */
>> +             ret = regmap_set_bits(sysmon->regmap, SYSMON_TEMP_EV_CFG,
>> +                                   SYSMON_OT_HYST_MASK |
>> +                                   SYSMON_TEMP_HYST_MASK);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             /* Initialize cached hysteresis from hardware registers */
>> +             ret = sysmon_init_hysteresis(sysmon, SYSMON_ADDR_TEMP_EVENT,
>> +                                          &sysmon->temp_hysteresis);
>> +             if (ret)
>> +                     return ret;
>> +             ret = sysmon_init_hysteresis(sysmon, SYSMON_ADDR_OT_EVENT,
>> +                                          &sysmon->ot_hysteresis);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             ret = sysmon_init_interrupt(sysmon, dev, indio_dev, irq);
>> +             if (ret)
>> +                     return ret;
>> +     }
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
>