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

[Rasmus Villemoes]

On 16/11/2022 11.22, 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.

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