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. When reading: gpriv->channels_mask << 16 I just see the magic number 16. No clue what this means at first glance. > Gen4 has 7 channels > G3E has 6 channels > V4M has 4 channels > V3H_2 has 3 channels > All other SoCs has 2 channels > > Am I missing anything here? Yours sincerely, Vincent Mailhol
