Re: [PATCH v8 3/7] can: ctucanfd: add support for CTU CAN FD open-source IP core - bus independent part. - Marc Kleine-Budde

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Pavel Pisa <pisa@cmp.felk.cvut.cz>
Cc: linux-can@vger.kernel.org,
	Marin Jerabek <martin.jerabek01@gmail.com>,
	Ondrej Ille <ondrej.ille@gmail.com>,
	Jiri Novak <jnovak@fel.cvut.cz>,
	Jaroslav Beran <jara.beran@gmail.com>,
	Petr Porazil <porazil@pikron.com>, Pavel Machek <pavel@ucw.cz>
Subject: Re: [PATCH v8 3/7] can: ctucanfd: add support for CTU CAN FD open-source IP core - bus independent part.
Date: Wed, 10 Aug 2022 22:00:43 +0200	[thread overview]
Message-ID: <20220810200043.pcpkbiq7f47cmi6t@pengutronix.de> (raw)
In-Reply-To: <1906e4941560ae2ce4b8d181131fd4963aa31611.1647904780.git.pisa@cmp.felk.cvut.cz>

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]