Re: [PATCH v4 4/7] can: replace can_dlc as variable/element for payload length

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Vincent,

On 09.11.20 13:59, Vincent MAILHOL wrote:
> On Mon. 9 Nov 2020 at 19:26, Oliver Hartkopp wrote:
>> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
>> index b2e8df8e4cb0..72671184a7a2 100644
>> --- a/include/linux/can/dev.h
>> +++ b/include/linux/can/dev.h
>> @@ -183,12 +183,12 @@ static inline void can_set_static_ctrlmode(struct net_device *dev,
>>          /* override MTU which was set by default in can_setup()? */
>>          if (static_mode & CAN_CTRLMODE_FD)
>>                  dev->mtu = CANFD_MTU;
>>   }
>>
>> -/* get data length from can_dlc with sanitized can_dlc */
>> -u8 can_dlc2len(u8 can_dlc);
>> +/* get data length from raw data length code (DLC) */
> 
> /*
>   * convert a given data length code (dlc) of an FD CAN frame into a
>   * valid data length of max. 64 bytes.
>   */
> 
> I missed this point during my previous review: the can_dlc2len() function
> is only valid for CAN FD frames. Comments should reflect this fact.
> 
>> +u8 can_dlc2len(u8 dlc);
> 
> Concerning the name:
>   * can_get_cc_len() converts a Classical CAN frame DLC into a data
>     length.
>   * can_dlc2len() converts an FD CAN frame DLC into a data length.
> 
> Just realized that both macro/function do similar things so we could
> think of a similar naming as well.
>   * Example 1: can_get_cc_len() and can_get_fd_len()
>   * Example 2: can_cc_dlc2len() and can_fd_dlc2len()

I like!

Patch set v5 is out now.

Thanks,
Oliver

> 
> Or we could simply leave things as they are, this is not a big issue
> as long as the comments clearly state which one is for classical
> frames and which one is for FD frames.
> 
>>
>>   /* map the sanitized data length to an appropriate data length code */
>>   u8 can_len2dlc(u8 len);
> 
> can_len2dlc() might be renamed (e.g. can_get_fd_dlc()) if Example 1
> solution is chosen.
> 
>>   struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
> 
> Yours sincerely,
> Vincent Mailhol
>