| 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: <20210625131516.qw3ig6dvmp4ftzos@pengutronix.de>
Date: Fri, 25 Jun 2021 15:15:16 +0200
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
Andy Gross <agross@...nel.org>,
Thierry Reding <thierry.reding@...il.com>,
Lee Jones <lee.jones@...aro.org>, 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,
Marijn Suijten <marijn.suijten@...ainline.org>,
Yassine Oudjana <y.oudjana@...tonmail.com>,
Luca Weiss <luca@...tu.xyz>,
Subbaraman Narayanamurthy <subbaram@...eaurora.org>
Subject: Re: [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/ |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists