linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Pavel Machek <pavel@ucw.cz>, Dan Murphy <dmurphy@ti.com>,
	Rob Herring <robh+dt@kernel.org>, Andy Gross <agross@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	Martin Botka <martin.botka1@gmail.com>,
	linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-pwm@vger.kernel.org
Subject: Re: [PATCH v6 2/4] leds: Add driver for Qualcomm LPG
Date: Thu, 13 May 2021 12:43:17 -0500	[thread overview]
Message-ID: <20210513174317.GM2484@yoga> (raw)
In-Reply-To: <20210505051534.id36dvocqfqg3jqc@pengutronix.de>

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



  parent reply	other threads:[~2021-05-13 17:43 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-21 20:12 [PATCH v6 0/4] Qualcomm Light Pulse Generator Bjorn Andersson
2020-10-21 20:12 ` [PATCH v6 1/4] dt-bindings: leds: Add Qualcomm Light Pulse Generator binding Bjorn Andersson
2020-10-26 15:02   ` Rob Herring
2020-10-21 20:12 ` [PATCH v6 2/4] leds: Add driver for Qualcomm LPG Bjorn Andersson
2020-10-22 19:25   ` Luca Weiss
2020-10-29 18:13   ` Pavel Machek
2021-04-29  0:12     ` Bjorn Andersson
2021-04-29 21:12       ` Pavel Machek
2021-04-29 21:29         ` Bjorn Andersson
2021-05-04 15:43           ` Pavel Machek
2021-05-04 16:13             ` Bjorn Andersson
2021-05-05  5:21               ` Uwe Kleine-König
2021-04-18 21:54   ` Marijn Suijten
2021-04-28 22:39     ` Bjorn Andersson
2021-04-29 19:31       ` Marijn Suijten
2021-04-29 20:54         ` Bjorn Andersson
2021-05-05  5:15   ` Uwe Kleine-König
2021-05-05  5:19     ` Uwe Kleine-König
2021-05-13 17:43     ` Bjorn Andersson [this message]
2020-10-21 20:12 ` [PATCH v6 3/4] arm64: dts: qcom: pm(i)8994: Add mpp and lpg blocks Bjorn Andersson
2020-10-21 20:12 ` [PATCH v6 4/4] arm64: dts: qcom: Add user LEDs on db820c Bjorn Andersson
2021-04-15 10:46 [PATCH v6 2/4] leds: Add driver for Qualcomm LPG Yassine Oudjana

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210513174317.GM2484@yoga \
    --to=bjorn.andersson@linaro.org \
    --cc=agross@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dmurphy@ti.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=martin.botka1@gmail.com \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --subject='Re: [PATCH v6 2/4] leds: Add driver for Qualcomm LPG' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).