Re: [RESEND PATCH v4 4/7] remoteproc: qcom: Modify clock enable and disable routine

["Dwivedi, Avaneesh Kumar (avani)" <akdwived@codeaurora.org>]

On 12/9/2016 8:27 AM, Bjorn Andersson wrote:
> On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Clock handling is made generic than earlier way to add new clock
>> every time when required to support new hexagon version. Also
>> clock disable interface is separated out into two separate routine
>> to unvote proxy clock alone when handover interrupt is arrived.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 93 ++++++++++++++++++++++++++------------
>>   1 file changed, 65 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 06d5bb2..c55dc9a 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -126,10 +126,6 @@ struct q6v5 {
>>   	struct qcom_smem_state *state;
>>   	unsigned stop_bit;
>>   
>> -
>> -	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];
>> @@ -284,6 +280,51 @@ static void q6v5_regulator_disable(struct q6v5 *qproc)
>>   	q6v5_active_regulator_disable(qproc);
>>   }
>>   
>> +static int q6v5_clk_enable(struct device *dev, struct clk **clks,
>> +								int clk_count)
>> +{
>> +	int rc = 0;
> No need to initialize to 0.
OK.
>
>> +	int i;
>> +
>> +	for (i = 0; i < clk_count; i++) {
>> +		rc = clk_prepare_enable(clks[i]);
>> +		if (rc) {
>> +			dev_err(dev, "Clock enable failed\n");
>> +			goto err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +err:
>> +	for (i--; i >= 0; i--)
>> +		clk_disable_unprepare(clks[i]);
>> +
>> +	return rc;
>> +}
>> +
>> +static void q6v5_proxy_clk_disable(struct q6v5 *qproc)
> Please make these follow the enable case, by taking the clock list and
> size as parameters.
OK.
>
>> +{
>> +	int i;
>> +	struct clk **clks = qproc->proxy_clks;
>> +
>> +	for (i = 0; i < qproc->proxy_clk_count; i++)
>> +		clk_disable_unprepare(clks[i]);
>> +}
>> +
>> +static void q6v5_active_clk_disable(struct q6v5 *qproc)
>> +{
>> +	int i;
>> +	struct clk **clks = qproc->proxy_clks;
>> +
>> +	for (i = 0; i < qproc->proxy_clk_count; i++)
>> +		clk_disable_unprepare(clks[i]);
>> +}
>> +
>> +static void q6v5_clk_disable(struct q6v5 *qproc)
>> +{
>> +	q6v5_proxy_clk_disable(qproc);
>> +	q6v5_active_clk_disable(qproc);
>> +}
> Just inline the two disable calls where you need them, rather than
> having this wrapper.
OK.
>
> 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.