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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2231 bytes --]

On 11/10/20 7:55 AM, Oliver Hartkopp wrote:
>>> 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 regex to replace can_cc_dlc2len() with can_get_cc_len() might be simpler,
but passing the cf by reference _and_ assigning the return value to a member of
cf looks strange.

> 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.

OK
>> 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.

OK

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]