From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dwivedi, Avaneesh Kumar (avani)" Subject: Re: [RESEND PATCH v4 3/7] remoteproc: qcom: Modify regulator enable and disable interface Date: Mon, 12 Dec 2016 13:51:10 +0530 Message-ID: <3a786ed8-2eb0-4f21-909d-6c432f397782@codeaurora.org> References: <1479981638-32069-1-git-send-email-akdwived@codeaurora.org> <1479981638-32069-4-git-send-email-akdwived@codeaurora.org> <20161209024442.GQ30492@tuxbot> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:54574 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752600AbcLLIVU (ORCPT ); Mon, 12 Dec 2016 03:21:20 -0500 In-Reply-To: <20161209024442.GQ30492@tuxbot> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Bjorn Andersson Cc: sboyd@codeaurora.org, agross@codeaurora.org, linux-arm-msm@vger.kernel.org On 12/9/2016 8:14 AM, Bjorn Andersson wrote: > On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote: > >> Regulator enable routine will get additional input parameter of >> regulator info and count, It will read regulator info and will do >> appropriate voltage and load configuration before turning them up. >> Also separate out disable interface into proxy and active disable >> so that on arrival of handover interrupt proxy regulators alone >> could be voted out. >> >> Signed-off-by: Avaneesh Kumar Dwivedi >> --- >> drivers/remoteproc/qcom_q6v5_pil.c | 113 ++++++++++++++++++++++++++++--------- >> 1 file changed, 86 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >> index b0f0fcf..06d5bb2 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pil.c >> +++ b/drivers/remoteproc/qcom_q6v5_pil.c >> @@ -126,7 +126,6 @@ struct q6v5 { >> struct qcom_smem_state *state; >> unsigned stop_bit; >> >> - struct regulator_bulk_data supply[4]; >> >> struct clk *ahb_clk; >> struct clk *axi_clk; >> @@ -160,13 +159,6 @@ enum { >> Q5V56_1_5_0, /*hexagon on msm8996*/ >> }; >> >> -enum { >> - Q6V5_SUPPLY_CX, >> - Q6V5_SUPPLY_MX, >> - Q6V5_SUPPLY_MSS, >> - Q6V5_SUPPLY_PLL, >> -}; >> - >> static int q6v5_regulator_init(struct device *dev, >> struct reg_info *regs, char **reg_str, int volatage_load[][2]) >> { >> @@ -206,35 +198,93 @@ static int q6v5_regulator_init(struct device *dev, >> return reg_count; >> } >> >> -static int q6v5_regulator_enable(struct q6v5 *qproc) >> +static int q6v5_regulator_enable(struct q6v5 *qproc, >> + struct reg_info *regs, int count) >> { >> - struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer; >> - struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer; >> - int ret; >> + int i, rc = 0; > One variable per line, no need to replace "ret" with "rc" and no need to > initialize it to 0 as it won't be read before written in any code path. OK. > >> + >> + for (i = 0; i < count; i++) { >> + if (regs[i].uV > 0) { >> + rc = regulator_set_voltage(regs[i].reg, >> + regs[i].uV, INT_MAX); >> + if (rc) { >> + dev_err(qproc->dev, >> + "Failed to request voltage for %d.\n", >> + i); >> + goto err; >> + } >> + } >> >> - /* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */ >> + if (regs[i].uA > 0) { >> + rc = regulator_set_load(regs[i].reg, >> + regs[i].uA); >> + if (rc < 0) { >> + dev_err(qproc->dev, "Failed to set regulator mode\n"); >> + goto err; >> + } >> + } >> >> - ret = regulator_set_voltage(mx, 1050000, INT_MAX); >> - if (ret) >> - return ret; >> + rc = regulator_enable(regs[i].reg); >> + if (rc) { >> + dev_err(qproc->dev, "Regulator enable failed\n"); >> + goto err; >> + } >> + } >> + >> + return 0; >> +err: >> + for (; i >= 0; i--) { >> + if (regs[i].uV > 0) >> + regulator_set_voltage(regs[i].reg, 0, INT_MAX); >> >> - regulator_set_voltage(mss, 1000000, 1150000); >> + if (regs[i].uA > 0) >> + regulator_set_load(regs[i].reg, 0); >> + >> + regulator_disable(regs[i].reg); >> + } >> >> - return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply); >> + return rc; >> } >> >> -static void q6v5_regulator_disable(struct q6v5 *qproc) >> +static void q6v5_proxy_regulator_disable(struct q6v5 *qproc) > Please have follow the same scheme as for enable, with a function taking > the list of regulators and count. Ok. > >> { >> - struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer; >> - struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer; >> + int i; >> + struct reg_info *regs = qproc->proxy_regs; >> >> - /* TODO: Q6V5_SUPPLY_CX corner votes should be released */ >> + for (i = 0; i < qproc->proxy_reg_count; i++) { >> + if (regs[i].uV > 0) >> + regulator_set_voltage(regs[i].reg, 0, INT_MAX); >> >> - regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply); >> - regulator_set_voltage(mx, 0, INT_MAX); >> - regulator_set_voltage(mss, 0, 1150000); >> + if (regs[i].uA > 0) >> + regulator_set_load(regs[i].reg, 0); >> + >> + regulator_disable(regs[i].reg); >> + } >> } >> >> +static void q6v5_active_regulator_disable(struct q6v5 *qproc) >> +{ >> + int i; >> + struct reg_info *regs = qproc->active_regs; >> + >> + for (i = 0; i < qproc->active_reg_count; i++) { >> + if (regs[i].uV > 0) >> + regulator_set_voltage(regs[i].reg, 0, INT_MAX); >> + >> + if (regs[i].uA > 0) >> + regulator_set_load(regs[i].reg, 0); >> + >> + regulator_disable(regs[i].reg); >> + } >> +} >> + >> +static void q6v5_regulator_disable(struct q6v5 *qproc) >> +{ >> + q6v5_proxy_regulator_disable(qproc); >> + q6v5_active_regulator_disable(qproc); >> +} >> + >> + >> static int q6v5_load(struct rproc *rproc, const struct firmware *fw) >> { >> struct q6v5 *qproc = rproc->priv; >> @@ -524,12 +574,19 @@ static int q6v5_start(struct rproc *rproc) >> struct q6v5 *qproc = (struct q6v5 *)rproc->priv; >> int ret; >> >> - ret = q6v5_regulator_enable(qproc); >> + ret = q6v5_regulator_enable(qproc, qproc->proxy_regs, >> + qproc->proxy_reg_count); > Align indentation with parenthesis on previous line. Ok. > >> if (ret) { >> - dev_err(qproc->dev, "failed to enable supplies\n"); >> + dev_err(qproc->dev, "failed to enable proxy supplies\n"); >> return ret; >> } >> >> + ret = q6v5_regulator_enable(qproc, qproc->active_regs, >> + qproc->active_reg_count); >> + if (ret) { >> + dev_err(qproc->dev, "failed to enable supplies\n"); >> + goto disable_proxy_reg; >> + } >> ret = reset_control_deassert(qproc->mss_restart); >> if (ret) { >> dev_err(qproc->dev, "failed to deassert mss restart\n"); >> @@ -600,6 +657,8 @@ static int q6v5_start(struct rproc *rproc) >> disable_vdd: >> q6v5_regulator_disable(qproc); > Here you disable both active and proxy regulators, then you call > through... Ok. > >> >> +disable_proxy_reg: >> + q6v5_proxy_regulator_disable(qproc); > ...and once again disable proxy regulators. > > Remove q6v5_regulator_disable() and just call the active and proxy > disable functions directly. Ok. Using single function to disable regulator but passing proxy or active regulator info as necessary. > >> return ret; >> } >> > Regards, > Bjorn -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.