Re: [PATCH v8 4/7] can: Add Nuvoton NCT6694 CANFD support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> 於 2025年2月28日 週五 下午6:44寫道:
>
> > +static void nct6694_can_handle_lost_msg(struct net_device *ndev)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct net_device_stats *stats = &ndev->stats;
> > +     struct can_frame *cf;
> > +     struct sk_buff *skb;
> > +
> > +     netdev_err(ndev, "RX FIFO overflow, message(s) lost.\n");
>
> Please make it a netdev_dbg()
>

Fix it in v9.

> > +
> > +     stats->rx_errors++;
> > +     stats->rx_over_errors++;
> > +
> > +     skb = alloc_can_err_skb(ndev, &cf);
> > +     if (!skb)
> > +             return;
> > +
> > +     cf->can_id |= CAN_ERR_CRTL;
> > +     cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> > +
> > +     nct6694_can_rx_offload(&priv->offload, skb);
> > +}
> > +
> > +static void nct6694_can_rx(struct net_device *ndev, u8 rx_evt)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct nct6694_can_frame *frame = &priv->rx;
> > +     const struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_RECEIVE,
> > +             .sel = NCT6694_CAN_RECEIVE_SEL(ndev->dev_port, 1),
> > +             .len = cpu_to_le16(sizeof(*frame))
> > +     };
> > +     struct sk_buff *skb;
> > +     int ret;
> > +
> > +     ret = nct6694_read_msg(priv->nct6694, &cmd_hd, frame);
> > +     if (ret)
> > +             return;
> > +
> > +     if (frame->flag & NCT6694_CAN_FRAME_FLAG_FD) {
> > +             struct canfd_frame *cfd;
> > +
> > +             skb = alloc_canfd_skb(priv->ndev, &cfd);
> > +             if (!skb)
>
> stats->rx_dropped++;
>

Add it in v9.

> > +                     return;
> > +
> > +             cfd->can_id = le32_to_cpu(frame->id);
> > +             cfd->len = canfd_sanitize_len(frame->length);
> > +             if (frame->flag & NCT6694_CAN_FRAME_FLAG_EFF)
> > +                     cfd->can_id |= CAN_EFF_FLAG;
> > +             if (frame->flag & NCT6694_CAN_FRAME_FLAG_BRS)
> > +                     cfd->flags |= CANFD_BRS;
> > +             if (frame->flag & NCT6694_CAN_FRAME_FLAG_ERR)
> > +                     cfd->flags |= CANFD_ESI;
> > +
> > +             memcpy(cfd->data, frame->data, cfd->len);
> > +     } else {
> > +             struct can_frame *cf;
> > +
> > +             skb = alloc_can_skb(priv->ndev, &cf);
> > +             if (!skb)
>
> stats->rx_dropped++;
>

Add it in v9.

> > +                     return;
> > +
> > +             cf->can_id = le32_to_cpu(frame->id);
> > +             cf->len = can_cc_dlc2len(frame->length);
> > +             if (frame->flag & NCT6694_CAN_FRAME_FLAG_EFF)
> > +                     cf->can_id |= CAN_EFF_FLAG;
> > +
> > +             if (frame->flag & NCT6694_CAN_FRAME_FLAG_RTR)
> > +                     cf->can_id |= CAN_RTR_FLAG;
> > +             else
> > +                     memcpy(cf->data, frame->data, cf->len);
> > +     }
> > +
> > +     nct6694_can_rx_offload(&priv->offload, skb);
> > +}
> > +
> > +static int nct6694_can_get_berr_counter(const struct net_device *ndev,
> > +                                     struct can_berr_counter *bec)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > +     *bec = priv->bec;
> > +
> > +     return 0;
> > +}
> > +
> > +static void nct6694_can_handle_state_change(struct net_device *ndev,
> > +                                         enum can_state new_state)
> > +{
>
> I just replied to one of your questions of the last series:
>
> | https://lore.kernel.org/20250228-magic-seahorse-of-abracadabra-f2a402-mkl@xxxxxxxxxxxxxx
>

Understood, I will make the modifications in the next patch.

> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct can_berr_counter bec;
> > +     struct can_frame *cf;
> > +     struct sk_buff *skb;
> > +
> > +     skb = alloc_can_err_skb(ndev, &cf);
> > +
> > +     nct6694_can_get_berr_counter(ndev, &bec);
> > +
> > +     switch (new_state) {
> > +     case CAN_STATE_ERROR_ACTIVE:
> > +             priv->can.can_stats.error_warning++;
> > +             priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +             if (cf)
> > +                     cf->data[1] |= CAN_ERR_CRTL_ACTIVE;
> > +             break;
> > +     case CAN_STATE_ERROR_WARNING:
> > +             priv->can.can_stats.error_warning++;
> > +             priv->can.state = CAN_STATE_ERROR_WARNING;
> > +             if (cf) {
> > +                     cf->can_id |= CAN_ERR_CRTL;
> > +                     if (bec.txerr > bec.rxerr)
> > +                             cf->data[1] = CAN_ERR_CRTL_TX_WARNING;
> > +                     else
> > +                             cf->data[1] = CAN_ERR_CRTL_RX_WARNING;
> > +                     cf->data[6] = bec.txerr;
> > +                     cf->data[7] = bec.rxerr;
> > +             }
> > +             break;
> > +     case CAN_STATE_ERROR_PASSIVE:
> > +             priv->can.can_stats.error_passive++;
> > +             priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > +             if (cf) {
> > +                     cf->can_id |= CAN_ERR_CRTL;
> > +                     cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> > +                     if (bec.txerr >= CAN_ERROR_PASSIVE_THRESHOLD)
> > +                             cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> > +                     cf->data[6] = bec.txerr;
> > +                     cf->data[7] = bec.rxerr;
> > +             }
> > +             break;
> > +     case CAN_STATE_BUS_OFF:
> > +             priv->can.state = CAN_STATE_BUS_OFF;
> > +             priv->can.can_stats.bus_off++;
> > +             if (cf)
> > +                     cf->can_id |= CAN_ERR_BUSOFF;
>
> no need for can_free_echo_skb() and netif_stop_queue()
>

Fix these in v9.

> > +             can_free_echo_skb(ndev, 0, NULL);
> > +             netif_stop_queue(ndev);
>
> > +             can_bus_off(ndev);
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
>
> if (skb)
>

Add it in v9.

> > +     nct6694_can_rx_offload(&priv->offload, skb);
> > +}
> > +
> > +static void nct6694_can_handle_state_errors(struct net_device *ndev, u8 status)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > +     if (status == NCT6694_CAN_EVT_STS_ERROR_ACTIVE &&
> > +         priv->can.state != CAN_STATE_ERROR_ACTIVE) {
> > +             netdev_dbg(ndev, "Error, entered active state\n");
> > +             nct6694_can_handle_state_change(ndev, CAN_STATE_ERROR_ACTIVE);
> > +     }
> > +
> > +     if (status == NCT6694_CAN_EVT_STS_WARNING &&
> > +         priv->can.state != CAN_STATE_ERROR_WARNING) {
> > +             netdev_dbg(ndev, "Error, entered warning state\n");
> > +             nct6694_can_handle_state_change(ndev, CAN_STATE_ERROR_WARNING);
> > +     }
> > +
> > +     if (status == NCT6694_CAN_EVT_STS_ERROR_PASSIVE &&
> > +         priv->can.state != CAN_STATE_ERROR_PASSIVE) {
> > +             netdev_dbg(ndev, "Error, entered passive state\n");
> > +             nct6694_can_handle_state_change(ndev, CAN_STATE_ERROR_PASSIVE);
> > +     }
> > +
> > +     if (status == NCT6694_CAN_EVT_STS_BUS_OFF &&
> > +         priv->can.state != CAN_STATE_BUS_OFF) {
> > +             netdev_dbg(ndev, "Error, entered bus-off state\n");
> > +             nct6694_can_handle_state_change(ndev, CAN_STATE_BUS_OFF);
> > +     }
> > +}
> > +
> > +static void nct6694_can_handle_bus_err(struct net_device *ndev, u8 bus_err)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct can_frame *cf;
> > +     struct sk_buff *skb;
> > +
> > +     if (bus_err == NCT6694_CAN_EVT_ERR_NO_ERROR)
> > +             return;
> > +
> > +     priv->can.can_stats.bus_error++;
> > +
> > +     skb = alloc_can_err_skb(ndev, &cf);
>
> make it: "if (cf)" instead of "if (skb)" then interacting with the "cf".
>

Fix it in v9.

> > +     if (skb)
> > +             cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > +
> > +     switch (bus_err) {
> > +     case NCT6694_CAN_EVT_ERR_CRC_ERROR:
> > +             netdev_dbg(ndev, "CRC error\n");
> > +             ndev->stats.rx_errors++;
> > +             if (skb)
> > +                     cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
> > +             break;
> > +
> > +     case NCT6694_CAN_EVT_ERR_STUFF_ERROR:
> > +             netdev_dbg(ndev, "Stuff error\n");
> > +             ndev->stats.rx_errors++;
> > +             if (skb)
> > +                     cf->data[2] |= CAN_ERR_PROT_STUFF;
> > +             break;
> > +
> > +     case NCT6694_CAN_EVT_ERR_ACK_ERROR:
> > +             netdev_dbg(ndev, "Ack error\n");
> > +             ndev->stats.tx_errors++;
> > +             if (skb) {
> > +                     cf->can_id |= CAN_ERR_ACK;
> > +                     cf->data[2] |= CAN_ERR_PROT_TX;
> > +             }
> > +             break;
> > +
> > +     case NCT6694_CAN_EVT_ERR_FORM_ERROR:
> > +             netdev_dbg(ndev, "Form error\n");
> > +             ndev->stats.rx_errors++;
> > +             if (skb)
> > +                     cf->data[2] |= CAN_ERR_PROT_FORM;
> > +             break;
> > +
> > +     case NCT6694_CAN_EVT_ERR_BIT_ERROR:
> > +             netdev_dbg(ndev, "Bit error\n");
> > +             ndev->stats.tx_errors++;
> > +             if (skb)
> > +                     cf->data[2] |= CAN_ERR_PROT_TX | CAN_ERR_PROT_BIT;
> > +             break;
> > +
> > +     default:
> > +             break;
> > +     }
> > +
> > +     if (skb)
> > +             nct6694_can_rx_offload(&priv->offload, skb);
> > +}
> > +
> > +static void nct6694_can_tx_irq(struct net_device *ndev)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct net_device_stats *stats = &ndev->stats;
> > +
> > +     stats->tx_bytes += can_rx_offload_get_echo_skb_queue_tail(&priv->offload,
> > +                                                               0, NULL);
> > +     stats->tx_packets++;
> > +     netif_wake_queue(ndev);
> > +}
> > +
> > +static irqreturn_t nct6694_can_irq(int irq, void *data)
> > +{
> > +     struct net_device *ndev = data;
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct nct6694_can_event *event = &priv->event[ndev->dev_port];
> > +     const struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_EVENT,
> > +             .sel = NCT6694_CAN_EVENT_SEL(ndev->dev_port, NCT6694_CAN_EVENT_MASK),
> > +             .len = cpu_to_le16(sizeof(priv->event))
> > +     };
> > +     irqreturn_t handled = IRQ_NONE;
> > +     int ret;
> > +
> > +     ret = nct6694_read_msg(priv->nct6694, &cmd_hd, priv->event);
> > +     if (ret < 0)
> > +             return handled;
> > +
> > +     if (event->rx_evt & NCT6694_CAN_EVT_RX_DATA_IN) {
> > +             nct6694_can_rx(ndev, event->rx_evt);
>
> just for symmetry name rename nct6694_can_rx() to nct6694_can_handle_rx()
>

Fix it in v9.

> > +             handled = IRQ_HANDLED;
> > +     }
> > +
> > +     if (event->rx_evt & NCT6694_CAN_EVT_RX_DATA_LOST) {
> > +             nct6694_can_handle_lost_msg(ndev);
> > +             handled = IRQ_HANDLED;
> > +     }
> > +
> > +     if (event->status) {
> > +             nct6694_can_handle_state_errors(ndev, event->status);
>
> I think you should call nct6694_can_handle_state_change() directly and
> get rid of the code on nct6694_can_handle_state_errors().
>

Fix it in v9.

> > +             handled = IRQ_HANDLED;
> > +     }
> > +
> > +     if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) {
> > +             nct6694_can_handle_bus_err(ndev, event->err);
> > +             handled = IRQ_HANDLED;
> > +     }
>
> The logic needs to be refined. If there was an IRQ you must say you've
> handled it, event if user space doesn't care for bus errors.
>
> The better solution is to disabled event->err related IRQs in the
> hardware, if CAN_CTRLMODE_BERR_REPORTING is disabled, so you don't get
> an IRQ in the first place.
>
>         if (event->err != NCT6694_CAN_EVT_ERR_NO_ERROR) {
>                 if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
>                         nct6694_can_handle_bus_err(ndev, event->err);
>                 handled = IRQ_HANDLED;
>         }
>

Fix it in v9.

> > +
> > +     if (event->tx_evt & NCT6694_CAN_EVT_TX_FIFO_EMPTY) {
> > +             nct6694_can_tx_irq(ndev);
>
> just for symmetry name rename nct6694_can_tx() to nct6694_can_handle_tx()
>

Fix it in v9.

> > +             handled = IRQ_HANDLED;
> > +     }
> > +
> > +     if (handled)
> > +             can_rx_offload_threaded_irq_finish(&priv->offload);
> > +
> > +     priv->bec.rxerr = event->rec;
> > +     priv->bec.txerr = event->tec;
> > +
> > +     return handled;
> > +}
> > +
> > +static void nct6694_can_tx_work(struct work_struct *work)
> > +{
> > +     struct nct6694_can_priv *priv = container_of(work,
> > +                                                  struct nct6694_can_priv,
> > +                                                  tx_work);
> > +     struct nct6694_can_frame *frame = &priv->tx;
> > +     struct net_device *ndev = priv->ndev;
> > +     struct net_device_stats *stats = &ndev->stats;
> > +     struct sk_buff *skb = priv->can.echo_skb[0];
> > +     static const struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_DELIVER,
> > +             .sel = NCT6694_CAN_DELIVER_SEL(1),
> > +             .len = cpu_to_le16(sizeof(*frame))
> > +     };
> > +     u32 txid;
> > +     int err;
> > +
> > +     memset(frame, 0, sizeof(*frame));
> > +
> > +     frame->tag = NCT6694_CAN_FRAME_TAG(ndev->dev_port);
> > +
> > +     if (can_is_canfd_skb(skb)) {
> > +             struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > +
> > +             if (cfd->flags & CANFD_BRS)
> > +                     frame->flag |= NCT6694_CAN_FRAME_FLAG_BRS;
> > +
> > +             if (cfd->can_id & CAN_EFF_FLAG) {
> > +                     txid = cfd->can_id & CAN_EFF_MASK;
> > +                     frame->flag |= NCT6694_CAN_FRAME_FLAG_EFF;
> > +             } else {
> > +                     txid = cfd->can_id & CAN_SFF_MASK;
> > +             }
> > +             frame->flag |= NCT6694_CAN_FRAME_FLAG_FD;
> > +             frame->id = cpu_to_le32(txid);
> > +             frame->length = cfd->len;
>
> frame->length = canfd_sanitize_len(cfd->len);
>

Fix it in v9.

> > +
> > +             memcpy(frame->data, cfd->data, cfd->len);
>
> memcpy(frame->data, cfd->data, frame->length);
>

Fix it in v9.

> > +     } else {
> > +             struct can_frame *cf = (struct can_frame *)skb->data;
> > +
> > +             if (cf->can_id & CAN_EFF_FLAG) {
> > +                     txid = cf->can_id & CAN_EFF_MASK;
> > +                     frame->flag |= NCT6694_CAN_FRAME_FLAG_EFF;
> > +             } else {
> > +                     txid = cf->can_id & CAN_SFF_MASK;
> > +             }
> > +
> > +             if (cf->can_id & CAN_RTR_FLAG)
> > +                     frame->flag |= NCT6694_CAN_FRAME_FLAG_RTR;
> > +             else
> > +                     memcpy(frame->data, cf->data, cf->len);
> > +
> > +             frame->id = cpu_to_le32(txid);
> > +             frame->length = cf->len;
> > +     }
> > +
> > +     err = nct6694_write_msg(priv->nct6694, &cmd_hd, frame);
> > +     if (err) {
> > +             netdev_err(ndev, "%s: TX FIFO is full!\n", __func__);
>
> The FIFO should not be full at this time. There also could be an USB
> error due to unlug or EMI. I think you should remove this error message.
>

Fix it in v9.

> > +             can_free_echo_skb(ndev, 0, NULL);
> > +             stats->tx_dropped++;
> > +             stats->tx_errors++;
> > +             netif_wake_queue(ndev);
> > +     }
> > +}
> > +
> > +static netdev_tx_t nct6694_can_start_xmit(struct sk_buff *skb,
> > +                                       struct net_device *ndev)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > +     if (can_dev_dropped_skb(ndev, skb))
> > +             return NETDEV_TX_OK;
> > +
> > +     netif_stop_queue(ndev);
> > +     can_put_echo_skb(skb, ndev, 0, 0);
> > +     queue_work(priv->wq, &priv->tx_work);
> > +
> > +     return NETDEV_TX_OK;
> > +}
> > +
> > +static int nct6694_can_start(struct net_device *ndev)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     const struct can_bittiming *d_bt = &priv->can.data_bittiming;
> > +     const struct can_bittiming *n_bt = &priv->can.bittiming;
> > +     struct nct6694_can_setting *setting __free(kfree) = NULL;
> > +     const struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_SETTING,
> > +             .sel = ndev->dev_port,
> > +             .len = cpu_to_le16(sizeof(*setting))
> > +     };
> > +     int ret;
> > +
> > +     setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> > +     if (!setting)
> > +             return -ENOMEM;
> > +
> > +     setting->nbr = cpu_to_le32(n_bt->bitrate);
> > +     setting->dbr = cpu_to_le32(d_bt->bitrate);
> > +
> > +     if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> > +             setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_SETTING_CTRL1_MON);
> > +
> > +     if (priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
> > +             setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_SETTING_CTRL1_NISO);
> > +
> > +     if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> > +             setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_SETTING_CTRL1_LBCK);
> > +
> > +     ret = nct6694_write_msg(priv->nct6694, &cmd_hd, setting);
> > +     if (ret)
> > +             return ret;
> > +
> > +     priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > +     return 0;
> > +}
> > +
> > +static int nct6694_can_stop(struct net_device *ndev)
>
> static void
>

Fix it in v9.

> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > +     priv->can.ctrlmode = CAN_CTRLMODE_LISTENONLY;
>
> As Vincent already pointed out, bring you device into listen only mode.
> Please add a comment why you do it, because it cannot be stopped.
>

Okay! I will add the comment in the next patch.

> > +     netif_stop_queue(ndev);
> > +     free_irq(ndev->irq, ndev);
> > +     destroy_workqueue(priv->wq);
> > +     can_rx_offload_disable(&priv->offload);
> > +     priv->can.state = CAN_STATE_STOPPED;
> > +     close_candev(ndev);
> > +
> > +     return 0;
> > +}
> > +
> > +static int nct6694_can_set_mode(struct net_device *ndev, enum can_mode mode)
> > +{
> > +     int ret;
> > +
> > +     switch (mode) {
> > +     case CAN_MODE_START:
> > +             ret = nct6694_can_start(ndev);
>
> if (ret)
>         return ret;
>
> netif_wake_queue(ndev);
>

Fix it in v9.

> > +             if (!ret && netif_queue_stopped(ndev))
> > +                     netif_wake_queue(ndev);
> > +             break;
> > +
> > +     default:
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int nct6694_can_open(struct net_device *ndev)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     int ret;
> > +
> > +     ret = open_candev(ndev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     can_rx_offload_enable(&priv->offload);
> > +
> > +     ret = request_threaded_irq(ndev->irq, NULL,
> > +                                nct6694_can_irq, IRQF_ONESHOT,
> > +                                "nct6694_can", ndev);
> > +     if (ret) {
> > +             netdev_err(ndev, "Failed to request IRQ\n");
> > +             goto close_candev;
> > +     }
> > +
> > +     priv->wq = alloc_ordered_workqueue("%s-nct6694_wq",
> > +                                        WQ_FREEZABLE | WQ_MEM_RECLAIM,
> > +                                        ndev->name);
> > +     if (!priv->wq) {
> > +             ret = -ENOMEM;
> > +             goto free_irq;
> > +     }
> > +
> > +     ret = nct6694_can_start(ndev);
> > +     if (ret)
> > +             goto destroy_wq;
> > +
> > +     netif_start_queue(ndev);
> > +
> > +     return 0;
> > +
> > +destroy_wq:
> > +     destroy_workqueue(priv->wq);
> > +free_irq:
> > +     free_irq(ndev->irq, ndev);
> > +close_candev:
> > +     can_rx_offload_disable(&priv->offload);
> > +     close_candev(ndev);
> > +     return ret;
> > +}
> > +
> > +static const struct net_device_ops nct6694_can_netdev_ops = {
> > +     .ndo_open = nct6694_can_open,
> > +     .ndo_stop = nct6694_can_stop,
> > +     .ndo_start_xmit = nct6694_can_start_xmit,
> > +     .ndo_change_mtu = can_change_mtu,
> > +};
> > +
> > +static const struct ethtool_ops nct6694_can_ethtool_ops = {
> > +     .get_ts_info = ethtool_op_get_ts_info,
> > +};
> > +
> > +static int nct6694_can_get_clock(struct nct6694_can_priv *priv)
> > +{
> > +     struct nct6694_can_information *info __free(kfree) = NULL;
> > +     static const struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_INFORMATION,
> > +             .sel = NCT6694_CAN_INFORMATION_SEL,
> > +             .len = cpu_to_le16(sizeof(*info))
> > +     };
> > +     int ret, can_clk;
> > +
> > +     info = kzalloc(sizeof(*info), GFP_KERNEL);
> > +     if (!info)
> > +             return -ENOMEM;
> > +
> > +     ret = nct6694_read_msg(priv->nct6694, &cmd_hd, info);
> > +     if (ret)
> > +             return ret;
> > +
> > +     can_clk = le32_to_cpu(info->can_clk);
> > +
> > +     return can_clk;
> > +}
> > +
> > +static int nct6694_can_probe(struct platform_device *pdev)
> > +{
> > +     const struct mfd_cell *cell = mfd_get_cell(pdev);
> > +     struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
> > +     struct nct6694_can_priv *priv;
> > +     struct net_device *ndev;
> > +     int ret, irq, can_clk;
> > +
> > +     irq = irq_create_mapping(nct6694->domain,
> > +                              NCT6694_IRQ_CAN0 + cell->id);
> > +     if (!irq)
> > +             return irq;
> > +
> > +     ndev = alloc_candev(sizeof(struct nct6694_can_priv), 1);
> > +     if (!ndev)
> > +             return -ENOMEM;
> > +
> > +     ndev->irq = irq;
> > +     ndev->flags |= IFF_ECHO;
> > +     ndev->dev_port = cell->id;
> > +     ndev->netdev_ops = &nct6694_can_netdev_ops;
> > +     ndev->ethtool_ops = &nct6694_can_ethtool_ops;
> > +
> > +     priv = netdev_priv(ndev);
> > +     priv->nct6694 = nct6694;
> > +     priv->ndev = ndev;
> > +
> > +     can_clk = nct6694_can_get_clock(priv);
> > +     if (can_clk < 0) {
> > +             ret = dev_err_probe(&pdev->dev, can_clk,
> > +                                 "Failed to get clock\n");
> > +             goto free_candev;
> > +     }
> > +
> > +     INIT_WORK(&priv->tx_work, nct6694_can_tx_work);
> > +
> > +     priv->can.state = CAN_STATE_STOPPED;
>
> please remove CAN_STATE_STOPPED, it's not needed
>

Fix it in v9.

> > +     priv->can.clock.freq = can_clk;
> > +     priv->can.bittiming_const = &nct6694_can_bittiming_nominal_const;
> > +     priv->can.data_bittiming_const = &nct6694_can_bittiming_data_const;
> > +     priv->can.do_set_mode = nct6694_can_set_mode;
> > +     priv->can.do_get_berr_counter = nct6694_can_get_berr_counter;
> > +     priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> > +             CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_BERR_REPORTING |
> > +             CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO;
>
> Does your device run in CAN-FD mode all the time? If so, please use
> can_set_static_ctrlmode() to set it after priv->can.ctrlmode_supported
> and remove CAN_CTRLMODE_FD from ctrlmode_supported.
>

Our device is designed to allow users to dynamically switch between
Classical CAN and CAN-FD mode via ip link set ... fd on/off.
Therefore, CAN_CTRLMODE_FD needs to remain in ctrlmode_supported, and
can_set_static_ctrlmode() is not suitable in this case.
Please let me know if you have any concerns about this approach.

> > +
> > +     ret = can_rx_offload_add_manual(ndev, &priv->offload,
> > +                                     NCT6694_NAPI_WEIGHT);
> > +     if (ret) {
> > +             dev_err_probe(&pdev->dev, ret, "Failed to add rx_offload\n");
> > +             goto free_candev;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, priv);
> > +     SET_NETDEV_DEV(priv->ndev, &pdev->dev);
> > +
> > +     ret = register_candev(priv->ndev);
> > +     if (ret)
> > +             goto rx_offload_del;
> > +
> > +     return 0;
> > +
> > +rx_offload_del:
> > +     can_rx_offload_del(&priv->offload);
> > +free_candev:
> > +     free_candev(ndev);
> > +     return ret;
> > +}
> > +
> > +static void nct6694_can_remove(struct platform_device *pdev)
> > +{
> > +     struct nct6694_can_priv *priv = platform_get_drvdata(pdev);
> > +
> > +     unregister_candev(priv->ndev);
> > +     can_rx_offload_del(&priv->offload);
> > +     free_candev(priv->ndev);
> > +}
> > +
> > +static struct platform_driver nct6694_can_driver = {
> > +     .driver = {
> > +             .name   = DRVNAME,
> > +     },
> > +     .probe          = nct6694_can_probe,
> > +     .remove         = nct6694_can_remove,
> > +};
> > +
> > +module_platform_driver(nct6694_can_driver);
> > +
> > +MODULE_DESCRIPTION("USB-CAN FD driver for NCT6694");
> > +MODULE_AUTHOR("Ming Yu <tmyu0@xxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:nct6694-can");
>
> There should be no need for the module alias.
>

Remove it in v9.

Thanks,
Ming





[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux