Re: [PATCH v5 8/8] can-dev: add len8_dlc support for various CAN USB adapters

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 09.11.20 21:12, Marc Kleine-Budde wrote:
> On 11/9/20 4:36 PM, Oliver Hartkopp wrote:
>> Support the Classical CAN raw DLC functionality to send and receive DLC
>> values from 9 .. 15 on various Classical CAN capable USB network drivers:
>>
>> - gs_usb
>> - pcan_usb
>> - pcan_usb_fd
>> - usb_8dev
>>
>> Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>>   drivers/net/can/usb/gs_usb.c               |  8 ++++++--
>>   drivers/net/can/usb/peak_usb/pcan_usb.c    |  8 ++++++--
>>   drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 17 ++++++++++++-----
>>   drivers/net/can/usb/usb_8dev.c             |  9 ++++++---
>>   4 files changed, 30 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
>> index 940589667a7f..cc0c30a33335 100644
>> --- a/drivers/net/can/usb/gs_usb.c
>> +++ b/drivers/net/can/usb/gs_usb.c
>> @@ -330,10 +330,13 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>>   			return;
>>   
>>   		cf->can_id = hf->can_id;
>>   
>>   		cf->len = can_cc_dlc2len(hf->len);
>> +		cf->len8_dlc = can_get_len8_dlc(dev->can.ctrlmode, cf->len,
>> +						hf->len);
> 
> What about introducing a function that sets len and len8_dlc at the same time:
> 
> void can_frame_set_length(const struct can_priv *can, struct can_frame *cfd, u8
> dlc);

Good idea.

I would suggest something like

u8 can_get_cc_len(const u32 ctrlmode, struct can_frame *cf, u8 dlc)

that still returns the 'len' element, so that we can replace 
can_cc_dlc2len() with can_get_cc_len() for CAN drivers that add support 
for len8_dlc.

The assignment cf->len = can_get_cc_len() fits better into the code 
which assigns cf->can_id too.

And I would stay on 'u32 ctrlmode' as ctrlmode is the parameter which is 
namely needed here. A pointer to can_priv can mean anything.

> 
> And maybe a function that takes a canfd_frame, so that we don't need to cast....

No. The len8_dlc element is from struct can_frame. When people use the 
struct canfd_frame in their driver this might have some benefits for them.
But when it comes to access the len8_dlc element this has to be casted IMO.

But with the suggested can_get_cc_len() function a needed cast could be 
put into the parameter list without adding extra code somewhere else in 
the driver.

(..)


>> @@ -1033,11 +1040,11 @@ static const struct can_bittiming_const pcan_usb_fd_data_const = {
>>   
>>   const struct peak_usb_adapter pcan_usb_fd = {
>>   	.name = "PCAN-USB FD",
>>   	.device_id = PCAN_USBFD_PRODUCT_ID,
>>   	.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
>> -	.ctrlmode_supported = CAN_CTRLMODE_FD |
>> +	.ctrlmode_supported = CAN_CTRLMODE_FD | CAN_CTRLMODE_CC_LEN8_DLC |
>>   			CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
> 
> Please add the new CTRLMODE at the end, so that the list ist sorted. I don't
> mind if the diff is a bit larger.
> 

I had to decide between "make the patch looking good" and "make the 
result looking good" ;-)

But I'm fine with "make the patch looking good" too. Will change that in v6.

Best regards,
Oliver