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
+
+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>;
+      };
+    };
+
+...
-- 
2.43.0

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

Hi,

On Fri, Oct 25, 2024 at 02:03:11PM +0200, Krzysztof Kozlowski wrote:
> 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?

Thank you for catching this, the name should be 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.
> 

Ack. I will modify the compatible name based on our discussion and include an
appropriate description.

>
> > +
> > +  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.
>

I apologize for overlooking the discussions regarding this issue.

I would like to clarify that while the current boards use the same address,
but newly designed boards in the future may require different addresses.

Retaining this property would likely offer more flexibility.

>
> > +    $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.
>

Ack, will change to unevaluatedProperties in the next version.

Best regards,
Charles

Hi,

On Wed, Oct 30, 2024 at 7:37 PM Charles Wang <charles.goodix@gmail.com> wrote:
>
> > > +  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.
> >
>
> I apologize for overlooking the discussions regarding this issue.
>
> I would like to clarify that while the current boards use the same address,
> but newly designed boards in the future may require different addresses.
>
> Retaining this property would likely offer more flexibility.

I don't feel very strongly about it, but maybe Krzysztof does?
Possibly the path of least resistance would be:

1. You drop the property from the bindings.

2. You hardcode it in the driver to be the normal value.

3. If/when someone actually needs a different value then we can add it
as an optional property in the bindings and fall back to the default
value if the property isn't present.

What do you think? If you feel strongly about keeping the
"hid-report-addr" then you can certainly keep making your case.
However, it's probably best to wait to get agreement from Krzysztof
(or one of the other DT maintainers) before sending your next version
unless you're going to take the "path of least resistance" that I talk
about above.

-Doug

Hi Doug,

On Thu, Oct 31, 2024 at 10:58:07AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Oct 30, 2024 at 7:37 PM Charles Wang <charles.goodix@gmail.com> wrote:
> >
> > > > +  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.
> > >
> >
> > I apologize for overlooking the discussions regarding this issue.
> >
> > I would like to clarify that while the current boards use the same address,
> > but newly designed boards in the future may require different addresses.
> >
> > Retaining this property would likely offer more flexibility.
> 
> I don't feel very strongly about it, but maybe Krzysztof does?
> Possibly the path of least resistance would be:
> 
> 1. You drop the property from the bindings.
> 
> 2. You hardcode it in the driver to be the normal value.
> 
> 3. If/when someone actually needs a different value then we can add it
> as an optional property in the bindings and fall back to the default
> value if the property isn't present.
> 
> What do you think? If you feel strongly about keeping the
> "hid-report-addr" then you can certainly keep making your case.
> However, it's probably best to wait to get agreement from Krzysztof
> (or one of the other DT maintainers) before sending your next version
> unless you're going to take the "path of least resistance" that I talk
> about above.
>

Agreed, let's wait and see the opinions of Krzysztof (or the other DT
maintainers).

Thanks,
Charles

Charles,

On Thu, Oct 31, 2024 at 6:33 PM Charles Wang <charles.goodix@gmail.com> wrote:
>
> Hi Doug,
>
> On Thu, Oct 31, 2024 at 10:58:07AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Oct 30, 2024 at 7:37 PM Charles Wang <charles.goodix@gmail.com> wrote:
> > >
> > > > > +  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.
> > > >
> > >
> > > I apologize for overlooking the discussions regarding this issue.
> > >
> > > I would like to clarify that while the current boards use the same address,
> > > but newly designed boards in the future may require different addresses.
> > >
> > > Retaining this property would likely offer more flexibility.
> >
> > I don't feel very strongly about it, but maybe Krzysztof does?
> > Possibly the path of least resistance would be:
> >
> > 1. You drop the property from the bindings.
> >
> > 2. You hardcode it in the driver to be the normal value.
> >
> > 3. If/when someone actually needs a different value then we can add it
> > as an optional property in the bindings and fall back to the default
> > value if the property isn't present.
> >
> > What do you think? If you feel strongly about keeping the
> > "hid-report-addr" then you can certainly keep making your case.
> > However, it's probably best to wait to get agreement from Krzysztof
> > (or one of the other DT maintainers) before sending your next version
> > unless you're going to take the "path of least resistance" that I talk
> > about above.
> >
>
> Agreed, let's wait and see the opinions of Krzysztof (or the other DT
> maintainers).

As I went back and looked at this again, I'm curious: I don't know
much about how your protocol works, but is there any reason why your
driver can't figure out this "hid-report-addr" dynamically? Is there
some reason you can't talk to the device and ask it what the
"hid-report-addr" should be? From skimming through your driver there
appear to already be a few hardcoded addresses. Can one of those
provide you the info you need?


-Doug

On Mon, Nov 04, 2024 at 11:36:50AM -0800, Doug Anderson wrote:
> Charles,
> 
> On Thu, Oct 31, 2024 at 6:33 PM Charles Wang <charles.goodix@gmail.com> wrote:
> >
> > Hi Doug,
> >
> > On Thu, Oct 31, 2024 at 10:58:07AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Wed, Oct 30, 2024 at 7:37 PM Charles Wang <charles.goodix@gmail.com> wrote:
> > > >
> > > > > > +  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.
> > > > >
> > > >
> > > > I apologize for overlooking the discussions regarding this issue.
> > > >
> > > > I would like to clarify that while the current boards use the same address,
> > > > but newly designed boards in the future may require different addresses.
> > > >
> > > > Retaining this property would likely offer more flexibility.
> > >
> > > I don't feel very strongly about it, but maybe Krzysztof does?
> > > Possibly the path of least resistance would be:
> > >
> > > 1. You drop the property from the bindings.
> > >
> > > 2. You hardcode it in the driver to be the normal value.
> > >
> > > 3. If/when someone actually needs a different value then we can add it
> > > as an optional property in the bindings and fall back to the default
> > > value if the property isn't present.
> > >
> > > What do you think? If you feel strongly about keeping the
> > > "hid-report-addr" then you can certainly keep making your case.
> > > However, it's probably best to wait to get agreement from Krzysztof
> > > (or one of the other DT maintainers) before sending your next version
> > > unless you're going to take the "path of least resistance" that I talk
> > > about above.
> > >
> >
> > Agreed, let's wait and see the opinions of Krzysztof (or the other DT
> > maintainers).
> 
> As I went back and looked at this again, I'm curious: I don't know
> much about how your protocol works, but is there any reason why your
> driver can't figure out this "hid-report-addr" dynamically? Is there
> some reason you can't talk to the device and ask it what the
> "hid-report-addr" should be? From skimming through your driver there
> appear to already be a few hardcoded addresses. Can one of those
> provide you the info you need?
> 

Similar to a standard i2c-hid driver, which requires configuring the
address for hid-descr-addr in the DTS, other address information is
obtained using this address.

In theory, we can dynamically obtain most of the addresses from the chip.
However, for this chip, there always needs to be a known address for the
driver to communicate with, whether this address is 0x0000 or 0x0001,
and this address is related to the firmware.

Regarding this issue, since no further review comments received.
I think I can first remove the address as your previous suggestion
and use a default address for communication in the driver. In the
future, we can upgrade the firmware and driver to achieve dynamic
address acquisition.

Thanks,
Charles

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

Hi Doug,

On Fri, Oct 25, 2024 at 08:29:13AM -0700, Doug Anderson 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.
> 
> 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.
> 

I apologize for any confusion caused. As a newcomer, I am still learning
about the community practices.

Thank you very much for your patience and clear explanation. I will recheck
the previous review feedback and provide a new patch marked as v2.

Best regards,
Charles

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.
> 
> 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.
> 

Thank you for your explanation, I understand your point. I want to clarify
that the gt7986u-spi and gt7986u indeed use two entirely different drivers
and two distinct firmware.

Using "goodix,gt7986u-spi" could indeed cause confusion. How about modifying
it to "goodix,gt7986u-losto" by adding a special code?

Additionally, I would like to confirm: when submitting the v2 patch, should
it be based on this thread or the previous discussion thread?

Best regards,
Charles
 

Hi,

On Tue, Oct 29, 2024 at 11:57 PM Charles Wang <charles.goodix@gmail.com> wrote:
>
> 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.
> >
> > 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.
> >
>
> Thank you for your explanation, I understand your point. I want to clarify
> that the gt7986u-spi and gt7986u indeed use two entirely different drivers
> and two distinct firmware.
>
> Using "goodix,gt7986u-spi" could indeed cause confusion. How about modifying
> it to "goodix,gt7986u-losto" by adding a special code?

If "lotso" somehow means something real to people using this product
then that seems OK to me. If "lotso" is just a made up word because
you don't want to use "spi" or "spifw" then it's not great. In either
case you'll want to summarize our discussion here in your
"description" in the yaml and in the commit message.


> Additionally, I would like to confirm: when submitting the v2 patch, should
> it be based on this thread or the previous discussion thread?

No, v2 should _not_ be In-Reply-To this thread. It'll start a new
thread. You can add a link (via lore.kernel.org/r/<message-id>) to the
old discussion in your cover letter and/or version history.

Said another way:
* New versions of patches create new threads.
* The fact that new versions of patches create new threads is why
people usually want open questions answered before the next version is
sent.

:-)

On Wed, Oct 30, 2024 at 11:14:26AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Oct 29, 2024 at 11:57 PM Charles Wang <charles.goodix@gmail.com> wrote:
> >
> > 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.
> > >
> > > 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.
> > >
> >
> > Thank you for your explanation, I understand your point. I want to clarify
> > that the gt7986u-spi and gt7986u indeed use two entirely different drivers
> > and two distinct firmware.
> >
> > Using "goodix,gt7986u-spi" could indeed cause confusion. How about modifying
> > it to "goodix,gt7986u-losto" by adding a special code?
> 
> If "lotso" somehow means something real to people using this product
> then that seems OK to me. If "lotso" is just a made up word because
> you don't want to use "spi" or "spifw" then it's not great. In either
> case you'll want to summarize our discussion here in your
> "description" in the yaml and in the commit message.
> 

Okay, got it.

> 
> > Additionally, I would like to confirm: when submitting the v2 patch, should
> > it be based on this thread or the previous discussion thread?
> 
> No, v2 should _not_ be In-Reply-To this thread. It'll start a new
> thread. You can add a link (via lore.kernel.org/r/<message-id>) to the
> old discussion in your cover letter and/or version history.
> 
> Said another way:
> * New versions of patches create new threads.
> * The fact that new versions of patches create new threads is why
> people usually want open questions answered before the next version is
> sent.
> 

Okay, thank you very much for your patient explanation.

Best regards,
Charles

On 25/10/2024 18:19, 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.
> 
> 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.

And to re-iterate: both commit msg and hardware description in the
binding must clearly explain this why another compatible was chosen for
the same device.

Best regards,
Krzysztof

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.

-- 
Dmitry

Hi Dmitry,

On Fri, Oct 25, 2024 at 10:14:52AM -0700, Dmitry Torokhov wrote:
> 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?
> 
No, the SPI hardware design of this chip does not meet the requirements for
the Microsoft HID-over-SPI protocol. A new chip with a new ID will be
developed to support the Microsoft HID-over-SPI protocol.

Thanks,
Charles