Re: [PATCH v9 2/2] leds: Add driver for Qualcomm LPG

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 5928 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]