Closed StollD closed 1 year ago
@quo I know there were some suspend problems on ITHC recently, do you think its ready? Or should we wait with this?
I know there were some suspend problems on ITHC recently, do you think its ready? Or should we wait with this?
It's ready as far as I'm concerned.
To summarize the suspend issue: Before I added the dev_pm_ops struct (for hibernation), I don't think Linux was suspending the THC at all. Now it is suspending it, causing the touch controller to lose state (and/or communication to get messed up). I wasn't able to figure out a minimal way to restore state (or to avoid losing state) that works on all devices, so now the driver basically just shuts down on suspend and completely restarts on wake. This is effectively what the Windows driver does as well. The only minor downside is that the HID device is recreated, so iptsd has to restart, but that seems to work fine now.
I believe this is technically ready? The last issues are just style things (and if we're going to upstream this that's probably something that we can work on then / when it's merged). So I'd merge it this weekend if there are no other complaints.
Is it me, or does ithc-main.c
contain carriage returns? I'm seeing ^M
s all over in git diff
. I assume this is unintentional :)
It's not just you. I didn't notice that because GitHub doesn't show it... I can fix that up after merging if I don't forget. Anyway, the ITHC code does need quite a bit of work/formatting before we can consider it for upstream.
Cool. I'm testing it now on my Tigerlake Surface Laptop 4 and it works fine so far, including suspend and resume. (Just single touch though; I haven't figured out how to get iptsd@master to work with the ithc driver from this patch, if it's currently possible.)
edit: Figured it out. For some reason, I needed to specify the hid
parameter explicitly for it to use HID mode... Not sure why. It seems to me like it should be loading in HID mode by default based on this patch.
I can fix that up after merging if I don't forget.
I'd recommend doing any changes or fixups on quos repository, so that we don't diverge and updating is as simple as cp -r src/* drivers/hid/ithc/
edit: Figured it out. For some reason, I needed to specify the
hid
parameter explicitly for it to use HID mode... Not sure why. It seems to me like it should be loading in HID mode by default based on this patch.
If you can still replicate it, I would be interested in your dmesg when loading ithc without the parameter.
In that case, should we also move that repo to linux-surface/ithc?
Is it me, or does
ithc-main.c
contain carriage returns?
Yeah, I've been meaning to fix that.
In that case, should we also move that repo to linux-surface/ithc?
Our company is migrating away from the Surface devices, and we may not even end up using the SP7+ in production. So my plan is to clean up the ithc sources a bit (maybe over the holidays), and after that I'm happy to transfer the repository and hand over maintenance to someone else.
Just to be clear what I meant: I'd add you to this org, then you can transfer it and still work on it here (no strings attached). It's just to make this a bit more "official". However, it's fully up to you.
That's very kind. I'll let you know if I change my mind, but I just don't see myself spending a lot more time on the driver (esp. upstreaming, etc), so I think it would be better to just hand it over eventually.
Sure, whatever you're more comfortable with.
Marking this as a draft until I have a proper fix for a suspend issue on SP5 that verdre mentioned on Matrix.
Superceded by #133
iptsd feels pretty much ready and will work better than what is currently in the repo, so lets start doing this ...
Things that need to be done once this is merged:
CONFIG_MISC_IPTS=m
toCONFIG_HID_IPTS=m
in our kernel configsCONFIG_HID_ITHC=m
to the kernel configsWhy do this now if 6.1 is right around the corner? Because then we would have to wait until Fedora rebases to 6.1, or apply it to 6.0 anyways, because the new iptsd is incompatible with the old driver.