Re: [PATCH] sh7760mmc: host driver for SH7760 MMC interface

[Pierre Ossman <drzeus@drzeus.cx>]

[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]

On Thu, 17 Jul 2008 13:03:53 +0200
Manuel Lauss <mano@roarinelk.homelinux.net> wrote:

> From: Manuel Lauss <mano@roarinelk.homelinux.net>
> 
> Host driver for SH7760 on-chip MMCIF interface.
> 
> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
> ---

Just some minor things:

> +static void shmmc_cmd(struct shmmc_host *host, struct mmc_command *cmd)
> +{
> +	unsigned char cmdtyr, rsptyr, i;
> +
> +	shmmc_clock(host, 0);
> +	host->cmd = cmd;
> +
> +	/* enable MMCIF and "cmd xfer timeout" irqs */
> +	host->ien1 |= INTCR1_IRQEN | INTCR1_CTERIE;
> +
> +	/* setup command type */
> +	cmdtyr = 0;
> +	if (cmd->opcode == 12)
> +		cmdtyr = CMDTYR_CMD12;
> +

Seems more likely that this flag should be set on any stop command.

> +
> +	blksz_bits = ffs(data->blksz) - 1;
> +	BUG_ON((1 << blksz_bits) != data->blksz);

This is not a bug. You should fail the request with -EINVAL if you
cannot handle it.

> +static int __devexit shmmc_remove(struct platform_device *dev)
> +{
> +	struct mmc_host *mmc = platform_get_drvdata(dev);
> +	struct shmmc_host *host = mmc_priv(mmc);
> +	struct resource *r;
> +
> +	MMC_OUTB(host, INTCR0, 0);
> +	MMC_OUTB(host, INTCR1, 0);
> +	MMC_OUTB(host, INTCR2, 0);
> +	shmmc_clrirq(host);
> +	tasklet_disable(&host->done_task);
> +	mmc_remove_host(mmc);

Don't free up any resources needed to handle requests before
mmc_remove_host() has returned.

Also, remember to run the mmc_test test suite on the driver. It tends
to find a lot of problems.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]