Hi Uwe,

-----Original Message-----
From: Uwe Kleine-König <redacted> 
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:

quoted

On Thu, Oct 08, 2020 at 01:40:30AM +0800, vijayakannan.ayyathurai@intel.com wrote:

quoted

+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, &current_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