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

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]

Message-ID: <20221116102200.00003d16@Huawei.com>
Date:   Wed, 16 Nov 2022 10:22:00 +0000
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
CC:     Nuno Sá <noname.nuno@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        "Tanislav, Cosmin" <Cosmin.Tanislav@...log.com>,
        "Rob Herring" <robh+dt@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

On Tue, 15 Nov 2022 20:10:53 +0100
Rasmus Villemoes <linux@...musvillemoes.dk> wrote:

> On 15/11/2022 17.10, Jonathan Cameron wrote:
> > On Tue, 15 Nov 2022 15:49:46 +0100
> > Nuno Sá <noname.nuno@...il.com> 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@...log.com> 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. 

Jonathan

> 
> Rasmus
>