2023-06-05 11:05:48

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

On 05/06/2023 08:05, Cong Yang wrote:
> Add an ilitek touch screen chip ili9882t.
>
> Signed-off-by: Cong Yang <[email protected]>
> ---
> .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> index 05e6f2df604c..f0e7ffdce605 100644
> --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> @@ -15,11 +15,14 @@ description:
>
> properties:
> compatible:
> - items:
> - - const: elan,ekth6915
> + enum:
> + - elan,ekth6915
> + - ilitek,ili9882t
>
> reg:
> - const: 0x10
> + enum:
> + - 0x10
> + - 0x41
>
> interrupts:
> maxItems: 1
> @@ -29,11 +32,13 @@ properties:
>
> vcc33-supply:
> description: The 3.3V supply to the touchscreen.
> + If using ili9882t then this supply will not be needed.

What does it mean "will not be needed"? Describe the hardware, not your
drivers.

I don't think you tested your DTS. Submit DTS users, because I do not
believe you are testing your patches. You already got such comment and I
don't see much of improvements here.

>
> vccio-supply:
> description:
> The IO supply to the touchscreen. Need not be specified if this is the
> same as the 3.3V supply.
> + If using ili9882t, the IO supply is required.

Don't repeat constraints in free form text.
>
> required:
> - compatible
> @@ -41,6 +46,18 @@ required:
> - interrupts
> - vcc33-supply
>
> +if:

Keep it in allOf. Will save you one indentation later.

> + properties:
> + compatible:
> + contains:
> + const: ilitek,ili9882t
> +then:
> + required:
> + - compatible
> + - reg
> + - interrupts

Don't duplicate.

> + - vccio-supply


Best regards,
Krzysztof



2023-06-06 03:12:29

by cong yang

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

Hi,Krzysztof

On Mon, Jun 5, 2023 at 6:34 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 05/06/2023 08:05, Cong Yang wrote:
> > Add an ilitek touch screen chip ili9882t.
> >
> > Signed-off-by: Cong Yang <[email protected]>
> > ---
> > .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > index 05e6f2df604c..f0e7ffdce605 100644
> > --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > @@ -15,11 +15,14 @@ description:
> >
> > properties:
> > compatible:
> > - items:
> > - - const: elan,ekth6915
> > + enum:
> > + - elan,ekth6915
> > + - ilitek,ili9882t
> >
> > reg:
> > - const: 0x10
> > + enum:
> > + - 0x10
> > + - 0x41
> >
> > interrupts:
> > maxItems: 1
> > @@ -29,11 +32,13 @@ properties:
> >
> > vcc33-supply:
> > description: The 3.3V supply to the touchscreen.
> > + If using ili9882t then this supply will not be needed.
>
> What does it mean "will not be needed"? Describe the hardware, not your
> drivers.
>
> I don't think you tested your DTS. Submit DTS users, because I do not
> believe you are testing your patches. You already got such comment and I
> don't see much of improvements here.

I ran make dt_binding_check in the codebase root directory before
sending the V2 Patch, and there were no errors or warnings (the V1
version run reported some errors). Is there some other way to test DTS
?

>
> >
> > vccio-supply:
> > description:
> > The IO supply to the touchscreen. Need not be specified if this is the
> > same as the 3.3V supply.
> > + If using ili9882t, the IO supply is required.
>
> Don't repeat constraints in free form text.

Got it.thanks.

> >
> > required:
> > - compatible
> > @@ -41,6 +46,18 @@ required:
> > - interrupts
> > - vcc33-supply
> >
> > +if:
>
> Keep it in allOf. Will save you one indentation later.

Got it.thanks.

>
> > + properties:
> > + compatible:
> > + contains:
> > + const: ilitek,ili9882t
> > +then:
> > + required:
> > + - compatible
> > + - reg
> > + - interrupts
>
> Don't duplicate.

Got it.thanks.

>
> > + - vccio-supply
>
>
> Best regards,
> Krzysztof
>

2023-06-06 06:39:44

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

On 06/06/2023 04:18, cong yang wrote:
> Hi,Krzysztof
>
> On Mon, Jun 5, 2023 at 6:34 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 05/06/2023 08:05, Cong Yang wrote:
>>> Add an ilitek touch screen chip ili9882t.
>>>
>>> Signed-off-by: Cong Yang <[email protected]>
>>> ---
>>> .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
>>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
>>> index 05e6f2df604c..f0e7ffdce605 100644
>>> --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
>>> +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
>>> @@ -15,11 +15,14 @@ description:
>>>
>>> properties:
>>> compatible:
>>> - items:
>>> - - const: elan,ekth6915
>>> + enum:
>>> + - elan,ekth6915
>>> + - ilitek,ili9882t
>>>
>>> reg:
>>> - const: 0x10
>>> + enum:
>>> + - 0x10
>>> + - 0x41
>>>
>>> interrupts:
>>> maxItems: 1
>>> @@ -29,11 +32,13 @@ properties:
>>>
>>> vcc33-supply:
>>> description: The 3.3V supply to the touchscreen.
>>> + If using ili9882t then this supply will not be needed.
>>
>> What does it mean "will not be needed"? Describe the hardware, not your
>> drivers.
>>
>> I don't think you tested your DTS. Submit DTS users, because I do not
>> believe you are testing your patches. You already got such comment and I
>> don't see much of improvements here.
>
> I ran make dt_binding_check in the codebase root directory before
> sending the V2 Patch, and there were no errors or warnings (the V1
> version run reported some errors). Is there some other way to test DTS
> ?

https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/


Best regards,
Krzysztof


2023-06-06 19:06:27

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

On Mon, Jun 05, 2023 at 12:34:54PM +0200, Krzysztof Kozlowski wrote:
> On 05/06/2023 08:05, Cong Yang wrote:
> > Add an ilitek touch screen chip ili9882t.
> >
> > Signed-off-by: Cong Yang <[email protected]>
> > ---
> > .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > index 05e6f2df604c..f0e7ffdce605 100644
> > --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > @@ -15,11 +15,14 @@ description:
> >
> > properties:
> > compatible:
> > - items:
> > - - const: elan,ekth6915
> > + enum:
> > + - elan,ekth6915
> > + - ilitek,ili9882t
> >
> > reg:
> > - const: 0x10
> > + enum:
> > + - 0x10
> > + - 0x41
> >
> > interrupts:
> > maxItems: 1
> > @@ -29,11 +32,13 @@ properties:
> >
> > vcc33-supply:
> > description: The 3.3V supply to the touchscreen.
> > + If using ili9882t then this supply will not be needed.
>
> What does it mean "will not be needed"? Describe the hardware, not your
> drivers.

I do not think it makes sense to merge Ilitek and Elan into a single
binding. The only thing that they have in common is that we are trying
to reuse drivers/hid/i2c-hid/i2c-hid-of-elan.c to handle reset timings
(which is also questionable IMO).

Maybe if we had a single unified binding for HID-over-I2C touchscreens
then combining would make more sense, at least to me...

Thanks.

--
Dmitry

2023-06-07 09:02:46

by cong yang

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip

Hi,Dmitry

On Wed, Jun 7, 2023 at 2:51 AM Dmitry Torokhov
<[email protected]> wrote:
>
> On Mon, Jun 05, 2023 at 12:34:54PM +0200, Krzysztof Kozlowski wrote:
> > On 05/06/2023 08:05, Cong Yang wrote:
> > > Add an ilitek touch screen chip ili9882t.
> > >
> > > Signed-off-by: Cong Yang <[email protected]>
> > > ---
> > > .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
> > > 1 file changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > > index 05e6f2df604c..f0e7ffdce605 100644
> > > --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > > +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > > @@ -15,11 +15,14 @@ description:
> > >
> > > properties:
> > > compatible:
> > > - items:
> > > - - const: elan,ekth6915
> > > + enum:
> > > + - elan,ekth6915
> > > + - ilitek,ili9882t
> > >
> > > reg:
> > > - const: 0x10
> > > + enum:
> > > + - 0x10
> > > + - 0x41
> > >
> > > interrupts:
> > > maxItems: 1
> > > @@ -29,11 +32,13 @@ properties:
> > >
> > > vcc33-supply:
> > > description: The 3.3V supply to the touchscreen.
> > > + If using ili9882t then this supply will not be needed.
> >
> > What does it mean "will not be needed"? Describe the hardware, not your
> > drivers.
>
> I do not think it makes sense to merge Ilitek and Elan into a single
> binding. The only thing that they have in common is that we are trying
> to reuse drivers/hid/i2c-hid/i2c-hid-of-elan.c to handle reset timings
> (which is also questionable IMO).
>
> Maybe if we had a single unified binding for HID-over-I2C touchscreens
> then combining would make more sense, at least to me...
>

Okay, thanks for the suggestion, I will add an ilitek binding.

> Thanks.
>
> --
> Dmitry