Re: [PATCH v4 4/4] regulator: Prevent falling too fast

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4172 bytes --]

On Tue, Sep 06, 2016 at 12:05:24PM -0700, Matthias Kaehlcke wrote:

> On some boards it's possible that transitioning the PWM regulator
> downwards too fast will trigger the over voltage protection (OVP) on the
> regulator.  This is because until the voltage actually falls there is a
> time when the requested voltage is much lower than the actual voltage.

So your PWM regulators are not very good and overshooting?  Do you have
any numbers on this - what values do you end up setting for both the
delay and safe fall percentages?

> We'll fix this OVP problem by allowing users to specify the maximum
> voltage that we can safely fall.  Apparently this maximum safe voltage
> should be specified as a percentage of the current voltage.  The
> regulator will then break things into separate steps with a delay in
> between.

I'd like to see some more thorough analysis than just an "apparently"
here.  It's making the code more fiddly for something very handwavy.

> @@ -2753,8 +2753,6 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>  	int old_selector = -1;
>  	int old_uV = _regulator_get_voltage(rdev);
>  
> -	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> -
>  	min_uV += rdev->constraints->uV_offset;
>  	max_uV += rdev->constraints->uV_offset;
>  
> @@ -2842,11 +2840,53 @@ no_delay:
>  				     (void *)data);
>  	}
>  
> -	trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
> -
>  	return ret;

Let's remove some trace points too because...?  These *are* the actual
voltage changes in the device, we're just splitting things up into a
series of transitions.

> +static int _regulator_set_voltage(struct regulator_dev *rdev,
> +				     int min_uV, int max_uV)
> +{
> +	int safe_fall_percent = rdev->constraints->safe_fall_percent;
> +	int slowest_decay_rate = rdev->constraints->slowest_decay_rate;
> +	int orig_uV = _regulator_get_voltage(rdev);
> +	int uV = orig_uV;
> +	int ret;
> +
> +	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> +
> +	 /* If we're rising or we're falling but don't need to slow; easy */
> +	if (min_uV >= uV || !safe_fall_percent)

Indentation is broken, the two lines above don't agree with each other.

> +	ret = of_property_read_u32(np, "regulator-slowest-decay-rate", &pval);
> +	if (!ret)
> +		constraints->slowest_decay_rate = pval;
> +	else
> +		constraints->slowest_decay_rate = INT_MAX;

The documentation said this is mandatory if we have a safe fall rate
specified but the code says it's optional and we'll silently default to
an absurdly high rate instead (it's odd that a large number in a field
marked slowest is fast BTW).  Complaining loudly seems better than
ignoring the error.

> +	/* We use the value as int and as divider; sanity check */
> +	if (constraints->slowest_decay_rate == 0) {
> +		pr_err("%s: regulator-slowest-decay-rate must not be 0\n",
> +			np->name);
> +		constraints->slowest_decay_rate = INT_MAX;
> +	} else if (constraints->slowest_decay_rate > INT_MAX) {
> +		pr_err("%s: regulator-slowest-decay-rate (%u) too big\n",
> +			np->name, constraints->slowest_decay_rate);
> +		constraints->slowest_decay_rate = INT_MAX;
> +	}

This should be with the parsing of slowest-decay-rate and checked only
if we have a safe-fall-percent, there's no error if the value is omitted.

> +	if (constraints->safe_fall_percent > 100) {
> +		pr_err("%s: regulator-safe-fall-percent (%u) > 100\n",
> +			np->name, constraints->safe_fall_percent);
> +		constraints->safe_fall_percent = 0;
> +       }

Again indentation is borked here, I think you have tab/space issues.

> +	if (constraints->safe_fall_percent &&
> +		!constraints->slowest_decay_rate) {
> +		pr_err("%s: regulator-slowest-decay-rate requires "
> +			"regulator-safe-fall-percent\n", np->name);

Don't align the second line of the condition with the body of the if,
that just makes things hard to read - do what the rest of the code does
and align with the (.

Don't split text messages over multiple lines, it makes it impossible to
grep for the error as printed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]