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

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

Hi Jonathan,

On 16/05/2026 11:20, Jonathan Cameron wrote:
> 
> On Tue, 12 May 2026 12:35:21 +0100
> Salih Erim <salih.erim@amd.com> wrote:
> 
>> Hi Guenter and Jonathan,
>>
>> On 5/4/2026 8:26 PM, Guenter Roeck wrote:
>>>
>>>
>>> On 5/4/26 10:32, Jonathan Cameron wrote:
>>>> On Sat, 2 May 2026 12:19:48 +0100
>>>> Salih Erim <salih.erim@amd.com> wrote:
>>>>
>>>>> Add the AMD/Xilinx Versal System Monitor (SysMon) IIO driver.
>>>>>
>>>>> The driver is split into a bus-agnostic core module
>>>>> (versal-sysmon-core) and a memory-mapped I/O platform driver
>>>>> (versal-sysmon). The core uses the regmap API so that different
>>>>> bus implementations can share the same IIO logic.
>>>>>
>>>>> The core provides:
>>>>>     - Static temperature channels (current max/min, peak max/min)
>>>>>     - Supply voltage channels parsed from DT container nodes
>>>>>     - Temperature satellite channels parsed from DT container nodes
>>>>>     - read_raw for IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_PROCESSED
>>>>>     - read_label using the DT label property
>>>>
>>>> Various comments inline.  One thing to check.
>>>> Is this one strictly a hardware monitoring device? Or does it
>>>> get used for more general ADC purposes?  Did you consider an HWMON driver
>>>> for it? The above sounds a lot like hwmon. So why IIO for this one?
>>>>
>>>> I wasn't awake enough on v1 to raise this!  Sorry about that.
>>>> +CC Guenter and linux-hwmon for that discussion.
>>>>
>>>
>>> This very much sounds like a hardware monitoring device to me.
>>
>> The device is indeed used for hardware monitoring, but the hardware
>> characteristics push it towards IIO:
>>
>> - The predecessor (Zynq UltraScale+ AMS, xilinx-ams.c) is already
>>     in drivers/iio/adc/ upstream. This driver is the direct successor
>>     for the Versal generation.
> 
> Was a long time back but at the time I think it was argued that some
> usecases for that device were general purpose external ADC channels
> rather than just hardware monitoring. Is that true for the new IP?
> (might not have been true for the old one!)

Yes, Versal SysMon has a dedicated VP/VN differential analog
input pair on package pins plus up to 16 auxiliary analog inputs
(VAUXP[15:0]/VAUXN[15:0]) on MIO/HDIO pins. These 17 external
channels support unipolar and bipolar modes for measuring
arbitrary external voltages; bipolar in the binding exists for
these channels. Oversampling is also most relevant for external
inputs where signal noise matters.

> 
>>
>> - The supply voltage encoding is a modified floating-point format
>>     with per-register exponent and format bits. This non-linear
>>     encoding doesn't map well to hwmon's linear in*_input model.
> 
> Given IIO doesn't really do floating point either I assume that is
> getting converted to something fixed point which ever subsystem
> is used.

Correct, driver converts to millivolts. Both subsystems would
need the same conversion code.

> 
>>
>> - The device has configurable threshold events with per-channel
>>     alarm registers, hysteresis bits, and level-sensitive interrupt
>>     masking/unmasking -- which maps directly to the IIO event
>>     infrastructure.
> 
> What in that list doesn't map to hwmon events?

Comparable, agreed.

> 
>>
>> - Oversampling is hardware-configurable per channel type with
>>     per-channel averaging enable registers.
> 
> I think this is not present in hwmon (could be wrong!) but is there
> a 'right' configuration for a typical usecase?  I.e. would sensible
> defaults work?

Hardware default is no oversampling (ratio=1), which is fine for
supply monitoring. Configurable oversampling is mainly useful for
external analog inputs.

> 
>>
>> - Up to 160 voltage and 64 temperature channels are dynamically
>>     configured from DT, which fits IIO's dynamic channel model
>>     better than hwmon's compile-time attribute groups.
> 
> This used to be true, but hwmon has for some years supported a similar
> model for channel creation to that of IIO + you even for traditional
> attributes it is easy enough to create them dynamically (that's afterall
> what the IIO core does under the hood!)
> 
> Anyhow, take a look at struct hwmon_chip_info and the HWMON_CHANNEL_INFO()
> macro.  I couldn't immediately spot a dynamic user but maybe Guenter can
> point to one.

Looked at these and a few drivers using dynamic allocation
(hp-wmi-sensors, mr75203). Agreed, hwmon handles dynamic channels
well, not a differentiator.

> 
>>
>> - The follow-up thermal driver uses the IIO consumer API
>>     (iio_channel_read) to aggregate temperature data across
>>     multiple satellites into thermal zones. The iio-hwmon bridge
>>     then exposes the same data to hwmon userspace.
> 
> This might be a good reason for IIO. However what stops you just embedding
> all that in a single hwmon driver that also registers the thermal zones?

Nothing prevents it technically. IIO consumer API gives a clean
separation between measurement provider and thermal aggregation,
but hwmon can register thermal zones directly.

> 
>>
>> So the architecture is: IIO driver (provider) -> iio-hwmon bridge
>> (hwmon exposure) + IIO consumer (thermal zones). This gives both
>> hwmon and thermal framework access through a single IIO provider.
> 
> So overall there are some possible reasons in here for using IIO but
> I think a little more in depth analysis is needed.

With 17 external ADC channels in mind, do you think IIO remains
appropriate here, or would you prefer we move to hwmon?

Thanks,
Salih

> 
> Thanks,
> 
> Jonathan
> 
>>>
>>> Guenter
>>>
>>
>> Salih.
>>
>