On Mon, 10 Dec 2012, Chris Ball wrote: > Hi Shawn, Guennadi, > > On Fri, Dec 07 2012, Shawn Guo wrote: > >> > Use devm_* managed functions, so that slot-gpio users do not have to > >> > call mmc_gpio_free_ro/cd to free up resources requested in > >> > mmc_gpio_request_ro/cd. > >> > > >> > Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx> > >> > >> Thanks for the patch, but I'm not sure this is a good idea. Firstly, using > >> devm_* allocation functions means, that normally you don't have to free > >> these resources explicitly any more. So, actually you would have to remove > >> free_irq() and gpio_free() calls completely from the API instead of > >> replacing them with devm_* analogs. > > > > With the changes, most of the slot-gpio users will only need to call > > mmc_gpio_request_* functions at probe time, nothing else. That said, > > they will not call mmc_gpio_free_* functions at all. I patched > > mmc_gpio_free_* functions replacing free_irq() and gpio_free() with > > devm_* versions to 1) ease the migration of exiting users calling > > mmc_gpio_free_*; 2) provide a mean for users to manually free resources > > for whatever reasons. > > I'd like to find a way to merge this -- maybe we can compromise by > adding a comment or some documentation that explains that you need to be > careful (and use _request_* and _free_*) to avoid allocating more than > once if you're doing something odd like runtime switching between CD > methods? Ok, so, you agree, that the normal case is, when these GPIO functions are requested only once during probing and are released during driver unbinding, and that it's worth optimisine for this case. In principle I agree, and so far all 3 users of this API do that. So, maybe it would be good to extend this patch series with a patch (or 3 patches, if per-driver is preferred), removing mmc_gpio_free_*() calls from those drivers. I think, however, there is one more thing to consider here: the slot-gpio API contains a GPIO CD IRQ handler, which after these changes will only be freed after the calls to mmc_remove_host() and mmc_free_host(). This should be safe, because mmc_remove_host() sets the .rescan_disable flag. After the recent patch the GPIO CD ISR also calls host->ops->card_event(), so, hosts muct make sure, that this function is safe to call even after the driver's .remove() method has completed. With that in mind, I think, we're fine with this patch. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
