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

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

Hi Jonathan,

On 12/06/2026 13:10, Jonathan Cameron wrote:
> On Thu, 11 Jun 2026 23:27:35 +0100
> Salih Erim <salih.erim@amd.com> 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@amd.com>
>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>> Signed-off-by: Salih Erim <salih.erim@amd.com>
> 
> 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