Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 14 Dec 2018 08:08:20 +0000
From:   Priit Laes <plaes@plaes.org>
To:     Mark Brown <broonie@kernel.org>
Cc:     Olliver Schinagl <oliver@schinagl.nl>, linux-kernel@vger.kernel.org
Subject: Re: Applied "regulator: axp20x: add support for set_ramp_delay for
 AXP209" to the regulator tree
Message-ID: <20181214080820.mkukiqdcr5rk32y3@plaes.org>
References: <20181213181142.A5F3911254D1@debutante.sirena.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181213181142.A5F3911254D1@debutante.sirena.org.uk>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Dec 13, 2018 at 06:11:42PM +0000, Mark Brown wrote:
> The patch
> 

Heya, Mark!

Unfortunately, one error crept in, please see below:

[...]
> 
> From d29f54df8b167be6174881f265b12453a558e48a Mon Sep 17 00:00:00 2001
> From: Olliver Schinagl <oliver@schinagl.nl>
> Date: Tue, 11 Dec 2018 17:17:06 +0200
> Subject: [PATCH] regulator: axp20x: add support for set_ramp_delay for AXP209
> 
> The AXP209 supports ramping up voltages on several regulators such as
> DCDC2 and LDO3.
> 
> This patch adds preliminary support for the regulator-ramp-delay property
> for these 2 regulators. Note that the voltage ramp only works when
> regulator is already enabled. E.g. when going from say 0.7 V to 3.6 V.
> 
> When turning on the regulator, no voltage ramp is performed in hardware.
> 
> What this means, is that if the bootloader brings up the voltage at 0.7 V,
> the ramp delay property is properly applied. If however, the bootloader
> leaves the power off, no ramp delay is applied when the power is
> enabled by the regulator framework.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> Signed-off-by: Priit Laes <plaes@plaes.org>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  drivers/regulator/axp20x-regulator.c | 85 ++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
[...]
>  
> +static const int axp209_dcdc2_ldo3_slew_rates[] = {
> +	1600,
> +	 800,
> +};
> +
> +static int axp20x_set_ramp_delay(struct regulator_dev *rdev, int ramp)
> +{
> +	struct axp20x_dev *axp20x = rdev_get_drvdata(rdev);
> +	const struct regulator_desc *desc = rdev->desc;
> +	u8 reg, mask, enable, cfg = 0xff;
> +	const int *slew_rates;
> +	int rate_count = 0;
> +
> +	if (!rdev)
> +		return -EINVAL;
> +
> +	switch (axp20x->variant) {
> +	case AXP209_ID:
> +		if (desc->id == AXP20X_DCDC2) {

	slew_rates = axp209_dcdc2_ldo3_slew_rates;

> +			rate_count = ARRAY_SIZE(axp209_dcdc2_ldo3_slew_rates);
> +			reg = AXP20X_DCDC2_LDO3_V_RAMP;
> +			mask = AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_RATE_MASK |
> +			       AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_EN_MASK;
> +			enable = (ramp > 0) ?
> +				 AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_EN :
> +				 !AXP20X_DCDC2_LDO3_V_RAMP_DCDC2_EN;
> +			break;
> +		}
> +
> +		if (desc->id == AXP20X_LDO3) {
> +			slew_rates = axp209_dcdc2_ldo3_slew_rates;
> +			rate_count = ARRAY_SIZE(axp209_dcdc2_ldo3_slew_rates);
> +			reg = AXP20X_DCDC2_LDO3_V_RAMP;
> +			mask = AXP20X_DCDC2_LDO3_V_RAMP_LDO3_RATE_MASK |
> +			       AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN_MASK;
> +			enable = (ramp > 0) ?
> +				 AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN :
> +				 !AXP20X_DCDC2_LDO3_V_RAMP_LDO3_EN;
> +			break;
> +		}
> +
> +		if (rate_count > 0)
> +			break;
> +
> +		/* fall through */
> +	default:
> +		/* Not supported for this regulator */
> +		return -ENOTSUPP;
> +	}
> +
> +	if (ramp == 0) {
> +		cfg = enable;
> +	} else {
> +		int i;
> +
> +		for (i = 0; i < rate_count; i++) {
> +			if (ramp <= slew_rates[i])
> +				cfg = AXP20X_DCDC2_LDO3_V_RAMP_LDO3_RATE(i);
> +			else
> +				break;
> +		}
> +
> +		if (cfg == 0xff) {
> +			dev_err(axp20x->dev, "unsupported ramp value %d", ramp);
> +			return -EINVAL;
> +		}
> +
> +		cfg |= enable;
> +	}
> +
> +	return regmap_update_bits(axp20x->regmap, reg, mask, cfg);
> +}
> +
>  static const struct regulator_ops axp20x_ops_fixed = {
>  	.list_voltage		= regulator_list_voltage_linear,
>  };
> @@ -366,6 +450,7 @@ static const struct regulator_ops axp20x_ops = {
>  	.enable			= regulator_enable_regmap,
>  	.disable		= regulator_disable_regmap,
>  	.is_enabled		= regulator_is_enabled_regmap,
> +	.set_ramp_delay		= axp20x_set_ramp_delay,
>  };
>  
>  static const struct regulator_ops axp20x_ops_sw = {
> -- 
> 2.19.0.rc2
>