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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 5425 bytes --]

This looks really good now. Some questions and a nitpick inline.

On 09.04.2025 16:27:49, a0282524688@gmail.com wrote:

[...]

> +static void nct6694_canfd_handle_state_change(struct net_device *ndev, u8 status)
> +{
> +	struct nct6694_canfd_priv *priv = netdev_priv(ndev);
> +	enum can_state new_state, rx_state, tx_state;
> +	struct can_berr_counter bec;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +
> +	nct6694_canfd_get_berr_counter(ndev, &bec);
> +	can_state_get_by_berr_counter(ndev, &bec, &tx_state, &rx_state);
> +
> +	new_state = max(tx_state, rx_state);
> +
> +	/* state hasn't changed */
> +	if (new_state == priv->can.state)
> +		return;
> +
> +	skb = alloc_can_err_skb(ndev, &cf);
> +
> +	can_change_state(ndev, cf, tx_state, rx_state);
> +
> +	if (new_state == CAN_STATE_BUS_OFF) {
> +		can_bus_off(ndev);

What does your device do when it goes into bus off? Does it recover itself?

> +	} else if (cf) {
> +		cf->can_id |= CAN_ERR_CNT;
> +		cf->data[6] = bec.txerr;
> +		cf->data[7] = bec.rxerr;
> +	}
> +
> +	if (skb)
> +		nct6694_canfd_rx_offload(&priv->offload, skb);
> +}
> +
> +static void nct6694_canfd_handle_bus_err(struct net_device *ndev, u8 bus_err)
> +{
> +	struct nct6694_canfd_priv *priv = netdev_priv(ndev);
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +
> +	if (bus_err == NCT6694_CANFD_EVT_ERR_NO_ERROR)
> +		return;

I think this has already been checked nct6694_canfd_irq()

> +
> +	priv->can.can_stats.bus_error++;
> +
> +	skb = alloc_can_err_skb(ndev, &cf);
> +	if (cf)
> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> +	switch (bus_err) {
> +	case NCT6694_CANFD_EVT_ERR_CRC_ERROR:
> +		netdev_dbg(ndev, "CRC error\n");
> +		ndev->stats.rx_errors++;
> +		if (cf)
> +			cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
> +		break;
> +
> +	case NCT6694_CANFD_EVT_ERR_STUFF_ERROR:
> +		netdev_dbg(ndev, "Stuff error\n");
> +		ndev->stats.rx_errors++;
> +		if (cf)
> +			cf->data[2] |= CAN_ERR_PROT_STUFF;
> +		break;
> +
> +	case NCT6694_CANFD_EVT_ERR_ACK_ERROR:
> +		netdev_dbg(ndev, "Ack error\n");
> +		ndev->stats.tx_errors++;
> +		if (cf) {
> +			cf->can_id |= CAN_ERR_ACK;
> +			cf->data[2] |= CAN_ERR_PROT_TX;
> +		}
> +		break;
> +
> +	case NCT6694_CANFD_EVT_ERR_FORM_ERROR:
> +		netdev_dbg(ndev, "Form error\n");
> +		ndev->stats.rx_errors++;
> +		if (cf)
> +			cf->data[2] |= CAN_ERR_PROT_FORM;
> +		break;
> +
> +	case NCT6694_CANFD_EVT_ERR_BIT_ERROR:
> +		netdev_dbg(ndev, "Bit error\n");
> +		ndev->stats.tx_errors++;
> +		if (cf)
> +			cf->data[2] |= CAN_ERR_PROT_TX | CAN_ERR_PROT_BIT;
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	if (skb)
> +		nct6694_canfd_rx_offload(&priv->offload, skb);
> +}

[...]

> +static int nct6694_canfd_start(struct net_device *ndev)
> +{
> +	struct nct6694_canfd_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_canfd_setting *setting __free(kfree) = NULL;
> +	const struct nct6694_cmd_header cmd_hd = {
> +		.mod = NCT6694_CANFD_MOD,
> +		.cmd = NCT6694_CANFD_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_CANFD_SETTING_CTRL1_MON);
> +
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
> +		setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_NISO);
> +
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> +		setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_LBCK);
> +
> +	setting->nbtp = cpu_to_le32(FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NSJW,
> +					       n_bt->sjw - 1) |
> +				    FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NBRP,
> +					       n_bt->brp - 1) |
> +				    FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NTSEG2,
> +					       n_bt->phase_seg2 - 1) |
> +				    FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NTSEG1,
> +					       n_bt->prop_seg + n_bt->phase_seg1 - 1));
> +
> +	setting->dbtp = cpu_to_le32(FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DSJW,
> +					       d_bt->sjw - 1) |
> +				    FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DBRP,
> +					       d_bt->brp - 1) |
> +				    FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG2,
> +					       d_bt->phase_seg2 - 1) |
> +				    FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG1,
> +					       d_bt->prop_seg + d_bt->phase_seg1 - 1));

What does your device do, if you set the bitrates _and_ the bit timing
parameters? They are redundant.

> +	setting->active = NCT6694_CANFD_SETTING_ACTIVE_CTRL1 |
> +			  NCT6694_CANFD_SETTING_ACTIVE_NBTP_DBTP;
> +
> +	ret = nct6694_write_msg(priv->nct6694, &cmd_hd, setting);
> +	if (ret)
> +		return ret;
> +
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> +	return 0;
> +}

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]