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

[Miclaus, Antoniu]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> Sent: Monday, March 16, 2026 11:57 AM
> To: Jonathan Cameron <jic23@xxxxxxxxxx>
> Cc: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>;
> David Lechner <dlechner@xxxxxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>;
> Andy Shevchenko <andy@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Olivier Moysan <olivier.moysan@xxxxxxxxxxx>; linux-
> iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v6 4/4] iio: adc: ad4080: add support for AD4880 dual-
> channel ADC
> 
> [External]
> 
> 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:
> > > > Add support for the AD4880, a dual-channel 20-bit 40MSPS SAR ADC
> with
> > > > integrated fully differential amplifiers (FDA).
> > > >
> > > > The AD4880 has two independent ADC channels, each with its own SPI
> > > > configuration interface. The driver uses spi_new_ancillary_device() to
> > > > create an additional SPI device for the second channel, allowing both
> > > > channels to share the same SPI bus with different chip selects.
> > >
> > > I am still not sure this is the best approach we can have.
> > > In any case, I have immediate questions here about regmap usage.
> >
> > I think we have a fairly fundamental misalignment on what this is.
> >
> > To my understanding (diagram on first page of the datasheet)
> > + the functional block diagram on page 3 it's effectively two almost
> > entirely separate devices in one package (sharing of power etc) and
> > a few common wires for clocks references etc.  Pretty close to some
> > of the multi die devices we get for IMUs etc but with tighter coupling
> > that forces one driver (for the IMUs we just register separate drivers).
> >
> > It 'might' use one SPI bus, or 2 or even 4 (if using separate data
> > interfaces).
> >
> > Just to speed things up let me have a go at answering the questions.
> >
> > > - Why do we need to have a separate regmap per channel?
> >
> > Propose an alternative? It's two independent interfaces, so you
> > could spin a special regmap to handle that, but it's much simpler
> > to just use standard stuff and keep them separate. Not to mention it
> > would either have to do external locking or falsely imply
> > there was any restriction on using both interfaces at once
> > (there isn't)
> >
> > > - 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.
  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."
  3. devm_iio_backend_request_buffer() - requests the buffer from
  st->back[0] because all channel data is interleaved into a
  single stream (there's an inline comment). Only one buffer is needed.

  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)
- A single interleaved data output stream

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.

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