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

[Yao Zi <ziyao@disroot.org>]

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