On Thu, Feb 21, 2019 at 11:53 AM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote: > > 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. Series applied to for-5.1/hid-uclogic, please send further patches (if any) on top of this branch. Thanks. Cheers, Benjamin
