Dear Dileep Katta,
In message <1410417650-16522-2-git-send-email-dileep.ka...@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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
At the source of every error which is blamed on the computer you will
find at least two human errors, including the error of blaming it on
the computer.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
