From mboxrd@z Thu Jan  1 00:00:00 1970
From: Pavan Kondeti <pkondeti@codeaurora.org>
Subject: Re: [PATCH 4/5] USB: OTG: msm: Configure PHY Analog and Digital voltage
 domains
Date: Mon, 02 May 2011 11:33:15 +0530
Message-ID: <4DBE4923.2050802@codeaurora.org>
References: <1303977693-18389-1-git-send-email-pkondeti@codeaurora.org> <1303977693-18389-4-git-send-email-pkondeti@codeaurora.org> <4DBA8F6D.7000508@ru.mvista.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Return-path: <linux-arm-msm-owner@vger.kernel.org>
Received: from wolverine02.qualcomm.com ([199.106.114.251]:64294 "EHLO
	wolverine02.qualcomm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750805Ab1EBGDS (ORCPT
	<rfc822;linux-arm-msm@vger.kernel.org>);
	Mon, 2 May 2011 02:03:18 -0400
In-Reply-To: <4DBA8F6D.7000508@ru.mvista.com>
Sender: linux-arm-msm-owner@vger.kernel.org
List-Id: linux-arm-msm@vger.kernel.org
To: Sergei Shtylyov <sshtylyov@mvista.com>
Cc: greg@kroah.com, linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org, Anji jonnala <anjir@codeaurora.org>

Hi Sergei,

Thanks for the review.

On 4/29/2011 3:44 PM, Sergei Shtylyov wrote:
> 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>
> [...]
> 
<snip>

>> +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...
> 

Yep. Will fix it next patch set.

>> +		}
>> +
>> +		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?

Correct. regulator_disable() call is not required.

The error handling here seems to be wrong. We don't need to explicitly
call regulator_set_voltage() upon regulator_enable() failure. will fix
this in next patch set.

> 
>> +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?
> 
sounds good.

>> +	msm_hsusb_ldo_init(motg, 0);
>> +free_config_vddcx:
> 
>     'vddcx_exit' perhaps?
>
sounds good.

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


-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.