Hi Peter, A few comments below: On Tue Mar 21, 2023 at 9:11 AM CET, Ji-Ze Hong (Peter Hong) wrote: > [You don't often get email from peter_hong@xxxxxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > This patch add support for Fintek USB to 2CAN controller support. > > Signed-off-by: Ji-Ze Hong (Peter Hong) <peter_hong@xxxxxxxxxxxxx> > --- > Changelog: > v2: > 1. coding style refactoring. > 2. some const number are defined to describe itself. > 3. fix wrong usage for can_get_echo_skb() in f81604_write_bulk_callback(). > > drivers/net/can/usb/Kconfig | 9 + > drivers/net/can/usb/Makefile | 1 + > drivers/net/can/usb/f81604.c | 1179 ++++++++++++++++++++++++++++++++++ > 3 files changed, 1189 insertions(+) > create mode 100644 drivers/net/can/usb/f81604.c > ...snip... > +static int f81604_set_bittiming(struct net_device *dev) > +{ > + struct f81604_port_priv *priv = netdev_priv(dev); > + struct can_bittiming *bt = &priv->can.bittiming; > + int status = 0; > + u8 btr0, btr1; > + > + btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6); > + btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) | > + (((bt->phase_seg2 - 1) & 0x7) << 4); Use the FIELD_GET and GENMASK operators to make this more readable. > + > + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) > + btr1 |= 0x80; > + > + netdev_info(dev, "BTR0=0x%02x BTR1=0x%02x\n", btr0, btr1); > + > + status = f81604_set_sja1000_register(priv->dev, dev->dev_id, > + SJA1000_BTR0, btr0); > + if (status) { > + netdev_warn(dev, "%s: Set BTR0 failed: %d\n", __func__, > + status); > + return status; > + } > + > + status = f81604_set_sja1000_register(priv->dev, dev->dev_id, > + SJA1000_BTR1, btr1); > + if (status) { > + netdev_warn(dev, "%s: Set BTR1 failed: %d\n", __func__, > + status); > + return status; > + } > + > + return 0; > +} > + > +static int f81604_set_mode(struct net_device *netdev, enum can_mode mode) > +{ > + int err; > + > + switch (mode) { > + case CAN_MODE_START: > + err = f81604_start(netdev); > + if (!err && netif_queue_stopped(netdev)) > + netif_wake_queue(netdev); > + break; > + > + default: > + err = -EOPNOTSUPP; > + } > + > + return err; > +} > + > +static void f81604_process_rx_packet(struct urb *urb) > +{ > + struct net_device *netdev = urb->context; > + struct net_device_stats *stats; > + struct can_frame *cf; > + struct sk_buff *skb; > + int i, count; > + u8 *data; > + u8 *ptr; > + > + data = urb->transfer_buffer; > + stats = &netdev->stats; > + > + if (urb->actual_length % 14) You need a symbol for this value: 14 > + netdev_warn(netdev, "actual_length %% 14 != 0 (%d)\n", > + urb->actual_length); > + else if (!urb->actual_length) > + netdev_warn(netdev, "actual_length = 0 (%d)\n", > + urb->actual_length); > + > + count = urb->actual_length / F81604_DATA_SIZE; > + > + for (i = 0; i < count; ++i) { > + ptr = &data[i * F81604_DATA_SIZE]; > + > + if (ptr[F81604_CMD_OFFSET] != F81604_CMD_DATA) > + continue; > + > + skb = alloc_can_skb(netdev, &cf); > + if (!skb) { > + netdev_warn(netdev, "%s: not enough memory", __func__); > + continue; > + } > + > + cf->can_dlc = can_cc_dlc2len(ptr[F81604_DLC_OFFSET] & 0xF); > + > + if (ptr[F81604_DLC_OFFSET] & F81604_EFF_BIT) { > + cf->can_id = (ptr[F81604_ID1_OFFSET] << 21) | > + (ptr[F81604_ID2_OFFSET] << 13) | > + (ptr[F81604_ID3_OFFSET] << 5) | > + (ptr[F81604_ID4_OFFSET] >> 3); > + cf->can_id |= CAN_EFF_FLAG; > + } else { > + cf->can_id = (ptr[F81604_ID1_OFFSET] << 3) | > + (ptr[F81604_ID2_OFFSET] >> 5); > + } Again, use the field operators. > + > + if (ptr[F81604_DLC_OFFSET] & F81604_RTR_BIT) { > + cf->can_id |= CAN_RTR_FLAG; > + } else if (ptr[F81604_DLC_OFFSET] & F81604_EFF_BIT) { > + memcpy(cf->data, &ptr[F81604_EFF_DATA_OFFSET], > + cf->can_dlc); > + } else { > + memcpy(cf->data, &ptr[F81604_SFF_DATA_OFFSET], > + cf->can_dlc); > + } > + > + stats->rx_packets++; > + stats->rx_bytes += cf->can_dlc; > + netif_rx(skb); > + } > +} > + ...snip... > +static int f81604_probe(struct usb_interface *intf, > + const struct usb_device_id *id) > +{ > + struct usb_device *dev = interface_to_usbdev(intf); > + struct f81604_port_priv *port_priv; > + struct net_device *netdev; > + struct f81604_priv *priv; > + int i, err; > + > + dev_info(&intf->dev, "Detected Fintek F81604 device.\n"); > + dev_info(&intf->dev, > + "Please download newest driver from Fintek website\n"); > + dev_info(&intf->dev, "if you want to use customized functions.\n"); > + > + priv = devm_kzalloc(&intf->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + usb_set_intfdata(intf, priv); > + > + for (i = 0; i < F81604_MAX_DEV; ++i) { > + netdev = alloc_candev(sizeof(*port_priv), 1); > + if (!netdev) { > + dev_err(&intf->dev, "Couldn't alloc candev: %d\n", i); > + err = -ENOMEM; > + > + goto failure_cleanup; > + } > + > + port_priv = netdev_priv(netdev); > + netdev->dev_id = i; > + > + spin_lock_init(&port_priv->lock); > + INIT_WORK(&port_priv->handle_clear_overrun_work, > + f81604_handle_clear_overrun_work); > + INIT_WORK(&port_priv->handle_clear_reg_work, > + f81604_handle_clear_reg_work); > + > + port_priv->intf = intf; > + port_priv->dev = dev; > + port_priv->ocr = OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL; > + port_priv->cdr = CDR_CBP; > + port_priv->can.state = CAN_STATE_STOPPED; > + port_priv->can.clock.freq = 24000000 / 2; Can you get this frequency from a clock driver? Otherwise add a symbol for it... > + > + port_priv->can.bittiming_const = &f81604_bittiming_const; > + port_priv->can.do_set_bittiming = f81604_set_bittiming; > + port_priv->can.do_set_mode = f81604_set_mode; > + port_priv->can.do_get_berr_counter = f81604_get_berr_counter; > + port_priv->can.ctrlmode_supported = > + CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES | > + CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING; > + > + port_priv->can.ctrlmode_supported |= CAN_CTRLMODE_PRESUME_ACK; > + netdev->netdev_ops = &f81604_netdev_ops; > + netdev->flags |= IFF_ECHO; > + > + SET_NETDEV_DEV(netdev, &intf->dev); > + > + err = register_candev(netdev); > + if (err) { > + netdev_err(netdev, > + "couldn't register CAN device: %d\n", err); > + free_candev(netdev); > + > + goto failure_cleanup; > + } > + > + port_priv->netdev = netdev; > + priv->netdev[i] = netdev; > + > + dev_info(&intf->dev, "Channel #%d registered as %s\n", i, > + netdev->name); > + } > + > + return 0; > + > +failure_cleanup: > + f81604_disconnect(intf); > + return err; > +} > + > +static struct usb_driver f81604_driver = { > + .name = "f81604", > + .probe = f81604_probe, > + .disconnect = f81604_disconnect, > + .id_table = f81604_table, > +}; > + > +module_usb_driver(f81604_driver); > + > +MODULE_AUTHOR("Ji-Ze Hong (Peter Hong) <peter_hong@xxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("Fintek F81604 USB to 2xCANBUS"); > +MODULE_LICENSE("GPL"); > -- > 2.17.1 BR Steen
