Messages in this thread | | | Date | Thu, 25 Sep 2008 09:07:54 +0200 | From | chri <> | Subject | Re: [spi-devel-general] [PATCH RESEND] max3100 driver |
| |
On Thu, Sep 25, 2008 at 6:56 AM, David Brownell <david-b@pacbell.net> wrote: > > Small suggestion: next time you resend this, make sure > that $SUBJECT mentions it's a UART driver. Maybe even that > it's a SPI UART driver. That should help get more comments. >
OK!
> > This is a bit picky, but it's the first thing I noticed when > scanning the patch ... wierd comment layout! Either indent > those all, or (better) convert to kerneldoc style.
I will have a look at kerneldoc and do the change. I'm waiting for a minor number in "Low-density serial ports" before resending the modifications asked by Andrew and Alan, so I will do this too. I just trusted M-q in emacs for the comment layout.
> > Potentially less picky: probe() doesn't lock max3100s[], > neither does remove(), and in fact there seems to be no > lock for that table. Which suggests trouble in cases like
I (wrongly!) was assuming that probing of devices is serialized. I will add the lock.
> > And is that workqueue single threaded? >
it's freezeabe and (looking at include/linux/workqueue.) it implies that it's single-threaded. This is important because I don't do locking since I presume all the I/O to the MAX3100 is done in just one workqueue. When I do I/O in other places (suspend for example) I assume that the worqueue is friezed so not running.
Best regards and thanks for the review,
-- Christian Pellegrin, see http://www.evolware.org/chri/ "Real Programmers don't play tennis, or any other sport which requires you to change clothes. Mountain climbing is OK, and Real Programmers wear their climbing boots to work in case a mountain should suddenly spring up in the middle of the computer room."
| |