From: Mike Frysinger <vapier@gentoo.org>
Date: Tue, 27 Apr 2010 12:49:44 -0400
Subject: [U-Boot] [PATCH v3] mmc: add generic mmc spi driver
In-Reply-To: <1272338836-18718-1-git-send-email-thomas@wytron.com.tw>
References: <1271991214-7937-1-git-send-email-thomas@wytron.com.tw>
	<1272338836-18718-1-git-send-email-thomas@wytron.com.tw>
Message-ID: <201004271249.52634.vapier@gentoo.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Monday 26 April 2010 23:27:16 Thomas Chou wrote:
> --- /dev/null
> +++ b/common/cmd_mmc_spi.c
> +	struct mmc *mmc = NULL;

useless assignment since mmc gets set shortly after

> +	struct mmc_spi_priv *priv;
> +
> +	if (argc < 2)
> +		goto usage;
> +
> +	dev_num = simple_strtoul(argv[1], &endp, 0);
> +	if (*endp != 0)
> +			goto usage;
> +	mmc = find_mmc_device(dev_num);
> +	if (!mmc || strcmp(mmc->name, "MMC_SPI"))
> +			goto usage;

too many indents on the goto

also, this looks like it bails if no existing mmc spi device is found ?  i 
thought the point of this command was to create new instances on the fly.  
this looks like it just reprograms the settings for an existing device.

> +	if (bus != priv->bus || cs != priv->cs ||
> +	    speed != priv->speed || mode != priv->speed) {
> +		priv->bus = bus;
> +		priv->cs = cs;
> +		priv->speed = speed;
> +		priv->mode = mode;
> +		if (priv->slave) {
> +			free(priv->slave);
> +			priv->slave = NULL;
> +		}

you shouldnt be poking the spi internals like this.  use the common spi funcs 
to free/alloc the slave device.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100427/961b2b68/attachment.pgp