From: Thomas Chou <thomas@wytron.com.tw>
Date: Wed, 28 Apr 2010 11:14:58 +0800
Subject: [U-Boot] [PATCH v3] mmc: add generic mmc spi driver
In-Reply-To: <201004271249.52634.vapier@gentoo.org>
References: <1271991214-7937-1-git-send-email-thomas@wytron.com.tw>
	<1272338836-18718-1-git-send-email-thomas@wytron.com.tw>
	<201004271249.52634.vapier@gentoo.org>
Message-ID: <4BD7A832.1070107@wytron.com.tw>
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

Hi Mike,

On 04/28/2010 12:49 AM, Mike Frysinger wrote:
> 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
>    
OK.
>    
>> +	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
>    
The tabs were interfered by the '+' . It is correct when applied.
> 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.
>
>    
OK. Now v4 patch creates a MMC dev on the fly.

>> +	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.
>    
OK.

Best regards,
Thomas