Re: [PATCH 4/5] USB: OTG: msm: Configure PHY Analog and Digital voltage domains

[Sergei Shtylyov <sshtylyov@mvista.com>]

On 28.04.2011 12:01, Pavankumar Kondeti wrote:

> From: Anji jonnala <anjir@codeaurora.org>

> Signed-off-by: Anji jonnala <anjir@codeaurora.org>
> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
[...]

> diff --git a/drivers/usb/otg/msm_otg.c b/drivers/usb/otg/msm_otg.c
> index 526a650..425418d 100644
> --- a/drivers/usb/otg/msm_otg.c
> +++ b/drivers/usb/otg/msm_otg.c
> @@ -38,6 +38,7 @@
>   #include<linux/usb/hcd.h>
>   #include<linux/usb/msm_hsusb.h>
>   #include<linux/usb/msm_hsusb_hw.h>
> +#include<linux/regulator/consumer.h>
>
>   #include<mach/clk.h>
>
> @@ -45,6 +46,175 @@
>   #define DRIVER_NAME	"msm_otg"
>
>   #define ULPI_IO_TIMEOUT_USEC	(10 * 1000)
> +
> +#define USB_PHY_3P3_VOL_MIN	3050000 /* uV */
> +#define USB_PHY_3P3_VOL_MAX	3300000 /* uV */
> +#define USB_PHY_3P3_HPM_LOAD	50000	/* uA */
> +#define USB_PHY_3P3_LPM_LOAD	4000	/* uA */
> +
> +#define USB_PHY_1P8_VOL_MIN	1800000 /* uV */
> +#define USB_PHY_1P8_VOL_MAX	1800000 /* uV */
> +#define USB_PHY_1P8_HPM_LOAD	50000	/* uA */
> +#define USB_PHY_1P8_LPM_LOAD	4000	/* uA */
> +
> +#define USB_PHY_VDD_DIG_VOL_MIN	1000000 /* uV */
> +#define USB_PHY_VDD_DIG_VOL_MAX	1320000 /* uV */
> +
> +static struct regulator *hsusb_3p3;
> +static struct regulator *hsusb_1p8;
> +static struct regulator *hsusb_vddcx;
> +
> +static int msm_hsusb_init_vddcx(struct msm_otg *motg, int init)
> +{
> +	int ret = 0;
> +
> +	if (init) {
> +		hsusb_vddcx = regulator_get(motg->otg.dev, "HSUSB_VDDCX");
> +		if (IS_ERR(hsusb_vddcx)) {
> +			dev_err(motg->otg.dev, "unable to get hsusb vddcx\n");
> +			return PTR_ERR(hsusb_vddcx);
> +		}
> +
> +		ret = regulator_set_voltage(hsusb_vddcx,
> +				USB_PHY_VDD_DIG_VOL_MIN,
> +				USB_PHY_VDD_DIG_VOL_MAX);
> +		if (ret) {
> +			dev_err(motg->otg.dev, "unable to set the voltage"
> +					"for hsusb vddcx\n");
> +			regulator_put(hsusb_vddcx);
> +			return ret;
> +		}
> +
> +		ret = regulator_enable(hsusb_vddcx);
> +		if (ret) {
> +			dev_err(motg->otg.dev, "unable to enable hsusb vddcx\n");
> +			regulator_put(hsusb_vddcx);
> +		}
> +	} else {
> +		ret = regulator_set_voltage(hsusb_vddcx, 0,
> +			USB_PHY_VDD_DIG_VOL_MIN);
> +		if (ret) {
> +			dev_err(motg->otg.dev, "unable to set the voltage"
> +					"for hsusb vddcx\n");
> +			return ret;
> +		}
> +		ret = regulator_disable(hsusb_vddcx);
> +		if (ret) {
> +			dev_err(motg->otg.dev, "unable to disable hsusb vddcx\n");
> +			return ret;

    I suspect you should call regulator_put() on error cases as well...

> +		}
> +
> +		regulator_put(hsusb_vddcx);
> +	}
> +
> +	return ret;
> +}
> +
> +static int msm_hsusb_ldo_init(struct msm_otg *motg, int init)
> +{
> +	int rc = 0;
> +
> +	if (init) {
> +		hsusb_3p3 = regulator_get(motg->otg.dev, "HSUSB_3p3");
> +		if (IS_ERR(hsusb_3p3)) {
> +			dev_err(motg->otg.dev, "unable to get hsusb 3p3\n");
> +			return PTR_ERR(hsusb_3p3);
> +		}
> +
> +		rc = regulator_set_voltage(hsusb_3p3, USB_PHY_3P3_VOL_MIN,
> +				USB_PHY_3P3_VOL_MAX);
> +		if (rc) {
> +			dev_err(motg->otg.dev, "unable to set voltage level for"
> +					"hsusb 3p3\n");
> +			goto put_3p3;
> +		}
> +		rc = regulator_enable(hsusb_3p3);
> +		if (rc) {
> +			dev_err(motg->otg.dev, "unable to enable the hsusb 3p3\n");
> +			goto put_3p3_lpm;
> +		}
> +		hsusb_1p8 = regulator_get(motg->otg.dev, "HSUSB_1p8");
> +		if (IS_ERR(hsusb_1p8)) {
> +			dev_err(motg->otg.dev, "unable to get hsusb 1p8\n");
> +			rc = PTR_ERR(hsusb_1p8);
> +			goto put_3p3_lpm;
> +		}
> +		rc = regulator_set_voltage(hsusb_1p8, USB_PHY_1P8_VOL_MIN,
> +				USB_PHY_1P8_VOL_MAX);
> +		if (rc) {
> +			dev_err(motg->otg.dev, "unable to set voltage level for"
> +					"hsusb 1p8\n");
> +			goto put_1p8;
> +		}
> +		rc = regulator_enable(hsusb_1p8);
> +		if (rc) {
> +			dev_err(motg->otg.dev, "unable to enable the hsusb 1p8\n");
> +			goto disable_1p8;
> +		}
> +
> +		return 0;
> +	}
> +
> +disable_1p8:
> +	regulator_set_voltage(hsusb_1p8, 0, USB_PHY_1P8_VOL_MAX);
> +	regulator_disable(hsusb_1p8);

    Why call regualator_disable() if regulator_enable() has just failed?

> +put_1p8:
> +	regulator_put(hsusb_1p8);
> +put_3p3_lpm:
> +	regulator_set_voltage(hsusb_3p3, 0, USB_PHY_3P3_VOL_MAX);
> +put_3p3:
> +	regulator_put(hsusb_3p3);
> +	return rc;
> +}
[...]
> @@ -1345,6 +1532,10 @@ free_irq:
>   disable_clks:
>   	clk_disable(motg->pclk);
>   	clk_disable(motg->clk);
> +free_ldo_init:

    'ldo_exit' perhaps?

> +	msm_hsusb_ldo_init(motg, 0);
> +free_config_vddcx:

    'vddcx_exit' perhaps?

> +	msm_hsusb_init_vddcx(motg, 0);
>   free_regs:
>   	iounmap(motg->regs);
>   put_core_clk:

WBR, Sergei