Re: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

On Wed, Dec 14, 2016 at 8:39 AM, Bartosz Folta [off-list ref] wrote:

There are hardware PCI implementations of Cadence GEM network
controller. This patch will allow to use such hardware with reuse of
existing Platform Driver.

Since it's already applied, perhaps you would consider addressing my
below comments as follow up clean up.

+ * macb_pci.c - Cadence GEM PCI wrapper.

If at some point file is renamed it will be a bit more work to change
this line. Perhaps drop file name?

+static int macb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+       int err;
+       struct platform_device *plat_dev;
+       struct platform_device_info plat_info;
+       struct macb_platform_data plat_data;
+       struct resource res[2];
+

+       /* sanity check */
+       if (!id)
+               return -EINVAL;

Is it even possible in modern kernels?

+
+       /* enable pci device */
+       err = pci_enable_device(pdev);

pcim_enable_device();

+       if (err < 0) {
+               dev_err(&pdev->dev, "Enabling PCI device has failed: 0x%04X",

%x for error code?! Confusing.

+                       err);

+               return -EACCES;

return err;

+       }
+
+       pci_set_master(pdev);
+
+       /* set up resources */
+       memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));

+       res[0].start = pdev->resource[0].start;
+       res[0].end = pdev->resource[0].end;

pci_resource_start()
pci_resource_end()

+       res[0].name = PCI_DRIVER_NAME;
+       res[0].flags = IORESOURCE_MEM;

+       res[1].start = pdev->irq;

Btw, does it support MSI? If so and are planning to use it,
pci_irq_...() API would help here.

+       res[1].name = PCI_DRIVER_NAME;
+       res[1].flags = IORESOURCE_IRQ;
+
+       dev_info(&pdev->dev, "EMAC physical base addr = 0x%p\n",

+                (void *)(uintptr_t)pci_resource_start(pdev, 0));

Oh, %pa

+
+       /* set up macb platform data */
+       memset(&plat_data, 0, sizeof(plat_data));
+
+       /* initialize clocks */
+       plat_data.pclk = clk_register_fixed_rate(&pdev->dev, "pclk", NULL, 0,
+                                                GEM_PCLK_RATE);
+       if (IS_ERR(plat_data.pclk)) {
+               err = PTR_ERR(plat_data.pclk);
+               goto err_pclk_register;
+       }
+
+       plat_data.hclk = clk_register_fixed_rate(&pdev->dev, "hclk", NULL, 0,
+                                                GEM_HCLK_RATE);
+       if (IS_ERR(plat_data.hclk)) {
+               err = PTR_ERR(plat_data.hclk);
+               goto err_hclk_register;
+       }
+
+       /* set up platform device info */
+       memset(&plat_info, 0, sizeof(plat_info));
+       plat_info.parent = &pdev->dev;
+       plat_info.fwnode = pdev->dev.fwnode;
+       plat_info.name = PLAT_DRIVER_NAME;
+       plat_info.id = pdev->devfn;
+       plat_info.res = res;
+       plat_info.num_res = ARRAY_SIZE(res);
+       plat_info.data = &plat_data;
+       plat_info.size_data = sizeof(plat_data);
+       plat_info.dma_mask = DMA_BIT_MASK(32);
+
+       /* register platform device */
+       plat_dev = platform_device_register_full(&plat_info);
+       if (IS_ERR(plat_dev)) {
+               err = PTR_ERR(plat_dev);
+               goto err_plat_dev_register;
+       }
+
+       pci_set_drvdata(pdev, plat_dev);
+
+       return 0;
+
+err_plat_dev_register:
+       clk_unregister(plat_data.hclk);
+
+err_hclk_register:
+       clk_unregister(plat_data.pclk);
+

+err_pclk_register:
+       pci_disable_device(pdev);

Redundant when use pcim_enable_device().

+       return err;
+}
+
+static void macb_remove(struct pci_dev *pdev)
+{
+       struct platform_device *plat_dev = pci_get_drvdata(pdev);
+       struct macb_platform_data *plat_data = dev_get_platdata(&plat_dev->dev);
+
+       platform_device_unregister(plat_dev);

+       pci_disable_device(pdev);

Ditto.

+       clk_unregister(plat_data->pclk);
+       clk_unregister(plat_data->hclk);
+}

-- 
With Best Regards,
Andy Shevchenko