On Wed, Aug 04, 2021 at 11:18:58AM +0200, Marc Kleine-Budde wrote:
> >
> > - 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.
> > /* 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!
> >
> > 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
