Hi Vincent,
On Mon, 2 Jun 2025 at 15:16, Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> wrote:
> On 02/06/2025 at 20:54, 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.
> >
> > This fixes the addresses 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 RCANFD_F_CFDCFG().
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > ---
> > drivers/net/can/rcar/rcar_canfd.c | 22 ++++++++++------------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> > index 0cad3c198e58e494..7a9a88fa5fb1a521 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -425,18 +425,16 @@
> > #define RCANFD_C_RPGACC(r) (0x1900 + (0x04 * (r)))
> >
> > /* R-Car Gen4 Classical and CAN FD mode specific register map */
> > -#define RCANFD_GEN4_FDCFG(m) (0x1404 + (0x20 * (m)))
> > -
> > #define RCANFD_GEN4_GAFL_OFFSET (0x1800)
> >
> > /* CAN FD mode specific register map */
> >
> > /* RSCFDnCFDCmXXX -> RCANFD_F_XXX(m) */
> > -#define RCANFD_F_DCFG(gpriv, m) ((gpriv)->info->regs->f_dcfg + (0x20 * (m)))
> > -#define RCANFD_F_CFDCFG(m) (0x0504 + (0x20 * (m)))
> > -#define RCANFD_F_CFDCTR(m) (0x0508 + (0x20 * (m)))
> > -#define RCANFD_F_CFDSTS(m) (0x050c + (0x20 * (m)))
> > -#define RCANFD_F_CFDCRC(m) (0x0510 + (0x20 * (m)))
> > +#define RCANFD_F_DCFG(gpriv, m) ((gpriv)->info->regs->coffset + 0x00 + (0x20 * (m)))
> > +#define RCANFD_F_CFDCFG(gpriv, m) ((gpriv)->info->regs->coffset + 0x04 + (0x20 * (m)))
> > +#define RCANFD_F_CFDCTR(gpriv, m) ((gpriv)->info->regs->coffset + 0x08 + (0x20 * (m)))
> > +#define RCANFD_F_CFDSTS(gpriv, m) ((gpriv)->info->regs->coffset + 0x0c + (0x20 * (m)))
> > +#define RCANFD_F_CFDCRC(gpriv, m) ((gpriv)->info->regs->coffset + 0x10 + (0x20 * (m)))
>
> I really start to dislike all those function like macros in the rcar_canfd
> driver. The only benefits of a function like macro is either to have type
> polymorphism or to generate integer constant expression or to work with context
> specific info (e.g. __func__ or __LINE__).
I agree much can be improved in the way this driver accesses registers.
Unfortunately a large part of it is due to the horrendous naming of the
registers in the documentation, and the two different register layouts.
> Can you just change these five function like macros to static functions?
I assume you want something like was done commit 6b9f8b53a1f3ad8e
("can: rcar_canfd: Add rcar_canfd_setrnc()")?
These five macro just calculate the offsets for specific registers
and for the specified channel indices. Their return values are to
be passed to one of the five accessors that take register offsets
(rcar_canfd_{read,write,set_bit,cleat_bit, update}()). Hence
converting the macros to accessor functions means there will be more
than five functions...
> And from now on, each time there is a need to modify one of the rcar_canfd, I
> would like this to become an opportunity to little by little clean up that macro
> madness.
That's exactly what Biju and I are doing, slowly ;-)
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
