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

[Greg KH]

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