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

[wayne <shuweiwoo@163.com>]

|On 2025/12/16 12:16, |Yao Zi wrote:

> 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.

Thank you for pointing this out, and I apologize for not addressing your 
question

about the 24MHz clock earlier.

In v1, I referenced the vendor's implementation, though their device tree

did not specify this clock for the thermal node.

After your review, I revisited the SpacemiT K1 clock tree published by 
the vendor,

and found that TSENSOR relies only on the APBC clock, which in turn is 
ultimately

sourced from the 24MHz crystal via the PLL.

Disabling the 24MHz clock for the syscon_apbc node in the device tree 
had no impact

on TSENSOR operation in my testing, so I did not include it in the binding.

As for the PCTRL2 configuration, I confirmed that its default value 
after reset is zero,

and changing its configuration had no effect on the temperature sensor's 
behavior.

This led me to remove the PCTRL2 configuration code in v2.

>> +	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?

In v1, the reviewer mentioned that it is no need to print extra error 
message.

See:

https://lore.kernel.org/spacemit/20251127225848-GYA1797866@gentoo.org/T/#mc335bea36323d2d8b3afb09aa40c9c7160440d39 

>> +
>> +		/* 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.
Ditto.
>> +	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.
Ditto.
> Besides these questions, the driver itself looks pretty nice to me :)
>
> Best regards,
> Yao Zi

|Please let me know if you need further details or test results. Thank 
you for reviewing my patch. Best regards, Shuwei Wu|


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv