Hello,
On Wed, Jun 01, 2022 at 07:25:28PM +0000, Biju Das wrote:
> > On Tue, May 10, 2022 at 03:42:59PM +0100, Biju Das wrote:
> > > + clk_rate = clk_get_rate(pc->clk);
> > > + c = clk_rate * period_ns;
> >
> > This might overflow (once you keep period_ns as u64).
> OK, the logic is changed like below to avoid overflow.
>
> freq = div_u64(clk_get_rate(pc->clk), 1000000);
> period_cycles = div_u64(freq * period_ns, 1000);
This might still overflow for big period_ns and freq > 1.
Better use mul_u64_u64_div_u64 and limit clkrate to prevent an overflow.
This yields a higher precision without overflow. Something like:
rate = clk_get_rate(pc->clk);
/*
* Refuse clk rates > 1 GHz to prevent overflowing the following
* calculation.
*/
if (rate > NSEC_PER_SEC)
return -EINVAL;
period_cycles = mul_u64_u64_div_u64(rate, period_ns, NSEC_PER_SEC);
> > > + period_cycles = div_u64(c, NANO);
> >
> > Please use NSEC_PER_SEC here.
> >
> > > +
> > > + if (period_cycles < 1)
> > > + period_cycles = 1;
> > > +
> > > + prescale = -1;
> > > + /* prescale 1, 4, 16, 64, 256 and 1024 */
> > > + for (i = 0, prod = 1; i < 6; i++) {
> > > + if ((period_cycles / GTPR_MAX_VALUE * prod) == 0) {
> > > + prescale = i;
> > > + break;
> > > + }
> > > +
> > > + prod *= 4;
> > > + }
> >
> > This would be better understandable if you used:
> >
> > for (i = 0; i < 6; i++) {
> > prod = 1 << (2 * i);
> > ...
> >
> > }
> >
> > Have you tested this? The division by GTPR_MAX_VALUE (= 0xFFFFFFFF) looks
> > suspicious. Unless I'm missing something
> >
> > if ((period_cycles / GTPR_MAX_VALUE * prod) == 0)
> >
> > is equivalent to:
> >
> > if (period_cycles < GTPR_MAX_VALUE)
> >
> > . Is this really what you want here?
>
> On the next version, I have changed the logic to check this condition in caller function.
>
> if (period_cycles > 1UL * GTPR_MAX_VALUE * GPT_MAX_PRESCALE_VAL) {
> dev_warn(chip->dev, "ch=%d period exceed limit\n", pwm->hwpwm);
> return -EINVAL;
> }
Please always round down period, that is, if the request it's too big to
implement, do the biggest period that is possible.
With GTPR_MAX_VALUE = 0xffffffff and unsigned long being 32 bit on some
arch, this isn't a sensible test.
> > > + /* Set counting mode */
> > > + rzg2l_gpt_write(pc, GTUDDTYC, UP_COUNTING);
> > > + /* Set period */
> > > + rzg2l_gpt_write(pc, GTPR, pv);
> > > +
> > > + /* Enable pin output */
> > > + rzg2l_gpt_modify(pc, GTIOR, pwm->ph->mask, pwm->ph->value);
> > > +
> > > + /* Set duty cycle */
> > > + rzg2l_gpt_write(pc, pwm->ph->duty_reg_offset, dc);
> > > +
> > > + /* Set initial value for counter */
> > > + rzg2l_gpt_write(pc, GTCNT, 0);
> > > + /* Set no buffer operation */
> > > + rzg2l_gpt_write(pc, GTBER, 0);
> >
> > How does the output behave on reprogramming? Does it complete the currently
> > programmed period? Please document this behaviour as e.g.
> > drivers/pwm/pwm-sl28cpld.c does.
>
> Mode and Prescalar must be set, only when the GTCNT is stopped.
> This condition will document.
Please document this at the top of the driver. Take e.g. pwm-sifive.c as
a template. (And please stick to the format used there for easy
greppability, i.e. use "Limitations:" and a list of items with no empty
line between them.)
> > > + return 0;
> > > +}
> > > +
> > > +static int rzg2l_gpt_enable(struct rzg2l_gpt_chip *pc) {
> > > + /* Start count */
> > > + rzg2l_gpt_modify(pc, GTCR, GTCR_CST, GTCR_CST);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *pc) {
> > > + /* Stop count */
> > > + rzg2l_gpt_modify(pc, GTCR, GTCR_CST, 0);
> >
> > Same question here: How does the hardware behave? Does it complete the
> > currently running period? How does the output behave? (Typical candidates
> > are: freeze at the level where it is currently, constant 0,
> > high-z.)
>
> It is set to output low during stop.
OK, that should go to the Limitations section, too (even though it's not
a limitation).
> > > +}
> > > +
> > > +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device
> > *pwm,
> > > + const struct pwm_state *state)
> > > +{
> > > + struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> > > + int ret;
> >
> > As you don't support different polarities, there is the following
> > missing:
>
> There is a plan to add polarity in later version.
>
> >
> > if (state->polarity != PWM_POLARITY_NORMAL)
> > return -EINVAL;
>
> Agree, will add this check in initial version.
Does it output low or inactive level on disable if inversed polarity is
configured?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
