Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Andy Shevchenko <andy.shevchenko@xxxxxxxxx> writes:

>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
>> +                               u32 mask, u32 val)
>> +{
>> +       u32 old = nb8800_readb(priv, reg);
>> +       u32 new = (old & ~mask) | val;
>
> Shoudn't be "â | (val & mask);" ?

No, it's meant to replace the bits in mask with the corresponding bits
from val.

> + empty line?
>
>> +       if (new != old)
>> +               nb8800_writeb(priv, reg, new);
>> +}
>> +

[...]

>> +static void nb8800_receive(struct net_device *dev, int i, int len)
>
> unsigned int i ?
> len as well?

Does it matter?  The values are nowhere near overflowing signed int.


[...]

>> +               /* If a packet arrived after we last checked but
>> +                * before writing RX_ITR, the interrupt will be
>> +                * delayed, so we retrieve it now. */
>
> Block comments usually
> /*
>  * text
>  */

Documentation/CodingStyle says net/ and drivers/net/ are special, though
currently a mix of styles can be found.  Personally, I don't
particularly care.

> Can be longer lines?

Still won't fit on two lines.

>> +               if (priv->rx_descs[next].report)
>> +                       goto again;
>> +
>> +               napi_complete_done(napi, work);
>> +       }
>> +
>> +       return work;
>> +}
>> +
>> +static void nb8800_tx_dma_start(struct net_device *dev)
>> +{
>> +       struct nb8800_priv *priv = netdev_priv(dev);
>> +       struct nb8800_tx_buf *txb;
>> +       u32 txc_cr;
>> +
>> +       txb = &priv->tx_bufs[priv->tx_queue];
>> +       if (!txb->ready)
>> +               return;
>> +
>> +       txc_cr = nb8800_readl(priv, NB8800_TXC_CR);
>> +       if (txc_cr & TCR_EN)
>> +               return;
>> +
>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>> +       wmb();          /* ensure desc addr is written before starting DMA */
>
> Hmâ Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?

Possibly.

>> +       nb8800_writel(priv, NB8800_TXC_CR, txc_cr | TCR_EN);
>> +
>> +       priv->tx_queue = (priv->tx_queue + txb->chain_len) % TX_DESC_COUNT;
>> +}

-- 
MÃns RullgÃrd
mans@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/