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

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

[Vincent Pelletier]

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.
Symmetrically, scale the current limit up when querying a overdrive-enabled
regulator.

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
V4 -> V5:
- when disabling overdrive, adjust current limit first
- remove forgotten development comment
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 finds 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 | 115 ++++++++++++++++++++++++++-
 1 file changed, 113 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index cf7d5341750e..d595f88a00fc 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -412,6 +412,117 @@ 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_limit_set_overdrive(struct regulator_dev *rdev,
+					       int min_uA, int max_uA,
+					       unsigned int overdrive_mask)
+{
+	/* When enabling overdrive, do it before changing the current limit to
+	 * ensure sufficient supply throughout the switch.
+	 */
+	struct da9063_regulator *regl = rdev_get_drvdata(rdev);
+	int ret;
+	unsigned int orig_overdrive;
+
+	ret = regmap_read(regl->hw->regmap, DA9063_REG_CONFIG_H,
+			  &orig_overdrive);
+	if (ret < 0)
+		return ret;
+	ret = regmap_set_bits(regl->hw->regmap, DA9063_REG_CONFIG_H,
+			      overdrive_mask);
+	if (ret < 0)
+		return ret;
+	ret = regulator_set_current_limit_regmap(rdev, min_uA / 2, max_uA / 2);
+	if (ret < 0)
+		/* attempt to restore original overdrive state, ignore failure-
+		 * on-failure
+		 */
+		regmap_update_bits(regl->hw->regmap, DA9063_REG_CONFIG_H,
+				   overdrive_mask, orig_overdrive);
+	return ret;
+}
+
+static int da9063_buck_set_limit_clear_overdrive(struct regulator_dev *rdev,
+						 int min_uA, int max_uA,
+						 unsigned int overdrive_mask)
+{
+	/* When disabling overdrive, do it after changing the current limit to
+	 * ensure sufficient supply throughout the switch.
+	 */
+	struct da9063_regulator *regl = rdev_get_drvdata(rdev);
+	int ret, orig_limit;
+
+	ret = regmap_read(rdev->regmap, rdev->desc->csel_reg, &orig_limit);
+	if (ret < 0)
+		return ret;
+	ret = regulator_set_current_limit_regmap(rdev, min_uA, max_uA);
+	if (ret < 0)
+		return ret;
+	ret = regmap_clear_bits(regl->hw->regmap, DA9063_REG_CONFIG_H,
+				overdrive_mask);
+	if (ret < 0)
+		/* attempt to restore original current limit, ignore failure-
+		 * on-failure
+		 */
+		regmap_write(rdev->regmap, rdev->desc->csel_reg, orig_limit);
+	return ret;
+}
+
+static int da9063_buck_set_current_limit(struct regulator_dev *rdev,
+					 int min_uA, int max_uA)
+{
+	unsigned int overdrive_mask, n_currents;
+
+	overdrive_mask = da9063_get_overdrive_mask(rdev->desc);
+	if (overdrive_mask) {
+		n_currents = rdev->desc->n_current_limits;
+		if (n_currents == 0)
+			return -EINVAL;
+		if (max_uA > rdev->desc->curr_table[n_currents - 1])
+			return da9063_buck_set_limit_set_overdrive(rdev, min_uA,
+								   max_uA,
+								   overdrive_mask);
+		return da9063_buck_set_limit_clear_overdrive(rdev, min_uA,
+							     max_uA,
+							     overdrive_mask);
+	}
+	return regulator_set_current_limit_regmap(rdev, min_uA, max_uA);
+}
+
+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 +530,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

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

[Adam Thomson]

On 08 July 2021 11:33, 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.
> Symmetrically, scale the current limit up when querying a overdrive-enabled
> regulator.
>
> Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
> ---
> V4 -> V5:
> - when disabling overdrive, adjust current limit first
> - remove forgotten development comment
> 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 finds 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 | 115 ++++++++++++++++++++++++++-
>  1 file changed, 113 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-
> regulator.c
> index cf7d5341750e..d595f88a00fc 100644
> --- a/drivers/regulator/da9063-regulator.c
> +++ b/drivers/regulator/da9063-regulator.c
> @@ -412,6 +412,117 @@ 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_limit_set_overdrive(struct regulator_dev *rdev,
> +					       int min_uA, int max_uA,
> +					       unsigned int overdrive_mask)
> +{
> +	/* When enabling overdrive, do it before changing the current limit to
> +	 * ensure sufficient supply throughout the switch.
> +	 */

Have a look at the kernel documentation on coding style:

 https://www.kernel.org/doc/html/v4.10/process/coding-style.html

There's a general preference for multi-line comment blocks in the following
format:

 /*
  * First line
  * Second line
  */


> +	struct da9063_regulator *regl = rdev_get_drvdata(rdev);
> +	int ret;
> +	unsigned int orig_overdrive;
> +
> +	ret = regmap_read(regl->hw->regmap, DA9063_REG_CONFIG_H,
> +			  &orig_overdrive);
> +	if (ret < 0)
> +		return ret;

Newline? Code all feels clumped together IMHO and might be easier to read if
it's split a little more. Also if over-drive is already set, we don't need to do
the following I2C transaction, or the roll-back at the end of the function.

> +	ret = regmap_set_bits(regl->hw->regmap, DA9063_REG_CONFIG_H,
> +			      overdrive_mask);
> +	if (ret < 0)
> +		return ret;
> +	ret = regulator_set_current_limit_regmap(rdev, min_uA / 2, max_uA /
> 2);
> +	if (ret < 0)
> +		/* attempt to restore original overdrive state, ignore failure-
> +		 * on-failure
> +		 */
> +		regmap_update_bits(regl->hw->regmap,
> DA9063_REG_CONFIG_H,
> +				   overdrive_mask, orig_overdrive);

If I2C is failing here I'm not sure this is going to go through and you have
bigger problems. Not sure if it's really worth trying to roll-back at this point
but maybe Mark has another view. Personally I'd be tempted to just ditch this
and just always set the OD bit in this function, rather than trying an roll-back.
Will be much simpler code.

> +	return ret;
> +}
> +
> +static int da9063_buck_set_limit_clear_overdrive(struct regulator_dev *rdev,
> +						 int min_uA, int max_uA,
> +						 unsigned int overdrive_mask)
> +{
> +	/* When disabling overdrive, do it after changing the current limit to
> +	 * ensure sufficient supply throughout the switch.
> +	 */
> +	struct da9063_regulator *regl = rdev_get_drvdata(rdev);
> +	int ret, orig_limit;
> +
> +	ret = regmap_read(rdev->regmap, rdev->desc->csel_reg, &orig_limit);
> +	if (ret < 0)
> +		return ret;
> +	ret = regulator_set_current_limit_regmap(rdev, min_uA, max_uA);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_clear_bits(regl->hw->regmap, DA9063_REG_CONFIG_H,
> +				overdrive_mask);
> +	if (ret < 0)
> +		/* attempt to restore original current limit, ignore failure-
> +		 * on-failure
> +		 */
> +		regmap_write(rdev->regmap, rdev->desc->csel_reg, orig_limit);

Similar comments apply here.

> +	return ret;
> +}
> +
> +static int da9063_buck_set_current_limit(struct regulator_dev *rdev,
> +					 int min_uA, int max_uA)
> +{
> +	unsigned int overdrive_mask, n_currents;
> +
> +	overdrive_mask = da9063_get_overdrive_mask(rdev->desc);
> +	if (overdrive_mask) {
> +		n_currents = rdev->desc->n_current_limits;
> +		if (n_currents == 0)
> +			return -EINVAL;
> +		if (max_uA > rdev->desc->curr_table[n_currents - 1])
> +			return da9063_buck_set_limit_set_overdrive(rdev,
> min_uA,
> +								   max_uA,
> +
> overdrive_mask);
> +		return da9063_buck_set_limit_clear_overdrive(rdev, min_uA,
> +							     max_uA,
> +							     overdrive_mask);
> +	}
> +	return regulator_set_current_limit_regmap(rdev, min_uA, max_uA);
> +}
> +
> +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 +530,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

Re: [PATCH v5] regulator: da9063: Add support for full-current mode.

[Vincent Pelletier]

Hello,

Thanks for the review.

On Mon, 12 Jul 2021 10:42:26 +0000, Adam Thomson <Adam.Thomson.Opensource@diasemi.com> wrote:
> On 08 July 2021 11:33, Vincent Pelletier wrote:
> > +	if (ret < 0)
> > +		/* attempt to restore original overdrive state, ignore failure-
> > +		 * on-failure
> > +		 */
> > +		regmap_update_bits(regl->hw->regmap,
> > DA9063_REG_CONFIG_H,
> > +				   overdrive_mask, orig_overdrive);  
> 
> If I2C is failing here I'm not sure this is going to go through and you have
> bigger problems. Not sure if it's really worth trying to roll-back at this point
> but maybe Mark has another view. Personally I'd be tempted to just ditch this
> and just always set the OD bit in this function, rather than trying an roll-back.
> Will be much simpler code.

What I have in mind here is regulator_set_current_limit_regmap
rejecting the change not because of a bus issue, but rather because of
an unusable min_uA..max_uA range.
I add this to the error handling path comment to make the intent
clearer.

But your remark indeed fully applies in the case of
da9063_buck_set_limit_clear_overdrive. I will keep the roll-back
codepath for the next patch iteration, but I will drop it if the
consensus is against its presence.

Regards,
-- 
Vincent Pelletier
GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1