Message-ID: <Zl5kz_S4FJaGzfiW@google.com>
Date: Mon, 3 Jun 2024 17:50:23 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Charles Wang <charles.goodix@...il.com>
Cc: jikos@...nel.org, bentiss@...nel.org, hbarnor@...omium.org,
	dianders@...omium.org, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] HID: hid-goodix: Add Goodix HID-over-SPI driver

Hi Charles,

> This patch introduces a new driver to support the Goodix GT7986U
> touch controller. The data reported is packaged according to the
> HID protocol but uses SPI for communication to improve speed. This
> enables the device to transmit not only coordinate data but also
> corresponding raw data that can be accessed by user-space programs
> through the hidraw interface. The raw data can be utilized for
> functions like palm rejection, thereby improving the touch experience.
> 
> Key features:
> - Device connection confirmation and initialization
> - IRQ-based event reporting to the input subsystem
> - Support for HIDRAW operations (GET_REPORT and SET_REPORT)

Can you  please mention in the patch description that this device is not
compatible with Microsoft's HID-over-SPI protocol and therefore needs to
implement its own flavor.

> 
> Signed-off-by: Charles Wang <charles.goodix@...il.com>
> ---
> Changes in v2:
> - Fixed build warnings reported by kernel test robot
> ---
>  drivers/hid/Kconfig      |   6 +
>  drivers/hid/Makefile     |   1 +
>  drivers/hid/hid-goodix.c | 652 +++++++++++++++++++++++++++++++++++++++

Do you have similar i2c parts that are not compatible with MS
HID-over-I2C, and if you do have them do you have plans to add support
for them to this driver? If not maybe call this hid-goodix-spi.c?
Or maybe create drivers/hid/spi-hid/hid-goodix.c to separate HID upper
layer drivers from the HID low layer/transport drivers?


>  3 files changed, 659 insertions(+)
>  create mode 100644 drivers/hid/hid-goodix.c
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 4c682c650..f57d8fb88 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -404,6 +404,12 @@ config HID_VIVALDI_COMMON
>  	  option so that drivers can use common code to parse the HID
>  	  descriptors for vivaldi function row keymap.
>  
> +config HID_GOODIX
> +	tristate "Goodix GT7986U SPI HID touchscreen"
> +	depends on SPI_MASTER
> +	help
> +	  Support for Goodix GT7986U SPI HID touchscreen device.
> +
>  config HID_GOOGLE_HAMMER
>  	tristate "Google Hammer Keyboard"
>  	select HID_VIVALDI_COMMON
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 082a728ea..4e799f7e5 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_HID_GEMBIRD)	+= hid-gembird.o
>  obj-$(CONFIG_HID_GFRM)		+= hid-gfrm.o
>  obj-$(CONFIG_HID_GLORIOUS)  += hid-glorious.o
>  obj-$(CONFIG_HID_VIVALDI_COMMON) += hid-vivaldi-common.o
> +obj-$(CONFIG_HID_GOODIX)	+= hid-goodix.o
>  obj-$(CONFIG_HID_GOOGLE_HAMMER)	+= hid-google-hammer.o
>  obj-$(CONFIG_HID_GOOGLE_STADIA_FF)	+= hid-google-stadiaff.o
>  obj-$(CONFIG_HID_VIVALDI)	+= hid-vivaldi.o
> diff --git a/drivers/hid/hid-goodix.c b/drivers/hid/hid-goodix.c
> new file mode 100644
> index 000000000..a67f7d9ef
> --- /dev/null
> +++ b/drivers/hid/hid-goodix.c
> @@ -0,0 +1,652 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Goodix GT7986U SPI Driver Code for HID.
> + *
> + * Copyright (C) 2024 Godix, Inc.
> + */
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/hid.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/sizes.h>
> +#include <linux/spi/spi.h>
> +
> +#define GOODIX_DEV_CONFIRM_ADDR		0x10000
> +#define GOODIX_HID_DESC_ADDR		0x1058C
> +#define GOODIX_HID_REPORT_DESC_ADDR	0x105AA
> +#define GOODIX_HID_SIGN_ADDR		0x10D32
> +#define GOODIX_HID_REPORT_ADDR		0x22C8C

I wonder if some if not all of these should come from DT/device
properties.

> +
> +#define GOODIX_HID_GET_REPORT_CMD	0x02
> +#define GOODIX_HID_SET_REPORT_CMD	0x03
> +
> +#define GOODIX_HID_MAX_INBUF_SIZE	128
> +#define GOODIX_HID_ACK_READY_FLAG	0x01
> +#define GOODIX_HID_REPORT_READY_FLAG	0x80
> +
> +#define GOODIX_DEV_CONFIRM_VAL		0xAA
> +
> +#define GOODIX_SPI_WRITE_FLAG		0xF0
> +#define GOODIX_SPI_READ_FLAG		0xF1
> +#define GOODIX_SPI_TRANS_PREFIX_LEN	1
> +#define GOODIX_REGISTER_WIDTH		4
> +#define GOODIX_SPI_READ_DUMMY_LEN	3
> +#define GOODIX_SPI_READ_PREFIX_LEN	(GOODIX_SPI_TRANS_PREFIX_LEN + \
> +					 GOODIX_REGISTER_WIDTH + \
> +					 GOODIX_SPI_READ_DUMMY_LEN)
> +#define GOODIX_SPI_WRITE_PREFIX_LEN	(GOODIX_SPI_TRANS_PREFIX_LEN + \
> +					 GOODIX_REGISTER_WIDTH)
> +
> +#define GOODIX_CHECKSUM_SIZE		sizeof(u16)
> +#define GOODIX_NORMAL_RESET_DELAY_MS	150
> +
> +struct goodix_hid_report_header {
> +	u8 flag;
> +	__le16 size;
> +} __packed;
> +#define GOODIX_HID_ACK_HEADER_SIZE	sizeof(struct goodix_hid_report_header)
> +
> +struct goodix_hid_report_package {
> +	__le16 size;
> +	u8 data[];
> +};
> +
> +#define GOODIX_HID_PKG_LEN_SIZE		sizeof(u16)
> +#define GOODIX_HID_COOR_DATA_LEN	82
> +#define GOODIX_HID_COOR_PKG_LEN		(GOODIX_HID_PKG_LEN_SIZE + \
> +					 GOODIX_HID_COOR_DATA_LEN)
> +
> +#define GOODIX_REPORT_DATA_ADDR		(GOODIX_HID_REPORT_ADDR + \
> +					 GOODIX_HID_ACK_HEADER_SIZE + \
> +					 GOODIX_HID_PKG_LEN_SIZE)
> +
> +struct goodix_hid_report_event {
> +	struct goodix_hid_report_header hdr;
> +	u8 data[GOODIX_HID_COOR_PKG_LEN];
> +} __packed;
> +
> +struct goodix_hid_desc {
> +	__le16 desc_length;
> +	__le16 bcd_version;
> +	__le16 report_desc_lenght;
> +	__le16 report_desc_register;
> +	__le16 input_register;
> +	__le16 max_input_length;
> +	__le16 output_register;
> +	__le16 max_output_length;
> +	__le16 cmd_register;
> +	__le16 data_register;
> +	__le16 vendor_id;
> +	__le16 product_id;
> +	__le16 version_id;
> +	__le32 reserved;
> +} __packed;
> +
> +struct goodix_ts_data {
> +	struct device *dev;
> +	struct spi_device *spi;
> +	struct hid_device *hid;
> +	struct goodix_hid_desc hid_desc;
> +
> +	struct gpio_desc *reset_gpio;
> +
> +	/* Buffer used to store hid report data */
> +	u8 xfer_buf[SZ_4K];

Maybe have it as ____cacheline_aligned to allow SPI controller to DMA to
it directly. 

> +};
> +
> +static int goodix_spi_read(struct goodix_ts_data *ts, u32 addr,
> +			   u8 *data, unsigned int len)
> +{
> +	struct spi_device *spi = to_spi_device(&ts->spi->dev);
> +	struct spi_transfer xfers;
> +	struct spi_message spi_msg;
> +	u8 *buf;
> +	int error;
> +
> +	buf = kzalloc(GOODIX_SPI_READ_PREFIX_LEN + len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;

Can you try using ts->xfer_buf without making allocations and copies?
Maybe have goodix_spi_read() have data as u8 **data, and do

	*data = buf + GOODIX_SPI_READ_PREFIX_LEN;
	return 0;

at the end. I.e. callers do not supply buffer but rather are given one.
Of course you need to make sure there are no concurrent calls to
goodix_spi_read(), but I do not think you have them anyways.


> +
> +	spi_message_init(&spi_msg);
> +	memset(&xfers, 0, sizeof(xfers));
> +
> +	/* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
> +	buf[0] = GOODIX_SPI_READ_FLAG;
> +	put_unaligned_be32(addr, buf + GOODIX_SPI_TRANS_PREFIX_LEN);
> +	memset(buf + GOODIX_SPI_TRANS_PREFIX_LEN + GOODIX_REGISTER_WIDTH,
> +	       0xff, GOODIX_SPI_READ_DUMMY_LEN);

Does the "data" have to be set to 0xff?

> +
> +	xfers.tx_buf = buf;
> +	xfers.rx_buf = buf;
> +	xfers.len = GOODIX_SPI_READ_PREFIX_LEN + len;
> +	xfers.cs_change = 0;
> +	spi_message_add_tail(&xfers, &spi_msg);
> +
> +	error = spi_sync(spi, &spi_msg);
> +	if (error)
> +		dev_err(ts->dev, "spi transfer error:%d", error);
> +	else
> +		memcpy(data, buf + GOODIX_SPI_READ_PREFIX_LEN, len);
> +
> +	kfree(buf);
> +	return error;
> +}
> +
> +static int goodix_spi_write(struct goodix_ts_data *ts, u32 addr,
> +			    u8 *data, unsigned int len)
> +{
> +	struct spi_device *spi = to_spi_device(&ts->spi->dev);
> +	struct spi_transfer xfers;
> +	struct spi_message spi_msg;
> +	u8 *buf;
> +	int error;
> +
> +	buf = kzalloc(GOODIX_SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;

Same comments here as for goodix_spi_write()...

> +
> +	spi_message_init(&spi_msg);
> +	memset(&xfers, 0, sizeof(xfers));
> +
> +	/* buffer format: 0xF0 + addr(4bytes) + data */
> +	buf[0] = GOODIX_SPI_WRITE_FLAG;
> +	put_unaligned_be32(addr, buf + GOODIX_SPI_TRANS_PREFIX_LEN);
> +	memcpy(buf + GOODIX_SPI_WRITE_PREFIX_LEN, data, len);
> +
> +	xfers.tx_buf = buf;
> +	xfers.len = GOODIX_SPI_WRITE_PREFIX_LEN + len;
> +	xfers.cs_change = 0;
> +	spi_message_add_tail(&xfers, &spi_msg);
> +
> +	error = spi_sync(spi, &spi_msg);
> +	if (error)
> +		dev_err(ts->dev, "spi transfer error:%d", error);
> +
> +	kfree(buf);
> +	return error;
> +}
> +
> +static int goodix_dev_confirm(struct goodix_ts_data *ts)
> +{
> +	u8 tx_buf[8], rx_buf[8];
> +	int retry = 3;
> +	int error;
> +
> +	gpiod_set_value_cansleep(ts->reset_gpio, 0);
> +	usleep_range(4000, 4100);
> +
> +	memset(tx_buf, GOODIX_DEV_CONFIRM_VAL, sizeof(tx_buf));
> +	while (retry--) {
> +		error = goodix_spi_write(ts, GOODIX_DEV_CONFIRM_ADDR,
> +					 tx_buf, sizeof(tx_buf));
> +		if (error)
> +			return error;
> +
> +		error = goodix_spi_read(ts, GOODIX_DEV_CONFIRM_ADDR,
> +					rx_buf, sizeof(rx_buf));
> +		if (error)
> +			return error;
> +
> +		if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
> +			return 0;
> +
> +		usleep_range(5000, 5100);
> +	}
> +
> +	dev_err(ts->dev, "device confirm failed, rx_buf:%*ph", 8, rx_buf);
> +	return -EINVAL;
> +}
> +
> +/**
> + * goodix_hid_parse() - hid-core .parse() callback
> + * @hid: hid device instance
> + *
> + * This function gets called during call to hid_add_device
> + *
> + * Return: 0 on success and non zero on error
> + */
> +static int goodix_hid_parse(struct hid_device *hid)
> +{
> +	struct goodix_ts_data *ts = hid->driver_data;
> +	u16 rsize;
> +	u8 *rdesc;
> +	int error;
> +
> +	rsize = le16_to_cpu(ts->hid_desc.report_desc_lenght);
> +	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
> +		dev_err(ts->dev, "invalid report desc size %d", rsize);
> +		return -EINVAL;
> +	}
> +
> +	rdesc = kzalloc(rsize, GFP_KERNEL);

We now have nifty

	u8 *rdesc __free(kfree) = kzalloc(rsize, GFP_KERNEL);

(see include/linux/cleanup.h) and you do not need to free memory by
hand.

> +	if (!rdesc)
> +		return -ENOMEM;
> +
> +	error = goodix_spi_read(ts, GOODIX_HID_REPORT_DESC_ADDR, rdesc, rsize);
> +	if (error) {
> +		dev_err(ts->dev, "failed get report desc, %d", error);
> +		goto free_mem;
> +	}
> +
> +	error = hid_parse_report(hid, rdesc, rsize);
> +	if (error)
> +		dev_err(ts->dev, "failed parse report, %d", error);
> +
> +free_mem:
> +	kfree(rdesc);
> +	return error;
> +}
> +
> +/* Empty callbacks with success return code */

Hmm, I see you are using falling edge interrupt. Don't you have concern
of having it "stuck" here? I do not think all these should be stubs...

Does the device have low power mode that can be used when controller is
not in use (inhibited for example)?

> +static int goodix_hid_start(struct hid_device *hid)
> +{
> +	return 0;
> +}
> +
> +static void goodix_hid_stop(struct hid_device *hid)
> +{
> +}
> +
> +static int goodix_hid_open(struct hid_device *hid)
> +{
> +	return 0;
> +}
> +
> +static void goodix_hid_close(struct hid_device *hid)
> +{
> +}
> +
> +/* Return date length of response data */
> +static int goodix_hid_check_ack_status(struct goodix_ts_data *ts)
> +{
> +	struct goodix_hid_report_header hdr;
> +	int retry = 20;
> +	int error;
> +
> +	while (retry--) {
> +		/*
> +		 * 3 bytes of hid request response data
> +		 * - byte 0:    Ack flag, value of 1 for data ready
> +		 * - bytes 1-2: Response data length
> +		 */
> +		error = goodix_spi_read(ts, GOODIX_HID_REPORT_ADDR,
> +					(u8 *)&hdr, sizeof(hdr));
> +		if (!error && (hdr.flag & GOODIX_HID_ACK_READY_FLAG))
> +			return le16_to_cpu(hdr.size);
> +
> +		/* Wait 10ms for another try */
> +		usleep_range(10000, 11000);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +/**
> + * goodix_hid_get_raw_report() - Process hidraw GET REPORT operation
> + * @hid: hid device instance
> + * @reportnum: Report ID
> + * @buf: Buffer for store the reprot date
> + * @len: Length fo reprot data
> + * @report_type: Report type
> + *
> + * The function for hid_ll_driver.get_raw_report to handle the HIDRAW ioctl
> + * get report request. The transmitted data follows the standard i2c-hid
> + * protocol with a specified header.
> + *
> + * Return: The length of the data in the buf on success, negative error code
> + */
> +static int goodix_hid_get_raw_report(struct hid_device *hid,
> +				     unsigned char reportnum,
> +				     __u8 *buf, size_t len,
> +				     unsigned char report_type)
> +{
> +	struct goodix_ts_data *ts = hid->driver_data;
> +	u16 data_register = le16_to_cpu(ts->hid_desc.data_register);
> +	u16 cmd_register = le16_to_cpu(ts->hid_desc.cmd_register);
> +	u8 tmp_buf[GOODIX_HID_MAX_INBUF_SIZE];
> +	int tx_len = 0, args_len = 0;
> +	int response_data_len;
> +	u8 args[3];
> +	int error;
> +
> +	if (report_type == HID_OUTPUT_REPORT)
> +		return -EINVAL;
> +
> +	if (reportnum == 3) {
> +		/* Get win8 signature data */
> +		error = goodix_spi_read(ts, GOODIX_HID_SIGN_ADDR, buf, len);
> +		if (error) {
> +			dev_err(ts->dev, "failed get win8 sign:%d", error);
> +			return -EINVAL;
> +		}
> +		return len;
> +	}
> +
> +	if (reportnum >= 0x0F) {
> +		args[args_len++] = reportnum;
> +		reportnum = 0x0F;
> +	}
> +	put_unaligned_le16(data_register, args + args_len);
> +	args_len += sizeof(data_register);
> +
> +	/* Clean 3 bytes of hid ack header data */
> +	memset(tmp_buf, 0, GOODIX_HID_ACK_HEADER_SIZE);
> +	tx_len += GOODIX_HID_ACK_HEADER_SIZE;
> +
> +	put_unaligned_le16(cmd_register, tmp_buf + tx_len);
> +	tx_len += sizeof(cmd_register);
> +
> +	tmp_buf[tx_len++] = ((report_type == HID_FEATURE_REPORT ? 0x03 : 0x01) << 4) | reportnum;
> +	tmp_buf[tx_len++] = GOODIX_HID_GET_REPORT_CMD;
> +
> +	memcpy(tmp_buf + tx_len, args, args_len);
> +	tx_len += args_len;
> +
> +	/* Step1: write report request info */
> +	error = goodix_spi_write(ts, GOODIX_HID_REPORT_ADDR, tmp_buf, tx_len);
> +	if (error) {
> +		dev_err(ts->dev, "failed send read feature cmd, %d", error);
> +		return error;
> +	}
> +
> +	/* No need read response data */
> +	if (!len)
> +		return 0;
> +
> +	/* Step2: check response data status */
> +	response_data_len = goodix_hid_check_ack_status(ts);
> +	if (response_data_len <= 0)
> +		return -EINVAL;
> +
> +	/* Step3: read response data(skip 2bytes of hid pkg length) */
> +	error = goodix_spi_read(ts, GOODIX_REPORT_DATA_ADDR, buf,
> +				response_data_len - GOODIX_HID_PKG_LEN_SIZE);
> +	if (error) {
> +		dev_err(ts->dev, "failed read hid response data, %d", error);
> +		return error;
> +	}
> +
> +	return response_data_len - GOODIX_HID_PKG_LEN_SIZE;
> +}
> +
> +/**
> + * goodix_hid_set_raw_report() - process hidraw SET REPORT operation
> + * @hid: HID device
> + * @reportnum: Report ID
> + * @buf: Buffer for communication
> + * @len: Length of data in the buffer
> + * @report_type: Report type
> + *
> + * The function for hid_ll_driver.get_raw_report to handle the HIDRAW ioctl
> + * set report request. The transmitted data follows the standard i2c-hid
> + * protocol with a specified header.
> + *
> + * Return: The length of the data sent, negative error code on failure
> + */
> +static int goodix_hid_set_raw_report(struct hid_device *hid,
> +				     unsigned char reportnum,
> +				     __u8 *buf, size_t len,
> +				     unsigned char report_type)
> +{
> +	struct goodix_ts_data *ts = hid->driver_data;
> +	u16 data_register = le16_to_cpu(ts->hid_desc.data_register);
> +	u16 cmd_register = le16_to_cpu(ts->hid_desc.cmd_register);
> +	int tx_len = 0, args_len = 0;
> +	u8 tmp_buf[GOODIX_HID_MAX_INBUF_SIZE];
> +	u8 args[5];
> +	int error;
> +
> +	if (reportnum >= 0x0F) {
> +		args[args_len++] = reportnum;
> +		reportnum = 0x0F;
> +	}
> +
> +	put_unaligned_le16(data_register, args + args_len);
> +	args_len += sizeof(data_register);
> +
> +	put_unaligned_le16(GOODIX_HID_PKG_LEN_SIZE + len, args + args_len);
> +	args_len += GOODIX_HID_PKG_LEN_SIZE;
> +
> +	/* Clean 3 bytes of hid ack header data */
> +	memset(tmp_buf, 0, GOODIX_HID_ACK_HEADER_SIZE);
> +	tx_len += GOODIX_HID_ACK_HEADER_SIZE;
> +
> +	put_unaligned_le16(cmd_register, tmp_buf + tx_len);
> +	tx_len += sizeof(cmd_register);
> +
> +	tmp_buf[tx_len++] = ((report_type == HID_FEATURE_REPORT ? 0x03 : 0x02) << 4) | reportnum;
> +	tmp_buf[tx_len++] = GOODIX_HID_SET_REPORT_CMD;
> +
> +	memcpy(tmp_buf + tx_len, args, args_len);
> +	tx_len += args_len;
> +
> +	memcpy(tmp_buf + tx_len, buf, len);
> +	tx_len += len;
> +
> +	error = goodix_spi_write(ts, GOODIX_HID_REPORT_ADDR, tmp_buf, tx_len);
> +	if (error) {
> +		dev_err(ts->dev, "failed send report %*ph", tx_len, tmp_buf);
> +		return error;
> +	}
> +	return len;
> +}
> +
> +static int goodix_hid_raw_request(struct hid_device *hid,
> +				  unsigned char reportnum,
> +				  __u8 *buf, size_t len,
> +				  unsigned char rtype, int reqtype)
> +{
> +	switch (reqtype) {
> +	case HID_REQ_GET_REPORT:
> +		return goodix_hid_get_raw_report(hid, reportnum, buf,
> +						 len, rtype);
> +	case HID_REQ_SET_REPORT:
> +		if (buf[0] != reportnum)
> +			return -EINVAL;
> +		return goodix_hid_set_raw_report(hid, reportnum, buf,
> +						 len, rtype);
> +	default:
> +		return -EIO;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static struct hid_ll_driver goodix_hid_ll_driver = {
> +	.parse = goodix_hid_parse,
> +	.start = goodix_hid_start,
> +	.stop = goodix_hid_stop,
> +	.open = goodix_hid_open,
> +	.close = goodix_hid_close,
> +	.raw_request = goodix_hid_raw_request
> +};
> +
> +static irqreturn_t goodix_hid_irq(int irq, void *data)
> +{
> +	struct goodix_ts_data *ts = data;
> +	struct goodix_hid_report_event event;
> +	struct goodix_hid_report_package *pkg;
> +	u16 report_size;
> +	int error;
> +
> +	/*
> +	 * First, read buffer with space for header and coordinate package:
> +	 * - event header = 3 bytes
> +	 * - coordinate event = GOODIX_HID_COOR_PKG_LEN bytes
> +	 *
> +	 * If the data size info in the event header exceeds
> +	 * GOODIX_HID_COOR_PKG_LEN, it means that there are other packages
> +	 * besides the coordinate package.
> +	 */
> +	error = goodix_spi_read(ts, GOODIX_HID_REPORT_ADDR, (u8 *)&event,
> +				sizeof(event));
> +	if (error) {
> +		dev_err(ts->dev, "failed get coordinate data, %d", error);
> +		return IRQ_HANDLED;
> +	}
> +
> +	/* Check coordinate data valid falg */
> +	if (event.hdr.flag != GOODIX_HID_REPORT_READY_FLAG) {
> +		dev_err(ts->dev, "invalid event flag 0x%x", event.hdr.flag);
> +		return IRQ_HANDLED;
> +	}
> +
> +	pkg = (struct goodix_hid_report_package *)event.data;
> +	hid_input_report(ts->hid, HID_INPUT_REPORT, pkg->data,
> +			 le16_to_cpu(pkg->size) - GOODIX_HID_PKG_LEN_SIZE, 1);
> +
> +	report_size = le16_to_cpu(event.hdr.size);
> +	/* Check if there are other packages */
> +	if (report_size <= GOODIX_HID_COOR_PKG_LEN)
> +		return IRQ_HANDLED;
> +
> +	if (report_size - GOODIX_HID_COOR_PKG_LEN > sizeof(ts->xfer_buf)) {
> +		dev_err(ts->dev, "invalid package size, %d", report_size);
> +		return IRQ_HANDLED;
> +	}
> +
> +	/* Read the package behind the coordinate data */
> +	error = goodix_spi_read(ts, GOODIX_HID_REPORT_ADDR + sizeof(event),
> +				ts->xfer_buf,
> +				report_size - GOODIX_HID_COOR_PKG_LEN);
> +	if (error) {
> +		dev_err(ts->dev, "failed read data, %d", error);
> +		return IRQ_HANDLED;
> +	}
> +
> +	pkg = (struct goodix_hid_report_package *)ts->xfer_buf;
> +	hid_input_report(ts->hid, HID_INPUT_REPORT, pkg->data,
> +			 le16_to_cpu(pkg->size) - GOODIX_HID_PKG_LEN_SIZE, 1);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int goodix_hid_init(struct goodix_ts_data *ts)
> +{
> +	struct hid_device *hid;
> +	int error;
> +
> +	/* Get hid descriptor */
> +	error = goodix_spi_read(ts, GOODIX_HID_DESC_ADDR, (u8 *)&ts->hid_desc,
> +				sizeof(ts->hid_desc));
> +	if (error) {
> +		dev_err(ts->dev, "failed get hid desc, %d", error);
> +		return error;
> +	}
> +
> +	hid = hid_allocate_device();
> +	if (IS_ERR(hid))
> +		return PTR_ERR(hid);
> +
> +	hid->driver_data = ts;
> +	hid->ll_driver = &goodix_hid_ll_driver;
> +	hid->bus = BUS_SPI;
> +	hid->dev.parent = &ts->spi->dev;
> +
> +	hid->version = le16_to_cpu(ts->hid_desc.bcd_version);
> +	hid->vendor = le16_to_cpu(ts->hid_desc.vendor_id);
> +	hid->product = le16_to_cpu(ts->hid_desc.product_id);
> +	snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X", "hid-gdix",
> +		 hid->vendor, hid->product);
> +
> +	error = hid_add_device(hid);
> +	if (error) {
> +		dev_err(ts->dev, "failed add hid device, %d", error);
> +		hid_destroy_device(hid);
> +		return error;
> +	}
> +
> +	ts->hid = hid;
> +	return 0;
> +}
> +
> +static int goodix_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct goodix_ts_data *ts;
> +	int error;
> +
> +	/* init spi_device */
> +	spi->mode            = SPI_MODE_0;
> +	spi->bits_per_word   = 8;
> +	error = spi_setup(spi);
> +	if (error)
> +		return error;
> +
> +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> +	if (!ts)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, ts);
> +	ts->spi = spi;
> +	ts->dev = dev;
> +	ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ts->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(ts->reset_gpio),
> +				     "Failed to request reset gpio\n");
> +
> +	error = goodix_dev_confirm(ts);
> +	if (error)
> +		return error;
> +
> +	/* Waits 150ms for firmware to fully boot */
> +	msleep(GOODIX_NORMAL_RESET_DELAY_MS);
> +
> +	error = devm_request_threaded_irq(&ts->spi->dev, ts->spi->irq,
> +					  NULL, goodix_hid_irq,
> +					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					  "goodix_spi_hid", ts);
> +	if (error < 0) {
> +		dev_err(ts->dev, "could not register interrupt, irq = %d, %d",
> +			ts->spi->irq, error);
> +		return error;
> +	}

I do not think it is safe. Your interrupt is hot here, but you are
allocating and registering HID device instance in goodix_hid_init(). If
interrupt arrives right away you will likely crash.

> +
> +	error = goodix_hid_init(ts);
> +	if (error) {
> +		dev_err(dev, "failed init hid device");
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static void goodix_spi_remove(struct spi_device *spi)
> +{
> +	struct goodix_ts_data *ts = spi_get_drvdata(spi);
> +
> +	hid_destroy_device(ts->hid);

This is not safe either, you destroy the device, but interrupts are
enabled and nothing stops them from coming...

> +}
> +
> +static void goodix_spi_shutdown(struct spi_device *spi)
> +{
> +	struct goodix_ts_data *ts = spi_get_drvdata(spi);
> +
> +	disable_irq_nosync(spi->irq);

Why nosync? Seems dangerous. Please add a comment why nosync is needed
and why it is safe.

> +	hid_destroy_device(ts->hid);
> +}
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id goodix_spi_acpi_match[] = {
> +	{ "GXTS7986" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, goodix_spi_acpi_match);
> +#endif
> +
> +static struct spi_driver goodix_spi_driver = {
> +	.driver = {
> +		.name = "goodix-spi-hid",
> +		.acpi_match_table = ACPI_PTR(goodix_spi_acpi_match),
> +	},
> +	.probe =	goodix_spi_probe,
> +	.remove =	goodix_spi_remove,
> +	.shutdown =	goodix_spi_shutdown,
> +};
> +module_spi_driver(goodix_spi_driver);
> +
> +MODULE_DESCRIPTION("Goodix SPI driver for HID touchscreen");
> +MODULE_AUTHOR("Goodix, Inc.");
> +MODULE_LICENSE("GPL");
> -- 
> 2.43.0
> 
> 

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists