Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Nuno Sá]

On Wed, 2022-11-16 at 10:22 +0000, Jonathan Cameron wrote:
> On Tue, 15 Nov 2022 20:10:53 +0100
> Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote:
> 
> > On 15/11/2022 17.10, Jonathan Cameron wrote:
> > > On Tue, 15 Nov 2022 15:49:46 +0100
> > > Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
> > >   
> > > > On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:  
> > > > > On Mon, 14 Nov 2022 13:52:26 +0000
> > > > > "Tanislav, Cosmin" <Cosmin.Tanislav@xxxxxxxxxx> wrote:
> > > > >     
> > > > > > > 
> > > > > > > I'm a little confused on polarity here.  The pin is a
> > > > > > > !reset so
> > > > > > > we need to drive it low briefly to trigger a reset.
> > > > > > > I'm guessing for your board the pin is set to active low?
> > > > > > > (an
> > > > > > > example
> > > > > > > in the dt would have made that clearer) Hence the pulse
> > > > > > > in here to 1 is actually briefly driving it low before
> > > > > > > restoring
> > > > > > > to high?
> > > > > > > 
> > > > > > > For a pin documented as !reset that seems backwards
> > > > > > > though you
> > > > > > > have
> > > > > > > called it reset so that is fine, but this description
> > > > > > > doesn't
> > > > > > > make that
> > > > > > > celar.      
> > > > > > 
> > > > > > My opinion is that the driver shouldn't exactly know the
> > > > > > polarity
> > > > > > of the reset,
> > > > > > and just assume that setting the reset GPIO to 1 means
> > > > > > putting it
> > > > > > in reset,
> > > > > > and setting it to 0 means bringing out of reset.    
> > > > > 
> > > > > Agreed. I'd just like a comment + example in the dt-binding
> > > > > to make
> > > > > the point
> > > > > that the pin is !reset.
> > > > > 
> > > > > Preferably with an example in the dt binding of the common
> > > > > case of it
> > > > > being wired
> > > > > up to an active low pin.
> > > > > 
> > > > > The main oddity here is the need to pulse it rather than
> > > > > request it
> > > > > directly as
> > > > > in the reset state and then just set that to off.
> > > > > 
> > > > >     
> > > > 
> > > > Agreed... In theory we should be able to request the gpio with
> > > > GPIOD_OUT_HIGH and then just bring the device out of reset  
> > > 
> > > If I recall correctly the datasheet specifically calls out that a
> > > pulse
> > > should be used.  No idea if that's actually true, or if it was
> > > meant
> > > to be there just to say it needs to be set for X nsecs.  
> > 
> > So the data sheet says
> > 
> >   The hardware reset is initiated by pulsing the RESET pin low. The
> > RESET pulse width must comply with the specifications in Table 11.
> > 
> > and table 11 says that the pulse must be min 50us, max 1ms. We
> > don't
> > really have any way whatsoever to ensure that we're not rescheduled
> > right before pulling the gpio high again (deasserting the reset),
> > so the
> > pulse could effectively be much more than 1ms. But I have a hard
> > time
> > believing that that actually matters (i.e., what state would the
> > chip be
> > in if we happen to make a pulse 1234us wide?).
> 
> Test it maybe?  Otherwise we'd have to play games to do it again if
> the
> timing was too long to ensure after a couple of goes we do get a
> suitable
> width pulse.
> 
> > But what might be
> > relevant, and maybe where that 1ms figure really comes from, can
> > perhaps
> > be read in table 10, which lists a "device reset time" of 1ms, with
> > the
> > description
> > 
> >   Time taken for device reset and calibration memory upload to
> > complete
> > hardware or software reset events after the device is powered up
> > 
> > so perhaps we should ensure a 1ms delay after the reset (whether we
> > used
> > the software or gpio method). But that would be a separate fix IMO
> > (and
> > I'm not sure we actually need it).
> > 
> > I don't mind requesting the gpio with GPIOD_OUT_HIGH, but I'd still
> > keep
> > the gpiod_set_value(, 1) in the reset function, otherwise it's a
> > bit too
> > magic for my taste.
> 
> Without testing I'd worry that it really does need a pulse so
> probably better
> to leave it doing so. 
> 

Yeah, without testing maybe better to play safe. But, FWIW, what I read
from the datasheet is just another typical reset gpio usage with more
(fancy/confusing) description.

- Nuno Sá