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@xxxxxxxxxxxxxx>
>
>> Signed-off-by: Anji jonnala <anjir@xxxxxxxxxxxxxx>
>> Signed-off-by: Pavankumar Kondeti <pkondeti@xxxxxxxxxxxxxx>
> [...]
>
<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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
