From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dwivedi, Avaneesh Kumar (avani)" Subject: Re: [RESEND PATCH v4 2/7] remoteproc: qcom: Initialize proxy and active clock's and regulator's Date: Fri, 9 Dec 2016 21:23:47 +0530 Message-ID: References: <1479981638-32069-1-git-send-email-akdwived@codeaurora.org> <1479981638-32069-3-git-send-email-akdwived@codeaurora.org> <20161209023622.GP30492@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]:40012 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933630AbcLIPxx (ORCPT ); Fri, 9 Dec 2016 10:53:53 -0500 In-Reply-To: <20161209023622.GP30492@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:06 AM, Bjorn Andersson wrote: > On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote: > >> Certain regulators and clocks need voting by rproc on behalf of hexagon >> only during restart operation but certain clocks and voltage need to be >> voted till hexagon is up, these regulators and clocks are identified as >> proxy and active resource whose handle is being obtained by supplying >> private proxy and active regulator and clock string. >> > Please split this patch out into the regulator and clock patches > respectively, making each a clear functional change. OK. > >> Signed-off-by: Avaneesh Kumar Dwivedi >> --- >> drivers/remoteproc/qcom_q6v5_pil.c | 148 +++++++++++++++++++++++++++---------- >> 1 file changed, 107 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >> index 3360312..b0f0fcf 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pil.c >> +++ b/drivers/remoteproc/qcom_q6v5_pil.c >> @@ -37,7 +37,6 @@ >> >> #include >> >> -#define MBA_FIRMWARE_NAME "mba.b00" > Please move this to patch 1. OK. > >> #define MPSS_FIRMWARE_NAME "modem.mdt" >> >> #define MPSS_CRASH_REASON_SMEM 421 >> @@ -132,6 +131,14 @@ struct q6v5 { >> struct clk *ahb_clk; >> struct clk *axi_clk; >> struct clk *rom_clk; >> + struct clk *active_clks[8]; >> + struct clk *proxy_clks[4]; >> + struct reg_info active_regs[1]; >> + struct reg_info proxy_regs[3]; >> + int active_reg_count; >> + int proxy_reg_count; >> + int active_clk_count; >> + int proxy_clk_count; > Group clocks members together and regulators together. OK. > >> >> struct completion start_done; >> struct completion stop_done; >> @@ -160,27 +167,43 @@ enum { >> Q6V5_SUPPLY_PLL, >> }; >> >> -static int q6v5_regulator_init(struct q6v5 *qproc) >> +static int q6v5_regulator_init(struct device *dev, >> + struct reg_info *regs, char **reg_str, int volatage_load[][2]) >> { >> - int ret; >> + int reg_count = 0, i; > Please, one variable per line, sorted (descending) by line length. OK. > >> >> - qproc->supply[Q6V5_SUPPLY_CX].supply = "cx"; >> - qproc->supply[Q6V5_SUPPLY_MX].supply = "mx"; >> - qproc->supply[Q6V5_SUPPLY_MSS].supply = "mss"; >> - qproc->supply[Q6V5_SUPPLY_PLL].supply = "pll"; >> + if (!reg_str) >> + return 0; >> >> - ret = devm_regulator_bulk_get(qproc->dev, >> - ARRAY_SIZE(qproc->supply), qproc->supply); >> - if (ret < 0) { >> - dev_err(qproc->dev, "failed to get supplies\n"); >> - return ret; >> - } >> + while (reg_str[reg_count] != NULL) > Drop "!= NULL" from your condition. OK. > >> + reg_count++; >> >> - regulator_set_load(qproc->supply[Q6V5_SUPPLY_CX].consumer, 100000); >> - regulator_set_load(qproc->supply[Q6V5_SUPPLY_MSS].consumer, 100000); >> - regulator_set_load(qproc->supply[Q6V5_SUPPLY_PLL].consumer, 10000); >> + if (!reg_count) >> + return reg_count; > You can omit this check, as the for loop below will iterate 0 times and > we will then return 0. > >> >> - return 0; >> + if (!regs) >> + return -ENOMEM; > This will not happen, please drop. > >> + >> + for (i = 0; i < reg_count; i++) { >> + const char *reg_name; > Please keep local variables at the top of the function. > > And generally try to use short but clear variable names; in line with my > suggestion in patch 1 name it "supply" (or in this case where the > original variable is quite short just use it directly). OK > >> + >> + reg_name = reg_str[i]; >> + regs[i].reg = devm_regulator_get(dev, reg_name); >> + if (IS_ERR(regs[i].reg)) { >> + >> + int rc = PTR_ERR(regs[i].reg); >> + >> + if (rc != -EPROBE_DEFER) >> + dev_err(dev, "Failed to get %s\n regulator", >> + reg_name); >> + return rc; >> + } >> + >> + regs[i].uV = volatage_load[i][0]; >> + regs[i].uA = volatage_load[i][1]; >> + } >> + >> + return reg_count; >> } >> >> static int q6v5_regulator_enable(struct q6v5 *qproc) >> @@ -725,27 +748,41 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev) >> return 0; >> } >> >> -static int q6v5_init_clocks(struct q6v5 *qproc) >> +static int q6v5_init_clocks(struct device *dev, struct clk **clks, >> + char **clk_str) >> { >> - qproc->ahb_clk = devm_clk_get(qproc->dev, "iface"); >> - if (IS_ERR(qproc->ahb_clk)) { >> - dev_err(qproc->dev, "failed to get iface clock\n"); >> - return PTR_ERR(qproc->ahb_clk); >> - } >> + int clk_count = 0, i; > One variable per line, please. And "count" is unambiguous enough. OK. > >> >> - qproc->axi_clk = devm_clk_get(qproc->dev, "bus"); >> - if (IS_ERR(qproc->axi_clk)) { >> - dev_err(qproc->dev, "failed to get bus clock\n"); >> - return PTR_ERR(qproc->axi_clk); >> - } >> + if (!clk_str) >> + return 0; >> + >> + while (clk_str[clk_count] != NULL) > Drop "!= NULL" part OK. > >> + clk_count++; >> + >> + if (!clk_count) >> + return clk_count; > This is not needed. Yes. > >> + >> + if (!clks) >> + return -ENOMEM; > This can't happen. YES. > >> + >> + for (i = 0; i < clk_count; i++) { >> + const char *clock_name; >> + >> + clock_name = clk_str[i]; >> + clks[i] = devm_clk_get(dev, clock_name); >> + if (IS_ERR(clks[i])) { >> + >> + int rc = PTR_ERR(clks[i]); >> + >> + if (rc != -EPROBE_DEFER) >> + dev_err(dev, "Failed to get %s clock\n", >> + clock_name); >> + return rc; >> + } >> >> - qproc->rom_clk = devm_clk_get(qproc->dev, "mem"); >> - if (IS_ERR(qproc->rom_clk)) { >> - dev_err(qproc->dev, "failed to get mem clock\n"); >> - return PTR_ERR(qproc->rom_clk); >> } >> >> - return 0; >> + return clk_count; >> } >> >> static int q6v5_init_reset(struct q6v5 *qproc) >> @@ -830,10 +867,15 @@ static int q6v5_probe(struct platform_device *pdev) >> { >> struct q6v5 *qproc; >> struct rproc *rproc; >> - int ret; >> + const struct rproc_hexagon_res *desc; >> + int ret, count; > One variable per line please, and sort (descending) on line length. ok, > >> + >> + desc = of_device_get_match_data(&pdev->dev); >> + if (!desc) >> + return -EINVAL; >> >> rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops, >> - MBA_FIRMWARE_NAME, sizeof(*qproc)); >> + desc->hexagon_mba_image, sizeof(*qproc)); > Please move this to patch 1. OK, > >> if (!rproc) { >> dev_err(&pdev->dev, "failed to allocate rproc\n"); >> return -ENOMEM; >> @@ -857,18 +899,42 @@ static int q6v5_probe(struct platform_device *pdev) >> if (ret) >> goto free_rproc; >> >> - if (ret) >> - goto free_rproc; >> + count = q6v5_init_clocks(&pdev->dev, qproc->proxy_clks, >> + desc->proxy_clk_string); > Your "count" serves the same purpose as "ret", so just use "ret". > And align broken lines with the parenthesis. OK. > >> + if (count < 0) { >> + dev_err(&pdev->dev, "Failed to setup proxy clocks.\n"); >> + return count; > And using "ret" instead of "count" makes it easy to goto free_rproc, > which you must do after calling rproc_alloc(). OK. > >> + } >> + qproc->proxy_clk_count = count; >> >> - ret = q6v5_regulator_init(qproc); >> - if (ret) >> - goto free_rproc; >> + count = q6v5_init_clocks(&pdev->dev, qproc->active_clks, >> + desc->active_clk_string); >> + if (count < 0) { >> + dev_err(&pdev->dev, "Failed to setup active clocks.\n"); >> + return count; >> + } >> + qproc->active_clk_count = count; >> >> ret = q6v5_init_reset(qproc); >> if (ret) >> goto free_rproc; >> >> + count = q6v5_regulator_init(&pdev->dev, qproc->proxy_regs, >> + desc->proxy_reg_string, (int (*)[2])desc->proxy_uV_uA); > Moving proxy_reg_string and proxy_uV_uA into a struct (as suggested in > patch 1) will make this cleaner and we get rid of that typecast. OK. > >> + if (count < 0) { >> + dev_err(&pdev->dev, "Failed to setup active regulators.\n"); >> + return count; >> + } >> + qproc->proxy_reg_count = count; >> + >> + count = q6v5_regulator_init(&pdev->dev, qproc->active_regs, >> + desc->active_reg_string, (int (*)[2])desc->active_uV_uA); >> + if (count < 0) { >> + dev_err(&pdev->dev, "Failed to setup proxy regulators.\n"); >> + return count; >> + } >> + qproc->active_reg_count = count; >> + >> ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt); >> if (ret < 0) >> goto free_rproc; > Regards, > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.