Hi Uwe,
-----Original Message-----
From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
Sent: Tuesday, 13 October, 2020 2:31 AM
Subject: Re: [PATCH v10 1/2] pwm: Add PWM driver for Intel Keem Bay
Hello Ayyathurai,
you're quoting style is strange. I fixed it up and hope I got it right.
On Mon, Oct 12, 2020 at 08:04:47PM +0000, Ayyathurai, Vijayakannan wrote:
> > On Thu, Oct 08, 2020 at 01:40:30AM +0800, vijayakannan.ayyathurai@xxxxxxxxx wrote:
> > > +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > + const struct pwm_state *state) {
> > > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> > > + struct pwm_state current_state;
> > > + u16 pwm_h_count, pwm_l_count;
> > > + unsigned long long div;
> > > + unsigned long clk_rate;
> > > + u32 pwm_count = 0;
> > > +
> > > + if (state->polarity != PWM_POLARITY_NORMAL)
> > > + return -ENOSYS;
> > > +
> > > + keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0,
> > > + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
> > > +
> > > + keembay_pwm_get_state(chip, pwm, ¤t_state);
> > > +
> > > + if (!state->enabled) {
> > > + if (current_state.enabled)
> > > + keembay_pwm_disable(priv, pwm->hwpwm);
> > > + return 0;
> > > + }
> > > +
> > > + /*
> > > + * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register contain
> > > + * the high time of the waveform, while the last 16 bits contain
> > > + * the low time of the waveform, in terms of clock cycles.
> > > + *
> > > + * high time = clock rate * duty cycle
> > > + * low time = clock rate * (period - duty cycle)
> > > + */
> > > +
> > > + clk_rate = clk_get_rate(priv->clk);
> > > + /* Configure waveform high time */
> > > + div = clk_rate * state->duty_cycle;
> > > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC);
> > > + if (div > KMB_PWM_COUNT_MAX)
> > > + return -ERANGE;
> > > +
> > > + pwm_h_count = div;
> > > + /* Configure waveform low time */
> > > + div = clk_rate * (state->period - state->duty_cycle);
> > > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC - pwm_h_count);
> >
> > In reply to your v7 I suggested the example:
> >
> > clk_rate = 333334 [Hz]
> > state.duty_cycle = 1000500 [ns]
> > state.period = 2001000 [ns]
> >
> > where the expected outcome is
> >
> > pwm_h_count = 333
> > pwm_l_count = 334
> >
> > . Please reread my feedback there. I tried to construct an example
> > where the value is more wrong, but with the constraint that
> > pwm_h_count must be <= KMB_PWM_COUNT_MAX this isn't easy. The best I
> > could come up with is:
> >
> > clk_rate = 1000000000
> > state.duty_cycle = 65535 [ns]
> > state.period = 131070 [ns]
> >
> > where the right value for pwm_l_count is 65535 while you calculate
> > 65539 (and then quit with -ERANGE).
>
> I have applied the formula mentioned in v7 and got different duty
> cycle result then what was expected.
>
> Formula referred by Uwe at v7:
> pwm_l_count = (clk_rate * state->period) / NSEC_PER_SEC - pwm_h_count
>
> % clk_rate period duty_cycle NSEC_PER_SEC pwm_h_count pwm_l_count
> 50% 333334 2001000 1000500 1000000000 333 667
> 25% 500000000 20000 5000 1000000000 2500 10000
> 50% 100000000 131070 65535 1000000000 6553 13107
For the first line:
(clk_rate * state->period) // NSEC_PER_SEC - pwm_h_count =
(333334 * 2001000) // 1000000000 - 333 =
667.001334 - 333 =
334
This gives duty cycle = 333 * 1000000000 / 333334 = 998998.0020039959 ns and a period = (333 + 334) * 1000000000 / 333334 = 2000995.998008004 ns which is well in the limits.
Thank you for this clarification and I am clear in incorporating it in my next version. Is there any other feedback in this version v10?
I guess you assumed my formula to be
(clk_rate * state->period) / (NSEC_PER_SEC - pwm_h_count), but that's not what I meant.
> Whereas the below table is the result of minor modification from your formula and getting the right result.
> pwm_l_count = clk_rate * (state->period - state->duty_cycle) /
> NSEC_PER_SEC - pwm_h_count
>
> % clk_rate period duty_cycle NSEC_PER_SEC pwm_h_count pwm_l_count
> 50% 333334 2001000 1000500 1000000000 333 333
> 25% 500000000 20000 5000 1000000000 2500 7500
> 50% 100000000 131070 65535 1000000000 6553 6553
>
> Please review this and confirm.
In the code you used
clk_rate * (state->period - state->duty_cycle) / (NSEC_PER_SEC - pwm_h_count)
which is notably different. For the example in the first line again you then get 333, which is a less optimal result than 334 I get with my
(fixed) formula. I'm still convinced my formula is the right and optimal one.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Thanks,
Vijay
