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

[PATCH 00/14] 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.
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

[PATCH 01/14] 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 02/14] regulator: pwm: Drop unneeded pwm_enable() call

[Boris Brezillon]

Now that the PWM regulator driver implements the ->enable/disable() hooks
we can remove the pwm_enable() call from pwm_regulator_set_voltage().

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

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index fafa348..ab3cc02 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -159,11 +159,6 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 		return ret;
 	}
 
-	ret = pwm_enable(drvdata->pwm);
-	if (ret) {
-		dev_err(&rdev->dev, "Failed to enable PWM: %d\n", ret);
-		return ret;
-	}
 	drvdata->volt_uV = min_uV;
 
 	/* Delay required by PWM regulator to settle to the new voltage */
-- 
2.7.4

[PATCH 03/14] 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.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 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 04/14] 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>
---
 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

[PATCH 05/14] 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>
---
 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

[PATCH 06/14] 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>
---
 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 798a787..309940a 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,
@@ -332,7 +334,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 07/14] 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 08/14] 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 | 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

[PATCH 09/14] 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>
---
 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 10/14] 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.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.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 11/14] 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>
---
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 12/14] 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>
---
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 13/14] 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>
---
 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

[PATCH 14/14] 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>
---
 .../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

Applied "regulator: pwm: Drop unneeded pwm_enable() call" to the regulator tree

[Mark Brown]

The patch

   regulator: pwm: Drop unneeded pwm_enable() call

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 830583004e615a4637eacc77866b84908414d7a0 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@free-electrons.com>
Date: Fri, 3 Jun 2016 10:23:00 +0200
Subject: [PATCH] regulator: pwm: Drop unneeded pwm_enable() call

Now that the PWM regulator driver implements the ->enable/disable() hooks
we can remove the pwm_enable() call from pwm_regulator_set_voltage().

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

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index fafa3488e960..ab3cc0235843 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -159,11 +159,6 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 		return ret;
 	}
 
-	ret = pwm_enable(drvdata->pwm);
-	if (ret) {
-		dev_err(&rdev->dev, "Failed to enable PWM: %d\n", ret);
-		return ret;
-	}
 	drvdata->volt_uV = min_uV;
 
 	/* Delay required by PWM regulator to settle to the new voltage */
-- 
2.8.1

Re: [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle approximation

[Brian Norris]

On Fri, Jun 03, 2016 at 10:23:01AM +0200, Boris Brezillon wrote:
> The current implementation always round down the duty and period
> values, while it would be better to round them to the closest integer.

Agreed. As I noted to you elsewhere, not having this change can cause
problems where doing a series of pwm_get_state() / modify /
pwm_apply_state() will propagate rounding errors, which will change the
period unexpectedly. e.g., I 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) ...

Seems much saner to me.

Now, I note that in patch 10 you're now using pwm_prepare_new_state() to
avoid this propagation problem entirely (good idea anyway, IMO), but I
just wanted to further note what kind of real problems we can see when
we don't round to the closest value.

> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

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

Tested this whole series on rk3399's PWM regulators used for the CPUs,
to clarify what my Tested-by means.

Thanks for the patches.

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

Re: [PATCH 04/14] pwm: rockchip: Add support for hardware readout

[Brian Norris]

On Fri, Jun 03, 2016 at 10:23:02AM +0200, Boris Brezillon wrote:
> 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(+)

[...]

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

Re: [PATCH 04/14] pwm: rockchip: Add support for hardware readout

[Brian Norris]

On Fri, Jun 03, 2016 at 10:23:02AM +0200, Boris Brezillon wrote:
> 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);

Nit: I just noticed you've been starting to use readl()/writel() in this
series, where previously {readl,writel}_relaxed() were being used. Any
reason?

Anyway, LGTM:

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

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

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

[Brian Norris]

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
> 

Re: [PATCH 06/14] pwm: rockchip: Add support for atomic update

[Brian Norris]

On Fri, Jun 03, 2016 at 10:23:04AM +0200, Boris Brezillon wrote:
> 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>
> ---
>  drivers/pwm/pwm-rockchip.c | 84 ++++++++++++++++++++++++----------------------
>  1 file changed, 43 insertions(+), 41 deletions(-)

[...]

Looks good to me:

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

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

[Brian Norris]

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
> 

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

[Brian Norris]

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>

Re: [PATCH 10/14] regulator: pwm: Switch to the atomic PWM API

[Brian Norris]

+ Laxman

Hi,

On Fri, Jun 03, 2016 at 10:23:08AM +0200, Boris Brezillon wrote:
> 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.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.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);

Notably, you're dropping much of Laxman's commit fd786fb0276a ("regulator:
pwm: Try to avoid voltage error in duty cycle calculation"), but I
believe the DIV_ROUND_CLOSEST_ULL() in pwm_set_relative_duty_cycle()
solves his problem better.

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

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

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

[Brian Norris]

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>

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

[Brian Norris]

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>

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

[Brian Norris]

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

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

[Brian Norris]

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>

Re: [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle approximation

[Boris Brezillon]

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

> On Fri, Jun 03, 2016 at 10:23:01AM +0200, Boris Brezillon wrote:
> > The current implementation always round down the duty and period
> > values, while it would be better to round them to the closest integer.  
> 
> Agreed. As I noted to you elsewhere, not having this change can cause
> problems where doing a series of pwm_get_state() / modify /
> pwm_apply_state() will propagate rounding errors, which will change the
> period unexpectedly. e.g., I 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) ...

Woh! Thanks for the detailed explanation. Do you want me to put that in
a comment explaining why we're using DIV_ROUND_CLOSEST_ULL()?


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

Re: [PATCH 04/14] pwm: rockchip: Add support for hardware readout

[Boris Brezillon]

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

> On Fri, Jun 03, 2016 at 10:23:02AM +0200, Boris Brezillon wrote:
> > 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);  
> 
> Nit: I just noticed you've been starting to use readl()/writel() in this
> series, where previously {readl,writel}_relaxed() were being used. Any
> reason?

Because I'm lazy and usually don't take the time to think whether it's
safe of not to use the _relaxed() versions :-). Not sure you'll have a
noticeable improvement by using _relaxed() for a PWM device by the
way, but I can change that ;-).

> 
> Anyway, LGTM:
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> 
> > +	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
> >   



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

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

[Boris Brezillon]

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

Re: [PATCH 10/14] regulator: pwm: Switch to the atomic PWM API

[Boris Brezillon]

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

> + Laxman
> 
> Hi,
> 
> On Fri, Jun 03, 2016 at 10:23:08AM +0200, Boris Brezillon wrote:
> > 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.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.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);  
> 
> Notably, you're dropping much of Laxman's commit fd786fb0276a ("regulator:
> pwm: Try to avoid voltage error in duty cycle calculation"), but I
> believe the DIV_ROUND_CLOSEST_ULL() in pwm_set_relative_duty_cycle()
> solves his problem better.

Oops, forgot to comment on that in the commit message. Indeed, the use
of pwm_set_relative_duty_cycle() solves the problem Laxman was seeing.

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



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

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

[Boris Brezillon]

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

Re: [PATCH 10/14] regulator: pwm: Switch to the atomic PWM API

[Laxman Dewangan]

On Saturday 04 June 2016 11:58 AM, Boris Brezillon wrote:
> On Fri, 3 Jun 2016 13:50:28 -0700
> Brian Norris <briannorris@chromium.org> wrote:
>
>> + Laxman
>>
>> Hi,
>>
>> On Fri, Jun 03, 2016 at 10:23:08AM +0200, Boris Brezillon wrote:
>>> -	 * 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);
>> Notably, you're dropping much of Laxman's commit fd786fb0276a ("regulator:
>> pwm: Try to avoid voltage error in duty cycle calculation"), but I
>> believe the DIV_ROUND_CLOSEST_ULL() in pwm_set_relative_duty_cycle()
>> solves his problem better.
> Oops, forgot to comment on that in the commit message. Indeed, the use
> of pwm_set_relative_duty_cycle() solves the problem Laxman was seeing.
>
Yaah, the issue which I was seeing and had fix will be resolved with 
this also.
I wanted to do req_diff * period first before any scaling/division.

Function pwm_set_relative_duty_cycle() does the same,  and hence it is fine.

state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)val * state->period,
+                          scale);



Acked-by: Laxman Dewangan <ldewangan@nvidia.com>



Thanks,
Laxman

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

[Rob Herring]

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
> 

Re: [PATCH 03/14] pwm: rockchip: Fix period and duty_cycle approximation

[Brian Norris]

On Sat, Jun 04, 2016 at 08:19:55AM +0200, Boris Brezillon wrote:
> On Fri, 3 Jun 2016 13:03:26 -0700
> Brian Norris <briannorris@chromium.org> wrote:
> 
> > On Fri, Jun 03, 2016 at 10:23:01AM +0200, Boris Brezillon wrote:
> > > The current implementation always round down the duty and period
> > > values, while it would be better to round them to the closest integer.  
> > 
> > Agreed. As I noted to you elsewhere, not having this change can cause
> > problems where doing a series of pwm_get_state() / modify /
> > pwm_apply_state() will propagate rounding errors, which will change the
> > period unexpectedly. e.g., I 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) ...
> 
> Woh! Thanks for the detailed explanation. Do you want me to put that in
> a comment explaining why we're using DIV_ROUND_CLOSEST_ULL()?

If you'd like, feel free to add some of this to your v2 description.

Brian

Re: [PATCH 04/14] pwm: rockchip: Add support for hardware readout

[Brian Norris]

On Sat, Jun 04, 2016 at 08:24:26AM +0200, Boris Brezillon wrote:
> On Fri, 3 Jun 2016 13:20:06 -0700 Brian Norris <briannorris@chromium.org> wrote:
> > On Fri, Jun 03, 2016 at 10:23:02AM +0200, Boris Brezillon wrote:
> > > @@ -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);  
> > 
> > Nit: I just noticed you've been starting to use readl()/writel() in this
> > series, where previously {readl,writel}_relaxed() were being used. Any
> > reason?
> 
> Because I'm lazy and usually don't take the time to think whether it's
> safe of not to use the _relaxed() versions :-). Not sure you'll have a
> noticeable improvement by using _relaxed() for a PWM device by the
> way, but I can change that ;-).

I just figured consistency would be nice.

Brian