Re: [RESEND PATCH v4 2/7] remoteproc: qcom: Initialize proxy and active clock's and regulator's

["Dwivedi, Avaneesh Kumar (avani)" <akdwived@codeaurora.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 <akdwived@codeaurora.org>
>> ---
>>   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 <linux/qcom_scm.h>
>>   
>> -#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.