linux-kernel - Re: [PATCH v5 1/2] HID: hid-goodix: Add Goodix HID-over-SPI driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZrtXwr6HWF64akUP@ux-UP-WHL01>
Date: Tue, 13 Aug 2024 20:58:50 +0800
From: Charles Wang <charles.goodix@...il.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: dan.carpenter@...aro.org, conor@...nel.org, robh@...nel.org,
	dianders@...omium.org, krzk+dt@...nel.org, jikos@...nel.org,
	bentiss@...nel.org, hbarnor@...omium.org,
	linux-input@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/2] HID: hid-goodix: Add Goodix HID-over-SPI driver

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@...il.com>
> 
> 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