| Message ID | 20230605060524.1178164-2-yangcong5@huaqin.corp-partner.google.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add ili9882t timing | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 <yangcong5@huaqin.corp-partner.google.com> > --- > .../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
On 05/06/2023 08:05, Cong Yang wrote: > Add an ilitek touch screen chip ili9882t. > > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com> > --- > .../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
Hi,Conor, On Mon, Jun 5, 2023 at 6:20 PM Conor Dooley <conor@kernel.org> 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 <yangcong5@huaqin.corp-partner.google.com> > > --- > > .../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
Hi,Krzysztof On Mon, Jun 5, 2023 at 6:34 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 05/06/2023 08:05, Cong Yang wrote: > > Add an ilitek touch screen chip ili9882t. > > > > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com> > > --- > > .../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 >
On 06/06/2023 04:18, cong yang wrote: > Hi,Krzysztof > > On Mon, Jun 5, 2023 at 6:34 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 05/06/2023 08:05, Cong Yang wrote: >>> Add an ilitek touch screen chip ili9882t. >>> >>> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com> >>> --- >>> .../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
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 <conor@kernel.org> 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 <yangcong5@huaqin.corp-partner.google.com> > > > --- > > > .../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.
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 <yangcong5@huaqin.corp-partner.google.com> > > --- > > .../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.
Hi,Dmitry On Wed, Jun 7, 2023 at 2:51 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> 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 <yangcong5@huaqin.corp-partner.google.com> > > > --- > > > .../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
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 <conor@kernel.org> 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 <yangcong5@huaqin.corp-partner.google.com> > > > --- > > > .../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
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. 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. 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:
Add an ilitek touch screen chip ili9882t. Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com> --- .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)