On Wed, 27 Jan 2021 08:08:29 +0100 Mike Looijmans wrote: > > Andrew, I don't get what you're saying. > > > > Here is what happens depending on the pre-existing state of the > > reset signal: > > > > Reset (previously asserted): ~~~|_|~~~~|_______ > > Reset (previously deasserted): _____|~~~~|_______ > > ^ ^ ^ > > A B C > > > > At point A, the low going transition is because the reset line is > > requested using GPIOD_OUT_LOW. If the line is successfully requested, > > the first thing we do is set it high _without_ any delay. This is > > point B. So, a glitch occurs between A and B. > > > > We then fsleep() and finally set the GPIO low at point C. > > > > Requesting the line using GPIOD_OUT_HIGH eliminates the A and B > > transitions. Instead we get: > > > > Reset (previously asserted) : ~~~~~~~~~~|______ > > Reset (previously deasserted): ____|~~~~~|______ > > ^ ^ > > A C > > > > Where A and C are the points described above in the code. Point B > > has been eliminated. > > > > Therefore, to me the patch looks entirely reasonable and correct. > > > Thanks, excellent explanation. > > As a bit of background, we were using a Marvell PHY where the datasheet > states that thou shallt not release the reset within 50 ms of power-up. > A pull-down on the active-low reset was thus added. Looking at the reset > signal with a scope revealed a short spike, visible only because it was > being controlled by an I2C GPIO expander. So it's indeed point "B" that > we wanted to eliminate.
This is all useful information - can we roll more of it into the commit message? I'd think that calling out the part and the 50ms value could make things more "concrete" for a reader down the line?
