Re: [PATCH v2 4/9] irqchip: irq-renesas-rzg2l: Add support for RZ/G2UL SoC

From: Marc Zyngier <maz@kernel.org>
To: Prabhakar <prabhakar.csengg@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Magnus Damm <magnus.damm@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, linux-gpio@vger.kernel.org,
	Biju Das <biju.das.jz@bp.renesas.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: Re: [PATCH v2 4/9] irqchip: irq-renesas-rzg2l: Add support for RZ/G2UL SoC
Date: Wed, 21 Dec 2022 10:20:52 +0000	[thread overview]
Message-ID: <86o7rxawhn.wl-maz@kernel.org> (raw)
In-Reply-To: <20221221000242.340202-5-prabhakar.mahadev-lad.rj@bp.renesas.com>

On Wed, 21 Dec 2022 00:02:37 +0000,
Prabhakar <prabhakar.csengg@gmail.com> wrote:
> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> The IRQC block on RZ/G2UL SoC is almost identical to one found on the
> RZ/G2L SoC the only difference being it can support BUS_ERR_INT for
> which it has additional registers.
> 
> This patch adds a new entry for "renesas,rzg2ul-irqc" compatible string
> and now that we have interrupt-names property the driver code parses the
> interrupts based on names and for backward compatibility we fallback to
> parse interrupts based on index.
> 
> For now we will be using rzg2l_irqc_init() as a callback for RZ/G2UL SoC
> too and in future when the interrupt handler will be registered for
> BUS_ERR_INT we will have to implement a new callback.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Since you're posting from a different address, please add a second SoB
with your gmail address.

> ---
> v1 -> v2
> * New patch
> ---
>  drivers/irqchip/irq-renesas-rzg2l.c | 80 ++++++++++++++++++++++++++---
>  1 file changed, 74 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> index 7918fe201218..5bdf0106ef51 100644
> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -299,19 +299,86 @@ static const struct irq_domain_ops rzg2l_irqc_domain_ops = {
>  	.translate = irq_domain_translate_twocell,
>  };
>  
> -static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
> -				       struct device_node *np)
> +static int rzg2l_irqc_parse_interrupt_to_fwspec(struct rzg2l_irqc_priv *priv,
> +						struct device_node *np,
> +						unsigned int index,
> +						unsigned int fwspec_index)
>  {
>  	struct of_phandle_args map;
> +	int ret;
> +
> +	ret = of_irq_parse_one(np, index, &map);
> +	if (ret)
> +		return ret;
> +
> +	of_phandle_args_to_fwspec(np, map.args, map.args_count,
> +				  &priv->fwspec[fwspec_index]);
> +
> +	return 0;
> +}
> +
> +static int rzg2l_irqc_parse_interrupt_by_name_to_fwspec(struct rzg2l_irqc_priv *priv,
> +							struct device_node *np,
> +							char *irq_name,
> +							unsigned int fwspec_index)
> +{
> +	int index;
> +
> +	index = of_property_match_string(np, "interrupt-names", irq_name);
> +	if (index < 0)
> +		return index;
> +
> +	return rzg2l_irqc_parse_interrupt_to_fwspec(priv, np, index, fwspec_index);
> +}
> +
> +/* Parse hierarchy domain interrupts ie only IRQ0-7 and TINT0-31 */
> +static int rzg2l_irqc_parse_hierarchy_interrupts(struct rzg2l_irqc_priv *priv,
> +						 struct device_node *np)
> +{
> +	struct property *pp;
>  	unsigned int i;
>  	int ret;
>  
> +	/*
> +	 * first check if interrupt-names property exists if so parse them by name
> +	 * or else parse them by index for backward compatibility.
> +	 */
> +	pp = of_find_property(np, "interrupt-names", NULL);
> +	if (pp) {
> +		char *irq_name;
> +
> +		/* parse IRQ0-7 */
> +		for (i = 0; i < IRQC_IRQ_COUNT; i++) {
> +			irq_name = kasprintf(GFP_KERNEL, "irq%d", i);
> +			if (!irq_name)
> +				return -ENOMEM;
> +
> +			ret = rzg2l_irqc_parse_interrupt_by_name_to_fwspec(priv, np, irq_name, i);

Am I the only one that find it rather odd to construct a name from an
index, only to get another index back?

In any case, the string stuff could be moved into
rzg2l_irqc_parse_interrupt_by_name_to_fwspec(). Which could really do
with a name shortening)... rzg2l_irqc_name_to_fwspec? Same thing for
the other function (rzg2l_irqc_index_to_fwspec).

	M.

-- 
Without deviation from the norm, progress is not possible.