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

[Bjorn Helgaas]

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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> 
> 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.

Bjorn