Messages in this thread | | | Date | Thu, 24 Apr 2014 17:04:48 -0400 | From | Santosh Shilimkar <> | Subject | Re: [PATCH 2/2] net: Add Keystone NetCP ethernet driver |
| |
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
| |