On Wed, Nov 2, 2011 at 6:08 PM, Andy Fleming <aflem...@gmail.com> wrote:
> On Thu, Oct 13, 2011 at 4:57 PM, Anton Staaf <robot...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
