RE: [PATCH v6 4/4] iio: adc: ad4080: add support for AD4880 dual-channel ADC

[Miclaus, Antoniu]

> On Mon, Mar 16, 2026 at 03:09:18PM +0000, Miclaus, Antoniu wrote:
> > > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> > > Sent: Monday, March 16, 2026 4:42 PM
> > > On Mon, Mar 16, 2026 at 12:31:09PM +0000, Miclaus, Antoniu wrote:
> > > > > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> > > > > Sent: Monday, March 16, 2026 11:57 AM
> > > > > On Sat, Mar 14, 2026 at 12:00:22PM +0000, Jonathan Cameron
> wrote:
> > > > > > On Fri, 13 Mar 2026 16:22:53 +0200
> > > > > > Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:
> > > > > > > On Fri, Mar 13, 2026 at 01:58:53PM +0200, Antoniu Miclaus
> wrote:
> 
> ...
> 
> > > > > > > - What is special about channel 0?
> > > > > >
> > > > > > Nothing.
> > > > >
> > > > > Then why code does explicit access to regmap channel 0?
> > > > > We should have regmap[ch] in all cases in the code.
> > > > >
> > > >   There are three places that use channel 0 explicitly, none of which
> > > >   imply channel 0 is functionally special from a hardware perspective:
> > > >
> > > >   1. ad4080_reg_access() - the debugfs reg_access callback has no
> > > >   channel concept, it's a single (reg, val) interface. We have to
> > > >   pick one regmap, channel 0 is the default choice. I can improve
> > > >   the comment to make this clearer.
> > >
> > > Then it's simply wrong. You allow only one channel to be printed. The
> debugfs
> > > has to print two channels, no?
> >
> > The IIO debugfs_reg_access callback signature is fixed by the framework -
> > it provides (reg, writeval, readval) with no channel parameter.
> >
> > For v7 I can drop debugfs_reg_access. But doesn't hurt if we have at least
> half
> > access to the debugfs for it.
> 
> This will confuse the users. Either do not print or print it all.

Agreed, will drop it in v7.

> 
> > > >   2. ad4080_properties_parse() - uses regmap_get_device(st-
> >regmap[0])
> > > >   solely to obtain the struct device * for reading DT properties.
> > > >   The device tree properties live on the parent SPI node, which is
> > > >   channel 0's device. This isn't "channel 0 is special", it's just
> > > >   "DT properties belong to the primary SPI device."
> > >
> > > Can we simply pass the struct device to that function?
> > Yes, can do that in v7 if you think it is absolutely necessary.
> 
> Yes, please.

Will do.

> ...
> 
> > > >   All register configuration (setup, filter, decimation) already uses
> > > >   regmap[ch] throughout.
> > > >
> > > > > > > - Is it okay to communicate with different channels simultaneously?
> > > > > >
> > > > > > Yes. They are entirely parallel bits of silicon. Own state machines
> > > > > > and everything.
> > > > > > The configuration registers section of the datasheet says:
> > > > > > "Each channel has it's own independent configuration memory
> > > > > > accessible through it's separate configuration SPI interface."
> > > > > >
> > > > > > >   Wouldn't be a nasty race with HW IO?
> > > > > >
> > > > > > Nope. You are talking to different devices (more or less).
> > > > >
> > > > > If it's a twins in the package, why do we have a special handling and not
> > > just
> > > > > describing two independent devices in the DT/fw?
> > > >
> > > > Because they are not fully independent - they share:
> > > > - Power supplies and voltage reference
> > > > - The CNV clock (conversion trigger)
> > >
> > > Okay, then why not having a core part and a glue driver that registers as
> many
> > > devices as you wish and provides just a common stuff?
> 
> > Because the AD4880 is not two independent ADCs sharing a package - it is
> > a single device with a single interleaved data output. Splitting into
> > separate IIO devices would make synchronized dual-channel capture
> > impossible from userspace, which is the primary use case for this part.
> 
> Sounds to me like you need, probably, a virtual device for that.
> Maybe even on IIO level. Do we expect more devices like this to
> be enabled in the future (or maybe already in tree, but lacking this
> feature)?
>

 The AD4880 is a fairly unique part - having separate SPI config
 interfaces per channel with a shared interleaved data output is not
 a common pattern, and the chances of another device like this being
 upstreamed are low. Given that Jonathan has already reviewed and
 approved a previous version of this series, and the patch has
 collected multiple Reviewed-by tags, I'd prefer to keep the current
 approach.

> > The shared resources (supplies, CNV clock, interleaved data stream) are
> > not just "glue" - they define the device's operating model.
> 
> Sure, like any other resource for MFD (HW speaking).

MFD models a single device exposing multiple functionally distinct
sub-devices. The AD4880 channels are not distinct sub-devices - they share a
single interleaved data stream, and splitting them would break
synchronized capture.

> 
> > The per-channel SPI interfaces exist only for register configuration; the
> > actual data path is a single stream handled entirely by the backend.
> >
> > > We have similar (to some extend) cases with SPI/I²C where
> > > drivers/platform/x86/serial-multi-instantiate.c services as "MFD" for that
> > > type of busses.
> > >
> > > > - A single interleaved data output stream
> > >
> > > How does it work in non-racy way?
> >
> > The data path has no software involvement at runtime. The CNV clock
> >  triggers both channels to sample simultaneously, and the device outputs
> >  the conversion results as a single interleaved bitstream on the data
> >  lane(s). The FPGA backend captures this stream directly - no SPI
> >  register reads are involved in the data path. The only SPI traffic is
> >  for configuration, and each channel has its own independent SPI
> >  interface and regmap, so there is no shared bus contention.
> 
> Okay, so it's in a way more complex (like a camera sensor in terms of
> data/configuration paths) device. It's now even more looking that the
> current approach is a quick hack rather than a solution to make this
> properly fit Linux device model.
>

The driver uses spi_new_ancillary_device() for the config path and
 the IIO backend for the data path - both existing kernel
 infrastructure used as intended. No custom abstractions are added.

> > > > Describing them as two independent DT nodes would mean duplicating
> > > > all the shared resources, and more importantly, the data interface
> > > > is a single interleaved stream feeding into one IIO buffer. Having
> > > > two separate IIO devices would make synchronized capture impossible
> > > > from userspace.
> 
> > > > This is exactly the use case spi_new_ancillary_device() was designed
> > > > for - a multi-die device sharing a bus with separate chip selects for
> > > > configuration but common data/clock/power infrastructure.
> 
> This... It doesn't fit the data path as far as I read from the above.

Right, spi_new_ancillary_device() covers only the config path.
The data path is handled entirely by the IIO backend with no SPI
involvement at runtime.

> 
> > > See above.
> > >
> > > > > TO me is either something special about channel 0, then we have to
> > > > > synchronise
> > > > > accesses, or there is no point to have this patch at all, just make devices
> to
> > > > > be the same under the hood and describe as independent pair.
> 
> --
> With Best Regards,
> Andy Shevchenko
>