linux-kernel - Re: [PATCH v2 2/3] dt-bindings: touchscreen: Add HY46XX bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YGbXnGTpx1WTPpz0@latitude>
Date:   Fri, 2 Apr 2021 10:36:44 +0200
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Giulio Benetti <giulio.benetti@...ettiengineering.com>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Henrik Rydberg <rydberg@...math.org>,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Jonathan Neuschäfer <j.ne@...teo.net>
Subject: Re: [PATCH v2 2/3] dt-bindings: touchscreen: Add HY46XX bindings

On Fri, Apr 02, 2021 at 01:03:57AM +0200, Giulio Benetti wrote:
> This adds device tree bindings for the Hycon HY46XX touchscreen series.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@...ettiengineering.com>
> ---
> V1->V2:
> As suggested by Rob Herring:
> * fixed $id: address
> * added "hycon," in front of every custom property
> * changed all possible property to boolean type
> * removed proximity-sensor-switch property since it's not handled in driver
> ---
>  .../input/touchscreen/hycon,hy46xx.yaml       | 120 ++++++++++++++++++
>  MAINTAINERS                                   |   6 +
>  2 files changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/hycon,hy46xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/hycon,hy46xx.yaml b/Documentation/devicetree/bindings/input/touchscreen/hycon,hy46xx.yaml
> new file mode 100644
> index 000000000000..71a1dbabcd4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/hycon,hy46xx.yaml
> @@ -0,0 +1,120 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/hycon,hy46xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: hycon HY46XX series touchscreen controller Bindings

hycon -> Hycon        (because it's a name)
Bindings -> bindings  (because it's just a regular word)

> +
> +description: |
> +             There are 6 variants of the chip for various touch panel sizes and coverl len material

"coverl len material" looks like a typo. What does it mean?

> +              Glass: 0.3mm--4.0mm
> +              PET/PMMA: 0.2mm--2.0mm
> +               HY4613(B)-N048  < 6"
> +               HY4614(B)-N068  7" .. 10.1"

According to the datasheet I was able to find[1], HY4613-N048 supports
touch panel sizes smaller than 5.3". Did this change in newer
datasheets?

What does the (B) part of the part number mean?


[1]: https://datasheetspdf.com/pdf/1297773/HYCON/HY4613-N048/1

> +               HY4621-NS32  < 5"
> +               HY4623-NS48  5.1" .. 7"
> +              Glass: 0.3mm--8.0mm
> +              PET/PMMA: 0.2mm--4.0mm
> +               HY4633(B)-N048  < 6"
> +               HY4635(B)-N048  < 7" .. 10.1"

The description block seems unusually far indented. I'm not a YAML
expert, but according to the yamllint tool, it would work with much less
indentation:

description: |
  There are 6 variants of the chip for various touch panel sizes and coverl len material
   Glass: 0.3mm--4.0mm
[...]


> +  hycon,glove-enable:
> +    type: boolean
> +    description: Allows enabling or disabling glove setting.

Small nit: Due to the way boolean properties work in DT, you can't
really use the property to disable the glove setting (in order to
disable the setting, you would explicitly not use the property).
Perhaps:

+    description: Allows enabling the glove setting.

I don't really know :)


> +
> +  hycon,report-speed:
> +    description: Allows setting the report speed(i.e 0x64 => 100Hz).
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 255

Please add a space before the opening parenthesis:

+    description: Allows setting the report speed (i.e 0x64 => 100Hz).

Or perhaps like this:

+    description: Allows setting the report speed in Hertz.



Thanks,
Jonathan Neuschäfer

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)