From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: Sean Anderson <sean.anderson@seco.com> Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, Thierry Reding <thierry.reding@gmail.com>, Alvaro Gamez <alvaro.gamez@hazent.com>, michal.simek@xilinx.com, Lee Jones <lee.jones@linaro.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer Date: Sat, 14 Aug 2021 22:47:10 +0200 [thread overview] Message-ID: <20210814204710.retjwn5fycwtrypp@pengutronix.de> (raw) In-Reply-To: <20210719221322.3723009-3-sean.anderson@seco.com> [-- Attachment #1: Type: text/plain, Size: 5622 bytes --] Hello Sean, sorry for having you let waiting so long. Now here some more feedback: On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote: > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, > + const struct pwm_state *state) > +{ > + bool enabled; > + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip); > + u32 tlr0, tlr1, tcsr0, tcsr1; > + u64 period_cycles, duty_cycles; > + unsigned long rate; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + /* > + * To be representable by TLR, cycles must be between 2 and > + * priv->max + 2. To enforce this we can reduce the duty > + * cycle, but we may not increase it. > + */ > + rate = clk_get_rate(priv->clk); > + period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC); cool, I didn't know mul_u64_u32_div. Hmm, we still have a problem here if state->period * rate > 1000000000 * U64_MAX. So to be entirely save, we either need: /* * To ensure that period * rate / NSEC_PER_SEC fits into an u64 * we need: * U64_MAX * NSEC_PER_SEC * period < ---------------------- * rate * * . If rate is not bigger than NSEC_PER_SEC this is true for * sure as the RHS is bigger than U64_MAX. Otherwise we can * calculate the RHS using mul_u64_u32_div. */ if (rate > NSEC_PER_SEC) period = min(state->period, mul_u64_u32_div(U64_MAX, NSEC_PER_SEC, rate); else period = state->period; or we go a step further and check the priv->max limit in the same step: period = min(state->period, ((u64)priv->max + 2) * NSEC_PER_SEC / rate) . The latter is simpler and it's safe as priv->max is an u32 and so there is no overflow. > + if (period_cycles - 2 > priv->max || period_cycles < 2) I'd check for period_cycles < 2 first, because otherwise period_cycles - 2 might underflow. Nothing bad happens in this case, but reading from left to right my first thought was I found a bug. Also please decrease period_cycles if it's bigger than priv->max + 2. (With the suggestion above you don't need to check for period_cycles - 2 > priv->max any more however.) > + return -ERANGE; > + duty_cycles = mul_u64_u32_div(state->duty_cycle, rate, NSEC_PER_SEC); > + > + /* > + * If we specify 100% duty cycle, we will get 0% instead, so decrease > + * the duty cycle count by one. > + */ > + if (period_cycles == duty_cycles) > + duty_cycles--; > + > + /* Round down to 0% duty cycle for unrepresentable duty cycles */ > + if (duty_cycles < 2) > + duty_cycles = period_cycles; > + > + regmap_read(priv->map, TCSR0, &tcsr0); > + regmap_read(priv->map, TCSR1, &tcsr1); > + tlr0 = xilinx_timer_tlr_cycles(priv, tcsr0, period_cycles); > + tlr1 = xilinx_timer_tlr_cycles(priv, tcsr1, duty_cycles); > + regmap_write(priv->map, TLR0, tlr0); > + regmap_write(priv->map, TLR1, tlr1); > + > + enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); > + if (state->enabled) { > + /* > + * If the PWM is already running, then the counters will be > + * reloaded at the end of the current cycle. > + */ If state->enabled is false, $enabled isn't used, so you can move the assignment into the if body and also limit the scope of $enabled. > + if (!enabled) { > + /* Load TLR into TCR */ > + regmap_write(priv->map, TCSR0, tcsr0 | TCSR_LOAD); > + regmap_write(priv->map, TCSR1, tcsr1 | TCSR_LOAD); > + /* Enable timers all at once with ENALL */ > + tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); > + tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); > + regmap_write(priv->map, TCSR0, tcsr0); > + regmap_write(priv->map, TCSR1, tcsr1); > + } > + } else { > + regmap_write(priv->map, TCSR0, 0); > + regmap_write(priv->map, TCSR1, 0); > + } > + > + return 0; > +} > + > +static void xilinx_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *unused, > + struct pwm_state *state) > +{ > + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip); > + u32 tlr0, tlr1, tcsr0, tcsr1; > + > + regmap_read(priv->map, TLR0, &tlr0); > + regmap_read(priv->map, TLR1, &tlr1); > + regmap_read(priv->map, TCSR0, &tcsr0); > + regmap_read(priv->map, TCSR1, &tcsr1); > + state->period = xilinx_timer_get_period(priv, tlr0, tcsr0); xilinx_timer_get_period rounds down, this is however wrong for .get_state(). > + state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1); ditto for duty_cycle. > + state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); > + state->polarity = PWM_POLARITY_NORMAL; > [...] > +static int xilinx_timer_probe(struct platform_device *pdev) > +{ > + int ret; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct xilinx_timer_priv *priv; > + struct xilinx_pwm_device *pwm; > + u32 pwm_cells, one_timer; > + void __iomem *regs; > + > + ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells); > + if (ret == -EINVAL) > + return -ENODEV; > + else if (ret) > + return dev_err_probe(dev, ret, "could not read #pwm-cells\n"); Please capitalize error messages. > [...] > + if (ret) { > + clk_rate_exclusive_put(priv->clk); > + clk_disable_unprepare(priv->clk); > + return dev_err_probe(dev, ret, "could not register pwm chip\n"); s/pwm/PWM/ > + } > + > + return 0; > +} 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 --]
WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: Sean Anderson <sean.anderson@seco.com> Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, Thierry Reding <thierry.reding@gmail.com>, Alvaro Gamez <alvaro.gamez@hazent.com>, michal.simek@xilinx.com, Lee Jones <lee.jones@linaro.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer Date: Sat, 14 Aug 2021 22:47:10 +0200 [thread overview] Message-ID: <20210814204710.retjwn5fycwtrypp@pengutronix.de> (raw) In-Reply-To: <20210719221322.3723009-3-sean.anderson@seco.com> [-- Attachment #1.1: Type: text/plain, Size: 5622 bytes --] Hello Sean, sorry for having you let waiting so long. Now here some more feedback: On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote: > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, > + const struct pwm_state *state) > +{ > + bool enabled; > + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip); > + u32 tlr0, tlr1, tcsr0, tcsr1; > + u64 period_cycles, duty_cycles; > + unsigned long rate; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + /* > + * To be representable by TLR, cycles must be between 2 and > + * priv->max + 2. To enforce this we can reduce the duty > + * cycle, but we may not increase it. > + */ > + rate = clk_get_rate(priv->clk); > + period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC); cool, I didn't know mul_u64_u32_div. Hmm, we still have a problem here if state->period * rate > 1000000000 * U64_MAX. So to be entirely save, we either need: /* * To ensure that period * rate / NSEC_PER_SEC fits into an u64 * we need: * U64_MAX * NSEC_PER_SEC * period < ---------------------- * rate * * . If rate is not bigger than NSEC_PER_SEC this is true for * sure as the RHS is bigger than U64_MAX. Otherwise we can * calculate the RHS using mul_u64_u32_div. */ if (rate > NSEC_PER_SEC) period = min(state->period, mul_u64_u32_div(U64_MAX, NSEC_PER_SEC, rate); else period = state->period; or we go a step further and check the priv->max limit in the same step: period = min(state->period, ((u64)priv->max + 2) * NSEC_PER_SEC / rate) . The latter is simpler and it's safe as priv->max is an u32 and so there is no overflow. > + if (period_cycles - 2 > priv->max || period_cycles < 2) I'd check for period_cycles < 2 first, because otherwise period_cycles - 2 might underflow. Nothing bad happens in this case, but reading from left to right my first thought was I found a bug. Also please decrease period_cycles if it's bigger than priv->max + 2. (With the suggestion above you don't need to check for period_cycles - 2 > priv->max any more however.) > + return -ERANGE; > + duty_cycles = mul_u64_u32_div(state->duty_cycle, rate, NSEC_PER_SEC); > + > + /* > + * If we specify 100% duty cycle, we will get 0% instead, so decrease > + * the duty cycle count by one. > + */ > + if (period_cycles == duty_cycles) > + duty_cycles--; > + > + /* Round down to 0% duty cycle for unrepresentable duty cycles */ > + if (duty_cycles < 2) > + duty_cycles = period_cycles; > + > + regmap_read(priv->map, TCSR0, &tcsr0); > + regmap_read(priv->map, TCSR1, &tcsr1); > + tlr0 = xilinx_timer_tlr_cycles(priv, tcsr0, period_cycles); > + tlr1 = xilinx_timer_tlr_cycles(priv, tcsr1, duty_cycles); > + regmap_write(priv->map, TLR0, tlr0); > + regmap_write(priv->map, TLR1, tlr1); > + > + enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); > + if (state->enabled) { > + /* > + * If the PWM is already running, then the counters will be > + * reloaded at the end of the current cycle. > + */ If state->enabled is false, $enabled isn't used, so you can move the assignment into the if body and also limit the scope of $enabled. > + if (!enabled) { > + /* Load TLR into TCR */ > + regmap_write(priv->map, TCSR0, tcsr0 | TCSR_LOAD); > + regmap_write(priv->map, TCSR1, tcsr1 | TCSR_LOAD); > + /* Enable timers all at once with ENALL */ > + tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); > + tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); > + regmap_write(priv->map, TCSR0, tcsr0); > + regmap_write(priv->map, TCSR1, tcsr1); > + } > + } else { > + regmap_write(priv->map, TCSR0, 0); > + regmap_write(priv->map, TCSR1, 0); > + } > + > + return 0; > +} > + > +static void xilinx_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *unused, > + struct pwm_state *state) > +{ > + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip); > + u32 tlr0, tlr1, tcsr0, tcsr1; > + > + regmap_read(priv->map, TLR0, &tlr0); > + regmap_read(priv->map, TLR1, &tlr1); > + regmap_read(priv->map, TCSR0, &tcsr0); > + regmap_read(priv->map, TCSR1, &tcsr1); > + state->period = xilinx_timer_get_period(priv, tlr0, tcsr0); xilinx_timer_get_period rounds down, this is however wrong for .get_state(). > + state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1); ditto for duty_cycle. > + state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); > + state->polarity = PWM_POLARITY_NORMAL; > [...] > +static int xilinx_timer_probe(struct platform_device *pdev) > +{ > + int ret; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct xilinx_timer_priv *priv; > + struct xilinx_pwm_device *pwm; > + u32 pwm_cells, one_timer; > + void __iomem *regs; > + > + ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells); > + if (ret == -EINVAL) > + return -ENODEV; > + else if (ret) > + return dev_err_probe(dev, ret, "could not read #pwm-cells\n"); Please capitalize error messages. > [...] > + if (ret) { > + clk_rate_exclusive_put(priv->clk); > + clk_disable_unprepare(priv->clk); > + return dev_err_probe(dev, ret, "could not register pwm chip\n"); s/pwm/PWM/ > + } > + > + return 0; > +} Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-08-14 20:47 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-19 22:13 [PATCH v5 1/3] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson 2021-07-19 22:13 ` Sean Anderson 2021-07-19 22:13 ` [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson 2021-07-19 22:13 ` Sean Anderson 2021-07-20 21:41 ` kernel test robot 2021-07-20 21:41 ` kernel test robot 2021-07-20 21:41 ` kernel test robot 2021-07-20 21:44 ` Sean Anderson 2021-07-20 21:44 ` Sean Anderson 2021-07-20 21:44 ` Sean Anderson 2021-07-21 2:35 ` kernel test robot 2021-07-21 2:35 ` kernel test robot 2021-07-21 2:35 ` kernel test robot 2021-07-19 22:13 ` [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson 2021-07-19 22:13 ` Sean Anderson 2021-07-21 13:26 ` kernel test robot 2021-07-21 13:26 ` kernel test robot 2021-07-21 13:26 ` kernel test robot 2021-07-22 19:18 ` Sean Anderson 2021-07-22 19:18 ` Sean Anderson 2021-07-22 19:18 ` Sean Anderson 2021-08-09 14:48 ` Sean Anderson 2021-08-09 14:48 ` Sean Anderson 2021-08-14 20:47 ` Uwe Kleine-König [this message] 2021-08-14 20:47 ` Uwe Kleine-König 2021-08-16 23:51 ` Sean Anderson 2021-08-16 23:51 ` Sean Anderson 2021-08-17 18:04 ` Uwe Kleine-König 2021-08-17 18:04 ` Uwe Kleine-König 2021-08-17 22:56 ` Sean Anderson 2021-08-17 22:56 ` Sean Anderson 2021-07-20 13:11 ` [PATCH v5 1/3] dt-bindings: pwm: Add " Rob Herring 2021-07-20 13:11 ` Rob Herring 2021-07-20 21:45 ` Sean Anderson 2021-07-20 21:45 ` Sean Anderson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210814204710.retjwn5fycwtrypp@pengutronix.de \ --to=u.kleine-koenig@pengutronix.de \ --cc=alvaro.gamez@hazent.com \ --cc=devicetree@vger.kernel.org \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=michal.simek@xilinx.com \ --cc=sean.anderson@seco.com \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.