Hi Dmitry, On Wed, Jul 03, 2024 at 01:47:58PM -0700, Dmitry Torokhov wrote: > 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. > Ack, > > + 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. > After testing on the machine, we found that this method is only invoked during system shutdown. Since the device doesn't require any special operations at shutdown, so I think it can be removed. > > +} > > + > > +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? > Ack, calling this first seems better. Thanks, Charles