Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S968212AbXEHObT (ORCPT ); Tue, 8 May 2007 10:31:19 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S968080AbXEHObR (ORCPT ); Tue, 8 May 2007 10:31:17 -0400 Received: from khc.piap.pl ([195.187.100.11]:55951 "EHLO khc.piap.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S968127AbXEHObO (ORCPT ); Tue, 8 May 2007 10:31:14 -0400 To: Lennert Buytenhek Cc: Michael-Luke Jones , Jeff Garzik , netdev@vger.kernel.org, lkml , Russell King , ARM Linux Mailing List Subject: Re: [PATCH] Intel IXP4xx network drivers v.2 - Ethernet and HSS References: <5BB7E1AB-5CE1-43C8-8CE3-E0DE0236BD09@cam.ac.uk> <86D26EBE-5899-468F-9C79-23E83E0DE04B@cam.ac.uk> <20070508113735.GB32055@xi.wantstofly.org> From: Krzysztof Halasa Date: Tue, 08 May 2007 16:31:12 +0200 In-Reply-To: <20070508113735.GB32055@xi.wantstofly.org> (Lennert Buytenhek's message of "Tue, 8 May 2007 13:37:35 +0200") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2089 Lines: 57 Lennert Buytenhek writes: >> +/* Built-in 10/100 Ethernet MAC interfaces */ >> +static struct mac_plat_info ixdp425_plat_mac[] = { >> + { >> + .phy = 0, >> + .rxq = 3, >> + }, { >> + .phy = 1, >> + .rxq = 4, >> + } >> +}; > > As with Christian's driver (I'm feeling like a bit of a broken record > here :-), putting knowledge of which queue to use (which is firmware- > specific) in the _board_ support file is almost certainly wrong. > > I would just put the port number in there, and let the ethernet > driver map the port number to the hardware queue number. After all, > the ethernet driver knows which queues the firmware uses, while the > board support code doesn't. No, quite the opposite. The board code knows its set of hardware interfaces etc. and can let Ethernet driver use, say, HSS queues. The driver can't know that. It would make sense if we had many queues, but it doesn't seem the case (perhaps the upper queues could be used for some purposes, but Intel's code doesn't use them either and they probably know better). >> +static inline void set_regbits(u32 bits, u32 __iomem *reg) >> +{ >> + __raw_writel(__raw_readl(reg) | bits, reg); >> +} >> +static inline void clr_regbits(u32 bits, u32 __iomem *reg) >> +{ >> + __raw_writel(__raw_readl(reg) & ~bits, reg); >> +} > > I generally discourage the use of such wrappers, as it often makes > people forget that the set and clear operations are not atomic, and > it ignores the fact that some of the other bits in the register you > are modifying might have side-effects. Without them the code in question is hardly readable, I pick the need to remember about non-atomicity and possible side effects instead :-) I've outlined the current versions in a separate mail, generally 2 patches are marked "v.2" and one "v.3". -- Krzysztof Halasa - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/