On 16:09 Tue 17 Feb , unsik Kim wrote: > Hello? > > Thanks for comments. > Some patches will be posted for your requests. > Also I wrote my opinion for some comments. > >>> diff --git a/drivers/block/Makefile b/drivers/block/Makefile >>> index 59388d9..eccefc1 100644 >>> --- a/drivers/block/Makefile >>> +++ b/drivers/block/Makefile >>> @@ -25,13 +25,14 @@ include $(TOPDIR)/config.mk >>> LIB := $(obj)libblock.a >>> -COBJS-$(CONFIG_SCSI_AHCI) += ahci.o >> why? > This list badly sorted before I add. I just resort it to > alphabetical order and add new option. Nothing removed. > >>> COBJS-$(CONFIG_ATA_PIIX) += ata_piix.o >>> +COBJS-$(CONFIG_CMD_MG_DISK) += mg_disk.o >>> COBJS-$(CONFIG_FSL_SATA) += fsl_sata.o >>> +COBJS-$(CONFIG_IDE_SIL680) += sil680.o >>> COBJS-$(CONFIG_LIBATA) += libata.o >>> COBJS-$(CONFIG_PATA_BFIN) += pata_bfin.o >>> COBJS-$(CONFIG_SATA_SIL3114) += sata_sil3114.o >>> -COBJS-$(CONFIG_IDE_SIL680) += sil680.o >>> +COBJS-$(CONFIG_SCSI_AHCI) += ahci.o >>> COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o >>> COBJS-$(CONFIG_SYSTEMACE) += systemace.o >>> diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c >>> new file mode 100644 >>> index 0000000..4454fca >>> --- /dev/null >>> +++ b/drivers/block/mg_disk.c >>> + >>> +#define MG_BASE (host.drv_data->base) >> an inline function will be better > This isn't a function. yes but please use an inline function which will allow better compiler check and error message as done in a lots of the linux drivers as example the netdev with this static inline void *netdev_priv(const struct net_device *dev) { return dev->priv; } >>> + > >>> + >>> +/* >>> + * copy src to dest, skipping leading and trailing blanks and null >>> + * terminate the string >>> + * "len" is the size of available memory including the terminating '\0' >>> + */ >>> +static void mg_ident_cpy (unsigned char *dst, unsigned char *src, >>> + unsigned int len) >>> +{ >>> + unsigned char *end, *last; >>> + >>> + last = dst; >>> + end = src + len - 1; >>> + >>> + /* reserve space for '\0' */ >>> + if (len < 2) >>> + goto OUT; >>> + >>> + /* skip leading white space */ >>> + while ((*src) && (src<end) && (*src==' ')) >> please add a space before and after '<' '==' & co >>> + ++src; >>> + >>> + /* copy string, omitting trailing white space */ >>> + while ((*src) && (src<end)) { >>> + *dst++ = *src; >>> + if (*src++ != ' ') >>> + last = dst; >>> + } >>> +OUT: >>> + *last = '\0'; >>> +} >> why do you need this? > for parsing string parts of ATAID it's mflash specific?? > >>> +static unsigned int mg_do_read_sects(void *buff, u32 sect_num, u32 >>> sect_cnt) >>> +{ >>> + u32 i, j, err; >>> + u8 *buff_ptr = buff; >>> + >>> + err = mg_out(sect_num, sect_cnt, MG_CMD_RD); >>> + if (err) >>> + return err; >>> + >>> + for (i = 0; i < sect_cnt; i++) { >>> + err = mg_wait(MG_REG_STATUS_BIT_DATA_REQ, 3000); >>> + if (err) >>> + return err; >>> + >>> + /* TODO : u16 unaligned case */ >>> + for(j = 0; j < MG_SECTOR_SIZE >> 1; j++) { >>> + *(u16 *)buff_ptr = >>> + readw(MG_BASE + MG_BUFF_OFFSET + (j << 1)); >> ?? > I can't guess your intention. Please write more clearly you store a u16 in a u8 :( >>> + buff_ptr += 2; >>> + } >>> + >>> + writeb(MG_CMD_RD_CONF, MG_BASE + MG_REG_COMMAND); >>> + >>> + MG_DBG("%u (0x%8.8x) sector read", sect_num + i, >>> + (sect_num + i) * MG_SECTOR_SIZE); >>> + } >>> + >>> + return err; >>> +} > >>> +unsigned int mg_disk_read_sects(void *buff, u32 sect_num, u32 sect_cnt) >>> +{ >>> + u32 quotient, residue, i, err; >>> + u8 *buff_ptr = buff; >>> + >>> + quotient = sect_cnt >> 8; >>> + residue = sect_cnt % 256; >>> + >>> + for (i = 0; i < quotient; i++) { >>> + MG_DBG("sect num : %u buff : 0x%8.8x", sect_num, (u32)buff_ptr); >>> + err = mg_do_read_sects(buff_ptr, sect_num, 256); >>> + if (err) >>> + return err; >>> + sect_num += 256; >>> + buff_ptr += 256 * MG_SECTOR_SIZE; >>> + } >>> + >>> + if (residue) { >>> + MG_DBG("sect num : %u buff : %8.8x", sect_num, (u32)buff_ptr); >> please use debug(x) > MG_DBG prints lots of messages. I think changing MG_DBG to debug might > confuse other driver's debug message. > If u-boot policy only allow debug(x), I'll follow. >>> + err = mg_do_read_sects(buff_ptr, sect_num, residue); >>> + } >>> + >>> + return err; >>> +} > >>> +/* must override this function */ >>> +struct mg_drv_data * __attribute__((weak)) mg_get_drv_data (void) >>> +{ >>> + puts ("### WARNING ### port mg_get_drv_data function\n"); >>> + return NULL; >>> +} >> please do an compile error not a run time error > IMHO, compile error (or warning) is not a good choice for this case. sorry no beacause everyone does not have the board to test it, a compile error will at least help use to check if we brake the board or not > Compile time error always generate error even though override function > exist. not necessarely does you driver allow multiple instance of mflash? if no you does not implement the function
Best Regards, J. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot