Re: [PATCH v6 2/5] iio: adc: add Versal SysMon driver

[Erim, Salih]

Hi Jonathan,

On 12/06/2026 13:10, Jonathan Cameron wrote:
> On Thu, 11 Jun 2026 23:27:35 +0100
> Salih Erim <salih.erim@xxxxxxx> wrote:
>
> > Add the core driver and MMIO platform driver for the AMD/Xilinx Versal
> > System Monitor (SysMon) block.
> >
> > The SysMon block resides in the platform management controller (PMC) and
> > provides on-chip voltage and temperature monitoring through a 10-bit,
> > 200 kSPS ADC. It can monitor up to 160 voltage channels and 64
> > temperature satellites distributed across the SoC, with a consistent
> > sample rate of 8 kSPS per channel regardless of how many channels are
> > enabled.
> >
> > The driver is split into two compilation units:
> >    - versal-sysmon-core: Channel parsing, IIO registration, read_raw
> >    - versal-sysmon: MMIO platform driver with custom regmap accessors
> >
> > Voltage results are stored in a 19-bit modified floating-point format
> > and converted to millivolts. Temperature results are stored in Q8.7
> > signed fixed-point Celsius format and converted to millicelsius.
> >
> > The MMIO regmap backend uses a custom reg_write accessor that
> > automatically unlocks the NPI (NoC programming interface) lock
> > register before each write, as required by the hardware. The regmap
> > is configured with fast_io since the underlying MMIO accessors are
> > safe to call from atomic context.
> >
> > Co-developed-by: Michal Simek <michal.simek@xxxxxxx>
> > Signed-off-by: Michal Simek <michal.simek@xxxxxxx>
> > Signed-off-by: Salih Erim <salih.erim@xxxxxxx>
>
> One question on the static temp channels. I may have forgotten
> some earlier discussion!
>
> > diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> > new file mode 100644
> > index 00000000000..c875d156dbe
> > --- /dev/null
> > +++ b/drivers/iio/adc/versal-sysmon-core.c
> > @@ -0,0 +1,281 @@
>
> > +
> > +#define SYSMON_CHAN_TEMP(_chan, _address, _name)             \
> > +{                                                            \
> > +     .type = IIO_TEMP,                                       \
> > +     .indexed = 1,                                           \
> > +     .address = _address,                                    \
> > +     .channel = _chan,                                       \
> > +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> > +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> > +     .datasheet_name = _name,                                \
> > +}
> > +
> > +/* Static temperature channels (always present) */
> > +static const struct iio_chan_spec temp_channels[] = {
> > +     SYSMON_CHAN_TEMP(0, SYSMON_TEMP_MAX, "temp"),
> > +     SYSMON_CHAN_TEMP(1, SYSMON_TEMP_MIN, "min"),
> > +     SYSMON_CHAN_TEMP(2, SYSMON_TEMP_MAX_MAX, "max_max"),
> > +     SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
>
> Sorry, I missed this in previous reviews, but what are these channels?
> The labels are rather unusual.
>
> My guess is they are gathering up values from across a bunch of sensors
> but I'm not certain. They are unusual enough we probably need some documentation.

Your guess is correct. These are hardware-computed aggregate
registers across all active temperature satellites:

  - temp (0x1030): current max across all active satellites
  - min (0x1034): current min across all active satellites
  - max_max (0x1F90): highest peak since last hardware reset
  - min_min (0x1F8C): lowest trough since last hardware reset

The cover letter mentions this but the commit message and code
don't. Will add a comment block above the channel definitions
and expand the commit description in v7.

Thanks,
Salih