2023-06-05 10:38:00

by Conor Dooley

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

Hey Cong Yang,

On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> Add an ilitek touch screen chip ili9882t.

Could you add a comment here mentioning the relationship between these
chips?
On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:

> 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

Is 0x10 only valid for the elan,ekth6915 & 0x41 for the ilitek one?
If so, please add some enforcement of the values based on the
compatible.

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

There's no need for these sort of comments, you can rely on the required
sections to describe these relationships.

Cheers,
Conor.

>
> required:
> - compatible
> @@ -41,6 +46,18 @@ required:
> - interrupts
> - vcc33-supply
>
> +if:
> + properties:
> + compatible:
> + contains:
> + const: ilitek,ili9882t
> +then:
> + required:
> + - compatible
> + - reg
> + - interrupts
> + - vccio-supply
> +
> additionalProperties: false
>
> examples:
> --
> 2.25.1


Attachments:
(No filename) (2.16 kB)
signature.asc (235.00 B)
Download all attachments

2023-06-06 03:22:48

by cong yang

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

Hi,Conor,

On Mon, Jun 5, 2023 at 6:20 PM Conor Dooley <[email protected]> wrote:
>
> Hey Cong Yang,
>
> On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> > Add an ilitek touch screen chip ili9882t.
>
> Could you add a comment here mentioning the relationship between these
> chips?

Okay, I will add in V3 version.

> On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
>
> > 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
>
> Is 0x10 only valid for the elan,ekth6915 & 0x41 for the ilitek one?
> If so, please add some enforcement of the values based on the
> compatible.

I don't think 0x10 is the only address for ekth6915,(nor is 0x41 the
only address for ili9882t). It depends on the hardware design.

>
> >
> > 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.
> >
> > 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.
>
> There's no need for these sort of comments, you can rely on the required
> sections to describe these relationships.

Got it ,thanks.


>
> Cheers,
> Conor.
>
> >
> > required:
> > - compatible
> > @@ -41,6 +46,18 @@ required:
> > - interrupts
> > - vcc33-supply
> >
> > +if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: ilitek,ili9882t
> > +then:
> > + required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - vccio-supply
> > +
> > additionalProperties: false
> >
> > examples:
> > --
> > 2.25.1

2023-06-06 19:06:01

by Dmitry Torokhov

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

On Tue, Jun 06, 2023 at 10:06:05AM +0800, cong yang wrote:
> Hi,Conor,
>
> On Mon, Jun 5, 2023 at 6:20 PM Conor Dooley <[email protected]> wrote:
> >
> > Hey Cong Yang,
> >
> > On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> > > Add an ilitek touch screen chip ili9882t.
> >
> > Could you add a comment here mentioning the relationship between these
> > chips?
>
> Okay, I will add in V3 version.
>
> > On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> >
> > > 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
> >
> > Is 0x10 only valid for the elan,ekth6915 & 0x41 for the ilitek one?
> > If so, please add some enforcement of the values based on the
> > compatible.
>
> I don't think 0x10 is the only address for ekth6915,(nor is 0x41 the
> only address for ili9882t). It depends on the hardware design.

Only a handful of controllers allow switching between addresses, and
if they do they typically have a "main" and an "alternate" one, and not
something completely customizable. I do not believe Elan offers ways to
program different address, do they?

Thanks.

--
Dmitry

2023-06-09 22:55:14

by Rob Herring

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

On Tue, Jun 06, 2023 at 10:06:05AM +0800, cong yang wrote:
> Hi,Conor,
>
> On Mon, Jun 5, 2023 at 6:20 PM Conor Dooley <[email protected]> wrote:
> >
> > Hey Cong Yang,
> >
> > On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> > > Add an ilitek touch screen chip ili9882t.
> >
> > Could you add a comment here mentioning the relationship between these
> > chips?
>
> Okay, I will add in V3 version.
>
> > On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> >
> > > 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
> >
> > Is 0x10 only valid for the elan,ekth6915 & 0x41 for the ilitek one?
> > If so, please add some enforcement of the values based on the
> > compatible.
>
> I don't think 0x10 is the only address for ekth6915,(nor is 0x41 the
> only address for ili9882t). It depends on the hardware design.

I'd just drop the values as we don't typically enforce 'reg' values.

Rob