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/Kconfig b/drivers/thermal/Kconfig
index b10080d618604ddd90295bff973e337ae0509059..1c4a5cd5a23ee0a608bdb626427ce60d6624a532 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -472,6 +472,8 @@ endmenu
source "drivers/thermal/renesas/Kconfig"
+source "drivers/thermal/spacemit/Kconfig"
+
source "drivers/thermal/tegra/Kconfig"
config GENERIC_ADC_THERMAL
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index bb21e7ea7fc6b70aa84e5fed7cfdc7096e3fb1f7..3b249195c088efd453efcc92a95c2241d6f4d882 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -65,6 +65,7 @@ obj-y += mediatek/
obj-$(CONFIG_GENERIC_ADC_THERMAL) += thermal-generic-adc.o
obj-$(CONFIG_UNIPHIER_THERMAL) += uniphier_thermal.o
obj-$(CONFIG_AMLOGIC_THERMAL) += amlogic_thermal.o
+obj-y += spacemit/
obj-$(CONFIG_SPRD_THERMAL) += sprd_thermal.o
obj-$(CONFIG_KHADAS_MCU_FAN_THERMAL) += khadas_mcu_fan.o
obj-$(CONFIG_LOONGSON2_THERMAL) += loongson2_thermal.o
diff --git a/drivers/thermal/spacemit/Kconfig b/drivers/thermal/spacemit/Kconfig
new file mode 100644
index 0000000000000000000000000000000000000000..fbfd22f8fcd3554288a6991b67cbb0c2272ecf76
--- /dev/null
+++ b/drivers/thermal/spacemit/Kconfig
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0-only
+menu "SpacemiT thermal drivers"
+depends on ARCH_SPACEMIT || COMPILE_TEST
+
+config SPACEMIT_K1_TSENSOR
+ tristate "SpacemiT K1 thermal sensor driver"
+ depends on ARCH_SPACEMIT || COMPILE_TEST
+ help
+ This driver provides support for the thermal sensor
+ integrated in the SpacemiT K1 SoC.
+
+ The thermal sensor monitors temperatures for five thermal zones:
+ soc, package, gpu, cluster0, and cluster1. It supports reporting
+ temperature values and handling high/low threshold interrupts.
+
+ Say Y here if you want to enable thermal monitoring on SpacemiT K1.
+ If compiled as a module, it will be called k1_tsensor.
+
+endmenu
diff --git a/drivers/thermal/spacemit/Makefile b/drivers/thermal/spacemit/Makefile
new file mode 100644
index 0000000000000000000000000000000000000000..82b30741e4ecfc320af6b6eacdc3ebff5ab2e10d
--- /dev/null
+++ b/drivers/thermal/spacemit/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_SPACEMIT_K1_TSENSOR) += k1_tsensor.o
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
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Thermal sensor driver for SpacemiT K1 SoC
+ *
+ * Copyright (C) 2025 Shuwei Wu <shuweiwoo@163.com>
+ */
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/thermal.h>
+
+#include "../thermal_hwmon.h"
+
+#define K1_TSENSOR_PCTRL_REG 0x00
+#define K1_TSENSOR_PCTRL_ENABLE BIT(0)
+#define K1_TSENSOR_PCTRL_TEMP_MODE BIT(3)
+#define K1_TSENSOR_PCTRL_RAW_SEL BIT(7)
+
+#define K1_TSENSOR_PCTRL_CTUNE GENMASK(11, 8)
+#define K1_TSENSOR_PCTRL_SW_CTRL GENMASK(21, 18)
+#define K1_TSENSOR_PCTRL_HW_AUTO_MODE BIT(23)
+
+#define K1_TSENSOR_EN_REG 0x08
+#define K1_TSENSOR_EN_ALL GENMASK(MAX_SENSOR_NUMBER - 1, 0)
+
+#define K1_TSENSOR_TIME_REG 0x0C
+#define K1_TSENSOR_TIME_WAIT_REF_CNT GENMASK(3, 0)
+#define K1_TSENSOR_TIME_ADC_CNT_RST GENMASK(7, 4)
+#define K1_TSENSOR_TIME_FILTER_PERIOD GENMASK(21, 20)
+#define K1_TSENSOR_TIME_MASK GENMASK(23, 0)
+
+#define K1_TSENSOR_INT_CLR_REG 0x10
+#define K1_TSENSOR_INT_EN_REG 0x14
+#define K1_TSENSOR_INT_STA_REG 0x18
+
+#define K1_TSENSOR_INT_EN_MASK BIT(0)
+#define K1_TSENSOR_INT_MASK(x) (GENMASK(2, 1) << ((x) * 2))
+
+#define K1_TSENSOR_DATA_BASE_REG 0x20
+#define K1_TSENSOR_DATA_REG(x) (K1_TSENSOR_DATA_BASE_REG + ((x) / 2) * 4)
+#define K1_TSENSOR_DATA_LOW_MASK GENMASK(15, 0)
+#define K1_TSENSOR_DATA_HIGH_MASK GENMASK(31, 16)
+
+#define K1_TSENSOR_THRSH_BASE_REG 0x40
+#define K1_TSENSOR_THRSH_REG(x) (K1_TSENSOR_THRSH_BASE_REG + ((x) * 4))
+#define K1_TSENSOR_THRSH_LOW_MASK GENMASK(15, 0)
+#define K1_TSENSOR_THRSH_HIGH_MASK GENMASK(31, 16)
+
+#define MAX_SENSOR_NUMBER 5
+
+/* Hardware offset value required for temperature calculation */
+#define TEMPERATURE_OFFSET 278
+
+struct k1_tsensor_channel {
+ struct k1_tsensor *ts;
+ struct thermal_zone_device *tzd;
+ int id;
+};
+
+struct k1_tsensor {
+ void __iomem *base;
+ struct k1_tsensor_channel ch[MAX_SENSOR_NUMBER];
+};
+
+static void k1_tsensor_init(struct k1_tsensor *ts)
+{
+ 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;
+ writel(val, ts->base + K1_TSENSOR_TIME_REG);
+
+ /*
+ * Enable all sensors' auto mode, enable dither control,
+ * consecutive mode, and power up sensor.
+ */
+ val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
+ val &= ~K1_TSENSOR_PCTRL_SW_CTRL;
+ val &= ~K1_TSENSOR_PCTRL_CTUNE;
+ val |= K1_TSENSOR_PCTRL_RAW_SEL |
+ K1_TSENSOR_PCTRL_TEMP_MODE |
+ K1_TSENSOR_PCTRL_HW_AUTO_MODE |
+ K1_TSENSOR_PCTRL_ENABLE;
+ writel(val, ts->base + K1_TSENSOR_PCTRL_REG);
+
+ /* Enable thermal interrupt */
+ val = readl(ts->base + K1_TSENSOR_INT_EN_REG);
+ val |= K1_TSENSOR_INT_EN_MASK;
+ writel(val, ts->base + K1_TSENSOR_INT_EN_REG);
+
+ /* Enable each sensor */
+ val = readl(ts->base + K1_TSENSOR_EN_REG);
+ val |= K1_TSENSOR_EN_ALL;
+ writel(val, ts->base + K1_TSENSOR_EN_REG);
+}
+
+static void k1_tsensor_enable_irq(struct k1_tsensor_channel *ch)
+{
+ struct k1_tsensor *ts = ch->ts;
+ u32 val;
+
+ val = readl(ts->base + K1_TSENSOR_INT_CLR_REG);
+ val |= K1_TSENSOR_INT_MASK(ch->id);
+ writel(val, ts->base + K1_TSENSOR_INT_CLR_REG);
+
+ val = readl(ts->base + K1_TSENSOR_INT_EN_REG);
+ val &= ~K1_TSENSOR_INT_MASK(ch->id);
+ writel(val, ts->base + K1_TSENSOR_INT_EN_REG);
+}
+
+/*
+ * The conversion formula used is:
+ * T(m°C) = (((raw_value & mask) >> shift) - TEMPERATURE_OFFSET) * 1000
+ */
+static int k1_tsensor_get_temp(struct thermal_zone_device *tz, int *temp)
+{
+ struct k1_tsensor_channel *ch = thermal_zone_device_priv(tz);
+ struct k1_tsensor *ts = ch->ts;
+ u32 val;
+
+ val = readl(ts->base + K1_TSENSOR_DATA_REG(ch->id));
+ if (ch->id % 2)
+ *temp = FIELD_GET(K1_TSENSOR_DATA_HIGH_MASK, val);
+ else
+ *temp = FIELD_GET(K1_TSENSOR_DATA_LOW_MASK, val);
+
+ *temp -= TEMPERATURE_OFFSET;
+ *temp *= 1000;
+
+ return 0;
+}
+
+/*
+ * For each sensor, the hardware threshold register is 32 bits:
+ * - Lower 16 bits [15:0] configure the low threshold temperature.
+ * - Upper 16 bits [31:16] configure the high threshold temperature.
+ */
+static int k1_tsensor_set_trips(struct thermal_zone_device *tz, int low, int high)
+{
+ struct k1_tsensor_channel *ch = thermal_zone_device_priv(tz);
+ struct k1_tsensor *ts = ch->ts;
+ int high_code = high;
+ int low_code = low;
+ u32 val;
+
+ if (low >= high)
+ return -EINVAL;
+
+ if (low < 0)
+ low_code = 0;
+
+ high_code = high_code / 1000 + TEMPERATURE_OFFSET;
+ low_code = low_code / 1000 + TEMPERATURE_OFFSET;
+
+ val = readl(ts->base + K1_TSENSOR_THRSH_REG(ch->id));
+ val &= ~K1_TSENSOR_THRSH_HIGH_MASK;
+ val |= FIELD_PREP(K1_TSENSOR_THRSH_HIGH_MASK, high_code);
+
+ val &= ~K1_TSENSOR_THRSH_LOW_MASK;
+ val |= FIELD_PREP(K1_TSENSOR_THRSH_LOW_MASK, low_code);
+ writel(val, ts->base + K1_TSENSOR_THRSH_REG(ch->id));
+
+ return 0;
+}
+
+static const struct thermal_zone_device_ops k1_tsensor_ops = {
+ .get_temp = k1_tsensor_get_temp,
+ .set_trips = k1_tsensor_set_trips,
+};
+
+static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
+{
+ struct k1_tsensor *ts = (struct k1_tsensor *)data;
+ int mask, status, i;
+
+ status = readl(ts->base + K1_TSENSOR_INT_STA_REG);
+
+ for (i = 0; i < MAX_SENSOR_NUMBER; i++) {
+ if (status & K1_TSENSOR_INT_MASK(i)) {
+ mask = readl(ts->base + K1_TSENSOR_INT_CLR_REG);
+ mask |= K1_TSENSOR_INT_MASK(i);
+ writel(mask, ts->base + K1_TSENSOR_INT_CLR_REG);
+ thermal_zone_device_update(ts->ch[i].tzd, THERMAL_EVENT_UNSPECIFIED);
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int k1_tsensor_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct k1_tsensor *ts;
+ struct reset_control *reset;
+ struct clk *clk, *bus_clk;
+ int i, irq, ret;
+
+ ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+ if (!ts)
+ return -ENOMEM;
+
+ ts->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(ts->base))
+ return dev_err_probe(dev, PTR_ERR(ts->base), "Failed to get reg\n");
+
+ reset = devm_reset_control_get_exclusive_deasserted(dev, NULL);
+ if (IS_ERR(reset))
+ return dev_err_probe(dev, PTR_ERR(reset), "Failed to get/deassert reset control\n");
+
+ clk = devm_clk_get_enabled(dev, "core");
+ if (IS_ERR(clk))
+ return dev_err_probe(dev, PTR_ERR(clk), "Failed to get core clock\n");
+
+ bus_clk = devm_clk_get_enabled(dev, "bus");
+ if (IS_ERR(bus_clk))
+ return dev_err_probe(dev, PTR_ERR(bus_clk), "Failed to get bus clock\n");
+
+ k1_tsensor_init(ts);
+
+ 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);
+
+ /* 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;
+
+ ret = devm_request_threaded_irq(dev, irq, NULL,
+ k1_tsensor_irq_thread,
+ IRQF_ONESHOT, "k1_tsensor", ts);
+ if (ret < 0)
+ return ret;
+
+ platform_set_drvdata(pdev, ts);
+
+ return 0;
+}
+
+static const struct of_device_id k1_tsensor_dt_ids[] = {
+ { .compatible = "spacemit,k1-tsensor" },
+ { /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, k1_tsensor_dt_ids);
+
+static struct platform_driver k1_tsensor_driver = {
+ .driver = {
+ .name = "k1_tsensor",
+ .of_match_table = k1_tsensor_dt_ids,
+ },
+ .probe = k1_tsensor_probe,
+};
+module_platform_driver(k1_tsensor_driver);
+
+MODULE_DESCRIPTION("SpacemiT K1 Thermal Sensor Driver");
+MODULE_AUTHOR("Shuwei Wu <shuweiwoo@163.com>");
+MODULE_LICENSE("GPL");
--
2.52.0
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
> @@ -0,0 +1,283 @@
...
> +/*
> + * For each sensor, the hardware threshold register is 32 bits:
> + * - Lower 16 bits [15:0] configure the low threshold temperature.
> + * - Upper 16 bits [31:16] configure the high threshold temperature.
> + */
> +static int k1_tsensor_set_trips(struct thermal_zone_device *tz, int low, int high)
> +{
> + struct k1_tsensor_channel *ch = thermal_zone_device_priv(tz);
> + struct k1_tsensor *ts = ch->ts;
> + int high_code = high;
> + int low_code = low;
Since high_code and low_code are used to bit operations, please define
them as unsigned types. u32 would be pretty fine here.
Also, you could avoid the initialization of high_code and low_code
here...
> + u32 val;
> +
> + if (low >= high)
> + return -EINVAL;
> +
> + if (low < 0)
> + low_code = 0;
... if you change this if to
if (low < 0)
low = 0;
...
> + high_code = high_code / 1000 + TEMPERATURE_OFFSET;
> + low_code = low_code / 1000 + TEMPERATURE_OFFSET;
... and re-write the right side of the assignments to use high/low
instead.
> + val = readl(ts->base + K1_TSENSOR_THRSH_REG(ch->id));
> + val &= ~K1_TSENSOR_THRSH_HIGH_MASK;
> + val |= FIELD_PREP(K1_TSENSOR_THRSH_HIGH_MASK, high_code);
> +
> + val &= ~K1_TSENSOR_THRSH_LOW_MASK;
> + val |= FIELD_PREP(K1_TSENSOR_THRSH_LOW_MASK, low_code);
> + writel(val, ts->base + K1_TSENSOR_THRSH_REG(ch->id));
> +
> + return 0;
> +}
...
> +static int k1_tsensor_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct k1_tsensor *ts;
> + struct reset_control *reset;
> + struct clk *clk, *bus_clk;
You could drop bus_clk here, and re-use clk to retrieve the return value
of devm_clk_get_enabled(dev, "bus"), which also saves you some
characters.
...
> + clk = devm_clk_get_enabled(dev, "core");
> + if (IS_ERR(clk))
> + return dev_err_probe(dev, PTR_ERR(clk), "Failed to get core clock\n");
clk isn't used anywhere later, so overriding its value is fine.
> + bus_clk = devm_clk_get_enabled(dev, "bus");
> + if (IS_ERR(bus_clk))
> + return dev_err_probe(dev, PTR_ERR(bus_clk), "Failed to get bus clock\n");
> +
> + return PTR_ERR(ts->ch[i].tzd);
Regards,
Yao Zi
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
|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|
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
© 2016 - 2025 Red Hat, Inc.