Received-SPF: pass (google.com: domain of linux-kernel+bounces-68706-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 16 Feb 2024 14:08:26 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
Cc: linux-iio@vger.kernel.org, Michael Hennerich
 <Michael.Hennerich@analog.com>, Rob Herring <robh+dt@kernel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley
 <conor+dt@kernel.org>, Nuno =?UTF-8?B?U8Oh?= <nuno.sa@analog.com>, Liam
 Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: iio: adc: add ad7944 ADCs
Message-ID: <20240216140826.58b3318d@jic23-huawei>
In-Reply-To: <CAMknhBF8HKDftjBuwuA4GWUmn4j36Zut84d7xLKgZPDaiY87kA@mail.gmail.com>
References: <20240206-ad7944-mainline-v1-0-bf115fa9474f@baylibre.com>
	<20240206-ad7944-mainline-v1-1-bf115fa9474f@baylibre.com>
	<20240210174022.7a0c7cdc@jic23-huawei>
	<CAMknhBF8HKDftjBuwuA4GWUmn4j36Zut84d7xLKgZPDaiY87kA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

> > > +  adi,spi-mode:
> > > +    $ref: /schemas/types.yaml#/definitions/string
> > > +    enum: [ 3-wire, 4-wire, chain ]
> > > +    default: 4-wire
> > > +    description:
> > > +      This chip can operate in a 3-wire mode where SDI is tied to VIO, a 4-wire
> > > +      mode where SDI acts as the CS line, or a chain mode where SDI of one chip
> > > +      is tied to the SDO of the next chip in the chain and the SDI of the last
> > > +      chip in the chain is tied to GND.  
> >
> > there is a standard property in spi-controller.yaml for 3-wire. Does that cover
> > the selection between 3-wire and 4-wire here?  Seems like this might behave
> > differently from that (and so perhaps we shouldn't use 3-wire as the description
> > to avoid confusion, normally 3-wire is a half duplex link I think).  
> 
> I used "3-wire" because that is what the datasheet calls it. But yes,
> I see the potential for confusion here since this "3-wire" is
> completely unrelated to the standard "spi-3wire" property.
Maybe we fall back on a comment that says something like.

"This is not the same as spi-3wire." :)

Whatever we end up with here, I'd like everyone to agree it's
obviously different enough from existing SPI bindings that there won't
be any confusion. 

> 
> >
> > Chain mode is more fun.  We've had that before and I'm trying to remember what
> > the bindings look like. Devices like ad7280a do a different form of chaining.  
> 
> If there isn't a clear precedent for how to write bindings for chained
> devices, this may be something better left for when there is an actual
> use case to be sure we get it right.

Agreed.  Let us kick that into the future.

> 
> >
> > Anyhow, main thing here is we need to be careful that the terms don't overlap
> > with other possible interpretations.
> >
> > I think what this really means is:
> >
> > 3-wire - no chip select, exclusive use of the SPI bus (yuk)  
> 
> This can actually be done two ways. One where there is no CS and we
> use cnv-gpios to control the conversion. The other is where CS of the
> SPI controller is connected to the CNV pin on the ADC and cnv-gpios is
> omitted. This requires very creative use of spi xfers to get the right
> signal but does work.
> 
> In any case to achieve max sample rate these chips need to use this
> "3-wire" mode and have exclusive use of the bus whether is is using
> proper CS or not.
> 
> So maybe it would be more clear to split this one into two modes?
> 3-wire with CS and 3-wire without CS?
OK.

I'm not sure if the standard SPI bindings have an option for
CS tied active?  If so we should reuse that bit of [psson;e/

> 
> > 4-write - conventional SPI with CS  
> 
> Yes.
> 
> > chained - the 3 wire mode really but with some timing effects?  
> 
> Correct. With the exception that the SPI CS line can't be used in
> chain mode (unless maybe if you had an inverted CS signal since the
> CNV pin has to be high during the data transfer).
> 
> >
> > Can we figure out if chained is going on at runtime?  
> 
> No. We would always need the devicetree to at least say how many chips
> are in the chain. Also, in theory, each chip could have independent
> supplies and therefore different reference voltages.
That's one I think we only bother supporting when we actually see it.
For previous chained devices I don't think we've ever needed to do
it because they tend to be used for 'more of the same' rather than
measuring different things.  Supplies so far have always been wired
to single regulator (or single control anyway).


> >
> > If we are going to rule you supplying refin and ref supplies.  
> 
> Not sure what you mean here, but we can get rid of the adi,reference
> property and just add a check to not allow both ref-supply and
> refin-supply at the same time.

I think that is simplest route.

> 
> >  
> > > +
> > > +  cnv-gpios:
> > > +    description:
> > > +      The Convert Input (CNV). This input has multiple functions. It initiates
> > > +      the conversions and selects the SPI mode of the device (chain or CS). In
> > > +      3-wire mode, this property is omitted if the CNV pin is connected to the
> > > +      CS line of the SPI controller.
> > > +    maxItems: 1  
> >
> > ah, that's exciting - so in 3-wire mode, we basically put the CS on a different pin...  
> 
> I explained this above already, but just to have it in context here as
> well... In what the datasheet calls "3-wire" mode, we can either have
> CS connected and no cnv-gpios or we can have no CS and have cnv-gpios
> connected.
> 
> So the intention here was to make cnv-gpios required all other modes
> but in 3-wire mode, make it optional.

Seems reasonable. Thanks for the various explanations. This chip is just odd :)

> 
> 
> >
> > Mark, perhaps you can suggest how to handle this complex family of spi variants?
> >
> > Jonathan
> >