Re: [PATCH 6/7] Input: touchscreen: add Synaptics TCM oncell S3908

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Caleb,

On Mon, Jun 24, 2024 at 03:30:30AM +0200, Caleb Connolly wrote:
> The TCM oncell is the next generation of Synaptics touchscreen ICs.
> These run a very featured firmware with a reasonably well defined API.
> It is however entirely incompatible with the existing RMI4 interface.
> 
> Unfortunately, no public datasheet for the interface seems to be
> available, instead this driver was created through a combination of
> vendor drivers and trial and error.
> 
> The firmware interface implies support for defining the exact bit
> encoding of the touch reports, however on the S3908 chip + firmware
> found in the OnePlus 8t the TCM_SET_TOUCH_REPORT_CONFIG command appears
> to be unsupported.
> 
> Co-developed-by: Frieder Hannenheim <frieder.hannenheim@xxxxxxxxx>
> Signed-off-by: Frieder Hannenheim <frieder.hannenheim@xxxxxxxxx>
> Signed-off-by: Caleb Connolly <caleb@xxxxxxxxxxxxxxxx>
> ---
>  MAINTAINERS                                      |   7 +
>  drivers/input/touchscreen/Kconfig                |  11 +
>  drivers/input/touchscreen/Makefile               |   1 +
>  drivers/input/touchscreen/synaptics_tcm_oncell.c | 617 +++++++++++++++++++++++
>  4 files changed, 636 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2b9cfbf92d7a..db589c841d8c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21826,8 +21826,15 @@ M:	Icenowy Zheng <icenowy@xxxxxxx>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/regulator/silergy,sy8106a.yaml
>  F:	drivers/regulator/sy8106a-regulator.c
>  
> +SYNAPTICS TCM ONCELL TOUCHSCREEN DRIVER
> +M:	Caleb Connolly <caleb@xxxxxxxxxxxxxxxx>
> +L:	linux-input@xxxxxxxxxxxxxxx
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/input/touchscreen/syna,tcm-oncell.yaml
> +F:	drivers/input/touchscreen/synaptics_tcm_oncell.c
> +
>  SYNC FILE FRAMEWORK
>  M:	Sumit Semwal <sumit.semwal@xxxxxxxxxx>
>  R:	Gustavo Padovan <gustavo@xxxxxxxxxxx>
>  L:	linux-media@xxxxxxxxxxxxxxx
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index c821fe3ee794..43c4fd80601c 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -531,8 +531,19 @@ config TOUCHSCREEN_S6SY761
>  
>  	  To compile this driver as module, choose M here: the
>  	  module will be called s6sy761.
>  
> +config TOUCHSCREEN_SYNAPTICS_TCM_ONCELL
> +	tristate "Synaptics TCM Oncell Touchscreen driver"
> +	depends on I2C
> +	help
> +	  Say Y if you have the Synaptics S3908 TCM Oncell
> +
> +	  If unsure, say N
> +
> +	  To compile this driver as module, choose M here: the
> +	  module will be called synaptics_tcm_oncell.
> +
>  config TOUCHSCREEN_GUNZE
>  	tristate "Gunze AHL-51S touchscreen"
>  	select SERIO
>  	help
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index a81cb5aa21a5..6a2b85050c3a 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -88,8 +88,9 @@ obj-$(CONFIG_TOUCHSCREEN_STMFTS)	+= stmfts.o
>  obj-$(CONFIG_TOUCHSCREEN_STMPE)		+= stmpe-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_SUN4I)		+= sun4i-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_SUR40)		+= sur40.o
>  obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI)	+= surface3_spi.o
> +obj-$(CONFIG_TOUCHSCREEN_SYNAPTICS_TCM_ONCELL)	+= synaptics_tcm_oncell.o
>  obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC)	+= ti_am335x_tsc.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
> diff --git a/drivers/input/touchscreen/synaptics_tcm_oncell.c b/drivers/input/touchscreen/synaptics_tcm_oncell.c
> new file mode 100644
> index 000000000000..c1ba9a3a93c0
> --- /dev/null
> +++ b/drivers/input/touchscreen/synaptics_tcm_oncell.c
> @@ -0,0 +1,617 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  Driver for Synaptics TCM Oncell Touchscreens
> + *
> + *  Copyright (c) 2024 Frieder Hannenheim <frieder.hannenheim@xxxxxxxxx>
> + *  Copyright (c) 2024 Caleb Connolly <caleb@xxxxxxxxxxxxxxxx>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/of_gpio.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +/*
> + * The TCM oncell interface uses a command byte, which may be followed by additional
> + * data. The packet format is defined in the tcm_cmd struct.
> + *
> + * The following list only defines commands that are used in this driver (and their
> + * counterparts for context). Vendor reference implementations can be found at
> + * https://github.com/LineageOS/android_kernel_oneplus_sm8250/tree/ee0a7ee1939ffd53000e42051caf8f0800defb27/drivers/input/touchscreen/synaptics_tcm
> + */
> +
> +/*
> + * Request information about the chip. We don't send this command explicitly as
> + * the controller automatically sends this information when starting up.
> + */
> +#define TCM_IDENTIFY 0x02
> +
> +/* Enable/disable reporting touch inputs */
> +#define TCM_ENABLE_REPORT 0x05
> +#define TCM_DISABLE_REPORT 0x06
> +
> +/*
> + * After powering on, we send this to exit the bootloader mode and run the main
> + * firmware.
> + */
> +#define TCM_RUN_APPLICATION_FIRMWARE 0x14
> +
> +/*
> + * Reports information about the vendor provided application firmware. This is
> + * also used to determine when the firmware has finished booting.
> + */
> +#define TCM_GET_APPLICATION_INFO 0x20
> +
> +#define MODE_APPLICATION 0x01
> +
> +#define APP_STATUS_OK 0x00
> +#define APP_STATUS_BOOTING 0x01
> +#define APP_STATUS_UPDATING 0x02
> +#define APP_STATUS_BAD_APP_CONFIG 0xff
> +
> +/* status codes */
> +#define REPORT_IDLE 0x00
> +#define REPORT_OK 0x01
> +#define REPORT_BUSY 0x02
> +#define REPORT_CONTINUED_READ 0x03
> +#define REPORT_RECEIVE_BUFFER_OVERFLOW 0x0c
> +#define REPORT_PREVIOUS_COMMAND_PENDING 0x0d
> +#define REPORT_NOT_IMPLEMENTED 0x0e
> +#define REPORT_ERROR 0x0f
> +
> +/* report types */
> +#define REPORT_IDENTIFY 0x10
> +#define REPORT_TOUCH 0x11
> +#define REPORT_DELTA 0x12
> +#define REPORT_RAW 0x13
> +#define REPORT_DEBUG 0x14
> +#define REPORT_LOG 0x1d
> +#define REPORT_TOUCH_HOLD 0x20
> +#define REPORT_INVALID 0xff
> +
> +/* Touch report codes */
> +#define TOUCH_END 0
> +#define TOUCH_FOREACH_ACTIVE_OBJECT 1
> +#define TOUCH_FOREACH_OBJECT 2
> +#define TOUCH_FOREACH_END 3
> +#define TOUCH_PAD_TO_NEXT_BYTE 4
> +#define TOUCH_TIMESTAMP 5
> +#define TOUCH_OBJECT_N_INDEX 6
> +#define TOUCH_OBJECT_N_CLASSIFICATION 7
> +#define TOUCH_OBJECT_N_X_POSITION 8
> +#define TOUCH_OBJECT_N_Y_POSITION 9
> +#define TOUCH_OBJECT_N_Z 10
> +#define TOUCH_OBJECT_N_X_WIDTH 11
> +#define TOUCH_OBJECT_N_Y_WIDTH 12
> +#define TOUCH_OBJECT_N_TX_POSITION_TIXELS 13
> +#define TOUCH_OBJECT_N_RX_POSITION_TIXELS 14
> +#define TOUCH_0D_BUTTONS_STATE 15
> +#define TOUCH_GESTURE_DOUBLE_TAP 16
> +#define TOUCH_FRAME_RATE 17 /* Normally 80hz */
> +#define TOUCH_POWER_IM 18
> +#define TOUCH_CID_IM 19
> +#define TOUCH_RAIL_IM 20
> +#define TOUCH_CID_VARIANCE_IM 21
> +#define TOUCH_NSM_FREQUENCY 22
> +#define TOUCH_NSM_STATE 23
> +#define TOUCH_NUM_OF_ACTIVE_OBJECTS 23
> +#define TOUCH_NUM_OF_CPU_CYCLES_USED_SINCE_LAST_FRAME 24
> +#define TOUCH_TUNING_GAUSSIAN_WIDTHS 0x80
> +#define TOUCH_TUNING_SMALL_OBJECT_PARAMS 0x81
> +#define TOUCH_TUNING_0D_BUTTONS_VARIANCE 0x82
> +#define TOUCH_REPORT_GESTURE_SWIPE 193
> +#define TOUCH_REPORT_GESTURE_CIRCLE 194
> +#define TOUCH_REPORT_GESTURE_UNICODE 195
> +#define TOUCH_REPORT_GESTURE_VEE 196
> +#define TOUCH_REPORT_GESTURE_TRIANGLE 197
> +#define TOUCH_REPORT_GESTURE_INFO 198
> +#define TOUCH_REPORT_GESTURE_COORDINATE 199
> +#define TOUCH_REPORT_CUSTOMER_GRIP_INFO 203

I don't think all these are used. Also, could we align values on the
same column please?

> +
> +struct tcm_message_header {
> +	u8 marker;
> +	u8 code;
> +} __packed;

I don't think this needs to be packed.

> +
> +/* header + 2 bytes (which are length of data depending on report code) */
> +#define REPORT_PEAK_LEN (sizeof(struct tcm_message_header) + 2)
> +
> +struct tcm_cmd {
> +	u8 cmd;
> +	u16 length;
> +	u8 data[];
> +};
> +
> +struct tcm_identification {
> +	struct tcm_message_header header;
> +	u16 length;
> +	u8 version;
> +	u8 mode;
> +	char part_number[16];
> +	u8 build_id[4];
> +	u8 max_write_size[2];
> +} __packed;
> +
> +struct tcm_app_info {
> +	struct tcm_message_header header;
> +	u16 length;

On-wire data needs to be annotated with proper endiannes (__le16 or
__be16) and converted to CPU-endianness before use.

> +	u8 version[2];
> +	u16 status;
> +	u8 static_config_size[2];

Should this be __le16 or __be16.

> +	u8 dynamic_config_size[2];
> +	u8 app_config_start_write_block[2];
> +	u8 app_config_size[2];
> +	u8 max_touch_report_config_size[2];
> +	u8 max_touch_report_payload_size[2];
> +	char customer_config_id[16];
> +	u16 max_x;
> +	u16 max_y;
> +	u8 max_objects[2];
> +	u8 num_of_buttons[2];
> +	u8 num_of_image_rows[2];
> +	u8 num_of_image_cols[2];
> +	u8 has_hybrid_data[2];
> +} __packed;
> +
> +struct tcm_report_config_prop {
> +	u8 id; /* TOUCH_OBJECT_* */
> +	u8 bits; /* Size of the field in bits */
> +};
> +
> +struct tcm_report_config_entry {
> +	u8 foreach; /* TOUCH_FOREACH_* (and maybe other things?) */
> +	int n_props;
> +	const struct tcm_report_config_prop *props;
> +};
> +
> +struct tcm_report_config {
> +	int n_entries;
> +	const struct tcm_report_config_entry *entries;
> +};
> +
> +struct tcm_data {
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +	struct input_dev *input;
> +	struct touchscreen_properties prop;
> +	struct gpio_desc *reset_gpio;
> +	struct completion response;
> +	struct regulator_bulk_data supplies[2];
> +
> +	/* annoying state */
> +	u16 buf_size;
> +	char buf[256];
> +};
> +
> +static int tcm_send_cmd(struct tcm_data *tcm, struct tcm_cmd *cmd)
> +{
> +	struct i2c_client *client = tcm->client;
> +	struct i2c_msg msg;
> +	int ret;
> +
> +	dev_dbg(&client->dev, "sending command %#x\n", cmd->cmd);
> +
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +	msg.len = 1 + cmd->length;
> +	msg.buf = (u8 *)cmd;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret == 1)
> +		return 0;
> +	else if (ret < 0)
> +		return ret;
> +	else
> +		return -EIO;
> +}
> +
> +static int tcm_send_cmd_noargs(struct tcm_data *tcm, u8 cmd)
> +{
> +	struct tcm_cmd c = {
> +		.cmd = cmd,
> +		.length = 0,
> +	};
> +
> +	return tcm_send_cmd(tcm, &c);
> +}
> +
> +static int tcm_recv_report(struct tcm_data *tcm,
> +			   void *buf, size_t length)
> +{
> +	struct i2c_client *client = tcm->client;
> +	struct i2c_msg msg;
> +	int ret;
> +
> +	msg.addr = client->addr;
> +	msg.flags = I2C_M_RD;
> +	msg.len = length;
> +	msg.buf = buf;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret == 1)
> +		return 0;
> +	else if (ret < 0)
> +		return ret;
> +	else
> +		return -EIO;
> +}
> +
> +static int tcm_read_message(struct tcm_data *tcm, u8 cmd, void *buf, size_t length)
> +{
> +	int ret;
> +
> +	reinit_completion(&tcm->response);
> +	ret = tcm_send_cmd_noargs(tcm, cmd);
> +	if (ret)
> +		return ret;
> +
> +	ret = wait_for_completion_timeout(&tcm->response, msecs_to_jiffies(1000));
> +	if (ret == 0)
> +		return -ETIMEDOUT;
> +
> +	if (buf) {
> +		if (length > tcm->buf_size) {
> +			dev_warn(&tcm->client->dev, "expected %zu bytes, got %u\n",
> +				 length, tcm->buf_size);
> +		}
> +		length = min(tcm->buf_size, length);
> +		memcpy(buf, tcm->buf, length);
> +	}
> +
> +	return 0;
> +}
> +
> +static void tcm_power_off(void *data)
> +{
> +	struct tcm_data *tcm = data;
> +
> +	disable_irq(tcm->client->irq);
> +	regulator_bulk_disable(ARRAY_SIZE(tcm->supplies), tcm->supplies);
> +}
> +
> +static int tcm_input_open(struct input_dev *dev)
> +{
> +	struct tcm_data *tcm = input_get_drvdata(dev);
> +
> +	return i2c_smbus_write_byte(tcm->client, TCM_ENABLE_REPORT);
> +}
> +
> +static void tcm_input_close(struct input_dev *dev)
> +{
> +	struct tcm_data *tcm = input_get_drvdata(dev);
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte(tcm->client, TCM_DISABLE_REPORT);
> +	if (ret)
> +		dev_err(&tcm->client->dev, "failed to turn off sensing\n");
> +}
> +
> +/*
> + * The default report config looks like this:
> + *
> + * a5 01 80 00 11 08 1e 08 0f 01 04 01 06 04 07 04
> + * 08 0c 09 0c 0a 08 0b 08 0c 08 0d 10 0e 10 03 00
> + * 00 00
> + *
> + * a5 01 80 00 - HEADER + length
> + *
> + * 11 08 - TOUCH_FRAME_RATE (8 bits)
> + * 30 08 - UNKNOWN (8 bits)
> + * 0f 01 - TOUCH_0D_BUTTONS_STATE (1 bit)
> + * 04 01 - TOUCH_PAD_TO_NEXT_BYTE (7 bits - padding)
> + * 06 04 - TOUCH_OBJECT_N_INDEX (4 bits)
> + * 07 04 - TOUCH_OBJECT_N_CLASSIFICATION (4 bits)
> + * 08 0c - TOUCH_OBJECT_N_X_POSITION (12 bits)
> + * 09 0c - TOUCH_OBJECT_N_Y_POSITION (12 bits)
> + * 0a 08 - TOUCH_OBJECT_N_Z (8 bits)
> + * 0b 08 - TOUCH_OBJECT_N_X_WIDTH (8 bits)
> + * 0c 08 - TOUCH_OBJECT_N_Y_WIDTH (8 bits)
> + * 0d 10 - TOUCH_OBJECT_N_TX_POSITION_TIXELS (16 bits) ??
> + * 0e 10 - TOUCH_OBJECT_N_RX_POSITION_TIXELS (16 bits) ??
> + * 03 00 - TOUCH_FOREACH_END (0 bits)
> + * 00 00 - TOUCH_END (0 bits)
> + *
> + * Since we only support this report config, we just hardcode the format below.
> + * To support additional report configs, we would need to parse the config and
> + * use it to parse the reports dynamically.
> + */
> +
> +struct tcm_default_report_data {
> +	u8 fps;
> +	struct {
> +		u8 unknown;
> +		u8 buttons;
> +		u8 idx : 4;
> +		u8 classification : 4;
> +		u16 x : 12;
> +		u16 y : 12;

This will be a mess on big endian. Please no bitfields. Use normally
sized fields (u8, u16, etc) and masks/shifts to get data.

> +		u8 z;
> +		u8 width_x;
> +		u8 width_y;
> +		u8 tx;
> +		u8 rx;
> +	} __packed points[];
> +} __packed;
> +
> +static int tcm_handle_touch_report(struct tcm_data *tcm, char *buf, size_t len)
> +{
> +	struct tcm_default_report_data *data;
> +
> +	buf += REPORT_PEAK_LEN;
> +	len -= REPORT_PEAK_LEN;
> +
> +	dev_dbg(&tcm->client->dev, "touch report len %zu\n", len);
> +	if ((len - 1) % 11)
> +		dev_err(&tcm->client->dev, "invalid touch report length\n");
> +
> +	data = (struct tcm_default_report_data *)buf;
> +
> +	/* We don't need to report releases because we have INPUT_MT_DROP_UNUSED */
> +	for (int i = 0; i < (len - 1) / 11; i++) {
> +		u8 major_width, minor_width;
> +
> +		minor_width = data->points[i].width_x;
> +		major_width = data->points[i].width_y;
> +
> +		if (minor_width > major_width)
> +			swap(major_width, minor_width);
> +
> +		dev_dbg(&tcm->client->dev, "touch report: idx %u x %u y %u\n",
> +			data->points[i].idx, data->points[i].x, data->points[i].y);
> +		input_mt_slot(tcm->input, data->points[i].idx);
> +		input_mt_report_slot_state(tcm->input, MT_TOOL_FINGER, true);
> +
> +		input_report_abs(tcm->input, ABS_MT_POSITION_X, data->points[i].x);
> +		input_report_abs(tcm->input, ABS_MT_POSITION_Y, data->points[i].y);

touchscreen_report_pos(...) instead of the 2 above so that rotation/axis
swap will be handled properly.

> +		input_report_abs(tcm->input, ABS_MT_TOUCH_MAJOR, major_width);
> +		input_report_abs(tcm->input, ABS_MT_TOUCH_MINOR, minor_width);
> +		input_report_abs(tcm->input, ABS_MT_PRESSURE, data->points[i].z);
> +	}
> +
> +	input_mt_sync_frame(tcm->input);
> +	input_sync(tcm->input);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t tcm_report_irq(int irq, void *data)
> +{
> +	struct tcm_data *tcm = data;
> +	struct tcm_message_header *header;
> +	char buf[256];
> +	u16 len;
> +	int ret;
> +
> +	header = (struct tcm_message_header *)buf;
> +	ret = tcm_recv_report(tcm, buf, sizeof(buf));
> +	if (ret) {
> +		dev_err(&tcm->client->dev, "failed to read report: %d\n", ret);
> +		return IRQ_HANDLED;
> +	}
> +
> +	switch (header->code) {
> +	case REPORT_OK:
> +	case REPORT_IDENTIFY:
> +	case REPORT_TOUCH:
> +	case REPORT_DELTA:
> +	case REPORT_RAW:
> +	case REPORT_DEBUG:
> +	case REPORT_TOUCH_HOLD:
> +		break;
> +	default:
> +		dev_dbg(&tcm->client->dev, "Ignoring report %#x\n", header->code);
> +		return IRQ_HANDLED;
> +	}
> +
> +	/* Not present for REPORT_CONTINUED_READ */
> +	len = get_unaligned_le16(buf + sizeof(*header));
> +
> +	dev_dbg(&tcm->client->dev, "report %#x len %u\n", header->code, len);
> +	print_hex_dump_bytes("report: ", DUMP_PREFIX_OFFSET, buf,
> +			     min(sizeof(buf), len + sizeof(*header)));
> +
> +	if (len > sizeof(buf) - sizeof(*header)) {
> +		dev_err(&tcm->client->dev, "report too long\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	/* Check if this is a read response or an indication. For indications
> +	 * (user touched the screen) we just parse the report directly.
> +	 */
> +	if (completion_done(&tcm->response) && header->code == REPORT_TOUCH) {
> +		tcm_handle_touch_report(tcm, buf, len + sizeof(*header));
> +		return IRQ_HANDLED;
> +	}
> +
> +	tcm->buf_size = len + sizeof(*header);
> +	memcpy(tcm->buf, buf, len + sizeof(*header));
> +	complete(&tcm->response);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int tcm_hw_init(struct tcm_data *tcm, u16 *max_x, u16 *max_y)
> +{
> +	int ret;
> +	struct tcm_identification id = { 0 };
> +	struct tcm_app_info app_info = { 0 };
> +
> +	/*
> +	 * Tell the firmware to start up. After starting it sends an IDENTIFY report, which
> +	 * we treat like a response to this message even though it's technically a new report.
> +	 */
> +	ret = tcm_read_message(tcm, TCM_RUN_APPLICATION_FIRMWARE, &id, sizeof(id));
> +	if (ret) {
> +		dev_err(&tcm->client->dev, "failed to identify device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_dbg(&tcm->client->dev, "Synaptics TCM %s v%d mode %d\n",
> +		id.part_number, id.version, id.mode);
> +	if (id.mode != MODE_APPLICATION) {
> +		/* We don't support firmware updates or anything else */
> +		dev_err(&tcm->client->dev, "Device is not in application mode\n");
> +		return -ENODEV;
> +	}
> +
> +	do {
> +		msleep(20);
> +		ret = tcm_read_message(tcm, TCM_GET_APPLICATION_INFO, &app_info, sizeof(app_info));
> +		if (ret) {
> +			dev_err(&tcm->client->dev, "failed to get application info: %d\n", ret);
> +			return ret;
> +		}
> +	} while (app_info.status == APP_STATUS_BOOTING || app_info.status == APP_STATUS_UPDATING);
> +
> +	dev_dbg(&tcm->client->dev, "Application firmware v%d.%d (customer '%s') status %d\n",
> +		 app_info.version[0], app_info.version[1], app_info.customer_config_id,
> +		 app_info.status);
> +
> +	*max_x = app_info.max_x;
> +	*max_y = app_info.max_y;
> +
> +	return 0;
> +}
> +
> +static int tcm_power_on(struct tcm_data *tcm)
> +{
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(tcm->supplies),
> +				    tcm->supplies);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value(tcm->reset_gpio, false);
> +	usleep_range(10000, 11000);
> +	gpiod_set_value(tcm->reset_gpio, true);

Here you have reset GPIO asserted, which means that the controller will
stay in reset state indefinitely.

gpiod API operates on logical states (active/inactive) and performs
conversion to physical state (LOW/HIGH) internally.

Also we should use gpiod_set_value_cansleep() unless we are in atomic
context.

> +	usleep_range(80000, 81000);
> +
> +	return 0;
> +}
> +
> +static int tcm_probe(struct i2c_client *client)
> +{
> +	struct tcm_data *tcm;
> +	struct tcm_report_config __maybe_unused report_config;

It is definitely not used, there no "maybe" about it.

> +	u16 max_x, max_y;
> +	int ret;

Call this and similar variables holding error code "error" please.

> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> +						I2C_FUNC_SMBUS_BYTE_DATA |
> +						I2C_FUNC_SMBUS_I2C_BLOCK))
> +		return -ENODEV;
> +
> +	tcm = devm_kzalloc(&client->dev, sizeof(struct tcm_data), GFP_KERNEL);

sizeof(*tcm)

> +	if (!tcm)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, tcm);
> +	tcm->client = client;
> +
> +	init_completion(&tcm->response);
> +
> +	tcm->supplies[0].supply = "vdd";
> +	tcm->supplies[1].supply = "vcc";
> +	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(tcm->supplies),
> +				      tcm->supplies);
> +	if (ret)
> +		return ret;
> +
> +	tcm->reset_gpio = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);

Do all designs require GPIO line be specified? Can it be made optional?

> +
> +	ret = devm_add_action_or_reset(&client->dev, tcm_power_off,
> +				       tcm);
> +	if (ret)
> +		return ret;
> +
> +	ret = tcm_power_on(tcm);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					tcm_report_irq,
> +					IRQF_TRIGGER_LOW | IRQF_ONESHOT,

Please do not hardcode trigger (besides IRQF_ONESHOT), let platform
decide.

Also interrupt is hot there, are you sure we will not try to report
input events for not-yet-allocated input device if someone is thouching
the device at this point?

> +					"synaptics_tcm_report", tcm);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = tcm_hw_init(tcm, &max_x, &max_y);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to initialize hardware\n");
> +		return ret;
> +	}
> +
> +	tcm->input = devm_input_allocate_device(&client->dev);
> +	if (!tcm->input)
> +		return -ENOMEM;
> +
> +	tcm->input->name = "Synaptics TCM Oncell Touchscreen";
> +	tcm->input->id.bustype = BUS_I2C;
> +	tcm->input->open = tcm_input_open;
> +	tcm->input->close = tcm_input_close;
> +
> +	input_set_abs_params(tcm->input, ABS_MT_POSITION_X, 0, max_x, 0, 0);
> +	input_set_abs_params(tcm->input, ABS_MT_POSITION_Y, 0, max_y, 0, 0);
> +	input_set_abs_params(tcm->input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +	input_set_abs_params(tcm->input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
> +	input_set_abs_params(tcm->input, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> +	touchscreen_parse_properties(tcm->input, true, &tcm->prop);
> +
> +	ret = input_mt_init_slots(tcm->input, 10, INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> +	if (ret)
> +		return ret;
> +
> +	input_set_drvdata(tcm->input, tcm);
> +
> +	ret = input_register_device(tcm->input);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id syna_driver_ids[] = {
> +	{
> +		.compatible = "syna,s3908",
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, syna_driver_ids);
> +
> +static const struct i2c_device_id syna_i2c_ids[] = {
> +	{ "synaptics-tcm", 0 },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, syna_i2c_ids);
> +
> +static struct i2c_driver syna_i2c_driver = {
> +	.probe	= tcm_probe,
> +	.id_table	= syna_i2c_ids,
> +	.driver	= {
> +	.name	= "synaptics-tcm",
> +	.of_match_table = syna_driver_ids,
> +	},
> +};
> +
> +module_i2c_driver(syna_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Frieder Hannenheim <frieder.hannenheim@xxxxxxxxx>");
> +MODULE_AUTHOR("Caleb Connolly <caleb@xxxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("A driver for Synaptics TCM Oncell Touchpanels");
> +
> 
> -- 
> 2.45.0
> 

-- 
Dmitry



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux