netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Miller <davem@davemloft.net>
To: romieu@fr.zoreil.com
Cc: balaji.v@thotakaa.com, netdev@vger.kernel.org, mirqus@gmail.com,
	mohan@thotakaa.com, blue.cube@seznam.cz,
	lanconelli.claudio@eptar.com, sriram@thotakaa.com,
	vbalaji.acs@gmail.com
Subject: Re: [PATCH]netdev: add driver for enc424j600 ethernet chip on SPI bus
Date: Sun, 30 Jan 2011 04:01:00 -0800 (PST)	[thread overview]
Message-ID: <20110130.040100.48496301.davem@davemloft.net> (raw)
In-Reply-To: <20110130115516.GA8392@electric-eye.fr.zoreil.com>

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sun, 30 Jan 2011 12:55:16 +0100

> Balaji Venkatachalam <balaji.v@thotakaa.com> :
>> From: Balaji Venkatachalam <balaji.v@thotakaa.com>
>> Updated patch [V1.29] for Microchip enc424j600 ethernet chip controlled via SPI.
>> 
>> I tested it on my custom board with ARM9 (Freescale i.MX233) with
>> Kernel 2.6.31.14.
> [...]
>> 4. mapped enc424j600_dump_regs to eth_ops get_regs
> 
> There is some kind of misunderstanding.
> 
> See include/linux/ethtool.h
> [...]
> struct ethtool_ops {
>         int     (*get_settings)(struct net_device *, struct ethtool_cmd *);
>         int     (*set_settings)(struct net_device *, struct ethtool_cmd *);
>         void    (*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
>         int     (*get_regs_len)(struct net_device *);
>         void    (*get_regs)(struct net_device *, struct ethtool_regs *, void *);
> 
> It should not even compile.

Balaji, please take care of your changes and do not rush this review
process like you seem to be doing.

If you're not even watching for compiler warnings and errors, I can
expect that you did exaclty zero functional testing of the changed
code.

That's simply unacceptable.

  reply	other threads:[~2011-01-30 12:00 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-06  6:43 [PATCH]netdev: add driver for enc424j600 ethernet chip on SPI bus Balaji Venkatachalam
2011-01-06 14:56 ` Michał Mirosław
2011-01-09  9:00   ` Balaji Venkatachalam
2011-01-16 10:51     ` Balaji Venkatachalam
2011-01-24 23:04       ` David Miller
2011-01-29  8:24         ` Balaji Venkatachalam
2011-01-30 10:14           ` Balaji Venkatachalam
2011-01-30 11:55             ` Francois Romieu
2011-01-30 12:01               ` David Miller [this message]
     [not found]                 ` <AANLkTimC3bsvT24cz4A+iJfjgL3LxBWxKzb182ocqbiL@mail.gmail.com>
2011-01-30 12:34                   ` Balaji Venkatachalam
2011-02-08 14:48                     ` Balaji Venkatachalam
2011-02-23  6:00                       ` Balaji Venkatachalam
  -- strict thread matches above, loose matches on Subject: below --
2011-01-29  8:03 Balaji Venkatachalam
2011-01-29 11:58 ` Francois Romieu

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=20110130.040100.48496301.davem@davemloft.net \
    --to=davem@davemloft.net \
    --cc=balaji.v@thotakaa.com \
    --cc=blue.cube@seznam.cz \
    --cc=lanconelli.claudio@eptar.com \
    --cc=mirqus@gmail.com \
    --cc=mohan@thotakaa.com \
    --cc=netdev@vger.kernel.org \
    --cc=romieu@fr.zoreil.com \
    --cc=sriram@thotakaa.com \
    --cc=vbalaji.acs@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).