Hi Mathieu,
On Mon, Nov 30, 2015 at 10:17:45AM -0700, Mathieu Poirier wrote:
> On 30 November 2015 at 08:29, Maxime Ripard
> <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> > Some boards, in order to power devices that have a quite high power
> > consumption, wire multiple regulators in parallel.
> >
> > In such a case, the regulators need to be kept in sync, all of them being
> > enabled or disabled in parallel.
> >
> > This also requires to expose only the voltages that are common to all the
> > regulators.
> >
> > Eventually support for changing the voltage in parallel should be added
> > too, possibly with delays between each other to avoid having a too brutal
> > peak consumption.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/regulator/Kconfig | 7 +
> > drivers/regulator/Makefile | 1 +
> > drivers/regulator/coupled-voltage-regulator.c | 241 ++++++++++++++++++++++++++
> > 3 files changed, 249 insertions(+)
> > create mode 100644 drivers/regulator/coupled-voltage-regulator.c
> >
> > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> > index 8df0b0e62976..6ba7bc8fda13 100644
> > --- a/drivers/regulator/Kconfig
> > +++ b/drivers/regulator/Kconfig
> > @@ -35,6 +35,13 @@ config REGULATOR_FIXED_VOLTAGE
> > useful for systems which use a combination of software
> > managed regulators and simple non-configurable regulators.
> >
> > +config REGULATOR_COUPLED_VOLTAGE
> > + tristate "Coupled voltage regulator support"
> > + help
> > + This driver provides support for regulators that are an
> > + aggregate of other regulators in the system, and need to
> > + keep them all in sync.
> > +
> > config REGULATOR_VIRTUAL_CONSUMER
> > tristate "Virtual regulator consumer support"
> > help
> > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> > index 0f8174913c17..c05839257386 100644
> > --- a/drivers/regulator/Makefile
> > +++ b/drivers/regulator/Makefile
> > @@ -22,6 +22,7 @@ obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
> > obj-$(CONFIG_REGULATOR_AS3722) += as3722-regulator.o
> > obj-$(CONFIG_REGULATOR_AXP20X) += axp20x-regulator.o
> > obj-$(CONFIG_REGULATOR_BCM590XX) += bcm590xx-regulator.o
> > +obj-$(CONFIG_REGULATOR_COUPLED_VOLTAGE) += coupled-voltage-regulator.o
> > obj-$(CONFIG_REGULATOR_DA903X) += da903x.o
> > obj-$(CONFIG_REGULATOR_DA9052) += da9052-regulator.o
> > obj-$(CONFIG_REGULATOR_DA9055) += da9055-regulator.o
> > diff --git a/drivers/regulator/coupled-voltage-regulator.c b/drivers/regulator/coupled-voltage-regulator.c
> > new file mode 100644
> > index 000000000000..dc7aa2aca7e6
> > --- /dev/null
> > +++ b/drivers/regulator/coupled-voltage-regulator.c
> > @@ -0,0 +1,241 @@
> > +/*
> > + * Copyright 2015 Free Electrons
> > + * Copyright 2015 NextThing Co.
> > + *
> > + * Author: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of the
> > + * License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/of_regulator.h>
> > +
> > +#define COUPLED_REGULATOR_MAX_SUPPLIES 16
> > +
> > +struct coupled_regulator {
> > + struct regulator **regulators;
> > + int n_regulators;
> > +
>
> Extra line.
>
> > + int *voltages;
> > + int n_voltages;
> > +};
>
> This new structure needs documentation.
Ack.
> > +
> > +static int coupled_regulator_disable(struct regulator_dev *rdev)
> > +{
> > + struct coupled_regulator *creg = rdev_get_drvdata(rdev);
> > + int ret, i;
> > +
> > + for (i = 0; i < creg->n_regulators; i++) {
> > + ret = regulator_disable(creg->regulators[i]);
> > + if (ret)
> > + break;
> > + }
> > +
> > + return ret;
> > +}
>
> What happens to the other regulators when an element of the chain
> fails to disable? Should they be powered on again?
>
> > +
> > +static int coupled_regulator_enable(struct regulator_dev *rdev)
> > +{
> > + struct coupled_regulator *creg = rdev_get_drvdata(rdev);
> > + int ret, i;
> > +
> > + for (i = 0; i < creg->n_regulators; i++) {
> > + ret = regulator_enable(creg->regulators[i]);
> > + if (ret)
> > + break;
> > + }
> > +
> > + return ret;
> > +}
>
> Same thing here - shouldn't the previously enabled regulators be
> switched off when one fails to come on? It might be worth documenting
> the behaviour being enacted.
That's actually a very good question, and I don't have a good answer
to it. I guess the safest approach would be to roll back and do the
opposite operation on the one we previously enabled / disabled.
I wonder whether it might damage the hardware or not though.
Mark?
> > +
> > +static int coupled_regulator_is_enabled(struct regulator_dev *rdev)
> > +{
> > + struct coupled_regulator *creg = rdev_get_drvdata(rdev);
> > + int ret = 0, i;
> > +
> > + for (i = 0; i < creg->n_regulators; i++) {
> > + ret &= regulator_is_enabled(creg->regulators[i]);
>
> Why is the '&=' here? Since it is set to '0' from the start, won't it
> always clear all the bits in a potential error return code? Apologies
> if I'm missing something.
It was supposed to be a bitwise or, and not an and, sorry.
But your comment on the error code that might be altered is still
valid though, I'll rework that part.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Attachment:
signature.asc
Description: Digital signature
