Hi Charles,
On Tue, Jun 18, 2024 at 04:44:53PM +0800, Charles Wang wrote:
> This patch introduces a new driver to support the Goodix GT7986U
> touch controller. This device is not compatible with Microsoft's
> HID-over-SPI protocol and therefore needs to implement its own
> flavor. 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)
This looks reasonable good, so:
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
A couple of suggestions/nits below:
> +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;
> +
If you do:
ts->event_buf = devm_kmalloc(dev, <some nominal size>, GFP_KERNEL);
here and use devm_krealloc() once you figure the true size of event
buffer you can stop calling kfree() by hand on remove.
> + mutex_init(&ts->hid_request_lock);
> + 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 = device_property_read_u32(dev, "goodix,hid-report-addr",
> + &ts->hid_report_addr);
> + if (error)
> + return dev_err_probe(dev, error,
> + "failed get hid report addr\n");
> +
> + error = goodix_dev_confirm(ts);
> + if (error)
> + return error;
> +
> + /* Waits 150ms for firmware to fully boot */
> + msleep(GOODIX_NORMAL_RESET_DELAY_MS);
> +
> + error = goodix_hid_init(ts);
> + if (error) {
> + dev_err(dev, "failed init hid device");
> + return error;
> + }
> +
> + error = devm_request_threaded_irq(&ts->spi->dev, ts->spi->irq,
> + NULL, goodix_hid_irq, IRQF_ONESHOT,
> + "goodix_spi_hid", ts);
> + if (error) {
> + dev_err(ts->dev, "could not register interrupt, irq = %d, %d",
> + ts->spi->irq, error);
> + goto err_destroy_hid;
> + }
> +
> + return 0;
> +
> +err_destroy_hid:
> + hid_destroy_device(ts->hid);
> + return error;
> +}
> +
> +static void goodix_spi_remove(struct spi_device *spi)
> +{
> + struct goodix_ts_data *ts = spi_get_drvdata(spi);
> +
> + disable_irq(spi->irq);
> + hid_destroy_device(ts->hid);
> + kfree(ts->event_buf);
> +}
> +
> +static void goodix_spi_shutdown(struct spi_device *spi)
> +{
> + struct goodix_ts_data *ts = spi_get_drvdata(spi);
> +
> + disable_irq(spi->irq);
> + hid_destroy_device(ts->hid);
Do we really need shutdown() method? It is very rarely that it is
needed.
> +}
> +
> +static int goodix_spi_set_power(struct goodix_ts_data *ts, int power_state)
> +{
> + u8 power_control_cmd[] = {0x00, 0x00, 0x00, 0x87, 0x02, 0x00, 0x08};
> + int error;
> +
> + /* value 0 for power on, 1 for power sleep */
> + power_control_cmd[5] = power_state;
> +
> + guard(mutex)(&ts->hid_request_lock);
> + error = goodix_spi_write(ts, ts->hid_report_addr, power_control_cmd,
> + sizeof(power_control_cmd));
> + if (error) {
> + dev_err(ts->dev, "failed set power mode: %s",
> + power_state == GOODIX_SPI_POWER_ON ? "on" : "sleep");
> + return error;
> + }
> + return 0;
> +}
> +
> +static int goodix_spi_suspend(struct device *dev)
> +{
> + struct goodix_ts_data *ts = dev_get_drvdata(dev);
> + int error;
> +
> + error = goodix_spi_set_power(ts, GOODIX_SPI_POWER_SLEEP);
> + disable_irq(ts->spi->irq);
Can disable_irq() be called first?
Thanks.
--
Dmitry
