Date:   Tue, 15 Nov 2022 20:10:53 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Nuno Sá <noname.nuno@...il.com>
Cc:     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 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?). 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.

Rasmus

Powered by blists - more mailing lists