On Tue, 30 Nov 2021 10:31:57 +0000 Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> wrote: > 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. After I went through it again, I think it would be better to split it into 3 patches. I am going to do that this way. Marek
