Re: Usb to can driver

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

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

On 05/02/2013 01:07 PM, Marc Kleine-Budde wrote:
> On 04/24/2013 11:33 PM, Max S. wrote:
>> From 8a83080bcddc558cd016e58e408b56ca68bc5bc8 Mon Sep 17 00:00:00 2001
>> From: Maximilian Schneider <max@schneidersoft.net>
>> Date: Wed, 24 Apr 2013 21:03:31 +0000
>> Subject: [PATCH] Added ss_usb source.
> 
> Comments inline.
> 
>> Signed-off-by: Maximilian Schneider <max@schneidersoft.net>
>> ---
>>  drivers/net/can/usb/Makefile |    1 +
> 
> You should add your driver to Kconfig, too.
> 
>>  drivers/net/can/usb/ss_usb.c |  944 ++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 945 insertions(+)
>>  create mode 100644 drivers/net/can/usb/ss_usb.c
>>
>> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
>> index becef46..c341a1b 100644
>> --- a/drivers/net/can/usb/Makefile
>> +++ b/drivers/net/can/usb/Makefile
>> @@ -7,5 +7,6 @@ obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
>>  obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o
>>  obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
>>  obj-$(CONFIG_CAN_8DEV_USB) += usb_8dev.o
>> +obj-$(CONFIG_CAN_SS_USB) += ss_usb.o
>>  
>>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
>> diff --git a/drivers/net/can/usb/ss_usb.c b/drivers/net/can/usb/ss_usb.c
>> new file mode 100644
>> index 0000000..69710ff
>> --- /dev/null
>> +++ b/drivers/net/can/usb/ss_usb.c
>> @@ -0,0 +1,944 @@
> 
> Please add a header stating your license.
> 
>> +#include <linux/init.h>
>> +#include <linux/signal.h>
>> +#include <linux/module.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/usb.h>
>> +
>> +#include <linux/can.h>
>> +#include <linux/can/dev.h>
>> +#include <linux/can/error.h>
>> +
>> +/* Device specific constants */
>> +#define USB_SSUSB_VENDOR_ID         0xbeef
>> +#define USB_SSUSB_AT32_PRODUCT_ID   0x1234
> 
> Please get in touch with the Openmoko people for a real vendor/product
> id as Sven suggested. Sven can you arrange a contect?
> 
>> +
>> +#define SS_USB_ENDPOINT_IN          1
>> +#define SS_USB_ENDPOINT_OUT         2
>> +
>> +#define SS_USB_BREQ_HOST_FORMAT     0
>> +#define SS_USB_BREQ_BITTIMING       1
>> +#define SS_USB_BREQ_MODE            2
>> +#define SS_USB_BREQ_BERR            3
>> +#define SS_USB_BREQ_BT_CONST        4
>> +
>> +
>> +#define SS_CAN_MODE_RESET           0
>> +#define SS_CAN_MODE_START           1
>> +
>> +#define SS_CAN_STATE_ERROR_ACTIVE   0
>> +#define SS_CAN_STATE_ERROR_WARNING  1
>> +#define SS_CAN_STATE_ERROR_PASSIVE  2
>> +#define SS_CAN_STATE_BUS_OFF        3
>> +#define SS_CAN_STATE_STOPPED        4
>> +#define SS_CAN_STATE_SLEEPING       5
> 
> Can you pelase create an enum for each of these.
> 
>> +
>> +/* data types passed between host and device */
>> +struct __packed device_config {
>> +	u16 byte_order;
> 
> u32, byteorder please, so that you have natural alignment for this struct.
> 
>> +
>> +	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;
>> +};

Please give your struct a common prefix "ss_" like your defines already
have.

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