Hi Biju, Thanks for your patch! On Thu, 6 Mar 2025 at 13:43, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote: > R-Car Gen3 and Gen4 has some differences in the mask bits. Add a have > mask table to handle these differences. > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> Suggestions for improvements below. > --- a/drivers/net/can/rcar/rcar_canfd.c > +++ b/drivers/net/can/rcar/rcar_canfd.c > @@ -82,25 +82,24 @@ > #define RCANFD_GERFL_DEF BIT(0) > > #define RCANFD_GERFL_ERR(gpriv, x) \ > - ((x) & (reg_gen4(gpriv, RCANFD_GERFL_EEF0_7, \ > - RCANFD_GERFL_EEF(0) | RCANFD_GERFL_EEF(1)) | \ > - RCANFD_GERFL_MES | \ > + ((x) & ((gpriv)->info->mask_table[RCANFD_EEF_MASK] | RCANFD_GERFL_MES | \ > ((gpriv)->fdmode ? RCANFD_GERFL_CMPOF : 0))) RZ/G3E documents only EFF bits 0-5. I think this can just be replaced by gpriv->channels_mask << 16, especially as there is only one user of RCANFD_GERFL_ERR(). > > /* AFL Rx rules registers */ > > /* RSCFDnCFDGAFLCFG0 / RSCFDnGAFLCFG0 */ > #define RCANFD_GAFLCFG_SETRNC(gpriv, n, x) \ > - (((x) & reg_gen4(gpriv, 0x1ff, 0xff)) << \ > + (((x) & (gpriv)->info->mask_table[RCANFD_RNC_MASK]) << \ > (reg_gen4(gpriv, 16, 24) - ((n) & 1) * reg_gen4(gpriv, 16, 8))) All other mask values are just the maximum value of a parameter. E.g. the RNC mask is the number of supported rules - 1. So perhaps store these as parameters instead? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds