On 02/06/2025 at 22:49, Geert Uytterhoeven wrote:
> 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...
Not necessarily, you can just have:
rcar_canfd_f_dcfg()
rcar_canfd_f_cfdcfg()
...
and so on. Those would be used exactly the same way as the current function like
macro.
Or you can have:
#define RCANFD_F_DCFG_BASE 0x00
#define RCANFD_F_CFDCFS_BASE 0x04
...
and then a:
rcar_canfd_get_c_reg(coffset, base, m)
I am keen on letting you decide what you think is best.
>> 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 ;-)
Yours sincerely,
Vincent Mailhol
