[PATCH 0/3] Add support for PEAK CAN-FD USB adapters

[PATCH 0/3] Add support for PEAK CAN-FD USB adapters

[Stephane Grosjean]

That serie of three patches adds support for the new PEAK-System Technik USB
adapters that enable access to CAN-FD standard, from the linux-can socket based
API.

Stephane Grosjean (3):
  can/peak_usb: CAN-FD: existing source files modifications
  can/peak_usb: CAN-FD: add new adapters specific files
  can/peak_usb: CAN-FD: driver's core modifications

 drivers/net/can/usb/Kconfig                  |  14 +-
 drivers/net/can/usb/peak_usb/Makefile        |   2 +-
 drivers/net/can/usb/peak_usb/pcan_ucan.h     | 208 ++++++
 drivers/net/can/usb/peak_usb/pcan_usb.c      |   1 +
 drivers/net/can/usb/peak_usb/pcan_usb_core.c |  84 ++-
 drivers/net/can/usb/peak_usb/pcan_usb_core.h |  14 +-
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 955 +++++++++++++++++++++++++++
 drivers/net/can/usb/peak_usb/pcan_usb_fd.h   | 108 +++
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c  |  22 +-
 drivers/net/can/usb/peak_usb/pcan_usb_pro.h  |   8 +
 10 files changed, 1377 insertions(+), 39 deletions(-)
 create mode 100644 drivers/net/can/usb/peak_usb/pcan_ucan.h
 create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.c
 create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.h

-- 
1.9.1

[PATCH 1/3] can/peak_usb: CAN-FD: existing source files modifications

[Stephane Grosjean]

This patch does some modifications to the existing files that support the
PEAK-System Technik CAN 2.0b USB adapters, for preparing the support of CAN-FD.

In particular, this patch changes some static identifiers into global ones, to
share common functionalities with the further incoming CAN-FD specific source
files.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb.c     |  1 +
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 22 +++++++---------------
 drivers/net/can/usb/peak_usb/pcan_usb_pro.h |  8 ++++++++
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 925ab8e..10a4ceb 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -858,6 +858,7 @@ struct peak_usb_adapter pcan_usb = {
 	.name = "PCAN-USB",
 	.device_id = PCAN_USB_PRODUCT_ID,
 	.ctrl_count = 1,
+	.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
 	.clock = {
 		.freq = PCAN_USB_CRYSTAL_HZ / 2 ,
 	},
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index 263dd92..10887b0 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -27,14 +27,6 @@
 
 MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro adapter");
 
-/* PCAN-USB Pro Endpoints */
-#define PCAN_USBPRO_EP_CMDOUT		1
-#define PCAN_USBPRO_EP_CMDIN		(PCAN_USBPRO_EP_CMDOUT | USB_DIR_IN)
-#define PCAN_USBPRO_EP_MSGOUT_0		2
-#define PCAN_USBPRO_EP_MSGIN		(PCAN_USBPRO_EP_MSGOUT_0 | USB_DIR_IN)
-#define PCAN_USBPRO_EP_MSGOUT_1		3
-#define PCAN_USBPRO_EP_UNUSED		(PCAN_USBPRO_EP_MSGOUT_1 | USB_DIR_IN)
-
 #define PCAN_USBPRO_CHANNEL_COUNT	2
 
 /* PCAN-USB Pro adapter internal clock (MHz) */
@@ -322,8 +314,8 @@ static int pcan_usb_pro_wait_rsp(struct peak_usb_device *dev,
 	return (i >= PCAN_USBPRO_RSP_SUBMIT_MAX) ? -ERANGE : err;
 }
 
-static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
-				 int req_value, void *req_addr, int req_size)
+int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
+			  int req_value, void *req_addr, int req_size)
 {
 	int err;
 	u8 req_type;
@@ -333,8 +325,6 @@ static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
 	if (!(dev->state & PCAN_USB_STATE_CONNECTED))
 		return 0;
 
-	memset(req_addr, '\0', req_size);
-
 	req_type = USB_TYPE_VENDOR | USB_RECIP_OTHER;
 
 	switch (req_id) {
@@ -345,6 +335,7 @@ static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
 	default:
 		p = usb_rcvctrlpipe(dev->udev, 0);
 		req_type |= USB_DIR_IN;
+		memset(req_addr, '\0', req_size);
 		break;
 	}
 
@@ -476,7 +467,7 @@ static int pcan_usb_pro_set_bittiming(struct peak_usb_device *dev,
 	return pcan_usb_pro_set_bitrate(dev, ccbt);
 }
 
-static void pcan_usb_pro_restart_complete(struct urb *urb)
+void pcan_usb_pro_restart_complete(struct urb *urb)
 {
 	/* can delete usb resources */
 	peak_usb_async_complete(urb);
@@ -932,7 +923,7 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
 
 	return 0;
 
- err_out:
+err_out:
 	kfree(bi);
 	kfree(fi);
 	kfree(usb_if);
@@ -978,7 +969,7 @@ static void pcan_usb_pro_free(struct peak_usb_device *dev)
 /*
  * probe function for new PCAN-USB Pro usb interface
  */
-static int pcan_usb_pro_probe(struct usb_interface *intf)
+int pcan_usb_pro_probe(struct usb_interface *intf)
 {
 	struct usb_host_interface *if_desc;
 	int i;
@@ -1016,6 +1007,7 @@ struct peak_usb_adapter pcan_usb_pro = {
 	.name = "PCAN-USB Pro",
 	.device_id = PCAN_USBPRO_PRODUCT_ID,
 	.ctrl_count = PCAN_USBPRO_CHANNEL_COUNT,
+	.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
 	.clock = {
 		.freq = PCAN_USBPRO_CRYSTAL_HZ,
 	},
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
index 32275af..1101c9c 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
@@ -27,6 +27,14 @@
 #define PCAN_USBPRO_INFO_BL		0
 #define PCAN_USBPRO_INFO_FW		1
 
+/* PCAN-USB Pro (FD) Endpoints */
+#define PCAN_USBPRO_EP_CMDOUT		1
+#define PCAN_USBPRO_EP_CMDIN		(PCAN_USBPRO_EP_CMDOUT | USB_DIR_IN)
+#define PCAN_USBPRO_EP_MSGOUT_0		2
+#define PCAN_USBPRO_EP_MSGIN		(PCAN_USBPRO_EP_MSGOUT_0 | USB_DIR_IN)
+#define PCAN_USBPRO_EP_MSGOUT_1		3
+#define PCAN_USBPRO_EP_UNUSED		(PCAN_USBPRO_EP_MSGOUT_1 | USB_DIR_IN)
+
 /* Vendor Request value for XXX_FCT */
 #define PCAN_USBPRO_FCT_DRVLD		5 /* tell device driver is loaded */
 #define PCAN_USBPRO_FCT_DRVLD_REQ_LEN	16
-- 
1.9.1

Re: [PATCH 1/3] can/peak_usb: CAN-FD: existing source files modifications

[Oliver Hartkopp]

On 27.11.2014 11:32, Stephane Grosjean wrote:

> @@ -322,8 +314,8 @@ static int pcan_usb_pro_wait_rsp(struct peak_usb_device *dev,
>   	return (i >= PCAN_USBPRO_RSP_SUBMIT_MAX) ? -ERANGE : err;
>   }
>
> -static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
> -				 int req_value, void *req_addr, int req_size)
> +int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
> +			  int req_value, void *req_addr, int req_size)
>   {
>   	int err;
>   	u8 req_type;
> @@ -333,8 +325,6 @@ static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
>   	if (!(dev->state & PCAN_USB_STATE_CONNECTED))
>   		return 0;
>
> -	memset(req_addr, '\0', req_size);
> -
>   	req_type = USB_TYPE_VENDOR | USB_RECIP_OTHER;
>
>   	switch (req_id) {
> @@ -345,6 +335,7 @@ static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
>   	default:
>   		p = usb_rcvctrlpipe(dev->udev, 0);
>   		req_type |= USB_DIR_IN;
> +		memset(req_addr, '\0', req_size);
>   		break;
>   	}
>

This movement of the memset also looks like a stable candidate, right?

Best regards,
Oliver

Re: [PATCH 1/3] can/peak_usb: CAN-FD: existing source files modifications

[Marc Kleine-Budde]

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

On 11/27/2014 11:32 AM, Stephane Grosjean wrote:
> This patch does some modifications to the existing files that support the
> PEAK-System Technik CAN 2.0b USB adapters, for preparing the support of CAN-FD.
> 
> In particular, this patch changes some static identifiers into global ones, to
> share common functionalities with the further incoming CAN-FD specific source
> files.

The subject of this patch should describe what it does and the
description why. You should not split your patches by source files it
touches but thematically. Only handle one problem per patch.

> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> ---
>  drivers/net/can/usb/peak_usb/pcan_usb.c     |  1 +
>  drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 22 +++++++---------------
>  drivers/net/can/usb/peak_usb/pcan_usb_pro.h |  8 ++++++++
>  3 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 925ab8e..10a4ceb 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -858,6 +858,7 @@ struct peak_usb_adapter pcan_usb = {
>  	.name = "PCAN-USB",
>  	.device_id = PCAN_USB_PRODUCT_ID,
>  	.ctrl_count = 1,
> +	.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,

This change seems unrelated to the make functions non static.

and FTBFS:

> linux/drivers/net/can/usb/peak_usb/pcan_usb.c:861:2: error: unknown field ‘ctrlmode_supported’ specified in initializer
>   .ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
>   ^
> linux/drivers/net/can/usb/peak_usb/pcan_usb.c:861:2: warning: initialization makes pointer from integer without a cast
> linux/drivers/net/can/usb/peak_usb/pcan_usb.c:861:2: warning: (near initialization for ‘pcan_usb.intf_probe’)

>  	.clock = {
>  		.freq = PCAN_USB_CRYSTAL_HZ / 2 ,
>  	},
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> index 263dd92..10887b0 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> @@ -27,14 +27,6 @@
>  
>  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro adapter");
>  
> -/* PCAN-USB Pro Endpoints */
> -#define PCAN_USBPRO_EP_CMDOUT		1
> -#define PCAN_USBPRO_EP_CMDIN		(PCAN_USBPRO_EP_CMDOUT | USB_DIR_IN)
> -#define PCAN_USBPRO_EP_MSGOUT_0		2
> -#define PCAN_USBPRO_EP_MSGIN		(PCAN_USBPRO_EP_MSGOUT_0 | USB_DIR_IN)
> -#define PCAN_USBPRO_EP_MSGOUT_1		3
> -#define PCAN_USBPRO_EP_UNUSED		(PCAN_USBPRO_EP_MSGOUT_1 | USB_DIR_IN)
> -
>  #define PCAN_USBPRO_CHANNEL_COUNT	2
>  
>  /* PCAN-USB Pro adapter internal clock (MHz) */
> @@ -322,8 +314,8 @@ static int pcan_usb_pro_wait_rsp(struct peak_usb_device *dev,
>  	return (i >= PCAN_USBPRO_RSP_SUBMIT_MAX) ? -ERANGE : err;
>  }
>  
> -static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
> -				 int req_value, void *req_addr, int req_size)
> +int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
> +			  int req_value, void *req_addr, int req_size)
>  {
>  	int err;
>  	u8 req_type;
> @@ -333,8 +325,6 @@ static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
>  	if (!(dev->state & PCAN_USB_STATE_CONNECTED))
>  		return 0;
>  
> -	memset(req_addr, '\0', req_size);
> -
>  	req_type = USB_TYPE_VENDOR | USB_RECIP_OTHER;
>  
>  	switch (req_id) {
> @@ -345,6 +335,7 @@ static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
>  	default:
>  		p = usb_rcvctrlpipe(dev->udev, 0);
>  		req_type |= USB_DIR_IN;
> +		memset(req_addr, '\0', req_size);

Why is this memset moved?

>  		break;
>  	}
>  
> @@ -476,7 +467,7 @@ static int pcan_usb_pro_set_bittiming(struct peak_usb_device *dev,
>  	return pcan_usb_pro_set_bitrate(dev, ccbt);
>  }
>  
> -static void pcan_usb_pro_restart_complete(struct urb *urb)
> +void pcan_usb_pro_restart_complete(struct urb *urb)
>  {
>  	/* can delete usb resources */
>  	peak_usb_async_complete(urb);
> @@ -932,7 +923,7 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
>  
>  	return 0;
>  
> - err_out:
> +err_out:

Please don't change this.

>  	kfree(bi);
>  	kfree(fi);
>  	kfree(usb_if);
> @@ -978,7 +969,7 @@ static void pcan_usb_pro_free(struct peak_usb_device *dev)
>  /*
>   * probe function for new PCAN-USB Pro usb interface
>   */
> -static int pcan_usb_pro_probe(struct usb_interface *intf)
> +int pcan_usb_pro_probe(struct usb_interface *intf)
>  {
>  	struct usb_host_interface *if_desc;
>  	int i;
> @@ -1016,6 +1007,7 @@ struct peak_usb_adapter pcan_usb_pro = {
>  	.name = "PCAN-USB Pro",
>  	.device_id = PCAN_USBPRO_PRODUCT_ID,
>  	.ctrl_count = PCAN_USBPRO_CHANNEL_COUNT,
> +	.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
>  	.clock = {
>  		.freq = PCAN_USBPRO_CRYSTAL_HZ,
>  	},
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> index 32275af..1101c9c 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> @@ -27,6 +27,14 @@
>  #define PCAN_USBPRO_INFO_BL		0
>  #define PCAN_USBPRO_INFO_FW		1
>  
> +/* PCAN-USB Pro (FD) Endpoints */
> +#define PCAN_USBPRO_EP_CMDOUT		1
> +#define PCAN_USBPRO_EP_CMDIN		(PCAN_USBPRO_EP_CMDOUT | USB_DIR_IN)
> +#define PCAN_USBPRO_EP_MSGOUT_0		2
> +#define PCAN_USBPRO_EP_MSGIN		(PCAN_USBPRO_EP_MSGOUT_0 | USB_DIR_IN)
> +#define PCAN_USBPRO_EP_MSGOUT_1		3
> +#define PCAN_USBPRO_EP_UNUSED		(PCAN_USBPRO_EP_MSGOUT_1 | USB_DIR_IN)
> +
>  /* Vendor Request value for XXX_FCT */
>  #define PCAN_USBPRO_FCT_DRVLD		5 /* tell device driver is loaded */
>  #define PCAN_USBPRO_FCT_DRVLD_REQ_LEN	16
> 

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: 819 bytes --]

Re: [PATCH 1/3] can/peak_usb: CAN-FD: existing source files modifications

[Stephane Grosjean]

> -------- Original Message --------
> Subject: Re: [PATCH 1/3] can/peak_usb: CAN-FD: existing source files
> modifications
> Date: Thu, 27 Nov 2014 22:27:17 +0100
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> To: Stephane Grosjean <s.grosjean@peak-system.com>,
> linux-can@vger.kernel.org
> CC: Oliver Hartkopp <socketcan@hartkopp.net>
>
> On 11/27/2014 11:32 AM, Stephane Grosjean wrote:
>> This patch does some modifications to the existing files that support the
>> PEAK-System Technik CAN 2.0b USB adapters, for preparing the support of CAN-FD.
>>
>> In particular, this patch changes some static identifiers into global ones, to
>> share common functionalities with the further incoming CAN-FD specific source
>> files.
> The subject of this patch should describe what it does and the
> description why. You should not split your patches by source files it
> touches but thematically. Only handle one problem per patch.

Ok.

>
>> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
>> ---
>>   drivers/net/can/usb/peak_usb/pcan_usb.c     |  1 +
>>   drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 22 +++++++---------------
>>   drivers/net/can/usb/peak_usb/pcan_usb_pro.h |  8 ++++++++
>>   3 files changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
>> index 925ab8e..10a4ceb 100644
>> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
>> @@ -858,6 +858,7 @@ struct peak_usb_adapter pcan_usb = {
>>   	.name = "PCAN-USB",
>>   	.device_id = PCAN_USB_PRODUCT_ID,
>>   	.ctrl_count = 1,
>> +	.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
> This change seems unrelated to the make functions non static.

Ok.

>
> and FTBFS:
>
>> linux/drivers/net/can/usb/peak_usb/pcan_usb.c:861:2: error: unknown field ‘ctrlmode_supported’ specified in initializer
>>    .ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
>>    ^
>> linux/drivers/net/can/usb/peak_usb/pcan_usb.c:861:2: warning: initialization makes pointer from integer without a cast
>> linux/drivers/net/can/usb/peak_usb/pcan_usb.c:861:2: warning: (near initialization for ‘pcan_usb.intf_probe’)

well, this FTBS is "normal" while you doesn't apply PATCH 3/3. I thought 
a "serie" of patches was not cleavable... Can u confirm, please?

>>   	.clock = {
>>   		.freq = PCAN_USB_CRYSTAL_HZ / 2 ,
>>   	},
>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>> index 263dd92..10887b0 100644
>> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>> @@ -27,14 +27,6 @@
>>   
>>   MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro adapter");
>>   
>> -/* PCAN-USB Pro Endpoints */
>> -#define PCAN_USBPRO_EP_CMDOUT		1
>> -#define PCAN_USBPRO_EP_CMDIN		(PCAN_USBPRO_EP_CMDOUT | USB_DIR_IN)
>> -#define PCAN_USBPRO_EP_MSGOUT_0		2
>> -#define PCAN_USBPRO_EP_MSGIN		(PCAN_USBPRO_EP_MSGOUT_0 | USB_DIR_IN)
>> -#define PCAN_USBPRO_EP_MSGOUT_1		3
>> -#define PCAN_USBPRO_EP_UNUSED		(PCAN_USBPRO_EP_MSGOUT_1 | USB_DIR_IN)
>> -
>>   #define PCAN_USBPRO_CHANNEL_COUNT	2
>>   
>>   /* PCAN-USB Pro adapter internal clock (MHz) */
>> @@ -322,8 +314,8 @@ static int pcan_usb_pro_wait_rsp(struct peak_usb_device *dev,
>>   	return (i >= PCAN_USBPRO_RSP_SUBMIT_MAX) ? -ERANGE : err;
>>   }
>>   
>> -static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
>> -				 int req_value, void *req_addr, int req_size)
>> +int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
>> +			  int req_value, void *req_addr, int req_size)
>>   {
>>   	int err;
>>   	u8 req_type;
>> @@ -333,8 +325,6 @@ static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
>>   	if (!(dev->state & PCAN_USB_STATE_CONNECTED))
>>   		return 0;
>>   
>> -	memset(req_addr, '\0', req_size);
>> -
>>   	req_type = USB_TYPE_VENDOR | USB_RECIP_OTHER;
>>   
>>   	switch (req_id) {
>> @@ -345,6 +335,7 @@ static int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
>>   	default:
>>   		p = usb_rcvctrlpipe(dev->udev, 0);
>>   		req_type |= USB_DIR_IN;
>> +		memset(req_addr, '\0', req_size);
> Why is this memset moved?

It isn't part of this patch since v2.

>
>>   		break;
>>   	}
>>   
>> @@ -476,7 +467,7 @@ static int pcan_usb_pro_set_bittiming(struct peak_usb_device *dev,
>>   	return pcan_usb_pro_set_bitrate(dev, ccbt);
>>   }
>>   
>> -static void pcan_usb_pro_restart_complete(struct urb *urb)
>> +void pcan_usb_pro_restart_complete(struct urb *urb)
>>   {
>>   	/* can delete usb resources */
>>   	peak_usb_async_complete(urb);
>> @@ -932,7 +923,7 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
>>   
>>   	return 0;
>>   
>> - err_out:
>> +err_out:
> Please don't change this.

ok.
>
>>   	kfree(bi);
>>   	kfree(fi);
>>   	kfree(usb_if);
>> @@ -978,7 +969,7 @@ static void pcan_usb_pro_free(struct peak_usb_device *dev)
>>   /*
>>    * probe function for new PCAN-USB Pro usb interface
>>    */
>> -static int pcan_usb_pro_probe(struct usb_interface *intf)
>> +int pcan_usb_pro_probe(struct usb_interface *intf)
>>   {
>>   	struct usb_host_interface *if_desc;
>>   	int i;
>> @@ -1016,6 +1007,7 @@ struct peak_usb_adapter pcan_usb_pro = {
>>   	.name = "PCAN-USB Pro",
>>   	.device_id = PCAN_USBPRO_PRODUCT_ID,
>>   	.ctrl_count = PCAN_USBPRO_CHANNEL_COUNT,
>> +	.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
>>   	.clock = {
>>   		.freq = PCAN_USBPRO_CRYSTAL_HZ,
>>   	},
>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
>> index 32275af..1101c9c 100644
>> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
>> @@ -27,6 +27,14 @@
>>   #define PCAN_USBPRO_INFO_BL		0
>>   #define PCAN_USBPRO_INFO_FW		1
>>   
>> +/* PCAN-USB Pro (FD) Endpoints */
>> +#define PCAN_USBPRO_EP_CMDOUT		1
>> +#define PCAN_USBPRO_EP_CMDIN		(PCAN_USBPRO_EP_CMDOUT | USB_DIR_IN)
>> +#define PCAN_USBPRO_EP_MSGOUT_0		2
>> +#define PCAN_USBPRO_EP_MSGIN		(PCAN_USBPRO_EP_MSGOUT_0 | USB_DIR_IN)
>> +#define PCAN_USBPRO_EP_MSGOUT_1		3
>> +#define PCAN_USBPRO_EP_UNUSED		(PCAN_USBPRO_EP_MSGOUT_1 | USB_DIR_IN)
>> +
>>   /* Vendor Request value for XXX_FCT */
>>   #define PCAN_USBPRO_FCT_DRVLD		5 /* tell device driver is loaded */
>>   #define PCAN_USBPRO_FCT_DRVLD_REQ_LEN	16
>>
> Marc
>

Stéphane
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--

Re: [PATCH 1/3] can/peak_usb: CAN-FD: existing source files modifications

[Marc Kleine-Budde]

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

On 12/02/2014 02:55 PM, Stephane Grosjean wrote:
>>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c
>>> b/drivers/net/can/usb/peak_usb/pcan_usb.c
>>> index 925ab8e..10a4ceb 100644
>>> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
>>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
>>> @@ -858,6 +858,7 @@ struct peak_usb_adapter pcan_usb = {
>>>       .name = "PCAN-USB",
>>>       .device_id = PCAN_USB_PRODUCT_ID,
>>>       .ctrl_count = 1,
>>> +    .ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
>>> CAN_CTRLMODE_LISTENONLY,
>> This change seems unrelated to the make functions non static.
> 
> Ok.
> 
>>
>> and FTBFS:
>>
>>> linux/drivers/net/can/usb/peak_usb/pcan_usb.c:861:2: error: unknown
>>> field ‘ctrlmode_supported’ specified in initializer
>>>    .ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
>>> CAN_CTRLMODE_LISTENONLY,
>>>    ^
>>> linux/drivers/net/can/usb/peak_usb/pcan_usb.c:861:2: warning:
>>> initialization makes pointer from integer without a cast
>>> linux/drivers/net/can/usb/peak_usb/pcan_usb.c:861:2: warning: (near
>>> initialization for ‘pcan_usb.intf_probe’)
> 
> well, this FTBS is "normal" while you doesn't apply PATCH 3/3. I thought
> a "serie" of patches was not cleavable... Can u confirm, please?

No, that would break bisection. The kernel must compile and work after
each patch.

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: 819 bytes --]

[PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Stephane Grosjean]

This patch adds 3 new files to add support to the following CAN-FD USB adapters
from PEAK-System Technik:

PCAN-USB FD             single CAN-FD channel USB adapter
PCAN-USB Pro FD         dual CAN-FD channels USB adapter

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>²
---
 drivers/net/can/usb/peak_usb/pcan_ucan.h   | 208 +++++++
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 955 +++++++++++++++++++++++++++++
 drivers/net/can/usb/peak_usb/pcan_usb_fd.h | 108 ++++
 3 files changed, 1271 insertions(+)
 create mode 100644 drivers/net/can/usb/peak_usb/pcan_ucan.h
 create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.c
 create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.h

diff --git a/drivers/net/can/usb/peak_usb/pcan_ucan.h b/drivers/net/can/usb/peak_usb/pcan_ucan.h
new file mode 100644
index 0000000..2223c05
--- /dev/null
+++ b/drivers/net/can/usb/peak_usb/pcan_ucan.h
@@ -0,0 +1,208 @@
+/*
+ * CAN driver for PEAK System micro-CAN based adapters
+ *
+ * Copyright (C) 2003-2011 PEAK System-Technik GmbH
+ * Copyright (C) 2011-2013 Stephane Grosjean <s.grosjean@peak-system.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+#ifndef PUCAN_H
+#define PUCAN_H
+
+/* uCAN commands opcodes list (low-order 10 bits) */
+#define PUCAN_CMD_NOP			0x000
+#define PUCAN_CMD_RESET_MODE		0x001
+#define PUCAN_CMD_NORMAL_MODE		0x002
+#define PUCAN_CMD_LISTEN_ONLY_MODE	0x003
+#define PUCAN_CMD_TIMING_SLOW		0x004
+#define PUCAN_CMD_TIMING_FAST		0x005
+#define PUCAN_CMD_FILTER_STD		0x008
+#define PUCAN_CMD_TX_ABORT		0x009
+#define PUCAN_CMD_WR_ERR_CNT		0x00a
+#define PUCAN_CMD_RX_FRAME_ENABLE	0x00b
+#define PUCAN_CMD_RX_FRAME_DISABLE	0x00c
+#define PUCAN_CMD_END_OF_COLLECTION	0x3ff
+
+/* uCAN received messages list */
+#define PUCAN_MSG_CAN_RX		0x0001
+#define PUCAN_MSG_ERROR			0x0002
+#define PUCAN_MSG_STATUS		0x0003
+#define PUCAN_MSG_BUSLOAD		0x0004
+#define PUCAN_MSG_CAN_TX		0x1000
+
+/* uCAN command common header */
+#define PUCAN_CMD_OPCODE(c)		((c)->opcode_channel & 0x3ff)
+#define PUCAN_CMD_CHANNEL(c)		((c)->opcode_channel >> 12)
+#define PUCAN_CMD_OPCODE_CHANNEL(c, o)	cpu_to_le16(((c) << 12) | ((o) & 0x3ff))
+
+struct __packed pucan_command {
+	u16	opcode_channel;
+	u16	args[3];
+};
+
+/* uCAN TIMING_SLOW command fields */
+#define PUCAN_TSLOW_SJW_T(s, t)		(((s) & 0xf) | ((!!(t)) << 7))
+#define PUCAN_TSLOW_TSEG2(t)		((t) & 0xf)
+#define PUCAN_TSLOW_TSEG1(t)		((t) & 0x3f)
+#define PUCAN_TSLOW_BRP(b)		cpu_to_le16((b) & 0x3ff)
+
+struct __packed pucan_timing_slow {
+	u16	opcode_channel;
+
+	u8	ewl;		/* Error Warning limit */
+	u8	sjw_t;		/* Sync Jump Width + Triple sampling */
+	u8	tseg2;		/* Timing SEGment 2 */
+	u8	tseg1;		/* Timing SEGment 1 */
+
+	u16	brp;		/* BaudRate Prescaler */
+};
+
+/* uCAN TIMING_FAST command fields */
+#define PUCAN_TFAST_SJW(s)		((s) & 0x3)
+#define PUCAN_TFAST_TSEG2(t)		((t) & 0x7)
+#define PUCAN_TFAST_TSEG1(t)		((t) & 0xf)
+#define PUCAN_TFAST_BRP(b)		cpu_to_le16((b) & 0x3ff)
+
+struct __packed pucan_timing_fast {
+	u16	opcode_channel;
+
+	u8	unused;
+	u8	sjw;		/* Sync Jump Width */
+	u8	tseg2;		/* Timing SEGment 2 */
+	u8	tseg1;		/* Timing SEGment 1 */
+
+	u16	brp;		/* BaudRate Prescaler */
+};
+
+/* uCAN FILTER_STD command fields */
+#define PUCAN_FLTSTD_ROW_IDX_BITS	6
+
+struct __packed pucan_filter_std {
+	u16	opcode_channel;
+
+	u16	idx;
+	u32	mask;		/* CAN-ID bitmask in idx range */
+};
+
+/* uCAN WR_ERR_CNT command fields */
+#define PUCAN_WRERRCNT_TE(c)		0x4000	/* Tx error cntr write Enable */
+#define PUCAN_WRERRCNT_RE(c)		0x8000	/* Rx error cntr write Enable */
+
+struct __packed pucan_wr_err_cnt {
+	u16	opcode_channel;
+
+	u16	sel_mask;
+	u8	tx_counter;	/* Tx error counter new value */
+	u8	rx_counter;	/* Rx error counter new value */
+
+	u16	unused;
+};
+
+/* uCAN RX_FRAME_ENABLE command fields */
+#define PUCAN_FLTEXT_ERROR		0x0001
+#define PUCAN_FLTEXT_BUSLOAD		0x0002
+
+struct __packed pucan_filter_ext {
+	u16	opcode_channel;
+
+	u16	ext_mask;
+	u32	unused;
+};
+
+/* uCAN received messages global format */
+struct __packed pucan_msg {
+	__le16	size;
+	__le16	type;
+	__le32	ts_low;
+	__le32	ts_high;
+};
+
+/* uCAN flags for CAN/CANFD messages */
+#define PUCAN_MSG_SELF_RECEIVE		0x80
+#define PUCAN_MSG_ERROR_STATE_IND	0x40	/* error state indicator */
+#define PUCAN_MSG_BITRATE_SWITCH	0x20	/* bitrate switch */
+#define PUCAN_MSG_EXT_DATA_LEN		0x10	/* extended data length */
+#define PUCAN_MSG_SINGLE_SHOT		0x08
+#define PUCAN_MSG_LOOPED_BACK		0x04
+#define PUCAN_MSG_EXT_ID		0x02
+#define PUCAN_MSG_RTR			0x01
+
+#define PUCAN_MSG_CHANNEL(m)		((m)->channel_dlc & 0xf)
+#define PUCAN_MSG_DLC(m)		((m)->channel_dlc >> 4)
+
+struct __packed pucan_rx_msg {
+	__le16	size;
+	__le16	type;
+	__le32	ts_low;
+	__le32	ts_high;
+	__le32	tag_low;
+	__le32	tag_high;
+	u8	channel_dlc;
+	u8	client;
+	__le16	flags;
+	__le32	can_id;
+	u8	d[0];
+};
+
+/* uCAN error types */
+#define PUCAN_ERMSG_BIT_ERROR		0
+#define PUCAN_ERMSG_FORM_ERROR		1
+#define PUCAN_ERMSG_STUFF_ERROR		2
+#define PUCAN_ERMSG_OTHER_ERROR		3
+#define PUCAN_ERMSG_ERR_CNT_DEC		4
+
+#define PUCAN_ERMSG_CHANNEL(e)		((e)->channel_type_d & 0x0f)
+#define PUCAN_ERMSG_ERRTYPE(e)		(((e)->channel_type_d >> 4) & 0x07)
+#define PUCAN_ERMSG_D(e)		((e)->channel_type_d & 0x80)
+
+#define PUCAN_ERMSG_ERRCODE(e)		((e)->code_g & 0x7f)
+#define PUCAN_ERMSG_G(e)		((e)->code_g & 0x80)
+
+struct __packed pucan_error_msg {
+	__le16	size;
+	__le16	type;
+	__le32	ts_low;
+	__le32	ts_high;
+	u8	channel_type_d;
+	u8	code_g;
+	u8	tx_err_cnt;
+	u8	rx_err_cnt;
+};
+
+#define PUCAN_STMSG_CHANNEL(e)		((e)->channel_p_w_b & 0x0f)
+#define PUCAN_STMSG_PASSIVE(e)		((e)->channel_p_w_b & 0x20)
+#define PUCAN_STMSG_WARNING(e)		((e)->channel_p_w_b & 0x40)
+#define PUCAN_STMSG_BUSOFF(e)		((e)->channel_p_w_b & 0x80)
+
+struct __packed pucan_status_msg {
+	__le16	size;
+	__le16	type;
+	__le32	ts_low;
+	__le32	ts_high;
+	u8	channel_p_w_b;
+	u8	unused[3];
+};
+
+/* uCAN transmitted message format */
+#define PUCAN_MSG_CHANNEL_DLC(c, d)	(((c) & 0xf) | ((d) << 4))
+
+struct __packed pucan_tx_msg {
+	__le16	size;
+	__le16	type;
+	__le32	tag_low;
+	__le32	tag_high;
+	u8	channel_dlc;
+	u8	client;
+	u16	flags;
+	__le32	can_id;
+	u8	d[0];
+};
+
+#endif
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
new file mode 100644
index 0000000..3d392a8
--- /dev/null
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -0,0 +1,955 @@
+/*
+ * CAN driver for PEAK System PCAN-USB FD / PCAN-USB Pro FD adapter
+ *
+ * Copyright (C) 2013-2014 Stephane Grosjean <s.grosjean@peak-system.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+#include <linux/netdevice.h>
+#include <linux/usb.h>
+#include <linux/module.h>
+
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/error.h>
+
+#include "pcan_usb_core.h"
+#include "pcan_usb_fd.h"
+
+MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB FD adapter");
+MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro FD adapter");
+
+#define ALIGN32(x)			(((x) + 3) & 0xFFFFFFFC)
+
+#define PCAN_USBPROFD_CHANNEL_COUNT	2
+#define PCAN_USBFD_CHANNEL_COUNT	1
+
+/* PCAN-USB Pro FD adapter internal clock (MHz) */
+#define PCAN_UFD_CRYSTAL_HZ		80000000
+
+#define PCAN_UFD_CMD_BUFFER_SIZE	512
+#define PCAN_UFD_LOSPD_PKT_SIZE		64
+
+/* PCAN-USB Pro FD command timeout (ms.) */
+#define PCAN_UFD_COMMAND_TIMEOUT	1000
+
+/* PCAN-USB Pro FD rx/tx buffers size */
+#define PCAN_UFD_RX_BUFFER_SIZE		2048
+#define PCAN_UFD_TX_BUFFER_SIZE		512
+
+/* handle device specific info used by the netdevices */
+struct pcan_usb_fd_if {
+	struct peak_usb_device *dev[PCAN_USB_MAX_CHANNEL];
+	struct peak_time_ref time_ref;
+	int cm_ignore_count;
+	int dev_opened_count;
+};
+
+/* device information */
+struct pcan_usb_fd_device {
+	struct peak_usb_device dev;
+	struct pcan_usb_fd_if *usb_if;
+
+	u8 *cmd_buffer_addr;
+
+	uint tx_error_counter;
+	uint rx_error_counter;
+};
+
+/* Clock mode frequence values */
+static const u32 pcan_usb_fd_clk_freq[6] = {
+	[PCAN_UFD_CLK_80MHZ] = 80000000,
+	[PCAN_UFD_CLK_60MHZ] = 60000000,
+	[PCAN_UFD_CLK_40MHZ] = 40000000,
+	[PCAN_UFD_CLK_30MHZ] = 30000000,
+	[PCAN_UFD_CLK_24MHZ] = 24000000,
+	[PCAN_UFD_CLK_20MHZ] = 20000000
+};
+
+/* functions exported from PCAN-USB Pro interface */
+extern int pcan_usb_pro_probe(struct usb_interface *intf);
+extern int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
+				 int req_value, void *req_addr, int req_size);
+extern void pcan_usb_pro_restart_complete(struct urb *urb);
+
+/* return a device USB interface */
+static inline
+struct pcan_usb_fd_if *pcan_usb_fd_dev_if(struct peak_usb_device *dev)
+{
+	struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+	return pdev->usb_if;
+}
+
+/* return a device USB commands buffer */
+static inline void *pcan_usb_fd_cmd_buffer(struct peak_usb_device *dev)
+{
+	struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+	return pdev->cmd_buffer_addr;
+}
+
+/* send PCAN-USB Pro FD commands synchronously */
+static int pcan_usb_fd_send_cmd(struct peak_usb_device *dev, void *cmd_tail)
+{
+	void *cmd_head = pcan_usb_fd_cmd_buffer(dev);
+	int actual_length;
+	int err, cmd_len;
+	u8 *packet_ptr;
+	int p, n = 1, packet_len;
+
+	/* usb device unregistered? */
+	if (!(dev->state & PCAN_USB_STATE_CONNECTED))
+		return 0;
+
+	/*
+	 * if a packet is not filled completely by commands, the command list
+	 * is terminated with an "end of collection" record.
+	 */
+	cmd_len = cmd_tail - cmd_head;
+	if (cmd_len < PCAN_UFD_CMD_BUFFER_SIZE) {
+		memset(cmd_tail, 0xff, sizeof(u64));
+		cmd_len += sizeof(u64);
+	}
+
+	packet_ptr = cmd_head;
+
+	/* firmware is not able to re-assemble 512 bytes buffer in full-speed */
+	if ((dev->udev->speed != USB_SPEED_HIGH) &&
+	    (cmd_len > PCAN_UFD_LOSPD_PKT_SIZE)) {
+		packet_len = PCAN_UFD_LOSPD_PKT_SIZE;
+		n += cmd_len / packet_len;
+	} else {
+		packet_len = cmd_len;
+	}
+
+	for (p = 1; p <= n; p++) {
+		err = usb_bulk_msg(dev->udev,
+				   usb_sndbulkpipe(dev->udev,
+						   PCAN_USBPRO_EP_CMDOUT),
+				   packet_ptr, packet_len,
+				   &actual_length, PCAN_UFD_COMMAND_TIMEOUT);
+		if (err) {
+			netdev_err(dev->netdev,
+				   "sending command failure: %d\n", err);
+			break;
+		}
+
+		packet_ptr += packet_len;
+	}
+
+	return err;
+}
+
+static int pcan_usb_fd_set_bus(struct peak_usb_device *dev, u8 onoff)
+{
+	struct pucan_command *cmd = pcan_usb_fd_cmd_buffer(dev);
+	u16 opcode;
+
+	if (onoff) {
+		opcode = (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) ?
+				PUCAN_CMD_LISTEN_ONLY_MODE :
+				PUCAN_CMD_NORMAL_MODE;
+	} else {
+		opcode = PUCAN_CMD_RESET_MODE;
+	}
+
+	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx, opcode);
+
+	/* send the command */
+	return pcan_usb_fd_send_cmd(dev, ++cmd);
+}
+
+/*
+ * Set filtering masks:
+ *
+ *	idx in range [0..63] selects row #idx, all rows otherwise.
+ *	mask in range [0..0xffffffff]
+ *
+ */
+static int pcan_usb_fd_set_filter_std(struct peak_usb_device *dev, int idx,
+				      u32 mask)
+{
+	return 0;
+}
+
+/*
+ * set/unset notifications filter:
+ *
+ *	onoff	sets(1)/unset(0) notifications
+ *	mask	each bit defines a kind of notification to set/unset
+ */
+static int pcan_usb_fd_set_filter_ext(struct peak_usb_device *dev,
+				      int onoff, u16 ext_mask, u16 usb_mask)
+{
+	struct pcan_ufd_filter_ext *cmd = pcan_usb_fd_cmd_buffer(dev);
+
+	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
+					(onoff) ? PUCAN_CMD_RX_FRAME_ENABLE :
+						  PUCAN_CMD_RX_FRAME_DISABLE);
+
+	cmd->ext_mask = cpu_to_le16(ext_mask);
+	cmd->usb_mask = cpu_to_le16(usb_mask);
+
+	/* send the command */
+	return pcan_usb_fd_send_cmd(dev, ++cmd);
+}
+
+/* setup LED control */
+static int pcan_usb_fd_set_can_led(struct peak_usb_device *dev, u8 led_mode)
+{
+	struct pcan_ufd_led *cmd = pcan_usb_fd_cmd_buffer(dev);
+
+	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
+						       PCAN_UFD_CMD_LED_SET);
+	cmd->mode = led_mode;
+
+	/* send the command */
+	return pcan_usb_fd_send_cmd(dev, ++cmd);
+}
+
+/* set CAN clock domain */
+static int pcan_usb_fd_set_clock_domain(struct peak_usb_device *dev,
+					u8 clk_mode)
+{
+	struct pcan_ufd_clock *cmd = pcan_usb_fd_cmd_buffer(dev);
+
+	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
+						       PCAN_UFD_CMD_CLK_SET);
+	cmd->mode = clk_mode;
+
+	/* send the command */
+	return pcan_usb_fd_send_cmd(dev, ++cmd);
+}
+
+/* set bittiming for CAN and CAN-FD header */
+static int pcan_usb_fd_set_bittiming_slow(struct peak_usb_device *dev,
+					  struct can_bittiming *bt)
+{
+	struct pucan_timing_slow *cmd = pcan_usb_fd_cmd_buffer(dev);
+
+	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
+						       PUCAN_CMD_TIMING_SLOW);
+	cmd->sjw_t = PUCAN_TSLOW_SJW_T(bt->sjw - 1,
+				dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES);
+
+	cmd->tseg2 = PUCAN_TSLOW_TSEG2(bt->phase_seg2 - 1);
+	cmd->tseg1 = PUCAN_TSLOW_TSEG1(bt->prop_seg + bt->phase_seg1 - 1);
+	cmd->brp = PUCAN_TSLOW_BRP(bt->brp - 1);
+
+	cmd->ewl = 96;	/* default */
+
+	/* send the command */
+	return pcan_usb_fd_send_cmd(dev, ++cmd);
+}
+
+/* set CAN-FD bittiming for data */
+static int pcan_usb_fd_set_bittiming_fast(struct peak_usb_device *dev,
+					  struct can_bittiming *bt)
+{
+	struct pucan_timing_fast *cmd = pcan_usb_fd_cmd_buffer(dev);
+
+	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
+						       PUCAN_CMD_TIMING_FAST);
+	cmd->sjw = PUCAN_TFAST_SJW(bt->sjw - 1);
+	cmd->tseg2 = PUCAN_TFAST_TSEG2(bt->phase_seg2 - 1);
+	cmd->tseg1 = PUCAN_TFAST_TSEG1(bt->prop_seg + bt->phase_seg1 - 1);
+	cmd->brp = PUCAN_TFAST_BRP(bt->brp - 1);
+
+	/* send the command */
+	return pcan_usb_fd_send_cmd(dev, ++cmd);
+}
+
+/*
+ * handle restart but in asynchronously way
+ * (uses PCAN-USB Pro code to complete)
+ */
+static int pcan_usb_fd_restart_async(struct peak_usb_device *dev,
+				     struct urb *urb, u8 *buf)
+{
+	struct pucan_command *cmd = (struct pucan_command *)buf;
+
+	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
+				(dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) ?
+						PUCAN_CMD_LISTEN_ONLY_MODE :
+						PUCAN_CMD_NORMAL_MODE);
+	/* EOC */
+	memset(cmd+1, 0xff, sizeof(struct pucan_command));
+
+	usb_fill_bulk_urb(urb, dev->udev,
+			  usb_sndbulkpipe(dev->udev, PCAN_USBPRO_EP_CMDOUT),
+			  buf, 2 * sizeof(struct pucan_command),
+			  pcan_usb_pro_restart_complete, dev);
+
+	return usb_submit_urb(urb, GFP_ATOMIC);
+}
+
+static int pcan_usb_fd_drv_loaded(struct peak_usb_device *dev, int loaded)
+{
+	struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+
+	pdev->cmd_buffer_addr[0] = 0;
+	pdev->cmd_buffer_addr[1] = !!loaded;
+
+	return pcan_usb_pro_send_req(dev,
+				PCAN_USBPRO_REQ_FCT,
+				PCAN_USBPRO_FCT_DRVLD,
+				pdev->cmd_buffer_addr,
+				PCAN_USBPRO_FCT_DRVLD_REQ_LEN);
+}
+
+static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
+				     struct pucan_msg *rx_msg)
+{
+	struct pucan_rx_msg *rm = (struct pucan_rx_msg *)rx_msg;
+	struct peak_usb_device *dev = usb_if->dev[PUCAN_MSG_CHANNEL(rm)];
+	struct net_device *netdev = dev->netdev;
+	struct canfd_frame *cfd;
+	struct sk_buff *skb;
+
+	if (rm->flags & PUCAN_MSG_EXT_DATA_LEN) {
+		/* CANFD frame case */
+		skb = alloc_canfd_skb(netdev, &cfd);
+		if (!skb)
+			return -ENOMEM;
+
+		if (rm->flags & PUCAN_MSG_BITRATE_SWITCH)
+			cfd->flags |= CANFD_BRS;
+
+		if (rm->flags & PUCAN_MSG_ERROR_STATE_IND)
+			cfd->flags |= CANFD_ESI;
+
+		cfd->len = can_dlc2len(PUCAN_MSG_DLC(rm));
+	} else {
+		/* CANFD frame case */
+		skb = alloc_can_skb(netdev, (struct can_frame **)&cfd);
+		if (!skb)
+			return -ENOMEM;
+
+		cfd->len = get_can_dlc(PUCAN_MSG_DLC(rm));
+	}
+
+	cfd->can_id = le32_to_cpu(rm->can_id);
+
+	if (rm->flags & PUCAN_MSG_EXT_ID)
+		cfd->can_id |= CAN_EFF_FLAG;
+
+	if (rm->flags & PUCAN_MSG_RTR)
+		cfd->can_id |= CAN_RTR_FLAG;
+	else
+		memcpy(cfd->data, rm->d, cfd->len);
+
+	peak_usb_netif_rx(skb, &usb_if->time_ref,
+			  le32_to_cpu(rm->ts_low), le32_to_cpu(rm->ts_high));
+
+	netdev->stats.rx_packets++;
+	netdev->stats.rx_bytes += cfd->len;
+
+	return 0;
+}
+
+/* handle uCAN status message */
+static int pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
+				     struct pucan_msg *rx_msg)
+{
+	struct pucan_status_msg *st = (struct pucan_status_msg *)rx_msg;
+	struct peak_usb_device *dev = usb_if->dev[PUCAN_STMSG_CHANNEL(st)];
+	struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+	enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
+	struct net_device *netdev = dev->netdev;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+
+	/* nothing should be sent while in BUS_OFF state */
+	if (dev->can.state == CAN_STATE_BUS_OFF)
+		return 0;
+
+	if (PUCAN_STMSG_BUSOFF(st)) {
+		new_state = CAN_STATE_BUS_OFF;
+	} else if (PUCAN_STMSG_PASSIVE(st)) {
+		new_state = CAN_STATE_ERROR_PASSIVE;
+	} else if (PUCAN_STMSG_WARNING(st)) {
+		new_state = CAN_STATE_ERROR_WARNING;
+	} else {
+		/* no error bit (so, no error skb, back to active state) */
+		dev->can.state = CAN_STATE_ERROR_ACTIVE;
+		pdev->tx_error_counter = 0;
+		pdev->rx_error_counter = 0;
+		return 0;
+	}
+
+	/* donot post any error if current state didn't change */
+	if (dev->can.state == new_state)
+		return 0;
+
+	/* allocate an skb to store the error frame */
+	skb = alloc_can_err_skb(netdev, &cf);
+	if (!skb)
+		return -ENOMEM;
+
+	switch (new_state) {
+	case CAN_STATE_BUS_OFF:
+		cf->can_id |= CAN_ERR_BUSOFF;
+		can_bus_off(netdev);
+		break;
+
+	case CAN_STATE_ERROR_PASSIVE:
+		cf->can_id |= CAN_ERR_CRTL;
+		if (pdev->rx_error_counter > 127)
+			cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
+		if (pdev->tx_error_counter > 127)
+			cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
+
+		dev->can.can_stats.error_passive++;
+		break;
+
+	case CAN_STATE_ERROR_WARNING:
+		cf->can_id |= CAN_ERR_CRTL;
+		if (pdev->rx_error_counter > 96)
+			cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
+		if (pdev->tx_error_counter > 96)
+			cf->data[1] |= CAN_ERR_CRTL_TX_WARNING;
+
+		dev->can.can_stats.error_warning++;
+		break;
+
+	default:
+		/* default case never happens, only for warnings */
+		new_state = CAN_STATE_ERROR_ACTIVE;
+
+	case CAN_STATE_ERROR_ACTIVE:	/* fallthrough */
+		pdev->tx_error_counter = 0;
+		pdev->rx_error_counter = 0;
+		break;
+	}
+
+	dev->can.state = new_state;
+
+	peak_usb_netif_rx(skb, &usb_if->time_ref,
+			  le32_to_cpu(st->ts_low), le32_to_cpu(st->ts_high));
+
+	netdev->stats.rx_packets++;
+	netdev->stats.rx_bytes += cf->can_dlc;
+
+	return 0;
+}
+
+/* handle uCAN error message */
+static int pcan_usb_fd_decode_error(struct pcan_usb_fd_if *usb_if,
+				    struct pucan_msg *rx_msg)
+{
+	struct pucan_error_msg *er = (struct pucan_error_msg *)rx_msg;
+	struct peak_usb_device *dev = usb_if->dev[PUCAN_ERMSG_CHANNEL(er)];
+	struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+
+	/* keep a trace of tx and rx error counters for later use */
+	pdev->tx_error_counter = er->tx_err_cnt;
+	pdev->rx_error_counter = er->rx_err_cnt;
+
+	return 0;
+}
+
+/* handle uCAN overrun message */
+static int pcan_usb_fd_decode_overrun(struct pcan_usb_fd_if *usb_if,
+				      struct pucan_msg *rx_msg)
+{
+	struct pcan_ufd_ovr_msg *ov = (struct pcan_ufd_ovr_msg *)rx_msg;
+	struct peak_usb_device *dev = usb_if->dev[PCAN_UFD_OVMSG_CHANNEL(ov)];
+	struct net_device *netdev = dev->netdev;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+
+	/* allocate an skb to store the error frame */
+	skb = alloc_can_err_skb(netdev, &cf);
+	if (!skb)
+		return -ENOMEM;
+
+	cf->can_id |= CAN_ERR_CRTL;
+	cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
+
+	peak_usb_netif_rx(skb, &usb_if->time_ref,
+			  le32_to_cpu(ov->ts_low), le32_to_cpu(ov->ts_high));
+
+	netdev->stats.rx_over_errors++;
+	netdev->stats.rx_errors++;
+
+	return 0;
+}
+
+/* handle USB calibration message */
+static void pcan_usb_fd_decode_ts(struct pcan_usb_fd_if *usb_if,
+				  struct pucan_msg *rx_msg)
+{
+	struct pcan_ufd_ts_msg *ts = (struct pcan_ufd_ts_msg *)rx_msg;
+
+	/* should wait until clock is stabilized */
+	if (usb_if->cm_ignore_count > 0)
+		usb_if->cm_ignore_count--;
+	else
+		peak_usb_set_ts_now(&usb_if->time_ref, le32_to_cpu(ts->ts_low));
+}
+
+/* callback for bulk IN urb */
+static int pcan_usb_fd_decode_buf(struct peak_usb_device *dev, struct urb *urb)
+{
+	struct pcan_usb_fd_if *usb_if = pcan_usb_fd_dev_if(dev);
+	struct net_device *netdev = dev->netdev;
+	struct pucan_msg *rx_msg;
+	u8 *msg_ptr, *msg_end;
+	int err = 0;
+
+	/* loop reading all the records from the incoming message */
+	msg_ptr = urb->transfer_buffer;
+	msg_end = urb->transfer_buffer + urb->actual_length;
+	for (; msg_ptr < msg_end;) {
+		u16 rx_msg_type, rx_msg_size;
+
+		rx_msg = (struct pucan_msg *)msg_ptr;
+		if (!rx_msg->size) {
+			/* null packet found: end of list */
+			break;
+		}
+
+		rx_msg_size = le16_to_cpu(rx_msg->size);
+		rx_msg_type = le16_to_cpu(rx_msg->type);
+
+		/* check if the record goes out of current packet */
+		if (msg_ptr + rx_msg_size > msg_end) {
+			netdev_err(netdev,
+				   "got frag rec: should inc usb rx buf sze\n");
+			err = -EBADMSG;
+			break;
+		}
+
+		switch (rx_msg_type) {
+		case PUCAN_MSG_CAN_RX:
+			err = pcan_usb_fd_decode_canmsg(usb_if, rx_msg);
+			if (err < 0)
+				goto fail;
+			break;
+
+		case PCAN_UFD_MSG_CALIBRATION:
+			pcan_usb_fd_decode_ts(usb_if, rx_msg);
+			break;
+
+		case PUCAN_MSG_ERROR:
+			err = pcan_usb_fd_decode_error(usb_if, rx_msg);
+			if (err < 0)
+				goto fail;
+			break;
+
+		case PUCAN_MSG_STATUS:
+			err = pcan_usb_fd_decode_status(usb_if, rx_msg);
+			if (err < 0)
+				goto fail;
+			break;
+
+		case PCAN_UFD_MSG_OVERRUN:
+			err = pcan_usb_fd_decode_overrun(usb_if, rx_msg);
+			if (err < 0)
+				goto fail;
+			break;
+
+		default:
+			netdev_err(netdev,
+				   "unhandled msg type 0x%02x (%d): ignored\n",
+				   rx_msg_type, rx_msg_type);
+			break;
+		}
+
+		msg_ptr += rx_msg_size;
+	}
+
+fail:
+	if (err)
+		pcan_dump_mem("received msg",
+			      urb->transfer_buffer, urb->actual_length);
+	return err;
+}
+
+/* CAN/CANFD frames encoding callback */
+static int pcan_usb_fd_encode_msg(struct peak_usb_device *dev,
+				  struct sk_buff *skb, u8 *obuf, size_t *size)
+{
+	struct pucan_tx_msg *tx_msg = (struct pucan_tx_msg *)obuf;
+	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+	u16 tx_msg_size;
+	u8 can_dlc;
+
+	tx_msg_size = ALIGN32(sizeof(struct pucan_tx_msg) + cfd->len);
+	tx_msg->size = cpu_to_le16(tx_msg_size);
+	tx_msg->type = cpu_to_le16(PUCAN_MSG_CAN_TX);
+
+	tx_msg->flags = 0;
+	if (cfd->can_id & CAN_EFF_FLAG) {
+		tx_msg->flags |= PUCAN_MSG_EXT_ID;
+		tx_msg->can_id = cpu_to_le32(cfd->can_id & CAN_EFF_MASK);
+	} else {
+		tx_msg->can_id = cpu_to_le32(cfd->can_id & CAN_SFF_MASK);
+	}
+
+	if (skb->len == CANFD_MTU) {
+		/* considering a CANFD frame */
+		can_dlc = can_len2dlc(cfd->len);
+
+		tx_msg->flags |= PUCAN_MSG_EXT_DATA_LEN;
+
+		if (cfd->flags & CANFD_BRS)
+			tx_msg->flags |= PUCAN_MSG_BITRATE_SWITCH;
+
+		if (cfd->flags & CANFD_ESI)
+			tx_msg->flags |= PUCAN_MSG_ERROR_STATE_IND;
+	} else {
+		/* CAND 2.0 frames */
+		can_dlc = cfd->len;
+
+		if (cfd->can_id & CAN_RTR_FLAG)
+			tx_msg->flags |= PUCAN_MSG_RTR;
+	}
+
+	tx_msg->channel_dlc = PUCAN_MSG_CHANNEL_DLC(dev->ctrl_idx, can_dlc);
+	memcpy(tx_msg->d, cfd->data, cfd->len);
+
+	/* add null size message to tag the end (messages are 32-bits aligned)*/
+	tx_msg = (struct pucan_tx_msg *)(obuf + tx_msg_size);
+
+	tx_msg->size = 0;
+
+	/* set the whole size of the USB packet to send */
+	*size = tx_msg_size + sizeof(u32);
+
+	return 0;
+}
+
+/* start the interface (last chance before set bus on) */
+static int pcan_usb_fd_start(struct peak_usb_device *dev)
+{
+	struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+	int err;
+
+	/* set filter mode: all acceptance */
+	err = pcan_usb_fd_set_filter_std(dev, -1, 0xffffffff);
+	if (err)
+		return err;
+
+	/* opening first device: */
+	if (pdev->usb_if->dev_opened_count == 0) {
+		/* reset time_ref */
+		peak_usb_init_time_ref(&pdev->usb_if->time_ref,
+				       &pcan_usb_pro_fd);
+
+		/* enable USB calibration messages */
+		err = pcan_usb_fd_set_filter_ext(dev, 1,
+						 PUCAN_FLTEXT_ERROR,
+						 PCAN_UFD_FLTEXT_CALIBRATION);
+	}
+
+	pdev->usb_if->dev_opened_count++;
+
+	/* reset cached error counters */
+	pdev->tx_error_counter = 0;
+	pdev->rx_error_counter = 0;
+
+	return err;
+}
+
+/* stop interface (last chance before set bus off) */
+static int pcan_usb_fd_stop(struct peak_usb_device *dev)
+{
+	struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+
+	/* turn off special msgs for that interface if no other dev opened */
+	if (pdev->usb_if->dev_opened_count == 1)
+		pcan_usb_fd_set_filter_ext(dev, 0,
+					   PUCAN_FLTEXT_ERROR,
+					   PCAN_UFD_FLTEXT_CALIBRATION);
+	pdev->usb_if->dev_opened_count--;
+
+	return 0;
+}
+
+/* called when probing, to initialize a device object */
+static int pcan_usb_fd_init(struct peak_usb_device *dev)
+{
+	struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+	int i, err = -ENOMEM;
+
+	/* do this for 1st channel only */
+	if (!dev->prev_siblings) {
+		struct pcan_ufd_fw_info *fi;
+
+		/* allocate netdevices common structure attached to first one */
+		pdev->usb_if = kzalloc(sizeof(*pdev->usb_if), GFP_KERNEL);
+		if (!pdev->usb_if)
+			goto err_out;
+
+		/* allocate command buffer once for all for the interface */
+		pdev->cmd_buffer_addr = kmalloc(PCAN_UFD_CMD_BUFFER_SIZE,
+						GFP_KERNEL);
+		if (!pdev->cmd_buffer_addr)
+			goto err_out_1;
+
+		/* number of ts msgs to ignore before taking one into account */
+		pdev->usb_if->cm_ignore_count = 5;
+
+		/*
+		 * explicit use of dev_xxx() instead of netdev_xxx() here:
+		 * information displayed are related to the device itself, not
+		 * to the canx netdevices.
+		 */
+		fi = (struct pcan_ufd_fw_info *)pdev->cmd_buffer_addr;
+		err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
+					    PCAN_USBPRO_INFO_FW,
+					    fi, sizeof(*fi));
+		if (err) {
+			dev_err(dev->netdev->dev.parent,
+				"unable to read %s firmware info (err %d)\n",
+				dev->adapter->name, err);
+			goto err_out_2;
+		}
+
+		dev_info(dev->netdev->dev.parent,
+			 "PEAK-System %s v%u fw v%u.%u.%u (%u channels)\n",
+			 dev->adapter->name, fi->hw_version,
+			 fi->fw_version[0], fi->fw_version[1],
+			 fi->fw_version[2], dev->adapter->ctrl_count);
+
+		/* tell the device the can driver is running */
+		err = pcan_usb_fd_drv_loaded(dev, 1);
+		if (err) {
+			dev_err(dev->netdev->dev.parent,
+				"unable to tell %s driver is loaded (err %d)\n",
+				dev->adapter->name, err);
+			goto err_out_2;
+		}
+
+	} else {
+		/* otherwise, simply copy previous sibling's values */
+		struct pcan_usb_fd_device *ppdev =
+			container_of(dev->prev_siblings,
+				     struct pcan_usb_fd_device, dev);
+
+		pdev->usb_if = ppdev->usb_if;
+		pdev->cmd_buffer_addr = ppdev->cmd_buffer_addr;
+	}
+
+	pdev->usb_if->dev[dev->ctrl_idx] = dev;
+
+	/* set clock domain */
+	for (i = 0; i < ARRAY_SIZE(pcan_usb_fd_clk_freq); i++)
+		if (dev->adapter->clock.freq == pcan_usb_fd_clk_freq[i])
+			break;
+
+	if (i >= ARRAY_SIZE(pcan_usb_fd_clk_freq)) {
+		dev_warn(dev->netdev->dev.parent,
+			 "incompatible clock frequencies\n");
+		err = -EINVAL;
+		goto err_out_2;
+	}
+
+	pcan_usb_fd_set_clock_domain(dev, i);
+
+	/* set LED in default state (end of init phase) */
+	pcan_usb_fd_set_can_led(dev, PCAN_UFD_LED_DEF);
+
+	return 0;
+
+err_out_2:
+	kfree(pdev->cmd_buffer_addr);
+err_out_1:
+	kfree(pdev->usb_if);
+err_out:
+	return err;
+}
+
+/* called when driver module is being unloaded */
+static void pcan_usb_fd_exit(struct peak_usb_device *dev)
+{
+	struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+
+	/*
+	 * when rmmod called before unplug and if down, should reset things
+	 * before leaving
+	 */
+	if (dev->can.state != CAN_STATE_STOPPED) {
+		/* set bus off on the corresponding channel */
+		pcan_usb_fd_set_bus(dev, 0);
+	}
+
+	/* switch off corresponding CAN LEDs */
+	pcan_usb_fd_set_can_led(dev, PCAN_UFD_LED_OFF);
+
+	/* if channel #0 (only) */
+	if (dev->ctrl_idx == 0) {
+		/* turn off calibration message if any device were opened */
+		if (pdev->usb_if->dev_opened_count > 0)
+			pcan_usb_fd_set_filter_ext(dev, 0,
+						   PUCAN_FLTEXT_ERROR,
+						   PCAN_UFD_FLTEXT_CALIBRATION);
+
+		/* tell USB adapter that the driver is being unloaded */
+		pcan_usb_fd_drv_loaded(dev, 0);
+	}
+}
+
+/* called when the USB adapter is unplugged */
+static void pcan_usb_fd_free(struct peak_usb_device *dev)
+{
+	/* last device: can free shared objects now */
+	if (!dev->prev_siblings && !dev->next_siblings) {
+		struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+
+		/* free commands buffer */
+		kfree(pdev->cmd_buffer_addr);
+
+		/* free usb interface object */
+		kfree(pdev->usb_if);
+	}
+}
+
+/* describes the PCAN-USB FD adapter */
+struct peak_usb_adapter pcan_usb_fd = {
+	.name = "PCAN-USB FD",
+	.device_id = PCAN_USBFD_PRODUCT_ID,
+	.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
+	.ctrlmode_supported = CAN_CTRLMODE_FD |
+			CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
+	.clock = {
+		.freq = PCAN_UFD_CRYSTAL_HZ,
+	},
+	.bittiming_const = {
+		.name = "pcan_usb_fd",
+		.tseg1_min = 1,
+		.tseg1_max = 64,
+		.tseg2_min = 1,
+		.tseg2_max = 16,
+		.sjw_max = 16,
+		.brp_min = 1,
+		.brp_max = 1024,
+		.brp_inc = 1,
+	},
+	.data_bittiming_const = {
+		.name = "pcan_usb_fd",
+		.tseg1_min = 1,
+		.tseg1_max = 16,
+		.tseg2_min = 1,
+		.tseg2_max = 8,
+		.sjw_max = 4,
+		.brp_min = 1,
+		.brp_max = 1024,
+		.brp_inc = 1,
+	},
+
+	/* size of device private data */
+	.sizeof_dev_private = sizeof(struct pcan_usb_fd_device),
+
+	/* timestamps usage */
+	.ts_used_bits = 32,
+	.ts_period = 1000000, /* calibration period in ts. */
+	.us_per_ts_scale = 1, /* us = (ts * scale) >> shift */
+	.us_per_ts_shift = 0,
+
+	/* give here messages in/out endpoints */
+	.ep_msg_in = PCAN_USBPRO_EP_MSGIN,
+	.ep_msg_out = {PCAN_USBPRO_EP_MSGOUT_0},
+
+	/* size of rx/tx usb buffers */
+	.rx_buffer_size = PCAN_UFD_RX_BUFFER_SIZE,
+	.tx_buffer_size = PCAN_UFD_TX_BUFFER_SIZE,
+
+	/* device callbacks */
+	.intf_probe = pcan_usb_pro_probe,	/* same as PCAN-USB Pro */
+	.dev_init = pcan_usb_fd_init,
+
+	.dev_exit = pcan_usb_fd_exit,
+	.dev_free = pcan_usb_fd_free,
+	.dev_set_bus = pcan_usb_fd_set_bus,
+	.dev_set_bittiming = pcan_usb_fd_set_bittiming_slow,
+	.dev_set_data_bittiming = pcan_usb_fd_set_bittiming_fast,
+	.dev_decode_buf = pcan_usb_fd_decode_buf,
+	.dev_start = pcan_usb_fd_start,
+	.dev_stop = pcan_usb_fd_stop,
+	.dev_restart_async = pcan_usb_fd_restart_async,
+	.dev_encode_msg = pcan_usb_fd_encode_msg,
+};
+
+/* describes the PCAN-USB Pro FD adapter */
+struct peak_usb_adapter pcan_usb_pro_fd = {
+	.name = "PCAN-USB Pro FD",
+	.device_id = PCAN_USBPROFD_PRODUCT_ID,
+	.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
+	.ctrlmode_supported = CAN_CTRLMODE_FD |
+			CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
+	.clock = {
+		.freq = PCAN_UFD_CRYSTAL_HZ,
+	},
+	.bittiming_const = {
+		.name = "pcan_usb_pro_fd",
+		.tseg1_min = 1,
+		.tseg1_max = 64,
+		.tseg2_min = 1,
+		.tseg2_max = 16,
+		.sjw_max = 16,
+		.brp_min = 1,
+		.brp_max = 1024,
+		.brp_inc = 1,
+	},
+	.data_bittiming_const = {
+		.name = "pcan_usb_pro_fd",
+		.tseg1_min = 1,
+		.tseg1_max = 16,
+		.tseg2_min = 1,
+		.tseg2_max = 8,
+		.sjw_max = 4,
+		.brp_min = 1,
+		.brp_max = 1024,
+		.brp_inc = 1,
+	},
+
+	/* size of device private data */
+	.sizeof_dev_private = sizeof(struct pcan_usb_fd_device),
+
+	/* timestamps usage */
+	.ts_used_bits = 32,
+	.ts_period = 1000000, /* calibration period in ts. */
+	.us_per_ts_scale = 1, /* us = (ts * scale) >> shift */
+	.us_per_ts_shift = 0,
+
+	/* give here messages in/out endpoints */
+	.ep_msg_in = PCAN_USBPRO_EP_MSGIN,
+	.ep_msg_out = {PCAN_USBPRO_EP_MSGOUT_0, PCAN_USBPRO_EP_MSGOUT_1},
+
+	/* size of rx/tx usb buffers */
+	.rx_buffer_size = PCAN_UFD_RX_BUFFER_SIZE,
+	.tx_buffer_size = PCAN_UFD_TX_BUFFER_SIZE,
+
+	/* device callbacks */
+	.intf_probe = pcan_usb_pro_probe,	/* same as PCAN-USB Pro */
+	.dev_init = pcan_usb_fd_init,
+
+	.dev_exit = pcan_usb_fd_exit,
+	.dev_free = pcan_usb_fd_free,
+	.dev_set_bus = pcan_usb_fd_set_bus,
+	.dev_set_bittiming = pcan_usb_fd_set_bittiming_slow,
+	.dev_set_data_bittiming = pcan_usb_fd_set_bittiming_fast,
+	.dev_decode_buf = pcan_usb_fd_decode_buf,
+	.dev_start = pcan_usb_fd_start,
+	.dev_stop = pcan_usb_fd_stop,
+	.dev_restart_async = pcan_usb_fd_restart_async,
+	.dev_encode_msg = pcan_usb_fd_encode_msg,
+};
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.h b/drivers/net/can/usb/peak_usb/pcan_usb_fd.h
new file mode 100644
index 0000000..2f66ef9
--- /dev/null
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.h
@@ -0,0 +1,108 @@
+/*
+ * CAN driver for PEAK System PCAN-USB FD / PCAN-USB Pro FD adapter
+ *
+ * Copyright (C) 2003-2011 PEAK System-Technik GmbH
+ * Copyright (C) 2011-2013 Stephane Grosjean <s.grosjean@peak-system.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+#ifndef PCAN_UFD_H
+#define PCAN_UFD_H
+
+#include "pcan_usb_pro.h"
+#include "pcan_ucan.h"
+
+/* Extended commands (non uCAN commands) */
+
+/* Clock Modes command */
+#define PCAN_UFD_CMD_CLK_SET	0x80
+
+#define PCAN_UFD_CLK_80MHZ	0x0
+#define PCAN_UFD_CLK_60MHZ	0x1
+#define PCAN_UFD_CLK_40MHZ	0x2
+#define PCAN_UFD_CLK_30MHZ	0x3
+#define PCAN_UFD_CLK_24MHZ	0x4
+#define PCAN_UFD_CLK_20MHZ	0x5
+#define PCAN_UFD_CLK_DEF	PCAN_UFD_CLK_80MHZ
+
+struct __packed pcan_ufd_clock {
+	u16	opcode_channel;
+
+	u8	mode;
+	u8	unused[5];
+};
+
+/* LED control command */
+#define PCAN_UFD_CMD_LED_SET	0x86
+
+#define PCAN_UFD_LED_DEV	0x00
+#define PCAN_UFD_LED_FAST	0x01
+#define PCAN_UFD_LED_SLOW	0x02
+#define PCAN_UFD_LED_ON		0x03
+#define PCAN_UFD_LED_OFF	0x04
+#define PCAN_UFD_LED_DEF	PCAN_UFD_LED_DEV
+
+struct __packed pcan_ufd_led {
+	u16	opcode_channel;
+
+	u8	mode;
+	u8	unused[5];
+};
+
+/* Extended usage of uCAN commands CMD_RX_FRAME_xxxABLE for PCAN-USB Pro FD */
+#define PCAN_UFD_FLTEXT_CALIBRATION	0x8000
+
+struct __packed pcan_ufd_filter_ext {
+	u16	opcode_channel;
+
+	u16	ext_mask;
+	u16	unused;
+	u16	usb_mask;
+};
+
+/* Extended usage of uCAN messages for PCAN-USB Pro FD */
+#define PCAN_UFD_MSG_CALIBRATION	0x100
+
+struct __packed pcan_ufd_ts_msg {
+	__le16	size;
+	__le16	type;
+	__le32	ts_low;
+	__le32	ts_high;
+	__le16	usb_frame_index;
+	u16	unused;
+};
+
+#define PCAN_UFD_MSG_OVERRUN		0x101
+
+#define PCAN_UFD_OVMSG_CHANNEL(o)	((o)->channel & 0xf)
+
+struct __packed pcan_ufd_ovr_msg {
+	__le16	size;
+	__le16	type;
+	__le32	ts_low;
+	__le32	ts_high;
+	u8	channel;
+	u8	unused[3];
+};
+
+/* read some versions info from the hw devcie */
+struct __packed pcan_ufd_fw_info {
+	u16	size_of;	/* sizeof this */
+	u16	type;		/* type of thsi structure */
+	u8	hw_type;	/* Type of hardware (HW_TYPE_xxx) */
+	u8	bl_version[3];	/* Bootloader version */
+	u8	hw_version;	/* Hardware version (PCB) */
+	u8	fw_version[3];	/* Firmware version */
+	u32	dev_id[2];	/* "device id" per CAN */
+	u32	ser_no;		/* S/N */
+	u32	flags;		/* special functions */
+};
+
+#endif
-- 
1.9.1

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Marc Kleine-Budde]

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

On 11/27/2014 11:32 AM, Stephane Grosjean wrote:
> This patch adds 3 new files to add support to the following CAN-FD USB adapters
> from PEAK-System Technik:
> 
> PCAN-USB FD             single CAN-FD channel USB adapter
> PCAN-USB Pro FD         dual CAN-FD channels USB adapter
> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>²
> ---
>  drivers/net/can/usb/peak_usb/pcan_ucan.h   | 208 +++++++
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 955 +++++++++++++++++++++++++++++
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.h | 108 ++++

Please don't create a header file, if it's only included once. Please
merge into the correct .c file.

Please have a look the _all_ multi-byte values and check for endianess.

Hint, you might want to fix the existing driver first:

pcan_usb_core.c:863:60: warning: restricted __le16 degrades to integer           
pcan_usb.c:322:34: warning: cast to restricted __le32                            
pcan_usb.c:357:20: warning: cast to restricted __le16                            
pcan_usb.c:382:28: warning: cast to restricted __le16                            
pcan_usb.c:625:30: warning: cast to restricted __le32                            
pcan_usb.c:635:30: warning: cast to restricted __le16                            
pcan_usb_pro.c:158:28: warning: incorrect type in assignment (different base types)
pcan_usb_pro.c:158:28:    expected unsigned int [unsigned] [usertype] <noident>  
pcan_usb_pro.c:158:28:    got restricted __le32 [usertype] <noident>             
pcan_usb_pro.c:168:28: warning: incorrect type in assignment (different base types)
pcan_usb_pro.c:168:28:    expected unsigned int [unsigned] [usertype] <noident>  
pcan_usb_pro.c:168:28:    got restricted __le32 [usertype] <noident>             
pcan_usb_pro.c:176:28: warning: incorrect type in assignment (different base types)
pcan_usb_pro.c:176:28:    expected unsigned short [unsigned] [short] [usertype] <noident>
pcan_usb_pro.c:176:28:    got restricted __le16 [usertype] <noident>             
pcan_usb_pro.c:182:28: warning: incorrect type in assignment (different base types)
pcan_usb_pro.c:182:28:    expected unsigned short [unsigned] [short] [usertype] <noident>
pcan_usb_pro.c:182:28:    got restricted __le16 [usertype] <noident>             
pcan_usb_pro.c:184:28: warning: incorrect type in assignment (different base types)
pcan_usb_pro.c:184:28:    expected unsigned int [unsigned] [usertype] <noident>  
pcan_usb_pro.c:184:28:    got restricted __le32 [usertype] <noident>             
pcan_usb_pro.c:190:28: warning: incorrect type in assignment (different base types)
pcan_usb_pro.c:190:28:    expected unsigned short [unsigned] [short] [usertype] <noident>
pcan_usb_pro.c:190:28:    got restricted __le16 [usertype] <noident>             
pcan_usb_pro.c:203:32: warning: incorrect type in assignment (different base types)
pcan_usb_pro.c:203:32:    expected unsigned int [unsigned] [usertype] <noident>  
pcan_usb_pro.c:203:32:    got restricted __le32 [usertype] <noident>             
pcan_usb_pro.c:286:27: warning: cast to restricted __le32                        
pcan_usb_pro.c:458:30: warning: cast to restricted __le32                        
pcan_usb_pro.c:550:29: warning: cast to restricted __le32                        
pcan_usb_pro.c:561:47: warning: cast to restricted __le32                        
pcan_usb_pro.c:575:32: warning: cast to restricted __le32                        
pcan_usb_pro.c:605:35: warning: cast to restricted __le32                        
pcan_usb_pro.c:606:35: warning: cast to restricted __le32                        
pcan_usb_pro.c:678:47: warning: cast to restricted __le32                        
pcan_usb_pro.c:696:37: warning: cast to restricted __le32                        
pcan_usb_pro.c:720:19: warning: cast to restricted __le16                        

Compile your kernel with "make C=1 CF=-D__CHECK_ENDIAN__", you neet to 
have the tool "sparse" install. On debian/ubuntu/... it's: "sudo aptitude install sparse"

>  3 files changed, 1271 insertions(+)
>  create mode 100644 drivers/net/can/usb/peak_usb/pcan_ucan.h
>  create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>  create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.h
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_ucan.h b/drivers/net/can/usb/peak_usb/pcan_ucan.h
> new file mode 100644
> index 0000000..2223c05
> --- /dev/null
> +++ b/drivers/net/can/usb/peak_usb/pcan_ucan.h
> @@ -0,0 +1,208 @@
> +/*
> + * CAN driver for PEAK System micro-CAN based adapters
> + *
> + * Copyright (C) 2003-2011 PEAK System-Technik GmbH
> + * Copyright (C) 2011-2013 Stephane Grosjean <s.grosjean@peak-system.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published
> + * by the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +#ifndef PUCAN_H
> +#define PUCAN_H
> +
> +/* uCAN commands opcodes list (low-order 10 bits) */
> +#define PUCAN_CMD_NOP			0x000
> +#define PUCAN_CMD_RESET_MODE		0x001
> +#define PUCAN_CMD_NORMAL_MODE		0x002
> +#define PUCAN_CMD_LISTEN_ONLY_MODE	0x003
> +#define PUCAN_CMD_TIMING_SLOW		0x004
> +#define PUCAN_CMD_TIMING_FAST		0x005
> +#define PUCAN_CMD_FILTER_STD		0x008
> +#define PUCAN_CMD_TX_ABORT		0x009
> +#define PUCAN_CMD_WR_ERR_CNT		0x00a
> +#define PUCAN_CMD_RX_FRAME_ENABLE	0x00b
> +#define PUCAN_CMD_RX_FRAME_DISABLE	0x00c
> +#define PUCAN_CMD_END_OF_COLLECTION	0x3ff
> +
> +/* uCAN received messages list */
> +#define PUCAN_MSG_CAN_RX		0x0001
> +#define PUCAN_MSG_ERROR			0x0002
> +#define PUCAN_MSG_STATUS		0x0003
> +#define PUCAN_MSG_BUSLOAD		0x0004
> +#define PUCAN_MSG_CAN_TX		0x1000
> +
> +/* uCAN command common header */
> +#define PUCAN_CMD_OPCODE(c)		((c)->opcode_channel & 0x3ff)
> +#define PUCAN_CMD_CHANNEL(c)		((c)->opcode_channel >> 12)

Please don't hide the pointer deref in a macro. Is this actually used?

> +#define PUCAN_CMD_OPCODE_CHANNEL(c, o)	cpu_to_le16(((c) << 12) | ((o) & 0x3ff))

I'm not sure, but I think I don't like hiding of the cpu_to_le16.
Can you make this a static inline function?

> +
> +struct __packed pucan_command {
> +	u16	opcode_channel;
> +	u16	args[3];
> +};
> +
> +/* uCAN TIMING_SLOW command fields */
> +#define PUCAN_TSLOW_SJW_T(s, t)		(((s) & 0xf) | ((!!(t)) << 7))
> +#define PUCAN_TSLOW_TSEG2(t)		((t) & 0xf)
> +#define PUCAN_TSLOW_TSEG1(t)		((t) & 0x3f)
> +#define PUCAN_TSLOW_BRP(b)		cpu_to_le16((b) & 0x3ff)
> +
> +struct __packed pucan_timing_slow {
> +	u16	opcode_channel;
> +
> +	u8	ewl;		/* Error Warning limit */
> +	u8	sjw_t;		/* Sync Jump Width + Triple sampling */
> +	u8	tseg2;		/* Timing SEGment 2 */
> +	u8	tseg1;		/* Timing SEGment 1 */
> +
> +	u16	brp;		/* BaudRate Prescaler */
> +};
> +
> +/* uCAN TIMING_FAST command fields */
> +#define PUCAN_TFAST_SJW(s)		((s) & 0x3)
> +#define PUCAN_TFAST_TSEG2(t)		((t) & 0x7)
> +#define PUCAN_TFAST_TSEG1(t)		((t) & 0xf)
> +#define PUCAN_TFAST_BRP(b)		cpu_to_le16((b) & 0x3ff)
> +
> +struct __packed pucan_timing_fast {
> +	u16	opcode_channel;
> +
> +	u8	unused;
> +	u8	sjw;		/* Sync Jump Width */
> +	u8	tseg2;		/* Timing SEGment 2 */
> +	u8	tseg1;		/* Timing SEGment 1 */
> +
> +	u16	brp;		/* BaudRate Prescaler */
> +};
> +
> +/* uCAN FILTER_STD command fields */
> +#define PUCAN_FLTSTD_ROW_IDX_BITS	6
> +
> +struct __packed pucan_filter_std {
> +	u16	opcode_channel;
> +
> +	u16	idx;
> +	u32	mask;		/* CAN-ID bitmask in idx range */
> +};
> +
> +/* uCAN WR_ERR_CNT command fields */
> +#define PUCAN_WRERRCNT_TE(c)		0x4000	/* Tx error cntr write Enable */
> +#define PUCAN_WRERRCNT_RE(c)		0x8000	/* Rx error cntr write Enable */
What's the purpose of this   ^

Is this actually used?

> +
> +struct __packed pucan_wr_err_cnt {
> +	u16	opcode_channel;
> +
> +	u16	sel_mask;
> +	u8	tx_counter;	/* Tx error counter new value */
> +	u8	rx_counter;	/* Rx error counter new value */
> +
> +	u16	unused;
> +};
> +
> +/* uCAN RX_FRAME_ENABLE command fields */
> +#define PUCAN_FLTEXT_ERROR		0x0001
> +#define PUCAN_FLTEXT_BUSLOAD		0x0002
> +
> +struct __packed pucan_filter_ext {
> +	u16	opcode_channel;
> +
> +	u16	ext_mask;
> +	u32	unused;
> +};
> +
> +/* uCAN received messages global format */
> +struct __packed pucan_msg {
> +	__le16	size;
> +	__le16	type;
> +	__le32	ts_low;
> +	__le32	ts_high;
> +};
> +
> +/* uCAN flags for CAN/CANFD messages */
> +#define PUCAN_MSG_SELF_RECEIVE		0x80
> +#define PUCAN_MSG_ERROR_STATE_IND	0x40	/* error state indicator */
> +#define PUCAN_MSG_BITRATE_SWITCH	0x20	/* bitrate switch */
> +#define PUCAN_MSG_EXT_DATA_LEN		0x10	/* extended data length */
> +#define PUCAN_MSG_SINGLE_SHOT		0x08
> +#define PUCAN_MSG_LOOPED_BACK		0x04
> +#define PUCAN_MSG_EXT_ID		0x02
> +#define PUCAN_MSG_RTR			0x01
> +
> +#define PUCAN_MSG_CHANNEL(m)		((m)->channel_dlc & 0xf)
> +#define PUCAN_MSG_DLC(m)		((m)->channel_dlc >> 4)
> +
> +struct __packed pucan_rx_msg {
> +	__le16	size;
> +	__le16	type;
> +	__le32	ts_low;
> +	__le32	ts_high;
> +	__le32	tag_low;
> +	__le32	tag_high;
> +	u8	channel_dlc;
> +	u8	client;
> +	__le16	flags;
> +	__le32	can_id;
> +	u8	d[0];
> +};
> +
> +/* uCAN error types */
> +#define PUCAN_ERMSG_BIT_ERROR		0
> +#define PUCAN_ERMSG_FORM_ERROR		1
> +#define PUCAN_ERMSG_STUFF_ERROR		2
> +#define PUCAN_ERMSG_OTHER_ERROR		3
> +#define PUCAN_ERMSG_ERR_CNT_DEC		4
> +
> +#define PUCAN_ERMSG_CHANNEL(e)		((e)->channel_type_d & 0x0f)
> +#define PUCAN_ERMSG_ERRTYPE(e)		(((e)->channel_type_d >> 4) & 0x07)
> +#define PUCAN_ERMSG_D(e)		((e)->channel_type_d & 0x80)
> +
> +#define PUCAN_ERMSG_ERRCODE(e)		((e)->code_g & 0x7f)
> +#define PUCAN_ERMSG_G(e)		((e)->code_g & 0x80)
> +
> +struct __packed pucan_error_msg {
> +	__le16	size;
> +	__le16	type;
> +	__le32	ts_low;
> +	__le32	ts_high;
> +	u8	channel_type_d;
> +	u8	code_g;
> +	u8	tx_err_cnt;
> +	u8	rx_err_cnt;
> +};
> +
> +#define PUCAN_STMSG_CHANNEL(e)		((e)->channel_p_w_b & 0x0f)
> +#define PUCAN_STMSG_PASSIVE(e)		((e)->channel_p_w_b & 0x20)
> +#define PUCAN_STMSG_WARNING(e)		((e)->channel_p_w_b & 0x40)
> +#define PUCAN_STMSG_BUSOFF(e)		((e)->channel_p_w_b & 0x80)
> +
> +struct __packed pucan_status_msg {
> +	__le16	size;
> +	__le16	type;
> +	__le32	ts_low;
> +	__le32	ts_high;
> +	u8	channel_p_w_b;
> +	u8	unused[3];
> +};
> +
> +/* uCAN transmitted message format */
> +#define PUCAN_MSG_CHANNEL_DLC(c, d)	(((c) & 0xf) | ((d) << 4))
> +
> +struct __packed pucan_tx_msg {
> +	__le16	size;
> +	__le16	type;
> +	__le32	tag_low;
> +	__le32	tag_high;
> +	u8	channel_dlc;
> +	u8	client;
> +	u16	flags;
> +	__le32	can_id;
> +	u8	d[0];
> +};
> +
> +#endif
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> new file mode 100644
> index 0000000..3d392a8
> --- /dev/null
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -0,0 +1,955 @@
> +/*
> + * CAN driver for PEAK System PCAN-USB FD / PCAN-USB Pro FD adapter
> + *
> + * Copyright (C) 2013-2014 Stephane Grosjean <s.grosjean@peak-system.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published
> + * by the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +#include <linux/netdevice.h>
> +#include <linux/usb.h>
> +#include <linux/module.h>
> +
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +
> +#include "pcan_usb_core.h"
> +#include "pcan_usb_fd.h"
> +
> +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB FD adapter");
> +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro FD adapter");
> +
> +#define ALIGN32(x)			(((x) + 3) & 0xFFFFFFFC)

Please use http://lxr.free-electrons.com/source/include/linux/kernel.h#L49

> +
> +#define PCAN_USBPROFD_CHANNEL_COUNT	2
> +#define PCAN_USBFD_CHANNEL_COUNT	1
> +
> +/* PCAN-USB Pro FD adapter internal clock (MHz) */
> +#define PCAN_UFD_CRYSTAL_HZ		80000000
> +
> +#define PCAN_UFD_CMD_BUFFER_SIZE	512
> +#define PCAN_UFD_LOSPD_PKT_SIZE		64
> +
> +/* PCAN-USB Pro FD command timeout (ms.) */
> +#define PCAN_UFD_COMMAND_TIMEOUT	1000
                                 add _MS here, makes it more readable
> +
> +/* PCAN-USB Pro FD rx/tx buffers size */
> +#define PCAN_UFD_RX_BUFFER_SIZE		2048
> +#define PCAN_UFD_TX_BUFFER_SIZE		512
> +
> +/* handle device specific info used by the netdevices */
> +struct pcan_usb_fd_if {
> +	struct peak_usb_device *dev[PCAN_USB_MAX_CHANNEL];
> +	struct peak_time_ref time_ref;
> +	int cm_ignore_count;
> +	int dev_opened_count;
> +};
> +
> +/* device information */
> +struct pcan_usb_fd_device {
> +	struct peak_usb_device dev;
> +	struct pcan_usb_fd_if *usb_if;
> +
> +	u8 *cmd_buffer_addr;
> +
> +	uint tx_error_counter;
> +	uint rx_error_counter;
> +};
> +
> +/* Clock mode frequence values */
> +static const u32 pcan_usb_fd_clk_freq[6] = {
> +	[PCAN_UFD_CLK_80MHZ] = 80000000,
> +	[PCAN_UFD_CLK_60MHZ] = 60000000,
> +	[PCAN_UFD_CLK_40MHZ] = 40000000,
> +	[PCAN_UFD_CLK_30MHZ] = 30000000,
> +	[PCAN_UFD_CLK_24MHZ] = 24000000,
> +	[PCAN_UFD_CLK_20MHZ] = 20000000
> +};
> +
> +/* functions exported from PCAN-USB Pro interface */
> +extern int pcan_usb_pro_probe(struct usb_interface *intf);
> +extern int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
> +				 int req_value, void *req_addr, int req_size);
> +extern void pcan_usb_pro_restart_complete(struct urb *urb);

This belongs into a header file. No "extern" though.

> +
> +/* return a device USB interface */
> +static inline
> +struct pcan_usb_fd_if *pcan_usb_fd_dev_if(struct peak_usb_device *dev)
> +{
> +	struct pcan_usb_fd_device *pdev =
> +			container_of(dev, struct pcan_usb_fd_device, dev);
> +	return pdev->usb_if;
> +}
> +
> +/* return a device USB commands buffer */
> +static inline void *pcan_usb_fd_cmd_buffer(struct peak_usb_device *dev)
> +{
> +	struct pcan_usb_fd_device *pdev =
> +			container_of(dev, struct pcan_usb_fd_device, dev);
> +	return pdev->cmd_buffer_addr;
> +}
> +
> +/* send PCAN-USB Pro FD commands synchronously */
> +static int pcan_usb_fd_send_cmd(struct peak_usb_device *dev, void *cmd_tail)
> +{
> +	void *cmd_head = pcan_usb_fd_cmd_buffer(dev);
> +	int actual_length;
> +	int err, cmd_len;

cmd_len is a long (or ptrdiff_t), as it's the diff of two pointers.

> +	u8 *packet_ptr;
> +	int p, n = 1, packet_len;
> +
> +	/* usb device unregistered? */
> +	if (!(dev->state & PCAN_USB_STATE_CONNECTED))
> +		return 0;
> +
> +	/*
> +	 * if a packet is not filled completely by commands, the command list
> +	 * is terminated with an "end of collection" record.
> +	 */
> +	cmd_len = cmd_tail - cmd_head;
> +	if (cmd_len < PCAN_UFD_CMD_BUFFER_SIZE) {

What happens if cmd_len turns out to be 511?

> +		memset(cmd_tail, 0xff, sizeof(u64));
> +		cmd_len += sizeof(u64);
> +	}
> +
> +	packet_ptr = cmd_head;
> +
> +	/* firmware is not able to re-assemble 512 bytes buffer in full-speed */
> +	if ((dev->udev->speed != USB_SPEED_HIGH) &&
> +	    (cmd_len > PCAN_UFD_LOSPD_PKT_SIZE)) {
> +		packet_len = PCAN_UFD_LOSPD_PKT_SIZE;
> +		n += cmd_len / packet_len;
> +	} else {
> +		packet_len = cmd_len;
> +	}
> +
> +	for (p = 1; p <= n; p++) {

why not the usual, start at 0? (p = 0, p < n, p++), or even "i" :)

> +		err = usb_bulk_msg(dev->udev,
> +				   usb_sndbulkpipe(dev->udev,
> +						   PCAN_USBPRO_EP_CMDOUT),
> +				   packet_ptr, packet_len,
> +				   &actual_length, PCAN_UFD_COMMAND_TIMEOUT);

Do you have to take care about actual_length? If not, you can pass NULL here AFAICS.

> +		if (err) {
> +			netdev_err(dev->netdev,
> +				   "sending command failure: %d\n", err);
> +			break;
> +		}
> +
> +		packet_ptr += packet_len;
> +	}
> +
> +	return err;
> +}
> +
> +static int pcan_usb_fd_set_bus(struct peak_usb_device *dev, u8 onoff)
                                                               bool on
> +{
> +	struct pucan_command *cmd = pcan_usb_fd_cmd_buffer(dev);
> +	u16 opcode;
> +
> +	if (onoff) {
> +		opcode = (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) ?
> +				PUCAN_CMD_LISTEN_ONLY_MODE :
> +				PUCAN_CMD_NORMAL_MODE;
> +	} else {
> +		opcode = PUCAN_CMD_RESET_MODE;
> +	}
> +
> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx, opcode);
> +
> +	/* send the command */
> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
> +}
> +
> +/*
> + * Set filtering masks:
> + *
> + *	idx in range [0..63] selects row #idx, all rows otherwise.
> + *	mask in range [0..0xffffffff]
> + *
> + */
> +static int pcan_usb_fd_set_filter_std(struct peak_usb_device *dev, int idx,
> +				      u32 mask)
> +{
> +	return 0;
empty function?
> +}
> +
> +/*
> + * set/unset notifications filter:
> + *
> + *	onoff	sets(1)/unset(0) notifications
> + *	mask	each bit defines a kind of notification to set/unset
> + */
> +static int pcan_usb_fd_set_filter_ext(struct peak_usb_device *dev,
> +				      int onoff, u16 ext_mask, u16 usb_mask)
> +{
> +	struct pcan_ufd_filter_ext *cmd = pcan_usb_fd_cmd_buffer(dev);
> +
> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
> +					(onoff) ? PUCAN_CMD_RX_FRAME_ENABLE :
> +						  PUCAN_CMD_RX_FRAME_DISABLE);
> +
> +	cmd->ext_mask = cpu_to_le16(ext_mask);
> +	cmd->usb_mask = cpu_to_le16(usb_mask);
> +
> +	/* send the command */
> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
> +}
> +
> +/* setup LED control */
> +static int pcan_usb_fd_set_can_led(struct peak_usb_device *dev, u8 led_mode)
> +{
> +	struct pcan_ufd_led *cmd = pcan_usb_fd_cmd_buffer(dev);
> +
> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
> +						       PCAN_UFD_CMD_LED_SET);
> +	cmd->mode = led_mode;
> +
> +	/* send the command */
> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
> +}
> +
> +/* set CAN clock domain */
> +static int pcan_usb_fd_set_clock_domain(struct peak_usb_device *dev,
> +					u8 clk_mode)
> +{
> +	struct pcan_ufd_clock *cmd = pcan_usb_fd_cmd_buffer(dev);
> +
> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
> +						       PCAN_UFD_CMD_CLK_SET);
> +	cmd->mode = clk_mode;
> +
> +	/* send the command */
> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
> +}
> +
> +/* set bittiming for CAN and CAN-FD header */
> +static int pcan_usb_fd_set_bittiming_slow(struct peak_usb_device *dev,
> +					  struct can_bittiming *bt)
> +{
> +	struct pucan_timing_slow *cmd = pcan_usb_fd_cmd_buffer(dev);
> +
> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
> +						       PUCAN_CMD_TIMING_SLOW);
> +	cmd->sjw_t = PUCAN_TSLOW_SJW_T(bt->sjw - 1,
> +				dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES);
> +
> +	cmd->tseg2 = PUCAN_TSLOW_TSEG2(bt->phase_seg2 - 1);
> +	cmd->tseg1 = PUCAN_TSLOW_TSEG1(bt->prop_seg + bt->phase_seg1 - 1);
> +	cmd->brp = PUCAN_TSLOW_BRP(bt->brp - 1);
> +
> +	cmd->ewl = 96;	/* default */
> +
> +	/* send the command */
> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
> +}
> +
> +/* set CAN-FD bittiming for data */
> +static int pcan_usb_fd_set_bittiming_fast(struct peak_usb_device *dev,
> +					  struct can_bittiming *bt)
> +{
> +	struct pucan_timing_fast *cmd = pcan_usb_fd_cmd_buffer(dev);
> +
> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
> +						       PUCAN_CMD_TIMING_FAST);
> +	cmd->sjw = PUCAN_TFAST_SJW(bt->sjw - 1);
> +	cmd->tseg2 = PUCAN_TFAST_TSEG2(bt->phase_seg2 - 1);
> +	cmd->tseg1 = PUCAN_TFAST_TSEG1(bt->prop_seg + bt->phase_seg1 - 1);
> +	cmd->brp = PUCAN_TFAST_BRP(bt->brp - 1);
> +
> +	/* send the command */
> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
> +}

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: 819 bytes --]

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Stephane Grosjean]

Thanks for your review, Marc.

Please see my acks and comments below...

Regards,

Stéphane.

Le 02/12/2014 14:13, Marc Kleine-Budde a écrit :
>
>
> -------- Original Message --------
> Subject: Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific
> files
> Date: Thu, 27 Nov 2014 23:28:51 +0100
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> To: Stephane Grosjean <s.grosjean@peak-system.com>,
> linux-can@vger.kernel.org
> CC: Oliver Hartkopp <socketcan@hartkopp.net>
>
> On 11/27/2014 11:32 AM, Stephane Grosjean wrote:
>> This patch adds 3 new files to add support to the following CAN-FD USB adapters
>> from PEAK-System Technik:
>>
>> PCAN-USB FD             single CAN-FD channel USB adapter
>> PCAN-USB Pro FD         dual CAN-FD channels USB adapter
>>
>> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>²
>> ---
>>   drivers/net/can/usb/peak_usb/pcan_ucan.h   | 208 +++++++
>>   drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 955 +++++++++++++++++++++++++++++
>>   drivers/net/can/usb/peak_usb/pcan_usb_fd.h | 108 ++++
> Please don't create a header file, if it's only included once. Please
> merge into the correct .c file.

Ok. So I merge both header files into the .c file. There will be only a 
single (and big) pcan_usb_fd.c file.

> Please have a look the _all_ multi-byte values and check for endianess.
>
> Hint, you might want to fix the existing driver first:
>
> pcan_usb_core.c:863:60: warning: restricted __le16 degrades to integer
>
> pcan_usb.c:322:34: warning: cast to restricted __le32
>
> pcan_usb.c:357:20: warning: cast to restricted __le16
>
> pcan_usb.c:382:28: warning: cast to restricted __le16
>
> pcan_usb.c:625:30: warning: cast to restricted __le32
>
> pcan_usb.c:635:30: warning: cast to restricted __le16
>
> pcan_usb_pro.c:158:28: warning: incorrect type in assignment (different
> base types)
> pcan_usb_pro.c:158:28:    expected unsigned int [unsigned] [usertype]
> <noident>
> pcan_usb_pro.c:158:28:    got restricted __le32 [usertype] <noident>
>
> pcan_usb_pro.c:168:28: warning: incorrect type in assignment (different
> base types)
> pcan_usb_pro.c:168:28:    expected unsigned int [unsigned] [usertype]
> <noident>
> pcan_usb_pro.c:168:28:    got restricted __le32 [usertype] <noident>
>
> pcan_usb_pro.c:176:28: warning: incorrect type in assignment (different
> base types)
> pcan_usb_pro.c:176:28:    expected unsigned short [unsigned] [short]
> [usertype] <noident>
> pcan_usb_pro.c:176:28:    got restricted __le16 [usertype] <noident>
>
> pcan_usb_pro.c:182:28: warning: incorrect type in assignment (different
> base types)
> pcan_usb_pro.c:182:28:    expected unsigned short [unsigned] [short]
> [usertype] <noident>
> pcan_usb_pro.c:182:28:    got restricted __le16 [usertype] <noident>
>
> pcan_usb_pro.c:184:28: warning: incorrect type in assignment (different
> base types)
> pcan_usb_pro.c:184:28:    expected unsigned int [unsigned] [usertype]
> <noident>
> pcan_usb_pro.c:184:28:    got restricted __le32 [usertype] <noident>
>
> pcan_usb_pro.c:190:28: warning: incorrect type in assignment (different
> base types)
> pcan_usb_pro.c:190:28:    expected unsigned short [unsigned] [short]
> [usertype] <noident>
> pcan_usb_pro.c:190:28:    got restricted __le16 [usertype] <noident>
>
> pcan_usb_pro.c:203:32: warning: incorrect type in assignment (different
> base types)
> pcan_usb_pro.c:203:32:    expected unsigned int [unsigned] [usertype]
> <noident>
> pcan_usb_pro.c:203:32:    got restricted __le32 [usertype] <noident>
>
> pcan_usb_pro.c:286:27: warning: cast to restricted __le32
>
> pcan_usb_pro.c:458:30: warning: cast to restricted __le32
>
> pcan_usb_pro.c:550:29: warning: cast to restricted __le32
>
> pcan_usb_pro.c:561:47: warning: cast to restricted __le32
>
> pcan_usb_pro.c:575:32: warning: cast to restricted __le32
>
> pcan_usb_pro.c:605:35: warning: cast to restricted __le32
>
> pcan_usb_pro.c:606:35: warning: cast to restricted __le32
>
> pcan_usb_pro.c:678:47: warning: cast to restricted __le32
>
> pcan_usb_pro.c:696:37: warning: cast to restricted __le32
>
> pcan_usb_pro.c:720:19: warning: cast to restricted __le16
>
>
> Compile your kernel with "make C=1 CF=-D__CHECK_ENDIAN__", you neet to
> have the tool "sparse" install. On debian/ubuntu/... it's: "sudo
> aptitude install sparse"

Ok, I'll do it too. And I suppose I'll push these changes in a separate 
patch too...

>>   3 files changed, 1271 insertions(+)
>>   create mode 100644 drivers/net/can/usb/peak_usb/pcan_ucan.h
>>   create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>>   create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.h
>>
>> diff --git a/drivers/net/can/usb/peak_usb/pcan_ucan.h b/drivers/net/can/usb/peak_usb/pcan_ucan.h
>> new file mode 100644
>> index 0000000..2223c05
>> --- /dev/null
>> +++ b/drivers/net/can/usb/peak_usb/pcan_ucan.h
>> @@ -0,0 +1,208 @@
>> +/*
>> + * CAN driver for PEAK System micro-CAN based adapters
>> + *
>> + * Copyright (C) 2003-2011 PEAK System-Technik GmbH
>> + * Copyright (C) 2011-2013 Stephane Grosjean <s.grosjean@peak-system.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published
>> + * by the Free Software Foundation; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + */
>> +#ifndef PUCAN_H
>> +#define PUCAN_H
>> +
>> +/* uCAN commands opcodes list (low-order 10 bits) */
>> +#define PUCAN_CMD_NOP			0x000
>> +#define PUCAN_CMD_RESET_MODE		0x001
>> +#define PUCAN_CMD_NORMAL_MODE		0x002
>> +#define PUCAN_CMD_LISTEN_ONLY_MODE	0x003
>> +#define PUCAN_CMD_TIMING_SLOW		0x004
>> +#define PUCAN_CMD_TIMING_FAST		0x005
>> +#define PUCAN_CMD_FILTER_STD		0x008
>> +#define PUCAN_CMD_TX_ABORT		0x009
>> +#define PUCAN_CMD_WR_ERR_CNT		0x00a
>> +#define PUCAN_CMD_RX_FRAME_ENABLE	0x00b
>> +#define PUCAN_CMD_RX_FRAME_DISABLE	0x00c
>> +#define PUCAN_CMD_END_OF_COLLECTION	0x3ff
>> +
>> +/* uCAN received messages list */
>> +#define PUCAN_MSG_CAN_RX		0x0001
>> +#define PUCAN_MSG_ERROR			0x0002
>> +#define PUCAN_MSG_STATUS		0x0003
>> +#define PUCAN_MSG_BUSLOAD		0x0004
>> +#define PUCAN_MSG_CAN_TX		0x1000
>> +
>> +/* uCAN command common header */
>> +#define PUCAN_CMD_OPCODE(c)		((c)->opcode_channel & 0x3ff)
>> +#define PUCAN_CMD_CHANNEL(c)		((c)->opcode_channel >> 12)
> Please don't hide the pointer deref in a macro. Is this actually used?

No, you're right. Both aren't used in fact. Removed.

>
>> +#define PUCAN_CMD_OPCODE_CHANNEL(c, o)	cpu_to_le16(((c) << 12) | ((o) & 0x3ff))
> I'm not sure, but I think I don't like hiding of the cpu_to_le16.
> Can you make this a static inline function?

Ok. So I suppose I have to do the same (inline function making) for all 
the other occurrences of "cpu_to_le16()"  in this file, right?

>> +
>> +struct __packed pucan_command {
>> +	u16	opcode_channel;
>> +	u16	args[3];
>> +};
>> +
>> +/* uCAN TIMING_SLOW command fields */
>> +#define PUCAN_TSLOW_SJW_T(s, t)		(((s) & 0xf) | ((!!(t)) << 7))
>> +#define PUCAN_TSLOW_TSEG2(t)		((t) & 0xf)
>> +#define PUCAN_TSLOW_TSEG1(t)		((t) & 0x3f)
>> +#define PUCAN_TSLOW_BRP(b)		cpu_to_le16((b) & 0x3ff)
>> +
>> +struct __packed pucan_timing_slow {
>> +	u16	opcode_channel;
>> +
>> +	u8	ewl;		/* Error Warning limit */
>> +	u8	sjw_t;		/* Sync Jump Width + Triple sampling */
>> +	u8	tseg2;		/* Timing SEGment 2 */
>> +	u8	tseg1;		/* Timing SEGment 1 */
>> +
>> +	u16	brp;		/* BaudRate Prescaler */
>> +};
>> +
>> +/* uCAN TIMING_FAST command fields */
>> +#define PUCAN_TFAST_SJW(s)		((s) & 0x3)
>> +#define PUCAN_TFAST_TSEG2(t)		((t) & 0x7)
>> +#define PUCAN_TFAST_TSEG1(t)		((t) & 0xf)
>> +#define PUCAN_TFAST_BRP(b)		cpu_to_le16((b) & 0x3ff)
>> +
>> +struct __packed pucan_timing_fast {
>> +	u16	opcode_channel;
>> +
>> +	u8	unused;
>> +	u8	sjw;		/* Sync Jump Width */
>> +	u8	tseg2;		/* Timing SEGment 2 */
>> +	u8	tseg1;		/* Timing SEGment 1 */
>> +
>> +	u16	brp;		/* BaudRate Prescaler */
>> +};
>> +
>> +/* uCAN FILTER_STD command fields */
>> +#define PUCAN_FLTSTD_ROW_IDX_BITS	6
>> +
>> +struct __packed pucan_filter_std {
>> +	u16	opcode_channel;
>> +
>> +	u16	idx;
>> +	u32	mask;		/* CAN-ID bitmask in idx range */
>> +};
>> +
>> +/* uCAN WR_ERR_CNT command fields */
>> +#define PUCAN_WRERRCNT_TE(c)		0x4000	/* Tx error cntr write Enable */
>> +#define PUCAN_WRERRCNT_RE(c)		0x8000	/* Rx error cntr write Enable */
> What's the purpose of this   ^
>
> Is this actually used?

No it isn't actually used is this driver. I'll remove them.

>
>> +
>> +struct __packed pucan_wr_err_cnt {
>> +	u16	opcode_channel;
>> +
>> +	u16	sel_mask;
>> +	u8	tx_counter;	/* Tx error counter new value */
>> +	u8	rx_counter;	/* Rx error counter new value */
>> +
>> +	u16	unused;
>> +};
>> +
>> +/* uCAN RX_FRAME_ENABLE command fields */
>> +#define PUCAN_FLTEXT_ERROR		0x0001
>> +#define PUCAN_FLTEXT_BUSLOAD		0x0002
>> +
>> +struct __packed pucan_filter_ext {
>> +	u16	opcode_channel;
>> +
>> +	u16	ext_mask;
>> +	u32	unused;
>> +};
>> +
>> +/* uCAN received messages global format */
>> +struct __packed pucan_msg {
>> +	__le16	size;
>> +	__le16	type;
>> +	__le32	ts_low;
>> +	__le32	ts_high;
>> +};
>> +
>> +/* uCAN flags for CAN/CANFD messages */
>> +#define PUCAN_MSG_SELF_RECEIVE		0x80
>> +#define PUCAN_MSG_ERROR_STATE_IND	0x40	/* error state indicator */
>> +#define PUCAN_MSG_BITRATE_SWITCH	0x20	/* bitrate switch */
>> +#define PUCAN_MSG_EXT_DATA_LEN		0x10	/* extended data length */
>> +#define PUCAN_MSG_SINGLE_SHOT		0x08
>> +#define PUCAN_MSG_LOOPED_BACK		0x04
>> +#define PUCAN_MSG_EXT_ID		0x02
>> +#define PUCAN_MSG_RTR			0x01
>> +
>> +#define PUCAN_MSG_CHANNEL(m)		((m)->channel_dlc & 0xf)
>> +#define PUCAN_MSG_DLC(m)		((m)->channel_dlc >> 4)
>> +
>> +struct __packed pucan_rx_msg {
>> +	__le16	size;
>> +	__le16	type;
>> +	__le32	ts_low;
>> +	__le32	ts_high;
>> +	__le32	tag_low;
>> +	__le32	tag_high;
>> +	u8	channel_dlc;
>> +	u8	client;
>> +	__le16	flags;
>> +	__le32	can_id;
>> +	u8	d[0];
>> +};
>> +
>> +/* uCAN error types */
>> +#define PUCAN_ERMSG_BIT_ERROR		0
>> +#define PUCAN_ERMSG_FORM_ERROR		1
>> +#define PUCAN_ERMSG_STUFF_ERROR		2
>> +#define PUCAN_ERMSG_OTHER_ERROR		3
>> +#define PUCAN_ERMSG_ERR_CNT_DEC		4
>> +
>> +#define PUCAN_ERMSG_CHANNEL(e)		((e)->channel_type_d & 0x0f)
>> +#define PUCAN_ERMSG_ERRTYPE(e)		(((e)->channel_type_d >> 4) & 0x07)
>> +#define PUCAN_ERMSG_D(e)		((e)->channel_type_d & 0x80)
>> +
>> +#define PUCAN_ERMSG_ERRCODE(e)		((e)->code_g & 0x7f)
>> +#define PUCAN_ERMSG_G(e)		((e)->code_g & 0x80)
>> +
>> +struct __packed pucan_error_msg {
>> +	__le16	size;
>> +	__le16	type;
>> +	__le32	ts_low;
>> +	__le32	ts_high;
>> +	u8	channel_type_d;
>> +	u8	code_g;
>> +	u8	tx_err_cnt;
>> +	u8	rx_err_cnt;
>> +};
>> +
>> +#define PUCAN_STMSG_CHANNEL(e)		((e)->channel_p_w_b & 0x0f)
>> +#define PUCAN_STMSG_PASSIVE(e)		((e)->channel_p_w_b & 0x20)
>> +#define PUCAN_STMSG_WARNING(e)		((e)->channel_p_w_b & 0x40)
>> +#define PUCAN_STMSG_BUSOFF(e)		((e)->channel_p_w_b & 0x80)
>> +
>> +struct __packed pucan_status_msg {
>> +	__le16	size;
>> +	__le16	type;
>> +	__le32	ts_low;
>> +	__le32	ts_high;
>> +	u8	channel_p_w_b;
>> +	u8	unused[3];
>> +};
>> +
>> +/* uCAN transmitted message format */
>> +#define PUCAN_MSG_CHANNEL_DLC(c, d)	(((c) & 0xf) | ((d) << 4))
>> +
>> +struct __packed pucan_tx_msg {
>> +	__le16	size;
>> +	__le16	type;
>> +	__le32	tag_low;
>> +	__le32	tag_high;
>> +	u8	channel_dlc;
>> +	u8	client;
>> +	u16	flags;
>> +	__le32	can_id;
>> +	u8	d[0];
>> +};
>> +
>> +#endif
>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>> new file mode 100644
>> index 0000000..3d392a8
>> --- /dev/null
>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>> @@ -0,0 +1,955 @@
>> +/*
>> + * CAN driver for PEAK System PCAN-USB FD / PCAN-USB Pro FD adapter
>> + *
>> + * Copyright (C) 2013-2014 Stephane Grosjean <s.grosjean@peak-system.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published
>> + * by the Free Software Foundation; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + */
>> +#include <linux/netdevice.h>
>> +#include <linux/usb.h>
>> +#include <linux/module.h>
>> +
>> +#include <linux/can.h>
>> +#include <linux/can/dev.h>
>> +#include <linux/can/error.h>
>> +
>> +#include "pcan_usb_core.h"
>> +#include "pcan_usb_fd.h"
>> +
>> +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB FD adapter");
>> +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro FD adapter");
>> +
>> +#define ALIGN32(x)			(((x) + 3) & 0xFFFFFFFC)
> Please use http://lxr.free-electrons.com/source/include/linux/kernel.h#L49

Ok.

>
>> +
>> +#define PCAN_USBPROFD_CHANNEL_COUNT	2
>> +#define PCAN_USBFD_CHANNEL_COUNT	1
>> +
>> +/* PCAN-USB Pro FD adapter internal clock (MHz) */
>> +#define PCAN_UFD_CRYSTAL_HZ		80000000
>> +
>> +#define PCAN_UFD_CMD_BUFFER_SIZE	512
>> +#define PCAN_UFD_LOSPD_PKT_SIZE		64
>> +
>> +/* PCAN-USB Pro FD command timeout (ms.) */
>> +#define PCAN_UFD_COMMAND_TIMEOUT	1000
>                                   add _MS here, makes it more readable
Ok.
>> +
>> +/* PCAN-USB Pro FD rx/tx buffers size */
>> +#define PCAN_UFD_RX_BUFFER_SIZE		2048
>> +#define PCAN_UFD_TX_BUFFER_SIZE		512
>> +
>> +/* handle device specific info used by the netdevices */
>> +struct pcan_usb_fd_if {
>> +	struct peak_usb_device *dev[PCAN_USB_MAX_CHANNEL];
>> +	struct peak_time_ref time_ref;
>> +	int cm_ignore_count;
>> +	int dev_opened_count;
>> +};
>> +
>> +/* device information */
>> +struct pcan_usb_fd_device {
>> +	struct peak_usb_device dev;
>> +	struct pcan_usb_fd_if *usb_if;
>> +
>> +	u8 *cmd_buffer_addr;
>> +
>> +	uint tx_error_counter;
>> +	uint rx_error_counter;
>> +};
>> +
>> +/* Clock mode frequence values */
>> +static const u32 pcan_usb_fd_clk_freq[6] = {
>> +	[PCAN_UFD_CLK_80MHZ] = 80000000,
>> +	[PCAN_UFD_CLK_60MHZ] = 60000000,
>> +	[PCAN_UFD_CLK_40MHZ] = 40000000,
>> +	[PCAN_UFD_CLK_30MHZ] = 30000000,
>> +	[PCAN_UFD_CLK_24MHZ] = 24000000,
>> +	[PCAN_UFD_CLK_20MHZ] = 20000000
>> +};
>> +
>> +/* functions exported from PCAN-USB Pro interface */
>> +extern int pcan_usb_pro_probe(struct usb_interface *intf);
>> +extern int pcan_usb_pro_send_req(struct peak_usb_device *dev, int req_id,
>> +				 int req_value, void *req_addr, int req_size);
>> +extern void pcan_usb_pro_restart_complete(struct urb *urb);
> This belongs into a header file. No "extern" though.

Well, ok. But FYI, scripts/checkpatch.pl doesn't like either "extern" 
keyword in the .h file...

>
>> +
>> +/* return a device USB interface */
>> +static inline
>> +struct pcan_usb_fd_if *pcan_usb_fd_dev_if(struct peak_usb_device *dev)
>> +{
>> +	struct pcan_usb_fd_device *pdev =
>> +			container_of(dev, struct pcan_usb_fd_device, dev);
>> +	return pdev->usb_if;
>> +}
>> +
>> +/* return a device USB commands buffer */
>> +static inline void *pcan_usb_fd_cmd_buffer(struct peak_usb_device *dev)
>> +{
>> +	struct pcan_usb_fd_device *pdev =
>> +			container_of(dev, struct pcan_usb_fd_device, dev);
>> +	return pdev->cmd_buffer_addr;
>> +}
>> +
>> +/* send PCAN-USB Pro FD commands synchronously */
>> +static int pcan_usb_fd_send_cmd(struct peak_usb_device *dev, void *cmd_tail)
>> +{
>> +	void *cmd_head = pcan_usb_fd_cmd_buffer(dev);
>> +	int actual_length;
>> +	int err, cmd_len;
> cmd_len is a long (or ptrdiff_t), as it's the diff of two pointers.

Ok. So it will be a "ptrdiff_t" .

>
>> +	u8 *packet_ptr;
>> +	int p, n = 1, packet_len;
>> +
>> +	/* usb device unregistered? */
>> +	if (!(dev->state & PCAN_USB_STATE_CONNECTED))
>> +		return 0;
>> +
>> +	/*
>> +	 * if a packet is not filled completely by commands, the command list
>> +	 * is terminated with an "end of collection" record.
>> +	 */
>> +	cmd_len = cmd_tail - cmd_head;
>> +	if (cmd_len < PCAN_UFD_CMD_BUFFER_SIZE) {
> What happens if cmd_len turns out to be 511?

Well, it can't: "commands" are 64-bits records. The "end of record" 
"command" must be added when the list of commands doesn't fill the 
entire buffer content.
Maybe it would be better to write:

    if (cmdlen <= (PCAN_UFD_CMD_BUFFER_SIZE - sizeof(u64)) {


instead. What's your opinion about that?

>
>> +		memset(cmd_tail, 0xff, sizeof(u64));
>> +		cmd_len += sizeof(u64);
>> +	}
>> +
>> +	packet_ptr = cmd_head;
>> +
>> +	/* firmware is not able to re-assemble 512 bytes buffer in full-speed */
>> +	if ((dev->udev->speed != USB_SPEED_HIGH) &&
>> +	    (cmd_len > PCAN_UFD_LOSPD_PKT_SIZE)) {
>> +		packet_len = PCAN_UFD_LOSPD_PKT_SIZE;
>> +		n += cmd_len / packet_len;
>> +	} else {
>> +		packet_len = cmd_len;
>> +	}
>> +
>> +	for (p = 1; p <= n; p++) {
> why not the usual, start at 0? (p = 0, p < n, p++), or even "i" :)

Well ok no problemo...

>
>> +		err = usb_bulk_msg(dev->udev,
>> +				   usb_sndbulkpipe(dev->udev,
>> +						   PCAN_USBPRO_EP_CMDOUT),
>> +				   packet_ptr, packet_len,
>> +				   &actual_length, PCAN_UFD_COMMAND_TIMEOUT);
> Do you have to take care about actual_length? If not, you can pass NULL
> here AFAICS.

Ok, you're right. This parameter can be NULL. Will be changed.

>
>> +		if (err) {
>> +			netdev_err(dev->netdev,
>> +				   "sending command failure: %d\n", err);
>> +			break;
>> +		}
>> +
>> +		packet_ptr += packet_len;
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +static int pcan_usb_fd_set_bus(struct peak_usb_device *dev, u8 onoff)
>                                                                 bool on

Humm... ok, but be advised that this change will have some side-effects 
on the other existing files (pcan_usb_core.h, pcan_usb.c and 
pcan_usb_pro.c should be modified accordingly).

>> +{
>> +	struct pucan_command *cmd = pcan_usb_fd_cmd_buffer(dev);
>> +	u16 opcode;
>> +
>> +	if (onoff) {
>> +		opcode = (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) ?
>> +				PUCAN_CMD_LISTEN_ONLY_MODE :
>> +				PUCAN_CMD_NORMAL_MODE;
>> +	} else {
>> +		opcode = PUCAN_CMD_RESET_MODE;
>> +	}
>> +
>> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx, opcode);
>> +
>> +	/* send the command */
>> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
>> +}
>> +
>> +/*
>> + * Set filtering masks:
>> + *
>> + *	idx in range [0..63] selects row #idx, all rows otherwise.
>> + *	mask in range [0..0xffffffff]
>> + *
>> + */
>> +static int pcan_usb_fd_set_filter_std(struct peak_usb_device *dev, int idx,
>> +				      u32 mask)
>> +{
>> +	return 0;
> empty function?

I'm afraid it is... ;-)

>> +}
>> +
>> +/*
>> + * set/unset notifications filter:
>> + *
>> + *	onoff	sets(1)/unset(0) notifications
>> + *	mask	each bit defines a kind of notification to set/unset
>> + */
>> +static int pcan_usb_fd_set_filter_ext(struct peak_usb_device *dev,
>> +				      int onoff, u16 ext_mask, u16 usb_mask)
>> +{
>> +	struct pcan_ufd_filter_ext *cmd = pcan_usb_fd_cmd_buffer(dev);
>> +
>> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
>> +					(onoff) ? PUCAN_CMD_RX_FRAME_ENABLE :
>> +						  PUCAN_CMD_RX_FRAME_DISABLE);
>> +
>> +	cmd->ext_mask = cpu_to_le16(ext_mask);
>> +	cmd->usb_mask = cpu_to_le16(usb_mask);
>> +
>> +	/* send the command */
>> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
>> +}
>> +
>> +/* setup LED control */
>> +static int pcan_usb_fd_set_can_led(struct peak_usb_device *dev, u8 led_mode)
>> +{
>> +	struct pcan_ufd_led *cmd = pcan_usb_fd_cmd_buffer(dev);
>> +
>> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
>> +						       PCAN_UFD_CMD_LED_SET);
>> +	cmd->mode = led_mode;
>> +
>> +	/* send the command */
>> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
>> +}
>> +
>> +/* set CAN clock domain */
>> +static int pcan_usb_fd_set_clock_domain(struct peak_usb_device *dev,
>> +					u8 clk_mode)
>> +{
>> +	struct pcan_ufd_clock *cmd = pcan_usb_fd_cmd_buffer(dev);
>> +
>> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
>> +						       PCAN_UFD_CMD_CLK_SET);
>> +	cmd->mode = clk_mode;
>> +
>> +	/* send the command */
>> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
>> +}
>> +
>> +/* set bittiming for CAN and CAN-FD header */
>> +static int pcan_usb_fd_set_bittiming_slow(struct peak_usb_device *dev,
>> +					  struct can_bittiming *bt)
>> +{
>> +	struct pucan_timing_slow *cmd = pcan_usb_fd_cmd_buffer(dev);
>> +
>> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
>> +						       PUCAN_CMD_TIMING_SLOW);
>> +	cmd->sjw_t = PUCAN_TSLOW_SJW_T(bt->sjw - 1,
>> +				dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES);
>> +
>> +	cmd->tseg2 = PUCAN_TSLOW_TSEG2(bt->phase_seg2 - 1);
>> +	cmd->tseg1 = PUCAN_TSLOW_TSEG1(bt->prop_seg + bt->phase_seg1 - 1);
>> +	cmd->brp = PUCAN_TSLOW_BRP(bt->brp - 1);
>> +
>> +	cmd->ewl = 96;	/* default */
>> +
>> +	/* send the command */
>> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
>> +}
>> +
>> +/* set CAN-FD bittiming for data */
>> +static int pcan_usb_fd_set_bittiming_fast(struct peak_usb_device *dev,
>> +					  struct can_bittiming *bt)
>> +{
>> +	struct pucan_timing_fast *cmd = pcan_usb_fd_cmd_buffer(dev);
>> +
>> +	cmd->opcode_channel = PUCAN_CMD_OPCODE_CHANNEL(dev->ctrl_idx,
>> +						       PUCAN_CMD_TIMING_FAST);
>> +	cmd->sjw = PUCAN_TFAST_SJW(bt->sjw - 1);
>> +	cmd->tseg2 = PUCAN_TFAST_TSEG2(bt->phase_seg2 - 1);
>> +	cmd->tseg1 = PUCAN_TFAST_TSEG1(bt->prop_seg + bt->phase_seg1 - 1);
>> +	cmd->brp = PUCAN_TFAST_BRP(bt->brp - 1);
>> +
>> +	/* send the command */
>> +	return pcan_usb_fd_send_cmd(dev, ++cmd);
>> +}
> Marc
>

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Marc Kleine-Budde]

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

On 12/02/2014 04:08 PM, Stephane Grosjean wrote:
>>>   drivers/net/can/usb/peak_usb/pcan_ucan.h   | 208 +++++++
>>>   drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 955
>>> +++++++++++++++++++++++++++++
>>>   drivers/net/can/usb/peak_usb/pcan_usb_fd.h | 108 ++++
>> Please don't create a header file, if it's only included once. Please
>> merge into the correct .c file.
> 
> Ok. So I merge both header files into the .c file. There will be only a
> single (and big) pcan_usb_fd.c file.

Yes, we only want code in header files that's used in more than one .c file.

>> Please have a look the _all_ multi-byte values and check for endianess.
>>
>> Hint, you might want to fix the existing driver first:
>>
>> pcan_usb_core.c:863:60: warning: restricted __le16 degrades to integer
>>
>> pcan_usb.c:322:34: warning: cast to restricted __le32
>>
>> pcan_usb.c:357:20: warning: cast to restricted __le16
>>
>> pcan_usb.c:382:28: warning: cast to restricted __le16
>>
>> pcan_usb.c:625:30: warning: cast to restricted __le32
>>
>> pcan_usb.c:635:30: warning: cast to restricted __le16
>>
>> pcan_usb_pro.c:158:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:158:28:    expected unsigned int [unsigned] [usertype]
>> <noident>
>> pcan_usb_pro.c:158:28:    got restricted __le32 [usertype] <noident>
>>
>> pcan_usb_pro.c:168:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:168:28:    expected unsigned int [unsigned] [usertype]
>> <noident>
>> pcan_usb_pro.c:168:28:    got restricted __le32 [usertype] <noident>
>>
>> pcan_usb_pro.c:176:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:176:28:    expected unsigned short [unsigned] [short]
>> [usertype] <noident>
>> pcan_usb_pro.c:176:28:    got restricted __le16 [usertype] <noident>
>>
>> pcan_usb_pro.c:182:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:182:28:    expected unsigned short [unsigned] [short]
>> [usertype] <noident>
>> pcan_usb_pro.c:182:28:    got restricted __le16 [usertype] <noident>
>>
>> pcan_usb_pro.c:184:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:184:28:    expected unsigned int [unsigned] [usertype]
>> <noident>
>> pcan_usb_pro.c:184:28:    got restricted __le32 [usertype] <noident>
>>
>> pcan_usb_pro.c:190:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:190:28:    expected unsigned short [unsigned] [short]
>> [usertype] <noident>
>> pcan_usb_pro.c:190:28:    got restricted __le16 [usertype] <noident>
>>
>> pcan_usb_pro.c:203:32: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:203:32:    expected unsigned int [unsigned] [usertype]
>> <noident>
>> pcan_usb_pro.c:203:32:    got restricted __le32 [usertype] <noident>
>>
>> pcan_usb_pro.c:286:27: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:458:30: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:550:29: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:561:47: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:575:32: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:605:35: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:606:35: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:678:47: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:696:37: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:720:19: warning: cast to restricted __le16
>>
>>
>> Compile your kernel with "make C=1 CF=-D__CHECK_ENDIAN__", you neet to
>> have the tool "sparse" install. On debian/ubuntu/... it's: "sudo
>> aptitude install sparse"
> 
> Ok, I'll do it too. And I suppose I'll push these changes in a separate
> patch too...

Yes, as this is a fix this should be done before doing any improvements.

> 
>>>   3 files changed, 1271 insertions(+)
>>>   create mode 100644 drivers/net/can/usb/peak_usb/pcan_ucan.h
>>>   create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>>>   create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.h
>>>
>>> diff --git a/drivers/net/can/usb/peak_usb/pcan_ucan.h
>>> b/drivers/net/can/usb/peak_usb/pcan_ucan.h
>>> new file mode 100644
>>> index 0000000..2223c05
>>> --- /dev/null
>>> +++ b/drivers/net/can/usb/peak_usb/pcan_ucan.h
[...]

>>> +#define PUCAN_CMD_OPCODE_CHANNEL(c, o)    cpu_to_le16(((c) << 12) |
>>> ((o) & 0x3ff))
>> I'm not sure, but I think I don't like hiding of the cpu_to_le16.
>> Can you make this a static inline function?
> 
> Ok. So I suppose I have to do the same (inline function making) for all
> the other occurrences of "cpu_to_le16()"  in this file, right?

Yes, the function would have a return value of __le16 then.

[...]

>>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>>> b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>>> new file mode 100644
>>> index 0000000..3d392a8
>>> --- /dev/null
>>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c

[...]

>>> +/* send PCAN-USB Pro FD commands synchronously */
>>> +static int pcan_usb_fd_send_cmd(struct peak_usb_device *dev, void
>>> *cmd_tail)
>>> +{
>>> +    void *cmd_head = pcan_usb_fd_cmd_buffer(dev);
>>> +    int actual_length;
>>> +    int err, cmd_len;
>> cmd_len is a long (or ptrdiff_t), as it's the diff of two pointers.
> 
> Ok. So it will be a "ptrdiff_t" .
> 
>>
>>> +    u8 *packet_ptr;
>>> +    int p, n = 1, packet_len;
>>> +
>>> +    /* usb device unregistered? */
>>> +    if (!(dev->state & PCAN_USB_STATE_CONNECTED))
>>> +        return 0;
>>> +
>>> +    /*
>>> +     * if a packet is not filled completely by commands, the command
>>> list
>>> +     * is terminated with an "end of collection" record.
>>> +     */
>>> +    cmd_len = cmd_tail - cmd_head;
>>> +    if (cmd_len < PCAN_UFD_CMD_BUFFER_SIZE) {
>> What happens if cmd_len turns out to be 511?
> 
> Well, it can't: "commands" are 64-bits records. The "end of record"
> "command" must be added when the list of commands doesn't fill the
> entire buffer content.
> Maybe it would be better to write:
> 
>    if (cmdlen <= (PCAN_UFD_CMD_BUFFER_SIZE - sizeof(u64)) {
> 
> 
> instead. What's your opinion about that?

Yes, better.

> 
>>
>>> +        memset(cmd_tail, 0xff, sizeof(u64));
>>> +        cmd_len += sizeof(u64);
>>> +    }
>>> +
>>> +    packet_ptr = cmd_head;
>>> +
>>> +    /* firmware is not able to re-assemble 512 bytes buffer in
>>> full-speed */
>>> +    if ((dev->udev->speed != USB_SPEED_HIGH) &&
>>> +        (cmd_len > PCAN_UFD_LOSPD_PKT_SIZE)) {
>>> +        packet_len = PCAN_UFD_LOSPD_PKT_SIZE;
>>> +        n += cmd_len / packet_len;
>>> +    } else {
>>> +        packet_len = cmd_len;
>>> +    }
>>> +
>>> +    for (p = 1; p <= n; p++) {
>> why not the usual, start at 0? (p = 0, p < n, p++), or even "i" :)
> 
> Well ok no problemo...
> 
>>
>>> +        err = usb_bulk_msg(dev->udev,
>>> +                   usb_sndbulkpipe(dev->udev,
>>> +                           PCAN_USBPRO_EP_CMDOUT),
>>> +                   packet_ptr, packet_len,
>>> +                   &actual_length, PCAN_UFD_COMMAND_TIMEOUT);
>> Do you have to take care about actual_length? If not, you can pass NULL
>> here AFAICS.
> 
> Ok, you're right. This parameter can be NULL. Will be changed.
> 
>>
>>> +        if (err) {
>>> +            netdev_err(dev->netdev,
>>> +                   "sending command failure: %d\n", err);
>>> +            break;
>>> +        }
>>> +
>>> +        packet_ptr += packet_len;
>>> +    }
>>> +
>>> +    return err;
>>> +}
>>> +
>>> +static int pcan_usb_fd_set_bus(struct peak_usb_device *dev, u8 onoff)
>>                                                                 bool on
> 
> Humm... ok, but be advised that this change will have some side-effects
> on the other existing files (pcan_usb_core.h, pcan_usb.c and
> pcan_usb_pro.c should be modified accordingly).

Okay, I haven't checked this. Keep it an u8 then.

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: 819 bytes --]

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Stephane Grosjean]

Hi Marc,

Le 02/12/2014 16:17, Marc Kleine-Budde a écrit :
> On 12/02/2014 04:08 PM, Stephane Grosjean wrote:
>>>>    drivers/net/can/usb/peak_usb/pcan_ucan.h   | 208 +++++++
>>>>    drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 955
>>>> +++++++++++++++++++++++++++++
>>>>    drivers/net/can/usb/peak_usb/pcan_usb_fd.h | 108 ++++
>>> Please don't create a header file, if it's only included once. Please
>>> merge into the correct .c file.
>> Ok. So I merge both header files into the .c file. There will be only a
>> single (and big) pcan_usb_fd.c file.
> Yes, we only want code in header files that's used in more than one .c file.

Is this rule quite recent? I mean, the current version of peak_usb 
contains "pcan_usb_pro.h" which is included only by "pcan_usb_pro.c"?

Moreover, "pcan_ucan.h" defines some data structures that might be used 
by some other new "non USB" PEAK -FD devices (peak_pci, for example).
So, how to manage to avoid to have to define several times the same data 
structures and constants, please?

Stéphane
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Marc Kleine-Budde]

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

On 12/03/2014 10:38 AM, Stephane Grosjean wrote:
> Le 02/12/2014 16:17, Marc Kleine-Budde a écrit :
>> On 12/02/2014 04:08 PM, Stephane Grosjean wrote:
>>>>>    drivers/net/can/usb/peak_usb/pcan_ucan.h   | 208 +++++++
>>>>>    drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 955
>>>>> +++++++++++++++++++++++++++++
>>>>>    drivers/net/can/usb/peak_usb/pcan_usb_fd.h | 108 ++++
>>>> Please don't create a header file, if it's only included once. Please
>>>> merge into the correct .c file.
>>> Ok. So I merge both header files into the .c file. There will be only a
>>> single (and big) pcan_usb_fd.c file.
>> Yes, we only want code in header files that's used in more than one .c
>> file.
> 
> Is this rule quite recent? I mean, the current version of peak_usb
> contains "pcan_usb_pro.h" which is included only by "pcan_usb_pro.c"?
> 
> Moreover, "pcan_ucan.h" defines some data structures that might be used
> by some other new "non USB" PEAK -FD devices (peak_pci, for example).
> So, how to manage to avoid to have to define several times the same data
> structures and constants, please?

If something is used by more than one .c file, put it in a header. If
it's only used in one .c file, put it into that .c file.

Follow this rule when adding this driver to the kernel. When you add a
new driver in the future and now some data structures/definitions/etc...
are shared between more then one driver or .c file, simply move that
code into a header and include the header in all .c file where you need it.

Hope this clarifies it,
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: 819 bytes --]

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Oliver Hartkopp]

Hi Stephane,

when there are defines that are definitely only relevant for one C file these 
defines should go into this specific C file.

But defines that are (even potentially) relevant for more users should go into 
a .h file, e.g. the ucan defines - or when you have common USB functions that 
are used by PCAN USB and PCAN USB pro.

But definitions that are *only* PCAN USB pro specific should go into 
pcan_usb_pro.c then.

Regards,
Oliver


On 03.12.2014 10:38, Stephane Grosjean wrote:
> Hi Marc,
>
> Le 02/12/2014 16:17, Marc Kleine-Budde a écrit :
>> On 12/02/2014 04:08 PM, Stephane Grosjean wrote:
>>>>>    drivers/net/can/usb/peak_usb/pcan_ucan.h   | 208 +++++++
>>>>>    drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 955
>>>>> +++++++++++++++++++++++++++++
>>>>>    drivers/net/can/usb/peak_usb/pcan_usb_fd.h | 108 ++++
>>>> Please don't create a header file, if it's only included once. Please
>>>> merge into the correct .c file.
>>> Ok. So I merge both header files into the .c file. There will be only a
>>> single (and big) pcan_usb_fd.c file.
>> Yes, we only want code in header files that's used in more than one .c file.
>
> Is this rule quite recent? I mean, the current version of peak_usb contains
> "pcan_usb_pro.h" which is included only by "pcan_usb_pro.c"?
>
> Moreover, "pcan_ucan.h" defines some data structures that might be used by
> some other new "non USB" PEAK -FD devices (peak_pci, for example).
> So, how to manage to avoid to have to define several times the same data
> structures and constants, please?
>
> Stéphane
> --
> PEAK-System Technik GmbH
> Sitz der Gesellschaft Darmstadt
> Handelsregister Darmstadt HRB 9183 Geschaeftsfuehrung: Alexander Gach, Uwe
> Wilhelm
> --

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Stephane Grosjean]

Hi Oliver,

Le 03/12/2014 11:16, Oliver Hartkopp a écrit :
> Hi Stephane,
>
> when there are defines that are definitely only relevant for one C 
> file these defines should go into this specific C file.
>
> But defines that are (even potentially) relevant for more users should 
> go into a .h file, e.g. the ucan defines - or when you have common USB 
> functions that are used by PCAN USB and PCAN USB pro.
>
> But definitions that are *only* PCAN USB pro specific should go into 
> pcan_usb_pro.c then.

Well I know that. My remark was about two points:

- the *kernel 3.4* version of pcan_usb_pro.h defines some data struct 
and constants that are *only* used by pcan_usb_pro.c (see "struct 
pcan_usb_pro_fwinfo", for example). Since it has been acked in the 
mainline in these early times, I simply asked if and when this rule did 
evolve? Underlying question: what to do now? Should I (also) post 
patches to setup things right between pcan_usb_pro.c and pcan_usb_pro.h, 
in order to define (for ex) "struct pcan_usb_pro_fwinfo" in 
pcan_usb_pro.c instead? Or do we let things "as is"?

- pcan_ucan.h defines some data structs and constants that will be 
common across different kinds of CAN-FD hardwares. How should I include 
this "pcan_ucan.h" file from a future (for example) PCI CAN-FD driver 
(which, obvioulsy) won't be stored  under "usb/peak_usb" directory).

Regards,

Stéphane
>
> Regards,
> Oliver
>
>
> On 03.12.2014 10:38, Stephane Grosjean wrote:
>> Hi Marc,
>>
>> Le 02/12/2014 16:17, Marc Kleine-Budde a écrit :
>>> On 12/02/2014 04:08 PM, Stephane Grosjean wrote:
>>>>>> drivers/net/can/usb/peak_usb/pcan_ucan.h   | 208 +++++++
>>>>>>    drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 955
>>>>>> +++++++++++++++++++++++++++++
>>>>>>    drivers/net/can/usb/peak_usb/pcan_usb_fd.h | 108 ++++
>>>>> Please don't create a header file, if it's only included once. Please
>>>>> merge into the correct .c file.
>>>> Ok. So I merge both header files into the .c file. There will be 
>>>> only a
>>>> single (and big) pcan_usb_fd.c file.
>>> Yes, we only want code in header files that's used in more than one 
>>> .c file.
>>
>> Is this rule quite recent? I mean, the current version of peak_usb 
>> contains
>> "pcan_usb_pro.h" which is included only by "pcan_usb_pro.c"?
>>
>> Moreover, "pcan_ucan.h" defines some data structures that might be 
>> used by
>> some other new "non USB" PEAK -FD devices (peak_pci, for example).
>> So, how to manage to avoid to have to define several times the same data
>> structures and constants, please?
>>
>> Stéphane
>> -- 
>> PEAK-System Technik GmbH
>> Sitz der Gesellschaft Darmstadt
>> Handelsregister Darmstadt HRB 9183 Geschaeftsfuehrung: Alexander 
>> Gach, Uwe
>> Wilhelm
>> -- 

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Marc Kleine-Budde]

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

On 12/03/2014 11:38 AM, Stephane Grosjean wrote:
> Well I know that. My remark was about two points:
> 
> - the *kernel 3.4* version of pcan_usb_pro.h defines some data struct
> and constants that are *only* used by pcan_usb_pro.c (see "struct
> pcan_usb_pro_fwinfo", for example). Since it has been acked in the
> mainline in these early times, I simply asked if and when this rule did
> evolve? Underlying question: what to do now? Should I (also) post
> patches to setup things right between pcan_usb_pro.c and pcan_usb_pro.h,
> in order to define (for ex) "struct pcan_usb_pro_fwinfo" in
> pcan_usb_pro.c instead? Or do we let things "as is"?

No need to clean up the .h files already in the kernel. But please stick
to the rule when adding new files.

> - pcan_ucan.h defines some data structs and constants that will be
> common across different kinds of CAN-FD hardwares. How should I include
> this "pcan_ucan.h" file from a future (for example) PCI CAN-FD driver
> (which, obvioulsy) won't be stored  under "usb/peak_usb" directory).

If it's CAN-FD relevant than it will go under include/linux/can/ somewhere.

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: 819 bytes --]

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Oliver Hartkopp]

On 03.12.2014 11:45, Marc Kleine-Budde wrote:
> On 12/03/2014 11:38 AM, Stephane Grosjean wrote:


>> - pcan_ucan.h defines some data structs and constants that will be
>> common across different kinds of CAN-FD hardwares. How should I include
>> this "pcan_ucan.h" file from a future (for example) PCI CAN-FD driver
>> (which, obvioulsy) won't be stored  under "usb/peak_usb" directory).
>
> If it's CAN-FD relevant than it will go under include/linux/can/ somewhere.

Hm - AFAICS it's not a CAN FD specific thing but these definitions belog to 
the ucan controller IP - similar to a M_CAN or C_CAN controller.

So maybe it makes sense to create a

	linux/drivers/net/can/ucan

directory where the (new/current?) peak_pci cards with ucan might go in too.

The pcan_ucan.h is something like sja1000.h

Regards,
Oliver

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Stephane Grosjean]

Le 03/12/2014 11:50, Oliver Hartkopp a écrit :
> On 03.12.2014 11:45, Marc Kleine-Budde wrote:
>> On 12/03/2014 11:38 AM, Stephane Grosjean wrote:
>
>
>>> - pcan_ucan.h defines some data structs and constants that will be
>>> common across different kinds of CAN-FD hardwares. How should I include
>>> this "pcan_ucan.h" file from a future (for example) PCI CAN-FD driver
>>> (which, obvioulsy) won't be stored  under "usb/peak_usb" directory).
>>
>> If it's CAN-FD relevant than it will go under include/linux/can/ 
>> somewhere.
>
> Hm - AFAICS it's not a CAN FD specific thing but these definitions 
> belog to the ucan controller IP - similar to a M_CAN or C_CAN controller.
>
> So maybe it makes sense to create a
>
>     linux/drivers/net/can/ucan
>
> directory where the (new/current?) peak_pci cards with ucan might go 
> in too.
>
> The pcan_ucan.h is something like sja1000.h
>

Yes it is! But what about the new USB devices I'm pushing now? Is it 
possible to #include "pcan_ucan.h" from "usb/peak_usb" if it is stored 
into "ucan" ??? AFAIT it isn't without doing dirty things... but I'd 
like to be wrong...

> Regards,
> Oliver

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Oliver Hartkopp]

On 03.12.2014 12:20, Stephane Grosjean wrote:

>> So maybe it makes sense to create a
>>
>>     linux/drivers/net/can/ucan
>>
>> directory where the (new/current?) peak_pci cards with ucan might go in too.
>>
>> The pcan_ucan.h is something like sja1000.h
>>
>
> Yes it is! But what about the new USB devices I'm pushing now? Is it possible
> to #include "pcan_ucan.h" from "usb/peak_usb" if it is stored into "ucan" ???
> AFAIT it isn't without doing dirty things... but I'd like to be wrong...

Yes. That would look ugly.

I wonder if it makes send to leave the drivers in

	drivers/net/can/sja1000
and

	drivers/net/can/usb/peak_usb

as-is and put the new ucan based adapters (usb/pci/ ...) into

	drivers/net/can/ucan

???

Do you assume this to create a code duplication problem for the USB drivers?

@Marc: Any suggestion from you?

Regards,
Oliver

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Stephane Grosjean]

Le 03/12/2014 12:51, Oliver Hartkopp a écrit :
> On 03.12.2014 12:20, Stephane Grosjean wrote:
>
>>> So maybe it makes sense to create a
>>>
>>>     linux/drivers/net/can/ucan
>>>
>>> directory where the (new/current?) peak_pci cards with ucan might go 
>>> in too.
>>>
>>> The pcan_ucan.h is something like sja1000.h
>>>
>>
>> Yes it is! But what about the new USB devices I'm pushing now? Is it 
>> possible
>> to #include "pcan_ucan.h" from "usb/peak_usb" if it is stored into 
>> "ucan" ???
>> AFAIT it isn't without doing dirty things... but I'd like to be wrong...
>
> Yes. That would look ugly.
>
> I wonder if it makes send to leave the drivers in
>
>     drivers/net/can/sja1000
> and
>
>     drivers/net/can/usb/peak_usb
>
> as-is and put the new ucan based adapters (usb/pci/ ...) into
>
>     drivers/net/can/ucan
>
> ???
>
> Do you assume this to create a code duplication problem for the USB 
> drivers?

Arf I'm afraid it would... I managed to create a "peak usb core" to 
minimize code duplication, with device specific files around, so that it 
would be easier to add any new usb devices. In fact it was. Until "ucan" 
came into the story...

>
> @Marc: Any suggestion from you?
>
> Regards,
> Oliver

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Oliver Hartkopp]

Then I would suggest to stay in the layout of the current patch set (with 
pcan_ucan.h in drivers/usb/peak_usb) and discuss about a generic approach when 
some other ucan-based devices (e.g. pci) appear ...

?!?

Regards,
Oliver

On 03.12.2014 13:34, Stephane Grosjean wrote:
>
> Le 03/12/2014 12:51, Oliver Hartkopp a écrit :
>> On 03.12.2014 12:20, Stephane Grosjean wrote:
>>
>>>> So maybe it makes sense to create a
>>>>
>>>>     linux/drivers/net/can/ucan
>>>>
>>>> directory where the (new/current?) peak_pci cards with ucan might go in too.
>>>>
>>>> The pcan_ucan.h is something like sja1000.h
>>>>
>>>
>>> Yes it is! But what about the new USB devices I'm pushing now? Is it possible
>>> to #include "pcan_ucan.h" from "usb/peak_usb" if it is stored into "ucan" ???
>>> AFAIT it isn't without doing dirty things... but I'd like to be wrong...
>>
>> Yes. That would look ugly.
>>
>> I wonder if it makes send to leave the drivers in
>>
>>     drivers/net/can/sja1000
>> and
>>
>>     drivers/net/can/usb/peak_usb
>>
>> as-is and put the new ucan based adapters (usb/pci/ ...) into
>>
>>     drivers/net/can/ucan
>>
>> ???
>>
>> Do you assume this to create a code duplication problem for the USB drivers?
>
> Arf I'm afraid it would... I managed to create a "peak usb core" to minimize
> code duplication, with device specific files around, so that it would be
> easier to add any new usb devices. In fact it was. Until "ucan" came into the
> story...
>
>>
>> @Marc: Any suggestion from you?
>>
>> Regards,
>> Oliver
>
> --
> PEAK-System Technik GmbH
> Sitz der Gesellschaft Darmstadt
> Handelsregister Darmstadt HRB 9183 Geschaeftsfuehrung: Alexander Gach, Uwe
> Wilhelm
> --

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Stephane Grosjean]

Le 03/12/2014 13:57, Oliver Hartkopp a écrit :
> Then I would suggest to stay in the layout of the current patch set 
> (with pcan_ucan.h in drivers/usb/peak_usb) and discuss about a generic 
> approach when some other ucan-based devices (e.g. pci) appear ...
>
> ?!?
>

Ok, I agree with you. And I move the content of "pcan_usb_fd.h" into 
"pcan_usb_fd.c".

Regards,

Stéphane

> Regards,
> Oliver
>
> On 03.12.2014 13:34, Stephane Grosjean wrote:
>>
>> Le 03/12/2014 12:51, Oliver Hartkopp a écrit :
>>> On 03.12.2014 12:20, Stephane Grosjean wrote:
>>>
>>>>> So maybe it makes sense to create a
>>>>>
>>>>>     linux/drivers/net/can/ucan
>>>>>
>>>>> directory where the (new/current?) peak_pci cards with ucan might 
>>>>> go in too.
>>>>>
>>>>> The pcan_ucan.h is something like sja1000.h
>>>>>
>>>>
>>>> Yes it is! But what about the new USB devices I'm pushing now? Is 
>>>> it possible
>>>> to #include "pcan_ucan.h" from "usb/peak_usb" if it is stored into 
>>>> "ucan" ???
>>>> AFAIT it isn't without doing dirty things... but I'd like to be 
>>>> wrong...
>>>
>>> Yes. That would look ugly.
>>>
>>> I wonder if it makes send to leave the drivers in
>>>
>>>     drivers/net/can/sja1000
>>> and
>>>
>>>     drivers/net/can/usb/peak_usb
>>>
>>> as-is and put the new ucan based adapters (usb/pci/ ...) into
>>>
>>>     drivers/net/can/ucan
>>>
>>> ???
>>>
>>> Do you assume this to create a code duplication problem for the USB 
>>> drivers?
>>
>> Arf I'm afraid it would... I managed to create a "peak usb core" to 
>> minimize
>> code duplication, with device specific files around, so that it would be
>> easier to add any new usb devices. In fact it was. Until "ucan" came 
>> into the
>> story...
>>
>>>
>>> @Marc: Any suggestion from you?
>>>
>>> Regards,
>>> Oliver
>>
>> -- 
>> PEAK-System Technik GmbH
>> Sitz der Gesellschaft Darmstadt
>> Handelsregister Darmstadt HRB 9183 Geschaeftsfuehrung: Alexander 
>> Gach, Uwe
>> Wilhelm
>> -- 

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--

Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Marc Kleine-Budde]

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

On 12/03/2014 01:57 PM, Oliver Hartkopp wrote:
> Then I would suggest to stay in the layout of the current patch set
> (with pcan_ucan.h in drivers/usb/peak_usb) and discuss about a generic
> approach when some other ucan-based devices (e.g. pci) appear ...

+1
Sounds good. Keep it simple first.

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: 819 bytes --]

[PATCH 3/3] can/peak_usb: CAN-FD: driver's core modifications

[Stephane Grosjean]

This patch modifies the core of the PEAK-System CAN USB adapters driver to
include the support the new CAN-FD USB adapters.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/Kconfig                  | 14 ++++-
 drivers/net/can/usb/peak_usb/Makefile        |  2 +-
 drivers/net/can/usb/peak_usb/pcan_usb_core.c | 84 ++++++++++++++++++++++------
 drivers/net/can/usb/peak_usb/pcan_usb_core.h | 14 ++++-
 4 files changed, 90 insertions(+), 24 deletions(-)

diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
index a77db919..6dbbc85 100644
--- a/drivers/net/can/usb/Kconfig
+++ b/drivers/net/can/usb/Kconfig
@@ -53,10 +53,18 @@ config CAN_KVASER_USB
 	  module will be called kvaser_usb.
 
 config CAN_PEAK_USB
-	tristate "PEAK PCAN-USB/USB Pro interfaces"
+	tristate "PEAK PCAN-USB/USB Pro interfaces for CAN 2.0b/CAN-FD"
 	---help---
-	  This driver supports the PCAN-USB and PCAN-USB Pro adapters
-	  from PEAK-System Technik (http://www.peak-system.com).
+	  This driver supports the PEAK-System Technik USB adapters that enable
+	  access to the CAN bus, with repect to the CAN 2.0b and/or CAN-FD
+	  standards, that is:
+
+	  PCAN-USB             single CAN 2.0b channel USB adapter
+	  PCAN-USB Pro         dual CAN 2.0b channels USB adapter
+	  PCAN-USB FD          single CAN-FD channel USB adapter
+	  PCAN-USB Pro FD      dual CAN-FD channels USB adapter
+
+	  (see also http://www.peak-system.com).
 
 config CAN_8DEV_USB
 	tristate "8 devices USB2CAN interface"
diff --git a/drivers/net/can/usb/peak_usb/Makefile b/drivers/net/can/usb/peak_usb/Makefile
index 1aefbc8..1839e9c 100644
--- a/drivers/net/can/usb/peak_usb/Makefile
+++ b/drivers/net/can/usb/peak_usb/Makefile
@@ -1,2 +1,2 @@
 obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
-peak_usb-y = pcan_usb_core.o pcan_usb.o pcan_usb_pro.o
+peak_usb-y = pcan_usb_core.o pcan_usb.o pcan_usb_pro.o pcan_usb_fd.o
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index 644e6ab..2f034c1 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -37,6 +37,8 @@ MODULE_LICENSE("GPL v2");
 static struct usb_device_id peak_usb_table[] = {
 	{USB_DEVICE(PCAN_USB_VENDOR_ID, PCAN_USB_PRODUCT_ID)},
 	{USB_DEVICE(PCAN_USB_VENDOR_ID, PCAN_USBPRO_PRODUCT_ID)},
+	{USB_DEVICE(PCAN_USB_VENDOR_ID, PCAN_USBPROFD_PRODUCT_ID)},
+	{USB_DEVICE(PCAN_USB_VENDOR_ID, PCAN_USBFD_PRODUCT_ID)},
 	{} /* Terminating entry */
 };
 
@@ -46,6 +48,8 @@ MODULE_DEVICE_TABLE(usb, peak_usb_table);
 static struct peak_usb_adapter *peak_usb_adapters_list[] = {
 	&pcan_usb,
 	&pcan_usb_pro,
+	&pcan_usb_pro_fd,
+	&pcan_usb_fd,
 	NULL,
 };
 
@@ -89,7 +93,7 @@ static void peak_usb_add_us(struct timeval *tv, u32 delta_us)
 }
 
 /*
- * sometimes, another now may be  more recent than current one...
+ * sometimes, another "now" may be more recent than current one...
  */
 void peak_usb_update_ts_now(struct peak_time_ref *time_ref, u32 ts_now)
 {
@@ -107,7 +111,7 @@ void peak_usb_update_ts_now(struct peak_time_ref *time_ref, u32 ts_now)
 }
 
 /*
- * register device timestamp as now
+ * register device timestamp as "now"
  */
 void peak_usb_set_ts_now(struct peak_time_ref *time_ref, u32 ts_now)
 {
@@ -165,6 +169,21 @@ void peak_usb_get_ts_tv(struct peak_time_ref *time_ref, u32 ts,
 }
 
 /*
+ * post received skb after having set any hw timestamp
+ */
+int peak_usb_netif_rx(struct sk_buff *skb,
+		      struct peak_time_ref *time_ref, u32 ts_low, u32 ts_high)
+{
+	struct skb_shared_hwtstamps *hwts = skb_hwtstamps(skb);
+	struct timeval tv;
+
+	peak_usb_get_ts_tv(time_ref, ts_low, &tv);
+	hwts->hwtstamp = timeval_to_ktime(tv);
+
+	return netif_rx(skb);
+}
+
+/*
  * callback for bulk Rx urb
  */
 static void peak_usb_read_bulk_callback(struct urb *urb)
@@ -253,7 +272,7 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
 	case 0:
 		/* transmission complete */
 		netdev->stats.tx_packets++;
-		netdev->stats.tx_bytes += context->dlc;
+		netdev->stats.tx_bytes += context->data_len;
 
 		/* prevent tx timeout */
 		netdev->trans_start = jiffies;
@@ -322,7 +341,9 @@ static netdev_tx_t peak_usb_ndo_start_xmit(struct sk_buff *skb,
 	}
 
 	context->echo_index = i;
-	context->dlc = cf->can_dlc;
+
+	/* Note: this works with CANFD frames too */
+	context->data_len = cf->can_dlc;
 
 	usb_anchor_urb(urb, &dev->tx_submitted);
 
@@ -679,19 +700,43 @@ static int peak_usb_set_mode(struct net_device *netdev, enum can_mode mode)
 }
 
 /*
- * candev callback used to set device bitrate.
+ * candev callback used to set device nominal bitrate.
  */
 static int peak_usb_set_bittiming(struct net_device *netdev)
 {
 	struct peak_usb_device *dev = netdev_priv(netdev);
-	struct can_bittiming *bt = &dev->can.bittiming;
+	struct peak_usb_adapter *pa = dev->adapter;
 
-	if (dev->adapter->dev_set_bittiming) {
-		int err = dev->adapter->dev_set_bittiming(dev, bt);
+	if (pa->dev_set_bittiming) {
+		struct can_bittiming *bt = &dev->can.bittiming;
+		int err = pa->dev_set_bittiming(dev, bt);
 
 		if (err)
 			netdev_info(netdev, "couldn't set bitrate (err %d)\n",
-				err);
+				    err);
+		return err;
+	}
+
+	return 0;
+}
+
+/*
+ * candev callback used to set device data bitrate.
+ */
+static int peak_usb_set_data_bittiming(struct net_device *netdev)
+{
+	struct peak_usb_device *dev = netdev_priv(netdev);
+	struct peak_usb_adapter *pa = dev->adapter;
+
+	if (pa->dev_set_data_bittiming) {
+		struct can_bittiming *bt = &dev->can.data_bittiming;
+		int err = pa->dev_set_data_bittiming(dev, bt);
+
+		if (err)
+			netdev_info(netdev,
+				    "couldn't set data bitrate (err %d)\n",
+				    err);
+
 		return err;
 	}
 
@@ -735,7 +780,7 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
 	dev->cmd_buf = kmalloc(PCAN_USB_MAX_CMD_LEN, GFP_KERNEL);
 	if (!dev->cmd_buf) {
 		err = -ENOMEM;
-		goto lbl_set_intf_data;
+		goto lbl_free_candev;
 	}
 
 	dev->udev = usb_dev;
@@ -750,9 +795,10 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
 	dev->can.clock = peak_usb_adapter->clock;
 	dev->can.bittiming_const = &peak_usb_adapter->bittiming_const;
 	dev->can.do_set_bittiming = peak_usb_set_bittiming;
+	dev->can.data_bittiming_const = &peak_usb_adapter->data_bittiming_const;
+	dev->can.do_set_data_bittiming = peak_usb_set_data_bittiming;
 	dev->can.do_set_mode = peak_usb_set_mode;
-	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
-				      CAN_CTRLMODE_LISTENONLY;
+	dev->can.ctrlmode_supported = peak_usb_adapter->ctrlmode_supported;
 
 	netdev->netdev_ops = &peak_usb_netdev_ops;
 
@@ -770,12 +816,11 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
 	usb_set_intfdata(intf, dev);
 
 	SET_NETDEV_DEV(netdev, &intf->dev);
-	netdev->dev_id = ctrl_idx;
 
 	err = register_candev(netdev);
 	if (err) {
 		dev_err(&intf->dev, "couldn't register CAN device: %d\n", err);
-		goto lbl_free_cmd_buf;
+		goto lbl_set_intf_data;
 	}
 
 	if (dev->prev_siblings)
@@ -788,14 +833,14 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
 	if (dev->adapter->dev_init) {
 		err = dev->adapter->dev_init(dev);
 		if (err)
-			goto lbl_free_cmd_buf;
+			goto lbl_unregister;
 	}
 
 	/* set bus off */
 	if (dev->adapter->dev_set_bus) {
 		err = dev->adapter->dev_set_bus(dev, 0);
 		if (err)
-			goto lbl_free_cmd_buf;
+			goto lbl_unregister;
 	}
 
 	/* get device number early */
@@ -807,11 +852,14 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
 
 	return 0;
 
-lbl_free_cmd_buf:
-	kfree(dev->cmd_buf);
+lbl_unregister:
+	unregister_netdev(netdev);
 
 lbl_set_intf_data:
 	usb_set_intfdata(intf, dev->prev_siblings);
+	kfree(dev->cmd_buf);
+
+lbl_free_candev:
 	free_candev(netdev);
 
 	return err;
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
index 073b47f..13d44a5 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
@@ -25,6 +25,8 @@
 /* supported device ids. */
 #define PCAN_USB_PRODUCT_ID		0x000c
 #define PCAN_USBPRO_PRODUCT_ID		0x000d
+#define PCAN_USBPROFD_PRODUCT_ID	0x0011
+#define PCAN_USBFD_PRODUCT_ID		0x0012
 
 #define PCAN_USB_DRIVER_NAME		"peak_usb"
 
@@ -44,8 +46,10 @@ struct peak_usb_device;
 struct peak_usb_adapter {
 	char *name;
 	u32 device_id;
+	u32 ctrlmode_supported;
 	struct can_clock clock;
 	const struct can_bittiming_const bittiming_const;
+	const struct can_bittiming_const data_bittiming_const;
 	unsigned int ctrl_count;
 
 	int (*intf_probe)(struct usb_interface *intf);
@@ -57,6 +61,8 @@ struct peak_usb_adapter {
 	int (*dev_close)(struct peak_usb_device *dev);
 	int (*dev_set_bittiming)(struct peak_usb_device *dev,
 					struct can_bittiming *bt);
+	int (*dev_set_data_bittiming)(struct peak_usb_device *dev,
+				      struct can_bittiming *bt);
 	int (*dev_set_bus)(struct peak_usb_device *dev, u8 onoff);
 	int (*dev_get_device_id)(struct peak_usb_device *dev, u32 *device_id);
 	int (*dev_decode_buf)(struct peak_usb_device *dev, struct urb *urb);
@@ -80,6 +86,8 @@ struct peak_usb_adapter {
 
 extern struct peak_usb_adapter pcan_usb;
 extern struct peak_usb_adapter pcan_usb_pro;
+extern struct peak_usb_adapter pcan_usb_pro_fd;
+extern struct peak_usb_adapter pcan_usb_fd;
 
 struct peak_time_ref {
 	struct timeval tv_host_0, tv_host;
@@ -92,7 +100,7 @@ struct peak_time_ref {
 struct peak_tx_urb_context {
 	struct peak_usb_device *dev;
 	u32 echo_index;
-	u8 dlc;
+	u8 data_len;
 	struct urb *urb;
 };
 
@@ -139,7 +147,9 @@ void peak_usb_update_ts_now(struct peak_time_ref *time_ref, u32 ts_now);
 void peak_usb_set_ts_now(struct peak_time_ref *time_ref, u32 ts_now);
 void peak_usb_get_ts_tv(struct peak_time_ref *time_ref, u32 ts,
 			struct timeval *tv);
-
+int peak_usb_netif_rx(struct sk_buff *skb,
+		      struct peak_time_ref *time_ref, u32 ts_low, u32 ts_high);
 void peak_usb_async_complete(struct urb *urb);
 void peak_usb_restart_complete(struct peak_usb_device *dev);
+
 #endif
-- 
1.9.1

Re: [PATCH 3/3] can/peak_usb: CAN-FD: driver's core modifications

[Oliver Hartkopp]

Hello Stephane,

I have two more questions:

On 27.11.2014 11:32, Stephane Grosjean wrote:

> @@ -322,7 +341,9 @@ static netdev_tx_t peak_usb_ndo_start_xmit(struct sk_buff *skb,
>   	}
>
>   	context->echo_index = i;
> -	context->dlc = cf->can_dlc;
> +
> +	/* Note: this works with CANFD frames too */
> +	context->data_len = cf->can_dlc;
>
>   	usb_anchor_urb(urb, &dev->tx_submitted);
>

Even with this comment the assignment looks fishy.

What about defining

	struct canfd_frame *cfd = (struct can_frame *)skb->data;

instead of

	struct can_frame *cf = (struct can_frame *)skb->data;

at the top of the function and assign

	context->data_len = cfd->len;

??


> @@ -679,19 +700,43 @@ static int peak_usb_set_mode(struct net_device *netdev, enum can_mode mode)
>   }
>
>   /*
> - * candev callback used to set device bitrate.
> + * candev callback used to set device nominal bitrate.

Is this the nominal bitrate or the arbitration bitrate ?

What about stating both:
candev callback used to set device nominal/arbitration bitrate.


> @@ -735,7 +780,7 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>   	dev->cmd_buf = kmalloc(PCAN_USB_MAX_CMD_LEN, GFP_KERNEL);
>   	if (!dev->cmd_buf) {
>   		err = -ENOMEM;
> -		goto lbl_set_intf_data;
> +		goto lbl_free_candev;

These label changes

>   	}
>
>   	dev->udev = usb_dev;
> @@ -750,9 +795,10 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>   	dev->can.clock = peak_usb_adapter->clock;
>   	dev->can.bittiming_const = &peak_usb_adapter->bittiming_const;
>   	dev->can.do_set_bittiming = peak_usb_set_bittiming;
> +	dev->can.data_bittiming_const = &peak_usb_adapter->data_bittiming_const;
> +	dev->can.do_set_data_bittiming = peak_usb_set_data_bittiming;
>   	dev->can.do_set_mode = peak_usb_set_mode;
> -	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
> -				      CAN_CTRLMODE_LISTENONLY;
> +	dev->can.ctrlmode_supported = peak_usb_adapter->ctrlmode_supported;
>
>   	netdev->netdev_ops = &peak_usb_netdev_ops;
>
> @@ -770,12 +816,11 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>   	usb_set_intfdata(intf, dev);
>
>   	SET_NETDEV_DEV(netdev, &intf->dev);
> -	netdev->dev_id = ctrl_idx;
>
>   	err = register_candev(netdev);
>   	if (err) {
>   		dev_err(&intf->dev, "couldn't register CAN device: %d\n", err);
> -		goto lbl_free_cmd_buf;
> +		goto lbl_set_intf_data;

here

>   	}
>
>   	if (dev->prev_siblings)
> @@ -788,14 +833,14 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>   	if (dev->adapter->dev_init) {
>   		err = dev->adapter->dev_init(dev);
>   		if (err)
> -			goto lbl_free_cmd_buf;
> +			goto lbl_unregister;

here

>   	}
>
>   	/* set bus off */
>   	if (dev->adapter->dev_set_bus) {
>   		err = dev->adapter->dev_set_bus(dev, 0);
>   		if (err)
> -			goto lbl_free_cmd_buf;
> +			goto lbl_unregister;

here

>   	}
>
>   	/* get device number early */
> @@ -807,11 +852,14 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>
>   	return 0;
>
> -lbl_free_cmd_buf:
> -	kfree(dev->cmd_buf);
> +lbl_unregister:
> +	unregister_netdev(netdev);
>
>   lbl_set_intf_data:
>   	usb_set_intfdata(intf, dev->prev_siblings);
> +	kfree(dev->cmd_buf);
> +
> +lbl_free_candev:
>   	free_candev(netdev);

and here ...

Are these changes necessary for the current PCAN USB driver too?
Should they go into the stable tree with a separate patch?

If so this separate patch should be the first in the series to be picked.

There's something like this in patch 1/3 too - will send it after this mail.

Regards,
Oliver

Re: [PATCH 3/3] can/peak_usb: CAN-FD: driver's core modifications

[Stephane Grosjean]

Hi Oliver,

Le 27/11/2014 12:43, Oliver Hartkopp a écrit :
> Hello Stephane,
>
> I have two more questions:
>
> On 27.11.2014 11:32, Stephane Grosjean wrote:
>
>> @@ -322,7 +341,9 @@ static netdev_tx_t peak_usb_ndo_start_xmit(struct 
>> sk_buff *skb,
>>       }
>>
>>       context->echo_index = i;
>> -    context->dlc = cf->can_dlc;
>> +
>> +    /* Note: this works with CANFD frames too */
>> +    context->data_len = cf->can_dlc;
>>
>>       usb_anchor_urb(urb, &dev->tx_submitted);
>>
>
> Even with this comment the assignment looks fishy.
>
> What about defining
>
>     struct canfd_frame *cfd = (struct can_frame *)skb->data;
>
> instead of
>
>     struct can_frame *cf = (struct can_frame *)skb->data;
>
> at the top of the function and assign
>
>     context->data_len = cfd->len;
>
> ??
>

Simply because I always think in terms of being compatible with (very) 
older versions of the Kernel, for example <= 3.5, which is obviously not 
the right (TM) way of thinking when pushing code into the mainline 
Kernel ;-) !
So I agree with you and will do the changes accordingly.

>
>> @@ -679,19 +700,43 @@ static int peak_usb_set_mode(struct net_device 
>> *netdev, enum can_mode mode)
>>   }
>>
>>   /*
>> - * candev callback used to set device bitrate.
>> + * candev callback used to set device nominal bitrate.
>
> Is this the nominal bitrate or the arbitration bitrate ?
>
> What about stating both:
> candev callback used to set device nominal/arbitration bitrate.

Okay!

>
>
>> @@ -735,7 +780,7 @@ static int peak_usb_create_dev(struct 
>> peak_usb_adapter *peak_usb_adapter,
>>       dev->cmd_buf = kmalloc(PCAN_USB_MAX_CMD_LEN, GFP_KERNEL);
>>       if (!dev->cmd_buf) {
>>           err = -ENOMEM;
>> -        goto lbl_set_intf_data;
>> +        goto lbl_free_candev;
>
> These label changes
>
>>       }
>>
>>       dev->udev = usb_dev;
>> @@ -750,9 +795,10 @@ static int peak_usb_create_dev(struct 
>> peak_usb_adapter *peak_usb_adapter,
>>       dev->can.clock = peak_usb_adapter->clock;
>>       dev->can.bittiming_const = &peak_usb_adapter->bittiming_const;
>>       dev->can.do_set_bittiming = peak_usb_set_bittiming;
>> +    dev->can.data_bittiming_const = 
>> &peak_usb_adapter->data_bittiming_const;
>> +    dev->can.do_set_data_bittiming = peak_usb_set_data_bittiming;
>>       dev->can.do_set_mode = peak_usb_set_mode;
>> -    dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
>> -                      CAN_CTRLMODE_LISTENONLY;
>> +    dev->can.ctrlmode_supported = peak_usb_adapter->ctrlmode_supported;
>>
>>       netdev->netdev_ops = &peak_usb_netdev_ops;
>>
>> @@ -770,12 +816,11 @@ static int peak_usb_create_dev(struct 
>> peak_usb_adapter *peak_usb_adapter,
>>       usb_set_intfdata(intf, dev);
>>
>>       SET_NETDEV_DEV(netdev, &intf->dev);
>> -    netdev->dev_id = ctrl_idx;
>>
>>       err = register_candev(netdev);
>>       if (err) {
>>           dev_err(&intf->dev, "couldn't register CAN device: %d\n", 
>> err);
>> -        goto lbl_free_cmd_buf;
>> +        goto lbl_set_intf_data;
>
> here
>
>>       }
>>
>>       if (dev->prev_siblings)
>> @@ -788,14 +833,14 @@ static int peak_usb_create_dev(struct 
>> peak_usb_adapter *peak_usb_adapter,
>>       if (dev->adapter->dev_init) {
>>           err = dev->adapter->dev_init(dev);
>>           if (err)
>> -            goto lbl_free_cmd_buf;
>> +            goto lbl_unregister;
>
> here
>
>>       }
>>
>>       /* set bus off */
>>       if (dev->adapter->dev_set_bus) {
>>           err = dev->adapter->dev_set_bus(dev, 0);
>>           if (err)
>> -            goto lbl_free_cmd_buf;
>> +            goto lbl_unregister;
>
> here
>
>>       }
>>
>>       /* get device number early */
>> @@ -807,11 +852,14 @@ static int peak_usb_create_dev(struct 
>> peak_usb_adapter *peak_usb_adapter,
>>
>>       return 0;
>>
>> -lbl_free_cmd_buf:
>> -    kfree(dev->cmd_buf);
>> +lbl_unregister:
>> +    unregister_netdev(netdev);
>>
>>   lbl_set_intf_data:
>>       usb_set_intfdata(intf, dev->prev_siblings);
>> +    kfree(dev->cmd_buf);
>> +
>> +lbl_free_candev:
>>       free_candev(netdev);
>
> and here ...
>
> Are these changes necessary for the current PCAN USB driver too?
> Should they go into the stable tree with a separate patch?
>
> If so this separate patch should be the first in the series to be picked.
>
> There's something like this in patch 1/3 too - will send it after this 
> mail.

Ok! I will prepare a single patch for these changes.

>
> Regards,
> Oliver

Regards,

Stéphane

> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--