Hi Vincent, > -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > Sent: 28 March 2025 09:37 > Subject: Re: [PATCH v7 05/18] can: rcar_canfd: Drop RCANFD_GERFL_EEF* macros in RCANFD_GERFL_ERR > > On 28/03/2025 at 18:21, Biju Das wrote: > > Hi Vincent, > > > > Thanks for the feedback. > > > >> -----Original Message----- > >> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > >> Sent: 28 March 2025 09:10 > >> Subject: Re: [PATCH v7 05/18] can: rcar_canfd: Drop RCANFD_GERFL_EEF* > >> macros in RCANFD_GERFL_ERR > >> > >> On 26/03/2025 à 21:19, Biju Das wrote: > >>> The macros RCANFD_GERFL_EEF* in RCANFD_GERFL_ERR can be replaced by > >>> gpriv->channels_mask << 16. > >>> > >>> After this drop the macro RCANFD_GERFL_EEF0_7 as it is unused. > >>> > >>> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > >>> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > >>> --- > >>> v6->v7: > >>> * No change. > >>> v5->v6: > >>> * Collected tag. > >>> v5: > >>> * New patch. > >>> --- > >>> drivers/net/can/rcar/rcar_canfd.c | 5 +---- > >>> 1 file changed, 1 insertion(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/net/can/rcar/rcar_canfd.c > >>> b/drivers/net/can/rcar/rcar_canfd.c > >>> index 565a91c2ca83..a9e962a1510e 100644 > >>> --- a/drivers/net/can/rcar/rcar_canfd.c > >>> +++ b/drivers/net/can/rcar/rcar_canfd.c > >>> @@ -74,7 +74,6 @@ > >>> #define RCANFD_GSTS_GNOPM (BIT(0) | BIT(1) | BIT(2) | BIT(3)) > >>> > >>> /* RSCFDnCFDGERFL / RSCFDnGERFL */ > >>> -#define RCANFD_GERFL_EEF0_7 GENMASK(23, 16) > >>> #define RCANFD_GERFL_EEF(ch) BIT(16 + (ch)) > >>> #define RCANFD_GERFL_CMPOF BIT(3) /* CAN FD only */ > >>> #define RCANFD_GERFL_THLES BIT(2) > >>> @@ -82,9 +81,7 @@ > >>> #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->channels_mask << 16) | RCANFD_GERFL_MES | \ > >> > >> The previous RCANFD_GERFL_EEF0_7 macro documented that the register > >> was from bit index 16 to bit index 23. Here, we loose this piece of information. > >> > >> What about: > >> > >> FIELD_PREP(RCANFD_GERFL_EEF0_7, gpriv->channels_mask) > > > > For all SoCs, ECC Error flag for Channel x (a.k.a. EEFx) starts from BIT position 16. > > > > By using gpriv->channels_mask, we allow only enabled channels and << > > 16 says it is from Bit position 16. > > Yes, it starts at bit 16, but at which bit does it end? > > The GENMASK() helps to document the register names. Of course is works if you replace the FIELD_PREP > with a left shift, but you are replacing some meaningful information about the register name, register > start bit and end bit by just a start bit value. See? You just lost the register name and end bit > info. > > FIELD_PREP() is not only about doing the correct shift but also about documenting that you are putting > the value into a specific register. > > When reading: > > FIELD_PREP(RCANFD_GERFL_EEF0_7, gpriv->channels_mask) > > I immediately understand that you are putting the gpriv->channels_mask value into the GERFL_EEF0_7 > register and I can look at the datasheet for details about that GERFL_EEF0_7 if I want to. Gen4 has 8 channels (GENMASK(16, 23) G3E has 6 channels (GENMASK(16, 21) V4M has 4 channels (GENMASK(16, 19) V3H_2 has 3 channels (GENMASK(16,18) All other SoCs has 2 channels (GENMASK(16,17) So you mean, I should replace RCANFD_GERFL_EEF0_7 with a Generic one RCANFD_GERFL_EEF(x) GENMASK(16, 16 + (x) - 1) to handle this differences Where x is the number of supported channels(info->max_channels) and then use FIELD_PREP(RCANFD_GERFL_EEF(x), gpriv->channels_mask) Cheers, Biju
