While implementing fixed duty writing through hwmon, I just noticed that we don't set the pwmX_enable value when writing to pwmX directly, we just write it to the device. I think we should cache it and then check if we should write it, like for the curve?

Can I add the additional hwmon_ctrl_mapping parameter here, since that's new and not being tested there?

Can I add the additional hwmon_ctrl_mapping parameter here, since that's new and not being tested there?

Yes!

That should be the majority of work for this PR, now to tie the loose ends if we find any.

At a glance, this LGTM. But I have to admit that I haven't been able to thoroughly review. I'll give you write access to this repo, and you can merge this if or as soon as you're comfortable with it.

UPDATE: you now have write access here too.

Just one question: the 200 ms timeout, why is it required, and is another way to solve that problem?

Thanks for the invite, much appreciated! I'll work on getting these PRs across the finish line as soon as college stuff clears up.

As for the 200ms timeout, in the driver we apply the whole curve whenever one point of it is changed, if it was already in curve mode. So from liquidctl we set those curve points in succession and trigger the same amount of HID reports sent to the device, from the driver. Setting pwmX_enable immediately after causes it to do nothing IIRC, it just gets confused.

Looking at it again, we pause a bit before setting pwmX_enable to 2, if it was already 2, which doesn't make much sense, since that curve is now wholly applied anyway. Only setting it to 2 if it's in some other mode should work, without the timeout. Will test that.

So I investigated this on the Z53 (for now), and what I said above doesn't work.

Scenario 1: pwmX_enable is set to 2 after a small timeout, regardless of what it is. Different curves get applied properly, even when spammed in quick succession.

Scenario 2: pwmX_enable is set to 2 only if was something else, without any timeout. Order of actions (channel was in fixed mode):

• Setting first curve: it's set correctly, pwmX_enable is set to 2
• Setting second curve: nothing happens
• Setting first curve again: nothing seems to happen, it's still on the first curve from step 1 (same curve)

And so on, until it happens that setting the first curve activates the second one, and vice versa. Or that setting a curve has no effect.

Looks like to me that the firmware is finicky when spammed with lots of reports, which happens when 40 temp points are set through the driver in quick succession. Maybe it bails after a few, caches others and reprocesses them at some point when new ones arrive, or discards them after some period... I'm not sure what it does exactly, but it always honors the latest single report that's sent from setting pwmX_enable to 2, which sends the final curve (initiated after a small wait so it has time to decide it won't act on the previous reports).

@aleksamagicka, if I correctly understood your results, scenario 1 is significantly more robust/reliable and, also, simple. So let's go with it (i.e. keep everything as is).

On the kernel side, we might want to document his finicky behavior for other users of the driver.

That's right. I've been meaning to add an expanded note to liquidctl earlier today, but it's getting late now and I'll do it tomorrow (for the kernel driver as well).

I'll test this on both devices tomorrow once again and then we can merge this and plan on trying to upstream the kernel part.

Thanks!