[PATCH] thermal: spacemit: k1: disable hardware on driver remove

[PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Pei Xiao]

The driver uses devm_request_threaded_irq() but never disables the
hardware interrupt enable, sensor enable, or sensor power bits when
the driver is unbound.

Add a devm action that clears K1_TSENSOR_INT_EN_REG,
K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
what k1_tsensor_init() sets up.

Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
index 79222d233129..cf79dc7d403f 100644
--- a/drivers/thermal/spacemit/k1_tsensor.c
+++ b/drivers/thermal/spacemit/k1_tsensor.c
@@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static void k1_tsensor_shutdown(struct k1_tsensor *ts)
+{
+	u32 val;
+
+	/* Disable all interrupts */
+	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
+
+	/* Disable all sensor */
+	val = readl(ts->base + K1_TSENSOR_EN_REG);
+	val &= ~K1_TSENSOR_EN_ALL;
+	writel(val, ts->base + K1_TSENSOR_EN_REG);
+
+	/* Power down the sensor module */
+	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
+	val &= ~K1_TSENSOR_PCTRL_ENABLE;
+	writel(val, ts->base + K1_TSENSOR_PCTRL_REG);
+}
+
+static void devm_k1_tsensor_shutdown(void *data)
+{
+	k1_tsensor_shutdown(data);
+}
+
 static int k1_tsensor_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -229,6 +252,10 @@ static int k1_tsensor_probe(struct platform_device *pdev)
 
 	k1_tsensor_init(ts);
 
+	ret = devm_add_action_or_reset(dev, devm_k1_tsensor_shutdown, ts);
+	if (ret)
+		return ret;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Pei Xiao]

The driver uses devm_request_threaded_irq() but never disables the
hardware interrupt enable, sensor enable, or sensor power bits when
the driver is unbound.

Add a devm action that clears K1_TSENSOR_INT_EN_REG,
K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
what k1_tsensor_init() sets up.

Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
index 79222d233129..cf79dc7d403f 100644
--- a/drivers/thermal/spacemit/k1_tsensor.c
+++ b/drivers/thermal/spacemit/k1_tsensor.c
@@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static void k1_tsensor_shutdown(struct k1_tsensor *ts)
+{
+	u32 val;
+
+	/* Disable all interrupts */
+	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
+
+	/* Disable all sensor */
+	val = readl(ts->base + K1_TSENSOR_EN_REG);
+	val &= ~K1_TSENSOR_EN_ALL;
+	writel(val, ts->base + K1_TSENSOR_EN_REG);
+
+	/* Power down the sensor module */
+	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
+	val &= ~K1_TSENSOR_PCTRL_ENABLE;
+	writel(val, ts->base + K1_TSENSOR_PCTRL_REG);
+}
+
+static void devm_k1_tsensor_shutdown(void *data)
+{
+	k1_tsensor_shutdown(data);
+}
+
 static int k1_tsensor_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -229,6 +252,10 @@ static int k1_tsensor_probe(struct platform_device *pdev)
 
 	k1_tsensor_init(ts);
 
+	ret = devm_add_action_or_reset(dev, devm_k1_tsensor_shutdown, ts);
+	if (ret)
+		return ret;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
-- 
2.25.1

Re: [PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Troy Mitchell]

On Mon Jul 13, 2026 at 1:32 AM PDT, Pei Xiao wrote:
> The driver uses devm_request_threaded_irq() but never disables the
> hardware interrupt enable, sensor enable, or sensor power bits when
> the driver is unbound.
>
> Add a devm action that clears K1_TSENSOR_INT_EN_REG,
> K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
> what k1_tsensor_init() sets up.
>
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
> index 79222d233129..cf79dc7d403f 100644
> --- a/drivers/thermal/spacemit/k1_tsensor.c
> +++ b/drivers/thermal/spacemit/k1_tsensor.c
> @@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
> +{
> +	u32 val;
> +
> +	/* Disable all interrupts */
> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
> +
> +	/* Disable all sensor */
> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
> +	val &= ~K1_TSENSOR_EN_ALL;
> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
> +
> +	/* Power down the sensor module */
> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
> +	val &= ~K1_TSENSOR_PCTRL_ENABLE;
Why is only the ENABLE bit cleared here?
At init time the driver also sets:
    val |= K1_TSENSOR_PCTRL_RAW_SEL |
    K1_TSENSOR_PCTRL_TEMP_MODE |
    K1_TSENSOR_PCTRL_HW_AUTO_MODE |
    K1_TSENSOR_PCTRL_ENABLE;

Should the other bits be cleared in shutdown as well, or is clearing ENABLE
alone?

                            - Troy

Re: [PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Troy Mitchell]

On Mon Jul 13, 2026 at 1:32 AM PDT, Pei Xiao wrote:
> The driver uses devm_request_threaded_irq() but never disables the
> hardware interrupt enable, sensor enable, or sensor power bits when
> the driver is unbound.
>
> Add a devm action that clears K1_TSENSOR_INT_EN_REG,
> K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
> what k1_tsensor_init() sets up.
>
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
> index 79222d233129..cf79dc7d403f 100644
> --- a/drivers/thermal/spacemit/k1_tsensor.c
> +++ b/drivers/thermal/spacemit/k1_tsensor.c
> @@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
> +{
> +	u32 val;
> +
> +	/* Disable all interrupts */
> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
> +
> +	/* Disable all sensor */
> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
> +	val &= ~K1_TSENSOR_EN_ALL;
> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
> +
> +	/* Power down the sensor module */
> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
> +	val &= ~K1_TSENSOR_PCTRL_ENABLE;
Why is only the ENABLE bit cleared here?
At init time the driver also sets:
    val |= K1_TSENSOR_PCTRL_RAW_SEL |
    K1_TSENSOR_PCTRL_TEMP_MODE |
    K1_TSENSOR_PCTRL_HW_AUTO_MODE |
    K1_TSENSOR_PCTRL_ENABLE;

Should the other bits be cleared in shutdown as well, or is clearing ENABLE
alone?

                            - Troy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Pei Xiao]

在 2026/7/13 16:40, Troy Mitchell 写道:
> On Mon Jul 13, 2026 at 1:32 AM PDT, Pei Xiao wrote:
>> The driver uses devm_request_threaded_irq() but never disables the
>> hardware interrupt enable, sensor enable, or sensor power bits when
>> the driver is unbound.
>>
>> Add a devm action that clears K1_TSENSOR_INT_EN_REG,
>> K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
>> what k1_tsensor_init() sets up.
>>
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>>  drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
>> index 79222d233129..cf79dc7d403f 100644
>> --- a/drivers/thermal/spacemit/k1_tsensor.c
>> +++ b/drivers/thermal/spacemit/k1_tsensor.c
>> @@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
>> +{
>> +	u32 val;
>> +
>> +	/* Disable all interrupts */
>> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
>> +
>> +	/* Disable all sensor */
>> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
>> +	val &= ~K1_TSENSOR_EN_ALL;
>> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
>> +
>> +	/* Power down the sensor module */
>> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
>> +	val &= ~K1_TSENSOR_PCTRL_ENABLE;
> Why is only the ENABLE bit cleared here?
> At init time the driver also sets:
>     val |= K1_TSENSOR_PCTRL_RAW_SEL |
>     K1_TSENSOR_PCTRL_TEMP_MODE |
>     K1_TSENSOR_PCTRL_HW_AUTO_MODE |
>     K1_TSENSOR_PCTRL_ENABLE;
>
> Should the other bits be cleared in shutdown as well, or is clearing ENABLE
> alone?
Hi Troy,
 Maybe Wu(author) have someting to say about these regs.
 Thanks.

Pei.


>                             - Troy

Re: [PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Pei Xiao]

在 2026/7/13 16:40, Troy Mitchell 写道:
> On Mon Jul 13, 2026 at 1:32 AM PDT, Pei Xiao wrote:
>> The driver uses devm_request_threaded_irq() but never disables the
>> hardware interrupt enable, sensor enable, or sensor power bits when
>> the driver is unbound.
>>
>> Add a devm action that clears K1_TSENSOR_INT_EN_REG,
>> K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
>> what k1_tsensor_init() sets up.
>>
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>>  drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
>> index 79222d233129..cf79dc7d403f 100644
>> --- a/drivers/thermal/spacemit/k1_tsensor.c
>> +++ b/drivers/thermal/spacemit/k1_tsensor.c
>> @@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
>> +{
>> +	u32 val;
>> +
>> +	/* Disable all interrupts */
>> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
>> +
>> +	/* Disable all sensor */
>> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
>> +	val &= ~K1_TSENSOR_EN_ALL;
>> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
>> +
>> +	/* Power down the sensor module */
>> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
>> +	val &= ~K1_TSENSOR_PCTRL_ENABLE;
> Why is only the ENABLE bit cleared here?
> At init time the driver also sets:
>     val |= K1_TSENSOR_PCTRL_RAW_SEL |
>     K1_TSENSOR_PCTRL_TEMP_MODE |
>     K1_TSENSOR_PCTRL_HW_AUTO_MODE |
>     K1_TSENSOR_PCTRL_ENABLE;
>
> Should the other bits be cleared in shutdown as well, or is clearing ENABLE
> alone?
Hi Troy,
 Maybe Wu(author) have someting to say about these regs.
 Thanks.

Pei.


>                             - Troy


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Shuwei Wu]

Hi Pei,

On Mon Jul 13, 2026 at 4:32 PM CST, Pei Xiao wrote:
> The driver uses devm_request_threaded_irq() but never disables the
> hardware interrupt enable, sensor enable, or sensor power bits when
> the driver is unbound.
>
> Add a devm action that clears K1_TSENSOR_INT_EN_REG,
> K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
> what k1_tsensor_init() sets up.
>
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
> index 79222d233129..cf79dc7d403f 100644
> --- a/drivers/thermal/spacemit/k1_tsensor.c
> +++ b/drivers/thermal/spacemit/k1_tsensor.c
> @@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
> +{
> +	u32 val;
> +
> +	/* Disable all interrupts */
> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
> +
> +	/* Disable all sensor */
> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
> +	val &= ~K1_TSENSOR_EN_ALL;
> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
> +
> +	/* Power down the sensor module */
> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
> +	val &= ~K1_TSENSOR_PCTRL_ENABLE;
> +	writel(val, ts->base + K1_TSENSOR_PCTRL_REG);
> +}
> +
> +static void devm_k1_tsensor_shutdown(void *data)
> +{
> +	k1_tsensor_shutdown(data);
> +}
> +
>  static int k1_tsensor_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -229,6 +252,10 @@ static int k1_tsensor_probe(struct platform_device *pdev)
>  
>  	k1_tsensor_init(ts);
>  
> +	ret = devm_add_action_or_reset(dev, devm_k1_tsensor_shutdown, ts);
> +	if (ret)
> +		return ret;
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;

Sorry, I missed the hardware cleanup in the original driver.
Thanks for fixing it.

I also found that registering the shutdown action immediately after
k1_tsensor_init() would cause the thermal zones to be released before
the IRQ during devres cleanup. The IRQ thread may then access an
unregistered thermal zone.

I changed the order so that the thermal zones are registered first,
followed by the IRQ and the shutdown action.
On removal, the hardware interrupt is disabled first, then the IRQ
is released, and finally the thermal zones are released.

-> % git --no-pager diff drivers/thermal/spacemit/k1_tsensor.c
diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
index 79222d233129..57dbc652a405 100644
--- a/drivers/thermal/spacemit/k1_tsensor.c
+++ b/drivers/thermal/spacemit/k1_tsensor.c
@@ -199,6 +199,32 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }

+static void k1_tsensor_shutdown(struct k1_tsensor *ts)
+{
+	u32 val;
+
+	/* Disable all interrupts */
+	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
+
+	/* Disable all sensors */
+	val = readl(ts->base + K1_TSENSOR_EN_REG);
+	val &= ~K1_TSENSOR_EN_ALL;
+	writel(val, ts->base + K1_TSENSOR_EN_REG);
+
+	/* Clear the sampling configuration set by k1_tsensor_init(). */
+	val = readl(ts->base + K1_TSENSOR_TIME_REG);
+	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);
+
+	/* Clear the control bits configured by k1_tsensor_init(). */
+	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
+	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);
+}
+
+static void devm_k1_tsensor_shutdown(void *data)
+{
+	k1_tsensor_shutdown(data);
+}
+
 static int k1_tsensor_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -229,34 +255,48 @@ static int k1_tsensor_probe(struct platform_device *pdev)

 	k1_tsensor_init(ts);

-	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;
-
 	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);
+		if (IS_ERR(ts->ch[i].tzd)) {
+			ret = PTR_ERR(ts->ch[i].tzd);
+			goto err_shutdown;
+		}

 		/* 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);
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		goto err_shutdown;
 	}

+	ret = devm_request_threaded_irq(dev, irq, NULL,
+					k1_tsensor_irq_thread,
+					IRQF_ONESHOT, "k1_tsensor", ts);
+	if (ret < 0)
+		goto err_shutdown;
+
+	ret = devm_add_action_or_reset(dev, devm_k1_tsensor_shutdown, ts);
+	if (ret)
+		return ret;
+
+	/* Enable interrupts only after all zones and the handler are ready. */
+	for (i = 0; i < MAX_SENSOR_NUMBER; ++i)
+		k1_tsensor_enable_irq(ts->ch + i);
+
 	platform_set_drvdata(pdev, ts);

 	return 0;
+
+err_shutdown:
+	k1_tsensor_shutdown(ts);
+	return ret;
 }

 static const struct of_device_id k1_tsensor_dt_ids[] = {

-- 
Best regards,
Shuwei Wu

Re: [PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Shuwei Wu]

Hi Pei,

On Mon Jul 13, 2026 at 4:32 PM CST, Pei Xiao wrote:
> The driver uses devm_request_threaded_irq() but never disables the
> hardware interrupt enable, sensor enable, or sensor power bits when
> the driver is unbound.
>
> Add a devm action that clears K1_TSENSOR_INT_EN_REG,
> K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
> what k1_tsensor_init() sets up.
>
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
> index 79222d233129..cf79dc7d403f 100644
> --- a/drivers/thermal/spacemit/k1_tsensor.c
> +++ b/drivers/thermal/spacemit/k1_tsensor.c
> @@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
> +{
> +	u32 val;
> +
> +	/* Disable all interrupts */
> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
> +
> +	/* Disable all sensor */
> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
> +	val &= ~K1_TSENSOR_EN_ALL;
> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
> +
> +	/* Power down the sensor module */
> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
> +	val &= ~K1_TSENSOR_PCTRL_ENABLE;
> +	writel(val, ts->base + K1_TSENSOR_PCTRL_REG);
> +}
> +
> +static void devm_k1_tsensor_shutdown(void *data)
> +{
> +	k1_tsensor_shutdown(data);
> +}
> +
>  static int k1_tsensor_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -229,6 +252,10 @@ static int k1_tsensor_probe(struct platform_device *pdev)
>  
>  	k1_tsensor_init(ts);
>  
> +	ret = devm_add_action_or_reset(dev, devm_k1_tsensor_shutdown, ts);
> +	if (ret)
> +		return ret;
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;

Sorry, I missed the hardware cleanup in the original driver.
Thanks for fixing it.

I also found that registering the shutdown action immediately after
k1_tsensor_init() would cause the thermal zones to be released before
the IRQ during devres cleanup. The IRQ thread may then access an
unregistered thermal zone.

I changed the order so that the thermal zones are registered first,
followed by the IRQ and the shutdown action.
On removal, the hardware interrupt is disabled first, then the IRQ
is released, and finally the thermal zones are released.

-> % git --no-pager diff drivers/thermal/spacemit/k1_tsensor.c
diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
index 79222d233129..57dbc652a405 100644
--- a/drivers/thermal/spacemit/k1_tsensor.c
+++ b/drivers/thermal/spacemit/k1_tsensor.c
@@ -199,6 +199,32 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }

+static void k1_tsensor_shutdown(struct k1_tsensor *ts)
+{
+	u32 val;
+
+	/* Disable all interrupts */
+	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
+
+	/* Disable all sensors */
+	val = readl(ts->base + K1_TSENSOR_EN_REG);
+	val &= ~K1_TSENSOR_EN_ALL;
+	writel(val, ts->base + K1_TSENSOR_EN_REG);
+
+	/* Clear the sampling configuration set by k1_tsensor_init(). */
+	val = readl(ts->base + K1_TSENSOR_TIME_REG);
+	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);
+
+	/* Clear the control bits configured by k1_tsensor_init(). */
+	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
+	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);
+}
+
+static void devm_k1_tsensor_shutdown(void *data)
+{
+	k1_tsensor_shutdown(data);
+}
+
 static int k1_tsensor_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -229,34 +255,48 @@ static int k1_tsensor_probe(struct platform_device *pdev)

 	k1_tsensor_init(ts);

-	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;
-
 	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);
+		if (IS_ERR(ts->ch[i].tzd)) {
+			ret = PTR_ERR(ts->ch[i].tzd);
+			goto err_shutdown;
+		}

 		/* 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);
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		goto err_shutdown;
 	}

+	ret = devm_request_threaded_irq(dev, irq, NULL,
+					k1_tsensor_irq_thread,
+					IRQF_ONESHOT, "k1_tsensor", ts);
+	if (ret < 0)
+		goto err_shutdown;
+
+	ret = devm_add_action_or_reset(dev, devm_k1_tsensor_shutdown, ts);
+	if (ret)
+		return ret;
+
+	/* Enable interrupts only after all zones and the handler are ready. */
+	for (i = 0; i < MAX_SENSOR_NUMBER; ++i)
+		k1_tsensor_enable_irq(ts->ch + i);
+
 	platform_set_drvdata(pdev, ts);

 	return 0;
+
+err_shutdown:
+	k1_tsensor_shutdown(ts);
+	return ret;
 }

 static const struct of_device_id k1_tsensor_dt_ids[] = {

-- 
Best regards,
Shuwei Wu

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Shuwei Wu]

Hi Pei,

On Mon Jul 13, 2026 at 4:55 PM CST, Pei Xiao wrote:
>
>
> 在 2026/7/13 16:40, Troy Mitchell 写道:
>> On Mon Jul 13, 2026 at 1:32 AM PDT, Pei Xiao wrote:
>>> The driver uses devm_request_threaded_irq() but never disables the
>>> hardware interrupt enable, sensor enable, or sensor power bits when
>>> the driver is unbound.
>>>
>>> Add a devm action that clears K1_TSENSOR_INT_EN_REG,
>>> K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
>>> what k1_tsensor_init() sets up.
>>>
>>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>>> ---
>>>  drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
>>> index 79222d233129..cf79dc7d403f 100644
>>> --- a/drivers/thermal/spacemit/k1_tsensor.c
>>> +++ b/drivers/thermal/spacemit/k1_tsensor.c
>>> @@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>>>  	return IRQ_HANDLED;
>>>  }
>>>  
>>> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
>>> +{
>>> +	u32 val;
>>> +
>>> +	/* Disable all interrupts */
>>> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
>>> +
>>> +	/* Disable all sensor */
>>> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
>>> +	val &= ~K1_TSENSOR_EN_ALL;
>>> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
>>> +
>>> +	/* Power down the sensor module */
>>> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
>>> +	val &= ~K1_TSENSOR_PCTRL_ENABLE;
>> Why is only the ENABLE bit cleared here?
>> At init time the driver also sets:
>>     val |= K1_TSENSOR_PCTRL_RAW_SEL |
>>     K1_TSENSOR_PCTRL_TEMP_MODE |
>>     K1_TSENSOR_PCTRL_HW_AUTO_MODE |
>>     K1_TSENSOR_PCTRL_ENABLE;
>>
>> Should the other bits be cleared in shutdown as well, or is clearing ENABLE
>> alone?
> Hi Troy,
>  Maybe Wu(author) have someting to say about these regs.
>  Thanks.
>
> Pei.

Thanks for pointing this out.

I agree that the shutdown path should clear all control bits
set by k1_tsensor_init().

The managed cleanup will assert the reset, but the reset operation may fail
and its return value is ignored. Therefore, it is safer to disable these
settings explicitly before releasing the resources.

>
>
>>                             - Troy

-- 
Best regards,
Shuwei Wu

Re: [PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Shuwei Wu]

Hi Pei,

On Mon Jul 13, 2026 at 4:55 PM CST, Pei Xiao wrote:
>
>
> 在 2026/7/13 16:40, Troy Mitchell 写道:
>> On Mon Jul 13, 2026 at 1:32 AM PDT, Pei Xiao wrote:
>>> The driver uses devm_request_threaded_irq() but never disables the
>>> hardware interrupt enable, sensor enable, or sensor power bits when
>>> the driver is unbound.
>>>
>>> Add a devm action that clears K1_TSENSOR_INT_EN_REG,
>>> K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
>>> what k1_tsensor_init() sets up.
>>>
>>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>>> ---
>>>  drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
>>> index 79222d233129..cf79dc7d403f 100644
>>> --- a/drivers/thermal/spacemit/k1_tsensor.c
>>> +++ b/drivers/thermal/spacemit/k1_tsensor.c
>>> @@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>>>  	return IRQ_HANDLED;
>>>  }
>>>  
>>> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
>>> +{
>>> +	u32 val;
>>> +
>>> +	/* Disable all interrupts */
>>> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
>>> +
>>> +	/* Disable all sensor */
>>> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
>>> +	val &= ~K1_TSENSOR_EN_ALL;
>>> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
>>> +
>>> +	/* Power down the sensor module */
>>> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
>>> +	val &= ~K1_TSENSOR_PCTRL_ENABLE;
>> Why is only the ENABLE bit cleared here?
>> At init time the driver also sets:
>>     val |= K1_TSENSOR_PCTRL_RAW_SEL |
>>     K1_TSENSOR_PCTRL_TEMP_MODE |
>>     K1_TSENSOR_PCTRL_HW_AUTO_MODE |
>>     K1_TSENSOR_PCTRL_ENABLE;
>>
>> Should the other bits be cleared in shutdown as well, or is clearing ENABLE
>> alone?
> Hi Troy,
>  Maybe Wu(author) have someting to say about these regs.
>  Thanks.
>
> Pei.

Thanks for pointing this out.

I agree that the shutdown path should clear all control bits
set by k1_tsensor_init().

The managed cleanup will assert the reset, but the reset operation may fail
and its return value is ignored. Therefore, it is safer to disable these
settings explicitly before releasing the resources.

>
>
>>                             - Troy

-- 
Best regards,
Shuwei Wu

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Pei Xiao]

在 2026/7/14 18:36, Shuwei Wu 写道:
> Hi Pei,
>
> On Mon Jul 13, 2026 at 4:32 PM CST, Pei Xiao wrote:
>> The driver uses devm_request_threaded_irq() but never disables the
>> hardware interrupt enable, sensor enable, or sensor power bits when
>> the driver is unbound.
>>
>> Add a devm action that clears K1_TSENSOR_INT_EN_REG,
>> K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
>> what k1_tsensor_init() sets up.
>>
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>>  drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
>> index 79222d233129..cf79dc7d403f 100644
>> --- a/drivers/thermal/spacemit/k1_tsensor.c
>> +++ b/drivers/thermal/spacemit/k1_tsensor.c
>> @@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
>> +{
>> +	u32 val;
>> +
>> +	/* Disable all interrupts */
>> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
>> +
>> +	/* Disable all sensor */
>> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
>> +	val &= ~K1_TSENSOR_EN_ALL;
>> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
>> +
>> +	/* Power down the sensor module */
>> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
>> +	val &= ~K1_TSENSOR_PCTRL_ENABLE;
>> +	writel(val, ts->base + K1_TSENSOR_PCTRL_REG);
>> +}
>> +
>> +static void devm_k1_tsensor_shutdown(void *data)
>> +{
>> +	k1_tsensor_shutdown(data);
>> +}
>> +
>>  static int k1_tsensor_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> @@ -229,6 +252,10 @@ static int k1_tsensor_probe(struct platform_device *pdev)
>>  
>>  	k1_tsensor_init(ts);
>>  
>> +	ret = devm_add_action_or_reset(dev, devm_k1_tsensor_shutdown, ts);
>> +	if (ret)
>> +		return ret;
>> +
>>  	irq = platform_get_irq(pdev, 0);
>>  	if (irq < 0)
>>  		return irq;
> Sorry, I missed the hardware cleanup in the original driver.
> Thanks for fixing it.
>
> I also found that registering the shutdown action immediately after
> k1_tsensor_init() would cause the thermal zones to be released before
> the IRQ during devres cleanup. The IRQ thread may then access an
> unregistered thermal zone.
>
> I changed the order so that the thermal zones are registered first,
> followed by the IRQ and the shutdown action.
> On removal, the hardware interrupt is disabled first, then the IRQ
> is released, and finally the thermal zones are released.
I notice that the changes you've made are significantly more extensive
than my original patch.
 Could you please let me know whether you plan to submit this patch
yourself, 
or whether you would like me to send it? If you would prefer me to
submit it, 
I would be delighted to contribute to the community.

Thanks！
Pei.

> -> % git --no-pager diff drivers/thermal/spacemit/k1_tsensor.c
> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
> index 79222d233129..57dbc652a405 100644
> --- a/drivers/thermal/spacemit/k1_tsensor.c
> +++ b/drivers/thermal/spacemit/k1_tsensor.c
> @@ -199,6 +199,32 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>
> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
> +{
> +	u32 val;
> +
> +	/* Disable all interrupts */
> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
> +
> +	/* Disable all sensors */
> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
> +	val &= ~K1_TSENSOR_EN_ALL;
> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
> +
> +	/* Clear the sampling configuration set by k1_tsensor_init(). */
> +	val = readl(ts->base + K1_TSENSOR_TIME_REG);
> +	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);
> +
> +	/* Clear the control bits configured by k1_tsensor_init(). */
> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
> +	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);
> +}
> +
> +static void devm_k1_tsensor_shutdown(void *data)
> +{
> +	k1_tsensor_shutdown(data);
> +}
> +
>  static int k1_tsensor_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -229,34 +255,48 @@ static int k1_tsensor_probe(struct platform_device *pdev)
>
>  	k1_tsensor_init(ts);
>
> -	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;
> -
>  	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);
> +		if (IS_ERR(ts->ch[i].tzd)) {
> +			ret = PTR_ERR(ts->ch[i].tzd);
> +			goto err_shutdown;
> +		}
>
>  		/* 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);
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		goto err_shutdown;
>  	}
irq = platform_get_irq(pdev, 0); if (irq < 0) { ret = irq; goto
err_shutdown; }    

>
> +	ret = devm_request_threaded_irq(dev, irq, NULL,
> +					k1_tsensor_irq_thread,
> +					IRQF_ONESHOT, "k1_tsensor", ts);
> +	if (ret < 0)
> +		goto err_shutdown;
> +
> +	ret = devm_add_action_or_reset(dev, devm_k1_tsensor_shutdown, ts);
*Do we need to rename |devm_k1_tsensor_shutdown| to a function name 
without the |devm| prefix? I feel the |devm| prefix is a bit redundant.

*
> +	if (ret)
> +		return ret;
> +
> +	/* Enable interrupts only after all zones and the handler are ready. */
> +	for (i = 0; i < MAX_SENSOR_NUMBER; ++i)
> +		k1_tsensor_enable_irq(ts->ch + i);
> +
>  	platform_set_drvdata(pdev, ts);
>
>  	return 0;
> +
> +err_shutdown:
> +	k1_tsensor_shutdown(ts);
> +	return ret;
>  }
>
>  static const struct of_device_id k1_tsensor_dt_ids[] = {
>

Re: [PATCH] thermal: spacemit: k1: disable hardware on driver remove

[Pei Xiao]

在 2026/7/14 18:36, Shuwei Wu 写道:
> Hi Pei,
>
> On Mon Jul 13, 2026 at 4:32 PM CST, Pei Xiao wrote:
>> The driver uses devm_request_threaded_irq() but never disables the
>> hardware interrupt enable, sensor enable, or sensor power bits when
>> the driver is unbound.
>>
>> Add a devm action that clears K1_TSENSOR_INT_EN_REG,
>> K1_TSENSOR_EN_ALL, and K1_TSENSOR_PCTRL_ENABLE on remove, mirroring
>> what k1_tsensor_init() sets up.
>>
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>>  drivers/thermal/spacemit/k1_tsensor.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
>> index 79222d233129..cf79dc7d403f 100644
>> --- a/drivers/thermal/spacemit/k1_tsensor.c
>> +++ b/drivers/thermal/spacemit/k1_tsensor.c
>> @@ -199,6 +199,29 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
>> +{
>> +	u32 val;
>> +
>> +	/* Disable all interrupts */
>> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
>> +
>> +	/* Disable all sensor */
>> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
>> +	val &= ~K1_TSENSOR_EN_ALL;
>> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
>> +
>> +	/* Power down the sensor module */
>> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
>> +	val &= ~K1_TSENSOR_PCTRL_ENABLE;
>> +	writel(val, ts->base + K1_TSENSOR_PCTRL_REG);
>> +}
>> +
>> +static void devm_k1_tsensor_shutdown(void *data)
>> +{
>> +	k1_tsensor_shutdown(data);
>> +}
>> +
>>  static int k1_tsensor_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> @@ -229,6 +252,10 @@ static int k1_tsensor_probe(struct platform_device *pdev)
>>  
>>  	k1_tsensor_init(ts);
>>  
>> +	ret = devm_add_action_or_reset(dev, devm_k1_tsensor_shutdown, ts);
>> +	if (ret)
>> +		return ret;
>> +
>>  	irq = platform_get_irq(pdev, 0);
>>  	if (irq < 0)
>>  		return irq;
> Sorry, I missed the hardware cleanup in the original driver.
> Thanks for fixing it.
>
> I also found that registering the shutdown action immediately after
> k1_tsensor_init() would cause the thermal zones to be released before
> the IRQ during devres cleanup. The IRQ thread may then access an
> unregistered thermal zone.
>
> I changed the order so that the thermal zones are registered first,
> followed by the IRQ and the shutdown action.
> On removal, the hardware interrupt is disabled first, then the IRQ
> is released, and finally the thermal zones are released.
I notice that the changes you've made are significantly more extensive
than my original patch.
 Could you please let me know whether you plan to submit this patch
yourself, 
or whether you would like me to send it? If you would prefer me to
submit it, 
I would be delighted to contribute to the community.

Thanks！
Pei.

> -> % git --no-pager diff drivers/thermal/spacemit/k1_tsensor.c
> diff --git a/drivers/thermal/spacemit/k1_tsensor.c b/drivers/thermal/spacemit/k1_tsensor.c
> index 79222d233129..57dbc652a405 100644
> --- a/drivers/thermal/spacemit/k1_tsensor.c
> +++ b/drivers/thermal/spacemit/k1_tsensor.c
> @@ -199,6 +199,32 @@ static irqreturn_t k1_tsensor_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>
> +static void k1_tsensor_shutdown(struct k1_tsensor *ts)
> +{
> +	u32 val;
> +
> +	/* Disable all interrupts */
> +	writel(0xffffffff, ts->base + K1_TSENSOR_INT_EN_REG);
> +
> +	/* Disable all sensors */
> +	val = readl(ts->base + K1_TSENSOR_EN_REG);
> +	val &= ~K1_TSENSOR_EN_ALL;
> +	writel(val, ts->base + K1_TSENSOR_EN_REG);
> +
> +	/* Clear the sampling configuration set by k1_tsensor_init(). */
> +	val = readl(ts->base + K1_TSENSOR_TIME_REG);
> +	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);
> +
> +	/* Clear the control bits configured by k1_tsensor_init(). */
> +	val = readl(ts->base + K1_TSENSOR_PCTRL_REG);
> +	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);
> +}
> +
> +static void devm_k1_tsensor_shutdown(void *data)
> +{
> +	k1_tsensor_shutdown(data);
> +}
> +
>  static int k1_tsensor_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -229,34 +255,48 @@ static int k1_tsensor_probe(struct platform_device *pdev)
>
>  	k1_tsensor_init(ts);
>
> -	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;
> -
>  	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);
> +		if (IS_ERR(ts->ch[i].tzd)) {
> +			ret = PTR_ERR(ts->ch[i].tzd);
> +			goto err_shutdown;
> +		}
>
>  		/* 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);
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		goto err_shutdown;
>  	}
irq = platform_get_irq(pdev, 0); if (irq < 0) { ret = irq; goto
err_shutdown; }    

>
> +	ret = devm_request_threaded_irq(dev, irq, NULL,
> +					k1_tsensor_irq_thread,
> +					IRQF_ONESHOT, "k1_tsensor", ts);
> +	if (ret < 0)
> +		goto err_shutdown;
> +
> +	ret = devm_add_action_or_reset(dev, devm_k1_tsensor_shutdown, ts);
*Do we need to rename |devm_k1_tsensor_shutdown| to a function name 
without the |devm| prefix? I feel the |devm| prefix is a bit redundant.

*
> +	if (ret)
> +		return ret;
> +
> +	/* Enable interrupts only after all zones and the handler are ready. */
> +	for (i = 0; i < MAX_SENSOR_NUMBER; ++i)
> +		k1_tsensor_enable_irq(ts->ch + i);
> +
>  	platform_set_drvdata(pdev, ts);
>
>  	return 0;
> +
> +err_shutdown:
> +	k1_tsensor_shutdown(ts);
> +	return ret;
>  }
>
>  static const struct of_device_id k1_tsensor_dt_ids[] = {
>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv