linux-kernel - Re: [PATCH v12] pwm: airoha: Add support for EN7581 SoC

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: <pbutokheq6zm4gzjzvhhmxvnse3uudb5obpuyd55z7emlm3pju@7l7hocoktu37>
Date: Fri, 9 May 2025 21:43:55 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org, 
	Benjamin Larsson <benjamin.larsson@...exis.eu>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Lorenzo Bianconi <lorenzo@...nel.org>
Subject: Re: [PATCH v12] pwm: airoha: Add support for EN7581 SoC

Hello Christian,

On Fri, May 09, 2025 at 03:03:23PM +0200, Christian Marangi wrote:
> thanks a lot for the review. I was just starting reviewing some patch on
> patchwork so I could remove some work from you but you were faster...

You're still invited to comment ...
 
> On Fri, May 09, 2025 at 12:39:37PM +0200, Uwe Kleine-König wrote:
> > On Mon, Apr 07, 2025 at 07:35:53PM +0200, Christian Marangi wrote:
> > > +static void airoha_pwm_get_ticks_from_ns(u64 period_ns, u32 *period_tick,
> > > +					 u64 duty_ns, u32 *duty_tick)
> > > +{
> > > +	u64 tmp_duty_tick;
> > > +
> > > +	*period_tick = div_u64(period_ns, AIROHA_PWM_PERIOD_TICK_NS);
> > > +
> > > +	tmp_duty_tick = mul_u64_u64_div_u64(duty_ns, AIROHA_PWM_DUTY_FULL,
> > > +					    period_ns);
> > 
> > So period can be set to multiples of 4 ms. If you request
> > 
> > 	.period_ns = 11999 ns
> > 	.duty_ns = 4016 ns
> > 
> > the hardware should configure 
> > 
> > 	.period = 8000 ns
> > 	.duty_cycle = 4015.6862745098038 ns (i.e. 128/255 * period)
> > 
> > corresponding to period_tick = 2 and duty_tick = 128.
> > 
> > However you calculate duty_tick = 85.
> > 
> > I would expect that with having PWM_DEBUG enabled you get a warning when
> > you do:
> > 
> > 	pwmset -P 8000 -D 4016
> > 	pwmset -P 11999 -D 4016
> > 
> 
> I addressed all the other comments but this is the only thing that I'm
> confused about.
> 
> Where 85 comes from?

4016 * 255 / 11999 -> 85. The problem I suspected is that duty_tick is
calculated using the requested period value instead of the real one.

> I tested your command and I can correctly observe the values getting
> set to the expected tick.

I didn't recheck in detail and now I'm unsure if that really happens
because period_ns might be already round to a multiple of 4 ms?
 
> And I don't have the idempotent warning from PWM debug.
> 
> With period = 8000 and duty to 4016, period tick is set to 0x2 and duty
> is set to 0x80 (128)
> 
> When PWM debug repply the configuration and read the state, it gets
> period 0x8000 and duty 4015,687.
> 
> And those are the expected values. Am I missing something here?

If you don't get the warning, you most probably only miss that I
misjudged the code :-)

Best regards
Uwe

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