Re: [PATCH 1/2] thermal: qcom: tsens: switch wake IRQ handling to PM callbacks

[Daniel Lezcano]

On 5/26/26 12:18, Priyansh Jain wrote:
> This change improves power management by using the standardized PM
> framework for wake IRQ handling.
>
> Move wake IRQ control to the PM suspend/resume path:
> - store uplow/critical IRQ numbers in struct tsens_priv
> - enable wake IRQs in tsens_suspend_common() when wakeup is allowed
> - disable wake IRQs in tsens_resume_common()
> - mark the device wakeup-capable during probe
>
> This aligns TSENS wake behavior with suspend flow and avoids keeping
> wake IRQs permanently enabled during runtime.
>
> Signed-off-by: Priyansh Jain <priyansh.jain@xxxxxxxxxxxxxxxx>
> ---

[ ... ]

>   static SIMPLE_DEV_PM_OPS(tsens_pm_ops, tsens_suspend, tsens_resume);
> @@ -1172,7 +1180,7 @@ static const struct thermal_zone_device_ops tsens_of_ops = {
>   };
>   
>   static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
> -			      irq_handler_t thread_fn)
> +			      irq_handler_t thread_fn, int *irq_num)
>   {
>   	struct platform_device *pdev;
>   	int ret, irq;
> @@ -1204,8 +1212,8 @@ static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
>   		if (ret)
>   			dev_err(&pdev->dev, "%s: failed to get irq\n",
>   				__func__);
> -		else
> -			enable_irq_wake(irq);
> +		else if (irq_num)
> +			*irq_num = irq;

You can remove this check, it is a static function called inside the 
driver which is supposed to know what it does (like not passing a NULL 
pointer parameter).

>   	}
>   
>   	put_device(&pdev->dev);
> @@ -1232,11 +1240,44 @@ static int tsens_reinit(struct tsens_priv *priv)
>   	return 0;
>   }
>   
> +int tsens_suspend_common(struct tsens_priv *priv)
> +{
> +	if (!device_may_wakeup(priv->dev))
> +		return 0;
> +
> +	if (priv->feat->combo_int) {
> +		if (priv->combined_irq > 0)
> +			enable_irq_wake(priv->combined_irq);
> +	} else {
> +		if (priv->uplow_irq > 0)
> +			enable_irq_wake(priv->uplow_irq);
> +
> +		if (priv->crit_irq > 0)
> +			enable_irq_wake(priv->crit_irq);
> +	}
> +
> +	return 0;
> +}
> +
>   int tsens_resume_common(struct tsens_priv *priv)
>   {
>   	if (pm_suspend_target_state == PM_SUSPEND_MEM)
>   		tsens_reinit(priv);
>   
> +	if (!device_may_wakeup(priv->dev))
> +		return 0;
> +
> +	if (priv->feat->combo_int) {
> +		if (priv->combined_irq > 0)
> +			disable_irq_wake(priv->combined_irq);
> +	} else {
> +		if (priv->uplow_irq > 0)
> +			disable_irq_wake(priv->uplow_irq);
> +
> +		if (priv->crit_irq > 0)
> +			disable_irq_wake(priv->crit_irq);
> +	}
> +
>   	return 0;
>   }

If tsens_register_irq() fails, tsens_register() fails, then 
tsens_probe() fails and tsens_suspend_common() / tsens_resume_common() 
can not be called because the driver failed to start.

With the version of old DT without irq, the device_may_wakeup() should 
always return false (no irq == no wakeup)

Using the check on combined_irq / uplow_irq / crit_irq is not necessary 
because if the code goes after the device_may_wakeup() block, it is 
because interrupts are set.


> @@ -1276,15 +1317,18 @@ static int tsens_register(struct tsens_priv *priv)
>   
>   	if (priv->feat->combo_int) {
>   		ret = tsens_register_irq(priv, "combined",
> -					 tsens_combined_irq_thread);
> +					 tsens_combined_irq_thread,  &priv->combined_irq);
>   	} else {
> -		ret = tsens_register_irq(priv, "uplow", tsens_irq_thread);
> +		ret = tsens_register_irq(priv, "uplow", tsens_irq_thread,
> +					 &priv->uplow_irq);
>   		if (ret < 0)
>   			return ret;
>   
> -		if (priv->feat->crit_int)
> +		if (priv->feat->crit_int) {
>   			ret = tsens_register_irq(priv, "critical",
> -						 tsens_critical_irq_thread);
> +						 tsens_critical_irq_thread,
> +						 &priv->crit_irq);
> +		}
>   	}
>   
>   	return ret;
> @@ -1343,6 +1387,8 @@ static int tsens_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, priv);
>   
> +	device_init_wakeup(dev, true);
> +
>   	if (!priv->ops || !priv->ops->init || !priv->ops->get_temp)
>   		return -EINVAL;
>   
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index 2a7afa4c899b..83a8f3580ed0 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -567,6 +567,9 @@ struct tsens_context {
>    * @ops: pointer to list of callbacks supported by this device
>    * @debug_root: pointer to debugfs dentry for all tsens
>    * @debug: pointer to debugfs dentry for tsens controller
> + * @uplow_irq: IRQ number for uplow (upper/lower) threshold interrupts
> + * @crit_irq: IRQ number for critical threshold interrupts
> + * @combined_irq: IRQ number for combined threshold interrupts
>    * @sensor: list of sensors attached to this device
>    */
>   struct tsens_priv {
> @@ -588,6 +591,10 @@ struct tsens_priv {
>   	struct dentry			*debug_root;
>   	struct dentry			*debug;
>   
> +	int				uplow_irq;
> +	int				crit_irq;
> +	int				combined_irq;
> +
>   	struct tsens_sensor		sensor[] __counted_by(num_sensors);
>   };
>   
> @@ -639,8 +646,17 @@ int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp);
>   int get_temp_common(const struct tsens_sensor *s, int *temp);
>   #ifdef CONFIG_SUSPEND
>   int tsens_resume_common(struct tsens_priv *priv);
> +int tsens_suspend_common(struct tsens_priv *priv);
>   #else
> -#define tsens_resume_common            NULL
> +static inline int tsens_resume_common(struct tsens_priv *priv)
> +{
> +		return 0;

Strange indentation

> +}
> +
> +static inline int tsens_suspend_common(struct tsens_priv *priv)
> +{
> +		return 0;

Ditto

> +}
>   #endif
>   
>   /* TSENS target */