diff mbox series

[4/4] ARM: tegra: Avoid setting edp_irq when not relevant

Message ID 20200914133739.60020-5-kwizart@gmail.com
State New
Headers show
Series ARM: tegra: soctherm bugfixes | expand

Commit Message

Nicolas Chauvet Sept. 14, 2020, 1:37 p.m. UTC
According to the binding, the edp_irq is not available on tegra124/132

This commit moves the initialization of tegra->edp_irq after the
introduced SoCs condition. This will have the following effects:
 - soctherm_interrupts_init will not return prematurely with unfinished
thermal_irq initialization on tegra124 and tegra132
 - edp_irq initialization will be bypassed when not relevant

As a result, this will clear the following error when loading the driver:
  kernel: tegra_soctherm 700e2000.thermal-sensor: IRQ index 1 not found

Fixes: 4a04beb1bf2e (thermal: tegra: add support for EDP IRQ)
Cc: stable@vger.kernel.org
Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
---
 drivers/thermal/tegra/soctherm.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

Thierry Reding Sept. 21, 2020, 1:14 p.m. UTC | #1
On Mon, Sep 14, 2020 at 03:37:39PM +0200, Nicolas Chauvet wrote:
> According to the binding, the edp_irq is not available on tegra124/132
> 
> This commit moves the initialization of tegra->edp_irq after the
> introduced SoCs condition. This will have the following effects:
>  - soctherm_interrupts_init will not return prematurely with unfinished
> thermal_irq initialization on tegra124 and tegra132
>  - edp_irq initialization will be bypassed when not relevant
> 
> As a result, this will clear the following error when loading the driver:
>   kernel: tegra_soctherm 700e2000.thermal-sensor: IRQ index 1 not found
> 
> Fixes: 4a04beb1bf2e (thermal: tegra: add support for EDP IRQ)
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
>  drivers/thermal/tegra/soctherm.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)

Your subject needs a different prefix. As it is this looks like
something to apply to the Tegra tree, but it actually needs to go
through Zhang's and Daniel's thermal tree. Also make sure to send
patches To: the maintainers of the subsystem.

> 
> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
> index 66e0639da4bf..0a7dc988f25f 100644
> --- a/drivers/thermal/tegra/soctherm.c
> +++ b/drivers/thermal/tegra/soctherm.c
> @@ -2025,12 +2025,6 @@ static int soctherm_interrupts_init(struct platform_device *pdev,
>  		return 0;
>  	}
>  
> -	tegra->edp_irq = platform_get_irq(pdev, 1);
> -	if (tegra->edp_irq < 0) {
> -		dev_dbg(&pdev->dev, "get 'edp_irq' failed.\n");
> -		return 0;
> -	}
> -
>  	ret = devm_request_threaded_irq(&pdev->dev,
>  					tegra->thermal_irq,
>  					soctherm_thermal_isr,
> @@ -2043,6 +2037,17 @@ static int soctherm_interrupts_init(struct platform_device *pdev,
>  		return ret;
>  	}
>  
> +	/* None of the tegra124 and tegra132 SoCs have edp_irq */
> +	if (of_machine_is_compatible("nvidia,tegra124") ||
> +		of_machine_is_compatible("nvidia,tegra132"))
> +			return 0;
> +

I'd prefer to turn this into a per-SoC capability flag. You can add
something like this:

	struct tegra_soctherm_soc {
		...
		bool has_edp_irq;
	};

	...

	const struct tegra_soctherm_soc tegra124_soctherm = {
		...
		.has_edp_irq = false,
	};

	...

	const struct tegra_soctherm_soc tegra210_soctherm = {
		...
		.has_edp_irq = true,
	};

	...

and so on. This makes it more obvious why you conditionalize certain
code segments and avoids complicated conditionals.

Also, please avoid returning success early. That's very confusing
because it can lead to people adding code to the end of this function
that will never be run on the chips that you've excluded above.

So I think a better way to write this would be:

	if (tegra->soc->has_edp_irq) {
		/* get IRQ */

		/* request IRQ */
	}

That way people can simply continue adding to the bottom of the function
and that code will get executed, which is much more straightforward than
if you invert the condition.

Thierry

> +	tegra->edp_irq = platform_get_irq(pdev, 1);
> +	if (tegra->edp_irq < 0) {
> +		dev_dbg(&pdev->dev, "get 'edp_irq' failed.\n");
> +		return 0;
> +	}
> +
>  	ret = devm_request_threaded_irq(&pdev->dev,
>  					tegra->edp_irq,
>  					soctherm_edp_isr,
> -- 
> 2.25.4
>
diff mbox series

Patch

diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index 66e0639da4bf..0a7dc988f25f 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -2025,12 +2025,6 @@  static int soctherm_interrupts_init(struct platform_device *pdev,
 		return 0;
 	}
 
-	tegra->edp_irq = platform_get_irq(pdev, 1);
-	if (tegra->edp_irq < 0) {
-		dev_dbg(&pdev->dev, "get 'edp_irq' failed.\n");
-		return 0;
-	}
-
 	ret = devm_request_threaded_irq(&pdev->dev,
 					tegra->thermal_irq,
 					soctherm_thermal_isr,
@@ -2043,6 +2037,17 @@  static int soctherm_interrupts_init(struct platform_device *pdev,
 		return ret;
 	}
 
+	/* None of the tegra124 and tegra132 SoCs have edp_irq */
+	if (of_machine_is_compatible("nvidia,tegra124") ||
+		of_machine_is_compatible("nvidia,tegra132"))
+			return 0;
+
+	tegra->edp_irq = platform_get_irq(pdev, 1);
+	if (tegra->edp_irq < 0) {
+		dev_dbg(&pdev->dev, "get 'edp_irq' failed.\n");
+		return 0;
+	}
+
 	ret = devm_request_threaded_irq(&pdev->dev,
 					tegra->edp_irq,
 					soctherm_edp_isr,