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@xxxxxxxxx>
> > ---
> > 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
