Re: [PATCH v3 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: divisor 3 misses 41MHz gear
  in thc_ipts_spi_from_cfg()
--

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

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

This commit adds a PCI port driver for Intel Touch Host Controller ports in
IPTS mode, reading the controller configuration block over PIO and applying
SPI timing to enable 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..264479630ff6e
> --- /dev/null
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
[ ... ]
> +static void thc_ipts_spi_from_cfg(struct thc_ipts *idev, u32 *hz, u32 *mode)
> +{
> +	u32 cfg = le32_to_cpu(idev->cfg.spi_cfg);
> +	u32 div = THC_IPTS_SPI_CLKDIV(cfg);
> +
> +	if (!div) {
> +		/* Nothing valid asks for this; use the slowest gear. */
> +		dev_warn_once(idev->dev, "controller asks for divisor 0\n");
> +		*hz = THC_IPTS_SPI_MIN_HZ;
> +	} else {
> +		*hz = THC_IPTS_SPI_BASE_HZ / div;
> +		if (cfg & THC_IPTS_SPI_CLKDIV8)
> +			*hz /= THC_IPTS_SPI_CLKDIV8_DIV;
> +	}
> +
> +	if (*hz < THC_IPTS_SPI_MIN_HZ) {
> +		dev_warn_once(idev->dev,
> +			      "controller asks for %u Hz, slowest is %u Hz\n",
> +			      *hz, THC_IPTS_SPI_MIN_HZ);
> +		*hz = THC_IPTS_SPI_MIN_HZ;
> +	}
[ ... ]
> +int thc_ipts_port_start(struct thc_ipts *idev)
> +{
[ ... ]
> +	thc_ipts_spi_from_cfg(idev, &hz, &mode);
> +	ret = thc_ipts_spi_set(idev, hz, mode);
> +	if (ret) {
> +		dev_warn(idev->dev,
> +			 "cannot follow the SPI timing, going slow\n");
> +		ret = thc_ipts_spi_set(idev, THC_IPTS_BOOT_SPI_HZ,
> +				       THC_SINGLE_IO);
> +		if (ret)
> +			return ret;
> +	} else {
> +		dev_dbg(idev->dev, "SPI clock asked for %u Hz, rounded down\n",
> +			hz);
> +	}

[Severity: Medium]
Does THC_IPTS_SPI_BASE_HZ / div miss the 41 MHz gear when the controller
requests divisor 3?

With THC_IPTS_SPI_BASE_HZ set to 125000000, integer division by 3 yields
41666666 Hz, which is 34 Hz below THC_SPI_FREQUENCY_41M (41666700):

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c:thc_spi_read_config() {
    ...
	/* 10M, 35M and 50M CLK need 1.5, 3.5 and 2.5 half divider */
	if ((freq_div == THC_SPI_FRQ_DIV_2 && spi_freq_val >=  THC_SPI_FREQUENCY_50M) ||
		(freq_div == THC_SPI_FRQ_DIV_3 && spi_freq_val <  THC_SPI_FREQUENCY_41M) ||
		(freq_div == THC_SPI_FRQ_DIV_1 && spi_freq_val <  THC_SPI_FREQUENCY_15M)) {
		regmap_write_bits(dev->thc_regmap, THC_M_PRT_SPI_DUTYC_CFG_OFFSET,
				  THC_M_PRT_SPI_DUTYC_CFG_SPI_TCRF_HALF_DIV_EN,
				  THC_M_PRT_SPI_DUTYC_CFG_SPI_TCRF_HALF_DIV_EN);

		regmap_write_bits(dev->thc_regmap, THC_M_PRT_SPARE_REG_OFFSET,
				  THC_M_PRT_SPARE_REG_SPI_CLK_INV_ENABLE,
				  THC_M_PRT_SPARE_REG_SPI_CLK_INV_ENABLE);
    ...
}

Because 41666666 < THC_SPI_FREQUENCY_41M, thc_spi_read_config() and
thc_spi_write_config() program the 3.5 half-divider (35.7 MHz) with SPI
clock inversion enabled instead of plain divisor 3.

Also, because thc_ipts_spi_from_cfg() clamps *hz to at least
THC_IPTS_SPI_MIN_HZ (7812500), thc_get_spi_freq_div_val() never returns
THC_SPI_FRQ_RESERVED, making the error fallback in thc_ipts_port_start()
unreachable.

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