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
