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

[Guenter Roeck]

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.

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

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

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.