Re: [RESEND PATCH v4 3/7] remoteproc: qcom: Modify regulator enable and disable interface

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

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 <akdwived@codeaurora.org>
> ---
>  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.

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

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

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

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

>  	return ret;
>  }
>  

Regards,
Bjorn