[PATCH v6 1/2] dt-bindings: input/touchscreen: add bindings for msg2638

[PATCH v6 1/2] dt-bindings: input/touchscreen: add bindings for msg2638

[Vincent Knecht]

This adds dts bindings for the mstar msg2638 touchscreen.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org>
---
Changed in v6:
- change touchscreen-size-x/y values in example section to reflect
  scaling removal in driver (Dmitry)
- add Linus W. Reviewed-by
Changed in v5: nothing
Changed in v4:
- don't use wildcards in compatible strings (Rob H)
- rename from msg26xx to msg2638
- rename example pinctrl-0 to &ts_int_reset_default for consistency
Changed in v3:
- added `touchscreen-size-x: true` and `touchscreen-size-y: true` properties
Changed in v2:
- changed M-Star to MStar in title line
- changed reset gpio to active-low in example section
---
 .../input/touchscreen/mstar,msg2638.yaml      | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/mstar,msg2638.yaml

diff --git a/Documentation/devicetree/bindings/input/touchscreen/mstar,msg2638.yaml b/Documentation/devicetree/bindings/input/touchscreen/mstar,msg2638.yaml
new file mode 100644
index 000000000000..3a42c23faf6f
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/mstar,msg2638.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/mstar,msg2638.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MStar msg2638 touchscreen controller Bindings
+
+maintainers:
+  - Vincent Knecht <vincent.knecht@mailoo.org>
+
+allOf:
+  - $ref: touchscreen.yaml#
+
+properties:
+  compatible:
+    const: mstar,msg2638
+
+  reg:
+    const: 0x26
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  vdd-supply:
+    description: Power supply regulator for the chip
+
+  vddio-supply:
+    description: Power supply regulator for the I2C bus
+
+  touchscreen-size-x: true
+  touchscreen-size-y: true
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - reset-gpios
+  - touchscreen-size-x
+  - touchscreen-size-y
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      touchscreen@26 {
+        compatible = "mstar,msg2638";
+        reg = <0x26>;
+        interrupt-parent = <&msmgpio>;
+        interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+        reset-gpios = <&msmgpio 100 GPIO_ACTIVE_LOW>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&ts_int_reset_default>;
+        vdd-supply = <&pm8916_l17>;
+        vddio-supply = <&pm8916_l5>;
+        touchscreen-size-x = <2048>;
+        touchscreen-size-y = <2048>;
+      };
+    };
+
+...
-- 
2.29.2

[PATCH v6 2/2] Input: add MStar MSG2638 touchscreen driver

[Vincent Knecht]

Add support for the msg2638 touchscreen IC from MStar.
Firmware handling, wakeup gestures and other specialties are not supported.
This driver reuses zinitix.c structure, while the checksum and irq handler
functions are based on out-of-tree driver for Alcatel Idol 3 (4.7").

Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org>
---
Changed in v6:
- minor formatting changes
- mention wakeup gestures not being supported (yet?) in commit message
- do not scale coordinates in the driver (Dmitry)
- multiple suggestions from Linus W.
  - include linux/gpio/consumer.h instead of linux/gpio.h
  - rename delay defines to include time unit like _MS and _US
  - rename `error` variable to `ret`
  - move enable_irq() call from msg2638_power_on() to msg2638_start()
  - remove CONFIG_OF #ifdef around of_device_id struct
Changed in v5:
- use gpiod_set_value_cansleep() (Stephan G)
- use msleep/usleep_range() rathen than mdelay() (Stephan G)
Changed in v4:
- rename from msg26xx to msg2638, following compatible string change
- rename mstar_* functions to msg2638_* for consistency
- add RESET_DELAY define and use it in msg2638_power_on()
- change a few dev_err() calls to be on one line only
- add missing \n in a few dev_err() strings
Changed in v3:
- no change
Changed in v2:
- don't use bitfields in packet struct, to prevent endian-ness related problems (Dmitry)
- fix reset gpiod calls order in mstar_power_on() (Dmitry)
---
 drivers/input/touchscreen/Kconfig   |  12 +
 drivers/input/touchscreen/Makefile  |   1 +
 drivers/input/touchscreen/msg2638.c | 354 ++++++++++++++++++++++++++++
 3 files changed, 367 insertions(+)
 create mode 100644 drivers/input/touchscreen/msg2638.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index f012fe746df0..fefbe1c1bb10 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1334,4 +1334,16 @@ config TOUCHSCREEN_ZINITIX
 	  To compile this driver as a module, choose M here: the
 	  module will be called zinitix.
 
+config TOUCHSCREEN_MSG2638
+	tristate "MStar msg2638 touchscreen support"
+	depends on I2C
+	depends on GPIOLIB || COMPILE_TEST
+	help
+	  Say Y here if you have an I2C touchscreen using MStar msg2638.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called msg2638.
+
 endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 6233541e9173..83e516cb3d33 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -112,3 +112,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)	+= rohm_bu21023.o
 obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
 obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
 obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
+obj-$(CONFIG_TOUCHSCREEN_MSG2638)	+= msg2638.o
diff --git a/drivers/input/touchscreen/msg2638.c b/drivers/input/touchscreen/msg2638.c
new file mode 100644
index 000000000000..8eb3f195d743
--- /dev/null
+++ b/drivers/input/touchscreen/msg2638.c
@@ -0,0 +1,354 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for MStar msg2638 touchscreens
+ *
+ * Copyright (c) 2021 Vincent Knecht <vincent.knecht@mailoo.org>
+ *
+ * Checksum and IRQ handler based on mstar_drv_common.c and mstar_drv_mutual_fw_control.c
+ * Copyright (c) 2006-2012 MStar Semiconductor, Inc.
+ *
+ * Driver structure based on zinitix.c by Michael Srba <Michael.Srba@seznam.cz>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#define MODE_DATA_RAW			0x5A
+
+#define MAX_SUPPORTED_FINGER_NUM	5
+
+#define CHIP_ON_DELAY_MS		15
+#define FIRMWARE_ON_DELAY_MS		50
+#define RESET_DELAY_MIN_US		10000
+#define RESET_DELAY_MAX_US		11000
+
+struct point_coord {
+	u16	x;
+	u16	y;
+};
+
+struct packet {
+	u8	xy_hi; /* higher bits of x and y coordinates */
+	u8	x_low;
+	u8	y_low;
+	u8	pressure;
+};
+
+struct touch_event {
+	u8	mode;
+	struct	packet pkt[MAX_SUPPORTED_FINGER_NUM];
+	u8	proximity;
+	u8	checksum;
+};
+
+struct msg2638_ts_data {
+	struct i2c_client *client;
+	struct input_dev *input_dev;
+	struct touchscreen_properties prop;
+	struct regulator_bulk_data supplies[2];
+	struct gpio_desc *reset_gpiod;
+};
+
+static int msg2638_init_regulators(struct msg2638_ts_data *msg2638)
+{
+	struct i2c_client *client = msg2638->client;
+	int ret;
+
+	msg2638->supplies[0].supply = "vdd";
+	msg2638->supplies[1].supply = "vddio";
+	ret = devm_regulator_bulk_get(&client->dev,
+				      ARRAY_SIZE(msg2638->supplies),
+				      msg2638->supplies);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to get regulators: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void msg2638_power_on(struct msg2638_ts_data *msg2638)
+{
+	gpiod_set_value_cansleep(msg2638->reset_gpiod, 1);
+	usleep_range(RESET_DELAY_MIN_US, RESET_DELAY_MAX_US);
+	gpiod_set_value_cansleep(msg2638->reset_gpiod, 0);
+	msleep(FIRMWARE_ON_DELAY_MS);
+}
+
+static void msg2638_report_finger(struct msg2638_ts_data *msg2638, int slot,
+				const struct point_coord *pc)
+{
+	input_mt_slot(msg2638->input_dev, slot);
+	input_mt_report_slot_state(msg2638->input_dev, MT_TOOL_FINGER, true);
+	touchscreen_report_pos(msg2638->input_dev, &msg2638->prop, pc->x, pc->y, true);
+	input_report_abs(msg2638->input_dev, ABS_MT_TOUCH_MAJOR, 1);
+}
+
+static u8 msg2638_checksum(u8 *data, u32 length)
+{
+	s32 sum = 0;
+	u32 i;
+
+	for (i = 0; i < length; i++)
+		sum += data[i];
+
+	return (u8)((-sum) & 0xFF);
+}
+
+static irqreturn_t msg2638_ts_irq_handler(int irq, void *msg2638_handler)
+{
+	struct msg2638_ts_data *msg2638 = msg2638_handler;
+	struct i2c_client *client = msg2638->client;
+	struct touch_event touch_event;
+	struct point_coord coord;
+	struct i2c_msg msg[1];
+	struct packet *p;
+	u32 len;
+	int ret;
+	int i;
+
+	len = sizeof(struct touch_event);
+	memset(&touch_event, 0, len);
+
+	msg[0].addr = client->addr;
+	msg[0].flags = I2C_M_RD;
+	msg[0].len = len;
+	msg[0].buf = (u8 *)&touch_event;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "Failed I2C transfer in irq handler!\n");
+		goto out;
+	}
+
+	if (touch_event.mode != MODE_DATA_RAW)
+		goto out;
+
+	if (msg2638_checksum((u8 *)&touch_event, len - 1) != touch_event.checksum) {
+		dev_err(&client->dev, "Failed checksum!\n");
+		goto out;
+	}
+
+	for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++) {
+		p = &touch_event.pkt[i];
+		/* Ignore non-pressed finger data */
+		if (p->xy_hi == 0xFF && p->x_low == 0xFF && p->y_low == 0xFF)
+			continue;
+
+		coord.x = (((p->xy_hi & 0xF0) << 4) | p->x_low);
+		coord.y = (((p->xy_hi & 0x0F) << 8) | p->y_low);
+		msg2638_report_finger(msg2638, i, &coord);
+	}
+
+	input_mt_sync_frame(msg2638->input_dev);
+	input_sync(msg2638->input_dev);
+
+out:
+	return IRQ_HANDLED;
+}
+
+static int msg2638_start(struct msg2638_ts_data *msg2638)
+{
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(msg2638->supplies), msg2638->supplies);
+	if (ret) {
+		dev_err(&msg2638->client->dev, "Failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	msleep(CHIP_ON_DELAY_MS);
+	msg2638_power_on(msg2638);
+	enable_irq(msg2638->client->irq);
+
+	return 0;
+}
+
+static int msg2638_stop(struct msg2638_ts_data *msg2638)
+{
+	int ret;
+
+	disable_irq(msg2638->client->irq);
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(msg2638->supplies), msg2638->supplies);
+	if (ret) {
+		dev_err(&msg2638->client->dev, "Failed to disable regulators: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int msg2638_input_open(struct input_dev *dev)
+{
+	struct msg2638_ts_data *msg2638 = input_get_drvdata(dev);
+
+	return msg2638_start(msg2638);
+}
+
+static void msg2638_input_close(struct input_dev *dev)
+{
+	struct msg2638_ts_data *msg2638 = input_get_drvdata(dev);
+
+	msg2638_stop(msg2638);
+}
+
+static int msg2638_init_input_dev(struct msg2638_ts_data *msg2638)
+{
+	struct input_dev *input_dev;
+	int ret;
+
+	input_dev = devm_input_allocate_device(&msg2638->client->dev);
+	if (!input_dev) {
+		dev_err(&msg2638->client->dev, "Failed to allocate input device.\n");
+		return -ENOMEM;
+	}
+
+	input_set_drvdata(input_dev, msg2638);
+	msg2638->input_dev = input_dev;
+
+	input_dev->name = "MStar TouchScreen";
+	input_dev->phys = "input/ts";
+	input_dev->id.bustype = BUS_I2C;
+	input_dev->open = msg2638_input_open;
+	input_dev->close = msg2638_input_close;
+
+	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_WIDTH_MAJOR, 0, 15, 0, 0);
+	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
+	touchscreen_parse_properties(input_dev, true, &msg2638->prop);
+	if (!msg2638->prop.max_x || !msg2638->prop.max_y) {
+		dev_err(&msg2638->client->dev,
+			"touchscreen-size-x and/or touchscreen-size-y not set in dts\n");
+		return -EINVAL;
+	}
+
+	ret = input_mt_init_slots(input_dev, MAX_SUPPORTED_FINGER_NUM,
+				  INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+	if (ret) {
+		dev_err(&msg2638->client->dev, "Failed to initialize MT slots: %d\n", ret);
+		return ret;
+	}
+
+	ret = input_register_device(input_dev);
+	if (ret) {
+		dev_err(&msg2638->client->dev, "Failed to register input device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int msg2638_ts_probe(struct i2c_client *client)
+{
+	struct msg2638_ts_data *msg2638;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(&client->dev, "Failed to assert adapter's support for plain I2C.\n");
+		return -ENXIO;
+	}
+
+	msg2638 = devm_kzalloc(&client->dev, sizeof(*msg2638), GFP_KERNEL);
+	if (!msg2638)
+		return -ENOMEM;
+
+	msg2638->client = client;
+	i2c_set_clientdata(client, msg2638);
+
+	ret = msg2638_init_regulators(msg2638);
+	if (ret) {
+		dev_err(&client->dev, "Failed to initialize regulators: %d\n", ret);
+		return ret;
+	}
+
+	msg2638->reset_gpiod = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(msg2638->reset_gpiod)) {
+		ret = PTR_ERR(msg2638->reset_gpiod);
+		dev_err(&client->dev, "Failed to request reset GPIO: %d\n", ret);
+		return ret;
+	}
+
+	ret = msg2638_init_input_dev(msg2638);
+	if (ret) {
+		dev_err(&client->dev, "Failed to initialize input device: %d\n", ret);
+		return ret;
+	}
+
+	irq_set_status_flags(client->irq, IRQ_NOAUTOEN);
+	ret = devm_request_threaded_irq(&client->dev, client->irq,
+					NULL, msg2638_ts_irq_handler,
+					IRQF_ONESHOT, client->name, msg2638);
+	if (ret) {
+		dev_err(&client->dev, "Failed to request IRQ: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __maybe_unused msg2638_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct msg2638_ts_data *msg2638 = i2c_get_clientdata(client);
+
+	mutex_lock(&msg2638->input_dev->mutex);
+
+	if (input_device_enabled(msg2638->input_dev))
+		msg2638_stop(msg2638);
+
+	mutex_unlock(&msg2638->input_dev->mutex);
+
+	return 0;
+}
+
+static int __maybe_unused msg2638_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct msg2638_ts_data *msg2638 = i2c_get_clientdata(client);
+	int ret = 0;
+
+	mutex_lock(&msg2638->input_dev->mutex);
+
+	if (input_device_enabled(msg2638->input_dev))
+		ret = msg2638_start(msg2638);
+
+	mutex_unlock(&msg2638->input_dev->mutex);
+
+	return ret;
+}
+
+static SIMPLE_DEV_PM_OPS(msg2638_pm_ops, msg2638_suspend, msg2638_resume);
+
+static const struct of_device_id msg2638_of_match[] = {
+	{ .compatible = "mstar,msg2638" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, msg2638_of_match);
+
+static struct i2c_driver msg2638_ts_driver = {
+	.probe_new = msg2638_ts_probe,
+	.driver = {
+		.name = "MStar-TS",
+		.pm = &msg2638_pm_ops,
+		.of_match_table = of_match_ptr(msg2638_of_match),
+	},
+};
+module_i2c_driver(msg2638_ts_driver);
+
+MODULE_AUTHOR("Vincent Knecht <vincent.knecht@mailoo.org>");
+MODULE_DESCRIPTION("MStar MSG2638 touchscreen driver");
+MODULE_LICENSE("GPL v2");
-- 
2.29.2

Re: [PATCH v6 1/2] dt-bindings: input/touchscreen: add bindings for msg2638

[Rob Herring]

On Fri, 05 Mar 2021 16:38:04 +0100, Vincent Knecht wrote:
> This adds dts bindings for the mstar msg2638 touchscreen.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org>
> ---
> Changed in v6:
> - change touchscreen-size-x/y values in example section to reflect
>   scaling removal in driver (Dmitry)
> - add Linus W. Reviewed-by
> Changed in v5: nothing
> Changed in v4:
> - don't use wildcards in compatible strings (Rob H)
> - rename from msg26xx to msg2638
> - rename example pinctrl-0 to &ts_int_reset_default for consistency
> Changed in v3:
> - added `touchscreen-size-x: true` and `touchscreen-size-y: true` properties
> Changed in v2:
> - changed M-Star to MStar in title line
> - changed reset gpio to active-low in example section
> ---
>  .../input/touchscreen/mstar,msg2638.yaml      | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/mstar,msg2638.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v6 2/2] Input: add MStar MSG2638 touchscreen driver

[Linus Walleij]

On Fri, Mar 5, 2021 at 4:38 PM Vincent Knecht <vincent.knecht@mailoo.org> wrote:

> Add support for the msg2638 touchscreen IC from MStar.
> Firmware handling, wakeup gestures and other specialties are not supported.
> This driver reuses zinitix.c structure, while the checksum and irq handler
> functions are based on out-of-tree driver for Alcatel Idol 3 (4.7").
>
> Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org>
> ---
> Changed in v6:

Looks good to me:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v6 2/2] Input: add MStar MSG2638 touchscreen driver

[Vincent Knecht]

Le vendredi 05 mars 2021 à 16:38 +0100, Vincent Knecht a écrit :
> Add support for the msg2638 touchscreen IC from MStar.
> Firmware handling, wakeup gestures and other specialties are not supported.
> This driver reuses zinitix.c structure, while the checksum and irq handler
> functions are based on out-of-tree driver for Alcatel Idol 3 (4.7").
> 
> Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org>
> ---
> Changed in v6:
> - minor formatting changes
> - mention wakeup gestures not being supported (yet?) in commit message
> - do not scale coordinates in the driver (Dmitry)
> - multiple suggestions from Linus W.
>   - include linux/gpio/consumer.h instead of linux/gpio.h
>   - rename delay defines to include time unit like _MS and _US
>   - rename `error` variable to `ret`
>   - move enable_irq() call from msg2638_power_on() to msg2638_start()
>   - remove CONFIG_OF #ifdef around of_device_id struct

Hello,

just a little gentle ping: do I need to change anything to get this merged ?
So far seems to work fine with idol347 port of postmarketOS :-)

Re: [PATCH v6 2/2] Input: add MStar MSG2638 touchscreen driver

[Dmitry Torokhov]

Hi Vincent,

On Fri, Mar 05, 2021 at 04:38:05PM +0100, Vincent Knecht wrote:
> Add support for the msg2638 touchscreen IC from MStar.
> Firmware handling, wakeup gestures and other specialties are not supported.
> This driver reuses zinitix.c structure, while the checksum and irq handler
> functions are based on out-of-tree driver for Alcatel Idol 3 (4.7").
> 
> Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org>
> ---
> Changed in v6:
> - minor formatting changes
> - mention wakeup gestures not being supported (yet?) in commit message
> - do not scale coordinates in the driver (Dmitry)
> - multiple suggestions from Linus W.
>   - include linux/gpio/consumer.h instead of linux/gpio.h
>   - rename delay defines to include time unit like _MS and _US
>   - rename `error` variable to `ret`
>   - move enable_irq() call from msg2638_power_on() to msg2638_start()
>   - remove CONFIG_OF #ifdef around of_device_id struct
> Changed in v5:
> - use gpiod_set_value_cansleep() (Stephan G)
> - use msleep/usleep_range() rathen than mdelay() (Stephan G)
> Changed in v4:
> - rename from msg26xx to msg2638, following compatible string change
> - rename mstar_* functions to msg2638_* for consistency
> - add RESET_DELAY define and use it in msg2638_power_on()
> - change a few dev_err() calls to be on one line only
> - add missing \n in a few dev_err() strings
> Changed in v3:
> - no change
> Changed in v2:
> - don't use bitfields in packet struct, to prevent endian-ness related problems (Dmitry)
> - fix reset gpiod calls order in mstar_power_on() (Dmitry)
> ---
>  drivers/input/touchscreen/Kconfig   |  12 +
>  drivers/input/touchscreen/Makefile  |   1 +
>  drivers/input/touchscreen/msg2638.c | 354 ++++++++++++++++++++++++++++
>  3 files changed, 367 insertions(+)
>  create mode 100644 drivers/input/touchscreen/msg2638.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index f012fe746df0..fefbe1c1bb10 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1334,4 +1334,16 @@ config TOUCHSCREEN_ZINITIX
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called zinitix.
>  
> +config TOUCHSCREEN_MSG2638
> +	tristate "MStar msg2638 touchscreen support"
> +	depends on I2C
> +	depends on GPIOLIB || COMPILE_TEST
> +	help
> +	  Say Y here if you have an I2C touchscreen using MStar msg2638.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called msg2638.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 6233541e9173..83e516cb3d33 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -112,3 +112,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)	+= rohm_bu21023.o
>  obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
>  obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
> +obj-$(CONFIG_TOUCHSCREEN_MSG2638)	+= msg2638.o
> diff --git a/drivers/input/touchscreen/msg2638.c b/drivers/input/touchscreen/msg2638.c
> new file mode 100644
> index 000000000000..8eb3f195d743
> --- /dev/null
> +++ b/drivers/input/touchscreen/msg2638.c
> @@ -0,0 +1,354 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for MStar msg2638 touchscreens
> + *
> + * Copyright (c) 2021 Vincent Knecht <vincent.knecht@mailoo.org>
> + *
> + * Checksum and IRQ handler based on mstar_drv_common.c and mstar_drv_mutual_fw_control.c
> + * Copyright (c) 2006-2012 MStar Semiconductor, Inc.
> + *
> + * Driver structure based on zinitix.c by Michael Srba <Michael.Srba@seznam.cz>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +#define MODE_DATA_RAW			0x5A
> +
> +#define MAX_SUPPORTED_FINGER_NUM	5
> +
> +#define CHIP_ON_DELAY_MS		15
> +#define FIRMWARE_ON_DELAY_MS		50
> +#define RESET_DELAY_MIN_US		10000
> +#define RESET_DELAY_MAX_US		11000
> +
> +struct point_coord {
> +	u16	x;
> +	u16	y;
> +};
> +
> +struct packet {
> +	u8	xy_hi; /* higher bits of x and y coordinates */
> +	u8	x_low;
> +	u8	y_low;
> +	u8	pressure;
> +};
> +
> +struct touch_event {
> +	u8	mode;
> +	struct	packet pkt[MAX_SUPPORTED_FINGER_NUM];
> +	u8	proximity;
> +	u8	checksum;
> +};
> +
> +struct msg2638_ts_data {
> +	struct i2c_client *client;
> +	struct input_dev *input_dev;
> +	struct touchscreen_properties prop;
> +	struct regulator_bulk_data supplies[2];
> +	struct gpio_desc *reset_gpiod;
> +};
> +
> +static int msg2638_init_regulators(struct msg2638_ts_data *msg2638)
> +{
> +	struct i2c_client *client = msg2638->client;
> +	int ret;
> +
> +	msg2638->supplies[0].supply = "vdd";
> +	msg2638->supplies[1].supply = "vddio";
> +	ret = devm_regulator_bulk_get(&client->dev,
> +				      ARRAY_SIZE(msg2638->supplies),
> +				      msg2638->supplies);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to get regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;

I do not see benefit of factoring out into separate function so I moved
it directly into probe(), similarly how we acquire reset GPIO.

> +}
> +
> +static void msg2638_power_on(struct msg2638_ts_data *msg2638)

Renamed to msg2638_reset().

> +{
> +	gpiod_set_value_cansleep(msg2638->reset_gpiod, 1);
> +	usleep_range(RESET_DELAY_MIN_US, RESET_DELAY_MAX_US);
> +	gpiod_set_value_cansleep(msg2638->reset_gpiod, 0);
> +	msleep(FIRMWARE_ON_DELAY_MS);
> +}
> +
> +static void msg2638_report_finger(struct msg2638_ts_data *msg2638, int slot,
> +				const struct point_coord *pc)
> +{
> +	input_mt_slot(msg2638->input_dev, slot);
> +	input_mt_report_slot_state(msg2638->input_dev, MT_TOOL_FINGER, true);
> +	touchscreen_report_pos(msg2638->input_dev, &msg2638->prop, pc->x, pc->y, true);
> +	input_report_abs(msg2638->input_dev, ABS_MT_TOUCH_MAJOR, 1);

The device does not really report "touch major", this is synthetic data,
and therefore I removed it.

All in all this is quite simple function, I pushed it down into
msg2638_ts_irq_handler().

> +}
> +
> +static u8 msg2638_checksum(u8 *data, u32 length)
> +{
> +	s32 sum = 0;
> +	u32 i;
> +
> +	for (i = 0; i < length; i++)
> +		sum += data[i];
> +
> +	return (u8)((-sum) & 0xFF);
> +}
> +
> +static irqreturn_t msg2638_ts_irq_handler(int irq, void *msg2638_handler)
> +{
> +	struct msg2638_ts_data *msg2638 = msg2638_handler;
> +	struct i2c_client *client = msg2638->client;
> +	struct touch_event touch_event;
> +	struct point_coord coord;
> +	struct i2c_msg msg[1];
> +	struct packet *p;
> +	u32 len;
> +	int ret;
> +	int i;
> +
> +	len = sizeof(struct touch_event);
> +	memset(&touch_event, 0, len);
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = I2C_M_RD;
> +	msg[0].len = len;
> +	msg[0].buf = (u8 *)&touch_event;
> +
> +	ret = i2c_transfer(client->adapter, msg, 1);
> +	if (ret != 1) {
> +		dev_err(&client->dev, "Failed I2C transfer in irq handler!\n");
> +		goto out;
> +	}
> +
> +	if (touch_event.mode != MODE_DATA_RAW)
> +		goto out;
> +
> +	if (msg2638_checksum((u8 *)&touch_event, len - 1) != touch_event.checksum) {
> +		dev_err(&client->dev, "Failed checksum!\n");
> +		goto out;
> +	}
> +
> +	for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++) {
> +		p = &touch_event.pkt[i];
> +		/* Ignore non-pressed finger data */
> +		if (p->xy_hi == 0xFF && p->x_low == 0xFF && p->y_low == 0xFF)
> +			continue;
> +
> +		coord.x = (((p->xy_hi & 0xF0) << 4) | p->x_low);
> +		coord.y = (((p->xy_hi & 0x0F) << 8) | p->y_low);
> +		msg2638_report_finger(msg2638, i, &coord);
> +	}
> +
> +	input_mt_sync_frame(msg2638->input_dev);
> +	input_sync(msg2638->input_dev);
> +
> +out:
> +	return IRQ_HANDLED;
> +}
> +
> +static int msg2638_start(struct msg2638_ts_data *msg2638)
> +{
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(msg2638->supplies), msg2638->supplies);
> +	if (ret) {
> +		dev_err(&msg2638->client->dev, "Failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	msleep(CHIP_ON_DELAY_MS);
> +	msg2638_power_on(msg2638);
> +	enable_irq(msg2638->client->irq);
> +
> +	return 0;
> +}
> +
> +static int msg2638_stop(struct msg2638_ts_data *msg2638)
> +{
> +	int ret;
> +
> +	disable_irq(msg2638->client->irq);
> +
> +	ret = regulator_bulk_disable(ARRAY_SIZE(msg2638->supplies), msg2638->supplies);
> +	if (ret) {
> +		dev_err(&msg2638->client->dev, "Failed to disable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int msg2638_input_open(struct input_dev *dev)
> +{
> +	struct msg2638_ts_data *msg2638 = input_get_drvdata(dev);
> +
> +	return msg2638_start(msg2638);
> +}
> +
> +static void msg2638_input_close(struct input_dev *dev)
> +{
> +	struct msg2638_ts_data *msg2638 = input_get_drvdata(dev);
> +
> +	msg2638_stop(msg2638);
> +}
> +
> +static int msg2638_init_input_dev(struct msg2638_ts_data *msg2638)
> +{
> +	struct input_dev *input_dev;
> +	int ret;
> +
> +	input_dev = devm_input_allocate_device(&msg2638->client->dev);
> +	if (!input_dev) {
> +		dev_err(&msg2638->client->dev, "Failed to allocate input device.\n");
> +		return -ENOMEM;
> +	}
> +
> +	input_set_drvdata(input_dev, msg2638);
> +	msg2638->input_dev = input_dev;
> +
> +	input_dev->name = "MStar TouchScreen";
> +	input_dev->phys = "input/ts";
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->open = msg2638_input_open;
> +	input_dev->close = msg2638_input_close;
> +
> +	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_WIDTH_MAJOR, 0, 15, 0, 0);
> +	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);

The last 2 events are not reported and should not be advertised in the
capabilities.

> +
> +	touchscreen_parse_properties(input_dev, true, &msg2638->prop);
> +	if (!msg2638->prop.max_x || !msg2638->prop.max_y) {
> +		dev_err(&msg2638->client->dev,
> +			"touchscreen-size-x and/or touchscreen-size-y not set in dts\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = input_mt_init_slots(input_dev, MAX_SUPPORTED_FINGER_NUM,
> +				  INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> +	if (ret) {
> +		dev_err(&msg2638->client->dev, "Failed to initialize MT slots: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = input_register_device(input_dev);
> +	if (ret) {
> +		dev_err(&msg2638->client->dev, "Failed to register input device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int msg2638_ts_probe(struct i2c_client *client)
> +{
> +	struct msg2638_ts_data *msg2638;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev, "Failed to assert adapter's support for plain I2C.\n");
> +		return -ENXIO;
> +	}
> +
> +	msg2638 = devm_kzalloc(&client->dev, sizeof(*msg2638), GFP_KERNEL);
> +	if (!msg2638)
> +		return -ENOMEM;
> +
> +	msg2638->client = client;
> +	i2c_set_clientdata(client, msg2638);
> +
> +	ret = msg2638_init_regulators(msg2638);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to initialize regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	msg2638->reset_gpiod = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(msg2638->reset_gpiod)) {
> +		ret = PTR_ERR(msg2638->reset_gpiod);
> +		dev_err(&client->dev, "Failed to request reset GPIO: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = msg2638_init_input_dev(msg2638);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to initialize input device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	irq_set_status_flags(client->irq, IRQ_NOAUTOEN);

We now have IRQF_NO_AUTOEN as a flag for request_irq() and friends so I
used it instead of irq_set_status_flags().

> +	ret = devm_request_threaded_irq(&client->dev, client->irq,
> +					NULL, msg2638_ts_irq_handler,
> +					IRQF_ONESHOT, client->name, msg2638);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to request IRQ: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused msg2638_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct msg2638_ts_data *msg2638 = i2c_get_clientdata(client);
> +
> +	mutex_lock(&msg2638->input_dev->mutex);
> +
> +	if (input_device_enabled(msg2638->input_dev))
> +		msg2638_stop(msg2638);
> +
> +	mutex_unlock(&msg2638->input_dev->mutex);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused msg2638_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct msg2638_ts_data *msg2638 = i2c_get_clientdata(client);
> +	int ret = 0;
> +
> +	mutex_lock(&msg2638->input_dev->mutex);
> +
> +	if (input_device_enabled(msg2638->input_dev))
> +		ret = msg2638_start(msg2638);
> +
> +	mutex_unlock(&msg2638->input_dev->mutex);
> +
> +	return ret;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(msg2638_pm_ops, msg2638_suspend, msg2638_resume);
> +
> +static const struct of_device_id msg2638_of_match[] = {
> +	{ .compatible = "mstar,msg2638" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, msg2638_of_match);
> +
> +static struct i2c_driver msg2638_ts_driver = {
> +	.probe_new = msg2638_ts_probe,
> +	.driver = {
> +		.name = "MStar-TS",
> +		.pm = &msg2638_pm_ops,
> +		.of_match_table = of_match_ptr(msg2638_of_match),

Given that the driver is not depending on OF, when !CONFIG_OF this will
cause unused variable msg2638_of_match. Given that we can use OF
matchinv with ACPI I dropped of_match_ptr().

> +	},
> +};
> +module_i2c_driver(msg2638_ts_driver);
> +
> +MODULE_AUTHOR("Vincent Knecht <vincent.knecht@mailoo.org>");
> +MODULE_DESCRIPTION("MStar MSG2638 touchscreen driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.29.2
> 
> 
> 

Thanks.

-- 
Dmitry