Re: [RFC PATCH v3 09/35] drivers/pci/controller: SH7751 PCI Host bridge driver.

"git am" complained about a couple whitespace errors elsewhere in this
series:

  Applying: arch/sh/boot/compressed/head_32.S: passing FDT address to initialize function.
  .git/rebase-apply/patch:25: trailing whitespace.
  Applying: drivers/irqchip: SH7751 IRL external encoder with enable gate.
  .git/rebase-apply/patch:33: new blank line at EOF.

On Sat, Oct 14, 2023 at 11:53:44PM +0900, Yoshinori Sato wrote:
> pci-sh7751.h move from "arch/sh/drivers/pci/pci-sh7751.h"
> pci-sh7751.c convert from "arch/sh/drivers/pci/pci-sh7751.c"

Note the subject line conventions in drivers/pci (use "git log
--oneline" to see them): use something like this:

  PCI: sh7751: Add SH7751 PCI host bridge driver

with no period at the end.

arch/sh/drivers/pci/pci-sh7751.h and arch/sh/drivers/pci/pci-sh7751.c
still exist after applying this series.  Better to have a single patch
that moves the content from arch/sh/drivers/pci/ to
drivers/pci/controller/.

Neither file looks like a simple move; there's a lot of reorganization
going on at the same time.  It's really difficult to review a patch
like that because we can't compare the content from before and after.

If you make a patch that does the cleanup/reorganization, and a
separate patch that is just a simple move, and you use "git mv" for
the move, git should notice that this is just a rename, and that diff
will be tiny.

One of the reorganization patches should be to incorporate the
pci-sh7751.h content directly into pci-sh7751.c.  Since it's only used
in pci-sh7751.c, there's really no advantage to having it in a
separate file.

Bjorn

> Signed-off-by: Yoshinori Sato <[email protected]>
> ---
>  drivers/pci/controller/Kconfig      |   9 +
>  drivers/pci/controller/Makefile     |   1 +
>  drivers/pci/controller/pci-sh7751.c | 285 ++++++++++++++++++++++++++++
>  drivers/pci/controller/pci-sh7751.h | 267 ++++++++++++++++++++++++++
>  4 files changed, 562 insertions(+)
>  create mode 100644 drivers/pci/controller/pci-sh7751.c
>  create mode 100644 drivers/pci/controller/pci-sh7751.h
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index c0c3f2824990..037ff44bd1e8 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -342,6 +342,15 @@ config PCIE_XILINX_CPM
>  	  Say 'Y' here if you want kernel support for the
>  	  Xilinx Versal CPM host bridge.
>  
> +config PCI_SH7751
> +	bool "Renesas SH7751 PCI controller"
> +	depends on OF
> +	depends on CPU_SUBTYPE_SH7751 || CPU_SUBTYPE_SH7751R || COMPILE_TEST
> +	select PCI_HOST_COMMON
> +	help
> +	  Say 'Y' here if you want kernel to support the Renesas SH7751 PCI
> +	  Host Bridge driver.

Move this so the menu entries stay sorted by vendor name.

Bjorn