Re: [PATCH v6 06/14] irqchip/irq-mvebu-icu: support ICU subnodes

From: Marc Zyngier <marc.zyngier@arm.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Mark Rutland <mark.rutland@arm.com>, Andrew Lunn <andrew@lunn.ch>,
	Jason Cooper <jason@lakedaemon.net>,
	devicetree@vger.kernel.org,
	Antoine Tenart <antoine.tenart@bootlin.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Gregory Clement <gregory.clement@bootlin.com>,
	Haim Boot <hayim@marvell.com>, Will Deacon <will.deacon@arm.com>,
	Maxime Chevallier <maxime.chevallier@bootlin.com>,
	Nadav Haklai <nadavh@marvell.com>,
	Rob Herring <robh+dt@kernel.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Hanna Hawa <hannah@marvell.com>,
	linux-arm-kernel@lists.infradead.org,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Subject: Re: [PATCH v6 06/14] irqchip/irq-mvebu-icu: support ICU subnodes
Date: Mon, 01 Oct 2018 17:49:56 +0100	[thread overview]
Message-ID: <86k1n1vcgb.wl-marc.zyngier@arm.com> (raw)
In-Reply-To: <20181001141358.31508-7-miquel.raynal@bootlin.com>

On Mon, 01 Oct 2018 15:13:50 +0100,
Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> The ICU can handle several type of interrupt, each of them being handled
> differently on AP side. On CP side, the ICU should be able to make the
> distinction between each interrupt group by pointing to the right parent.
> 
> This is done through the introduction of new bindings, presenting the ICU
> node as the parent of multiple ICU sub-nodes, each of them being an
> interrupt type with a different interrupt parent. ICU interrupt 'clients'
> now directly point to the right sub-node, avoiding the need for the extra
> ICU_GRP_* parameter.
> 
> ICU subnodes are probed automatically with devm_platform_populate(). If
> the node as no child, the probe function for NSRs will still be called
> 'manually' in order to preserve backward compatibility with DT using the
> old binding.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/irqchip/irq-mvebu-icu.c | 77 ++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> index d09f220a2701..c79d2cb787a0 100644
> --- a/drivers/irqchip/irq-mvebu-icu.c
> +++ b/drivers/irqchip/irq-mvebu-icu.c

[...]

> +static const struct of_device_id mvebu_icu_subset_of_match[] = {
> +	{
> +		.compatible = "marvell,cp110-icu-nsr",
> +	},
> +	{},
> +};
> +
>  static int mvebu_icu_subset_probe(struct platform_device *pdev)
>  {
>  	struct device_node *msi_parent_dn;
> @@ -210,7 +224,14 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
>  	struct irq_domain *irq_domain;
>  	struct mvebu_icu *icu;
>  
> -	icu = dev_get_drvdata(dev);
> +	/*
> +	 * Device data being populated means we are using the legacy bindings.
> +	 * Using the parent device data means we are using the new bindings.
> +	 */
> +	if (dev_get_drvdata(dev))
> +		icu = dev_get_drvdata(dev);
> +	else
> +		icu = dev_get_drvdata(dev->parent);

We now have a static key to determine which binding to use, and we
should certainly have set it correctly by the time we start end up
here. Why aren't you using it?

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.