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?). 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. Rasmus
