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