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

[Erim, Salih]

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.

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.