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

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

Hi Jonathan,

On 28/05/2026 13:06, Jonathan Cameron wrote:
> 
> 
> On Wed, 27 May 2026 12:42:06 +0100
> Salih Erim <salih.erim@amd.com> 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@amd.com/
>>    v1: https://lore.kernel.org/all/cover.1757061697.git.michal.simek@amd.com/
>>
> 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