lkml.org 
[lkml]   [2023]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V3] can: usb: f81604: add Fintek F81604 support
From
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

\
 
 \ /
  Last update: 2023-03-30 07:42    [W:0.118 / U:0.820 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site