From: Wolfgang Grandegger <wg@grandegger.com>
To: "s.grosjean@peak-system.com" <s.grosjean@peak-system.com>
Cc: "socketcan@hartkopp.net" <socketcan@hartkopp.net>,
"linux-can@vger.kernel.org" <linux-can@vger.kernel.org>
Subject: Re: "[PATCH] Add support for PEAK-System GmbH CAN USB adapters (v2) "
Date: Wed, 21 Dec 2011 10:33:42 +0100 [thread overview]
Message-ID: <4EF1A7F6.3030106@grandegger.com> (raw)
In-Reply-To: <301939.630738588-sendEmail@ubuntu-i386>
More comments on the coding style...
On 12/16/2011 11:19 AM, s.grosjean@peak-system.com wrote:
>>From 1cee3be3875f27a2ee3942b00d611450f4369325 Mon Sep 17 00:00:00 2001
> From: Stephane Grosjean <s.grosjean@peak-system.com>
> Date: Fri, 16 Dec 2011 11:11:37 +0100
> Subject: [PATCH] Add support for PEAK-System GmbH CAN USB adapters (v2)
>
> v2 includes:
> change dev_xxx() into netdev_xxx() macros
> add missing include linux/module.h
> pre-allocate urbs and buffers for tx path at _open() and free them at _close()
> rather than into _start_xmit()
> remove some unused code and (boring) #ifdef/#endif
> use "menuconfig" in Kconfig rather than "config" entry
> ---
> drivers/net/can/usb/Kconfig | 20 +
> drivers/net/can/usb/Makefile | 12 +
> drivers/net/can/usb/pcan_usb.c | 654 +++++++++++++++++++
> drivers/net/can/usb/pcan_usb_core.c | 895 ++++++++++++++++++++++++++
> drivers/net/can/usb/pcan_usb_pro.c | 1207 +++++++++++++++++++++++++++++++++++
> drivers/net/can/usb/pcan_usb_pro.h | 468 ++++++++++++++
> drivers/net/can/usb/peak_usb.h | 148 +++++
> 7 files changed, 3404 insertions(+), 0 deletions(-)
> create mode 100644 drivers/net/can/usb/pcan_usb.c
> create mode 100644 drivers/net/can/usb/pcan_usb_core.c
> create mode 100644 drivers/net/can/usb/pcan_usb_pro.c
> create mode 100644 drivers/net/can/usb/pcan_usb_pro.h
> create mode 100644 drivers/net/can/usb/peak_usb.h
>
....
> +++ b/drivers/net/can/usb/pcan_usb_pro.h
> @@ -0,0 +1,468 @@
> +/*
> + * CAN driver for PEAK System PCAN-USB-PRO adapter
> + *
> + * Copyright (C) 2011-2012 PEAK-System GmbH
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +#ifndef __pcan_usb_pro_h__
> +#define __pcan_usb_pro_h__
> +
> +/*
> + * USB Vendor Request Data Types
> + */
> +
> +/* Vendor (other) request */
> +#define USB_VENDOR_REQUEST_INFO 0
> +#define USB_VENDOR_REQUEST_ZERO 1
> +#define USB_VENDOR_REQUEST_FKT 2
Looks like emums to me, here and in many other places (because used in
switch statements).
> +/* Vendor Request wValue for XXX_INFO */
> +#define USB_VENDOR_REQUEST_wVALUE_INFO_BOOTLOADER 0
> +#define USB_VENDOR_REQUEST_wVALUE_INFO_FIRMWARE 1
> +#define USB_VENDOR_REQUEST_wVALUE_INFO_uC_CHIPID 2
Please use upper case.
http://lxr.linux.no/#linux+v3.1.5/Documentation/CodingStyle#L590
> +#define USB_VENDOR_REQUEST_wVALUE_INFO_USB_CHIPID 3
> +#define USB_VENDOR_REQUEST_wVALUE_INFO_DEVICENR 4
> +#define USB_VENDOR_REQUEST_wVALUE_INFO_CPLD 5
> +#define USB_VENDOR_REQUEST_wVALUE_INFO_MODE 6
> +#define USB_VENDOR_REQUEST_wVALUE_INFO_TIMEMODE 7
Please, no hungarian notation in Linux code please. See:
http://lxr.linux.no/#linux+v3.1.5/Documentation/CodingStyle#L252
> +/* Vendor Request wValue for XXX_ZERO */
> +/* Value is Endpoint Number 0-7 */
> +
> +/* Vendor Request wValue for XXX_FKT */
> +#define USB_VENDOR_REQUEST_wVALUE_SETFKT_BOOT 0 // set Bootloader Mode
> +#define USB_VENDOR_REQUEST_wVALUE_SETFKT_DEBUG_CAN 1 // not used
> +#define USB_VENDOR_REQUEST_wVALUE_SETFKT_DEBUG_LIN 2 // not used
> +#define USB_VENDOR_REQUEST_wVALUE_SETFKT_DEBUG1 3 // not used
> +#define USB_VENDOR_REQUEST_wVALUE_SETFKT_DEBUG2 4 // not used
> +#define USB_VENDOR_REQUEST_wVALUE_SETFKT_INTERFACE_DRIVER_LOADED 5
Ditto...
> +/* ctrl_type value */
> +#define INTERN_FIRMWARE_INFO_STRUCT_TYPE 0x11223322
> +#define EXT_FIRMWARE_INFO_STRUCT_TYPE 0x11223344
> +
> +#define BOOTLOADER_INFO_STRUCT_TYPE 0x11112222
> +#define uC_CHIPID_STRUCT_TYPE 0
> +#define USB_CHIPID_STRUCT_TYPE 0
> +#define DEVICE_NR_STRUCT_TYPE 0x3738393A
> +#define CPLD_INFO_STRUCT_TYPE 0x1A1A2277
> +
> +/* USB_VENDOR_REQUEST_wVALUE_INFO_BOOTLOADER vendor request record type */
> +struct __packed pcan_usb_pro_bootloader_info_t
> +{
> + u32 ctrl_type;
> + u8 version[4]; //[0] -> main [1]-> sub [2]-> debug
> + u8 day;
> + u8 month;
> + u8 year;
> + u8 dummy;
> + u32 serial_num_high;
> + u32 serial_num_low;
> + u32 hw_type;
> + u32 hw_rev;
> +};
> +
> +struct __packed pcan_usb_pro_crc_block_t
> +{
> + u32 address;
> + u32 len;
> + u32 crc;
> +};
> +
> +/* USB_VENDOR_REQUEST_wVALUE_INFO_FIRMWARE vendor request record type */
> +struct __packed pcan_usb_pro_ext_firmware_info_t
> +{
> + u32 ctrl_type;
> + u8 version[4]; //[0] -> main [1]-> sub [2]-> debug
> + u8 day;
> + u8 month;
> + u8 year;
> + u8 dummy;
> + u32 fw_type;
> +};
Could you explain the rational behind the prefix
USB_VENDOR_REQUEST_wVALUE_INFO. Why does the struct use a different
prefix. I think this is some remainder from old Windoze code using
hungarian notation!?
> +/* USB_VENDOR_REQUEST_wVALUE_INFO_uC_CHIPID vendor request record type */
> +struct __packed pcan_usb_pro_uc_chipid_t
> +{
> + u32 ctrl_type;
> + u32 chip_id;
> +};
Ditto here and in many other places.
> +/*
> + * USB Command Record types
> + */
> +
> +#define DATA_TYPE_USB2CAN_STRUCT_CANMSG_RX_8 0x80
> +#define DATA_TYPE_USB2CAN_STRUCT_CANMSG_RX_4 0x81
> +#define DATA_TYPE_USB2CAN_STRUCT_CANMSG_RX_0 0x82
> +#define DATA_TYPE_USB2CAN_STRUCT_CANMSG_RTR_RX 0x83
> +#define DATA_TYPE_USB2CAN_STRUCT_CANMSG_STATUS_ERROR_RX 0x84
> +#define DATA_TYPE_USB2CAN_STRUCT_CALIBRATION_TIMESTAMP_RX 0x85
> +#define DATA_TYPE_USB2CAN_STRUCT_BUSLAST_RX 0x86
> +#define DATA_TYPE_USB2CAN_STRUCT_CANMSG_TX_8 0x41
> +#define DATA_TYPE_USB2CAN_STRUCT_CANMSG_TX_4 0x42
> +#define DATA_TYPE_USB2CAN_STRUCT_CANMSG_TX_0 0x43
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_GETBAUDRATE 0x01
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETBAUDRATE 0x02
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_GETCANBUSACTIVATE 0x03
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETCANBUSACTIVATE 0x04
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETSILENTMODE 0x05
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETDEVICENR 0x06
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETWARNINGLIMIT 0x07
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETLOOKUP_EXPLICIT 0x08
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETLOOKUP_GROUP 0x09
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETFILTERMODE 0x0a
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETRESET_MODE 0x0b
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETERRORFRAME 0x0c
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_GETCANBUS_ERROR_STATUS 0x0D
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETREGISTER 0x0e
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_GETREGISTER 0x0f
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETGET_CALIBRATION_MSG 0x10
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETGET_BUSLAST_MSG 0x11
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_GETDEVICENR 0x12
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SETSTRING 0x13
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_GETSTRING 0x14
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_STRING 0x15
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SAVEEEPROM 0x16
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_USB_IN_PACKET_DELAY 0x17
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_TIMESTAMP_PARAM 0x18
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_ERROR_GEN_ID 0x19
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_ERROR_GEN_NOW 0x1A
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SET_SOFTFILER 0x1B
> +#define DATA_TYPE_USB2CAN_STRUCT_FKT_SET_CANLED 0x1C
Ditto, why is the prefix "DATA_TYPE_USB2CAN_STRUCT_FKT"? Please use
logical and consistent prefixes. I will make the code more readable.
Wolfgang.
prev parent reply other threads:[~2011-12-21 9:33 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-12-16 10:19 "[PATCH] Add support for PEAK-System GmbH CAN USB adapters (v2) " s.grosjean
2011-12-16 11:34 ` Wolfgang Grandegger
2011-12-16 12:41 ` Marc Kleine-Budde
2011-12-20 12:10 ` Grosjean Stephane
2011-12-20 15:57 ` Wolfgang Grandegger
2011-12-20 20:50 ` Marc Kleine-Budde
2011-12-17 13:17 ` Sebastian Haas
2011-12-19 17:03 ` Stephane Grosjean
2011-12-21 9:33 ` Wolfgang Grandegger [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4EF1A7F6.3030106@grandegger.com \
--to=wg@grandegger.com \
--cc=linux-can@vger.kernel.org \
--cc=s.grosjean@peak-system.com \
--cc=socketcan@hartkopp.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).