Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Ulrich Hecht <uli+renesas@fpond.eu>
Cc: linux-renesas-soc@vger.kernel.org, netdev@vger.kernel.org,
	davem@davemloft.net, linux-can@vger.kernel.org,
	prabhakar.mahadev-lad.rj@bp.renesas.com,
	biju.das.jz@bp.renesas.com, wsa@kernel.org,
	yoshihiro.shimoda.uh@renesas.com, wg@grandegger.com,
	mkl@pengutronix.de, kuba@kernel.org, socketcan@hartkopp.net,
	geert@linux-m68k.org, kieran.bingham@ideasonboard.com
Subject: Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC
Date: Mon, 31 Jan 2022 20:04:06 +0900	[thread overview]
Message-ID: <CAMZ6Rq+o6Di8wAQeAB4_yq+jNBoWvGTZ297+5jCc1=KSC9f0EA@mail.gmail.com> (raw)
In-Reply-To: <20220111162231.10390-3-uli+renesas@fpond.eu>

 Two more comments!

On Mon. 12 Jan 2022 at 01:22, Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> Adds support for the CANFD IP variant in the V3U SoC.
>
> Differences to controllers in other SoCs are limited to an increase in
> the number of channels from two to eight, an absence of dedicated
> registers for "classic" CAN mode, and a number of differences in magic
> numbers (register offsets and layouts).
>
> Inspired by BSP patch by Kazuya Mizuguchi.
>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 231 ++++++++++++++++++++----------
>  1 file changed, 153 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index ff9d0f5ae0dd..b1c9870d2a82 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c

...

> @@ -1488,22 +1543,29 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
>  static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
>  {
>         struct net_device_stats *stats = &priv->ndev->stats;
> +       struct rcar_canfd_global *gpriv = priv->gpriv;
>         struct canfd_frame *cf;
>         struct sk_buff *skb;
>         u32 sts = 0, id, dlc;
>         u32 ch = priv->channel;
>         u32 ridx = ch + RCANFD_RFFIFO_IDX;
>
> -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> +           gpriv->chip_id == RENESAS_R8A779A0) {
>                 id = rcar_canfd_read(priv->base, RCANFD_F_RFID(ridx));
>                 dlc = rcar_canfd_read(priv->base, RCANFD_F_RFPTR(ridx));
>
>                 sts = rcar_canfd_read(priv->base, RCANFD_F_RFFDSTS(ridx));
> -               if (sts & RCANFD_RFFDSTS_RFFDF)
> -                       skb = alloc_canfd_skb(priv->ndev, &cf);
> -               else
> +               if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +                       if (sts & RCANFD_RFFDSTS_RFFDF)
> +                               skb = alloc_canfd_skb(priv->ndev, &cf);
> +                       else
> +                               skb = alloc_can_skb(priv->ndev,
> +                                                   (struct can_frame **)&cf);
> +               } else {
>                         skb = alloc_can_skb(priv->ndev,
>                                             (struct can_frame **)&cf);

It seems to me that we can factorize the two alloc_can_skb() calls:

+               if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
+                   sts & RCANFD_RFFDSTS_RFFDF)
+                       skb = alloc_canfd_skb(priv->ndev, &cf);
+               else
+                       skb = alloc_can_skb(priv->ndev, (struct
can_frame **)&cf);

> +               }
>         } else {
>                 id = rcar_canfd_read(priv->base, RCANFD_C_RFID(ridx));
>                 dlc = rcar_canfd_read(priv->base, RCANFD_C_RFPTR(ridx));
> @@ -1541,10 +1603,16 @@ static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
>                 }
>         } else {
>                 cf->len = can_cc_dlc2len(RCANFD_RFPTR_RFDLC(dlc));
> -               if (id & RCANFD_RFID_RFRTR)
> +               if (id & RCANFD_RFID_RFRTR) {
>                         cf->can_id |= CAN_RTR_FLAG;
> -               else
> -                       rcar_canfd_get_data(priv, cf, RCANFD_C_RFDF(ridx, 0));
> +               } else {
> +                       if (gpriv->chip_id == RENESAS_R8A779A0)
> +                               rcar_canfd_get_data(priv, cf,
> +                                                   RCANFD_F_RFDF(ridx, 0));
> +                       else
> +                               rcar_canfd_get_data(priv, cf,
> +                                                   RCANFD_C_RFDF(ridx, 0));
> +               }

Put the else if on a single line and remove one level of indentation:

+               else if (gpriv->chip_id == RENESAS_R8A779A0)
+                       rcar_canfd_get_data(priv, cf, RCANFD_F_RFDF(ridx, 0));
+               else
+                       rcar_canfd_get_data(priv, cf, RCANFD_C_RFDF(ridx, 0));

Also, a global comment, once you turn IS_V3U to an inline
function, you can use it in place of the many
"gpriv->chip_id == RENESAS_R8A779A0" checks.

Yours sincerely,
Vincent Mailhol