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 13:50:07 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Ceclan Dumitru <mitrutzceclan@...il.com>
Cc: David Lechner <dlechner@...libre.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 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


>