RE: [PATCH v4] regulator: da9063: Add support for full-current mode.

[Adam Thomson]

On 05 July 2021 14:20, Vincent Pelletier wrote:

> In addition to the ability of merging some power outputs, this chip has
> an overdrive mode.
> BCORE1, BCORE2 and BPRO have this ability, in which case the legal
> current draw is increased from 2 amps to 2.5 amps (at the expense of
> a quiescent current increase), and the configurable current limits
> are doubled.
> If a current higher than maximum half-current mode is requested, enable
> overdrive, and scale the current limit down.
> Symetrically, scale the current limit up when querying a overdrive-enabled
> regulator.
> 
> Signed-off-by: Vincent Pelletier <plr.vincent@xxxxxxxxx>
> ---
> V3 -> V4:
> - complete logic change: my original approach was backwards: the driver
>   should take full control the overdrive bit, and not depend on the state
>   it find the hardware in.
> V2 -> V3:
> - ACTUALLY skip DA9063_ID_BCORES_MERGED_OD when not full-current, and
>   vice-versa.
> - head put in brown paper bag
> V1 -> V2:
> - skip DA9063_ID_BCORES_MERGED_OD when not full-current, and vice-versa
> - cc linux-kernel ML
> - fix subject prefix
> 
> 
>  drivers/regulator/da9063-regulator.c | 75 +++++++++++++++++++++++++++-
>  1 file changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-
> regulator.c
> index cf7d5341750e..4be7cfd06cd4 100644
> --- a/drivers/regulator/da9063-regulator.c
> +++ b/drivers/regulator/da9063-regulator.c
> @@ -412,6 +412,77 @@ static int da9063_ldo_set_suspend_mode(struct
> regulator_dev *rdev,
>  	return regmap_field_write(regl->suspend_sleep, val);
>  }
> 
> +static unsigned int da9063_get_overdrive_mask(const struct regulator_desc
> *desc)
> +{
> +	switch (desc->id) {
> +	case DA9063_ID_BCORES_MERGED:
> +	case DA9063_ID_BCORE1:
> +		return DA9063_BCORE1_OD;
> +	case DA9063_ID_BCORE2:
> +		return DA9063_BCORE2_OD;
> +	case DA9063_ID_BPRO:
> +		return DA9063_BPRO_OD;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int da9063_buck_set_current_limit(struct regulator_dev *rdev,
> +					 int min_uA, int max_uA)
> +{
> +	struct da9063_regulator *regl = rdev_get_drvdata(rdev);
> +	unsigned int mask, n_currents;
> +	int ret, overdrive;
> +	bool overdrive_changed = false;
> +
> +	mask = da9063_get_overdrive_mask(rdev->desc);
> +	if (mask) {
> +		n_currents = rdev->desc->n_current_limits;
> +		if (n_currents == 0)
> +			return -EINVAL;
> +		if (max_uA > rdev->desc->curr_table[n_currents - 1]) {
> +			overdrive = mask;
> +			min_uA /= 2; // XXX: rounding ?
> +			max_uA /= 2;
> +		} else {
> +			overdrive = 0;
> +		}
> +		ret = regmap_update_bits_check(regl->hw->regmap,
> +					       DA9063_REG_CONFIG_H, mask,
> +					       overdrive, &overdrive_changed);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	ret = regulator_set_current_limit_regmap(rdev, min_uA, max_uA);
> +	if (ret < 0 && overdrive_changed)
> +		/* attempt to restore original overdrive state, ignore failure-
> +		 * on-failure
> +		 */
> +		regmap_update_bits(regl->hw->regmap,
> DA9063_REG_CONFIG_H,
> +				   mask, ~overdrive);
> +	return ret;
> +}

Thinking about this further, the one concern I have here is stepping down the
current limit. If we just set the OD bit first before setting the current limit,
say we were running at 3A in OD and we wanted to drop to 1.8A which falls in to
the lower range, setting OD bit first would set us to 1.5A before we then step
up to 1.8A again with the call to regulator_set_current_limit_regmap(). We
could in theory starve whatever is being supplied. Might need to set current
limit higher first before then setting the OD bit to halve it in that case so
we don't undershoot the current limit.

> +
> +static int da9063_buck_get_current_limit(struct regulator_dev *rdev)
> +{
> +	struct da9063_regulator *regl = rdev_get_drvdata(rdev);
> +	int val, ret, limit;
> +	unsigned int mask;
> +
> +	limit = regulator_get_current_limit_regmap(rdev);
> +	if (limit < 0)
> +		return limit;
> +	mask = da9063_get_overdrive_mask(rdev->desc);
> +	if (mask) {
> +		ret = regmap_read(regl->hw->regmap, DA9063_REG_CONFIG_H,
> &val);
> +		if (ret < 0)
> +			return ret;
> +		if (val & mask)
> +			limit *= 2;
> +	}
> +	return limit;
> +}
> +
>  static const struct regulator_ops da9063_buck_ops = {
>  	.enable			= regulator_enable_regmap,
>  	.disable		= regulator_disable_regmap,
> @@ -419,8 +490,8 @@ static const struct regulator_ops da9063_buck_ops = {
>  	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
>  	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
>  	.list_voltage		= regulator_list_voltage_linear,
> -	.set_current_limit	= regulator_set_current_limit_regmap,
> -	.get_current_limit	= regulator_get_current_limit_regmap,
> +	.set_current_limit	= da9063_buck_set_current_limit,
> +	.get_current_limit	= da9063_buck_get_current_limit,
>  	.set_mode		= da9063_buck_set_mode,
>  	.get_mode		= da9063_buck_get_mode,
>  	.get_status		= da9063_buck_get_status,
> --
> 2.32.0