Message ID | 20241025114642.40793-2-charles.goodix@gmail.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | dt-bindings: input: Goodix SPI HID Touchscreen | expand |
On Fri, Oct 25, 2024 at 07:46:43PM +0800, Charles Wang wrote: > The Goodix GT7986U touch controller report touch data according to the > HID protocol through the SPI bus. However, it is incompatible with > Microsoft's HID-over-SPI protocol. > > Signed-off-by: Charles Wang <charles.goodix@gmail.com> > --- > .../bindings/input/goodix,gt7986u.yaml | 71 +++++++++++++++++++ > 1 file changed, 71 insertions(+) > create mode 100644 Documentation/devicetree/bindings/input/goodix,gt7986u.yaml > > diff --git a/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml > new file mode 100644 > index 000000000..849117639 > --- /dev/null > +++ b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml > @@ -0,0 +1,71 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/input/goodix,gt7986u.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: GOODIX GT7986U SPI HID Touchscreen GOODIX or Goodix? > + > +maintainers: > + - Charles Wang <charles.goodix@gmail.com> > + > +description: Supports the Goodix GT7986U touchscreen. > + This touch controller reports data packaged according to the HID protocol, > + but is incompatible with Microsoft's HID-over-SPI protocol. > + > +allOf: > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + > +properties: > + compatible: > + enum: > + - goodix,gt7986u-spi Compatible is already documented and nothing here explains why we should spi variant. > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + reset-gpios: > + maxItems: 1 > + > + goodix,hid-report-addr: I do not see this patch addressing previous review. Sending something like this as v1 after long discussions also does not help. No, you keep sending the same and the same, without improvements. > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + The register address for retrieving HID report data. > + This address is related to the device firmware and may > + change after a firmware update. > + > + spi-max-frequency: true > + > +additionalProperties: false Wasn't there a comment about this as well? This goes after required: block. Best regards, Krzysztof
Charles, On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > +properties: > > + compatible: > > + enum: > > + - goodix,gt7986u-spi > > Compatible is already documented and nothing here explains why we should > spi variant. > > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + reset-gpios: > > + maxItems: 1 > > + > > + goodix,hid-report-addr: > > I do not see this patch addressing previous review. Sending something > like this as v1 after long discussions also does not help. Krzysztof is right that it's better to wait until we get consensus on the previous discussion before sending a new patch. I know you were just trying to help move things forward, but because of the way the email workflow works, sending a new version tends to fork the discussion into two threads and adds confusion. I know Krzysztof and Rob have been silent during our recent discussion, but it's also a long discussion. I've been assuming that they will take some time to digest and reply in a little bit. If they didn't, IMO it would have been reasonable to explicitly ask them for feedback in the other thread after giving a bit of time. As Krzysztof mentioned, if/when you send the "goodix,gt7986u-spi" bindings again you'd want to: * Make sure it's marked as v2. * Make sure any previous review feedback has been addressed. For instance, I think Krzysztof requested that you _remove_ the goodix,hid-report-addr from the bindings and hardcode this into the driver because every GT7986U will have the same hid-report-addr. I know that kinda got lost in the discussion but it still needs to be addressed or at least responded to. I guess there was at least one other comment about "additionalProperties" that you should look for and address. * Make sure there's some type of version history after-the-cut. Tools like "patman" and "b4" can help with this. * The commit message should proactively address concerns that came up during the review process. In this case if we go with "goodix,gt7986u-spi" the commit message would want to say something explaining _why_ the "-spi" suffix is appropriate here even though normally it wouldn't be. That will help anyone digging through history. -Doug
On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote: > > Charles, > > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > +properties: > > > + compatible: > > > + enum: > > > + - goodix,gt7986u-spi > > > > Compatible is already documented and nothing here explains why we should > > spi variant. > > > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + maxItems: 1 > > > + > > > + reset-gpios: > > > + maxItems: 1 > > > + > > > + goodix,hid-report-addr: > > > > I do not see this patch addressing previous review. Sending something > > like this as v1 after long discussions also does not help. > > Krzysztof is right that it's better to wait until we get consensus on > the previous discussion before sending a new patch. I know you were > just trying to help move things forward, but because of the way the > email workflow works, sending a new version tends to fork the > discussion into two threads and adds confusion. > > I know Krzysztof and Rob have been silent during our recent > discussion, but it's also a long discussion. I've been assuming that > they will take some time to digest and reply in a little bit. If they > didn't, IMO it would have been reasonable to explicitly ask them for > feedback in the other thread after giving a bit of time. If the firmware creates fundamentally different interfaces, then different compatibles makes sense. If the same driver handles both bus interfaces, then 1 compatible should be fine. The addition of '-spi' to the compatible doesn't give any indication of a different programming model. I wouldn't care except for folks who will see it and just copy it when their only difference is the bus interface and we get to have the same discussion all over again. So if appending '-spi' is the only thing you can come up with, make it abundantly clear so that others don't blindly copy it. The commit msg is useful for convincing us, but not for that purpose. Rob
Hi, On Fri, Oct 25, 2024 at 8:59 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Charles, > > > > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > > > +properties: > > > > + compatible: > > > > + enum: > > > > + - goodix,gt7986u-spi > > > > > > Compatible is already documented and nothing here explains why we should > > > spi variant. > > > > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + interrupts: > > > > + maxItems: 1 > > > > + > > > > + reset-gpios: > > > > + maxItems: 1 > > > > + > > > > + goodix,hid-report-addr: > > > > > > I do not see this patch addressing previous review. Sending something > > > like this as v1 after long discussions also does not help. > > > > Krzysztof is right that it's better to wait until we get consensus on > > the previous discussion before sending a new patch. I know you were > > just trying to help move things forward, but because of the way the > > email workflow works, sending a new version tends to fork the > > discussion into two threads and adds confusion. > > > > I know Krzysztof and Rob have been silent during our recent > > discussion, but it's also a long discussion. I've been assuming that > > they will take some time to digest and reply in a little bit. If they > > didn't, IMO it would have been reasonable to explicitly ask them for > > feedback in the other thread after giving a bit of time. > > If the firmware creates fundamentally different interfaces, then > different compatibles makes sense. If the same driver handles both bus > interfaces, then 1 compatible should be fine. The addition of '-spi' > to the compatible doesn't give any indication of a different > programming model. I wouldn't care except for folks who will see it > and just copy it when their only difference is the bus interface and > we get to have the same discussion all over again. So if appending > '-spi' is the only thing you can come up with, make it abundantly > clear so that others don't blindly copy it. The commit msg is useful > for convincing us, but not for that purpose. OK, makes sense. Charles: Can you think of any better description for this interface than "goodix,gt7986u-spi"? I suppose you could make it super obvious that it's running different firmware with "goodix,gt7986u-spifw" and maybe that would be a little better. I think what Rob is asking for to make it super obvious is that in the "description" of the binding you mention that in this case we're running a substantially different firmware than GT7986U touchscreens represented by the "goodix,gt7986u" binding and thus is considered a distinct device. At this point, IMO you could wait until Monday in case Krzysztof wants to add his $0.02 worth and then you could send a "v2" patch addressing the comments so far, though of course you could continue to reply to this thread if you have further questions / comments. -Doug
On Fri, Oct 25, 2024 at 09:19:14AM -0700, Doug Anderson wrote: > Hi, > > On Fri, Oct 25, 2024 at 8:59 AM Rob Herring <robh@kernel.org> wrote: > > > > On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Charles, > > > > > > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > > > > > +properties: > > > > > + compatible: > > > > > + enum: > > > > > + - goodix,gt7986u-spi > > > > > > > > Compatible is already documented and nothing here explains why we should > > > > spi variant. > > > > > > > > > + > > > > > + reg: > > > > > + maxItems: 1 > > > > > + > > > > > + interrupts: > > > > > + maxItems: 1 > > > > > + > > > > > + reset-gpios: > > > > > + maxItems: 1 > > > > > + > > > > > + goodix,hid-report-addr: > > > > > > > > I do not see this patch addressing previous review. Sending something > > > > like this as v1 after long discussions also does not help. > > > > > > Krzysztof is right that it's better to wait until we get consensus on > > > the previous discussion before sending a new patch. I know you were > > > just trying to help move things forward, but because of the way the > > > email workflow works, sending a new version tends to fork the > > > discussion into two threads and adds confusion. > > > > > > I know Krzysztof and Rob have been silent during our recent > > > discussion, but it's also a long discussion. I've been assuming that > > > they will take some time to digest and reply in a little bit. If they > > > didn't, IMO it would have been reasonable to explicitly ask them for > > > feedback in the other thread after giving a bit of time. > > > > If the firmware creates fundamentally different interfaces, then > > different compatibles makes sense. If the same driver handles both bus > > interfaces, then 1 compatible should be fine. The addition of '-spi' > > to the compatible doesn't give any indication of a different > > programming model. I wouldn't care except for folks who will see it > > and just copy it when their only difference is the bus interface and > > we get to have the same discussion all over again. So if appending > > '-spi' is the only thing you can come up with, make it abundantly > > clear so that others don't blindly copy it. The commit msg is useful > > for convincing us, but not for that purpose. > > OK, makes sense. Charles: Can you think of any better description for > this interface than "goodix,gt7986u-spi"? I suppose you could make it > super obvious that it's running different firmware with > "goodix,gt7986u-spifw" and maybe that would be a little better. Is there any chance for Microsoft-compatible HID-over-SPI versions of the firmware for this chip? Will this require new compatible string? Or it will be a different chip ID and the issue will be moot? Thanks.
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml new file mode 100644 index 000000000..849117639 --- /dev/null +++ b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml @@ -0,0 +1,71 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/input/goodix,gt7986u.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: GOODIX GT7986U SPI HID Touchscreen + +maintainers: + - Charles Wang <charles.goodix@gmail.com> + +description: Supports the Goodix GT7986U touchscreen. + This touch controller reports data packaged according to the HID protocol, + but is incompatible with Microsoft's HID-over-SPI protocol. + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + +properties: + compatible: + enum: + - goodix,gt7986u-spi + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + reset-gpios: + maxItems: 1 + + goodix,hid-report-addr: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + The register address for retrieving HID report data. + This address is related to the device firmware and may + change after a firmware update. + + spi-max-frequency: true + +additionalProperties: false + +required: + - compatible + - reg + - interrupts + - reset-gpios + - goodix,hid-report-addr + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/gpio/gpio.h> + + spi { + #address-cells = <1>; + #size-cells = <0>; + + touchscreen@0 { + compatible = "goodix,gt7986u-spi"; + reg = <0>; + interrupt-parent = <&gpio>; + interrupts = <25 IRQ_TYPE_LEVEL_LOW>; + reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>; + spi-max-frequency = <10000000>; + goodix,hid-report-addr = <0x22c8c>; + }; + }; + +...
The Goodix GT7986U touch controller report touch data according to the HID protocol through the SPI bus. However, it is incompatible with Microsoft's HID-over-SPI protocol. Signed-off-by: Charles Wang <charles.goodix@gmail.com> --- .../bindings/input/goodix,gt7986u.yaml | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/goodix,gt7986u.yaml