Thanks for your review!
> On 02/14/2022 8:10 AM Vincent MAILHOL <mailhol.vincent@xxxxxxxxxx> 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
