* [PATCH v2 0/6] pwm: renesas-tpu: Various improvements
@ 2022-04-20 12:12 Uwe Kleine-König
2022-04-20 12:12 ` [PATCH v2 1/6] pwm: renesas-tpu: Make use of dev_err_probe() Uwe Kleine-König
` (7 more replies)
0 siblings, 8 replies; 14+ messages in thread
From: Uwe Kleine-König @ 2022-04-20 12:12 UTC (permalink / raw)
To: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Thierry Reding, Lee Jones
Cc: linux-renesas-soc, linux-pwm, kernel
Hello,
I reworked the series to improve the renesas-tpu PWM driver after
feedback I got from Geert. The changes are:
- Add Geert's tags for the first two patches
(I didn't add them for the other patches as I changed these enough to
not carry the tags forward.)
- Use tpu_pwm_disable instead chip->ops->disable (which is NULL now)
fixing a crash in v1.
- The calculation now uses a switch instead of ilog2.
Looking at the compiled code for ARM this is more effective and I
assume the same holds true for other platforms.
Thanks again to Geert for his valuable feedback and testing.
Best regards
Uwe
Uwe Kleine-König (6):
pwm: renesas-tpu: Make use of dev_err_probe()
pwm: renesas-tpu: Make use of devm functions
pwm: renesas-tpu: Implement .apply() callback
pwm: renesas-tpu: Rename variables to match the usual naming
pwm: renesas-tpu: Improve maths to compute register settings
pwm: renesas-tpu: Improve precision of period and duty_cycle
calculation
drivers/pwm/pwm-renesas-tpu.c | 317 +++++++++++++++++++---------------
1 file changed, 180 insertions(+), 137 deletions(-)
base-commit: 3123109284176b1532874591f7c81f3837bbdc17
--
2.35.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/6] pwm: renesas-tpu: Make use of dev_err_probe()
2022-04-20 12:12 [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Uwe Kleine-König
@ 2022-04-20 12:12 ` Uwe Kleine-König
2022-04-20 12:12 ` [PATCH v2 2/6] pwm: renesas-tpu: Make use of devm functions Uwe Kleine-König
` (6 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Uwe Kleine-König @ 2022-04-20 12:12 UTC (permalink / raw)
To: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Thierry Reding, Lee Jones
Cc: linux-renesas-soc, linux-pwm, kernel
The added benefit is that the error code is mentioned in the error message
and its usage is a bit more compact than open coding it. This also
improves behaviour in case devm_clk_get() returns -EPROBE_DEFER.
While touching this code, consistently start error messages with upper
case.
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/pwm/pwm-renesas-tpu.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index 4381df90a527..c3ae78e37789 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -398,10 +398,8 @@ static int tpu_probe(struct platform_device *pdev)
return PTR_ERR(tpu->base);
tpu->clk = devm_clk_get(&pdev->dev, NULL);
- if (IS_ERR(tpu->clk)) {
- dev_err(&pdev->dev, "cannot get clock\n");
- return PTR_ERR(tpu->clk);
- }
+ if (IS_ERR(tpu->clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(tpu->clk), "Failed to get clock\n");
/* Initialize and register the device. */
platform_set_drvdata(pdev, tpu);
@@ -414,9 +412,8 @@ static int tpu_probe(struct platform_device *pdev)
ret = pwmchip_add(&tpu->chip);
if (ret < 0) {
- dev_err(&pdev->dev, "failed to register PWM chip\n");
pm_runtime_disable(&pdev->dev);
- return ret;
+ return dev_err_probe(&pdev->dev, ret, "Failed to register PWM chip\n");
}
return 0;
--
2.35.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/6] pwm: renesas-tpu: Make use of devm functions
2022-04-20 12:12 [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Uwe Kleine-König
2022-04-20 12:12 ` [PATCH v2 1/6] pwm: renesas-tpu: Make use of dev_err_probe() Uwe Kleine-König
@ 2022-04-20 12:12 ` Uwe Kleine-König
2022-04-20 12:12 ` [PATCH v2 3/6] pwm: renesas-tpu: Implement .apply() callback Uwe Kleine-König
` (5 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Uwe Kleine-König @ 2022-04-20 12:12 UTC (permalink / raw)
To: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Thierry Reding, Lee Jones
Cc: linux-renesas-soc, linux-pwm, kernel
This simplifies an error path in .probe() and allows to drop the .remove()
function.
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/pwm/pwm-renesas-tpu.c | 22 +++++-----------------
1 file changed, 5 insertions(+), 17 deletions(-)
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index c3ae78e37789..12376988c70e 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -408,24 +408,13 @@ static int tpu_probe(struct platform_device *pdev)
tpu->chip.ops = &tpu_pwm_ops;
tpu->chip.npwm = TPU_CHANNEL_MAX;
- pm_runtime_enable(&pdev->dev);
+ ret = devm_pm_runtime_enable(&pdev->dev);
+ if (ret < 0)
+ return dev_err_probe(&pdev->dev, ret, "Failed to enable runtime PM\n");
- ret = pwmchip_add(&tpu->chip);
- if (ret < 0) {
- pm_runtime_disable(&pdev->dev);
+ ret = devm_pwmchip_add(&pdev->dev, &tpu->chip);
+ if (ret < 0)
return dev_err_probe(&pdev->dev, ret, "Failed to register PWM chip\n");
- }
-
- return 0;
-}
-
-static int tpu_remove(struct platform_device *pdev)
-{
- struct tpu_device *tpu = platform_get_drvdata(pdev);
-
- pwmchip_remove(&tpu->chip);
-
- pm_runtime_disable(&pdev->dev);
return 0;
}
@@ -444,7 +433,6 @@ MODULE_DEVICE_TABLE(of, tpu_of_table);
static struct platform_driver tpu_driver = {
.probe = tpu_probe,
- .remove = tpu_remove,
.driver = {
.name = "renesas-tpu-pwm",
.of_match_table = of_match_ptr(tpu_of_table),
--
2.35.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/6] pwm: renesas-tpu: Implement .apply() callback
2022-04-20 12:12 [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Uwe Kleine-König
2022-04-20 12:12 ` [PATCH v2 1/6] pwm: renesas-tpu: Make use of dev_err_probe() Uwe Kleine-König
2022-04-20 12:12 ` [PATCH v2 2/6] pwm: renesas-tpu: Make use of devm functions Uwe Kleine-König
@ 2022-04-20 12:12 ` Uwe Kleine-König
2022-04-20 18:07 ` Geert Uytterhoeven
2022-04-20 12:12 ` [PATCH v2 4/6] pwm: renesas-tpu: Rename variables to match the usual naming Uwe Kleine-König
` (4 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Uwe Kleine-König @ 2022-04-20 12:12 UTC (permalink / raw)
To: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Thierry Reding, Lee Jones
Cc: linux-renesas-soc, linux-pwm, kernel
To eventually get rid of all legacy drivers convert this driver to the
modern world implementing .apply().
As pwm->state might not be updated in tpu_pwm_apply() before calling
tpu_pwm_config(), an additional parameter is needed for tpu_pwm_config()
to not change the implemented logic.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/pwm/pwm-renesas-tpu.c | 44 ++++++++++++++++++++++++++++++-----
1 file changed, 38 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index 12376988c70e..bebae65a6ed7 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -242,7 +242,7 @@ static void tpu_pwm_free(struct pwm_chip *chip, struct pwm_device *_pwm)
}
static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm,
- int duty_ns, int period_ns)
+ int duty_ns, int period_ns, bool enabled)
{
static const unsigned int prescalers[] = { 1, 4, 16, 64 };
struct tpu_pwm_device *pwm = pwm_get_chip_data(_pwm);
@@ -293,7 +293,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 (!pwm_is_enabled(_pwm))
+ if (!enabled)
return 0;
if (duty_only && pwm->timer_on) {
@@ -366,13 +366,45 @@ static void tpu_pwm_disable(struct pwm_chip *chip, struct pwm_device *_pwm)
tpu_pwm_timer_stop(pwm);
}
+static int tpu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ int err;
+ bool enabled = pwm->state.enabled;
+
+ if (state->polarity != pwm->state.polarity) {
+ if (enabled) {
+ tpu_pwm_disable(chip, pwm);
+ enabled = false;
+ }
+
+ err = tpu_pwm_set_polarity(chip, pwm, state->polarity);
+ if (err)
+ return err;
+ }
+
+ if (!state->enabled) {
+ if (enabled)
+ tpu_pwm_disable(chip, pwm);
+
+ return 0;
+ }
+
+ err = tpu_pwm_config(pwm->chip, pwm,
+ state->duty_cycle, state->period, enabled);
+ if (err)
+ return err;
+
+ if (!enabled)
+ err = tpu_pwm_enable(chip, pwm);
+
+ return err;
+}
+
static const struct pwm_ops tpu_pwm_ops = {
.request = tpu_pwm_request,
.free = tpu_pwm_free,
- .config = tpu_pwm_config,
- .set_polarity = tpu_pwm_set_polarity,
- .enable = tpu_pwm_enable,
- .disable = tpu_pwm_disable,
+ .apply = tpu_pwm_apply,
.owner = THIS_MODULE,
};
--
2.35.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/6] pwm: renesas-tpu: Rename variables to match the usual naming
2022-04-20 12:12 [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Uwe Kleine-König
` (2 preceding siblings ...)
2022-04-20 12:12 ` [PATCH v2 3/6] pwm: renesas-tpu: Implement .apply() callback Uwe Kleine-König
@ 2022-04-20 12:12 ` Uwe Kleine-König
2022-04-20 12:25 ` Geert Uytterhoeven
2022-04-20 12:12 ` [PATCH v2 5/6] pwm: renesas-tpu: Improve maths to compute register settings Uwe Kleine-König
` (3 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Uwe Kleine-König @ 2022-04-20 12:12 UTC (permalink / raw)
To: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Thierry Reding, Lee Jones
Cc: linux-renesas-soc, linux-pwm, kernel
The driver used "pwm" for struct tpu_pwm_device pointers. This name is
usually only used for struct pwm_device pointers which this driver calls
"_pwm". So rename to the driver data pointers to "tpd" which then allows
to drop the underscore from "_pwm".
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/pwm/pwm-renesas-tpu.c | 172 +++++++++++++++++-----------------
1 file changed, 86 insertions(+), 86 deletions(-)
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index bebae65a6ed7..5bced2050ece 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -89,71 +89,71 @@ struct tpu_device {
#define to_tpu_device(c) container_of(c, struct tpu_device, chip)
-static void tpu_pwm_write(struct tpu_pwm_device *pwm, int reg_nr, u16 value)
+static void tpu_pwm_write(struct tpu_pwm_device *tpd, int reg_nr, u16 value)
{
- void __iomem *base = pwm->tpu->base + TPU_CHANNEL_OFFSET
- + pwm->channel * TPU_CHANNEL_SIZE;
+ void __iomem *base = tpd->tpu->base + TPU_CHANNEL_OFFSET
+ + tpd->channel * TPU_CHANNEL_SIZE;
iowrite16(value, base + reg_nr);
}
-static void tpu_pwm_set_pin(struct tpu_pwm_device *pwm,
+static void tpu_pwm_set_pin(struct tpu_pwm_device *tpd,
enum tpu_pin_state state)
{
static const char * const states[] = { "inactive", "PWM", "active" };
- dev_dbg(&pwm->tpu->pdev->dev, "%u: configuring pin as %s\n",
- pwm->channel, states[state]);
+ dev_dbg(&tpd->tpu->pdev->dev, "%u: configuring pin as %s\n",
+ tpd->channel, states[state]);
switch (state) {
case TPU_PIN_INACTIVE:
- tpu_pwm_write(pwm, TPU_TIORn,
- pwm->polarity == PWM_POLARITY_INVERSED ?
+ tpu_pwm_write(tpd, TPU_TIORn,
+ tpd->polarity == PWM_POLARITY_INVERSED ?
TPU_TIOR_IOA_1 : TPU_TIOR_IOA_0);
break;
case TPU_PIN_PWM:
- tpu_pwm_write(pwm, TPU_TIORn,
- pwm->polarity == PWM_POLARITY_INVERSED ?
+ tpu_pwm_write(tpd, TPU_TIORn,
+ tpd->polarity == PWM_POLARITY_INVERSED ?
TPU_TIOR_IOA_0_SET : TPU_TIOR_IOA_1_CLR);
break;
case TPU_PIN_ACTIVE:
- tpu_pwm_write(pwm, TPU_TIORn,
- pwm->polarity == PWM_POLARITY_INVERSED ?
+ tpu_pwm_write(tpd, TPU_TIORn,
+ tpd->polarity == PWM_POLARITY_INVERSED ?
TPU_TIOR_IOA_0 : TPU_TIOR_IOA_1);
break;
}
}
-static void tpu_pwm_start_stop(struct tpu_pwm_device *pwm, int start)
+static void tpu_pwm_start_stop(struct tpu_pwm_device *tpd, int start)
{
unsigned long flags;
u16 value;
- spin_lock_irqsave(&pwm->tpu->lock, flags);
- value = ioread16(pwm->tpu->base + TPU_TSTR);
+ spin_lock_irqsave(&tpd->tpu->lock, flags);
+ value = ioread16(tpd->tpu->base + TPU_TSTR);
if (start)
- value |= 1 << pwm->channel;
+ value |= 1 << tpd->channel;
else
- value &= ~(1 << pwm->channel);
+ value &= ~(1 << tpd->channel);
- iowrite16(value, pwm->tpu->base + TPU_TSTR);
- spin_unlock_irqrestore(&pwm->tpu->lock, flags);
+ iowrite16(value, tpd->tpu->base + TPU_TSTR);
+ spin_unlock_irqrestore(&tpd->tpu->lock, flags);
}
-static int tpu_pwm_timer_start(struct tpu_pwm_device *pwm)
+static int tpu_pwm_timer_start(struct tpu_pwm_device *tpd)
{
int ret;
- if (!pwm->timer_on) {
+ if (!tpd->timer_on) {
/* Wake up device and enable clock. */
- pm_runtime_get_sync(&pwm->tpu->pdev->dev);
- ret = clk_prepare_enable(pwm->tpu->clk);
+ pm_runtime_get_sync(&tpd->tpu->pdev->dev);
+ ret = clk_prepare_enable(tpd->tpu->clk);
if (ret) {
- dev_err(&pwm->tpu->pdev->dev, "cannot enable clock\n");
+ dev_err(&tpd->tpu->pdev->dev, "cannot enable clock\n");
return ret;
}
- pwm->timer_on = true;
+ tpd->timer_on = true;
}
/*
@@ -161,8 +161,8 @@ static int tpu_pwm_timer_start(struct tpu_pwm_device *pwm)
* completely. First drive the pin to the inactive state to avoid
* glitches.
*/
- tpu_pwm_set_pin(pwm, TPU_PIN_INACTIVE);
- tpu_pwm_start_stop(pwm, false);
+ tpu_pwm_set_pin(tpd, TPU_PIN_INACTIVE);
+ tpu_pwm_start_stop(tpd, false);
/*
* - Clear TCNT on TGRB match
@@ -172,80 +172,80 @@ static int tpu_pwm_timer_start(struct tpu_pwm_device *pwm)
* - Output 1 until TGRA, output 0 until TGRB (active high polarity
* - PWM mode
*/
- tpu_pwm_write(pwm, TPU_TCRn, TPU_TCR_CCLR_TGRB | TPU_TCR_CKEG_RISING |
- pwm->prescaler);
- tpu_pwm_write(pwm, TPU_TMDRn, TPU_TMDR_MD_PWM);
- tpu_pwm_set_pin(pwm, TPU_PIN_PWM);
- tpu_pwm_write(pwm, TPU_TGRAn, pwm->duty);
- tpu_pwm_write(pwm, TPU_TGRBn, pwm->period);
+ tpu_pwm_write(tpd, TPU_TCRn, TPU_TCR_CCLR_TGRB | TPU_TCR_CKEG_RISING |
+ tpd->prescaler);
+ tpu_pwm_write(tpd, TPU_TMDRn, TPU_TMDR_MD_PWM);
+ tpu_pwm_set_pin(tpd, TPU_PIN_PWM);
+ tpu_pwm_write(tpd, TPU_TGRAn, tpd->duty);
+ tpu_pwm_write(tpd, TPU_TGRBn, tpd->period);
- dev_dbg(&pwm->tpu->pdev->dev, "%u: TGRA 0x%04x TGRB 0x%04x\n",
- pwm->channel, pwm->duty, pwm->period);
+ dev_dbg(&tpd->tpu->pdev->dev, "%u: TGRA 0x%04x TGRB 0x%04x\n",
+ tpd->channel, tpd->duty, tpd->period);
/* Start the channel. */
- tpu_pwm_start_stop(pwm, true);
+ tpu_pwm_start_stop(tpd, true);
return 0;
}
-static void tpu_pwm_timer_stop(struct tpu_pwm_device *pwm)
+static void tpu_pwm_timer_stop(struct tpu_pwm_device *tpd)
{
- if (!pwm->timer_on)
+ if (!tpd->timer_on)
return;
/* Disable channel. */
- tpu_pwm_start_stop(pwm, false);
+ tpu_pwm_start_stop(tpd, false);
/* Stop clock and mark device as idle. */
- clk_disable_unprepare(pwm->tpu->clk);
- pm_runtime_put(&pwm->tpu->pdev->dev);
+ clk_disable_unprepare(tpd->tpu->clk);
+ pm_runtime_put(&tpd->tpu->pdev->dev);
- pwm->timer_on = false;
+ tpd->timer_on = false;
}
/* -----------------------------------------------------------------------------
* PWM API
*/
-static int tpu_pwm_request(struct pwm_chip *chip, struct pwm_device *_pwm)
+static int tpu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct tpu_device *tpu = to_tpu_device(chip);
- struct tpu_pwm_device *pwm;
+ struct tpu_pwm_device *tpd;
- if (_pwm->hwpwm >= TPU_CHANNEL_MAX)
+ if (pwm->hwpwm >= TPU_CHANNEL_MAX)
return -EINVAL;
- pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
- if (pwm == NULL)
+ tpd = kzalloc(sizeof(*tpd), GFP_KERNEL);
+ if (tpd == NULL)
return -ENOMEM;
- pwm->tpu = tpu;
- pwm->channel = _pwm->hwpwm;
- pwm->polarity = PWM_POLARITY_NORMAL;
- pwm->prescaler = 0;
- pwm->period = 0;
- pwm->duty = 0;
+ tpd->tpu = tpu;
+ tpd->channel = pwm->hwpwm;
+ tpd->polarity = PWM_POLARITY_NORMAL;
+ tpd->prescaler = 0;
+ tpd->period = 0;
+ tpd->duty = 0;
- pwm->timer_on = false;
+ tpd->timer_on = false;
- pwm_set_chip_data(_pwm, pwm);
+ pwm_set_chip_data(pwm, tpd);
return 0;
}
-static void tpu_pwm_free(struct pwm_chip *chip, struct pwm_device *_pwm)
+static void tpu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
{
- struct tpu_pwm_device *pwm = pwm_get_chip_data(_pwm);
+ struct tpu_pwm_device *tpd = pwm_get_chip_data(pwm);
- tpu_pwm_timer_stop(pwm);
- kfree(pwm);
+ tpu_pwm_timer_stop(tpd);
+ kfree(tpd);
}
-static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm,
+static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
int duty_ns, int period_ns, bool enabled)
{
static const unsigned int prescalers[] = { 1, 4, 16, 64 };
- struct tpu_pwm_device *pwm = pwm_get_chip_data(_pwm);
+ struct tpu_pwm_device *tpd = pwm_get_chip_data(pwm);
struct tpu_device *tpu = to_tpu_device(chip);
unsigned int prescaler;
bool duty_only = false;
@@ -285,29 +285,29 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm,
"rate %u, prescaler %u, period %u, duty %u\n",
clk_rate, prescalers[prescaler], period, duty);
- if (pwm->prescaler == prescaler && pwm->period == period)
+ if (tpd->prescaler == prescaler && tpd->period == period)
duty_only = true;
- pwm->prescaler = prescaler;
- pwm->period = period;
- pwm->duty = duty;
+ tpd->prescaler = prescaler;
+ tpd->period = period;
+ tpd->duty = duty;
/* If the channel is disabled we're done. */
if (!enabled)
return 0;
- if (duty_only && pwm->timer_on) {
+ if (duty_only && tpd->timer_on) {
/*
* If only the duty cycle changed and the timer is already
* running, there's no need to reconfigure it completely, Just
* modify the duty cycle.
*/
- tpu_pwm_write(pwm, TPU_TGRAn, pwm->duty);
- dev_dbg(&tpu->pdev->dev, "%u: TGRA 0x%04x\n", pwm->channel,
- pwm->duty);
+ tpu_pwm_write(tpd, TPU_TGRAn, tpd->duty);
+ dev_dbg(&tpu->pdev->dev, "%u: TGRA 0x%04x\n", tpd->channel,
+ tpd->duty);
} else {
/* Otherwise perform a full reconfiguration. */
- ret = tpu_pwm_timer_start(pwm);
+ ret = tpu_pwm_timer_start(tpd);
if (ret < 0)
return ret;
}
@@ -317,29 +317,29 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm,
* To avoid running the timer when not strictly required, handle
* 0% and 100% duty cycles as fixed levels and stop the timer.
*/
- tpu_pwm_set_pin(pwm, duty ? TPU_PIN_ACTIVE : TPU_PIN_INACTIVE);
- tpu_pwm_timer_stop(pwm);
+ tpu_pwm_set_pin(tpd, duty ? TPU_PIN_ACTIVE : TPU_PIN_INACTIVE);
+ tpu_pwm_timer_stop(tpd);
}
return 0;
}
-static int tpu_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *_pwm,
+static int tpu_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
enum pwm_polarity polarity)
{
- struct tpu_pwm_device *pwm = pwm_get_chip_data(_pwm);
+ struct tpu_pwm_device *tpd = pwm_get_chip_data(pwm);
- pwm->polarity = polarity;
+ tpd->polarity = polarity;
return 0;
}
-static int tpu_pwm_enable(struct pwm_chip *chip, struct pwm_device *_pwm)
+static int tpu_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
{
- struct tpu_pwm_device *pwm = pwm_get_chip_data(_pwm);
+ struct tpu_pwm_device *tpd = pwm_get_chip_data(pwm);
int ret;
- ret = tpu_pwm_timer_start(pwm);
+ ret = tpu_pwm_timer_start(tpd);
if (ret < 0)
return ret;
@@ -347,23 +347,23 @@ static int tpu_pwm_enable(struct pwm_chip *chip, struct pwm_device *_pwm)
* To avoid running the timer when not strictly required, handle 0% and
* 100% duty cycles as fixed levels and stop the timer.
*/
- if (pwm->duty == 0 || pwm->duty == pwm->period) {
- tpu_pwm_set_pin(pwm, pwm->duty ?
+ if (tpd->duty == 0 || tpd->duty == tpd->period) {
+ tpu_pwm_set_pin(tpd, tpd->duty ?
TPU_PIN_ACTIVE : TPU_PIN_INACTIVE);
- tpu_pwm_timer_stop(pwm);
+ tpu_pwm_timer_stop(tpd);
}
return 0;
}
-static void tpu_pwm_disable(struct pwm_chip *chip, struct pwm_device *_pwm)
+static void tpu_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
{
- struct tpu_pwm_device *pwm = pwm_get_chip_data(_pwm);
+ struct tpu_pwm_device *tpd = pwm_get_chip_data(pwm);
/* The timer must be running to modify the pin output configuration. */
- tpu_pwm_timer_start(pwm);
- tpu_pwm_set_pin(pwm, TPU_PIN_INACTIVE);
- tpu_pwm_timer_stop(pwm);
+ tpu_pwm_timer_start(tpd);
+ tpu_pwm_set_pin(tpd, TPU_PIN_INACTIVE);
+ tpu_pwm_timer_stop(tpd);
}
static int tpu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
--
2.35.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 5/6] pwm: renesas-tpu: Improve maths to compute register settings
2022-04-20 12:12 [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Uwe Kleine-König
` (3 preceding siblings ...)
2022-04-20 12:12 ` [PATCH v2 4/6] pwm: renesas-tpu: Rename variables to match the usual naming Uwe Kleine-König
@ 2022-04-20 12:12 ` Uwe Kleine-König
2022-04-20 18:08 ` Geert Uytterhoeven
2022-04-20 12:12 ` [PATCH v2 6/6] pwm: renesas-tpu: Improve precision of period and duty_cycle calculation Uwe Kleine-König
` (2 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Uwe Kleine-König @ 2022-04-20 12:12 UTC (permalink / raw)
To: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Thierry Reding, Lee Jones
Cc: linux-renesas-soc, linux-pwm, kernel
The newly computed register values are intended to exactly match the
previously computed values. The main improvement is that the prescaler
is computed without a loop that involves two divisions in each step.
This uses the fact, that prescalers[i] = 1 << (2 * i).
Assuming a moderately smart compiler, the needed number of divisions for
the case where the requested period is too big, is reduced from 5 to 2.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/pwm/pwm-renesas-tpu.c | 50 ++++++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 15 deletions(-)
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index 5bced2050ece..60ba7cf275c7 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -244,7 +244,6 @@ static void tpu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
int duty_ns, int period_ns, bool enabled)
{
- static const unsigned int prescalers[] = { 1, 4, 16, 64 };
struct tpu_pwm_device *tpd = pwm_get_chip_data(pwm);
struct tpu_device *tpu = to_tpu_device(chip);
unsigned int prescaler;
@@ -254,26 +253,47 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
u32 duty;
int ret;
+ clk_rate = clk_get_rate(tpu->clk);
+
+ period = clk_rate / (NSEC_PER_SEC / period_ns);
+
/*
- * Pick a prescaler to avoid overflowing the counter.
- * TODO: Pick the highest acceptable prescaler.
+ * Find the minimal prescaler in [0..3] such that
+ *
+ * period >> (2 * prescaler) < 0x10000
+ *
+ * This could be calculated using something like:
+ *
+ * prescaler = max(ilog2(period) / 2, 7) - 7;
+ *
+ * but given there are only four allowed results and that ilog2 isn't
+ * cheap on all platforms using a switch statement is more effective.
*/
- clk_rate = clk_get_rate(tpu->clk);
+ switch (period) {
+ case 1 ... 0xffff:
+ prescaler = 0;
+ break;
- for (prescaler = 0; prescaler < ARRAY_SIZE(prescalers); ++prescaler) {
- period = clk_rate / prescalers[prescaler]
- / (NSEC_PER_SEC / period_ns);
- if (period <= 0xffff)
- break;
- }
+ case 0x10000 ... 0x3ffff:
+ prescaler = 1;
+ break;
- if (prescaler == ARRAY_SIZE(prescalers) || period == 0) {
- dev_err(&tpu->pdev->dev, "clock rate mismatch\n");
- return -ENOTSUPP;
+ case 0x40000 ... 0xfffff:
+ prescaler = 2;
+ break;
+
+ case 0x100000 ... 0x3fffff:
+ prescaler = 3;
+ break;
+
+ default:
+ return -EINVAL;
}
+ period >>= 2 * prescaler;
+
if (duty_ns) {
- duty = clk_rate / prescalers[prescaler]
+ duty = (clk_rate >> 2 * prescaler)
/ (NSEC_PER_SEC / duty_ns);
if (duty > period)
return -EINVAL;
@@ -283,7 +303,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
dev_dbg(&tpu->pdev->dev,
"rate %u, prescaler %u, period %u, duty %u\n",
- clk_rate, prescalers[prescaler], period, duty);
+ clk_rate, 1 << (2 * prescaler), period, duty);
if (tpd->prescaler == prescaler && tpd->period == period)
duty_only = true;
--
2.35.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 6/6] pwm: renesas-tpu: Improve precision of period and duty_cycle calculation
2022-04-20 12:12 [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Uwe Kleine-König
` (4 preceding siblings ...)
2022-04-20 12:12 ` [PATCH v2 5/6] pwm: renesas-tpu: Improve maths to compute register settings Uwe Kleine-König
@ 2022-04-20 12:12 ` Uwe Kleine-König
2022-04-20 18:10 ` Geert Uytterhoeven
2022-04-20 18:12 ` [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Geert Uytterhoeven
2022-05-20 14:19 ` Thierry Reding
7 siblings, 1 reply; 14+ messages in thread
From: Uwe Kleine-König @ 2022-04-20 12:12 UTC (permalink / raw)
To: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Thierry Reding, Lee Jones
Cc: linux-renesas-soc, linux-pwm, kernel
Dividing by the result of a division looses precision. Consider for example
clk_rate = 33000000 and period_ns = 500001. Then
clk_rate / (NSEC_PER_SEC / period_ns)
has the exact value 16500.033, but in C this evaluates to 16508. It gets
worse for even bigger values of period_ns, so with period_ns = 500000001,
the exact result is 16500000.033 while in C we get 33000000.
For that reason use
clk_rate * period_ns / NSEC_PER_SEC
instead which doesn't suffer from this problem. To ensure this doesn't
overflow add a safeguard check for clk_rate.
Note that duty > period can never happen, so the respective check can be
dropped.
Incidentally this fixes a division by zero if period_ns > NSEC_PER_SEC.
Another side effect is that values bigger than INT_MAX for period and
duty_cyle are not wrongly discarded any more.
Fixes: 99b82abb0a35 ("pwm: Add Renesas TPU PWM driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/pwm/pwm-renesas-tpu.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index 60ba7cf275c7..9e6978c39788 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -242,20 +242,29 @@ static void tpu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
}
static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
- int duty_ns, int period_ns, bool enabled)
+ u64 duty_ns, u64 period_ns, bool enabled)
{
struct tpu_pwm_device *tpd = pwm_get_chip_data(pwm);
struct tpu_device *tpu = to_tpu_device(chip);
unsigned int prescaler;
bool duty_only = false;
u32 clk_rate;
- u32 period;
+ u64 period;
u32 duty;
int ret;
clk_rate = clk_get_rate(tpu->clk);
+ if (unlikely(clk_rate > NSEC_PER_SEC)) {
+ /*
+ * This won't happen in the nearer future, so this is only a
+ * safeguard to prevent the following calculation from
+ * overflowing. With this clk_rate * period_ns / NSEC_PER_SEC is
+ * not greater than period_ns and so fits into an u64.
+ */
+ return -EINVAL;
+ }
- period = clk_rate / (NSEC_PER_SEC / period_ns);
+ period = mul_u64_u64_div_u64(clk_rate, period_ns, NSEC_PER_SEC);
/*
* Find the minimal prescaler in [0..3] such that
@@ -292,18 +301,15 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
period >>= 2 * prescaler;
- if (duty_ns) {
- duty = (clk_rate >> 2 * prescaler)
- / (NSEC_PER_SEC / duty_ns);
- if (duty > period)
- return -EINVAL;
- } else {
+ if (duty_ns)
+ duty = mul_u64_u64_div_u64(clk_rate, duty_ns,
+ (u64)NSEC_PER_SEC << (2 * prescaler));
+ else
duty = 0;
- }
dev_dbg(&tpu->pdev->dev,
"rate %u, prescaler %u, period %u, duty %u\n",
- clk_rate, 1 << (2 * prescaler), period, duty);
+ clk_rate, 1 << (2 * prescaler), (u32)period, duty);
if (tpd->prescaler == prescaler && tpd->period == period)
duty_only = true;
--
2.35.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/6] pwm: renesas-tpu: Rename variables to match the usual naming
2022-04-20 12:12 ` [PATCH v2 4/6] pwm: renesas-tpu: Rename variables to match the usual naming Uwe Kleine-König
@ 2022-04-20 12:25 ` Geert Uytterhoeven
0 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2022-04-20 12:25 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Thierry Reding, Lee Jones, Linux-Renesas, Linux PWM List,
Sascha Hauer
On Wed, Apr 20, 2022 at 2:12 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> The driver used "pwm" for struct tpu_pwm_device pointers. This name is
> usually only used for struct pwm_device pointers which this driver calls
> "_pwm". So rename to the driver data pointers to "tpd" which then allows
> to drop the underscore from "_pwm".
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
This patch didn't change (compared to v1), so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/6] pwm: renesas-tpu: Implement .apply() callback
2022-04-20 12:12 ` [PATCH v2 3/6] pwm: renesas-tpu: Implement .apply() callback Uwe Kleine-König
@ 2022-04-20 18:07 ` Geert Uytterhoeven
0 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2022-04-20 18:07 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Laurent Pinchart, Simon Horman, Magnus Damm, Thierry Reding,
Lee Jones, Linux-Renesas, Linux PWM List, Sascha Hauer
On Wed, Apr 20, 2022 at 2:12 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> To eventually get rid of all legacy drivers convert this driver to the
> modern world implementing .apply().
>
> As pwm->state might not be updated in tpu_pwm_apply() before calling
> tpu_pwm_config(), an additional parameter is needed for tpu_pwm_config()
> to not change the implemented logic.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/6] pwm: renesas-tpu: Improve maths to compute register settings
2022-04-20 12:12 ` [PATCH v2 5/6] pwm: renesas-tpu: Improve maths to compute register settings Uwe Kleine-König
@ 2022-04-20 18:08 ` Geert Uytterhoeven
2022-06-19 20:03 ` Uwe Kleine-König
0 siblings, 1 reply; 14+ messages in thread
From: Geert Uytterhoeven @ 2022-04-20 18:08 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Thierry Reding, Lee Jones, Linux-Renesas, Linux PWM List,
Sascha Hauer
Hi Uwe,
On Wed, Apr 20, 2022 at 2:12 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> The newly computed register values are intended to exactly match the
> previously computed values. The main improvement is that the prescaler
> is computed without a loop that involves two divisions in each step.
> This uses the fact, that prescalers[i] = 1 << (2 * i).
>
> Assuming a moderately smart compiler, the needed number of divisions for
> the case where the requested period is too big, is reduced from 5 to 2.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Thanks for your patch!
> --- a/drivers/pwm/pwm-renesas-tpu.c
> +++ b/drivers/pwm/pwm-renesas-tpu.c
> @@ -244,7 +244,6 @@ static void tpu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> int duty_ns, int period_ns, bool enabled)
> {
> - static const unsigned int prescalers[] = { 1, 4, 16, 64 };
> struct tpu_pwm_device *tpd = pwm_get_chip_data(pwm);
> struct tpu_device *tpu = to_tpu_device(chip);
> unsigned int prescaler;
> @@ -254,26 +253,47 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> u32 duty;
> int ret;
>
> + clk_rate = clk_get_rate(tpu->clk);
> +
> + period = clk_rate / (NSEC_PER_SEC / period_ns);
> +
> /*
> - * Pick a prescaler to avoid overflowing the counter.
> - * TODO: Pick the highest acceptable prescaler.
> + * Find the minimal prescaler in [0..3] such that
> + *
> + * period >> (2 * prescaler) < 0x10000
scripts/checkpatch.pl:
WARNING: please, no space before tabs
> + *
> + * This could be calculated using something like:
> + *
> + * prescaler = max(ilog2(period) / 2, 7) - 7;
WARNING: please, no space before tabs
The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 6/6] pwm: renesas-tpu: Improve precision of period and duty_cycle calculation
2022-04-20 12:12 ` [PATCH v2 6/6] pwm: renesas-tpu: Improve precision of period and duty_cycle calculation Uwe Kleine-König
@ 2022-04-20 18:10 ` Geert Uytterhoeven
0 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2022-04-20 18:10 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Laurent Pinchart, Simon Horman, Magnus Damm, Thierry Reding,
Lee Jones, Linux-Renesas, Linux PWM List, Sascha Hauer
On Wed, Apr 20, 2022 at 2:12 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Dividing by the result of a division looses precision. Consider for example
> clk_rate = 33000000 and period_ns = 500001. Then
>
> clk_rate / (NSEC_PER_SEC / period_ns)
>
> has the exact value 16500.033, but in C this evaluates to 16508. It gets
> worse for even bigger values of period_ns, so with period_ns = 500000001,
> the exact result is 16500000.033 while in C we get 33000000.
>
> For that reason use
>
> clk_rate * period_ns / NSEC_PER_SEC
>
> instead which doesn't suffer from this problem. To ensure this doesn't
> overflow add a safeguard check for clk_rate.
>
> Note that duty > period can never happen, so the respective check can be
> dropped.
>
> Incidentally this fixes a division by zero if period_ns > NSEC_PER_SEC.
> Another side effect is that values bigger than INT_MAX for period and
> duty_cyle are not wrongly discarded any more.
>
> Fixes: 99b82abb0a35 ("pwm: Add Renesas TPU PWM driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/6] pwm: renesas-tpu: Various improvements
2022-04-20 12:12 [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Uwe Kleine-König
` (5 preceding siblings ...)
2022-04-20 12:12 ` [PATCH v2 6/6] pwm: renesas-tpu: Improve precision of period and duty_cycle calculation Uwe Kleine-König
@ 2022-04-20 18:12 ` Geert Uytterhoeven
2022-05-20 14:19 ` Thierry Reding
7 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2022-04-20 18:12 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Thierry Reding, Lee Jones, Linux-Renesas, Linux PWM List,
Sascha Hauer
Hi Uwe,
On Wed, Apr 20, 2022 at 2:12 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> I reworked the series to improve the renesas-tpu PWM driver after
> feedback I got from Geert. The changes are:
>
> - Add Geert's tags for the first two patches
> (I didn't add them for the other patches as I changed these enough to
> not carry the tags forward.)
> - Use tpu_pwm_disable instead chip->ops->disable (which is NULL now)
> fixing a crash in v1.
> - The calculation now uses a switch instead of ilog2.
> Looking at the compiled code for ARM this is more effective and I
> assume the same holds true for other platforms.
Display backlight on the R-Mobile A1-based Atmark Techno
Armadillo-800EVA still works fine, so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/6] pwm: renesas-tpu: Various improvements
2022-04-20 12:12 [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Uwe Kleine-König
` (6 preceding siblings ...)
2022-04-20 18:12 ` [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Geert Uytterhoeven
@ 2022-05-20 14:19 ` Thierry Reding
7 siblings, 0 replies; 14+ messages in thread
From: Thierry Reding @ 2022-05-20 14:19 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Laurent Pinchart, Simon Horman, Geert Uytterhoeven, Magnus Damm,
Lee Jones, linux-renesas-soc, linux-pwm, kernel
[-- Attachment #1: Type: text/plain, Size: 1401 bytes --]
On Wed, Apr 20, 2022 at 02:12:34PM +0200, Uwe Kleine-König wrote:
> Hello,
>
> I reworked the series to improve the renesas-tpu PWM driver after
> feedback I got from Geert. The changes are:
>
> - Add Geert's tags for the first two patches
> (I didn't add them for the other patches as I changed these enough to
> not carry the tags forward.)
> - Use tpu_pwm_disable instead chip->ops->disable (which is NULL now)
> fixing a crash in v1.
> - The calculation now uses a switch instead of ilog2.
> Looking at the compiled code for ARM this is more effective and I
> assume the same holds true for other platforms.
>
> Thanks again to Geert for his valuable feedback and testing.
>
> Best regards
> Uwe
>
> Uwe Kleine-König (6):
> pwm: renesas-tpu: Make use of dev_err_probe()
> pwm: renesas-tpu: Make use of devm functions
> pwm: renesas-tpu: Implement .apply() callback
> pwm: renesas-tpu: Rename variables to match the usual naming
> pwm: renesas-tpu: Improve maths to compute register settings
> pwm: renesas-tpu: Improve precision of period and duty_cycle
> calculation
>
> drivers/pwm/pwm-renesas-tpu.c | 317 +++++++++++++++++++---------------
> 1 file changed, 180 insertions(+), 137 deletions(-)
Applied, with the checkpatch warning in patch 5/6 addressed (I opted to
replace the tabs by spaces).
Thanks,
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/6] pwm: renesas-tpu: Improve maths to compute register settings
2022-04-20 18:08 ` Geert Uytterhoeven
@ 2022-06-19 20:03 ` Uwe Kleine-König
0 siblings, 0 replies; 14+ messages in thread
From: Uwe Kleine-König @ 2022-06-19 20:03 UTC (permalink / raw)
To: Geert Uytterhoeven, Thierry Reding
Cc: Linux PWM List, Geert Uytterhoeven, Magnus Damm, Linux-Renesas,
Laurent Pinchart, Sascha Hauer, Simon Horman, Lee Jones
[-- Attachment #1: Type: text/plain, Size: 2555 bytes --]
On Wed, Apr 20, 2022 at 08:08:34PM +0200, Geert Uytterhoeven wrote:
> Hi Uwe,
>
> On Wed, Apr 20, 2022 at 2:12 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > The newly computed register values are intended to exactly match the
> > previously computed values. The main improvement is that the prescaler
> > is computed without a loop that involves two divisions in each step.
> > This uses the fact, that prescalers[i] = 1 << (2 * i).
> >
> > Assuming a moderately smart compiler, the needed number of divisions for
> > the case where the requested period is too big, is reduced from 5 to 2.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> Thanks for your patch!
>
> > --- a/drivers/pwm/pwm-renesas-tpu.c
> > +++ b/drivers/pwm/pwm-renesas-tpu.c
> > @@ -244,7 +244,6 @@ static void tpu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > int duty_ns, int period_ns, bool enabled)
> > {
> > - static const unsigned int prescalers[] = { 1, 4, 16, 64 };
> > struct tpu_pwm_device *tpd = pwm_get_chip_data(pwm);
> > struct tpu_device *tpu = to_tpu_device(chip);
> > unsigned int prescaler;
> > @@ -254,26 +253,47 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > u32 duty;
> > int ret;
> >
> > + clk_rate = clk_get_rate(tpu->clk);
> > +
> > + period = clk_rate / (NSEC_PER_SEC / period_ns);
> > +
> > /*
> > - * Pick a prescaler to avoid overflowing the counter.
> > - * TODO: Pick the highest acceptable prescaler.
> > + * Find the minimal prescaler in [0..3] such that
> > + *
> > + * period >> (2 * prescaler) < 0x10000
>
> scripts/checkpatch.pl:
> WARNING: please, no space before tabs
>
> > + *
> > + * This could be calculated using something like:
> > + *
> > + * prescaler = max(ilog2(period) / 2, 7) - 7;
>
> WARNING: please, no space before tabs
>
> The rest LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
@Thierry: Assuming you agree to this patch, too: Should I resend for the
checkpack warning (I'd s/\t/ /), or do you want to fixup at apply
time?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-06-19 20:03 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-20 12:12 [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Uwe Kleine-König
2022-04-20 12:12 ` [PATCH v2 1/6] pwm: renesas-tpu: Make use of dev_err_probe() Uwe Kleine-König
2022-04-20 12:12 ` [PATCH v2 2/6] pwm: renesas-tpu: Make use of devm functions Uwe Kleine-König
2022-04-20 12:12 ` [PATCH v2 3/6] pwm: renesas-tpu: Implement .apply() callback Uwe Kleine-König
2022-04-20 18:07 ` Geert Uytterhoeven
2022-04-20 12:12 ` [PATCH v2 4/6] pwm: renesas-tpu: Rename variables to match the usual naming Uwe Kleine-König
2022-04-20 12:25 ` Geert Uytterhoeven
2022-04-20 12:12 ` [PATCH v2 5/6] pwm: renesas-tpu: Improve maths to compute register settings Uwe Kleine-König
2022-04-20 18:08 ` Geert Uytterhoeven
2022-06-19 20:03 ` Uwe Kleine-König
2022-04-20 12:12 ` [PATCH v2 6/6] pwm: renesas-tpu: Improve precision of period and duty_cycle calculation Uwe Kleine-König
2022-04-20 18:10 ` Geert Uytterhoeven
2022-04-20 18:12 ` [PATCH v2 0/6] pwm: renesas-tpu: Various improvements Geert Uytterhoeven
2022-05-20 14:19 ` Thierry Reding
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.