Re: [PATCH 2/3] thermal: K1: Add driver for K1 SoC thermal sensor

Re: [PATCH 2/3] thermal: K1: Add driver for K1 SoC thermal sensor

[Yao Zi]

On Thu, Nov 27, 2025 at 02:44:08AM +0800, Shuwei Wu wrote:
> The thermal sensor unit (TSU) 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>
> ---
>  drivers/thermal/Kconfig      |  14 ++
>  drivers/thermal/Makefile     |   1 +
>  drivers/thermal/k1_thermal.c | 307 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 322 insertions(+)

...

> diff --git a/drivers/thermal/k1_thermal.c b/drivers/thermal/k1_thermal.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..a0e9585cbc5a4e0f7c3a47debb3cfa8e82082d88
> --- /dev/null
> +++ b/drivers/thermal/k1_thermal.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Thermal sensor driver for SpacemiT K1 SoC
> + *
> + * Copyright (C) 2025 Shuwei Wu <shuweiwoo@163.com>
> + */
> +#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/thermal.h>

devm_kzalloc() is used in the file, but include for linux/slab.h is
missing.

> +#include "thermal_hwmon.h"
> +
> +#define MAX_SENSOR_NUMBER		5
> +#define TEMPERATURE_OFFSET		278
> +
> +#define K1_TSU_INT_EN			0x14
> +#define K1_TSU_INT_CLR			0x10
> +#define K1_TSU_INT_STA			0x18

...

> +#define K1_TSU_EN			0x8

...

> +#define K1_TSU_DATA_BASE		0x20

...

> +#define K1_TSU_THRSH_BASE		0x40

...

> +#define K1_TSU_TIME			0x0C

...

> +#define K1_TSU_PCTRL			0x00

...

> +#define K1_TSU_PCTRL2			0x04

Why not sort these register offsets?

> +struct k1_thermal_sensor {
> +	struct k1_thermal_priv *priv;
> +	struct thermal_zone_device *tzd;
> +	int id;
> +};

> +struct k1_thermal_priv {
> +	void __iomem *base;
> +	struct device *dev;

This variable is set but used nowhere, so I think this could be dropped.

> +	struct clk *clk;
> +	struct clk *bus_clk;
> +	struct reset_control *reset;

With devres-managed API, these three variables are only used in the
probe function, thus could be dropped, too.

> +	struct k1_thermal_sensor sensors[MAX_SENSOR_NUMBER];
> +};
> +
> +static int k1_init_sensors(struct platform_device *pdev)

Suggest passing k1_thermal_priv directly into the function, since
struct platform_device isn't really necessary for it. Also it could
return void, since there's no error to happen.

> +{

...

> +	/*
> +	 * Enable all sensors' auto mode, enable dither control,
> +	 * consecutive mode, and power up sensor.
> +	 */
> +	temp = readl(priv->base + K1_TSU_PCTRL);
> +	temp |= K1_TSU_PCTRL_RAW_SEL |
> +		K1_TSU_PCTRL_TEMP_MODE |
> +		K1_TSU_PCTRL_HW_AUTO_MODE |
> +		K1_TSU_PCTRL_ENABLE;
> +	temp &= ~K1_TSU_PCTRL_SW_CTRL;
> +	temp &= ~K1_TSU_PCTRL_CTUNE;
> +	writel(temp, priv->base + K1_TSU_PCTRL);

It's a nitpick, but I think it'll be better if you follow the same
pattern as in other readl-modification-writel blocks, to clear the bits
then set the desired ones later,

> +	/* Select 24M clk for high speed mode */

This looks a little confusing, in dt-bindings you only listed a core
clock and a bus clock, but neither core nor bus clock runs at 24MHz. So
I suspect there's another clock source supplying the "24MHz clk",
possibly the 24MHz oscillator, and it should be described in
dt-bindings, too.

> +	temp = readl(priv->base + K1_TSU_PCTRL2);
> +	temp &= ~K1_TSU_PCTRL2_CLK_SEL_MASK;
> +	temp |= K1_TSU_PCTRL2_CLK_SEL_24M;
> +	writel(temp, priv->base + K1_TSU_PCTRL2);

...

> +	/* Enable each sensor */
> +	for (i = 0; i < MAX_SENSOR_NUMBER; ++i) {
> +		temp = readl(priv->base + K1_TSU_EN);
> +		temp &= ~K1_TSU_EN_MASK(i);
> +		temp |= K1_TSU_EN_MASK(i);

What's the point of clearing a bit and setting it again?

Furthermore, this is the only place K1_TSU_EN_MASK is used. If you fold
the modified bits into a macro, let's say, K1_TSU_EN_ALL, to be
GENMASK(MAX_SENSOR_NUNBER - 1, 0), this loop could be replaced with
readl-or-writel operation, which seems much simpler to me.

> +		writel(temp, priv->base + K1_TSU_EN);
> +	}
> +
> +	return 0;
> +}

...

> +/*
> + * The conversion formula used is:
> + * T(m°C) = (((raw_value & mask) >> shift) - TEMPERATURE_OFFSET) * 1000
> + */
> +static int k1_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +	struct k1_thermal_sensor *sensor = thermal_zone_device_priv(tz);
> +	struct k1_thermal_priv *priv = sensor->priv;
> +
> +	*temp = readl(priv->base + K1_TSU_DATA(sensor->id));
> +	*temp &= K1_TSU_DATA_MASK(sensor->id);
> +	*temp >>= K1_TSU_DATA_SHIFT(sensor->id);

FIELD_GET() would help here.

> +
> +	*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_thermal_set_trips(struct thermal_zone_device *tz, int low, int high)
> +{

...

> +	high_code = high_code / 1000 + TEMPERATURE_OFFSET;
> +	temp = readl(priv->base + K1_TSU_THRSH(sensor->id));
> +	temp &= ~K1_TSU_THRSH_HIGH_MASK;
> +	temp |= (high_code << K1_TSU_THRSH_HIGH_SHIFT);
> +	writel(temp, priv->base + K1_TSU_THRSH(sensor->id));
> +
> +	low_code = low_code / 1000 + TEMPERATURE_OFFSET;
> +	temp = readl(priv->base + K1_TSU_THRSH(sensor->id));
> +	temp &= ~K1_TSU_THRSH_LOW_MASK;
> +	temp |= (low_code << K1_TSU_THRSH_LOW_SHIFT);
> +	writel(temp, priv->base + K1_TSU_THRSH(sensor->id));

Similarly, FIELD_PUT() could simplify these threshold setting code.

> +
> +	return 0;
> +}

...

> +static irqreturn_t k1_thermal_irq_thread(int irq, void *data)
> +{
> +	struct k1_thermal_priv *priv = (struct k1_thermal_priv *)data;
> +	int msk, status, i;
> +
> +	status = readl(priv->base + K1_TSU_INT_STA);
> +
> +	for (i = 0; i < MAX_SENSOR_NUMBER; i++) {
> +		if (status & K1_TSU_INT_MASK(i)) {
> +			msk = readl(priv->base + K1_TSU_INT_CLR);
> +			msk |= K1_TSU_INT_MASK(i);
> +			writel(msk, priv->base + K1_TSU_INT_CLR);
> +			/* Notify thermal framework to update trips */

Purpose of the code looks obvious, do you think the comment should be
dropped?

> +			thermal_zone_device_update(priv->sensors[i].tzd, THERMAL_EVENT_UNSPECIFIED);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int k1_thermal_probe(struct platform_device *pdev)
> +{

...

> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);

Why not using dev_err_probe() here?

...

> +	ret = k1_init_sensors(pdev);

k1_init_sensors would never fail, suggest changing it to return void,
and get rid of assignment to ret here.

Best regards,
Yao Zi

[PATCH 0/3] thermal: spacemit: Add support for SpacemiT K1 SoC thermal sensor

[Shuwei Wu]

Introduce support for the on-die thermal sensor unit (TSU)
found on the SpacemiT K1 SoC.

Include the device tree binding documentation in YAML format, the
thermal sensor driver implementation, and the device tree changes to
enable the sensor on K1 SoC.

Test logs:
Hardware: OrangePi-RV2 integrates SpacemiT K1 SoC
Kernel: 6.18.0-rc4 mainline

Verified that all five thermal sensors are registered and reporting
valid temperatures.

$ cat /sys/class/thermal/thermal_zone*/type
soc-thermal
package-thermal
gpu-thermal
cluster0-thermal
cluster1-thermal

$ cat /sys/class/thermal/thermal_zone3/temp
28000

Dynamic threshold and interrupt tests passed via sysfs trip_point
manipulation.

---
Shuwei Wu (3):
      dt-bindings: thermal: Add SpacemiT K1 thermal sensor
      thermal: K1: Add driver for K1 SoC thermal sensor
      riscv: dts: spacemit: Add thermal sensor for K1 SoC

 .../bindings/thermal/spacemit,k1-thermal.yaml      |  76 +++++
 arch/riscv/boot/dts/spacemit/k1.dtsi               | 101 +++++++
 drivers/thermal/Kconfig                            |  14 +
 drivers/thermal/Makefile                           |   1 +
 drivers/thermal/k1_thermal.c                       | 307 +++++++++++++++++++++
 5 files changed, 499 insertions(+)
---
base-commit: f5f2e20b1cbc5f9ea20b372d15967b24921ede19
change-id: 20251124-b4-k1-thermal-eca906e6dd7a

Best regards,
-- 
Shuwei Wu <shuweiwoo@163.com>

[PATCH 1/3] dt-bindings: thermal: Add SpacemiT K1 thermal sensor

[Shuwei Wu]

Document the SpacemiT K1 Thermal Sensor Unit (TSU), which supports
monitoring temperatures for five zones: soc, package, gpu, cluster0,
and cluster1.

Signed-off-by: Shuwei Wu <shuweiwoo@163.com>
---
 .../bindings/thermal/spacemit,k1-thermal.yaml      | 76 ++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/spacemit,k1-thermal.yaml b/Documentation/devicetree/bindings/thermal/spacemit,k1-thermal.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..6057161b4b00c8f869d16199a1cc0fc964fed998
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/spacemit,k1-thermal.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/spacemit,k1-thermal.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K1 Thermal Sensor Unit
+
+description:
+  The SpacemiT K1 Thermal Sensor Unit (TSU) monitors the temperature of
+  the SoC using multiple internal sensors (e.g., soc, package, gpu, clusters).
+
+maintainers:
+  - Shuwei Wu <shuweiwoo@163.com>
+
+$ref: thermal-sensor.yaml#
+
+properties:
+  compatible:
+    const: spacemit,k1-thermal
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Core clock for thermal sensor
+      - description: Bus clock for thermal sensor
+
+  clock-names:
+    items:
+      - const: core
+      - const: bus
+
+  interrupts:
+    maxItems: 1
+
+  resets:
+    items:
+      - description: Reset for the thermal sensor
+
+  "#thermal-sensor-cells":
+    const: 1
+    description:
+      The first cell indicates the sensor ID.
+      0 = soc
+      1 = package
+      2 = gpu
+      3 = cluster0
+      4 = cluster1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - resets
+  - "#thermal-sensor-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/spacemit,k1-syscon.h>
+
+    thermal@d4018000 {
+        compatible = "spacemit,k1-thermal";
+        reg = <0xd4018000 0x100>;
+        clocks = <&syscon_apbc CLK_TSEN>,
+                 <&syscon_apbc CLK_TSEN_BUS>;
+        clock-names = "core", "bus";
+        interrupts = <61>;
+        resets = <&syscon_apbc RESET_TSEN>;
+        #thermal-sensor-cells = <1>;
+    };

-- 
2.51.0

[PATCH 2/3] thermal: K1: Add driver for K1 SoC thermal sensor

[Shuwei Wu]

The thermal sensor unit (TSU) 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>
---
 drivers/thermal/Kconfig      |  14 ++
 drivers/thermal/Makefile     |   1 +
 drivers/thermal/k1_thermal.c | 307 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 322 insertions(+)

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index a09c188b9ad11377afe232d89c60504eb7000417..76095d2888980718b39470c09731092a21f7159b 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -495,6 +495,20 @@ config SPRD_THERMAL
 	  Support for the Spreadtrum thermal sensor driver in the Linux thermal
 	  framework.
 
+config K1_THERMAL
+	tristate "SpacemiT K1 thermal sensor driver"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	help
+	  This driver provides support for the thermal sensor unit (TSU)
+	  integrated in the SpacemiT K1 SoC.
+
+	  The TSU 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_thermal.
+
 config KHADAS_MCU_FAN_THERMAL
 	tristate "Khadas MCU controller FAN cooling support"
 	depends on OF
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index d7718978db245faffba98ff95a07c7bcbc776fd2..bf28ffe7a39f916acd608ea6d592c82049b0be17 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_GENERIC_ADC_THERMAL)	+= thermal-generic-adc.o
 obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
 obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
 obj-$(CONFIG_SPRD_THERMAL)	+= sprd_thermal.o
+obj-$(CONFIG_K1_THERMAL)	+= k1_thermal.o
 obj-$(CONFIG_KHADAS_MCU_FAN_THERMAL)	+= khadas_mcu_fan.o
 obj-$(CONFIG_LOONGSON2_THERMAL)	+= loongson2_thermal.o
 obj-$(CONFIG_THERMAL_CORE_TESTING)	+= testing/
diff --git a/drivers/thermal/k1_thermal.c b/drivers/thermal/k1_thermal.c
new file mode 100644
index 0000000000000000000000000000000000000000..a0e9585cbc5a4e0f7c3a47debb3cfa8e82082d88
--- /dev/null
+++ b/drivers/thermal/k1_thermal.c
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Thermal sensor driver for SpacemiT K1 SoC
+ *
+ * Copyright (C) 2025 Shuwei Wu <shuweiwoo@163.com>
+ */
+#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/thermal.h>
+
+#include "thermal_hwmon.h"
+
+#define MAX_SENSOR_NUMBER		5
+#define TEMPERATURE_OFFSET		278
+
+#define K1_TSU_INT_EN			0x14
+#define K1_TSU_INT_CLR			0x10
+#define K1_TSU_INT_STA			0x18
+
+#define K1_TSU_INT_EN_MASK		BIT(0)
+#define K1_TSU_INT_MASK(x)		(GENMASK(2, 1) << ((x) * 2))
+
+#define K1_TSU_EN			0x8
+#define K1_TSU_EN_MASK(x)		BIT(x)
+
+#define K1_TSU_DATA_BASE		0x20
+#define K1_TSU_DATA(x)			(K1_TSU_DATA_BASE + ((x) / 2) * 4)
+#define K1_TSU_DATA_MASK(x)		(((x) % 2) ? GENMASK(31, 16) : GENMASK(15, 0))
+#define K1_TSU_DATA_SHIFT(x)		(((x) % 2) ? 16 : 0)
+
+#define K1_TSU_THRSH_BASE		0x40
+#define K1_TSU_THRSH(x)			(K1_TSU_THRSH_BASE + ((x) * 4))
+#define K1_TSU_THRSH_HIGH_MASK		GENMASK(31, 16)
+#define K1_TSU_THRSH_LOW_MASK		GENMASK(15, 0)
+#define K1_TSU_THRSH_HIGH_SHIFT		16
+#define K1_TSU_THRSH_LOW_SHIFT		0
+
+#define K1_TSU_TIME			0x0C
+#define K1_TSU_TIME_MASK		GENMASK(23, 0)
+#define K1_TSU_TIME_FILTER_PERIOD	GENMASK(21, 20)
+#define K1_TSU_TIME_ADC_CNT_RST		GENMASK(7, 4)
+#define K1_TSU_TIME_WAIT_REF_CNT	GENMASK(3, 0)
+
+#define K1_TSU_PCTRL			0x00
+#define K1_TSU_PCTRL_RAW_SEL		BIT(7)
+#define K1_TSU_PCTRL_TEMP_MODE		BIT(3)
+#define K1_TSU_PCTRL_ENABLE		BIT(0)
+
+#define K1_TSU_PCTRL_SW_CTRL		GENMASK(21, 18)
+#define K1_TSU_PCTRL_CTUNE		GENMASK(11, 8)
+#define K1_TSU_PCTRL_HW_AUTO_MODE	BIT(23)
+
+#define K1_TSU_PCTRL2			0x04
+#define K1_TSU_PCTRL2_CLK_SEL_MASK	GENMASK(15, 14)
+#define K1_TSU_PCTRL2_CLK_SEL_24M	(0 << 14)
+
+struct k1_thermal_sensor {
+	struct k1_thermal_priv *priv;
+	struct thermal_zone_device *tzd;
+	int id;
+};
+
+struct k1_thermal_priv {
+	void __iomem *base;
+	struct device *dev;
+	struct clk *clk;
+	struct clk *bus_clk;
+	struct reset_control *reset;
+	struct k1_thermal_sensor sensors[MAX_SENSOR_NUMBER];
+};
+
+static int k1_init_sensors(struct platform_device *pdev)
+{
+	struct k1_thermal_priv *priv = platform_get_drvdata(pdev);
+	unsigned int temp;
+	int i;
+
+	/* Disable all the interrupts */
+	writel(0xffffffff, priv->base + K1_TSU_INT_EN);
+
+	/* Configure ADC sampling time and filter period */
+	temp = readl(priv->base + K1_TSU_TIME);
+	temp &= ~K1_TSU_TIME_MASK;
+	temp |= K1_TSU_TIME_FILTER_PERIOD |
+		K1_TSU_TIME_ADC_CNT_RST |
+		K1_TSU_TIME_WAIT_REF_CNT;
+	writel(temp, priv->base + K1_TSU_TIME);
+
+	/*
+	 * Enable all sensors' auto mode, enable dither control,
+	 * consecutive mode, and power up sensor.
+	 */
+	temp = readl(priv->base + K1_TSU_PCTRL);
+	temp |= K1_TSU_PCTRL_RAW_SEL |
+		K1_TSU_PCTRL_TEMP_MODE |
+		K1_TSU_PCTRL_HW_AUTO_MODE |
+		K1_TSU_PCTRL_ENABLE;
+	temp &= ~K1_TSU_PCTRL_SW_CTRL;
+	temp &= ~K1_TSU_PCTRL_CTUNE;
+	writel(temp, priv->base + K1_TSU_PCTRL);
+
+	/* Select 24M clk for high speed mode */
+	temp = readl(priv->base + K1_TSU_PCTRL2);
+	temp &= ~K1_TSU_PCTRL2_CLK_SEL_MASK;
+	temp |= K1_TSU_PCTRL2_CLK_SEL_24M;
+	writel(temp, priv->base + K1_TSU_PCTRL2);
+
+	/* Enable thermal interrupt */
+	temp = readl(priv->base + K1_TSU_INT_EN);
+	temp |= K1_TSU_INT_EN_MASK;
+	writel(temp, priv->base + K1_TSU_INT_EN);
+
+	/* Enable each sensor */
+	for (i = 0; i < MAX_SENSOR_NUMBER; ++i) {
+		temp = readl(priv->base + K1_TSU_EN);
+		temp &= ~K1_TSU_EN_MASK(i);
+		temp |= K1_TSU_EN_MASK(i);
+		writel(temp, priv->base + K1_TSU_EN);
+	}
+
+	return 0;
+}
+
+static void k1_enable_sensor_irq(struct k1_thermal_sensor *sensor)
+{
+	struct k1_thermal_priv *priv = sensor->priv;
+	unsigned int temp;
+
+	temp = readl(priv->base + K1_TSU_INT_CLR);
+	temp |= K1_TSU_INT_MASK(sensor->id);
+	writel(temp, priv->base + K1_TSU_INT_CLR);
+
+	temp = readl(priv->base + K1_TSU_INT_EN);
+	temp &= ~K1_TSU_INT_MASK(sensor->id);
+	writel(temp, priv->base + K1_TSU_INT_EN);
+}
+
+/*
+ * The conversion formula used is:
+ * T(m°C) = (((raw_value & mask) >> shift) - TEMPERATURE_OFFSET) * 1000
+ */
+static int k1_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
+{
+	struct k1_thermal_sensor *sensor = thermal_zone_device_priv(tz);
+	struct k1_thermal_priv *priv = sensor->priv;
+
+	*temp = readl(priv->base + K1_TSU_DATA(sensor->id));
+	*temp &= K1_TSU_DATA_MASK(sensor->id);
+	*temp >>= K1_TSU_DATA_SHIFT(sensor->id);
+
+	*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_thermal_set_trips(struct thermal_zone_device *tz, int low, int high)
+{
+	struct k1_thermal_sensor *sensor = thermal_zone_device_priv(tz);
+	struct k1_thermal_priv *priv = sensor->priv;
+	int high_code = high;
+	int low_code = low;
+	unsigned int temp;
+
+	if (low >= high)
+		return -EINVAL;
+
+	if (low < 0)
+		low_code = 0;
+
+	high_code = high_code / 1000 + TEMPERATURE_OFFSET;
+	temp = readl(priv->base + K1_TSU_THRSH(sensor->id));
+	temp &= ~K1_TSU_THRSH_HIGH_MASK;
+	temp |= (high_code << K1_TSU_THRSH_HIGH_SHIFT);
+	writel(temp, priv->base + K1_TSU_THRSH(sensor->id));
+
+	low_code = low_code / 1000 + TEMPERATURE_OFFSET;
+	temp = readl(priv->base + K1_TSU_THRSH(sensor->id));
+	temp &= ~K1_TSU_THRSH_LOW_MASK;
+	temp |= (low_code << K1_TSU_THRSH_LOW_SHIFT);
+	writel(temp, priv->base + K1_TSU_THRSH(sensor->id));
+
+	return 0;
+}
+
+static const struct thermal_zone_device_ops k1_thermal_ops = {
+	.get_temp = k1_thermal_get_temp,
+	.set_trips = k1_thermal_set_trips,
+};
+
+static irqreturn_t k1_thermal_irq_thread(int irq, void *data)
+{
+	struct k1_thermal_priv *priv = (struct k1_thermal_priv *)data;
+	int msk, status, i;
+
+	status = readl(priv->base + K1_TSU_INT_STA);
+
+	for (i = 0; i < MAX_SENSOR_NUMBER; i++) {
+		if (status & K1_TSU_INT_MASK(i)) {
+			msk = readl(priv->base + K1_TSU_INT_CLR);
+			msk |= K1_TSU_INT_MASK(i);
+			writel(msk, priv->base + K1_TSU_INT_CLR);
+			/* Notify thermal framework to update trips */
+			thermal_zone_device_update(priv->sensors[i].tzd, THERMAL_EVENT_UNSPECIFIED);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int k1_thermal_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct k1_thermal_priv *priv;
+	int i, irq, ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	platform_set_drvdata(pdev, priv);
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->reset = devm_reset_control_get_exclusive_deasserted(dev, NULL);
+	if (IS_ERR(priv->reset))
+		return dev_err_probe(dev, PTR_ERR(priv->reset),
+				     "Failed to get/deassert reset control\n");
+
+	priv->clk = devm_clk_get_enabled(dev, "core");
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "Failed to get core clock\n");
+
+	priv->bus_clk = devm_clk_get_enabled(dev, "bus");
+	if (IS_ERR(priv->bus_clk))
+		return dev_err_probe(dev, PTR_ERR(priv->bus_clk),
+				     "Failed to get bus clock\n");
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = k1_init_sensors(pdev);
+
+	for (i = 0; i < MAX_SENSOR_NUMBER; ++i) {
+		priv->sensors[i].id = i;
+		priv->sensors[i].priv = priv;
+		priv->sensors[i].tzd = devm_thermal_of_zone_register(dev,
+									i, priv->sensors + i,
+									&k1_thermal_ops);
+		if (IS_ERR(priv->sensors[i].tzd))
+			return dev_err_probe(dev, PTR_ERR(priv->sensors[i].tzd),
+						"Failed to register thermal zone: %d\n", i);
+
+		/* Attach sysfs hwmon attributes for userspace monitoring */
+		ret = devm_thermal_add_hwmon_sysfs(dev, priv->sensors[i].tzd);
+		if (ret)
+			dev_warn(dev, "Failed to add hwmon sysfs attributes\n");
+
+		k1_enable_sensor_irq(priv->sensors + i);
+	}
+
+	ret = devm_request_threaded_irq(dev, irq, NULL,
+					k1_thermal_irq_thread,
+					IRQF_ONESHOT, "k1_thermal", priv);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
+
+	return 0;
+}
+
+static const struct of_device_id k1_thermal_dt_ids[] = {
+	{ .compatible = "spacemit,k1-thermal" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, k1_thermal_dt_ids);
+
+static struct platform_driver k1_thermal_driver = {
+	.driver = {
+		.name		= "k1_thermal",
+		.of_match_table = k1_thermal_dt_ids,
+	},
+	.probe	= k1_thermal_probe,
+};
+module_platform_driver(k1_thermal_driver);
+
+MODULE_DESCRIPTION("SpacemiT K1 Thermal Sensor Driver");
+MODULE_AUTHOR("Shuwei Wu <shuweiwoo@163.com>");
+MODULE_LICENSE("GPL");

-- 
2.51.0

[PATCH 3/3] riscv: dts: spacemit: Add thermal sensor for K1 SoC

[Shuwei Wu]

Include the Thermal Sensor Unit (TSU) node in the SpacemiT K1 dtsi
with definitions for registers, clocks, and interrupts.
Additionally, configure thermal zones for the soc, package, gpu, and
clusters to enable temperature monitoring via the thermal framework.

Signed-off-by: Shuwei Wu <shuweiwoo@163.com>
---
 arch/riscv/boot/dts/spacemit/k1.dtsi | 101 +++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
index 6cdcd80a7c83b3f62500f226e8ed16787ded5016..67596c230396056eea4b2cfa9c6d7f04627c0209 100644
--- a/arch/riscv/boot/dts/spacemit/k1.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
@@ -338,6 +338,96 @@ osc_32k: clock-32k {
 		};
 	};
 
+	thermal-zones {
+		soc-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&thermal 0>;
+
+			trips {
+				soc-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+		};
+
+		package-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&thermal 1>;
+
+			trips {
+				package-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+		};
+
+		gpu-thermal {
+			polling-delay-passive = <100>;
+			polling-delay = <0>;
+			thermal-sensors = <&thermal 2>;
+
+			trips {
+				gpu-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				gpu-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+		};
+
+		cluster0-thermal {
+			polling-delay-passive = <100>;
+			polling-delay = <0>;
+			thermal-sensors = <&thermal 3>;
+
+			trips {
+				cluster0-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cluster0-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+		};
+
+		cluster1-thermal {
+			polling-delay-passive = <100>;
+			polling-delay = <0>;
+			thermal-sensors = <&thermal 4>;
+
+			trips {
+				cluster1-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cluster1-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+		};
+	};
+
 	soc {
 		compatible = "simple-bus";
 		interrupt-parent = <&plic>;
@@ -369,6 +459,17 @@ syscon_apbc: system-controller@d4015000 {
 			#reset-cells = <1>;
 		};
 
+		thermal: thermal@d4018000 {
+			compatible = "spacemit,k1-thermal";
+			reg = <0x0 0xd4018000 0x0 0x100>;
+			clocks = <&syscon_apbc CLK_TSEN>,
+				 <&syscon_apbc CLK_TSEN_BUS>;
+			clock-names = "core", "bus";
+			interrupts = <61>;
+			resets = <&syscon_apbc RESET_TSEN>;
+			#thermal-sensor-cells = <1>;
+		};
+
 		gpio: gpio@d4019000 {
 			compatible = "spacemit,k1-gpio";
 			reg = <0x0 0xd4019000 0x0 0x100>;

-- 
2.51.0

Re: [PATCH 1/3] dt-bindings: thermal: Add SpacemiT K1 thermal sensor

[Krzysztof Kozlowski]

On Thu, Nov 27, 2025 at 02:44:07AM +0800, Shuwei Wu wrote:
> Document the SpacemiT K1 Thermal Sensor Unit (TSU), which supports
> monitoring temperatures for five zones: soc, package, gpu, cluster0,
> and cluster1.
> 
> Signed-off-by: Shuwei Wu <shuweiwoo@163.com>
> ---
>  .../bindings/thermal/spacemit,k1-thermal.yaml      | 76 ++++++++++++++++++++++
>  1 file changed, 76 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>


<form letter>
This is an automated instruction, just in case, because many review
tags are being ignored. If you know the process, just skip it entirely
(please do not feel offended by me posting it here - no bad intentions
intended, no patronizing, I just want to avoid wasted efforts). If you
do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions of patchset, under or above your Signed-off-by tag, unless
patch changed significantly (e.g. new properties added to the DT
bindings). Tag is "received", when provided in a message replied to you
on the mailing list. Tools like b4 can help here ('b4 trailers -u ...').
However, there's no need to repost patches *only* to add the tags. The
upstream maintainer will do that for tags received on the version they
apply.

https://elixir.bootlin.com/linux/v6.15/source/Documentation/process/submitting-patches.rst#L591
</form letter>

Best regards,
Krzysztof

Re: [PATCH 2/3] thermal: K1: Add driver for K1 SoC thermal sensor

[Yixun Lan]

Hi Shuwei,

On 02:44 Thu 27 Nov     , Shuwei Wu wrote:
> The thermal sensor unit (TSU) 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>
> ---
>  drivers/thermal/Kconfig      |  14 ++
>  drivers/thermal/Makefile     |   1 +
>  drivers/thermal/k1_thermal.c | 307 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 322 insertions(+)
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index a09c188b9ad11377afe232d89c60504eb7000417..76095d2888980718b39470c09731092a21f7159b 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -495,6 +495,20 @@ config SPRD_THERMAL
>  	  Support for the Spreadtrum thermal sensor driver in the Linux thermal
>  	  framework.
>  
> +config K1_THERMAL
please name it as SPACEMIT_K1_THERMAL, having K1 only is too short

> +	tristate "SpacemiT K1 thermal sensor driver"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	help
> +	  This driver provides support for the thermal sensor unit (TSU)
> +	  integrated in the SpacemiT K1 SoC.
> +
> +	  The TSU 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_thermal.
> +
>  config KHADAS_MCU_FAN_THERMAL
>  	tristate "Khadas MCU controller FAN cooling support"
>  	depends on OF
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index d7718978db245faffba98ff95a07c7bcbc776fd2..bf28ffe7a39f916acd608ea6d592c82049b0be17 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_GENERIC_ADC_THERMAL)	+= thermal-generic-adc.o
>  obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
>  obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
>  obj-$(CONFIG_SPRD_THERMAL)	+= sprd_thermal.o
> +obj-$(CONFIG_K1_THERMAL)	+= k1_thermal.o
same reason to adjust filename
>  obj-$(CONFIG_KHADAS_MCU_FAN_THERMAL)	+= khadas_mcu_fan.o
>  obj-$(CONFIG_LOONGSON2_THERMAL)	+= loongson2_thermal.o
>  obj-$(CONFIG_THERMAL_CORE_TESTING)	+= testing/
> diff --git a/drivers/thermal/k1_thermal.c b/drivers/thermal/k1_thermal.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..a0e9585cbc5a4e0f7c3a47debb3cfa8e82082d88
> --- /dev/null
> +++ b/drivers/thermal/k1_thermal.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Thermal sensor driver for SpacemiT K1 SoC
> + *
> + * Copyright (C) 2025 Shuwei Wu <shuweiwoo@163.com>
> + */
> +#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/thermal.h>
> +
> +#include "thermal_hwmon.h"
> +
> +#define MAX_SENSOR_NUMBER		5
> +#define TEMPERATURE_OFFSET		278
this seems a magic number? could you improve the name a bit,
or better put a comment to have a explanation

> +
> +#define K1_TSU_INT_EN			0x14
> +#define K1_TSU_INT_CLR			0x10
> +#define K1_TSU_INT_STA			0x18
> +
> +#define K1_TSU_INT_EN_MASK		BIT(0)
> +#define K1_TSU_INT_MASK(x)		(GENMASK(2, 1) << ((x) * 2))
> +
> +#define K1_TSU_EN			0x8
> +#define K1_TSU_EN_MASK(x)		BIT(x)
> +
> +#define K1_TSU_DATA_BASE		0x20
..
> +#define K1_TSU_DATA(x)			(K1_TSU_DATA_BASE + ((x) / 2) * 4)
so this is a register after taking a look at the code..
can you add a 'REG' explicitly? same reason for others

> +#define K1_TSU_DATA_MASK(x)		(((x) % 2) ? GENMASK(31, 16) : GENMASK(15, 0))
> +#define K1_TSU_DATA_SHIFT(x)		(((x) % 2) ? 16 : 0)
There is only one call, can you just fold it there? instead of using this magic

> +
> +#define K1_TSU_THRSH_BASE		0x40
> +#define K1_TSU_THRSH(x)			(K1_TSU_THRSH_BASE + ((x) * 4))
> +#define K1_TSU_THRSH_HIGH_MASK		GENMASK(31, 16)
> +#define K1_TSU_THRSH_LOW_MASK		GENMASK(15, 0)
> +#define K1_TSU_THRSH_HIGH_SHIFT		16
> +#define K1_TSU_THRSH_LOW_SHIFT		0
> +
> +#define K1_TSU_TIME			0x0C
> +#define K1_TSU_TIME_MASK		GENMASK(23, 0)
> +#define K1_TSU_TIME_FILTER_PERIOD	GENMASK(21, 20)
> +#define K1_TSU_TIME_ADC_CNT_RST		GENMASK(7, 4)
> +#define K1_TSU_TIME_WAIT_REF_CNT	GENMASK(3, 0)
> +
> +#define K1_TSU_PCTRL			0x00
> +#define K1_TSU_PCTRL_RAW_SEL		BIT(7)
> +#define K1_TSU_PCTRL_TEMP_MODE		BIT(3)
> +#define K1_TSU_PCTRL_ENABLE		BIT(0)
> +
> +#define K1_TSU_PCTRL_SW_CTRL		GENMASK(21, 18)
> +#define K1_TSU_PCTRL_CTUNE		GENMASK(11, 8)
> +#define K1_TSU_PCTRL_HW_AUTO_MODE	BIT(23)
> +
> +#define K1_TSU_PCTRL2			0x04
> +#define K1_TSU_PCTRL2_CLK_SEL_MASK	GENMASK(15, 14)
> +#define K1_TSU_PCTRL2_CLK_SEL_24M	(0 << 14)
> +
> +struct k1_thermal_sensor {
> +	struct k1_thermal_priv *priv;
> +	struct thermal_zone_device *tzd;
> +	int id;
> +};
> +
> +struct k1_thermal_priv {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct clk *clk;
> +	struct clk *bus_clk;
> +	struct reset_control *reset;
> +	struct k1_thermal_sensor sensors[MAX_SENSOR_NUMBER];
> +};
> +
> +static int k1_init_sensors(struct platform_device *pdev)
> +{
> +	struct k1_thermal_priv *priv = platform_get_drvdata(pdev);
> +	unsigned int temp;
so is 'temp' short for temperature? if not, for intermediate register value,
please simply use 'val' for less confusion, sometimes people prefer 'tmp',
but I'd avoid here

> +	int i;
> +
> +	/* Disable all the interrupts */
> +	writel(0xffffffff, priv->base + K1_TSU_INT_EN);
> +
> +	/* Configure ADC sampling time and filter period */
> +	temp = readl(priv->base + K1_TSU_TIME);
> +	temp &= ~K1_TSU_TIME_MASK;
> +	temp |= K1_TSU_TIME_FILTER_PERIOD |
> +		K1_TSU_TIME_ADC_CNT_RST |
> +		K1_TSU_TIME_WAIT_REF_CNT;
> +	writel(temp, priv->base + K1_TSU_TIME);
> +
> +	/*
> +	 * Enable all sensors' auto mode, enable dither control,
> +	 * consecutive mode, and power up sensor.
> +	 */
> +	temp = readl(priv->base + K1_TSU_PCTRL);
> +	temp |= K1_TSU_PCTRL_RAW_SEL |
> +		K1_TSU_PCTRL_TEMP_MODE |
> +		K1_TSU_PCTRL_HW_AUTO_MODE |
> +		K1_TSU_PCTRL_ENABLE;
> +	temp &= ~K1_TSU_PCTRL_SW_CTRL;
> +	temp &= ~K1_TSU_PCTRL_CTUNE;
> +	writel(temp, priv->base + K1_TSU_PCTRL);
> +
> +	/* Select 24M clk for high speed mode */
> +	temp = readl(priv->base + K1_TSU_PCTRL2);
> +	temp &= ~K1_TSU_PCTRL2_CLK_SEL_MASK;
> +	temp |= K1_TSU_PCTRL2_CLK_SEL_24M;
> +	writel(temp, priv->base + K1_TSU_PCTRL2);
> +
> +	/* Enable thermal interrupt */
> +	temp = readl(priv->base + K1_TSU_INT_EN);
> +	temp |= K1_TSU_INT_EN_MASK;
> +	writel(temp, priv->base + K1_TSU_INT_EN);
> +
> +	/* Enable each sensor */
> +	for (i = 0; i < MAX_SENSOR_NUMBER; ++i) {
> +		temp = readl(priv->base + K1_TSU_EN);
> +		temp &= ~K1_TSU_EN_MASK(i);
> +		temp |= K1_TSU_EN_MASK(i);
> +		writel(temp, priv->base + K1_TSU_EN);
> +	}
> +
> +	return 0;
> +}
> +
> +static void k1_enable_sensor_irq(struct k1_thermal_sensor *sensor)
> +{
> +	struct k1_thermal_priv *priv = sensor->priv;
> +	unsigned int temp;
> +
> +	temp = readl(priv->base + K1_TSU_INT_CLR);
> +	temp |= K1_TSU_INT_MASK(sensor->id);
> +	writel(temp, priv->base + K1_TSU_INT_CLR);
> +
> +	temp = readl(priv->base + K1_TSU_INT_EN);
> +	temp &= ~K1_TSU_INT_MASK(sensor->id);
> +	writel(temp, priv->base + K1_TSU_INT_EN);
> +}
> +
> +/*
> + * The conversion formula used is:
> + * T(m°C) = (((raw_value & mask) >> shift) - TEMPERATURE_OFFSET) * 1000
> + */
> +static int k1_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +	struct k1_thermal_sensor *sensor = thermal_zone_device_priv(tz);
> +	struct k1_thermal_priv *priv = sensor->priv;
> +
ditto, suggest to introduce intermediate variable 'val', then assign at last step
> +	*temp = readl(priv->base + K1_TSU_DATA(sensor->id));
> +	*temp &= K1_TSU_DATA_MASK(sensor->id);
> +	*temp >>= K1_TSU_DATA_SHIFT(sensor->id);
> +
> +	*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_thermal_set_trips(struct thermal_zone_device *tz, int low, int high)
> +{
> +	struct k1_thermal_sensor *sensor = thermal_zone_device_priv(tz);
> +	struct k1_thermal_priv *priv = sensor->priv;
> +	int high_code = high;
> +	int low_code = low;
> +	unsigned int temp;
> +
> +	if (low >= high)
> +		return -EINVAL;
> +
> +	if (low < 0)
> +		low_code = 0;
> +
> +	high_code = high_code / 1000 + TEMPERATURE_OFFSET;
> +	temp = readl(priv->base + K1_TSU_THRSH(sensor->id));
> +	temp &= ~K1_TSU_THRSH_HIGH_MASK;
> +	temp |= (high_code << K1_TSU_THRSH_HIGH_SHIFT);
> +	writel(temp, priv->base + K1_TSU_THRSH(sensor->id));
> +
> +	low_code = low_code / 1000 + TEMPERATURE_OFFSET;
> +	temp = readl(priv->base + K1_TSU_THRSH(sensor->id));
> +	temp &= ~K1_TSU_THRSH_LOW_MASK;
> +	temp |= (low_code << K1_TSU_THRSH_LOW_SHIFT);
> +	writel(temp, priv->base + K1_TSU_THRSH(sensor->id));
> +
any reason why not to combine above two readl/writel() into one?
> +	return 0;
> +}
> +
> +static const struct thermal_zone_device_ops k1_thermal_ops = {
> +	.get_temp = k1_thermal_get_temp,
> +	.set_trips = k1_thermal_set_trips,
> +};
> +
> +static irqreturn_t k1_thermal_irq_thread(int irq, void *data)
> +{
> +	struct k1_thermal_priv *priv = (struct k1_thermal_priv *)data;
> +	int msk, status, i;
s/msk/mask/, for better consistency
> +
> +	status = readl(priv->base + K1_TSU_INT_STA);
> +
> +	for (i = 0; i < MAX_SENSOR_NUMBER; i++) {
> +		if (status & K1_TSU_INT_MASK(i)) {
> +			msk = readl(priv->base + K1_TSU_INT_CLR);
> +			msk |= K1_TSU_INT_MASK(i);
> +			writel(msk, priv->base + K1_TSU_INT_CLR);
> +			/* Notify thermal framework to update trips */
> +			thermal_zone_device_update(priv->sensors[i].tzd, THERMAL_EVENT_UNSPECIFIED);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int k1_thermal_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct k1_thermal_priv *priv;
> +	int i, irq, ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
..
> +	priv->dev = dev;
> +	platform_set_drvdata(pdev, priv);
I'd suggest moving above behind to resource acquisition
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->reset = devm_reset_control_get_exclusive_deasserted(dev, NULL);
> +	if (IS_ERR(priv->reset))
> +		return dev_err_probe(dev, PTR_ERR(priv->reset),
> +				     "Failed to get/deassert reset control\n");
> +
> +	priv->clk = devm_clk_get_enabled(dev, "core");
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "Failed to get core clock\n");
> +
> +	priv->bus_clk = devm_clk_get_enabled(dev, "bus");
> +	if (IS_ERR(priv->bus_clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->bus_clk),
> +				     "Failed to get bus clock\n");
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = k1_init_sensors(pdev);
> +
> +	for (i = 0; i < MAX_SENSOR_NUMBER; ++i) {
> +		priv->sensors[i].id = i;
> +		priv->sensors[i].priv = priv;
> +		priv->sensors[i].tzd = devm_thermal_of_zone_register(dev,
> +									i, priv->sensors + i,
> +									&k1_thermal_ops);
> +		if (IS_ERR(priv->sensors[i].tzd))
> +			return dev_err_probe(dev, PTR_ERR(priv->sensors[i].tzd),
> +						"Failed to register thermal zone: %d\n", i);
> +
> +		/* Attach sysfs hwmon attributes for userspace monitoring */
> +		ret = devm_thermal_add_hwmon_sysfs(dev, priv->sensors[i].tzd);
> +		if (ret)
> +			dev_warn(dev, "Failed to add hwmon sysfs attributes\n");
> +
> +		k1_enable_sensor_irq(priv->sensors + i);
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL,
> +					k1_thermal_irq_thread,
> +					IRQF_ONESHOT, "k1_thermal", priv);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id k1_thermal_dt_ids[] = {
> +	{ .compatible = "spacemit,k1-thermal" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, k1_thermal_dt_ids);
> +
> +static struct platform_driver k1_thermal_driver = {
> +	.driver = {
> +		.name		= "k1_thermal",
> +		.of_match_table = k1_thermal_dt_ids,
> +	},
> +	.probe	= k1_thermal_probe,
> +};
> +module_platform_driver(k1_thermal_driver);
> +
> +MODULE_DESCRIPTION("SpacemiT K1 Thermal Sensor Driver");
> +MODULE_AUTHOR("Shuwei Wu <shuweiwoo@163.com>");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.51.0
> 

-- 
Yixun Lan (dlan)

Re: [PATCH 2/3] thermal: K1: Add driver for K1 SoC thermal sensor

[Yixun Lan]

Hi Shuwei, 

for the title, you could simplify it by adding short prefix/annotation with
my previous comments

thermal: K1: Add driver for K1 SoC thermal sensor
-> thermal: spacemit: k1: Add thermal sensor support


On 02:44 Thu 27 Nov     , Shuwei Wu wrote:
> The thermal sensor unit (TSU) 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>
> ---
>  drivers/thermal/Kconfig      |  14 ++
>  drivers/thermal/Makefile     |   1 +
>  drivers/thermal/k1_thermal.c | 307 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 322 insertions(+)
> 
> +}
[snip]..
> +
> +static int k1_thermal_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct k1_thermal_priv *priv;
> +	int i, irq, ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->reset = devm_reset_control_get_exclusive_deasserted(dev, NULL);
> +	if (IS_ERR(priv->reset))
> +		return dev_err_probe(dev, PTR_ERR(priv->reset),
> +				     "Failed to get/deassert reset control\n");
no need to break the line, it's ok to have 100 column per line
https://elixir.bootlin.com/linux/v6.18-rc7/source/Documentation/dev-tools/checkpatch.rst#L688

P.S: this isn't hard rule and may still up to maintainer..
> +
> +	priv->clk = devm_clk_get_enabled(dev, "core");
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "Failed to get core clock\n");
ditto
> +
> +	priv->bus_clk = devm_clk_get_enabled(dev, "bus");
> +	if (IS_ERR(priv->bus_clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->bus_clk),
> +				     "Failed to get bus clock\n");
ditto
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
I suggest to group this with dev_request_threaded_irq(), since both of them
are taking care of IRQ related operation
> +
> +	ret = k1_init_sensors(pdev);
> +
> +	for (i = 0; i < MAX_SENSOR_NUMBER; ++i) {
> +		priv->sensors[i].id = i;
> +		priv->sensors[i].priv = priv;
> +		priv->sensors[i].tzd = devm_thermal_of_zone_register(dev,
> +									i, priv->sensors + i,
> +									&k1_thermal_ops);
                                                                  ~~~~~~~~
here is wrong, alignment should match open parentheses, didn't checkpatch.pl warn about this?

> +		if (IS_ERR(priv->sensors[i].tzd))
> +			return dev_err_probe(dev, PTR_ERR(priv->sensors[i].tzd),
> +						"Failed to register thermal zone: %d\n", i);
I'd say, no need to do the verbose print, almost every error path has
print message already
> +
> +		/* Attach sysfs hwmon attributes for userspace monitoring */
> +		ret = devm_thermal_add_hwmon_sysfs(dev, priv->sensors[i].tzd);
> +		if (ret)
> +			dev_warn(dev, "Failed to add hwmon sysfs attributes\n");
> +
> +		k1_enable_sensor_irq(priv->sensors + i);
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL,
> +					k1_thermal_irq_thread,
> +					IRQF_ONESHOT, "k1_thermal", priv);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
no need to print extra error message, please refer to:
 https://lore.kernel.org/all/20250805092922.135500-2-panchuang@vivo.com
 https://github.com/torvalds/linux/commit/55b48e23f5c4b6f5ca9b7ab09599b17dcf501c10

-- 
Yixun Lan (dlan)