Hi Andy,
On Thu, Apr 28, 2011 at 1:00 AM, Andy Fleming afleming@gmail.com wrote:
On Fri, Apr 15, 2011 at 9:17 PM, Lei Wen leiwen@marvell.com wrote:
From now on, follow the general rule "mmc dev [dev]" to change the mmc command applied device, like ide and usb...
Signed-off-by: Lei Wen leiwen@marvell.com
Changelog: V2: use the "mmc_cur_dev" to replace explicitly specify the dev num in mmc command.
I'd really prefer if there were still the option to specify the device in the read/write commands. I appreciate the convenience of not having to specify it every time, but I feel that by doing things this way, we create an artificial separation between a transaction, and the target of the transaction. It tends to encourage a notion that a transaction can only be done to the global-current device, which is fine for command-line interfaces, but can result in broken programming interfaces
int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) {
- int rc = 0;
- if (argc < 2)
- return cmd_usage(cmdtp);
- switch (argc) {
- case 3:
- if (strcmp(argv[1], "rescan") == 0) {
- int dev = simple_strtoul(argv[2], NULL, 10);
- struct mmc *mmc = find_mmc_device(dev);
- if (strcmp(argv[1], "rescan") == 0) {
- struct mmc *mmc = find_mmc_device(mmc_cur_dev);
- if (!mmc)
- return 1;
- if (!mmc)
- return 1;
- mmc_init(mmc);
- mmc_init(mmc);
return 0;
} else if (strncmp(argv[1], "part", 4) == 0) {
block_dev_desc_t *mmc_dev;
struct mmc *mmc = find_mmc_device(mmc_cur_dev);
if (!mmc) {
puts("no mmc devices available\n");
Technically, this just means that no device is selected. I'm sure that would only happen if no devices were available, but that's an assumption carried outside of the vicinity of this code. Someone might change that assumption (for instance, if something could cause an mmc device to be removed)
If that device be removed, I think the following mmc_init could check it out, wouldn't it? For this check, my understanding is to see whether that slot has been register or not, then following init would double check device whether function.
- return 1;
- }
- mmc_init(mmc);
- mmc_dev = mmc_get_dev(mmc_cur_dev);
- if (mmc_dev != NULL &&
- mmc_dev->type != DEV_TYPE_UNKNOWN) {
- print_part(mmc_dev);
[...]
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 6805b33..377f13a 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -34,7 +34,7 @@ #include <div64.h>
static struct list_head mmc_devices; -static int cur_dev_num = -1;
Please don't remove this, or conflate this value with the global "current" pointer. This is used to assign the block dev num. mmc_cur_dev is a runtime value that will change. If, for some reason, someone needs to interact with an mmc device before all of them have come up, we will utterly break. This is the sort of reason that I'm not big on this sort of interaction paradigm.
+int mmc_cur_dev = -1;
You should make this a pointer to the mmc device, not an integer. Also, you should keep it in the cmd_mmc file. The *only* place this should be used is to declare what the current device for user interaction is. Board code that wants to interact with MMC devices should feel free to do so, irrespective of the user (ie, the user's desired card should not affect which card the board code accesses, and the board code should not cause the user's selection to change).
I agree with your concern. The main reason I mmc_cur_dev global is for cmd_mmc cannot get the info of how many slots has been enabled mmc.c
I think make a function in mmc.c like mmc_get_num() which would also fulfill my need.
int __board_mmc_getcd(u8 *cd, struct mmc *mmc) { return -1; @@ -849,7 +849,7 @@ int mmc_register(struct mmc *mmc) { /* Setup the universal parts of the block interface just once */ mmc->block_dev.if_type = IF_TYPE_MMC;
- mmc->block_dev.dev = cur_dev_num++;
- mmc->block_dev.dev = mmc_cur_dev++;
Make sure to switch this back
mmc->block_dev.removable = 1; mmc->block_dev.block_read = mmc_bread; mmc->block_dev.block_write = mmc_bwrite; @@ -936,12 +936,20 @@ void print_mmc_devices(char separator)
int mmc_initialize(bd_t *bis) {
- int ret = -1;
INIT_LIST_HEAD (&mmc_devices);
- cur_dev_num = 0;
- mmc_cur_dev = 0;
if (board_mmc_init(bis) < 0)
- cpu_mmc_init(bis);
ret = cpu_mmc_init(bis);
else
ret = 0;
/* Put the device 0 as the default */
if (ret < 0)
mmc_cur_dev = -1;
else
mmc_cur_dev = 0;
These seems needlessly awkward. I suggest that we can initialize it either to zero, or to the last register device, inside mmc_register(). You don't need "ret" at all, then. Because this should be in cmd_mmc, we would add a function to cmd_mmc, like: mmc_set_current_dev(struct mmc *)
I agree. mmc_set_current_dev may not need, as cmd_mmc still pass the dev num when it call the read/write function to the mmc.c.
print_mmc_devices(',');
return 0; diff --git a/include/mmc.h b/include/mmc.h index fcd0fd1..dd9bf15 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -295,4 +295,7 @@ int atmel_mci_init(void *regs); int mmc_legacy_init(int verbose); #endif
+#ifdef CONFIG_GENERIC_MMC +extern int mmc_cur_dev; +#endif
And then this isn't necessary, but you can declare mmc_set_current_dev(), here
Best regards, Lei