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.
What do you think?
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
