Re: [PATCH 0/8] PCI: rockchip: Fix PCIe endpoint controller driver

[Bjorn Helgaas <helgaas@kernel.org>]

Hi Rick,

Thanks very much for your work.

On Thu, Jan 26, 2023 at 02:50:40PM +0100, Rick Wertenbroek wrote:
> This is a series of patches that fixes the PCIe endpoint controller driver
> for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19
> 
> The original driver in mainline had issues and would not allow for the
> RK3399 to operate in PCIe endpoint mode. This patch series fixes that so
> that the PCIe core controller of the RK3399 SoC can now act as a PCIe
> endpoint.

So we merged cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip
PCIe controller") when it actually didn't work?  Ouch.  Thanks for
fixing it and testing it.

> Rick Wertenbroek (8):
>   PCI: rockchip: Removed writes to unused registers
>   PCI: rockchip: Fixed setup of Device ID
>   PCI: rockchip: Fixed endpoint controller Configuration Request Retry
>     Status
>   PCI: rockchip: Added poll and timeout to wait for PHY PLLs to be
>     locked
>   PCI: rockchip: Added dtsi entry for PCIe endpoint controller
>   PCI: rockchip: Fixed window mapping and address translation for
>     endpoint
>   PCI: rockchip: Fixed legacy IRQ generation for endpoint
>   PCI: rockchip: Fixed MSI generation from PCIe endpoint core

For the next iteration, can you please update these subject lines and
commit logs to:

  - Use imperative mood, i.e., read like a command, instead of a past
    tense description of what was done.  For example, say "Remove
    writes to unused registers" instead of "Removed writes ..."

  - Be more specific when possible.  "Fix" conveys no information
    about the actual code change.  For example, "Fixed endpoint
    controller Configuration Request Retry Status" gives a general
    idea, but it would be more useful if it said something about
    clearing config mode after probe.

  - Say what the patch does in the commit log.  The current ones often
    describe a *problem*, but do not explicitly say what the patch
    does.  The commit log should be complete in itself even without
    the subject line, so it usually contains a slightly expanded
    version of the subject line.

  - Split patches that do more than one logical thing.  The commit log
    for "Fixed MSI generation ..." talks about a u16/u32 shift issue,
    but the patch also adds an unrelated check for multi-function
    devices.

  - If a patch is a fix for an existing issue and may need to be
    backported, identify the commit that introduced the issue and add
    "Fixes: " lines.  This helps distros figure out whether and how
    far to backport patches.

  - Refer to the device consistently.  I see:
      RK3399 PCI EP core
      RK3399 SoC PCIe EP core
      RK3399 PCIe endpoint core
    I vote for "RK3399 PCIe Endpoint core".

Notes about imperative mood:
  https://chris.beams.io/posts/git-commit/
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n94

>  arch/arm64/boot/dts/rockchip/rk3399.dtsi  |  25 ++++
>  drivers/pci/controller/pcie-rockchip-ep.c | 149 +++++++++++-----------
>  drivers/pci/controller/pcie-rockchip.c    |  16 +++
>  drivers/pci/controller/pcie-rockchip.h    |  36 ++++--
>  4 files changed, 137 insertions(+), 89 deletions(-)
> 
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel