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

["Shuwei Wu" <shuwei.wu@mailbox.org>]

Hi Pei,

On Wed Jul 15, 2026 at 11:11 AM CST, Pei Xiao wrote:
>
>
> 在 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.

Thank you for your reply. Since the original cleanup issue was
identified by you, I would be happy for you to submit the update patch.

>
> 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; }    

agreed.

>
>>
>> +	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.

yes, k1_tsensor_shutdown_action() would be a clearer name.

>
> *
>> +	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