On 12/06/2025 at 20:33, Geert Uytterhoeven wrote:
> 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?
It is needed to counterbalance my lack of confidence on whether I am able to
count up to 8 :)
It is just for debug, you can remove. Similarly, feel free to adjust the names.
I just wanted to convey my idea, and a piece of code was easier than a long
paragraph for that.
>> @@ -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);
I was wondering if there was some kind of helper function to do that? If not,
maybe adding a
io_set_bitl(void __iomem *addr, val);
and so on into linux/io.h could be a good idea?
> 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.
@Marc, review is done, the series is ready for pick-up.
> I will create a proper patch (with your Suggested-by) later,
> I have more CAN-FD items on my TODO list...
No problem, take your time! My only wish is to not see more complexity added the
the function-like macros. Any other refactor is just a bonus!
Yours sincerely,
Vincent Mailhol
