Am 28.06.2018 um 18:18 schrieb Wolfgang Grandegger:
> Hello Florian,
>
> Am 28.06.2018 um 15:52 schrieb Florian Ferg:
>> Hello Wolfgang,
>>
>> On Wed, 27 Jun 2018, Wolfgang Grandegger wrote:
>>
>>> ... but there is still duplicated code. The following functions in
>>> ixxat_usb_cl1.c and ixxat_usb_cl2.c are identical:
>>>
>>> ixxat_usb_handle_status()
>>> ixxat_usb_handle_error()
>>>
>>> And almost identical are:
>>>
>>> ixxat_usb_handle_canmsg()
>>> ixxat_usb_decode_buf()
>>> ixxat_usb_encode_msg()
>>>
>>> Could you move the netif and skb handling to the main part, at least...
>>> Looking at:
>>>
>>> $ meld ixxat_usb_cl1.c ixxat_usb_cl2.c
>>>
>>> I really wonder if you are not just done with a few
>>>
>>> if (priv->adapter == XYZ)
>>>
>>> Wolfgang.
>>>
>>
>> CL1 and CL2 have different can message formats.
>> This is why the message handling was implemented in the adapters.
>
> OK, just the message data are at a different offset. That's why the code
> to interpret the raw messages is rather similar.
>
>> I moved the can message handling functions to the main part now.
>> The adapter type is now checked to select the proper message format.
>> (See the following patch.)
>
> To avoid the intermediate copy from and to "can_msg", why not using
> unions like the Kvasar and other drivers do:
>
> struct ixxat_can_msg {
> u8 size;
> __le32 time;
> __le32 msg_id;
> __le32 flags;
> union {
> struct {
> u8 data[CAN_MAX_DLEN];
> } __packed cl1;
> struct {
> __le32 client_id;
> u8 data[CANFD_MAX_DLEN];
> } __packed cl2;
> } __packed;
> } __packed;
>
> Then you just need an "if" or better a callback to set and get the data.
It's probably not worth a separate callback. Doing a review of you patch
later today...
Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
