Re: [PATCH v2 2/3] thermal: spacemit: k1: Add thermal sensor support

[Yao Zi <me@ziyao.cc>]

On Tue, Dec 16, 2025 at 10:00:36AM +0800, Shuwei Wu wrote:
> The thermal sensor on K1 supports monitoring five temperature zones.
> The driver registers these sensors with the thermal framework
> and supports standard operations:
> - Reading temperature (millidegree Celsius)
> - Setting high/low thresholds for interrupts
> 
> Signed-off-by: Shuwei Wu <shuweiwoo@163.com>
> ---
> Changes in v2:
> - Rename k1_thermal.c to k1_tsensor.c for better hardware alignment
> - Move driver to drivers/thermal/spacemit/
> - Add Kconfig/Makefile for spacemit and update top-level build files
> - Refactor names, style, code alignment, and comments
> - Simplify probe and error handling
> ---
>  drivers/thermal/Kconfig               |   2 +
>  drivers/thermal/Makefile              |   1 +
>  drivers/thermal/spacemit/Kconfig      |  19 +++
>  drivers/thermal/spacemit/Makefile     |   3 +
>  drivers/thermal/spacemit/k1_tsensor.c | 283 ++++++++++++++++++++++++++++++++++
>  5 files changed, 308 insertions(+)
> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..f164754e807ddd311c8cf98bcc074fd580514aa2
> --- /dev/null
> +++ b/drivers/thermal/spacemit/k1_tsensor.c

...

> +static void k1_tsensor_init(struct k1_tsensor *ts)
> +{

Configuration of K1_TSU_PCTRL2 (offset 0x04) is removed in this
revision, but why? Isn't it necessary for the sensor to function?

And you didn't ask my question raised in v1 about the source of 24MHz
clock. I still suspect whether the binding is complete or not.

> +	u32 val;
> +
> +	/* Disable all the interrupts */
> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
> +
> +	/* Configure ADC sampling time and filter period */
> +	val = readl(ts->base + K1_TSENSOR_TIME_REG);
> +	val &= ~K1_TSENSOR_TIME_MASK;
> +	val |= K1_TSENSOR_TIME_FILTER_PERIOD |
> +		K1_TSENSOR_TIME_ADC_CNT_RST |
> +		K1_TSENSOR_TIME_WAIT_REF_CNT;

It's more natural to align K1_TSENSOR_TIME_ADC_CNT_RST and other
following constants with K1_TSENSOR_TIME_FILTER_PERIOD. This applies for
other multiple-line assignments, too.

...

> +static int k1_tsensor_probe(struct platform_device *pdev)
> +{

...

> +	for (i = 0; i < MAX_SENSOR_NUMBER; ++i) {
> +		ts->ch[i].id = i;
> +		ts->ch[i].ts = ts;
> +		ts->ch[i].tzd = devm_thermal_of_zone_register(dev, i, ts->ch + i, &k1_tsensor_ops);
> +		if (IS_ERR(ts->ch[i].tzd))
> +			return PTR_ERR(ts->ch[i].tzd);

Would emitting a error message with dev_err_probe() help here?

> +
> +		/* Attach sysfs hwmon attributes for userspace monitoring */
> +		ret = devm_thermal_add_hwmon_sysfs(dev, ts->ch[i].tzd);
> +		if (ret)
> +			dev_warn(dev, "Failed to add hwmon sysfs attributes\n");
> +
> +		k1_tsensor_enable_irq(ts->ch + i);
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;

Same as the above question.

> +	ret = devm_request_threaded_irq(dev, irq, NULL,
> +					k1_tsensor_irq_thread,
> +					IRQF_ONESHOT, "k1_tsensor", ts);
> +	if (ret < 0)
> +		return ret;

Same as above.

Besides these questions, the driver itself looks pretty nice to me :)

Best regards,
Yao Zi