RE: [RFC PATCH] usb: host: xhci: plat: add support for otg_set_host() call

[Manish Narani]

Thanks Greg. I have send another patch with changes as per your suggestions. Please review it and let me know if you have any concerns.

- Manish

-----Original Message-----
From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
Sent: Thursday, December 15, 2016 2:29 AM
To: Manish Narani <MNARANI@xxxxxxxxxx>
Cc: mathias.nyman@xxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Punnaiah Choudary Kalluri <punnaia@xxxxxxxxxx>; Anirudha Sarangi <anirudh@xxxxxxxxxx>; Manish Narani <MNARANI@xxxxxxxxxx>
Subject: Re: [RFC PATCH] usb: host: xhci: plat: add support for otg_set_host() call

On Thu, Dec 15, 2016 at 12:25:08AM +0530, Manish Narani wrote:
> This patch will add support for OTG host initialization. This will
> help OTG drivers to populate their host subsystem.
>
> Signed-off-by: Manish Narani <mnarani@xxxxxxxxxx>
> ---
>  drivers/usb/host/xhci-plat.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c
> b/drivers/usb/host/xhci-plat.c index ddfab30..b4cadbd 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -20,6 +20,10 @@
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
>
> +#ifdef CONFIG_USB_OTG
> +#include <linux/usb/otg.h>
> +#endif

never use a #ifdef in a .c file if at all possible.  Here you don't need it at all.

> +
>  #include "xhci.h"
>  #include "xhci-plat.h"
>  #include "xhci-mvebu.h"
> @@ -255,6 +259,24 @@ static int xhci_plat_probe(struct platform_device *pdev)
>       if (ret)
>               goto dealloc_usb2_hcd;
>
> +#ifdef CONFIG_USB_OTG
> +     hcd->usb_phy = usb_get_phy(USB_PHY_TYPE_USB3);
> +     if (!IS_ERR_OR_NULL(hcd->usb_phy) && hcd->usb_phy->otg) {
> +             dev_dbg(&pdev->dev, "%s otg support available\n", __func__);
> +             ret = otg_set_host(hcd->usb_phy->otg, &hcd->self);
> +             if (ret) {
> +                     dev_err(&pdev->dev, "%s otg_set_host failed\n",
> +                                     __func__);
> +                     usb_put_phy(hcd->usb_phy);
> +                     hcd->usb_phy = NULL;
> +                     goto dealloc_usb2_hcd;
> +             }
> +     } else {
> +             usb_put_phy(hcd->usb_phy);
> +             hcd->usb_phy = NULL;
> +     }
> +#endif

Can't you wrap this in a function to get rid of this #ifdef mess?

> +
>       return 0;
>
>
> @@ -283,6 +305,19 @@ static int xhci_plat_remove(struct platform_device *dev)
>       struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>       struct clk *clk = xhci->clk;
>
> +#ifdef CONFIG_USB_OTG
> +     if (hcd->usb_phy) {
> +             if (!IS_ERR(hcd->usb_phy)) {
> +                     if (hcd->usb_phy->otg)
> +                             otg_set_host(hcd->usb_phy->otg, NULL);
> +                     usb_put_phy(hcd->usb_phy);
> +             }
> +             hcd->usb_phy = NULL;
> +             if (xhci->shared_hcd)
> +                     xhci->shared_hcd->usb_phy = NULL;
> +     }
> +#endif

same here.

thanks,

greg k-h


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.