Hi Nick, On Sun, Feb 10, 2019 at 11:14 AM Nikolai Kondrashov <spbnick@xxxxxxxxx> wrote: > > Hi everyone, Benjamin, > > It took me a while (I can only afford a couple hours per week on this), but > I finally split my original patchset into finer commits. If this is too fine, > please feel free to squash them. Hope it makes more sense this way and is > easier to review. Thanks a lot. This makes the review easier. I think we are good to go. The patches are really only local to your subset of drivers, so I guess if something breaks, that will be on you :) > > Benjamin, I agree that just repeating switch statements to branch for > e.g. models can work better. However, I think it works well with simple > one-two level switch statements only. In this case just branching by e.g. > VID:PID pairs won't be enough. > > The worst offenders are Huion tablets which *all* use the same VID:PID pair, > while they use two major initialization and report protocols, with various > quirks within. Those could only be distinguished using several different > methods. I would have to have extra flags for those anyway, and mixing them > with switch statements would be the worst of both worlds. So I decided to use > just one: have a description of tablet quirks and only one switch statement on > initialization/discovery. > > I took a look at what you're doing with uhid and testing, and it looks good. > However, since for UC-Logic (Huion especially) reporting protocols are > discovered outside the HID protocol (by requesting various USB strings), there > would be no way for the driver to determine which protocol the uhid device > would use. For now I just fail initialization if the underlying device is > anything but a USB device. Well, thanks for fixing that. This will plug a hole already. Too bad for the regressions tests. I'll need more time to see if we can circumvent that :/ > > I'll think about this a little more, though. Maybe I can fallback to choosing > one (most complex) protocol for uhid devices and test that, or maybe I can > pass the selection within the HID protocol somehow. Hmm... Simulating a USB > string request with get report requests perhaps for uhid devices? Or maybe I > can just limit myself to testing simpler, non-Huion devices only. It would be nice if those device where only using HID requests. But I do not think adding a simulation of HID requests when the code detects a uhid device is a good thing. You end up not testing the actual code, and this will give a false feeling of safety. I think we can emulate USB devices, that would require an other layer in hid-tools, but why not? > > It's great news that hid-generic now deals with unbinding/rebinding smoothly! > I'm not adding entries to hid_have_special_driver anymore in my patches, and I > added three patches removing the existing UC-Logic/KYE/Waltop entries, after a > bit of testing. Thanks a lot. I am planning in my head to remove entirely the list, but not sure when I'll plug the switch. The most beneficial thing from that is for projects like DIGImend or any other out of the tree HID drivers: no need to patch the kernel, just load the module and done. > > Should I move the quirk setting to hid-kye.c/hid-waltop.c too? Reducing hid_quirks would be good, yes. This list is run for every device we plug, so the shorter the better. This can come in a followup series. No rush. > > Please write if you'd like me to change anything else. I think I am happy with v2. There are a few things we could enhance (see the 2 nitpicks), but it doesn't worth requiring a v3. I'll apply this today after a last run of the tests. Cheers, Benjamin > > Thank you! > Nick >
