All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg KH <greg@kroah.com>
To: "Mark A. Allyn" <mark.a.allyn@intel.com>
Cc: linux-kernel@vger.kernel.org, alan@linux.intel.com,
	jayant.mangalampalli@intel.com, venkat.r.gokulrangan@intel.com
Subject: Re: Re-send (What else needs to be done to the sep driver (staging/sep))
Date: Mon, 25 Apr 2011 17:40:05 -0700	[thread overview]
Message-ID: <20110426004005.GA29534@kroah.com> (raw)
In-Reply-To: <alpine.DEB.2.00.1104131427260.31147@allyn-test>

On Wed, Apr 13, 2011 at 02:29:35PM -0700, Mark A. Allyn wrote:
> Sorry, I had an incorrect return address config in alpine. . .
> 
> What else needs to be done to the sep driver in order for it to be
> moved to the kernel from staging?

Some things at first glance:
	- you have a lot of ioctls, do you really need them all?
	- your ioctls use structures with very "generic" names, please
	  prefix them with "sep_" as you are joining the global
	  namespace here.
	- sep_driver_api.h seems to have a lot of information in it that
	  doesn't need to be there (i.e. move it to a private .h file.)
	- is there documentation for how to use this device through the
	  ioctls anywhere?
	- are you sure your ioctl magic number isn't already reserved by
	  some other driver?
	- the structures you use for the ioctls, shouldn't they use the
	  correct "__" prefixes on their type?  How about 64/32 bit
	  thunking layer, isn't that needed?
	- you have a number of basic checkpatch formatting issues to fix
	  up, please do so.
	- do you really even need all of the .h files you have?  Can't
	  they just go into the .c file?
	- sep_wait_sram_write() has no way to abort, if the hardware
	  hangs, you just locked up your kernel :(

that's good for a first round of review, right?

thanks,

greg k-h

      parent reply	other threads:[~2011-04-26  0:45 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-13 21:29 Re-send (What else needs to be done to the sep driver (staging/sep)) Mark A. Allyn
2011-04-13 21:56 ` Randy Dunlap
2011-04-13 22:22   ` Mark A. Allyn
2011-04-13 22:46     ` Greg KH
2011-04-13 23:41     ` Joe Perches
2011-04-13 21:56 ` Joe Perches
2011-04-13 22:23   ` Greg KH
2011-04-13 22:30     ` Joe Perches
2011-04-13 22:46       ` Greg KH
2011-04-13 23:12         ` Joe Perches
2011-04-14 14:46           ` Allyn, Mark A
2011-04-14 14:55             ` Alan Cox
2011-04-14 14:57               ` Allyn, Mark A
2011-04-26  0:40 ` Greg KH [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=20110426004005.GA29534@kroah.com \
    --to=greg@kroah.com \
    --cc=alan@linux.intel.com \
    --cc=jayant.mangalampalli@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.a.allyn@intel.com \
    --cc=venkat.r.gokulrangan@intel.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.