Hi Vincent,
On Thu, 12 Jun 2025 at 06:00, Vincent Mailhol
<mailhol.vincent@xxxxxxxxxx> wrote:
> On 12/06/2025 at 00:37, Geert Uytterhoeven wrote:
> > Reuse the existing Channel Data Bitrate Configuration Register offset
> > member in the register configuration as the base offset for all related
> > channel-specific registers.
> > Rename the member and update the (incorrect) comment to reflect this.
> > Replace the function-like channel-specific register offset macros by
> > inline functions.
> >
> > This fixes the offsets of all other (currently unused) channel-specific
> > registers on R-Car Gen4 and RZ/G3E, and allows us to replace
> > RCANFD_GEN4_FDCFG() by the more generic rcar_canfd_f_cfdcfg().
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > ---
> > v2:
> > - Add Reviewed-by.
> > - Replace function-like macros by inline functions,
>
> Thanks!
> Thinking of your code, you are still using some magic numbers, e.g.
>
> 0x04 + 0x20 * ch
>
> to access your registers. But at the end those magic numbers are just describing
> a memory layout.
>
> I think this can be describe as a C structure. This is what I have in mind:
>
> --------------8<--------------
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 1e559c0ff038..487f40320c20 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -560,10 +560,21 @@ struct rcar_canfd_channel {
> spinlock_t tx_lock; /* To protect tx path */
> };
>
> +struct rcar_canfd_f {
> + u32 dcfg;
> + u32 cfdcfg;
> + u32 cfdctr;
> + u32 cfdsts;
> + u32 cfdcrc;
> + u32 padding[3];
> +};
> +static_assert(sizeof(struct rcar_canfd_f) == 0x20);
Is that really needed?
> @@ -883,8 +883,7 @@ static void rcar_canfd_set_mode(struct rcar_canfd_global *gpriv)
>
> for_each_set_bit(ch, &gpriv->channels_mask,
> gpriv->info->max_channels)
> - rcar_canfd_set_bit(gpriv->base,
> - rcar_canfd_f_cfdcfg(gpriv, ch), val);
> + rcar_canfd_set_bit_reg(&gpriv->cbase[ch].cfdcfg, val);
Nice!
> To be honnest, I am happy to accept your patch as it is now, but what
> do you think of the above? I think that this approach works with your
> other macro as well.
Please take this as-is, so we can move forward.
I will create a proper patch (with your Suggested-by) later,
I have more CAN-FD items on my TODO list...
Thanks!
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
