Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [mmc] alternative TI FM MMC/SD driver for 2.6.21-rc7 | Date | Fri, 20 Apr 2007 00:56:33 +0200 |
| |
On Thursday 19 April 2007, Sergey Yanovich wrote: > The device is present in many notebooks. Notebooks depend heavily on > suspend/resume functionality. tifm_core/7xx1/sd family is an ambitous, > but uncompleted project. It used to crash on resuming, or hang up on > suspending. A less common failure used to be trigerred by a fast card > insert/removal sequence. Finally, tifm_sd module needs to be manually > inserted.
As very general comments, you should have the maintainer of the subsystem (Pierre in this case) on Cc when posting a driver, and you should include the patch inline in your mail, see Documentation/SubmittingPatches.
More specific to your patch:
You should include the Makefile and Kconfig changes in the same patch/mail, no point splitting these out.
Don't define your own DBG macro, instead use the predefined dev_dbg() that has a similar definition.
Your mmc_tifm_irq_chip() function does a _very_ long delay of 100 miliseconds. This is normally not acceptable, since it is a noticeable time in which the system is completely unresponsive. Maybe you can convert the tasklet to a workqueue, which lets you call msleep instead of mdelay.
Your use of pci_map_sg() looks wrong, you simply can't assume that the return value is '1' in general. I've stumbled over that same problem in the sdhci driver, so it may be inherent to the mmc layer and not be driver specific.
Other than that, your driver looks pretty good to me.
Arnd <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |