Hi Tylor, [in addition to any other reviews] On Oct 17 2023, Tylor Yang wrote: > The hx83102j is a TDDI IC (Touch with Display Driver). The > IC using SPI to transferring HID packet to host CPU. The IC also > report HID report descriptor for driver to register HID device. > The driver is designed as a framework for future expansion and > hx83102j is the first case. Each hx_spi_hid_hx8xxxxx modules are > mutual exclusive, it should be initiate one at a time. > > This driver takes a position similar to i2c-hid, it initialize > and control the touch IC below and register HID to upper hid-core. > When touch ic report an interrupt, it receive the data from IC > and report as HID input to hid-core. Let hid-core dispatch input > to registered hid-protocol and report to related input sub-system. > > This driver also provide advanced functions by hidraw interface: Generally speaking, when your commit message has an "also" in the middle, it means that the next feature(s) need to be split in their own patches. > - runtime firmware update > - debug functions, such as reg r/w > - self test for touch panel So this means that this patch should at least be split in 4. > > Due to patch size is too big, separate into 3 part. This is part 1. This is the wrong reason to split a patch series. Well, it's true, it's too big, but you have to take into account the reviewers/maintainers point of view: - we don't know the internals of your device - we don't (necessarily) have access to the docs - we don't have a lot of time to spend on a review - we can not focus on a 9000 lines of code patch and remember every single aspect when reviewing, to be able to point bugs Given that you compared this driver to i2c-hid, please have a look at the history of it: - my first initial submission[0] (v1) was a single patch of 1000 loc, but it contained only the core functionality to bind a driver. I stripped everything else that could make it useful (ACPI or DT bindings) but it was a an attempt at being a one-to-one mapping of the I2C part of the publicly available specification - shortly after I sent a separate 14 patches series to do more cleanups on the initial patch, as things were moving forward - then Mika submitted the ACPI handling[1] - and DT bindings came later [2] (8 months after the initial submission) My point is, when the code is that big, it's perfectly fine to have it split and maybe not have all of the functionalities available in the first submission. Bonus point for not having everything and in smaller patches: it's less of a pain to change or drop stuff :) > > Signed-off-by: Tylor Yang <tylor_yang@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx> > --- > MAINTAINERS | 1 + > drivers/hid/hx-hid/hx_acpi.c | 81 ++ > drivers/hid/hx-hid/hx_core.c | 1605 +++++++++++++++++++++++++++++ > drivers/hid/hx-hid/hx_core.h | 489 +++++++++ > drivers/hid/hx-hid/hx_hid.c | 753 ++++++++++++++ > drivers/hid/hx-hid/hx_hid.h | 96 ++ > drivers/hid/hx-hid/hx_ic_83102j.c | 340 ++++++ > drivers/hid/hx-hid/hx_ic_83102j.h | 42 + hx-hid is a terrible name. Why not at least "himax-hid"? Or maybe "himax-spi-hid"? Also, I can't remember if this was already asked, but is that driver vaguely related to the HID over SPI specification from Microsoft? We have seen one submission in the past regarding that even if it didn't went through, but if your driver implements this protocol following Microsoft's specification, I'd rather not have a custom vendor code when we can have a "standardized" one. [...] Cheers, Benjamin [0] https://lore.kernel.org/linux-input/1347630103-4105-1-git-send-email-benjamin.tissoires@xxxxxxxxx/ [1] https://lore.kernel.org/linux-input/1357650332-30031-1-git-send-email-mika.westerberg@xxxxxxxxxxxxxxx/ [2] https://lore.kernel.org/linux-input/1371109835-30796-1-git-send-email-benjamin.tissoires@xxxxxxxxxx/
