Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1412904 > unrolled thread

[PATCH 00/14] regulator: pwm: various improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/14] regulator: pwm: various improvements Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
    [PATCH 05/14] pwm: rockchip: Avoid glitches on already running PWMs Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
      Re: [PATCH 05/14] pwm: rockchip: Avoid glitches on already running  PWMs Brian Norris <briannorris@chromium.org> - 2016-06-03 22:30 +0200
        Re: [PATCH 05/14] pwm: rockchip: Avoid glitches on already running  PWMs Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-04 08:30 +0200
    [PATCH 13/14] regulator: pwm: Support extra continuous mode cases Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
      Re: [PATCH 13/14] regulator: pwm: Support extra continuous mode cases Brian Norris <briannorris@chromium.org> - 2016-06-03 23:10 +0200
        Re: [PATCH 13/14] regulator: pwm: Support extra continuous mode  cases Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-04 08:40 +0200
    [PATCH 07/14] pwm: sti: Add support for hardware readout Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
    [PATCH 08/14] pwm: sti: Avoid glitches on already running PWMs Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
      Re: [PATCH 08/14] pwm: sti: Avoid glitches on already running PWMs Brian Norris <briannorris@chromium.org> - 2016-06-03 22:40 +0200
    [PATCH 12/14] regulator: pwm: Retrieve correct voltage Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
      Re: [PATCH 12/14] regulator: pwm: Retrieve correct voltage Brian Norris <briannorris@chromium.org> - 2016-06-03 23:00 +0200
    [PATCH 14/14] regulator: pwm: Document pwm-dutycycle-unit and pwm-dutycycle-range Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
      Re: [PATCH 14/14] regulator: pwm: Document pwm-dutycycle-unit and  pwm-dutycycle-range Brian Norris <briannorris@chromium.org> - 2016-06-03 23:10 +0200
      Re: [PATCH 14/14] regulator: pwm: Document pwm-dutycycle-unit and  pwm-dutycycle-range Rob Herring <robh@kernel.org> - 2016-06-06 16:20 +0200
    [PATCH 11/14] regulator: pwm: properly initialize the ->state field Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
      Re: [PATCH 11/14] regulator: pwm: properly initialize the ->state  field Brian Norris <briannorris@chromium.org> - 2016-06-03 23:00 +0200
    [PATCH 09/14] regulator: pwm: Adjust PWM config at probe time Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
      Re: [PATCH 09/14] regulator: pwm: Adjust PWM config at probe time Brian Norris <briannorris@chromium.org> - 2016-06-03 22:50 +0200
    [PATCH 04/14] pwm: rockchip: Add support for hardware readout Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:40 +0200
      Re: [PATCH 04/14] pwm: rockchip: Add support for hardware readout Brian Norris <briannorris@chromium.org> - 2016-06-03 22:10 +0200
      Re: [PATCH 04/14] pwm: rockchip: Add support for hardware readout Brian Norris <briannorris@chromium.org> - 2016-06-03 22:30 +0200
        Re: [PATCH 04/14] pwm: rockchip: Add support for hardware readout Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-04 08:30 +0200
          Re: [PATCH 04/14] pwm: rockchip: Add support for hardware readout Brian Norris <briannorris@chromium.org> - 2016-06-07 19:30 +0200

Page 1 of 2  [1] 2  Next page →

#1412904 — [PATCH 00/14] regulator: pwm: various improvements

Hello,

This patch series series aims at adding two important features to the
pwm-regulator driver.

The first one is the support for 'smooth handover' between the
bootloader and the kernel. This is mainly solving problems we have when
the PWM is controlling a critical regulator (like the one powering the
DDR chip). Currently, when the PWM regulator acquire the PWM device it
assumes it was off and it's safe to change the configuration before
enabling it, which can generate glitches on the PWM signal which in turn
generated glitches on the output voltage.
To solve that we've introduced support for hardware readout to the
PWM framework, so that the PWM regulator driver can adjust the PWM
a probe time and avoid glitches.
Atomic update is also helping in this regard.

Patch 1 is adding convenient helpers (at the PWM level) that will be
used by the PWM regulator driver.
Patch 2 is just removing the pwm_enable() call from the ->set_voltage()
implementation (pwm_enable() is already called in the reg->enable()
hook).
Patches 3 to 8 are preparing everything on the PWM driver side to make
hardware readout available to all platforms using the PWM regulator
driver (rockchip and sti).
Patches 9 to 12 are making use of the atomic update and hardware readout
features to implement this smooth handover.

The second feature we add to the driver is the capability of using
a sub duty_cycle range in continuous mode. By default the regulator
is assuming that min_uV is achieved with a 0% dutycyle and max_uV
with a 100% dutycycle, but this is not necessarily true.
Moreover, in some cases (when the PWM device does not support
polarity inversion), we might have min_uV at 100% and max_uV at 0%.
Hence the addition of new properties to the existing DT bindings.
The feature is added in patch 13 and 14.

Best Regards,

Boris

Boris Brezillon (14):
  pwm: Add new helpers to create/manipulate PWM states
  regulator: pwm: Drop unneeded pwm_enable() call
  pwm: rockchip: Fix period and duty_cycle approximation
  pwm: rockchip: Add support for hardware readout
  pwm: rockchip: Avoid glitches on already running PWMs
  pwm: rockchip: Add support for atomic update
  pwm: sti: Add support for hardware readout
  pwm: sti: Avoid glitches on already running PWMs
  regulator: pwm: Adjust PWM config at probe time
  regulator: pwm: Switch to the atomic PWM API
  regulator: pwm: properly initialize the ->state field
  regulator: pwm: Retrieve correct voltage
  regulator: pwm: Support extra continuous mode cases
  regulator: pwm: Document pwm-dutycycle-unit and pwm-dutycycle-range

 .../bindings/regulator/pwm-regulator.txt           |  12 ++
 drivers/pwm/pwm-rockchip.c                         | 182 +++++++++++++++------
 drivers/pwm/pwm-sti.c                              |  55 ++++++-
 drivers/regulator/pwm-regulator.c                  | 159 ++++++++++++------
 include/linux/pwm.h                                |  81 +++++++++
 5 files changed, 391 insertions(+), 98 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1412905 — [PATCH 05/14] pwm: rockchip: Avoid glitches on already running PWMs

The current logic will disable the PWM clk even if the PWM was left
enabled by the bootloader (because it's controlling a critical device
like a regulator for example).
Keep the PWM clk enabled if the PWM is enabled to avoid any glitches.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/pwm-rockchip.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index dfacf7d..798a787 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -299,6 +299,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *id;
 	struct rockchip_pwm_chip *pc;
+	struct pwm_state state;
 	struct resource *r;
 	int ret;
 
@@ -319,7 +320,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(pc->clk))
 		return PTR_ERR(pc->clk);
 
-	ret = clk_prepare(pc->clk);
+	ret = clk_prepare_enable(pc->clk);
 	if (ret)
 		return ret;
 
@@ -342,12 +343,33 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
 	}
 
+	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
+	pwm_get_state(pc->chip.pwms, &state);
+	if (!state.enabled)
+		clk_disable(pc->clk);
+
 	return ret;
 }
 
 static int rockchip_pwm_remove(struct platform_device *pdev)
 {
 	struct rockchip_pwm_chip *pc = platform_get_drvdata(pdev);
+	struct pwm_state state;
+
+	/*
+	 * Disable the PWM clk before unpreparing it if the PWM device is still
+	 * running. This should only happen when the last PWM user left it
+	 * enabled, or when nobody requested a PWM that was previously enabled
+	 * by the bootloader.
+	 *
+	 * FIXME: Maybe the core should disable all PWM devices in
+	 * pwmchip_remove(). In this case we'd only have to call
+	 * clk_unprepare() after pwmchip_remove().
+	 *
+	 */
+	pwm_get_state(pc->chip.pwms, &state);
+	if (state.enabled)
+		clk_disable(pc->clk);
 
 	clk_unprepare(pc->clk);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1413451 — Re: [PATCH 05/14] pwm: rockchip: Avoid glitches on already running PWMs

Hi,

Just noticed a few things:

On Fri, Jun 03, 2016 at 10:23:03AM +0200, Boris Brezillon wrote:
> The current logic will disable the PWM clk even if the PWM was left
> enabled by the bootloader (because it's controlling a critical device
> like a regulator for example).
> Keep the PWM clk enabled if the PWM is enabled to avoid any glitches.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/pwm-rockchip.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index dfacf7d..798a787 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -299,6 +299,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *id;
>  	struct rockchip_pwm_chip *pc;
> +	struct pwm_state state;
>  	struct resource *r;
>  	int ret;
>  
> @@ -319,7 +320,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(pc->clk))
>  		return PTR_ERR(pc->clk);
>  
> -	ret = clk_prepare(pc->clk);
> +	ret = clk_prepare_enable(pc->clk);
>  	if (ret)
>  		return ret;
>  
> @@ -342,12 +343,33 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
>  	}
>  
> +	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
> +	pwm_get_state(pc->chip.pwms, &state);
> +	if (!state.enabled)

Why not just if (!pwm_is_enabled())?

> +		clk_disable(pc->clk);
> +
>  	return ret;
>  }
>  
>  static int rockchip_pwm_remove(struct platform_device *pdev)
>  {
>  	struct rockchip_pwm_chip *pc = platform_get_drvdata(pdev);
> +	struct pwm_state state;
> +
> +	/*
> +	 * Disable the PWM clk before unpreparing it if the PWM device is still
> +	 * running. This should only happen when the last PWM user left it
> +	 * enabled, or when nobody requested a PWM that was previously enabled
> +	 * by the bootloader.
> +	 *
> +	 * FIXME: Maybe the core should disable all PWM devices in
> +	 * pwmchip_remove(). In this case we'd only have to call
> +	 * clk_unprepare() after pwmchip_remove().
> +	 *
> +	 */
> +	pwm_get_state(pc->chip.pwms, &state);
> +	if (state.enabled)

Same here.

With that:

Reviewed-by: Brian Norris <briannorris@chromium.org>

And it tests out fine:

Tested-by: Brian Norris <briannorris@chromium.org>


> +		clk_disable(pc->clk);
>  
>  	clk_unprepare(pc->clk);
>  
> -- 
> 2.7.4
> 

[toc] | [prev] | [next] | [standalone]

#1413662 — Re: [PATCH 05/14] pwm: rockchip: Avoid glitches on already running PWMs

On Fri, 3 Jun 2016 13:28:59 -0700
Brian Norris <briannorris@chromium.org> wrote:

> Hi,
> 
> Just noticed a few things:
> 
> On Fri, Jun 03, 2016 at 10:23:03AM +0200, Boris Brezillon wrote:
> > The current logic will disable the PWM clk even if the PWM was left
> > enabled by the bootloader (because it's controlling a critical device
> > like a regulator for example).
> > Keep the PWM clk enabled if the PWM is enabled to avoid any glitches.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/pwm/pwm-rockchip.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> > index dfacf7d..798a787 100644
> > --- a/drivers/pwm/pwm-rockchip.c
> > +++ b/drivers/pwm/pwm-rockchip.c
> > @@ -299,6 +299,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
> >  {
> >  	const struct of_device_id *id;
> >  	struct rockchip_pwm_chip *pc;
> > +	struct pwm_state state;
> >  	struct resource *r;
> >  	int ret;
> >  
> > @@ -319,7 +320,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
> >  	if (IS_ERR(pc->clk))
> >  		return PTR_ERR(pc->clk);
> >  
> > -	ret = clk_prepare(pc->clk);
> > +	ret = clk_prepare_enable(pc->clk);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -342,12 +343,33 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
> >  		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> >  	}
> >  
> > +	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
> > +	pwm_get_state(pc->chip.pwms, &state);
> > +	if (!state.enabled)  
> 
> Why not just if (!pwm_is_enabled())?

It's a leftover from a previous version where I was deprecating
pwm_enable(). I'll switch back to pwm_enable().

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1412906 — [PATCH 13/14] regulator: pwm: Support extra continuous mode cases

The continuous mode allows one to declare a PWM regulator without having
to declare the voltage <-> dutycycle association table. It works fine as
long as your voltage(dutycycle) function is linear, but also has the
following constraints:

- dutycycle for min_uV = 0%
- dutycycle for max_uV = 100%
- dutycycle for min_uV < dutycycle for max_uV

While the linearity constraint is acceptable for now, we sometimes need to
restrict of the PWM range (to limit the maximum/minimum voltage for
example) or have a min_uV_dutycycle > max_uV_dutycycle (this could be
tweaked with PWM polarity, but not all PWMs support inverted polarity).

Add the pwm-dutycycle-range and pwm-dutycycle-unit DT properties to define
such constraints. If those properties are not defined, the PWM regulator
use the default pwm-dutycycle-range = <0 100> and
pwm-dutycycle-unit = <100> values (existing behavior).

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/regulator/pwm-regulator.c | 80 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 71 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index c39ecd1..2e70eb1 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -21,6 +21,12 @@
 #include <linux/of_device.h>
 #include <linux/pwm.h>
 
+struct pwm_continuous_reg_data {
+	unsigned int min_uV_dutycycle;
+	unsigned int max_uV_dutycycle;
+	unsigned int dutycycle_unit;
+};
+
 struct pwm_regulator_data {
 	/*  Shared */
 	struct pwm_device *pwm;
@@ -28,6 +34,9 @@ struct pwm_regulator_data {
 	/* Voltage table */
 	struct pwm_voltages *duty_cycle_table;
 
+	/* Continuous mode info */
+	struct pwm_continuous_reg_data continuous;
+
 	/* regulator descriptor */
 	struct regulator_desc desc;
 
@@ -132,31 +141,67 @@ 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);
+	unsigned int min_uV_duty = drvdata->continuous.min_uV_dutycycle;
+	unsigned int max_uV_duty = drvdata->continuous.max_uV_dutycycle;
+	unsigned int duty_unit = drvdata->continuous.dutycycle_unit;
 	int min_uV = rdev->constraints->min_uV;
-	int diff = rdev->constraints->max_uV - min_uV;
+	int max_uV = rdev->constraints->max_uV;
+	int diff_uV = max_uV - min_uV;
 	struct pwm_state pstate;
+	unsigned int diff_duty;
+	unsigned int voltage;
 
 	pwm_get_state(drvdata->pwm, &pstate);
 
-	return min_uV + pwm_get_relative_duty_cycle(&pstate, diff);
+	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
+
+	if (max_uV_duty < min_uV_duty) {
+		voltage = min_uV_duty - voltage;
+		diff_duty = min_uV_duty - max_uV_duty;
+	} else {
+		voltage = voltage - min_uV_duty;
+		diff_duty = max_uV_duty - min_uV_duty;
+	}
+
+	voltage = DIV_ROUND_CLOSEST_ULL((u64)voltage * diff_uV, diff_duty);
+
+	return voltage + min_uV;
 }
 
 static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
-					int min_uV, int max_uV,
-					unsigned *selector)
+				     int req_min_uV, int req_max_uV,
+				     unsigned int *selector)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
+	unsigned int min_uV_duty = drvdata->continuous.min_uV_dutycycle;
+	unsigned int max_uV_duty = drvdata->continuous.max_uV_dutycycle;
+	unsigned int duty_unit = drvdata->continuous.dutycycle_unit;
 	unsigned int ramp_delay = rdev->constraints->ramp_delay;
-	unsigned int req_diff = min_uV - rdev->constraints->min_uV;
+	int min_uV = rdev->constraints->min_uV;
+	int max_uV = rdev->constraints->max_uV;
+	int diff_uV = max_uV - min_uV;
 	struct pwm_state pstate;
-	unsigned int diff;
+	unsigned int diff_duty;
+	unsigned int dutycycle;
 	int ret;
 
 	pwm_prepare_new_state(drvdata->pwm, &pstate);
-	diff = rdev->constraints->max_uV - rdev->constraints->min_uV;
 
-	/* We pass diff as the scale to get a uV precision. */
-	pwm_set_relative_duty_cycle(&pstate, req_diff, diff);
+	if (max_uV_duty < min_uV_duty)
+		diff_duty = min_uV_duty - max_uV_duty;
+	else
+		diff_duty = max_uV_duty - min_uV_duty;
+
+	dutycycle = DIV_ROUND_CLOSEST_ULL((u64)(req_min_uV - min_uV) *
+					  diff_duty,
+					  diff_uV);
+
+	if (max_uV_duty < min_uV_duty)
+		dutycycle = min_uV_duty - dutycycle;
+	else
+		dutycycle = min_uV_duty + dutycycle;
+
+	pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
 
 	ret = pwm_apply_state(drvdata->pwm, &pstate);
 	if (ret) {
@@ -237,11 +282,28 @@ static int pwm_regulator_init_table(struct platform_device *pdev,
 static int pwm_regulator_init_continuous(struct platform_device *pdev,
 					 struct pwm_regulator_data *drvdata)
 {
+	u32 dutycycle_range[2] = { 0, 100 };
+	u32 dutycycle_unit = 100;
+
 	memcpy(&drvdata->ops, &pwm_regulator_voltage_continuous_ops,
 	       sizeof(drvdata->ops));
 	drvdata->desc.ops = &drvdata->ops;
 	drvdata->desc.continuous_voltage_range = true;
 
+	of_property_read_u32_array(pdev->dev.of_node,
+				   "pwm-dutycycle-range",
+				   dutycycle_range, 2);
+	of_property_read_u32(pdev->dev.of_node, "pwm-dutycycle-unit",
+			     &dutycycle_unit);
+
+	if (dutycycle_range[0] > dutycycle_unit ||
+	    dutycycle_range[1] > dutycycle_unit)
+		return -EINVAL;
+
+	drvdata->continuous.dutycycle_unit = dutycycle_unit;
+	drvdata->continuous.min_uV_dutycycle = dutycycle_range[0];
+	drvdata->continuous.max_uV_dutycycle = dutycycle_range[1];
+
 	return 0;
 }
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1413482 — Re: [PATCH 13/14] regulator: pwm: Support extra continuous mode cases

Hi Boris,

On Fri, Jun 03, 2016 at 10:23:11AM +0200, Boris Brezillon wrote:
> The continuous mode allows one to declare a PWM regulator without having
> to declare the voltage <-> dutycycle association table. It works fine as
> long as your voltage(dutycycle) function is linear, but also has the
> following constraints:
> 
> - dutycycle for min_uV = 0%
> - dutycycle for max_uV = 100%
> - dutycycle for min_uV < dutycycle for max_uV
> 
> While the linearity constraint is acceptable for now, we sometimes need to
> restrict of the PWM range (to limit the maximum/minimum voltage for
> example) or have a min_uV_dutycycle > max_uV_dutycycle (this could be
> tweaked with PWM polarity, but not all PWMs support inverted polarity).
> 
> Add the pwm-dutycycle-range and pwm-dutycycle-unit DT properties to define
> such constraints. If those properties are not defined, the PWM regulator
> use the default pwm-dutycycle-range = <0 100> and
> pwm-dutycycle-unit = <100> values (existing behavior).
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/regulator/pwm-regulator.c | 80 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 71 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index c39ecd1..2e70eb1 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c

...

> @@ -132,31 +141,67 @@ 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);
> +	unsigned int min_uV_duty = drvdata->continuous.min_uV_dutycycle;
> +	unsigned int max_uV_duty = drvdata->continuous.max_uV_dutycycle;
> +	unsigned int duty_unit = drvdata->continuous.dutycycle_unit;
>  	int min_uV = rdev->constraints->min_uV;
> -	int diff = rdev->constraints->max_uV - min_uV;
> +	int max_uV = rdev->constraints->max_uV;
> +	int diff_uV = max_uV - min_uV;
>  	struct pwm_state pstate;
> +	unsigned int diff_duty;
> +	unsigned int voltage;
>  
>  	pwm_get_state(drvdata->pwm, &pstate);
>  
> -	return min_uV + pwm_get_relative_duty_cycle(&pstate, diff);
> +	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> +
> +	if (max_uV_duty < min_uV_duty) {

I still might have appreciated a comment above this line (and similar
in set_voltage()) to help explain why max can be less than min -- you
have it in the commit message, but nowhere in the code. Not a big deal,
and the code looks otherwise good:

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>


> +		voltage = min_uV_duty - voltage;
> +		diff_duty = min_uV_duty - max_uV_duty;
> +	} else {
> +		voltage = voltage - min_uV_duty;
> +		diff_duty = max_uV_duty - min_uV_duty;
> +	}
> +
> +	voltage = DIV_ROUND_CLOSEST_ULL((u64)voltage * diff_uV, diff_duty);
> +
> +	return voltage + min_uV;
>  }
>  
>  static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
> -					int min_uV, int max_uV,
> -					unsigned *selector)
> +				     int req_min_uV, int req_max_uV,
> +				     unsigned int *selector)
>  {
>  	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
> +	unsigned int min_uV_duty = drvdata->continuous.min_uV_dutycycle;
> +	unsigned int max_uV_duty = drvdata->continuous.max_uV_dutycycle;
> +	unsigned int duty_unit = drvdata->continuous.dutycycle_unit;
>  	unsigned int ramp_delay = rdev->constraints->ramp_delay;
> -	unsigned int req_diff = min_uV - rdev->constraints->min_uV;
> +	int min_uV = rdev->constraints->min_uV;
> +	int max_uV = rdev->constraints->max_uV;
> +	int diff_uV = max_uV - min_uV;
>  	struct pwm_state pstate;
> -	unsigned int diff;
> +	unsigned int diff_duty;
> +	unsigned int dutycycle;
>  	int ret;
>  
>  	pwm_prepare_new_state(drvdata->pwm, &pstate);
> -	diff = rdev->constraints->max_uV - rdev->constraints->min_uV;
>  
> -	/* We pass diff as the scale to get a uV precision. */
> -	pwm_set_relative_duty_cycle(&pstate, req_diff, diff);
> +	if (max_uV_duty < min_uV_duty)
> +		diff_duty = min_uV_duty - max_uV_duty;
> +	else
> +		diff_duty = max_uV_duty - min_uV_duty;
> +
> +	dutycycle = DIV_ROUND_CLOSEST_ULL((u64)(req_min_uV - min_uV) *
> +					  diff_duty,
> +					  diff_uV);
> +
> +	if (max_uV_duty < min_uV_duty)
> +		dutycycle = min_uV_duty - dutycycle;
> +	else
> +		dutycycle = min_uV_duty + dutycycle;
> +
> +	pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
>  
>  	ret = pwm_apply_state(drvdata->pwm, &pstate);
>  	if (ret) {

[...]

Brian

[toc] | [prev] | [next] | [standalone]

#1413666 — Re: [PATCH 13/14] regulator: pwm: Support extra continuous mode cases

On Fri, 3 Jun 2016 14:03:08 -0700
Brian Norris <briannorris@chromium.org> wrote:

> Hi Boris,
> 
> On Fri, Jun 03, 2016 at 10:23:11AM +0200, Boris Brezillon wrote:
> > The continuous mode allows one to declare a PWM regulator without having
> > to declare the voltage <-> dutycycle association table. It works fine as
> > long as your voltage(dutycycle) function is linear, but also has the
> > following constraints:
> > 
> > - dutycycle for min_uV = 0%
> > - dutycycle for max_uV = 100%
> > - dutycycle for min_uV < dutycycle for max_uV
> > 
> > While the linearity constraint is acceptable for now, we sometimes need to
> > restrict of the PWM range (to limit the maximum/minimum voltage for
> > example) or have a min_uV_dutycycle > max_uV_dutycycle (this could be
> > tweaked with PWM polarity, but not all PWMs support inverted polarity).
> > 
> > Add the pwm-dutycycle-range and pwm-dutycycle-unit DT properties to define
> > such constraints. If those properties are not defined, the PWM regulator
> > use the default pwm-dutycycle-range = <0 100> and
> > pwm-dutycycle-unit = <100> values (existing behavior).
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/regulator/pwm-regulator.c | 80 ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 71 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> > index c39ecd1..2e70eb1 100644
> > --- a/drivers/regulator/pwm-regulator.c
> > +++ b/drivers/regulator/pwm-regulator.c  
> 
> ...
> 
> > @@ -132,31 +141,67 @@ 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);
> > +	unsigned int min_uV_duty = drvdata->continuous.min_uV_dutycycle;
> > +	unsigned int max_uV_duty = drvdata->continuous.max_uV_dutycycle;
> > +	unsigned int duty_unit = drvdata->continuous.dutycycle_unit;
> >  	int min_uV = rdev->constraints->min_uV;
> > -	int diff = rdev->constraints->max_uV - min_uV;
> > +	int max_uV = rdev->constraints->max_uV;
> > +	int diff_uV = max_uV - min_uV;
> >  	struct pwm_state pstate;
> > +	unsigned int diff_duty;
> > +	unsigned int voltage;
> >  
> >  	pwm_get_state(drvdata->pwm, &pstate);
> >  
> > -	return min_uV + pwm_get_relative_duty_cycle(&pstate, diff);
> > +	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> > +
> > +	if (max_uV_duty < min_uV_duty) {  
> 
> I still might have appreciated a comment above this line (and similar
> in set_voltage()) to help explain why max can be less than min -- you
> have it in the commit message, but nowhere in the code. Not a big deal,
> and the code looks otherwise good:

Sure, I'll add more comments in the next version.

Thanks,

Boris

> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> 
> 
> > +		voltage = min_uV_duty - voltage;
> > +		diff_duty = min_uV_duty - max_uV_duty;
> > +	} else {
> > +		voltage = voltage - min_uV_duty;
> > +		diff_duty = max_uV_duty - min_uV_duty;
> > +	}
> > +
> > +	voltage = DIV_ROUND_CLOSEST_ULL((u64)voltage * diff_uV, diff_duty);
> > +
> > +	return voltage + min_uV;
> >  }
> >  
> >  static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
> > -					int min_uV, int max_uV,
> > -					unsigned *selector)
> > +				     int req_min_uV, int req_max_uV,
> > +				     unsigned int *selector)
> >  {
> >  	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
> > +	unsigned int min_uV_duty = drvdata->continuous.min_uV_dutycycle;
> > +	unsigned int max_uV_duty = drvdata->continuous.max_uV_dutycycle;
> > +	unsigned int duty_unit = drvdata->continuous.dutycycle_unit;
> >  	unsigned int ramp_delay = rdev->constraints->ramp_delay;
> > -	unsigned int req_diff = min_uV - rdev->constraints->min_uV;
> > +	int min_uV = rdev->constraints->min_uV;
> > +	int max_uV = rdev->constraints->max_uV;
> > +	int diff_uV = max_uV - min_uV;
> >  	struct pwm_state pstate;
> > -	unsigned int diff;
> > +	unsigned int diff_duty;
> > +	unsigned int dutycycle;
> >  	int ret;
> >  
> >  	pwm_prepare_new_state(drvdata->pwm, &pstate);
> > -	diff = rdev->constraints->max_uV - rdev->constraints->min_uV;
> >  
> > -	/* We pass diff as the scale to get a uV precision. */
> > -	pwm_set_relative_duty_cycle(&pstate, req_diff, diff);
> > +	if (max_uV_duty < min_uV_duty)
> > +		diff_duty = min_uV_duty - max_uV_duty;
> > +	else
> > +		diff_duty = max_uV_duty - min_uV_duty;
> > +
> > +	dutycycle = DIV_ROUND_CLOSEST_ULL((u64)(req_min_uV - min_uV) *
> > +					  diff_duty,
> > +					  diff_uV);
> > +
> > +	if (max_uV_duty < min_uV_duty)
> > +		dutycycle = min_uV_duty - dutycycle;
> > +	else
> > +		dutycycle = min_uV_duty + dutycycle;
> > +
> > +	pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
> >  
> >  	ret = pwm_apply_state(drvdata->pwm, &pstate);
> >  	if (ret) {  
> 
> [...]
> 
> Brian



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1412907 — [PATCH 07/14] pwm: sti: Add support for hardware readout

Implement ->get_state() to provide support for initial state retrieval.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/pwm-sti.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 92abbd5..6300d3e 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -238,6 +238,43 @@ static void sti_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	mutex_unlock(&pc->sti_pwm_lock);
 }
 
+static void sti_pwm_get_state(struct pwm_chip *chip,
+			      struct pwm_device *pwm,
+			      struct pwm_state *state)
+{
+	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
+	unsigned int regval, prescaler;
+	int ret;
+
+	/* The clock has to be enabled to access PWM registers */
+	ret = clk_enable(pc->clk);
+	if (ret) {
+		dev_err(chip->dev, "Failed to enable PWM clk");
+		return;
+	}
+
+	regmap_field_read(pc->prescale_high, &regval);
+	prescaler = regval << 4;
+	regmap_field_read(pc->prescale_low, &regval);
+	prescaler |= regval;
+	state->period = DIV_ROUND_CLOSEST_ULL((u64)(prescaler + 1) *
+					      NSEC_PER_SEC *
+					      (pc->cdata->max_pwm_cnt + 1),
+					      pc->clk_rate);
+
+	regmap_read(pc->regmap, STI_DS_REG(pwm->hwpwm), &regval);
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)(regval + 1) *
+						  state->period,
+						  pc->cdata->max_pwm_cnt + 1);
+
+	regmap_field_read(pc->pwm_en, &regval);
+	state->enabled = regval;
+
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	clk_disable(pc->clk);
+}
+
 static void sti_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
@@ -249,6 +286,7 @@ static const struct pwm_ops sti_pwm_ops = {
 	.config = sti_pwm_config,
 	.enable = sti_pwm_enable,
 	.disable = sti_pwm_disable,
+	.get_state = sti_pwm_get_state,
 	.free = sti_pwm_free,
 	.owner = THIS_MODULE,
 };
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1412910 — [PATCH 08/14] pwm: sti: Avoid glitches on already running PWMs

The current logic will disable the PWM clk even if a PWM was left
enabled by the bootloader (because it's controlling a critical device
like a regulator for example).
Keep the PWM clk enabled if at least one PWM is enabled to avoid any
glitches.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/pwm-sti.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 6300d3e..8232c5b 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -340,7 +340,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	struct sti_pwm_compat_data *cdata;
 	struct sti_pwm_chip *pc;
 	struct resource *res;
-	int ret;
+	int i, ret;
 
 	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc)
@@ -391,7 +391,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	ret = clk_prepare(pc->clk);
+	ret = clk_prepare_enable(pc->clk);
 	if (ret) {
 		dev_err(dev, "failed to prepare clock\n");
 		return ret;
@@ -409,6 +409,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/*
+	 * Keep the PWM clk enabled if some PWMs appear to be up and
+	 * running.
+	 */
+	for (i = 0; i < pc->chip.npwm; i++) {
+		struct pwm_state state;
+
+		pwm_get_state(&pc->chip.pwms[i], &state);
+		if (state.enabled)
+			clk_enable(pc->clk);
+	}
+	clk_disable(pc->clk);
+
 	platform_set_drvdata(pdev, pc);
 
 	return 0;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1413459 — Re: [PATCH 08/14] pwm: sti: Avoid glitches on already running PWMs

On Fri, Jun 03, 2016 at 10:23:06AM +0200, Boris Brezillon wrote:
> The current logic will disable the PWM clk even if a PWM was left
> enabled by the bootloader (because it's controlling a critical device
> like a regulator for example).
> Keep the PWM clk enabled if at least one PWM is enabled to avoid any
> glitches.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/pwm-sti.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> index 6300d3e..8232c5b 100644
> --- a/drivers/pwm/pwm-sti.c
> +++ b/drivers/pwm/pwm-sti.c
> @@ -340,7 +340,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  	struct sti_pwm_compat_data *cdata;
>  	struct sti_pwm_chip *pc;
>  	struct resource *res;
> -	int ret;
> +	int i, ret;
>  
>  	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
>  	if (!pc)
> @@ -391,7 +391,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	ret = clk_prepare(pc->clk);
> +	ret = clk_prepare_enable(pc->clk);
>  	if (ret) {
>  		dev_err(dev, "failed to prepare clock\n");
>  		return ret;
> @@ -409,6 +409,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	/*
> +	 * Keep the PWM clk enabled if some PWMs appear to be up and
> +	 * running.
> +	 */
> +	for (i = 0; i < pc->chip.npwm; i++) {
> +		struct pwm_state state;
> +
> +		pwm_get_state(&pc->chip.pwms[i], &state);
> +		if (state.enabled)

Similar to pwm-rockchip: you could just do if (pwm_is_enabled(...)).

Brian

> +			clk_enable(pc->clk);
> +	}
> +	clk_disable(pc->clk);
> +
>  	platform_set_drvdata(pdev, pc);
>  
>  	return 0;
> -- 
> 2.7.4
> 

[toc] | [prev] | [next] | [standalone]

#1412912 — [PATCH 12/14] regulator: pwm: Retrieve correct voltage

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>
---
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 8c56e16..c39ecd1 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);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1413471 — Re: [PATCH 12/14] regulator: pwm: Retrieve correct voltage

On Fri, Jun 03, 2016 at 10:23:10AM +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>
> ---
> 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 8c56e16..c39ecd1 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);
>  

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

[toc] | [prev] | [next] | [standalone]

#1412914 — [PATCH 14/14] regulator: pwm: Document pwm-dutycycle-unit and pwm-dutycycle-range

Document the pwm-dutycycle-unit and pwm-dutycycle-range properties.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 .../devicetree/bindings/regulator/pwm-regulator.txt          | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
index ed936f0..5303235 100644
--- a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
@@ -34,6 +34,18 @@ Only required for Voltage Table Mode:
 			    First cell is voltage in microvolts (uV)
 			    Second cell is duty-cycle in percent (%)
 
+Optional properties for Continuous mode:
+- pwm-dutycycle-unit:	Integer value encoding the dutycycle unit. If not
+			defined, <100> is assumed, meaning that
+			pwm-dutycycle-range contains values expressed in
+			percent.
+
+- pwm-dutycycle-range:	Should contain 2 entries. The first entry is encoding
+			the dutycycle for regulator-min-microvolt and the
+			second one the dutycycle for regulator-max-microvolt.
+			Dutycyle values are expressed in pwm-dutycycle-unit.
+			If not defined, <0 100> is assumed.
+
 NB: To be clear, if voltage-table is provided, then the device will be used
 in Voltage Table Mode.  If no voltage-table is provided, then the device will
 be used in Continuous Voltage Mode.
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1413487 — Re: [PATCH 14/14] regulator: pwm: Document pwm-dutycycle-unit and pwm-dutycycle-range

On Fri, Jun 03, 2016 at 10:23:12AM +0200, Boris Brezillon wrote:
> Document the pwm-dutycycle-unit and pwm-dutycycle-range properties.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  .../devicetree/bindings/regulator/pwm-regulator.txt          | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> index ed936f0..5303235 100644
> --- a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> @@ -34,6 +34,18 @@ Only required for Voltage Table Mode:
>  			    First cell is voltage in microvolts (uV)
>  			    Second cell is duty-cycle in percent (%)
>  
> +Optional properties for Continuous mode:
> +- pwm-dutycycle-unit:	Integer value encoding the dutycycle unit. If not
> +			defined, <100> is assumed, meaning that
> +			pwm-dutycycle-range contains values expressed in
> +			percent.
> +
> +- pwm-dutycycle-range:	Should contain 2 entries. The first entry is encoding
> +			the dutycycle for regulator-min-microvolt and the
> +			second one the dutycycle for regulator-max-microvolt.
> +			Dutycyle values are expressed in pwm-dutycycle-unit.
> +			If not defined, <0 100> is assumed.
> +
>  NB: To be clear, if voltage-table is provided, then the device will be used
>  in Voltage Table Mode.  If no voltage-table is provided, then the device will
>  be used in Continuous Voltage Mode.

Acked-by: Brian Norris <briannorris@chromium.org>

[toc] | [prev] | [next] | [standalone]

#1415103 — Re: [PATCH 14/14] regulator: pwm: Document pwm-dutycycle-unit and pwm-dutycycle-range

On Fri, Jun 03, 2016 at 10:23:12AM +0200, Boris Brezillon wrote:
> Document the pwm-dutycycle-unit and pwm-dutycycle-range properties.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  .../devicetree/bindings/regulator/pwm-regulator.txt          | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> index ed936f0..5303235 100644
> --- a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> @@ -34,6 +34,18 @@ Only required for Voltage Table Mode:
>  			    First cell is voltage in microvolts (uV)
>  			    Second cell is duty-cycle in percent (%)
>  
> +Optional properties for Continuous mode:
> +- pwm-dutycycle-unit:	Integer value encoding the dutycycle unit. If not

dutycycle should be 2 words.

> +			defined, <100> is assumed, meaning that
> +			pwm-dutycycle-range contains values expressed in
> +			percent.

Perhaps an example not in percent. This is essentially the maximum 
value?

> +
> +- pwm-dutycycle-range:	Should contain 2 entries. The first entry is encoding
> +			the dutycycle for regulator-min-microvolt and the
> +			second one the dutycycle for regulator-max-microvolt.
> +			Dutycyle values are expressed in pwm-dutycycle-unit.

typo                    ^^^^^^^^

> +			If not defined, <0 100> is assumed.
> +
>  NB: To be clear, if voltage-table is provided, then the device will be used
>  in Voltage Table Mode.  If no voltage-table is provided, then the device will
>  be used in Continuous Voltage Mode.
> -- 
> 2.7.4
> 

[toc] | [prev] | [next] | [standalone]

#1412915 — [PATCH 11/14] regulator: pwm: properly initialize the ->state field

The ->state field is currently initialized to 0, thus referencing the
voltage selector at index 0, which might not reflect the current voltage
value.
If possible, retrieve the current voltage selector from the PWM state, else
return -EINVAL.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
Heiko, 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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index bf033fd..8c56e16 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -48,10 +48,31 @@ struct pwm_voltages {
 /**
  * Voltage table call-backs
  */
+static void pwm_regulator_init_state(struct regulator_dev *rdev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
+	struct pwm_state pwm_state;
+	unsigned int dutycycle;
+	int i;
+
+	pwm_get_state(drvdata->pwm, &pwm_state);
+	dutycycle = pwm_get_relative_duty_cycle(&pwm_state, 100);
+
+	for (i = 0; i < rdev->desc->n_voltages; i++) {
+		if (dutycycle == drvdata->duty_cycle_table[i].dutycycle) {
+			drvdata->state = i;
+			return;
+		}
+	}
+}
+
 static int pwm_regulator_get_voltage_sel(struct regulator_dev *rdev)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
 
+	if (drvdata->state < 0)
+		pwm_regulator_init_state(rdev);
+
 	return drvdata->state;
 }
 
@@ -203,6 +224,7 @@ static int pwm_regulator_init_table(struct platform_device *pdev,
 		return ret;
 	}
 
+	drvdata->state			= -EINVAL;
 	drvdata->duty_cycle_table	= duty_cycle_table;
 	memcpy(&drvdata->ops, &pwm_regulator_voltage_table_ops,
 	       sizeof(drvdata->ops));
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1413472 — Re: [PATCH 11/14] regulator: pwm: properly initialize the ->state field

On Fri, Jun 03, 2016 at 10:23:09AM +0200, Boris Brezillon wrote:
> The ->state field is currently initialized to 0, thus referencing the
> voltage selector at index 0, which might not reflect the current voltage
> value.
> If possible, retrieve the current voltage selector from the PWM state, else
> return -EINVAL.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Heiko, 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 | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

Tested-by: Brian Norris <briannorris@chromium.org>

[toc] | [prev] | [next] | [standalone]

#1412917 — [PATCH 09/14] regulator: pwm: Adjust PWM config at probe time

The PWM attached to a PWM regulator device might have been previously
configured by the bootloader.
Make sure the bootloader and linux config are in sync, and adjust the PWM
config if that's not the case.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/pwm-regulator.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index ab3cc02..524b43f 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -285,11 +285,9 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to the
-	 * atomic PWM API.
-	 */
-	pwm_apply_args(drvdata->pwm);
+	ret = pwm_adjust_config(drvdata->pwm);
+	if (ret)
+		return ret;
 
 	regulator = devm_regulator_register(&pdev->dev,
 					    &drvdata->desc, &config);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1413462 — Re: [PATCH 09/14] regulator: pwm: Adjust PWM config at probe time

On Fri, Jun 03, 2016 at 10:23:07AM +0200, Boris Brezillon wrote:
> The PWM attached to a PWM regulator device might have been previously
> configured by the bootloader.
> Make sure the bootloader and linux config are in sync, and adjust the PWM
> config if that's not the case.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> ---
>  drivers/regulator/pwm-regulator.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index ab3cc02..524b43f 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -285,11 +285,9 @@ static int pwm_regulator_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	/*
> -	 * FIXME: pwm_apply_args() should be removed when switching to the
> -	 * atomic PWM API.
> -	 */
> -	pwm_apply_args(drvdata->pwm);
> +	ret = pwm_adjust_config(drvdata->pwm);
> +	if (ret)
> +		return ret;
>  
>  	regulator = devm_regulator_register(&pdev->dev,
>  					    &drvdata->desc, &config);

Acked-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

[toc] | [prev] | [next] | [standalone]

#1412925 — [PATCH 04/14] pwm: rockchip: Add support for hardware readout

Implement the ->get_state() function to expose initial state.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/pwm-rockchip.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 68d72ce..dfacf7d 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -51,6 +51,8 @@ struct rockchip_pwm_data {
 
 	void (*set_enable)(struct pwm_chip *chip,
 			   struct pwm_device *pwm, bool enable);
+	void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
+			  struct pwm_state *state);
 };
 
 static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
@@ -75,6 +77,19 @@ static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip,
 	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_get_state_v1(struct pwm_chip *chip,
+				      struct pwm_device *pwm,
+				      struct pwm_state *state)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
+	u32 val;
+
+	val = readl(pc->base + pc->data->regs.ctrl);
+	if ((val & enable_conf) == enable_conf)
+		state->enabled = true;
+}
+
 static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
 				       struct pwm_device *pwm, bool enable)
 {
@@ -98,6 +113,53 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
 	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_get_state_v2(struct pwm_chip *chip,
+				      struct pwm_device *pwm,
+				      struct pwm_state *state)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
+			  PWM_CONTINUOUS;
+	u32 val;
+
+	val = readl(pc->base + pc->data->regs.ctrl);
+	if ((val & enable_conf) != enable_conf)
+		return;
+
+	state->enabled = true;
+
+	if (!(val & PWM_DUTY_POSITIVE))
+		state->polarity = PWM_POLARITY_INVERSED;
+}
+
+static void rockchip_pwm_get_state(struct pwm_chip *chip,
+				   struct pwm_device *pwm,
+				   struct pwm_state *state)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	unsigned long clk_rate;
+	u64 tmp;
+	int ret;
+
+	ret = clk_enable(pc->clk);
+	if (ret)
+		return;
+
+	clk_rate = clk_get_rate(pc->clk);
+
+	tmp = readl(pc->base + pc->data->regs.period);
+	tmp *= pc->data->prescaler * NSEC_PER_SEC;
+	state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
+
+	tmp = readl(pc->base + pc->data->regs.duty);
+	tmp *= pc->data->prescaler * NSEC_PER_SEC;
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
+
+	pc->data->get_state(chip, pwm, state);
+
+	clk_disable(pc->clk);
+}
+
 static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			       int duty_ns, int period_ns)
 {
@@ -170,6 +232,7 @@ static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 }
 
 static const struct pwm_ops rockchip_pwm_ops_v1 = {
+	.get_state = rockchip_pwm_get_state,
 	.config = rockchip_pwm_config,
 	.enable = rockchip_pwm_enable,
 	.disable = rockchip_pwm_disable,
@@ -177,6 +240,7 @@ static const struct pwm_ops rockchip_pwm_ops_v1 = {
 };
 
 static const struct pwm_ops rockchip_pwm_ops_v2 = {
+	.get_state = rockchip_pwm_get_state,
 	.config = rockchip_pwm_config,
 	.set_polarity = rockchip_pwm_set_polarity,
 	.enable = rockchip_pwm_enable,
@@ -194,6 +258,7 @@ static const struct rockchip_pwm_data pwm_data_v1 = {
 	.prescaler = 2,
 	.ops = &rockchip_pwm_ops_v1,
 	.set_enable = rockchip_pwm_set_enable_v1,
+	.get_state = rockchip_pwm_get_state_v1,
 };
 
 static const struct rockchip_pwm_data pwm_data_v2 = {
@@ -206,6 +271,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
 	.prescaler = 1,
 	.ops = &rockchip_pwm_ops_v2,
 	.set_enable = rockchip_pwm_set_enable_v2,
+	.get_state = rockchip_pwm_get_state_v2,
 };
 
 static const struct rockchip_pwm_data pwm_data_vop = {
@@ -218,6 +284,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
 	.prescaler = 1,
 	.ops = &rockchip_pwm_ops_v2,
 	.set_enable = rockchip_pwm_set_enable_v2,
+	.get_state = rockchip_pwm_get_state_v2,
 };
 
 static const struct of_device_id rockchip_pwm_dt_ids[] = {
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web