Re: [U-Boot] [PATCH v3] mmc: add generic mmc spi driver

2010-04-27 Thread Thomas Chou
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3] mmc: add generic mmc spi driver

2010-04-27 Thread Mike Frysinger
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


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


[U-Boot] [PATCH v3] mmc: add generic mmc spi driver

2010-04-26 Thread Thomas Chou
This patch supports mmc/sd card with spi interface. It is based on
the generic mmc framework. It works with SDHC and supports write.

The crc7 lib func is merged from linux and used to compute mmc
command checksum.

There is a subcomamnd "mmc_spi" to setup spi bus and cs at run time.

Signed-off-by: Thomas Chou 
---
v3: add mmc_spi_init() proto to mmc_spi.h.
v2: add crc7, use cmd58 to read ocr, add subcommand mmc_spi.

 common/Makefile   |1 +
 common/cmd_mmc_spi.c  |   92 ++
 drivers/mmc/Makefile  |1 +
 drivers/mmc/mmc_spi.c |  316 +
 include/linux/crc7.h  |   14 ++
 include/mmc_spi.h |   26 
 lib/Makefile  |1 +
 lib/crc7.c|   62 ++
 8 files changed, 513 insertions(+), 0 deletions(-)
 create mode 100644 common/cmd_mmc_spi.c
 create mode 100644 drivers/mmc/mmc_spi.c
 create mode 100644 include/linux/crc7.h
 create mode 100644 include/mmc_spi.h
 create mode 100644 lib/crc7.c

diff --git a/common/Makefile b/common/Makefile
index dbf7a05..ee23e2f 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -118,6 +118,7 @@ COBJS-$(CONFIG_CMD_MII) += miiphyutil.o
 COBJS-$(CONFIG_CMD_MII) += cmd_mii.o
 COBJS-$(CONFIG_CMD_MISC) += cmd_misc.o
 COBJS-$(CONFIG_CMD_MMC) += cmd_mmc.o
+COBJS-$(CONFIG_CMD_MMC_SPI) += cmd_mmc_spi.o
 COBJS-$(CONFIG_MP) += cmd_mp.o
 COBJS-$(CONFIG_CMD_MTDPARTS) += cmd_mtdparts.o
 COBJS-$(CONFIG_CMD_NAND) += cmd_nand.o
diff --git a/common/cmd_mmc_spi.c b/common/cmd_mmc_spi.c
new file mode 100644
index 000..578d7a7
--- /dev/null
+++ b/common/cmd_mmc_spi.c
@@ -0,0 +1,92 @@
+/*
+ * Command for mmc_spi setup.
+ *
+ * Copyright (C) 2010 Thomas Chou 
+ * Licensed under the GPL-2 or later.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static int do_mmc_spi(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+   int dev_num;
+   uint bus;
+   uint cs;
+   uint speed;
+   uint mode;
+   char *endp;
+   struct mmc *mmc = NULL;
+   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;
+   priv = mmc->priv;
+   bus = priv->bus;
+   cs = priv->cs;
+   speed = priv->speed;
+   mode = priv->mode;
+
+   if (argc < 3)
+   goto info;
+
+   cs = simple_strtoul(argv[2], &endp, 0);
+   if (*argv[2] == 0 || (*endp != 0 && *endp != ':'))
+   goto usage;
+   if (*endp == ':') {
+   if (endp[1] == 0)
+   goto usage;
+   bus = cs;
+   cs = simple_strtoul(endp + 1, &endp, 0);
+   if (*endp != 0)
+   goto usage;
+   }
+
+   if (argc >= 4) {
+   speed = simple_strtoul(argv[3], &endp, 0);
+   if (*argv[3] == 0 || *endp != 0)
+   goto usage;
+   }
+   if (argc >= 5) {
+   mode = simple_strtoul(argv[4], &endp, 16);
+   if (*argv[4] == 0 || *endp != 0)
+   goto usage;
+   }
+
+   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;
+   }
+   }
+info:
+   printf("%s:%d at %u:%u %u %u\n", mmc->name, dev_num,
+  bus, cs, speed, mode);
+   return 0;
+
+usage:
+   cmd_usage(cmdtp);
+   return 1;
+}
+
+U_BOOT_CMD(
+   mmc_spi,5,  0,  do_mmc_spi,
+   "mmc_spi setup",
+   "dev_num [bus:][cs] [hz] [mode] - setup mmc_spi device on given\n"
+   "  SPI bus and chip select\n"
+);
diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
index 6fa04b8..02ed329 100644
--- a/drivers/mmc/Makefile
+++ b/drivers/mmc/Makefile
@@ -28,6 +28,7 @@ LIB   := $(obj)libmmc.a
 COBJS-$(CONFIG_GENERIC_MMC) += mmc.o
 COBJS-$(CONFIG_ATMEL_MCI) += atmel_mci.o
 COBJS-$(CONFIG_BFIN_SDH) += bfin_sdh.o
+COBJS-$(CONFIG_MMC_SPI) += mmc_spi.o
 COBJS-$(CONFIG_OMAP3_MMC) += omap3_mmc.o
 COBJS-$(CONFIG_FSL_ESDHC) += fsl_esdhc.o
 COBJS-$(CONFIG_MXC_MMC) += mxcmmc.o
diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c
new file mode 100644
index 000..3ff171f
--- /dev/null
+++ b/drivers/mmc/mmc_spi.c
@@ -0,0 +1,316 @@
+/*
+ * generic mmc spi driver
+ *
+ * Copyright (C) 2010 Thomas Chou 
+ * Licensed under the GPL-2 or later.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define CTOUT 0x10
+#define RTOUT 0x1
+#define WTOUT 0