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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3380 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]