Re: [PATCH 7/7] PCI: aardvark: Reset PCIe card and disable PHY at driver unbind

["Marek Behún" <kabel@kernel.org>]

On Tue, 30 Nov 2021 10:31:57 +0000
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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@arm.com> wrote:
> >   
> > > On Sun, Oct 31, 2021 at 07:12:33PM +0100, Marek Behún wrote:  
> > > > From: Pali Rohár <pali@kernel.org>
> > > > 
> > > > 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