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

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

Hi Guenter,

On 16/05/2026 16:04, Guenter Roeck wrote:
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
> 
> 
> On 5/16/26 03: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!)
>>
>>>
>>> - 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.
>>
>>>
>>> - 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?
>>
>>>
>>> - 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?
>>
> 
> There is a per sensor type "samples" attribute. I don't recall a request
> or even a chip supporting per-channel oversampling, though such an 
> attribute
> could easily be added if there is a use case.

Good to know, thanks.

> 
>>>
>>> - 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.
>>
> 
> There are several hardware monitoring drivers with dynamic sensor 
> allocation.

Acknowledged, dynamic channels are not 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?
>>
> The hardware monitoring subsystem supports registering thermal sensors as
> thermal zones. It would be up to the thermal subsystem to aggregate them.

Understood.

To add some context from Jonathan's question about general-purpose
ADC use: the Versal SysMon has 17 external analog channels (1 dedicated 
VP/VN differential pair + 16 auxiliary inputs on MIO/HDIO pins) that 
support unipolar and bipolar modes for measuring arbitrary external 
voltages, in addition to the internal supply and temperature monitoring.

Thanks,
Salih.

> 
> Guenter
> 
>>>
>>> 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.
>>
>> Thanks,
>>
>> Jonathan
>>
>>>>
>>>> Guenter
>>>>
>>>
>>> Salih.
>>>
>>
>