Hello Bjorn,

On Wed, Oct 21, 2020 at 01:12:22PM -0700, Bjorn Andersson wrote:

+static const unsigned int lpg_clk_table[NUM_PWM_PREDIV][NUM_PWM_CLK] = {
+	{
+		1 * (NSEC_PER_SEC / 1024),
+		1 * (NSEC_PER_SEC / 32768),
+		1 * (NSEC_PER_SEC / 19200000),
+	},
+	{
+		3 * (NSEC_PER_SEC / 1024),
+		3 * (NSEC_PER_SEC / 32768),

1000000000 / 32768 is 30517.578125. Because of the parenthesis this is
truncated to 30517. Multiplied by 3 this results in 91551. The exact
result is 91552.734375 however.

+		3 * (NSEC_PER_SEC / 19200000),
+	},
+	{
+		5 * (NSEC_PER_SEC / 1024),
+		5 * (NSEC_PER_SEC / 32768),
+		5 * (NSEC_PER_SEC / 19200000),
+	},
+	{
+		6 * (NSEC_PER_SEC / 1024),
+		6 * (NSEC_PER_SEC / 32768),
+		6 * (NSEC_PER_SEC / 19200000),
+	},
+};
+
+/*
+ * PWM Frequency = Clock Frequency / (N * T)
+ *      or
+ * PWM Period = Clock Period * (N * T)
+ *      where
+ * N = 2^9 or 2^6 for 9-bit or 6-bit PWM size
+ * T = Pre-divide * 2^m, where m = 0..7 (exponent)
+ *
+ * This is the formula to figure out m for the best pre-divide and clock:
+ * (PWM Period / N) = (Pre-divide * Clock Period) * 2^m
+ */
+static void lpg_calc_freq(struct lpg_channel *chan, unsigned int period_us)
+{
+	int             n, m, clk, div;
+	int             best_m, best_div, best_clk;
+	unsigned int    last_err, cur_err, min_err;
+	unsigned int    tmp_p, period_n;
+
+	if (period_us == chan->period_us)
+		return;
+
+	/* PWM Period / N */
+	if (period_us < UINT_MAX / NSEC_PER_USEC)
+		n = 6;
+	else
+		n = 9;
+
+	period_n = ((u64)period_us * NSEC_PER_USEC) >> n;
+
+	min_err = UINT_MAX;
+	last_err = UINT_MAX;
+	best_m = 0;
+	best_clk = 0;
+	best_div = 0;
+	for (clk = 0; clk < NUM_PWM_CLK; clk++) {
+		for (div = 0; div < NUM_PWM_PREDIV; div++) {
+			/* period_n = (PWM Period / N) */
+			/* tmp_p = (Pre-divide * Clock Period) * 2^m */
+			tmp_p = lpg_clk_table[div][clk];
+			for (m = 0; m <= NUM_EXP; m++) {
+				cur_err = abs(period_n - tmp_p);
+				if (cur_err < min_err) {
+					min_err = cur_err;
+					best_m = m;
+					best_clk = clk;
+					best_div = div;
+				}
+
+				if (m && cur_err > last_err)
+					/* Break for bigger cur_err */
+					break;
+
+				last_err = cur_err;
+				tmp_p <<= 1;

This is inexact. Consider again the case where tmp_p is
3 * (NSEC_PER_SEC / 32768). The values you use and the exact values are:

	m     |       0        |      1       |      2      |      3     | ... |        7 |
	tmp_p |   91551        | 183102       | 366204      | 732408     |     | 11718528 |
        actual|   91552.734375 | 183105.46875 | 366210.9375 | 732421.875 | ... | 11718750 |

So while you save some cycles by precalculating the values in
lpg_clk_table, you trade that for lost precision.

+			}
+		}
+	}

Please don't pick a period that is longer than the requested period (for
the PWM functionality that is).

This can be simplified, you can at least calculate the optimal m
directly.

+	/* Use higher resolution */
+	if (best_m >= 3 && n == 6) {
+		n += 3;
+		best_m -= 3;
+	}
+
+	chan->clk = best_clk;
+	chan->pre_div = best_div;
+	chan->pre_div_exp = best_m;
+	chan->pwm_size = n;
+
+	chan->period_us = period_us;
+}
+
+static void lpg_calc_duty(struct lpg_channel *chan, unsigned int duty_us)
+{
+	unsigned int max = (1 << chan->pwm_size) - 1;
+	unsigned int val = div_u64((u64)duty_us << chan->pwm_size, chan->period_us);

Please use the actually implemented period here instead of the
requested. This improves precision, see commit
8035e6c66a5e98f098edf7441667de74affb4e78 for a similar case.

+
+	chan->pwm_value = min(val, max);
+}
+
[...]
+static const struct pwm_ops lpg_pwm_ops = {
+	.request = lpg_pwm_request,
+	.apply = lpg_pwm_apply,

Can you please test your driver with PWM_DEBUG enabled? The first thing
this will critizise is that there is no .get_state callback.

+	.owner = THIS_MODULE,
+};
+
+static int lpg_add_pwm(struct lpg *lpg)
+{
+	int ret;
+
+	lpg->pwm.base = -1;

Please drop this assignment.

+	lpg->pwm.dev = lpg->dev;
+	lpg->pwm.npwm = lpg->num_channels;
+	lpg->pwm.ops = &lpg_pwm_ops;
+
+	ret = pwmchip_add(&lpg->pwm);
+	if (ret)
+		dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret);
+
+	return ret;
+}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |