Re: [PATCH v5 3/4] memory: tegra: Add support for multiple irqs

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

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

On Fri, Dec 19, 2025 at 11:43:53AM +0000, Ketan Patil wrote:
> Add support to handle multiple MC interrupts lines as Tegra264 supports
> multiple MC interrupt lines. So make the necessary changes to the Tegra
> MC driver to support devices with more than one interrupt line.
> Add field to specify the number of interrupts and iterate over the
> number of interrupts to register handler for each interrupt.
> 
> Signed-off-by: Ketan Patil <ketanp@nvidia.com>
> ---
>  drivers/memory/tegra/mc.c       | 35 +++++++++++++++++++++------------
>  drivers/memory/tegra/mc.h       |  1 +
>  drivers/memory/tegra/tegra186.c |  3 ++-
>  drivers/memory/tegra/tegra20.c  |  7 ++++++-
>  include/soc/tegra/mc.h          |  4 +++-
>  5 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index 6c1578b25a61..82a81ae3c50d 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -551,10 +551,15 @@ int tegra30_mc_probe(struct tegra_mc *mc)
>  
>  const struct tegra_mc_ops tegra30_mc_ops = {
>  	.probe = tegra30_mc_probe,
> -	.handle_irq = tegra30_mc_handle_irq,
> +	.handle_irq = tegra30_mc_irq_handlers,
> +	.num_interrupts = 1,
>  };
>  #endif
>  
> +const irq_handler_t tegra30_mc_irq_handlers[] = {
> +	tegra30_mc_handle_irq
> +};
> +

Technically not necessary since we have the forward-declaration in the
header, but I think it'd make sense to move the tegra30_mc_irq_handlers
definition above tegra30_mc_ops so we keep the natural ordering of
things.

>  static int mc_global_intstatus_to_channel(const struct tegra_mc *mc, u32 status,
>  					  unsigned int *mc_channel)
>  {
> @@ -953,25 +958,29 @@ static int tegra_mc_probe(struct platform_device *pdev)
>  	tegra_mc_num_channel_enabled(mc);
>  
>  	if (mc->soc->ops && mc->soc->ops->handle_irq) {
> -		mc->irq = platform_get_irq(pdev, 0);
> -		if (mc->irq < 0)
> -			return mc->irq;
> -
>  		WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n");
>  
> +		for (int i = 0; i < mc->soc->ops->num_interrupts; i++) {

It's more idiomatic to declare these variables outside of the loop. Also
make it unsigned to match the type of num_interrupts.

> +			int irq;
> +
> +			irq = platform_get_irq(pdev, i);
> +			if (irq < 0)
> +				return irq;
> +
> +			err = devm_request_irq(&pdev->dev, irq, mc->soc->ops->handle_irq[i], 0,
> +					       dev_name(&pdev->dev), mc);
> +			if (err < 0) {
> +				dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n", irq,
> +					err);
> +				return err;
> +			}
> +		}
> +
>  		if (mc->soc->num_channels)
>  			mc_ch_writel(mc, MC_BROADCAST_CHANNEL, mc->soc->intmask,
>  				     MC_INTMASK);
>  		else
>  			mc_writel(mc, mc->soc->intmask, MC_INTMASK);
> -
> -		err = devm_request_irq(&pdev->dev, mc->irq, mc->soc->ops->handle_irq, 0,
> -				       dev_name(&pdev->dev), mc);
> -		if (err < 0) {
> -			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n", mc->irq,
> -				err);
> -			return err;
> -		}
>  	}
>  
>  	if (mc->soc->reset_ops) {
> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
> index 482f836f7816..06ae3dd37a47 100644
> --- a/drivers/memory/tegra/mc.h
> +++ b/drivers/memory/tegra/mc.h
> @@ -194,6 +194,7 @@ extern const struct tegra_mc_ops tegra186_mc_ops;
>  #endif
>  
>  irqreturn_t tegra30_mc_handle_irq(int irq, void *data);
> +extern const irq_handler_t tegra30_mc_irq_handlers[];
>  extern const char * const tegra_mc_status_names[32];
>  extern const char * const tegra_mc_error_names[8];
>  
> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> index a30158d92412..a3727fc383ac 100644
> --- a/drivers/memory/tegra/tegra186.c
> +++ b/drivers/memory/tegra/tegra186.c
> @@ -174,7 +174,8 @@ const struct tegra_mc_ops tegra186_mc_ops = {
>  	.remove = tegra186_mc_remove,
>  	.resume = tegra186_mc_resume,
>  	.probe_device = tegra186_mc_probe_device,
> -	.handle_irq = tegra30_mc_handle_irq,
> +	.handle_irq = tegra30_mc_irq_handlers,
> +	.num_interrupts = 1,
>  };
>  
>  #if defined(CONFIG_ARCH_TEGRA_186_SOC)
> diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c
> index 46e97bb10163..75eeb49054cc 100644
> --- a/drivers/memory/tegra/tegra20.c
> +++ b/drivers/memory/tegra/tegra20.c
> @@ -761,9 +761,14 @@ static irqreturn_t tegra20_mc_handle_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static const irq_handler_t tegra20_mc_irq_handlers[] = {
> +	tegra20_mc_handle_irq
> +};
> +
>  static const struct tegra_mc_ops tegra20_mc_ops = {
>  	.probe = tegra20_mc_probe,
> -	.handle_irq = tegra20_mc_handle_irq,
> +	.handle_irq = tegra20_mc_irq_handlers,
> +	.num_interrupts = 1,

Probably not worth it since this will likely never change, but you could
use ARRAY_SIZE here.

>  };
>  
>  const struct tegra_mc_soc tegra20_mc_soc = {
> diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
> index d11dfefbe551..4a2cadbc0084 100644
> --- a/include/soc/tegra/mc.h
> +++ b/include/soc/tegra/mc.h
> @@ -14,6 +14,7 @@
>  #include <linux/reset-controller.h>
>  #include <linux/types.h>
>  #include <linux/tegra-icc.h>
> +#include <linux/interrupt.h>

These should be sorted alphabetically. I realize that tegra-icc.h is not
in the right order, so we might want to fix that eventually (though it's
Tegra-specific and therefore could be kept separate, too).

>  struct clk;
>  struct device;
> @@ -164,8 +165,9 @@ struct tegra_mc_ops {
>  	int (*probe)(struct tegra_mc *mc);
>  	void (*remove)(struct tegra_mc *mc);
>  	int (*resume)(struct tegra_mc *mc);
> -	irqreturn_t (*handle_irq)(int irq, void *data);
> +	const irq_handler_t *handle_irq;
>  	int (*probe_device)(struct tegra_mc *mc, struct device *dev);
> +	unsigned int num_interrupts;

It's a good idea to keep the array and the variable that counts elements
in it close together.

Thierry

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