Hi Benjamin,
First of all, thanks a lot for the review :)
As a rule of thumb, we don't normally allow more than one file per
vendor in the HID tree. But when the file gets out of control (or when
it has a strong history) we can make exception.
In your case, I think the split is justified, but I would only split
the file in 2: hid-uclogic-rdesc.c and hid-uclogic-core.c.
I don't really like the `params` one, especially if it it only the
init sequence of the devices. I do not really see a benefit of
splitting this init logic from the rest of the driver.
I think I see what you mean. Yeah, the main file has no initialization, is
pretty short, and the initialization is in hid-uclogic-params.c.
However, this one thing I'd like to defend. As you can see, there is a ton of
tablet models, and each with their own quirk, many of which cannot be
described with a report descriptor. There's also the thing where tablet
parameter discovery cannot be separated from the initialization, or I would've
done it.
What I was trying to achieve by separating hid-uclogic-params.c is to narrow
down and explicitly and clearly define all the possible quirks a tablet can
have (see hid-uclogic-params.h). So that I could stop all the various ways
they can be detected/handled from spreading all over the code, and then
mutating. Things like those big switch statements checking VID:PID pairs in
multiple places. This was starting to happen as we tried to handle the new
Huion models (which wasn't contributed to the kernel). They were close enough,
and at the same time different enough, to cause a big mess and it was
difficult to disentangle the tablet description from tablet state.
I think the goal of keeping the number of HID driver files to a minimum is a
worthy one, I sympathize. Yes, I can simply merge hid-uclogic-params.c into
hid-uclogic-core.c and it will work, of course. However, having tablet
description and feature discovery in a separate file from the actual
functionality makes it easier for me to think about them, and makes it easier
to prevent the code from regressing to quirk discovery all over the place, and
making the actual driver functionality code much bigger. This way we detect
them and describe them once. And I also get to be a little saner.
While you are working on this (well attempt to at least), I'd like to
know if we can simply drop the hid_have_special_driver related to
hid-uclogic.ko. I am hesitant in blindly removing the full list, but
reducing it to the bare minimum is something I'd like to achieve.
I'm a little confused by this and your other comments regarding the
hid_have_special_driver entries. IIRC, it was that the driver wouldn't be used
at all without an entry there, unless you rebind it manually. I'm assuming
that has changed, will look at the fresh code to see what's going on, and if
it's true, will gladly drop the entries, but if you have a moment I would
appreciate an explanation :)
hid-uclogic is using direct calls to usb. Could you guard them by
`hid_is_using_ll_driver(hdev, &usb_hid_driver)` before each calls or
make .probe() refuses to bind if not using true USB?
Hmm. I'll figure it out and will fix, thanks!
Last, I understand the burden of rewriting the history. However, I'd
like this patch to be split in 3 at least:
- one patch to separate hid-uclogic.c into hid-uclogic-core and
hid-uclogic-rdesc
- one patch that rewrites the handling of the init sequence
- one patch that adds the new devices
Aaargh. It's painful, but I understand why you need this, and I'll try to do
it.
I'll also look into fixing the other patches in the series, of course.
All of that likely on the weekend.
Thank you, Benjamin!
Nick
On 1/7/19 6:47 PM, Benjamin Tissoires wrote:
Hi Nick,
On Fri, Dec 28, 2018 at 1:59 PM Nikolai Kondrashov <spbnick@xxxxxxxxx> wrote:
On 12/27/18 7:16 PM, Nikolai Kondrashov wrote:
Rewrite the hid-uclogic driver by splitting the driver into three files:
* hid-uclogic-core.c - the "front-end" of a normal HID driver,
* hid-uclogic-rdesc.c - a collection of report descriptors, and
* hid-uclogic-params.c - probing and initializing tablet parameters.
Did this one get stuck somewhere in a moderation queue?
Looks like the LKML didn't like it. It doesn't show up on
https://patchwork.kernel.org/project/linux-input/list/?series=60781
I have a few comments on 4/4, they are general so I hope you don't
mind I put them here:
As a rule of thumb, we don't normally allow more than one file per
vendor in the HID tree. But when the file gets out of control (or when
it has a strong history) we can make exception.
In your case, I think the split is justified, but I would only split
the file in 2: hid-uclogic-rdesc.c and hid-uclogic-core.c.
I don't really like the `params` one, especially if it it only the
init sequence of the devices. I do not really see a benefit of
splitting this init logic from the rest of the driver.
While you are working on this (well attempt to at least), I'd like to
know if we can simply drop the hid_have_special_driver related to
hid-uclogic.ko. I am hesitant in blindly removing the full list, but
reducing it to the bare minimum is something I'd like to achieve.
hid-uclogic is using direct calls to usb. Could you guard them by
`hid_is_using_ll_driver(hdev, &usb_hid_driver)` before each calls or
make .probe() refuses to bind if not using true USB?
Last, I understand the burden of rewriting the history. However, I'd
like this patch to be split in 3 at least:
- one patch to separate hid-uclogic.c into hid-uclogic-core and
hid-uclogic-rdesc
- one patch that rewrites the handling of the init sequence
- one patch that adds the new devices
I hope it is not too much to ask.
Cheers,
Benjamin
Don't see it in the archive.
Nick
