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

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

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

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 :-)

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.