On Wed, Nov 2, 2011 at 6:08 PM, Andy Fleming afleming@gmail.com wrote:
On Thu, Oct 13, 2011 at 4:57 PM, Anton Staaf robotboy@chromium.org wrote:
diff --git a/drivers/mmc/tegra2_mmc.c b/drivers/mmc/tegra2_mmc.c index 8b6f829..195f89d 100644 --- a/drivers/mmc/tegra2_mmc.c +++ b/drivers/mmc/tegra2_mmc.c @@ -256,9 +256,15 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, __func__, mask); return -1; } else if (mask & (1 << 3)) {
[...]
- writel((1 << 3), &host->reg->norintsts);
- writel(address, &host->reg->sysad);
} else if (mask & (1 << 1)) { /* Transfer Complete */ debug("r/w is done\n"); @@ -442,12 +448,13 @@ static int mmc_core_init(struct mmc *mmc) * NORMAL Interrupt Status Enable Register init * [5] ENSTABUFRDRDY : Buffer Read Ready Status Enable * [4] ENSTABUFWTRDY : Buffer write Ready Status Enable
- * [3] ENSTADMAINT : DMA Interrupt Status Enable
* [1] ENSTASTANSCMPLT : Transfre Complete Status Enable * [0] ENSTACMDCMPLT : Command Complete Status Enable
- */
- */
mask = readl(&host->reg->norintstsen); mask &= ~(0xffff);
- mask |= (1 << 5) | (1 << 4) | (1 << 1) | (1 << 0);
- mask |= (1 << 5) | (1 << 4) | (1 << 3) | (1 << 1) | (1 << 0);
I'm pretty sure that a similar patch resulted in a request from Wolfgang to clean up the magic numbers in the code. Commenting the numbers above is helpful, but still very prone to maintenance issues. Why not just set mask:
mask |= (ENSTABUFRDRDY | ENSTABUFWTRDY | ENSTADMAINT | ENSTASTANSCMPLT | ENSTACMDCMPLT);
If all of the uses of those masks were replaced like that, we'd have much greater confidence that a random current or future typo wouldn't break the driver. It would also make it much simpler to identify other places where that bit was being set/checked.
Please fix, and resubmit.
Will do.
Thanks, Anton
Andy