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

[cong yang]

Hi,Krzysztof

On Mon, Jun 5, 2023 at 6:34 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 05/06/2023 08:05, Cong Yang wrote:
> > Add an ilitek touch screen chip ili9882t.
> >
> > Signed-off-by: Cong Yang <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  .../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
>