From: Daniel Silverstone <dsilvers@simtec.co.uk>
To: "Figo.zhang" <figo1802@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
netdev@vger.kernel.org, Vincent Sanders <vince@simtec.co.uk>,
Ben Dooks <ben@simtec.co.uk>
Subject: Re: [PATCH]NET/KS8695: add support NAPI for Rx
Date: Mon, 26 Oct 2009 16:27:21 +0000 [thread overview]
Message-ID: <20091026162721.GD9480@digital-scurf.org> (raw)
In-Reply-To: <1256572828.2148.5.camel@myhost>
On Tue, Oct 27, 2009 at 12:00:28AM +0800, Figo.zhang wrote:
> +#ifdef KS8695NET_NAPI
> +static irqreturn_t
> +ks8695_rx_irq(int irq, void *dev_id)
This routine lacks its documentation comment. This driver is fully documented
in order to serve as a good example for others. Indeed this lack of
documentation comments continues through your patch, I won't bring up each
instance, instead trusting you to go back over your patch and sort them out.
> + status = __raw_readl(KS8695_IRQ_VA + KS8695_INTST);
[snip]
> + __raw_writel(status | mask_bit , KS8695_IRQ_VA + KS8695_INTST);
[snip]
> + __raw_writel(status , KS8695_IRQ_VA + KS8695_INTEN);
[snip]
> + unsigned long isr = __raw_readl(KS8695_IRQ_VA + KS8695_INTEN);
[snip]
> + __raw_writel(isr | mask_bit, KS8695_IRQ_VA + KS8695_INTEN);
Please don't use __raw_readl or __raw_writel. This driver was nice and clean,
don't ruin it.
Also, as an aside, you seem to add a spinlock (rx_lock) which afaict is only
used by NAPI related routines, and yet you include it regardless of NAPI being
enabled or not. Did I misread your patch, or is this an oversight?
Regards,
Daniel.
--
Daniel Silverstone http://www.simtec.co.uk/
next prev parent reply other threads:[~2009-10-26 16:46 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-10-26 16:00 [PATCH]NET/KS8695: add support NAPI for Rx Figo.zhang
2009-10-26 16:27 ` Daniel Silverstone [this message]
2009-10-26 16:49 ` Ben Hutchings
2009-10-26 22:43 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20091026162721.GD9480@digital-scurf.org \
--to=dsilvers@simtec.co.uk \
--cc=ben@simtec.co.uk \
--cc=davem@davemloft.net \
--cc=figo1802@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=vince@simtec.co.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).