linux-kernel - Re: [PATCH v10 0/3] Change PWM-controlled LED pin active mode and algorithm

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7bcnckef23w6g47ll5l3bktygedrcfvr7fk3qjuq2swtoffhec@zs4w4tuh6qvm>
Date: Tue, 21 Jan 2025 19:12:10 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Nylon Chen <nylon.chen@...ive.com>
Cc: linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org, 
	devicetree@...r.kernel.org, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Samuel Holland <samuel.holland@...ive.com>, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v10 0/3] Change PWM-controlled LED pin active mode and
 algorithm

On Tue, Jan 21, 2025 at 04:47:46PM +0800, Nylon Chen wrote:
> Uwe Kleine-König <u.kleine-koenig@...libre.com> 於 2025年1月21日 週二 下午3:47寫道：
> >
> > Hello,
> >
> > On Sun, Jan 19, 2025 at 03:03:16PM +0800, Nylon Chen wrote:
> > > I ran some basic tests by changing the period and duty cycle in both
> > > decreasing and increasing sequences (see the script below).
> >
> > What is clk_get_rate(ddata->clk) for you?
> 130 MHz

OK, so the possible period lengths are

	(1 << (16 + scale)) / (130 MHz)

for scale in [0, .. 15], right? That's

	2^scale * 504123.07692307694 ns

So testing period in the range between 5000 ns and 15000 ns isn't
sensible? Did I get something wrong? If the above is right, switching
between period=1008246 ns and 1008247 ns is likely to trigger a
warning.

Maybe also something like

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 7e863c2cd44b..6c82aca84472 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -2247,9 +2247,10 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 
 	for (i = 0; i < chip->npwm; i++) {
 		struct pwm_device *pwm = &chip->pwms[i];
-		struct pwm_state state;
+		struct pwm_state state, hwstate;
 
 		pwm_get_state(pwm, &state);
+		pwm_get_state_hw(pwm, &hwstate);
 
 		seq_printf(s, " pwm-%-3d (%-20.20s):", i, pwm->label);
 
@@ -2259,8 +2260,8 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 		if (state.enabled)
 			seq_puts(s, " enabled");
 
-		seq_printf(s, " period: %llu ns", state.period);
-		seq_printf(s, " duty: %llu ns", state.duty_cycle);
+		seq_printf(s, " period: %llu (%llu) ns", state.period, hwstate.period);
+		seq_printf(s, " duty: %llu (%llu) ns", state.duty_cycle, hwstate.duty_cycle);
 		seq_printf(s, " polarity: %s",
 			   state.polarity ? "inverse" : "normal");
 

is useful for debugging to see what is actually implemented for a given
request.

Having said that, I don't like struct pwm_sifive_ddata::real_period and
pwm_sifive_ddata::approx_period. These complicate the calculation and
.get_state should better calculate the period instead of just sticking
to ddata->real_period.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)