From: santosh.shilimkar@ti•com (Santosh Shilimkar)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 2/2] net: Add Keystone NetCP ethernet driver
Date: Thu, 24 Apr 2014 17:04:48 -0400 [thread overview]
Message-ID: <53597C70.1040501@ti.com> (raw)
In-Reply-To: <20140424.124744.1296959126932595844.davem@davemloft.net>
On Thursday 24 April 2014 12:47 PM, David Miller wrote:
> From: Santosh Shilimkar <santosh.shilimkar@ti•com>
> 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
prev parent reply other threads:[~2014-04-24 21:04 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-04-22 21:21 [PATCH 0/2] net: Add Keystone NetCP ethernet driver support Santosh Shilimkar
2014-04-22 21:21 ` [PATCH 1/2] Documentation: dt: net: Add binding doc for Keystone NetCP ethernet driver Santosh Shilimkar
2014-04-22 22:02 ` [PATCH 0/2] net: Add Keystone NetCP ethernet driver support Stephen Hemminger
2014-04-22 23:56 ` Santosh Shilimkar
[not found] ` <1398201675-17379-3-git-send-email-santosh.shilimkar@ti.com>
2014-04-24 16:47 ` [PATCH 2/2] net: Add Keystone NetCP ethernet driver David Miller
2014-04-24 21:04 ` Santosh Shilimkar [this message]
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=53597C70.1040501@ti.com \
--to=santosh.shilimkar@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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