Hi Vincent,
On Fri, 28 Mar 2025 at 12:21, Vincent Mailhol
<mailhol.vincent@xxxxxxxxxx> wrote:
> On 28/03/2025 at 19:54, Biju Das wrote:
> >> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> >> 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>
> >>> --- 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…)
>
> >> 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.
I agree the heavy use of macros can be improved.
E.g. handling the access to the RNC field is spread across
two macros:
- RCANFD_GAFLCFG_SETRNC() handles ch % something,
- RCANFD_GAFLCFG() handles ch / something.
A single function to write to the RNC field would make this more readable.
This is just an example, there are many more of these...
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
