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
