From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Thu, 31 Jul 2014 03:37:50 +0200
Subject: [U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support
	for flash command
In-Reply-To: <1403813604-31685-3-git-send-email-srae@broadcom.com>
References: <1403813604-31685-1-git-send-email-srae@broadcom.com>
	<1403813604-31685-3-git-send-email-srae@broadcom.com>
Message-ID: <201407310337.50805.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
[...]
> +
> +#include <common.h>
> +#include <fb_mmc.h>
> +#include <part.h>
> +#include <sparse_format.h>
> +
> +/* The 64 defined bytes plus \0 */
> +#define RESPONSE_LEN	(64 + 1)
> +
> +static char *response_str;

I'd suggest to pass this "response_str" around instead of making it global.

> +static void fastboot_resp(const char *s)
> +{
> +	strncpy(response_str, s, RESPONSE_LEN);
> +	response_str[RESPONSE_LEN - 1] = '\0';

This could be shrunk to a single snprintf(response_str, RESPONSE_LENGTH, s); I 
think, but I'm not sure if the overhead won't grow.

> +}
> +
> +static int is_sparse_image(void *buf)
> +{
> +	sparse_header_t *s_header = (sparse_header_t *)buf;
> +
> +	if ((le32_to_cpu(s_header->magic) == SPARSE_HEADER_MAGIC) &&
> +	    (le16_to_cpu(s_header->major_version) == 1))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static void write_sparse_image(block_dev_desc_t *dev_desc,
> +		disk_partition_t *info, const char *part_name,
> +		void *buffer, unsigned int download_bytes)
> +{
> +	lbaint_t blk;
> +	lbaint_t blkcnt;
> +	lbaint_t blks;
> +	sparse_header_t *s_header = (sparse_header_t *)buffer;
> +	chunk_header_t *c_header;
> +	void *buf;
> +	uint32_t blk_sz;
> +	uint32_t remaining_chunks;
> +	uint32_t bytes_written = 0;
> +
> +	blk_sz = le32_to_cpu(s_header->blk_sz);
> +
> +	/* verify s_header->blk_sz is exact multiple of info->blksz */
> +	if (blk_sz != (blk_sz & ~(info->blksz - 1))) {
> +		printf("%s: Sparse image block size issue [%u]\n",
> +		       __func__, blk_sz);
> +		fastboot_resp("FAILsparse image block size issue");

Can't you just make the fastboot_resp() function a variadic one AND move the 
printf() into the fastboot_resp() function? You could then even get consistent 
output on both the device and in the response if you snprintf() into the 
response_str first and then printf() the response_str .

> +		return;
> +	}

[...]

> +static void write_raw_image(block_dev_desc_t *dev_desc, disk_partition_t
> *info, +		const char *part_name, void *buffer,
> +		unsigned int download_bytes)
> +{
> +	lbaint_t blkcnt;
> +	lbaint_t blks;
> +
> +	/* determine number of blocks to write */
> +	blkcnt = ((download_bytes + (info->blksz - 1)) & ~(info->blksz - 1));
> +	blkcnt = blkcnt / info->blksz;
> +
> +	if (blkcnt > info->size) {
> +		printf("%s: too large for partition: '%s'\n", __func__,
> +		       part_name);
> +		fastboot_resp("FAILtoo large for partition");
> +		return;
> +	}
> +
> +	printf("Flashing Raw Image\n");

Use puts() here and everywhere where printf() is not taking any args please.

> +	blks = dev_desc->block_write(dev_desc->dev, info->start, blkcnt,
> +				     buffer);
> +	if (blks != blkcnt) {
> +		printf("%s: failed writing to device %d\n", __func__,
> +		       dev_desc->dev);
> +		fastboot_resp("FAILfailed writing to device");
> +		return;
> +	}
> +
> +	printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * info->blksz,
> +	       part_name);
> +	fastboot_resp("OKAY");
> +}
[...]