[RFC PATCH 00/15] pwm: add support for atomic update

[RFC PATCH 00/15] pwm: add support for atomic update

[Boris Brezillon]

Hello Thierry,

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

This implementation is still experimental, and I may have missed some key
aspect, so any feedback are welcome.

Also note that I haven't protected the state update with any locking.
That's because the existing config does not protect against concurrent
access to a requested PWM device (see the pwm_config implementation).
I guess the PWM framework assume the user will implement the proper locking
scheme if it has to concurrently access the device.

The 5 first patches prepare the addition of the pwm_state concept, which
will be used to allow atomic updates.
The following patches introduce the pwm_state struct, initial state
retrieval and atomic update concepts.

Patches 12 and 13 are showing how one can implement the initial state
retrieval and atomic update features in a PWM driver (in this specific
case I implemented it in the rockchip driver).

The last 2 patches are making use of those changes to improve the
pwm-regulator driver (initializing the regulator state based on the
initial PWM state).

Best Regards,

Boris

Boris Brezillon (15):
  pwm: add the pwm_is_enabled() helper
  pwm: fix pwm_get_period and pwm_get_duty_cycle prototypes
  pwm: add pwm_get_polarity helper function
  pwm: make use of pwm_get_xxx helpers where appropriate
  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

 drivers/leds/leds-pwm.c              |   2 +-
 drivers/pwm/core.c                   | 136 ++++++++++++++++++++++++++++++-----
 drivers/pwm/pwm-atmel-tcb.c          |   2 +-
 drivers/pwm/pwm-atmel.c              |   6 +-
 drivers/pwm/pwm-bcm-kona.c           |   7 +-
 drivers/pwm/pwm-ep93xx.c             |   4 +-
 drivers/pwm/pwm-imx.c                |   5 +-
 drivers/pwm/pwm-mxs.c                |   4 +-
 drivers/pwm/pwm-pxa.c                |   2 +-
 drivers/pwm/pwm-renesas-tpu.c        |   2 +-
 drivers/pwm/pwm-rockchip.c           | 122 ++++++++++++++++++++++++-------
 drivers/pwm/pwm-sun4i.c              |   3 +-
 drivers/pwm/pwm-tegra.c              |   6 +-
 drivers/pwm/pwm-tiecap.c             |  10 +--
 drivers/pwm/pwm-tiehrpwm.c           |   6 +-
 drivers/pwm/sysfs.c                  |  13 ++--
 drivers/regulator/pwm-regulator.c    |  60 ++++++++++++++--
 drivers/video/backlight/lm3630a_bl.c |   4 +-
 drivers/video/backlight/pwm_bl.c     |   6 +-
 drivers/video/fbdev/ssd1307fb.c      |   2 +-
 include/linux/pwm.h                  |  82 ++++++++++++++++++---
 21 files changed, 380 insertions(+), 104 deletions(-)

-- 
1.9.1

[RFC PATCH 01/15] pwm: add the pwm_is_enabled() helper

[Boris Brezillon]

Some PWM drivers are testing the PWMF_ENABLED flag. Create an helper
function to hide the logic behind enabled test.
This will allow us to smoothly move from the current approach to an atomic
PWM update approach.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/core.c            |  4 ++--
 drivers/pwm/pwm-atmel-tcb.c   |  2 +-
 drivers/pwm/pwm-atmel.c       |  6 +++---
 drivers/pwm/pwm-bcm-kona.c    |  4 ++--
 drivers/pwm/pwm-ep93xx.c      |  4 ++--
 drivers/pwm/pwm-imx.c         |  2 +-
 drivers/pwm/pwm-mxs.c         |  4 ++--
 drivers/pwm/pwm-renesas-tpu.c |  2 +-
 drivers/pwm/pwm-tegra.c       |  6 +++---
 drivers/pwm/pwm-tiecap.c      | 10 +++++-----
 drivers/pwm/pwm-tiehrpwm.c    |  6 +++---
 drivers/pwm/sysfs.c           |  2 +-
 include/linux/pwm.h           |  5 +++++
 13 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3a7769f..f7c11d2 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -455,7 +455,7 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
 	if (!pwm->chip->ops->set_polarity)
 		return -ENOSYS;
 
-	if (test_bit(PWMF_ENABLED, &pwm->flags))
+	if (pwm_is_enabled(pwm))
 		return -EBUSY;
 
 	err = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
@@ -853,7 +853,7 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 		if (test_bit(PWMF_REQUESTED, &pwm->flags))
 			seq_puts(s, " requested");
 
-		if (test_bit(PWMF_ENABLED, &pwm->flags))
+		if (pwm_is_enabled(pwm))
 			seq_puts(s, " enabled");
 
 		seq_puts(s, "\n");
diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index d14e067..6da01b3 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -347,7 +347,7 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	tcbpwm->duty = duty;
 
 	/* If the PWM is enabled, call enable to apply the new conf */
-	if (test_bit(PWMF_ENABLED, &pwm->flags))
+	if (pwm_is_enabled(pwm))
 		atmel_tcb_pwm_enable(chip, pwm);
 
 	return 0;
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index a947c90..b3b294d 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -114,7 +114,7 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	u32 val;
 	int ret;
 
-	if (test_bit(PWMF_ENABLED, &pwm->flags) && (period_ns != pwm->period)) {
+	if (pwm_is_enabled(pwm) && (period_ns != pwm->period)) {
 		dev_err(chip->dev, "cannot change PWM period while enabled\n");
 		return -EBUSY;
 	}
@@ -176,7 +176,7 @@ static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * If the PWM channel is enabled, only update CDTY by using the update
 	 * register, it needs to set bit 10 of CMR to 0
 	 */
-	if (test_bit(PWMF_ENABLED, &pwm->flags))
+	if (pwm_is_enabled(pwm))
 		return;
 	/*
 	 * If the PWM channel is disabled, write value to duty and period
@@ -191,7 +191,7 @@ static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
 
-	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+	if (pwm_is_enabled(pwm)) {
 		/*
 		 * If the PWM channel is enabled, using the duty update register
 		 * to update the value.
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 7af8fea..dfdcf88 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -134,7 +134,7 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	/* If the PWM channel is enabled, write the settings to the HW */
-	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+	if (pwm_is_enabled(pwm)) {
 		value = readl(kp->base + PRESCALE_OFFSET);
 		value &= ~PRESCALE_MASK(chan);
 		value |= prescale << PRESCALE_SHIFT(chan);
@@ -287,7 +287,7 @@ static int kona_pwmc_remove(struct platform_device *pdev)
 	unsigned int chan;
 
 	for (chan = 0; chan < kp->chip.npwm; chan++)
-		if (test_bit(PWMF_ENABLED, &kp->chip.pwms[chan].flags))
+		if (pwm_is_enabled(&kp->chip.pwms[chan]))
 			clk_disable_unprepare(kp->clk);
 
 	return pwmchip_remove(&kp->chip);
diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c
index e593e9c..bbf10ae 100644
--- a/drivers/pwm/pwm-ep93xx.c
+++ b/drivers/pwm/pwm-ep93xx.c
@@ -82,7 +82,7 @@ static int ep93xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * The clock needs to be enabled to access the PWM registers.
 	 * Configuration can be changed at any time.
 	 */
-	if (!test_bit(PWMF_ENABLED, &pwm->flags)) {
+	if (!pwm_is_enabled(pwm)) {
 		ret = clk_enable(ep93xx_pwm->clk);
 		if (ret)
 			return ret;
@@ -113,7 +113,7 @@ static int ep93xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		ret = -EINVAL;
 	}
 
-	if (!test_bit(PWMF_ENABLED, &pwm->flags))
+	if (!pwm_is_enabled(pwm))
 		clk_disable(ep93xx_pwm->clk);
 
 	return ret;
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 66d6f0c..008dc64 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -114,7 +114,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	unsigned long long c;
 	unsigned long period_cycles, duty_cycles, prescale;
 	unsigned int period_ms;
-	bool enable = test_bit(PWMF_ENABLED, &pwm->flags);
+	bool enable = pwm_is_enabled(pwm);
 	int wait_count = 0, fifoav;
 	u32 cr, sr;
 
diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
index b430811..9a59632 100644
--- a/drivers/pwm/pwm-mxs.c
+++ b/drivers/pwm/pwm-mxs.c
@@ -77,7 +77,7 @@ static int mxs_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * If the PWM channel is disabled, make sure to turn on the clock
 	 * before writing the register. Otherwise, keep it enabled.
 	 */
-	if (!test_bit(PWMF_ENABLED, &pwm->flags)) {
+	if (!pwm_is_enabled(pwm)) {
 		ret = clk_prepare_enable(mxs->clk);
 		if (ret)
 			return ret;
@@ -92,7 +92,7 @@ static int mxs_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	/*
 	 * If the PWM is not enabled, turn the clock off again to save power.
 	 */
-	if (!test_bit(PWMF_ENABLED, &pwm->flags))
+	if (!pwm_is_enabled(pwm))
 		clk_disable_unprepare(mxs->clk);
 
 	return 0;
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index ee63f9e..075c1a7 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -301,7 +301,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm,
 	pwm->duty = duty;
 
 	/* If the channel is disabled we're done. */
-	if (!test_bit(PWMF_ENABLED, &_pwm->flags))
+	if (!pwm_is_enabled(_pwm))
 		return 0;
 
 	if (duty_only && pwm->timer_on) {
diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index cabd7d8..d4de060 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -112,7 +112,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * If the PWM channel is disabled, make sure to turn on the clock
 	 * before writing the register. Otherwise, keep it enabled.
 	 */
-	if (!test_bit(PWMF_ENABLED, &pwm->flags)) {
+	if (!pwm_is_enabled(pwm)) {
 		err = clk_prepare_enable(pc->clk);
 		if (err < 0)
 			return err;
@@ -124,7 +124,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	/*
 	 * If the PWM is not enabled, turn the clock off again to save power.
 	 */
-	if (!test_bit(PWMF_ENABLED, &pwm->flags))
+	if (!pwm_is_enabled(pwm))
 		clk_disable_unprepare(pc->clk);
 
 	return 0;
@@ -214,7 +214,7 @@ static int tegra_pwm_remove(struct platform_device *pdev)
 	for (i = 0; i < NUM_PWM; i++) {
 		struct pwm_device *pwm = &pc->chip.pwms[i];
 
-		if (!test_bit(PWMF_ENABLED, &pwm->flags))
+		if (!pwm_is_enabled(pwm))
 			if (clk_prepare_enable(pc->clk) < 0)
 				continue;
 
diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index e557bef..616af76 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -97,7 +97,7 @@ static int ecap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writew(reg_val, pc->mmio_base + ECCTL2);
 
-	if (!test_bit(PWMF_ENABLED, &pwm->flags)) {
+	if (!pwm_is_enabled(pwm)) {
 		/* Update active registers if not running */
 		writel(duty_cycles, pc->mmio_base + CAP2);
 		writel(period_cycles, pc->mmio_base + CAP1);
@@ -111,7 +111,7 @@ static int ecap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		writel(period_cycles, pc->mmio_base + CAP3);
 	}
 
-	if (!test_bit(PWMF_ENABLED, &pwm->flags)) {
+	if (!pwm_is_enabled(pwm)) {
 		reg_val = readw(pc->mmio_base + ECCTL2);
 		/* Disable APWM mode to put APWM output Low */
 		reg_val &= ~ECCTL2_APWM_MODE;
@@ -179,7 +179,7 @@ static void ecap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 static void ecap_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+	if (pwm_is_enabled(pwm)) {
 		dev_warn(chip->dev, "Removing PWM device without disabling\n");
 		pm_runtime_put_sync(chip->dev);
 	}
@@ -306,7 +306,7 @@ static int ecap_pwm_suspend(struct device *dev)
 	ecap_pwm_save_context(pc);
 
 	/* Disable explicitly if PWM is running */
-	if (test_bit(PWMF_ENABLED, &pwm->flags))
+	if (pwm_is_enabled(pwm))
 		pm_runtime_put_sync(dev);
 
 	return 0;
@@ -318,7 +318,7 @@ static int ecap_pwm_resume(struct device *dev)
 	struct pwm_device *pwm = pc->chip.pwms;
 
 	/* Enable explicitly if PWM was running */
-	if (test_bit(PWMF_ENABLED, &pwm->flags))
+	if (pwm_is_enabled(pwm))
 		pm_runtime_get_sync(dev);
 
 	ecap_pwm_restore_context(pc);
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 694b3cf..6a41e66 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -407,7 +407,7 @@ static void ehrpwm_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
 
-	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+	if (pwm_is_enabled(pwm)) {
 		dev_warn(chip->dev, "Removing PWM device without disabling\n");
 		pm_runtime_put_sync(chip->dev);
 	}
@@ -565,7 +565,7 @@ static int ehrpwm_pwm_suspend(struct device *dev)
 	for (i = 0; i < pc->chip.npwm; i++) {
 		struct pwm_device *pwm = &pc->chip.pwms[i];
 
-		if (!test_bit(PWMF_ENABLED, &pwm->flags))
+		if (!pwm_is_enabled(pwm))
 			continue;
 
 		/* Disable explicitly if PWM is running */
@@ -582,7 +582,7 @@ static int ehrpwm_pwm_resume(struct device *dev)
 	for (i = 0; i < pc->chip.npwm; i++) {
 		struct pwm_device *pwm = &pc->chip.pwms[i];
 
-		if (!test_bit(PWMF_ENABLED, &pwm->flags))
+		if (!pwm_is_enabled(pwm))
 			continue;
 
 		/* Enable explicitly if PWM was running */
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 4bd0c63..eecf21d 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -97,7 +97,7 @@ static ssize_t pwm_enable_show(struct device *child,
 			       char *buf)
 {
 	const struct pwm_device *pwm = child_to_pwm_device(child);
-	int enabled = test_bit(PWMF_ENABLED, &pwm->flags);
+	int enabled = pwm_is_enabled(pwm);
 
 	return sprintf(buf, "%d\n", enabled);
 }
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 36262d0..ec34f4d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -92,6 +92,11 @@ struct pwm_device {
 	enum pwm_polarity	polarity;
 };
 
+static inline bool pwm_is_enabled(const struct pwm_device *pwm)
+{
+	return test_bit(PWMF_ENABLED, &pwm->flags);
+}
+
 static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
 {
 	if (pwm)
-- 
1.9.1

Re: [RFC PATCH 01/15] pwm: add the pwm_is_enabled() helper

[Thierry Reding]

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

On Wed, Jul 01, 2015 at 10:21:47AM +0200, Boris Brezillon wrote:
> Some PWM drivers are testing the PWMF_ENABLED flag. Create an helper
> function to hide the logic behind enabled test.
> This will allow us to smoothly move from the current approach to an atomic
> PWM update approach.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/core.c            |  4 ++--
>  drivers/pwm/pwm-atmel-tcb.c   |  2 +-
>  drivers/pwm/pwm-atmel.c       |  6 +++---
>  drivers/pwm/pwm-bcm-kona.c    |  4 ++--
>  drivers/pwm/pwm-ep93xx.c      |  4 ++--
>  drivers/pwm/pwm-imx.c         |  2 +-
>  drivers/pwm/pwm-mxs.c         |  4 ++--
>  drivers/pwm/pwm-renesas-tpu.c |  2 +-
>  drivers/pwm/pwm-tegra.c       |  6 +++---
>  drivers/pwm/pwm-tiecap.c      | 10 +++++-----
>  drivers/pwm/pwm-tiehrpwm.c    |  6 +++---
>  drivers/pwm/sysfs.c           |  2 +-
>  include/linux/pwm.h           |  5 +++++
>  13 files changed, 31 insertions(+), 26 deletions(-)

Applied, thanks.

Thierry

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

[RFC PATCH 02/15] pwm: fix pwm_get_period and pwm_get_duty_cycle prototypes

[Boris Brezillon]

The pwm argument is not modified in pwm_get helpers, make it a const
argument so that they can be used from the sysfs functions.

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

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index ec34f4d..d8f6913 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -103,7 +103,7 @@ static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
 		pwm->period = period;
 }
 
-static inline unsigned int pwm_get_period(struct pwm_device *pwm)
+static inline unsigned int pwm_get_period(const struct pwm_device *pwm)
 {
 	return pwm ? pwm->period : 0;
 }
@@ -114,7 +114,7 @@ static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
 		pwm->duty_cycle = duty;
 }
 
-static inline unsigned int pwm_get_duty_cycle(struct pwm_device *pwm)
+static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm)
 {
 	return pwm ? pwm->duty_cycle : 0;
 }
-- 
1.9.1

Re: [RFC PATCH 02/15] pwm: fix pwm_get_period and pwm_get_duty_cycle prototypes

[Thierry Reding]

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

On Wed, Jul 01, 2015 at 10:21:48AM +0200, Boris Brezillon wrote:
> The pwm argument is not modified in pwm_get helpers, make it a const
> argument so that they can be used from the sysfs functions.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  include/linux/pwm.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied with a slightly reworded commit message. The prototypes weren't
"broken" so there's nothing to "fix". Also s/pwm/PWM/.

Thanks,
Thierry

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

[RFC PATCH 03/15] pwm: add pwm_get_polarity helper function

[Boris Brezillon]

Some drivers are directly accessing the ->polarity field in pwm_device.
Add an helper to retrieve the current polarity so that we can easily move
this field elsewhere (required to support atomic update).

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

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d8f6913..6f286df 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -124,6 +124,11 @@ 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 enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
+{
+	return pwm ? pwm->polarity : PWM_POLARITY_NORMAL;
+}
+
 /**
  * struct pwm_ops - PWM controller operations
  * @request: optional hook for requesting a PWM
-- 
1.9.1

Re: [RFC PATCH 03/15] pwm: add pwm_get_polarity helper function

[Thierry Reding]

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

On Wed, Jul 01, 2015 at 10:21:49AM +0200, Boris Brezillon wrote:
> Some drivers are directly accessing the ->polarity field in pwm_device.
> Add an helper to retrieve the current polarity so that we can easily move
> this field elsewhere (required to support atomic update).
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  include/linux/pwm.h | 5 +++++
>  1 file changed, 5 insertions(+)

Applied thanks.

Thierry

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

[RFC PATCH 04/15] pwm: make use of pwm_get_xxx helpers where appropriate

[Boris Brezillon]

Use the pwm_get_xxx helpers instead of directly accessing the fields in
pwm_device. This will allow us to smoothly move to the atomic update
approach.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/pwm-atmel.c    |  2 +-
 drivers/pwm/pwm-bcm-kona.c |  3 ++-
 drivers/pwm/pwm-imx.c      |  3 ++-
 drivers/pwm/pwm-rockchip.c |  2 +-
 drivers/pwm/sysfs.c        | 11 ++++++-----
 5 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index b3b294d..0e4bd4e 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -114,7 +114,7 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	u32 val;
 	int ret;
 
-	if (pwm_is_enabled(pwm) && (period_ns != pwm->period)) {
+	if (pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) {
 		dev_err(chip->dev, "cannot change PWM period while enabled\n");
 		return -EBUSY;
 	}
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index dfdcf88..920cd1b 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -194,7 +194,8 @@ static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 		return ret;
 	}
 
-	ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
+	ret = kona_pwmc_config(chip, pwm, pwm_get_duty_cycle(pwm),
+			       pwm_get_period(pwm));
 	if (ret < 0) {
 		clk_disable_unprepare(kp->clk);
 		return ret;
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 008dc64..d600fd5 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -129,7 +129,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 		sr = readl(imx->mmio_base + MX3_PWMSR);
 		fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
 		if (fifoav = MX3_PWMSR_FIFOAV_4WORDS) {
-			period_ms = DIV_ROUND_UP(pwm->period, NSEC_PER_MSEC);
+			period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
+						 NSEC_PER_MSEC);
 			msleep(period_ms);
 
 			sr = readl(imx->mmio_base + MX3_PWMSR);
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 9442df2..7d9cc90 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -83,7 +83,7 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
 			  PWM_CONTINUOUS;
 	u32 val;
 
-	if (pwm->polarity = PWM_POLARITY_INVERSED)
+	if (pwm_get_polarity(pwm) = PWM_POLARITY_INVERSED)
 		enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
 	else
 		enable_conf |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index eecf21d..ac0abec 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -46,7 +46,7 @@ static ssize_t pwm_period_show(struct device *child,
 {
 	const struct pwm_device *pwm = child_to_pwm_device(child);
 
-	return sprintf(buf, "%u\n", pwm->period);
+	return sprintf(buf, "%u\n", pwm_get_period(pwm));
 }
 
 static ssize_t pwm_period_store(struct device *child,
@@ -61,7 +61,7 @@ static ssize_t pwm_period_store(struct device *child,
 	if (ret)
 		return ret;
 
-	ret = pwm_config(pwm, pwm->duty_cycle, val);
+	ret = pwm_config(pwm, pwm_get_duty_cycle(pwm), val);
 
 	return ret ? : size;
 }
@@ -72,7 +72,7 @@ static ssize_t pwm_duty_cycle_show(struct device *child,
 {
 	const struct pwm_device *pwm = child_to_pwm_device(child);
 
-	return sprintf(buf, "%u\n", pwm->duty_cycle);
+	return sprintf(buf, "%u\n", pwm_get_duty_cycle(pwm));
 }
 
 static ssize_t pwm_duty_cycle_store(struct device *child,
@@ -87,7 +87,7 @@ static ssize_t pwm_duty_cycle_store(struct device *child,
 	if (ret)
 		return ret;
 
-	ret = pwm_config(pwm, val, pwm->period);
+	ret = pwm_config(pwm, val, pwm_get_period(pwm));
 
 	return ret ? : size;
 }
@@ -134,7 +134,8 @@ static ssize_t pwm_polarity_show(struct device *child,
 {
 	const struct pwm_device *pwm = child_to_pwm_device(child);
 
-	return sprintf(buf, "%s\n", pwm->polarity ? "inversed" : "normal");
+	return sprintf(buf, "%s\n",
+		       pwm_get_polarity(pwm) ? "inversed" : "normal");
 }
 
 static ssize_t pwm_polarity_store(struct device *child,
-- 
1.9.1

Re: [RFC PATCH 04/15] pwm: make use of pwm_get_xxx helpers where appropriate

[Thierry Reding]

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

On Wed, Jul 01, 2015 at 10:21:50AM +0200, Boris Brezillon wrote:
> Use the pwm_get_xxx helpers instead of directly accessing the fields in
> pwm_device. This will allow us to smoothly move to the atomic update
> approach.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/pwm-atmel.c    |  2 +-
>  drivers/pwm/pwm-bcm-kona.c |  3 ++-
>  drivers/pwm/pwm-imx.c      |  3 ++-
>  drivers/pwm/pwm-rockchip.c |  2 +-
>  drivers/pwm/sysfs.c        | 11 ++++++-----
>  5 files changed, 12 insertions(+), 9 deletions(-)

Applied, thanks.

Thierry

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

[RFC PATCH 05/15] pwm: introduce default period and polarity concepts

[Boris Brezillon]

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 <boris.brezillon@free-electrons.com>
---
 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    |  2 +-
 drivers/video/backlight/lm3630a_bl.c |  4 ++--
 drivers/video/backlight/pwm_bl.c     |  2 +-
 drivers/video/fbdev/ssd1307fb.c      |  2 +-
 include/linux/pwm.h                  | 15 +++++++++++++++
 9 files changed, 31 insertions(+), 15 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 f7c11d2..7ffad2b 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;
 }
@@ -262,7 +262,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);
 	}
@@ -704,8 +704,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 ffa9612..12b4d9d 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -46,7 +46,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;
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 3e153c0..6949626 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 6f286df..72a21d5 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -103,11 +103,21 @@ 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)
@@ -124,6 +134,11 @@ 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

Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts

[Uwe Kleine-König]

On Wed, Jul 01, 2015 at 10:21:51AM +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.
Would it make sense to put the prototypes of
pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
then?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts

[Boris Brezillon]

On Thu, 2 Jul 2015 08:44:45 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> On Wed, Jul 01, 2015 at 10:21:51AM +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.
> Would it make sense to put the prototypes of
> pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
> then?
> 

Yes, definitely. I was thinking about moving those functions/prototypes
into include/linux/pwm-provider.h, but I'm fine with
drivers/pwm/pwm-private.h too.

Thierry, any opinion ?


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

Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts

[Thierry Reding]

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

On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> On Thu, 2 Jul 2015 08:44:45 +0200
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > On Wed, Jul 01, 2015 at 10:21:51AM +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.
> > Would it make sense to put the prototypes of
> > pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
> > then?
> > 
> 
> Yes, definitely. I was thinking about moving those functions/prototypes
> into include/linux/pwm-provider.h, but I'm fine with
> drivers/pwm/pwm-private.h too.
> 
> Thierry, any opinion ?

I'm not sure I see the need for this. If they are the default values and
drivers have no need to change them, then storing them in the regular
period and polarity fields seems just fine (they'll be propagated into
new state objects as they get created).

And if the driver has a need to change them, then why would it ever care
about the default values?

Thierry

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

Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts

[Boris Brezillon]

Hi Thierry,

On Mon, 20 Jul 2015 10:03:14 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> > On Thu, 2 Jul 2015 08:44:45 +0200
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > 
> > > On Wed, Jul 01, 2015 at 10:21:51AM +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.
> > > Would it make sense to put the prototypes of
> > > pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
> > > then?
> > > 
> > 
> > Yes, definitely. I was thinking about moving those functions/prototypes
> > into include/linux/pwm-provider.h, but I'm fine with
> > drivers/pwm/pwm-private.h too.
> > 
> > Thierry, any opinion ?
> 
> I'm not sure I see the need for this. If they are the default values and
> drivers have no need to change them, then storing them in the regular
> period and polarity fields seems just fine (they'll be propagated into
> new state objects as they get created).
> 
> And if the driver has a need to change them, then why would it ever care
> about the default values?

Because the period is often directly extracted from the DT, and this
extracted period may not match the one configured by the bootloader.

If the driver wants to display the current status without changing the
PWM state, then the driver will use the current state. ITOH, if it
has to apply a new config, the driver will use the default period
value (extracted from the DT) and change the duty-cycle depending on its
needs.
This is the case we have with the pwm-regulator driver: we want to
display the initial voltage value without changing the PWM config, and
when someone decides to change the voltage, we want to use the default
period instead of keeping the one configured by the bootloader.

Best Regards,

Boris


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

Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 10:14:43AM +0200, Boris Brezillon wrote:
> Hi Thierry,
> 
> On Mon, 20 Jul 2015 10:03:14 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> > > On Thu, 2 Jul 2015 08:44:45 +0200
> > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > > 
> > > > On Wed, Jul 01, 2015 at 10:21:51AM +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.
> > > > Would it make sense to put the prototypes of
> > > > pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
> > > > then?
> > > > 
> > > 
> > > Yes, definitely. I was thinking about moving those functions/prototypes
> > > into include/linux/pwm-provider.h, but I'm fine with
> > > drivers/pwm/pwm-private.h too.
> > > 
> > > Thierry, any opinion ?
> > 
> > I'm not sure I see the need for this. If they are the default values and
> > drivers have no need to change them, then storing them in the regular
> > period and polarity fields seems just fine (they'll be propagated into
> > new state objects as they get created).
> > 
> > And if the driver has a need to change them, then why would it ever care
> > about the default values?
> 
> Because the period is often directly extracted from the DT, and this
> extracted period may not match the one configured by the bootloader.
> 
> If the driver wants to display the current status without changing the
> PWM state, then the driver will use the current state. ITOH, if it
> has to apply a new config, the driver will use the default period
> value (extracted from the DT) and change the duty-cycle depending on its
> needs.
> This is the case we have with the pwm-regulator driver: we want to
> display the initial voltage value without changing the PWM config, and
> when someone decides to change the voltage, we want to use the default
> period instead of keeping the one configured by the bootloader.

Wouldn't it make more sense to postpone this until the introduction of
the default state, then? That way we'd be getting a more consistent way
of dealing with default vs. initial by looking only at state objects.

Ideally initial state should be the same as the default state. Except
maybe for the duty-cycle, which won't be encoded in the default state
anyway.

Thierry

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

Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts

[Boris Brezillon]

On Mon, 20 Jul 2015 10:22:42 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jul 20, 2015 at 10:14:43AM +0200, Boris Brezillon wrote:
> > Hi Thierry,
> > 
> > On Mon, 20 Jul 2015 10:03:14 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> > > > On Thu, 2 Jul 2015 08:44:45 +0200
> > > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > > > 
> > > > > On Wed, Jul 01, 2015 at 10:21:51AM +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.
> > > > > Would it make sense to put the prototypes of
> > > > > pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
> > > > > then?
> > > > > 
> > > > 
> > > > Yes, definitely. I was thinking about moving those functions/prototypes
> > > > into include/linux/pwm-provider.h, but I'm fine with
> > > > drivers/pwm/pwm-private.h too.
> > > > 
> > > > Thierry, any opinion ?
> > > 
> > > I'm not sure I see the need for this. If they are the default values and
> > > drivers have no need to change them, then storing them in the regular
> > > period and polarity fields seems just fine (they'll be propagated into
> > > new state objects as they get created).
> > > 
> > > And if the driver has a need to change them, then why would it ever care
> > > about the default values?
> > 
> > Because the period is often directly extracted from the DT, and this
> > extracted period may not match the one configured by the bootloader.
> > 
> > If the driver wants to display the current status without changing the
> > PWM state, then the driver will use the current state. ITOH, if it
> > has to apply a new config, the driver will use the default period
> > value (extracted from the DT) and change the duty-cycle depending on its
> > needs.
> > This is the case we have with the pwm-regulator driver: we want to
> > display the initial voltage value without changing the PWM config, and
> > when someone decides to change the voltage, we want to use the default
> > period instead of keeping the one configured by the bootloader.
> 
> Wouldn't it make more sense to postpone this until the introduction of
> the default state, then? That way we'd be getting a more consistent way
> of dealing with default vs. initial by looking only at state objects.

Hm, I was trying to keep the series bisectable. If we do that
after introducing the default state concept, then some drivers will
retrieve invalid values until the patches introducing the default
helpers and changing the different drivers to call the default helpers
where appropriate are introduced.

> 
> Ideally initial state should be the same as the default state. Except
> maybe for the duty-cycle, which won't be encoded in the default state
> anyway.

Yes, but we don't live in an ideal world ;-), and the value set in an
old bootloaders might be considered wrong at some point, and new dts
versions might decide to change a bit the period value.


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

[RFC PATCH 06/15] pwm: define a new pwm_state struct

[Boris Brezillon]

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 <boris.brezillon@free-electrons.com>
---
 drivers/pwm/core.c  |  6 +++---
 include/linux/pwm.h | 20 ++++++++++++--------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 7ffad2b..a6bc8e6 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -431,8 +431,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;
 }
@@ -462,7 +462,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 72a21d5..bed7234 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -79,6 +79,12 @@ enum {
 	PWMF_EXPORTED = 1 << 2,
 };
 
+struct pwm_state {
+	unsigned int		period; 	/* in nanoseconds */
+	unsigned int		duty_cycle;	/* in nanoseconds */
+	enum pwm_polarity	polarity;
+};
+
 struct pwm_device {
 	const char		*label;
 	unsigned long		flags;
@@ -87,9 +93,7 @@ struct pwm_device {
 	struct pwm_chip		*chip;
 	void			*chip_data;
 
-	unsigned int		period; 	/* in nanoseconds */
-	unsigned int		duty_cycle;	/* in nanoseconds */
-	enum pwm_polarity	polarity;
+	struct pwm_state	state;
 };
 
 static inline bool pwm_is_enabled(const struct pwm_device *pwm)
@@ -100,7 +104,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, unsigned int period)
@@ -110,7 +114,7 @@ static inline void pwm_set_default_period(struct pwm_device *pwm, unsigned int p
 
 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)
@@ -121,12 +125,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;
 }
 
 /*
@@ -141,7 +145,7 @@ static inline void pwm_set_default_polarity(struct pwm_device *pwm, enum pwm_pol
 
 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

Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct

[Thierry Reding]

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

On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
[...]
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
[...]
> +struct pwm_state {
> +	unsigned int		period; 	/* in nanoseconds */
> +	unsigned int		duty_cycle;	/* in nanoseconds */
> +	enum pwm_polarity	polarity;
> +};

No need for the extra padding here.

Thierry

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

Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct

[Boris Brezillon]

On Mon, 20 Jul 2015 10:04:59 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
> [...]
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> [...]
> > +struct pwm_state {
> > +	unsigned int		period; 	/* in nanoseconds */
> > +	unsigned int		duty_cycle;	/* in nanoseconds */
> > +	enum pwm_polarity	polarity;
> > +};
> 
> No need for the extra padding here.

What do you mean by "extra padding" ?
I just reused the indentation used in the pwm_device struct.

Would you prefer something like that ?

struct pwm_state {
	unsigned int period; 		/* in nanoseconds */
	unsigned int duty_cycle;	/* in nanoseconds */
	enum pwm_polarity polarity;
};

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

Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 12:01:16PM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:04:59 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
> > [...]
> > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > [...]
> > > +struct pwm_state {
> > > +	unsigned int		period; 	/* in nanoseconds */
> > > +	unsigned int		duty_cycle;	/* in nanoseconds */
> > > +	enum pwm_polarity	polarity;
> > > +};
> > 
> > No need for the extra padding here.
> 
> What do you mean by "extra padding" ?
> I just reused the indentation used in the pwm_device struct.

Yeah, I have a local patch to fix that up. I find it useless to pad
things like this, and it has the downside that it will become totally
inconsistent (or cause a lot of churn by reformatting) if ever you add a
field that extends beyond the padding. Single spaces don't have any such
drawbacks and, in my opinion, look just as good.

> Would you prefer something like that ?
> 
> struct pwm_state {
> 	unsigned int period; 		/* in nanoseconds */
> 	unsigned int duty_cycle;	/* in nanoseconds */
> 	enum pwm_polarity polarity;
> };

Yeah. I'd say even the comments would be more suited in a kerneldoc-
style comment:

	/**
	 * 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;
	};

This is something that users will need to deal with, so eventually
somebody might look at this via some DocBook generated HTML or PDF.

Thierry

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

Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct

[Boris Brezillon]

On Mon, 20 Jul 2015 12:09:26 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jul 20, 2015 at 12:01:16PM +0200, Boris Brezillon wrote:
> > On Mon, 20 Jul 2015 10:04:59 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
> > > [...]
> > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > [...]
> > > > +struct pwm_state {
> > > > +	unsigned int		period; 	/* in nanoseconds */
> > > > +	unsigned int		duty_cycle;	/* in nanoseconds */
> > > > +	enum pwm_polarity	polarity;
> > > > +};
> > > 
> > > No need for the extra padding here.
> > 
> > What do you mean by "extra padding" ?
> > I just reused the indentation used in the pwm_device struct.
> 
> Yeah, I have a local patch to fix that up. I find it useless to pad
> things like this, and it has the downside that it will become totally
> inconsistent (or cause a lot of churn by reformatting) if ever you add a
> field that extends beyond the padding. Single spaces don't have any such
> drawbacks and, in my opinion, look just as good.

I prefer the single space approach too, so I won't complain ;-).

> 
> > Would you prefer something like that ?
> > 
> > struct pwm_state {
> > 	unsigned int period; 		/* in nanoseconds */
> > 	unsigned int duty_cycle;	/* in nanoseconds */
> > 	enum pwm_polarity polarity;
> > };
> 
> Yeah. I'd say even the comments would be more suited in a kerneldoc-
> style comment:
> 
> 	/**
> 	 * 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;
> 	};
> 
> This is something that users will need to deal with, so eventually
> somebody might look at this via some DocBook generated HTML or PDF.

I agree.

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

[RFC PATCH 07/15] pwm: move the enabled/disabled info to pwm_state struct

[Boris Brezillon]

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 <boris.brezillon@free-electrons.com>
---
 drivers/pwm/core.c  | 15 ++++++++++++---
 include/linux/pwm.h |  6 +++---
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index a6bc8e6..3e830ce 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -474,8 +474,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;
 }
@@ -487,8 +494,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 bed7234..fd3e0f0 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -75,14 +75,14 @@ enum pwm_polarity {
 
 enum {
 	PWMF_REQUESTED = 1 << 0,
-	PWMF_ENABLED = 1 << 1,
-	PWMF_EXPORTED = 1 << 2,
+	PWMF_EXPORTED = 1 << 1,
 };
 
 struct pwm_state {
 	unsigned int		period; 	/* in nanoseconds */
 	unsigned int		duty_cycle;	/* in nanoseconds */
 	enum pwm_polarity	polarity;
+	bool			enabled;
 };
 
 struct pwm_device {
@@ -98,7 +98,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

Re: [RFC PATCH 07/15] pwm: move the enabled/disabled info to pwm_state struct

[Thierry Reding]

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

On Wed, Jul 01, 2015 at 10:21:53AM +0200, Boris Brezillon wrote:
> 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 <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/core.c  | 15 ++++++++++++---
>  include/linux/pwm.h |  6 +++---
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index a6bc8e6..3e830ce 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -474,8 +474,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;
> +	}

Technically there's now a race between the pwm_is_enabled() and
pwm->state.enabled = true; statements, but as discussed in the cover
letter I think that's fine because of the assumptions about concurrent
usage of PWMs.

The most important check (PWMF_REQUESTED) is still atomic, so it is
still up to drivers to properly lock concurrent access to a PWM device
and the core will make sure that a device can only be requested once.

Thierry

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

[RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period

[Boris Brezillon]

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 <boris.brezillon@free-electrons.com>
---
 drivers/video/backlight/pwm_bl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index ae498c1..fe5597c 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	 * via the PWM lookup table.
 	 */
 	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

Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period

[Thierry Reding]

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

On Wed, Jul 01, 2015 at 10:21:54AM +0200, 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 <boris.brezillon@free-electrons.com>
> ---
>  drivers/video/backlight/pwm_bl.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index ae498c1..fe5597c 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	 * via the PWM lookup table.
>  	 */
>  	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);

As far as I remember this line is there in order to pass in a period if
the backlight driver is initialized from board setup files. In such a
case there won't be an period associated with the PWM channel in the
first place.

I think even with the introduction of a default period, we'd be missing
out on the board setup case because there is no standard place where it
is being set, so it must come from the platform data.

Thierry

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

Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period

[Boris Brezillon]

On Mon, 20 Jul 2015 10:16:00 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Jul 01, 2015 at 10:21:54AM +0200, 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 <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/video/backlight/pwm_bl.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index ae498c1..fe5597c 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> >  	 * via the PWM lookup table.
> >  	 */
> >  	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);
> 
> As far as I remember this line is there in order to pass in a period if
> the backlight driver is initialized from board setup files. In such a
> case there won't be an period associated with the PWM channel in the
> first place.
> 
> I think even with the introduction of a default period, we'd be missing
> out on the board setup case because there is no standard place where it
> is being set, so it must come from the platform data.

AFAICT, we don't need to explicitly set the period when probing the
backlight device, because it will be set next time we call
pwm_config(), and since we're passing pb->period when calling
pwm_config() everything should be fine.


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

Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:16:00 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Wed, Jul 01, 2015 at 10:21:54AM +0200, 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 <boris.brezillon@free-electrons.com>
> > > ---
> > >  drivers/video/backlight/pwm_bl.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > index ae498c1..fe5597c 100644
> > > --- a/drivers/video/backlight/pwm_bl.c
> > > +++ b/drivers/video/backlight/pwm_bl.c
> > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > >  	 * via the PWM lookup table.
> > >  	 */
> > >  	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);
> > 
> > As far as I remember this line is there in order to pass in a period if
> > the backlight driver is initialized from board setup files. In such a
> > case there won't be an period associated with the PWM channel in the
> > first place.
> > 
> > I think even with the introduction of a default period, we'd be missing
> > out on the board setup case because there is no standard place where it
> > is being set, so it must come from the platform data.
> 
> AFAICT, we don't need to explicitly set the period when probing the
> backlight device, because it will be set next time we call
> pwm_config(), and since we're passing pb->period when calling
> pwm_config() everything should be fine.

Calling pwm_set_period() is still good for consistency. Consider for
example what happens if after the driver were to call pwm_get_period().
It would return some more or less random value (likely 0 or whatever it
had been set to by an earlier user).

Technically I think the most proper equivalent here would be to set the
default state's period to data->pwm_period_ns, but I don't think that's
proper to do. Perhaps since this is only relevant to boards where the
backlight device is created from board setup code we don't have to care
so much about messing up the initial state because either the board
setup code has been carefully written to match what the bootloader set
up, or because they don't match at all, in which case we don't have to
worry anyway.

Of course the right thing to do would be to replace all initialization
of the data->pwm_period_ns by proper PWM lookup tables, but that's
proven difficult in the past since very few people still have access to
hardware where that code gets executed.

Thierry

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

Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period

[Boris Brezillon]

On Mon, 20 Jul 2015 10:36:50 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > On Mon, 20 Jul 2015 10:16:00 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, 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 <boris.brezillon@free-electrons.com>
> > > > ---
> > > >  drivers/video/backlight/pwm_bl.c | 4 +---
> > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > index ae498c1..fe5597c 100644
> > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > >  	 * via the PWM lookup table.
> > > >  	 */
> > > >  	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);
> > > 
> > > As far as I remember this line is there in order to pass in a period if
> > > the backlight driver is initialized from board setup files. In such a
> > > case there won't be an period associated with the PWM channel in the
> > > first place.
> > > 
> > > I think even with the introduction of a default period, we'd be missing
> > > out on the board setup case because there is no standard place where it
> > > is being set, so it must come from the platform data.
> > 
> > AFAICT, we don't need to explicitly set the period when probing the
> > backlight device, because it will be set next time we call
> > pwm_config(), and since we're passing pb->period when calling
> > pwm_config() everything should be fine.
> 
> Calling pwm_set_period() is still good for consistency. Consider for
> example what happens if after the driver were to call pwm_get_period().
> It would return some more or less random value (likely 0 or whatever it
> had been set to by an earlier user).

Yes, that's true in general, but in this specific driver
pwm_get_period() is never called, and the driver only relies on the
pb->period value.

> 
> Technically I think the most proper equivalent here would be to set the
> default state's period to data->pwm_period_ns, but I don't think that's
> proper to do. Perhaps since this is only relevant to boards where the
> backlight device is created from board setup code we don't have to care
> so much about messing up the initial state because either the board
> setup code has been carefully written to match what the bootloader set
> up, or because they don't match at all, in which case we don't have to
> worry anyway.

IMHO, if we had to support default period values for non DT boards, the
proper way would be to pass something in the PWM platform data and let
the PWM driver (or PWM core) initialize the default PWM state.
This way the PWM user could rely on the pwm_get_default_period() helper
to extract the default period value.

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

Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 10:50:03AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:36:50 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > > On Mon, 20 Jul 2015 10:16:00 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > 
> > > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, 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 <boris.brezillon@free-electrons.com>
> > > > > ---
> > > > >  drivers/video/backlight/pwm_bl.c | 4 +---
> > > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > index ae498c1..fe5597c 100644
> > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > >  	 * via the PWM lookup table.
> > > > >  	 */
> > > > >  	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);
> > > > 
> > > > As far as I remember this line is there in order to pass in a period if
> > > > the backlight driver is initialized from board setup files. In such a
> > > > case there won't be an period associated with the PWM channel in the
> > > > first place.
> > > > 
> > > > I think even with the introduction of a default period, we'd be missing
> > > > out on the board setup case because there is no standard place where it
> > > > is being set, so it must come from the platform data.
> > > 
> > > AFAICT, we don't need to explicitly set the period when probing the
> > > backlight device, because it will be set next time we call
> > > pwm_config(), and since we're passing pb->period when calling
> > > pwm_config() everything should be fine.
> > 
> > Calling pwm_set_period() is still good for consistency. Consider for
> > example what happens if after the driver were to call pwm_get_period().
> > It would return some more or less random value (likely 0 or whatever it
> > had been set to by an earlier user).
> 
> Yes, that's true in general, but in this specific driver
> pwm_get_period() is never called, and the driver only relies on the
> pb->period value.

Perhaps that's something that should change. If the PWM core has all
this infrastructure there should be no need for the backlight driver to
keep it's own copy of that variable.

> > Technically I think the most proper equivalent here would be to set the
> > default state's period to data->pwm_period_ns, but I don't think that's
> > proper to do. Perhaps since this is only relevant to boards where the
> > backlight device is created from board setup code we don't have to care
> > so much about messing up the initial state because either the board
> > setup code has been carefully written to match what the bootloader set
> > up, or because they don't match at all, in which case we don't have to
> > worry anyway.
> 
> IMHO, if we had to support default period values for non DT boards, the
> proper way would be to pass something in the PWM platform data and let
> the PWM driver (or PWM core) initialize the default PWM state.
> This way the PWM user could rely on the pwm_get_default_period() helper
> to extract the default period value.

Yes, that's what PWM lookup tables are meant to address. I tried to
convert existing users a number of times, but never got any replies and
since it's board code I couldn't merge this through the PWM tree...

Thierry

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

Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period

[Boris Brezillon]

On Mon, 20 Jul 2015 11:10:04 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jul 20, 2015 at 10:50:03AM +0200, Boris Brezillon wrote:
> > On Mon, 20 Jul 2015 10:36:50 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > > > On Mon, 20 Jul 2015 10:16:00 +0200
> > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > 
> > > > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, 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 <boris.brezillon@free-electrons.com>
> > > > > > ---
> > > > > >  drivers/video/backlight/pwm_bl.c | 4 +---
> > > > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > > index ae498c1..fe5597c 100644
> > > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > > >  	 * via the PWM lookup table.
> > > > > >  	 */
> > > > > >  	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);
> > > > > 
> > > > > As far as I remember this line is there in order to pass in a period if
> > > > > the backlight driver is initialized from board setup files. In such a
> > > > > case there won't be an period associated with the PWM channel in the
> > > > > first place.
> > > > > 
> > > > > I think even with the introduction of a default period, we'd be missing
> > > > > out on the board setup case because there is no standard place where it
> > > > > is being set, so it must come from the platform data.
> > > > 
> > > > AFAICT, we don't need to explicitly set the period when probing the
> > > > backlight device, because it will be set next time we call
> > > > pwm_config(), and since we're passing pb->period when calling
> > > > pwm_config() everything should be fine.
> > > 
> > > Calling pwm_set_period() is still good for consistency. Consider for
> > > example what happens if after the driver were to call pwm_get_period().
> > > It would return some more or less random value (likely 0 or whatever it
> > > had been set to by an earlier user).
> > 
> > Yes, that's true in general, but in this specific driver
> > pwm_get_period() is never called, and the driver only relies on the
> > pb->period value.
> 
> Perhaps that's something that should change. If the PWM core has all
> this infrastructure there should be no need for the backlight driver to
> keep it's own copy of that variable.

Yes, probably. In any case, I don't think we want PWM users to be able
to mess up with the current or default PWM state, that's why I was
planning on making the pwm_set_default_xxx helpers private to PWM
drivers and core infrastructure.

Also note that if we keep this assignment it should at least be changed
to a pwm_set_default_period() so that it does not override the current
PWM state.


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

Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 11:57:04AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 11:10:04 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Mon, Jul 20, 2015 at 10:50:03AM +0200, Boris Brezillon wrote:
> > > On Mon, 20 Jul 2015 10:36:50 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > 
> > > > On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > > > > On Mon, 20 Jul 2015 10:16:00 +0200
> > > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > 
> > > > > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, 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 <boris.brezillon@free-electrons.com>
> > > > > > > ---
> > > > > > >  drivers/video/backlight/pwm_bl.c | 4 +---
> > > > > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > > > index ae498c1..fe5597c 100644
> > > > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > > > >  	 * via the PWM lookup table.
> > > > > > >  	 */
> > > > > > >  	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);
> > > > > > 
> > > > > > As far as I remember this line is there in order to pass in a period if
> > > > > > the backlight driver is initialized from board setup files. In such a
> > > > > > case there won't be an period associated with the PWM channel in the
> > > > > > first place.
> > > > > > 
> > > > > > I think even with the introduction of a default period, we'd be missing
> > > > > > out on the board setup case because there is no standard place where it
> > > > > > is being set, so it must come from the platform data.
> > > > > 
> > > > > AFAICT, we don't need to explicitly set the period when probing the
> > > > > backlight device, because it will be set next time we call
> > > > > pwm_config(), and since we're passing pb->period when calling
> > > > > pwm_config() everything should be fine.
> > > > 
> > > > Calling pwm_set_period() is still good for consistency. Consider for
> > > > example what happens if after the driver were to call pwm_get_period().
> > > > It would return some more or less random value (likely 0 or whatever it
> > > > had been set to by an earlier user).
> > > 
> > > Yes, that's true in general, but in this specific driver
> > > pwm_get_period() is never called, and the driver only relies on the
> > > pb->period value.
> > 
> > Perhaps that's something that should change. If the PWM core has all
> > this infrastructure there should be no need for the backlight driver to
> > keep it's own copy of that variable.
> 
> Yes, probably. In any case, I don't think we want PWM users to be able
> to mess up with the current or default PWM state, that's why I was
> planning on making the pwm_set_default_xxx helpers private to PWM
> drivers and core infrastructure.
> 
> Also note that if we keep this assignment it should at least be changed
> to a pwm_set_default_period() so that it does not override the current
> PWM state.

I think we should be able to live without the assignment. Perhaps when
replacing it, add a comment saying that this is for very legacy cases
only and that PWM lookup tables are the right way to fix this.

Thierry

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

[RFC PATCH 09/15] pwm: declare a default PWM state

[Boris Brezillon]

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 <boris.brezillon@free-electrons.com>
---
 include/linux/pwm.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index fd3e0f0..0f36a06 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -94,6 +94,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)
@@ -109,7 +110,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)
@@ -119,7 +121,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)
@@ -140,7 +142,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

[RFC PATCH 10/15] pwm: add the PWM initial state retrieval infra

[Boris Brezillon]

Add a ->init_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 <boris.brezillon@free-electrons.com>
---
 drivers/pwm/core.c  | 3 +++
 include/linux/pwm.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3e830ce..30631f5 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -264,6 +264,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->hwpwm = i;
 		pwm_set_default_polarity(pwm, polarity);
 
+		if (chip->ops->init_state)
+			chip->ops->init_state(chip, pwm);
+
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 0f36a06..b47244a 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -177,6 +177,8 @@ struct pwm_ops {
 					  struct pwm_device *pwm);
 	void			(*disable)(struct pwm_chip *chip,
 					   struct pwm_device *pwm);
+	void			(*init_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

Re: [RFC PATCH 10/15] pwm: add the PWM initial state retrieval infra

[Thierry Reding]

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

On Wed, Jul 01, 2015 at 10:21:56AM +0200, Boris Brezillon wrote:
> Add a ->init_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 <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/core.c  | 3 +++
>  include/linux/pwm.h | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3e830ce..30631f5 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -264,6 +264,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>  		pwm->hwpwm = i;
>  		pwm_set_default_polarity(pwm, polarity);
>  
> +		if (chip->ops->init_state)
> +			chip->ops->init_state(chip, pwm);
> +
>  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>  	}
>  
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 0f36a06..b47244a 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -177,6 +177,8 @@ struct pwm_ops {
>  					  struct pwm_device *pwm);
>  	void			(*disable)(struct pwm_chip *chip,
>  					   struct pwm_device *pwm);
> +	void			(*init_state)(struct pwm_chip *chip,
> +					      struct pwm_device *pwm);

I think I'd call this reset_state. init has this connotation of setting
a set of default values. For reset it's clearer in my opinion that it's
resetting to the hardware state.

Thierry

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

Re: [RFC PATCH 10/15] pwm: add the PWM initial state retrieval infra

[Boris Brezillon]

On Mon, 20 Jul 2015 11:01:24 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Jul 01, 2015 at 10:21:56AM +0200, Boris Brezillon wrote:
> > Add a ->init_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 <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/pwm/core.c  | 3 +++
> >  include/linux/pwm.h | 2 ++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 3e830ce..30631f5 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -264,6 +264,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> >  		pwm->hwpwm = i;
> >  		pwm_set_default_polarity(pwm, polarity);
> >  
> > +		if (chip->ops->init_state)
> > +			chip->ops->init_state(chip, pwm);
> > +
> >  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
> >  	}
> >  
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 0f36a06..b47244a 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -177,6 +177,8 @@ struct pwm_ops {
> >  					  struct pwm_device *pwm);
> >  	void			(*disable)(struct pwm_chip *chip,
> >  					   struct pwm_device *pwm);
> > +	void			(*init_state)(struct pwm_chip *chip,
> > +					      struct pwm_device *pwm);
> 
> I think I'd call this reset_state. init has this connotation of setting
> a set of default values. For reset it's clearer in my opinion that it's
> resetting to the hardware state.

I'm fine with the reset_state name, I'll change that in my v2.

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

[RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update

[Boris Brezillon]

Add an ->apply() method to the pwm_ops struct to allow PWM drivers to
implement atomic update.
This method will be prefered 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 <boris.brezillon@free-electrons.com>
---
 drivers/pwm/core.c  | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
 include/linux/pwm.h |  26 +++++++++++++
 2 files changed, 124 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 30631f5..6dafd8e 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -238,8 +238,9 @@ 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->ops->apply &&
+	    (!chip->ops->config || !chip->ops->enable ||
+	     !chip->ops->disable)) || !chip->npwm)
 		return -EINVAL;
 
 	mutex_lock(&pwm_lock);
@@ -430,7 +431,17 @@ 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->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;
 
@@ -455,6 +466,17 @@ 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->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;
 
@@ -477,17 +499,27 @@ 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->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);
 
@@ -497,13 +529,67 @@ 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->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);
 
+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->state = *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 b47244a..7e99679 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 	return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
 }
 
+/*
+ * pwm_apply_state - apply a new state to the PWM device
+ */
+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
+
+/*
+ * pwm_get_state - retrieve 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
+ */
+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
@@ -177,6 +200,9 @@ struct pwm_ops {
 					  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			(*init_state)(struct pwm_chip *chip,
 					      struct pwm_device *pwm);
 #ifdef CONFIG_DEBUG_FS
-- 
1.9.1

Re: [RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update

[Thierry Reding]

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

On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
> Add an ->apply() method to the pwm_ops struct to allow PWM drivers to
> implement atomic update.
> This method will be prefered 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 <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/core.c  | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
>  include/linux/pwm.h |  26 +++++++++++++
>  2 files changed, 124 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 30631f5..6dafd8e 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -238,8 +238,9 @@ 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->ops->apply &&
> +	    (!chip->ops->config || !chip->ops->enable ||
> +	     !chip->ops->disable)) || !chip->npwm)
>  		return -EINVAL;

This is becoming really unreadable, perhaps split it into two checks, or
even split out the sanity check on the ops into a separate function to
make the negations easier to read:

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

and then use this:

	if (!chip || !chip->dev || !chip->ops || !chip->npwm)
		return -EINVAL;

	if (!pwm_ops_check(chip->ops))
		return -EINVAL;

>  	mutex_lock(&pwm_lock);
> @@ -430,7 +431,17 @@ 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->state;

Shouldn't this use pwm_get_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;
>  
> @@ -455,6 +466,17 @@ 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->state;

Same here.

> +
> +		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;
>  
> @@ -477,17 +499,27 @@ 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->state;

And here.

> +
> +		state.enabled = true;
> +		err = pwm->chip->ops->apply(pwm->chip, pwm, &state);

There should be a space between the above two lines.

> +	} 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);
>  
> @@ -497,13 +529,67 @@ 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->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);

Same comments as for pwm_enable().

>  
> +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->state = *state;

Maybe we want pwm_set_state() for this?

> +	} 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 b47244a..7e99679 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
>  	return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
>  }
>  
> +/*
> + * pwm_apply_state - apply a new state to the PWM device
> + */
> +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);

If you add kerneldoc, please add it properly. It should start with /**
and you need to list at least the parameters and return value.

Thierry

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

Re: [RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update

[Boris Brezillon]

On Mon, 20 Jul 2015 10:59:40 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
> > Add an ->apply() method to the pwm_ops struct to allow PWM drivers to
> > implement atomic update.
> > This method will be prefered 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 <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/pwm/core.c  | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
> >  include/linux/pwm.h |  26 +++++++++++++
> >  2 files changed, 124 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 30631f5..6dafd8e 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -238,8 +238,9 @@ 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->ops->apply &&
> > +	    (!chip->ops->config || !chip->ops->enable ||
> > +	     !chip->ops->disable)) || !chip->npwm)
> >  		return -EINVAL;
> 
> This is becoming really unreadable, perhaps split it into two checks, or
> even split out the sanity check on the ops into a separate function to
> make the negations easier to read:
> 
> 	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;
> 	}
> 
> and then use this:
> 
> 	if (!chip || !chip->dev || !chip->ops || !chip->npwm)
> 		return -EINVAL;
> 
> 	if (!pwm_ops_check(chip->ops))
> 		return -EINVAL;
> 

Sure, I'll change that to make it more readable.

> >  	mutex_lock(&pwm_lock);
> > @@ -430,7 +431,17 @@ 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->state;
> 
> Shouldn't this use pwm_get_state()?

Yes, I'll fix all of them

[...]

> 
> > +
> > +		state.enabled = true;
> > +		err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
> 
> There should be a space between the above two lines.

I'll add an empty line.


> 
> >  
> > +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->state = *state;
> 
> Maybe we want pwm_set_state() for this?

I'm not opposed to the addition of the pwm_set_state() function as long
as it's a private one: I don't want to let PMW drivers or users mess up
with the current 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 b47244a..7e99679 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
> >  	return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
> >  }
> >  
> > +/*
> > + * pwm_apply_state - apply a new state to the PWM device
> > + */
> > +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
> 
> If you add kerneldoc, please add it properly. It should start with /**
> and you need to list at least the parameters and return value.

Yes, I'll fix that.
BTW, I remember that you were expecting another name for this function
(pwm_update IIRC).



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

Re: [RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update

[Thierry Reding]

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

On Mon, Jul 20, 2015 at 11:48:27AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:59:40 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
[...]
> > > +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->state = *state;
> > 
> > Maybe we want pwm_set_state() for this?
> 
> I'm not opposed to the addition of the pwm_set_state() function as long
> as it's a private one: I don't want to let PMW drivers or users mess up
> with the current PWM state.

Yeah, it could be a static function in core.c. What I want to avoid is
having to change a bunch of code if ever state assignment becomes
something other than merely copying a structure.

[...]
> > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > index b47244a..7e99679 100644
> > > --- a/include/linux/pwm.h
> > > +++ b/include/linux/pwm.h
> > > @@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
> > >  	return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
> > >  }
> > >  
> > > +/*
> > > + * pwm_apply_state - apply a new state to the PWM device
> > > + */
> > > +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
> > 
> > If you add kerneldoc, please add it properly. It should start with /**
> > and you need to list at least the parameters and return value.
> 
> Yes, I'll fix that.
> BTW, I remember that you were expecting another name for this function
> (pwm_update IIRC).

I don't mind the pwm_apply_state() name very much. It's pretty accurate
with regards to what it does.

Thierry

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

[RFC PATCH 12/15] pwm: rockchip: add initial state retrieval

[Boris Brezillon]

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

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

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 7d9cc90..11e932d 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 (*init)(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,17 @@ static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip,
 	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_init_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 +110,36 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
 	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_init_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;
+	tmp = do_div(tmp, clk_rate);
+	pwm->state.period = tmp;
+
+	tmp = readl(pc->base + pc->data->regs.duty);
+	tmp *= pc->data->prescaler * NSEC_PER_SEC;
+	tmp = do_div(tmp, clk_rate);
+	pwm->state.duty_cycle = tmp;
+
+	pc->data->init(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 +213,7 @@ static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 }
 
 static const struct pwm_ops rockchip_pwm_ops_v1 = {
+	.init_state = rockchip_pwm_init_state,
 	.config = rockchip_pwm_config,
 	.enable = rockchip_pwm_enable,
 	.disable = rockchip_pwm_disable,
@@ -178,6 +221,7 @@ static const struct pwm_ops rockchip_pwm_ops_v1 = {
 };
 
 static const struct pwm_ops rockchip_pwm_ops_v2 = {
+	.init_state = rockchip_pwm_init_state,
 	.config = rockchip_pwm_config,
 	.set_polarity = rockchip_pwm_set_polarity,
 	.enable = rockchip_pwm_enable,
@@ -195,6 +239,7 @@ static const struct rockchip_pwm_data pwm_data_v1 = {
 	.prescaler = 2,
 	.ops = &rockchip_pwm_ops_v1,
 	.set_enable = rockchip_pwm_set_enable_v1,
+	.init = rockchip_pwm_init_v1,
 };
 
 static const struct rockchip_pwm_data pwm_data_v2 = {
@@ -207,6 +252,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
 	.prescaler = 1,
 	.ops = &rockchip_pwm_ops_v2,
 	.set_enable = rockchip_pwm_set_enable_v2,
+	.init = rockchip_pwm_init_v2,
 };
 
 static const struct rockchip_pwm_data pwm_data_vop = {
@@ -219,6 +265,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
 	.prescaler = 1,
 	.ops = &rockchip_pwm_ops_v2,
 	.set_enable = rockchip_pwm_set_enable_v2,
+	.init = rockchip_pwm_init_v2,
 };
 
 static const struct of_device_id rockchip_pwm_dt_ids[] = {
-- 
1.9.1

Re: [RFC PATCH 12/15] pwm: rockchip: add initial state retrieval

[Heiko Stübner]

Hi Boris,

Am Mittwoch, 1. Juli 2015, 10:21:58 schrieb Boris Brezillon:
> Implement the ->init_state() function to expose initial state.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---

[...]

> @@ -98,6 +110,36 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
>  }
> 
> +static void rockchip_pwm_init_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;
> +	tmp = do_div(tmp, clk_rate);

I guess you want to have the division result here and not the remainder, so
-       tmp = do_div(tmp, clk_rate);
+       do_div(tmp, clk_rate);


> +	pwm->state.period = tmp;
> +
> +	tmp = readl(pc->base + pc->data->regs.duty);
> +	tmp *= pc->data->prescaler * NSEC_PER_SEC;
> +	tmp = do_div(tmp, clk_rate);

ditto

> +	pwm->state.duty_cycle = tmp;
> +
> +	pc->data->init(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 +213,7 @@ static void rockchip_pwm_disable(struct pwm_chip *chip,
> struct pwm_device *pwm) }
> 
>  static const struct pwm_ops rockchip_pwm_ops_v1 = {
> +	.init_state = rockchip_pwm_init_state,
>  	.config = rockchip_pwm_config,
>  	.enable = rockchip_pwm_enable,
>  	.disable = rockchip_pwm_disable,
> @@ -178,6 +221,7 @@ static const struct pwm_ops rockchip_pwm_ops_v1 = {
>  };
> 
>  static const struct pwm_ops rockchip_pwm_ops_v2 = {
> +	.init_state = rockchip_pwm_init_state,
>  	.config = rockchip_pwm_config,
>  	.set_polarity = rockchip_pwm_set_polarity,
>  	.enable = rockchip_pwm_enable,
> @@ -195,6 +239,7 @@ static const struct rockchip_pwm_data pwm_data_v1 = {
>  	.prescaler = 2,
>  	.ops = &rockchip_pwm_ops_v1,
>  	.set_enable = rockchip_pwm_set_enable_v1,
> +	.init = rockchip_pwm_init_v1,
>  };
> 
>  static const struct rockchip_pwm_data pwm_data_v2 = {
> @@ -207,6 +252,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
>  	.prescaler = 1,
>  	.ops = &rockchip_pwm_ops_v2,
>  	.set_enable = rockchip_pwm_set_enable_v2,
> +	.init = rockchip_pwm_init_v2,

you're referencing the v2 init here, but only add it in the next patch?
[pwm: rockchip: add support for atomic update]


>  };
> 
>  static const struct rockchip_pwm_data pwm_data_vop = {
> @@ -219,6 +265,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
>  	.prescaler = 1,
>  	.ops = &rockchip_pwm_ops_v2,
>  	.set_enable = rockchip_pwm_set_enable_v2,
> +	.init = rockchip_pwm_init_v2,

ditto


Heiko

Re: [RFC PATCH 12/15] pwm: rockchip: add initial state retrieval

[Boris Brezillon]

On Wed, 01 Jul 2015 23:44:46 +0200
Heiko Stübner <heiko@sntech.de> wrote:

> Hi Boris,
> 
> Am Mittwoch, 1. Juli 2015, 10:21:58 schrieb Boris Brezillon:
> > Implement the ->init_state() function to expose initial state.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> 
> [...]
> 
> > @@ -98,6 +110,36 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> > *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> >  }
> > 
> > +static void rockchip_pwm_init_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;
> > +	tmp = do_div(tmp, clk_rate);
> 
> I guess you want to have the division result here and not the remainder, so
> -       tmp = do_div(tmp, clk_rate);
> +       do_div(tmp, clk_rate);
> 

Oh crap. I make the same mistake over and over again.

[...]
> >  static const struct rockchip_pwm_data pwm_data_v2 = {
> > @@ -207,6 +252,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
> >  	.prescaler = 1,
> >  	.ops = &rockchip_pwm_ops_v2,
> >  	.set_enable = rockchip_pwm_set_enable_v2,
> > +	.init = rockchip_pwm_init_v2,
> 
> you're referencing the v2 init here, but only add it in the next patch?
> [pwm: rockchip: add support for atomic update]

Yep, this function should be added in this patch.

Thanks,

Boris


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

[RFC PATCH 13/15] pwm: rockchip: add support for atomic update

[Boris Brezillon]

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

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

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 11e932d..cbe619bf 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 (*init)(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;
@@ -88,14 +90,15 @@ static void rockchip_pwm_init_v1(struct pwm_chip *chip, struct pwm_device *pwm)
 }
 
 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;
@@ -110,6 +113,26 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
 	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_init_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;
+
+	enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
+
+	if ((val & enable_conf) = enable_conf)
+		pwm->state.polarity = PWM_POLARITY_INVERSED;
+}
+
 static void rockchip_pwm_init_state(struct pwm_chip *chip,
 				    struct pwm_device *pwm)
 {
@@ -146,7 +169,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);
 
@@ -163,15 +185,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;
 }
@@ -189,43 +204,51 @@ 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 = {
 	.init_state = rockchip_pwm_init_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 = {
 	.init_state = rockchip_pwm_init_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

Re: [RFC PATCH 13/15] pwm: rockchip: add support for atomic update

[Heiko Stübner]

Hi Boris,


Am Mittwoch, 1. Juli 2015, 10:21:59 schrieb Boris Brezillon:
> Implement the ->apply() function to add support for atomic update.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> @@ -110,6 +113,26 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
>  }
> 
> +static void rockchip_pwm_init_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;
> +
> +	enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> +
> +	if ((val & enable_conf) = enable_conf)
> +		pwm->state.polarity = PWM_POLARITY_INVERSED;

the inactive setting does not affect the polarity of the running pwm, only what 
to do when it gets turned off. Also PWM_DUTY_NEGATIVE is the "0" value for the 
bit so also is bad to compare against (and results in wrong readings). So I 
would suggest changing this like

-       enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
+       enable_conf = PWM_DUTY_POSITIVE;
 
-       if ((val & enable_conf) = enable_conf)
+       if ((val & enable_conf) != enable_conf)
 

> +}
> +
>  static void rockchip_pwm_init_state(struct pwm_chip *chip,
>  				    struct pwm_device *pwm)
>  {


Heiko

Re: [RFC PATCH 13/15] pwm: rockchip: add support for atomic update

[Boris Brezillon]

Hi Heiko,

On Wed, 01 Jul 2015 23:48:31 +0200
Heiko Stübner <heiko@sntech.de> wrote:

> Hi Boris,
> 
> 
> Am Mittwoch, 1. Juli 2015, 10:21:59 schrieb Boris Brezillon:
> > Implement the ->apply() function to add support for atomic update.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > @@ -110,6 +113,26 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> > *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> >  }
> > 
> > +static void rockchip_pwm_init_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;
> > +
> > +	enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> > +
> > +	if ((val & enable_conf) = enable_conf)
> > +		pwm->state.polarity = PWM_POLARITY_INVERSED;
> 
> the inactive setting does not affect the polarity of the running pwm, only what 
> to do when it gets turned off. Also PWM_DUTY_NEGATIVE is the "0" value for the 
> bit so also is bad to compare against (and results in wrong readings). So I 
> would suggest changing this like

Indeed

> 
> -       enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> +       enable_conf = PWM_DUTY_POSITIVE;
>  
> -       if ((val & enable_conf) = enable_conf)
> +       if ((val & enable_conf) != enable_conf)

Or just:

	if(val & PWM_DUTY_POSITIVE)


Thanks for the fix.

Best Regards,

Boris

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

[RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods

[Boris Brezillon]

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 <boris.brezillon@free-electrons.com>
---
 drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 12b4d9d..8159518 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -59,12 +59,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;
 }
 
@@ -79,11 +73,37 @@ 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);
+}
+
 static struct regulator_ops pwm_regulator_voltage_ops = {
 	.set_voltage_sel = pwm_regulator_set_voltage_sel,
 	.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_desc pwm_regulator_desc = {
-- 
1.9.1

Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods

[Heiko Stübner]

Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
> 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 <boris.brezillon@free-electrons.com>
> ---
>  drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/regulator/pwm-regulator.c
> b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -59,12 +59,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;
>  }
> 
> @@ -79,11 +73,37 @@ 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);
> +}

nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of tabs)

Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods

[Boris Brezillon]

Hi Heiko,

On Wed, 01 Jul 2015 13:58:09 +0200
Heiko Stübner <heiko@sntech.de> wrote:

> Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
> > 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 <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
> >  1 file changed, 26 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/regulator/pwm-regulator.c
> > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
> > --- a/drivers/regulator/pwm-regulator.c
> > +++ b/drivers/regulator/pwm-regulator.c
> > @@ -59,12 +59,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;
> >  }
> > 
> > @@ -79,11 +73,37 @@ 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);
> > +}
> 
> nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of tabs)

Yep, I noticed checkpatch warnings/errors before sending the patch, but
since this is just an RFC I decided to fix them for the next version ;-)

Best Regards,

Boris

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

Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods

[Heiko Stübner]

Am Mittwoch, 1. Juli 2015, 14:05:31 schrieb Boris Brezillon:
> Hi Heiko,
> 
> On Wed, 01 Jul 2015 13:58:09 +0200
> 
> Heiko Stübner <heiko@sntech.de> wrote:
> > Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
> > > 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 <boris.brezillon@free-electrons.com>
> > > ---
> > > 
> > >  drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
> > >  1 file changed, 26 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/regulator/pwm-regulator.c
> > > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
> > > --- a/drivers/regulator/pwm-regulator.c
> > > +++ b/drivers/regulator/pwm-regulator.c
> > > @@ -59,12 +59,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;
> > >  
> > >  }
> > > 
> > > @@ -79,11 +73,37 @@ 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);
> > > +}
> > 
> > nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of
> > tabs)
> Yep, I noticed checkpatch warnings/errors before sending the patch, but
> since this is just an RFC I decided to fix them for the next version ;-)

ok, so I'll just skip over any more style issues for now. Making my way 
through your series and trying it on my veyron right now :-)


Heiko

Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods

[Boris Brezillon]

On Wed, 01 Jul 2015 14:08:18 +0200
Heiko Stübner <heiko@sntech.de> wrote:

> Am Mittwoch, 1. Juli 2015, 14:05:31 schrieb Boris Brezillon:
> > Hi Heiko,
> > 
> > On Wed, 01 Jul 2015 13:58:09 +0200
> > 
> > Heiko Stübner <heiko@sntech.de> wrote:
> > > Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
> > > > 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 <boris.brezillon@free-electrons.com>
> > > > ---
> > > > 
> > > >  drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
> > > >  1 file changed, 26 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/regulator/pwm-regulator.c
> > > > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
> > > > --- a/drivers/regulator/pwm-regulator.c
> > > > +++ b/drivers/regulator/pwm-regulator.c
> > > > @@ -59,12 +59,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;
> > > >  
> > > >  }
> > > > 
> > > > @@ -79,11 +73,37 @@ 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);
> > > > +}
> > > 
> > > nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of
> > > tabs)
> > Yep, I noticed checkpatch warnings/errors before sending the patch, but
> > since this is just an RFC I decided to fix them for the next version ;-)
> 
> ok, so I'll just skip over any more style issues for now. Making my way 
> through your series and trying it on my veyron right now :-)

Also note that I haven't tested the series on a real board (just compile
tested) because I don't have the board with me right now, but I wanted
to post the RFC early so that we can discuss the concepts.

Anyway, any feedback on the implementation (including bug reports) is
welcome.

This is the version I actually tested on the veyron board:

https://github.com/bbrezillon/linux-rk/tree/rk-3.14

Best Regards,

Boris

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

Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods

[Mark Brown]

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

On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote:
> Implement the ->enable(), ->disable() and ->is_enabled methods and remove
> the PWM call in ->set_voltage_sel().

This doesn't apply, please check and resend.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods

[Boris Brezillon]

Hi Mark,

On Tue, 14 Jul 2015 11:50:19 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote:
> > Implement the ->enable(), ->disable() and ->is_enabled methods and remove
> > the PWM call in ->set_voltage_sel().
> 
> This doesn't apply, please check and resend.

This series was made on top of Linus' tree (4.2-rc1 IIRC) and patch 14
and 15 were not meant to be applied without Thierry's approval (they
depend on other changes in the PWM framework).
I can rebase them on top of linux-next (or just on top of Linus'
4.2-rc2) if Thierry is okay with that, but I don't think rebasing them
on your regulator's for-next branch is a good idea.

Thierry, Mark, let me know what you prefer.

Thanks,

Boris

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

Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods

[Mark Brown]

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

On Tue, Jul 14, 2015 at 01:02:22PM +0200, Boris Brezillon wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote:
> > > Implement the ->enable(), ->disable() and ->is_enabled methods and remove
> > > the PWM call in ->set_voltage_sel().

> > This doesn't apply, please check and resend.

> This series was made on top of Linus' tree (4.2-rc1 IIRC) and patch 14
> and 15 were not meant to be applied without Thierry's approval (they
> depend on other changes in the PWM framework).
> I can rebase them on top of linux-next (or just on top of Linus'
> 4.2-rc2) if Thierry is okay with that, but I don't think rebasing them
> on your regulator's for-next branch is a good idea.

What is the dependency for the enable patch?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods

[Boris Brezillon]

On Tue, 14 Jul 2015 12:08:19 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Tue, Jul 14, 2015 at 01:02:22PM +0200, Boris Brezillon wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote:
> > > > Implement the ->enable(), ->disable() and ->is_enabled methods and remove
> > > > the PWM call in ->set_voltage_sel().
> 
> > > This doesn't apply, please check and resend.
> 
> > This series was made on top of Linus' tree (4.2-rc1 IIRC) and patch 14
> > and 15 were not meant to be applied without Thierry's approval (they
> > depend on other changes in the PWM framework).
> > I can rebase them on top of linux-next (or just on top of Linus'
> > 4.2-rc2) if Thierry is okay with that, but I don't think rebasing them
> > on your regulator's for-next branch is a good idea.
> 
> What is the dependency for the enable patch?

Oh, indeed. This patch has no dependency on the PWM changes. I'll send
it on its own then.

Thanks,

Boris

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

[RFC PATCH 15/15] regulator: pwm: properly initialize the ->state field

[Boris Brezillon]

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

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 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 8159518..4e29717 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -31,10 +31,35 @@ struct pwm_voltages {
 	unsigned int dutycycle;
 };
 
+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;
 }
 
@@ -170,6 +195,7 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 	config.of_node = np;
 	config.dev = &pdev->dev;
 	config.driver_data = drvdata;
+	drvdata->state = -EINVAL;
 
 	drvdata->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(drvdata->pwm)) {
-- 
1.9.1

Re: [RFC PATCH 15/15] regulator: pwm: properly initialize the ->state field

[Mark Brown]

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

On Wed, Jul 01, 2015 at 10:22:01AM +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.

This looks like it'll need reworking on top of Lee's recent changes to
implement continuous voltage support for PWM regulators - we may be
calculating the values to set dynamically at runtime.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 15/15] regulator: pwm: properly initialize the ->state field

[Boris Brezillon]

On Tue, 14 Jul 2015 11:51:55 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Jul 01, 2015 at 10:22:01AM +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.
> 
> This looks like it'll need reworking on top of Lee's recent changes to
> implement continuous voltage support for PWM regulators - we may be
> calculating the values to set dynamically at runtime.

Yep, I was planning on doing that in the next iteration, but I'd like
to have Thierry's feedback before sending a new version.


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

[RFC PATCH 16/15] pwm: add informations about polarity, duty cycle and period to debugfs

[Heiko Stübner]

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>
---
might be nice to have too ;-)

 drivers/pwm/core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6dafd8e..79037a2 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -951,9 +951,18 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 		if (test_bit(PWMF_REQUESTED, &pwm->flags))
 			seq_puts(s, " requested");
 
-		if (pwm_is_enabled(pwm))
+		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");
 	}
 }
-- 
2.1.4

Re: [RFC PATCH 16/15] pwm: add informations about polarity, duty cycle and period to debugfs

[Boris Brezillon]

On Wed, 01 Jul 2015 23:50:46 +0200
Heiko Stübner <heiko@sntech.de> wrote:

> 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>
> ---
> might be nice to have too ;-)

Yes.

> 
>  drivers/pwm/core.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6dafd8e..79037a2 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -951,9 +951,18 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>  		if (test_bit(PWMF_REQUESTED, &pwm->flags))
>  			seq_puts(s, " requested");
>  
> -		if (pwm_is_enabled(pwm))
> +		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");

I would print those values even if the PWM is not enabled.



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

[PATCH] pwm: add informations about polarity, duty cycle and period to debugfs

[Heiko Stübner]

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>
---
now values are printed independent of the enable status :-)

 drivers/pwm/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6dafd8e..4360ebb 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -954,6 +954,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");
 	}
 }
-- 
2.1.4

Re: [RFC PATCH 00/15] pwm: add support for atomic update

[Heiko Stübner]

Hi Boris,

Am Mittwoch, 1. Juli 2015, 10:21:46 schrieb Boris Brezillon:
> Hello Thierry,
> 
> This series adds support for atomic PWM update, or ITO, the capability
> to update all the parameters of a PWM device (enabled/disabled, period,
> duty and polarity) in one go.
> 
> This implementation is still experimental, and I may have missed some key
> aspect, so any feedback are welcome.
> 
> Also note that I haven't protected the state update with any locking.
> That's because the existing config does not protect against concurrent
> access to a requested PWM device (see the pwm_config implementation).
> I guess the PWM framework assume the user will implement the proper locking
> scheme if it has to concurrently access the device.
> 
> The 5 first patches prepare the addition of the pwm_state concept, which
> will be used to allow atomic updates.
> The following patches introduce the pwm_state struct, initial state
> retrieval and atomic update concepts.
> 
> Patches 12 and 13 are showing how one can implement the initial state
> retrieval and atomic update features in a PWM driver (in this specific
> case I implemented it in the rockchip driver).
> 
> The last 2 patches are making use of those changes to improve the
> pwm-regulator driver (initializing the regulator state based on the
> initial PWM state).

at first I got very strange readings (very wrong values and wrong polarity), 
which resulted from the issues I pointed out in the replies to individual 
patches. After fixing these, the pwm read-back now returns exactly the expected 
values :-) .

And with the original voltage table from the Chromeos-devicetrees, the pwm-
regulator also returns the expected 1.2V that coreboot initially set.


Heiko

Re: [RFC PATCH 00/15] pwm: add support for atomic update

[Boris Brezillon]

On Wed, 01 Jul 2015 23:57:57 +0200
Heiko Stübner <heiko@sntech.de> wrote:

> Hi Boris,
> 
> Am Mittwoch, 1. Juli 2015, 10:21:46 schrieb Boris Brezillon:
> > Hello Thierry,
> > 
> > This series adds support for atomic PWM update, or ITO, the capability
> > to update all the parameters of a PWM device (enabled/disabled, period,
> > duty and polarity) in one go.
> > 
> > This implementation is still experimental, and I may have missed some key
> > aspect, so any feedback are welcome.
> > 
> > Also note that I haven't protected the state update with any locking.
> > That's because the existing config does not protect against concurrent
> > access to a requested PWM device (see the pwm_config implementation).
> > I guess the PWM framework assume the user will implement the proper locking
> > scheme if it has to concurrently access the device.
> > 
> > The 5 first patches prepare the addition of the pwm_state concept, which
> > will be used to allow atomic updates.
> > The following patches introduce the pwm_state struct, initial state
> > retrieval and atomic update concepts.
> > 
> > Patches 12 and 13 are showing how one can implement the initial state
> > retrieval and atomic update features in a PWM driver (in this specific
> > case I implemented it in the rockchip driver).
> > 
> > The last 2 patches are making use of those changes to improve the
> > pwm-regulator driver (initializing the regulator state based on the
> > initial PWM state).
> 
> at first I got very strange readings (very wrong values and wrong polarity), 
> which resulted from the issues I pointed out in the replies to individual 
> patches. After fixing these, the pwm read-back now returns exactly the expected 
> values :-) .

Sorry about that, as I said I only compile tested the series :-/.
Anyway, thanks for providing fixes for these bugs, they'll be applied
in the next version.

> 
> And with the original voltage table from the Chromeos-devicetrees, the pwm-
> regulator also returns the expected 1.2V that coreboot initially set.

Great! And thanks for testing the patches.

Best Regards,

Boris

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

Re: [RFC PATCH 00/15] pwm: add support for atomic update

[Uwe Kleine-König]

Hello Boris,

On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
> This series adds support for atomic PWM update, or ITO, the capability
> to update all the parameters of a PWM device (enabled/disabled, period,
> duty and polarity) in one go.
on first reading the subject of your series I thought it was about
asserting that the newly set config is active before the call to
pwm_config (et al) returns. That's a problem I addressed a few times in
the past. I wonder if it's only me or if a different wording should be
used for "update all parameters with a single function call".

But other than that I like the series.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC PATCH 00/15] pwm: add support for atomic update

[Tomi Valkeinen]

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



On 02/07/15 10:03, Uwe Kleine-König wrote:
> Hello Boris,
> 
> On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
>> This series adds support for atomic PWM update, or ITO, the capability
>> to update all the parameters of a PWM device (enabled/disabled, period,
>> duty and polarity) in one go.
> on first reading the subject of your series I thought it was about
> asserting that the newly set config is active before the call to
> pwm_config (et al) returns. That's a problem I addressed a few times in
> the past. I wonder if it's only me or if a different wording should be
> used for "update all parameters with a single function call".

In my vocabulary "blocking" means that the work is done before the
function returns, and "atomic" means the work is done in one step.

 Tomi


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

Re: [RFC PATCH 00/15] pwm: add support for atomic update

[Uwe Kleine-König]

On Thu, Jul 02, 2015 at 10:17:00AM +0300, Tomi Valkeinen wrote:
> 
> 
> On 02/07/15 10:03, Uwe Kleine-König wrote:
> > Hello Boris,
> > 
> > On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
> >> This series adds support for atomic PWM update, or ITO, the capability
> >> to update all the parameters of a PWM device (enabled/disabled, period,
> >> duty and polarity) in one go.
> > on first reading the subject of your series I thought it was about
> > asserting that the newly set config is active before the call to
> > pwm_config (et al) returns. That's a problem I addressed a few times in
> > the past. I wonder if it's only me or if a different wording should be
> > used for "update all parameters with a single function call".
> 
> In my vocabulary "blocking" means that the work is done before the
> function returns, and "atomic" means the work is done in one step.
blocking is IMHO something slightly different, maybe "synchronous" is a
good term for "done when the call returns".

For write(2) I'd say
 - blocking means to only return when the write request has reached the
   kernel, but not necessarily the medium. I.e. the caller doesn't need
   to care further; and
 - atomic means that the contents of two concurrent writers don't mix in
   the resulting file content; and
 - synchronous means that once write() returns the data is on the
   medium.

So atomic seems to be fine to use here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC PATCH 00/15] pwm: add support for atomic update

[Boris Brezillon]

Hi Uwe,

On Thu, 2 Jul 2015 09:03:43 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> Hello Boris,
> 
> On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
> > This series adds support for atomic PWM update, or ITO, the capability
> > to update all the parameters of a PWM device (enabled/disabled, period,
> > duty and polarity) in one go.
> on first reading the subject of your series I thought it was about
> asserting that the newly set config is active before the call to
> pwm_config (et al) returns. That's a problem I addressed a few times in
> the past. I wonder if it's only me or if a different wording should be
> used for "update all parameters with a single function call".

Yep, I can reword it differently.

> 
> But other than that I like the series.

Great!

Best Regards,

Boris


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

Re: [RFC PATCH 00/15] pwm: add support for atomic update

[Boris Brezillon]

Hi Thierry,

I'd like to send a new version of this series fixing the problems
reported by Heiko, but I remember you were not happy with the naming
convention I have chosen for the atomic update function.
Could you have a quick look at this series (I'm not asking for a
detailed review) and let me know which things you'd like me to rename.

Best Regards,

Boris

On Wed,  1 Jul 2015 10:21:46 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hello Thierry,
> 
> This series adds support for atomic PWM update, or ITO, the capability
> to update all the parameters of a PWM device (enabled/disabled, period,
> duty and polarity) in one go.
> 
> This implementation is still experimental, and I may have missed some key
> aspect, so any feedback are welcome.
> 
> Also note that I haven't protected the state update with any locking.
> That's because the existing config does not protect against concurrent
> access to a requested PWM device (see the pwm_config implementation).
> I guess the PWM framework assume the user will implement the proper locking
> scheme if it has to concurrently access the device.
> 
> The 5 first patches prepare the addition of the pwm_state concept, which
> will be used to allow atomic updates.
> The following patches introduce the pwm_state struct, initial state
> retrieval and atomic update concepts.
> 
> Patches 12 and 13 are showing how one can implement the initial state
> retrieval and atomic update features in a PWM driver (in this specific
> case I implemented it in the rockchip driver).
> 
> The last 2 patches are making use of those changes to improve the
> pwm-regulator driver (initializing the regulator state based on the
> initial PWM state).
> 
> Best Regards,
> 
> Boris
> 
> Boris Brezillon (15):
>   pwm: add the pwm_is_enabled() helper
>   pwm: fix pwm_get_period and pwm_get_duty_cycle prototypes
>   pwm: add pwm_get_polarity helper function
>   pwm: make use of pwm_get_xxx helpers where appropriate
>   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
> 
>  drivers/leds/leds-pwm.c              |   2 +-
>  drivers/pwm/core.c                   | 136 ++++++++++++++++++++++++++++++-----
>  drivers/pwm/pwm-atmel-tcb.c          |   2 +-
>  drivers/pwm/pwm-atmel.c              |   6 +-
>  drivers/pwm/pwm-bcm-kona.c           |   7 +-
>  drivers/pwm/pwm-ep93xx.c             |   4 +-
>  drivers/pwm/pwm-imx.c                |   5 +-
>  drivers/pwm/pwm-mxs.c                |   4 +-
>  drivers/pwm/pwm-pxa.c                |   2 +-
>  drivers/pwm/pwm-renesas-tpu.c        |   2 +-
>  drivers/pwm/pwm-rockchip.c           | 122 ++++++++++++++++++++++++-------
>  drivers/pwm/pwm-sun4i.c              |   3 +-
>  drivers/pwm/pwm-tegra.c              |   6 +-
>  drivers/pwm/pwm-tiecap.c             |  10 +--
>  drivers/pwm/pwm-tiehrpwm.c           |   6 +-
>  drivers/pwm/sysfs.c                  |  13 ++--
>  drivers/regulator/pwm-regulator.c    |  60 ++++++++++++++--
>  drivers/video/backlight/lm3630a_bl.c |   4 +-
>  drivers/video/backlight/pwm_bl.c     |   6 +-
>  drivers/video/fbdev/ssd1307fb.c      |   2 +-
>  include/linux/pwm.h                  |  82 ++++++++++++++++++---
>  21 files changed, 380 insertions(+), 104 deletions(-)
> 



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

Re: [RFC PATCH 00/15] pwm: add support for atomic update

[Thierry Reding]

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

On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
> Hello Thierry,
> 
> This series adds support for atomic PWM update, or ITO, the capability
> to update all the parameters of a PWM device (enabled/disabled, period,
> duty and polarity) in one go.
> 
> This implementation is still experimental, and I may have missed some key
> aspect, so any feedback are welcome.
> 
> Also note that I haven't protected the state update with any locking.
> That's because the existing config does not protect against concurrent
> access to a requested PWM device (see the pwm_config implementation).
> I guess the PWM framework assume the user will implement the proper locking
> scheme if it has to concurrently access the device.

Actually the reason why no protection is needed is because there can
ever only be a single user of a PWM channel. pwm_request() will only
hand out a handle to the PWM channel if it hasn't been requested
before.

And in the unlikely case where a single user wants to use the same PWM
channel from different threads, then it will have to use locking for
other reasons already, so accesses to the PWM can be serialized using
the same mechanism.

Thierry

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