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

[Jonathan Cameron]

On Mon, 18 May 2026 00:11:36 +0100
"Erim, Salih" <salih.erim@xxxxxxx> wrote:

> 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@xxxxxxx> 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@xxxxxxx> 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.

Ok. I think this is the one item that has shown up in the discussion that
makes an argument this is a lot more general purpose than the name would
suggest and so possibly should be an IIO driver.

It is a fairly slow ADC though at only 8ksps so not sure how general
purpose these actually are but the potential is there.

Jonathan




> 
> 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.
> >>>  
> >>  
> >   
> 
>