On Wed, Aug 04, 2021 at 11:18:58AM +0200, Marc Kleine-Budde wrote:

quoted

 
-	cdev->ops->write_fifo(cdev, addr_offset, val);
+	result = cdev->ops->write_fifo(cdev, addr_offset, val, val_count);
+	WARN_ON(result != 0);

What about converting all read/write functions to return an error, and
handle the error in the caller?

Yeah, that would be cleaner.

quoted

 	/* acknowledge rx fifo 0 */

@@ -1546,8 +1548,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	struct net_device *dev = cdev->net;
 	struct sk_buff *skb = cdev->tx_skb;
 	u32 id, cccr, fdflags;
-	int i;
 	int putidx;
+	u32 id_and_dlc[2];

Can you create a struct for this?

Ditto, sure!

quoted

 
 	cdev->tx_skb = NULL;
 

@@ -1563,18 +1565,16 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 	if (cf->can_id & CAN_RTR_FLAG)
 		id |= TX_BUF_RTR;
 
+	id_and_dlc[0] = id;
+
 	if (cdev->version == 30) {
 		netif_stop_queue(dev);
 
-		/* message ram configuration */
-		m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, id);
-		m_can_fifo_write(cdev, 0, M_CAN_FIFO_DLC,
-				 can_fd_len2dlc(cf->len) << 16);
+		id_and_dlc[1] = can_fd_len2dlc(cf->len) << 16;
 
-		for (i = 0; i < cf->len; i += 4)
-			m_can_fifo_write(cdev, 0,
-					 M_CAN_FIFO_DATA(i / 4),
-					 *(u32 *)(cf->data + i));
+		/* Write the frame ID, DLC, and payload to the FIFO element. */
+		m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, id_and_dlc, ARRAY_SIZE(id_and_dlc));
+		m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA, cf->data, DIV_ROUND_UP(cf->len, 4));

Does it make sense to combine these, too? Same for the v3.1 variant.

I think that's the eventual goal, but since the ID, DLC, and frame data would
have to be contiguous for a single m_can_fifo_write(), you'd end up copying
things around. I wanted to start with this smaller, simpler patch first.
Is that alright?

I'll try to send a v3 up shortly.

Best,
Matt