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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



 Two more comments!

On Mon. 12 Jan 2022 at 01:22, Ulrich Hecht <uli+renesas@xxxxxxxx> 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@xxxxxxxx>
> ---
>  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



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux