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

[Giulio Benetti]

Hi Jonathan,

thank you for reviewing,

On 4/2/21 10:36 AM, Jonathan Neuschäfer wrote:
> 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@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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)

ok

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

yes

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

Yes, there is a newer document called "HY46XX Application Notes
V4.0", here is the screenshot of models:
https://pasteboard.co/JVtOMcO.png

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

It's an upgraded version even if hardware is fully compatible, so (B) 
means with and without "B".

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

Ah yes, I've started from edt,ft5x06.yaml and there it was indented that 
way. Anyway I've changed it.

> > +  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 :)

Ah yes, it's true. If not specified it's simply not enabled, so I use 
your suggested form.

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

Oh yes! I didn't realize it was exactly in Hertz, 0x64 is 100, not only 
equivalent to 100Hz, so every step is 1 Hz

> Thanks,
> Jonathan Neuschäfer

Thanks again!
Best regards
--
Giulio Benetti
Benetti Engineering sas