Re: [PATCH v6 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver

Bjorn Helgaas
2021-05-05T14:37:10Z

On Wed, May 05, 2021 at 12:26:31PM +0800, Greentime Hu wrote:
> Bjorn Helgaas <helgaas@kernel.org> 於 2021年5月4日 週二 下午9:46寫道：
> > On Tue, May 04, 2021 at 06:59:39PM +0800, Greentime Hu wrote:
> > > From: Paul Walmsley <paul.walmsley@sifive.com>
> > >
> > > Add driver for the SiFive FU740 PCIe host controller.
> > > This controller is based on the DesignWare PCIe core.
> > >
> > > Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> > > Co-developed-by: Henry Styles <hes@sifive.com>
> > > Signed-off-by: Henry Styles <hes@sifive.com>
> > > Co-developed-by: Erik Danie <erik.danie@sifive.com>
> > > Signed-off-by: Erik Danie <erik.danie@sifive.com>
> > > Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
> > > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > > ---
> > > drivers/pci/controller/dwc/Kconfig | 10 +
> > > drivers/pci/controller/dwc/Makefile | 1 +
> > > drivers/pci/controller/dwc/pcie-fu740.c | 309 ++++++++++++++++++++++++
> > > 3 files changed, 320 insertions(+)
> > > create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c
> > >
> > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > index 22c5529e9a65..255d43b1661b 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -318,4 +318,14 @@ config PCIE_AL
> > > required only for DT-based platforms. ACPI platforms with the
> > > Annapurna Labs PCIe controller don't need to enable this.
> > >
> > > +config PCIE_FU740
> > > + bool "SiFive FU740 PCIe host controller"
> > > + depends on PCI_MSI_IRQ_DOMAIN
> > > + depends on SOC_SIFIVE || COMPILE_TEST
> > > + depends on GPIOLIB
> > ...
> > 2) I would prefer not to depend on GPIOLIB because it reduces
> > compile-test coverage. For example, the x86_64 defconfig does not
> > enable GPIOLIB, so one must manually enable it to even be able to
> > enable PCIE_FU740.
> > ...

> Sorry for late to debug this case. I was working on other works and
> just missed the email.
> How about this?

We already dropped the "depends on GPIOLIB" for v5.13.

You can add a select later, for v5.14. Of course, you should post a
complete patch including commit log and signed-off-by.

And please take a look at how other drivers handle this so you do it
the same way.

> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> index e1b2690b6e45..66f57f2db49d 100644
> --- a/arch/riscv/Kconfig.socs
> +++ b/arch/riscv/Kconfig.socs
> @@ -7,6 +7,7 @@ config SOC_SIFIVE
> select CLK_SIFIVE
> select CLK_SIFIVE_PRCI
> select SIFIVE_PLIC
> + select GPIOLIB if PCIE_FU740
> help
> This enables support for SiFive SoC platform hardware.
>
> diff --git a/drivers/pci/controller/dwc/Kconfig
> b/drivers/pci/controller/dwc/Kconfig
> index 255d43b1661b..0a37d21ed64e 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -322,7 +322,6 @@ config PCIE_FU740
> bool "SiFive FU740 PCIe host controller"
> depends on PCI_MSI_IRQ_DOMAIN
> depends on SOC_SIFIVE || COMPILE_TEST
> - depends on GPIOLIB
> select PCIE_DW_HOST
> help
> Say Y here if you want PCIe controller support for the SiFive