diff mbox series

[v4,3/3] dt-bindings: touchscreen: Add binding for Novatek NT36xxx series driver

Message ID 20201008181514.668548-4-kholk11@gmail.com
State Superseded
Headers show
Series Add Novatek NT36xxx touchscreen driver | expand

Commit Message

AngeloGioacchino Del Regno Oct. 8, 2020, 6:15 p.m. UTC 
From: AngeloGioacchino Del Regno <kholk11@gmail.com>

Add binding for the Novatek NT36xxx series touchscreen driver.

Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
---
 .../input/touchscreen/novatek,nt36xxx.yaml    | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml

Comments

Krzysztof Kozlowski Oct. 8, 2020, 6:21 p.m. UTC | #1 
On Thu, 8 Oct 2020 at 20:15, <kholk11@gmail.com> wrote:
>
> From: AngeloGioacchino Del Regno <kholk11@gmail.com>
>
> Add binding for the Novatek NT36xxx series touchscreen driver.
>
> Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> ---
>  .../input/touchscreen/novatek,nt36xxx.yaml    | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> new file mode 100644
> index 000000000000..e747cacae036
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/novatek,nt36xxx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Novatek NT36xxx series touchscreen controller Bindings
> +
> +maintainers:
> +  - Dmitry Torokhov <dmitry.torokhov@gmail.com>
> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +
> +properties:
> +  compatible:
> +    const: novatek,nt36xxx

Thanks for the changes, they look good except this part here which I
missed before. The compatible should not contain wildcards. If all
devices are really compatible, just add here one const, e.g. "const:
novatek,nt36525". If they are different, you could add multiple
compatibles in enum.

Best regards,
Krzysztof
AngeloGioacchino Del Regno Oct. 8, 2020, 8:30 p.m. UTC | #2 
Il giorno gio 8 ott 2020 alle ore 20:21 Krzysztof Kozlowski
<krzk@kernel.org> ha scritto:
>
> On Thu, 8 Oct 2020 at 20:15, <kholk11@gmail.com> wrote:
> >
> > From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> >
> > Add binding for the Novatek NT36xxx series touchscreen driver.
> >
> > Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> > ---
> >  .../input/touchscreen/novatek,nt36xxx.yaml    | 59 +++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> > new file mode 100644
> > index 000000000000..e747cacae036
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/touchscreen/novatek,nt36xxx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Novatek NT36xxx series touchscreen controller Bindings
> > +
> > +maintainers:
> > +  - Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > +
> > +allOf:
> > +  - $ref: touchscreen.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: novatek,nt36xxx
>
> Thanks for the changes, they look good except this part here which I
> missed before. The compatible should not contain wildcards. If all
> devices are really compatible, just add here one const, e.g. "const:
> novatek,nt36525". If they are different, you could add multiple
> compatibles in enum.
>
> Best regards,
> Krzysztof

They are all managed the same way, but the page addresses are
changing between all of them... the driver is reading the chip ID
while the TS MCU is in "boot mode", then checking in a ID table
if the chip is supported and finally assigning a page address table.
This is done for the entire NT36*** series.

If wildcards are not permitted, perhaps I can change it to something
like "novatek,nt36" or "novatek,nt36-ts"... as then specifying the
specific IC model into the DT means that I would have to logically
change the driver itself to also crosscheck a DT-specified model
with whatever gets recognized by reading the chip (which then would
be a triple check of what's going on, imo overcomplicating the logic).

What would you propose, at this point?
Krzysztof Kozlowski Oct. 12, 2020, 4:52 p.m. UTC | #3 
On Thu, Oct 08, 2020 at 10:30:35PM +0200, AngeloGioacchino Del Regno wrote:
> Il giorno gio 8 ott 2020 alle ore 20:21 Krzysztof Kozlowski
> <krzk@kernel.org> ha scritto:
> >
> > On Thu, 8 Oct 2020 at 20:15, <kholk11@gmail.com> wrote:
> > >
> > > From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> > >
> > > Add binding for the Novatek NT36xxx series touchscreen driver.
> > >
> > > Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> > > ---
> > >  .../input/touchscreen/novatek,nt36xxx.yaml    | 59 +++++++++++++++++++
> > >  1 file changed, 59 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> > > new file mode 100644
> > > index 000000000000..e747cacae036
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> > > @@ -0,0 +1,59 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/input/touchscreen/novatek,nt36xxx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Novatek NT36xxx series touchscreen controller Bindings
> > > +
> > > +maintainers:
> > > +  - Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > +
> > > +allOf:
> > > +  - $ref: touchscreen.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: novatek,nt36xxx
> >
> > Thanks for the changes, they look good except this part here which I
> > missed before. The compatible should not contain wildcards. If all
> > devices are really compatible, just add here one const, e.g. "const:
> > novatek,nt36525". If they are different, you could add multiple
> > compatibles in enum.
> >
> > Best regards,
> > Krzysztof
> 
> They are all managed the same way, but the page addresses are
> changing between all of them... the driver is reading the chip ID
> while the TS MCU is in "boot mode", then checking in a ID table
> if the chip is supported and finally assigning a page address table.
> This is done for the entire NT36*** series.
> 
> If wildcards are not permitted, perhaps I can change it to something
> like "novatek,nt36" or "novatek,nt36-ts"... as then specifying the
> specific IC model into the DT means that I would have to logically
> change the driver itself to also crosscheck a DT-specified model
> with whatever gets recognized by reading the chip (which then would
> be a triple check of what's going on, imo overcomplicating the logic).
> 
> What would you propose, at this point?

If you want the autodetection based on chip ID, then use the
oldest/earliest device as compatible, so "novatek,nt36525" and keep
everything else as is. In your case the HW description for all devices
is the same, thus one compatible is enough.  This way if in future you
need to bring a difference for a new HW (let's say some imaginary
NT36999), you can simply add a new compatible.

Best regards,
Krzysztof
Rob Herring Oct. 13, 2020, 2:42 p.m. UTC | #4 
On Thu, Oct 08, 2020 at 10:30:35PM +0200, AngeloGioacchino Del Regno wrote:
> Il giorno gio 8 ott 2020 alle ore 20:21 Krzysztof Kozlowski
> <krzk@kernel.org> ha scritto:
> >
> > On Thu, 8 Oct 2020 at 20:15, <kholk11@gmail.com> wrote:
> > >
> > > From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> > >
> > > Add binding for the Novatek NT36xxx series touchscreen driver.
> > >
> > > Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> > > ---
> > >  .../input/touchscreen/novatek,nt36xxx.yaml    | 59 +++++++++++++++++++
> > >  1 file changed, 59 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> > > new file mode 100644
> > > index 000000000000..e747cacae036
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> > > @@ -0,0 +1,59 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/input/touchscreen/novatek,nt36xxx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Novatek NT36xxx series touchscreen controller Bindings
> > > +
> > > +maintainers:
> > > +  - Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > +
> > > +allOf:
> > > +  - $ref: touchscreen.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: novatek,nt36xxx
> >
> > Thanks for the changes, they look good except this part here which I
> > missed before. The compatible should not contain wildcards. If all
> > devices are really compatible, just add here one const, e.g. "const:
> > novatek,nt36525". If they are different, you could add multiple
> > compatibles in enum.
> >
> > Best regards,
> > Krzysztof
> 
> They are all managed the same way, but the page addresses are
> changing between all of them... the driver is reading the chip ID
> while the TS MCU is in "boot mode", then checking in a ID table
> if the chip is supported and finally assigning a page address table.
> This is done for the entire NT36*** series.

The important part is whether everything needed to read the chip ID 
is identical? Same power supplies and sequencing, clocks, resets, 
enables, etc.? If any of those vary then you'll need something more 
specific. You can always have a common fallback.

Rob
Rob Herring Oct. 13, 2020, 2:45 p.m. UTC | #5 
On Thu, Oct 08, 2020 at 08:15:14PM +0200, kholk11@gmail.com wrote:
> From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> 
> Add binding for the Novatek NT36xxx series touchscreen driver.
> 
> Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> ---
>  .../input/touchscreen/novatek,nt36xxx.yaml    | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> new file mode 100644
> index 000000000000..e747cacae036
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/novatek,nt36xxx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Novatek NT36xxx series touchscreen controller Bindings
> +
> +maintainers:
> +  - Dmitry Torokhov <dmitry.torokhov@gmail.com>

This should be an owner for this device, not subsystem maintainers.

> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +
> +properties:
> +  compatible:
> +    const: novatek,nt36xxx
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpio:

reset-gpios

> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: Power supply regulator for VDD pin
> +
> +  vio-supply:
> +    description: Power supply regulator on VDD-IO pin
> +
> +additionalProperties: false

This won't work with the ref to touchscreen.yaml. Use 
'unevaluatedProperties: false' instead.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      touchscreen@62 {
> +        compatible = "novatek,nt36xxx";
> +        reg = <0x62>;
> +        interrupt-parent = <&tlmm>;
> +        interrupts = <45 IRQ_TYPE_EDGE_RISING>;
> +        reset-gpio = <&tlmm 102 GPIO_ACTIVE_HIGH>;
> +      };
> +    };
> +
> +...
> -- 
> 2.28.0
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
new file mode 100644
index 000000000000..e747cacae036
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt36xxx.yaml
@@ -0,0 +1,59 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/novatek,nt36xxx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Novatek NT36xxx series touchscreen controller Bindings
+
+maintainers:
+  - Dmitry Torokhov <dmitry.torokhov@gmail.com>
+
+allOf:
+  - $ref: touchscreen.yaml#
+
+properties:
+  compatible:
+    const: novatek,nt36xxx
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpio:
+    maxItems: 1
+
+  vdd-supply:
+    description: Power supply regulator for VDD pin
+
+  vio-supply:
+    description: Power supply regulator on VDD-IO pin
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      touchscreen@62 {
+        compatible = "novatek,nt36xxx";
+        reg = <0x62>;
+        interrupt-parent = <&tlmm>;
+        interrupts = <45 IRQ_TYPE_EDGE_RISING>;
+        reset-gpio = <&tlmm 102 GPIO_ACTIVE_HIGH>;
+      };
+    };
+
+...