Re: [PATCH v3 0/5] iio: adc: add AMD/Xilinx Versal SysMon driver

[Erim, Salih]

Hi Jonathan,

On 28/05/2026 13:06, Jonathan Cameron wrote:
> On Wed, 27 May 2026 12:42:06 +0100
> Salih Erim <salih.erim@xxxxxxx> wrote:
>
> > This series adds a new IIO driver for the AMD/Xilinx Versal System
> > Monitor (SysMon), providing on-chip voltage and temperature monitoring.
> >
> > The Versal SysMon measures up to 160 supply voltages and reads up to
> > 64 temperature satellites distributed across the SoC. The hardware
> > also provides aggregated device temperature registers: the current
> > max and min across all active satellites, and peak/trough values
> > recorded since last hardware reset. The device can be accessed via
> > memory-mapped I/O or via an I2C interface.
> >
> > The driver is split into a bus-agnostic core module using the regmap
> > API, an MMIO platform driver, and an I2C driver. This allows the
> > same IIO logic to be shared across different bus transports.
> >
> > Previous submissions:
> >    v2: https://lore.kernel.org/all/cover.1746182670.git.salih.erim@xxxxxxx/
> >    v1: https://lore.kernel.org/all/cover.1757061697.git.michal.simek@xxxxxxx/
> https://sashiko.dev/#/patchset/20260527114211.174288-1-salih.erim%40amd.com
> Quite a bit of feedback.  Some of which is clearly garbage, like the
> ARCH_VERSAL suggestion, but take a close look as it does tend to pick up on
> stuff that humans miss.

Thanks Jonathan. I've gone through all the Sashiko findings.

False positives:
- ARCH_VERSAL: doesn't exist in mainline; all Versal drivers use
  ARCH_ZYNQMP (EDAC_VERSAL, Versal TRNG, etc.)
- I2C bus atomicity: the SysMon I2C slave protocol requires a
  STOP between command and response; the vendor driver uses the
  same send/recv sequence
- Hardirq on I2C: IRQ handler is never registered on the I2C
  path (has_irq is false when fwnode_irq_get returns negative)
- Voltage threshold clobbering format bits: threshold registers
  only use the mantissa (bits 15:0); vendor driver does the same
- IRQ storm on regmap error: ISR is cleared before handle_events
  is called, so the interrupt won't re-fire
- VERSAL_SYSMON_I2C missing ARCH: no I2C ADC driver in the
  kernel has an ARCH dependency; I2C clients are bus-agnostic

Reviewed but keeping as-is:
- Left-shift of negative in millicelsius_to_q8p7: GCC defines
  this behavior and it's consistent with the read direction
  (right-shift); Andy asked for this symmetry in v2
- Oversampling read without mutex: reading a single int is
  atomic on arm64; adding contention for no practical benefit
- Voltage alarms one-shot: follows the IIO event model where
  userspace re-arms via write_event_config
- Event config reads 0 during masked alarm: standard behavior,
  xilinx-ams does the same
- TOCTOU on masked_temp after spinlock drop: benign; worst case
  the worker reschedules one extra cycle
- Interrupts not disabled on teardown: devm_request_irq frees
  the IRQ handler; probe already disables all interrupts via
  IDR at init, and LIFO devm ordering is correct
- Oversampling shared_by_type for all temp: this is a per-type
  HW setting; static channels report the aggregate result
- val*scale overflow in processedtoraw: realistic voltages
  (< 3.3V) are well within int32 range; overflow requires
  > 32V input which is beyond the ADC range
- sysmon_update_temp_lower underflow: hysteresis is validated
  as non-negative and realistic thresholds keep the subtraction
  well within Q8.7 range

Will address in v4:
- scan_type: dropping entirely (no buffered support), which
  also resolves the realbits question
- RAW + PROCESSED: dropping RAW for voltage (PROCESSED only),
  and using RAW + SCALE for temperature (linear Q8.7)
- temp_mask: take irq_lock in write_event_config to
  synchronize with the unmask worker

Happy to fold these into v4 along with your review feedback.

Thanks,
Salih