Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> > Sent: 02 July 2025 08:38 > Subject: Re: [PATCH] can: rcar_canfd: Drop unused macros > > 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@wanad > > oo.fr/ > > 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. Yes, you are correct. Initially I kept these bit definitions and removed Only register offsets. Then I ran make W=2 drivers/net/can/rcar/rcar_canfd.o to make sure there are no more warnings. I can keep the bit definitions if it is OK to everyone. > > [...] > > > @@ -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... OK. > > > #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? + 1 > > > -#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. I am OK for restoring it back. > > > #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 OK. > > #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"). OK. But removing unused reg offset is not an issue at the moment, I guess?? Cheers, Biju
