| Message ID | 1465895602-31008-13-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 14, 2016 at 11:13:20AM +0200, Boris Brezillon wrote: > The continuous PWM voltage regulator is caching the voltage value in > the ->volt_uV field. While most of the time this value should reflect the > real voltage, sometime it can be sightly different if the PWM device > rounded the set_duty_cycle request. > Moreover, this value is not valid until someone has modified the regulator > output. Acked-by: Mark Brown <broonie@kernel.org>
On Tue, Jun 14, 2016 at 11:13:20AM +0200, Boris Brezillon wrote: > The continuous PWM voltage regulator is caching the voltage value in > the ->volt_uV field. While most of the time this value should reflect the > real voltage, sometime it can be sightly different if the PWM device > rounded the set_duty_cycle request. > Moreover, this value is not valid until someone has modified the regulator > output. > > Remove the ->volt_uV field and always rely on the PWM state to calculate > the regulator output. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > Reviewed-by: Brian Norris <briannorris@chromium.org> > Tested-by: Brian Norris <briannorris@chromium.org> > Tested-by: Heiko Stuebner <heiko@sntech.de> > --- > Mark, > > I know you already added your Tested-by/Acked-by tags on this patch > but this version has slightly change and is now making use of the > pwm_get_relative_duty_cycle() helper instead of manually converting > the absolute duty_cycle value into a relative one. > --- > drivers/regulator/pwm-regulator.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c > index 2000118..80d083f 100644 > --- a/drivers/regulator/pwm-regulator.c > +++ b/drivers/regulator/pwm-regulator.c > @@ -35,9 +35,6 @@ struct pwm_regulator_data { > struct regulator_ops ops; > > int state; > - > - /* Continuous voltage */ > - int volt_uV; > }; > > struct pwm_voltages { > @@ -135,8 +132,13 @@ static int pwm_regulator_is_enabled(struct regulator_dev *dev) > static int pwm_regulator_get_voltage(struct regulator_dev *rdev) > { > struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev); > + int min_uV = rdev->constraints->min_uV; > + int diff = rdev->constraints->max_uV - min_uV; > + struct pwm_state pstate; > > - return drvdata->volt_uV; > + pwm_get_state(drvdata->pwm, &pstate); > + > + return min_uV + pwm_get_relative_duty_cycle(&pstate, diff); > } > > static int pwm_regulator_set_voltage(struct regulator_dev *rdev, > @@ -162,8 +164,6 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev, > return ret; > } > > - drvdata->volt_uV = min_uV; > - > /* Delay required by PWM regulator to settle to the new voltage */ > usleep_range(ramp_delay, ramp_delay + 1000); > This hunk has a minor conflict with the regulator tree and the commit 830583004e61 ("regulator: pwm: Drop unneeded pwm_enable() call") that it contains. Mark, do you want me to provide a stable branch with the PWM regulator patches and resolve that conflict in your tree? Or would you rather take the whole set based on a stable branch from the PWM tree? Or maybe yet another possibility would be to base the PWM tree on a stable branch from the regulator tree containing the above commit. Or we can let Linus sort out the conflict, it's really quite trivial. Thierry
On Sat, Jul 09, 2016 at 11:47:18AM +0200, Mark Brown wrote: > On Fri, Jul 08, 2016 at 05:43:02PM +0200, Thierry Reding wrote: > > > Mark, do you want me to provide a stable branch with the PWM regulator > > patches and resolve that conflict in your tree? Or would you rather take > > the whole set based on a stable branch from the PWM tree? Or maybe yet > > another possibility would be to base the PWM tree on a stable branch > > from the regulator tree containing the above commit. > > Probably easiest to use this signed tag and resolve it in your tree: > > The following changes since commit 1a695a905c18548062509178b98bc91e67510864: > > Linux 4.7-rc1 (2016-05-29 09:29:24 -0700) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git tags/pwm-modernization > > for you to fetch changes up to c2588393e6315ab68207323d37d2a73713d6bc81: > > regulator: pwm: Fix regulator ramp delay for continuous mode (2016-07-07 11:45:06 +0200) > > ---------------------------------------------------------------- > regulator: Provide a branch for moderninzation of the PWM code > > There's a new, improved PWM API which allows a lot of improvements in > the PWM regulator driver. Since the bulk of the changes are in the PWM > API this is being managed in the PWM tree, merge pending regulator API > changes to allow this to be resolved more easily. > > ---------------------------------------------------------------- > Alexandre Courbot (1): > regulator: pwm: Support for enable GPIO > > Boris Brezillon (1): > regulator: pwm: Drop unneeded pwm_enable() call > > Douglas Anderson (1): > regulator: pwm: Fix regulator ramp delay for continuous mode > > .../bindings/regulator/pwm-regulator.txt | 7 +++- > drivers/regulator/pwm-regulator.c | 40 ++++++++++++++++++---- > 2 files changed, 39 insertions(+), 8 deletions(-) Merged into for-4.8/regulator of the PWM tree and rebased Boris' pwm-regulator patches on top. Boris, everything looks right to me, but can you take a quick look to see if it all matches up with what you expect? Thanks, Thierry
On Mon, 11 Jul 2016 09:02:51 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Sat, Jul 09, 2016 at 11:47:18AM +0200, Mark Brown wrote: > > On Fri, Jul 08, 2016 at 05:43:02PM +0200, Thierry Reding wrote: > > > > > Mark, do you want me to provide a stable branch with the PWM regulator > > > patches and resolve that conflict in your tree? Or would you rather take > > > the whole set based on a stable branch from the PWM tree? Or maybe yet > > > another possibility would be to base the PWM tree on a stable branch > > > from the regulator tree containing the above commit. > > > > Probably easiest to use this signed tag and resolve it in your tree: > > > > The following changes since commit 1a695a905c18548062509178b98bc91e67510864: > > > > Linux 4.7-rc1 (2016-05-29 09:29:24 -0700) > > > > are available in the git repository at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git tags/pwm-modernization > > > > for you to fetch changes up to c2588393e6315ab68207323d37d2a73713d6bc81: > > > > regulator: pwm: Fix regulator ramp delay for continuous mode (2016-07-07 11:45:06 +0200) > > > > ---------------------------------------------------------------- > > regulator: Provide a branch for moderninzation of the PWM code > > > > There's a new, improved PWM API which allows a lot of improvements in > > the PWM regulator driver. Since the bulk of the changes are in the PWM > > API this is being managed in the PWM tree, merge pending regulator API > > changes to allow this to be resolved more easily. > > > > ---------------------------------------------------------------- > > Alexandre Courbot (1): > > regulator: pwm: Support for enable GPIO > > > > Boris Brezillon (1): > > regulator: pwm: Drop unneeded pwm_enable() call > > > > Douglas Anderson (1): > > regulator: pwm: Fix regulator ramp delay for continuous mode > > > > .../bindings/regulator/pwm-regulator.txt | 7 +++- > > drivers/regulator/pwm-regulator.c | 40 ++++++++++++++++++---- > > 2 files changed, 39 insertions(+), 8 deletions(-) > > Merged into for-4.8/regulator of the PWM tree and rebased Boris' > pwm-regulator patches on top. > > Boris, everything looks right to me, but can you take a quick look to > see if it all matches up with what you expect? It looks good, but I won't be able to test it on real hardware until next week. Heiko, Brian, can you try this pwm/for-4.8/regulator branch? Thanks, Boris
Hi, On Mon, Jul 11, 2016 at 12:02 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Sat, Jul 09, 2016 at 11:47:18AM +0200, Mark Brown wrote: >> On Fri, Jul 08, 2016 at 05:43:02PM +0200, Thierry Reding wrote: >> >> > Mark, do you want me to provide a stable branch with the PWM regulator >> > patches and resolve that conflict in your tree? Or would you rather take >> > the whole set based on a stable branch from the PWM tree? Or maybe yet >> > another possibility would be to base the PWM tree on a stable branch >> > from the regulator tree containing the above commit. >> >> Probably easiest to use this signed tag and resolve it in your tree: >> >> The following changes since commit 1a695a905c18548062509178b98bc91e67510864: >> >> Linux 4.7-rc1 (2016-05-29 09:29:24 -0700) >> >> are available in the git repository at: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git tags/pwm-modernization >> >> for you to fetch changes up to c2588393e6315ab68207323d37d2a73713d6bc81: >> >> regulator: pwm: Fix regulator ramp delay for continuous mode (2016-07-07 11:45:06 +0200) >> >> ---------------------------------------------------------------- >> regulator: Provide a branch for moderninzation of the PWM code >> >> There's a new, improved PWM API which allows a lot of improvements in >> the PWM regulator driver. Since the bulk of the changes are in the PWM >> API this is being managed in the PWM tree, merge pending regulator API >> changes to allow this to be resolved more easily. >> >> ---------------------------------------------------------------- >> Alexandre Courbot (1): >> regulator: pwm: Support for enable GPIO >> >> Boris Brezillon (1): >> regulator: pwm: Drop unneeded pwm_enable() call >> >> Douglas Anderson (1): >> regulator: pwm: Fix regulator ramp delay for continuous mode >> >> .../bindings/regulator/pwm-regulator.txt | 7 +++- >> drivers/regulator/pwm-regulator.c | 40 ++++++++++++++++++---- >> 2 files changed, 39 insertions(+), 8 deletions(-) > > Merged into for-4.8/regulator of the PWM tree and rebased Boris' > pwm-regulator patches on top. > > Boris, everything looks right to me, but can you take a quick look to > see if it all matches up with what you expect? As I mentioned in the other thread about the linuxnext conflict, pwm_regulator_set_voltage() is wrong. You have: ramp_delay = DIV_ROUND_UP(abs(min_uV - old_uV), ramp_delay); You should have: ramp_delay = DIV_ROUND_UP(abs(req_min_uV - old_uV), ramp_delay); -Doug
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c index 2000118..80d083f 100644 --- a/drivers/regulator/pwm-regulator.c +++ b/drivers/regulator/pwm-regulator.c @@ -35,9 +35,6 @@ struct pwm_regulator_data { struct regulator_ops ops; int state; - - /* Continuous voltage */ - int volt_uV; }; struct pwm_voltages { @@ -135,8 +132,13 @@ static int pwm_regulator_is_enabled(struct regulator_dev *dev) static int pwm_regulator_get_voltage(struct regulator_dev *rdev) { struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev); + int min_uV = rdev->constraints->min_uV; + int diff = rdev->constraints->max_uV - min_uV; + struct pwm_state pstate; - return drvdata->volt_uV; + pwm_get_state(drvdata->pwm, &pstate); + + return min_uV + pwm_get_relative_duty_cycle(&pstate, diff); } static int pwm_regulator_set_voltage(struct regulator_dev *rdev, @@ -162,8 +164,6 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev, return ret; } - drvdata->volt_uV = min_uV; - /* Delay required by PWM regulator to settle to the new voltage */ usleep_range(ramp_delay, ramp_delay + 1000);