From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755204Ab1ATIqi (ORCPT ); Thu, 20 Jan 2011 03:46:38 -0500 Received: from mail.perches.com ([173.55.12.10]:3213 "EHLO mail.perches.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754906Ab1ATIqg (ORCPT ); Thu, 20 Jan 2011 03:46:36 -0500 Subject: Re: [PATCH v2] net: add Faraday FTMAC100 10/100 Ethernet driver From: Joe Perches To: Po-Yu Chuang Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, ratbert@faraday-tech.com, bhutchings@solarflare.com, eric.dumazet@gmail.com, dilinger@queued.net In-Reply-To: References: <1294919372-1904-1-git-send-email-ratbert.chuang@gmail.com> <1295256060-2091-1-git-send-email-ratbert.chuang@gmail.com> <1295284788.21277.65.camel@Joe-Laptop> <1295455290.1952.22.camel@Joe-Laptop> Content-Type: text/plain; charset="UTF-8" Date: Thu, 20 Jan 2011 00:46:34 -0800 Message-ID: <1295513194.1952.96.camel@Joe-Laptop> Mime-Version: 1.0 X-Mailer: Evolution 2.30.3 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2011-01-20 at 13:30 +0800, Po-Yu Chuang wrote: > One more question: how to deal with this? Add a local variable for the > 2nd argument? > > ftmac100_set_receive_ring_base(priv, > priv->descs_dma_addr + offsetof(struct ftmac100_descs, rxdes)); That's one way, another is: ftmac100_set_receive_ring_base(priv, priv->descs_dma_addr + offsetof(struct ftmac100_descs, rxdes)); Another would be to change set_receive_ring_base to just pass priv and determine the dma address there: static void ftmac100_set_receive_ring_base(struct ftmac100 *priv) { dma_addr_t addr; addr = priv->descs_dma_addr + offsetof(struct ftmac100_descs, rxdes); iowrite32(addr, priv->base + FTMAC100_OFFSET_RXR_BADR); } ... ftmac100_set_receive_ring_base(priv); Another is not to be overly adherent to 80 columns. Pick one that suits you. You chose to use a lot of single use, single line functions with descriptive names that use iowrite32 or return some flag. I probably would have just used iowrite32 or tested the flag directly, but that's your choice and it's perfectly fine. There are a lot of coding choices that are readable and good. There isn't and shouldn't be some mandate for some specific code appearance before inclusion or acceptance. checkpatch is just a style guide. Ignore it and ignore me when you feel it's appropriate. I won't mind. cheers, Joe