Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 16, 2023 at 10:45:25AM -0400, Frank Li wrote:
> > On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote:
> > > On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote:
> 
> > > > Ping
> > > 
> > > Read and follow please (and then ping us):
> > > https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com
> > 
> > Could you please help point which specic one was not follow aboved guide?
> > Then I can update my code. I think that's efficial communication method. I
> > think I have read it serial times. But not sure which one violate the
> > guide?
> > 
> > @Bjorn Helgaas. How do you think so? 
> 
> Since Lorenzo didn't point out anything specific in the patch itself,
> I think he was probably referring to the subject line and this advice:
> 
>   - Follow the existing convention, i.e., run "git log --oneline
>     <file>" and make yours match in format, capitalization, and
>     sentence structure.  For example, native host bridge driver patch
>     titles look like this:
> 
>       PCI: altera: Fix platform_get_irq() error handling
>       PCI: vmd: Remove IRQ affinity so we can allocate more IRQs
>       PCI: mediatek: Add MSI support for MT2712 and MT7622
>       PCI: rockchip: Remove IRQ domain if probe fails
> 
> In this case, your subject line was:
> 
>   PCI: layerscape: add suspend/resume for ls1021a
> 
> The advice was to run this:
> 
>   $ git log --oneline drivers/pci/controller/dwc/pci-layerscape.c
>   83c088148c8e PCI: Use PCI_HEADER_TYPE_* instead of literals
>   9fda4d09905d PCI: layerscape: Add power management support for ls1028a
>   277004d7a4a3 PCI: Remove unnecessary <linux/of_irq.h> includes
>   60b3c27fb9b9 PCI: dwc: Rename struct pcie_port to dw_pcie_rp
>   d23f0c11aca2 PCI: layerscape: Change to use the DWC common link-up check function
>   7007b745a508 PCI: layerscape: Convert to builtin_platform_driver()
>   60f5b73fa0f2 PCI: dwc: Remove unnecessary wrappers around dw_pcie_host_init()
>   b9ac0f9dc8ea PCI: dwc: Move dw_pcie_setup_rc() to DWC common code
>   f78f02638af5 PCI: dwc: Rework MSI initialization
> 
> Note that these summaries are all complete sentences that start with a
> capital letter:
> 
>   Use PCI_HEADER_TYPE_* instead of literals
>   Add power management support for ls1028a
>   Remove unnecessary <linux/of_irq.h> includes
>   ...
> 
> So yours could be this:
> 
>   PCI: layerscape: Add suspend/resume for ls1021a
>                    ^
> 
> This is trivial, obviously.  But the uppercase/lowercase distinction
> carries information, and it's an unnecessary distraction to notice
> that "oh, this is different from the rest; is the difference
> important or should I ignore it?"
> 
> Obviously Lorenzo *could* edit all your subject lines on your behalf,
> but it makes everybody's life easier if people look at the existing
> code and follow the style when making changes.
> 
> E.g., write subject lines that are similar in style to previous ones,
> name local variables similarly to other functions, use line lengths
> consistent with the rest of the file, etc.  After applying a change,
> the file should look like a coherent whole; we should not be able to
> tell that this hunk was added later by somebody else.  This all helps
> make the code (and the git history) more readable and maintainable.

Thank you very much Bjorn. Frank, now please resend your patches and
make sure that you follow these guidelines, they must be clear by now.

Lorenzo