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