Re: [PATCH V7 2/4] PCI: tegra: Add loadable kernel module support

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3969 bytes --]

On Tue, Jan 30, 2018 at 10:54:09AM +0530, Manikanta Maddireddy wrote:
> Implement remove callback function for Tegra PCIe driver to add
> loadable kernel module support. Change PCI_TEGRA config to tristate to
> allow pci-tegra driver to be build as a module.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> ---
> V2:
> * no change in this patch
> V3:
> * use tegra_pcie_debugfs_exit() helper function in tegra_pcie_debugfs_init()
> V4:
> * no change in this patch
> V5:
> * Decoupled from https://patchwork.ozlabs.org/patch/832053/ and
> rebased on linux-next
> V6:
> * no change in this patch
> V7:
> * no change in this patch
> 
>  drivers/pci/host/Kconfig     |  2 +-
>  drivers/pci/host/pci-tegra.c | 34 +++++++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 38d12980db0f..6fd2a5937804 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -34,7 +34,7 @@ config PCI_FTPCI100
>  	default ARCH_GEMINI
>  
>  config PCI_TEGRA
> -	bool "NVIDIA Tegra PCIe controller"
> +	tristate "NVIDIA Tegra PCIe controller"
>  	depends on ARCH_TEGRA
>  	help
>  	  Say Y here if you want support for the PCIe host controller found
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index e68507f658d8..981f126b14d6 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -35,6 +35,7 @@
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/msi.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
> @@ -2276,6 +2277,12 @@ static const struct file_operations tegra_pcie_ports_ops = {
>  	.release = seq_release,
>  };
>  
> +static void tegra_pcie_debugfs_exit(struct tegra_pcie *pcie)
> +{
> +	debugfs_remove_recursive(pcie->debugfs);
> +	pcie->debugfs = NULL;
> +}
> +
>  static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
>  {
>  	struct dentry *file;
> @@ -2292,8 +2299,7 @@ static int tegra_pcie_debugfs_init(struct tegra_pcie *pcie)
>  	return 0;
>  
>  remove:
> -	debugfs_remove_recursive(pcie->debugfs);
> -	pcie->debugfs = NULL;
> +	tegra_pcie_debugfs_exit(pcie);
>  	return -ENOMEM;
>  }
>  
> @@ -2311,6 +2317,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  
>  	pcie = pci_host_bridge_priv(host);
>  	host->sysdata = pcie;
> +	platform_set_drvdata(pdev, pcie);
>  
>  	pcie->soc = of_device_get_match_data(dev);
>  	INIT_LIST_HEAD(&pcie->ports);
> @@ -2388,6 +2395,25 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  	return err;
>  }
>  
> +static int tegra_pcie_remove(struct platform_device *pdev)
> +{
> +	struct tegra_pcie *pcie = platform_get_drvdata(pdev);
> +	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
> +
> +	if (IS_ENABLED(CONFIG_DEBUG_FS))
> +		tegra_pcie_debugfs_exit(pcie);
> +	pci_stop_root_bus(host->bus);
> +	pci_remove_root_bus(host->bus);
> +	tegra_pcie_disable_ports(pcie);
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		tegra_pcie_disable_msi(pcie);
> +	tegra_pcie_free_resources(pcie);
> +	tegra_pcie_disable_controller(pcie);
> +	tegra_pcie_put_resources(pcie);

I prefer blank lines around conditional blocks for readability, so the
above would be:

	if (IS_ENABLED(CONFIG_DEBUG_FS))
		tegra_pcie_debugfs_exit(pcie);

	pci_stop_root_bus(host->bus);
	pci_remove_root_bus(host->bus);
	tegra_pcie_disable_ports(pcie);

	if (IS_ENABLED(CONFIG_PCI_MSI))
		tegra_pcie_disable_msi(pcie);

	tegra_pcie_free_resources(pcie);
	tegra_pcie_disable_controller(pcie);
	tegra_pcie_put_resources(pcie);

Not sure if Lorenzo or Bjorn have any specific preferences, though. My
Acked-by remains valid, though, since this is nitpicky.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]