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 <[email protected]> > >> Signed-off-by: Anji jonnala <[email protected]> >> Signed-off-by: Pavankumar Kondeti <[email protected]> > [...] > <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-arm-msm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
