[PATCH v2 0/3] Imagis touch keys and FIELD_GET cleanup

[PATCH v2 0/3] Imagis touch keys and FIELD_GET cleanup

[Duje Mihanović]

Tiny series to clean up the field extraction and add touch key support.
Depends on the IST3032C series (at least the touch key patches, the
other one could be split up):
https://lore.kernel.org/20240120191940.3631-1-karelb@gimli.ms.mff.cuni.cz

Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
Changes in v2:
- Fix compile error
- Add FIELD_GET patch
- Allow specifying custom keycodes
- Link to v1: https://lore.kernel.org/20231112194124.24916-1-duje.mihanovic@skole.hr

---
Duje Mihanović (3):
      input: touchscreen: imagis: use FIELD_GET where applicable
      dt-bindings: input: imagis: Document touch keys
      input: touchscreen: imagis: Add touch key support

 .../input/touchscreen/imagis,ist3038c.yaml         | 11 ++++++
 drivers/input/touchscreen/imagis.c                 | 42 +++++++++++++++-------
 2 files changed, 41 insertions(+), 12 deletions(-)
---
base-commit: 553f2a83ae97246be401ca9b313063f5fa879f12
change-id: 20240120-b4-imagis-keys-a0a9f2b31740

Best regards,
-- 
Duje Mihanović <duje.mihanovic@skole.hr>

[PATCH v2 1/3] input: touchscreen: imagis: use FIELD_GET where applicable

[Duje Mihanović]

Instead of manually extracting certain bits from registers with binary
ANDs and shifts, the FIELD_GET macro can be used. With this in mind, the
*_SHIFT macros can be dropped.

Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
 drivers/input/touchscreen/imagis.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
index e1fafa561ee3..4eae98771bd2 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
@@ -29,12 +30,9 @@
 #define IST3038C_I2C_RETRY_COUNT	3
 #define IST3038C_MAX_FINGER_NUM		10
 #define IST3038C_X_MASK			GENMASK(23, 12)
-#define IST3038C_X_SHIFT		12
 #define IST3038C_Y_MASK			GENMASK(11, 0)
 #define IST3038C_AREA_MASK		GENMASK(27, 24)
-#define IST3038C_AREA_SHIFT		24
 #define IST3038C_FINGER_COUNT_MASK	GENMASK(15, 12)
-#define IST3038C_FINGER_COUNT_SHIFT	12
 #define IST3038C_FINGER_STATUS_MASK	GENMASK(9, 0)
 
 struct imagis_properties {
@@ -106,8 +104,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
 		goto out;
 	}
 
-	finger_count = (intr_message & IST3038C_FINGER_COUNT_MASK) >>
-				IST3038C_FINGER_COUNT_SHIFT;
+	finger_count = FIELD_GET(IST3038C_FINGER_COUNT_MASK, intr_message);
 	if (finger_count > IST3038C_MAX_FINGER_NUM) {
 		dev_err(&ts->client->dev,
 			"finger count %d is more than maximum supported\n",
@@ -115,7 +112,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
 		goto out;
 	}
 
-	finger_pressed = intr_message & IST3038C_FINGER_STATUS_MASK;
+	finger_pressed = FIELD_GET(IST3038C_FINGER_STATUS_MASK, intr_message);
 
 	for (i = 0; i < finger_count; i++) {
 		if (ts->tdata->protocol_b)
@@ -136,12 +133,10 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
 		input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER,
 					   finger_pressed & BIT(i));
 		touchscreen_report_pos(ts->input_dev, &ts->prop,
-				       (finger_status & IST3038C_X_MASK) >>
-						IST3038C_X_SHIFT,
-				       finger_status & IST3038C_Y_MASK, 1);
+				       FIELD_GET(IST3038C_X_MASK, finger_status),
+				       FIELD_GET(IST3038C_Y_MASK, finger_status), 1);
 		input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR,
-				 (finger_status & IST3038C_AREA_MASK) >>
-					IST3038C_AREA_SHIFT);
+				       FIELD_GET(IST3038C_AREA_MASK, finger_status));
 	}
 
 	input_mt_sync_frame(ts->input_dev);

-- 
2.43.0

[PATCH v2 2/3] dt-bindings: input: imagis: Document touch keys

[Duje Mihanović]

IST3032C (and possibly some other models) has touch keys. Document this.

Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
 .../bindings/input/touchscreen/imagis,ist3038c.yaml           | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
index 2af71cbcc97d..960e5436642f 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
@@ -42,6 +42,17 @@ properties:
   touchscreen-inverted-y: true
   touchscreen-swapped-x-y: true
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: imagis,ist3032c
+then:
+  properties:
+    linux,keycodes:
+      description: Keycodes for the touch keys
+      maxItems: 2
+
 additionalProperties: false
 
 required:

-- 
2.43.0

[PATCH v2 3/3] input: touchscreen: imagis: Add touch key support

[Duje Mihanović]

IST3032C (and possibly some other models) has touch keys. Add support
for them to the imagis driver.

Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
 drivers/input/touchscreen/imagis.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
index 4eae98771bd2..6dcb82313c32 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -34,6 +34,7 @@
 #define IST3038C_AREA_MASK		GENMASK(27, 24)
 #define IST3038C_FINGER_COUNT_MASK	GENMASK(15, 12)
 #define IST3038C_FINGER_STATUS_MASK	GENMASK(9, 0)
+#define IST3032C_KEY_STATUS_MASK	GENMASK(20, 16)
 
 struct imagis_properties {
 	unsigned int interrupt_msg_cmd;
@@ -41,6 +42,7 @@ struct imagis_properties {
 	unsigned int whoami_cmd;
 	unsigned int whoami_val;
 	bool protocol_b;
+	bool touch_keys_supported;
 };
 
 struct imagis_ts {
@@ -49,6 +51,8 @@ struct imagis_ts {
 	struct input_dev *input_dev;
 	struct touchscreen_properties prop;
 	struct regulator_bulk_data supplies[2];
+	u32 keycodes[2];
+	int num_keycodes;
 };
 
 static int imagis_i2c_read_reg(struct imagis_ts *ts,
@@ -93,7 +97,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
 {
 	struct imagis_ts *ts = dev_id;
 	u32 intr_message, finger_status;
-	unsigned int finger_count, finger_pressed;
+	unsigned int finger_count, finger_pressed, key_pressed;
 	int i;
 	int error;
 
@@ -139,6 +143,11 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
 				       FIELD_GET(IST3038C_AREA_MASK, finger_status));
 	}
 
+	key_pressed = FIELD_GET(IST3032C_KEY_STATUS_MASK, intr_message);
+
+	for (int i = 0; i < ts->num_keycodes; i++)
+		input_report_key(ts->input_dev, ts->keycodes[i], (key_pressed & BIT(i)));
+
 	input_mt_sync_frame(ts->input_dev);
 	input_sync(ts->input_dev);
 
@@ -224,6 +233,19 @@ static int imagis_init_input_dev(struct imagis_ts *ts)
 	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
 	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
 	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 16, 0, 0);
+	if (ts->tdata->touch_keys_supported) {
+		ts->num_keycodes = of_property_read_variable_u32_array(
+				ts->client->dev.of_node, "linux,keycodes",
+				ts->keycodes, 0, ARRAY_SIZE(ts->keycodes));
+		if (ts->num_keycodes <= 0) {
+			ts->keycodes[0] = KEY_APPSELECT;
+			ts->keycodes[1] = KEY_BACK;
+			ts->num_keycodes = 2;
+		}
+	}
+
+	for (int i = 0; i < ts->num_keycodes; i++)
+		input_set_capability(input_dev, EV_KEY, ts->keycodes[i]);
 
 	touchscreen_parse_properties(input_dev, true, &ts->prop);
 	if (!ts->prop.max_x || !ts->prop.max_y) {
@@ -365,6 +387,7 @@ static const struct imagis_properties imagis_3032c_data = {
 	.touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
 	.whoami_cmd = IST3038C_REG_CHIPID,
 	.whoami_val = IST3032C_WHOAMI,
+	.touch_keys_supported = true,
 };
 
 static const struct imagis_properties imagis_3038b_data = {

-- 
2.43.0

Re: [PATCH v2 2/3] dt-bindings: input: imagis: Document touch keys

[Krzysztof Kozlowski]

On 20/01/2024 22:16, Duje Mihanović wrote:
> IST3032C (and possibly some other models) has touch keys. Document this.
> 
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> ---

Please provide changelog describing what changed against v1. Cover
letter has something but it is not accurate. It says nothing about this
patch.


>  .../bindings/input/touchscreen/imagis,ist3038c.yaml           | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> index 2af71cbcc97d..960e5436642f 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> @@ -42,6 +42,17 @@ properties:
>    touchscreen-inverted-y: true
>    touchscreen-swapped-x-y: true
>  
> +if:

Move allOf here and keep it under allOf.

> +  properties:
> +    compatible:
> +      contains:
> +        const: imagis,ist3032c
> +then:
> +  properties:
> +    linux,keycodes:

No, this property is not allowed by your binding. I doubt this was
really tested.

Anyway, even if it works, it's not what we expect. Where is the property
defined?

Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: input: imagis: Document touch keys

[Duje Mihanović]

On Monday, January 22, 2024 11:27:09 AM CET Krzysztof Kozlowski wrote:
> On 20/01/2024 22:16, Duje Mihanović wrote:
> > diff --git
> > a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> > b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> > index 2af71cbcc97d..960e5436642f 100644
> > ---
> > a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> > +++
> > b/Documentation/devicetree/bindings/input/touchscreen/
imagis,ist3038c.yaml> 
> > @@ -42,6 +42,17 @@ properties:
> >    touchscreen-inverted-y: true
> >    touchscreen-swapped-x-y: true
> > 
> > +if:
> Move allOf here and keep it under allOf.
> 
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        const: imagis,ist3032c
> > +then:
> > +  properties:
> 
> > +    linux,keycodes:
> No, this property is not allowed by your binding. I doubt this was
> really tested.
> 
> Anyway, even if it works, it's not what we expect. Where is the property
> defined?

I was under the impression that it can be defined inside the if statement 
since "make dt_binding_check" ran without any errors. If that's wrong, I'm not 
sure how else to make the property valid only for ist3032c (the two devices 
using ist3038c, samsung-j5 and j5x seem to have tm2-touchkey instead).

Regards,
--
Duje