pwm: add support for atomic update

Hello,

This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.

Best Regards,

Boris

Changes since v2:
- rebased on top of 4.3-rc2
- reintroduced pwm-regulator patches

Changes since v1:
- dropped applied patches
- squashed Heiko's fixes into the rockchip driver changes
- made a few cosmetic changes
- added kerneldoc comments
- added Heiko's patch to display more information in debugfs
- dropped pwm-regulator patches (should be submitted separately)

Boris Brezillon (11):
  pwm: introduce default period and polarity concepts
  pwm: define a new pwm_state struct
  pwm: move the enabled/disabled info to pwm_state struct
  backlight: pwm_bl: remove useless call to pwm_set_period
  pwm: declare a default PWM state
  pwm: add the PWM initial state retrieval infra
  pwm: add the core infrastructure to allow atomic update
  pwm: rockchip: add initial state retrieval
  pwm: rockchip: add support for atomic update
  regulator: pwm: implement ->enable(), ->disable() and ->is_enabled
    methods
  regulator: pwm: properly initialize the ->state field

Heiko Stübner (1):
  pwm: add information about polarity, duty cycle and period to debugfs

 drivers/leds/leds-pwm.c              |   2 +-

14 more lines

When requested by a user, the PWM is assigned a default period and polarity
extracted from the DT, the platform data or statically set by the driver.
Those default values are currently stored in the period and polarity
fields of the pwm_device struct, but they will be stored somewhere else
once we have introduced the architecture allowing for hardware state
retrieval.

The pwm_set_default_polarity and pwm_set_default_period should only be
used by PWM drivers or the PWM core infrastructure to specify the
default period and polarity values.

PWM users might call the pwm_get_default_period to query the default
period value. There is currently no helper to query the default
polarity, but it might be added later on if there is a need for it.

This patch also modifies all the places where the default helpers should
be used in place of the standard ones.

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/leds/leds-pwm.c              |  2 +-
 drivers/pwm/core.c                   | 14 +++++++-------
 drivers/pwm/pwm-pxa.c                |  2 +-
 drivers/pwm/pwm-sun4i.c              |  3 ++-
 drivers/regulator/pwm-regulator.c    |  4 ++--
 drivers/video/backlight/lm3630a_bl.c |  4 ++--
 drivers/video/backlight/pwm_bl.c     |  2 +-
 drivers/video/fbdev/ssd1307fb.c      |  2 +-
 include/linux/pwm.h                  | 17 +++++++++++++++++
 9 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 1d07e3e..2c564d1 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c

@@ -125,7 +125,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	if (led_data->can_sleep)
 		INIT_WORK(&led_data->work, led_pwm_work);
 
-	led_data->period = pwm_get_period(led_data->pwm);
+	led_data->period = pwm_get_default_period(led_data->pwm);
 	if (!led_data->period && (led->pwm_period_ns > 0))
 		led_data->period = led->pwm_period_ns;
 

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3f9df3e..732375d 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c

@@ -146,12 +146,12 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 	if (IS_ERR(pwm))
 		return pwm;
 
-	pwm_set_period(pwm, args->args[1]);
+	pwm_set_default_period(pwm, args->args[1]);
 
 	if (args->args[2] & PWM_POLARITY_INVERTED)
-		pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
+		pwm_set_default_polarity(pwm, PWM_POLARITY_INVERSED);
 	else
-		pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
+		pwm_set_default_polarity(pwm, PWM_POLARITY_NORMAL);
 
 	return pwm;
 }

@@ -172,7 +172,7 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 	if (IS_ERR(pwm))
 		return pwm;
 
-	pwm_set_period(pwm, args->args[1]);
+	pwm_set_default_period(pwm, args->args[1]);
 
 	return pwm;
 }

@@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->chip = chip;
 		pwm->pwm = chip->base + i;
 		pwm->hwpwm = i;
-		pwm->polarity = polarity;
+		pwm_set_default_polarity(pwm, polarity);
 
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}

@@ -730,8 +730,8 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 	if (IS_ERR(pwm))
 		goto out;
 
-	pwm_set_period(pwm, chosen->period);
-	pwm_set_polarity(pwm, chosen->polarity);
+	pwm_set_default_period(pwm, chosen->period);
+	pwm_set_default_polarity(pwm, chosen->polarity);
 
 out:
 	mutex_unlock(&pwm_lookup_lock);

diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index cb2f702..65b80aa 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c

@@ -160,7 +160,7 @@ pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 	if (IS_ERR(pwm))
 		return pwm;
 
-	pwm_set_period(pwm, args->args[0]);
+	pwm_set_default_period(pwm, args->args[0]);
 
 	return pwm;
 }

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index cd9dde5..a364fb7 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c

@@ -333,7 +333,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 	val = sun4i_pwm_readl(pwm, PWM_CTRL_REG);
 	for (i = 0; i < pwm->chip.npwm; i++)
 		if (!(val & BIT_CH(PWM_ACT_STATE, i)))
-			pwm->chip.pwms[i].polarity = PWM_POLARITY_INVERSED;
+			pwm_set_default_polarity(&pwm->chip.pwms[i],
+						 PWM_POLARITY_INVERSED);
 	clk_disable_unprepare(pwm->clk);
 
 	return 0;

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index fc3166d..cc549b7 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c

@@ -56,7 +56,7 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
 	int dutycycle;
 	int ret;
 
-	pwm_reg_period = pwm_get_period(drvdata->pwm);
+	pwm_reg_period = pwm_get_default_period(drvdata->pwm);
 
 	dutycycle = (pwm_reg_period *
 		    drvdata->duty_cycle_table[selector].dutycycle) / 100;

@@ -114,7 +114,7 @@ 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;
-	unsigned int period = pwm_get_period(drvdata->pwm);
+	unsigned int period = pwm_get_default_period(drvdata->pwm);
 	int duty_cycle;
 	int ret;
 

diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 35fe482..449ebc3 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c

@@ -162,7 +162,7 @@ static int lm3630a_intr_config(struct lm3630a_chip *pchip)
 
 static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
 {
-	unsigned int period = pwm_get_period(pchip->pwmd);
+	unsigned int period = pwm_get_default_period(pchip->pwmd);
 	unsigned int duty = br * period / br_max;
 
 	pwm_config(pchip->pwmd, duty, period);

@@ -425,7 +425,7 @@ static int lm3630a_probe(struct i2c_client *client,
 			return PTR_ERR(pchip->pwmd);
 		}
 	}
-	pchip->pwmd->period = pdata->pwm_period;
+	pwm_set_default_period(pchip->pwmd, pdata->pwm_period);
 
 	/* interrupt enable  : irq 0 is not allowed */
 	pchip->irq = client->irq;

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index eff379b..ae498c1 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c

@@ -294,7 +294,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	 * set the period from platform data if it has not already been set
 	 * via the PWM lookup table.
 	 */
-	pb->period = pwm_get_period(pb->pwm);
+	pb->period = pwm_get_default_period(pb->pwm);
 	if (!pb->period && (data->pwm_period_ns > 0)) {
 		pb->period = data->pwm_period_ns;
 		pwm_set_period(pb->pwm, data->pwm_period_ns);

diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 93f4c90..ab3daf0 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c

@@ -294,7 +294,7 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
 			return PTR_ERR(par->pwm);
 		}
 
-		par->pwm_period = pwm_get_period(par->pwm);
+		par->pwm_period = pwm_get_default_period(par->pwm);
 		/* Enable the PWM */
 		pwm_config(par->pwm, par->pwm_period / 2, par->pwm_period);
 		pwm_enable(par->pwm);

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d681f68..31239a9 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h

@@ -115,11 +115,22 @@ static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
 		pwm->period = period;
 }
 
+static inline void pwm_set_default_period(struct pwm_device *pwm,
+					  unsigned int period)
+{
+	pwm_set_period(pwm, period);
+}
+
 static inline unsigned int pwm_get_period(const struct pwm_device *pwm)
 {
 	return pwm ? pwm->period : 0;
 }
 
+static inline unsigned int pwm_get_default_period(const struct pwm_device *pwm)
+{
+	return pwm_get_period(pwm);
+}
+
 static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
 {
 	if (pwm)

@@ -136,6 +147,12 @@ static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm)
  */
 int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity);
 
+static inline void pwm_set_default_polarity(struct pwm_device *pwm,
+					    enum pwm_polarity polarity)
+{
+	pwm_set_polarity(pwm, polarity);
+}
+
 static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 {
 	return pwm ? pwm->polarity : PWM_POLARITY_NORMAL;

-- 
1.9.1

The PWM state, represented by its period, duty_cycle and polarity,
is currently directly stored in the PWM device.
Declare a pwm_state structure embedding those field so that we can later
use this struct to atomically update all the PWM parameters at once.

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/pwm/core.c  |  6 +++---
 include/linux/pwm.h | 30 +++++++++++++++++++-----------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 732375d..09037de 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c

@@ -446,8 +446,8 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
 	if (err)
 		return err;
 
-	pwm->duty_cycle = duty_ns;
-	pwm->period = period_ns;
+	pwm->state.duty_cycle = duty_ns;
+	pwm->state.period = period_ns;
 
 	return 0;
 }

@@ -480,7 +480,7 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
 	if (err)
 		return err;
 
-	pwm->polarity = polarity;
+	pwm->state.polarity = polarity;
 
 	return 0;
 }

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 31239a9..e0e0ed8 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h

@@ -79,6 +79,18 @@ enum {
 	PWMF_EXPORTED = 1 << 2,
 };
 
+/*
+ * struct pwm_state - state of a PWM channel
+ * @period: PWM period (in nanoseconds)
+ * @duty_cycle: PWM duty cycle (in nanoseconds)
+ * @polarity: PWM polarity
+ */
+struct pwm_state {
+	unsigned int period;
+	unsigned int duty_cycle;
+	enum pwm_polarity polarity;
+};
+
 /**
  * struct pwm_device - PWM channel object
  * @label: name of the PWM device

@@ -87,9 +99,7 @@ enum {
  * @pwm: global index of the PWM device
  * @chip: PWM chip providing this PWM device
  * @chip_data: chip-private data associated with the PWM device
- * @period: period of the PWM signal (in nanoseconds)
- * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
- * @polarity: polarity of the PWM signal
+ * @state: curent PWM channel state
  */
 struct pwm_device {
 	const char *label;

@@ -99,9 +109,7 @@ struct pwm_device {
 	struct pwm_chip *chip;
 	void *chip_data;
 
-	unsigned int period;
-	unsigned int duty_cycle;
-	enum pwm_polarity polarity;
+	struct pwm_state state;
 };
 
 static inline bool pwm_is_enabled(const struct pwm_device *pwm)

@@ -112,7 +120,7 @@ static inline bool pwm_is_enabled(const struct pwm_device *pwm)
 static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
 {
 	if (pwm)
-		pwm->period = period;
+		pwm->state.period = period;
 }
 
 static inline void pwm_set_default_period(struct pwm_device *pwm,

@@ -123,7 +131,7 @@ static inline void pwm_set_default_period(struct pwm_device *pwm,
 
 static inline unsigned int pwm_get_period(const struct pwm_device *pwm)
 {
-	return pwm ? pwm->period : 0;
+	return pwm ? pwm->state.period : 0;
 }
 
 static inline unsigned int pwm_get_default_period(const struct pwm_device *pwm)

@@ -134,12 +142,12 @@ static inline unsigned int pwm_get_default_period(const struct pwm_device *pwm)
 static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
 {
 	if (pwm)
-		pwm->duty_cycle = duty;
+		pwm->state.duty_cycle = duty;
 }
 
 static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm)
 {
-	return pwm ? pwm->duty_cycle : 0;
+	return pwm ? pwm->state.duty_cycle : 0;
 }
 
 /*

@@ -155,7 +163,7 @@ static inline void pwm_set_default_polarity(struct pwm_device *pwm,
 
 static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 {
-	return pwm ? pwm->polarity : PWM_POLARITY_NORMAL;
+	return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
 }
 
 /**

-- 
1.9.1

Add an ->apply() method to the pwm_ops struct to allow PWM drivers to
implement atomic update.
This method will be preferred over the ->enable(), ->disable() and
->config() methods if available.

Add the pwm_get_state(), pwm_get_default_state() and pwm_apply_state()
functions for PWM users to be able to use the atomic update feature.

Note that the pwm_apply_state() does not guarantee the atomicity of the
update operation, it all depends on the availability and implementation
of the ->apply() method.

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/pwm/core.c  | 142 +++++++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/pwm.h |  27 ++++++++++
 2 files changed, 157 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 60ad758..ff3c662 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c

@@ -46,6 +46,12 @@ static struct pwm_device *pwm_to_device(unsigned int pwm)
 	return radix_tree_lookup(&pwm_tree, pwm);
 }
 
+static void pwm_set_state(struct pwm_device *pwm,
+			  const struct pwm_state *state)
+{
+	pwm->state = *state;
+}
+
 static int alloc_pwms(int pwm, unsigned int count)
 {
 	unsigned int from = 0;

@@ -226,6 +232,19 @@ void *pwm_get_chip_data(struct pwm_device *pwm)
 }
 EXPORT_SYMBOL_GPL(pwm_get_chip_data);
 
+static bool pwm_ops_check(const struct pwm_ops *ops)
+{
+	/* driver supports legacy, non-atomic operation */
+	if (ops->config && ops->enable && ops->disable)
+		return true;
+
+	/* driver supports atomic operation */
+	if (ops->apply)
+		return true;
+
+	return false;
+}
+
 /**
  * pwmchip_add_with_polarity() - register a new PWM chip
  * @chip: the PWM chip to add

@@ -244,8 +263,10 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 	unsigned int i;
 	int ret;
 
-	if (!chip || !chip->dev || !chip->ops || !chip->ops->config ||
-	    !chip->ops->enable || !chip->ops->disable || !chip->npwm)
+	if (!chip || !chip->dev || !chip->ops || !chip->npwm)
+		return -EINVAL;
+
+	if (!pwm_ops_check(chip->ops))
 		return -EINVAL;
 
 	mutex_lock(&pwm_lock);

@@ -445,7 +466,19 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
 	if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
 		return -EINVAL;
 
-	err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
+	if (pwm->chip->ops->apply) {
+		struct pwm_state state;
+
+		pwm_get_state(pwm, &state);
+		state.period = period_ns;
+		state.duty_cycle = duty_ns;
+
+		err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
+	} else {
+		err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns,
+					     period_ns);
+	}
+
 	if (err)
 		return err;
 

@@ -473,6 +506,18 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
 	if (!pwm || !pwm->chip->ops)
 		return -EINVAL;
 
+	if (pwm->chip->ops->apply) {
+		struct pwm_state state;
+
+		pwm_get_state(pwm, &state);
+		state.polarity = polarity;
+		err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
+		if (!err)
+			pwm->state.polarity = polarity;
+
+		return err;
+	}
+
 	if (!pwm->chip->ops->set_polarity)
 		return -ENOSYS;
 

@@ -497,17 +542,29 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
  */
 int pwm_enable(struct pwm_device *pwm)
 {
-	if (pwm && !pwm_is_enabled(pwm)) {
-		int err;
+	int err;
 
-		err = pwm->chip->ops->enable(pwm->chip, pwm);
-		if (!err)
-			pwm->state.enabled = true;
+	if (!pwm)
+		return -EINVAL;
 
-		return err;
+	if (pwm_is_enabled(pwm))
+		return 0;
+
+	if (pwm->chip->ops->apply) {
+		struct pwm_state state;
+
+		pwm_get_state(pwm, &state);
+		state.enabled = true;
+
+		err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
+	} else {
+		err = pwm->chip->ops->enable(pwm->chip, pwm);
 	}
 
-	return pwm ? 0 : -EINVAL;
+	if (!err)
+		pwm->state.enabled = true;
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(pwm_enable);
 

@@ -517,13 +574,74 @@ EXPORT_SYMBOL_GPL(pwm_enable);
  */
 void pwm_disable(struct pwm_device *pwm)
 {
-	if (pwm && pwm_is_enabled(pwm)) {
+	if (!pwm || !pwm_is_enabled(pwm))
+		return;
+
+	if (pwm->chip->ops->apply) {
+		struct pwm_state state;
+
+		pwm_get_state(pwm, &state);
+		state.enabled = false;
+
+		pwm->chip->ops->apply(pwm->chip, pwm, &state);
+	} else {
 		pwm->chip->ops->disable(pwm->chip, pwm);
-		pwm->state.enabled = false;
 	}
+
+	pwm->state.enabled = false;
 }
 EXPORT_SYMBOL_GPL(pwm_disable);
 
+/**
+ * pwm_apply_state() - atomically apply a new state to a PWM device
+ * @pwm: PWM device
+ * @state: new state to apply
+ */
+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
+{
+	int err = 0;
+
+	if (!pwm)
+		return -EINVAL;
+
+	if (!memcmp(state, &pwm->state, sizeof(*state)))
+		return 0;
+
+	if (pwm->chip->ops->apply) {
+		err = pwm->chip->ops->apply(pwm->chip, pwm, state);
+		if (!err)
+			pwm_set_state(pwm, state);
+	} else {
+		/*
+		 * FIXME: restore the initial state in case of error.
+		 */
+		if (state->polarity != pwm->state.polarity) {
+			pwm_disable(pwm);
+			err = pwm_set_polarity(pwm, state->polarity);
+			if (err)
+				goto out;
+		}
+
+		if (state->period != pwm->state.period ||
+		    state->duty_cycle != pwm->state.duty_cycle) {
+			err = pwm_config(pwm, state->period, state->duty_cycle);
+			if (err)
+				goto out;
+		}
+
+		if (state->enabled != pwm->state.enabled) {
+			if (state->enabled)
+				err = pwm_enable(pwm);
+			else
+				pwm_disable(pwm);
+		}
+	}
+
+out:
+	return err;
+}
+EXPORT_SYMBOL_GPL(pwm_apply_state);
+
 static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
 {
 	struct pwm_chip *chip;

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index cddb12b..11ee041 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h

@@ -171,6 +171,30 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 	return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
 }
 
+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
+
+/**
+ * pwm_get_state() - retrieve the current PWM state
+ * @pwm: PWM device
+ * @state: state to fill with the current PWM state
+ */
+static inline void pwm_get_state(struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	*state = pwm->state;
+}
+
+/**
+ * pwm_get_default_state() - retrieve the default PWM state
+ * @pwm: PWM device
+ * @state: state to fill with the default PWM state
+ */
+static inline void pwm_get_default_state(struct pwm_device *pwm,
+					 struct pwm_state *state)
+{
+	*state = pwm->default_state;
+}
+
 /**
  * struct pwm_ops - PWM controller operations
  * @request: optional hook for requesting a PWM

@@ -179,6 +203,7 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
  * @set_polarity: configure the polarity of this PWM
  * @enable: enable PWM output toggling
  * @disable: disable PWM output toggling
+ * @apply: atomically apply a new PWM config
  * @reset_state: reset the current PWM state (pwm->state) to the actual
  *		 hardware state. This function is only called once per
  *		 PWM device when the PWM chip is registered.

@@ -194,6 +219,8 @@ struct pwm_ops {
 			    enum pwm_polarity polarity);
 	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
 	void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
+	int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
+		     const struct pwm_state *state);
 	void (*reset_state)(struct pwm_chip *chip, struct pwm_device *pwm);
 #ifdef CONFIG_DEBUG_FS
 	void (*dbg_show)(struct pwm_chip *chip, struct seq_file *s);

-- 
1.9.1

Prepare the addition of the PWM initial state retrieval by adding a default
state where all the parameters retrieved from DT, platform data or
statically forced by the hardware will be stored.
Once done we will be able to store the initial state in the ->state field
without risking to loose the default parameters.

Update the pwm_set/get_default_xxx helpers accordingly.

Signed-off-by: Boris Brezillon <redacted>
---
 include/linux/pwm.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 433a097..f8cc460 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h

@@ -101,6 +101,7 @@ struct pwm_state {
  * @chip: PWM chip providing this PWM device
  * @chip_data: chip-private data associated with the PWM device
  * @state: curent PWM channel state
+ * @default_state: default PWM channel state
  */
 struct pwm_device {
 	const char *label;

@@ -111,6 +112,7 @@ struct pwm_device {
 	void *chip_data;
 
 	struct pwm_state state;
+	struct pwm_state default_state;
 };
 
 static inline bool pwm_is_enabled(const struct pwm_device *pwm)

@@ -127,7 +129,8 @@ static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
 static inline void pwm_set_default_period(struct pwm_device *pwm,
 					  unsigned int period)
 {
-	pwm_set_period(pwm, period);
+	if (pwm)
+		pwm->default_state.period = period;
 }
 
 static inline unsigned int pwm_get_period(const struct pwm_device *pwm)

@@ -137,7 +140,7 @@ static inline unsigned int pwm_get_period(const struct pwm_device *pwm)
 
 static inline unsigned int pwm_get_default_period(const struct pwm_device *pwm)
 {
-	return pwm_get_period(pwm);
+	return pwm ? pwm->default_state.period : 0;
 }
 
 static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)

@@ -159,7 +162,8 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity);
 static inline void pwm_set_default_polarity(struct pwm_device *pwm,
 					    enum pwm_polarity polarity)
 {
-	pwm_set_polarity(pwm, polarity);
+	if (pwm)
+		pwm->default_state.polarity = polarity;
 }
 
 static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)

-- 
1.9.1

Add a ->reset_state() function to the pwm_ops struct to let PWM drivers
initialize the PWM state attached to a PWM device.

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/pwm/core.c  | 3 +++
 include/linux/pwm.h | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 963238c3..60ad758 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c

@@ -270,6 +270,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->hwpwm = i;
 		pwm_set_default_polarity(pwm, polarity);
 
+		if (chip->ops->reset_state)
+			chip->ops->reset_state(chip, pwm);
+
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
 

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index f8cc460..cddb12b 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h

@@ -179,6 +179,9 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
  * @set_polarity: configure the polarity of this PWM
  * @enable: enable PWM output toggling
  * @disable: disable PWM output toggling
+ * @reset_state: reset the current PWM state (pwm->state) to the actual
+ *		 hardware state. This function is only called once per
+ *		 PWM device when the PWM chip is registered.
  * @dbg_show: optional routine to show contents in debugfs
  * @owner: helps prevent removal of modules exporting active PWMs
  */

@@ -191,6 +194,7 @@ struct pwm_ops {
 			    enum pwm_polarity polarity);
 	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
 	void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
+	void (*reset_state)(struct pwm_chip *chip, struct pwm_device *pwm);
 #ifdef CONFIG_DEBUG_FS
 	void (*dbg_show)(struct pwm_chip *chip, struct seq_file *s);
 #endif

-- 
1.9.1

From: Heiko Stübner <heiko@sntech.de>

The pwm-states make it possible to also output the polarity, duty cycle
and period information in the debugfs pwm summary-outout.
This makes it easier to gather overview information about pwms without
needing to walk through the sysfs attributes of every pwm.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Boris Brezillon <redacted>
---
 drivers/pwm/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ff3c662..7eba2f3 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c

@@ -1018,6 +1018,11 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 		if (pwm_is_enabled(pwm))
 			seq_puts(s, " enabled");
 
+		seq_printf(s, " period:%uns", pwm_get_period(pwm));
+		seq_printf(s, " duty:%uns", pwm_get_duty_cycle(pwm));
+		seq_printf(s, " polarity:%s", pwm_get_polarity(pwm) ? "inverse"
+								    : "normal");
+
 		seq_puts(s, "\n");
 	}
 }

-- 
1.9.1

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 <redacted>
---
 drivers/regulator/pwm-regulator.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 9ffdbd6..449e3b3 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c

@@ -41,10 +41,35 @@ 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);
+
+	if (!pwm_state.period)
+		return;
+
+	dutycycle = (pwm_state.duty_cycle * 100) / pwm_state.period;
+
+	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;
 }
 

@@ -211,6 +236,7 @@ static int pwm_regulator_init_table(struct platform_device *pdev,
 		return ret;
 	}
 
+	drvdata->state			= -EINVAL;
 	drvdata->duty_cycle_table	= duty_cycle_table;
 	pwm_regulator_desc.ops		= &pwm_regulator_voltage_table_ops;
 	pwm_regulator_desc.n_voltages	= length / sizeof(*duty_cycle_table);

-- 
1.9.1

Implement the ->enable(), ->disable() and ->is_enabled methods and remove
the PWM call in ->set_voltage_sel().
This is particularly important for critical regulators tagged as always-on,
because not claiming the PWM (and its dependencies) might lead to
unpredictable behavior (like a system hang because the PWM clk is only
claimed when the PWM device is enabled).

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/regulator/pwm-regulator.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index cc549b7..9ffdbd6 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c

@@ -69,12 +69,6 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
 
 	drvdata->state = selector;
 
-	ret = pwm_enable(drvdata->pwm);
-	if (ret) {
-		dev_err(&rdev->dev, "Failed to enable PWM\n");
-		return ret;
-	}
-
 	return 0;
 }
 

@@ -89,6 +83,29 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
 	return drvdata->duty_cycle_table[selector].uV;
 }
 
+static int pwm_regulator_enable(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	return pwm_enable(drvdata->pwm);
+}
+
+static int pwm_regulator_disable(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	pwm_disable(drvdata->pwm);
+
+	return 0;
+}
+
+static int pwm_regulator_is_enabled(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	return pwm_is_enabled(drvdata->pwm);
+}
+
 /**
  * Continuous voltage call-backs
  */

@@ -144,11 +161,17 @@ static struct regulator_ops pwm_regulator_voltage_table_ops = {
 	.get_voltage_sel = pwm_regulator_get_voltage_sel,
 	.list_voltage    = pwm_regulator_list_voltage,
 	.map_voltage     = regulator_map_voltage_iterate,
+	.enable          = pwm_regulator_enable,
+	.disable         = pwm_regulator_disable,
+	.is_enabled      = pwm_regulator_is_enabled,
 };
 
 static struct regulator_ops pwm_regulator_voltage_continuous_ops = {
 	.get_voltage = pwm_regulator_get_voltage,
 	.set_voltage = pwm_regulator_set_voltage,
+	.enable          = pwm_regulator_enable,
+	.disable         = pwm_regulator_disable,
+	.is_enabled      = pwm_regulator_is_enabled,
 };
 
 static struct regulator_desc pwm_regulator_desc = {

-- 
1.9.1

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

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/pwm/pwm-rockchip.c | 53 +++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 6eab25c..48b814d 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c

@@ -50,7 +50,8 @@ struct rockchip_pwm_data {
 	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 (*reset_state)(struct pwm_chip *chip, struct pwm_device *pwm);
 };
 

@@ -60,7 +61,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;

@@ -89,14 +91,15 @@ static void rockchip_pwm_reset_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;

@@ -165,7 +168,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,15 +184,8 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
 	duty = div;
 
-	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;
 }

@@ -208,43 +203,53 @@ static int rockchip_pwm_set_polarity(struct pwm_chip *chip,
 	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,
+			      const struct pwm_state *state)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	bool enabled = pwm_is_enabled(pwm);
 	int ret;
 
 	ret = clk_enable(pc->clk);
 	if (ret)
 		return ret;
 
-	pc->data->set_enable(chip, pwm, true);
+	if (state->polarity != pwm_get_polarity(pwm) && 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 != pwm_is_enabled(pwm))
+			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;
+	}
 
-	pc->data->set_enable(chip, pwm, false);
+	if (state->enabled != enabled)
+		pc->data->set_enable(chip, pwm, state->enabled,
+				     state->polarity);
 
+out:
 	clk_disable(pc->clk);
+
+	return ret;
 }
 
 static const struct pwm_ops rockchip_pwm_ops_v1 = {
 	.reset_state = rockchip_pwm_reset_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 = {
 	.reset_state = rockchip_pwm_reset_state,
 	.config = rockchip_pwm_config,
+	.apply = rockchip_pwm_apply,
 	.set_polarity = rockchip_pwm_set_polarity,
-	.enable = rockchip_pwm_enable,
-	.disable = rockchip_pwm_disable,
 	.owner = THIS_MODULE,
 };
 

-- 
1.9.1

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

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/pwm/pwm-rockchip.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 7d9cc90..6eab25c 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c

@@ -51,6 +51,7 @@ struct rockchip_pwm_data {
 
 	void (*set_enable)(struct pwm_chip *chip,
 			   struct pwm_device *pwm, bool enable);
+	void (*reset_state)(struct pwm_chip *chip, struct pwm_device *pwm);
 };
 
 static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)

@@ -75,6 +76,18 @@ static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip,
 	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_reset_state_v1(struct pwm_chip *chip,
+					struct pwm_device *pwm)
+{
+	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)
+		pwm->state.enabled = true;
+}
+
 static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
 				       struct pwm_device *pwm, bool enable)
 {

@@ -98,6 +111,54 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
 	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_reset_state_v2(struct pwm_chip *chip,
+					struct pwm_device *pwm)
+{
+	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;
+
+	pwm->state.enabled = true;
+
+	if (!(val & PWM_DUTY_POSITIVE))
+		pwm->state.polarity = PWM_POLARITY_INVERSED;
+}
+
+static void rockchip_pwm_reset_state(struct pwm_chip *chip,
+				     struct pwm_device *pwm)
+{
+	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;
+	do_div(tmp, clk_rate);
+	pwm->state.period = tmp;
+
+	tmp = readl(pc->base + pc->data->regs.duty);
+	tmp *= pc->data->prescaler * NSEC_PER_SEC;
+	do_div(tmp, clk_rate);
+	pwm->state.duty_cycle = tmp;
+
+	pc->data->reset_state(chip, chip->pwms);
+
+	if (!pwm_is_enabled(pwm))
+		clk_disable(pc->clk);
+}
+
 static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			       int duty_ns, int period_ns)
 {

@@ -171,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 = {
+	.reset_state = rockchip_pwm_reset_state,
 	.config = rockchip_pwm_config,
 	.enable = rockchip_pwm_enable,
 	.disable = rockchip_pwm_disable,

@@ -178,6 +240,7 @@ static const struct pwm_ops rockchip_pwm_ops_v1 = {
 };
 
 static const struct pwm_ops rockchip_pwm_ops_v2 = {
+	.reset_state = rockchip_pwm_reset_state,
 	.config = rockchip_pwm_config,
 	.set_polarity = rockchip_pwm_set_polarity,
 	.enable = rockchip_pwm_enable,

@@ -195,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,
+	.reset_state = rockchip_pwm_reset_state_v1,
 };
 
 static const struct rockchip_pwm_data pwm_data_v2 = {

@@ -207,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,
+	.reset_state = rockchip_pwm_reset_state_v2,
 };
 
 static const struct rockchip_pwm_data pwm_data_vop = {

@@ -219,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,
+	.reset_state = rockchip_pwm_reset_state_v2,
 };
 
 static const struct of_device_id rockchip_pwm_dt_ids[] = {

-- 
1.9.1

Prepare the transition to PWM atomic update by moving the enabled/disabled
state into the pwm_state struct. This way we can easily update the whole
PWM state by copying the new state in the ->state field.

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/pwm/core.c  | 15 ++++++++++++---
 include/linux/pwm.h |  7 ++++---
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 09037de..963238c3 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c

@@ -494,8 +494,15 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
  */
 int pwm_enable(struct pwm_device *pwm)
 {
-	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
-		return pwm->chip->ops->enable(pwm->chip, pwm);
+	if (pwm && !pwm_is_enabled(pwm)) {
+		int err;
+
+		err = pwm->chip->ops->enable(pwm->chip, pwm);
+		if (!err)
+			pwm->state.enabled = true;
+
+		return err;
+	}
 
 	return pwm ? 0 : -EINVAL;
 }

@@ -507,8 +514,10 @@ EXPORT_SYMBOL_GPL(pwm_enable);
  */
 void pwm_disable(struct pwm_device *pwm)
 {
-	if (pwm && test_and_clear_bit(PWMF_ENABLED, &pwm->flags))
+	if (pwm && pwm_is_enabled(pwm)) {
 		pwm->chip->ops->disable(pwm->chip, pwm);
+		pwm->state.enabled = false;
+	}
 }
 EXPORT_SYMBOL_GPL(pwm_disable);
 

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e0e0ed8..433a097 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h

@@ -75,8 +75,7 @@ enum pwm_polarity {
 
 enum {
 	PWMF_REQUESTED = 1 << 0,
-	PWMF_ENABLED = 1 << 1,
-	PWMF_EXPORTED = 1 << 2,
+	PWMF_EXPORTED = 1 << 1,
 };
 
 /*

@@ -84,11 +83,13 @@ enum {
  * @period: PWM period (in nanoseconds)
  * @duty_cycle: PWM duty cycle (in nanoseconds)
  * @polarity: PWM polarity
+ * @enabled: PWM enabled status
  */
 struct pwm_state {
 	unsigned int period;
 	unsigned int duty_cycle;
 	enum pwm_polarity polarity;
+	bool enabled;
 };
 
 /**

@@ -114,7 +115,7 @@ struct pwm_device {
 
 static inline bool pwm_is_enabled(const struct pwm_device *pwm)
 {
-	return test_bit(PWMF_ENABLED, &pwm->flags);
+	return pwm->state.enabled;
 }
 
 static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)

-- 
1.9.1

The PWM period will be set when calling pwm_config. Remove this useless
call to pwm_set_period, which might mess up with the initial PWM state
once we have added proper support for PWM init state retrieval.

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/video/backlight/pwm_bl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index ae498c1..71944f8 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c

@@ -293,12 +293,14 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	 * period, parsed from the DT, in the PWM device. For the non-DT case,
 	 * set the period from platform data if it has not already been set
 	 * via the PWM lookup table.
+	 * FIXME: This assignment should be dropped as soon as all the boards
+	 * have moved to the PWM lookup table approach. The same goes for the
+	 * pb->period field which should be replaced by
+	 * pwm_get_default_period() calls.
 	 */
 	pb->period = pwm_get_default_period(pb->pwm);
-	if (!pb->period && (data->pwm_period_ns > 0)) {
+	if (!pb->period && (data->pwm_period_ns > 0))
 		pb->period = data->pwm_period_ns;
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-	}
 
 	pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
 

-- 
1.9.1

On Mon, Sep 21, 2015 at 11:33:29AM +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.

Acked-by: Mark Brown <broonie@kernel.org>

On Mon, Sep 21, 2015 at 11:33:28AM +0200, Boris Brezillon wrote:

Implement the ->enable(), ->disable() and ->is_enabled methods and remove
the PWM call in ->set_voltage_sel().

I'll apply this so it definitely makes it into mainline, I'm happy to
provide a tag for this so it can be merged into the PWM tree if the rest
of the series is ready.

Hi Boris,

Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon:

This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.

I gave this v3 a spin on a rk3288-veyron device with the pwm-regulator 
supplying vdd_logic. Looks really nice and runs fine, so the series

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

Hi Boris,

On 09/21/2015 11:33 AM, Boris Brezillon wrote:

4 more quoted lines

Acked-by: Jacek Anaszewski <redacted>

-- 
Best Regards,
Jacek Anaszewski

On Mon, 21 Sep 2015, Boris Brezillon wrote:

When requested by a user, the PWM is assigned a default period and polarity
extracted from the DT, the platform data or statically set by the driver.
Those default values are currently stored in the period and polarity
fields of the pwm_device struct, but they will be stored somewhere else
once we have introduced the architecture allowing for hardware state
retrieval.

The pwm_set_default_polarity and pwm_set_default_period should only be
used by PWM drivers or the PWM core infrastructure to specify the
default period and polarity values.

PWM users might call the pwm_get_default_period to query the default
period value. There is currently no helper to query the default
polarity, but it might be added later on if there is a need for it.

This patch also modifies all the places where the default helpers should
be used in place of the standard ones.

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/leds/leds-pwm.c              |  2 +-
 drivers/pwm/core.c                   | 14 +++++++-------
 drivers/pwm/pwm-pxa.c                |  2 +-
 drivers/pwm/pwm-sun4i.c              |  3 ++-
 drivers/regulator/pwm-regulator.c    |  4 ++--
 drivers/video/backlight/lm3630a_bl.c |  4 ++--
 drivers/video/backlight/pwm_bl.c     |  2 +-

Acked-by: Lee Jones <redacted>

163 more quoted lines

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Mon, 21 Sep 2015, Boris Brezillon wrote:

The PWM period will be set when calling pwm_config. Remove this useless
call to pwm_set_period, which might mess up with the initial PWM state
once we have added proper support for PWM init state retrieval.

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/video/backlight/pwm_bl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Acked-by: Lee Jones <redacted>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Hi Thierry,

On Mon, 21 Sep 2015 11:33:17 +0200
Boris Brezillon [off-list ref] wrote:

Hello,

This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.

Anything preventing this series from being applied (AKA ping)?

Best Regards,

Boris

Changes since v2:
- rebased on top of 4.3-rc2
- reintroduced pwm-regulator patches

Changes since v1:
- dropped applied patches
- squashed Heiko's fixes into the rockchip driver changes
- made a few cosmetic changes
- added kerneldoc comments
- added Heiko's patch to display more information in debugfs
- dropped pwm-regulator patches (should be submitted separately)

Boris Brezillon (11):
  pwm: introduce default period and polarity concepts
  pwm: define a new pwm_state struct
  pwm: move the enabled/disabled info to pwm_state struct
  backlight: pwm_bl: remove useless call to pwm_set_period
  pwm: declare a default PWM state
  pwm: add the PWM initial state retrieval infra
  pwm: add the core infrastructure to allow atomic update
  pwm: rockchip: add initial state retrieval
  pwm: rockchip: add support for atomic update
  regulator: pwm: implement ->enable(), ->disable() and ->is_enabled
    methods
  regulator: pwm: properly initialize the ->state field

Heiko Stübner (1):
  pwm: add information about polarity, duty cycle and period to debugfs

 drivers/leds/leds-pwm.c              |   2 +-
 drivers/pwm/core.c                   | 169 +++++++++++++++++++++++++++++++----
 drivers/pwm/pwm-pxa.c                |   2 +-
 drivers/pwm/pwm-rockchip.c           | 119 +++++++++++++++++++-----
 drivers/pwm/pwm-sun4i.c              |   3 +-
 drivers/regulator/pwm-regulator.c    |  65 ++++++++++++--

6 more quoted lines

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

Hi Thierry,

Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon:

Hello,

This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.

is anything more blocking this series? It's now sitting on the lists for 
nearly a month and everybody seems happy with it, so it would be really nice 
to have in mainline :-) .

Especially as this also makes it possible for Rockchip Chromebooks to actually 
control the logic-regulator that is implemented as pwm-regulator there.


Thanks
Heiko

On 21/09/2015 at 11:33:18 +0200, Boris Brezillon wrote :

When requested by a user, the PWM is assigned a default period and polarity
extracted from the DT, the platform data or statically set by the driver.
Those default values are currently stored in the period and polarity
fields of the pwm_device struct, but they will be stored somewhere else
once we have introduced the architecture allowing for hardware state
retrieval.

The pwm_set_default_polarity and pwm_set_default_period should only be
used by PWM drivers or the PWM core infrastructure to specify the
default period and polarity values.

PWM users might call the pwm_get_default_period to query the default
period value. There is currently no helper to query the default
polarity, but it might be added later on if there is a need for it.

This patch also modifies all the places where the default helpers should
be used in place of the standard ones.

Signed-off-by: Boris Brezillon <redacted>

For pwm-sun4i:
Reviewed-by: Alexandre Belloni <redacted>


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote:

Hi Thierry,

Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon:

quoted

Hello,

This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.

is anything more blocking this series? It's now sitting on the lists for 
nearly a month and everybody seems happy with it, so it would be really nice 
to have in mainline :-) .

Especially as this also makes it possible for Rockchip Chromebooks to actually 
control the logic-regulator that is implemented as pwm-regulator there.

Last time I tried to put this into linux-next I got immediately
bombarded by a number of build failures, so I backed things out. The
current plan is to give this another try after v4.4-rc1.

Thierry

Hi Thierry,

On Tue, 10 Nov 2015 18:34:16 +0100
Thierry Reding [off-list ref] wrote:

On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote:

quoted

Hi Thierry,

Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon:

quoted

Hello,

This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.

is anything more blocking this series? It's now sitting on the lists for 
nearly a month and everybody seems happy with it, so it would be really nice 
to have in mainline :-) .

Especially as this also makes it possible for Rockchip Chromebooks to actually 
control the logic-regulator that is implemented as pwm-regulator there.

Last time I tried to put this into linux-next I got immediately
bombarded by a number of build failures, so I backed things out. The
current plan is to give this another try after v4.4-rc1.

Could you paste the build failures (I didn't receive any notification)?
BTW, I just rebased my branch on pwm/for-next and it seems to compile
correctly, but maybe you're compiling on more platforms (or different
drivers than I do).

Best Regards,

Boris



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

Thierry and Boris,

On Tue, Nov 10, 2015 at 9:34 AM, Thierry Reding
[off-list ref] wrote:

On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote:

quoted

Hi Thierry,

Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon:

quoted

Hello,

This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.

is anything more blocking this series? It's now sitting on the lists for
nearly a month and everybody seems happy with it, so it would be really nice
to have in mainline :-) .

Especially as this also makes it possible for Rockchip Chromebooks to actually
control the logic-regulator that is implemented as pwm-regulator there.

Last time I tried to put this into linux-next I got immediately
bombarded by a number of build failures, so I backed things out. The
current plan is to give this another try after v4.4-rc1.

We're now into the 4.5 timeframe.  Does anyone have a concrete set of
things that need to happen before this patch series makes it into
mainline?

From searching I see that the latest version of this series is v4 and
there are a smattering of comments on the 24-patch series.  Presumably
a v5 needs to be posted to address those things.

...but it looks like the big sticking point is that Boris is waiting
for a response to his questions in
<https://patchwork.kernel.org/patch/7622881/>.  Thierry: can you give
Boris some direction for what else he needs to do?  We need to come up
with _some_ solution since this series gets us much better support for
PWM regulators.  Without this series or some other solution, PWM
regulators aren't usable in mainline on any system that uses them for
system-critical rails.  Nearly all Rockchip reference boards and
shipping devices uses a PWM regulator for the system-critidal "logic"
rail.  That means any patches which need to change this rail in Linux
are blocked.

If there's already been off-list discussion and Boris already knows
what the next steps are then my apologies and I'll wait patiently for
the next series.  ;)

Thanks!

-Doug

On Mon, Jan 25, 2016 at 08:28:31AM -0800, Doug Anderson wrote:

Thierry and Boris,

On Tue, Nov 10, 2015 at 9:34 AM, Thierry Reding
[off-list ref] wrote:

quoted

On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote:

quoted

Hi Thierry,

Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon:

quoted

Hello,

This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.

is anything more blocking this series? It's now sitting on the lists for
nearly a month and everybody seems happy with it, so it would be really nice
to have in mainline :-) .

Especially as this also makes it possible for Rockchip Chromebooks to actually
control the logic-regulator that is implemented as pwm-regulator there.

Last time I tried to put this into linux-next I got immediately
bombarded by a number of build failures, so I backed things out. The
current plan is to give this another try after v4.4-rc1.

We're now into the 4.5 timeframe.  Does anyone have a concrete set of
things that need to happen before this patch series makes it into
mainline?

I think the current status is that we're more or less blocked on the
decision on what the reset state of the PWM should be. The question is
what to do if the PWM hardware readout differs from the settings found
in DT.

From searching I see that the latest version of this series is v4 and
there are a smattering of comments on the 24-patch series.  Presumably
a v5 needs to be posted to address those things.

...but it looks like the big sticking point is that Boris is waiting
for a response to his questions in
<https://patchwork.kernel.org/patch/7622881/>.  Thierry: can you give
Boris some direction for what else he needs to do?  We need to come up
with _some_ solution since this series gets us much better support for
PWM regulators.  Without this series or some other solution, PWM
regulators aren't usable in mainline on any system that uses them for
system-critical rails.  Nearly all Rockchip reference boards and
shipping devices uses a PWM regulator for the system-critidal "logic"
rail.  That means any patches which need to change this rail in Linux
are blocked.

I really don't understand this design decision. I presume that the PWM
controlling this system-critical logic is driven by the SoC? So if the
regulator is system-critical, doesn't that make it a chicken and egg
problem? How can the SoC turn the PWM on if it doesn't have power? But
perhaps I'm completely misunderstanding what you're saying. Perhaps if
somebody could summarize how exactly this works, it would help better
understand the requirements or what's the correct thing to do.

If there's already been off-list discussion and Boris already knows
what the next steps are then my apologies and I'll wait patiently for
the next series.  ;)

I don't think we reached a conclusion on this. And to be honest, I'm not
sure what the right way forward is in this situation. So in order to
make some forward progress I suggest we start a discussion, hopefully
that will clarify the situation and help lead to the conclusion. Let me
recap where we are:

Boris' series has two goals: 1) allow seamless hand-off from firmware to
kernel of a PWM channel and 2) apply changes to a regulator in a single
atomic operation. To achieve this the concept of PWM state is introduced
which encapsulates the settings of a PWM channel. On driver probe the
current state will be read from hardware and when one or more parameters
are to be changed, the current state is duplicated, the new values set
in the state and the new state applied.

The problem that we've encountered is that since the PWM parameters are
specified in DT (or board files), there is the possibility of the PWM
hardware state and the board parameters disagreeing. To resolve such
situations there must be a point in time where both hardware state and
software state must be synchronized. Now the most straightforward way to
do that would be to simply apply the software state and be done with it.
However the software state initially lacks the duty cycle because it is
a parameter that usually depends on the use-case (for backlight for
instance it controls the brightness, for regulators it controls the
output voltage, ...).

Applying the software state as-is also means that there's no reason at
all to read out the hardware state in the first place, because it will
simply be discarded.

An alternative would be to discard the software state and trust the
hardware to be configured correctly. That's somewhat risky because we
don't know if the hardware is properly configured. Or Linux might have
different requirements from the firmware and hence needs to configure
the PWM differently.

Neither of the above are very attractive options. The best I've been
able to come up with so far is to completely remove this decision from
the PWM subsystem and let users handle this. That is, a PWM regulator
driver would have to have all the knowledge about how to configure the

17 more lines

Hi Thierry,

On Mon, 25 Jan 2016 18:08:55 +0100
Thierry Reding [off-list ref] wrote:

On Mon, Jan 25, 2016 at 08:28:31AM -0800, Doug Anderson wrote:

quoted

Thierry and Boris,

On Tue, Nov 10, 2015 at 9:34 AM, Thierry Reding
[off-list ref] wrote:

quoted

On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote:

quoted

Hi Thierry,

Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon:

quoted

Hello,

This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.

is anything more blocking this series? It's now sitting on the lists for
nearly a month and everybody seems happy with it, so it would be really nice
to have in mainline :-) .

Especially as this also makes it possible for Rockchip Chromebooks to actually
control the logic-regulator that is implemented as pwm-regulator there.

Last time I tried to put this into linux-next I got immediately
bombarded by a number of build failures, so I backed things out. The
current plan is to give this another try after v4.4-rc1.

We're now into the 4.5 timeframe.  Does anyone have a concrete set of
things that need to happen before this patch series makes it into
mainline?

I think the current status is that we're more or less blocked on the
decision on what the reset state of the PWM should be. The question is
what to do if the PWM hardware readout differs from the settings found
in DT.

I think I already explained my PoV regarding the default/reference
setting (or what you later call the software state).
To me, this has never matched the state of the PWM device, it's just a
reference duty-cycle and polarity configuration that PWM users can
decide to apply or not. Currently, there's no mechanism to apply the
reference PWM config when a user request a PWM (which is the only
moment we could be able to apply this config, since reference configs
are per users and not attached to the PWM device itself).

All I'm trying to do in this series is teach some PWM users to make use
of hardware read-out state instead of blindly applying a default config
that can generate glitches in the PWM signal.

quoted

From searching I see that the latest version of this series is v4 and
there are a smattering of comments on the 24-patch series.  Presumably
a v5 needs to be posted to address those things.

...but it looks like the big sticking point is that Boris is waiting
for a response to his questions in
<https://patchwork.kernel.org/patch/7622881/>.  Thierry: can you give
Boris some direction for what else he needs to do?  We need to come up
with _some_ solution since this series gets us much better support for
PWM regulators.  Without this series or some other solution, PWM
regulators aren't usable in mainline on any system that uses them for
system-critical rails.  Nearly all Rockchip reference boards and
shipping devices uses a PWM regulator for the system-critidal "logic"
rail.  That means any patches which need to change this rail in Linux
are blocked.

I really don't understand this design decision. I presume that the PWM
controlling this system-critical logic is driven by the SoC? So if the
regulator is system-critical, doesn't that make it a chicken and egg
problem? How can the SoC turn the PWM on if it doesn't have power? But
perhaps I'm completely misunderstanding what you're saying. Perhaps if
somebody could summarize how exactly this works, it would help better
understand the requirements or what's the correct thing to do.

quoted

If there's already been off-list discussion and Boris already knows
what the next steps are then my apologies and I'll wait patiently for
the next series.  ;)

I don't think we reached a conclusion on this. And to be honest, I'm not
sure what the right way forward is in this situation. So in order to
make some forward progress I suggest we start a discussion, hopefully
that will clarify the situation and help lead to the conclusion. Let me
recap where we are:

Boris' series has two goals: 1) allow seamless hand-off from firmware to
kernel of a PWM channel and 2) apply changes to a regulator in a single
atomic operation. To achieve this the concept of PWM state is introduced
which encapsulates the settings of a PWM channel. On driver probe the
current state will be read from hardware and when one or more parameters

8 more quoted lines

Maybe we're just hunting ghosts here. There's only one valid state, and
this is the 'hardware state'. I reused the pwm_state struct to store the
reference config (what you call software state), but actually that's
not a state, that's a configuration template, it will only become the
PWM state after being applied.
Maybe I should use another struct to clarify that.

Now the most straightforward way to
do that would be to simply apply the software state and be done with it.

Why that? I mean what's the problem of having a reference/template
config that PWM users can rely on if they want to, and still keep
the real state retrieved from hardware read-out? This way PWM users can
decide which one they want to use.

However the software state initially lacks the duty cycle because it is
a parameter that usually depends on the use-case (for backlight for
instance it controls the brightness, for regulators it controls the
output voltage, ...).

Yes, and I perfectly understand that, we don't want to encode the
default duty cycle in the DT.

Applying the software state as-is also means that there's no reason at
all to read out the hardware state in the first place, because it will
simply be discarded.

Yes, but as stated above, this can be a source of glitches, which is
exactly what we're trying to avoid.

An alternative would be to discard the software state and trust the
hardware to be configured correctly. That's somewhat risky because we
don't know if the hardware is properly configured. Or Linux might have
different requirements from the firmware and hence needs to configure
the PWM differently.

Yep, I agree on that too.

Neither of the above are very attractive options. The best I've been
able to come up with so far is to completely remove this decision from
the PWM subsystem and let users handle this. That is, a PWM regulator
driver would have to have all the knowledge about how to configure the
PWM for its needs. So upon probe, the PWM regulator driver would inspect
the current state of the PWM and adjust if necessary, then apply again.
Ideally of course it wouldn't have to do anything because the hardware
PWM state would match the software configuration. The idea here is that
the PWM regulator driver knows exactly what duty cycle to configure to
obtain the desired output voltage.

That complicates a lot of things. I also suggested another approach:
provide an API to express the duty-cycle value relatively to the period
value (I actually sent a proposal for that, but you didn't comment on
it).
This would keep the PWM user implementations simple, and let the PWM
core switch from the bootloader/firmware period/polarity config to the
DT/PWM-lookup-table one on the first pwm_set_rel_duty_cycle() call.

That doesn't really get us closer, though. There is still the issue of
the user having to deal with two states: the current hardware state and
the software state as configured in DT or board files.

At the risk of repeating myself, I don't think we ever had a software
state, and existing PWM users are just assuming that the PWM devices
they are requesting were not used before. IOW, yes, patching PWM users
to support smoother transitions on the PWM devices require some changes,
but in the other hand, that's not like they were doing any better
before that series. And they still have the choice to completely ignore
the current PWM state and blindly apply their own config (extracted
from the reference PWM config)

Best Regards,

Boris

Like I said, I'm on the fence about this, so I'd appreciate any comments
and perhaps insight from user subsystem maintainers on how they'd like
this to look, or how this has been done with other resources (GPIOs,
...?)

Thierry

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

Hi,

On Mon, Jan 25, 2016 at 9:08 AM, Thierry Reding
[off-list ref] wrote:

I really don't understand this design decision. I presume that the PWM
controlling this system-critical logic is driven by the SoC? So if the
regulator is system-critical, doesn't that make it a chicken and egg
problem? How can the SoC turn the PWM on if it doesn't have power? But
perhaps I'm completely misunderstanding what you're saying. Perhaps if
somebody could summarize how exactly this works, it would help better
understand the requirements or what's the correct thing to do.

Sure, here's how the dang thing works, as I understand it.

First, an overview of PWM regulator in general (maybe you know this,
but to get us on the same page).  There's an external regulator on the
system.  Looking on at least one board I see a TLV62565 specifically.

From the docs of TLV62565, I see it describe the situation as the chip
being able to provide an adjustable output voltage configurable via an
external resistor divider.  In simplified terms words you can adjust
the output voltage of the regulator by tweaking the inputs to one of
its pins.  I'm just a software guy so I can't explain all the details
of it, but the net-net of the situation is is that you can hook this
configuration pin up to the output of a PWM (with a bunch of well
balanced resistors and capacitors) and then you can set the voltage
based on the output of the PWM.


OK, so what happens at bootup?  At bootup most of the pins of the
rk3288 (including the PWM) are configured as inputs with a pull.  The
particular pin hooked up to this PWM has a pulldown.  Remember that
we've got this nicely balanced set of resistors and capacitors hooked
up to the output of our PWM pin?  So what happens when we have this
pin configured as an input?  As I understand it / remember it:

* input w/ no pull: equivalent to 50% duty cycle on the PWM
* input w/ pull down: equivalent to slightly higher voltage than 50%
duty cycle on the PWM
* input w/ pull up: equivalent to slightly lower voltage than 50% duty
cycle on the PWM

On our particular board that means that the rail comes up with roughly
1.1V.  If you drive the PWM at 100% (or set the pin to output high)
you get .86V and if you drive the PWM at 0% (or set the pin to output
low) you get 1.36V.

Now, 1.1V is plenty of voltage to boot the system.  In fact most of
the logic within the SoC can run as low as 0.95V I think.  ...but 0.86
V is not enough to run the logic parts of the system (even at their
default bootup frequencies) 1.1V is _definitely_ not enough to run the

38 more lines

quoted

If there's already been off-list discussion and Boris already knows
what the next steps are then my apologies and I'll wait patiently for
the next series.  ;)

I don't think we reached a conclusion on this. And to be honest, I'm not
sure what the right way forward is in this situation. So in order to
make some forward progress I suggest we start a discussion, hopefully
that will clarify the situation and help lead to the conclusion. Let me
recap where we are:

Boris' series has two goals: 1) allow seamless hand-off from firmware to
kernel of a PWM channel and 2) apply changes to a regulator in a single
atomic operation. To achieve this the concept of PWM state is introduced
which encapsulates the settings of a PWM channel. On driver probe the
current state will be read from hardware and when one or more parameters
are to be changed, the current state is duplicated, the new values set
in the state and the new state applied.

At at even higher level the goal is to support PWM regulator for a
system-critical rail without ever glitching.  If we could solve that
problem in some other way that would also be fine too, I think.

The problem that we've encountered is that since the PWM parameters are
specified in DT (or board files), there is the possibility of the PWM
hardware state and the board parameters disagreeing. To resolve such
situations there must be a point in time where both hardware state and
software state must be synchronized. Now the most straightforward way to
do that would be to simply apply the software state and be done with it.
However the software state initially lacks the duty cycle because it is
a parameter that usually depends on the use-case (for backlight for
instance it controls the brightness, for regulators it controls the
output voltage, ...).

Excuse me for not knowing all details that have been talked about before, but...

A) The software state here is the period and flags (AKA "inverted),
right?  It does seem possible that you could apply the period and
flags while keeping the calculated bootup duty cycle percentage
(presuming that the PWM was actually enabled at probe time and there
was a bootup duty cycle at all).  That would basically say that
whenever you set the period of a PWM then the duty cycle of the PWM
should remain the same percentage.  That actually seems quite sane
IMHO.  It seems much saner than trying to keep the duty cycle "ns"
when the period changes or resetting the PWM to some default when the
period changes.


B) Alternatively, I'd also say that setting a period without a duty
cycle doesn't make a lot of sense.  ...so you could just apply the
period at the same time that you apply the duty cycle the first time.
Presumably you'd want to "lie" to the callers of the PWM subsystem and
tell them that you already changed the period even though the change
won't really take effect until they actually set the duty cycle.  If
anyone cared to find out the true hardware period we could add a new
pwm_get_hw_period().  ...or since the only reason you'd want to know
the hardware period would be if you're trying to read the current duty
cycle percentage, you could instead add "pwm_get_hw_state()" and have
that return both the hardware period ns and duty cycle ns (which is
the most accurate way to return the "percentage" without using fix or
floating point math).


Both of the above options seems like it could be sensible.  The 2nd
seems cleaner because it doesn't require you to recalculate /
approximate the old duty cycle using a new period, but it's slightly
uglier because it no longer returns the true hardware state from
pwm_get_period().

Applying the software state as-is also means that there's no reason at
all to read out the hardware state in the first place, because it will
simply be discarded.

Pretty sure we can't discard the hardware duty cycle at bootup, as per above.

An alternative would be to discard the software state and trust the
hardware to be configured correctly. That's somewhat risky because we
don't know if the hardware is properly configured. Or Linux might have
different requirements from the firmware and hence needs to configure
the PWM differently.

Doesn't seem like a good idea either.

Neither of the above are very attractive options. The best I've been
able to come up with so far is to completely remove this decision from
the PWM subsystem and let users handle this. That is, a PWM regulator
driver would have to have all the knowledge about how to configure the
PWM for its needs. So upon probe, the PWM regulator driver would inspect
the current state of the PWM and adjust if necessary, then apply again.
Ideally of course it wouldn't have to do anything because the hardware
PWM state would match the software configuration. The idea here is that
the PWM regulator driver knows exactly what duty cycle to configure to
obtain the desired output voltage.

I think this is like my suggestion B), right?  AKA the PWM regulator
would be the sole caller of pwm_get_hw_state() and it would use this
to figure out the existing duty cycle percentage.  Then it would
translate that into "ns" and would set the duty cycle.  Upon the first
set of the duty cycle both the period and duty cycle would be applied
at the same time.

That doesn't really get us closer, though. There is still the issue of
the user having to deal with two states: the current hardware state and
the software state as configured in DT or board files.

I think the only users that need to deal with this are one that need a
seamless transition from bootup settings.  Adding a new API call to
support a new feature like this doesn't seem insane, and anyone who
doesn't want this new feature can just never call the new API.

The only thing that would "change" from the point of view of old
drivers is that the PWM period wouldn't change at bootup until the
duty cycle was set.  IMHO this is probably a bug fix.  AKA, for a PWM
backlight, imagine:

1. Firmware sets period to 20000 ns, duty cycle to 8000 ns (40%)
2. Linux boots up and sets period to 10000 ns.  Brightness of
backlight instantly goes to 80%.
3. Eventually something decides to set the backlight duty cycle and it
goes to the proper rate.

Skipping #2 seems like the right move.  ...or did I misunderstand how
something works?

Like I said, I'm on the fence about this, so I'd appreciate any comments
and perhaps insight from user subsystem maintainers on how they'd like
this to look, or how this has been done with other resources (GPIOs,
...?)

-Doug

On Mon, Jan 25, 2016 at 10:51:20AM -0800, Doug Anderson wrote:

Hi,

On Mon, Jan 25, 2016 at 9:08 AM, Thierry Reding
[off-list ref] wrote:

quoted

I really don't understand this design decision. I presume that the PWM
controlling this system-critical logic is driven by the SoC? So if the
regulator is system-critical, doesn't that make it a chicken and egg
problem? How can the SoC turn the PWM on if it doesn't have power? But
perhaps I'm completely misunderstanding what you're saying. Perhaps if
somebody could summarize how exactly this works, it would help better
understand the requirements or what's the correct thing to do.

Sure, here's how the dang thing works, as I understand it.

First, an overview of PWM regulator in general (maybe you know this,
but to get us on the same page).  There's an external regulator on the
system.  Looking on at least one board I see a TLV62565 specifically.

From the docs of TLV62565, I see it describe the situation as the chip
being able to provide an adjustable output voltage configurable via an
external resistor divider.  In simplified terms words you can adjust
the output voltage of the regulator by tweaking the inputs to one of
its pins.  I'm just a software guy so I can't explain all the details
of it, but the net-net of the situation is is that you can hook this
configuration pin up to the output of a PWM (with a bunch of well
balanced resistors and capacitors) and then you can set the voltage
based on the output of the PWM.


OK, so what happens at bootup?  At bootup most of the pins of the
rk3288 (including the PWM) are configured as inputs with a pull.  The
particular pin hooked up to this PWM has a pulldown.  Remember that
we've got this nicely balanced set of resistors and capacitors hooked
up to the output of our PWM pin?  So what happens when we have this
pin configured as an input?  As I understand it / remember it:

* input w/ no pull: equivalent to 50% duty cycle on the PWM
* input w/ pull down: equivalent to slightly higher voltage than 50%
duty cycle on the PWM
* input w/ pull up: equivalent to slightly lower voltage than 50% duty

47 more quoted lines

Thanks for going into so much detail, this helps a lot in understanding
the actual problem we need to solve.

quoted

The problem that we've encountered is that since the PWM parameters are
specified in DT (or board files), there is the possibility of the PWM
hardware state and the board parameters disagreeing. To resolve such
situations there must be a point in time where both hardware state and
software state must be synchronized. Now the most straightforward way to
do that would be to simply apply the software state and be done with it.
However the software state initially lacks the duty cycle because it is
a parameter that usually depends on the use-case (for backlight for
instance it controls the brightness, for regulators it controls the
output voltage, ...).

Excuse me for not knowing all details that have been talked about before, but...

A) The software state here is the period and flags (AKA "inverted),
right?  It does seem possible that you could apply the period and
flags while keeping the calculated bootup duty cycle percentage
(presuming that the PWM was actually enabled at probe time and there
was a bootup duty cycle at all).  That would basically say that
whenever you set the period of a PWM then the duty cycle of the PWM
should remain the same percentage.  That actually seems quite sane
IMHO.  It seems much saner than trying to keep the duty cycle "ns"
when the period changes or resetting the PWM to some default when the
period changes.

That really depends on the use-case. If you're interested in the output
power of the PWM then, yes, this is sane. But it might not be the right
answer in other cases.

B) Alternatively, I'd also say that setting a period without a duty
cycle doesn't make a lot of sense.  ...so you could just apply the
period at the same time that you apply the duty cycle the first time.
Presumably you'd want to "lie" to the callers of the PWM subsystem and
tell them that you already changed the period even though the change
won't really take effect until they actually set the duty cycle.  If
anyone cared to find out the true hardware period we could add a new
pwm_get_hw_period().  ...or since the only reason you'd want to know
the hardware period would be if you're trying to read the current duty
cycle percentage, you could instead add "pwm_get_hw_state()" and have
that return both the hardware period ns and duty cycle ns (which is
the most accurate way to return the "percentage" without using fix or
floating point math).

But then you get into a situation where behaviour is dependent on the
PWM driver, whereas this is really very specific to one specific use-
case.

In the end the PWM API is as low-level as it is because it needs to be
flexible enough to cope with other use-cases. In the general case the
simple truth is that it doesn't make sense to set a period without the
duty cycle and vice versa. That's why pwm_config() takes both as input
parameters. The atomic API is going to take that one step further in
that you need to specify the complete state of the PWM when applying.

Both of the above options seems like it could be sensible.  The 2nd
seems cleaner because it doesn't require you to recalculate /
approximate the old duty cycle using a new period, but it's slightly
uglier because it no longer returns the true hardware state from
pwm_get_period().

quoted

Applying the software state as-is also means that there's no reason at
all to read out the hardware state in the first place, because it will
simply be discarded.

Pretty sure we can't discard the hardware duty cycle at bootup, as per above.

quoted

An alternative would be to discard the software state and trust the
hardware to be configured correctly. That's somewhat risky because we
don't know if the hardware is properly configured. Or Linux might have
different requirements from the firmware and hence needs to configure
the PWM differently.

Doesn't seem like a good idea either.

quoted

Neither of the above are very attractive options. The best I've been
able to come up with so far is to completely remove this decision from
the PWM subsystem and let users handle this. That is, a PWM regulator
driver would have to have all the knowledge about how to configure the
PWM for its needs. So upon probe, the PWM regulator driver would inspect
the current state of the PWM and adjust if necessary, then apply again.
Ideally of course it wouldn't have to do anything because the hardware
PWM state would match the software configuration. The idea here is that
the PWM regulator driver knows exactly what duty cycle to configure to
obtain the desired output voltage.

I think this is like my suggestion B), right?  AKA the PWM regulator
would be the sole caller of pwm_get_hw_state() and it would use this
to figure out the existing duty cycle percentage.  Then it would
translate that into "ns" and would set the duty cycle.  Upon the first
set of the duty cycle both the period and duty cycle would be applied
at the same time.

Yes and no. I think the PWM regulator would need to get the current
hardware state and derive the output power from duty cycle and period.
It would then need to rescale to whatever new period it wants to use
to ensure the same output power is used.

quoted

That doesn't really get us closer, though. There is still the issue of
the user having to deal with two states: the current hardware state and
the software state as configured in DT or board files.

I think the only users that need to deal with this are one that need a
seamless transition from bootup settings.  Adding a new API call to
support a new feature like this doesn't seem insane, and anyone who
doesn't want this new feature can just never call the new API.

The only thing that would "change" from the point of view of old
drivers is that the PWM period wouldn't change at bootup until the
duty cycle was set.  IMHO this is probably a bug fix.  AKA, for a PWM
backlight, imagine:

1. Firmware sets period to 20000 ns, duty cycle to 8000 ns (40%)
2. Linux boots up and sets period to 10000 ns.  Brightness of
backlight instantly goes to 80%.
3. Eventually something decides to set the backlight duty cycle and it
goes to the proper rate.

Skipping #2 seems like the right move.  ...or did I misunderstand how
something works?

I'm not aware of any code in the PWM subsystem that would do this
automatically. If you don't call any of the pwm_*() functions the
hardware state should not be modified. The responsibility is with
the user drivers.

That is, it is up to the regulator or backlight driver to apply any new
configuration to a PWM channel on boot, or leave it as is. For
backlight it's probably fine to simply apply some default, since having
the brightness change on boot isn't going to be terribly irritating.

With the atomic API it would be possible to avoid even this and have the
backlight driver simply read out the current hardware state and apply an
equivalent brightness even if the period changed.

For the regulator case you'd need to read out the current state and then
recompute the values that will yield the same output power given data
specified in DT. I think that much is already implemented in Boris'
series, and it's really only the details that are being debated.

The problematic issue is still that we might have a disparity between
the current hardware state and the state initially specified by DT. In
the general case the DT will specify the period and polarity of the PWM
signal and leave it up to the user driver to determine what a correct
duty cycle would be. With the atomic API we'll essentially have two
states: the current (hardware) state and the "initial" state, which is
what an OS will see as the state to apply. The problem now is that once
you have applied the initial state with a duty cycle you've determined,
there is no longer a need to keep it around. But there's also no way to
know when this is the case. So the controversial part about all this is
when to start using the current state rather that the initial state.

The most straightforward way to solve this would be to apply the initial
configuration on driver probe. That is, when the pwm-regulator driver
gets probed it would retrieve the current and initial states, then
adjust the current state such that it matches the initial state but with
a duty cycle that yields the same output power as the current state, and
finally apply the new state. After that, every regulator_set_voltage()
call could simply operate on the current state and adjust the duty cycle
exclusively.

4 more lines

Thierry

On Wed, Feb 3, 2016 at 6:53 AM, Thierry Reding [off-list ref] wrote:

quoted

A) The software state here is the period and flags (AKA "inverted),
right?  It does seem possible that you could apply the period and
flags while keeping the calculated bootup duty cycle percentage
(presuming that the PWM was actually enabled at probe time and there
was a bootup duty cycle at all).  That would basically say that
whenever you set the period of a PWM then the duty cycle of the PWM
should remain the same percentage.  That actually seems quite sane
IMHO.  It seems much saner than trying to keep the duty cycle "ns"
when the period changes or resetting the PWM to some default when the
period changes.

That really depends on the use-case. If you're interested in the output
power of the PWM then, yes, this is sane. But it might not be the right
answer in other cases.

Ah, I see.  You're envisioning a device where active time in "ns" is
more important than the percentage of active time.  Perhaps an LED
where it's more important to have it on for no more than .1 seconds
(so we don't drive it too long and burn it out?).  If we slowed down
the duty cycle and adjusted the period to match, we could get into a
bad state.

quoted

B) Alternatively, I'd also say that setting a period without a duty
cycle doesn't make a lot of sense.  ...so you could just apply the
period at the same time that you apply the duty cycle the first time.
Presumably you'd want to "lie" to the callers of the PWM subsystem and
tell them that you already changed the period even though the change
won't really take effect until they actually set the duty cycle.  If
anyone cared to find out the true hardware period we could add a new
pwm_get_hw_period().  ...or since the only reason you'd want to know
the hardware period would be if you're trying to read the current duty
cycle percentage, you could instead add "pwm_get_hw_state()" and have
that return both the hardware period ns and duty cycle ns (which is
the most accurate way to return the "percentage" without using fix or
floating point math).

But then you get into a situation where behaviour is dependent on the
PWM driver, whereas this is really very specific to one specific use-
case.

This is because only some drivers would be able to read the hardware
state?  I'm not sure how we can get away from that.  In all proposals
we've talked about (including what you propose below, right?) the PWM
regulator will need a PWM driver that can read hardware state.  Only
PWM drivers that have been upgraded to support reading hardware state
can use the PWM regulator (or at least only those drivers will be able
to use the PWM regulator glitch-free).

When we add a new feature then it's expected that only updated drivers
will support that feature.

We need to make sure that we don't regress / negatively change the
behavior for anyone running non-updated drivers.  ...and we should
strive to require as few changes to drivers as possible.  ...but if
the best we can do requires changes to the PWM driver API then we will
certainly have differences depending on the PWM driver.

In the end the PWM API is as low-level as it is because it needs to be
flexible enough to cope with other use-cases. In the general case the
simple truth is that it doesn't make sense to set a period without the
duty cycle and vice versa. That's why pwm_config() takes both as input
parameters. The atomic API is going to take that one step further in
that you need to specify the complete state of the PWM when applying.

I guess this is still showing the strangeness of the device tree
specifying a period without a duty cycle.  You just said it doesn't
make sense, but that's exactly what the device tree has.

I'd imagine that the only reason that the device tree specifies just
the period is that it's intended to be there only for clients that
don't care about the specific duty cycle in terms of seconds but
_only_ care about the duty cycle in terms of percentage.  Anyone who
cared about the duty cycle in terms of seconds would presumably also
care about specifying the period (in terms of seconds) in the same
place they specify the duty cycle.

quoted

I think this is like my suggestion B), right?  AKA the PWM regulator
would be the sole caller of pwm_get_hw_state() and it would use this
to figure out the existing duty cycle percentage.  Then it would
translate that into "ns" and would set the duty cycle.  Upon the first
set of the duty cycle both the period and duty cycle would be applied
at the same time.

Yes and no. I think the PWM regulator would need to get the current
hardware state and derive the output power from duty cycle and period.
It would then need to rescale to whatever new period it wants to use
to ensure the same output power is used.

Right.  We all agree that somehow this needs to happen, it's just a
question of the implementation.

quoted

quoted

That doesn't really get us closer, though. There is still the issue of
the user having to deal with two states: the current hardware state and
the software state as configured in DT or board files.

I think the only users that need to deal with this are one that need a
seamless transition from bootup settings.  Adding a new API call to
support a new feature like this doesn't seem insane, and anyone who
doesn't want this new feature can just never call the new API.

The only thing that would "change" from the point of view of old
drivers is that the PWM period wouldn't change at bootup until the
duty cycle was set.  IMHO this is probably a bug fix.  AKA, for a PWM
backlight, imagine:

1. Firmware sets period to 20000 ns, duty cycle to 8000 ns (40%)
2. Linux boots up and sets period to 10000 ns.  Brightness of
backlight instantly goes to 80%.
3. Eventually something decides to set the backlight duty cycle and it
goes to the proper rate.

Skipping #2 seems like the right move.  ...or did I misunderstand how
something works?

I'm not aware of any code in the PWM subsystem that would do this
automatically. If you don't call any of the pwm_*() functions the
hardware state should not be modified. The responsibility is with
the user drivers.

Ah, OK.  I must have gotten confused.

Oh, I see.  So pwm_get_period() is actually "lying" about the hardware
today!  So what you're saying is that at boot time we grab the period
out of the device tree but we _don't_ apply it to the hardware, right?
 I was thinking it would get applied right away...

That means that if you call pwm_get_period() right away at boot time
you're not getting the current period of the hardware but the period
that was specified in the device tree.

So all we need is a new API call that lets you read the hardware
values and make sure that the PWM regulator calls that before anyone
calls pwm_config().  That's roughly B) above.

That is, it is up to the regulator or backlight driver to apply any new
configuration to a PWM channel on boot, or leave it as is. For
backlight it's probably fine to simply apply some default, since having
the brightness change on boot isn't going to be terribly irritating.

With the atomic API it would be possible to avoid even this and have the
backlight driver simply read out the current hardware state and apply an
equivalent brightness even if the period changed.

For the regulator case you'd need to read out the current state and then
recompute the values that will yield the same output power given data
specified in DT. I think that much is already implemented in Boris'
series, and it's really only the details that are being debated.

The problematic issue is still that we might have a disparity between
the current hardware state and the state initially specified by DT. In
the general case the DT will specify the period and polarity of the PWM
signal and leave it up to the user driver to determine what a correct
duty cycle would be. With the atomic API we'll essentially have two
states: the current (hardware) state and the "initial" state, which is
what an OS will see as the state to apply. The problem now is that once
you have applied the initial state with a duty cycle you've determined,
there is no longer a need to keep it around. But there's also no way to
know when this is the case. So the controversial part about all this is
when to start using the current state rather that the initial state.

The most straightforward way to solve this would be to apply the initial
configuration on driver probe. That is, when the pwm-regulator driver
gets probed it would retrieve the current and initial states, then
adjust the current state such that it matches the initial state but with
a duty cycle that yields the same output power as the current state, and
finally apply the new state. After that, every regulator_set_voltage()
call could simply operate on the current state and adjust the duty cycle
exclusively.

Does that sound reasonable?

Sure.  ...but you agree that somehow you need a new API call for this,
right?  Somehow the PWM regulator needs to be able to say that it
wants the hardware state, not the initial state as specified in the
device tree.


-Doug

On Wed, Feb 03, 2016 at 11:04:20AM -0800, Doug Anderson wrote:

Sure.  ...but you agree that somehow you need a new API call for this,
right?  Somehow the PWM regulator needs to be able to say that it
wants the hardware state, not the initial state as specified in the
device tree.

Wouldn't the most direct way to do that be to just not specify anything
in the DT?  If there *is* something in DT but we ignore it that's a bit
weird.

Hi Mark, Thierry,

On Thu, 4 Feb 2016 11:02:03 +0000
Mark Brown [off-list ref] wrote:

On Wed, Feb 03, 2016 at 11:04:20AM -0800, Doug Anderson wrote:

quoted

Sure.  ...but you agree that somehow you need a new API call for this,
right?  Somehow the PWM regulator needs to be able to say that it
wants the hardware state, not the initial state as specified in the
device tree.

Wouldn't the most direct way to do that be to just not specify anything
in the DT?  If there *is* something in DT but we ignore it that's a bit
weird.

Just adding some inputs on this specific aspect. The reason we have to
specify a period (and, to a lesser extent, the polarity) in the DT or
PWM lookup table is because what most PWM users want is to specify a
dutycycle relatively to a predefined period value.
If we decide to remove those information from the DT, then you'll need
a way to define it somewhere else, and then the is question is 'where?'.

Users that really want to control their period (this could the case for
the clk-pwm driver) could completely ignore DT/lookup-table information
and set the period and absolute dutycycle directly.

Now, from what I seen, what most PWM users want to do is:

	pwm_set_rel_duty_scale(pwm, rel_value, scale);
or 
	rel_duty = pwm_get_rel_duty_cycle(pwm, scale);

where scale depends on the precision you need for your use case (most
of the time it's expressed in percent).

So, how about providing this kind of API (this is what I proposed in
one of my previous email)?

This would not only solve our problem (say you have a period at
boot-time that differs from the one you'll set when first applying a
new relative duty cycle, then the resulting relative value would still
be correct), but it would also remove a lot of boiler plate code from
PWM users code (if you take a look at pwm-regulator, pwm-leds, pwm-fan
and probably others, you'll see that they are all doing this conversion
manually).

Now, the last blocking point is, what if the PWM driver does not
implement HW-readout. In this case, the pwm-regulator will probably
expose a 0V output (IIRC, dutycycle is set to 0 by default) when it's
actually providing something else. But is this really important? I
mean, if the user really wants to have a reliable information, then he
will implement initial-state retrieval in its PWM controller driver.
Alternatively, we could put a flag specifying whether the PWM chip
supports initial state retrieval.

11 more lines

Thierry,

On Wed, Feb 3, 2016 at 11:04 AM, Doug Anderson [off-list ref] wrote:

Thierry

On Wed, Feb 3, 2016 at 6:53 AM, Thierry Reding [off-list ref] wrote:

quoted

quoted

A) The software state here is the period and flags (AKA "inverted),
right?  It does seem possible that you could apply the period and
flags while keeping the calculated bootup duty cycle percentage
(presuming that the PWM was actually enabled at probe time and there
was a bootup duty cycle at all).  That would basically say that
whenever you set the period of a PWM then the duty cycle of the PWM
should remain the same percentage.  That actually seems quite sane
IMHO.  It seems much saner than trying to keep the duty cycle "ns"
when the period changes or resetting the PWM to some default when the
period changes.

That really depends on the use-case. If you're interested in the output
power of the PWM then, yes, this is sane. But it might not be the right
answer in other cases.

Ah, I see.  You're envisioning a device where active time in "ns" is
more important than the percentage of active time.  Perhaps an LED
where it's more important to have it on for no more than .1 seconds
(so we don't drive it too long and burn it out?).  If we slowed down
the duty cycle and adjusted the period to match, we could get into a
bad state.

quoted

quoted

B) Alternatively, I'd also say that setting a period without a duty
cycle doesn't make a lot of sense.  ...so you could just apply the
period at the same time that you apply the duty cycle the first time.
Presumably you'd want to "lie" to the callers of the PWM subsystem and
tell them that you already changed the period even though the change
won't really take effect until they actually set the duty cycle.  If
anyone cared to find out the true hardware period we could add a new
pwm_get_hw_period().  ...or since the only reason you'd want to know
the hardware period would be if you're trying to read the current duty
cycle percentage, you could instead add "pwm_get_hw_state()" and have
that return both the hardware period ns and duty cycle ns (which is
the most accurate way to return the "percentage" without using fix or
floating point math).

143 more quoted lines

I'm hoping we can get your latest thoughts on this topic so that Boris
can make forward progress.  It would be nice to get to some resolution
somehow.  The unfortunate part about having only one back-and-forth
exchange every few weeks is that I'm worried that we'll forget what we
already talked about and have the same discussions over and over
again.  :(

-Doug

On Wed, Feb 03, 2016 at 11:04:20AM -0800, Doug Anderson wrote:

Thierry

On Wed, Feb 3, 2016 at 6:53 AM, Thierry Reding [off-list ref] wrote:

quoted

quoted

A) The software state here is the period and flags (AKA "inverted),
right?  It does seem possible that you could apply the period and
flags while keeping the calculated bootup duty cycle percentage
(presuming that the PWM was actually enabled at probe time and there
was a bootup duty cycle at all).  That would basically say that
whenever you set the period of a PWM then the duty cycle of the PWM
should remain the same percentage.  That actually seems quite sane
IMHO.  It seems much saner than trying to keep the duty cycle "ns"
when the period changes or resetting the PWM to some default when the
period changes.

That really depends on the use-case. If you're interested in the output
power of the PWM then, yes, this is sane. But it might not be the right
answer in other cases.

Ah, I see.  You're envisioning a device where active time in "ns" is
more important than the percentage of active time.  Perhaps an LED
where it's more important to have it on for no more than .1 seconds
(so we don't drive it too long and burn it out?).  If we slowed down
the duty cycle and adjusted the period to match, we could get into a
bad state.

Yes. Granted, the vast majority of users is not in this category, but
it's something that has been brought to my attention in the past and it
works fine with the current API.

quoted

quoted

B) Alternatively, I'd also say that setting a period without a duty
cycle doesn't make a lot of sense.  ...so you could just apply the
period at the same time that you apply the duty cycle the first time.
Presumably you'd want to "lie" to the callers of the PWM subsystem and
tell them that you already changed the period even though the change
won't really take effect until they actually set the duty cycle.  If
anyone cared to find out the true hardware period we could add a new
pwm_get_hw_period().  ...or since the only reason you'd want to know
the hardware period would be if you're trying to read the current duty
cycle percentage, you could instead add "pwm_get_hw_state()" and have
that return both the hardware period ns and duty cycle ns (which is
the most accurate way to return the "percentage" without using fix or
floating point math).

But then you get into a situation where behaviour is dependent on the
PWM driver, whereas this is really very specific to one specific use-
case.

This is because only some drivers would be able to read the hardware
state?  I'm not sure how we can get away from that.  In all proposals
we've talked about (including what you propose below, right?) the PWM
regulator will need a PWM driver that can read hardware state.  Only
PWM drivers that have been upgraded to support reading hardware state
can use the PWM regulator (or at least only those drivers will be able
to use the PWM regulator glitch-free).

Yes, the key here is glitch-free. There's no reason whatsoever that the
rugaltor-pwm driver should be limited to usage with a hardware readout-
capable PWM driver. If you don't care about glitches, likely because no
critical components depend on the regulator, you can simply force what
state you choose on boot.

As a matter of fact, I think that's how regulators work already. If the
current output voltage doesn't match the specified constraints, then a
valid value will be forced by the regulator core. If the voltage lies
within the constraints the core won't touch the regulator. Is this not
going to "just work" with the PWM regulator?

The problem is somewhat simplified if that's the case. An implementation
could then fail the regulator_get_voltage() if hardware readout is not
supported and return the current voltage when readout is possible.

When we add a new feature then it's expected that only updated drivers
will support that feature.

We need to make sure that we don't regress / negatively change the
behavior for anyone running non-updated drivers.  ...and we should
strive to require as few changes to drivers as possible.  ...but if
the best we can do requires changes to the PWM driver API then we will
certainly have differences depending on the PWM driver.

How so? Drivers should behave consistently, irrespective of the API. Of
course if you need to change behaviour of the user driver depending on
the availability of a certain feature, that's perfectly fine.

Furthermore it's out of the question that changes to the API will be
required. That's precisely the reason why the atomic PWM proposal came
about. It's an attempt to solve the shortcomings of the current API for
cases such as Rockchip.

quoted

In the end the PWM API is as low-level as it is because it needs to be
flexible enough to cope with other use-cases. In the general case the
simple truth is that it doesn't make sense to set a period without the
duty cycle and vice versa. That's why pwm_config() takes both as input
parameters. The atomic API is going to take that one step further in
that you need to specify the complete state of the PWM when applying.

I guess this is still showing the strangeness of the device tree
specifying a period without a duty cycle.  You just said it doesn't
make sense, but that's exactly what the device tree has.

The device tree specifies the period because there is no way for the
driver (such as pwm-backlight) to "guess" the right one. Furthermore
there is a certain degree of board-specificity to that parameter and
therefore any heuristic trying to come up with a sensible value will
inevitably fail eventually.

I'd imagine that the only reason that the device tree specifies just
the period is that it's intended to be there only for clients that
don't care about the specific duty cycle in terms of seconds but
_only_ care about the duty cycle in terms of percentage.  Anyone who
cared about the duty cycle in terms of seconds would presumably also
care about specifying the period (in terms of seconds) in the same
place they specify the duty cycle.

Yes, exactly.

quoted

quoted

quoted

That doesn't really get us closer, though. There is still the issue of
the user having to deal with two states: the current hardware state and
the software state as configured in DT or board files.

I think the only users that need to deal with this are one that need a
seamless transition from bootup settings.  Adding a new API call to
support a new feature like this doesn't seem insane, and anyone who
doesn't want this new feature can just never call the new API.

The only thing that would "change" from the point of view of old
drivers is that the PWM period wouldn't change at bootup until the
duty cycle was set.  IMHO this is probably a bug fix.  AKA, for a PWM
backlight, imagine:

1. Firmware sets period to 20000 ns, duty cycle to 8000 ns (40%)
2. Linux boots up and sets period to 10000 ns.  Brightness of
backlight instantly goes to 80%.
3. Eventually something decides to set the backlight duty cycle and it
goes to the proper rate.

Skipping #2 seems like the right move.  ...or did I misunderstand how
something works?

I'm not aware of any code in the PWM subsystem that would do this
automatically. If you don't call any of the pwm_*() functions the
hardware state should not be modified. The responsibility is with
the user drivers.

Ah, OK.  I must have gotten confused.

Oh, I see.  So pwm_get_period() is actually "lying" about the hardware
today!  So what you're saying is that at boot time we grab the period
out of the device tree but we _don't_ apply it to the hardware, right?
 I was thinking it would get applied right away...

That's correct. The value retrieved by pwm_get_period() is the one
specified in DT (or a PWM lookup table). It should match the hardware
value after the first call to pwm_config(), though.

That means that if you call pwm_get_period() right away at boot time
you're not getting the current period of the hardware but the period
that was specified in the device tree.

Yes.

So all we need is a new API call that lets you read the hardware
values and make sure that the PWM regulator calls that before anyone
calls pwm_config().  That's roughly B) above.

Yes. I'm thinking that we should have a pwm_get_state() which retrieves
the current state of the PWM. For drivers that support hardware readout
this state should match the hardware state. For other drivers it should
reflect whatever was specified in DT; essentially what pwm_get_period()
and friends return today.

That way if you want to get the current voltage in the regulator-pwm
driver you'd simply do a pwm_get_state() and compute the voltage from
the period and duty cycle. If the PWM driver that you happen to use
doesn't support hardware readout, you'll get an initial output voltage
of 0, which is as good as any, really.

quoted

That is, it is up to the regulator or backlight driver to apply any new
configuration to a PWM channel on boot, or leave it as is. For
backlight it's probably fine to simply apply some default, since having
the brightness change on boot isn't going to be terribly irritating.

With the atomic API it would be possible to avoid even this and have the
backlight driver simply read out the current hardware state and apply an
equivalent brightness even if the period changed.

For the regulator case you'd need to read out the current state and then
recompute the values that will yield the same output power given data
specified in DT. I think that much is already implemented in Boris'
series, and it's really only the details that are being debated.

The problematic issue is still that we might have a disparity between
the current hardware state and the state initially specified by DT. In
the general case the DT will specify the period and polarity of the PWM
signal and leave it up to the user driver to determine what a correct
duty cycle would be. With the atomic API we'll essentially have two
states: the current (hardware) state and the "initial" state, which is
what an OS will see as the state to apply. The problem now is that once
you have applied the initial state with a duty cycle you've determined,
there is no longer a need to keep it around. But there's also no way to
know when this is the case. So the controversial part about all this is
when to start using the current state rather that the initial state.

The most straightforward way to solve this would be to apply the initial
configuration on driver probe. That is, when the pwm-regulator driver
gets probed it would retrieve the current and initial states, then
adjust the current state such that it matches the initial state but with
a duty cycle that yields the same output power as the current state, and
finally apply the new state. After that, every regulator_set_voltage()
call could simply operate on the current state and adjust the duty cycle
exclusively.

Does that sound reasonable?

Sure.  ...but you agree that somehow you need a new API call for this,
right?  Somehow the PWM regulator needs to be able to say that it
wants the hardware state, not the initial state as specified in the

1 more quoted lines

The atomic PWM API is this new API. I'm not arguing that we don't need
new API. What's being discussed is what the API needs to look like to
support this new use-case and at the same time be maximally compatible
with existing users.

I think perhaps one question that needs answering to do that is whether
or not the regulator-pwm driver can guess the correct period. The issue
that was initially being discussed is what to do with hardware state vs
initial state (i.e. what's defined in DT). Mark commented in another
subthread that DT should simply leave out data that doesn't make sense
to be specified.

However I don't think there's any particularly reasonable period for the
regulator-pwm use-case, so specifying the period within DT would still
be necessary. What works for one board may not be the correct (or
optimal) value for another. Similarly one board may need to invert the
PWM signal to generate the proper voltage, or require other parameters
that we haven't even defined yet.

There's also the issue of a voltage table that you need to define in the
DT for the regulator-pwm device. Does that consist of only duty-cycle
values, or does it have corresponding period values as well. I'd guess
that it really only needs the duty cycle given any period. So something
like this is what I'd expect:

	regulator {
		compatible = "regulator-pwm";

		pwms = <&pwm0 5000>;

		voltages = <0 0>, <1000000 1000>, ..., <5000000 5000>;
	};

I think the pwm-regulator binding defines the duty cycle in percent. I
guess that would work equally well.

And now we've come full circle because, again, we need to differentiate
between the current hardware state and the initial state. Or, as was
also discussed previously, alternatively, ignore the period specified in

11 more lines

Thierry,

On Mon, Feb 22, 2016 at 9:59 AM, Thierry Reding
[off-list ref] wrote:

quoted

This is because only some drivers would be able to read the hardware
state?  I'm not sure how we can get away from that.  In all proposals
we've talked about (including what you propose below, right?) the PWM
regulator will need a PWM driver that can read hardware state.  Only
PWM drivers that have been upgraded to support reading hardware state
can use the PWM regulator (or at least only those drivers will be able
to use the PWM regulator glitch-free).

Yes, the key here is glitch-free. There's no reason whatsoever that the
rugaltor-pwm driver should be limited to usage with a hardware readout-
capable PWM driver. If you don't care about glitches, likely because no
critical components depend on the regulator, you can simply force what
state you choose on boot.

As a matter of fact, I think that's how regulators work already. If the
current output voltage doesn't match the specified constraints, then a
valid value will be forced by the regulator core. If the voltage lies
within the constraints the core won't touch the regulator. Is this not
going to "just work" with the PWM regulator?

The problem is somewhat simplified if that's the case. An implementation
could then fail the regulator_get_voltage() if hardware readout is not
supported and return the current voltage when readout is possible.

Based on looking at the current code, I believe it just returns 0V
until you call regulator_set_voltage() today.  I don't think that was
always the case.  Back before it was made continuous I think it
returned the voltage that matched with 0% duty cycle.

I haven't dug into what the regulator framework does in the current
system nor what happens if regulator_get_voltage() returns an error.
Perhaps Boris can dig / comment?

quoted

When we add a new feature then it's expected that only updated drivers
will support that feature.

We need to make sure that we don't regress / negatively change the
behavior for anyone running non-updated drivers.  ...and we should
strive to require as few changes to drivers as possible.  ...but if
the best we can do requires changes to the PWM driver API then we will
certainly have differences depending on the PWM driver.

How so? Drivers should behave consistently, irrespective of the API. Of
course if you need to change behaviour of the user driver depending on
the availability of a certain feature, that's perfectly fine.

Furthermore it's out of the question that changes to the API will be
required. That's precisely the reason why the atomic PWM proposal came
about. It's an attempt to solve the shortcomings of the current API for
cases such as Rockchip.

I _think_ we're on the same page here.  If there are shortcomings with
the current API that make it impossible to implement a feature, we've
got to change and/or add to the existing API.  ...but we don't want to
break existing users / drivers.

Note that historically I remember that Linus Torvalds has stated that
there is no stable API within the Linux kernel and that forcing the
in-kernel API to never change was bad for software development.  I
tracked down my memory and found
<http://lwn.net/1999/0211/a/lt-binary.html>.  Linus is rabid about not
breaking userspace, but in general there's no strong requirement to
never change the driver API inside the kernel.  That being said,
changing the driver API causes a lot of churn, so presumably changing
it in a backward compatible way (like adding to the API instead of
changing it) will make things happier.

quoted

That means that if you call pwm_get_period() right away at boot time
you're not getting the current period of the hardware but the period
that was specified in the device tree.

Yes.

quoted

So all we need is a new API call that lets you read the hardware
values and make sure that the PWM regulator calls that before anyone
calls pwm_config().  That's roughly B) above.

Yes. I'm thinking that we should have a pwm_get_state() which retrieves
the current state of the PWM. For drivers that support hardware readout
this state should match the hardware state. For other drivers it should
reflect whatever was specified in DT; essentially what pwm_get_period()
and friends return today.

Excellent, so pwm_get_period() gets the period as specified in the
device tree (or other board config) and pwm_get_state() returns the
hardware state.  SGTM.

That way if you want to get the current voltage in the regulator-pwm
driver you'd simply do a pwm_get_state() and compute the voltage from
the period and duty cycle. If the PWM driver that you happen to use
doesn't support hardware readout, you'll get an initial output voltage
of 0, which is as good as any, really.

Sounds fine to me.  PWM regulator is in charge of calling
pwm_get_state(), which can return 0 (or an error?) if driver (or
underlying hardware) doesn't support hardware readout.  PWM regulator
is in charge of using the resulting period / duty cycle to calculate a
percentage.

quoted

Sure.  ...but you agree that somehow you need a new API call for this,
right?  Somehow the PWM regulator needs to be able to say that it
wants the hardware state, not the initial state as specified in the
device tree.

The atomic PWM API is this new API. I'm not arguing that we don't need
new API. What's being discussed is what the API needs to look like to
support this new use-case and at the same time be maximally compatible
with existing users.

I think perhaps one question that needs answering to do that is whether
or not the regulator-pwm driver can guess the correct period. The issue
that was initially being discussed is what to do with hardware state vs
initial state (i.e. what's defined in DT). Mark commented in another
subthread that DT should simply leave out data that doesn't make sense
to be specified.

However I don't think there's any particularly reasonable period for the
regulator-pwm use-case, so specifying the period within DT would still
be necessary. What works for one board may not be the correct (or
optimal) value for another. Similarly one board may need to invert the
PWM signal to generate the proper voltage, or require other parameters
that we haven't even defined yet.

There's also the issue of a voltage table that you need to define in the
DT for the regulator-pwm device. Does that consist of only duty-cycle
values, or does it have corresponding period values as well. I'd guess
that it really only needs the duty cycle given any period. So something
like this is what I'd expect:

        regulator {
                compatible = "regulator-pwm";

                pwms = <&pwm0 5000>;

                voltages = <0 0>, <1000000 1000>, ..., <5000000 5000>;
        };

I think the pwm-regulator binding defines the duty cycle in percent. I
guess that would work equally well.

13 more quoted lines

To answer:

* No, PWM regulator can't guess the correct period.

* PWM regulator API is already fine as is.  Period specified in PWM
specifier and table is specified in percentages.

* Firmware may have voltage setup correctly but may not have the
"ideal" period.  Often firmware configures things in a way that works
but is sub-optimal.  Basically the kernel should be able to tell what
voltage the firmware set things up at (so we know not to go lower on
accident), but we shouldn't assume that the firmware values are
perfect.  Said another way: obviously the firmware made values that
were good enough to boot us to where we are (or we wouldn't be even
executing code), but we might want to configure things to reduce noise
on the lines (make HDMI work better?) or optimize power consumption.

--

I _think_ the end result of all this is just:

1. Introduce pwm_get_state() that gets hardware state.  Up for debate
if this returns 0 or ERROR if a driver doesn't implement this.

2. PWM regulator calls pwm_get_state at probe time to get hardware
state, calculates a percentage (and voltage) with this.

3. PWM regulator does nothing else until it is asked to set the
voltage, but uses the voltage calculated from #2 to satisfy any "get
voltage" calls.

4. When asked to set the voltage, PWM regulator uses pwm_get_period()
and calculates a duty cycle based on that, just like it does today.
This uses pwm_config() which includes a duty cycle and period and is
thus "atomic".


Said another way: the only action item from this point of view is just
that we introduce a new pwm_get_state().

14 more lines

On Mon, Feb 22, 2016 at 11:15:09AM -0800, Doug Anderson wrote:

Note that historically I remember that Linus Torvalds has stated that
there is no stable API within the Linux kernel and that forcing the
in-kernel API to never change was bad for software development.  I
tracked down my memory and found
<http://lwn.net/1999/0211/a/lt-binary.html>.  Linus is rabid about not
breaking userspace, but in general there's no strong requirement to
never change the driver API inside the kernel.  That being said,
changing the driver API causes a lot of churn, so presumably changing
it in a backward compatible way (like adding to the API instead of
changing it) will make things happier.

You do need to fix the users though, change is fine but you can't cause
people's systems to break.

Mark,

On Mon, Feb 22, 2016 at 1:24 PM, Mark Brown [off-list ref] wrote:

On Mon, Feb 22, 2016 at 11:15:09AM -0800, Doug Anderson wrote:

quoted

Note that historically I remember that Linus Torvalds has stated that
there is no stable API within the Linux kernel and that forcing the
in-kernel API to never change was bad for software development.  I
tracked down my memory and found
<http://lwn.net/1999/0211/a/lt-binary.html>.  Linus is rabid about not
breaking userspace, but in general there's no strong requirement to
never change the driver API inside the kernel.  That being said,
changing the driver API causes a lot of churn, so presumably changing
it in a backward compatible way (like adding to the API instead of
changing it) will make things happier.

You do need to fix the users though, change is fine but you can't cause
people's systems to break.

Yes, of course!  :)  Thanks for clarifying.

-Doug

On Mon, Feb 22, 2016 at 11:15:09AM -0800, Doug Anderson wrote:

Thierry,

On Mon, Feb 22, 2016 at 9:59 AM, Thierry Reding [off-list ref] wrote:

[...]

quoted

quoted

When we add a new feature then it's expected that only updated drivers
will support that feature.

We need to make sure that we don't regress / negatively change the
behavior for anyone running non-updated drivers.  ...and we should
strive to require as few changes to drivers as possible.  ...but if
the best we can do requires changes to the PWM driver API then we will
certainly have differences depending on the PWM driver.

How so? Drivers should behave consistently, irrespective of the API. Of
course if you need to change behaviour of the user driver depending on
the availability of a certain feature, that's perfectly fine.

Furthermore it's out of the question that changes to the API will be
required. That's precisely the reason why the atomic PWM proposal came
about. It's an attempt to solve the shortcomings of the current API for
cases such as Rockchip.

I _think_ we're on the same page here.  If there are shortcomings with
the current API that make it impossible to implement a feature, we've
got to change and/or add to the existing API.  ...but we don't want to
break existing users / drivers.

Note that historically I remember that Linus Torvalds has stated that
there is no stable API within the Linux kernel and that forcing the
in-kernel API to never change was bad for software development.  I
tracked down my memory and found
<http://lwn.net/1999/0211/a/lt-binary.html>.  Linus is rabid about not
breaking userspace, but in general there's no strong requirement to
never change the driver API inside the kernel.  That being said,
changing the driver API causes a lot of churn, so presumably changing
it in a backward compatible way (like adding to the API instead of
changing it) will make things happier.

I didn't say anything about stable API. All I said is that new API
should be well-thought-out. Those are two very different things.

quoted

quoted

So all we need is a new API call that lets you read the hardware
values and make sure that the PWM regulator calls that before anyone
calls pwm_config().  That's roughly B) above.

Yes. I'm thinking that we should have a pwm_get_state() which retrieves
the current state of the PWM. For drivers that support hardware readout
this state should match the hardware state. For other drivers it should
reflect whatever was specified in DT; essentially what pwm_get_period()
and friends return today.

Excellent, so pwm_get_period() gets the period as specified in the
device tree (or other board config) and pwm_get_state() returns the
hardware state.  SGTM.

That's not quite what I was thinking. If hardware readout is supported
then whatever we report back should be the current hardware state unless
we're explicitly asked for something else. If we start mixing the state
and legacy APIs this way, we'll get into a situation where drivers that
support hardware readout behave differently than drivers that don't.

For example: A PWM device that's controlled by a driver that supports
hardware readout has a current period of 50000 ns and the firmware set
the period to 25000 ns. pwm_get_period() for this PWM device will return
50000 ns. If you reconfigure the PWM to generate a PWM signal with a
period of 30000 ns, pwm_get_period() would still return 50000 ns.

A driver that doesn't support hardware readout, on the contrary, would
return 50000 ns from pwm_get_period() on the first call, but after you
have reconfigured it using pwm_config() it will return the new period.

quoted

That way if you want to get the current voltage in the regulator-pwm
driver you'd simply do a pwm_get_state() and compute the voltage from
the period and duty cycle. If the PWM driver that you happen to use
doesn't support hardware readout, you'll get an initial output voltage
of 0, which is as good as any, really.

Sounds fine to me.  PWM regulator is in charge of calling
pwm_get_state(), which can return 0 (or an error?) if driver (or
underlying hardware) doesn't support hardware readout.  PWM regulator
is in charge of using the resulting period / duty cycle to calculate a
percentage.

I'm not sure if pwm_get_state() should ever return an error. For drivers
that support hardware readout, the resulting state should match what's
programmed to the hardware.

But for drivers without hardware readout support pwm_get_state() still
makes sense because after a pwm_apply_state() the internal logical state
would again match hardware.

The simplest way to get rid of this would be to change the core to apply
an initial configuration on probe. However that's probably going to
break your use-case again (it would set a 0 duty cycle because it isn't
configured in DT).

To allow your use-case to work we'd need to deal with two states: the
current hardware state and the "initial" state as defined by DT.
Unfortunately the PWM specifier in DT is not a full definition, it is
more like a partial initial configuration. The problem with that, and
I think that's what Mark was originally objecting to, is that it isn't
clear when to use the "initial" state and when to use the read hardware
state. After the first pwm_apply_state() you wouldn't ever have to use
the "initial" state again, because it's the same as the current state
(modulo the duty cycle).

Also for drivers such as clk-pwm the usefulness of the "initial" state
is reduced even more, because it doesn't even need the period specified
in DT. It uses only the flags (if at all).

Perhaps to avoid this confusion a new type of object, e.g. pwm_args,
could be introduced to hold configuration arguments given in the PWM
specifier (in DT) or the PWM lookup table (in board files).

It would then be the responsibility of the users to deal with that
information in a sensible way. In (almost?) all cases I would expect
that to be to program the PWM device in the user's ->probe(). In the
case of regulator-pwm I'd expect that ->probe() would do something
along these lines (error handling excluded):

	struct pwm_state state;
	struct pwm_args args;

29 more lines

I _think_ the end result of all this is just:

1. Introduce pwm_get_state() that gets hardware state.  Up for debate
if this returns 0 or ERROR if a driver doesn't implement this.

Like I said above, I don't think pwm_get_state() should ever fail. It
should simply return the current state of the PWM, which might coincide
with the hardware state (for drivers that support hardware readout) or
will be the logical state (for drivers that don't).

Note that in the above example the logical state of the PWM, in cases
where the driver doesn't support hardware readout, the duty cycle will
be assumed to be 0, so the regulator-pwm driver would at ->probe() time
disable the regulator, at which point hardware state and logical state
will coincide again.

2. PWM regulator calls pwm_get_state at probe time to get hardware
state, calculates a percentage (and voltage) with this.

I don't think that's enough. If we do this, we'll keep carrying around
the mismatch between hardware state and logical state indefinitely.

3. PWM regulator does nothing else until it is asked to set the
voltage, but uses the voltage calculated from #2 to satisfy any "get
voltage" calls.

This should work out of the box in the above. The initial state would
yield a voltage of 0 if hardware readout is not supported, whereas for
drivers that support hardware readout, the proper value can be derived
from duty cycle, period and the lookup table.

4. When asked to set the voltage, PWM regulator uses pwm_get_period()
and calculates a duty cycle based on that, just like it does today.
This uses pwm_config() which includes a duty cycle and period and is
thus "atomic".

pwm_config() isn't atomic. pwm_apply_state() would be. The difference is
that pwm_config() can't at the same time enable/disable the PWM or set
the polarity.

Thierry

On Thu, Feb 04, 2016 at 03:01:50PM +0100, Boris Brezillon wrote:

Hi Mark, Thierry,

On Thu, 4 Feb 2016 11:02:03 +0000
Mark Brown [off-list ref] wrote:

quoted

On Wed, Feb 03, 2016 at 11:04:20AM -0800, Doug Anderson wrote:

quoted

Sure.  ...but you agree that somehow you need a new API call for this,
right?  Somehow the PWM regulator needs to be able to say that it
wants the hardware state, not the initial state as specified in the
device tree.

Wouldn't the most direct way to do that be to just not specify anything
in the DT?  If there *is* something in DT but we ignore it that's a bit
weird.

Just adding some inputs on this specific aspect. The reason we have to
specify a period (and, to a lesser extent, the polarity) in the DT or
PWM lookup table is because what most PWM users want is to specify a
dutycycle relatively to a predefined period value.

That's not quite correct. The reason why we need the information in DT
is because it can't be derived from anything else. It is board-specific
data for which there's no heuristic that will work in all cases.

If we decide to remove those information from the DT, then you'll need
a way to define it somewhere else, and then the is question is 'where?'.

Users that really want to control their period (this could the case for
the clk-pwm driver) could completely ignore DT/lookup-table information
and set the period and absolute dutycycle directly.

Yes, I think clk-pwm is very special in this regard because the period
can be derived from the requested clock rate. It would be complicated to
implement DT parsing that ignores parts of the specifier in some cases
but not in others. Simply having the clk-pwm driver ignore whatever is
in the table (or perhaps bail out on periods other than 0 for example).

Now, from what I seen, what most PWM users want to do is:

	pwm_set_rel_duty_scale(pwm, rel_value, scale);
or 
	rel_duty = pwm_get_rel_duty_cycle(pwm, scale);

where scale depends on the precision you need for your use case (most
of the time it's expressed in percent).

So, how about providing this kind of API (this is what I proposed in
one of my previous email)?

This would not only solve our problem (say you have a period at
boot-time that differs from the one you'll set when first applying a
new relative duty cycle, then the resulting relative value would still
be correct), but it would also remove a lot of boiler plate code from
PWM users code (if you take a look at pwm-regulator, pwm-leds, pwm-fan
and probably others, you'll see that they are all doing this conversion
manually).

I don't think this gains us much. The above would work for pwm-regulator
and pwm-fan, in both cases it'd replace a single line with two lines and
fitting the expressions into function arguments is likely going to be
hideous. For leds-pwm this wouldn't work, because of the low-active case
that it supports.

Now, the last blocking point is, what if the PWM driver does not
implement HW-readout. In this case, the pwm-regulator will probably
expose a 0V output (IIRC, dutycycle is set to 0 by default) when it's
actually providing something else. But is this really important? I
mean, if the user really wants to have a reliable information, then he
will implement initial-state retrieval in its PWM controller driver.
Alternatively, we could put a flag specifying whether the PWM chip
supports initial state retrieval.

I reached the same conclusion in another subthread. If hardware readout
isn't supported, I think the most natural thing to do is simply use the
initial state (i.e. what's defined in DT or board files) instead. There
is an argument, I think, to be made for having users apply the initial
state at probe time to forcibly apply the logical state to hardware and
subsequently not care about the initial state anymore. For most cases
that might not even be necessary, though.

Thierry

Thierry,

On Tue, Feb 23, 2016 at 6:38 AM, Thierry Reding
[off-list ref] wrote:

quoted

quoted

Furthermore it's out of the question that changes to the API will be
required. That's precisely the reason why the atomic PWM proposal came
about. It's an attempt to solve the shortcomings of the current API for
cases such as Rockchip.

I _think_ we're on the same page here.  If there are shortcomings with
the current API that make it impossible to implement a feature, we've
got to change and/or add to the existing API.  ...but we don't want to
break existing users / drivers.

Note that historically I remember that Linus Torvalds has stated that
there is no stable API within the Linux kernel and that forcing the
in-kernel API to never change was bad for software development.  I
tracked down my memory and found
<http://lwn.net/1999/0211/a/lt-binary.html>.  Linus is rabid about not
breaking userspace, but in general there's no strong requirement to
never change the driver API inside the kernel.  That being said,
changing the driver API causes a lot of churn, so presumably changing
it in a backward compatible way (like adding to the API instead of
changing it) will make things happier.

I didn't say anything about stable API. All I said is that new API
should be well-thought-out. Those are two very different things.

I guess I just misunderstood "it's out of the question that changes to
the API will be required".  In any case, I think everyone's on the
same page here, so no need to debate further.  :)

quoted

quoted

quoted

So all we need is a new API call that lets you read the hardware
values and make sure that the PWM regulator calls that before anyone
calls pwm_config().  That's roughly B) above.

Yes. I'm thinking that we should have a pwm_get_state() which retrieves
the current state of the PWM. For drivers that support hardware readout
this state should match the hardware state. For other drivers it should
reflect whatever was specified in DT; essentially what pwm_get_period()
and friends return today.

Excellent, so pwm_get_period() gets the period as specified in the
device tree (or other board config) and pwm_get_state() returns the
hardware state.  SGTM.

That's not quite what I was thinking. If hardware readout is supported
then whatever we report back should be the current hardware state unless
we're explicitly asked for something else. If we start mixing the state
and legacy APIs this way, we'll get into a situation where drivers that
support hardware readout behave differently than drivers that don't.

For example: A PWM device that's controlled by a driver that supports
hardware readout has a current period of 50000 ns and the firmware set
the period to 25000 ns. pwm_get_period() for this PWM device will return
50000 ns. If you reconfigure the PWM to generate a PWM signal with a
period of 30000 ns, pwm_get_period() would still return 50000 ns.

A driver that doesn't support hardware readout, on the contrary, would
return 50000 ns from pwm_get_period() on the first call, but after you
have reconfigured it using pwm_config() it will return the new period.

Ah, right!  I forgot that the existing API will be updated if you've
reconfigured the period via pwm_config().  Ugh, you're right that's a
little ugly then.

So do we define it as:

pwm_get_state(): always get the hardware state w/ no caching (maybe
even pwm_get_raw_state() or pwm_get_hw_state())

pwm_get_period(): get the period of the PWM; if the PWM has not yet
been configured by software this gets the default period (possibly
specified by the device tree).


Is that OK?  That is well defined and doesn't change the existing
behavior of pwm_get_period().

quoted

quoted

That way if you want to get the current voltage in the regulator-pwm
driver you'd simply do a pwm_get_state() and compute the voltage from
the period and duty cycle. If the PWM driver that you happen to use
doesn't support hardware readout, you'll get an initial output voltage
of 0, which is as good as any, really.

Sounds fine to me.  PWM regulator is in charge of calling
pwm_get_state(), which can return 0 (or an error?) if driver (or
underlying hardware) doesn't support hardware readout.  PWM regulator
is in charge of using the resulting period / duty cycle to calculate a
percentage.

I'm not sure if pwm_get_state() should ever return an error. For drivers
that support hardware readout, the resulting state should match what's
programmed to the hardware.

But for drivers without hardware readout support pwm_get_state() still
makes sense because after a pwm_apply_state() the internal logical state
would again match hardware.

I guess it depends on how you define things.  With my above
definitions it seems clearest if pwm_get_state() returns an error if
hardware readout is not supported.  If we call it pwm_get_hw_state()
it's even clearer that it should return an error.

The simplest way to get rid of this would be to change the core to apply
an initial configuration on probe. However that's probably going to
break your use-case again (it would set a 0 duty cycle because it isn't
configured in DT).

Right, so we can't do that.

To allow your use-case to work we'd need to deal with two states: the
current hardware state and the "initial" state as defined by DT.
Unfortunately the PWM specifier in DT is not a full definition, it is
more like a partial initial configuration. The problem with that, and
I think that's what Mark was originally objecting to, is that it isn't
clear when to use the "initial" state and when to use the read hardware
state. After the first pwm_apply_state() you wouldn't ever have to use
the "initial" state again, because it's the same as the current state
(modulo the duty cycle).

Also for drivers such as clk-pwm the usefulness of the "initial" state
is reduced even more, because it doesn't even need the period specified
in DT. It uses only the flags (if at all).

Perhaps to avoid this confusion a new type of object, e.g. pwm_args,
could be introduced to hold configuration arguments given in the PWM
specifier (in DT) or the PWM lookup table (in board files).

It would then be the responsibility of the users to deal with that
information in a sensible way. In (almost?) all cases I would expect
that to be to program the PWM device in the user's ->probe(). In the
case of regulator-pwm I'd expect that ->probe() would do something
along these lines (error handling excluded):

        struct pwm_state state;
        struct pwm_args args;
        unsigned int ratio;

        pwm = pwm_get(...);

        pwm_get_state(pwm, &state);
        pwm_get_args(pwm, &args);

        ratio = (state.duty_cycle * 100) / state.period;

        state.duty_cycle = (ratio * args.period) / 100;
        state.period = args.period;
        state.flags = args.flags;

        pwm_apply_state(pwm, &state);

14 more quoted lines

That should work with one minor problem.  If HW readout isn't
supported then pwm_get_state() in probe will presumably return 0 for
the duty cycle.  That means it will change the voltage.  That's in
contrast to how I think things work today where the voltage isn't
changed until the first set_voltage() call.  At least the last time I
tested things get_voltage() would simply report an incorrect value
until the first set_voltage().  I think existing behavior (reporting
the wrong value) is better than new behavior (change the value at
probe).

I'm curious, though.  In your proposal, how does pwm_get_period()
behave?  To be backward compatible, I'd imagine that even in your
proposal we'd have the same definition as I had above:

pwm_get_period(): get the period of the PWM; if the PWM has not yet
been configured by software this gets the default period (possibly
specified by the device tree).


If you have a different definition of pwm_get_period() in your
proposal, please let me know!  If my definition matches your thoughts
then I think we can actually just not touch the "set_voltage" call.
It can always use pwm_get_period() and always use pwm_config() just
like today.

...and if set_voltage() remains untouched then we can solve my probe
problem by renaming pwm_get_state() to pwm_get_hw_state() and having
it return an error if HW readout is not supported.  Then we only call
pwm_get_args() / pwm_apply_state() when we support HW readout.


Thus, if HW readout:

* In probe, we read HW state (pwm_get_hw_state) and atomically adjust
(pwm_apply_state) based on arguments (pwm_get_args).

* In set_voltage we use pwm_get_period which will return the period we
applied in pwm_apply_state() and use pwm_config() to change the duty
cycle.

13 more lines

On Tue, Feb 23, 2016 at 09:35:48AM -0800, Doug Anderson wrote:

On Tue, Feb 23, 2016 at 6:38 AM, Thierry Reding [off-list ref] wrote:

[...]

quoted

That's not quite what I was thinking. If hardware readout is supported
then whatever we report back should be the current hardware state unless
we're explicitly asked for something else. If we start mixing the state
and legacy APIs this way, we'll get into a situation where drivers that
support hardware readout behave differently than drivers that don't.

For example: A PWM device that's controlled by a driver that supports
hardware readout has a current period of 50000 ns and the firmware set
the period to 25000 ns. pwm_get_period() for this PWM device will return
50000 ns. If you reconfigure the PWM to generate a PWM signal with a
period of 30000 ns, pwm_get_period() would still return 50000 ns.

A driver that doesn't support hardware readout, on the contrary, would
return 50000 ns from pwm_get_period() on the first call, but after you
have reconfigured it using pwm_config() it will return the new period.

Ah, right!  I forgot that the existing API will be updated if you've
reconfigured the period via pwm_config().  Ugh, you're right that's a
little ugly then.

So do we define it as:

pwm_get_state(): always get the hardware state w/ no caching (maybe
even pwm_get_raw_state() or pwm_get_hw_state())

Caching vs. no caching should be irrelevant here. Unless PWM hardware is
autonomous the current state will always match the hardware state after
the initial hardware readout.

pwm_get_period(): get the period of the PWM; if the PWM has not yet
been configured by software this gets the default period (possibly
specified by the device tree).

No. I think we'll need a different construct for the period defined by
DT or board files. pwm_get_period() is the legacy API to retrieve the
"current" period, even if it was lying a little before the atomic API.

Is that OK?  That is well defined and doesn't change the existing
behavior of pwm_get_period().

pwm_get_period() is legacy API and in order to transition to the atomic
API it should be implemented in terms of atomic API. So the goal is to
get everything internally converted to deal with states only, then wrap
the existing API around that concept. pwm_get_period() would become:

	unsigned int pwm_get_period(struct pwm_device *pwm)
	{
		struct pwm_state state;

		pwm_get_state(pwm, &state);

		return state.period;
	}

If we don't do that, we'll never be able to get rid of the legacy API.

quoted

quoted

quoted

That way if you want to get the current voltage in the regulator-pwm
driver you'd simply do a pwm_get_state() and compute the voltage from
the period and duty cycle. If the PWM driver that you happen to use
doesn't support hardware readout, you'll get an initial output voltage
of 0, which is as good as any, really.

Sounds fine to me.  PWM regulator is in charge of calling
pwm_get_state(), which can return 0 (or an error?) if driver (or
underlying hardware) doesn't support hardware readout.  PWM regulator
is in charge of using the resulting period / duty cycle to calculate a
percentage.

I'm not sure if pwm_get_state() should ever return an error. For drivers
that support hardware readout, the resulting state should match what's
programmed to the hardware.

But for drivers without hardware readout support pwm_get_state() still
makes sense because after a pwm_apply_state() the internal logical state
would again match hardware.

I guess it depends on how you define things.  With my above
definitions it seems clearest if pwm_get_state() returns an error if
hardware readout is not supported.  If we call it pwm_get_hw_state()
it's even clearer that it should return an error.

Again, if we do this, we'll have to keep the legacy API around forever
and keep special-casing atomic vs. legacy API in every user. The goal is
to converge on the atomic API as the standard API in users so that the
legacy API can be removed when all users have been converted.

quoted

To allow your use-case to work we'd need to deal with two states: the
current hardware state and the "initial" state as defined by DT.
Unfortunately the PWM specifier in DT is not a full definition, it is
more like a partial initial configuration. The problem with that, and
I think that's what Mark was originally objecting to, is that it isn't
clear when to use the "initial" state and when to use the read hardware
state. After the first pwm_apply_state() you wouldn't ever have to use
the "initial" state again, because it's the same as the current state
(modulo the duty cycle).

Also for drivers such as clk-pwm the usefulness of the "initial" state
is reduced even more, because it doesn't even need the period specified
in DT. It uses only the flags (if at all).

Perhaps to avoid this confusion a new type of object, e.g. pwm_args,
could be introduced to hold configuration arguments given in the PWM
specifier (in DT) or the PWM lookup table (in board files).

It would then be the responsibility of the users to deal with that
information in a sensible way. In (almost?) all cases I would expect
that to be to program the PWM device in the user's ->probe(). In the
case of regulator-pwm I'd expect that ->probe() would do something
along these lines (error handling excluded):

        struct pwm_state state;
        struct pwm_args args;
        unsigned int ratio;

        pwm = pwm_get(...);

        pwm_get_state(pwm, &state);
        pwm_get_args(pwm, &args);

        ratio = (state.duty_cycle * 100) / state.period;

        state.duty_cycle = (ratio * args.period) / 100;
        state.period = args.period;
        state.flags = args.flags;

        pwm_apply_state(pwm, &state);

24 more quoted lines

That's exactly the point. Reporting a wrong value isn't really a good
option. Changing the voltage on boot is the only way to make the logical
state match the hardware state on boot. Chances are that if you don't
have hardware readout support you probably don't care what state your
regulator will be in.

Then again, if we don't support hardware readout, setting up the logical
state with data from DT (or board files) and defaulting the duty cycle
to 0, we end up with exactly what we had before, even with the atomic
API, right? Maybe that's okay, too.

I'm curious, though.  In your proposal, how does pwm_get_period()
behave?  To be backward compatible, I'd imagine that even in your
proposal we'd have the same definition as I had above:

pwm_get_period(): get the period of the PWM; if the PWM has not yet
been configured by software this gets the default period (possibly
specified by the device tree).

It would simply return the logical period of the PWM. For drivers that
support hardware readout it would always match the hardware period, but
for drivers that don't it might be wrong until state is first applied.

If you have a different definition of pwm_get_period() in your
proposal, please let me know!  If my definition matches your thoughts
then I think we can actually just not touch the "set_voltage" call.
It can always use pwm_get_period() and always use pwm_config() just
like today.

...and if set_voltage() remains untouched then we can solve my probe
problem by renaming pwm_get_state() to pwm_get_hw_state() and having
it return an error if HW readout is not supported.  Then we only call
pwm_get_args() / pwm_apply_state() when we support HW readout.

The problem is that we make the API clumsy to use. If we don't sync the
"initial" state (as defined by DT or board files) to hardware at any
point, then we need to add the pwm_args construct and always stick to
it. I think it weird to have to use the pwm_args.period instead of the
current period.

So we're back to square one, really. That's exactly what Mark brought up
originally.

Thus, if HW readout:

* In probe, we read HW state (pwm_get_hw_state) and atomically adjust
(pwm_apply_state) based on arguments (pwm_get_args).

* In set_voltage we use pwm_get_period which will return the period we
applied in pwm_apply_state() and use pwm_config() to change the duty
cycle.


If no HW readout, no behavior change at all from today:

* In probe we don't do anything to change the PWM

* Upon the first set_voltage we use pwm_get_period() to get the period
as specified in DT and use pwm_config() to change the duty cycle.


That seems pretty sane to me.  What do you think?

This has the big disadvantage of having to special case hardware readout
vs. non-hardware readout providers. I think that makes the API really
difficult to use. It requires too many details to be aware of.

I guess this boils down to whether applying the "initial" state on probe
really is problematic.

Thierry

Thierry,

On Tue, Feb 23, 2016 at 10:14 AM, Thierry Reding
[off-list ref] wrote:

quoted

pwm_get_period(): get the period of the PWM; if the PWM has not yet
been configured by software this gets the default period (possibly
specified by the device tree).

No. I think we'll need a different construct for the period defined by
DT or board files. pwm_get_period() is the legacy API to retrieve the
"current" period, even if it was lying a little before the atomic API.

Ah, got it.  I think I missed that you considered pwm_get_period()
legacy and that you eventually wanted to get rid of it.  OK, then what
you say makes sense.

quoted

That should work with one minor problem.  If HW readout isn't
supported then pwm_get_state() in probe will presumably return 0 for
the duty cycle.  That means it will change the voltage.  That's in
contrast to how I think things work today where the voltage isn't
changed until the first set_voltage() call.  At least the last time I
tested things get_voltage() would simply report an incorrect value
until the first set_voltage().  I think existing behavior (reporting
the wrong value) is better than new behavior (change the value at
probe).

That's exactly the point. Reporting a wrong value isn't really a good
option. Changing the voltage on boot is the only way to make the logical
state match the hardware state on boot. Chances are that if you don't
have hardware readout support you probably don't care what state your
regulator will be in.

Then again, if we don't support hardware readout, setting up the logical
state with data from DT (or board files) and defaulting the duty cycle
to 0, we end up with exactly what we had before, even with the atomic
API, right? Maybe that's okay, too.

IMHO this is a change in behavior that will break existing users.
Anyone using a PWM regulator will suddenly find their voltage changing
at bootup.  Certainly today all users of the PWM regulator don't seem
to mind (apparently) the the voltage is reported incorrectly at bootup
but I bet they'd mind if the voltage suddenly started changing for
them at bootup.

It seems better to preserve existing behavior and print a warning that
the voltage will be reported incorrectly until HW Readout is
supported.

Of course, we're only talking about two real users in mainline here:
Rockchip boards and the "stih407-family".  If we just fix both of
those to support HW Readout before landing the change then I'm fine
with doing what you say.

quoted

...and if set_voltage() remains untouched then we can solve my probe
problem by renaming pwm_get_state() to pwm_get_hw_state() and having
it return an error if HW readout is not supported.  Then we only call
pwm_get_args() / pwm_apply_state() when we support HW readout.

The problem is that we make the API clumsy to use. If we don't sync the
"initial" state (as defined by DT or board files) to hardware at any
point, then we need to add the pwm_args construct and always stick to
it. I think it weird to have to use the pwm_args.period instead of the
current period.

So we're back to square one, really. That's exactly what Mark brought up
originally.

I had missed the part where you wanted to deprecate pwm_get_period().
Thus my points here aren't really valid.

In my mind the old API was perfectly fine (and actually quite clean /
simple to use) except in the special case of avoiding the PWM
regulator glitches.  With that mindset I think my previous email make
sense.  However, this is your subsystem to maintain and if you think
moving everyone to a new atomic API makes more sense then you're in
the best position to make that decision.  :)


-Doug

Thierry,

On Tue, Feb 23, 2016 at 10:42 AM, Doug Anderson [off-list ref] wrote:

Thierry,

On Tue, Feb 23, 2016 at 10:14 AM, Thierry Reding
[off-list ref] wrote:

quoted

quoted

pwm_get_period(): get the period of the PWM; if the PWM has not yet
been configured by software this gets the default period (possibly
specified by the device tree).

No. I think we'll need a different construct for the period defined by
DT or board files. pwm_get_period() is the legacy API to retrieve the
"current" period, even if it was lying a little before the atomic API.

Ah, got it.  I think I missed that you considered pwm_get_period()
legacy and that you eventually wanted to get rid of it.  OK, then what
you say makes sense.

quoted

quoted

That should work with one minor problem.  If HW readout isn't
supported then pwm_get_state() in probe will presumably return 0 for
the duty cycle.  That means it will change the voltage.  That's in
contrast to how I think things work today where the voltage isn't
changed until the first set_voltage() call.  At least the last time I
tested things get_voltage() would simply report an incorrect value
until the first set_voltage().  I think existing behavior (reporting
the wrong value) is better than new behavior (change the value at
probe).

That's exactly the point. Reporting a wrong value isn't really a good
option. Changing the voltage on boot is the only way to make the logical
state match the hardware state on boot. Chances are that if you don't
have hardware readout support you probably don't care what state your
regulator will be in.

Then again, if we don't support hardware readout, setting up the logical
state with data from DT (or board files) and defaulting the duty cycle
to 0, we end up with exactly what we had before, even with the atomic
API, right? Maybe that's okay, too.

IMHO this is a change in behavior that will break existing users.
Anyone using a PWM regulator will suddenly find their voltage changing

38 more quoted lines

So just to summarize:

* Add pwm_get_state(), pwm_apply_state(), pwm_get_args().
pwm_get_state() initially returns 0 for duty cycle if driver doesn't
support readout.

* Re-implement pwm_get_period() (and maybe other similar functions)
atop pwm_get_state() as you describe earlier in the thread.

* Document pwm_get_period() (and maybe other similar functions) as deprecated.

* Fix drivers for all current 2 users of PWM regulator to support
hardware readout.

* Update PWM regulator as you described earlier in the thread (Feb 23).

* If PWM regulator is ever used on a new board whose PWM doesn't
support hardware readout, the voltage will change at probe time.


Did I get all that right?  Thanks!

-Doug

Thierry,

On Thu, Feb 25, 2016 at 3:14 PM, Doug Anderson [off-list ref] wrote:

So just to summarize:

* Add pwm_get_state(), pwm_apply_state(), pwm_get_args().
pwm_get_state() initially returns 0 for duty cycle if driver doesn't
support readout.

* Re-implement pwm_get_period() (and maybe other similar functions)
atop pwm_get_state() as you describe earlier in the thread.

* Document pwm_get_period() (and maybe other similar functions) as deprecated.

* Fix drivers for all current 2 users of PWM regulator to support
hardware readout.

* Update PWM regulator as you described earlier in the thread (Feb 23).

* If PWM regulator is ever used on a new board whose PWM doesn't
support hardware readout, the voltage will change at probe time.


Did I get all that right?  Thanks!

Can you provide a "yes, you got that right" or a "no, you didn't
understand"?  That will unblock Boris, I think.

-Doug

On Mon, Mar 07, 2016 at 08:34:19AM -0800, Doug Anderson wrote:

Thierry,

On Thu, Feb 25, 2016 at 3:14 PM, Doug Anderson [off-list ref] wrote:

quoted

So just to summarize:

* Add pwm_get_state(), pwm_apply_state(), pwm_get_args().
pwm_get_state() initially returns 0 for duty cycle if driver doesn't
support readout.

* Re-implement pwm_get_period() (and maybe other similar functions)
atop pwm_get_state() as you describe earlier in the thread.

* Document pwm_get_period() (and maybe other similar functions) as deprecated.

* Fix drivers for all current 2 users of PWM regulator to support
hardware readout.

* Update PWM regulator as you described earlier in the thread (Feb 23).

* If PWM regulator is ever used on a new board whose PWM doesn't
support hardware readout, the voltage will change at probe time.


Did I get all that right?  Thanks!

Can you provide a "yes, you got that right" or a "no, you didn't
understand"?  That will unblock Boris, I think.

Sounds about right. Hopefully this will eliminate any objections that
others had about the series.

Thierry

Hi Thierry,

On Thu, 10 Mar 2016 18:54:38 +0100
Thierry Reding [off-list ref] wrote:

On Mon, Mar 07, 2016 at 08:34:19AM -0800, Doug Anderson wrote:

quoted

Thierry,

On Thu, Feb 25, 2016 at 3:14 PM, Doug Anderson [off-list ref] wrote:

quoted

So just to summarize:

* Add pwm_get_state(), pwm_apply_state(), pwm_get_args().
pwm_get_state() initially returns 0 for duty cycle if driver doesn't
support readout.

* Re-implement pwm_get_period() (and maybe other similar functions)
atop pwm_get_state() as you describe earlier in the thread.

* Document pwm_get_period() (and maybe other similar functions) as deprecated.

* Fix drivers for all current 2 users of PWM regulator to support
hardware readout.

* Update PWM regulator as you described earlier in the thread (Feb 23).

* If PWM regulator is ever used on a new board whose PWM doesn't
support hardware readout, the voltage will change at probe time.


Did I get all that right?  Thanks!

Can you provide a "yes, you got that right" or a "no, you didn't
understand"?  That will unblock Boris, I think.

Sounds about right. Hopefully this will eliminate any objections that
others had about the series.

Okay, I'll rework the series accordingly and send a new version soon.

Thanks,

Boris

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