From: Dan Carpenter <dan.carpenter@oracle.com>
To: Pawel Laszczak <pawell@cadence.com>
Cc: gregkh@linuxfoundation.org, robh+dt@kernel.org,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
balbi@kernel.org, devicetree@vger.kernel.org,
ben.dooks@codethink.co.uk, colin.king@canonical.com,
rogerq@ti.com, peter.chen@nxp.com, weiyongjun1@huawei.com,
jpawar@cadence.com, kurahul@cadene.com, sparmar@cadence.com
Subject: Re: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
Date: Fri, 26 Jun 2020 14:40:58 +0300 [thread overview]
Message-ID: <20200626114057.GD2571@kadam> (raw)
In-Reply-To: <20200626045450.10205-3-pawell@cadence.com>
On Fri, Jun 26, 2020 at 06:54:47AM +0200, Pawel Laszczak wrote:
> +static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
> +{
> + struct pci_dev *func;
> +
> + /*
> + * Gets the second function.
> + * It's little tricky, but this platform has two function.
> + * The fist keeps resources for Host/Device while the second
> + * keeps resources for DRD/OTG.
> + */
> + func = pci_get_device(pdev->vendor, pdev->device, NULL);
> + if (unlikely(!func))
Delete all likely/unlikely annotations. Likely and unlikely annotations
make the code less readable. We are willing to sacrifice readability on
fast paths.
They're only supposed to be used where they're supported by benchmarking.
Probably it's pretty tricky to benchmark probe. The other rule of thumb
is don't add them to drivers. Another thing to consider is that this
error path is probably obvious enough for the compiler to figure out
without help.
> + return NULL;
> +
> + if (func->devfn == pdev->devfn) {
> + func = pci_get_device(pdev->vendor, pdev->device, func);
> + if (unlikely(!func))
> + return NULL;
> + }
> +
> + return func;
> +}
> +
> +static int cdnsp_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct platform_device_info plat_info;
> + struct cdnsp_wrap *wrap;
> + struct resource *res;
> + struct pci_dev *func;
> + int err;
> +
> + /*
> + * For GADGET/HOST PCI (devfn) function number is 0,
> + * for OTG PCI (devfn) function number is 1.
> + */
> + if (!id || (pdev->devfn != PCI_DEV_FN_HOST_DEVICE &&
> + pdev->devfn != PCI_DEV_FN_OTG))
> + return -EINVAL;
> +
> + func = cdnsp_get_second_fun(pdev);
> + if (unlikely(!func))
> + return -EINVAL;
> +
> + if (func->class == PCI_CLASS_SERIAL_USB_XHCI ||
> + pdev->class == PCI_CLASS_SERIAL_USB_XHCI)
> + return -EINVAL;
Do we need call pci_put_device(func) before returning?
ret = -EINVAL;
goto put_pci;
> +
> + err = pcim_enable_device(pdev);
> + if (err) {
> + dev_err(&pdev->dev, "Enabling PCI device has failed %d\n", err);
> + return err;
goto put_pci;
> + }
> +
> + pci_set_master(pdev);
> +
> + if (pci_is_enabled(func)) {
> + wrap = pci_get_drvdata(func);
> + } else {
> + wrap = kzalloc(sizeof(*wrap), GFP_KERNEL);
> + if (!wrap) {
> + pci_disable_device(pdev);
> + return -ENOMEM;
goto disable_pci;
> + }
> + }
> +
> + res = wrap->dev_res;
> +
> + /* For GADGET device function number is 0. */
> + if (pdev->devfn == 0) {
> + /* Function 0: host(BAR_0) + device(BAR_1).*/
> + dev_dbg(&pdev->dev, "Initialize Device resources\n");
> + res[RES_DEV_ID].start = pci_resource_start(pdev, PCI_BAR_DEV);
> + res[RES_DEV_ID].end = pci_resource_end(pdev, PCI_BAR_DEV);
> + res[RES_DEV_ID].name = "dev";
> + res[RES_DEV_ID].flags = IORESOURCE_MEM;
> + dev_dbg(&pdev->dev, "USBSS-DEV physical base addr: %pa\n",
> + &res[RES_DEV_ID].start);
> +
> + res[RES_HOST_ID].start = pci_resource_start(pdev, PCI_BAR_HOST);
> + res[RES_HOST_ID].end = pci_resource_end(pdev, PCI_BAR_HOST);
> + res[RES_HOST_ID].name = "xhci";
> + res[RES_HOST_ID].flags = IORESOURCE_MEM;
> + dev_dbg(&pdev->dev, "USBSS-XHCI physical base addr: %pa\n",
> + &res[RES_HOST_ID].start);
> +
> + /* Interrupt for XHCI, */
> + wrap->dev_res[RES_IRQ_HOST_ID].start = pdev->irq;
> + wrap->dev_res[RES_IRQ_HOST_ID].name = "host";
> + wrap->dev_res[RES_IRQ_HOST_ID].flags = IORESOURCE_IRQ;
> +
> + /* Interrupt device. It's the same as for HOST. */
> + wrap->dev_res[RES_IRQ_PERIPHERAL_ID].start = pdev->irq;
> + wrap->dev_res[RES_IRQ_PERIPHERAL_ID].name = "peripheral";
> + wrap->dev_res[RES_IRQ_PERIPHERAL_ID].flags = IORESOURCE_IRQ;
> + } else {
> + res[RES_DRD_ID].start = pci_resource_start(pdev, PCI_BAR_OTG);
> + res[RES_DRD_ID].end = pci_resource_end(pdev, PCI_BAR_OTG);
> + res[RES_DRD_ID].name = "otg";
> + res[RES_DRD_ID].flags = IORESOURCE_MEM;
> + dev_dbg(&pdev->dev, "CDNSP-DRD physical base addr: %pa\n",
> + &res[RES_DRD_ID].start);
> +
> + /* Interrupt for OTG/DRD. */
> + wrap->dev_res[RES_IRQ_OTG_ID].start = pdev->irq;
> + wrap->dev_res[RES_IRQ_OTG_ID].name = "otg";
> + wrap->dev_res[RES_IRQ_OTG_ID].flags = IORESOURCE_IRQ;
> + }
> +
> + if (pci_is_enabled(func)) {
> + /* 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;
> + wrap->devfn = pdev->devfn;
> + plat_info.res = wrap->dev_res;
> + plat_info.num_res = ARRAY_SIZE(wrap->dev_res);
> + plat_info.dma_mask = pdev->dma_mask;
> + /* Register platform device. */
> + wrap->plat_dev = platform_device_register_full(&plat_info);
> + if (IS_ERR(wrap->plat_dev)) {
> + pci_disable_device(pdev);
> + err = PTR_ERR(wrap->plat_dev);
> + kfree(wrap);
err = PTR_ERR(wrap->plat_dev);
goto free_wrap;
Except, do we really want to kfree(wrap)? It looks like it came from
pci_get_drvdata().
> + return err;
> + }
> + }
> +
> + pci_set_drvdata(pdev, wrap);
> + return err;
free_wrap:
if (!pci_is_enabled(func))
kfree(wrap);
disable_pci:
pci_disable_device(pdev);
put_pci:
pci_put_device(func);
> +}
> +
> +static void cdnsp_pci_remove(struct pci_dev *pdev)
> +{
> + struct cdnsp_wrap *wrap;
> + struct pci_dev *func;
> +
> + func = cdnsp_get_second_fun(pdev);
> +
> + wrap = (struct cdnsp_wrap *)pci_get_drvdata(pdev);
> + if (wrap->devfn == pdev->devfn)
> + platform_device_unregister(wrap->plat_dev);
> +
> + if (!pci_is_enabled(func))
> + kfree(wrap);
pci_put_device(func);
> +}
> +
regards,
dan carpenter
next prev parent reply other threads:[~2020-06-26 11:41 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-26 4:54 [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver Pawel Laszczak
2020-06-26 4:54 ` [PATCH RFC 1/5] dt-bindings: add binding for CDNSP-DRD controller Pawel Laszczak
2020-06-29 22:03 ` Rob Herring
2020-06-30 8:19 ` Pawel Laszczak
2020-06-26 4:54 ` [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper Pawel Laszczak
2020-06-26 11:40 ` Dan Carpenter [this message]
2020-06-26 15:10 ` Pawel Laszczak
2020-06-26 17:24 ` Dan Carpenter
2020-06-29 6:51 ` Pawel Laszczak
2020-06-26 13:43 ` Greg KH
2020-06-29 11:35 ` Pawel Laszczak
2020-06-29 11:43 ` Greg KH
2020-06-26 4:54 ` [PATCH RFC 3/5] usb: cdnsp: Device side header file for CDNSP driver Pawel Laszczak
2020-06-26 4:54 ` [PATCH RFC 4/5] usb: cdnsp: usb:cdns3 Add main part of Cadence USBSSP DRD Driver Pawel Laszczak
2020-06-26 9:04 ` kernel test robot
2020-06-29 10:16 ` Dan Carpenter
2020-07-02 3:48 ` Pawel Laszczak
2020-06-26 4:54 ` [PATCH RFC 5/5] usb: cdnsp: Add tracepoints for CDNSP driver Pawel Laszczak
2020-06-26 6:52 ` [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver Felipe Balbi
2020-06-26 7:19 ` Pawel Laszczak
2020-06-29 3:41 ` Peter Chen
2020-06-29 4:31 ` gregkh
2020-06-29 11:20 ` Pawel Laszczak
2020-06-29 11:43 ` gregkh
2020-09-22 13:06 ` Pawel Laszczak
2020-09-23 2:56 ` Peter Chen
2020-09-23 3:37 ` Pawel Laszczak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200626114057.GD2571@kadam \
--to=dan.carpenter@oracle.com \
--cc=balbi@kernel.org \
--cc=ben.dooks@codethink.co.uk \
--cc=colin.king@canonical.com \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jpawar@cadence.com \
--cc=kurahul@cadene.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=pawell@cadence.com \
--cc=peter.chen@nxp.com \
--cc=robh+dt@kernel.org \
--cc=rogerq@ti.com \
--cc=sparmar@cadence.com \
--cc=weiyongjun1@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.