On 2/21/19 12:53 PM, Benjamin Tissoires 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 :)
Hah, that's fine with me :)
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 :/
Yeah, it would be good to have tests.
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.
Weeelll, it will be just a small piece of functionality, which is much better
than nothing. And we would test protocol parsing, which is harder to keep
working. Just to make sure I got the idea across: it wouldn't be simulation of
HID requests, but the other way around: a simulation of USB requests via HID
requests.
I think we can emulate USB devices, that would require an other layer
in hid-tools, but why not?
If we have proper support for faking USB string request responses that would
be much better! Not sure how you would do that within the uhid interface
though.
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.
Yes, that's what we need, and this change makes it simpler :)
Although I got past that already in 2013 with an addition of automatic driver
rebinding via udev.
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.
OK, great :)
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.
Great! Glad that I managed to get this in with just one extra revision :)
Thank you, Benjamin!
Nick
