| Message ID | 20231017091900.801989-1-tylor_yang@himax.corp-partner.google.com |
|---|---|
| Headers | show |
| Series | HID: touchscreen: add himax hid-over-spi driver | expand |
On Wed, Oct 18, 2023 at 08:07:32AM +0200, Krzysztof Kozlowski wrote: > On 17/10/2023 23:41, Doug Anderson wrote: > > > > 3. The ChromeOS team is organized much more like the upstream > > community than a big hierarchical corporation. Just as it's not easy > > for you to control the behavior of other maintainers, it is not > > trivial for one person on the team to control what others on the team > > will do. We could make an attempt to institute rules like "all patches > > must go through internal review before being posted", but as per #2 I > > don't think this is a good idea. The ChromeOS team has even less > > control over what our partners may or may not do. In general it is > > always a struggle to get partners to even start working upstream and > > IMO it's a win when I see a partner post a patch. We should certainly > > help partners be successful here, but the right way to do that is by > > offering them support. > > I don't know who is exactly core team, who is partner. I see > "google.com" domain, so Google folks are responsible for not wasting > time of the community. If Google disagrees, please change the domain so > I will understand that and not feel like Google wants to use us all. I think it might help if you think of <company>.corp-partner.google.com addresses as gmail.com addresses. People who are using these addresses are not employees of Google nor contractors for Google; they work for their respective <company>. The main reason person@<company>.corp-partner.google.com addresses are being used for mainline submissions is because it is actually possible to set up "git send-email" with them, as their main domain typically handled by Exchange and mandates Outlook. Thanks.
Hi Krzysztof, On Wed, Oct 18, 2023 at 2:08 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 17/10/2023 11:18, Tylor Yang wrote: > > Hello, > > > > This patch series adds the driver for Himax HID-over-SPI touchscreen ICs. > > This driver takes a position in [1], it intends to take advantage of SPI > > transfer speed and HID interface. > > > > Dear Google/Chromium folks, > > As a multi-billion company I am sure you can spare some small amount of > time/effort/money for internal review before using community for this > purpose. I mean reviewing trivial issues, like coding style, or just > running checkpatch. You know, the obvious things. > > There is no need to use expensive time of community reviewers to review > very simple mistakes, the ones which we fixed in Linux kernel years ago > (also with automated tools). You can and you should do it, before > submitting drivers for community review. > > Thanks in advance. First of all, I can understand your sentiment towards some of the patches being in a very rough shape. As a community we have large volumes of patches to review and it would be really helpful if new contributors followed some basic simple steps, as described in our "Submitting patches" page... That said, it's not a fair assumption that there are no steps taken to offload the upstream reviewers community by the corporate contributors. We usually do have basic internal pre-reviews for patches coming from partners and even a pre-review bot (CoP) that can automate some of the checks such as checkpatch or bisectability. But as others said in this thread, we don't control our partners and they are free to send the patches just directly to the mailing lists if they want to do so. In a similar way, not everyone in ChromeOS is super experienced with upstream submissions, so sometimes they may not be aware of the best practices, etc. I haven't seen the patch in question, but I'd assume it's more like an exception rather than a usual pattern, so I'd appreciate it if we could avoid aggressive responses like that and try to solve the problems in a more productive way. Just a simple response with a link to https://www.kernel.org/doc/html/latest/process/submitting-patches.html wouldn't really cost you much, or actually even less than the entire litany in this email. Let's be nice to each other. Thanks. Best regards, Tomasz
On 22/01/2024 05:57, Tomasz Figa wrote: > Hi Krzysztof, > > On Wed, Oct 18, 2023 at 2:08 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 17/10/2023 11:18, Tylor Yang wrote: >>> Hello, >>> >>> This patch series adds the driver for Himax HID-over-SPI touchscreen ICs. >>> This driver takes a position in [1], it intends to take advantage of SPI >>> transfer speed and HID interface. >>> >> >> Dear Google/Chromium folks, >> >> As a multi-billion company I am sure you can spare some small amount of >> time/effort/money for internal review before using community for this >> purpose. I mean reviewing trivial issues, like coding style, or just >> running checkpatch. You know, the obvious things. >> >> There is no need to use expensive time of community reviewers to review >> very simple mistakes, the ones which we fixed in Linux kernel years ago >> (also with automated tools). You can and you should do it, before >> submitting drivers for community review. >> >> Thanks in advance. > > First of all, I can understand your sentiment towards some of the > patches being in a very rough shape. As a community we have large > volumes of patches to review and it would be really helpful if new > contributors followed some basic simple steps, as described in our > "Submitting patches" page... I don't really understand why responding to something which is three months old. Anyway, I talked with Doug on Plumbers about it so things are more or less clarified, however since two Google folks responded, let me continue. > > That said, it's not a fair assumption that there are no steps taken to > offload the upstream reviewers community by the corporate > contributors. We usually do have basic internal pre-reviews for > patches coming from partners and even a pre-review bot (CoP) that can Good to know. > automate some of the checks such as checkpatch or bisectability. But > as others said in this thread, we don't control our partners and they > are free to send the patches just directly to the mailing lists if > they want to do so. In a similar way, not everyone in ChromeOS is > super experienced with upstream submissions, so sometimes they may not > be aware of the best practices, etc. > > I haven't seen the patch in question, but I'd assume it's more like an > exception rather than a usual pattern, so I'd appreciate it if we Unfortunately that's the pattern. I was complaining few times about very poor quality of some patches from some partners before writing that email. Just to clarify: all the complains are about missing basic stuff, like running basic tools. They don't even require internal review by humans. > could avoid aggressive responses like that and try to solve the > problems in a more productive way. Just a simple response with a link > to https://www.kernel.org/doc/html/latest/process/submitting-patches.html > wouldn't really cost you much, or actually even less than the entire > litany in this email. Simple response to docs don't work. Docs are quite long and contributors questioned here just don't read them in details. Best regards, Krzysztof
On Mon, Jan 22, 2024 at 5:08 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 22/01/2024 05:57, Tomasz Figa wrote: > > Hi Krzysztof, > > > > On Wed, Oct 18, 2023 at 2:08 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 17/10/2023 11:18, Tylor Yang wrote: > >>> Hello, > >>> > >>> This patch series adds the driver for Himax HID-over-SPI touchscreen ICs. > >>> This driver takes a position in [1], it intends to take advantage of SPI > >>> transfer speed and HID interface. > >>> > >> > >> Dear Google/Chromium folks, > >> > >> As a multi-billion company I am sure you can spare some small amount of > >> time/effort/money for internal review before using community for this > >> purpose. I mean reviewing trivial issues, like coding style, or just > >> running checkpatch. You know, the obvious things. > >> > >> There is no need to use expensive time of community reviewers to review > >> very simple mistakes, the ones which we fixed in Linux kernel years ago > >> (also with automated tools). You can and you should do it, before > >> submitting drivers for community review. > >> > >> Thanks in advance. > > > > First of all, I can understand your sentiment towards some of the > > patches being in a very rough shape. As a community we have large > > volumes of patches to review and it would be really helpful if new > > contributors followed some basic simple steps, as described in our > > "Submitting patches" page... > > I don't really understand why responding to something which is three > months old. Uh, I got the reply from Dmitry 3 days ago and didn't realize it was that old. > Anyway, I talked with Doug on Plumbers about it so things > are more or less clarified, however since two Google folks responded, > let me continue. > > > > > That said, it's not a fair assumption that there are no steps taken to > > offload the upstream reviewers community by the corporate > > contributors. We usually do have basic internal pre-reviews for > > patches coming from partners and even a pre-review bot (CoP) that can > > Good to know. > > > automate some of the checks such as checkpatch or bisectability. But > > as others said in this thread, we don't control our partners and they > > are free to send the patches just directly to the mailing lists if > > they want to do so. In a similar way, not everyone in ChromeOS is > > super experienced with upstream submissions, so sometimes they may not > > be aware of the best practices, etc. > > > > I haven't seen the patch in question, but I'd assume it's more like an > > exception rather than a usual pattern, so I'd appreciate it if we > > Unfortunately that's the pattern. I was complaining few times about very > poor quality of some patches from some partners before writing that email. > > Just to clarify: all the complains are about missing basic stuff, like > running basic tools. They don't even require internal review by humans. > Hmm, that's sad then, but then also as I said, we don't control our partners, so we can't really guarantee that every single patch goes through some kind of internal review. Hopefully this has improved by now and continues to improve as the tooling I mentioned gets more widely used. > > could avoid aggressive responses like that and try to solve the > > problems in a more productive way. Just a simple response with a link > > to https://www.kernel.org/doc/html/latest/process/submitting-patches.html > > wouldn't really cost you much, or actually even less than the entire > > litany in this email. > > Simple response to docs don't work. Docs are quite long and contributors > questioned here just don't read them in details. If asking someone to read a doc doesn't lead to that person reading the doc, then that's another story. My personal experience is different, though... But well, again, we're talking about humans, so we're going to end up with a wide range of extreme cases, regardless of what we do. Tbh. I (as a kernel maintainer too) would really imagine we also have some more automated tooling for the upstream kernel workflows. I don't see anything preventing some kind of checkpatch (or whatnot) bots, responding to patches automatically. (Actually we're working on something like this for the Media subsystem...) Best regards, Tomasz
Hello, This patch series adds the driver for Himax HID-over-SPI touchscreen ICs. This driver takes a position in [1], it intends to take advantage of SPI transfer speed and HID interface. Patch 1 adds the Maintainer and devicetree bindings document for driver. Patch 2-4 adds the driver itself, Kconfig and Makefiles. Due to server could not receive patch >100000 chars, patch separate into 3 parts. [1] Overview: +--------------------------------+ | himax hid-over-spi TPIC driver | +--------------------------------+ | +-----------+ +-----------+ | +-----------+ +-----------+ | | Device #1 | | Device #i | | | Device #j | | Device #k | | +-----------+ +-----------+ | +-----------+ +-----------+ | \\ // | \\ // | +------------+ | +------------+ | | I/O Driver | | | I/O Driver | | +------------+ | +------------+ | || | || | +------------------+ | +------------------+ | | Transport Driver | | | Transport Driver | | +------------------+ | +------------------+ | \___ | ___/ +--------------------------------+ / \ / +----------------+ | HID Core | +----------------+ / | | \ / | | \ ____________/ | | \_________________ / | | \ / | | \ +----------------+ +-----------+ +------------------+ +------------------+ | Generic Driver | | MT Driver | | Custom Driver #1 | | Custom Driver #2 | +----------------+ +-----------+ +------------------+ +------------------+ version 3 changes -rename binding compatible from himax,hid-over-spi to himax,hid -rename document himax,hid-over-spi.yaml to himax,hid.yaml -rename binding properties himax,rst-gpio to reset -remove binding properties himax,irq-gpio, himax3v3-gpio and himax,fw_in_flash. -add binding properties vcca-power, vccd-power. -add binding sub-node panel and move himax,pid into this one. -HIMAX_DRIVER_VER update from 0.0.11 to 1.0.0 version 2 changes -rename bindings name from himax,hid-over-hx-spi to himax,hid-over-spi -remove "himax,fw_size", "himax,heatmap_16bits", "himax,fw_in_flash", "himax,pid" and "himax,boot_time_fw_upgrade" items from dt-bindings and related code -add "himax,id-gpios" for user to specify hardware id pins -rename "himax,ic_det_delay" to "himax,ic-det-delay-ms" and "himax,ic_resume_delay" to "himax,ic-resume-delay-ms" -MAINTAINER position adjust: move info to the position by letter order -Kconfig add HX_HID_HAS_FLASH option to replace "himax,fw_in_flash" -HIMAX_DRIVER_VER upadte from 0.0.10 to 0.0.11 -remove unused includes and description in hx_hid.c -add code to map id-gpios value to PID -correct variable naming style and misspellings in hx_ic_core.c/h -remove unnecessary global varaible debug_flag Tylor Yang (4): dt-bindings: input: Introduce Himax HID-over-SPI device HID: touchscreen: Add initial support for Himax HID-over-SPI HID: touchscreen: Add initial support for Himax HID-over-SPI HID: touchscreen: Add initial support for Himax HID-over-SPI .../devicetree/bindings/input/himax,hid.yaml | 123 + MAINTAINERS | 7 + drivers/hid/Kconfig | 2 + drivers/hid/Makefile | 2 + drivers/hid/hx-hid/Kconfig | 43 + drivers/hid/hx-hid/Makefile | 35 + 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 + drivers/hid/hx-hid/hx_ic_core.c | 3260 +++++++++++++++++ drivers/hid/hx-hid/hx_ic_core.h | 792 ++++ drivers/hid/hx-hid/hx_inspect.c | 652 ++++ drivers/hid/hx-hid/hx_inspect.h | 104 + drivers/hid/hx-hid/hx_of.c | 214 ++ drivers/hid/hx-hid/hx_plat.c | 502 +++ drivers/hid/hx-hid/hx_plat.h | 30 + 20 files changed, 9172 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/himax,hid.yaml create mode 100644 drivers/hid/hx-hid/Kconfig create mode 100644 drivers/hid/hx-hid/Makefile create mode 100644 drivers/hid/hx-hid/hx_acpi.c create mode 100644 drivers/hid/hx-hid/hx_core.c create mode 100644 drivers/hid/hx-hid/hx_core.h create mode 100644 drivers/hid/hx-hid/hx_hid.c create mode 100644 drivers/hid/hx-hid/hx_hid.h create mode 100644 drivers/hid/hx-hid/hx_ic_83102j.c create mode 100644 drivers/hid/hx-hid/hx_ic_83102j.h create mode 100644 drivers/hid/hx-hid/hx_ic_core.c create mode 100644 drivers/hid/hx-hid/hx_ic_core.h create mode 100644 drivers/hid/hx-hid/hx_inspect.c create mode 100644 drivers/hid/hx-hid/hx_inspect.h create mode 100644 drivers/hid/hx-hid/hx_of.c create mode 100644 drivers/hid/hx-hid/hx_plat.c create mode 100644 drivers/hid/hx-hid/hx_plat.h