I was about to convert these macros into static inline bool functions
and thinking the naming can be improved a bit:
On 22.03.2022 00:32:30, Pavel Pisa wrote:
> +#define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF, ctucan_read32(priv, CTUCANFD_STATUS)))
> +#define CTU_CAN_FD_ENABLED(priv) (!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE)))
[...]
The common prefix for functions seems to be "ctucan_", so if I convert
CTU_CAN_FD_ENABLED -> ctucan_fd_enabled() the unfamiliar reader might
think this functions tests if the controller is in FD mode.
As far as I understand the code, the test is if the controller is
enabled at all. This is done via the ctucan_chip_start() function and
undone via ctucan_chip_stop(). So a better function names might be:
- ctucan_chip_started()
- ctucan_chip_is_started()
- ctucan_chip_enabled()
- ctucan_chip_is_enabled()
> +/**
> + * ctucan_set_btr() - Sets CAN bus bit timing in CTU CAN FD
> + * @ndev: Pointer to net_device structure
> + * @bt: Pointer to Bit timing structure
> + * @nominal: True - Nominal bit timing, False - Data bit timing
> + *
> + * Return: 0 - OK, -%EPERM if controller is enabled
> + */
> +static int ctucan_set_btr(struct net_device *ndev, struct can_bittiming *bt, bool nominal)
> +{
> + struct ctucan_priv *priv = netdev_priv(ndev);
> + int max_ph1_len = 31;
> + u32 btr = 0;
> + u32 prop_seg = bt->prop_seg;
> + u32 phase_seg1 = bt->phase_seg1;
> +
> + if (CTU_CAN_FD_ENABLED(priv)) {
> + netdev_err(ndev, "BUG! Cannot set bittiming - CAN is enabled\n");
I would want to replace "CAN" with "Chip" or "Controller".
> + return -EPERM;
> + }
> +
> + if (nominal)
> + max_ph1_len = 63;
> +
> + /* The timing calculation functions have only constraints on tseg1, which is prop_seg +
> + * phase1_seg combined. tseg1 is then split in half and stored into prog_seg and phase_seg1.
> + * In CTU CAN FD, PROP is 6/7 bits wide but PH1 only 6/5, so we must re-distribute the
> + * values here.
> + */
> + if (phase_seg1 > max_ph1_len) {
> + prop_seg += phase_seg1 - max_ph1_len;
> + phase_seg1 = max_ph1_len;
> + bt->prop_seg = prop_seg;
> + bt->phase_seg1 = phase_seg1;
> + }
> +
> + if (nominal) {
> + btr = FIELD_PREP(REG_BTR_PROP, prop_seg);
> + btr |= FIELD_PREP(REG_BTR_PH1, phase_seg1);
> + btr |= FIELD_PREP(REG_BTR_PH2, bt->phase_seg2);
> + btr |= FIELD_PREP(REG_BTR_BRP, bt->brp);
> + btr |= FIELD_PREP(REG_BTR_SJW, bt->sjw);
> +
> + ctucan_write32(priv, CTUCANFD_BTR, btr);
> + } else {
> + btr = FIELD_PREP(REG_BTR_FD_PROP_FD, prop_seg);
> + btr |= FIELD_PREP(REG_BTR_FD_PH1_FD, phase_seg1);
> + btr |= FIELD_PREP(REG_BTR_FD_PH2_FD, bt->phase_seg2);
> + btr |= FIELD_PREP(REG_BTR_FD_BRP_FD, bt->brp);
> + btr |= FIELD_PREP(REG_BTR_FD_SJW_FD, bt->sjw);
> +
> + ctucan_write32(priv, CTUCANFD_BTR_FD, btr);
> + }
> +
> + return 0;
> +}
[...]
> +/**
> + * ctucan_start_xmit() - Starts the transmission
> + * @skb: sk_buff pointer that contains data to be Txed
> + * @ndev: Pointer to net_device structure
> + *
> + * Invoked from upper layers to initiate transmission. Uses the next available free TXT Buffer and
> + * populates its fields to start the transmission.
> + *
> + * Return: %NETDEV_TX_OK on success, %NETDEV_TX_BUSY when no free TXT buffer is available,
> + * negative return values reserved for error cases
> + */
> +static netdev_tx_t ctucan_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> + struct ctucan_priv *priv = netdev_priv(ndev);
> + struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> + u32 txtb_id;
> + bool ok;
> + unsigned long flags;
> +
> + if (can_dropped_invalid_skb(ndev, skb))
> + return NETDEV_TX_OK;
> +
> + if (unlikely(!CTU_CAN_FD_TXTNF(priv))) {
I'm also looking for nicer names for "CTU_CAN_FD_TXTNF".
What about ctucan_txt_buffer_full() and reverse the logic?
> + netif_stop_queue(ndev);
> + netdev_err(ndev, "BUG!, no TXB free when queue awake!\n");
> + return NETDEV_TX_BUSY;
> + }
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
