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

>> +
>> +/*
>> + * 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

>> +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
>> +                    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.
Compile time error always generate error even though override function
exist.
Also mg_disk_init() function will generate run time error when weak
function used. Typically, users will be read README.mflash that explains
how to override this function and finally port appropriately.

Regards,
unsik Kim
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to