Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Dec 2022 10:20:52 +0000
Message-ID: <86o7rxawhn.wl-maz@kernel.org>
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
In-Reply-To: <20221221000242.340202-5-prabhakar.mahadev-lad.rj@bp.renesas.com>
References: <20221221000242.340202-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
        <20221221000242.340202-5-prabhakar.mahadev-lad.rj@bp.renesas.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.