RE: [PATCH v2 2/3] can: rcar_canfd: Fix channel specific IRQ handling for RZ/G2L

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: "Wolfgang Grandegger" <wg@grandegger.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Vincent Mailhol" <mailhol.vincent@wanadoo.fr>,
	"Stefan Mätje" <stefan.maetje@esd.eu>,
	"Ulrich Hecht" <uli+renesas@fpond.eu>,
	"Fabrizio Castro" <fabrizio.castro.jz@renesas.com>,
	"Prabhakar Mahadev Lad" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"Oliver Hartkopp" <socketcan@hartkopp.net>,
	"Christophe JAILLET" <christophe.jaillet@wanadoo.fr>,
	"linux-can@vger.kernel.org" <linux-can@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"Geert Uytterhoeven" <geert+renesas@glider.be>,
	"Chris Paterson" <Chris.Paterson2@renesas.com>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v2 2/3] can: rcar_canfd: Fix channel specific IRQ handling for RZ/G2L
Date: Wed, 26 Oct 2022 09:34:41 +0000	[thread overview]
Message-ID: <OS0PR01MB5922FCC50E590DFDD041F99386309@OS0PR01MB5922.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <20221026073608.7h47b2axcayakfnn@pengutronix.de>

Hi Marc,

Thanks for the review.

> Subject: Re: [PATCH v2 2/3] can: rcar_canfd: Fix channel specific IRQ
> handling for RZ/G2L
> 
> On 25.10.2022 16:56:56, Biju Das wrote:
> > RZ/G2L has separate channel specific IRQs for transmit and error
> > interrupt. But the IRQ handler, process the code for both channels
> > even if there is no interrupt occurred on one of the channels.
> >
> > This patch fixes the issue by passing channel specific context
> > parameter instead of global one for irq register and on irq handler,
> > it just handles the channel which is triggered the interrupt.
> 
> Please clean up signatures of the IRQ handlers you touch, it's a
> little mess. Change:
> 
> | rcar_canfd_handle_channel_tx(struct rcar_canfd_global *gpriv, u32
> ch)
> 
> to:
> 
> | rcar_canfd_handle_channel_tx(struct rcar_canfd_channel *priv)
> 
> Same for:
> 
> | static void rcar_canfd_handle_channel_err(struct rcar_canfd_global
> | *gpriv, u32 ch)
> 

OK.

> 
> 
> In a separate patch, please clean up these, too:
> 
> | static void rcar_canfd_handle_global_err(struct rcar_canfd_global
> | *gpriv, u32 ch) static void rcar_canfd_handle_global_receive(struct
> | rcar_canfd_global *gpriv, u32 ch) static void
> | rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 ch)
> 
> Why are 2 of the above functions called "global" as they work on a
> specific channel? That can be streamlined, too.
> 

The function name is as per the hardware manual, Interrupt sources are classified into global and channel interrupts.

• Global interrupts (2 sources):
— Receive FIFO interrupt
— Global error interrupt
• Channel interrupts (3 sources/channel):

Maybe we could change "rcar_canfd_handle_global_receive"->"rcar_canfd_handle_channel_receive", as from driver point
It is not global anymore?? Please let me know.

Cheers,
Biju