Re: [PATCH v3 4/4] regulator: core: Balance coupled regulators voltages

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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

On Thu, Dec 07, 2017 at 10:46:15AM +0100, Maciej Purski wrote:

> @@ -2447,10 +2482,9 @@ static int _regulator_is_enabled(struct regulator_dev *rdev)
>  	return rdev->desc->ops->is_enabled(rdev);
>  }
>  
> -static int _regulator_list_voltage(struct regulator *regulator,
> -				    unsigned selector, int lock)
> +static int _regulator_list_voltage(struct regulator_dev *rdev,
> +				   unsigned selector, int lock)
>  {

Please split this refactoring of _list_voltage() into a separate patch
for ease of review.  It can go in separately which will make this change
smaller and easier to review.

> @@ -2928,6 +2961,35 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
>  	if (ret < 0)
>  		goto out2;
>  
> +	/*
> +	 * If the regulator is not coupled just set voltage normally, else
> +	 * return after changing consumer demands without changing voltage.
> +	 * This will be handled outside the function
> +	 * by regulator_balance_coupled()
> +	 */
> +	if (!rdev->coupling_desc) {
> +		ret = regulator_set_voltage_rdev(regulator->rdev,
> +						 min_uV, max_uV);
> +		if (ret < 0)
> +			goto out2;
> +	}

As I think I said on the previous version I'm not enthusiastic about
having two separate code paths for setting the voltage, it makes it much
more likely that things will break especially given how rare coupled
regulators are.  It would be cleaner to make uncoupled regulators just
be a special case of coupled regulators, that way more of the code is
shared.  To that end I'd adjust the code so that we always have a
coupling descriptor and then handle the case where there's only one
regulator described in there.

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