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
