Messages in this thread | | | Date | Thu, 30 Mar 2023 13:40:08 +0800 | Subject | Re: [PATCH V3] can: usb: f81604: add Fintek F81604 support | From | Peter Hong <> |
| |
Hi Marc,
Marc Kleine-Budde 於 2023/3/27 下午 11:09 寫道: > On 27.03.2023 13:10:48, Ji-Ze Hong (Peter Hong) wrote: >> This patch add support for Fintek USB to 2CAN controller support. > In addition to Vincent's comment, please describe your change > declarative, e.g.: > > | Add support for the Fintek USB to 2CAN controller. > > Please add an entry in the MAINTAINERS file. >
We'll add description to MAINTAINERS file as following:
FINTEK F81604 USB to 2CAN DEVICE DRIVERS M: Ji-Ze Hong (Peter Hong) <peter_hong@fintek.com.tw> L: linux-can@vger.kernel.org S: Maintained
Is this OK?
>> + u8 bulk_read_buffer[F81604_MAX_RX_URBS][F81604_BULK_SIZE]; > allocate the URB dynamic with kmalloc() and use urb->transfer_flags |= > URB_FREE_BUFFER for automatic free()ing.
Could I use kmalloc on device probe() and kfree on disconnect() ? It may reduce a lot malloc/free times.
>> + >> + struct urb *write_urb; >> + u8 bulk_write_buffer[F81604_DATA_SIZE]; > With just 1 TX URB the TX would be quite slow. Does your hardware > support more than 1 TX URB at a time? USB devices usually answer with > NAK it they are busy, automatic and transparent retry is handled by the > USB host controller.
This device only NAK when TX EP buf full. It maybe malfunction when sending TX URB while CAN transmitting.
>> +}; >> + >> +/* Interrupt endpoint data format: SR/IR/IER/ALC/ECC/EWLR/RXERR/TXERR/VAL */ >> +struct f81604_int_data { >> + u8 sr; >> + u8 isrc; >> + u8 ier; >> + u8 alc; >> + u8 ecc; >> + u8 ewlr; >> + u8 rxerr; >> + u8 txerr; >> + u8 val; >> +} __packed; > I think this struct is always aligned to 4 bytes, so add a __aligned(4) > behind __packed. >> + >> +struct f81604_sff { >> + __be16 id; >> + u8 data[CAN_MAX_DLEN]; >> +} __packed; > __aligned(2) > >> + >> +struct f81604_eff { >> + __be32 id; >> + u8 data[CAN_MAX_DLEN]; >> +} __packed; > __aligned(2) > >> + >> +struct f81604_bulk_data { >> + u8 cmd; >> + >> + /* According for F81604 DLC define: >> + * #define F81604_DLC_LEN_MASK 0x0f >> + * #define F81604_DLC_EFF_BIT BIT(7) >> + * #define F81604_DLC_RTR_BIT BIT(6) > no need to duplicate code > >> + */ >> + u8 dlc; >> + >> + union { >> + struct f81604_sff sff; >> + struct f81604_eff eff; >> + }; >> +} __packed; > __aligned(4)
I had add following aligned is OK. struct f81604_int_data; __aligned(4) struct f81604_sff; __aligned(2) struct f81604_eff; __aligned(2)
But when I add __aligned(4) to struct f81604_bulk_data, It'll generate error about size != 14.
./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct f81604_bulk_data) == F81604_DATA_SIZE"
So I'll add align to these structs without f81604_bulk_data. Is it OK?
>> + usb_fill_bulk_urb(priv->read_urb[i], priv->dev, >> + usb_rcvbulkpipe(priv->dev, bulk_in_addr[id]), >> + priv->bulk_read_buffer[i], F81604_BULK_SIZE, >> + f81604_read_bulk_callback, netdev); >> + } >> + >> + priv->write_urb = usb_alloc_urb(0, GFP_KERNEL); >> + if (!priv->write_urb) >> + goto error; >> + >> + usb_fill_bulk_urb(priv->write_urb, priv->dev, >> + usb_sndbulkpipe(priv->dev, bulk_out_addr[id]), >> + priv->bulk_write_buffer, F81604_DATA_SIZE, >> + f81604_write_bulk_callback, netdev); >> + >> + priv->int_urb = usb_alloc_urb(0, GFP_KERNEL); >> + if (!priv->int_urb) >> + goto error; >> + >> + usb_fill_int_urb(priv->int_urb, priv->dev, >> + usb_rcvintpipe(priv->dev, int_in_addr[id]), >> + priv->int_read_buffer, F81604_INT_SIZE, >> + f81604_read_int_callback, netdev, 1); > Does the endpoint descriptor specify a proper interval?
The interval of interrupt endpoint is 1 (1ms).
Bus 001 Device 010: ID 2c42:1709 FINTEK USB TO CANBUS BRIDGE Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 1.10 bDeviceClass 0 bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize0 64 idVendor 0x2c42 idProduct 0x1709 bcdDevice 0.01 iManufacturer 1 FINTEK iProduct 2 USB TO CANBUS BRIDGE iSerial 3 88635600168801 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 0x003c bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 bmAttributes 0xa0 (Bus Powered) Remote Wakeup MaxPower 100mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 6 bInterfaceClass 0 bInterfaceSubClass 0 bInterfaceProtocol 0 iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0010 1x 16 bytes bInterval 1 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x82 EP 2 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0040 1x 64 bytes bInterval 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x01 EP 1 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0040 1x 64 bytes bInterval 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0010 1x 16 bytes bInterval 1 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x84 EP 4 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0040 1x 64 bytes bInterval 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x03 EP 3 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0040 1x 64 bytes bInterval 0 Device Status: 0xa780 (Bus Powered)
Thanks
| |