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

[Yao Zi <me@ziyao.cc>]

On Tue, Dec 16, 2025 at 05:31:06PM +0800, wayne wrote:
> |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.

Thanks, this is a reasonable answer to me.

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

Oops, yeah, I definitely read this link before, but forgot it. So
keeping it as-is is okay.

> > > +
> > > +		/* 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|
> 
> 

Regards,
Yao Zi