Re: [mmc] alternative TI FM MMC/SD driver for 2.6.21-rc7

[Arnd Bergmann <arnd@arndb.de>]

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/