linux-kernel - Re: [PATCH v8 1/2] dt-bindings: adc: add AD7173

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 17 Dec 2023 19:00:32 -0600
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Ceclan Dumitru <mitrutzceclan@...il.com>, linus.walleij@...aro.org, brgl@...ev.pl, 
	andy@...nel.org, linux-gpio@...r.kernel.org, 
	Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Michael Walle <michael@...le.cc>, Andy Shevchenko <andy.shevchenko@...il.com>, 
	Arnd Bergmann <arnd@...db.de>, ChiaEn Wu <chiaen_wu@...htek.com>, 
	Niklas Schnelle <schnelle@...ux.ibm.com>, Leonard Göhrs <l.goehrs@...gutronix.de>, 
	Mike Looijmans <mike.looijmans@...ic.nl>, Haibo Chen <haibo.chen@....com>, 
	Hugo Villeneuve <hvilleneuve@...onoff.com>, Ceclan Dumitru <dumitru.ceclan@...log.com>, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v8 1/2] dt-bindings: adc: add AD7173

On Sun, Dec 17, 2023 at 7:50 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu, 14 Dec 2023 19:03:28 +0200
> Ceclan Dumitru <mitrutzceclan@...il.com> wrote:
>
> > On 12/14/23 18:12, David Lechner wrote:
> > > On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@...il.com> wrote:
> > >> On 12/12/23 17:09, David Lechner wrote:
> > >>> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@...il.com> wrote:
> >
> > >> ...
> > >>
> > >>>> +  interrupts:
> > >>>> +    maxItems: 1
> > >>>
> > >>> Shouldn't this be 2? The datasheet says there is a "Data Output Ready"
> > >>> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR
> > >>> pin. Although I could see how RDY could be considered part of the SPI
> > >>> bus. In any case, a description explaining what the interrupt is would
> > >>> be useful.
> > >>>
> > >>
> > >> I do not see how there could be 2 interrupts. DOUT/RDY is used as an
> > >> interrupt when waiting for a conversion to finalize.
> > >>
> > >> Sync and Error are sepparate pins, Sync(if enabled) works only as an
> > >> input that resets the modulator and the digital filter.
> > >
> > > I only looked at the AD7172-2 datasheet and pin 15 is labeled
> > > SYNC/ERROR. Maybe they are separate pins on other chips?
> >
> > Yep, sorry, missed that. All other supported models have them separate.
>
>
> > >
> > >>
> > >> Error can be configured as input, output or ERROR output (OR between all
> > >> internal error sources).
> > >>
> > >> Would this be alright
> > >>   interrupts:
> > >>
> > >>     description: Conversion completion interrupt.
> > >>                  Pin is shared with SPI DOUT.
> > >>     maxItems: 1
> > >
> > > Since ERROR is an output, I would expect it to be an interrupt. The
> > > RDY output, on the other hand, would be wired to a SPI controller with
> > > the SPI_READY feature (I use the Linux flag name here because I'm not
> > > aware of a corresponding devicetree flag). So I don't think the RDY
> > > signal would be an interrupt.
> > >
> >
> > Error does not have the purpose to be an interrupt. The only interrupt
> > used from this chip is the one from the DOUT/~RDY pin. Sure, it is wired
> > to the SPI controller, but when you can't also receive interrupts on
> > that very same CPU pin an issue arises. So that pin is also wired to
> > another GPIO with interrupt support.
>
> You've lost me.  It's a pin that has a state change when an error condition
> occurs.  Why not an interrupt?  Doesn't matter that the driver doesn't
> use this functionality. dt-bindings should be as comprehensive as possible.
> Given it's a multipurpose pin you'd also want to support it as a gpio to be
> complete alongside the other GPIOs.
>
> >
> > This is the same way that ad4130.yaml is written for example (with the
> > exception that ad4130 supports configuring where the interrupt is routed).
> >
> > In regards to SPI_READY _BITUL(7) /* slave pulls low to pause */: the
> > ad_sigma_delta framework (if it can be called that) is written to expect
> > a pin interrupt, not to use SPI_READY controller feature.
>
> SPI_READY is supported by only a couple of controllers. I'm not even that
> sure exactly how it is defined and whether that lines up with this usecase.
> From some old asci art. https://lore.kernel.org/all/1456747459-8559-1-git-send-email-linux@rempel-privat.de/
>
> Flow control: Ready Sequence
> Master CS   |-----1\_______________________|
> Slave  FC   |--------2\____________________|
> DATA        |-----------3\_________________|
>
> So you set master and then wait for a flow control pin (the ready signal) before
> you can actually talk to the device.
>
> Here we are indicating data is ready to be be read out.
>
> So I don't 'think' SPI_READY applies.
>
> Jonathan
>

I'm not arguing that SPI_READY applies in this particular case, but
FWIW it does seem to me like...

1) SPI_READY could be implemented in any SPI driver using a GPIO
interrupt (similar to how we already have GPIO CS)
2) In cases where the SPI controller does have actual hardware support
for SPI_READY and the ADC chip A) uses CS to trigger a conversion and
B) has a "busy" signal that goes low when the conversion is complete,
then the SPI_READY feature could be used to make reading sample data
more efficient by avoiding any CPU intervention between CS assertion
and starting the data xfer due to waiting for the conversion to
complete either by waiting for an interrupt or sleeping for a fixed
amount of time.