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
prev 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.