On Mon, Nov 29, 2021 at 06:15:53PM +0100, Marek Behún wrote: > On Mon, 29 Nov 2021 16:40:43 +0000 > Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> wrote: > > > On Sun, Oct 31, 2021 at 07:12:33PM +0100, Marek Behún wrote: > > > From: Pali Rohár <pali@xxxxxxxxxx> > > > > > > When unbinding driver, assert PERST# signal which prepares PCIe card for > > > power down. Then disable link training and PHY. > > > > This reads as three actions. If we carry them out as a single patch we > > have to explain why they are related and what problem they are solving > > as a _single_ commit. > > > > Otherwise we have to split this patch into three and explain each of > > them as a separate fix. > > > > I understand it is tempting to coalesce missing code in one single > > change but every commit must implement a single logical change. > > Hi Lorenzo, > > this is a fix for driver remove function. Although each of these things > could be introduced in separate commits, IMO it doesn't make sense to > split it. It should have been done this way in the first place when the > driver removal support was introduced. I guess we could rewrite the > commit message to: > > PCI: aardvark: Disable controller entirely at driver unbind "PCI: aardvark: Fix the controller disabling sequence" > Add the following to driver unbind to disable the controller entirely: > - asserting PERST# signal > - disabling link training > - disable PHY > > Would this be okay? Yes, that's what I meant. I would describe the change in its entirety not as three fixes - it makes sense to have one single patch as long as we describe it properly. Thanks, Lorenzo
