Re: Patch "pwm: rockchip: Round period/duty down on apply, up on get" has been added to the 6.16-stable tree

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

Hi Greg & Uwe,

On Tuesday, 12 August 2025 22:15:37 Central European Summer Time Uwe Kleine-König wrote:
> Hello Greg,
> 
> On Tue, Aug 12, 2025 at 12:53:49PM +0200, Greg KH wrote:
> > On Tue, Aug 12, 2025 at 12:36:48PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Aug 12, 2025 at 10:53:11AM +0200, Greg KH wrote:
> > > > On Sat, Aug 09, 2025 at 11:45:23AM +0200, Uwe Kleine-König wrote:
> > > > > while the new code makes the driver match the PWM rules now, I'd be
> > > > > conservative and not backport that patch because while I consider it a
> > > > > (very minor) fix that's a change in behaviour and maybe people depend on
> > > > > that old behaviour. So let's not break our user's workflows and reserve
> > > > > that for a major release. Please drop this patch from your queue.
> > > > 
> > > > Now dropped, but note, any behavior change is ok for ANY kernel version
> > > > as we guarantee they all work the same :)
> > > 
> > > Your statement makes no sense, I guess you missed to add a "don't".
> > > Otherwise I'd like to know who "we" is :-)
> > 
> > {sigh} yes, I mean "any behavior change is NOT ok..."
> 
> Ahh I though you wanted to say "any behavior change is ok for ANY kernel
> version as we don't guarantee they all work the same". So let me explain
> a bit:
> 
> A PWM consumer (think fan driver) gets an opaque handle to the used PWM
> device and then requests something like: "Configure the PWM to have a
> period length of 50 ms and a duty length of 20 ms." The typical PWM
> cannot fulfill the request exactly and has to choose if it configures
> (say) period=46 ms + duty=16 ms, or period=52 ms and duty=26 ms (or
> possibly a mixture of the two, or an error code). Traditionally each
> driver implemented its own policy here and so the generic fan driver
> knows neither the possible hardware restrictions (another hardware might
> be able to do period=51 ms and duty=19 ms) nor how the driver picked one
> of the options. Making things harder, it depends on the use case which
> policy is the best, which also explains that different drivers picked
> different algorithms. And also it's unclear even what "nearest" means.
> For example a hardware might be able to configure period=17 ms or
> period=24 ms in reply to a request of period=20ms. You probably say that
> 17 ms is nearer. But if you think in frequencies the request of
> period=20ms corresponds to 50 Hz and then 24 ms ~ 41.6667 Hz is better
> than 17 ms ~ 58.8235 Hz.
> 
> To improve that situation a bit at least new PWM drivers are supposed to
> round down both values. The commit under discussion modifies an existing
> driver to align to that policy. So from a global point of view this is
> an improvement, because it makes things a bit more deterministic for a
> PWM consumer that doesn't know about the hardware details. The down side
> is that a PWM consumer that does know these details might depend on the
> actual implementation of the previously implemented policy.

As the author of the patch in question, I thought I should also chime
in to elaborate on what this means in real terms on the hardware this
patch is applicable to. In my testing, I've found the difference is
usually a few tens of Hertz at a scale of tens of thousands of Hertz at
best. However, out of an abundance of caution, I didn't want this to be
picked up by literally every stable kernel still supported, because if
this causes an actual observable functional change in the real world
then it's only accidental as a regression. This should not be the case,
but I'd rather not tempt fate here.

What it does do however, aside from improving consistency here, is that
it makes the PWM core no longer produce a warning on kernels built with
PWM_DEBUG whenever the fan on my RADXA ROCK 5B changes speed. As someone
who worked on both a new PWM driver where I wanted PWM_DEBUG on at the
time as well as running this existing driver on the aforementioned device
while working on other drivers of this SoC vendor, I did not want to
switch between kernel configs all the time, and this greatly improved
my willingness to keep on living.

So in conclusion:
1. this does not change behaviour for any real-world use case, as the
   difference is so tiny it drowns out in the usual tolerances of PWM
   consumers to account for clock shenanigans.
2. even if someone does rely on 0.01% differences in PWM output on a
   consumer device SoC, they would probably appreciate predictable
   behaviour going forward, instead of their requested rate having
   an error that is either positive or negative depending on how
   the math works out. But they'd probably appreciate this change as
   part of a new major release, not a stable patch release.
3. The real motivation is making me less sad while working on other
   things.

Kind regards,
Nicolas Frattaroli

> 
> So this is a change in behaviour, but it adds to the consistency of the
> PWM framework. If you want to make an LED blink or drive a fan, that
> (most likely) doesn't matter to you. If you drive a robot arm in a
> construction facility, it might.
> 
> The mid term solution is a new PWM API that gives more control to the
> consumer. The framework bits for that are already done and three drivers
> are implementing that and two more are in the making. (And if you use
> these with the legacy consumer function you also get the round down
> behaviour.)
> 
> Best regards
> Uwe
>