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
