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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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.

> +	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.

> +{
> +	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.

Regards,
Bjorn