[PATCH 1/6] pwm: renesas-tpu: Make use of dev_err_probe()

[PATCH 1/6] pwm: renesas-tpu: Make use of dev_err_probe()

[Uwe Kleine-König]

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, consistenly start error messages with upper
case.

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;

base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.35.1

[PATCH 2/6] pwm: renesas-tpu: Make use of devm functions

[Uwe Kleine-König]

This simplifies an error path in .probe() and allows to drop the .remove()
function.

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

Re: [PATCH 2/6] pwm: renesas-tpu: Make use of devm functions

[Geert Uytterhoeven]

On Wed, Apr 13, 2022 at 10:51 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> This simplifies an error path in .probe() and allows to drop the .remove()
> function.
>
> 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

[PATCH 3/6] pwm: renesas-tpu: Implement .apply() callback

[Uwe Kleine-König]

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..100c21e27648 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)
+			chip->ops->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

Re: [PATCH 3/6] pwm: renesas-tpu: Implement .apply() callback

[Geert Uytterhoeven]

On Wed, Apr 13, 2022 at 10:51 AM 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>

LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

The display backlight still works fine on r8a7740/armadillo, 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

Re: [PATCH 3/6] pwm: renesas-tpu: Implement .apply() callback

[Geert Uytterhoeven]

Hi Uwe,

On Thu, Apr 14, 2022 at 11:18 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Apr 13, 2022 at 10:51 AM 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>
>
> LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> The display backlight still works fine on r8a7740/armadillo, so
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Oops, I spoke too soon...

> > @@ -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)
> > +                       chip->ops->disable(chip, pwm);

tpu_pwm_disable

else it crashes with a NULL-pointer dereference (e.g. during system
shutdown).

> > +
> > +               return 0;
> > +       }

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

[PATCH 4/6] pwm: renesas-tpu: Rename variables to match the usual naming

[Uwe Kleine-König]

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 100c21e27648..671f1f824da8 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

Re: [PATCH 4/6] pwm: renesas-tpu: Rename variables to match the usual naming

[Geert Uytterhoeven]

On Wed, Apr 13, 2022 at 10:51 AM 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>

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

[PATCH 5/6] pwm: renesas-tpu: Improve maths to compute register settings

[Uwe Kleine-König]

The newly computed register values are intended to exactly match the
previously computed values. The main improvement is that the prescaler
is computed directly instead of with a loop. 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 | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index 671f1f824da8..fce7df418d62 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,21 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	u32 duty;
 	int ret;
 
-	/*
-	 * Pick a prescaler to avoid overflowing the counter.
-	 * TODO: Pick the highest acceptable prescaler.
-	 */
 	clk_rate = clk_get_rate(tpu->clk);
 
-	for (prescaler = 0; prescaler < ARRAY_SIZE(prescalers); ++prescaler) {
-		period = clk_rate / prescalers[prescaler]
-		       / (NSEC_PER_SEC / period_ns);
-		if (period <= 0xffff)
-			break;
-	}
+	period = clk_rate / (NSEC_PER_SEC / period_ns);
+	if (period >= 64 * 0x10000 || period == 0)
+		return -EINVAL;
 
-	if (prescaler == ARRAY_SIZE(prescalers) || period == 0) {
-		dev_err(&tpu->pdev->dev, "clock rate mismatch\n");
-		return -ENOTSUPP;
-	}
+	if (period < 0x10000)
+		prescaler = 0;
+	else
+		prescaler = ilog2(period / 0x10000) / 2 + 1;
+
+	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 +277,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

Re: [PATCH 5/6] pwm: renesas-tpu: Improve maths to compute register settings

[Geert Uytterhoeven]

Hi Uwe,

Thanks for your patch!

On Wed, Apr 13, 2022 at 10:51 AM 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 directly instead of with a loop. 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.

I'm not worried about the divisions, but about the ilog2(), which
uses fls().  The TPU block also exists on SuperH SoCs (although
currently no SH Linux code has it enabled), and SH uses the fls()
implementation from asm-generic.

> --- 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,21 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         u32 duty;
>         int ret;
>
> -       /*
> -        * Pick a prescaler to avoid overflowing the counter.
> -        * TODO: Pick the highest acceptable prescaler.
> -        */
>         clk_rate = clk_get_rate(tpu->clk);
>
> -       for (prescaler = 0; prescaler < ARRAY_SIZE(prescalers); ++prescaler) {
> -               period = clk_rate / prescalers[prescaler]
> -                      / (NSEC_PER_SEC / period_ns);
> -               if (period <= 0xffff)
> -                       break;
> -       }
> +       period = clk_rate / (NSEC_PER_SEC / period_ns);
> +       if (period >= 64 * 0x10000 || period == 0)
> +               return -EINVAL;
>
> -       if (prescaler == ARRAY_SIZE(prescalers) || period == 0) {
> -               dev_err(&tpu->pdev->dev, "clock rate mismatch\n");
> -               return -ENOTSUPP;
> -       }
> +       if (period < 0x10000)
> +               prescaler = 0;
> +       else
> +               prescaler = ilog2(period / 0x10000) / 2 + 1;
> +
> +       period >>= 2 * prescaler;

Although the above is correct, I find it hard to read.
Hence I'd keep a loop, like:

    unsigned int prescaler = 0;
    ...
    while (period > 0x10000) {
            period >>= 2;
            prescalar++;
    }

This would even save 2 lines of code ;-)

>
>         if (duty_ns) {
> -               duty = clk_rate / prescalers[prescaler]
> +               duty = (clk_rate >> 2 * prescaler)
>                      / (NSEC_PER_SEC / duty_ns);
>                 if (duty > period)
>                         return -EINVAL;

Anyway:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

The display backlight still works fine on r8a7740/armadillo, 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

Re: [PATCH 5/6] pwm: renesas-tpu: Improve maths to compute register settings

[Uwe Kleine-König]

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

Hello Geert,

On Thu, Apr 14, 2022 at 12:10:02PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 13, 2022 at 10:51 AM 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 directly instead of with a loop. 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.
> 
> I'm not worried about the divisions, but about the ilog2(), which
> uses fls().  The TPU block also exists on SuperH SoCs (although
> currently no SH Linux code has it enabled), and SH uses the fls()
> implementation from asm-generic.
> 
> > --- 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,21 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >         u32 duty;
> >         int ret;
> >
> > -       /*
> > -        * Pick a prescaler to avoid overflowing the counter.
> > -        * TODO: Pick the highest acceptable prescaler.
> > -        */
> >         clk_rate = clk_get_rate(tpu->clk);
> >
> > -       for (prescaler = 0; prescaler < ARRAY_SIZE(prescalers); ++prescaler) {
> > -               period = clk_rate / prescalers[prescaler]
> > -                      / (NSEC_PER_SEC / period_ns);
> > -               if (period <= 0xffff)
> > -                       break;
> > -       }
> > +       period = clk_rate / (NSEC_PER_SEC / period_ns);
> > +       if (period >= 64 * 0x10000 || period == 0)
> > +               return -EINVAL;
> >
> > -       if (prescaler == ARRAY_SIZE(prescalers) || period == 0) {
> > -               dev_err(&tpu->pdev->dev, "clock rate mismatch\n");
> > -               return -ENOTSUPP;
> > -       }
> > +       if (period < 0x10000)
> > +               prescaler = 0;
> > +       else
> > +               prescaler = ilog2(period / 0x10000) / 2 + 1;
> > +
> > +       period >>= 2 * prescaler;
> 
> Although the above is correct, I find it hard to read.
> Hence I'd keep a loop, like:
> 
>     unsigned int prescaler = 0;
>     ...
>     while (period > 0x10000) {
>             period >>= 2;
>             prescalar++;
>     }
> 
> This would even save 2 lines of code ;-)

The "hard to read" part is subjective, I understand it just fine. (But I
admit I wouldn't be surprised if I'm the exception here as I do much
math.) I suggest to judge this by looking at the generated code. I'm not
an expert here (no sh toolchain here, no sh asm foo), but my expectation
is that the compiler notices that 1 <= period / 0x10000 < 64 and then
the inlined fls code should be simplified such that

	ilog2(period / 0x10000) / 2 + 1

simplifies to something like:

	x = period >> 16
	prescaler = 4
	if (!(x & 0xf0u)) {
		x <<= 4;
		prescaler -= 2;
	}
	if (!(x & 0xc0u)) {
		x <<= 2;
		prescaler -= 1;
	}

which I expect to be more efficient than the loop you suggested.

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

[PATCH 6/6] pwm: renesas-tpu: Improve precision of period and duty_cycle calculation

[Uwe Kleine-König]

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.

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 | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index fce7df418d62..c8c7a896fc55 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -242,42 +242,52 @@ 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 > 1000000000UL)) {
+		/*
+		 * 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);
 	if (period >= 64 * 0x10000 || period == 0)
 		return -EINVAL;
 
 	if (period < 0x10000)
 		prescaler = 0;
 	else
-		prescaler = ilog2(period / 0x10000) / 2 + 1;
+		/*
+		 * We know period to fit into an u32, so cast accordingly to
+		 * make the division a bit cheaper
+		 */
+		prescaler = ilog2((u32)period / 0x10000) / 2 + 1;
 
 	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

Re: [PATCH 6/6] pwm: renesas-tpu: Improve precision of period and duty_cycle calculation

[Geert Uytterhoeven]

Hi Uwe,

On Wed, Apr 13, 2022 at 10:51 AM 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.
>
> 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.

You forgot to mention that pwm_state.period is no longer truncated to u32.

>
> 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 | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
> index fce7df418d62..c8c7a896fc55 100644
> --- a/drivers/pwm/pwm-renesas-tpu.c
> +++ b/drivers/pwm/pwm-renesas-tpu.c
> @@ -242,42 +242,52 @@ 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);

As clk_get_rate() returns unsigned long, I think you should change
clk_rate from u32 to unsigned long, too.

> +       if (unlikely(clk_rate > 1000000000UL)) {

s/1000000000UL/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);
>         if (period >= 64 * 0x10000 || period == 0)
>                 return -EINVAL;

Perhaps use "u64 period64" above, and

    /* We know period to fit into an u32 */
    period = (u32)period64;

to avoid introducing all casts below.

>
>         if (period < 0x10000)
>                 prescaler = 0;
>         else
> -               prescaler = ilog2(period / 0x10000) / 2 + 1;
> +               /*
> +                * We know period to fit into an u32, so cast accordingly to
> +                * make the division a bit cheaper
> +                */
> +               prescaler = ilog2((u32)period / 0x10000) / 2 + 1;

Using a loop would avoid the need for a division...

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

With some (or all ;-) suggestions above taken into account:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

The display backlight still works fine on r8a7740/armadillo, 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

Re: [PATCH 6/6] pwm: renesas-tpu: Improve precision of period and duty_cycle calculation

[Geert Uytterhoeven]

Hi Uwe,

On Thu, Apr 14, 2022 at 12:27 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Apr 13, 2022 at 10:51 AM 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.
> >
> > 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.
>
> You forgot to mention that pwm_state.period is no longer truncated to u32.

Please ignore this bogus comment.
Sorry for the fuzz.

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

Re: [PATCH 6/6] pwm: renesas-tpu: Improve precision of period and duty_cycle calculation

[Uwe Kleine-König]

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

Hello Geert,

first of all thanks for your review and testing. It's great to get some
feedback (even though it means some work for me :-)

On Thu, Apr 14, 2022 at 12:27:28PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 13, 2022 at 10:51 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > 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 | 34 ++++++++++++++++++++++------------
> >  1 file changed, 22 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
> > index fce7df418d62..c8c7a896fc55 100644
> > --- a/drivers/pwm/pwm-renesas-tpu.c
> > +++ b/drivers/pwm/pwm-renesas-tpu.c
> > @@ -242,42 +242,52 @@ 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);
> 
> As clk_get_rate() returns unsigned long, I think you should change
> clk_rate from u32 to unsigned long, too.

Yeah, could do that. I guess I didn't because in my bubble a long is 32
bits wide :-) IMHO fixing that is worth a separate patch.

> > +       if (unlikely(clk_rate > 1000000000UL)) {
> 
> s/1000000000UL/NSEC_PER_SEC/

ok

> > +               /*
> > +                * 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);
> >         if (period >= 64 * 0x10000 || period == 0)
> >                 return -EINVAL;
> 
> Perhaps use "u64 period64" above, and
> 
>     /* We know period to fit into an u32 */
>     period = (u32)period64;
> 
> to avoid introducing all casts below.

I first had it that way, but didn't like it. Yeah, it makes the patch a
bit smaller, but IMHO it adds some burden to understand the code flow
because for a reader having two variables for the same (semantic) value
is harder to understand.

> 
> >
> >         if (period < 0x10000)
> >                 prescaler = 0;
> >         else
> > -               prescaler = ilog2(period / 0x10000) / 2 + 1;
> > +               /*
> > +                * We know period to fit into an u32, so cast accordingly to
> > +                * make the division a bit cheaper
> > +                */
> > +               prescaler = ilog2((u32)period / 0x10000) / 2 + 1;
> 
> Using a loop would avoid the need for a division...

I would "fix" this differently, there isn't really a division; at least
I would expect (but didn't check) that the compiler uses a cheap shift
to implement "/ 0x10000" and "/ 2". ilog2 might become a bit cheaper for
a 32 bit value. So I would replace that by:

	/*
	 * ilog2 might be a bit cheaper for u32 than u64 and we know
	 * period to fit into a u32, so cast accordingly.
	 */

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

Re: [PATCH 1/6] pwm: renesas-tpu: Make use of dev_err_probe()

[Geert Uytterhoeven]

On Wed, Apr 13, 2022 at 10:51 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> 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, consistenly start error messages with upper

consistently

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