Dear Vincent,
Thank you for reviewing,
Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> 於 2025年2月27日 週四 上午10:09寫道：
>
...
> > +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.
> > + 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.
> > + 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.
> > + 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);
> > +}
> > +
...
> > +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.
> > + 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_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.
> > + 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
