Dear Dileep Katta,
In message 1410417650-16522-2-git-send-email-dileep.katta@linaro.org you wrote:
Flash command internally uses DFU, and Fastboot command initialization is modified to add DFU and partition initialization Added oem format functionality for GPT table creation partitioning code is added as disk/part_fastboot.c for better usability
Fastboot flash command code is enabled and being tested on BeagleBone Black. OMAP3 Beagle configuration is modified to fix the build errors, but this configuration needs to be updated as per the flash functionality.
Please fix the line length in the commit message.
Also, checkpatch says:
WARNING: do not add new typedefs #1073: FILE: include/usb/fastboot.h:98: +typedef struct fastboot_ptentry fastboot_ptentry;
+int handle_flash(char *part_name, char *response, char *transfer_buffer) +{
- int status = 0;
- printf("download_bytes: 0x%x\n", download_bytes);
- if (download_bytes) {
struct fastboot_ptentry *ptn;/* Next is the partition name */ptn = fastboot_flash_find_ptn(part_name);if (ptn == 0) {printf("Partition:[%s] does not exist\n", part_name);sprintf(response, "FAILpartition does not exist");} else if ((download_bytes > ptn->length) &&!(ptn->flags & FASTBOOT_PTENTRY_FLAGS_WRITE_ENV)) {printf("Image too large for the partition\n");sprintf(response, "FAILimage too large for partition");} else if (ptn->flags & FASTBOOT_PTENTRY_FLAGS_WRITE_ENV) {/* Check if this is not really a flash write,* but instead a saveenv*/unsigned int i = 0;/* Env file is expected with a NULL delimeter between* env variables So replace New line Feeds(0x0a) with* NULL (0x00)*/
Incorrect multiline comment style. Please fix globally.
Also this sequence of comment - declaration - comment is highly confusing. What is the first comment ("Check if..") actually related to?
for (i = 0; i < download_bytes; i++) {if (transfer_buffer[i] == 0x0a)transfer_buffer[i] = 0x00;}memset(env_ptr->data, 0, ENV_SIZE);memcpy(env_ptr->data, transfer_buffer, download_bytes);do_env_save(NULL, 0, 1, NULL);printf("saveenv to '%s' DONE!\n", ptn->name);
I think it is a bad idea to split formatting / reformatting envrionment data alll over the place. This should be done only once, in the environment handling code. Also, I do not like you using do_env_save() here - this should remain a static function. Why do't you simply use saveenv() here?
Finally, the "saveenv to '%s' DONE!\n" is IMO too verbose. No news is Good News - we usually do not report abot the success of operations, as this is what we expect. We should only print error messages when such occur - which points out another problem of that code: there is no error checking nor handling there...
+static void end_ptbl(struct ptable *ptbl) +{
- struct efi_header *hdr = &ptbl->header;
- u32 n;
- n = crc32(0, 0, 0);
What exactly is this good for?
- n = crc32(n, (void *)ptbl->entry, sizeof(ptbl->entry));
- hdr->entries_crc32 = n;
- n = crc32(0, 0, 0);
What exactly is this good for?
- n = crc32(0, (void *)&ptbl->header, sizeof(ptbl->header));
- hdr->crc32 = n;
+}
- for (n = 0; n < (128/4); n++) {
/* read partition */source[0] = '\0';dest[0] = '\0';length[0] = '\0';
You overwrite source, dest, and length by the sprintf()s just a few lines below. So why are you doing this here?
mmc_read[2] = source;mmc_read[3] = dest;mmc_read[4] = length;sprintf(source, "%p", entry);sprintf(dest, "0x%x", 0x1+n);sprintf(length, "0x%x", 1);
+int board_mmc_fbtptn_init(void) +{
- char *mmc_init[2] = {"mmc", "rescan",};
- char dev[2];
- char *mmc_dev[3] = {"mmc", "dev", NULL};
- mmc_dev[2] = dev;
Why do you initialize mmc_dev[2] with another value just a line above? Why don't you use the correct value right from the beginning?
- if (do_mmcops(NULL, 0, 3, mmc_dev)) {
printf("MMC DEV: %d selection FAILED!\n", CONFIG_FB_MMCDEV);return -1;- }
- if (do_mmcops(NULL, 0, 2, mmc_init)) {
printf("FAIL:Init of MMC card\n");return 1;- }
What exactly are your return codes? Here you return -1 in case of error, there +1 - what's the logic behind that?
+int do_format(void) +{
- struct ptable *ptbl = &the_ptable;
- /* unsigned sector_sz; */
Please remove dead code.
- printf("\ndo_format ..!!");
Is this not overly verbose?
- /* get mmc info */
- struct mmc *mmc = find_mmc_device(CONFIG_FB_MMCDEV);
Please do not mix code and declarations. Also, always add a blank line after declarations.
- if (mmc_init(mmc)) {
printf("\n mmc init FAILED");return -1;- } else{
After the return, no else is needed here.
printf("\nmmc capacity is: %llu", mmc->capacity);printf("\nmmc: number of blocks:0x%lx", mmc->block_dev.lba);printf("\nmmc: block size:0x%lx", mmc->block_dev.blksz);- }
- /* 10/11:modified as per PSP release support */
- char *mmc_write[5] = {"mmc", "write", NULL, NULL, NULL};
- char source[32], dest[32], length[32];
- char dev[2];
- char *mmc_dev[3] = {"mmc", "dev", NULL};
- mmc_dev[2] = dev;
See above - please do not mix code and declarations, please use the right initialization right away.
- } else {
printf("Writing mbr is DONE!\n");
Less verbose, please.
Best regards,
Wolfgang Denk