* [PATCH v4 5/8] dt-bindings: input: Convert da906{1,2,3} onkey to json-schema
From: Biju Das @ 2023-12-06 15:57 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Biju Das, Support Opensource, linux-input, devicetree,
Geert Uytterhoeven, Prabhakar Mahadev Lad, Biju Das,
linux-renesas-soc
In-Reply-To: <20231206155740.5278-1-biju.das.jz@bp.renesas.com>
Convert the da906{1,2,3} onkey device tree binding documentation to
json-schema.
Update MAINTAINERS entries, description and onkey property by
referring to dlg,da9062-onkey binding file.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
* Squashed with patch#6 and patch#9 from v2.
* Replaced enum->const for dlg,da9061-onkey and its fallback.
* Dropped example
v2->v3:
* Updated MAINTAINERS entries.
v2:
* New patch
---
.../bindings/input/da9062-onkey.txt | 47 -------------------
.../bindings/input/dlg,da9062-onkey.yaml | 40 ++++++++++++++++
.../devicetree/bindings/mfd/da9062.txt | 2 +-
.../devicetree/bindings/mfd/dlg,da9063.yaml | 15 +-----
MAINTAINERS | 2 +-
5 files changed, 43 insertions(+), 63 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/input/da9062-onkey.txt
create mode 100644 Documentation/devicetree/bindings/input/dlg,da9062-onkey.yaml
diff --git a/Documentation/devicetree/bindings/input/da9062-onkey.txt b/Documentation/devicetree/bindings/input/da9062-onkey.txt
deleted file mode 100644
index e5eef59a93dc..000000000000
--- a/Documentation/devicetree/bindings/input/da9062-onkey.txt
+++ /dev/null
@@ -1,47 +0,0 @@
-* Dialog DA9061/62/63 OnKey Module
-
-This module is part of the DA9061/DA9062/DA9063. For more details about entire
-DA9062 and DA9061 chips see Documentation/devicetree/bindings/mfd/da9062.txt
-For DA9063 see Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
-
-This module provides the KEY_POWER event.
-
-Required properties:
-
-- compatible: should be one of the following valid compatible string lines:
- "dlg,da9061-onkey", "dlg,da9062-onkey"
- "dlg,da9062-onkey"
- "dlg,da9063-onkey"
-
-Optional properties:
-
-- dlg,disable-key-power : Disable power-down using a long key-press. If this
- entry exists the OnKey driver will remove support for the KEY_POWER key
- press when triggered using a long press of the OnKey.
-
-Example: DA9063
-
- pmic0: da9063@58 {
- onkey {
- compatible = "dlg,da9063-onkey";
- dlg,disable-key-power;
- };
- };
-
-Example: DA9062
-
- pmic0: da9062@58 {
- onkey {
- compatible = "dlg,da9062-onkey";
- dlg,disable-key-power;
- };
- };
-
-Example: DA9061 using a fall-back compatible for the DA9062 onkey driver
-
- pmic0: da9061@58 {
- onkey {
- compatible = "dlg,da9061-onkey", "dlg,da9062-onkey";
- dlg,disable-key-power;
- };
- };
diff --git a/Documentation/devicetree/bindings/input/dlg,da9062-onkey.yaml b/Documentation/devicetree/bindings/input/dlg,da9062-onkey.yaml
new file mode 100644
index 000000000000..4cff91f4bd34
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/dlg,da9062-onkey.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/dlg,da9062-onkey.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Dialog DA9061/62/63 OnKey Module
+
+maintainers:
+ - Biju Das <biju.das.jz@bp.renesas.com>
+
+description: |
+ This module is part of the DA9061/DA9062/DA9063. For more details about entire
+ DA9062 and DA9061 chips see Documentation/devicetree/bindings/mfd/da9062.txt
+ For DA9063 see Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
+
+ This module provides the KEY_POWER event.
+
+properties:
+ compatible:
+ oneOf:
+ - items:
+ - enum:
+ - dlg,da9062-onkey
+ - dlg,da9063-onkey
+ - items:
+ - const: dlg,da9061-onkey
+ - const: dlg,da9062-onkey
+
+ dlg,disable-key-power:
+ type: boolean
+ description:
+ Disable power-down using a long key-press. If this entry exists
+ the OnKey driver will remove support for the KEY_POWER key press
+ when triggered using a long press of the OnKey.
+
+required:
+ - compatible
+
+additionalProperties: false
diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt b/Documentation/devicetree/bindings/mfd/da9062.txt
index 18463b7fbb42..154c31fa4443 100644
--- a/Documentation/devicetree/bindings/mfd/da9062.txt
+++ b/Documentation/devicetree/bindings/mfd/da9062.txt
@@ -84,7 +84,7 @@ Sub-nodes:
with the DA9062. There are currently no entries in this binding, however
compatible = "dlg,da9062-rtc" should be added if a node is created.
-- onkey : See ../input/da9062-onkey.txt
+- onkey : See ../input/dlg,da9062-onkey.yaml
- watchdog: See ../watchdog/dlg,da9062-watchdog.yaml
diff --git a/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml b/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
index ce81e0b029cc..1e5a847a6be2 100644
--- a/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
+++ b/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
@@ -44,20 +44,7 @@ properties:
const: dlg,da9063-rtc
onkey:
- type: object
- $ref: /schemas/input/input.yaml#
- unevaluatedProperties: false
- properties:
- compatible:
- const: dlg,da9063-onkey
-
- dlg,disable-key-power:
- type: boolean
- description: |
- Disable power-down using a long key-press.
- If this entry does not exist then by default the key-press triggered
- power down is enabled and the OnKey will support both KEY_POWER and
- KEY_SLEEP.
+ $ref: /schemas/input/dlg,da9062-onkey.yaml
regulators:
type: object
diff --git a/MAINTAINERS b/MAINTAINERS
index fa3965f1bf0e..ea171a18217c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6110,8 +6110,8 @@ DIALOG SEMICONDUCTOR DRIVERS
M: Support Opensource <support.opensource@diasemi.com>
S: Supported
W: http://www.dialog-semiconductor.com/products
-F: Documentation/devicetree/bindings/input/da90??-onkey.txt
F: Documentation/devicetree/bindings/input/dlg,da72??.txt
+F: Documentation/devicetree/bindings/input/dlg,da9062-onkey.yaml
F: Documentation/devicetree/bindings/mfd/da90*.txt
F: Documentation/devicetree/bindings/mfd/dlg,da90*.yaml
F: Documentation/devicetree/bindings/regulator/da92*.txt
--
2.39.2
^ permalink raw reply related
* [PATCH v4 0/8] Convert DA906{1,2} bindings to json-schema
From: Biju Das @ 2023-12-06 15:57 UTC (permalink / raw)
To: Lee Jones, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: Biju Das, Support Opensource, Rafael J. Wysocki, Daniel Lezcano,
Zhang Rui, Lukasz Luba, Steve Twiss, linux-input, devicetree,
linux-pm, Geert Uytterhoeven, Prabhakar Mahadev Lad, Biju Das,
linux-renesas-soc
Convert the below bindings to json-schema
1) DA906{1,2} mfd bindings
2) DA906{1,2,3} onkey bindings
3) DA906{1,2,3} thermal bindings
Also add fallback for DA9061 watchdog device and document
DA9063 watchdog device.
Note:
This patch series is same as v3.1 as it allows the tools (PW, b4)
to compare against previous versions.
The review comments/tags received for v4 + (a.k.a v3.1) will be
addressed in the next version(v5).
Link to v3.1: https://lore.kernel.org/all/20231204172510.35041-1-biju.das.jz@bp.renesas.com/
v3->v4:
* Patch#1 is merge of patch#1 from v2 + patch#8 from v2.
* Dropped comment for d9061 watchdog fallback
* Replaced enum->const for dlg,da9061-watchdog and its fallback.
* Restored patch#4 in series 1 and dropped the thermal example
* Added Ack from Conor Dooley for da9063 watchdog binding support.
* Updated title DA9062/61->DA906{1,2,3} as it supports DA9063.
* Retained Rb tag since the changes are trivial.
* Added Ack from Conor for updating watchdog property
* Dropped link to product information.
* Patch#5(onkey) is squashed with patch#6 and patch#9 from v2.
* Replaced enum->const for dlg,da9061-onkey and its fallback.
* Dropped example
* Restored the thermal binding patch from v2.
* Dropped example
* Replaced enum->const for compatible property.
* Added Rb tag from Rob and retained Rb tag as changes are trivial.
* Added Ack from Conor Dooley for patch#7.
* Split the thermal binding patch separate
* Updated the description
v2->v3:
* Updated Maintainer entries for watchdog,onkey and thermal bindings
* Fixed bot errors related to MAINTAINERS entry, invalid doc
references and thermal examples by merging patch#4.
v1->v2:
Link: https://lore.kernel.org/all/20231201110840.37408-5-biju.das.jz@bp.renesas.com/
* DA9062 and DA9061 merged with DA9063
* Sorted the child devices
* mfd,onkey and thermal are pointing to child bindings
Biju Das (8):
dt-bindings: mfd: da9062: Update watchdog description
dt-bindings: watchdog: dlg,da9062-watchdog: Add fallback for DA9061
watchdog
dt-bindings: watchdog: dlg,da9062-watchdog: Document DA9063 watchdog
dt-bindings: mfd: dlg,da9063: Update watchdog property
dt-bindings: input: Convert da906{1,2,3} onkey to json-schema
dt-bindings: thermal: Convert da906{1,2} thermal to json-schema
dt-bindings: mfd: dlg,da9063: Sort child devices
dt-bindings: mfd: dlg,da9063: Convert da9062 to json-schema
.../bindings/input/da9062-onkey.txt | 47 ----
.../bindings/input/dlg,da9062-onkey.yaml | 39 ++++
.../devicetree/bindings/mfd/da9062.txt | 124 ----------
.../devicetree/bindings/mfd/dlg,da9063.yaml | 221 +++++++++++++++---
.../bindings/thermal/da9062-thermal.txt | 36 ---
.../bindings/thermal/dlg,da9062-thermal.yaml | 35 +++
.../watchdog/dlg,da9062-watchdog.yaml | 13 +-
MAINTAINERS | 6 +-
8 files changed, 272 insertions(+), 249 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/input/da9062-onkey.txt
create mode 100644 Documentation/devicetree/bindings/input/dlg,da9062-onkey.yaml
delete mode 100644 Documentation/devicetree/bindings/mfd/da9062.txt
delete mode 100644 Documentation/devicetree/bindings/thermal/da9062-thermal.txt
create mode 100644 Documentation/devicetree/bindings/thermal/dlg,da9062-thermal.yaml
--
2.39.2
^ permalink raw reply
* RE: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to json-schema
From: Biju Das @ 2023-12-06 15:14 UTC (permalink / raw)
To: Rob Herring
Cc: Lee Jones, Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley,
Support Opensource, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Steve Twiss, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-pm@vger.kernel.org,
Geert Uytterhoeven, Prabhakar Mahadev Lad, biju.das.au,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <20231206135849.GA2051159-robh@kernel.org>
Hi Rob Herring,
thanks for the feedback.
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, December 6, 2023 1:59 PM
> Subject: Re: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to json-schema
>
> On Mon, Dec 04, 2023 at 05:25:02PM +0000, Biju Das wrote:
> > Convert the below bindings to json-schema
> > 1) DA906{1,2} mfd bindings
> > 2) DA906{1,2,3} onkey bindings
> > 3) DA906{1,2,3} thermal bindings
> >
> > Also add fallback for DA9061 watchdog device and document
> > DA9063 watchdog device.
> >
> > v3->v3.1:
>
> No, it's v4. The various tools (PW, b4) don't understand point versions.
It is a mistake from my side.
I will send same patch series as v4?
So that one can, compare the current patch series(v4) against v2 and v3.
Cheers,
Biju
^ permalink raw reply
* Re: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to json-schema
From: Rob Herring @ 2023-12-06 13:58 UTC (permalink / raw)
To: Biju Das
Cc: Lee Jones, Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley,
Support Opensource, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Steve Twiss, linux-input, devicetree, linux-pm,
Geert Uytterhoeven, Prabhakar Mahadev Lad, Biju Das,
linux-renesas-soc
In-Reply-To: <20231204172510.35041-1-biju.das.jz@bp.renesas.com>
On Mon, Dec 04, 2023 at 05:25:02PM +0000, Biju Das wrote:
> Convert the below bindings to json-schema
> 1) DA906{1,2} mfd bindings
> 2) DA906{1,2,3} onkey bindings
> 3) DA906{1,2,3} thermal bindings
>
> Also add fallback for DA9061 watchdog device and document
> DA9063 watchdog device.
>
> v3->v3.1:
No, it's v4. The various tools (PW, b4) don't understand point versions.
Rob
^ permalink raw reply
* RE: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to json-schema
From: Biju Das @ 2023-12-06 11:26 UTC (permalink / raw)
To: Krzysztof Kozlowski, Lee Jones, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Support Opensource, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Steve Twiss, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-pm@vger.kernel.org,
Geert Uytterhoeven, Prabhakar Mahadev Lad, biju.das.au,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <33b097c7-0c7f-457c-b597-19c504df5a36@linaro.org>
Hi Krzysztof Kozlowski,
Thanks for the feedback.
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, December 6, 2023 11:18 AM
> Subject: Re: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to json-schema
>
> On 06/12/2023 12:16, Biju Das wrote:
> > Hi Krzysztof Kozlowski,
> >
> > Thanks for the feedback.
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Wednesday, December 6, 2023 11:06 AM
> >> Subject: Re: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to
> >> json-schema
> >>
> >> On 04/12/2023 18:25, Biju Das wrote:
> >>> Convert the below bindings to json-schema
> >>> 1) DA906{1,2} mfd bindings
> >>> 2) DA906{1,2,3} onkey bindings
> >>> 3) DA906{1,2,3} thermal bindings
> >>>
> >>> Also add fallback for DA9061 watchdog device and document
> >>> DA9063 watchdog device.
> >>>
> >>> v3->v3.1:
> >>> * Patch#1 is merge of patch#1 from v2 + patch#8 from v2.
> >>> * Dropped comment for d9061 watchdog fallback
> >>> * Replaced enum->const for dlg,da9061-watchdog and its fallback.
> >>> * Restored patch#4 in series 1 and dropped the thermal example
> >>> * Added Ack from Conor Dooley for da9063 watchdog binding support.
> >>> * Updated title DA9062/61->DA906{1,2,3} as it supports DA9063.
> >>> * Retained Rb tag since the changes are trivial.
> >>> * Added Ack from Conor for updating watchdog property
> >>> * Dropped link to product information.
> >>> * Patch#5(onkey) is squashed with patch#6 and patch#9 from v2.
> >>> * Replaced enum->const for dlg,da9061-onkey and its fallback.
> >>> * Dropped example
> >>> * Restored the thermal binding patch from v2.
> >>> * Dropped example
> >>> * Replaced enum->const for compatible property.
> >>> * Added Rb tag from Rob and retained Rb tag as changes are trivial.
> >>> * Added Ack from Conor Dooley for patch#7.
> >>> * Split the thermal binding patch separate
> >>> * Updated the description
> >>
> >>
> >> Hundreds of changes and just "3 -> 3.1"? This does not make sense.
> >
> > Bot reported some issues with v2. So immediately I send v3 which
> > clashed with review comments from Conor and Rob.
> >
> > No one has reviewed V3.
> >
> > V3.1 = basically Review comments from v2 + Fix for Bot errors.
>
> v4, don't introduce some minor numbering to create impression of no
> changes, especially if you have multiple changes.
OK, When I send next version, I will use V5 and revision history(log change)
I will use v4 instead of v3.1 as it has multiple changes.
Is it ok?
Cheers,
Biju
^ permalink raw reply
* Re: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to json-schema
From: Krzysztof Kozlowski @ 2023-12-06 11:18 UTC (permalink / raw)
To: Biju Das, Lee Jones, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Support Opensource, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Steve Twiss, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-pm@vger.kernel.org,
Geert Uytterhoeven, Prabhakar Mahadev Lad, biju.das.au,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <TYVPR01MB112796A859B42CC4AC6F6EC838684A@TYVPR01MB11279.jpnprd01.prod.outlook.com>
On 06/12/2023 12:16, Biju Das wrote:
> Hi Krzysztof Kozlowski,
>
> Thanks for the feedback.
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Wednesday, December 6, 2023 11:06 AM
>> Subject: Re: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to json-schema
>>
>> On 04/12/2023 18:25, Biju Das wrote:
>>> Convert the below bindings to json-schema
>>> 1) DA906{1,2} mfd bindings
>>> 2) DA906{1,2,3} onkey bindings
>>> 3) DA906{1,2,3} thermal bindings
>>>
>>> Also add fallback for DA9061 watchdog device and document
>>> DA9063 watchdog device.
>>>
>>> v3->v3.1:
>>> * Patch#1 is merge of patch#1 from v2 + patch#8 from v2.
>>> * Dropped comment for d9061 watchdog fallback
>>> * Replaced enum->const for dlg,da9061-watchdog and its fallback.
>>> * Restored patch#4 in series 1 and dropped the thermal example
>>> * Added Ack from Conor Dooley for da9063 watchdog binding support.
>>> * Updated title DA9062/61->DA906{1,2,3} as it supports DA9063.
>>> * Retained Rb tag since the changes are trivial.
>>> * Added Ack from Conor for updating watchdog property
>>> * Dropped link to product information.
>>> * Patch#5(onkey) is squashed with patch#6 and patch#9 from v2.
>>> * Replaced enum->const for dlg,da9061-onkey and its fallback.
>>> * Dropped example
>>> * Restored the thermal binding patch from v2.
>>> * Dropped example
>>> * Replaced enum->const for compatible property.
>>> * Added Rb tag from Rob and retained Rb tag as changes are trivial.
>>> * Added Ack from Conor Dooley for patch#7.
>>> * Split the thermal binding patch separate
>>> * Updated the description
>>
>>
>> Hundreds of changes and just "3 -> 3.1"? This does not make sense.
>
> Bot reported some issues with v2. So immediately I send v3 which clashed
> with review comments from Conor and Rob.
>
> No one has reviewed V3.
>
> V3.1 = basically Review comments from v2 + Fix for Bot errors.
v4, don't introduce some minor numbering to create impression of no
changes, especially if you have multiple changes.
>
>
>>
>> Also, use normal versioning:
>>
>> b4 diff '<20231204172510.35041-9-biju.das.jz@bp.renesas.com>'
>> Grabbing thread from
>> lore.kernel.org/all/20231204172510.35041-9-
>> biju.das.jz@bp.renesas.com/t.mbox.gz
>> ---
>> Analyzing 21 messages in the thread
>> ERROR: Could not auto-find previous revision
>> Run "b4 am -T" manually, then "b4 diff -m mbx1 mbx2"
>
>
> Can you please clarify more? I may be missing something here?
>
> I just rebase to linux-next and send patches using the command
>
> git send-email --dry-run --annotate *.patch
>
> All patches Updated with TO and CC recipients.
>
> Am I missing anything here w.r.to versioning?
v3 -> v4.
Best regards,
Krzysztof
^ permalink raw reply
* RE: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to json-schema
From: Biju Das @ 2023-12-06 11:16 UTC (permalink / raw)
To: Krzysztof Kozlowski, Lee Jones, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Support Opensource, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Steve Twiss, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-pm@vger.kernel.org,
Geert Uytterhoeven, Prabhakar Mahadev Lad, biju.das.au,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <332dfce5-f2a8-421a-878e-85f95aa64d10@linaro.org>
Hi Krzysztof Kozlowski,
Thanks for the feedback.
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, December 6, 2023 11:06 AM
> Subject: Re: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to json-schema
>
> On 04/12/2023 18:25, Biju Das wrote:
> > Convert the below bindings to json-schema
> > 1) DA906{1,2} mfd bindings
> > 2) DA906{1,2,3} onkey bindings
> > 3) DA906{1,2,3} thermal bindings
> >
> > Also add fallback for DA9061 watchdog device and document
> > DA9063 watchdog device.
> >
> > v3->v3.1:
> > * Patch#1 is merge of patch#1 from v2 + patch#8 from v2.
> > * Dropped comment for d9061 watchdog fallback
> > * Replaced enum->const for dlg,da9061-watchdog and its fallback.
> > * Restored patch#4 in series 1 and dropped the thermal example
> > * Added Ack from Conor Dooley for da9063 watchdog binding support.
> > * Updated title DA9062/61->DA906{1,2,3} as it supports DA9063.
> > * Retained Rb tag since the changes are trivial.
> > * Added Ack from Conor for updating watchdog property
> > * Dropped link to product information.
> > * Patch#5(onkey) is squashed with patch#6 and patch#9 from v2.
> > * Replaced enum->const for dlg,da9061-onkey and its fallback.
> > * Dropped example
> > * Restored the thermal binding patch from v2.
> > * Dropped example
> > * Replaced enum->const for compatible property.
> > * Added Rb tag from Rob and retained Rb tag as changes are trivial.
> > * Added Ack from Conor Dooley for patch#7.
> > * Split the thermal binding patch separate
> > * Updated the description
>
>
> Hundreds of changes and just "3 -> 3.1"? This does not make sense.
Bot reported some issues with v2. So immediately I send v3 which clashed
with review comments from Conor and Rob.
No one has reviewed V3.
V3.1 = basically Review comments from v2 + Fix for Bot errors.
>
> Also, use normal versioning:
>
> b4 diff '<20231204172510.35041-9-biju.das.jz@bp.renesas.com>'
> Grabbing thread from
> lore.kernel.org/all/20231204172510.35041-9-
> biju.das.jz@bp.renesas.com/t.mbox.gz
> ---
> Analyzing 21 messages in the thread
> ERROR: Could not auto-find previous revision
> Run "b4 am -T" manually, then "b4 diff -m mbx1 mbx2"
Can you please clarify more? I may be missing something here?
I just rebase to linux-next and send patches using the command
git send-email --dry-run --annotate *.patch
All patches Updated with TO and CC recipients.
Am I missing anything here w.r.to versioning?
Cheers,
Biju
^ permalink raw reply
* Re: [PATCH v3.1 0/8] Convert DA906{1,2} bindings to json-schema
From: Krzysztof Kozlowski @ 2023-12-06 11:06 UTC (permalink / raw)
To: Biju Das, Lee Jones, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Support Opensource, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Steve Twiss, linux-input, devicetree, linux-pm,
Geert Uytterhoeven, Prabhakar Mahadev Lad, Biju Das,
linux-renesas-soc
In-Reply-To: <20231204172510.35041-1-biju.das.jz@bp.renesas.com>
On 04/12/2023 18:25, Biju Das wrote:
> Convert the below bindings to json-schema
> 1) DA906{1,2} mfd bindings
> 2) DA906{1,2,3} onkey bindings
> 3) DA906{1,2,3} thermal bindings
>
> Also add fallback for DA9061 watchdog device and document
> DA9063 watchdog device.
>
> v3->v3.1:
> * Patch#1 is merge of patch#1 from v2 + patch#8 from v2.
> * Dropped comment for d9061 watchdog fallback
> * Replaced enum->const for dlg,da9061-watchdog and its fallback.
> * Restored patch#4 in series 1 and dropped the thermal example
> * Added Ack from Conor Dooley for da9063 watchdog binding support.
> * Updated title DA9062/61->DA906{1,2,3} as it supports DA9063.
> * Retained Rb tag since the changes are trivial.
> * Added Ack from Conor for updating watchdog property
> * Dropped link to product information.
> * Patch#5(onkey) is squashed with patch#6 and patch#9 from v2.
> * Replaced enum->const for dlg,da9061-onkey and its fallback.
> * Dropped example
> * Restored the thermal binding patch from v2.
> * Dropped example
> * Replaced enum->const for compatible property.
> * Added Rb tag from Rob and retained Rb tag as changes are trivial.
> * Added Ack from Conor Dooley for patch#7.
> * Split the thermal binding patch separate
> * Updated the description
Hundreds of changes and just "3 -> 3.1"? This does not make sense.
Also, use normal versioning:
b4 diff '<20231204172510.35041-9-biju.das.jz@bp.renesas.com>'
Grabbing thread from
lore.kernel.org/all/20231204172510.35041-9-biju.das.jz@bp.renesas.com/t.mbox.gz
---
Analyzing 21 messages in the thread
ERROR: Could not auto-find previous revision
Run "b4 am -T" manually, then "b4 diff -m mbx1 mbx2"
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: Add Himax HX83102J touchscreen driver
From: Krzysztof Kozlowski @ 2023-12-06 11:01 UTC (permalink / raw)
To: Allen_Lin, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
linux-kernel
In-Reply-To: <TY0PR06MB56119F0D60142F4C1435767C9E84A@TY0PR06MB5611.apcprd06.prod.outlook.com>
On 06/12/2023 11:35, Allen_Lin wrote:
> Add a new driver for Himax touchscreen series touchscreen controllers.
> This driver supports Himax IC using the SPI interface to
> acquire HID packets.
>
> Signed-off-by: Allen_Lin <allencl_lin@hotmail.com>
> ---
> v1 -> v2: Fix kernel test robot build warnings
> Signed-off-by: Allen_Lin <allencl_lin@hotmail.com>
> ---
> MAINTAINERS | 2 +
> drivers/hid/Kconfig | 9 +-
> drivers/hid/Makefile | 2 +-
> drivers/hid/hid-himax-83102j.c | 3175 ++++++++++++++++++++++++++++++++
> drivers/hid/hid-himax-83102j.h | 923 ++++++++++
> 5 files changed, 4109 insertions(+), 2 deletions(-)
> create mode 100644 drivers/hid/hid-himax-83102j.c
> create mode 100644 drivers/hid/hid-himax-83102j.h
Implement review from v1.
NAK
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: Add Himax HX83102J touchscreen
From: Krzysztof Kozlowski @ 2023-12-06 11:00 UTC (permalink / raw)
To: Allen_Lin, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
linux-kernel
In-Reply-To: <TY0PR06MB5611A2BEF3FAC1648CFFEE149E84A@TY0PR06MB5611.apcprd06.prod.outlook.com>
On 06/12/2023 11:35, Allen_Lin wrote:
> Add the HX83102j touchscreen device tree bindings documents.
>
> Signed-off-by: Allen_Lin <allencl_lin@hotmail.com>
> ---
> .../bindings/input/himax,hx8310xx.yaml | 70 +++++++++++++++++++
> MAINTAINERS | 6 ++
> 2 files changed, 76 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/himax,hx8310xx.yaml
>
Implement review from v1.
NAK
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 0/2] Add HX83102j driver for HIMAX HID touchscreen
From: Krzysztof Kozlowski @ 2023-12-06 11:00 UTC (permalink / raw)
To: Allen_Lin, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
linux-kernel
In-Reply-To: <TY0PR06MB5611F2CBE520DB21F15DF9B99E84A@TY0PR06MB5611.apcprd06.prod.outlook.com>
On 06/12/2023 11:35, Allen_Lin wrote:
> Hi,
> This driver implements for HIMAX HID touchscreen HX8310XX series.
>
> Using SPI interface to acquire HID packets from driver.
>
> Patchs notes as below
> 1. Add the Maintainer and devicetree bindings document for driver
> 2. Add the driver code and modify Kconfig/Makefiles to support the driver
>
> change in v2 :
> - Fix kernel test robot build warnings.
I reviewed v1, so you must fix everything I pointed out. This code is in
absolutely terrible shape. It way over complicated, with way too many
structures, way too many abstractions (you must have exactly 0
abstractions). It was not written in Linux style.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v1 2/2] Input: Add Himax HX83102J touchscreen driver
From: Krzysztof Kozlowski @ 2023-12-06 10:58 UTC (permalink / raw)
To: Allen_Lin, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
linux-kernel
In-Reply-To: <TY0PR06MB5611228C6B67733DC93842C99E85A@TY0PR06MB5611.apcprd06.prod.outlook.com>
On 05/12/2023 11:05, Allen_Lin wrote:
> Add a new driver for Himax touchscreen series touchscreen controllers.
> This driver supports Himax IC using the SPI interface to
> acquire HID packets.
>
> Signed-off-by: Allen_Lin <allencl_lin@hotmail.com>
> ---
> MAINTAINERS | 2 +
> drivers/hid/Kconfig | 9 +-
> drivers/hid/Makefile | 2 +-
> drivers/hid/hid-himax-83102j.c | 3176 ++++++++++++++++++++++++++++++++
> drivers/hid/hid-himax-83102j.h | 927 ++++++++++
> 5 files changed, 4114 insertions(+), 2 deletions(-)
> create mode 100644 drivers/hid/hid-himax-83102j.c
> create mode 100644 drivers/hid/hid-himax-83102j.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cadcbf7294a6..d0d5ab7ea1e6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9507,6 +9507,8 @@ M: Allen Lin <allencl_lin@hotmail.com>
> L: linux-input@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/input/himax,hx8310xx.yaml
> +F: drivers/hid/hid-himax-83102j.c
> +F: drivers/hid/hid-himax-83102j.h
drivers/hid/hid-himax-83102j*
>
> HIMAX HX83112B TOUCHSCREEN SUPPORT
> M: Job Noorman <job@noorman.info>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 4ce74af79657..896af236c1f8 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1324,7 +1324,14 @@ config HID_KUNIT_TEST
> Documentation/dev-tools/kunit/.
>
> If in doubt, say "N".
> -
Why?
> +config HID_HIMAX
> + tristate "Himax touchpanel CHIPSET"
> + depends on HID
> + help
> + Say Y here if you have a Himax CHIPSET touchscreen.
> + HIMAX controllers are multi touch controllers which can
> + report 10 touches at a time.
> + If unsure, say N.
> endmenu
>
> source "drivers/hid/bpf/Kconfig"
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 8a06d0f840bc..a0f0fc3261e0 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -156,7 +156,7 @@ hid-uclogic-test-objs := hid-uclogic-rdesc.o \
> hid-uclogic-params.o \
> hid-uclogic-rdesc-test.o
> obj-$(CONFIG_HID_KUNIT_TEST) += hid-uclogic-test.o
> -
Why?
> +obj-$(CONFIG_HID_HIMAX) += hid-himax-83102j.o
> obj-$(CONFIG_USB_HID) += usbhid/
> obj-$(CONFIG_USB_MOUSE) += usbhid/
> obj-$(CONFIG_USB_KBD) += usbhid/
> diff --git a/drivers/hid/hid-himax-83102j.c b/drivers/hid/hid-himax-83102j.c
> new file mode 100644
> index 000000000000..fa1b22477457
> --- /dev/null
> +++ b/drivers/hid/hid-himax-83102j.c
> @@ -0,0 +1,3176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Himax Driver Code for HX83102J to simulate HID
> + *
> + * Copyright (C) 2023 Himax Corporation.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
Drop boiler plate.
This is a very poor quality code. Looks like taken straight from crappy
downstream code and sent without cleaning up.
There is way too many things to fix here... not mentioning even multiple
build warning and failure reports!
> + */
> +
> +#include "hid-himax-83102j.h"
> +
> +struct himax_ts_data *g_himax_ts;
Drop, no global variables.
> +struct himax_core_fp g_core_fp;
Drop, no global variables.
> +
> +#define FW_EXT_NAME(FWNAME) FWNAME ".bin"
Drop
> +char *g_fw_boot_upgrade_name = FW_EXT_NAME(BOOT_UPGRADE_FWNAME);
Drop
> +struct himax_core_command_regs g_core_regs;
Drop
> +
> +
> +union host_ext_rd_t g_host_ext_rd = {
> + .host_report_descriptor = {
> + 0x06, 0x00, 0xFF,// Usage Page (Vendor-defined)
> + 0x09, 0x01,// Usage (0x1)
> + 0xA1, 0x01,// Collection (Application)
> + 0x75, 0x08,// Report Size (8)
> + 0x15, 0x00,// Logical Minimum (0)
> + 0x26, 0xFF, 0x00,// Logical Maximum (255)
> + 0x85, 0x05,// Report ID (5)
> + 0x09, 0x02,// Usage (0x2)
> + 0x96, 0xFF, 0x00,// Report Count (255)
> + 0xB1, 0x02,// Feature (ID: 5, sz: 2040 bits(255 bytes))
> + 0x85, 0x06,// Report ID (6)
> + 0x09, 0x02,// Usage (0x2)
> + 0x96, (HID_REG_SZ_MAX & 0xFF), (HID_REG_SZ_MAX >> 8),
> + 0xB1, 0x02,// Feature (ID: 6, sz: 72 bits(9 bytes))
> + 0x85, 0x07,// Report ID (7)
> + 0x09, 0x02,// Usage (0x2)
> + 0x96, 0x04, 0x00,// Report Count (4)
> + 0xB1, 0x02,// Feature (ID: 7, sz: 32 bits(4 bytes))
> + 0x85, 0x08,// Report ID (8)
> + 0x09, 0x02,// Usage (0x2)
> + 0x96, 0x8D, 0x13,// Report Count (5005)
> + 0xB1, 0x02,// Feature (ID: 8, sz: 40040 bits(5005 bytes))
> + // 0x85, 0x09,// Report ID (9)
> + // 0x09, 0x02,// Usage (0x2)
> + // 0x96, 0x4F, 0x03,// Report Count (847)
> + // 0xB1, 0x02,// Feature (ID: 9, sz: 6776 bits(847 bytes))
Why these are commented out? Every piece of dead code must be documented
why it is dead.
> + 0x85, 0x0A,// Report ID (10)
> + 0x09, 0x02,// Usage (0x2)
> + 0x96, 0x00, 0x04,// Report Count (1024)
> + 0x91, 0x02,// Output (ID: 10, sz: 8192 bits(1024 bytes))
> + 0x85, 0x0B,// Report ID (11)
> + 0x09, 0x02,// Usage (0x2)
> + 0x96, 0x01, 0x00,// Report Count (1)
> + 0xB1, 0x02,// Feature (ID: 11, sz: 8 bits(1 bytes))
> + 0x85, 0x0C,// Report ID (12)
> + 0x09, 0x02,// Usage (0x2)
> + 0x96, 0x01, 0x00,// Report Count (1)
> + 0xB1, 0x02,// Feature (ID: 12, sz: 8 bits(1 bytes))
> + 0x85, 0x31,// Report ID (49)
> + 0x09, 0x02,// Usage (0x2)
> + 0x96, 0x01, 0x00,// Report Count (1)
> + 0xB1, 0x02,// Feature (ID: 49, sz: 8 bits(1 bytes))
> + 0xC0,// End Collection
> + },
> +};
> +
> +const unsigned int host_ext_report_desc_sz =
> + sizeof(g_host_ext_rd.host_report_descriptor);
Drop, use directly sizeof() in the code.
> +
> +union heatmap_rd_t g_heatmap_rd = {
> + .host_report_descriptor = {
> + 0x05, 0x0D,// Usage Page (Digitizers)
> + 0x09, 0x0F,// Usage (0xF)
> + 0xA1, 0x01,// Collection (Application)
> + 0x85, 0x61,// Report ID (97)
> + 0x05, 0x0D,// Usage Page (Digitizers)
> + 0x15, 0x00,// Logical Minimum (0)
> + 0x27, 0xFF, 0xFF, 0x00, 0x00,// Logical Maximum (65535)
> + 0x75, 0x10,// Report Size (16)
> + 0x95, 0x01,// Report Count (1)
> + 0x09, 0x6A,// Usage (0x6A)
> + 0x81, 0x02,// Input (ID: 97, sz: 16 bits(2 bytes))
> + 0x09, 0x6B,// Usage (0x6B)
> + 0x81, 0x02,// Input (ID: 97, sz: 16 bits(2 bytes))
> + 0x27, 0xFF, 0xFF, 0xFF, 0xFF,// Logical Maximum (-1)
> + 0x75, 0x20,// Report Size (32)
> + 0x09, 0x56,// Usage (0x56)
> + 0x81, 0x02,// Input (ID: 97, sz: 32 bits(4 bytes))
> + 0x05, 0x01,// Usage Page (Generic Desktop)
> + 0x09, 0x3B,// Usage (0x3B)
> + 0x81, 0x02,// Input (ID: 97, sz: 32 bits(4 bytes))
> + 0x05, 0x0D,// Usage Page (Digitizers)
> + 0x26, 0xFF, 0x00,// Logical Maximum (255)
> + 0x09, 0x6C,// Usage (0x6C)
> + 0x75, 0x08,// Report Size (8)
> + 0x96, 0x00, 0x0C,// Report Count (3072)
> + 0x81, 0x02,// Input (ID: 97, sz: 24576 bits(3072 bytes))
> + 0xC0,// End Collection
> + },
> +};
> +
> +const unsigned int host_heatmap_report_desc_sz =
> + sizeof(g_heatmap_rd.host_report_descriptor);
Drop
> +
> +void himax_rst_gpio_set(int pinnum, u8 value)
Really, so nowwhere is static? Everything must be global? Sorry, but
this is not even kernel related, but basic C programming. All your
functions *MUST* be static.
> +{
> + gpio_direction_output(pinnum, value);
Anyway, useless wrapper, drop entire function.
> +}
> +
> +int himax_gpio_power_config(struct himax_ts_data *ts,
static
> + struct himax_platform_data *pdata)
> +{
> + int error = 0;
> +
> + if (gpio_is_valid(pdata->gpio_reset)) {
> + error = gpio_request(pdata->gpio_reset, "himax-reset");
What the crap is this? Isn't gpio_request() some crazy old legacy code?
Open some new driver and look how GPIO is handled there. You must use
gpio descriptors.
> +
> + if (error < 0) {
> + E("request reset pin failed");
Drop "E" whatever it is and use standard Linux functions: dev_err().
This applies everywhere.
Abyway, request of GPIO does not happen in power config but in probe.
> + goto err_gpio_reset_req;
> + }
> +
> + error = gpio_direction_output(pdata->gpio_reset, 0);
> +
> + if (error) {
> + E("unable to set direction for gpio [%d]",
> + pdata->gpio_reset);
> + goto err_gpio_reset_dir;
> + }
> + }
> +
> + if (pdata->vccd_supply) {
> + error = regulator_enable(pdata->vccd_supply);
error? Open existing driver and look how it is named there. Usually ret
> + if (error) {
> + E("unable to enable vccd supply");
This is not a Linux kernel code style. Open existing driver and look how
it is done there.
...
> +
> +int himax_chip_suspend(struct himax_ts_data *ts)
> +{
> + int ret = 0;
> +
> + if (ts->suspended) {
> + I("Already suspended, skip...");
> + goto END;
> + } else {
> + ts->suspended = true;
Why?
> + }
> +
> + I("enter");
Drop
> + g_core_fp.fp_suspend_proc(ts, ts->suspended);
> +
> + himax_int_enable(ts, false);
> +
> + if (!ts->use_irq) {
> + s32 cancel_state;
> +
> + cancel_state = cancel_work_sync(&ts->work);
> + if (cancel_state)
> + himax_int_enable(ts, true);
> + }
> +
> + atomic_set(&ts->suspend_mode, 1);
?!?!? Why?
> +
> + if (ts->pdata) {
> + if (ts->pdata->power_off_3v3) {
> + if (ts->pdata->vcca_supply)
> + ret = regulator_disable(ts->pdata->vcca_supply);
> + }
> + }
> +
> +END:
lowercase
> + hx_hid_remove(ts);
> + I("END");
No, come on, drop. Drop such silly code EVERYWHERE.
> +
> + return ret;
> +}
> +
> +int himax_chip_resume(struct himax_ts_data *ts)
> +{
> + int ret = 0;
> +
> + if (!ts->suspended && ts->resume_success) {
> + I("Already resumed, skip...");
> + goto END;
> + } else {
> + ts->suspended = false;
> + }
> + ts->resume_success = false;
> +
> + I("enter");
> + /* continuous N times record, not total N times. */
> + ts->excp_zero_event_count = 0;
> +
> + atomic_set(&ts->suspend_mode, 0);
> + if (ts->pdata) {
> + if (ts->pdata->power_off_3v3) {
> + if (ts->pdata->vcca_supply)
> + ret = regulator_enable(ts->pdata->vcca_supply);
> + }
> + }
> +
> + g_core_fp.fp_resume_proc(ts, ts->suspended);
> + // hx_report_all_leave_event(ts);
> + if (ts->resume_success) {
> + hx_hid_probe(ts);
> + himax_int_enable(ts, true);
> + } else {
> + E("resume failed!");
> + ret = -ECANCELED;
> + }
> +END:
> + I("END");
> +
> + return ret;
> +}
> +
> +int himax_suspend(struct device *dev)
> +{
> + struct himax_ts_data *ts = dev_get_drvdata(dev);
> +
> + I("enter");
> + if (!ts->initialized) {
> + E("init not ready, skip!");
> + return -ECANCELED;
> + }
> + himax_chip_suspend(ts);
> + return 0;
> +}
> +
> +int himax_resume(struct device *dev)
> +{
> + int ret = 0;
> + struct himax_ts_data *ts = dev_get_drvdata(dev);
> +
> + I("enter");
> + /*
> + * wait until device resume for TDDI
> + * TDDI: Touch and display Driver IC
> + */
> + if (!ts->initialized) {
> +#if !defined(CONFIG_FB)
> + if (himax_chip_init(ts))
> + return -ECANCELED;
> +#else
> + E("init not ready, skip!");
> + return -ECANCELED;
> +#endif
> + }
> + ret = himax_chip_resume(ts);
> + if (ret < 0) {
> + E("resume failed!");
> + I("retry resume");
> + schedule_delayed_work(&ts->work_resume_delayed_work,
> + msecs_to_jiffies(ts->pdata->ic_resume_delay));
> + // I("try int rescue");
> + // himax_int_enable(ts, 1);
You need to clean it up to look like Linux code. Sorry, it's just not
matching anything in the kernel.
> + }
> +
> + return ret;
> +}
> +
> +static void himax_resume_work_func(struct work_struct *work)
> +{
> + struct himax_ts_data *ts = NULL;
> +
> + ts = container_of(work, struct himax_ts_data,
> + work_resume_delayed_work.work);
> + if (!ts) {
> + E("ts is NULL");
> + return;
> + }
> + himax_chip_resume(ts);
> +}
> +
> +#if defined(CONFIG_PM_SLEEP)
> +static const struct dev_pm_ops hx_hid_pm = {
> + .suspend = himax_suspend,
> + .resume = himax_resume,
> + .restore = himax_resume,
> +};
> +
> +#define HX_HID_PM (&hx_hid_pm)
> +#else
> +#define HX_HID_PM NULL
> +#endif
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id himax_match_table[] = {
> + { .compatible = "himax,hx83102j" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, himax_match_table);
> +#define himax_match_table of_match_ptr(himax_match_table)
> +#else
> +#define himax_match_table NULL
> +#endif
> +
> +
> +#define hx_acpi_spi_table NULL
> +
> +
What are all these above? Empty defines? Drop.
> +int himax_chip_init(struct himax_ts_data *ts)
> +{
> + int err = PROBE_FAIL;
What is that? There is no errno like PROBE_FAIL. Does not exist.
> + struct himax_platform_data *pdata = ts->pdata;
> +
> + ts->chip_max_dsram_size = 0;
> + ts->notouch_frame = 0;
> + ts->ic_notouch_frame = 0;
> +
> + if (g_core_fp.fp_chip_init) {
> + g_core_fp.fp_chip_init(ts);
This is some weird code. You need to refactor all this to look like
Linux code.
> + } else {
> + E("function point of chip_init is NULL!");
> + goto error_ic_init_failed;
> + }
> + g_core_fp.fp_touch_information(ts);
> +
> + spin_lock_init(&ts->irq_lock);
> +
> + if (himax_ts_register_interrupt(ts)) {
> + E("register interrupt failed");
> + goto err_register_interrupt_failed;
> + }
> +
> + himax_int_enable(ts, false);
> +
> + ts->himax_hid_debug_wq =
> + create_singlethread_workqueue("HX_hid_debug");
> + if (!ts->himax_hid_debug_wq) {
> + E("allocate himax_hid_debug_wq failed");
> + err = -ENOMEM;
> + goto err_hid_debug_wq_failed;
> + }
> + INIT_DELAYED_WORK(&ts->work_hid_update, hx_hid_update);
> +
> + ts->himax_resume_delayed_work_wq =
> + create_singlethread_workqueue("HX_resume_delayed_work");
> + if (!ts->himax_resume_delayed_work_wq) {
> + E("allocate himax_resume_delayed_work_wq failed");
> + err = -ENOMEM;
> + goto err_resume_delayed_work_wq_failed;
> + }
> + INIT_DELAYED_WORK(&ts->work_resume_delayed_work, himax_resume_work_func);
> +
> + g_core_fp.fp_calc_touch_data_size(ts);
> +
> +#if defined(CONFIG_OF)
> + pdata->cable_config[0] = 0xF0;
> + pdata->cable_config[1] = 0x00;
> +#endif
> +
> + ts->suspended = false;
> + ts->usb_connected = 0x00;
> + ts->cable_config = pdata->cable_config;
> + ts->initialized = true;
> + return 0;
> +
> +err_resume_delayed_work_wq_failed:
> + destroy_workqueue(ts->himax_hid_debug_wq);
> +err_hid_debug_wq_failed:
> + himax_ts_unregister_interrupt(ts);
> +err_register_interrupt_failed:
> +error_ic_init_failed:
> + ts->probe_fail_flag = 1;
> + return err;
> +}
> +
> +void himax_chip_deinit(struct himax_ts_data *ts)
> +{
> + kfree(ts->zf_update_cfg_buffer);
> + ts->zf_update_cfg_buffer = NULL;
> +
> + himax_ts_unregister_interrupt(ts);
> +
> + himax_report_data_deinit(ts);
> +
> + cancel_delayed_work_sync(&ts->work_resume_delayed_work);
> + destroy_workqueue(ts->himax_resume_delayed_work_wq);
> + destroy_workqueue(ts->himax_hid_debug_wq);
> +
> + ts->probe_fail_flag = 0;
> +
> + I("Common section deinited!");
> +}
> +
> +static void himax_platform_deinit(struct himax_ts_data *ts)
> +{
> + struct himax_platform_data *pdata = NULL;
> +
> + I("entering");
> +
> + if (!ts) {
> + E("ts is NULL");
> + return;
> + }
> +
> + pdata = ts->pdata;
> + if (!pdata) {
> + E("pdata is NULL");
> + return;
> + }
> +
> + himax_gpio_power_deconfig(pdata);
> +
> + kfree(ts->ic_data);
> + ts->ic_data = NULL;
> +
> + kfree(pdata);
> + pdata = NULL;
> + ts->pdata = NULL;
> +
> + kfree(ts->xfer_buff);
> + ts->xfer_buff = NULL;
Spaghetti... Why this is not devm?
> +
> + I("exit");
> +}
> +
> +static bool himax_platform_init(struct himax_ts_data *ts,
> + struct himax_platform_data *local_pdata)
Four inits... spaghetti code.
> +{
> + int err = PROBE_FAIL;
> + struct himax_platform_data *pdata;
> +
> + I("entering");
> + ts->xfer_buff = kcalloc
> + (HX_FULL_STACK_RAWDATA_SIZE, sizeof(u8), GFP_KERNEL);
You must fix all checkpatch warnings. This is wrong wrapping.
> + if (!ts->xfer_buff) {
> + err = -ENOMEM;
> + goto err_xfer_buff_fail;
> + }
> +
> + I("PDATA START");
> + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) { /*Allocate Platform data space*/
Useless comment.
> + err = -ENOMEM;
> + goto err_dt_platform_data_fail;
> + }
> +
> + I("ts->ic_data START");
> + ts->ic_data = kzalloc(sizeof(*ts->ic_data), GFP_KERNEL);
> + if (!ts->ic_data) { /*Allocate IC data space*/
Useless comment.
> + err = -ENOMEM;
> + goto err_dt_ic_data_fail;
> + }
...
> +int himax_parse_dt(struct device_node *dt, struct himax_platform_data *pdata)
> +{
> + /* pid_fw_name size = length of default_fw_name + length of "_XXXX" +
> + * length of ".bin" + null terminator.
> + */
> + static char pid_fw_name[ARRAY_SIZE(default_fw_name) + 5 + 4 + 1] = {0};
> + int tmp = 0;
> + const int pid_prop_args = 2;
> + u32 data = 0;
> + int id_gpios[8] = {0};
> + int counter = 0;
> + int i = 0;
> + s16 id_value = -1;
> + int dts_irq = 0;
> + int err = 0;
Absolutely unreadable.
> +
> + UNUSED(default_fw_name);
???
> + if (!dt || !pdata) {
> + E("DT: dev or pdata is NULL");
> + return -EINVAL;
> + }
> +
> + dts_irq = of_irq_get(dt, 0);
No, use standard code.
> + D("DT: dts_irq = %d", dts_irq);
> + if (dts_irq <= 0) {
> + if (dts_irq == -EPROBE_DEFER)
> + E("DT: dts_irq = -EPROBE_DEFER");
dev_err_probe.
> + return -EIO;
?!?! Why do you cast errors?
> + }
> +
> + pdata->of_irq = dts_irq;
> + pdata->gpio_irq = -1;
> +
> + pdata->gpio_reset = of_get_named_gpio(dt, "reset-gpios", 0);
> + if (!gpio_is_valid(pdata->gpio_reset)) {
> + I(" DT:gpio-rst value is not valid");
> + return -EIO;
> + }
> +
> + I(" DT:interrupt=%d, reset=%d",
> + pdata->of_irq, pdata->gpio_reset);
> + counter = gpiod_count(pdata->ts->dev, "himax,id");
NAK, there is no such property.
> + if (counter > 0) {
> + for (i = 0 ; i < counter ; i++) {
> + id_gpios[i] = of_get_named_gpio(dt, "himax,id-gpios", i);
NAK, there is no such property.
> + if (!gpio_is_valid(id_gpios[i])) {
> + I(" DT:gpio-id value is not valid");
> + return -EIO;
> + }
> + I(" DT:gpio-id[%d]=%d", i, id_gpios[i]);
> + }
> + id_value = 0;
> + for (i = 0 ; i < counter ; i++) {
> + gpio_direction_input(id_gpios[i]);
> + id_value |= gpio_get_value(id_gpios[i]) << i;
> + }
> + I(" DT:gpio-id value=%04X", id_value);
> + pdata->panel_id = id_value;
> + } else {
> + pdata->panel_id = -1;
> + D(" DT:gpio-id not found");
> + }
This is absolutely unreadable code.
> +
> + // himax,ic_det_delay unit is millisecond
> + if (of_property_read_u32(dt, "himax,ic-det-delay-ms", &data)) {
NAK, there is no such property.
> + pdata->ic_det_delay = 0;
> + D(" DT:himax,ic-det-delay-ms not found");
> + } else {
> + pdata->ic_det_delay = data;
> + I(" DT:himax,ic-det-delay-ms=%d", pdata->ic_det_delay);
> + }
> +
> + // himax,ic_resume_delay unit is millisecond
> + if (of_property_read_u32(dt, "himax,ic-resume-delay-ms", &data)) {
NAK, there is no such property.
> + pdata->ic_resume_delay = 0;
> + D(" DT:himax,ic-resume-delay-ms not found");
> + } else {
> + pdata->ic_resume_delay = data;
> + I(" DT:himax,ic-resume-delay-ms=%d", pdata->ic_resume_delay);
> + }
> +
> + if (of_property_read_bool(dt, "himax,has-flash")) {
> + pdata->is_zf = false;
> + D(" DT:himax,has-flash");
> + } else {
> + pdata->is_zf = true;
> + I(" DT:himax,has-flash not found, load firmware from file");
> + }
> +
> + if (of_property_read_bool(dt, "vccd-supply")) {
> + pdata->vccd_supply = regulator_get(pdata->ts->dev, "vccd");
> + if (IS_ERR(pdata->vccd_supply)) {
> + E(" DT:failed to get vccd supply");
> + err = PTR_ERR(pdata->vccd_supply);
> + pdata->vccd_supply = NULL;
> + return err;
> + }
> + I(" DT:vccd-supply=%p", pdata->vccd_supply);
> + } else {
> + pdata->vccd_supply = NULL;
> + }
> +
> + if (of_property_read_bool(dt, "vcca-supply")) {
> + pdata->vcca_supply = regulator_get(pdata->ts->dev, "vcca");
> + if (IS_ERR(pdata->vcca_supply)) {
> + E(" DT:failed to get vcca supply");
> + err = PTR_ERR(pdata->vcca_supply);
> + pdata->vcca_supply = NULL;
> + return err;
> + }
> + I(" DT:vcca-supply=%p", pdata->vcca_supply);
> + } else {
> + pdata->vcca_supply = NULL;
> + }
> +
> + /*
> + * check himax,pid first, if exist then check if it is single.
> + * Single case: himax,pid = <0x1002>; // 0x1002 is pid value
> + * Multiple case: himax,pid = <id_value0 00x1001>, <id_value1 0x1002>;
> + * When id_value >= 0, check the mapping listed to use the pid value.
> + */
> + if (of_get_property(dt, "himax,pid", &data)) {
NAK, there is no such property.
This is very, very poor code with hundreds of style issues. This just
does not look like ready for sending upstream. Plus you stuffed here
many undocumented properties, so you bypass the bindings review.
...
> +
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/acpi.h>
> +#include <linux/spi/spi.h>
> +#include <linux/hid.h>
> +#include <linux/sizes.h>
> +#include <linux/fs.h>
> +#include <linux/gpio.h>
> +#include <linux/types.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_irq.h>
> +#include <linux/proc_fs.h>
> +#include <linux/version.h>
> +#include <linux/firmware.h>
> +#include <linux/stddef.h>
> +#include <linux/power_supply.h>
> +
> +#define HIMAX_DRIVER_VER "1.0.0"
Drop
> +
> +#define HIMAX_BUS_RETRY_TIMES 3
> +#define BUS_RW_MAX_LEN 0x20006
> +#define BUS_R_HLEN 3
> +#define BUS_R_DLEN ((BUS_RW_MAX_LEN - BUS_R_HLEN) - ((BUS_RW_MAX_LEN - BUS_R_HLEN) % 4))
> +#define BUS_W_HLEN 2
> +#define BUS_W_DLEN ((BUS_RW_MAX_LEN - BUS_W_HLEN) - ((BUS_RW_MAX_LEN - BUS_W_HLEN) % 4))
> +#define FIX_HX_INT_IS_EDGE (false)
Drop
> +
> +#define HX_DELAY_BOOT_UPDATE (2000)
> +#define HID_REG_SZ_MAX (1 + 4 + 1 + 4 + 256)
?!?! More cryptic constants...
> +
> +enum HID_ID_FUNCT {
> + ID_CONTACT_COUNT = 0x03,
???
> +};
> +
> +#define HID_RAW_DATA_TYPE_DELTA (0x09)
> +#define HID_RAW_DATA_TYPE_RAW (0x0A)
> +#define HID_RAW_DATA_TYPE_BASELINE (0x0B)
> +#define HID_RAW_DATA_TYPE_NORMAL (0x00)
???
> +
> +enum HID_FW_UPDATE_STATUS_CODE {
> + FWUP_ERROR_NO_ERROR = 0x77,
> + FWUP_ERROR_MCU_00 = 0x00,
> + FWUP_ERROR_MCU_A0 = 0xA0,
> + FWUP_ERROR_NO_BL = 0xC1,
> + FWUP_ERROR_NO_MAIN = 0xC2,
> + FWUP_ERROR_BL_COMPLETE = 0xB1,
> + FWUP_ERROR_BL = 0xB2,
> + FWUP_ERROR_PW = 0xB3,
> + FWUP_ERROR_ERASE_FLASH = 0xB4,
> + FWUP_ERROR_FLASH_PROGRAMMING = 0xB5,
> + FWUP_ERROR_NO_DEVICE = 0xFFFFFF00,
> + FWUP_ERROR_LOAD_FW_BIN = 0xFFFFFF01,
> + FWUP_ERROR_INITIAL = 0xFFFFFF02,
> + FWUP_ERROR_POLLING_TIMEOUT = 0xFFFFFF03,
> + FWUP_ERROR_FW_TRANSFER = 0xFFFFFF04
> +};
> +
> +struct himax_ts_data;
Drop
> +union host_ext_rd_t;
Drop
> +union heatmap_rd_t;
Drop
> +
> +#define HID_FW_UPDATE_BL_CMD (0x77)
> +#define HID_FW_UPDATE_MAIN_CMD (0x55)
> +
> +int hx_hid_probe(struct himax_ts_data *ts);
> +void hx_hid_remove(struct himax_ts_data *ts);
Drop
> +
> +void hx_hid_update_info(struct himax_ts_data *ts);
> +int hx_hid_report(const struct himax_ts_data *ts, u8 *data, s32 len);
Drop
> +
> +enum fix_touch_info {
> + FIX_HX_RX_NUM = 48,
> + FIX_HX_TX_NUM = 32,
> + FIX_HX_BT_NUM = 0,
> + FIX_HX_MAX_PT = 10,
> + FIX_HX_STYLUS_FUNC = 1,
> + FIX_HX_STYLUS_ID_V2 = 0,
> + FIX_HX_STYLUS_RATIO = 1,
> + HX_STACK_ORG_LEN = 128
> +};
?
> +
> +#define himax_dev_name "hx_spi_hid_tp"
Drop
> +
> +#if defined(CONFIG_FB)
> +#include <linux/notifier.h>
> +#include <linux/fb.h>
> +#endif
Drop everything above, entire file is so far useless.
> +
> +/* log macro */
> +#define I(fmt, arg...) pr_info("[HXTP][%s]: " fmt "\n", __func__, ##arg)
> +#define W(fmt, arg...) pr_warn("[HXTP][WARNING][%s]: " fmt "\n", __func__, ##arg)
> +#define E(fmt, arg...) pr_err("[HXTP][ERROR][%s]: " fmt "\n", __func__, ##arg)
> +#define D(fmt, arg...) pr_debug("[HXTP][DEBUG][%s]: " fmt "\n", __func__, ##arg)
NAK on all these
> +
> +#define himax_dev_name "hx_spi_hid_tp"
> +
> +#define BOOT_UPGRADE_FWNAME "himax_i2chid"
> +
> +#define MPAP_FWNAME "himax_mpfw.bin"
> +
> +#define UNUSED(x) ((void)(x))
NAK.
> +static const char default_fw_name[] = BOOT_UPGRADE_FWNAME;
> +
> +#define DATA_LEN_4 4
> +#define ADDR_LEN_4 4
> +#define MAX_I2C_TRANS_SZ 128
> +#define HIMAX_REG_RETRY_TIMES 5
> +#define FW_PAGE_SZ 128
> +#define HX1K 0x400
?
> +#define HX_48K_SZ 0xC000
> +#define HX64K 0x10000
?
> +
> +#define HX_RW_REG_FAIL (-1)
> +
> +#define hx83102j_fw_addr_raw_out_sel 0x100072EC
> +#define hx83102j_ic_adr_tcon_rst 0x80020004
> +#define hx83102j_data_adc_num 400 /* 200x2 */
> +#define hx83102j_notouch_frame 0
> +#define HX83102J_FLASH_SIZE 261120
> +#define HX83102j_ic_addr_hw_crc 0x80010000
> +#define HX83102j_data_hw_crc 0x0000ECCE
> +
> +/* CORE_IC */
> + #define ic_adr_ahb_addr_byte_0 0x00
All defines MUST be uppercase
> + #define ic_adr_ahb_rdata_byte_0 0x08
> + #define ic_adr_ahb_access_direction 0x0c
> + #define ic_adr_conti 0x13
> + #define ic_adr_incr4 0x0D
> + #define ic_adr_i2c_psw_lb 0x31
> + #define ic_adr_i2c_psw_ub 0x32
> + #define ic_cmd_ahb_access_direction_read 0x00
> + #define ic_cmd_conti 0x31
> + #define ic_cmd_incr4 0x10
> + #define ic_cmd_i2c_psw_lb 0x27
> + #define ic_cmd_i2c_psw_ub 0x95
> + #define ic_adr_tcon_on_rst 0x80020020
> + #define ic_adr_cs_central_state 0x900000A8
Weird indentation.
...
> + /* origin is 20/50 */
> + #define RST_LOW_PERIOD_S 5000
> + #define RST_LOW_PERIOD_E 5100
> + #define RST_HIGH_PERIOD_ZF_S 5000
> + #define RST_HIGH_PERIOD_ZF_E 5100
> + #define RST_HIGH_PERIOD_S 50000
> + #define RST_HIGH_PERIOD_E 50100
Missing blank line.
> +enum data_type {
So here it is lowercase, then why in other places you use upper case?
> + HX_REG = 0xA5,
> + HX_DATA
> +};
> +
> +struct hx_reg_t {
> + union {
> + u32 word;
> + u16 half[2];
> + u8 byte[4];
> + } data;
> + u32 len;
> + u32 data_type;
> +};
Pointless to have in header.
> +
> +struct data_pack_t {
> + union {
> + u32 *word;
> + u16 *half;
> + u8 *byte;
> + void *ptr;
> + } data;
> + /* length in byte unit */
> + u32 len;
> + u32 data_type;
> +};
> +
> +#define BYTE_REG(_reg, _data) \
> + { \
> + _reg.data.byte[0] = (_data) & 0xFF; \
> + _reg.len = 1; \
> + _reg.data_type = HX_REG; \
> + }
> +#define HALF_REG(_reg, _data) \
> + { \
> + _reg.data.half[0] = cpu_to_le16((_data) & 0xFFFF); \
> + _reg.len = 2; \
> + _reg.data_type = HX_REG; \
> + }
> +#define WORD_REG(_reg, _data) \
> + { \
> + _reg.data.word = cpu_to_le32(_data); \
> + _reg.len = 4; \
> + _reg.data_type = HX_REG; \
> + }
What are all thesE?
> +
> +// set val to already defined reg/data
> +#define VAL_SET(_var, _val) \
> + ({ \
> + bool _ret = true; \
> + do { \
> + if (_var.data_type == HX_DATA) { \
> + memset(_var.data.byte, 0, _var.len); \
> + do { \
> + switch (_var.len) { \
> + case 1: \
> + _var.data.byte[0] = (_val) & 0xFF; \
> + break; \
> + case 2: \
> + _var.data.half[0] = cpu_to_le16((_val) & 0xFFFF); \
> + break; \
> + case 3: \
> + _var.data.half[0] = cpu_to_le16((_val) & 0xFFFF); \
> + _var.data.byte[2] = ((_val) >> 16) & 0xFF; \
> + break; \
> + case 4: \
> + _var.data.word[0] =\
> + cpu_to_le32((_val) & 0xFFFFFFFF); \
> + break; \
> + default: \
> + _ret = false; \
> + break; \
> + };\
> + } while (0); \
> + } else { \
> + _ret = false; \
> + } \
> + } while (0); \
> + _ret; \
> + })
Drop entire macro.
> +
> +// set ptr/array to already defined reg/data
> +#define PTR_SET(_var, _ptr, _len) \
> + ({ \
> + bool _ret = true; \
> + do { \
> + if ((_len) > (_var).len) { \
> + _ret = false; \
> + break; \
> + } \
> + memcpy((_var).data.byte, _ptr, (_len)); \
> + (_var).len = (_len); \
> + } while (0); \
> + _ret; \
> + })
Drop.
> +
> +#define DEF_WORD_DATA(_data_name) \
> + u8 _data_name##_array[4] = {0}; \
> + struct data_pack_t _data_name = { \
> + .data.byte = _data_name##_array, \
> + .len = 4, \
> + .data_type = HX_DATA \
> + }
Drop
> +
> +#define ARRAY_DATA(_data, _byte_len) { \
> + _data.data.byte = (uint8_t *)_data, \
> + _data.len = _byte_len, \
> + .data_type = HX_DATA \
> + }
Drop
> +
> +#define REG_GET_VAL(_reg) \
> + ({ \
> + u32 _val = 0; \
> + do { \
> + switch (_reg.len) { \
> + case 1: \
> + _val = _reg.data.byte[0]; \
> + break; \
> + case 2: \
> + _val = le16_to_cpu(_reg.data.half[0]); \
> + break; \
> + case 3: \
> + _val = le16_to_cpu(_reg.data.half[0]) | (_reg.data.byte[2] << 16); \
> + break; \
> + case 4: \
> + _val = le32_to_cpu(_reg.data.word); \
> + break; \
> + } \
> + } while (0); \
> + _val; \
> + })
Drop
> +
> +struct ic_operation {
> + struct hx_reg_t addr_ahb_addr_byte_0;
> + struct hx_reg_t addr_ahb_rdata_byte_0;
> + struct hx_reg_t addr_ahb_access_direction;
> + struct hx_reg_t addr_conti;
> + struct hx_reg_t addr_incr4;
> + struct hx_reg_t adr_i2c_psw_lb;
> + struct hx_reg_t adr_i2c_psw_ub;
> + struct hx_reg_t data_ahb_access_direction_read;
> + struct hx_reg_t data_conti;
> + struct hx_reg_t data_incr4;
> + struct hx_reg_t data_i2c_psw_lb;
> + struct hx_reg_t data_i2c_psw_ub;
> + struct hx_reg_t addr_tcon_on_rst;
> + struct hx_reg_t addr_cs_central_state;
> +};
> +
> +struct fw_operation {
Why do you have 20 structures? This is real spaghetti...
> +struct rd_feature_unit_t {
> + u8 id_tag;
> + u8 id;
> + u8 usage_tag;
> + u8 usage;
> + u8 report_cnt_tag;
> + u16 report_cnt;
> + u8 feature_tag[2];
> +} __packed;
> +struct hx_hid_req_cfg_t {
> + u32 processing_id;
> + u32 data_type;
> + u32 self_test_type;
> + u32 handshake_set;
> + u32 handshake_get;
> + struct firmware *fw;
> + u32 current_size;
> + // HID REG READ/WRITE format:
> + // STANDARD TYPE
> + // [ID:1][READ/WRITE:1][REG_ADDR:4][REG_DATA:4] : 10 bytes
> + // 0 1 2~5 6~9
> + // EXT TYPE
> + // [ID:1][READ/WRITE:1][0xFFFFFFFF][REG_TYPE:1][REG_ADDR:1|4][REG_DATA:1~256]
> + // 0 1 2~5 6 7|7~10 8~263|11~266
> + union hx_dword_data_t reg_addr;
> + u32 reg_addr_sz;
> + u8 reg_data[HID_REG_SZ_MAX - 1 - 4];
> + u32 reg_data_sz;
> + u32 input_RD_de;
> +};
> +
> +enum cell_type {
> + CHIP_IS_ON_CELL,
> + CHIP_IS_IN_CELL
> +};
> +
> +#define HX_FULL_STACK_RAWDATA_SIZE \
> + (HX_STACK_ORG_LEN +\
> + (2 + FIX_HX_RX_NUM * FIX_HX_TX_NUM + FIX_HX_TX_NUM + FIX_HX_RX_NUM)\
> + * 2)
> +
> +struct himax_ic_data {
> + int vendor_fw_ver;
> + int vendor_config_ver;
> + int vendor_touch_cfg_ver;
> + int vendor_display_cfg_ver;
> + int vendor_cid_maj_ver;
> + int vendor_cid_min_ver;
> + int vendor_panel_ver;
> + int vendor_sensor_id;
> + int ic_adc_num;
> + u8 vendor_cus_info[12];
> + u8 vendor_proj_info[12];
> + u32 flash_size;
> + u32 HX_RX_NUM;
> + u32 HX_TX_NUM;
> + u32 HX_BT_NUM;
> + u32 HX_X_RES;
> + u32 HX_Y_RES;
> + u32 HX_MAX_PT;
> + u8 HX_INT_IS_EDGE;
> + u8 HX_STYLUS_FUNC;
> + u8 HX_STYLUS_ID_V2;
> + u8 HX_STYLUS_RATIO;
> + u32 icid;
> + bool enc16bits;
> + bool has_flash;
> +};
> +
> +enum HX_TS_PATH {
> + HX_REPORT_COORD = 1,
> + HX_REPORT_COORD_RAWDATA,
> +};
> +
> +enum HX_TS_STATUS {
> + HX_TS_GET_DATA_FAIL = -4,
> + HX_EXCP_EVENT,
> + HX_CHKSUM_FAIL,
> + HX_PATH_FAIL,
> + HX_TS_NORMAL_END = 0,
> + HX_EXCP_REC_OK,
> + HX_READY_SERVE,
> + HX_REPORT_DATA,
> + HX_EXCP_WARNING,
> + HX_IC_RUNNING,
> + HX_ZERO_EVENT_COUNT,
> + HX_RST_OK,
> +};
> +
> +enum HX_ERROR_CODE {
> + NO_ERR = 0,
> + READY_TO_SERVE = 1,
> + WORK_OUT = 2,
> + HX_EMBEDDED_FW = 3,
> + BUS_FAIL = -1,
> + HX_INIT_FAIL = -1,
> + MEM_ALLOC_FAIL = -2,
> + CHECKSUM_FAIL = -3,
> + GESTURE_DETECT_FAIL = -4,
> + INPUT_REGISTER_FAIL = -5,
> + FW_NOT_READY = -6,
> + LENGTH_FAIL = -7,
> + OPEN_FILE_FAIL = -8,
> + PROBE_FAIL = -9,
> + ERR_WORK_OUT = -10,
> + ERR_STS_WRONG = -11,
> + ERR_TEST_FAIL = -12,
> + HW_CRC_FAIL = 1
> +};
> +
> +struct himax_platform_data {
> + struct himax_ts_data *ts;
> + u16 pid;
> + bool power_off_3v3;
> + u8 cable_config[2];
> + int gpio_irq;
> + int of_irq;
> + int gpio_reset;
> + int ic_det_delay;
> + int ic_resume_delay;
> + int panel_id;
> + bool is_zf;
> + struct regulator *vccd_supply;
> + struct regulator *vcca_supply;
> +};
> +
> +struct hx_hid_fw_unit_t {
> + u8 cmd;
> + u16 bin_start_offset;
> + u16 unit_sz;
> +} __packed;
> +
> +struct hx_bin_desc_t {
> + u16 passwd;
> + u16 cid;
> + u8 panel_ver;
> + u16 fw_ver;
> + u8 ic_sign;
> + char customer[12];
> + char project[12];
> + char fw_major[12];
> + char fw_minor[12];
> + char date[12];
> + char ic_sign_2[12];
> +} __packed;
> +
> +struct hx_hid_desc_t {
> + u16 desc_length;
> + u16 bcd_version;
> + u16 report_desc_length;
> + u16 max_input_length;
> + u16 max_output_length;
> + u16 max_fragment_length;
> + u16 vendor_id;
> + u16 product_id;
> + u16 version_id;
> + u16 flags;
> + u32 reserved;
> +} __packed;
Why packed?
> +struct hx_hid_info_t {
> + struct hx_hid_fw_unit_t main_mapping[9];
> + struct hx_hid_fw_unit_t bl_mapping;
> + struct hx_bin_desc_t fw_bin_desc;
> + u16 vid;
> + u16 pid;
> + u8 cfg_info[32];
> + u8 cfg_version;
> + u8 disp_version;
> + u8 rx;
> + u8 tx;
> + u16 yres;
> + u16 xres;
> + u8 pt_num;
> + u8 mkey_num;
> + u8 debug_info[78];
> +} __packed;
Why packed?
> +
> +struct himax_ts_data {
Ok, this is final cherry on top.
You have 20 structures but it is not enough. You have now master
structure that for simple driver has 100 members.
Absolutely NAK.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 0/7] HID: i2c-hid: Rework wait for reset to match Windows
From: Jiri Kosina @ 2023-12-06 10:50 UTC (permalink / raw)
To: Hans de Goede
Cc: Benjamin Tissoires, Douglas Anderson, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231202224615.24818-1-hdegoede@redhat.com>
On Sat, 2 Dec 2023, Hans de Goede wrote:
> Here is v3 of my i2c-hid series reworking how the i2c-hid-core waits
> for reset to complete.
I've agree that it's worth proceeding with this series even despite it not
fixing the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET completely for good.
Now in hid.git#for-6.8/i2c-hid.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH v2 15/15] selftests/hid: fix ruff linter complains
From: Benjamin Tissoires @ 2023-12-06 10:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
rename ambiguous variables l, r, and m, and ignore the return values
of uhdev.get_evdev() and uhdev.get_slot()
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
new in v2
---
tools/testing/selftests/hid/tests/test_mouse.py | 14 +++++++-------
tools/testing/selftests/hid/tests/test_wacom_generic.py | 6 +++---
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_mouse.py b/tools/testing/selftests/hid/tests/test_mouse.py
index fd2ba62e783a..66daf7e5975c 100644
--- a/tools/testing/selftests/hid/tests/test_mouse.py
+++ b/tools/testing/selftests/hid/tests/test_mouse.py
@@ -52,13 +52,13 @@ class BaseMouse(base.UHIDTestDevice):
:param reportID: the numeric report ID for this report, if needed
"""
if buttons is not None:
- l, r, m = buttons
- if l is not None:
- self.left = l
- if r is not None:
- self.right = r
- if m is not None:
- self.middle = m
+ left, right, middle = buttons
+ if left is not None:
+ self.left = left
+ if right is not None:
+ self.right = right
+ if middle is not None:
+ self.middle = middle
left = self.left
right = self.right
middle = self.middle
diff --git a/tools/testing/selftests/hid/tests/test_wacom_generic.py b/tools/testing/selftests/hid/tests/test_wacom_generic.py
index f92fe8e02c1b..49186a27467e 100644
--- a/tools/testing/selftests/hid/tests/test_wacom_generic.py
+++ b/tools/testing/selftests/hid/tests/test_wacom_generic.py
@@ -909,7 +909,7 @@ class TestDTH2452Tablet(test_multitouch.BaseTest.TestMultitouch, TouchTabletTest
Ensure that the confidence bit being set to false should not result in a touch event.
"""
uhdev = self.uhdev
- evdev = uhdev.get_evdev()
+ _evdev = uhdev.get_evdev()
t0 = test_multitouch.Touch(1, 50, 100)
t0.confidence = False
@@ -917,6 +917,6 @@ class TestDTH2452Tablet(test_multitouch.BaseTest.TestMultitouch, TouchTabletTest
events = uhdev.next_sync_events()
self.debug_reports(r, uhdev, events)
- slot = self.get_slot(uhdev, t0, 0)
+ _slot = self.get_slot(uhdev, t0, 0)
- assert not events
\ No newline at end of file
+ assert not events
--
2.41.0
^ permalink raw reply related
* [PATCH v2 14/15] selftests/hid: fix mypy complains
From: Benjamin Tissoires @ 2023-12-06 10:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
No code change, only typing information added/ignored
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
new in v2
---
tools/testing/selftests/hid/tests/base.py | 4 ++--
tools/testing/selftests/hid/tests/test_tablet.py | 10 +++++-----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/base.py b/tools/testing/selftests/hid/tests/base.py
index 5d9c26dfc460..51433063b227 100644
--- a/tools/testing/selftests/hid/tests/base.py
+++ b/tools/testing/selftests/hid/tests/base.py
@@ -14,7 +14,7 @@ import logging
from hidtools.device.base_device import BaseDevice, EvdevMatch, SysfsFile
from pathlib import Path
-from typing import Final
+from typing import Final, List, Tuple
logger = logging.getLogger("hidtools.test.base")
@@ -155,7 +155,7 @@ class BaseTestCase:
# if any module is not available (not compiled), the test will skip.
# Each element is a tuple '(kernel driver name, kernel module)',
# for example ("playstation", "hid-playstation")
- kernel_modules = []
+ kernel_modules: List[Tuple[str, str]] = []
def assertInputEventsIn(self, expected_events, effective_events):
effective_events = effective_events.copy()
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index 9374bd7524ef..dc8b0fe9e7f3 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -87,9 +87,9 @@ class PenState(Enum):
)
def __init__(self, touch: BtnTouch, tool: Optional[ToolType], button: Optional[BtnPressed]):
- self.touch = touch
- self.tool = tool
- self.button = button
+ self.touch = touch # type: ignore
+ self.tool = tool # type: ignore
+ self.button = button # type: ignore
@classmethod
def from_evdev(cls, evdev) -> "PenState":
@@ -122,7 +122,7 @@ class PenState(Enum):
if tool is None:
button = None
- return cls((touch, tool, button))
+ return cls((touch, tool, button)) # type: ignore
def apply(self, events: List[libevdev.InputEvent], strict: bool) -> "PenState":
if libevdev.EV_SYN.SYN_REPORT in events:
@@ -162,7 +162,7 @@ class PenState(Enum):
if tool is None:
button = None
- new_state = PenState((touch, tool, button))
+ new_state = PenState((touch, tool, button)) # type: ignore
if strict:
assert (
new_state in self.valid_transitions()
--
2.41.0
^ permalink raw reply related
* [PATCH v2 13/15] selftests/hid: tablets: be stricter for some transitions
From: Benjamin Tissoires @ 2023-12-06 10:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
To accommodate for legacy devices, we rely on the last state of a
transition to be valid:
for example when we test PEN_IS_OUT_OF_RANGE to PEN_IS_IN_CONTACT,
any "normal" device that reports an InRange bit would insert a
PEN_IS_IN_RANGE state between the 2.
This is of course valid, but this solution prevents to detect false
releases emitted by some firmware:
when pressing an "eraser mode" button, they might send an extra
PEN_IS_OUT_OF_RANGE that we may want to filter.
So define 2 sets of transitions: one that is the ideal behavior, and
one that is OK, it won't break user space, but we have serious doubts
if we are doing the right thing. And depending on the test, either
ask only for valid transitions, or tolerate weird ones.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
changes in v2:
- fixed type annotations for apply()
- s/historical/historically
- explicitly call self._test_states with
allow_intermediate_states=(True|False)
---
tools/testing/selftests/hid/tests/test_tablet.py | 132 +++++++++++++++++++----
1 file changed, 113 insertions(+), 19 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index a82db66264c5..9374bd7524ef 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -13,7 +13,7 @@ from hidtools.util import BusType
import libevdev
import logging
import pytest
-from typing import Dict, Optional, Tuple
+from typing import Dict, List, Optional, Tuple
logger = logging.getLogger("hidtools.test.tablet")
@@ -124,7 +124,7 @@ class PenState(Enum):
return cls((touch, tool, button))
- def apply(self, events) -> "PenState":
+ def apply(self, events: List[libevdev.InputEvent], strict: bool) -> "PenState":
if libevdev.EV_SYN.SYN_REPORT in events:
raise ValueError("EV_SYN is in the event sequence")
touch = self.touch
@@ -163,13 +163,97 @@ class PenState(Enum):
button = None
new_state = PenState((touch, tool, button))
- assert (
- new_state in self.valid_transitions()
- ), f"moving from {self} to {new_state} is forbidden"
+ if strict:
+ assert (
+ new_state in self.valid_transitions()
+ ), f"moving from {self} to {new_state} is forbidden"
+ else:
+ assert (
+ new_state in self.historically_tolerated_transitions()
+ ), f"moving from {self} to {new_state} is forbidden"
return new_state
def valid_transitions(self) -> Tuple["PenState", ...]:
+ """Following the state machine in the URL above.
+
+ Note that those transitions are from the evdev point of view, not HID"""
+ if self == PenState.PEN_IS_OUT_OF_RANGE:
+ return (
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_ERASING,
+ )
+
+ if self == PenState.PEN_IS_IN_RANGE:
+ return (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_IN_CONTACT,
+ )
+
+ if self == PenState.PEN_IS_IN_CONTACT:
+ return (
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ )
+
+ if self == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
+ return (
+ PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_ERASING,
+ )
+
+ if self == PenState.PEN_IS_ERASING:
+ return (
+ PenState.PEN_IS_ERASING,
+ PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
+ )
+
+ if self == PenState.PEN_IS_IN_RANGE_WITH_BUTTON:
+ return (
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ )
+
+ if self == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON:
+ return (
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ )
+
+ if self == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON:
+ return (
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ )
+
+ if self == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON:
+ return (
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ )
+
+ return tuple()
+
+ def historically_tolerated_transitions(self) -> Tuple["PenState", ...]:
"""Following the state machine in the URL above, with a couple of addition
for skipping the in-range state, due to historical reasons.
@@ -693,10 +777,14 @@ class BaseTest:
self.debug_reports(r, uhdev, events)
return events
- def validate_transitions(self, from_state, pen, evdev, events):
+ def validate_transitions(
+ self, from_state, pen, evdev, events, allow_intermediate_states
+ ):
# check that the final state is correct
pen.assert_expected_input_events(evdev)
+ state = from_state
+
# check that the transitions are valid
sync_events = []
while libevdev.InputEvent(libevdev.EV_SYN.SYN_REPORT) in events:
@@ -706,12 +794,12 @@ class BaseTest:
events = events[idx + 1 :]
# now check for a valid transition
- from_state = from_state.apply(sync_events)
+ state = state.apply(sync_events, not allow_intermediate_states)
if events:
- from_state = from_state.apply(sync_events)
+ state = state.apply(sync_events, not allow_intermediate_states)
- def _test_states(self, state_list, scribble):
+ def _test_states(self, state_list, scribble, allow_intermediate_states):
"""Internal method to test against a list of
transition between states.
state_list is a list of PenState objects
@@ -726,7 +814,9 @@ class BaseTest:
p = Pen(50, 60)
uhdev.move_to(p, PenState.PEN_IS_OUT_OF_RANGE)
events = self.post(uhdev, p)
- self.validate_transitions(cur_state, p, evdev, events)
+ self.validate_transitions(
+ cur_state, p, evdev, events, allow_intermediate_states
+ )
cur_state = p.current_state
@@ -735,14 +825,18 @@ class BaseTest:
p.x += 1
p.y -= 1
events = self.post(uhdev, p)
- self.validate_transitions(cur_state, p, evdev, events)
+ self.validate_transitions(
+ cur_state, p, evdev, events, allow_intermediate_states
+ )
assert len(events) >= 3 # X, Y, SYN
uhdev.move_to(p, state)
if scribble and state != PenState.PEN_IS_OUT_OF_RANGE:
p.x += 1
p.y -= 1
events = self.post(uhdev, p)
- self.validate_transitions(cur_state, p, evdev, events)
+ self.validate_transitions(
+ cur_state, p, evdev, events, allow_intermediate_states
+ )
cur_state = p.current_state
@pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
@@ -755,7 +849,7 @@ class BaseTest:
we don't have Invert nor Erase bits, so just move in/out-of-range or proximity.
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
"""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, allow_intermediate_states=False)
@pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
@pytest.mark.parametrize(
@@ -769,7 +863,7 @@ class BaseTest:
"""This is not adhering to the Windows Pen Implementation state machine
but we should expect the kernel to behave properly, mostly for historical
reasons."""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, allow_intermediate_states=True)
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Barrel Switch" not in uhdev.fields,
@@ -785,7 +879,7 @@ class BaseTest:
)
def test_valid_primary_button_pen_states(self, state_list, scribble):
"""Rework the transition state machine by adding the primary button."""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, allow_intermediate_states=False)
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Secondary Barrel Switch" not in uhdev.fields,
@@ -801,7 +895,7 @@ class BaseTest:
)
def test_valid_secondary_button_pen_states(self, state_list, scribble):
"""Rework the transition state machine by adding the secondary button."""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, allow_intermediate_states=False)
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Invert" not in uhdev.fields,
@@ -821,7 +915,7 @@ class BaseTest:
to erase.
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
"""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, allow_intermediate_states=False)
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Invert" not in uhdev.fields,
@@ -841,7 +935,7 @@ class BaseTest:
to erase.
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
"""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, allow_intermediate_states=True)
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Invert" not in uhdev.fields,
@@ -858,7 +952,7 @@ class BaseTest:
For example, a pen that has the eraser button might wobble between
touching and erasing if the tablet doesn't enforce the Windows
state machine."""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, allow_intermediate_states=True)
class GXTP_pen(PenDigitizer):
--
2.41.0
^ permalink raw reply related
* [PATCH v2 12/15] selftests/hid: tablets: add a secondary barrel switch test
From: Benjamin Tissoires @ 2023-12-06 10:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
Some tablets report 2 barrel switches. We better test those too.
Use the same transistions description from the primary button tests.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
tools/testing/selftests/hid/tests/test_tablet.py | 67 ++++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index 20a7a7fdcd9d..a82db66264c5 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -366,6 +366,56 @@ class PenState(Enum):
),
}
+ @staticmethod
+ def legal_transitions_with_secondary_button() -> Dict[str, Tuple["PenState", ...]]:
+ """We revisit the Windows Pen Implementation state machine:
+ we now have a secondary button.
+ Note: we don't looks for 2 buttons interactions.
+ """
+ return {
+ "hover-button": (PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,),
+ "hover-button -> out-of-range": (
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ ),
+ "in-range -> button-press": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ ),
+ "in-range -> button-press -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ "in-range -> touch -> button-press -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ ),
+ "in-range -> touch -> button-press -> release -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ "in-range -> button-press -> touch -> release -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ "in-range -> button-press -> touch -> button-release -> release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ }
+
@staticmethod
def tolerated_transitions() -> Dict[str, Tuple["PenState", ...]]:
"""This is not adhering to the Windows Pen Implementation state machine
@@ -444,6 +494,7 @@ class Pen(object):
self.width = 10
self.height = 10
self.barrelswitch = False
+ self.secondarybarrelswitch = False
self.invert = False
self.eraser = False
self.xtilt = 1
@@ -736,6 +787,22 @@ class BaseTest:
"""Rework the transition state machine by adding the primary button."""
self._test_states(state_list, scribble)
+ @pytest.mark.skip_if_uhdev(
+ lambda uhdev: "Secondary Barrel Switch" not in uhdev.fields,
+ "Device not compatible, missing Secondary Barrel Switch usage",
+ )
+ @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
+ @pytest.mark.parametrize(
+ "state_list",
+ [
+ pytest.param(v, id=k)
+ for k, v in PenState.legal_transitions_with_secondary_button().items()
+ ],
+ )
+ def test_valid_secondary_button_pen_states(self, state_list, scribble):
+ """Rework the transition state machine by adding the secondary button."""
+ self._test_states(state_list, scribble)
+
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Invert" not in uhdev.fields,
"Device not compatible, missing Invert usage",
--
2.41.0
^ permalink raw reply related
* [PATCH v2 11/15] selftests/hid: tablets: convert the primary button tests
From: Benjamin Tissoires @ 2023-12-06 10:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
We get more descriptive in what we are doing, and also get more
information of what is actually being tested. Instead of having a non
exhaustive button changes that are semi-randomly done, we can describe
all the states we want to test.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
tools/testing/selftests/hid/tests/test_tablet.py | 160 +++++++++--------------
1 file changed, 65 insertions(+), 95 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index a77534f23c75..20a7a7fdcd9d 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -317,6 +317,55 @@ class PenState(Enum):
),
}
+ @staticmethod
+ def legal_transitions_with_primary_button() -> Dict[str, Tuple["PenState", ...]]:
+ """We revisit the Windows Pen Implementation state machine:
+ we now have a primary button.
+ """
+ return {
+ "hover-button": (PenState.PEN_IS_IN_RANGE_WITH_BUTTON,),
+ "hover-button -> out-of-range": (
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ ),
+ "in-range -> button-press": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ ),
+ "in-range -> button-press -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ "in-range -> touch -> button-press -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ ),
+ "in-range -> touch -> button-press -> release -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ "in-range -> button-press -> touch -> release -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ "in-range -> button-press -> touch -> button-release -> release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ }
+
@staticmethod
def tolerated_transitions() -> Dict[str, Tuple["PenState", ...]]:
"""This is not adhering to the Windows Pen Implementation state machine
@@ -671,6 +720,22 @@ class BaseTest:
reasons."""
self._test_states(state_list, scribble)
+ @pytest.mark.skip_if_uhdev(
+ lambda uhdev: "Barrel Switch" not in uhdev.fields,
+ "Device not compatible, missing Barrel Switch usage",
+ )
+ @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
+ @pytest.mark.parametrize(
+ "state_list",
+ [
+ pytest.param(v, id=k)
+ for k, v in PenState.legal_transitions_with_primary_button().items()
+ ],
+ )
+ def test_valid_primary_button_pen_states(self, state_list, scribble):
+ """Rework the transition state machine by adding the primary button."""
+ self._test_states(state_list, scribble)
+
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Invert" not in uhdev.fields,
"Device not compatible, missing Invert usage",
@@ -728,101 +793,6 @@ class BaseTest:
state machine."""
self._test_states(state_list, scribble)
- @pytest.mark.skip_if_uhdev(
- lambda uhdev: "Barrel Switch" not in uhdev.fields,
- "Device not compatible, missing Barrel Switch usage",
- )
- def test_primary_button(self):
- """Primary button (stylus) pressed, reports as pressed even while hovering.
- Actual reporting from the device: hid=TIPSWITCH,BARRELSWITCH,INRANGE (code=TOUCH,STYLUS,PEN):
- { 0, 0, 1 } <- hover
- { 0, 1, 1 } <- primary button pressed
- { 0, 1, 1 } <- liftoff
- { 0, 0, 0 } <- leaves
- """
-
- uhdev = self.uhdev
- evdev = uhdev.get_evdev()
-
- p = Pen(50, 60)
- p.inrange = True
- events = self.post(uhdev, p)
- assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_PEN, 1) in events
- assert evdev.value[libevdev.EV_ABS.ABS_X] == 50
- assert evdev.value[libevdev.EV_ABS.ABS_Y] == 60
- assert not evdev.value[libevdev.EV_KEY.BTN_STYLUS]
-
- p.barrelswitch = True
- events = self.post(uhdev, p)
- assert libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS, 1) in events
-
- p.x += 1
- p.y -= 1
- events = self.post(uhdev, p)
- assert len(events) == 3 # X, Y, SYN
- assert libevdev.InputEvent(libevdev.EV_ABS.ABS_X, 51) in events
- assert libevdev.InputEvent(libevdev.EV_ABS.ABS_Y, 59) in events
-
- p.barrelswitch = False
- events = self.post(uhdev, p)
- assert libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS, 0) in events
-
- p.inrange = False
- events = self.post(uhdev, p)
- assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_PEN, 0) in events
-
- @pytest.mark.skip_if_uhdev(
- lambda uhdev: "Barrel Switch" not in uhdev.fields,
- "Device not compatible, missing Barrel Switch usage",
- )
- def test_contact_primary_button(self):
- """Primary button (stylus) pressed, reports as pressed even while hovering.
- Actual reporting from the device: hid=TIPSWITCH,BARRELSWITCH,INRANGE (code=TOUCH,STYLUS,PEN):
- { 0, 0, 1 } <- hover
- { 0, 1, 1 } <- primary button pressed
- { 1, 1, 1 } <- touch-down
- { 1, 1, 1 } <- still touch, scribble on the screen
- { 0, 1, 1 } <- liftoff
- { 0, 0, 0 } <- leaves
- """
-
- uhdev = self.uhdev
- evdev = uhdev.get_evdev()
-
- p = Pen(50, 60)
- p.inrange = True
- events = self.post(uhdev, p)
- assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_PEN, 1) in events
- assert evdev.value[libevdev.EV_ABS.ABS_X] == 50
- assert evdev.value[libevdev.EV_ABS.ABS_Y] == 60
- assert not evdev.value[libevdev.EV_KEY.BTN_STYLUS]
-
- p.barrelswitch = True
- events = self.post(uhdev, p)
- assert libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS, 1) in events
-
- p.tipswitch = True
- events = self.post(uhdev, p)
- assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOUCH, 1) in events
- assert evdev.value[libevdev.EV_KEY.BTN_STYLUS]
-
- p.x += 1
- p.y -= 1
- events = self.post(uhdev, p)
- assert len(events) == 3 # X, Y, SYN
- assert libevdev.InputEvent(libevdev.EV_ABS.ABS_X, 51) in events
- assert libevdev.InputEvent(libevdev.EV_ABS.ABS_Y, 59) in events
-
- p.tipswitch = False
- events = self.post(uhdev, p)
- assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOUCH, 0) in events
-
- p.barrelswitch = False
- p.inrange = False
- events = self.post(uhdev, p)
- assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_PEN, 0) in events
- assert libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS, 0) in events
-
class GXTP_pen(PenDigitizer):
def event(self, pen):
--
2.41.0
^ permalink raw reply related
* [PATCH v2 10/15] selftests/hid: tablets: add variants of states with buttons
From: Benjamin Tissoires @ 2023-12-06 10:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
Turns out that there are transitions that are unlikely to happen:
for example, having both the tip switch and a button being changed
at the same time (in the same report) would require either a very talented
and precise user or a very bad hardware with a very low sampling rate.
So instead of manually building the button test by hand and forgetting
about some cases, let's reuse the state machine and transitions we have.
This patch only adds the states and the valid transitions. The actual
tests will be replaced later.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
changes in v2:
- adapt to use dedicated Enum BtnPressed
---
tools/testing/selftests/hid/tests/test_tablet.py | 173 +++++++++++++++++++++--
1 file changed, 160 insertions(+), 13 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index cec65294c9ec..a77534f23c75 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -30,25 +30,72 @@ class ToolType(Enum):
RUBBER = libevdev.EV_KEY.BTN_TOOL_RUBBER
+class BtnPressed(Enum):
+ """Represents whether a button is pressed on the stylus"""
+
+ PRIMARY_PRESSED = libevdev.EV_KEY.BTN_STYLUS
+ SECONDARY_PRESSED = libevdev.EV_KEY.BTN_STYLUS2
+
+
class PenState(Enum):
"""Pen states according to Microsoft reference:
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
- """
- PEN_IS_OUT_OF_RANGE = BtnTouch.UP, None
- PEN_IS_IN_RANGE = BtnTouch.UP, ToolType.PEN
- PEN_IS_IN_CONTACT = BtnTouch.DOWN, ToolType.PEN
- PEN_IS_IN_RANGE_WITH_ERASING_INTENT = BtnTouch.UP, ToolType.RUBBER
- PEN_IS_ERASING = BtnTouch.DOWN, ToolType.RUBBER
+ We extend it with the various buttons when we need to check them.
+ """
- def __init__(self, touch: BtnTouch, tool: Optional[ToolType]):
+ PEN_IS_OUT_OF_RANGE = BtnTouch.UP, None, None
+ PEN_IS_IN_RANGE = BtnTouch.UP, ToolType.PEN, None
+ PEN_IS_IN_RANGE_WITH_BUTTON = BtnTouch.UP, ToolType.PEN, BtnPressed.PRIMARY_PRESSED
+ PEN_IS_IN_RANGE_WITH_SECOND_BUTTON = (
+ BtnTouch.UP,
+ ToolType.PEN,
+ BtnPressed.SECONDARY_PRESSED,
+ )
+ PEN_IS_IN_CONTACT = BtnTouch.DOWN, ToolType.PEN, None
+ PEN_IS_IN_CONTACT_WITH_BUTTON = (
+ BtnTouch.DOWN,
+ ToolType.PEN,
+ BtnPressed.PRIMARY_PRESSED,
+ )
+ PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON = (
+ BtnTouch.DOWN,
+ ToolType.PEN,
+ BtnPressed.SECONDARY_PRESSED,
+ )
+ PEN_IS_IN_RANGE_WITH_ERASING_INTENT = BtnTouch.UP, ToolType.RUBBER, None
+ PEN_IS_IN_RANGE_WITH_ERASING_INTENT_WITH_BUTTON = (
+ BtnTouch.UP,
+ ToolType.RUBBER,
+ BtnPressed.PRIMARY_PRESSED,
+ )
+ PEN_IS_IN_RANGE_WITH_ERASING_INTENT_WITH_SECOND_BUTTON = (
+ BtnTouch.UP,
+ ToolType.RUBBER,
+ BtnPressed.SECONDARY_PRESSED,
+ )
+ PEN_IS_ERASING = BtnTouch.DOWN, ToolType.RUBBER, None
+ PEN_IS_ERASING_WITH_BUTTON = (
+ BtnTouch.DOWN,
+ ToolType.RUBBER,
+ BtnPressed.PRIMARY_PRESSED,
+ )
+ PEN_IS_ERASING_WITH_SECOND_BUTTON = (
+ BtnTouch.DOWN,
+ ToolType.RUBBER,
+ BtnPressed.SECONDARY_PRESSED,
+ )
+
+ def __init__(self, touch: BtnTouch, tool: Optional[ToolType], button: Optional[BtnPressed]):
self.touch = touch
self.tool = tool
+ self.button = button
@classmethod
def from_evdev(cls, evdev) -> "PenState":
touch = BtnTouch(evdev.value[libevdev.EV_KEY.BTN_TOUCH])
tool = None
+ button = None
if (
evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER]
and not evdev.value[libevdev.EV_KEY.BTN_TOOL_PEN]
@@ -65,7 +112,17 @@ class PenState(Enum):
):
raise ValueError("2 tools are not allowed")
- return cls((touch, tool))
+ # we take only the highest button in account
+ for b in [libevdev.EV_KEY.BTN_STYLUS, libevdev.EV_KEY.BTN_STYLUS2]:
+ if bool(evdev.value[b]):
+ button = b
+
+ # the kernel tends to insert an EV_SYN once removing the tool, so
+ # the button will be released after
+ if tool is None:
+ button = None
+
+ return cls((touch, tool, button))
def apply(self, events) -> "PenState":
if libevdev.EV_SYN.SYN_REPORT in events:
@@ -74,6 +131,8 @@ class PenState(Enum):
touch_found = False
tool = self.tool
tool_found = False
+ button = self.button
+ button_found = False
for ev in events:
if ev == libevdev.InputEvent(libevdev.EV_KEY.BTN_TOUCH):
@@ -88,12 +147,22 @@ class PenState(Enum):
if tool_found:
raise ValueError(f"duplicated BTN_TOOL_* in {events}")
tool_found = True
- if ev.value:
- tool = ToolType(ev.code)
- else:
- tool = None
+ tool = ToolType(ev.code) if ev.value else None
+ elif ev in (
+ libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS),
+ libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS2),
+ ):
+ if button_found:
+ raise ValueError(f"duplicated BTN_STYLUS* in {events}")
+ button_found = True
+ button = ev.code if ev.value else None
- new_state = PenState((touch, tool))
+ # the kernel tends to insert an EV_SYN once removing the tool, so
+ # the button will be released after
+ if tool is None:
+ button = None
+
+ new_state = PenState((touch, tool, button))
assert (
new_state in self.valid_transitions()
), f"moving from {self} to {new_state} is forbidden"
@@ -109,14 +178,20 @@ class PenState(Enum):
return (
PenState.PEN_IS_OUT_OF_RANGE,
PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
PenState.PEN_IS_ERASING,
)
if self == PenState.PEN_IS_IN_RANGE:
return (
PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
PenState.PEN_IS_OUT_OF_RANGE,
PenState.PEN_IS_IN_CONTACT,
)
@@ -124,6 +199,8 @@ class PenState(Enum):
if self == PenState.PEN_IS_IN_CONTACT:
return (
PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
PenState.PEN_IS_IN_RANGE,
PenState.PEN_IS_OUT_OF_RANGE,
)
@@ -142,6 +219,38 @@ class PenState(Enum):
PenState.PEN_IS_OUT_OF_RANGE,
)
+ if self == PenState.PEN_IS_IN_RANGE_WITH_BUTTON:
+ return (
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ )
+
+ if self == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON:
+ return (
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ )
+
+ if self == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON:
+ return (
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ )
+
+ if self == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON:
+ return (
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ )
+
return tuple()
@staticmethod
@@ -376,26 +485,64 @@ class PenDigitizer(base.UHIDTestDevice):
pen.xtilt = 0
pen.ytilt = 0
pen.twist = 0
+ pen.barrelswitch = False
+ pen.secondarybarrelswitch = False
elif state == PenState.PEN_IS_IN_RANGE:
pen.tipswitch = False
pen.inrange = True
pen.invert = False
pen.eraser = False
+ pen.barrelswitch = False
+ pen.secondarybarrelswitch = False
elif state == PenState.PEN_IS_IN_CONTACT:
pen.tipswitch = True
pen.inrange = True
pen.invert = False
pen.eraser = False
+ pen.barrelswitch = False
+ pen.secondarybarrelswitch = False
+ elif state == PenState.PEN_IS_IN_RANGE_WITH_BUTTON:
+ pen.tipswitch = False
+ pen.inrange = True
+ pen.invert = False
+ pen.eraser = False
+ pen.barrelswitch = True
+ pen.secondarybarrelswitch = False
+ elif state == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON:
+ pen.tipswitch = True
+ pen.inrange = True
+ pen.invert = False
+ pen.eraser = False
+ pen.barrelswitch = True
+ pen.secondarybarrelswitch = False
+ elif state == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON:
+ pen.tipswitch = False
+ pen.inrange = True
+ pen.invert = False
+ pen.eraser = False
+ pen.barrelswitch = False
+ pen.secondarybarrelswitch = True
+ elif state == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON:
+ pen.tipswitch = True
+ pen.inrange = True
+ pen.invert = False
+ pen.eraser = False
+ pen.barrelswitch = False
+ pen.secondarybarrelswitch = True
elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
pen.tipswitch = False
pen.inrange = True
pen.invert = True
pen.eraser = False
+ pen.barrelswitch = False
+ pen.secondarybarrelswitch = False
elif state == PenState.PEN_IS_ERASING:
pen.tipswitch = False
pen.inrange = True
pen.invert = False
pen.eraser = True
+ pen.barrelswitch = False
+ pen.secondarybarrelswitch = False
pen.current_state = state
--
2.41.0
^ permalink raw reply related
* [PATCH v2 09/15] selftests/hid: tablets: define the elements of PenState
From: Benjamin Tissoires @ 2023-12-06 10:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
This introduces a little bit more readability by not using the raw values
but a dedicated Enum
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
new in v2
---
tools/testing/selftests/hid/tests/test_tablet.py | 36 ++++++++++++++++--------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index 0ddf82695ed9..cec65294c9ec 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -13,40 +13,52 @@ from hidtools.util import BusType
import libevdev
import logging
import pytest
-from typing import Dict, Tuple
+from typing import Dict, Optional, Tuple
logger = logging.getLogger("hidtools.test.tablet")
+class BtnTouch(Enum):
+ """Represents whether the BTN_TOUCH event is set to True or False"""
+
+ DOWN = True
+ UP = False
+
+
+class ToolType(Enum):
+ PEN = libevdev.EV_KEY.BTN_TOOL_PEN
+ RUBBER = libevdev.EV_KEY.BTN_TOOL_RUBBER
+
+
class PenState(Enum):
"""Pen states according to Microsoft reference:
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
"""
- PEN_IS_OUT_OF_RANGE = (False, None)
- PEN_IS_IN_RANGE = (False, libevdev.EV_KEY.BTN_TOOL_PEN)
- PEN_IS_IN_CONTACT = (True, libevdev.EV_KEY.BTN_TOOL_PEN)
- PEN_IS_IN_RANGE_WITH_ERASING_INTENT = (False, libevdev.EV_KEY.BTN_TOOL_RUBBER)
- PEN_IS_ERASING = (True, libevdev.EV_KEY.BTN_TOOL_RUBBER)
+ PEN_IS_OUT_OF_RANGE = BtnTouch.UP, None
+ PEN_IS_IN_RANGE = BtnTouch.UP, ToolType.PEN
+ PEN_IS_IN_CONTACT = BtnTouch.DOWN, ToolType.PEN
+ PEN_IS_IN_RANGE_WITH_ERASING_INTENT = BtnTouch.UP, ToolType.RUBBER
+ PEN_IS_ERASING = BtnTouch.DOWN, ToolType.RUBBER
- def __init__(self, touch, tool):
+ def __init__(self, touch: BtnTouch, tool: Optional[ToolType]):
self.touch = touch
self.tool = tool
@classmethod
def from_evdev(cls, evdev) -> "PenState":
- touch = bool(evdev.value[libevdev.EV_KEY.BTN_TOUCH])
+ touch = BtnTouch(evdev.value[libevdev.EV_KEY.BTN_TOUCH])
tool = None
if (
evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER]
and not evdev.value[libevdev.EV_KEY.BTN_TOOL_PEN]
):
- tool = libevdev.EV_KEY.BTN_TOOL_RUBBER
+ tool = ToolType(libevdev.EV_KEY.BTN_TOOL_RUBBER)
elif (
evdev.value[libevdev.EV_KEY.BTN_TOOL_PEN]
and not evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER]
):
- tool = libevdev.EV_KEY.BTN_TOOL_PEN
+ tool = ToolType(libevdev.EV_KEY.BTN_TOOL_PEN)
elif (
evdev.value[libevdev.EV_KEY.BTN_TOOL_PEN]
or evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER]
@@ -68,7 +80,7 @@ class PenState(Enum):
if touch_found:
raise ValueError(f"duplicated BTN_TOUCH in {events}")
touch_found = True
- touch = bool(ev.value)
+ touch = BtnTouch(ev.value)
elif ev in (
libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_PEN),
libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_RUBBER),
@@ -77,7 +89,7 @@ class PenState(Enum):
raise ValueError(f"duplicated BTN_TOOL_* in {events}")
tool_found = True
if ev.value:
- tool = ev.code
+ tool = ToolType(ev.code)
else:
tool = None
--
2.41.0
^ permalink raw reply related
* [PATCH v2 08/15] selftests/hid: tablets: set initial data for tilt/twist
From: Benjamin Tissoires @ 2023-12-06 10:45 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
Avoids getting a null event when these usages are set
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
tools/testing/selftests/hid/tests/test_tablet.py | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index cb3955bf0ec5..0ddf82695ed9 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -276,9 +276,9 @@ class Pen(object):
self.barrelswitch = False
self.invert = False
self.eraser = False
- self.x_tilt = 0
- self.y_tilt = 0
- self.twist = 0
+ self.xtilt = 1
+ self.ytilt = 1
+ self.twist = 1
self._old_values = None
self.current_state = None
@@ -292,8 +292,8 @@ class Pen(object):
"width",
"height",
"twist",
- "x_tilt",
- "y_tilt",
+ "xtilt",
+ "ytilt",
]:
setattr(self, i, getattr(self._old_values, i))
@@ -361,8 +361,8 @@ class PenDigitizer(base.UHIDTestDevice):
pen.height = 0
pen.invert = False
pen.eraser = False
- pen.x_tilt = 0
- pen.y_tilt = 0
+ pen.xtilt = 0
+ pen.ytilt = 0
pen.twist = 0
elif state == PenState.PEN_IS_IN_RANGE:
pen.tipswitch = False
--
2.41.0
^ permalink raw reply related
* [PATCH v2 07/15] selftests/hid: tablets: do not set invert when the eraser is used
From: Benjamin Tissoires @ 2023-12-06 10:45 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
Turns out that the chart from Microsoft is not exactly what I got here:
when the rubber is used, and is touching the surface, invert can (should)
be set to 0...
[0] https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
tools/testing/selftests/hid/tests/test_tablet.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index 27260dc02cc4..cb3955bf0ec5 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -382,7 +382,7 @@ class PenDigitizer(base.UHIDTestDevice):
elif state == PenState.PEN_IS_ERASING:
pen.tipswitch = False
pen.inrange = True
- pen.invert = True
+ pen.invert = False
pen.eraser = True
pen.current_state = state
--
2.41.0
^ permalink raw reply related
* [PATCH v2 06/15] selftests/hid: tablets: move move_to function to PenDigitizer
From: Benjamin Tissoires @ 2023-12-06 10:45 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
We can easily subclass PenDigitizer for introducing firmware bugs when
subclassing Pen is harder.
Move move_to from Pen to PenDigitizer so we get that ability
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
tools/testing/selftests/hid/tests/test_tablet.py | 97 ++++++++++++------------
1 file changed, 50 insertions(+), 47 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index ddf28c245046..27260dc02cc4 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -282,7 +282,7 @@ class Pen(object):
self._old_values = None
self.current_state = None
- def _restore(self):
+ def restore(self):
if self._old_values is not None:
for i in [
"x",
@@ -297,50 +297,8 @@ class Pen(object):
]:
setattr(self, i, getattr(self._old_values, i))
- def move_to(self, state):
- # fill in the previous values
- if self.current_state == PenState.PEN_IS_OUT_OF_RANGE:
- self._restore()
-
- print(f"\n *** pen is moving to {state} ***")
-
- if state == PenState.PEN_IS_OUT_OF_RANGE:
- self._old_values = copy.copy(self)
- self.x = 0
- self.y = 0
- self.tipswitch = False
- self.tippressure = 0
- self.azimuth = 0
- self.inrange = False
- self.width = 0
- self.height = 0
- self.invert = False
- self.eraser = False
- self.x_tilt = 0
- self.y_tilt = 0
- self.twist = 0
- elif state == PenState.PEN_IS_IN_RANGE:
- self.tipswitch = False
- self.inrange = True
- self.invert = False
- self.eraser = False
- elif state == PenState.PEN_IS_IN_CONTACT:
- self.tipswitch = True
- self.inrange = True
- self.invert = False
- self.eraser = False
- elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
- self.tipswitch = False
- self.inrange = True
- self.invert = True
- self.eraser = False
- elif state == PenState.PEN_IS_ERASING:
- self.tipswitch = False
- self.inrange = True
- self.invert = True
- self.eraser = True
-
- self.current_state = state
+ def backup(self):
+ self._old_values = copy.copy(self)
def __assert_axis(self, evdev, axis, value):
if (
@@ -384,6 +342,51 @@ class PenDigitizer(base.UHIDTestDevice):
continue
self.fields = [f.usage_name for f in r]
+ def move_to(self, pen, state):
+ # fill in the previous values
+ if pen.current_state == PenState.PEN_IS_OUT_OF_RANGE:
+ pen.restore()
+
+ print(f"\n *** pen is moving to {state} ***")
+
+ if state == PenState.PEN_IS_OUT_OF_RANGE:
+ pen.backup()
+ pen.x = 0
+ pen.y = 0
+ pen.tipswitch = False
+ pen.tippressure = 0
+ pen.azimuth = 0
+ pen.inrange = False
+ pen.width = 0
+ pen.height = 0
+ pen.invert = False
+ pen.eraser = False
+ pen.x_tilt = 0
+ pen.y_tilt = 0
+ pen.twist = 0
+ elif state == PenState.PEN_IS_IN_RANGE:
+ pen.tipswitch = False
+ pen.inrange = True
+ pen.invert = False
+ pen.eraser = False
+ elif state == PenState.PEN_IS_IN_CONTACT:
+ pen.tipswitch = True
+ pen.inrange = True
+ pen.invert = False
+ pen.eraser = False
+ elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
+ pen.tipswitch = False
+ pen.inrange = True
+ pen.invert = True
+ pen.eraser = False
+ elif state == PenState.PEN_IS_ERASING:
+ pen.tipswitch = False
+ pen.inrange = True
+ pen.invert = True
+ pen.eraser = True
+
+ pen.current_state = state
+
def event(self, pen):
rs = []
r = self.create_report(application=self.cur_application, data=pen)
@@ -462,7 +465,7 @@ class BaseTest:
cur_state = PenState.PEN_IS_OUT_OF_RANGE
p = Pen(50, 60)
- p.move_to(PenState.PEN_IS_OUT_OF_RANGE)
+ uhdev.move_to(p, PenState.PEN_IS_OUT_OF_RANGE)
events = self.post(uhdev, p)
self.validate_transitions(cur_state, p, evdev, events)
@@ -475,7 +478,7 @@ class BaseTest:
events = self.post(uhdev, p)
self.validate_transitions(cur_state, p, evdev, events)
assert len(events) >= 3 # X, Y, SYN
- p.move_to(state)
+ uhdev.move_to(p, state)
if scribble and state != PenState.PEN_IS_OUT_OF_RANGE:
p.x += 1
p.y -= 1
--
2.41.0
^ permalink raw reply related
* [PATCH v2 05/15] selftests/hid: tablets: move the transitions to PenState
From: Benjamin Tissoires @ 2023-12-06 10:45 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
Those transitions have nothing to do with `Pen`, so migrate them to
`PenState`.
The hidden agenda is to remove `Pen` and integrate it into `PenDigitizer`
so that we can tweak the events in each state to emulate firmware bugs.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
tools/testing/selftests/hid/tests/test_tablet.py | 215 ++++++++++++-----------
1 file changed, 109 insertions(+), 106 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index cd9c1269afa6..ddf28c245046 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -132,104 +132,8 @@ class PenState(Enum):
return tuple()
-
-class Pen(object):
- def __init__(self, x, y):
- self.x = x
- self.y = y
- self.tipswitch = False
- self.tippressure = 15
- self.azimuth = 0
- self.inrange = False
- self.width = 10
- self.height = 10
- self.barrelswitch = False
- self.invert = False
- self.eraser = False
- self.x_tilt = 0
- self.y_tilt = 0
- self.twist = 0
- self._old_values = None
- self.current_state = None
-
- def _restore(self):
- if self._old_values is not None:
- for i in [
- "x",
- "y",
- "tippressure",
- "azimuth",
- "width",
- "height",
- "twist",
- "x_tilt",
- "y_tilt",
- ]:
- setattr(self, i, getattr(self._old_values, i))
-
- def move_to(self, state):
- # fill in the previous values
- if self.current_state == PenState.PEN_IS_OUT_OF_RANGE:
- self._restore()
-
- print(f"\n *** pen is moving to {state} ***")
-
- if state == PenState.PEN_IS_OUT_OF_RANGE:
- self._old_values = copy.copy(self)
- self.x = 0
- self.y = 0
- self.tipswitch = False
- self.tippressure = 0
- self.azimuth = 0
- self.inrange = False
- self.width = 0
- self.height = 0
- self.invert = False
- self.eraser = False
- self.x_tilt = 0
- self.y_tilt = 0
- self.twist = 0
- elif state == PenState.PEN_IS_IN_RANGE:
- self.tipswitch = False
- self.inrange = True
- self.invert = False
- self.eraser = False
- elif state == PenState.PEN_IS_IN_CONTACT:
- self.tipswitch = True
- self.inrange = True
- self.invert = False
- self.eraser = False
- elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
- self.tipswitch = False
- self.inrange = True
- self.invert = True
- self.eraser = False
- elif state == PenState.PEN_IS_ERASING:
- self.tipswitch = False
- self.inrange = True
- self.invert = True
- self.eraser = True
-
- self.current_state = state
-
- def __assert_axis(self, evdev, axis, value):
- if (
- axis == libevdev.EV_KEY.BTN_TOOL_RUBBER
- and evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER] is None
- ):
- return
-
- assert (
- evdev.value[axis] == value
- ), f"assert evdev.value[{axis}] ({evdev.value[axis]}) != {value}"
-
- def assert_expected_input_events(self, evdev):
- assert evdev.value[libevdev.EV_ABS.ABS_X] == self.x
- assert evdev.value[libevdev.EV_ABS.ABS_Y] == self.y
- assert self.current_state == PenState.from_evdev(evdev)
-
@staticmethod
- def legal_transitions() -> Dict[str, Tuple[PenState, ...]]:
+ def legal_transitions() -> Dict[str, Tuple["PenState", ...]]:
"""This is the first half of the Windows Pen Implementation state machine:
we don't have Invert nor Erase bits, so just move in/out-of-range or proximity.
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
@@ -255,7 +159,7 @@ class Pen(object):
}
@staticmethod
- def legal_transitions_with_invert() -> Dict[str, Tuple[PenState, ...]]:
+ def legal_transitions_with_invert() -> Dict[str, Tuple["PenState", ...]]:
"""This is the second half of the Windows Pen Implementation state machine:
we now have Invert and Erase bits, so move in/out or proximity with the intend
to erase.
@@ -293,7 +197,7 @@ class Pen(object):
}
@staticmethod
- def tolerated_transitions() -> Dict[str, Tuple[PenState, ...]]:
+ def tolerated_transitions() -> Dict[str, Tuple["PenState", ...]]:
"""This is not adhering to the Windows Pen Implementation state machine
but we should expect the kernel to behave properly, mostly for historical
reasons."""
@@ -306,7 +210,7 @@ class Pen(object):
}
@staticmethod
- def tolerated_transitions_with_invert() -> Dict[str, Tuple[PenState, ...]]:
+ def tolerated_transitions_with_invert() -> Dict[str, Tuple["PenState", ...]]:
"""This is the second half of the Windows Pen Implementation state machine:
we now have Invert and Erase bits, so move in/out or proximity with the intend
to erase.
@@ -321,7 +225,7 @@ class Pen(object):
}
@staticmethod
- def broken_transitions() -> Dict[str, Tuple[PenState, ...]]:
+ def broken_transitions() -> Dict[str, Tuple["PenState", ...]]:
"""Those tests are definitely not part of the Windows specification.
However, a half broken device might export those transitions.
For example, a pen that has the eraser button might wobble between
@@ -359,6 +263,102 @@ class Pen(object):
}
+class Pen(object):
+ def __init__(self, x, y):
+ self.x = x
+ self.y = y
+ self.tipswitch = False
+ self.tippressure = 15
+ self.azimuth = 0
+ self.inrange = False
+ self.width = 10
+ self.height = 10
+ self.barrelswitch = False
+ self.invert = False
+ self.eraser = False
+ self.x_tilt = 0
+ self.y_tilt = 0
+ self.twist = 0
+ self._old_values = None
+ self.current_state = None
+
+ def _restore(self):
+ if self._old_values is not None:
+ for i in [
+ "x",
+ "y",
+ "tippressure",
+ "azimuth",
+ "width",
+ "height",
+ "twist",
+ "x_tilt",
+ "y_tilt",
+ ]:
+ setattr(self, i, getattr(self._old_values, i))
+
+ def move_to(self, state):
+ # fill in the previous values
+ if self.current_state == PenState.PEN_IS_OUT_OF_RANGE:
+ self._restore()
+
+ print(f"\n *** pen is moving to {state} ***")
+
+ if state == PenState.PEN_IS_OUT_OF_RANGE:
+ self._old_values = copy.copy(self)
+ self.x = 0
+ self.y = 0
+ self.tipswitch = False
+ self.tippressure = 0
+ self.azimuth = 0
+ self.inrange = False
+ self.width = 0
+ self.height = 0
+ self.invert = False
+ self.eraser = False
+ self.x_tilt = 0
+ self.y_tilt = 0
+ self.twist = 0
+ elif state == PenState.PEN_IS_IN_RANGE:
+ self.tipswitch = False
+ self.inrange = True
+ self.invert = False
+ self.eraser = False
+ elif state == PenState.PEN_IS_IN_CONTACT:
+ self.tipswitch = True
+ self.inrange = True
+ self.invert = False
+ self.eraser = False
+ elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
+ self.tipswitch = False
+ self.inrange = True
+ self.invert = True
+ self.eraser = False
+ elif state == PenState.PEN_IS_ERASING:
+ self.tipswitch = False
+ self.inrange = True
+ self.invert = True
+ self.eraser = True
+
+ self.current_state = state
+
+ def __assert_axis(self, evdev, axis, value):
+ if (
+ axis == libevdev.EV_KEY.BTN_TOOL_RUBBER
+ and evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER] is None
+ ):
+ return
+
+ assert (
+ evdev.value[axis] == value
+ ), f"assert evdev.value[{axis}] ({evdev.value[axis]}) != {value}"
+
+ def assert_expected_input_events(self, evdev):
+ assert evdev.value[libevdev.EV_ABS.ABS_X] == self.x
+ assert evdev.value[libevdev.EV_ABS.ABS_Y] == self.y
+ assert self.current_state == PenState.from_evdev(evdev)
+
+
class PenDigitizer(base.UHIDTestDevice):
def __init__(
self,
@@ -486,7 +486,7 @@ class BaseTest:
@pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
@pytest.mark.parametrize(
"state_list",
- [pytest.param(v, id=k) for k, v in Pen.legal_transitions().items()],
+ [pytest.param(v, id=k) for k, v in PenState.legal_transitions().items()],
)
def test_valid_pen_states(self, state_list, scribble):
"""This is the first half of the Windows Pen Implementation state machine:
@@ -498,7 +498,10 @@ class BaseTest:
@pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
@pytest.mark.parametrize(
"state_list",
- [pytest.param(v, id=k) for k, v in Pen.tolerated_transitions().items()],
+ [
+ pytest.param(v, id=k)
+ for k, v in PenState.tolerated_transitions().items()
+ ],
)
def test_tolerated_pen_states(self, state_list, scribble):
"""This is not adhering to the Windows Pen Implementation state machine
@@ -515,7 +518,7 @@ class BaseTest:
"state_list",
[
pytest.param(v, id=k)
- for k, v in Pen.legal_transitions_with_invert().items()
+ for k, v in PenState.legal_transitions_with_invert().items()
],
)
def test_valid_invert_pen_states(self, state_list, scribble):
@@ -535,7 +538,7 @@ class BaseTest:
"state_list",
[
pytest.param(v, id=k)
- for k, v in Pen.tolerated_transitions_with_invert().items()
+ for k, v in PenState.tolerated_transitions_with_invert().items()
],
)
def test_tolerated_invert_pen_states(self, state_list, scribble):
@@ -553,7 +556,7 @@ class BaseTest:
@pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
@pytest.mark.parametrize(
"state_list",
- [pytest.param(v, id=k) for k, v in Pen.broken_transitions().items()],
+ [pytest.param(v, id=k) for k, v in PenState.broken_transitions().items()],
)
def test_tolerated_broken_pen_states(self, state_list, scribble):
"""Those tests are definitely not part of the Windows specification.
--
2.41.0
^ permalink raw reply related
* [PATCH v2 04/15] selftests/hid: tablets: remove unused class
From: Benjamin Tissoires @ 2023-12-06 10:45 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231206-wip-selftests-v2-0-c0350c2f5986@kernel.org>
Looks like this is a leftover
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
tools/testing/selftests/hid/tests/test_tablet.py | 4 ----
1 file changed, 4 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index 303ffff9ee8d..cd9c1269afa6 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -133,10 +133,6 @@ class PenState(Enum):
return tuple()
-class Data(object):
- pass
-
-
class Pen(object):
def __init__(self, x, y):
self.x = x
--
2.41.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox