lkml.org 
[lkml]   [2021]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v9 2/2] leds: Add driver for Qualcomm LPG
    Hello Bjorn,

    On Tue, Jun 22, 2021 at 08:50:39PM -0700, Bjorn Andersson wrote:
    > +static const unsigned int lpg_clk_rates[] = {1024, 32768, 19200000};
    > +static const unsigned int lpg_pre_divs[] = {1, 3, 5, 6};
    > +
    > +static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
    > +{
    > + unsigned int clk, best_clk = 0;
    > + unsigned int div, best_div = 0;
    > + unsigned int m, best_m = 0;
    > + unsigned int error;
    > + unsigned int best_err = UINT_MAX;
    > + u64 denom;
    > + u64 best_period = 0;
    > + u64 actual;
    > + u64 ratio;
    > + u64 nom;
    > +
    > + /*
    > + * The PWM period is determined by:
    > + *
    > + * resolution * pre_div * 2^M
    > + * period = --------------------------
    > + * refclk
    > + *
    > + * With resolution fixed at 2^9 bits, pre_div = {1, 3, 5, 6} and
    > + * M = [0..7].
    > + *
    > + * This allows for periods between 27uS and 381s, as the PWM framework
    > + * wants a period of equal or lower length than requested, reject
    > + * anything below 27uS.
    > + */
    > + if (period <= (u64)NSEC_PER_SEC * LPG_RESOLUTION / 19200000)
    > + return -EINVAL;
    > +
    > + /* Limit period to largest possible value, to avoid overflows */
    > + if (period > 381 * (u64)NSEC_PER_SEC)
    > + period = 381 * (u64)NSEC_PER_SEC;

    Where does the magic 381 come from? This would be more obviously correct
    if you write out the formula as you did for the check above.

    > + /*
    > + * Search for the pre_div, clk and M by solving the rewritten formula
    > + * for each clk and pre_div value:
    > + *
    > + * period * clk
    > + * M = log2 -------------------------------------
    > + * NSEC_PER_SEC * pre_div * resolution
    > + */
    > + for (clk = 0; clk < ARRAY_SIZE(lpg_clk_rates); clk++) {
    > + nom = period * lpg_clk_rates[clk];

    nom is only used in this block, so the declaration can be put in here,
    too. Ditto for at least ratio and actual.

    > +
    > + for (div = 0; div < ARRAY_SIZE(lpg_pre_divs); div++) {
    > + denom = (u64)NSEC_PER_SEC * lpg_pre_divs[div] * (1 << 9);
    > +
    > + if (nom < denom)
    > + continue;
    > +
    > + ratio = div64_u64(nom, denom);
    > + m = ilog2(ratio);
    > + if (m > LPG_MAX_M)
    > + m = LPG_MAX_M;
    > +
    > + actual = DIV_ROUND_UP_ULL(denom * (1 << m), lpg_clk_rates[clk]);
    > +
    > + error = period - actual;
    > + if (error < best_err) {
    > + best_err = error;
    > +
    > + best_div = div;
    > + best_m = m;
    > + best_clk = clk;
    > + best_period = actual;
    > + }
    > + }
    > + }
    > +
    > + chan->clk = best_clk;
    > + chan->pre_div = best_div;
    > + chan->pre_div_exp = best_m;
    > + chan->period = best_period;
    > +
    > + return 0;
    > +}
    > +
    > +static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
    > +{
    > + unsigned int max = LPG_RESOLUTION - 1;
    > + unsigned int val = div_u64(duty * max, chan->period);

    You're losing precision here as chan->period is a rounded value.

    duty * max might overflow.

    > + chan->pwm_value = min(val, max);
    > +}
    > [...]
    > +static void lpg_apply(struct lpg_channel *chan)
    > +{
    > + lpg_disable_glitch(chan);

    Why do you have to do this?

    > + lpg_apply_freq(chan);
    > + lpg_apply_pwm_value(chan);
    > + lpg_apply_control(chan);
    > + lpg_apply_sync(chan);
    > + lpg_apply_lut_control(chan);
    > + lpg_enable_glitch(chan);
    > +}
    > [...]
    > +/*
    > + * Limitations:
    > + * - Updating both duty and period is not done atomically, so the output signal
    > + * will momentarily be a mix of the settings.
    > + */
    > +static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
    > + const struct pwm_state *state)
    > +{
    > + struct lpg *lpg = container_of(chip, struct lpg, pwm);
    > + struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
    > + int ret;
    > +

    You have to care for state->polarity here.

    > + ret = lpg_calc_freq(chan, state->period);
    > + if (ret < 0)
    > + return ret;
    > +
    > + lpg_calc_duty(chan, state->duty_cycle);
    > + chan->enabled = state->enabled;
    > +
    > + lpg_apply(chan);
    > +
    > + triled_set(lpg, chan->triled_mask, chan->enabled ? chan->triled_mask : 0);
    > +
    > + return 0;
    > +}
    > [...]
    > +static int lpg_probe(struct platform_device *pdev)
    > +{
    > + struct device_node *np;
    > + struct lpg *lpg;
    > + int ret;
    > + int i;
    > +
    > + lpg = devm_kzalloc(&pdev->dev, sizeof(*lpg), GFP_KERNEL);
    > + if (!lpg)
    > + return -ENOMEM;
    > +
    > + lpg->data = of_device_get_match_data(&pdev->dev);
    > + if (!lpg->data)
    > + return -EINVAL;
    > +
    > + lpg->dev = &pdev->dev;
    > +
    > + lpg->map = dev_get_regmap(pdev->dev.parent, NULL);
    > + if (!lpg->map) {
    > + dev_err(&pdev->dev, "parent regmap unavailable\n");
    > + return -ENXIO;
    > + }
    > +
    > + ret = lpg_init_channels(lpg);
    > + if (ret < 0)
    > + return ret;
    > +
    > + ret = lpg_parse_dtest(lpg);
    > + if (ret < 0)
    > + return ret;
    > +
    > + ret = lpg_init_triled(lpg);
    > + if (ret < 0)
    > + return ret;
    > +
    > + ret = lpg_init_lut(lpg);
    > + if (ret < 0)
    > + return ret;
    > +
    > + for_each_available_child_of_node(pdev->dev.of_node, np) {
    > + ret = lpg_add_led(lpg, np);
    > + if (ret)
    > + return ret;
    > + }
    > +
    > + for (i = 0; i < lpg->num_channels; i++)
    > + lpg_apply_dtest(&lpg->channels[i]);

    I wonder what all these register initialisations do. You should not do
    anything that modifies the PWM output here that the bootloader might
    have setup. Is this given?

    > +
    > + ret = lpg_add_pwm(lpg);

    The patch would be easier to review if you split it into a led part and
    a pwm part. Then the responsibilities would be more clear, too.

    > + if (ret)
    > + return ret;
    > +
    > + platform_set_drvdata(pdev, lpg);
    > +
    > + return 0;

    If you do the platform_set_drvdata() earlier you can just

    return ret;

    here.

    > +}

    Best regards
    Uwe

    --
    Pengutronix e.K. | Uwe Kleine-König |
    Industrial Linux Solutions | https://www.pengutronix.de/ |
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2021-06-25 15:16    [W:7.432 / U:0.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site