Re: [PATCH v2] usb: phy: msm: Unregister VBUS and ID events notifiers

[Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>]

On 09/07/2015 01:07 AM, Ivan T. Ivanov wrote:
> Right now even if driver failed to probe extcon framework will
> still deliver its VBUS and ID events, which will lead to random
> exception codes.
> 
> Fix this by removing VBUS and ID events notifiers when probe fail.
> 
> Fixes: 591fc116f330 ("usb: phy: msm: Use extcon framework for VBUS and ID detection")
> 
> Reported-by: Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> Signed-off-by: Ivan T. Ivanov <ivan.ivanov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> 
> Patch reworked to use new extcon API.
> 
>  drivers/usb/phy/phy-msm-usb.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
> index c58c3c0dbe35..0fd21c147c7e 100644
> --- a/drivers/usb/phy/phy-msm-usb.c
> +++ b/drivers/usb/phy/phy-msm-usb.c
> @@ -1598,6 +1598,8 @@ static int msm_otg_read_dt(struct platform_device *pdev, struct msm_otg *motg)
>  		ret = extcon_register_notifier(ext_id, EXTCON_USB_HOST,
>  						&motg->id.nb);
>  		if (ret < 0) {
> +			extcon_unregister_notifier(motg->vbus.extcon,
> +						   EXTCON_USB, &motg->vbus.nb);
>  			dev_err(&pdev->dev, "register ID notifier failed\n");
>  			return ret;
>  		}
> @@ -1660,15 +1662,6 @@ static int msm_otg_probe(struct platform_device *pdev)
>  	if (!motg)
>  		return -ENOMEM;
> 
> -	pdata = dev_get_platdata(&pdev->dev);
> -	if (!pdata) {
> -		if (!np)
> -			return -ENXIO;
> -		ret = msm_otg_read_dt(pdev, motg);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	motg->phy.otg = devm_kzalloc(&pdev->dev, sizeof(struct usb_otg),
>  				     GFP_KERNEL);
>  	if (!motg->phy.otg)
> @@ -1731,6 +1724,15 @@ static int msm_otg_probe(struct platform_device *pdev)
>  		return motg->irq;
>  	}
> 
> +	pdata = dev_get_platdata(&pdev->dev);
> +	if (!pdata) {
> +		if (!np)
> +			return -ENXIO;
> +		ret = msm_otg_read_dt(pdev, motg);
> +		if (ret)
> +			return ret;
> +	}
> +

The following code depends on msm_otg_read_dt(), but the
movement of the call puts the phy_number test before msm_otg_read_dt().

        /*
         * NOTE: The PHYs can be multiplexed between the chipidea controller
         * and the dwc3 controller, using a single bit. It is important that
         * the dwc3 driver does not set this bit in an incompatible way.
         */
        if (motg->phy_number) {
                phy_select = devm_ioremap_nocache(&pdev->dev, USB2_PHY_SEL, 4);
                if (!phy_select)
                        return -ENOMEM;
                /* Enable second PHY with the OTG port */
                writel(0x1, phy_select);
        }

Can you please move the motg->phy_number code after the msm_otg_read_dt() call?
(right here, in the patch, would be good :-)

>  	regs[0].supply = "vddcx";
>  	regs[1].supply = "v3p3";
>  	regs[2].supply = "v1p8";
> @@ -1834,6 +1836,11 @@ disable_clks:
>  	clk_disable_unprepare(motg->clk);
>  	if (!IS_ERR(motg->core_clk))
>  		clk_disable_unprepare(motg->core_clk);
> +
> +	extcon_unregister_notifier(motg->id.extcon,
> +				   EXTCON_USB_HOST, &motg->id.nb);
> +	extcon_unregister_notifier(motg->vbus.extcon,
> +				   EXTCON_USB, &motg->vbus.nb);
>  	return ret;
>  }

Other than that - it looks good.
 -- Tim

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html