All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marco Felsch <m.felsch@pengutronix.de>
To: thierry.reding@gmail.com, u.kleine-koenig@pengutronix.de,
	lee.jones@linaro.org, shawnguo@kernel.org,
	s.hauer@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
	Anson.Huang@nxp.com, michal.vokac@ysoft.com,
	l.majewski@majess.pl
Cc: linux-pwm@vger.kernel.org, kernel@pengutronix.de,
	linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 1/5] pwm: imx27: enable clock unconditional for register access
Date: Fri, 25 Sep 2020 17:53:26 +0200	[thread overview]
Message-ID: <20200925155330.32301-2-m.felsch@pengutronix.de> (raw)
In-Reply-To: <20200925155330.32301-1-m.felsch@pengutronix.de>

The current implementation enables the clock if the current PWM state
is '!enabled' to ensure the register access and left the clock on if the
new state is 'enabled'. Further apply calls don't enable the clock since
they relying on the fact the the clock is already running. Change this
behaviour since it is not very intuitive.

This commit changes this behaviour. Now the clocks are unconditional
enabled/disabled before/after the register access. If the PWM should be
turned on (state.enabled) we enable the clock again and vice versa if
the PWM should be turned off (!state.enabled).

Therefore I added the enable member to the driver state struct since
the usage of cstate and pwm_get_state() is a layer violation. I removed
this violation while on it.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- use enable var which can be shared later on
- remove cstate and pwm_get_state() layer violation
- explicite enable/disable the clock twice if the pwm should be
  enabled/disabled rather than tracking the clock usage within the
  pwm_imx27_clk_prepare_enable() state.
- rename commit message

 drivers/pwm/pwm-imx27.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index c50d453552bd..7edac4ac6395 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -91,6 +91,7 @@ struct pwm_imx27_chip {
 	 * value to return in that case.
 	 */
 	unsigned int duty_cycle;
+	bool enabled;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -217,13 +218,14 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	unsigned long period_cycles, duty_cycles, prescale;
 	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
-	struct pwm_state cstate;
 	unsigned long long c;
 	unsigned long long clkrate;
 	int ret;
 	u32 cr;
 
-	pwm_get_state(pwm, &cstate);
+	ret = pwm_imx27_clk_prepare_enable(imx);
+	if (ret)
+		return ret;
 
 	clkrate = clk_get_rate(imx->clk_per);
 	c = clkrate * state->period;
@@ -251,15 +253,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * Wait for a free FIFO slot if the PWM is already enabled, and flush
 	 * the FIFO if the PWM was disabled and is about to be enabled.
 	 */
-	if (cstate.enabled) {
+	if (imx->enabled)
 		pwm_imx27_wait_fifo_slot(chip, pwm);
-	} else {
-		ret = pwm_imx27_clk_prepare_enable(imx);
-		if (ret)
-			return ret;
-
+	else
 		pwm_imx27_sw_reset(chip);
-	}
 
 	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
@@ -284,10 +281,21 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writel(cr, imx->mmio_base + MX3_PWMCR);
 
-	if (!state->enabled)
-		pwm_imx27_clk_disable_unprepare(imx);
+	if (imx->enabled != state->enabled) {
+		if (state->enabled) {
+			ret = pwm_imx27_clk_prepare_enable(imx);
+			if (ret)
+				goto out;
+		} else {
+			pwm_imx27_clk_disable_unprepare(imx);
+		}
+		imx->enabled = state->enabled;
+	}
 
-	return 0;
+out:
+	pwm_imx27_clk_disable_unprepare(imx);
+
+	return ret;
 }
 
 static const struct pwm_ops pwm_imx27_ops = {
-- 
2.20.1


WARNING: multiple messages have this Message-ID (diff)
From: Marco Felsch <m.felsch@pengutronix.de>
To: thierry.reding@gmail.com, u.kleine-koenig@pengutronix.de,
	lee.jones@linaro.org, shawnguo@kernel.org,
	s.hauer@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
	Anson.Huang@nxp.com, michal.vokac@ysoft.com,
	l.majewski@majess.pl
Cc: linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	kernel@pengutronix.de
Subject: [PATCH v2 1/5] pwm: imx27: enable clock unconditional for register access
Date: Fri, 25 Sep 2020 17:53:26 +0200	[thread overview]
Message-ID: <20200925155330.32301-2-m.felsch@pengutronix.de> (raw)
In-Reply-To: <20200925155330.32301-1-m.felsch@pengutronix.de>

The current implementation enables the clock if the current PWM state
is '!enabled' to ensure the register access and left the clock on if the
new state is 'enabled'. Further apply calls don't enable the clock since
they relying on the fact the the clock is already running. Change this
behaviour since it is not very intuitive.

This commit changes this behaviour. Now the clocks are unconditional
enabled/disabled before/after the register access. If the PWM should be
turned on (state.enabled) we enable the clock again and vice versa if
the PWM should be turned off (!state.enabled).

Therefore I added the enable member to the driver state struct since
the usage of cstate and pwm_get_state() is a layer violation. I removed
this violation while on it.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- use enable var which can be shared later on
- remove cstate and pwm_get_state() layer violation
- explicite enable/disable the clock twice if the pwm should be
  enabled/disabled rather than tracking the clock usage within the
  pwm_imx27_clk_prepare_enable() state.
- rename commit message

 drivers/pwm/pwm-imx27.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index c50d453552bd..7edac4ac6395 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -91,6 +91,7 @@ struct pwm_imx27_chip {
 	 * value to return in that case.
 	 */
 	unsigned int duty_cycle;
+	bool enabled;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -217,13 +218,14 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	unsigned long period_cycles, duty_cycles, prescale;
 	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
-	struct pwm_state cstate;
 	unsigned long long c;
 	unsigned long long clkrate;
 	int ret;
 	u32 cr;
 
-	pwm_get_state(pwm, &cstate);
+	ret = pwm_imx27_clk_prepare_enable(imx);
+	if (ret)
+		return ret;
 
 	clkrate = clk_get_rate(imx->clk_per);
 	c = clkrate * state->period;
@@ -251,15 +253,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * Wait for a free FIFO slot if the PWM is already enabled, and flush
 	 * the FIFO if the PWM was disabled and is about to be enabled.
 	 */
-	if (cstate.enabled) {
+	if (imx->enabled)
 		pwm_imx27_wait_fifo_slot(chip, pwm);
-	} else {
-		ret = pwm_imx27_clk_prepare_enable(imx);
-		if (ret)
-			return ret;
-
+	else
 		pwm_imx27_sw_reset(chip);
-	}
 
 	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
@@ -284,10 +281,21 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writel(cr, imx->mmio_base + MX3_PWMCR);
 
-	if (!state->enabled)
-		pwm_imx27_clk_disable_unprepare(imx);
+	if (imx->enabled != state->enabled) {
+		if (state->enabled) {
+			ret = pwm_imx27_clk_prepare_enable(imx);
+			if (ret)
+				goto out;
+		} else {
+			pwm_imx27_clk_disable_unprepare(imx);
+		}
+		imx->enabled = state->enabled;
+	}
 
-	return 0;
+out:
+	pwm_imx27_clk_disable_unprepare(imx);
+
+	return ret;
 }
 
 static const struct pwm_ops pwm_imx27_ops = {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-09-25 15:53 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-25 15:53 [PATCH v2 0/5] PWM i.MX27 fix disabled state for inverted signals Marco Felsch
2020-09-25 15:53 ` Marco Felsch
2020-09-25 15:53 ` Marco Felsch [this message]
2020-09-25 15:53   ` [PATCH v2 1/5] pwm: imx27: enable clock unconditional for register access Marco Felsch
2020-09-26 13:28   ` Uwe Kleine-König
2020-09-26 13:28     ` Uwe Kleine-König
2020-09-26 13:48   ` Uwe Kleine-König
2020-09-26 13:48     ` Uwe Kleine-König
2020-09-28  5:52     ` Marco Felsch
2020-09-28  5:52       ` Marco Felsch
2020-09-25 15:53 ` [PATCH v2 2/5] pwm: imx27: move constant PWMCR register values into probe Marco Felsch
2020-09-25 15:53   ` Marco Felsch
2020-09-26 13:46   ` Uwe Kleine-König
2020-09-26 13:46     ` Uwe Kleine-König
2020-09-28  5:50     ` Marco Felsch
2020-09-28  5:50       ` Marco Felsch
2020-09-25 15:53 ` [PATCH v2 3/5] pwm: imx27: reset the PWM if it is not running Marco Felsch
2020-09-25 15:53   ` Marco Felsch
2020-09-28  7:30   ` Uwe Kleine-König
2020-09-28  7:30     ` Uwe Kleine-König
2020-09-28  9:29     ` Marco Felsch
2020-09-28  9:29       ` Marco Felsch
2020-09-25 15:53 ` [PATCH v2 4/5] pwm: imx27: fix disable state for inverted PWMs Marco Felsch
2020-09-25 15:53   ` Marco Felsch
2020-09-28  7:47   ` Uwe Kleine-König
2020-09-28  7:47     ` Uwe Kleine-König
2020-09-28  9:52     ` Marco Felsch
2020-09-28  9:52       ` Marco Felsch
2020-09-28 19:06       ` Uwe Kleine-König
2020-09-28 19:06         ` Uwe Kleine-König
2020-09-29  5:23         ` Marco Felsch
2020-09-29  5:23           ` Marco Felsch
2020-09-25 15:53 ` [PATCH v2 5/5] pwm: imx27: wait till the duty cycle is applied Marco Felsch
2020-09-25 15:53   ` Marco Felsch
2020-09-28  8:04   ` Uwe Kleine-König
2020-09-28  8:04     ` Uwe Kleine-König
2020-09-28  9:59     ` Marco Felsch
2020-09-28  9:59       ` Marco Felsch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200925155330.32301-2-m.felsch@pengutronix.de \
    --to=m.felsch@pengutronix.de \
    --cc=Anson.Huang@nxp.com \
    --cc=festevam@gmail.com \
    --cc=kernel@pengutronix.de \
    --cc=l.majewski@majess.pl \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-pwm@vger.kernel.org \
    --cc=michal.vokac@ysoft.com \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.