Re: [RFC] mmc: add OMAP driver

From: Russell King
Date: Thu Feb 16 2006 - 10:13:08 EST


On Thu, Feb 16, 2006 at 10:27:22AM -0400, Carlos Aguiar wrote:
> + /* Any data transfer means adtc type (but that information is not
> + * in command structure, so we flagged it into host struct.)
> + * However, telling bc, bcr and ac apart based on response is
> + * not foolproof:
> + * CMD0 = bc = resp0 CMD15 = ac = resp0
> + * CMD2 = bcr = resp2 CMD10 = ac = resp2
> + *
> + * Resolve to best guess with some exception testing:
> + * resp0 -> bc, except CMD15 = ac
> + * rest are ac, except if opendrain
> + */
> + if (host->data) {
> + cmdtype = OMAP_MMC_CMDTYPE_ADTC;
> + } else if (resptype == 0 && cmd->opcode != 15) {
> + cmdtype = OMAP_MMC_CMDTYPE_BC;
> + } else if (host->bus_mode == MMC_BUSMODE_OPENDRAIN) {
> + cmdtype = OMAP_MMC_CMDTYPE_BCR;
> + } else {
> + cmdtype = OMAP_MMC_CMDTYPE_AC;
> + }

This is no longer necessary - we now supply the command type via
cmd->flags.

> +static void
> +mmc_omap_cmd_done(struct mmc_omap_host *host, struct mmc_command *cmd)
> +{
> + host->cmd = NULL;
> +

if (cmd->flags & MMC_RSP_PRESENT) {
if (cmd->flags & MMC_RSP_136) {
cmd->resp[3] =
OMAP_MMC_READ(host->base, RSP0) |
(OMAP_MMC_READ(host->base, RSP1) << 16);
cmd->resp[2] =
OMAP_MMC_READ(host->base, RSP2) |
(OMAP_MMC_READ(host->base, RSP3) << 16);
cmd->resp[1] =
OMAP_MMC_READ(host->base, RSP4) |
(OMAP_MMC_READ(host->base, RSP5) << 16);
cmd->resp[0] =
OMAP_MMC_READ(host->base, RSP6) |
(OMAP_MMC_READ(host->base, RSP7) << 16);
} else {
cmd->resp[0] =
OMAP_MMC_READ(host->base, RSP6) |
(OMAP_MMC_READ(host->base, RSP7) << 16);
}
}

would be clearer, and probably more efficient. (gcc isn't really
that good at working out bit patterns from switch statements.)

> + /* Optimize the loop a bit by calculating the register only
> + * once */
> + reg = host->base + OMAP_MMC_REG_DATA;
> + p = host->buffer;
> + n /= 2;
> + if (write) {
> + while (n--)
> + __raw_writew(*p++, reg);
> + } else {
> + while (n-- > 0)
> + *p++ = __raw_readw(reg);
> + }

readsw / writesw would be a far more efficient implementation.

> + host->base = (void __iomem *)pdev->resource[0].start;

This is still very very very very wrong. MMIO resources do not contain
virtual addresses. Casting it to a virtual address like this is insane.

> + mmc_add_host(mmc);

At this point the host becomes live. Is the following initialisation
safe to do after the host has been manipulated to talk to MMC cards?

> +
> + if (host->switch_pin >= 0) {
> + INIT_WORK(&host->switch_work, mmc_omap_switch_handler, host);
> + init_timer(&host->switch_timer);
> + host->switch_timer.function = mmc_omap_switch_timer;
> + host->switch_timer.data = (unsigned long) host;
> + if (omap_request_gpio(host->switch_pin) != 0) {
> + printk(KERN_WARNING "MMC%d: Unable to get GPIO pin for MMC cover switch\n",
> + host->id);
> + host->switch_pin = -1;
> + goto no_switch;
> + }
> +
> + omap_set_gpio_direction(host->switch_pin, 1);
> + set_irq_type(OMAP_GPIO_IRQ(host->switch_pin), IRQT_RISING);
> + ret = request_irq(OMAP_GPIO_IRQ(host->switch_pin),
> + mmc_omap_switch_irq, 0, DRIVER_NAME, host);

Don't use set_irq_type, use request_irq with SA_TRIGGER_RISING please.

> + if (host) {
> + mmc_remove_host(host->mmc);
> + free_irq(host->irq, host);
> +#ifdef CONFIG_I2C
> + mmc_omap_power(host, 0);
> +#endif

Power should be turned off by the MMC layer anyway.

> +#ifdef CONFIG_PM
> +static int mmc_omap_suspend(struct platform_device *pdev, pm_message_t mesg)
> +{
> + int ret = 0;
> + struct mmc_omap_host *host = platform_get_drvdata(pdev);
> +
> + if (host && host->suspended)
> + return 0;
> +
> + if (!irqs_disabled())
> + return -EAGAIN;

IRQs will never be disabled here.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/