Dear Vincent,

Thank you for reviewing,

Vincent Mailhol [off-list ref] 於 2025年2月27日 週四 上午10:09寫道：

...

quoted

+static void nct6694_can_handle_state_change(struct net_device *ndev,
+                                         enum can_state new_state)
+{
+     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)

Set the CAN_ER_CRTL flag:

                if (cf) {
                        cf->can_id |= CAN_ERR_CRTL;
                        cf->data[1] |= CAN_ERR_CRTL_ACTIVE;
                }

Fix it in the v9.

quoted

+                     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;

Set the CAN_ERR_CNT flag when you populate cf->data[6] and cf->data[7]:

                        cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;

Fix it in the v9.

quoted

+                     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;

Set the CAN_ERR_CNT flag when you populate cf->data[6] and cf->data[7]:

                        cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;

Fix it in the v9.

quoted

+                     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;
+             can_free_echo_skb(ndev, 0, NULL);
+             netif_stop_queue(ndev);> +              can_bus_off(ndev);
+             break;
+     default:
+             break;
+     }
+
+     nct6694_can_rx_offload(&priv->offload, skb);
+}
+

...

quoted

+static int nct6694_can_stop(struct net_device *ndev)
+{
+     struct nct6694_can_priv *priv = netdev_priv(ndev);
+
+     priv->can.ctrlmode = CAN_CTRLMODE_LISTENONLY;

Hmmm, when Marc asked you to put the device in listen only mode, I think
he meant that you set it on the device side (i.e. flag
NCT6694_CAN_SETTING_CTRL1_MON) and not on the driver side. If you set
CAN_CTRLMODE_LISTENONLY flag, that will be reported in the netlink
interface. So you should not change that flag.

But before that, did you check the datasheet? Don't you have a device
flag to actually turn the device off (e.g. sleep mode)?

Our firmware currently does not provide an interface to turn the
device off, I will put the device in listen-only mode as an
alternative.

quoted

+     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;
+}
+

...

quoted

+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;

Marc asked you to remove this line during the v7 review.

Sorry, drop it in the v9.

quoted

+     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;
+
+     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;
+}
+

Best regards,
Ming