Re: [PATCH v2] usb_8dev: Add support for USB2CAN interface from 8 devices
Hi, at a closer look I see a few more issues:
Please use s/dev_warn(netdev->dev.parent,/netdev_warn(/ for dev_warn and
friends.
You can drop the do_set_bittiming callback if it's not needed. There is
no need for a dummy function, IIRC.
And please also drop the remaining sysfs files for firmware and
hardware. I thinks it's enough that the versions are printed when the
device is probed.
On 12/03/2012 01:50 AM, krumbo...@universalnet.at wrote:
> Add device driver for USB2CAN interface from "8 devices"
> (http://www.8devices.com).
>
> Signed-off-by: Bernd Krumboeck
> ---
> drivers/net/can/usb/Kconfig|6 +
> drivers/net/can/usb/Makefile |1 +
> drivers/net/can/usb/usb_8dev.c | 1098
>
> 3 files changed, 1105 insertions(+)
> create mode 100644 drivers/net/can/usb/usb_8dev.c
>
> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index a4e4bee..2162233 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -48,4 +48,10 @@ config CAN_PEAK_USB
>This driver supports the PCAN-USB and PCAN-USB Pro adapters
>from PEAK-System Technik (http://www.peak-system.com).
>
> +config CAN_8DEV_USB
> +tristate "8 devices USB2CAN interface"
> +---help---
> + This driver supports the USB2CAN interface
> + from 8 devices (http://www.8devices.com).
> +
> endmenu
> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index 80a2ee4..becef46 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -6,5 +6,6 @@ obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
> obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
> obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o
> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
> +obj-$(CONFIG_CAN_8DEV_USB) += usb_8dev.o
>
> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/usb/usb_8dev.c
> b/drivers/net/can/usb/usb_8dev.c
> new file mode 100644
> index 000..345ce6e
> --- /dev/null
> +++ b/drivers/net/can/usb/usb_8dev.c
> @@ -0,0 +1,1098 @@
> +/*
> + * CAN driver for UAB "8 devices" USB2CAN converter
UAB means?
> +/* Get firmware version */
> +static ssize_t show_firmware(struct device *d, struct device_attribute
> *attr,
> + char *buf)
> +{
> +struct usb_8dev_cmd_msgoutmsg;
> +struct usb_8dev_cmd_msginmsg;
Just one space.
> +int err = 0;
> +u16 result;
> +struct usb_interface *intf = to_usb_interface(d);
> +struct usb_8dev *dev = usb_get_intfdata(intf);
> +
> +memset(&outmsg, 0, sizeof(struct usb_8dev_cmd_msg));
> +outmsg.command = USB_8DEV_GET_SOFTW_VER;
> +
> +err = usb_8dev_send_cmd(dev, &outmsg, &inmsg);
> +if (err)
> +return -EIO;
> +
> +result = be16_to_cpu(*(u16 *)inmsg.data);
> +
> +return sprintf(buf, "%d.%d\n", (u8)(result>>8), (u8)result);
> +}
> +
> +/* Get hardware version */
> +static ssize_t show_hardware(struct device *d, struct device_attribute
> *attr,
> + char *buf)
> +{
> +struct usb_8dev_cmd_msgoutmsg;
> +struct usb_8dev_cmd_msginmsg;
Ditto.
> +static DEVICE_ATTR(firmware, S_IRUGO, show_firmware, NULL);
> +static DEVICE_ATTR(hardware, S_IRUGO, show_hardware, NULL);
> +
> +/*
> + * Set network device mode
> + *
> + * Maybe we should leave this function empty, because the device
> + * set mode variable with open command.
> + */
> +static int usb_8dev_set_mode(struct net_device *netdev, enum can_mode
> mode)
> +{
> +struct usb_8dev *dev = netdev_priv(netdev);
> +int err = 0;
> +
> +switch (mode) {
> +case CAN_MODE_START:
> +err = usb_8dev_cmd_open(dev);
> +if (err)
> +dev_warn(netdev->dev.parent, "couldn't start device");
> +
> +if (netif_queue_stopped(netdev))
> +netif_wake_queue(netdev);
> +break;
> +
> +default:
> +return -EOPNOTSUPP;
> +}
> +
> +return 0;
> +}
> +
> +/*
> + * Empty function: We set bittiming when we start the interface.
> + * This is a firmware limitation.
> + */
> +static int usb_8dev_set_bittiming(struct net_device *netdev)
> +{
> +return 0;
> +}
> +
> +/* Read data and status frames */
> +static void usb_8dev_rx_can_msg(struct usb_8dev *dev,
> +struct usb_8dev_rx_msg *msg)
> +{
> +struct can_frame *cf;
> +struct sk_buff *skb;
> +int i;
> +struct net_device_stats *stats = &dev->netdev->stats;
> +
> +if (msg->type == USB_8DEV_TYPE_CAN_FRAME) {
> +skb = alloc_can_skb(dev->netdev, &cf);
> +if (!skb)
> +return;
> +
> +cf->can_id = be32_to_cpu(msg->id);
> +cf->can_dlc = get_can_dlc(msg->dlc & 0xF);
> +
> +if (msg->flags & USB_8DEV_EXTID)
> +cf->can_id |= CAN_EFF_FLAG;
> +
> +if (msg->flags & USB_8DEV_RTR)
> +cf->can_id |= CAN_RTR_FLAG;
> +else
> +for (i = 0; i < cf->can_dlc; i++)
> +cf->data[i] = msg->da
Re: [PATCH v2] usb_8dev: Add support for USB2CAN interface from 8 devices
On 12/03/2012 01:50 AM, krumbo...@universalnet.at wrote:
> Add device driver for USB2CAN interface from "8 devices"
> (http://www.8devices.com).
>
> Signed-off-by: Bernd Krumboeck
Please send your patch with git send-email, as with this one all tabs
are converted to spaces.
> ---
> drivers/net/can/usb/Kconfig|6 +
> drivers/net/can/usb/Makefile |1 +
> drivers/net/can/usb/usb_8dev.c | 1098
>
> 3 files changed, 1105 insertions(+)
> create mode 100644 drivers/net/can/usb/usb_8dev.c
>
> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index a4e4bee..2162233 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -48,4 +48,10 @@ config CAN_PEAK_USB
>This driver supports the PCAN-USB and PCAN-USB Pro adapters
>from PEAK-System Technik (http://www.peak-system.com).
>
> +config CAN_8DEV_USB
> +tristate "8 devices USB2CAN interface"
> +---help---
> + This driver supports the USB2CAN interface
> + from 8 devices (http://www.8devices.com).
> +
> endmenu
> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index 80a2ee4..becef46 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -6,5 +6,6 @@ obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
> obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
> obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o
> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
> +obj-$(CONFIG_CAN_8DEV_USB) += usb_8dev.o
>
> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/usb/usb_8dev.c
> b/drivers/net/can/usb/usb_8dev.c
> new file mode 100644
> index 000..345ce6e
> --- /dev/null
> +++ b/drivers/net/can/usb/usb_8dev.c
> @@ -0,0 +1,1098 @@
> +/*
> + * CAN driver for UAB "8 devices" USB2CAN converter
> + *
> + * Copyright (C) 2012 Bernd Krumboeck (krumbo...@universalnet.at)
> + *
> + * 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.
> + *
> + * This driver is inspired by the 3.2.0 version of
> drivers/net/can/usb/ems_usb.c
> + * and drivers/net/can/usb/esd_usb2.c
> + *
> + * Many thanks to Gerhard Bertelsmann (i...@gerhard-bertelsmann.de)
> + * for testing and fixing this driver. Also many thanks to "8 devices",
> + * who were very cooperative and answered my questions.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +/* driver constants */
> +#define MAX_RX_URBS10
> +#define MAX_TX_URBS10
> +#define RX_BUFFER_SIZE64
> +
> +/* vendor and product id */
> +#define USB_8DEV_VENDOR_ID0x0483
> +#define USB_8DEV_PRODUCT_ID0x1234
> +
> +/* bittiming constants */
> +#define USB_8DEV_ABP_CLOCK3200
> +#define USB_8DEV_BAUD_MANUAL0x09
> +#define USB_8DEV_TSEG1_MIN1
> +#define USB_8DEV_TSEG1_MAX16
> +#define USB_8DEV_TSEG2_MIN1
> +#define USB_8DEV_TSEG2_MAX8
> +#define USB_8DEV_SJW_MAX4
> +#define USB_8DEV_BRP_MIN1
> +#define USB_8DEV_BRP_MAX1024
> +#define USB_8DEV_BRP_INC1
> +
> +/* setup flags */
> +#define USB_8DEV_SILENT0x01
> +#define USB_8DEV_LOOPBACK0x02
> +#define USB_8DEV_DISABLE_AUTO_RESTRANS0x04
> +#define USB_8DEV_STATUS_FRAME0x08
> +
> +/* commands */
> +enum usb_8dev_cmd {
> +USB_8DEV_RESET = 1,
> +USB_8DEV_OPEN,
> +USB_8DEV_CLOSE,
> +USB_8DEV_SET_SPEED,
> +USB_8DEV_SET_MASK_FILTER,
> +USB_8DEV_GET_STATUS,
> +USB_8DEV_GET_STATISTICS,
> +USB_8DEV_GET_SERIAL,
> +USB_8DEV_GET_SOFTW_VER,
> +USB_8DEV_GET_HARDW_VER,
> +USB_8DEV_RESET_TIMESTAMP,
> +USB_8DEV_GET_SOFTW_HARDW_VER
> +};
> +
> +#define USB_8DEV_CMD_START0x11
> +#define USB_8DEV_CMD_END0x22
> +
> +#define USB_8DEV_CMD_SUCCESS0
> +#define USB_8DEV_CMD_ERROR255
> +
> +/* frames */
> +#define USB_8DEV_DATA_START0x55
> +#define USB_8DEV_DATA_END0xAA
> +
> +#define USB_8DEV_TYPE_CAN_FRAME0
> +#define USB_8DEV_TYPE_ERROR_FRAME3
> +
> +#define USB_8DEV_EXTID0x01
> +#define USB_8DEV_RTR0x02
> +#define USB_8DEV_ERR_FLAG0x04
> +
> +/* status */
> +#define USB_8DEV_STATUSMSG_OK0x00 /* Normal condition. */
> +#d
Re: [PATCH v2] usb_8dev: Add support for USB2CAN interface from 8 devices
Hi Wolfgang! And please also drop the remaining sysfs files for firmware and hardware. I thinks it's enough that the versions are printed when the device is probed. Systemadministrators often use versions for configuration scripts. If you insist I will remove. +/* + * CAN driver for UAB "8 devices" USB2CAN converter UAB means? I don't know. I've taken from the homepage. Maybe it is a type of company. I will remove... Thanks for your comments and suggestions. regards, Bernd -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb_8dev: Add support for USB2CAN interface from 8 devices
Hi Marc!
+/* Send open command to device */
+static int usb_8dev_cmd_open(struct usb_8dev *dev)
+{
+struct can_bittiming *bt = &dev->can.bittiming;
+struct usb_8dev_cmd_msg outmsg;
+struct usb_8dev_cmd_msg inmsg;
+u32 flags = 0;
+u32 beflags;
+u16 bebrp;
+u32 ctrlmode = dev->can.ctrlmode;
+
+if (ctrlmode & CAN_CTRLMODE_LOOPBACK)
+flags |= USB_8DEV_LOOPBACK;
+if (ctrlmode & CAN_CTRLMODE_LISTENONLY)
+flags |= USB_8DEV_SILENT;
+if (ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+flags |= USB_8DEV_DISABLE_AUTO_RESTRANS;
+
+flags |= USB_8DEV_STATUS_FRAME;
+
+memset(&outmsg, 0, sizeof(struct usb_8dev_cmd_msg));
+outmsg.command = USB_8DEV_OPEN;
+outmsg.opt1 = USB_8DEV_BAUD_MANUAL;
+outmsg.data[0] = (bt->prop_seg + bt->phase_seg1);
+outmsg.data[1] = bt->phase_seg2;
+outmsg.data[2] = bt->sjw;
But you should not use usb_bulk_msg() to send data which is on the
stack, please use you already allocated memory in priv or kmalloc
something here.
The function usb_8dev_send_cmd copies the data to dev->cmd_msg_buffer
(allocated with kzalloc, GFP_KERNEL).
+
+/* BRP */
+bebrp = cpu_to_be16((u16) bt->brp);
+memcpy(&outmsg.data[3], &bebrp, sizeof(bebrp));
Are you sure about the endianess? Some data types are BE some are LE?
Where is LE used?
+
+/* flags */
+beflags = cpu_to_be32(flags);
+memcpy(&outmsg.data[5], &beflags, sizeof(beflags));
+
+return usb_8dev_send_cmd(dev, &outmsg, &inmsg);
+}
+
+/* Send close command to device */
+static int usb_8dev_cmd_close(struct usb_8dev *dev)
+{
+struct usb_8dev_cmd_msgoutmsg;
+struct usb_8dev_cmd_msginmsg;
+
+memset(&outmsg, 0, sizeof(struct usb_8dev_cmd_msg));
+outmsg.command = USB_8DEV_CLOSE;
Same here, don't use stack allocated mem for usb_bulk_msg
All command functions use usb_8dev_send_cmd.
Are you sure?Do you really want to wake the queue?
No, I've removed.
needed?
I don't know. -> removed.
Thanks for your comments.
best regards,
Bernd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb_8dev: Add support for USB2CAN interface from 8 devices
On 12/03/2012 09:32 PM, krumbo...@universalnet.at wrote: > Hi Wolfgang! > >> >> And please also drop the remaining sysfs files for firmware and >> hardware. I thinks it's enough that the versions are printed when the >> device is probed. > > Systemadministrators often use versions for configuration scripts. > If you insist I will remove. Marc? Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb_8dev: Add support for USB2CAN interface from 8 devices
On 12/03/2012 09:42 PM, krumbo...@universalnet.at wrote:
> Hi Marc!
>
>
>>> +/* Send open command to device */
>>> +static int usb_8dev_cmd_open(struct usb_8dev *dev)
>>> +{
>>> +struct can_bittiming *bt = &dev->can.bittiming;
>>> +struct usb_8dev_cmd_msg outmsg;
>>> +struct usb_8dev_cmd_msg inmsg;
>>> +u32 flags = 0;
>>> +u32 beflags;
>>> +u16 bebrp;
>>> +u32 ctrlmode = dev->can.ctrlmode;
>>> +
>>> +if (ctrlmode & CAN_CTRLMODE_LOOPBACK)
>>> +flags |= USB_8DEV_LOOPBACK;
>>> +if (ctrlmode & CAN_CTRLMODE_LISTENONLY)
>>> +flags |= USB_8DEV_SILENT;
>>> +if (ctrlmode & CAN_CTRLMODE_ONE_SHOT)
>>> +flags |= USB_8DEV_DISABLE_AUTO_RESTRANS;
>>> +
>>> +flags |= USB_8DEV_STATUS_FRAME;
>>> +
>>> +memset(&outmsg, 0, sizeof(struct usb_8dev_cmd_msg));
>>> +outmsg.command = USB_8DEV_OPEN;
>>> +outmsg.opt1 = USB_8DEV_BAUD_MANUAL;
>>> +outmsg.data[0] = (bt->prop_seg + bt->phase_seg1);
>>> +outmsg.data[1] = bt->phase_seg2;
>>> +outmsg.data[2] = bt->sjw;
>>
>> But you should not use usb_bulk_msg() to send data which is on the
>> stack, please use you already allocated memory in priv or kmalloc
>> something here.
>
> The function usb_8dev_send_cmd copies the data to dev->cmd_msg_buffer
> (allocated with kzalloc, GFP_KERNEL).
Okay, then you might want to set outmsg via C99 initializers.
>>> +
>>> +/* BRP */
>>> +bebrp = cpu_to_be16((u16) bt->brp);
>>> +memcpy(&outmsg.data[3], &bebrp, sizeof(bebrp));
>>
>> Are you sure about the endianess? Some data types are BE some are LE?
>
> Where is LE used?
Opps, yeah right. only BE.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2] usb_8dev: Add support for USB2CAN interface from 8 devices
On 12/03/2012 09:15 PM, Wolfgang Grandegger wrote: > On 12/03/2012 09:32 PM, krumbo...@universalnet.at wrote: >> Hi Wolfgang! >> >>> >>> And please also drop the remaining sysfs files for firmware and >>> hardware. I thinks it's enough that the versions are printed when the >>> device is probed. >> >> Systemadministrators often use versions for configuration scripts. >> If you insist I will remove. > > Marc? Are these values exported via lsusb -v (e.g. iSerial)? Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
