[PATCH v2 00/13] regulator: pwm: various improvements

[PATCH v2 00/13] regulator: pwm: various improvements

[Boris Brezillon]

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.
Patches 2 to 7 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 8 to 11 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 12 and 13.

Best Regards,

Boris

Changes since v1:
- dropped already applied patches
- added R-b/A-b/T-b tags
- s/readl/readl_relaxed/ in patch 3 (as suggested by Brian)
- fixed pwm-regulator DT binding doc
- added some comments in the code
- replaced pwm_get_state() + if (state.enabled) by if (pwm_is_enabled())

Boris Brezillon (13):
  pwm: Add new helpers to create/manipulate PWM states
  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           |  19 +++
 drivers/pwm/pwm-rockchip.c                         | 178 +++++++++++++++------
 drivers/pwm/pwm-sti.c                              |  52 +++++-
 drivers/regulator/pwm-regulator.c                  | 162 ++++++++++++++-----
 include/linux/pwm.h                                |  81 ++++++++++
 5 files changed, 401 insertions(+), 91 deletions(-)

-- 
2.7.4

[PATCH v2 01/13] pwm: Add new helpers to create/manipulate PWM states

[Boris Brezillon]

The pwm_prepare_new_state() helper prepares a new state object
containing the current PWM state except for the polarity and period
fields which are set to the reference values.
This is particurly useful for PWM users who want to apply a new
duty-cycle expressed relatively to the reference period without
changing the enable state.

The PWM framework expect PWM users to configure the duty cycle in
nanosecond, but most users just want to express this duty cycle
relatively to the period value (i.e. duty_cycle = 33% of the period).
Add pwm_{get,set}_relative_duty_cycle() helpers to ease this kind of
conversion.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 include/linux/pwm.h | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 17018f3..e09babf 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -148,6 +148,87 @@ static inline void pwm_get_args(const struct pwm_device *pwm,
 }
 
 /**
+ * pwm_prepare_new_state() - prepare a new state to be applied with
+ *			     pwm_apply_state()
+ * @pwm: PWM device
+ * @state: state to fill with the prepared PWM state
+ *
+ * This functions prepares a state that can later be tweaked and applied
+ * to the PWM device with pwm_apply_state(). This is a convenient function
+ * that first retrieves the current PWM state and the replaces the period
+ * and polarity fields with the reference values defined in pwm->args.
+ * Once the new state is created you can adjust the ->enable and ->duty_cycle
+ * according to your need before calling pwm_apply_state().
+ */
+static inline void pwm_prepare_new_state(const struct pwm_device *pwm,
+					 struct pwm_state *state)
+{
+	struct pwm_args args;
+
+	/* First get the current state. */
+	pwm_get_state(pwm, state);
+
+	/* Then fill it with the reference config */
+	pwm_get_args(pwm, &args);
+
+	state->period = args.period;
+	state->polarity = args.polarity;
+	state->duty_cycle = 0;
+}
+
+/**
+ * pwm_get_relative_duty_cycle() - Get a relative duty_cycle value
+ * @state: the PWM state to extract period and duty_cycle from
+ * @scale: the scale you want to use for you relative duty_cycle value
+ *
+ * This functions converts the absolute duty_cycle stored in @state
+ * (expressed in nanosecond) into a relative value.
+ * For example if you want to get the duty_cycle expressed in nanosecond,
+ * call:
+ *
+ * pwm_get_state(pwm, &state);
+ * duty = pwm_get_relative_duty_cycle(&state, 100);
+ */
+static inline unsigned int
+pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)
+{
+	if (!state->period)
+		return 0;
+
+	return DIV_ROUND_CLOSEST_ULL((u64)state->duty_cycle * scale,
+				     state->period);
+}
+
+/**
+ * pwm_set_relative_duty_cycle() - Set a relative duty_cycle value
+ * @state: the PWM state to fill
+ * @val: the relative duty_cycle value
+ * @scale: the scale you use for you relative duty_cycle value
+ *
+ * This functions converts a relative duty_cycle stored into an absolute
+ * one (expressed in nanoseconds), and put the result in state->duty_cycle.
+ * For example if you want to change configure a 50% duty_cycle, call:
+ *
+ * pwm_prepare_new_state(pwm, &state);
+ * pwm_set_relative_duty_cycle(&state, 50, 100);
+ * pwm_apply_state(pwm, &state);
+ */
+static inline void
+pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int val,
+			    unsigned int scale)
+{
+	if (!scale)
+		return;
+
+	/* Make sure we don't have duty_cycle > period */
+	if (val > scale)
+		val = scale;
+
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)val * state->period,
+						  scale);
+}
+
+/**
  * struct pwm_ops - PWM controller operations
  * @request: optional hook for requesting a PWM
  * @free: optional hook for freeing a PWM
-- 
2.7.4

[PATCH v2 02/13] pwm: rockchip: Fix period and duty_cycle approximation

[Boris Brezillon]

The current implementation always round down the duty and period
values, while it would be better to round them to the closest integer.

These changes are needed in preparation of atomic update support to
prevent a period/duty cycle drift when executing several time the
'pwm_get_state() / modify / pwm_apply_state()' sequence.

Say you have an expected period of 3.333 us and a clk rate of
112.666667 MHz -- the clock frequency doesn't divide evenly,
so the period (stashed in nanoseconds) shrinks when we convert to
the register value and back, as follows:

  pwm_apply_state(): register = period * 112666667 / 1000000000;
  pwm_get_state(): period = register * 1000000000 / 112666667;

or in other words:

  period = period * 112666667 / 1000000000 * 1000000000 / 112666667;

which yields a sequence like:

  3333 -> 3328
  3328 -> 3319
  3319 -> 3310
  3310 -> 3301
  3301 -> 3292
  3292 -> ... (etc) ...

With this patch, we'd see instead:

  period = div_round_closest(period * 112666667, 1000000000) *
	   1000000000 / 112666667;

which yields a stable sequence:

  3333 -> 3337
  3337 -> 3337
  3337 -> ... (etc) ...

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
 drivers/pwm/pwm-rockchip.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 7d9cc90..68d72ce 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -114,12 +114,11 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * default prescaler value for all practical clock rate values.
 	 */
 	div = clk_rate * period_ns;
-	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
-	period = div;
+	period = DIV_ROUND_CLOSEST_ULL(div,
+				       pc->data->prescaler * NSEC_PER_SEC);
 
 	div = clk_rate * duty_ns;
-	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
-	duty = div;
+	duty = DIV_ROUND_CLOSEST_ULL(div, pc->data->prescaler * NSEC_PER_SEC);
 
 	ret = clk_enable(pc->clk);
 	if (ret)
-- 
2.7.4

[PATCH v2 03/13] pwm: rockchip: Add support for hardware readout

[Boris Brezillon]

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

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
 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..c72b419 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_relaxed(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_relaxed(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_relaxed(pc->base + pc->data->regs.period);
+	tmp *= pc->data->prescaler * NSEC_PER_SEC;
+	state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
+
+	tmp = readl_relaxed(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

[PATCH v2 04/13] pwm: rockchip: Avoid glitches on already running PWMs

[Boris Brezillon]

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>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
 drivers/pwm/pwm-rockchip.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index c72b419..dd8ca86 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -319,7 +319,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,6 +342,10 @@ 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. */
+	if (!pwm_is_enabled(pc->chip.pwms))
+		clk_disable(pc->clk);
+
 	return ret;
 }
 
@@ -349,6 +353,20 @@ static int rockchip_pwm_remove(struct platform_device *pdev)
 {
 	struct rockchip_pwm_chip *pc = platform_get_drvdata(pdev);
 
+	/*
+	 * 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().
+	 *
+	 */
+	if (pwm_is_enabled(pc->chip.pwms))
+		clk_disable(pc->clk);
+
 	clk_unprepare(pc->clk);
 
 	return pwmchip_remove(&pc->chip);
-- 
2.7.4

[PATCH v2 05/13] pwm: rockchip: Add support for atomic update

[Boris Brezillon]

Implement the ->apply() function to add support for atomic update.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
 drivers/pwm/pwm-rockchip.c | 84 ++++++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 41 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index dd8ca86..ef89df1 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -47,10 +47,12 @@ struct rockchip_pwm_regs {
 struct rockchip_pwm_data {
 	struct rockchip_pwm_regs regs;
 	unsigned int prescaler;
+	bool supports_polarity;
 	const struct pwm_ops *ops;
 
 	void (*set_enable)(struct pwm_chip *chip,
-			   struct pwm_device *pwm, bool enable);
+			   struct pwm_device *pwm, bool enable,
+			   enum pwm_polarity polarity);
 	void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
 			  struct pwm_state *state);
 };
@@ -61,7 +63,8 @@ static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
 }
 
 static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip,
-				       struct pwm_device *pwm, bool enable)
+				       struct pwm_device *pwm, bool enable,
+				       enum pwm_polarity polarity)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
@@ -91,14 +94,15 @@ static void rockchip_pwm_get_state_v1(struct pwm_chip *chip,
 }
 
 static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
-				       struct pwm_device *pwm, bool enable)
+				       struct pwm_device *pwm, bool enable,
+				       enum pwm_polarity polarity)
 {
 	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;
 
-	if (pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED)
+	if (polarity == PWM_POLARITY_INVERSED)
 		enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
 	else
 		enable_conf |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
@@ -166,7 +170,6 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	unsigned long period, duty;
 	u64 clk_rate, div;
-	int ret;
 
 	clk_rate = clk_get_rate(pc->clk);
 
@@ -182,69 +185,66 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	div = clk_rate * duty_ns;
 	duty = DIV_ROUND_CLOSEST_ULL(div, pc->data->prescaler * NSEC_PER_SEC);
 
-	ret = clk_enable(pc->clk);
-	if (ret)
-		return ret;
-
 	writel(period, pc->base + pc->data->regs.period);
 	writel(duty, pc->base + pc->data->regs.duty);
-	writel(0, pc->base + pc->data->regs.cntr);
-
-	clk_disable(pc->clk);
-
-	return 0;
-}
-
-static int rockchip_pwm_set_polarity(struct pwm_chip *chip,
-				     struct pwm_device *pwm,
-				     enum pwm_polarity polarity)
-{
-	/*
-	 * No action needed here because pwm->polarity will be set by the core
-	 * and the core will only change polarity when the PWM is not enabled.
-	 * We'll handle things in set_enable().
-	 */
 
 	return 0;
 }
 
-static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			      struct pwm_state *state)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	struct pwm_state curstate;
+	bool enabled;
 	int ret;
 
+	pwm_get_state(pwm, &curstate);
+	enabled = curstate.enabled;
+
 	ret = clk_enable(pc->clk);
 	if (ret)
 		return ret;
 
-	pc->data->set_enable(chip, pwm, true);
+	if (state->polarity != curstate.polarity && enabled) {
+		pc->data->set_enable(chip, pwm, false, state->polarity);
+		enabled = false;
+	}
 
-	return 0;
-}
+	ret = rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	if (ret) {
+		if (enabled != curstate.enabled)
+			pc->data->set_enable(chip, pwm, !enabled,
+					     state->polarity);
 
-static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+		goto out;
+	}
+
+	if (state->enabled != enabled)
+		pc->data->set_enable(chip, pwm, state->enabled,
+				     state->polarity);
 
-	pc->data->set_enable(chip, pwm, false);
+	/*
+	 * Update the state with the real hardware, which can differ a bit
+	 * because of period/duty_cycle approximation.
+	 */
+	rockchip_pwm_get_state(chip, pwm, state);
 
+out:
 	clk_disable(pc->clk);
+
+	return ret;
 }
 
 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,
+	.apply = rockchip_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
 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,
-	.disable = rockchip_pwm_disable,
+	.apply = rockchip_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
@@ -269,6 +269,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
 		.ctrl = 0x0c,
 	},
 	.prescaler = 1,
+	.supports_polarity = true,
 	.ops = &rockchip_pwm_ops_v2,
 	.set_enable = rockchip_pwm_set_enable_v2,
 	.get_state = rockchip_pwm_get_state_v2,
@@ -282,6 +283,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
 		.ctrl = 0x00,
 	},
 	.prescaler = 1,
+	.supports_polarity = true,
 	.ops = &rockchip_pwm_ops_v2,
 	.set_enable = rockchip_pwm_set_enable_v2,
 	.get_state = rockchip_pwm_get_state_v2,
@@ -331,7 +333,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	pc->chip.base = -1;
 	pc->chip.npwm = 1;
 
-	if (pc->data->ops->set_polarity) {
+	if (pc->data->supports_polarity) {
 		pc->chip.of_xlate = of_pwm_xlate_with_flags;
 		pc->chip.of_pwm_n_cells = 3;
 	}
-- 
2.7.4

[PATCH v2 06/13] pwm: sti: Add support for hardware readout

[Boris Brezillon]

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

[PATCH v2 07/13] pwm: sti: Avoid glitches on already running PWMs

[Boris Brezillon]

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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 6300d3e..5bda51d 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,16 @@ 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++) {
+		if (pwm_is_enabled(&pc->chip.pwms[i]))
+			clk_enable(pc->clk);
+	}
+	clk_disable(pc->clk);
+
 	platform_set_drvdata(pdev, pc);
 
 	return 0;
-- 
2.7.4

[PATCH v2 08/13] regulator: pwm: Adjust PWM config at probe time

[Boris Brezillon]

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>
Acked-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.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

[PATCH v2 09/13] regulator: pwm: Switch to the atomic PWM API

[Boris Brezillon]

Use the atomic API wherever appropriate and get rid of pwm_apply_args()
call (the reference period and polarity are now explicitly set when
calling pwm_apply_state()).

We also make use of the pwm_set_relative_duty_cycle() helper to ease
relative to absolute duty_cycle conversion.

Note that changes introduced by commit fd786fb0276a ("regulator: pwm:
Try to avoid voltage error in duty cycle calculation") are no longer
needed because pwm_set_relative_duty_cycle() takes care of all rounding
approximation for us.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/pwm-regulator.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 524b43f..bf033fd 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -59,16 +59,14 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
 					 unsigned selector)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
-	struct pwm_args pargs;
-	int dutycycle;
+	struct pwm_state pstate;
 	int ret;
 
-	pwm_get_args(drvdata->pwm, &pargs);
+	pwm_prepare_new_state(drvdata->pwm, &pstate);
+	pwm_set_relative_duty_cycle(&pstate,
+			drvdata->duty_cycle_table[selector].dutycycle, 100);
 
-	dutycycle = (pargs.period *
-		    drvdata->duty_cycle_table[selector].dutycycle) / 100;
-
-	ret = pwm_config(drvdata->pwm, dutycycle, pargs.period);
+	ret = pwm_apply_state(drvdata->pwm, &pstate);
 	if (ret) {
 		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
 		return ret;
@@ -126,34 +124,18 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
 	unsigned int ramp_delay = rdev->constraints->ramp_delay;
-	struct pwm_args pargs;
 	unsigned int req_diff = min_uV - rdev->constraints->min_uV;
+	struct pwm_state pstate;
 	unsigned int diff;
-	unsigned int duty_pulse;
-	u64 req_period;
-	u32 rem;
 	int ret;
 
-	pwm_get_args(drvdata->pwm, &pargs);
+	pwm_prepare_new_state(drvdata->pwm, &pstate);
 	diff = rdev->constraints->max_uV - rdev->constraints->min_uV;
 
-	/* First try to find out if we get the iduty cycle time which is
-	 * factor of PWM period time. If (request_diff_to_min * pwm_period)
-	 * is perfect divided by voltage_range_diff then it is possible to
-	 * get duty cycle time which is factor of PWM period. This will help
-	 * to get output voltage nearer to requested value as there is no
-	 * calculation loss.
-	 */
-	req_period = req_diff * pargs.period;
-	div_u64_rem(req_period, diff, &rem);
-	if (!rem) {
-		do_div(req_period, diff);
-		duty_pulse = (unsigned int)req_period;
-	} else {
-		duty_pulse = (pargs.period / 100) * ((req_diff * 100) / diff);
-	}
+	/* We pass diff as the scale to get a uV precision. */
+	pwm_set_relative_duty_cycle(&pstate, req_diff, diff);
 
-	ret = pwm_config(drvdata->pwm, duty_pulse, pargs.period);
+	ret = pwm_apply_state(drvdata->pwm, &pstate);
 	if (ret) {
 		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
 		return ret;
-- 
2.7.4

[PATCH v2 10/13] regulator: pwm: Properly initialize the ->state field

[Boris Brezillon]

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>
Tested-by: Brian Norris <briannorris@chromium.org>
---
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

[PATCH v2 11/13] regulator: pwm: Retrieve correct voltage

[Boris Brezillon]

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>
---
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

[PATCH v2 12/13] regulator: pwm: Support extra continuous mode cases

[Boris Brezillon]

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>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
---
 drivers/regulator/pwm-regulator.c | 92 +++++++++++++++++++++++++++++++++++----
 1 file changed, 83 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index c39ecd1..3e8680a 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,79 @@ 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);
+
+	/*
+	 * The dutycycle for the min_uV voltage might be greater than the
+	 * one for the max_uV. This is happening when the user needs an
+	 * inversed polarity, but the PWM device does not support inversing
+	 * it in hardware.
+	 */
+	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);
+	/*
+	 * The dutycycle for the min_uV voltage might be greater than the
+	 * one for the max_uV. This is happening when the user needs an
+	 * inversed polarity, but the PWM device does not support inversing
+	 * it in hardware.
+	 */
+	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 +294,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

[PATCH v2 13/13] regulator: pwm: Document pwm-dutycycle-unit and pwm-dutycycle-range

[Boris Brezillon]

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

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Brian Norris <briannorris@chromium.org>
---
 .../devicetree/bindings/regulator/pwm-regulator.txt   | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
index ed936f0..9fbc7b1 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 duty cycle 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.
+			Duty cycle 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.
@@ -48,6 +60,13 @@ Continuous Voltage Example:
 		regulator-min-microvolt = <1016000>;
 		regulator-max-microvolt = <1114000>;
 		regulator-name = "vdd_logic";
+		/* unit == per-mille */
+		pwm-dutycycle-unit = <1000>;
+		/*
+		 * Inverted PWM logic, and the duty cycle range is limited
+		 * to 30%-70%.
+		 */
+		pwm-dutycycle-range <700 300>; /* */
 	};
 
 Voltage Table Example:
-- 
2.7.4

Re: [PATCH v2 00/13] regulator: pwm: various improvements

[Heiko Stübner]

Hi Boris,

Am Mittwoch, 8. Juni 2016, 18:34:35 schrieb Boris Brezillon:
> 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.

[...]

> 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 12 and 13.

I've tested this series on a rk3288-veyron chromebook and both the backlight-
pwm as well as the pwm-regulator still work as expected, so for everything 
except the STI parts:

Tested-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH v2 13/13] regulator: pwm: Document pwm-dutycycle-unit and pwm-dutycycle-range

[Rob Herring]

On Wed, Jun 08, 2016 at 06:34:48PM +0200, Boris Brezillon wrote:
> Document the pwm-dutycycle-unit and pwm-dutycycle-range properties.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Acked-by: Brian Norris <briannorris@chromium.org>
> ---
>  .../devicetree/bindings/regulator/pwm-regulator.txt   | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 01/13] pwm: Add new helpers to create/manipulate PWM states

[Thierry Reding]

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

On Wed, Jun 08, 2016 at 06:34:36PM +0200, Boris Brezillon wrote:
> The pwm_prepare_new_state() helper prepares a new state object
> containing the current PWM state except for the polarity and period
> fields which are set to the reference values.
> This is particurly useful for PWM users who want to apply a new
> duty-cycle expressed relatively to the reference period without
> changing the enable state.
> 
> The PWM framework expect PWM users to configure the duty cycle in
> nanosecond, but most users just want to express this duty cycle
> relatively to the period value (i.e. duty_cycle = 33% of the period).
> Add pwm_{get,set}_relative_duty_cycle() helpers to ease this kind of
> conversion.

This looks pretty useful and good, though I think these could be two
separate patches. A couple of suggestions on wording and such below.

> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  include/linux/pwm.h | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 17018f3..e09babf 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -148,6 +148,87 @@ static inline void pwm_get_args(const struct pwm_device *pwm,
>  }
>  
>  /**
> + * pwm_prepare_new_state() - prepare a new state to be applied with
> + *			     pwm_apply_state()

This is weirdly indented.

> + * @pwm: PWM device
> + * @state: state to fill with the prepared PWM state
> + *
> + * This functions prepares a state that can later be tweaked and applied
> + * to the PWM device with pwm_apply_state(). This is a convenient function
> + * that first retrieves the current PWM state and the replaces the period
> + * and polarity fields with the reference values defined in pwm->args.
> + * Once the new state is created you can adjust the ->enable and ->duty_cycle

"created" has the connotation of allocating. Perhaps: "Once the function
returns, you can adjust the ->enabled and ->duty_cycle fields according
to your needs before calling pwm_apply_state()."?

Also note that the field is called ->enabled.

> + * according to your need before calling pwm_apply_state().

Maybe mention that the ->duty_cycle field is explicitly zeroed. Then
again, do we really need it? If users are going to overwrite it anyway,
do we even need to bother? I suppose it makes some sense because the
current duty cycle is stale when the ->period gets set to the value from
args. I think the documentation should mention this in some way.

> + */
> +static inline void pwm_prepare_new_state(const struct pwm_device *pwm,
> +					 struct pwm_state *state)

Perhaps choose a shorter name, such as: pwm_init_state()?

> +{
> +	struct pwm_args args;
> +
> +	/* First get the current state. */
> +	pwm_get_state(pwm, state);
> +
> +	/* Then fill it with the reference config */
> +	pwm_get_args(pwm, &args);
> +
> +	state->period = args.period;
> +	state->polarity = args.polarity;
> +	state->duty_cycle = 0;
> +}
> +
> +/**
> + * pwm_get_relative_duty_cycle() - Get a relative duty_cycle value
> + * @state: the PWM state to extract period and duty_cycle from
> + * @scale: the scale you want to use for you relative duty_cycle value

Other kerneldoc comments in this file don't prefix the description with
"the".

Also, perhaps the following descriptions would be slightly less
confusing:

	@state: PWM state to extract the duty cycle from

We don't extract (i.e. return) period from the state, so it's a little
confusing to mention it here.

	@scale: scale in which to return the relative duty cycle

This is slightly more explicit in that it says what the returned duty
cycle will be in. Perhaps as an alternative:

	@scale: target scale of the relative duty cycle

> + *
> + * This functions converts the absolute duty_cycle stored in @state
> + * (expressed in nanosecond) into a relative value.

Perhaps: "... into a value relative to the period."?

> + * For example if you want to get the duty_cycle expressed in nanosecond,

The example below would give you the duty cycle in percent, not
nanoseconds, right?

> + * call:
> + *
> + * pwm_get_state(pwm, &state);
> + * duty = pwm_get_relative_duty_cycle(&state, 100);
> + */
> +static inline unsigned int
> +pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)
> +{
> +	if (!state->period)
> +		return 0;
> +
> +	return DIV_ROUND_CLOSEST_ULL((u64)state->duty_cycle * scale,
> +				     state->period);
> +}
> +
> +/**
> + * pwm_set_relative_duty_cycle() - Set a relative duty_cycle value
> + * @state: the PWM state to fill
> + * @val: the relative duty_cycle value
> + * @scale: the scale you use for you relative duty_cycle value

"scale in which @duty_cycle is expressed"?

> + *
> + * This functions converts a relative duty_cycle stored into an absolute
> + * one (expressed in nanoseconds), and put the result in state->duty_cycle.
> + * For example if you want to change configure a 50% duty_cycle, call:
> + *
> + * pwm_prepare_new_state(pwm, &state);
> + * pwm_set_relative_duty_cycle(&state, 50, 100);
> + * pwm_apply_state(pwm, &state);
> + */
> +static inline void
> +pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int val,

Any reason why we can't call "val" "duty_cycle"? That's what it is,
after all.

> +			    unsigned int scale)
> +{
> +	if (!scale)
> +		return;
> +
> +	/* Make sure we don't have duty_cycle > period */
> +	if (val > scale)
> +		val = scale;

Can we return -EINVAL for both of the above checks? I don't like
silently clamping the duty cycle that the user passed in.

Thierry

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

Re: [PATCH v2 01/13] pwm: Add new helpers to create/manipulate PWM states

[Boris Brezillon]

Hi Thierry,

On Fri, 10 Jun 2016 17:21:09 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Jun 08, 2016 at 06:34:36PM +0200, Boris Brezillon wrote:
> > The pwm_prepare_new_state() helper prepares a new state object
> > containing the current PWM state except for the polarity and period
> > fields which are set to the reference values.
> > This is particurly useful for PWM users who want to apply a new
> > duty-cycle expressed relatively to the reference period without
> > changing the enable state.
> > 
> > The PWM framework expect PWM users to configure the duty cycle in
> > nanosecond, but most users just want to express this duty cycle
> > relatively to the period value (i.e. duty_cycle = 33% of the period).
> > Add pwm_{get,set}_relative_duty_cycle() helpers to ease this kind of
> > conversion.  
> 
> This looks pretty useful and good, though I think these could be two
> separate patches. A couple of suggestions on wording and such below.

Sure, I can split those changes.

> 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  include/linux/pwm.h | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> > 
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 17018f3..e09babf 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -148,6 +148,87 @@ static inline void pwm_get_args(const struct pwm_device *pwm,
> >  }
> >  
> >  /**
> > + * pwm_prepare_new_state() - prepare a new state to be applied with
> > + *			     pwm_apply_state()  
> 
> This is weirdly indented.

I'll fix the indentation.

> 
> > + * @pwm: PWM device
> > + * @state: state to fill with the prepared PWM state
> > + *
> > + * This functions prepares a state that can later be tweaked and applied
> > + * to the PWM device with pwm_apply_state(). This is a convenient function
> > + * that first retrieves the current PWM state and the replaces the period
> > + * and polarity fields with the reference values defined in pwm->args.
> > + * Once the new state is created you can adjust the ->enable and ->duty_cycle  
> 
> "created" has the connotation of allocating. Perhaps: "Once the function
> returns, you can adjust the ->enabled and ->duty_cycle fields according
> to your needs before calling pwm_apply_state()."?

Okay.

> 
> Also note that the field is called ->enabled.
> 
> > + * according to your need before calling pwm_apply_state().  
> 
> Maybe mention that the ->duty_cycle field is explicitly zeroed. Then
> again, do we really need it? If users are going to overwrite it anyway,
> do we even need to bother? I suppose it makes some sense because the
> current duty cycle is stale when the ->period gets set to the value from
> args. I think the documentation should mention this in some way.

Yes, if we keep the current duty_cycle it can exceed the period value.
I'm fine dropping the ->duty_cycle = 0 assignment and documenting the
behavior.

> 
> > + */
> > +static inline void pwm_prepare_new_state(const struct pwm_device *pwm,
> > +					 struct pwm_state *state)  
> 
> Perhaps choose a shorter name, such as: pwm_init_state()?

Sure.

> 
> > +{
> > +	struct pwm_args args;
> > +
> > +	/* First get the current state. */
> > +	pwm_get_state(pwm, state);
> > +
> > +	/* Then fill it with the reference config */
> > +	pwm_get_args(pwm, &args);
> > +
> > +	state->period = args.period;
> > +	state->polarity = args.polarity;
> > +	state->duty_cycle = 0;
> > +}
> > +
> > +/**
> > + * pwm_get_relative_duty_cycle() - Get a relative duty_cycle value
> > + * @state: the PWM state to extract period and duty_cycle from
> > + * @scale: the scale you want to use for you relative duty_cycle value  
> 
> Other kerneldoc comments in this file don't prefix the description with
> "the".
> 
> Also, perhaps the following descriptions would be slightly less
> confusing:
> 
> 	@state: PWM state to extract the duty cycle from

Agreed.

> 
> We don't extract (i.e. return) period from the state, so it's a little
> confusing to mention it here.
> 
> 	@scale: scale in which to return the relative duty cycle
> 
> This is slightly more explicit in that it says what the returned duty
> cycle will be in. Perhaps as an alternative:
> 
> 	@scale: target scale of the relative duty cycle

I'll go for this one.

> 
> > + *
> > + * This functions converts the absolute duty_cycle stored in @state
> > + * (expressed in nanosecond) into a relative value.  
> 
> Perhaps: "... into a value relative to the period."?

Okay.

> 
> > + * For example if you want to get the duty_cycle expressed in nanosecond,  
> 
> The example below would give you the duty cycle in percent, not
> nanoseconds, right?

Absolutely. I'll fix that.

> 
> > + * call:
> > + *
> > + * pwm_get_state(pwm, &state);
> > + * duty = pwm_get_relative_duty_cycle(&state, 100);
> > + */
> > +static inline unsigned int
> > +pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)
> > +{
> > +	if (!state->period)
> > +		return 0;
> > +
> > +	return DIV_ROUND_CLOSEST_ULL((u64)state->duty_cycle * scale,
> > +				     state->period);
> > +}
> > +
> > +/**
> > + * pwm_set_relative_duty_cycle() - Set a relative duty_cycle value
> > + * @state: the PWM state to fill
> > + * @val: the relative duty_cycle value
> > + * @scale: the scale you use for you relative duty_cycle value  
> 
> "scale in which @duty_cycle is expressed"?

Yep.

> 
> > + *
> > + * This functions converts a relative duty_cycle stored into an absolute
> > + * one (expressed in nanoseconds), and put the result in state->duty_cycle.
> > + * For example if you want to change configure a 50% duty_cycle, call:
> > + *
> > + * pwm_prepare_new_state(pwm, &state);
> > + * pwm_set_relative_duty_cycle(&state, 50, 100);
> > + * pwm_apply_state(pwm, &state);
> > + */
> > +static inline void
> > +pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int val,  
> 
> Any reason why we can't call "val" "duty_cycle"? That's what it is,
> after all.

Nope, I'll switch to duty_cycle.

> 
> > +			    unsigned int scale)
> > +{
> > +	if (!scale)
> > +		return;
> > +
> > +	/* Make sure we don't have duty_cycle > period */
> > +	if (val > scale)
> > +		val = scale;  
> 
> Can we return -EINVAL for both of the above checks? I don't like
> silently clamping the duty cycle that the user passed in.

I'm fine returning -EINVAL in this case.

Thanks for rewording some of my sentences and reviewing the patch.

Regards,

Boris

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

Re: [PATCH v2 01/13] pwm: Add new helpers to create/manipulate PWM states

[Thierry Reding]

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

On Fri, Jun 10, 2016 at 06:29:42PM +0200, Boris Brezillon wrote:
> On Fri, 10 Jun 2016 17:21:09 +0200, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Wed, Jun 08, 2016 at 06:34:36PM +0200, Boris Brezillon wrote:
[...]
> > > + * according to your need before calling pwm_apply_state().  
> > 
> > Maybe mention that the ->duty_cycle field is explicitly zeroed. Then
> > again, do we really need it? If users are going to overwrite it anyway,
> > do we even need to bother? I suppose it makes some sense because the
> > current duty cycle is stale when the ->period gets set to the value from
> > args. I think the documentation should mention this in some way.
> 
> Yes, if we keep the current duty_cycle it can exceed the period value.
> I'm fine dropping the ->duty_cycle = 0 assignment and documenting the
> behavior.

Actually what I was trying to suggest is that we keep the code as-is but
document the behaviour (and rationale behind it).

I think it's fine to zero out the value precisely because it could
become invalid after the function (and there's no other reasonable value
to set it to). Just wanted to make sure it's all properly documented.

Thierry

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