Cheers,
On Fri, Feb 16, 2007 at 07:31:25PM -0700, Grant Likely wrote:
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot/cpu/arm920t/s3c24x0/mmc.c 2007-02-16 23:43:28.000000000 +0100
+//#define MMC_DEBUG
C++ style comment; bad! :-)
well, I was a long-time proponent of this, too. But it's now officially part of the C99 standard. So there's not really much [technical] argument you can still make about this :(
+#ifdef MMC_DEBUG +#define DEBUGP printf +#else +#define DEBUGP(x, args ...) do { } while(0) +#endif
Should really use the debug/debugX macros from common.h. To enable them for a single file, just add '#define DEBUG' at the top of the file before any #include statements.
I know this is done a lot in u-boot; but I think it should be avoided as much as possible; especially in new code.
ok, no problem, have changed in my tree
+static S3C2410_SDI *sdi;
+extern int +fat_register_device(block_dev_desc_t *dev_desc, int part_no);
External prototype in a .c file; bad practice.
A very common one in u-boot. I'm just trying to fit in ;) Using '#include <fat.h>' now.
Include the correct header instead. If the prototype doesn't exist in a header included by the function implementation, then write a patch to add it.
will do.
+/****************************************************/ +mmc_cmd(ushort cmd, ulong arg, ushort flags) +/****************************************************/
I'm not so fond of the style putting a comment line between the function name and the return type. I think they should be together. What does everyone else think?
I've removed this already in my tree.
braces not necessary on single line conditionals
removed them in my tree.
while (fifo--) {//DEBUGP("dst_u32 = 0x%08x\n", dst_u32);*(dst_u32++) = sdi->SDIDAT;if (len >= 4)len -= 4;else {len = 0;break;}Seems a little verbose; what about something like this? for ( ; fifo && len > 0; fifo--, len += 4) *(dst_u32++) = sdi->SDIDAT;
I guess this si a question of taste/preferecne. I personally think the explicit variant (minus the //indented DEBUGP statement) is more readable than a for loop with no initial statement and ',' style multiple commands in each loop.
Can/should this ever time out?
probably yes, if you do something strange to the hardware...
Should (1 << 4) be a #defined constant?
probably, too.
/* all block aligned accesses */DEBUGP("src %lx dst %lx end %lx pstart %lx pend %lx astart %lxaend %lx\n",
src, (ulong)dst, end, part_start, part_end, aligned_start,aligned_end);
This long debug statement appears a lot; should it be a macro?
that's (like much of the stuff) just a copy of something that is already in u-boot. So one of the things while adopting it from pxa to s3c2410 was: keep it as close as possible to the original. It seems like that one was a bad example ;)
+typedef struct mmc_cid +{ +/* FIXME: BYTE_ORDER */
- uchar year:4,
- month:4;
- uchar sn[3];
- uchar fwrev:4,
- hwrev:4;
- uchar name[6];
- uchar id[3];
+} mmc_cid_t;
- Inconsistent line spacing
will fix that.
- Bit fields are non-portable (but that might not matter for this code)
yes. S3C2410 supports big-endian mode, but I doubt that anyone is using it, especially so in combination with u-boot.
Ditto here
sine those are actual MMC protocol definitions, it might be worth having one common definition. Much the same goes for the MMC read/write code in general. But I don't really know too many SD/MMC controllers to decide on the level of the API, etc.
Index: u-boot/include/s2c24x0.h
--- u-boot.orig/include/s3c24x0.h 2007-02-16 23:42:02.000000000 +0100 +++ u-boot/include/s3c24x0.h 2007-02-16 23:42:19.000000000 +0100 @@ -637,13 +637,7 @@ S3C24X0_REG32 SDIDCNT; S3C24X0_REG32 SDIDSTA; S3C24X0_REG32 SDIFSTA; -#ifdef __BIG_ENDIAN
S3C24X0_REG8 res[3];S3C24X0_REG8 SDIDAT;-#else
S3C24X0_REG8 SDIDAT;S3C24X0_REG8 res[3];-#endif
Is this a bug fix? If so, it should go in another patch with an appropriate description
well, you can access those registers as byte or as long register. If you use the long address, it will be the same addres independent of the endianness.
There was no existing user of those regs in u-boot. Since my code uses long accesses to use it, there's no point in having them defined as byte registers.
So it's not really a bugfix, just adopting the existing unused definitions to the needs of my mmc driver.
@@ -1123,11 +1117,7 @@ #define rSDIDatCnt (*(volatile unsigned *)0x5A000030) #define rSDIDatSta (*(volatile unsigned *)0x5A000034) #define rSDIFSTA (*(volatile unsigned *)0x5A000038) -#ifdef __BIG_ENDIAN -#define rSDIDAT (*(volatile unsigned char *)0x5A00003F) -#else -#define rSDIDAT (*(volatile unsigned char *)0x5A00003C) -#endif +#define rSDIDAT (*(volatile unsigned *)0x5A00003C)
Ditto
ditto ;)