| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230614073345.5ejzkbcdiel5v7zg@pengutronix.de>
Date: Wed, 14 Jun 2023 09:33:45 +0200
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Philipp Zabel <p.zabel@...gutronix.de>
Cc: Fabrice Gasnier <fabrice.gasnier@...s.st.com>,
Thierry Reding <thierry.reding@...il.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
linux-pwm@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pwm: stm32: Implement .get_state()
On Thu, Jun 08, 2023 at 04:06:02PM +0200, Philipp Zabel wrote:
> Stop stm32_pwm_detect_channels() from disabling all channels and count
> the number of enabled PWMs to keep the clock running. Implement the
> &pwm_ops->get_state callback so drivers can inherit PWM state set by
> the bootloader.
>
> Signed-off-by: Philipp Zabel <p.zabel@...gutronix.de>
> ---
> Make the necessary changes to allow inheriting PWM state set by the
> bootloader, for example to avoid flickering with a pre-enabled PWM
> backlight.
> ---
> drivers/pwm/pwm-stm32.c | 75 ++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 59 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 62e397aeb9aa..e0677c954bdf 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -52,6 +52,21 @@ static u32 active_channels(struct stm32_pwm *dev)
> return ccer & TIM_CCER_CCXE;
> }
>
> +static int read_ccrx(struct stm32_pwm *dev, int ch, u32 *value)
> +{
> + switch (ch) {
> + case 0:
> + return regmap_read(dev->regmap, TIM_CCR1, value);
> + case 1:
> + return regmap_read(dev->regmap, TIM_CCR2, value);
> + case 2:
> + return regmap_read(dev->regmap, TIM_CCR3, value);
> + case 3:
> + return regmap_read(dev->regmap, TIM_CCR4, value);
> + }
> + return -EINVAL;
> +}
I'd prefer having something like:
#define TIM_CCR(i) (0x30 + 4 * (i)) /* Capt/Comp Register i, for i in 1 .. 4 */
#define TIM_CCR1 TIM_CCR(1)
#define TIM_CCR2 TIM_CCR(2)
#define TIM_CCR3 TIM_CCR(3)
#define TIM_CCR4 TIM_CCR(4)
and then read_ccrx could be simplified to:
return regmap_read(dev->regmap, TIM_CCR(i + 1), value);
. (Not sure if passing an invalid channel really needs handling.)
But given that write_ccrx looks the same, I'm ok with that.
> +
> static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value)
> {
> switch (ch) {
> @@ -486,9 +501,40 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
> return ret;
> }
>
> +static int stm32_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm, struct pwm_state *state)
> +{
> + struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> + int ch = pwm->hwpwm;
> + unsigned long rate;
> + u32 ccer, psc, arr, ccr;
> + u64 dty, prd;
> + int ret;
> +
> + ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> + if (ret)
> + return ret;
> +
> + state->enabled = ccer & (TIM_CCER_CC1E << (ch * 4));
Other parts of the driver use the macros from <linux/bitfield.h>. With a
similar approach as suggested for TIM_CCR above, this could be made to
read as:
state->enabled = FIELD_GET(TIM_CCER_CCxE(ch + 1), ccer);
> + state->polarity = (ccer & (TIM_CCER_CC1P << (ch * 4))) ?
> + PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> + regmap_read(priv->regmap, TIM_PSC, &psc);
> + regmap_read(priv->regmap, TIM_ARR, &arr);
> + read_ccrx(priv, ch, &ccr);
You don't use the return value of read_ccrx(), so make it void (or check
it)? If you check it, also do it for regmap_read().
> + rate = clk_get_rate(priv->clk);
> +
> + prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
This might overflow?!
> + state->period = DIV_ROUND_UP_ULL(prd, rate);
> + dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
> + state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
> +
> + return ret;
> +}
While looking at stm32_pwm_config() to check if it matches your
stm32_pwm_get_state() implementation, I noticed that for small values of
period_ns, prd might become 0 which than yields surprising effects in
combination with
regmap_write(priv->regmap, TIM_ARR, prd - 1);
Also the driver needs locking because of the PSC and ARR registers are
shared for all channels and there are rounding issues (prd is used for
the calculation of dty).
> +
> static const struct pwm_ops stm32pwm_ops = {
> .owner = THIS_MODULE,
> .apply = stm32_pwm_apply_locked,
> + .get_state = stm32_pwm_get_state,
> .capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
> };
>
> @@ -579,30 +625,22 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
> priv->have_complementary_output = (ccer != 0);
> }
>
> -static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
> +static int stm32_pwm_detect_channels(struct stm32_pwm *priv, int *n_enabled)
> {
> - u32 ccer;
> - int npwm = 0;
> + u32 ccer, ccer_backup;
> + int npwm;
>
> /*
> * If channels enable bits don't exist writing 1 will have no
> * effect so we can detect and count them.
> */
> + regmap_read(priv->regmap, TIM_CCER, &ccer_backup);
> regmap_set_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE);
> regmap_read(priv->regmap, TIM_CCER, &ccer);
> - regmap_clear_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE);
> + regmap_write(priv->regmap, TIM_CCER, ccer_backup);
>
> - if (ccer & TIM_CCER_CC1E)
> - npwm++;
> -
> - if (ccer & TIM_CCER_CC2E)
> - npwm++;
> -
> - if (ccer & TIM_CCER_CC3E)
> - npwm++;
> -
> - if (ccer & TIM_CCER_CC4E)
> - npwm++;
> + npwm = hweight32(ccer & TIM_CCER_CCXE);
> + *n_enabled = hweight32(ccer_backup & TIM_CCER_CCXE);
hweight32 returns an unsigned int. While there is probably no problem
with values that don't fit, using unsigned for *n_enabled (and also
npwm) looks better IMHO. Maybe split making npwm unsigned and using
hweight32 to assign it to a separate preparing patch? The inconsistency
between "npwm" (without underscore) and "n_enabled" (with underscore)
strikes me a bit. given that struct pwm_chip has "npwm", too, maybe drop
the _ from "n_enabled"?
> return npwm;
> }
> @@ -613,7 +651,9 @@ static int stm32_pwm_probe(struct platform_device *pdev)
> struct device_node *np = dev->of_node;
> struct stm32_timers *ddata = dev_get_drvdata(pdev->dev.parent);
> struct stm32_pwm *priv;
> + int n_enabled;
> int ret;
> + int i;
>
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> @@ -635,7 +675,10 @@ static int stm32_pwm_probe(struct platform_device *pdev)
>
> priv->chip.dev = dev;
> priv->chip.ops = &stm32pwm_ops;
> - priv->chip.npwm = stm32_pwm_detect_channels(priv);
> + priv->chip.npwm = stm32_pwm_detect_channels(priv, &n_enabled);
> +
> + for (i = 0; i < n_enabled; i++)
> + clk_enable(priv->clk);
>
> ret = pwmchip_add(&priv->chip);
> if (ret < 0)
>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists