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

[Giulio Benetti <giulio.benetti@benettiengineering.com>]

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