Re: [RFC v3 1/7] platform/x86: intel_pmc_ipc: Use devm_* calls in driver probe function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,


On 10/01/2017 07:34 AM, Andy Shevchenko wrote:
On Tue, Sep 5, 2017 at 8:37 AM,
<sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> wrote:
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>

This patch cleans up unnecessary free/alloc calls in ipc_plat_probe(),
ipc_pci_probe() and ipc_plat_get_res() functions by using devm_*
calls.

This patch also adds proper error handling for failure cases in
ipc_pci_probe() function.

Pushed (this patch only) to my review queue with slight style changes, thanks.
Thanks Andy. Will rebase my rest of the patches on top of this patch.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
---
  drivers/platform/x86/intel_pmc_ipc.c | 104 ++++++++++++-----------------------
  1 file changed, 34 insertions(+), 70 deletions(-)

Changes since v2:
  * Used pcim_* device managed functions.

Changes since v1:
  * Merged devm_* related changes into a single function.
  * Instead of removing free_irq, use devm_free_irq function.

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index bb792a5..5eef649 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -480,52 +480,39 @@ static irqreturn_t ioc(int irq, void *dev_id)

  static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  {
-       resource_size_t pci_resource;
         int ret;
-       int len;
+       struct intel_pmc_ipc_dev *pmc = &ipcdev;

-       ipcdev.dev = &pci_dev_get(pdev)->dev;
-       ipcdev.irq_mode = IPC_TRIGGER_MODE_IRQ;
+       /* Only one PMC is supported */
+       if (pmc->dev)
+               return -EBUSY;

-       ret = pci_enable_device(pdev);
+       pmc->irq_mode = IPC_TRIGGER_MODE_IRQ;
+
+       ret = pcim_enable_device(pdev);
         if (ret)
                 return ret;

-       ret = pci_request_regions(pdev, "intel_pmc_ipc");
+       ret = pcim_iomap_regions(pdev, 1 << 0, pci_name(pdev));
         if (ret)
                 return ret;

-       pci_resource = pci_resource_start(pdev, 0);
-       len = pci_resource_len(pdev, 0);
-       if (!pci_resource || !len) {
-               dev_err(&pdev->dev, "Failed to get resource\n");
-               return -ENOMEM;
-       }
+       init_completion(&pmc->cmd_complete);

-       init_completion(&ipcdev.cmd_complete);
+       pmc->ipc_base =  pcim_iomap_table(pdev)[0];

-       if (request_irq(pdev->irq, ioc, 0, "intel_pmc_ipc", &ipcdev)) {
+       ret = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_pmc_ipc",
+                               pmc);
+       if (ret) {
                 dev_err(&pdev->dev, "Failed to request irq\n");
-               return -EBUSY;
+               return ret;
         }

-       ipcdev.ipc_base = ioremap_nocache(pci_resource, len);
-       if (!ipcdev.ipc_base) {
-               dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
-               free_irq(pdev->irq, &ipcdev);
-               ret = -ENOMEM;
-       }
+       pmc->dev = &pdev->dev;

-       return ret;
-}
+       pci_set_drvdata(pdev, pmc);

-static void ipc_pci_remove(struct pci_dev *pdev)
-{
-       free_irq(pdev->irq, &ipcdev);
-       pci_release_regions(pdev);
-       pci_dev_put(pdev);
-       iounmap(ipcdev.ipc_base);
-       ipcdev.dev = NULL;
+       return 0;
  }

  static const struct pci_device_id ipc_pci_ids[] = {
@@ -540,7 +527,6 @@ 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,
@@ -849,18 +835,16 @@ static int ipc_plat_get_res(struct platform_device *pdev)
                 dev_err(&pdev->dev, "Failed to get ipc resource\n");
                 return -ENXIO;
         }
-       size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;
+       res->end = (res->start + PLAT_RESOURCE_IPC_SIZE +
+                   PLAT_RESOURCE_GCR_SIZE - 1);

-       if (!request_mem_region(res->start, size, pdev->name)) {
-               dev_err(&pdev->dev, "Failed to request ipc resource\n");
-               return -EBUSY;
-       }
-       addr = ioremap_nocache(res->start, size);
-       if (!addr) {
-               dev_err(&pdev->dev, "I/O memory remapping failed\n");
-               release_mem_region(res->start, size);
-               return -ENOMEM;
+       addr = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(addr)) {
+               dev_err(&pdev->dev,
+                       "PMC I/O memory remapping failed\n");
+               return PTR_ERR(addr);
         }
+
         ipcdev.ipc_base = addr;

         ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
@@ -917,7 +901,6 @@ MODULE_DEVICE_TABLE(acpi, ipc_acpi_ids);

  static int ipc_plat_probe(struct platform_device *pdev)
  {
-       struct resource *res;
         int ret;

         ipcdev.dev = &pdev->dev;
@@ -939,61 +922,42 @@ static int ipc_plat_probe(struct platform_device *pdev)
         ret = ipc_create_pmc_devices();
         if (ret) {
                 dev_err(&pdev->dev, "Failed to create pmc devices\n");
-               goto err_device;
+               return ret;
         }

-       if (request_irq(ipcdev.irq, ioc, IRQF_NO_SUSPEND,
-                       "intel_pmc_ipc", &ipcdev)) {
+       if (devm_request_irq(&pdev->dev, ipcdev.irq, ioc, IRQF_NO_SUSPEND,
+                            "intel_pmc_ipc", &ipcdev)) {
                 dev_err(&pdev->dev, "Failed to request irq\n");
                 ret = -EBUSY;
-               goto err_irq;
+               goto unregister_devices;
         }

         ret = sysfs_create_group(&pdev->dev.kobj, &intel_ipc_group);
         if (ret) {
                 dev_err(&pdev->dev, "Failed to create sysfs group %d\n",
                         ret);
-               goto err_sys;
+               goto unregister_devices;
         }

         ipcdev.has_gcr_regs = true;

         return 0;
-err_sys:
-       free_irq(ipcdev.irq, &ipcdev);
-err_irq:
+
+unregister_devices:
         platform_device_unregister(ipcdev.tco_dev);
         platform_device_unregister(ipcdev.punit_dev);
         platform_device_unregister(ipcdev.telemetry_dev);
-err_device:
-       iounmap(ipcdev.ipc_base);
-       res = platform_get_resource(pdev, IORESOURCE_MEM,
-                                   PLAT_RESOURCE_IPC_INDEX);
-       if (res) {
-               release_mem_region(res->start,
-                                  PLAT_RESOURCE_IPC_SIZE +
-                                  PLAT_RESOURCE_GCR_SIZE);
-       }
+
         return ret;
  }

  static int ipc_plat_remove(struct platform_device *pdev)
  {
-       struct resource *res;
-
         sysfs_remove_group(&pdev->dev.kobj, &intel_ipc_group);
-       free_irq(ipcdev.irq, &ipcdev);
+       devm_free_irq(&pdev->dev, ipcdev.irq, &ipcdev);
         platform_device_unregister(ipcdev.tco_dev);
         platform_device_unregister(ipcdev.punit_dev);
         platform_device_unregister(ipcdev.telemetry_dev);
-       iounmap(ipcdev.ipc_base);
-       res = platform_get_resource(pdev, IORESOURCE_MEM,
-                                   PLAT_RESOURCE_IPC_INDEX);
-       if (res) {
-               release_mem_region(res->start,
-                                  PLAT_RESOURCE_IPC_SIZE +
-                                  PLAT_RESOURCE_GCR_SIZE);
-       }
         ipcdev.dev = NULL;
         return 0;
  }
--
2.7.4




--
Sathyanarayanan Kuppuswamy
Linux kernel developer




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux