Hi Biju, On Sun, 29 Jun 2025 at 17:04, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote: > Drop unused macros from the rcar_canfd.c. > > Reported-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > Closes: https://lore.kernel.org/all/7ff93ff9-f578-4be2-bdc6-5b09eab64fe6@xxxxxxxxxx/ > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> Thanks for your patch! > --- a/drivers/net/can/rcar/rcar_canfd.c > +++ b/drivers/net/can/rcar/rcar_canfd.c > @@ -51,25 +51,17 @@ > #define RCANFD_GCFG_EEFE BIT(6) > #define RCANFD_GCFG_CMPOC BIT(5) /* CAN FD only */ > #define RCANFD_GCFG_DCS BIT(4) > -#define RCANFD_GCFG_DCE BIT(1) > -#define RCANFD_GCFG_TPRI BIT(0) Does it make sense to remove all simple bit definitions? They serve as documentation. [...] > @@ -121,15 +108,9 @@ > #define RCANFD_NCFG_NBRP(x) (((x) & 0x3ff) << 0) > > /* RSCFDnCFDCmCTR / RSCFDnCmCTR */ > -#define RCANFD_CCTR_CTME BIT(24) CTME will be needed for adding listen-only and loopback support. Of course it can be added back later... > #define RCANFD_CCTR_ERRD BIT(23) > #define RCANFD_CCTR_BOM_MASK (0x3 << 21) > -#define RCANFD_CCTR_BOM_ISO (0x0 << 21) > #define RCANFD_CCTR_BOM_BENTRY (0x1 << 21) > -#define RCANFD_CCTR_BOM_BEND (0x2 << 21) If the driver uses one of the possible field values, why not keep the other values for documentation purposes? > -#define RCANFD_CCTR_TDCVFIE BIT(19) > -#define RCANFD_CCTR_SOCOIE BIT(18) > -#define RCANFD_CCTR_EOCOIE BIT(17) > #define RCANFD_CCTR_TAIE BIT(16) > #define RCANFD_CCTR_ALIE BIT(15) > #define RCANFD_CCTR_BLIE BIT(14) > @@ -196,13 +169,6 @@ > #define RCANFD_FDCFG_TDCOC BIT(8) > > /* RSCFDnCFDCmFDSTS */ > -#define RCANFD_FDSTS_SOC GENMASK(31, 24) > -#define RCANFD_FDSTS_EOC GENMASK(23, 16) > -#define RCANFD_GEN4_FDSTS_TDCVF BIT(15) > -#define RCANFD_GEN4_FDSTS_PNSTS GENMASK(13, 12) > -#define RCANFD_FDSTS_SOCO BIT(9) > -#define RCANFD_FDSTS_EOCO BIT(8) > -#define RCANFD_FDSTS_TDCVF BIT(7) Does it make sense to remove all simple bit and proper mask definitions? They serve as documentation. > #define RCANFD_FDSTS_TDCR GENMASK(7, 0) > > /* RSCFDnCFDRFCCx */ > @@ -215,7 +181,6 @@ > /* RSCFDnCFDRFSTSx */ > #define RCANFD_RFSTS_RFIF BIT(3) > #define RCANFD_RFSTS_RFMLT BIT(2) > -#define RCANFD_RFSTS_RFFLL BIT(1) > #define RCANFD_RFSTS_RFEMP BIT(0) > > /* RSCFDnCFDRFIDx */ > @@ -224,8 +189,6 @@ > > /* RSCFDnCFDRFPTRx */ > #define RCANFD_RFPTR_RFDLC(x) (((x) >> 28) & 0xf) > -#define RCANFD_RFPTR_RFPTR(x) (((x) >> 16) & 0xfff) > -#define RCANFD_RFPTR_RFTS(x) (((x) >> 0) & 0xffff) OK, as macros like this will probably be reworked anyway when these ever become used with FIELD_{GET,PREP}(), .e.g #define RCANFD_RFPTR_RFDLC GENMASK(31, 28) > @@ -298,16 +256,10 @@ > #define RCANFD_GSTS (0x008c) > /* RSCFDnCFDGERFL / RSCFDnGERFL */ > #define RCANFD_GERFL (0x0090) > -/* RSCFDnCFDGTSC / RSCFDnGTSC */ > -#define RCANFD_GTSC (0x0094) Note that removed register offsets will become anonymous gaps when the register offsets are replaced by C structs, cfr. commit ab2aa5453bb83d05 ("can: rcar_canfd: Describe channel-specific FD registers using C struct"). > /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */ > #define RCANFD_GAFLECTR (0x0098) > /* RSCFDnCFDGAFLCFG / RSCFDnGAFLCFG */ > #define RCANFD_GAFLCFG(w) (0x009c + (0x04 * (w))) > -/* RSCFDnCFDRMNB / RSCFDnRMNB */ > -#define RCANFD_RMNB (0x00a4) > -/* RSCFDnCFDRMND / RSCFDnRMND */ > -#define RCANFD_RMND(y) (0x00a8 + (0x04 * (y))) > > /* RSCFDnCFDRFCCx / RSCFDnRFCCx */ > #define RCANFD_RFCC(gpriv, x) ((gpriv)->info->regs->rfcc + (0x04 * (x))) > @@ -482,23 +358,11 @@ struct rcar_canfd_f_c { > (RCANFD_F_CFOFFSET(gpriv) + 0x0c + (0x180 * (ch)) + (0x80 * (idx)) + \ > (0x04 * (df))) > > -/* RSCFDnCFDTMXXp -> RCANFD_F_TMXX(p) */ > -#define RCANFD_F_TMID(p) (0x4000 + (0x20 * (p))) > -#define RCANFD_F_TMPTR(p) (0x4004 + (0x20 * (p))) > -#define RCANFD_F_TMFDCTR(p) (0x4008 + (0x20 * (p))) > -#define RCANFD_F_TMDF(p, b) (0x400c + (0x20 * (p)) + (0x04 * (b))) > - > -/* RSCFDnCFDTHLACCm */ > -#define RCANFD_F_THLACC(m) (0x6000 + (0x04 * (m))) > -/* RSCFDnCFDRPGACCr */ > -#define RCANFD_F_RPGACC(r) (0x6400 + (0x04 * (r))) OK, as probably the high register offsets will never be used. > - > /* Constants */ > #define RCANFD_FIFO_DEPTH 8 /* Tx FIFO depth */ > #define RCANFD_NAPI_WEIGHT 8 /* Rx poll quota */ > > #define RCANFD_NUM_CHANNELS 8 /* Eight channels max */ > -#define RCANFD_CHANNELS_MASK BIT((RCANFD_NUM_CHANNELS) - 1) Yeah, this one was never used. > > #define RCANFD_GAFL_PAGENUM(entry) ((entry) / 16) > #define RCANFD_CHANNEL_NUMRULES 1 /* only one rule per channel */ As the patch is correct: Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> 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
