| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210513174317.GM2484@yoga>
Date: Thu, 13 May 2021 12:43:17 -0500
From: Bjorn Andersson <bjorn.andersson@...aro.org>
To: Uwe Kleine-K?nig <u.kleine-koenig@...gutronix.de>
Cc: Pavel Machek <pavel@....cz>, Dan Murphy <dmurphy@...com>,
Rob Herring <robh+dt@...nel.org>,
Andy Gross <agross@...nel.org>,
Thierry Reding <thierry.reding@...il.com>,
Lee Jones <lee.jones@...aro.org>,
Martin Botka <martin.botka1@...il.com>,
linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-pwm@...r.kernel.org
Subject: Re: [PATCH v6 2/4] leds: Add driver for Qualcomm LPG
On Wed 05 May 00:15 CDT 2021, Uwe Kleine-K?nig wrote:
> Hello Bjorn,
>
Thanks for your feedback, and the input on extending the PWM api related
to patterns. I'll revisit the calculations, and PWM_DEBUG accordingly.
Regards,
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/ |
Powered by blists - more mailing lists