Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 2 Apr 2021 10:36:44 +0200
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Giulio Benetti <giulio.benetti@benettiengineering.com>
Cc:     Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Henrik Rydberg <rydberg@bitmath.org>,
        linux-input@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.ne@posteo.net>
Subject: Re: [PATCH v2 2/3] dt-bindings: touchscreen: Add HY46XX bindings
Message-ID: <YGbXnGTpx1WTPpz0@latitude>
References: <20210306194120.GA1075725@robh.at.kernel.org>
 <20210401230358.2468618-1-giulio.benetti@benettiengineering.com>
 <20210401230358.2468618-3-giulio.benetti@benettiengineering.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="taR3GFVu4MBwJP54"
Content-Disposition: inline
In-Reply-To: <20210401230358.2468618-3-giulio.benetti@benettiengineering.com>
Precedence: bulk


--taR3GFVu4MBwJP54
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Apr 02, 2021 at 01:03:57AM +0200, Giulio Benetti wrote:
> This adds device tree bindings for the Hycon HY46XX touchscreen series.
>=20
> Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.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 driv=
er
> ---
>  .../input/touchscreen/hycon,hy46xx.yaml       | 120 ++++++++++++++++++
>  MAINTAINERS                                   |   6 +
>  2 files changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/h=
ycon,hy46xx.yaml
>=20
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/hycon,hy=
46xx.yaml b/Documentation/devicetree/bindings/input/touchscreen/hycon,hy46x=
x.yaml
> new file mode 100644
> index 000000000000..71a1dbabcd4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/hycon,hy46xx.ya=
ml
> @@ -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 si=
zes 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 =3D> 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 =3D> 100Hz).

Or perhaps like this:

+    description: Allows setting the report speed in Hertz.


Thanks,
Jonathan Neusch=C3=A4fer

--taR3GFVu4MBwJP54
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAmBm14sACgkQCDBEmo7z
X9uLlQ/9GV+3KAqRp5/lDFRCT2Chuaw1xKxClHon1fMiSSzMz3V9H+0zUgobDM3i
aOe/fkprdekhBhoJ3JK3iQFjFgh+xED9mVkcq5FMZOTWcwB7rtrZrWm2Gfw2VoWV
WipwZ4VZWoAISplWlsfrSyfBnXU731ttYvlGe9ddzrxmMwhm9CSYDKhOm1YCpsPP
+VSPcn0nQIaY83vkZ2yj6xznF7yiG+YoF+hpSxef8b08py+TR576E7n8t1udth5p
qvIxyKfyovVhCQetGnUnscGvX3qZEUrFOdajXBnWewqnUB0YsZsq0KaD1FhZhtb9
x2dUJes7rlVDTGpDhb5MddfiEd47UzPy1Fa2bpXg5/OExvtSubF4rUSBcCV7Nc+5
cLoW1DVQE4RfwVFnxTg6VCCl12CzS1q5vbF0mvhEb73+y/V7sNlg9gk3l7T+ZKK6
4mDbNZM8vD70h2u/Qop7h3fUAsR7o1Tl/fSfndjECsIUxWU5XCCoK0p5qUBnRwdZ
9l5NxOeWFlaM5oQ2K5SR4EOWcVxdSoiqmCzHYFANFvYf8DbQyII8qWRiQvw6ualq
jQRDZhUHNKn/ms+4yHD91rWfjWyfJZqt98qaBljWvUrahAkIw6j1tAmyZeHsRwiM
ud6+SttlUQx3H+xzqglDb3QdK8vvQKNS1tnc+2Tpgf4fVPrC/co=
=rsUz
-----END PGP SIGNATURE-----

--taR3GFVu4MBwJP54--