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. > 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 >
