lkml.org 
[lkml]   [2021]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/ |


    \
     
     \ /
      Last update: 2021-05-13 19:43    [W:2.318 / U:0.420 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site