From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751957AbbKJWfE (ORCPT <rfc822;w@1wt.eu>);
	Tue, 10 Nov 2015 17:35:04 -0500
Received: from unicorn.mansr.com ([81.2.72.234]:43305 "EHLO unicorn.mansr.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751031AbbKJWfC (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 10 Nov 2015 17:35:02 -0500
From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= <mans@mansr.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>, slash.tmp@free.fr
Subject: Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
References: <1447172063-27234-1-git-send-email-mans@mansr.com>
	<CAHp75Vch3vJEqYsCRUoctEmF0EoFx+0y7ZO+LjmdpQKAoikiOA@mail.gmail.com>
Date: Tue, 10 Nov 2015 22:34:53 +0000
In-Reply-To: <CAHp75Vch3vJEqYsCRUoctEmF0EoFx+0y7ZO+LjmdpQKAoikiOA@mail.gmail.com>
	(Andy Shevchenko's message of "Wed, 11 Nov 2015 00:09:23 +0200")
Message-ID: <yw1xr3jxpjoy.fsf@unicorn.mansr.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Andy Shevchenko <andy.shevchenko@gmail.com> 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@mansr.com