Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754276AbaDXVFW (ORCPT ); Thu, 24 Apr 2014 17:05:22 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:36920 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753208AbaDXVFP (ORCPT ); Thu, 24 Apr 2014 17:05:15 -0400 Message-ID: <53597C70.1040501@ti.com> Date: Thu, 24 Apr 2014 17:04:48 -0400 From: Santosh Shilimkar User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 MIME-Version: 1.0 To: David Miller CC: "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "devicetree@vger.kernel.org" , "Nair, Sandeep" , "robh+dt@kernel.org" , "grant.likely@linaro.org" Subject: Re: [PATCH 2/2] net: Add Keystone NetCP ethernet driver References: <1398201675-17379-1-git-send-email-santosh.shilimkar@ti.com> <1398201675-17379-3-git-send-email-santosh.shilimkar@ti.com> <20140424.124744.1296959126932595844.davem@davemloft.net> In-Reply-To: <20140424.124744.1296959126932595844.davem@davemloft.net> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday 24 April 2014 12:47 PM, David Miller wrote: > From: Santosh Shilimkar > Date: Tue, 22 Apr 2014 17:21:15 -0400 > >> +struct netcp_tx_pipe { >> + struct netcp_device *netcp_device; >> + void *dma_queue; > > Indent *dma_queue the same as the other struct members. > sure >> + unsigned dma_queue_id; > > Use explicit "unsigned int". > >> + unsigned dma_chan_id; > > Likewise. > ok >> +struct netcp_addr { >> + struct netcp_intf *netcp; >> + unsigned char addr[MAX_ADDR_LEN]; > > If this is just an ethernet driver, ETH_ALEN is more appropriate here. > Yep. Will use ETH_ALEN >> + unsigned tx_compl_qid; > > Explicit "unsigned int" please. I'm not going to point out all of the other > instances, audit your entire submission for this problem please. > Thats true... Will fix all instances of those. >> +static inline u32 *netcp_push_psdata(struct netcp_packet *p_info, >> + unsigned bytes) >> +{ >> + u32 *buf; >> + unsigned words; > > Do not use tabs between the type and the variable name in local variable > declarations. > ok > Please audit for and fix this in your entire submission. > Will Do. >> +static inline u32 hwval_to_host(bool big_endian, u32 hwval) >> +{ >> + if (big_endian) >> + return be32_to_cpu(hwval); >> + else >> + return le32_to_cpu(hwval); >> +} > > You're much better off having a set of methods, one for big endian and > one for little endian, that just straight line codes the appropriate endian > accesses. > good idea. > These conditionals peppered all over the place are just ugly. > Agree. Will try to fit into some logical functions. Thanks for the review David !! regards, Santosh -- 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/