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

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Oct 16, 2023 at 12:27:42PM -0500, Bjorn Helgaas wrote:
> "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.

BTW, I mentioned all these things and more a month or so ago:

  https://lore.kernel.org/r/20230918191602.GA201859@bhelgaas
  https://lore.kernel.org/r/20230918193337.GA203483@bhelgaas
  https://lore.kernel.org/r/20230918193036.GA203163@bhelgaas

I *thought* this seemed familiar ;)

Bjorn

> 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 <ysato@users.sourceforge.jp>
> > ---
> >  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