LKML Archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Add ili9882t timing
@ 2023-06-05  6:05 Cong Yang
  2023-06-05  6:05 ` [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip Cong Yang
  2023-06-05  6:05 ` [PATCH v2 2/2] HID: i2c-hid: elan: Add ili9882t timing Cong Yang
  0 siblings, 2 replies; 12+ messages in thread
From: Cong Yang @ 2023-06-05  6:05 UTC (permalink / raw)
  To: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
	benjamin.tissoires, dianders, hsinyi
  Cc: linux-input, devicetree, linux-kernel, Cong Yang

Add ili9882t dt-bindings and timing

Changes in v2:
- PATCH 1/2: fix ran make dt_binding_check warnings/errors.
- PATCH 1/2: remove oneOf,just enum.
- Link to v1: https://lore.kernel.org/all/20230602140948.2138668-1-yangcong5@huaqin.corp-partner.google.com/

Cong Yang (2):
  dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  HID: i2c-hid: elan: Add ili9882t timing

 .../bindings/input/elan,ekth6915.yaml         | 23 ++++++++++++++++---
 drivers/hid/i2c-hid/i2c-hid-of-elan.c         | 20 ++++++++++++----
 2 files changed, 36 insertions(+), 7 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  2023-06-05  6:05 [PATCH v2 0/2] Add ili9882t timing Cong Yang
@ 2023-06-05  6:05 ` Cong Yang
  2023-06-05 10:20   ` Conor Dooley
  2023-06-05 10:34   ` Krzysztof Kozlowski
  2023-06-05  6:05 ` [PATCH v2 2/2] HID: i2c-hid: elan: Add ili9882t timing Cong Yang
  1 sibling, 2 replies; 12+ messages in thread
From: Cong Yang @ 2023-06-05  6:05 UTC (permalink / raw)
  To: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
	benjamin.tissoires, dianders, hsinyi
  Cc: linux-input, devicetree, linux-kernel, Cong Yang

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/2] HID: i2c-hid: elan: Add ili9882t timing
  2023-06-05  6:05 [PATCH v2 0/2] Add ili9882t timing Cong Yang
  2023-06-05  6:05 ` [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip Cong Yang
@ 2023-06-05  6:05 ` Cong Yang
  1 sibling, 0 replies; 12+ messages in thread
From: Cong Yang @ 2023-06-05  6:05 UTC (permalink / raw)
  To: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
	benjamin.tissoires, dianders, hsinyi
  Cc: linux-input, devicetree, linux-kernel, Cong Yang

The ili9882t is a TDDI IC (Touch with Display Driver). The datasheet
specifies there should be 60ms between touch SDA sleep and panel RESX.
Doug's series[1] allows panels and touchscreens to power on/off together,
so we can add the 65 ms delay in i2c_hid_core_suspend before panel_unprepare.

[1]: https: //lore.kernel.org/all/20230523193017.4109557-1-dianders@chromium.org/

Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
---
 drivers/hid/i2c-hid/i2c-hid-of-elan.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
index 76ddc8be1cbb..411d7ea2725d 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
@@ -18,7 +18,8 @@
 #include "i2c-hid.h"
 
 struct elan_i2c_hid_chip_data {
-	unsigned int post_gpio_reset_delay_ms;
+	unsigned int post_gpio_reset_on_delay_ms;
+	unsigned int post_gpio_reset_off_delay_ms;
 	unsigned int post_power_delay_ms;
 	u16 hid_descriptor_address;
 };
@@ -52,8 +53,8 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
 		msleep(ihid_elan->chip_data->post_power_delay_ms);
 
 	gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0);
-	if (ihid_elan->chip_data->post_gpio_reset_delay_ms)
-		msleep(ihid_elan->chip_data->post_gpio_reset_delay_ms);
+	if (ihid_elan->chip_data->post_gpio_reset_on_delay_ms)
+		msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
 
 	return 0;
 }
@@ -64,6 +65,9 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
 		container_of(ops, struct i2c_hid_of_elan, ops);
 
 	gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
+	if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
+		msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
+
 	regulator_disable(ihid_elan->vccio);
 	regulator_disable(ihid_elan->vcc33);
 }
@@ -101,12 +105,20 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
 
 static const struct elan_i2c_hid_chip_data elan_ekth6915_chip_data = {
 	.post_power_delay_ms = 1,
-	.post_gpio_reset_delay_ms = 300,
+	.post_gpio_reset_on_delay_ms = 300,
+	.hid_descriptor_address = 0x0001,
+};
+
+static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = {
+	.post_power_delay_ms = 1,
+	.post_gpio_reset_on_delay_ms = 200,
+	.post_gpio_reset_off_delay_ms = 65,
 	.hid_descriptor_address = 0x0001,
 };
 
 static const struct of_device_id elan_i2c_hid_of_match[] = {
 	{ .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data },
+	{ .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_chip_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, elan_i2c_hid_of_match);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  2023-06-05  6:05 ` [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip Cong Yang
@ 2023-06-05 10:20   ` Conor Dooley
  2023-06-06  2:06     ` cong yang
  2023-06-05 10:34   ` Krzysztof Kozlowski
  1 sibling, 1 reply; 12+ messages in thread
From: Conor Dooley @ 2023-06-05 10:20 UTC (permalink / raw)
  To: Cong Yang
  Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
	benjamin.tissoires, dianders, hsinyi, linux-input, devicetree,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  2023-06-05  6:05 ` [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip Cong Yang
  2023-06-05 10:20   ` Conor Dooley
@ 2023-06-05 10:34   ` Krzysztof Kozlowski
  2023-06-06  2:18     ` cong yang
  2023-06-06 18:51     ` Dmitry Torokhov
  1 sibling, 2 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-05 10:34 UTC (permalink / raw)
  To: Cong Yang, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	dmitry.torokhov, jikos, benjamin.tissoires, dianders, hsinyi
  Cc: linux-input, devicetree, linux-kernel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  2023-06-05 10:20   ` Conor Dooley
@ 2023-06-06  2:06     ` cong yang
  2023-06-06 18:47       ` Dmitry Torokhov
  2023-06-09 22:03       ` Rob Herring
  0 siblings, 2 replies; 12+ messages in thread
From: cong yang @ 2023-06-06  2:06 UTC (permalink / raw)
  To: Conor Dooley
  Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
	benjamin.tissoires, dianders, hsinyi, linux-input, devicetree,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  2023-06-05 10:34   ` Krzysztof Kozlowski
@ 2023-06-06  2:18     ` cong yang
  2023-06-06  6:21       ` Krzysztof Kozlowski
  2023-06-06 18:51     ` Dmitry Torokhov
  1 sibling, 1 reply; 12+ messages in thread
From: cong yang @ 2023-06-06  2:18 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
	benjamin.tissoires, dianders, hsinyi, linux-input, devicetree,
	linux-kernel

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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  2023-06-06  2:18     ` cong yang
@ 2023-06-06  6:21       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-06  6:21 UTC (permalink / raw)
  To: cong yang
  Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
	benjamin.tissoires, dianders, hsinyi, linux-input, devicetree,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  2023-06-06  2:06     ` cong yang
@ 2023-06-06 18:47       ` Dmitry Torokhov
  2023-06-09 22:03       ` Rob Herring
  1 sibling, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2023-06-06 18:47 UTC (permalink / raw)
  To: cong yang
  Cc: Conor Dooley, robh+dt, krzysztof.kozlowski+dt, conor+dt, jikos,
	benjamin.tissoires, dianders, hsinyi, linux-input, devicetree,
	linux-kernel

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  2023-06-05 10:34   ` Krzysztof Kozlowski
  2023-06-06  2:18     ` cong yang
@ 2023-06-06 18:51     ` Dmitry Torokhov
  2023-06-07  8:55       ` cong yang
  1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2023-06-06 18:51 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Cong Yang, robh+dt, krzysztof.kozlowski+dt, conor+dt, jikos,
	benjamin.tissoires, dianders, hsinyi, linux-input, devicetree,
	linux-kernel

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  2023-06-06 18:51     ` Dmitry Torokhov
@ 2023-06-07  8:55       ` cong yang
  0 siblings, 0 replies; 12+ messages in thread
From: cong yang @ 2023-06-07  8:55 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Krzysztof Kozlowski, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	jikos, benjamin.tissoires, dianders, hsinyi, linux-input,
	devicetree, linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
  2023-06-06  2:06     ` cong yang
  2023-06-06 18:47       ` Dmitry Torokhov
@ 2023-06-09 22:03       ` Rob Herring
  1 sibling, 0 replies; 12+ messages in thread
From: Rob Herring @ 2023-06-09 22:03 UTC (permalink / raw)
  To: cong yang
  Cc: Conor Dooley, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov,
	jikos, benjamin.tissoires, dianders, hsinyi, linux-input,
	devicetree, linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-06-09 22:03 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-05  6:05 [PATCH v2 0/2] Add ili9882t timing Cong Yang
2023-06-05  6:05 ` [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip Cong Yang
2023-06-05 10:20   ` Conor Dooley
2023-06-06  2:06     ` cong yang
2023-06-06 18:47       ` Dmitry Torokhov
2023-06-09 22:03       ` Rob Herring
2023-06-05 10:34   ` Krzysztof Kozlowski
2023-06-06  2:18     ` cong yang
2023-06-06  6:21       ` Krzysztof Kozlowski
2023-06-06 18:51     ` Dmitry Torokhov
2023-06-07  8:55       ` cong yang
2023-06-05  6:05 ` [PATCH v2 2/2] HID: i2c-hid: elan: Add ili9882t timing Cong Yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).