Re: [PATCH v2 32/36] platform/x86: intel_pmc_ipc: Move PCI IDs to intel_scu_pcidrv.c

[Andy Shevchenko]

On Wed, Jan 08, 2020 at 02:41:57PM +0300, Mika Westerberg wrote:
> The PCI probe driver in intel_pmc_ipc.c is a duplicate of what we
> already have in intel_scu_pcidrv.c with the exception that the later also
> creates SCU specific devices. Move the PCI IDs from the intel_pmc_ipc.c
> to intel_scu.c and use driver_data to detect whether SCU devices need to
> be created or not.
> 
> Also update Kconfig entry to mention all platforms supported by the
> Intel SCU PCI driver.

One comment below. After addressing,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

> 
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
>  drivers/platform/x86/Kconfig            | 13 +++--
>  drivers/platform/x86/intel_pmc_ipc.c    | 73 +------------------------
>  drivers/platform/x86/intel_scu_pcidrv.c | 21 +++++--
>  3 files changed, 27 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 797683c5d005..1c5afb9e4965 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -994,13 +994,18 @@ config INTEL_SCU
>  
>  config INTEL_SCU_PCI
>  	bool "Intel SCU PCI driver"
> -	depends on X86_INTEL_MID
> +	depends on X86_INTEL_MID || PCI

Dependency on PCI is much more generic than Intel MID one. I think we may drop
X86_INTEL_MID here completely -- less users of it better.

>  	select INTEL_SCU
>  	help
>  	  SCU is used to bridge the communications between kernel and
>  	  SCU on some embedded Intel x86 platforms. It also creates
> -	  devices that are connected to the SoC through the SCU. This is
> -	  not needed for PC-type machines.
> +	  devices that are connected to the SoC through the SCU.
> +	  Platforms supported:
> +	    Medfield
> +	    Clovertrail
> +	    Merrifield
> +	    Broxton
> +	    Apollo Lake
>  
>  config INTEL_SCU_IPC_UTIL
>  	tristate "Intel SCU IPC utility driver"
> @@ -1192,7 +1197,7 @@ config INTEL_SMARTCONNECT
>  
>  config INTEL_PMC_IPC
>  	tristate "Intel PMC IPC Driver"
> -	depends on ACPI && PCI
> +	depends on ACPI
>  	select INTEL_SCU_IPC
>  	---help---
>  	This driver provides support for PMC control on some Intel platforms.
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 241bce603183..acec1c6d2069 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -17,7 +17,6 @@
>  #include <linux/interrupt.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/module.h>
> -#include <linux/pci.h>
>  #include <linux/platform_device.h>
>  
>  #include <asm/intel_pmc_ipc.h>
> @@ -194,62 +193,6 @@ static int update_no_reboot_bit(void *priv, bool set)
>  				    PMC_CFG_NO_REBOOT_MASK, value);
>  }
>  
> -static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> -{
> -	struct intel_pmc_ipc_dev *pmc = &ipcdev;
> -	struct intel_scu_ipc_pdata pdata;
> -	struct intel_scu_ipc_dev *scu;
> -	int ret;
> -
> -	/* Only one PMC is supported */
> -	if (pmc->dev)
> -		return -EBUSY;
> -
> -	memset(&pdata, 0, sizeof(pdata));
> -	spin_lock_init(&ipcdev.gcr_lock);
> -
> -	ret = pcim_enable_device(pdev);
> -	if (ret)
> -		return ret;
> -
> -	ret = pcim_iomap_regions(pdev, 1 << 0, pci_name(pdev));
> -	if (ret)
> -		return ret;
> -
> -	pdata.ipc_regs = pcim_iomap_table(pdev)[0];
> -
> -	scu = intel_scu_ipc_probe(&pdev->dev, &pdata);
> -	if (IS_ERR(scu))
> -		return PTR_ERR(scu);
> -
> -	pmc->dev = &pdev->dev;
> -
> -	pci_set_drvdata(pdev, scu);
> -
> -	return 0;
> -}
> -
> -static void ipc_pci_remove(struct pci_dev *pdev)
> -{
> -	intel_scu_ipc_remove(pci_get_drvdata(pdev));
> -	ipcdev.dev = NULL;
> -}
> -
> -static const struct pci_device_id ipc_pci_ids[] = {
> -	{PCI_VDEVICE(INTEL, 0x0a94), 0},
> -	{PCI_VDEVICE(INTEL, 0x1a94), 0},
> -	{PCI_VDEVICE(INTEL, 0x5a94), 0},
> -	{ 0,}
> -};
> -MODULE_DEVICE_TABLE(pci, ipc_pci_ids);
> -
> -static struct pci_driver ipc_pci_driver = {
> -	.name = "intel_pmc_ipc",
> -	.id_table = ipc_pci_ids,
> -	.probe = ipc_pci_probe,
> -	.remove = ipc_pci_remove,
> -};
> -
>  static ssize_t intel_pmc_ipc_simple_cmd_store(struct device *dev,
>  					      struct device_attribute *attr,
>  					      const char *buf, size_t count)
> @@ -697,25 +640,11 @@ static struct platform_driver ipc_plat_driver = {
>  
>  static int __init intel_pmc_ipc_init(void)
>  {
> -	int ret;
> -
> -	ret = platform_driver_register(&ipc_plat_driver);
> -	if (ret) {
> -		pr_err("Failed to register PMC ipc platform driver\n");
> -		return ret;
> -	}
> -	ret = pci_register_driver(&ipc_pci_driver);
> -	if (ret) {
> -		pr_err("Failed to register PMC ipc pci driver\n");
> -		platform_driver_unregister(&ipc_plat_driver);
> -		return ret;
> -	}
> -	return ret;
> +	return platform_driver_register(&ipc_plat_driver);
>  }
>  
>  static void __exit intel_pmc_ipc_exit(void)
>  {
> -	pci_unregister_driver(&ipc_pci_driver);
>  	platform_driver_unregister(&ipc_plat_driver);
>  }
>  
> diff --git a/drivers/platform/x86/intel_scu_pcidrv.c b/drivers/platform/x86/intel_scu_pcidrv.c
> index 42030bdb3e08..4f2a7ca5c5f7 100644
> --- a/drivers/platform/x86/intel_scu_pcidrv.c
> +++ b/drivers/platform/x86/intel_scu_pcidrv.c
> @@ -17,6 +17,7 @@
>  static int intel_scu_pci_probe(struct pci_dev *pdev,
>  			       const struct pci_device_id *id)
>  {
> +	void (*setup_fn)(void) = (void (*)(void))id->driver_data;
>  	struct intel_scu_ipc_pdata *pdata;
>  	struct intel_scu_ipc_dev *scu;
>  	int ret;
> @@ -40,14 +41,26 @@ static int intel_scu_pci_probe(struct pci_dev *pdev,
>  	if (IS_ERR(scu))
>  		return PTR_ERR(scu);
>  
> -	intel_scu_devices_create();
> +	if (setup_fn)
> +		setup_fn();
>  	return 0;
>  }
>  
> +static void intel_mid_scu_setup(void)
> +{
> +	intel_scu_devices_create();
> +}
> +
>  static const struct pci_device_id pci_ids[] = {
> -	{ PCI_VDEVICE(INTEL, 0x080e) },
> -	{ PCI_VDEVICE(INTEL, 0x08ea) },
> -	{ PCI_VDEVICE(INTEL, 0x11a0) },
> +	{ PCI_VDEVICE(INTEL, 0x080e),
> +	  .driver_data = (kernel_ulong_t)intel_mid_scu_setup },
> +	{ PCI_VDEVICE(INTEL, 0x08ea),
> +	  .driver_data = (kernel_ulong_t)intel_mid_scu_setup },
> +	{ PCI_VDEVICE(INTEL, 0x0a94) },
> +	{ PCI_VDEVICE(INTEL, 0x11a0),
> +	  .driver_data = (kernel_ulong_t)intel_mid_scu_setup },
> +	{ PCI_VDEVICE(INTEL, 0x1a94) },
> +	{ PCI_VDEVICE(INTEL, 0x5a94) },
>  	{}
>  };
>  
> -- 
> 2.24.1
> 

-- 
With Best Regards,
Andy Shevchenko