Hi, On 11/22/2017 11:33 PM, Stephen Boyd wrote: > On 11/21/2017 01:23 AM, Manu Gautam wrote: >> PHY must be powered on before turning ON clocks and >> attempting to initialize it. Driver is exposing >> separate init and power_on routines for this. >> Apparently USB dwc3 core driver performs power-on after >> init. Also, poweron and init for QMP PHY need to be > Why does dwc3 driver power on after init? Seems backwards. There are not many PHY drivers implementing power_on, rather they rely on init to take care of complete initialization. However though the name indicates power_on, but PHY drivers are not using it to turn on power supplies but rather PHY register operations to enable/ start PHY - somewhat like init only. > >> executed together always, hence remove poweron callback >> from phy_ops and explicitly perform this from com_init, > Why do they need to be executed together? Hardware programming guide requires PHY supplies to be ON before it is initialized. And if PHY supplies were turned-off, then it must be reset after turning them ON. > >> similar changes needed for poweroff. On similar lines move >> clk_enable from init to com_init which can be called once >> for multi lane PHYs. > Please add parenthesis, clk_enable() for example, to functions so we > know they're functions. Ok. > >> Signed-off-by: Manu Gautam <mgautam@xxxxxxxxxxxxxx> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp.c | 61 +++++++++++++------------------------ >> 1 file changed, 21 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c >> index 90794dd..2f427e3 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c >> @@ -720,33 +720,6 @@ static void qcom_qmp_phy_configure(void __iomem *base, >> } >> } >> >> -static int qcom_qmp_phy_poweron(struct phy *phy) >> -{ >> - struct qmp_phy *qphy = phy_get_drvdata(phy); >> - struct qcom_qmp *qmp = qphy->qmp; >> - int num = qmp->cfg->num_vregs; >> - int ret; >> - >> - dev_vdbg(&phy->dev, "Powering on QMP phy\n"); >> - >> - /* turn on regulator supplies */ >> - ret = regulator_bulk_enable(num, qmp->vregs); >> - if (ret) >> - dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret); >> - >> - return ret; >> -} >> - >> -static int qcom_qmp_phy_poweroff(struct phy *phy) >> -{ >> - struct qmp_phy *qphy = phy_get_drvdata(phy); >> - struct qcom_qmp *qmp = qphy->qmp; >> - >> - regulator_bulk_disable(qmp->cfg->num_vregs, qmp->vregs); >> - >> - return 0; >> -} >> - >> static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) >> { >> const struct qmp_phy_cfg *cfg = qmp->cfg; >> @@ -759,6 +732,19 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) >> return 0; >> } >> >> + /* turn on regulator supplies */ >> + ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); >> + if (ret) { >> + mutex_unlock(&qmp->phy_mutex); >> + return ret; > This could also be a goto. Yes, I can replace with goto here. > >> + } >> + >> + ret = clk_bulk_prepare_enable(cfg->num_clks, qmp->clks); >> + if (ret) { >> + dev_err(qmp->dev, "failed to enable clks, err=%d\n", ret); >> + goto err_clk_enable; >> + } >> + >> for (i = 0; i < cfg->num_resets; i++) { >> ret = reset_control_deassert(qmp->resets[i]); >> if (ret) { -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html