On Wed, 2022-11-16 at 10:22 +0000, 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. > > > 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. > Yeah, without testing maybe better to play safe. But, FWIW, what I read from the datasheet is just another typical reset gpio usage with more (fancy/confusing) description. - Nuno Sá
