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

[Jonathan Cameron <jic23@kernel.org>]

> >> +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 mask, shift;
> >> +     u32 raw_val;
> >> +     int offset;
> >> +     int ret;
> >> +
> >> +     guard(mutex)(&sysmon->lock);
> >> +
> >> +     if (chan->type == IIO_TEMP) {
> >> +             if (info == IIO_EV_INFO_VALUE) {
> >> +                     offset = sysmon_temp_thresh_offset(chan->address, dir);
> >> +                     if (offset < 0)
> >> +                             return offset;
> >> +                     sysmon_millicelsius_to_q8p7(&raw_val, val);
> >> +                     return regmap_write(sysmon->regmap, offset, raw_val);
> >> +             }
> >> +             if (info == IIO_EV_INFO_HYSTERESIS) {
> >> +                     mask = (chan->address == SYSMON_ADDR_OT_EVENT) ?
> >> +                             SYSMON_OT_HYST_BIT : SYSMON_TEMP_HYST_BIT;
> >> +                     shift = (chan->address == SYSMON_ADDR_OT_EVENT) ? 0 : 1;
> >> +                     if (val & ~1)  
> > 
> > Just to confirm - this only has hysteresis values of 0 or 1?  That's unusually
> > small given hysteresis should be in same units as _raw.  
> 
> You're right, this is wrong. The current code exposes a mode-select
> bit from ALARM_CONFIG (0 = window mode, 1 = hysteresis mode), not an
> actual hysteresis value.
> 
> The hardware has independent upper and lower threshold registers for
> each temperature alarm (DEVICE_TEMP and OT), plus a mode bit in
> ALARM_CONFIG that selects between window mode (alarm on crossing
> either threshold) and hysteresis mode (upper triggers, lower clears).
> Since the hardware has a single alarm bit per temperature channel,
> even in window mode you can't distinguish which threshold was
> crossed. Hysteresis mode maps naturally to the IIO event model.
> 
> I'll rework this as follows:
> - Hard-code ALARM_CONFIG to hysteresis mode during init (both
>    DEVICE_TEMP and OT)
> - Expose hysteresis as a writable value in millicelsius, stored in
>    the driver
> - Keep both rising (upper) and falling (lower) thresholds writable

> - Couple the three attributes:
>      Write rising    -> recompute lower = upper - stored_hysteresis
>      Write falling   -> update stored_hysteresis = upper - lower
>      Write hysteresis -> recompute lower = upper - new_hysteresis
> - Read hysteresis returns the stored value
> 
> This keeps full user control over both thresholds while exposing
> hysteresis as a proper temperature value, matching IIO semantics.
> Window mode support could be added later if needed without ABI
> changes.
> 
Not quite because that falling attribute is not in line with the ABI.

This needs a little hammering to fit in the oddly shaped hole of our ABI.

If you are sticking to hystersis only (not window mode) then you need
to expose it as rising threshold + hysteresis (not falling threshold as
there is no even triggered in that direction - unless you are doing something
nastier like triggering on both edges of the interrupt - in which case this
is very similar to window mode and for both event directions you'd need
to set the hysteresis to the difference between the threshold values).

If you do want to do window mode - that would be fine but you'd need
to then do a falling event where the enable sets the hysteresis reported
to 0.  Whether a write to hysteresis would then fail or we'd disable the
falling direction would need some discussion - the ABI doesn't constrain
that so it's a case of what is less likely to confuse a user.

Jonathan


> > 
> > Also similar to above, I'd split the two cases and use FIELD_PREP()  
> 
> This block will be replaced by the hysteresis rework described above.
> 
> Best regards,
> Salih
> 
> 
> >   
> >> +                             return -EINVAL;
> >> +                     return regmap_update_bits(sysmon->regmap,
> >> +                                               SYSMON_TEMP_EV_CFG,
> >> +                                               mask, val << shift);
> >> +             }
> >> +     } else if (chan->type == 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);
> >> +     }
> >> +
> >> +     return -EINVAL;
> >> +}  
> > 
> >   
> 
> 
> 
>