Re: [PATCH v3 1/4] can: rcar_canfd: Add support for r8a779a0 SoC

[Ulrich Hecht <uli@fpond.eu>]

Thanks for your review!

> On 02/14/2022 8:10 AM Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote:
> > -#define RCANFD_GERFL_ERR(gpriv, x)     ((x) & (RCANFD_GERFL_EEF1 |\
> > -                                       RCANFD_GERFL_EEF0 | RCANFD_GERFL_MES |\
> > -                                       (gpriv->fdmode ?\
> > -                                        RCANFD_GERFL_CMPOF : 0)))
> > +#define RCANFD_GERFL_ERR(x)            ((x) & (reg_v3u(gpriv, RCANFD_GERFL_EEF0_7, \
> > +                                       RCANFD_GERFL_EEF0 | RCANFD_GERFL_EEF1) | \
> > +                                       RCANFD_GERFL_MES | ((gpriv)->fdmode ? \
> > +                                       RCANFD_GERFL_CMPOF : 0)))
> 
> Instead of packing everything on the right, I suggest putting in a bit more air.
> Something like that:
> 
> #define RCANFD_GERFL_ERR(x)                                             \
>         ((x) & (reg_v3u(gpriv, RCANFD_GERFL_EEF0_7,                     \
>                         RCANFD_GERFL_EEF0 | RCANFD_GERFL_EEF1) |        \
>                 RCANFD_GERFL_MES |                                      \
>                 ((gpriv)->fdmode ? RCANFD_GERFL_CMPOF : 0)))
> 
> Same comment applies to other macros.

That does indeed look a lot better.

> >  /* Helper functions */
> > +static inline bool is_v3u(struct rcar_canfd_global *gpriv)
> > +{
> > +       return gpriv->chip_id == RENESAS_R8A779A0;
> > +}
> > +
> > +static inline u32 reg_v3u(struct rcar_canfd_global *gpriv,
> > +                         u32 v3u, u32 not_v3u)
> > +{
> > +       return is_v3u(gpriv) ? v3u : not_v3u;
> > +}
> 
> Nitpick but I would personally prefer if is_v3u() and reg_v3u()
> were declared before the macros in which they are being used.

So would I, but that would require extensive reshuffling of the code, which I think is not worth the effort for such a minor issue.

> > +               if (is_v3u(gpriv)) {
> > +                       cfg = (RCANFD_NCFG_NTSEG1(tseg1) |
> > +                              RCANFD_NCFG_NBRP(brp) |
> > +                              RCANFD_NCFG_NSJW(sjw) |
> > +                              RCANFD_NCFG_NTSEG2(tseg2));
> > +               } else {
> > +                       cfg = (RCANFD_CFG_TSEG1(tseg1) |
> > +                              RCANFD_CFG_BRP(brp) |
> > +                              RCANFD_CFG_SJW(sjw) |
> > +                              RCANFD_CFG_TSEG2(tseg2));
> > +               }
> 
> Nitpick: can't you use one of your reg_v3u() functions here?
> |        cfg = reg_v3u(gpriv, ..., ...)?

Technically yes, but I think of reg_v3u() as choosing between two register layouts, and this is something else.

CU
Uli