Hi Vincent, Thanks for the feedback. > -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > Sent: 28 March 2025 11:21 > Subject: Re: [PATCH v7 08/18] can: rcar_canfd: Simplify RCANFD_GAFLCFG_SETRNC macro > > On 28/03/2025 at 19:54, Biju Das wrote: > > Hi Vincent, > > > >> -----Original Message----- > >> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > >> Sent: 28 March 2025 10:39 > >> Subject: Re: [PATCH v7 08/18] can: rcar_canfd: Simplify > >> RCANFD_GAFLCFG_SETRNC macro > >> > >> On 26/03/2025 at 21:19, Biju Das wrote: > >>> The shift values in RCANFD_GAFLCFG_SETRNC are dictated by the field width: > >>> - R-Car Gen4 packs 2 values in a 32-bit word, using a field width > >>> of 16 bits, > >>> - R-Car Gen3 packs up to 4 values in a 32-bit word, using a field > >>> width of 8 bits. > >>> > >>> Add rnc_field_width variable to struct rcar_canfd_hw_info to handle > >>> this difference and simplify the shift value in > >>> RCANFD_GAFLCFG_SETRNC macro by using a formula (32 - (n % rnc_stride + 1) * rnc_field_width). > >>> > >>> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > >>> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > >>> --- > >>> v6->7: > >>> * Collected tag. > >>> v5->6: > >>> * Replaced RCANFD_RNC_PER_REG macro with rnc_stride variable. > >>> * Updated commit description > >>> * Dropped the Rb tag. > >>> v5: > >>> * New patch. > >>> --- > >>> drivers/net/can/rcar/rcar_canfd.c | 6 +++++- > >>> 1 file changed, 5 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/net/can/rcar/rcar_canfd.c > >>> b/drivers/net/can/rcar/rcar_canfd.c > >>> index 0001c8043c25..62cde1efa0c0 100644 > >>> --- a/drivers/net/can/rcar/rcar_canfd.c > >>> +++ b/drivers/net/can/rcar/rcar_canfd.c > >>> @@ -89,7 +89,7 @@ > >>> /* RSCFDnCFDGAFLCFG0 / RSCFDnGAFLCFG0 */ #define > >>> RCANFD_GAFLCFG_SETRNC(gpriv, n, x) \ > >>> (((x) & ((gpriv)->info->num_supported_rules - 1)) << \ > >>> - (reg_gen4(gpriv, 16, 24) - ((n) & 1) * reg_gen4(gpriv, 16, 8))) > >>> + (32 - (((n) % (gpriv)->info->rnc_stride + 1) * > >>> +(gpriv)->info->rnc_field_width))) > >> > >> I can not follow how this is the same. Let's take the gen4 as an example. Before: > >> > >> (reg_gen4(gpriv, 16, 24) - ((n) & 1) * reg_gen4(gpriv, 16, 8))) = > >> 16 - ((n & 1) * 16) > >> > >> So, I have: > >> > >> n shift value > >> --------------------------------- > >> 0 16 - ((0 & 1) * 16) = 0 > >> 1 16 - ((1 & 1) * 16) = 16 > >> 2 16 - ((2 & 1) * 16) = 0 > >> 3 16 - ((3 & 1) * 16) = 16 > >> 4 16 - ((4 & 1) * 16) = 0 > >> > >> After: > >> > >> (32 - ((n % rnc_stride + 1)) * rnc_field_width) = > > > > 32 - (n % rnc_stride) + 1 = > >> 32 - ((n % (2 + 1)) * 16) = > > > > > >> 32 - ((n % 3) * 16) > > 32 - ((n % 2) + 1)) * 16) = > > > >> > >> n shift value > >> --------------------------------- > >> 0 32 - ((0 % 3) * 16) = 32 > >> 1 32 - ((1 % 3) * 16) = 16 > >> 2 32 - ((2 % 3) * 16) = 0 > >> 3 32 - ((3 % 3) * 16) = 32 > >> 4 32 - ((4 % 3) * 16) = 16 > >> > >> Is there something wrong in my calculation? What am I missing? > > > > 0 32 - ((0 % 2) + 1) * 16) = 16 > > 1 32 - ((1 % 2) + 1) * 16) = 0 > > OK. Thanks. I didn't parse the parenthesis correctly. It's been a long week, sorry for the noise. > > >> More generally, it is really hard to review and understand what this macro does. > > > > Macro is doing a simple calculation. > > > > (32 - (n % rnc_stride + 1) * rnc_field_width) > > This is before adding the tons of parenthesis. I did my review of best effort and got it wrong not > because the calculation is complication but because of the noise introduced by those parenthesis (plus > admittedly some fatigue from the long week…) I agree, too much parenthesis in macro is a noise. > > >> Can add one more patch: > >> > >> can: rcar_canfd: turn RCANFD_GAFLCFG_SETRNC into a function > >> > >> and then apply your change? > >> > >> I do not see the reason why this needs to be a macro. If you make > >> this a function, at least, it will be easier to follow what is going > >> on and the compiler optimizer will inline it anyway so you should not get any penalty. > > > > I am leaving Marc, Geert to provide their feedback on this. > > The maintainers of the CAN drivers and Marc and myself. Of course, I do appreciate Geert feedback > here. But for doing this maintenance as a volunteer, in exchange, I want you to make my review work > (and the long term maintenance) easier. > > function-like macro are not welcome unless strictly needed and I am not keen on dropping this ask. OK, will convert this into a function. Cheers, Biju
