Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	Stephen Boyd <swboyd@chromium.org>,
	linux-serial@vger.kernel.org,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
Date: Thu, 16 Mar 2023 17:13:00 +0100	[thread overview]
Message-ID: <CAMuHMdVX=sM-1y+-3PQDsjf8K3nLoS4WfSYfv6UAP=92T_oHaQ@mail.gmail.com> (raw)
In-Reply-To: <20230316160118.133182-1-biju.das.jz@bp.renesas.com>

Hi Biju,

On Thu, Mar 16, 2023 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt
> is transmit end interrupt, so shuffle the interrupts to fix the
> transmit end interrupt handler for these IPs.
>
> Fixes: 392fb8df528b ("serial: sh-sci: Use platform_get_irq_optional() for optional interrupts")

I don't think that's the right bad commit.

> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -31,6 +31,7 @@
>  #include <linux/ioport.h>
>  #include <linux/ktime.h>
>  #include <linux/major.h>
> +#include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/mm.h>
>  #include <linux/of.h>
> @@ -2841,6 +2842,7 @@ static int sci_init_single(struct platform_device *dev,
>         struct uart_port *port = &sci_port->port;
>         const struct resource *res;
>         unsigned int i;
> +       int irq_cnt;
>         int ret;
>
>         sci_port->cfg   = p;
> @@ -2864,6 +2866,14 @@ static int sci_init_single(struct platform_device *dev,
>                         sci_port->irqs[i] = platform_get_irq(dev, i);
>         }
>
> +       /*
> +        * RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt
> +        * is transmit end interrupt, so shuffle the interrupts.
> +        */
> +       irq_cnt = platform_irq_count(dev);
> +       if (irq_cnt == 4)
> +               swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port->irqs[SCIx_TEI_IRQ]);
> +

I think it's simpler to just check if SCIx_TEI_IRQ is missing:

    if (sci_port->irqs[SCIx_TEI_IRQ] < 0)
            swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port->irqs[SCIx_TEI_IRQ]);

We already rely on "make dtbs_check" to catch invalid combinations
(anything different from 1/4/6 interrupts).

And please move that code below, together with the other checks for
non-existing interrupts.

>         /* The SCI generates several interrupts. They can be muxed together or
>          * connected to different interrupt lines. In the muxed case only one
>          * interrupt resource is specified as there is only one interrupt ID.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds