Anton Vorontsov wrote: > On Mon, Dec 14, 2009 at 08:33:25PM +0100, Albert Herranz wrote: >> This patch breaks down sdhci-of into a core portion and a eSDHC portion, >> clearing the path to easily support additional hardware using the same >> OF driver. >> >> Signed-off-by: Albert Herranz <albert_herranz@xxxxxxxx> > > Looks really good, thanks a lot for your work! > > Few minor nits down below. > > [...] >> +++ b/drivers/mmc/host/sdhci-of-core.c > > Not sure if adding -core prefix makes things better (it > actually makes the patch harder to review). > > Can we leave the core in sdhci-of.c, and just factor out > esdhc stuff from it? > I wanted to preserve the module name (sdhci-of). I basically renamed sdhci-of.c to sdhci-of-core.c to avoid a recursive rule in the Makefile. Is there an easy way to make the module build as sdhci-of from sdhci-of.c + sdhci-of-esdhc.c + whatever ? > [...] >> +#include <linux/mmc/host.h> >> +#include <asm/machdep.h> >> +#include "sdhci-of.h" >> + > > You still need to include sdhci.h. Files need to include all > the headers they need. I.e., here you should not rely on the > fact that sdhci-of.h includes sdhci.h. > Including sdhci.h more than once currently results in duplicate definition errors because that header file is not protected with #ifdef'ery. I can fix sdhci.h and then explicitly include it again. > [...] >> --- /dev/null >> +++ b/drivers/mmc/host/sdhci-of.h >> @@ -0,0 +1,42 @@ > [...] >> +extern struct sdhci_of_data sdhci_esdhc; >> + >> +#endif /* __SDHCI_OF_H */ >> + >> -- >> 1.6.3.3 > > Unneeded empty line at the end of sdhci-of.h. > Thanks. > If you'll manage to resend the patches before Linus roll the -rc1 > out, I'd be glad to beg Andrew to send this for v2.6.33. Because > it will be a pity if it has to wait for 2.6.34. > I'll try, as long as we solve nit #1 quickly enough :) Cheers, Albert -- 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
