Re: [PATCH v6 2/2] PCI: keembay: Add support for Intel Keem Bay

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jan 22, 2021 at 08:56:10AM +0530, srikanth.thokala@intel.com wrote:
> From: Srikanth Thokala <srikanth.thokala@intel.com>
> 
> Add driver for Intel Keem Bay SoC PCIe controller. This controller
> is based on DesignWare PCIe core.
> 
> In root complex mode, only internal reference clock is possible for
> Keem Bay A0. For Keem Bay B0, external reference clock can be used
> and will be the default configuration. Currently, keembay_pcie_of_data
> structure has one member. It will be expanded later to handle this
> difference.
> 
> Endpoint mode link initialization is handled by the boot firmware.
> 
> Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> Signed-off-by: Srikanth Thokala <srikanth.thokala@intel.com>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  MAINTAINERS                               |   7 +
>  drivers/pci/controller/dwc/Kconfig        |  28 ++
>  drivers/pci/controller/dwc/Makefile       |   1 +
>  drivers/pci/controller/dwc/pcie-keembay.c | 446 ++++++++++++++++++++++
>  4 files changed, 482 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-keembay.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 00836f6452f0..2fc0fb03c430 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13852,6 +13852,13 @@ L:	linux-pci@vger.kernel.org
>  S:	Maintained
>  F:	drivers/pci/controller/dwc/*spear*
>  
> +PCI DRIVER FOR INTEL KEEM BAY
> +M:	Srikanth Thokala <srikanth.thokala@intel.com>
> +L:	linux-pci@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/pci/intel,keembay-pcie*
> +F:	drivers/pci/controller/dwc/pcie-keembay.c

<checks MAINTAINERS> ... yep, all previous entries are in alphabetical
order.  This new one just got dropped at the end.

I feel like a broken record, but please, please, take a look at the
surrounding code/text/whatever, and MAKE YOUR NEW STUFF MATCH THE
EXISTING STYLE.  We want the whole thing to be reasonably consistent
so readers can make sense of it without being confused by the
idiosyncrasies of every contributor.

Also, probably s/PCI DRIVER/PCIE DRIVER/.  We have both (an existing
inconsistency), but pick one, put it in the section that matches, and
alphabetize.

> +
>  PCMCIA SUBSYSTEM
>  M:	Dominik Brodowski <linux@dominikbrodowski.net>
>  S:	Odd Fixes

> +static void keembay_ep_reset_deassert(struct keembay_pcie *pcie)
> +{
> +	msleep(100);

Please note the spec section that requires this sleep.  Otherwise it's
just an unmaintainable magic number.

> +	gpiod_set_value_cansleep(pcie->reset, 0);
> +	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> +}