Re: [PATCH v2 1/5] HID: intel-thc-hid: intel-ipts: bring up the port

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] HID: intel-thc-hid: intel-ipts: set idev->running in
  thc_ipts_port_start()
--

commit 603b533dc41071ba0e983a0108dc8c85c96dd22a
Author: Emfox Zhou <emfox@debian.org>

HID: intel-thc-hid: intel-ipts: bring up the port

This commit adds a PCI driver for Intel Touch Host Controller ports in
IPTS mode that brings up the SPI port, reads the controller configuration
block, configures SPI timing, and enables the touch engine.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> new file mode 100644
> index 0000000000000..f139b8f9443d0
> --- /dev/null
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c

[ ... ]

> +int thc_ipts_port_start(struct thc_ipts *idev)
> +{

[ ... ]

> +	ret = thc_ipts_touch_set(idev, true);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/**
> + * thc_ipts_port_stop - tear one port down
> + * @idev: port to stop
> + */
> +void thc_ipts_port_stop(struct thc_ipts *idev)
> +{
> +	if (!idev->thc)
> +		return;
> +
> +	if (idev->running)
> +		thc_ipts_touch_set(idev, false);
> +
> +	idev->running = false;
> +	regmap_clear_bits(idev->thc->thc_regmap,
> +			  THC_M_PRT_CONTROL_OFFSET,
> +			  THC_M_PRT_CONTROL_DEVRST);
> +	thc_ipts_quiesce(idev, true);
> +}

[Severity: Medium]
Should thc_ipts_port_start() set idev->running to true after
thc_ipts_touch_set(idev, true) succeeds?  Without that, idev->running is
never set in this commit, so thc_ipts_port_stop() skips disabling and
powering down the touch engine before asserting hardware reset on driver
removal or shutdown.  Note that a later patch in the series, commit
"HID: intel-thc-hid: intel-ipts: receive reports", adds this assignment.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114170740.2068179-1-emfox@debian.org?part=1