Date: Tue, 4 Jun 2024 19:23:59 +0800
From: Charles Wang <charles.goodix@...il.com>
To: Dmitry Torokhov <dmitry.torokhov@...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 Dmitry,

On Mon, Jun 03, 2024 at 05:50:23PM -0700, Dmitry Torokhov wrote:
> 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.
> 
Ack,

> > 
> > 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?
>

No, all Goodix I2C devices will comply with the MS HID-over-I2C standard,
so there's no need to support them in this driver. Renaming the file to
hid-goodix-spi.c seems better. I belive the path drivers/hid/spi-hid/hid-goodix.c
would be more appropriate for future MS HID-over-SPI compatible devices.

> 
> >  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.
>

Thanks, I will move the addresses that might change in the future
to be obtained from DT/device properties, to enhance the adaptability
of the driver.

> > +
> > +#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. 
>

Ack.

> > +};
> > +
> > +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.
>

Unfortunately, there are concurrent calls to goodix_spi_read(). The functions
goodix_hid_get_raw_report() and goodix_hid_irq() may execute concurrently.

Anyways, I will try to optimize this part and reduce the malloc operations
where possible.

> 
> > +
> > +	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?
>

Rechecked the datasheet the dummy value are no requirement.

> > +
> > +	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()...
> 

Ack.

> > +
> > +	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.
> 

Ack, this great.

> > +	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...
>
Thank you for pointing this out. The trigger method shouldn't be fixed
within the driver. As for "stuck", I believe this issue does not exit.
The firmware won't wait for the host's response.

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

The low power control function is currently being tested and will be
updated later.

> > +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.
> 

Thank you for identifying this issue. You are right; Will address this
problem in the next version.

> > +
> > +	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...
> 


Thanks a lot, will fix this in the next version.

After rereading the kernel source, I realized that the devres release
are called after driver remove function.

> > +}
> > +
> > +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.
> 

Sorry, I just copied this from another place without deep thinking.
disable_irq seems more suitable here.

> > +	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

Charles

Powered by blists - more mailing lists