Re: Usb to can driver

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

[-- Attachment #1: Type: text/plain, Size: 2444 bytes --]

On 04/24/2013 05:48 PM, Max S. wrote:
>> Please use "./scripts/checkpatch.pl" from the kernel and fix all
>> coding style errors. Then repost :).

Please check your patch with ./scripts/checkpatch.pl, fix the errors,
then repost.

>> Can you explain how the "struct device_config" works.
>>
> struct __packed device_config {
> 	u16 byte_order;
> 
> 	u32 hf_size;	/* size of a host frame */
> 	u32 hf_can_id;
> 	u32 hf_can_dlc;
> 	u32 hf_data;
> 	u32 hf_echo_id;
> 	u32 hf_channel;
> } conf;
> 
> conf = (struct device_config){
> 	.byte_order = 0xbeef,
> 	.hf_size = sizeof(struct ss_host_frame),
> 	.hf_can_id = offsetof(struct ss_host_frame, frame.can_id),
> 	.hf_can_dlc = offsetof(struct ss_host_frame, frame.can_dlc),
> 	.hf_data = offsetof(struct ss_host_frame, frame.data),
> 	.hf_echo_id = offsetof(struct ss_host_frame, echo_id),
> 	.hf_channel = offsetof(struct ss_host_frame, channel),
> };
> 
> ... send conf to device, packed that should be 26 bytes.
> 
> The device uses byte_order which will appear as either 0xbeef or 0xefbe
> to convert the other members to device byte order. (and to convert all
> future messages)

Use a u32 here instead of an u16, this way the struct is properly aligned.

> The device can then tell how the host prefers its can frames formatted.
> The idea is that the struct device_config holds enough data to allow the
> device to construct a struct ss_host_frame in host format, including
> padding & byte order.

Nice.

> since a struct ss_host_frame is simply a wrapper that includes a struct
> can_frame, when a message arrives from the device, the host can copy the
> struct can frame member without modification from the struct
> ss_host_frame into the skb.
> 
> assumptions that are made:
> * The sizes of the struct can_frame members are id:u32 dlc:u8 data:u64 .
> * It is also assumed that existing defines like CAN_EFF_FLAG in can.h
> and can/error.h don't change, as they are used by the device to
> construct the can_id field.

You cannot rely in your firmware, that the struct can_frame and
CAN_*_FLAG doesn't change. Please define your own struct.

Marc

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


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