Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <730ef480-8f10-6a38-b78a-13600a805dea@rasmusvillemoes.dk>
Date:   Fri, 18 Nov 2022 12:21:11 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio
Content-Language: en-US, da
To:     Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc:     =?UTF-8?Q?Nuno_S=c3=a1?= <noname.nuno@gmail.com>,
        Jonathan Cameron <jic23@kernel.org>,
        "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com>,
        Rob Herring <robh+dt@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        "Hennerich, Michael" <Michael.Hennerich@analog.com>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20221111143921.742194-1-linux@rasmusvillemoes.dk>
 <20221111143921.742194-6-linux@rasmusvillemoes.dk>
 <20221112170705.7efe1673@jic23-huawei>
 <095a454b55cf497392a621649f24e067@analog.com>
 <20221114194447.2528f699@jic23-huawei>
 <0d6b3e4047df9f560079a562bc167bd7a0bf2d28.camel@gmail.com>
 <20221115161052.00002633@Huawei.com>
 <6d76cc6d-9db7-5b18-e4f1-dc220b3929a3@rasmusvillemoes.dk>
 <20221116102200.00003d16@Huawei.com>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
In-Reply-To: <20221116102200.00003d16@Huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 16/11/2022 11.22, Jonathan Cameron wrote:
> On Tue, 15 Nov 2022 20:10:53 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
>> On 15/11/2022 17.10, Jonathan Cameron wrote:
>>> On Tue, 15 Nov 2022 15:49:46 +0100
>>> Nuno Sá <noname.nuno@gmail.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@analog.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.

So I've booted quite a number of times with various large sleep values
(between 1 and 10ms), and never seen a problem. Our hardware guys also
confirm that there should be no such thing as a "too long" pulse.

So do you want me to respin, moving the gpio request into the reset
function (i.e. not storing the descriptor in the ad74413r_state as Nuno
pointed out), requesting it in asserted state, and then, if the gpio was
found, just do the fsleep(50) and then deassert it?

Rasmus