Re: [PATCH V2] can: usb: f81604: add Fintek F81604 support

From: Steen.Hegelund
Date: Tue Mar 21 2023 - 08:43:37 EST

Next message: Rob Herring: "Re: [PATCH] dt-bindings: usb: Drop unneeded quotes"
Previous message: Mike Rapoport: "Re: [PATCH v11 0/7] Implement IOCTL to get and optionally clear info about PTEs"
In reply to: Ji-Ze Hong (Peter Hong): "[PATCH V2] can: usb: f81604: add Fintek F81604 support"
Next in thread: Vincent MAILHOL: "Re: [PATCH V2] can: usb: f81604: add Fintek F81604 support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Rob Herring: "Re: [PATCH] dt-bindings: usb: Drop unneeded quotes"
Previous message: Mike Rapoport: "Re: [PATCH v11 0/7] Implement IOCTL to get and optionally clear info about PTEs"
In reply to: Ji-Ze Hong (Peter Hong): "[PATCH V2] can: usb: f81604: add Fintek F81604 support"
Next in thread: Vincent MAILHOL: "Re: [PATCH V2] can: usb: f81604: add Fintek F81604 support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]