Re: [PATCH 1/2] regulator: core: Add coupled regulators mechanism

[Mark Brown]

On Mon, Sep 18, 2017 at 10:39:51AM +0200, Maciej Purski wrote:
> On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
> different devices on the board are supplied by different regulators
> with non-fixed voltages. If one of these devices temporarily requires
> higher voltage, there might occur a situation that the spread between
> two devices' voltages is so high, that there is a risk of changing
> 'high' and 'low' states on the interconnection between devices powered
> by those two regulators.
> 
> Keeping spread between those voltages below defined max_spread should
> be handled by the framework. Information required to do so is obtained
> from the device tree. On each voltage change the core should find the
> best voltages which suit all consumers' demands and max_spread.
> Then set them for a coupled regulator also.

This leads me none the wiser as to how this will be implemented which
makes this rather hard to review, especially since this appears to have
a lot of random refactoring mixed in with it.

> +static int regulator_set_voltage_safe(struct regulator_dev *rdev,
> +				      int min_uV, int max_uV);

Why would we want a way to set voltages that isn't safe?

> @@ -2181,6 +2183,8 @@ static int _regulator_enable(struct regulator_dev *rdev)
>  		/* Fallthrough on positive return values - already enabled */
>  	}
>  
> +	if (rdev->coupled_desc)
> +		rdev->coupled_desc->enable_count++;
>  	rdev->use_count++;
>  
>  	return 0;

There's no locking here, and we appear to take no action when these
counts change - do we need to bother with this at all?

> +	/* check if changing voltage won't interfere with other
> +	 * consumers' demands
> +	 */
>  	ret = regulator_check_consumers(rdev, &min_uV, &max_uV);
>  	if (ret < 0)
>  		goto out2;

These extra comments that are being added are pretty much all readouts
of the name of the function that's being called (and many of them are
misformatted)...

> +static int regulator_set_voltage_safe(struct regulator_dev *rdev, int min_uV,
> +					   int max_uV)
> +{

...which is a bit ironic given that this isn't documented at all :/

> +static int regulator_set_coupled_voltage(struct coupled_reg_desc *c_desc)
> +{
> +	struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
> +	int max_spread = c_desc->max_spread;
> +	int best_volt[2] = { };
> +	int actual_volt[2];
> +	int min_volt, max_volt;
> +	int ret = 0, i, max;
> +	int ready = 0;
> +
> +	/* Get voltages desired by all consumers of the coupled regulator */
> +	for (i = 0; i < 2; i++) {

It appears we can't couple more than two regulators?

> +	max_volt = max(best_volt[0], best_volt[1]);
> +	min_volt = min(best_volt[0], best_volt[1]);

So the maximum and minimum are the *least* constrained?

> +			max_possible = actual_volt[(i + 1) % 2] + max_spread;
> +			min_possible = actual_volt[(i + 1) % 2] - max_spread;

I'm lost, this magic array indexing is just illegible.

Attachment: signature.asc
Description: PGP signature