| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aScnQgLuV3kMLBJg@pie>
Date: Wed, 26 Nov 2025 16:13:54 +0000
From: Yao Zi <ziyao@...root.org>
To: Shuwei Wu <shuweiwoo@....com>, "Rafael J. Wysocki" <rafael@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Zhang Rui <rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Yixun Lan <dlan@...too.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Paul Walmsley <pjw@...nel.org>, Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>
Cc: linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] thermal: K1: Add driver for K1 SoC thermal sensor
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@....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@....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
Powered by blists - more mailing lists